feat!: Restrict access by default with Spring Security #21831

mshabarov · 2025-07-02T11:18:11Z

Denies access to urls that have no request matcher in Spring Security config.

github-actions · 2025-07-02T11:32:56Z

Test Results

1 237 files ± 0 1 237 suites ±0 1h 14m 53s ⏱️ - 5m 38s
8 505 tests +13 8 445 ✅ +13 60 💤 ±0 0 ❌ ±0
8 883 runs +12 8 816 ✅ +14 67 💤 - 2 0 ❌ ±0

Results for commit 38c1104. ± Comparison against base commit 7bc36c6.

This pull request removes 1 and adds 14 tests. Note that renamed tests count towards both.

com.vaadin.flow.spring.security.stateless.JwtStatelessAuthenticationTest ‑ authenticated_jwtCookieUpdatedAtEveryRequest

com.vaadin.flow.spring.flowsecurity.AppViewIT ‑ access_restricted_to_all_by_default
com.vaadin.flow.spring.flowsecuritycontextpath.AppViewIT ‑ access_restricted_to_all_by_default
com.vaadin.flow.spring.flowsecurityreverseproxy.AppViewIT ‑ access_restricted_to_all_by_default
com.vaadin.flow.spring.flowsecurityurlmapping.AppViewIT ‑ access_restricted_to_all_by_default
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ access_restricted_to_all_by_default
com.vaadin.flow.spring.security.RequestUtilTest ‑ testFlowRoute_fooMappedServlet_notAView
com.vaadin.flow.spring.security.RequestUtilTest ‑ testFlowRoute_fooMappedServlet_privateView
com.vaadin.flow.spring.security.RequestUtilTest ‑ testFlowRoute_fooMappedServlet_publicView
com.vaadin.flow.spring.security.RequestUtilTest ‑ testFlowRoute_fooMappedServlet_publicViewPathOutsideServlet
com.vaadin.flow.spring.security.RequestUtilTest ‑ testFlowRoute_rootMappedServlet_notAView
…

♻️ This comment has been updated with latest results.

knoobie · 2025-07-02T18:09:50Z

...aadin-spring-tests/test-spring-security-flow/src/main/resources/static/restricted/secret.txt

@@ -0,0 +1 @@
+Top secret restricted content


nit: shouldn't this be labelled feat! instead of chore! so that it's pick-uped by the release notes? It's really frustrating to look at git diff what has really happened between releases.. especially if something so important happens 😬

Makes sense! I was just thinking that this change is not "valuable" enough to be called feature, but this is subjective.

You could also call it fix! or include chore! in the release notes templates :) in the end I'm open for all options.. just no information is bad imho

Let's use this as example #21711 I had to bisect the last releases to check if this commit was released in there because it wasn't mentioned 😬

mshabarov · 2025-07-03T06:02:57Z

...t-spring-security-flow/src/main/java/com/vaadin/flow/spring/flowsecurity/SecurityConfig.java

+                .requestMatchers(new AntPathRequestMatcher("/private"))
+                .authenticated()
+                .requestMatchers(new AntPathRequestMatcher("/admin"))
+                .hasAnyRole(ROLE_ADMIN));


I don't really understand why these additions are needed after removing authenticated() from Vaadin web security. This SecurityConfig is not based on Vaadin class so how changes there affect this configuration...Main question how it worked before

After removing authenticated() request matcher, flow's access control is not called unless navigation happens via Flow API (e.g. from menu). Flow should apply authenticated() request matcher for flow routes annotated with PermitAll or RolesAllowed automatically. This needs an update in VaadinSecurityConfigurer and VaadinWebSecurity.
AnonymousAllowed has a request matcher already. DenyAll should work already since access is denied by default.

I've updated PR to do this now, but it might still need to also check parent layout annotations too.

Added check for parent layouts and auto-layout.

knoobie · 2025-07-24T12:55:36Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinSecurityConfigurer.java

@@ -734,7 +734,9 @@ private AccessDeniedHandler createAccessDeniedHandler() {

    private void customizeAuthorizeHttpRequests(
            AuthorizeHttpRequestsConfigurer<HttpSecurity>.AuthorizationManagerRequestMatcherRegistry registry) {
-        registry.requestMatchers(defaultPermitMatcher()).permitAll();
+        registry.requestMatchers(defaultPermitMatcher()).permitAll()
+                .requestMatchers(getRequestUtil()::isAuthenticatedRoute)


I'm wondering if this should only be done if enableNavigationAccessControl returns true. I don't wanna use your access control annotations in my application - wouldn't this require me to add them? (see your test where you had to add @permitAll

This wouldn't require you to add them. but if you do add them, then they are noticed automatically by this request matcher.
You can also disable all default configuration of authorized requests (including this whole customizeAuthorizeHttpRequests method) with enableAuthorizedRequestsConfiguration set to false.

Oh, and as isAuthenticatedRoute is not actually calling navigation access control, we don't need to check enableNavigationAccessControl here.

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/RequestUtil.java

mcollovati · 2025-07-25T09:51:01Z

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/RequestUtil.java

+            return false;
+        }
+
+        if (isAuthenticationNeeded(targetView)) {


I think this method should also use NavigationAccessControl (if enabled) to determine if the route requires authentication. I'd say that at the very end it should return true if the NAC decision is DENY or REJECT.
This would take into account out-of-the-box AnnotatedViewAccessChecker (if used, since checkers can be disabled) and custom access checker as well.

We can probably simplify this by removing this annotation check and return true here if it's flow view. Then all flow views will go through access control in the end. Just need to test this once more.

And let's rename the method.

Makes sense, given that anonymous routes are already allowed before.
However, I think that the check should return true only if NAC is enabled, otherwise we cannot be sure if the view should be protected or not.

Flow routes with PermitAll or RolesAllowed annotation are checked by default with `.requestMatchers(requestUtil::isAuthenticatedRoute).authenticated()`. checkForBrowserErrors method in AbstractIT allows now 403 error for various tests.

Keeping 'routepathaccesschecker' in purpose using deprecated VaadinWebSecurity for now.

Added check for parent layouts and auto-layout.

All flow views are configured with authenticated() rule, not just PermitAll and RolesAllowed annotated routes. This will ensure that all configured access checkers are triggered for not-annotated views too.

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/RequestUtil.java

sonarqubecloud · 2025-07-30T10:37:34Z

Quality Gate passed

Issues
61 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

vaadin-bot added the +0.0.1 label Jul 2, 2025

knoobie reviewed Jul 2, 2025

View reviewed changes

mshabarov commented Jul 3, 2025

View reviewed changes

mshabarov changed the title ~~chore!: Restrict access by default with Spring Security~~ feat!: Restrict access by default with Spring Security Jul 3, 2025

mcollovati requested a review from tltv July 21, 2025 11:36

vaadin-bot added +1.0.0 and removed +0.0.1 labels Jul 24, 2025

knoobie reviewed Jul 24, 2025

View reviewed changes

vaadin-bot added +0.0.1 and removed +1.0.0 labels Jul 24, 2025

tltv requested review from mcollovati and removed request for tltv July 25, 2025 09:19

mcollovati requested changes Jul 25, 2025

View reviewed changes

vaadin-bot added +0.1.0 and removed +0.0.1 labels Jul 25, 2025

mshabarov and others added 10 commits July 30, 2025 10:34

chore!: Restrict access by default with Spring Security

a4e8ab6

add new test and fix existing tests

d3ced55

Require auth for PermitAll/RolesAllowed routes

aca72ca

Flow routes with PermitAll or RolesAllowed annotation are checked by default with `.requestMatchers(requestUtil::isAuthenticatedRoute).authenticated()`. checkForBrowserErrors method in AbstractIT allows now 403 error for various tests.

Updated more ITs for deny by default

bdbda30

Keeping 'routepathaccesschecker' in purpose using deprecated VaadinWebSecurity for now.

Updated even more ITs for deny by default

713dac7

Update RequestUtil.isAuthenticatedRouteInternal(HttpServletRequest)

9330fb1

Added check for parent layouts and auto-layout.

Apply authenticated() for all flow views

fdd5472

All flow views are configured with authenticated() rule, not just PermitAll and RolesAllowed annotated routes. This will ensure that all configured access checkers are triggered for not-annotated views too.

Add NAC enabled check

0e4c78c

Refactor by code review findings

2f52530

Resolve conflicts after rebase

b6641f1

tltv force-pushed the deny-by-default branch from 266eae7 to b6641f1 Compare July 30, 2025 07:53

vaadin-bot added +0.0.1 and removed +0.1.0 labels Jul 30, 2025

Fix compilation error

71fecc0

vaadin-bot added +0.1.0 and removed +0.0.1 labels Jul 30, 2025

mcollovati reviewed Jul 30, 2025

View reviewed changes

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/RequestUtil.java Outdated Show resolved Hide resolved

Merge branch 'main' into deny-by-default

ff06b16

vaadin-bot added +0.0.1 and removed +0.1.0 labels Jul 30, 2025

Add isSecuredFlowRouteInternal and refactor isFlowRouteInternal

38c1104

mcollovati approved these changes Jul 30, 2025

View reviewed changes

mshabarov marked this pull request as ready for review August 6, 2025 10:43

mshabarov merged commit cc1d64f into main Aug 6, 2025
27 checks passed

mshabarov deleted the deny-by-default branch August 6, 2025 10:44

feat!: Restrict access by default with Spring Security #21831

feat!: Restrict access by default with Spring Security #21831

Uh oh!

Conversation

mshabarov commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

knoobie Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knoobie Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Jul 30, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2025 •

edited

Loading

knoobie Jul 2, 2025 •

edited

Loading

knoobie Jul 3, 2025 •

edited

Loading