From c0446c682c7ad4e54ee4d989164522c98a8e4dd1 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Fri, 12 Apr 2024 14:43:43 -0400 Subject: [PATCH] Polish gh-1552 --- ...ationCodeRequestAuthenticationContext.java | 24 +++-- ...tionCodeRequestAuthenticationProvider.java | 62 +++++------- ...odeRequestAuthenticationProviderTests.java | 99 +++++-------------- 3 files changed, 62 insertions(+), 123 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java index b59b0253..8301596d 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2022 the original author or authors. + * Copyright 2020-2024 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. @@ -19,6 +19,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.function.Consumer; +import java.util.function.Predicate; import org.springframework.lang.Nullable; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; @@ -28,13 +29,14 @@ import org.springframework.util.Assert; /** * An {@link OAuth2AuthenticationContext} that holds an {@link OAuth2AuthorizationCodeRequestAuthenticationToken} and additional information - * and is used when validating the OAuth 2.0 Authorization Request used in the Authorization Code Grant. + * and is used when validating the OAuth 2.0 Authorization Request parameters, as well as, determining if authorization consent is required. * * @author Joe Grandja * @since 0.4.0 * @see OAuth2AuthenticationContext * @see OAuth2AuthorizationCodeRequestAuthenticationToken * @see OAuth2AuthorizationCodeRequestAuthenticationProvider#setAuthenticationValidator(Consumer) + * @see OAuth2AuthorizationCodeRequestAuthenticationProvider#setAuthorizationConsentRequired(Predicate) */ public final class OAuth2AuthorizationCodeRequestAuthenticationContext implements OAuth2AuthenticationContext { private final Map context; @@ -66,22 +68,24 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationContext implement } /** - * Returns the {@link OAuth2AuthorizationRequest oauth2 authorization request}. + * Returns the {@link OAuth2AuthorizationRequest authorization request}. * * @return the {@link OAuth2AuthorizationRequest} + * @since 1.3 */ @Nullable - public OAuth2AuthorizationRequest getOAuth2AuthorizationRequest() { + public OAuth2AuthorizationRequest getAuthorizationRequest() { return get(OAuth2AuthorizationRequest.class); } /** - * Returns the {@link OAuth2AuthorizationConsent oauth2 authorization consent}. + * Returns the {@link OAuth2AuthorizationConsent authorization consent}. * * @return the {@link OAuth2AuthorizationConsent} + * @since 1.3 */ @Nullable - public OAuth2AuthorizationConsent getOAuth2AuthorizationConsent() { + public OAuth2AuthorizationConsent getAuthorizationConsent() { return get(OAuth2AuthorizationConsent.class); } @@ -116,22 +120,22 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationContext implement } /** - * Sets the {@link OAuth2AuthorizationRequest oauth2 authorization request}. + * Sets the {@link OAuth2AuthorizationRequest authorization request}. * * @param authorizationRequest the {@link OAuth2AuthorizationRequest} * @return the {@link Builder} for further configuration - * @since 1.3.0 + * @since 1.3 */ public Builder authorizationRequest(OAuth2AuthorizationRequest authorizationRequest) { return put(OAuth2AuthorizationRequest.class, authorizationRequest); } /** - * Sets the {@link OAuth2AuthorizationConsent oauth2 authorization consent}. + * Sets the {@link OAuth2AuthorizationConsent authorization consent}. * * @param authorizationConsent the {@link OAuth2AuthorizationConsent} * @return the {@link Builder} for further configuration - * @since 1.3.0 + * @since 1.3 */ public Builder authorizationConsent(OAuth2AuthorizationConsent authorizationConsent) { return put(OAuth2AuthorizationConsent.class, authorizationConsent); diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java index ca2197de..dad6e4b8 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProvider.java @@ -81,7 +81,8 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen private OAuth2TokenGenerator authorizationCodeGenerator = new OAuth2AuthorizationCodeGenerator(); private Consumer authenticationValidator = new OAuth2AuthorizationCodeRequestAuthenticationValidator(); - private Predicate requiresAuthorizationConsent; + private Predicate authorizationConsentRequired = + OAuth2AuthorizationCodeRequestAuthenticationProvider::isAuthorizationConsentRequired; /** * Constructs an {@code OAuth2AuthorizationCodeRequestAuthenticationProvider} using the provided parameters. @@ -98,7 +99,6 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen this.registeredClientRepository = registeredClientRepository; this.authorizationService = authorizationService; this.authorizationConsentService = authorizationConsentService; - this.requiresAuthorizationConsent = this::requireAuthorizationConsent; } @Override @@ -117,11 +117,10 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen this.logger.trace("Retrieved registered client"); } - OAuth2AuthorizationCodeRequestAuthenticationContext authenticationContext = + OAuth2AuthorizationCodeRequestAuthenticationContext.Builder authenticationContextBuilder = OAuth2AuthorizationCodeRequestAuthenticationContext.with(authorizationCodeRequestAuthentication) - .registeredClient(registeredClient) - .build(); - this.authenticationValidator.accept(authenticationContext); + .registeredClient(registeredClient); + this.authenticationValidator.accept(authenticationContextBuilder.build()); if (!registeredClient.getAuthorizationGrantTypes().contains(AuthorizationGrantType.AUTHORIZATION_CODE)) { if (this.logger.isDebugEnabled()) { @@ -170,23 +169,15 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen .state(authorizationCodeRequestAuthentication.getState()) .additionalParameters(authorizationCodeRequestAuthentication.getAdditionalParameters()) .build(); + authenticationContextBuilder.authorizationRequest(authorizationRequest); OAuth2AuthorizationConsent currentAuthorizationConsent = this.authorizationConsentService.findById( registeredClient.getId(), principal.getName()); - - OAuth2AuthorizationCodeRequestAuthenticationContext.Builder authenticationContextBuilder = - OAuth2AuthorizationCodeRequestAuthenticationContext.with(authorizationCodeRequestAuthentication) - .registeredClient(registeredClient) - .authorizationRequest(authorizationRequest); - if (currentAuthorizationConsent != null) { authenticationContextBuilder.authorizationConsent(currentAuthorizationConsent); } - OAuth2AuthorizationCodeRequestAuthenticationContext contextWithAuthorizationRequestAndAuthorizationConsent = - authenticationContextBuilder.build(); - - if (requiresAuthorizationConsent.test(contextWithAuthorizationRequestAndAuthorizationConsent)) { + if (this.authorizationConsentRequired.test(authenticationContextBuilder.build())) { String state = DEFAULT_STATE_GENERATOR.generateKey(); OAuth2Authorization authorization = authorizationBuilder(registeredClient, principal, authorizationRequest) .attribute(OAuth2ParameterNames.STATE, state) @@ -280,47 +271,44 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen } /** - * Sets the {@link Predicate} used to determine if authorization consent is required. + * Sets the {@code Predicate} used to determine if authorization consent is required. * *

* The {@link OAuth2AuthorizationCodeRequestAuthenticationContext} gives the predicate access to the {@link OAuth2AuthorizationCodeRequestAuthenticationToken}, * as well as, the following context attributes: - * {@link OAuth2AuthorizationCodeRequestAuthenticationContext#getRegisteredClient()} containing {@link RegisteredClient} used to make the request. - * {@link OAuth2AuthorizationCodeRequestAuthenticationContext#getOAuth2AuthorizationRequest()} containing {@link OAuth2AuthorizationRequest}. - * {@link OAuth2AuthorizationCodeRequestAuthenticationContext#getOAuth2AuthorizationConsent()} containing {@link OAuth2AuthorizationConsent} granted in the request. + *

    + *
  • The {@link RegisteredClient} associated with the authorization request.
    • + *
  • The {@link OAuth2AuthorizationRequest} containing the authorization request parameters.
    • + *
  • The {@link OAuth2AuthorizationConsent} previously granted to the {@link RegisteredClient}, or {@code null} if not available.
    • + *
* - * @param requiresAuthorizationConsent the {@link Predicate} that determines if authorization consent is required. - * @since 1.3.0 + * @param authorizationConsentRequired the {@code Predicate} used to determine if authorization consent is required + * @since 1.3 */ - public void setRequiresAuthorizationConsent(Predicate requiresAuthorizationConsent) { - Assert.notNull(requiresAuthorizationConsent, "requiresAuthorizationConsent cannot be null"); - this.requiresAuthorizationConsent = requiresAuthorizationConsent; + public void setAuthorizationConsentRequired(Predicate authorizationConsentRequired) { + Assert.notNull(authorizationConsentRequired, "authorizationConsentRequired cannot be null"); + this.authorizationConsentRequired = authorizationConsentRequired; } - private boolean requireAuthorizationConsent(OAuth2AuthorizationCodeRequestAuthenticationContext context) { - RegisteredClient registeredClient = context.getRegisteredClient(); - if (!registeredClient.getClientSettings().isRequireAuthorizationConsent()) { + private static boolean isAuthorizationConsentRequired(OAuth2AuthorizationCodeRequestAuthenticationContext authenticationContext) { + if (!authenticationContext.getRegisteredClient().getClientSettings().isRequireAuthorizationConsent()) { return false; } - - OAuth2AuthorizationRequest authorizationRequest = context.getOAuth2AuthorizationRequest(); // 'openid' scope does not require consent - if (authorizationRequest.getScopes().contains(OidcScopes.OPENID) && - authorizationRequest.getScopes().size() == 1) { + if (authenticationContext.getAuthorizationRequest().getScopes().contains(OidcScopes.OPENID) && + authenticationContext.getAuthorizationRequest().getScopes().size() == 1) { return false; } - OAuth2AuthorizationConsent authorizationConsent = context.getOAuth2AuthorizationConsent(); - if (authorizationConsent != null && - authorizationConsent.getScopes().containsAll(authorizationRequest.getScopes())) { + if (authenticationContext.getAuthorizationConsent() != null && + authenticationContext.getAuthorizationConsent().getScopes().containsAll(authenticationContext.getAuthorizationRequest().getScopes())) { return false; } return true; } - private static OAuth2Authorization.Builder authorizationBuilder(RegisteredClient registeredClient, - Authentication principal, + private static OAuth2Authorization.Builder authorizationBuilder(RegisteredClient registeredClient, Authentication principal, OAuth2AuthorizationRequest authorizationRequest) { return OAuth2Authorization.withRegisteredClient(registeredClient) .principalName(principal.getName()) diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProviderTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProviderTests.java index 042e2a6b..98e4c1e2 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProviderTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeRequestAuthenticationProviderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2020-2023 the original author or authors. + * Copyright 2020-2024 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. @@ -73,7 +73,6 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { private OAuth2AuthorizationConsentService authorizationConsentService; private OAuth2AuthorizationCodeRequestAuthenticationProvider authenticationProvider; private TestingAuthenticationToken principal; - private Predicate requiresAuthorizationConsent; @BeforeEach public void setUp() { @@ -132,10 +131,10 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void setRequiresAuthorizationConsentWhenNullThenThrowIllegalArgumentException() { - assertThatThrownBy(() -> this.authenticationProvider.setRequiresAuthorizationConsent(null)) + public void setAuthorizationConsentRequiredWhenNullThenThrowIllegalArgumentException() { + assertThatThrownBy(() -> this.authenticationProvider.setAuthorizationConsentRequired(null)) .isInstanceOf(IllegalArgumentException.class) - .hasMessage("requiresAuthorizationConsent cannot be null"); + .hasMessage("authorizationConsentRequired cannot be null"); } @Test @@ -453,64 +452,12 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void authenticateWhenRequireAuthorizationConsentAndRequiresAuthorizationConsentPredicateTrueThenReturnAuthorizationConsent() { - this.authenticationProvider.setRequiresAuthorizationConsent((authenticationContext) -> true); - - RegisteredClient registeredClient = TestRegisteredClients.registeredClient() - .clientSettings(ClientSettings.builder().requireAuthorizationConsent(true).build()) - .build(); - when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId()))) - .thenReturn(registeredClient); - - String redirectUri = registeredClient.getRedirectUris().toArray(new String[0])[0]; - OAuth2AuthorizationCodeRequestAuthenticationToken authentication = - new OAuth2AuthorizationCodeRequestAuthenticationToken( - AUTHORIZATION_URI, registeredClient.getClientId(), principal, - redirectUri, STATE, registeredClient.getScopes(), null); - - OAuth2AuthorizationConsentAuthenticationToken authenticationResult = - (OAuth2AuthorizationConsentAuthenticationToken) this.authenticationProvider.authenticate(authentication); - - ArgumentCaptor authorizationCaptor = ArgumentCaptor.forClass(OAuth2Authorization.class); - verify(this.authorizationService).save(authorizationCaptor.capture()); - OAuth2Authorization authorization = authorizationCaptor.getValue(); - - OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute(OAuth2AuthorizationRequest.class.getName()); - assertThat(authorizationRequest.getGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE); - assertThat(authorizationRequest.getResponseType()).isEqualTo(OAuth2AuthorizationResponseType.CODE); - assertThat(authorizationRequest.getAuthorizationUri()).isEqualTo(authentication.getAuthorizationUri()); - assertThat(authorizationRequest.getClientId()).isEqualTo(registeredClient.getClientId()); - assertThat(authorizationRequest.getRedirectUri()).isEqualTo(authentication.getRedirectUri()); - assertThat(authorizationRequest.getScopes()).isEqualTo(authentication.getScopes()); - assertThat(authorizationRequest.getState()).isEqualTo(authentication.getState()); - assertThat(authorizationRequest.getAdditionalParameters()).isEqualTo(authentication.getAdditionalParameters()); - - assertThat(authorization.getRegisteredClientId()).isEqualTo(registeredClient.getId()); - assertThat(authorization.getPrincipalName()).isEqualTo(this.principal.getName()); - assertThat(authorization.getAuthorizationGrantType()).isEqualTo(AuthorizationGrantType.AUTHORIZATION_CODE); - assertThat(authorization.getAttribute(Principal.class.getName())).isEqualTo(this.principal); - String state = authorization.getAttribute(OAuth2ParameterNames.STATE); - assertThat(state).isNotNull(); - assertThat(state).isNotEqualTo(authentication.getState()); - - assertThat(authenticationResult.getClientId()).isEqualTo(registeredClient.getClientId()); - assertThat(authenticationResult.getPrincipal()).isEqualTo(this.principal); - assertThat(authenticationResult.getAuthorizationUri()).isEqualTo(authorizationRequest.getAuthorizationUri()); - assertThat(authenticationResult.getScopes()).isEmpty(); - assertThat(authenticationResult.getState()).isEqualTo(state); - assertThat(authenticationResult.isAuthenticated()).isTrue(); - } - - @Test - public void authenticateWhenRequireAuthorizationConsentAndRequiresAuthorizationConsentPredicateFalseThenAuthorizationConsentNotRequired() { - this.authenticationProvider.setRequiresAuthorizationConsent((authenticationContext) -> false); - + public void authenticateWhenRequireAuthorizationConsentAndOnlyOpenidScopeRequestedThenAuthorizationConsentNotRequired() { RegisteredClient registeredClient = TestRegisteredClients.registeredClient() .clientSettings(ClientSettings.builder().requireAuthorizationConsent(true).build()) .scopes(scopes -> { scopes.clear(); scopes.add(OidcScopes.OPENID); - scopes.add(OidcScopes.EMAIL); }) .build(); when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId()))) @@ -529,18 +476,21 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void authenticateWhenRequireAuthorizationConsentAndOnlyOpenidScopeRequestedThenAuthorizationConsentNotRequired() { + public void authenticateWhenRequireAuthorizationConsentAndAllPreviouslyApprovedThenAuthorizationConsentNotRequired() { RegisteredClient registeredClient = TestRegisteredClients.registeredClient() .clientSettings(ClientSettings.builder().requireAuthorizationConsent(true).build()) - .scopes(scopes -> { - scopes.clear(); - scopes.add(OidcScopes.OPENID); - }) .build(); when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId()))) .thenReturn(registeredClient); - String redirectUri = registeredClient.getRedirectUris().toArray(new String[0])[1]; + OAuth2AuthorizationConsent.Builder builder = + OAuth2AuthorizationConsent.withId(registeredClient.getId(), this.principal.getName()); + registeredClient.getScopes().forEach(builder::scope); + OAuth2AuthorizationConsent previousAuthorizationConsent = builder.build(); + when(this.authorizationConsentService.findById(eq(registeredClient.getId()), eq(this.principal.getName()))) + .thenReturn(previousAuthorizationConsent); + + String redirectUri = registeredClient.getRedirectUris().toArray(new String[0])[2]; OAuth2AuthorizationCodeRequestAuthenticationToken authentication = new OAuth2AuthorizationCodeRequestAuthenticationToken( AUTHORIZATION_URI, registeredClient.getClientId(), principal, @@ -553,21 +503,16 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void authenticateWhenRequireAuthorizationConsentAndAllPreviouslyApprovedThenAuthorizationConsentNotRequired() { - RegisteredClient registeredClient = TestRegisteredClients.registeredClient() - .clientSettings(ClientSettings.builder().requireAuthorizationConsent(true).build()) - .build(); + public void authenticateWhenCustomAuthorizationConsentRequiredThenUsed() { + @SuppressWarnings("unchecked") + Predicate authorizationConsentRequired = mock(Predicate.class); + this.authenticationProvider.setAuthorizationConsentRequired(authorizationConsentRequired); + + RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build(); when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId()))) .thenReturn(registeredClient); - OAuth2AuthorizationConsent.Builder builder = - OAuth2AuthorizationConsent.withId(registeredClient.getId(), this.principal.getName()); - registeredClient.getScopes().forEach(builder::scope); - OAuth2AuthorizationConsent previousAuthorizationConsent = builder.build(); - when(this.authorizationConsentService.findById(eq(registeredClient.getId()), eq(this.principal.getName()))) - .thenReturn(previousAuthorizationConsent); - - String redirectUri = registeredClient.getRedirectUris().toArray(new String[0])[2]; + String redirectUri = registeredClient.getRedirectUris().toArray(new String[0])[1]; OAuth2AuthorizationCodeRequestAuthenticationToken authentication = new OAuth2AuthorizationCodeRequestAuthenticationToken( AUTHORIZATION_URI, registeredClient.getClientId(), principal, @@ -577,6 +522,8 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { (OAuth2AuthorizationCodeRequestAuthenticationToken) this.authenticationProvider.authenticate(authentication); assertAuthorizationCodeRequestWithAuthorizationCodeResult(registeredClient, authentication, authenticationResult); + + verify(authorizationConsentRequired).test(any()); } @Test