From 2ba711c83a08607a19963bc2fa40dbb4afcd8f21 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Thu, 27 Oct 2022 16:33:42 -0400 Subject: [PATCH] Polish gh-929 --- .../src/docs/asciidoc/protocol-endpoints.adoc | 10 +++---- .../OidcUserInfoEndpointConfigurer.java | 29 ++++++++++--------- .../oidc/web/OidcUserInfoEndpointFilter.java | 24 +++++++-------- .../web/configurers/OidcUserInfoTests.java | 10 ++++--- .../web/OidcUserInfoEndpointFilterTests.java | 14 ++++----- 5 files changed, 45 insertions(+), 42 deletions(-) diff --git a/docs/src/docs/asciidoc/protocol-endpoints.adoc b/docs/src/docs/asciidoc/protocol-endpoints.adoc index 8371e278..dd5dd6c4 100644 --- a/docs/src/docs/asciidoc/protocol-endpoints.adoc +++ b/docs/src/docs/asciidoc/protocol-endpoints.adoc @@ -271,7 +271,7 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h `OidcUserInfoEndpointConfigurer` provides the ability to customize the https://openid.net/specs/openid-connect-core-1_0.html#UserInfo[OpenID Connect 1.0 UserInfo endpoint]. It defines extension points that let you customize the https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse[UserInfo response]. -`OidcUserInfoEndpointConfigurer` provides the following configuration option: +`OidcUserInfoEndpointConfigurer` provides the following configuration options: [source,java] ---- @@ -303,8 +303,8 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h <2> `userInfoRequestConverters()`: Sets the `Consumer` providing access to the `List` of default and (optionally) added ``AuthenticationConverter``'s allowing the ability to add, remove, or customize a specific `AuthenticationConverter`. <3> `authenticationProvider()`: Adds an `AuthenticationProvider` (_main processor_) used for authenticating the `OidcUserInfoAuthenticationToken`. <4> `authenticationProviders()`: Sets the `Consumer` providing access to the `List` of default and (optionally) added ``AuthenticationProvider``'s allowing the ability to add, remove, or customize a specific `AuthenticationProvider`. -<5> `revocationResponseHandler()`: The `AuthenticationSuccessHandler` (_post-processor_) used for handling an "`authenticated`" `OidcUserInfoAuthenticationToken` and returning the https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse[UserInfo response]. -<6> `userInfoResponseHandler()`: The `AuthenticationFailureHandler` (_post-processor_) used for handling an `OAuth2AuthenticationException` and returning the https://openid.net/specs/openid-connect-core-1_0.html#UserInfoError[UserInfo Error response]. +<5> `userInfoResponseHandler()`: The `AuthenticationSuccessHandler` (_post-processor_) used for handling an "`authenticated`" `OidcUserInfoAuthenticationToken` and returning the https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse[UserInfo response]. +<6> `errorResponseHandler()`: The `AuthenticationFailureHandler` (_post-processor_) used for handling an `OAuth2AuthenticationException` and returning the https://openid.net/specs/openid-connect-core-1_0.html#UserInfoError[UserInfo Error response]. <7> `userInfoMapper()`: The `Function` used to extract claims from `OidcUserInfoAuthenticationContext` to an instance of `OidcUserInfo`. `OidcUserInfoEndpointConfigurer` configures the `OidcUserInfoEndpointFilter` and registers it with the OAuth2 authorization server `SecurityFilterChain` `@Bean`. @@ -312,9 +312,9 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h `OidcUserInfoEndpointFilter` is configured with the following defaults: -* `*AuthenticationConverter*` -- An internal implementation that obtains the `Authentication` from the `SecurityContext` and wraps the principal in an `OidcUserInfoAuthenticationToken`. +* `*AuthenticationConverter*` -- An internal implementation that obtains the `Authentication` from the `SecurityContext` and creates an `OidcUserInfoAuthenticationToken` with the principal. * `*AuthenticationManager*` -- An `AuthenticationManager` composed of `OidcUserInfoAuthenticationProvider`, which is associated with an internal implementation of `userInfoMapper` that extracts https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims[standard claims] from the https://openid.net/specs/openid-connect-core-1_0.html#IDToken[ID Token] based on the https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims[scopes requested] during authorization. -* `*AuthenticationSuccessHandler*` -- An internal implementation that handles an "`authenticated`" `OidcUserInfoAuthenticationToken` and returns the UserInfo response. +* `*AuthenticationSuccessHandler*` -- An internal implementation that handles an "`authenticated`" `OidcUserInfoAuthenticationToken` and returns the `OidcUserInfo` response. * `*AuthenticationFailureHandler*` -- An internal implementation that uses the `OAuth2Error` associated with the `OAuth2AuthenticationException` and returns the `OAuth2Error` response. [TIP] diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoEndpointConfigurer.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoEndpointConfigurer.java index 52a1a917..26538044 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoEndpointConfigurer.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoEndpointConfigurer.java @@ -61,7 +61,7 @@ import org.springframework.util.Assert; public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configurer { private RequestMatcher requestMatcher; private final List userInfoRequestConverters = new ArrayList<>(); - private Consumer> userInfoRequestConvertersConsumer = (authenticationConverters) -> {}; + private Consumer> userInfoRequestConvertersConsumer = (userInfoRequestConverters) -> {}; private final List authenticationProviders = new ArrayList<>(); private Consumer> authenticationProvidersConsumer = (authenticationProviders) -> {}; private AuthenticationSuccessHandler userInfoResponseHandler; @@ -76,10 +76,10 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur } /** - * Sets the {@link AuthenticationConverter} used when attempting to extract the OAuth2 Access Token from {@link HttpServletRequest} - * to an instance of {@link OidcUserInfoAuthenticationToken} used for authenticating the User Info request. + * Adds an {@link AuthenticationConverter} used when attempting to extract an UserInfo Request from {@link HttpServletRequest} + * to an instance of {@link OidcUserInfoAuthenticationToken} used for authenticating the request. * - * @param userInfoRequestConverter the {@link AuthenticationConverter} used when attempting to extract an OIDC User Info from {@link HttpServletRequest} + * @param userInfoRequestConverter an {@link AuthenticationConverter} used when attempting to extract an UserInfo Request from {@link HttpServletRequest} * @return the {@link OidcUserInfoEndpointConfigurer} for further configuration * @since 0.4.0 */ @@ -106,9 +106,9 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur } /** - * Adds an {@link AuthenticationProvider} used for authenticating a type of {@link OidcUserInfoAuthenticationToken}. + * Adds an {@link AuthenticationProvider} used for authenticating an {@link OidcUserInfoAuthenticationToken}. * - * @param authenticationProvider a {@link AuthenticationProvider} used for authenticating a type of {@link OidcUserInfoAuthenticationToken} + * @param authenticationProvider an {@link AuthenticationProvider} used for authenticating an {@link OidcUserInfoAuthenticationToken} * @return the {@link OidcUserInfoEndpointConfigurer} for further configuration * @since 0.4.0 */ @@ -135,8 +135,8 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur } /** - * Sets the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} and - * returning the {@link OidcUserInfo User Info Response}. + * Sets the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} + * and returning the {@link OidcUserInfo UserInfo Response}. * * @param userInfoResponseHandler the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} * @return the {@link OidcUserInfoEndpointConfigurer} for further configuration @@ -148,8 +148,8 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur } /** - * Sets the {@link AuthenticationFailureHandler} used for handling an {@link OAuth2AuthenticationException} and - * returning the {@link OAuth2Error Error Response}. + * Sets the {@link AuthenticationFailureHandler} used for handling an {@link OAuth2AuthenticationException} + * and returning the {@link OAuth2Error Error Response}. * * @param errorResponseHandler the {@link AuthenticationFailureHandler} used for handling an {@link OAuth2AuthenticationException} * @return the {@link OidcUserInfoEndpointConfigurer} for further configuration @@ -190,12 +190,10 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur new AntPathRequestMatcher(userInfoEndpointUri, HttpMethod.POST.name())); List authenticationProviders = createDefaultAuthenticationProviders(httpSecurity); - if (!this.authenticationProviders.isEmpty()) { authenticationProviders.addAll(0, this.authenticationProviders); } this.authenticationProvidersConsumer.accept(authenticationProviders); - authenticationProviders.forEach(authenticationProvider -> httpSecurity.authenticationProvider(postProcess(authenticationProvider))); } @@ -232,20 +230,23 @@ public final class OidcUserInfoEndpointConfigurer extends AbstractOAuth2Configur private static List createDefaultAuthenticationConverters() { List authenticationConverters = new ArrayList<>(); + authenticationConverters.add( (request) -> { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); return new OidcUserInfoAuthenticationToken(authentication); } ); + return authenticationConverters; } private List createDefaultAuthenticationProviders(HttpSecurity httpSecurity) { List authenticationProviders = new ArrayList<>(); - OidcUserInfoAuthenticationProvider oidcUserInfoAuthenticationProvider = new OidcUserInfoAuthenticationProvider( - OAuth2ConfigurerUtils.getAuthorizationService(httpSecurity)); + OidcUserInfoAuthenticationProvider oidcUserInfoAuthenticationProvider = + new OidcUserInfoAuthenticationProvider( + OAuth2ConfigurerUtils.getAuthorizationService(httpSecurity)); if (this.userInfoMapper != null) { oidcUserInfoAuthenticationProvider.setUserInfoMapper(this.userInfoMapper); } diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilter.java index 1ca5b2c3..355f3b4f 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilter.java @@ -35,6 +35,7 @@ import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.OAuth2ErrorCodes; import org.springframework.security.oauth2.core.http.converter.OAuth2ErrorHttpMessageConverter; import org.springframework.security.oauth2.core.oidc.OidcUserInfo; +import org.springframework.security.oauth2.server.authorization.oidc.authentication.OidcUserInfoAuthenticationProvider; import org.springframework.security.oauth2.server.authorization.oidc.authentication.OidcUserInfoAuthenticationToken; import org.springframework.security.oauth2.server.authorization.oidc.http.converter.OidcUserInfoHttpMessageConverter; import org.springframework.security.web.authentication.AuthenticationConverter; @@ -51,8 +52,10 @@ import org.springframework.web.filter.OncePerRequestFilter; * * @author Ido Salomon * @author Steve Riesenberg + * @author Daniel Garnier-Moiroux * @since 0.2.1 * @see OidcUserInfo + * @see OidcUserInfoAuthenticationProvider * @see 5.3. UserInfo Endpoint */ public final class OidcUserInfoEndpointFilter extends OncePerRequestFilter { @@ -64,14 +67,11 @@ public final class OidcUserInfoEndpointFilter extends OncePerRequestFilter { private final AuthenticationManager authenticationManager; private final RequestMatcher userInfoEndpointMatcher; - - private AuthenticationConverter authenticationConverter = this::createAuthentication; - private final HttpMessageConverter userInfoHttpMessageConverter = new OidcUserInfoHttpMessageConverter(); private final HttpMessageConverter errorHttpResponseConverter = new OAuth2ErrorHttpMessageConverter(); - + private AuthenticationConverter authenticationConverter = this::createAuthentication; private AuthenticationSuccessHandler authenticationSuccessHandler = this::sendUserInfoResponse; private AuthenticationFailureHandler authenticationFailureHandler = this::sendErrorResponse; @@ -111,8 +111,8 @@ public final class OidcUserInfoEndpointFilter extends OncePerRequestFilter { try { Authentication userInfoAuthentication = this.authenticationConverter.convert(request); - OidcUserInfoAuthenticationToken userInfoAuthenticationResult = - (OidcUserInfoAuthenticationToken) this.authenticationManager.authenticate(userInfoAuthentication); + Authentication userInfoAuthenticationResult = + this.authenticationManager.authenticate(userInfoAuthentication); this.authenticationSuccessHandler.onAuthenticationSuccess(request, response, userInfoAuthenticationResult); } catch (OAuth2AuthenticationException ex) { @@ -130,10 +130,10 @@ public final class OidcUserInfoEndpointFilter extends OncePerRequestFilter { } /** - * Sets the {@link AuthenticationConverter} used when attempting to extract the OAuth2 Access Token from {@link HttpServletRequest} - * to an instance of {@link OidcUserInfoAuthenticationToken} used for authenticating the User Info request. + * Sets the {@link AuthenticationConverter} used when attempting to extract an UserInfo Request from {@link HttpServletRequest} + * to an instance of {@link OidcUserInfoAuthenticationToken} used for authenticating the request. * - * @param authenticationConverter the {@link AuthenticationConverter} used when attempting to extract an OIDC User Info from {@link HttpServletRequest} + * @param authenticationConverter the {@link AuthenticationConverter} used when attempting to extract an UserInfo Request from {@link HttpServletRequest} * @since 0.4.0 */ public void setAuthenticationConverter(AuthenticationConverter authenticationConverter) { @@ -142,10 +142,10 @@ public final class OidcUserInfoEndpointFilter extends OncePerRequestFilter { } /** - * Sets the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} and - * returning the {@link OidcUserInfo OIDC User Info}. + * Sets the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} + * and returning the {@link OidcUserInfo UserInfo Response}. * - * @param authenticationSuccessHandler the {@link AuthenticationSuccessHandler} for handling an {@link OidcUserInfoAuthenticationToken} + * @param authenticationSuccessHandler the {@link AuthenticationSuccessHandler} used for handling an {@link OidcUserInfoAuthenticationToken} * @since 0.4.0 */ public void setAuthenticationSuccessHandler(AuthenticationSuccessHandler authenticationSuccessHandler) { diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoTests.java index 07d464bf..b42b7763 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OidcUserInfoTests.java @@ -208,6 +208,7 @@ public class OidcUserInfoTests { .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken.getTokenValue())) .andExpect(status().is2xxSuccessful()); // @formatter:on + verify(userInfoMapper).apply(any()); verify(authenticationConverter).convert(any()); verify(authenticationSuccessHandler).onAuthenticationSuccess(any(), any(), any()); @@ -228,7 +229,7 @@ public class OidcUserInfoTests { } @Test - public void requestWhenUserInfoEndpointCustomizedThenAuthenticationProviderUsed() throws Exception { + public void requestWhenUserInfoEndpointCustomizedWithAuthenticationProviderThenUsed() throws Exception { this.spring.register(CustomUserInfoConfiguration.class).autowire(); OAuth2Authorization authorization = createAuthorization(); @@ -247,6 +248,7 @@ public class OidcUserInfoTests { .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken.getTokenValue())) .andExpect(status().is2xxSuccessful()); // @formatter:on + verify(authenticationSuccessHandler).onAuthenticationSuccess(any(), any(), any()); verify(authenticationProvider).authenticate(any()); verifyNoInteractions(authenticationFailureHandler); @@ -254,8 +256,9 @@ public class OidcUserInfoTests { } @Test - public void requestWhenUserInfoEndpointCustomizedAndErrorThenUsed() throws Exception { + public void requestWhenUserInfoEndpointCustomizedWithAuthenticationFailureHandlerThenUsed() throws Exception { this.spring.register(CustomUserInfoConfiguration.class).autowire(); + when(userInfoMapper.apply(any())).thenReturn(createUserInfo()); doAnswer( invocation -> { @@ -267,13 +270,12 @@ public class OidcUserInfoTests { ).when(authenticationFailureHandler).onAuthenticationFailure(any(), any(), any()); OAuth2AccessToken accessToken = createAuthorization().getAccessToken().getToken(); - - // @formatter:off this.mvc.perform(get(DEFAULT_OIDC_USER_INFO_ENDPOINT_URI) .header(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken.getTokenValue())) .andExpect(status().is4xxClientError()); // @formatter:on + verify(authenticationFailureHandler).onAuthenticationFailure(any(), any(), any()); verifyNoInteractions(authenticationSuccessHandler); verifyNoInteractions(userInfoMapper); diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilterTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilterTests.java index ad7497c6..60e7b939 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilterTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/oidc/web/OidcUserInfoEndpointFilterTests.java @@ -88,21 +88,21 @@ public class OidcUserInfoEndpointFilterTests { } @Test - public void setAuthenticationConverterNullThenThrowIllegalArgumentException() { + public void setAuthenticationConverterWhenNullThenThrowIllegalArgumentException() { assertThatIllegalArgumentException() .isThrownBy(() -> this.filter.setAuthenticationConverter(null)) .withMessage("authenticationConverter cannot be null"); } @Test - public void setAuthenticationSuccessHandlerNullThenThrowIllegalArgumentException() { + public void setAuthenticationSuccessHandlerWhenNullThenThrowIllegalArgumentException() { assertThatIllegalArgumentException() .isThrownBy(() -> this.filter.setAuthenticationSuccessHandler(null)) .withMessage("authenticationSuccessHandler cannot be null"); } @Test - public void setAuthenticationFailureHandlerNullThenThrowIllegalArgumentException() { + public void setAuthenticationFailureHandlerWhenNullThenThrowIllegalArgumentException() { assertThatIllegalArgumentException() .isThrownBy(() -> this.filter.setAuthenticationFailureHandler(null)) .withMessage("authenticationFailureHandler cannot be null"); @@ -201,7 +201,7 @@ public class OidcUserInfoEndpointFilterTests { } @Test - public void doFilterWhenCustomAuthenticationConverterThenUses() throws Exception { + public void doFilterWhenCustomAuthenticationConverterThenUsed() throws Exception { Authentication principal = new TestingAuthenticationToken("principal", "credentials"); OidcUserInfoAuthenticationToken authentication = new OidcUserInfoAuthenticationToken(principal); AuthenticationConverter authenticationConverter = mock(AuthenticationConverter.class); @@ -220,13 +220,14 @@ public class OidcUserInfoEndpointFilterTests { this.filter.doFilter(request, response, filterChain); + verifyNoInteractions(filterChain); verify(authenticationConverter).convert(request); verify(this.authenticationManager).authenticate(authentication); assertUserInfoResponse(response.getContentAsString()); } @Test - public void doFilterWhenCustomAuthenticationSuccessHandlerThenUses() throws Exception { + public void doFilterWhenCustomAuthenticationSuccessHandlerThenUsed() throws Exception { AuthenticationSuccessHandler successHandler = mock(AuthenticationSuccessHandler.class); this.filter.setAuthenticationSuccessHandler(successHandler); @@ -249,7 +250,7 @@ public class OidcUserInfoEndpointFilterTests { } @Test - public void doFilterWhenCustomFailureHandlerThenUses() throws Exception { + public void doFilterWhenCustomAuthenticationFailureHandlerThenUsed() throws Exception { AuthenticationFailureHandler failureHandler = mock(AuthenticationFailureHandler.class); this.filter.setAuthenticationFailureHandler(failureHandler); @@ -269,7 +270,6 @@ public class OidcUserInfoEndpointFilterTests { this.filter.doFilter(request, response, filterChain); verifyNoInteractions(filterChain); - verify(failureHandler).onAuthenticationFailure(request, response, authenticationException); }