From 6bae6d913582b1422a643461ff67b1a775a08527 Mon Sep 17 00:00:00 2001 From: Indeewai Wijesiri Date: Mon, 6 Jan 2025 10:44:53 +0530 Subject: [PATCH 1/3] Reducing the token synchronize weight for exisiting tokens --- .../AbstractAuthorizationGrantHandler.java | 133 ++++++++++++++---- 1 file changed, 105 insertions(+), 28 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java index 4a628f0bef..a9021ba58b 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java @@ -172,11 +172,48 @@ public OAuth2AccessTokenRespDTO issue(OAuthTokenReqMessageContext tokReqMsgCtx) "Error while retrieving oauth issuer for the app with clientId: " + consumerKey, e); } - synchronized ((consumerKey + ":" + authorizedUserId + ":" + scope + ":" + tokenBindingReference).intern()) { - AccessTokenDO existingTokenBean = null; + AccessTokenDO existingTokenBean = null; + OAuthAppDO oAuthAppDO = (OAuthAppDO) tokReqMsgCtx.getProperty(OAUTH_APP); + String tokenType = oauthTokenIssuer.getAccessTokenType(); + + if (isHashDisabled) { + existingTokenBean = getExistingToken(tokReqMsgCtx, + getOAuthCacheKey(scope, consumerKey, authorizedUserId, authenticatedIDP, + tokenBindingReference, authorizedOrganization)); + } + + /* + This segment handles access token requests that neither require generating a new token + nor renewing an existing one. Instead, it returns the existing token if it is still valid. + As a result, no synchronization lock is needed for these operations. + Additionally, this segment should strictly avoid any write or update operations. + It was introduced to minimize traffic at the token issuance lock for certain token requests. + */ + if (!(JWT.equalsIgnoreCase(tokenType) && getRenewWithoutRevokingExistingStatus() && + OAuth2ServiceComponentHolder.getJwtRenewWithoutRevokeAllowedGrantTypes() + .contains(tokReqMsgCtx.getOauth2AccessTokenReqDTO().getGrantType()))) { + + if (existingTokenBean != null && !accessTokenRenewedPerRequest(oauthTokenIssuer, tokReqMsgCtx)) { + + String requestGrantType = tokReqMsgCtx.getOauth2AccessTokenReqDTO().getGrantType(); + boolean isConsentRequiredGrant = OIDCClaimUtil.isConsentBasedClaimFilteringApplicable(requestGrantType); + + if (!isConsentRequiredGrant) { + + long expireTime = getAccessTokenExpiryTimeMillis(existingTokenBean); + if (isExistingTokenValid(existingTokenBean, expireTime)) { + if (log.isDebugEnabled()) { + log.debug("Existing token is active for client Id: " + consumerKey + ", user: " + + authorizedUserId + " and scope: " + scope + ". Therefore issuing the same token."); + } + return issueExistingAccessToken(tokReqMsgCtx, scope, expireTime, + existingTokenBean); + } + } + } + } - OAuthAppDO oAuthAppDO = (OAuthAppDO) tokReqMsgCtx.getProperty(OAUTH_APP); - String tokenType = oauthTokenIssuer.getAccessTokenType(); + synchronized ((consumerKey + ":" + authorizedUserId + ":" + scope + ":" + tokenBindingReference).intern()) { /* Check if the token type is JWT and renew without revoking existing tokens is enabled. @@ -197,24 +234,10 @@ If the application does not have a token binding type (i.e., no specific binding } } - if (isHashDisabled) { - existingTokenBean = getExistingToken(tokReqMsgCtx, - getOAuthCacheKey(scope, consumerKey, authorizedUserId, authenticatedIDP, - tokenBindingReference, authorizedOrganization)); - } - if (existingTokenBean != null) { if (log.isDebugEnabled()) { log.debug("Latest access token is found in the OAuthCache for the app: " + consumerKey); } - if (accessTokenRenewedPerRequest(oauthTokenIssuer, tokReqMsgCtx)) { - if (log.isDebugEnabled()) { - log.debug("TokenRenewalPerRequest is enabled. " + - "Proceeding to revoke any existing active tokens and issue new token for client Id: " + - consumerKey + ", user: " + authorizedUserId + " and scope: " + scope + "."); - } - return renewAccessToken(tokReqMsgCtx, scope, consumerKey, existingTokenBean, oauthTokenIssuer); - } long expireTime = getAccessTokenExpiryTimeMillis(existingTokenBean); if (isExistingTokenValid(existingTokenBean, expireTime)) { @@ -222,7 +245,7 @@ If the application does not have a token binding type (i.e., no specific binding log.debug("Existing token is active for client Id: " + consumerKey + ", user: " + authorizedUserId + " and scope: " + scope + ". Therefore issuing the same token."); } - return issueExistingAccessToken(tokReqMsgCtx, scope, expireTime, existingTokenBean); + return issueExistingAccessTokenWithConsent(tokReqMsgCtx, scope, expireTime, existingTokenBean); } } @@ -438,16 +461,73 @@ private OAuth2AccessTokenRespDTO renewAccessToken(OAuthTokenReqMessageContext to oauthTokenIssuer); } - private OAuth2AccessTokenRespDTO issueExistingAccessToken(OAuthTokenReqMessageContext tokReqMsgCtx, String scope, - long expireTime, AccessTokenDO existingTokenBean) + /** + * Issues an existing access token by preparing the necessary details in the token request context + * and creating a response with the existing token. This method avoids generating a new token + * when a valid token already exists and can be reused. + * + * @param tokReqMsgCtx The OAuth token request message context containing the details of the current token + * request. + * @param scope The scope associated with the token request. + * @param expireTime The expiration time of the existing token. + * @param existingTokenBean The existing access token object to be reused for the current request. + * @return An OAuth2AccessTokenRespDTO object containing the response details of the + * reused token. + * @throws IdentityOAuth2Exception If an error occurs while setting details to the message context + * or creating the token response. + */ + private OAuth2AccessTokenRespDTO issueExistingAccessToken(OAuthTokenReqMessageContext tokReqMsgCtx, + String scope, + long expireTime, + AccessTokenDO existingTokenBean) throws IdentityOAuth2Exception { tokReqMsgCtx.addProperty(EXISTING_TOKEN_ISSUED, true); + setDetailsToMessageContext(tokReqMsgCtx, existingTokenBean); + return createResponseWithTokenBean(existingTokenBean, expireTime, scope); + } + + /** + * Issues an existing access token while ensuring that it complies with the consent requirements + * of the current grant type. If the existing token was originally issued for a grant type that did + * not require consent but the current grant type does, the token's consent status is updated accordingly. + * This method avoids unnecessary token generation by reusing an existing token whenever possible, + * while updating its properties to meet the requirements of the current request. + * + * @param tokReqMsgCtx The OAuth token request message context containing the details of the current token + * request. + * @param scope The scope associated with the token request. + * @param expireTime The expiration time of the existing token. + * @param existingTokenBean The existing access token object to be evaluated, updated, and reused if valid. + * @return An {@link OAuth2AccessTokenRespDTO} object containing the response details of the + * issued token. + * @throws IdentityOAuth2Exception If an error occurs while handling consent or issuing the existing access token. + */ + private OAuth2AccessTokenRespDTO issueExistingAccessTokenWithConsent(OAuthTokenReqMessageContext tokReqMsgCtx, + String scope, + long expireTime, + AccessTokenDO existingTokenBean) + throws IdentityOAuth2Exception { + + handleConsent(tokReqMsgCtx, existingTokenBean); + return issueExistingAccessToken(tokReqMsgCtx, scope, expireTime, existingTokenBean); + } + + /** + * Handles consent updates for an existing access token when issuing it for a new grant type. + * If the existing access token was originally issued for a grant type that does not require consent, + * but the current grant type does require consent, this method updates the existing token to + * reflect that it is now consented. + * + * @param tokReqMsgCtx The OAuth token request message context containing the details + * of the current token request. + * @param existingTokenBean The existing access token to be evaluated and updated as needed. + * @throws IdentityOAuth2Exception If an error occurs while updating the consent status of the token. + */ + private void handleConsent(OAuthTokenReqMessageContext tokReqMsgCtx, AccessTokenDO existingTokenBean) + throws IdentityOAuth2Exception { + String requestGrantType = tokReqMsgCtx.getOauth2AccessTokenReqDTO().getGrantType(); - /* When issuing the existing access token, that access token may be originated from a different grant - type. The origin grant type can be a consent required one. If the existing token is issued previously - for a consent not required grant and the current grant requires consent, we update the existing token as - a consented token. */ boolean isConsentRequiredGrant = OIDCClaimUtil. isConsentBasedClaimFilteringApplicable(requestGrantType); if (isConsentRequiredGrant && !existingTokenBean.isConsentedToken()) { @@ -455,9 +535,6 @@ private OAuth2AccessTokenRespDTO issueExistingAccessToken(OAuthTokenReqMessageCo OAuthTokenPersistenceFactory.getInstance().getAccessTokenDAO().updateTokenIsConsented( existingTokenBean.getTokenId(), true); } - - setDetailsToMessageContext(tokReqMsgCtx, existingTokenBean); - return createResponseWithTokenBean(existingTokenBean, expireTime, scope); } private OAuth2AccessTokenRespDTO generateNewAccessToken(OAuthTokenReqMessageContext tokReqMsgCtx, String scope, From 5606e85388e5c660b7a704335a1f19b65722c680 Mon Sep 17 00:00:00 2001 From: Indeewai Wijesiri Date: Tue, 7 Jan 2025 13:24:00 +0530 Subject: [PATCH 2/3] Modifying the logic order --- .../grant/AbstractAuthorizationGrantHandler.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java index a9021ba58b..7db753f677 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java @@ -176,12 +176,6 @@ public OAuth2AccessTokenRespDTO issue(OAuthTokenReqMessageContext tokReqMsgCtx) OAuthAppDO oAuthAppDO = (OAuthAppDO) tokReqMsgCtx.getProperty(OAUTH_APP); String tokenType = oauthTokenIssuer.getAccessTokenType(); - if (isHashDisabled) { - existingTokenBean = getExistingToken(tokReqMsgCtx, - getOAuthCacheKey(scope, consumerKey, authorizedUserId, authenticatedIDP, - tokenBindingReference, authorizedOrganization)); - } - /* This segment handles access token requests that neither require generating a new token nor renewing an existing one. Instead, it returns the existing token if it is still valid. @@ -193,6 +187,12 @@ public OAuth2AccessTokenRespDTO issue(OAuthTokenReqMessageContext tokReqMsgCtx) OAuth2ServiceComponentHolder.getJwtRenewWithoutRevokeAllowedGrantTypes() .contains(tokReqMsgCtx.getOauth2AccessTokenReqDTO().getGrantType()))) { + if (isHashDisabled) { + existingTokenBean = getExistingToken(tokReqMsgCtx, + getOAuthCacheKey(scope, consumerKey, authorizedUserId, authenticatedIDP, + tokenBindingReference, authorizedOrganization)); + } + if (existingTokenBean != null && !accessTokenRenewedPerRequest(oauthTokenIssuer, tokReqMsgCtx)) { String requestGrantType = tokReqMsgCtx.getOauth2AccessTokenReqDTO().getGrantType(); From 7c18cd0fbf87d2fc2b49c5f4acf21ff0e8594a33 Mon Sep 17 00:00:00 2001 From: Indeewai Wijesiri Date: Wed, 8 Jan 2025 13:40:52 +0530 Subject: [PATCH 3/3] Unit test coverage --- ...AbstractAuthorizationGrantHandlerTest.java | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandlerTest.java b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandlerTest.java index 7661ddf35f..d661aed7f1 100644 --- a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandlerTest.java +++ b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandlerTest.java @@ -40,6 +40,9 @@ import org.wso2.carbon.identity.core.util.IdentityUtil; import org.wso2.carbon.identity.event.services.IdentityEventService; import org.wso2.carbon.identity.oauth.IdentityOAuthAdminException; +import org.wso2.carbon.identity.oauth.cache.CacheEntry; +import org.wso2.carbon.identity.oauth.cache.OAuthCache; +import org.wso2.carbon.identity.oauth.cache.OAuthCacheKey; import org.wso2.carbon.identity.oauth.common.GrantType; import org.wso2.carbon.identity.oauth.common.OAuthConstants; import org.wso2.carbon.identity.oauth.config.OAuthCallbackHandlerMetaData; @@ -48,7 +51,9 @@ import org.wso2.carbon.identity.oauth.dao.OAuthAppDO; import org.wso2.carbon.identity.oauth.internal.OAuthComponentServiceHolder; import org.wso2.carbon.identity.oauth2.IdentityOAuth2Exception; +import org.wso2.carbon.identity.oauth2.OAuth2Constants; import org.wso2.carbon.identity.oauth2.TestConstants; +import org.wso2.carbon.identity.oauth2.dao.AccessTokenDAO; import org.wso2.carbon.identity.oauth2.dao.AccessTokenDAOImpl; import org.wso2.carbon.identity.oauth2.dao.OAuthTokenPersistenceFactory; import org.wso2.carbon.identity.oauth2.dto.OAuth2AccessTokenReqDTO; @@ -58,6 +63,7 @@ import org.wso2.carbon.identity.oauth2.token.JWTTokenIssuer; import org.wso2.carbon.identity.oauth2.token.OAuthTokenReqMessageContext; import org.wso2.carbon.identity.oauth2.token.OauthTokenIssuer; +import org.wso2.carbon.identity.oauth2.token.OauthTokenIssuerImpl; import org.wso2.carbon.identity.oauth2.token.bindings.TokenBinding; import org.wso2.carbon.identity.oauth2.util.OAuth2Util; import org.wso2.carbon.identity.oauth2.validators.OAuth2ScopeHandler; @@ -72,6 +78,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyMap; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.verify; @@ -455,6 +463,78 @@ public void testIsAuthorizedClient(Object tokenRequestMsgCtx, boolean debugEnabl assertEquals(handler.isAuthorizedClient(tokReqMsgCtx), expectedValue); } + @Test(dataProvider = "IssueExistingAccessTokensWithoutConsent") + public void testIssueExistingAccessTokensWithoutConsent(boolean idpIdColumnEnabled) throws Exception { + + OAuth2ServiceComponentHolder.setIDPIdColumnEnabled(idpIdColumnEnabled); + + OAuth2AccessTokenReqDTO oAuth2AccessTokenReqDTO = new OAuth2AccessTokenReqDTO(); + oAuth2AccessTokenReqDTO.setClientId(clientId); + oAuth2AccessTokenReqDTO.setGrantType("client_credential"); + + AccessTokenDO existingAccessTokenDO = new AccessTokenDO(); + existingAccessTokenDO.setAuthzUser(authenticatedUser); + existingAccessTokenDO.setScope(new String[]{"scope1", "scope2"}); + existingAccessTokenDO.setAccessToken("existingAccessToken"); + existingAccessTokenDO.setTokenState(TOKEN_STATE_ACTIVE); + existingAccessTokenDO.setConsumerKey(clientId); + + OAuthTokenReqMessageContext tokReqMsgCtx = new OAuthTokenReqMessageContext(oAuth2AccessTokenReqDTO); + authenticatedUser.setAccessingOrganization("orgabc"); + tokReqMsgCtx.setAuthorizedUser(authenticatedUser); + tokReqMsgCtx.setScope(new String[]{"scope1", "scope2"}); + + try (MockedStatic identityUtil = mockStatic(IdentityUtil.class); + MockedStatic oauth2Util = mockStatic(OAuth2Util.class); + MockedStatic factoryMock = mockStatic(OAuthTokenPersistenceFactory.class)) { + + OAuthComponentServiceHolder.getInstance().setActionExecutorService(mockActionExecutionService); + + identityUtil.when(() -> IdentityUtil.getProperty(anyString())).thenReturn(Boolean.TRUE.toString()); + identityUtil.when(() -> IdentityUtil.isTokenLoggable(anyString())).thenCallRealMethod(); + + OauthTokenIssuer oauthTokenIssuer = mock(OauthTokenIssuerImpl.class); + when(oauthTokenIssuer.getAccessTokenType()).thenReturn(OAuth2Constants.TokenTypes.OPAQUE); + + oauth2Util.when(() -> OAuth2Util.getOAuthTokenIssuerForOAuthApp(clientId)).thenReturn(oauthTokenIssuer); + oauth2Util.when(() -> OAuth2Util.getAppInformationByClientId(clientId)).thenReturn(oAuthAppDO); + oauth2Util.when(() -> OAuth2Util.isOrganizationValidAndActive(anyString())).thenReturn(true); + oauth2Util.when(() -> OAuth2Util.buildScopeString(any())).thenCallRealMethod(); + oauth2Util.when(() -> OAuth2Util.getTokenPartitionedSqlByUserStore(anyString(), any())) + .thenCallRealMethod(); + oauth2Util.when(() -> OAuth2Util.getTokenExpireTimeMillis(eq(existingAccessTokenDO), eq(false))) + .thenReturn(3600L); + + OAuthCache mockOAuthCache = mock(OAuthCache.class); + handler.oauthCache = mockOAuthCache; + doNothing().when(mockOAuthCache).addToCache(any(OAuthCacheKey.class), any(CacheEntry.class)); + + OAuthTokenPersistenceFactory mockFactory = mock(OAuthTokenPersistenceFactory.class); + AccessTokenDAO mockAccessTokenDAO = mock(AccessTokenDAO.class); + factoryMock.when(OAuthTokenPersistenceFactory::getInstance).thenReturn(mockFactory); + when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO); + when(mockAccessTokenDAO.getLatestAccessToken( + eq(clientId), + eq(authenticatedUser), + eq(null), + eq("scope1 scope2"), + eq("NONE"), + eq(false))).thenReturn(existingAccessTokenDO); + + OAuth2AccessTokenRespDTO tokenRespDTO = handler.issue(tokReqMsgCtx); + assertNotNull(tokenRespDTO.getAccessToken()); + assertEquals(tokenRespDTO.getAccessToken(), existingAccessTokenDO.getAccessToken()); + } + } + + @DataProvider(name = "IssueExistingAccessTokensWithoutConsent") + public Object[][] issueExistingAccessTokensWithoutConsent() { + + return new Object[][]{ + {true}, {false} + }; + } + private static class MockAuthzGrantHandler extends AbstractAuthorizationGrantHandler { }