From 495028eb85475d9d0ba25a1ae5d2e2c4eb049209 Mon Sep 17 00:00:00 2001 From: Evgeniy Cheban Date: Tue, 17 May 2022 19:55:45 +0300 Subject: [PATCH] Some Security Expressions cause NPE when used within Query annotation Added trustResolver, roleHierarchy, permissionEvaluator, defaultRolePrefix fields to SecurityEvaluationContextExtension along with setter methods to override defaults. Closes gh-11196 --- .../SecurityEvaluationContextExtension.java | 73 ++++++++++++++++- ...curityEvaluationContextExtensionTests.java | 79 ++++++++++++++++++- 2 files changed, 149 insertions(+), 3 deletions(-) diff --git a/data/src/main/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtension.java b/data/src/main/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtension.java index 3696904a9a..a4ec7b00d2 100644 --- a/data/src/main/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtension.java +++ b/data/src/main/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtension.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,10 +17,17 @@ package org.springframework.security.data.repository.query; import org.springframework.data.spel.spi.EvaluationContextExtension; +import org.springframework.security.access.PermissionEvaluator; +import org.springframework.security.access.expression.DenyAllPermissionEvaluator; import org.springframework.security.access.expression.SecurityExpressionRoot; +import org.springframework.security.access.hierarchicalroles.NullRoleHierarchy; +import org.springframework.security.access.hierarchicalroles.RoleHierarchy; +import org.springframework.security.authentication.AuthenticationTrustResolver; +import org.springframework.security.authentication.AuthenticationTrustResolverImpl; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContext; import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.util.Assert; /** *

@@ -77,12 +84,21 @@ import org.springframework.security.core.context.SecurityContextHolder; * it. * * @author Rob Winch + * @author Evgeniy Cheban * @since 4.0 */ public class SecurityEvaluationContextExtension implements EvaluationContextExtension { private Authentication authentication; + private AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); + + private RoleHierarchy roleHierarchy = new NullRoleHierarchy(); + + private PermissionEvaluator permissionEvaluator = new DenyAllPermissionEvaluator(); + + private String defaultRolePrefix = "ROLE_"; + /** * Creates a new instance that uses the current {@link Authentication} found on the * {@link org.springframework.security.core.context.SecurityContextHolder}. @@ -106,8 +122,13 @@ public class SecurityEvaluationContextExtension implements EvaluationContextExte @Override public SecurityExpressionRoot getRootObject() { Authentication authentication = getAuthentication(); - return new SecurityExpressionRoot(authentication) { + SecurityExpressionRoot root = new SecurityExpressionRoot(authentication) { }; + root.setTrustResolver(this.trustResolver); + root.setRoleHierarchy(this.roleHierarchy); + root.setPermissionEvaluator(this.permissionEvaluator); + root.setDefaultRolePrefix(this.defaultRolePrefix); + return root; } private Authentication getAuthentication() { @@ -118,4 +139,52 @@ public class SecurityEvaluationContextExtension implements EvaluationContextExte return context.getAuthentication(); } + /** + * Sets the {@link AuthenticationTrustResolver} to be used. Default is + * {@link AuthenticationTrustResolverImpl}. Cannot be null. + * @param trustResolver the {@link AuthenticationTrustResolver} to use + * @since 5.8 + */ + public void setTrustResolver(AuthenticationTrustResolver trustResolver) { + Assert.notNull(trustResolver, "trustResolver cannot be null"); + this.trustResolver = trustResolver; + } + + /** + * Sets the {@link RoleHierarchy} to be used. Default is {@link NullRoleHierarchy}. + * Cannot be null. + * @param roleHierarchy the {@link RoleHierarchy} to use + * @since 5.8 + */ + public void setRoleHierarchy(RoleHierarchy roleHierarchy) { + Assert.notNull(roleHierarchy, "roleHierarchy cannot be null"); + this.roleHierarchy = roleHierarchy; + } + + /** + * Sets the {@link PermissionEvaluator} to be used. Default is + * {@link DenyAllPermissionEvaluator}. Cannot be null. + * @param permissionEvaluator the {@link PermissionEvaluator} to use + * @since 5.8 + */ + public void setPermissionEvaluator(PermissionEvaluator permissionEvaluator) { + Assert.notNull(permissionEvaluator, "permissionEvaluator cannot be null"); + this.permissionEvaluator = permissionEvaluator; + } + + /** + * Sets the default prefix to be added to + * {@link org.springframework.security.access.expression.SecurityExpressionRoot#hasAnyRole(String...)} + * or + * {@link org.springframework.security.access.expression.SecurityExpressionRoot#hasRole(String)}. + * For example, if hasRole("ADMIN") or hasRole("ROLE_ADMIN") is passed in, then the + * role ROLE_ADMIN will be used when the defaultRolePrefix is "ROLE_" (default). + * @param defaultRolePrefix the default prefix to add to roles. The default is + * "ROLE_". + * @since 5.8 + */ + public void setDefaultRolePrefix(String defaultRolePrefix) { + this.defaultRolePrefix = defaultRolePrefix; + } + } diff --git a/data/src/test/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java b/data/src/test/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java index a890a463af..293c114fb1 100644 --- a/data/src/test/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java +++ b/data/src/test/java/org/springframework/security/data/repository/query/SecurityEvaluationContextExtensionTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2022 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,7 +20,13 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.security.access.PermissionEvaluator; +import org.springframework.security.access.expression.DenyAllPermissionEvaluator; import org.springframework.security.access.expression.SecurityExpressionRoot; +import org.springframework.security.access.hierarchicalroles.NullRoleHierarchy; +import org.springframework.security.access.hierarchicalroles.RoleHierarchy; +import org.springframework.security.authentication.AuthenticationTrustResolver; +import org.springframework.security.authentication.AuthenticationTrustResolverImpl; import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.core.context.SecurityContextHolder; @@ -69,6 +75,77 @@ public class SecurityEvaluationContextExtensionTests { assertThat(getRoot().getAuthentication()).isSameAs(explicit); } + @Test + public void setTrustResolverWhenNullThenIllegalArgumentException() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + assertThatIllegalArgumentException().isThrownBy(() -> this.securityExtension.setTrustResolver(null)) + .withMessage("trustResolver cannot be null"); + } + + @Test + public void setTrustResolverWhenNotNullThenVerifyRootObject() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + AuthenticationTrustResolver trustResolver = new AuthenticationTrustResolverImpl(); + this.securityExtension.setTrustResolver(trustResolver); + assertThat(getRoot()).extracting("trustResolver").isEqualTo(trustResolver); + } + + @Test + public void setRoleHierarchyWhenNullThenIllegalArgumentException() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + assertThatIllegalArgumentException().isThrownBy(() -> this.securityExtension.setRoleHierarchy(null)) + .withMessage("roleHierarchy cannot be null"); + } + + @Test + public void setRoleHierarchyWhenNotNullThenVerifyRootObject() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + RoleHierarchy roleHierarchy = new NullRoleHierarchy(); + this.securityExtension.setRoleHierarchy(roleHierarchy); + assertThat(getRoot()).extracting("roleHierarchy").isEqualTo(roleHierarchy); + } + + @Test + public void setPermissionEvaluatorWhenNullThenIllegalArgumentException() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + assertThatIllegalArgumentException().isThrownBy(() -> this.securityExtension.setPermissionEvaluator(null)) + .withMessage("permissionEvaluator cannot be null"); + } + + @Test + public void setPermissionEvaluatorWhenNotNullThenVerifyRootObject() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + PermissionEvaluator permissionEvaluator = new DenyAllPermissionEvaluator(); + this.securityExtension.setPermissionEvaluator(permissionEvaluator); + assertThat(getRoot()).extracting("permissionEvaluator").isEqualTo(permissionEvaluator); + } + + @Test + public void setDefaultRolePrefixWhenCustomThenVerifyRootObject() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + String defaultRolePrefix = "CUSTOM_"; + this.securityExtension.setDefaultRolePrefix(defaultRolePrefix); + assertThat(getRoot()).extracting("defaultRolePrefix").isEqualTo(defaultRolePrefix); + } + + @Test + public void getRootObjectWhenAdditionalFieldsNotSetThenVerifyDefaults() { + TestingAuthenticationToken explicit = new TestingAuthenticationToken("explicit", "password", "ROLE_EXPLICIT"); + this.securityExtension = new SecurityEvaluationContextExtension(explicit); + SecurityExpressionRoot root = getRoot(); + assertThat(root).extracting("trustResolver").isInstanceOf(AuthenticationTrustResolverImpl.class); + assertThat(root).extracting("roleHierarchy").isInstanceOf(NullRoleHierarchy.class); + assertThat(root).extracting("permissionEvaluator").isInstanceOf(DenyAllPermissionEvaluator.class); + assertThat(root).extracting("defaultRolePrefix").isEqualTo("ROLE_"); + } + private SecurityExpressionRoot getRoot() { return this.securityExtension.getRootObject(); }