From c418306fd994d25f9095d602a7a0cc31b05fe701 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Fri, 26 Nov 2021 04:51:54 -0500 Subject: [PATCH] Polish Authorization Consent Deny Request Issue gh-470 --- ...tionCodeRequestAuthenticationProvider.java | 32 +++++++----- ...odeRequestAuthenticationProviderTests.java | 49 ++++++++++++++++++- 2 files changed, 66 insertions(+), 15 deletions(-) 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 d5d9930d..c6cc64eb 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 @@ -32,6 +32,7 @@ import org.springframework.security.authentication.AnonymousAuthenticationToken; import org.springframework.security.authentication.AuthenticationProvider; import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.GrantedAuthority; import org.springframework.security.crypto.keygen.Base64StringKeyGenerator; import org.springframework.security.crypto.keygen.StringKeyGenerator; import org.springframework.security.oauth2.core.AuthorizationGrantType; @@ -329,19 +330,6 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen Set currentAuthorizedScopes = currentAuthorizationConsent != null ? currentAuthorizationConsent.getScopes() : Collections.emptySet(); - if (authorizedScopes.isEmpty() && currentAuthorizedScopes.isEmpty() && - authorizationCodeRequestAuthentication.getAdditionalParameters().isEmpty()) { - // Authorization consent denied - this.authorizationService.remove(authorization); - throwError(OAuth2ErrorCodes.ACCESS_DENIED, OAuth2ParameterNames.CLIENT_ID, - authorizationCodeRequestAuthentication, registeredClient, authorizationRequest); - } - - if (requestedScopes.contains(OidcScopes.OPENID)) { - // 'openid' scope is auto-approved as it does not require consent - authorizedScopes.add(OidcScopes.OPENID); - } - if (!currentAuthorizedScopes.isEmpty()) { for (String requestedScope : requestedScopes) { if (currentAuthorizedScopes.contains(requestedScope)) { @@ -350,6 +338,11 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen } } + if (!authorizedScopes.isEmpty() && requestedScopes.contains(OidcScopes.OPENID)) { + // 'openid' scope is auto-approved as it does not require consent + authorizedScopes.add(OidcScopes.OPENID); + } + OAuth2AuthorizationConsent.Builder authorizationConsentBuilder; if (currentAuthorizationConsent != null) { authorizationConsentBuilder = OAuth2AuthorizationConsent.from(currentAuthorizationConsent); @@ -371,6 +364,19 @@ public final class OAuth2AuthorizationCodeRequestAuthenticationProvider implemen this.authorizationConsentCustomizer.accept(authorizationConsentAuthenticationContext); } + Set authorities = new HashSet<>(); + authorizationConsentBuilder.authorities(authorities::addAll); + + if (authorities.isEmpty()) { + // Authorization consent denied (or revoked) + if (currentAuthorizationConsent != null) { + this.authorizationConsentService.remove(currentAuthorizationConsent); + } + this.authorizationService.remove(authorization); + throwError(OAuth2ErrorCodes.ACCESS_DENIED, OAuth2ParameterNames.CLIENT_ID, + authorizationCodeRequestAuthentication, registeredClient, authorizationRequest); + } + OAuth2AuthorizationConsent authorizationConsent = authorizationConsentBuilder.build(); if (!authorizationConsent.equals(currentAuthorizationConsent)) { this.authorizationConsentService.save(authorizationConsent); 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 dc1a73bc..fc1fcc16 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 @@ -868,7 +868,52 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void authenticateWhenConsentRequestWithPreviouslyApprovedThenAuthorizationConsentUpdated() { + public void authenticateWhenConsentRequestApproveNoneAndRevokePreviouslyApprovedThenAuthorizationConsentRemoved() { + String previouslyApprovedScope = "message.read"; + String requestedScope = "message.write"; + RegisteredClient registeredClient = TestRegisteredClients.registeredClient() + .scopes(scopes -> { + scopes.clear(); + scopes.add(previouslyApprovedScope); + scopes.add(requestedScope); + }) + .build(); + when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId()))) + .thenReturn(registeredClient); + OAuth2Authorization authorization = TestOAuth2Authorizations.authorization(registeredClient) + .principalName(this.principal.getName()) + .build(); + OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute(OAuth2AuthorizationRequest.class.getName()); + OAuth2AuthorizationCodeRequestAuthenticationToken authentication = + authorizationConsentRequestAuthentication(registeredClient, this.principal) + .scopes(new HashSet<>()) // No scopes approved + .build(); + when(this.authorizationService.findByToken(eq(authentication.getState()), eq(STATE_TOKEN_TYPE))) + .thenReturn(authorization); + OAuth2AuthorizationConsent previousAuthorizationConsent = + OAuth2AuthorizationConsent.withId(authorization.getRegisteredClientId(), authorization.getPrincipalName()) + .scope(previouslyApprovedScope) + .build(); + when(this.authorizationConsentService.findById(eq(authorization.getRegisteredClientId()), eq(authorization.getPrincipalName()))) + .thenReturn(previousAuthorizationConsent); + + // Revoke all (including previously approved) + this.authenticationProvider.setAuthorizationConsentCustomizer((authorizationConsentContext) -> + authorizationConsentContext.getAuthorizationConsent().authorities(Set::clear)); + + assertThatThrownBy(() -> this.authenticationProvider.authenticate(authentication)) + .isInstanceOf(OAuth2AuthorizationCodeRequestAuthenticationException.class) + .satisfies(ex -> + assertAuthenticationException((OAuth2AuthorizationCodeRequestAuthenticationException) ex, + OAuth2ErrorCodes.ACCESS_DENIED, OAuth2ParameterNames.CLIENT_ID, authorizationRequest.getRedirectUri()) + ); + + verify(this.authorizationConsentService).remove(eq(previousAuthorizationConsent)); + verify(this.authorizationService).remove(eq(authorization)); + } + + @Test + public void authenticateWhenConsentRequestApproveSomeAndPreviouslyApprovedThenAuthorizationConsentUpdated() { String previouslyApprovedScope = "message.read"; String requestedScope = "message.write"; String otherPreviouslyApprovedScope = "other.scope"; @@ -923,7 +968,7 @@ public class OAuth2AuthorizationCodeRequestAuthenticationProviderTests { } @Test - public void authenticateWhenConsentRequestApproveNoneButPreviouslyApprovedThenAuthorizationConsentNotUpdated() { + public void authenticateWhenConsentRequestApproveNoneAndPreviouslyApprovedThenAuthorizationConsentNotUpdated() { String previouslyApprovedScope = "message.read"; String requestedScope = "message.write"; RegisteredClient registeredClient = TestRegisteredClients.registeredClient()