Skip to content

Commit 246a40a

Browse files
authored
fix: warn less eagerly about denied access (#22798)
Updates AnnotatedViewAccessChecker to log a warning about denied access, based on Layout permissions, only when navigation is ongoing. Logs with trace level otherwise. Fixes: #22737
1 parent 63e4d4d commit 246a40a

File tree

4 files changed

+66
-12
lines changed

4 files changed

+66
-12
lines changed

flow-server/src/main/java/com/vaadin/flow/server/auth/AnnotatedViewAccessChecker.java

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,7 @@ public AccessCheckResult check(NavigationContext context) {
8484
boolean hasAccess = accessAnnotationChecker.hasAccess(layout,
8585
context.getPrincipal(), context::hasRole);
8686
if (!hasAccess) {
87-
LOGGER.warn(
88-
"Denied access to view due to layout '{}' access rules",
89-
layout.getSimpleName());
87+
logDeniedByLayoutAccessRules(context, targetView);
9088
return context.deny("Denied access to view due to layout '"
9189
+ targetView.getSimpleName() + "' access rules."
9290
+ "Consider adding one of the following annotations "
@@ -107,9 +105,8 @@ public AccessCheckResult check(NavigationContext context) {
107105
boolean hasAccess = accessAnnotationChecker.hasAccess(
108106
parent, context.getPrincipal(), context::hasRole);
109107
if (!hasAccess) {
110-
LOGGER.warn(
111-
"Denied access to view due to parent layout '{}' access rules",
112-
parent.getSimpleName());
108+
logDeniedByLayoutAccessRules(context, parent,
109+
"Denied access to view due to parent layout '{}' access rules");
113110
return context.deny(
114111
"Denied access to view due to parent layout '"
115112
+ targetView.getSimpleName()
@@ -141,9 +138,7 @@ public AccessCheckResult check(NavigationContext context) {
141138
.getTargetUrl(
142139
(Class<? extends Component>) targetView)
143140
.isEmpty()) {
144-
LOGGER.warn(
145-
"Denied access to view due to layout '{}' access rules",
146-
targetView.getSimpleName());
141+
logDeniedByLayoutAccessRules(context, targetView);
147142
denyReason = "Denied access to view due to layout '"
148143
+ targetView.getSimpleName() + "' access rules."
149144
+ "Consider adding one of the following annotations "
@@ -156,6 +151,21 @@ public AccessCheckResult check(NavigationContext context) {
156151
return context.deny(denyReason);
157152
}
158153

154+
private void logDeniedByLayoutAccessRules(NavigationContext context,
155+
Class<?> viewClass) {
156+
logDeniedByLayoutAccessRules(context, viewClass,
157+
"Denied access to view due to layout '{}' access rules");
158+
}
159+
160+
private void logDeniedByLayoutAccessRules(NavigationContext context,
161+
Class<?> viewClass, String msg) {
162+
if (context.isNavigating()) {
163+
LOGGER.warn(msg, viewClass.getSimpleName());
164+
} else {
165+
LOGGER.trace(msg, viewClass.getSimpleName());
166+
}
167+
}
168+
159169
private boolean isImplicitlyDenyAllAnnotated(Class<?> targetView) {
160170
return !(targetView.isAnnotationPresent(DenyAll.class)
161171
|| targetView.isAnnotationPresent(PermitAll.class)

flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationAccessControl.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,6 @@ public NavigationContext createNavigationContext(Class<?> navigationTarget,
447447
return new NavigationContext(vaadinService.getRouter(),
448448
navigationTarget, new Location(path), RouteParameters.empty(),
449449
getPrincipal(vaadinRequest), getRolesChecker(vaadinRequest),
450-
false);
450+
false, false);
451451
}
452452
}

flow-server/src/main/java/com/vaadin/flow/server/auth/NavigationContext.java

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ public final class NavigationContext {
6060
private final Predicate<String> roleChecker;
6161

6262
private final boolean errorHandling;
63+
private final boolean navigating;
6364

6465
/**
6566
* Creates a new navigation context instance.
@@ -80,10 +81,14 @@ public final class NavigationContext {
8081
* {@literal true} if the current navigation is related to an
8182
* error handling phase, {@literal false} for a regular
8283
* navigation to a target view
84+
* @param navigating
85+
* {@literal true} if the navigation is ongoing, {@literal false}
86+
* if not (e.g. during access checks outside of navigation)
8387
*/
8488
public NavigationContext(Router router, Class<?> navigationTarget,
8589
Location location, RouteParameters parameters, Principal principal,
86-
Predicate<String> roleChecker, boolean errorHandling) {
90+
Predicate<String> roleChecker, boolean errorHandling,
91+
boolean navigating) {
8792
this.router = Objects.requireNonNull(router, "router must no be null");
8893
this.navigationTarget = Objects.requireNonNull(navigationTarget,
8994
"navigationTarget must no be null");
@@ -95,6 +100,34 @@ public NavigationContext(Router router, Class<?> navigationTarget,
95100
"roleChecker must no be null");
96101
this.principal = principal;
97102
this.errorHandling = errorHandling;
103+
this.navigating = navigating;
104+
}
105+
106+
/**
107+
* Creates a new navigation context instance for ongoing navigation.
108+
*
109+
* @param router
110+
* the router that triggered the change, not {@literal null}
111+
* @param navigationTarget
112+
* navigation target class, not {@literal null}
113+
* @param location
114+
* the requested location, not {@literal null}
115+
* @param parameters
116+
* route parameters, not {@literal null}
117+
* @param principal
118+
* the principal of the user
119+
* @param roleChecker
120+
* a function that can answer if a user has a given role
121+
* @param errorHandling
122+
* {@literal true} if the current navigation is related to an
123+
* error handling phase, {@literal false} for a regular
124+
* navigation to a target view
125+
*/
126+
public NavigationContext(Router router, Class<?> navigationTarget,
127+
Location location, RouteParameters parameters, Principal principal,
128+
Predicate<String> roleChecker, boolean errorHandling) {
129+
this(router, navigationTarget, location, parameters, principal,
130+
roleChecker, errorHandling, true);
98131
}
99132

100133
/**
@@ -183,6 +216,17 @@ public boolean isErrorHandling() {
183216
return errorHandling;
184217
}
185218

219+
/**
220+
* Gets if navigation is ongoing or not. If not, navigation context is not
221+
* expected to produce access denial warnings in the logs eagerly.
222+
*
223+
* @return {@literal true} if the navigation is ongoing, {@literal false} if
224+
* not
225+
*/
226+
public boolean isNavigating() {
227+
return navigating;
228+
}
229+
186230
/**
187231
* Gets if the current user belongs the specified logical role.
188232
*

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ private boolean isAnonymousRouteInternal(HttpServletRequest request) {
366366
targetView,
367367
new Location(path,
368368
QueryParameters.full(request.getParameterMap())),
369-
target.getRouteParameters(), null, role -> false, false);
369+
target.getRouteParameters(), null, role -> false, false, false);
370370

371371
AccessCheckResult result = navigationAccessControl
372372
.checkAccess(navigationContext, productionMode);

0 commit comments

Comments
 (0)