From 72109e292142feca0a9376db09b1652637812b13 Mon Sep 17 00:00:00 2001 From: Igor Pelesic Date: Thu, 18 Nov 2021 12:48:43 +0100 Subject: [PATCH] PermitAllSupport supports AuthorizeHttpRequestsConfigurer PermitAllSupport supports either an ExpressionUrlAuthorizationConfigurer or an AuthorizeHttpRequestsConfigurer. If none or both are configured an error message is thrown. Closes gh-10482 --- .../AuthorizeHttpRequestsConfigurer.java | 27 +++++++- .../web/configurers/PermitAllSupport.java | 19 ++++-- .../configurers/PermitAllSupportTests.java | 66 ++++++++++++++++++- ...MatcherDelegatingAuthorizationManager.java | 17 ++++- ...erDelegatingAuthorizationManagerTests.java | 38 +++++++++++ 5 files changed, 157 insertions(+), 10 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java index d8357f38fd8..c2e02ecb177 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/AuthorizeHttpRequestsConfigurer.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -16,6 +16,7 @@ package org.springframework.security.config.annotation.web.configurers; +import java.util.LinkedHashMap; import java.util.List; import jakarta.servlet.http.HttpServletRequest; @@ -46,6 +47,9 @@ public final class AuthorizeHttpRequestsConfigurer> extends AbstractHttpConfigurer, H> { + static final AuthorizationManager permitAllAuthorizationManager = (a, + o) -> new AuthorizationDecision(true); + private final AuthorizationManagerRequestMatcherRegistry registry; /** @@ -81,6 +85,12 @@ private AuthorizationManagerRequestMatcherRegistry addMapping(List manager) { + this.registry.addFirst(matcher, manager); + return this.registry; + } + /** * Registry for mapping a {@link RequestMatcher} to an {@link AuthorizationManager}. * @@ -106,6 +116,19 @@ private void addMapping(RequestMatcher matcher, AuthorizationManager manager) { + this.unmappedMatchers = null; + this.managerBuilder.mappings((m) -> { + LinkedHashMap> reorderedMap = new LinkedHashMap<>( + m.size() + 1); + reorderedMap.put(matcher, manager); + reorderedMap.putAll(m); + m.clear(); + m.putAll(reorderedMap); + }); + this.mappingCount++; + } + private AuthorizationManager createAuthorizationManager() { Assert.state(this.unmappedMatchers == null, () -> "An incomplete mapping was found for " + this.unmappedMatchers @@ -209,7 +232,7 @@ protected List getMatchers() { * customizations */ public AuthorizationManagerRequestMatcherRegistry permitAll() { - return access((a, o) -> new AuthorizationDecision(true)); + return access(permitAllAuthorizationManager); } /** diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupport.java b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupport.java index bb6bd63f60c..ea0eb21b0fc 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupport.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupport.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2021 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. @@ -48,11 +48,22 @@ static void permitAll(HttpSecurityBuilder> http RequestMatcher... requestMatchers) { ExpressionUrlAuthorizationConfigurer configurer = http .getConfigurer(ExpressionUrlAuthorizationConfigurer.class); - Assert.state(configurer != null, "permitAll only works with HttpSecurity.authorizeRequests()"); + AuthorizeHttpRequestsConfigurer httpConfigurer = http.getConfigurer(AuthorizeHttpRequestsConfigurer.class); + + boolean oneConfigurerPresent = configurer == null ^ httpConfigurer == null; + Assert.state(oneConfigurerPresent, + "permitAll only works with either HttpSecurity.authorizeRequests() or HttpSecurity.authorizeHttpRequests(). " + + "Please define one or the other but not both."); + for (RequestMatcher matcher : requestMatchers) { if (matcher != null) { - configurer.getRegistry().addMapping(0, new UrlMapping(matcher, - SecurityConfig.createList(ExpressionUrlAuthorizationConfigurer.permitAll))); + if (configurer != null) { + configurer.getRegistry().addMapping(0, new UrlMapping(matcher, + SecurityConfig.createList(ExpressionUrlAuthorizationConfigurer.permitAll))); + } + else { + httpConfigurer.addFirst(matcher, AuthorizeHttpRequestsConfigurer.permitAllAuthorizationManager); + } } } } diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupportTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupportTests.java index 70752fb1c5f..9a5174f810c 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupportTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/PermitAllSupportTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2021 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. @@ -61,11 +61,32 @@ public void performWhenUsingPermitAllExactUrlRequestMatcherThenMatchesExactUrl() this.mvc.perform(getWithCsrf).andExpect(status().isFound()); } + @Test + public void performWhenUsingPermitAllExactUrlRequestMatcherThenMatchesExactUrlWithAuthorizeHttp() throws Exception { + this.spring.register(PermitAllConfigAuthorizeHttpRequests.class).autowire(); + MockHttpServletRequestBuilder request = get("/app/xyz").contextPath("/app"); + this.mvc.perform(request).andExpect(status().isNotFound()); + MockHttpServletRequestBuilder getWithQuery = get("/app/xyz?def").contextPath("/app"); + this.mvc.perform(getWithQuery).andExpect(status().isFound()); + MockHttpServletRequestBuilder postWithQueryAndCsrf = post("/app/abc?def").with(csrf()).contextPath("/app"); + this.mvc.perform(postWithQueryAndCsrf).andExpect(status().isNotFound()); + MockHttpServletRequestBuilder getWithCsrf = get("/app/abc").with(csrf()).contextPath("/app"); + this.mvc.perform(getWithCsrf).andExpect(status().isFound()); + } + @Test public void configureWhenNotAuthorizeRequestsThenException() { assertThatExceptionOfType(BeanCreationException.class) - .isThrownBy(() -> this.spring.register(NoAuthorizedUrlsConfig.class).autowire()) - .withMessageContaining("permitAll only works with HttpSecurity.authorizeRequests"); + .isThrownBy(() -> this.spring.register(NoAuthorizedUrlsConfig.class).autowire()).withMessageContaining( + "permitAll only works with either HttpSecurity.authorizeRequests() or HttpSecurity.authorizeHttpRequests()"); + } + + @Test + public void configureWhenBothAuthorizeRequestsAndAuthorizeHttpRequestsThenException() { + assertThatExceptionOfType(BeanCreationException.class) + .isThrownBy(() -> this.spring.register(PermitAllConfigWithBothConfigs.class).autowire()) + .withMessageContaining( + "permitAll only works with either HttpSecurity.authorizeRequests() or HttpSecurity.authorizeHttpRequests()"); } @EnableWebSecurity @@ -86,6 +107,45 @@ protected void configure(HttpSecurity http) throws Exception { } + @EnableWebSecurity + static class PermitAllConfigAuthorizeHttpRequests extends WebSecurityConfigurerAdapter { + + @Override + protected void configure(HttpSecurity http) throws Exception { + // @formatter:off + http + .authorizeHttpRequests() + .anyRequest().authenticated() + .and() + .formLogin() + .loginPage("/xyz").permitAll() + .loginProcessingUrl("/abc?def").permitAll(); + // @formatter:on + } + + } + + @EnableWebSecurity + static class PermitAllConfigWithBothConfigs extends WebSecurityConfigurerAdapter { + + @Override + protected void configure(HttpSecurity http) throws Exception { + // @formatter:off + http + .authorizeRequests() + .anyRequest().authenticated() + .and() + .authorizeHttpRequests() + .anyRequest().authenticated() + .and() + .formLogin() + .loginPage("/xyz").permitAll() + .loginProcessingUrl("/abc?def").permitAll(); + // @formatter:on + } + + } + @EnableWebSecurity static class NoAuthorizedUrlsConfig extends WebSecurityConfigurerAdapter { diff --git a/web/src/main/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManager.java b/web/src/main/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManager.java index f00b027b0e4..ef0616709a4 100644 --- a/web/src/main/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManager.java +++ b/web/src/main/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -18,6 +18,7 @@ import java.util.LinkedHashMap; import java.util.Map; +import java.util.function.Consumer; import java.util.function.Supplier; import jakarta.servlet.http.HttpServletRequest; @@ -112,6 +113,20 @@ public Builder add(RequestMatcher matcher, AuthorizationManager>> mappingsConsumer) { + Assert.notNull(mappingsConsumer, "mappingsConsumer cannot be null"); + mappingsConsumer.accept(this.mappings); + return this; + } + /** * Creates a {@link RequestMatcherDelegatingAuthorizationManager} instance. * @return the {@link RequestMatcherDelegatingAuthorizationManager} instance diff --git a/web/src/test/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManagerTests.java b/web/src/test/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManagerTests.java index 829866340a1..952132522b1 100644 --- a/web/src/test/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManagerTests.java +++ b/web/src/test/java/org/springframework/security/web/access/intercept/RequestMatcherDelegatingAuthorizationManagerTests.java @@ -22,9 +22,11 @@ import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.authorization.AuthorityAuthorizationManager; import org.springframework.security.authorization.AuthorizationDecision; import org.springframework.security.core.Authentication; import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher; +import org.springframework.security.web.util.matcher.AnyRequestMatcher; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -83,4 +85,40 @@ public void checkWhenMultipleMappingsConfiguredThenDelegatesMatchingManager() { assertThat(abstain).isNull(); } + @Test + public void checkWhenMultipleMappingsConfiguredWithConsumerThenDelegatesMatchingManager() { + RequestMatcherDelegatingAuthorizationManager manager = RequestMatcherDelegatingAuthorizationManager.builder() + .mappings((m) -> { + m.put(new MvcRequestMatcher(null, "/grant"), (a, o) -> new AuthorizationDecision(true)); + m.put(AnyRequestMatcher.INSTANCE, AuthorityAuthorizationManager.hasRole("ADMIN")); + m.put(new MvcRequestMatcher(null, "/deny"), (a, o) -> new AuthorizationDecision(false)); + m.put(new MvcRequestMatcher(null, "/afterAny"), (a, o) -> new AuthorizationDecision(true)); + }).build(); + + Supplier authentication = () -> new TestingAuthenticationToken("user", "password", "ROLE_USER"); + + AuthorizationDecision grant = manager.check(authentication, new MockHttpServletRequest(null, "/grant")); + assertThat(grant).isNotNull(); + assertThat(grant.isGranted()).isTrue(); + + AuthorizationDecision deny = manager.check(authentication, new MockHttpServletRequest(null, "/deny")); + assertThat(deny).isNotNull(); + assertThat(deny.isGranted()).isFalse(); + + AuthorizationDecision afterAny = manager.check(authentication, new MockHttpServletRequest(null, "/afterAny")); + assertThat(afterAny).isNotNull(); + assertThat(afterAny.isGranted()).isFalse(); + + AuthorizationDecision unmapped = manager.check(authentication, new MockHttpServletRequest(null, "/unmapped")); + assertThat(unmapped).isNotNull(); + assertThat(unmapped.isGranted()).isFalse(); + } + + @Test + public void addWhenMappingsConsumerNullThenException() { + assertThatIllegalArgumentException() + .isThrownBy(() -> RequestMatcherDelegatingAuthorizationManager.builder().mappings(null).build()) + .withMessage("mappingsConsumer cannot be null"); + } + }