From f16d47c7b5d6e2789d5f80f995026c58621885ce Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 5 Oct 2022 15:57:02 -0600 Subject: [PATCH 1/3] Polish DefaultHttpSecurityExpressionHandler Issue gh-11105 --- .../access/expression/DefaultHttpSecurityExpressionHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/main/java/org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java b/web/src/main/java/org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java index a75c0d2959..7a3b74c630 100644 --- a/web/src/main/java/org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java +++ b/web/src/main/java/org/springframework/security/web/access/expression/DefaultHttpSecurityExpressionHandler.java @@ -90,7 +90,7 @@ public class DefaultHttpSecurityExpressionHandler extends AbstractSecurityExpres * "ROLE_". */ public void setDefaultRolePrefix(String defaultRolePrefix) { - Assert.hasText(defaultRolePrefix, "defaultRolePrefix cannot be empty"); + Assert.notNull(defaultRolePrefix, "defaultRolePrefix cannot be null"); this.defaultRolePrefix = defaultRolePrefix; } From 7043ef6ccbb90e0680069fa638fadba05c7b3187 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 5 Oct 2022 16:35:14 -0600 Subject: [PATCH 2/3] Polish OpaqueTokenAuthenticationConverterTests Issue gh-11665 --- .../OAuth2ResourceServerConfigurerTests.java | 6 +-- ...sourceServerBeanDefinitionParserTests.java | 47 ++----------------- ...-OpaqueTokenAndAuthenticationConverter.xml | 3 +- 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java index b66bb86bd5..681ccc9681 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java @@ -1391,11 +1391,10 @@ public class OAuth2ResourceServerConfigurerTests { } @Test - public void getWhenCustomAuthenticationConverterThenConverts() throws Exception { + public void getWhenCustomAuthenticationConverterThenUsed() throws Exception { this.spring.register(RestOperationsConfig.class, OpaqueTokenAuthenticationConverterConfig.class, BasicController.class).autowire(); - OpaqueTokenAuthenticationConverter authenticationConverter = this.spring.getContext() - .getBean(OpaqueTokenAuthenticationConverter.class); + OpaqueTokenAuthenticationConverter authenticationConverter = bean(OpaqueTokenAuthenticationConverter.class); given(authenticationConverter.convert(anyString(), any(OAuth2AuthenticatedPrincipal.class))) .willReturn(new TestingAuthenticationToken("jdoe", null, Collections.emptyList())); mockRestOperations(json("Active")); @@ -1404,6 +1403,7 @@ public class OAuth2ResourceServerConfigurerTests { .andExpect(status().isOk()) .andExpect(content().string("jdoe")); // @formatter:on + verify(authenticationConverter).convert(any(), any()); } private static void registerMockBean(GenericApplicationContext context, String name, Class clazz) { diff --git a/config/src/test/java/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests.java b/config/src/test/java/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests.java index f89883a646..ce7d2e1838 100644 --- a/config/src/test/java/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests.java +++ b/config/src/test/java/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests.java @@ -23,7 +23,6 @@ import java.security.interfaces.RSAPublicKey; import java.time.Clock; import java.time.Instant; import java.time.ZoneId; -import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -69,17 +68,14 @@ import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.RequestEntity; import org.springframework.http.ResponseEntity; -import org.springframework.security.authentication.AbstractAuthenticationToken; import org.springframework.security.authentication.AuthenticationManagerResolver; import org.springframework.security.authentication.AuthenticationServiceException; +import org.springframework.security.authentication.TestingAuthenticationToken; import org.springframework.security.config.http.OAuth2ResourceServerBeanDefinitionParser.JwtBeanDefinitionParser; import org.springframework.security.config.http.OAuth2ResourceServerBeanDefinitionParser.OpaqueTokenBeanDefinitionParser; import org.springframework.security.config.test.SpringTestContext; import org.springframework.security.config.test.SpringTestContextExtension; -import org.springframework.security.core.Authentication; -import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.context.SecurityContextHolderStrategy; -import org.springframework.security.oauth2.core.OAuth2AuthenticatedPrincipal; import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.OAuth2TokenValidator; import org.springframework.security.oauth2.core.OAuth2TokenValidatorResult; @@ -673,13 +669,13 @@ public class OAuth2ResourceServerBeanDefinitionParserTests { this.spring.configLocations(xml("OpaqueTokenRestOperations"), xml("OpaqueTokenAndAuthenticationConverter")) .autowire(); mockRestOperations(json("Active")); + OpaqueTokenAuthenticationConverter converter = bean(OpaqueTokenAuthenticationConverter.class); + given(converter.convert(any(), any())).willReturn(new TestingAuthenticationToken("user", "pass", "app")); // @formatter:off this.mvc.perform(get("/authenticated").header("Authorization", "Bearer token")) .andExpect(status().isNotFound()); - - this.mvc.perform(get("/authenticated").header("Authorization", "Bearer invalidToken")) - .andExpect(status().isUnauthorized()); // @formatter:on + verify(converter).convert(any(), any()); } @Test @@ -1116,39 +1112,4 @@ public class OAuth2ResourceServerBeanDefinitionParserTests { } - public static class TestAuthentication extends AbstractAuthenticationToken { - - private final String introspectedToken; - - public TestAuthentication(String introspectedToken, Collection authorities) { - super(authorities); - this.introspectedToken = introspectedToken; - } - - @Override - public Object getCredentials() { - return this.introspectedToken; - } - - @Override - public Object getPrincipal() { - return this.introspectedToken; - } - - @Override - public boolean isAuthenticated() { - return "token".equals(this.introspectedToken); - } - - } - - public static class TestOpaqueTokenAuthenticationConverter implements OpaqueTokenAuthenticationConverter { - - @Override - public Authentication convert(String introspectedToken, OAuth2AuthenticatedPrincipal authenticatedPrincipal) { - return new TestAuthentication(introspectedToken, Collections.emptyList()); - } - - } - } diff --git a/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-OpaqueTokenAndAuthenticationConverter.xml b/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-OpaqueTokenAndAuthenticationConverter.xml index 5a22b05c8a..8df4949985 100644 --- a/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-OpaqueTokenAndAuthenticationConverter.xml +++ b/config/src/test/resources/org/springframework/security/config/http/OAuth2ResourceServerBeanDefinitionParserTests-OpaqueTokenAndAuthenticationConverter.xml @@ -22,7 +22,8 @@ http://www.springframework.org/schema/beans https://www.springframework.org/schema/beans/spring-beans.xsd"> + class="org.mockito.Mockito" factory-method="mock"> + From b4d13e7726838311fd192de54e9c6faedc1c9c89 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 5 Oct 2022 19:49:06 -0600 Subject: [PATCH 3/3] Polish use-authorization-manager - Use SecurityContextHolderStrategy - Allow empty role prefix - Disallow access-decision-manager-ref and authorization-manager-ref together Issue gh-11305 --- .../config/http/AuthorizationFilterParser.java | 18 +++++++++++++++++- .../config/http/HttpConfigurationBuilder.java | 2 +- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/http/AuthorizationFilterParser.java b/config/src/main/java/org/springframework/security/config/http/AuthorizationFilterParser.java index e3ac6c915a..1dcd2888c0 100644 --- a/config/src/main/java/org/springframework/security/config/http/AuthorizationFilterParser.java +++ b/config/src/main/java/org/springframework/security/config/http/AuthorizationFilterParser.java @@ -50,6 +50,8 @@ class AuthorizationFilterParser implements BeanDefinitionParser { private static final String ATT_USE_EXPRESSIONS = "use-expressions"; + private static final String ATT_ACCESS_DECISION_MANAGER_REF = "access-decision-manager-ref"; + private static final String ATT_HTTP_METHOD = "method"; private static final String ATT_PATTERN = "pattern"; @@ -60,6 +62,12 @@ class AuthorizationFilterParser implements BeanDefinitionParser { private String authorizationManagerRef; + private final BeanMetadataElement securityContextHolderStrategy; + + AuthorizationFilterParser(BeanMetadataElement securityContextHolderStrategy) { + this.securityContextHolderStrategy = securityContextHolderStrategy; + } + @Override public BeanDefinition parse(Element element, ParserContext parserContext) { if (!isUseExpressions(element)) { @@ -67,10 +75,16 @@ class AuthorizationFilterParser implements BeanDefinitionParser { element); return null; } + if (StringUtils.hasText(element.getAttribute(ATT_ACCESS_DECISION_MANAGER_REF))) { + parserContext.getReaderContext().error( + "AuthorizationManager cannot be used in conjunction with `access-decision-manager-ref`", element); + return null; + } this.authorizationManagerRef = createAuthorizationManager(element, parserContext); BeanDefinitionBuilder filterBuilder = BeanDefinitionBuilder.rootBeanDefinition(AuthorizationFilter.class); filterBuilder.getRawBeanDefinition().setSource(parserContext.extractSource(element)); BeanDefinition filter = filterBuilder.addConstructorArgReference(this.authorizationManagerRef) + .addPropertyValue("securityContextHolderStrategy", this.securityContextHolderStrategy) .getBeanDefinition(); String id = element.getAttribute(AbstractBeanDefinitionParser.ID_ATTRIBUTE); if (StringUtils.hasText(id)) { @@ -172,7 +186,9 @@ class AuthorizationFilterParser implements BeanDefinitionParser { @Override public DefaultHttpSecurityExpressionHandler getBean() { - this.handler.setDefaultRolePrefix(this.rolePrefix); + if (this.rolePrefix != null) { + this.handler.setDefaultRolePrefix(this.rolePrefix); + } return this.handler; } diff --git a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java index 9bc23955bb..32c2fd9d73 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java @@ -729,7 +729,7 @@ class HttpConfigurationBuilder { } private void createAuthorizationFilter() { - AuthorizationFilterParser authorizationFilterParser = new AuthorizationFilterParser(); + AuthorizationFilterParser authorizationFilterParser = new AuthorizationFilterParser(this.holderStrategyRef); BeanDefinition fsiBean = authorizationFilterParser.parse(this.httpElt, this.pc); String fsiId = this.pc.getReaderContext().generateBeanName(fsiBean); this.pc.registerBeanComponent(new BeanComponentDefinition(fsiBean, fsiId));