From 1b676f192ccd89a5e5770bea88568b2e05a31e05 Mon Sep 17 00:00:00 2001 From: thumimku Date: Sat, 18 Jan 2025 18:40:31 +0530 Subject: [PATCH] fix hybrid flow --- .../config/OAuthServerConfiguration.java | 13 +++ .../identity/oauth/dao/OAuthAppDAO.java | 39 ++++++- .../AbstractResponseTypeRequestValidator.java | 32 ++--- .../identity/oauth/dao/OAuthAppDAOTest.java | 110 ++++++++++++++++++ .../identity/oauth2/OAuth2ServiceTest.java | 89 +++++++++++--- 5 files changed, 249 insertions(+), 34 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java index 6ccffbb41ee..27f74dff9f6 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java @@ -343,6 +343,9 @@ public class OAuthServerConfiguration { private List supportedTokenEndpointSigningAlgorithms = new ArrayList<>(); private Boolean roleBasedScopeIssuerEnabledConfig = false; private String scopeMetadataExtensionImpl = null; + private static final List HYBRID_RESPONSE_TYPES = Arrays.asList("code token", + "code id_token", "code id_token token"); + private List configuredHybridResponseTypes = new ArrayList<>(); private final List restrictedQueryParameters = new ArrayList<>(); @@ -973,6 +976,11 @@ public boolean useRetainOldAccessTokens() { return Boolean.TRUE.toString().equalsIgnoreCase(retainOldAccessTokens); } + public List getConfiguredHybridResponseTypes() { + + return configuredHybridResponseTypes; + } + public boolean isTokenCleanupEnabled() { return Boolean.TRUE.toString().equalsIgnoreCase(tokenCleanupFeatureEnable); @@ -2966,6 +2974,11 @@ private void parseSupportedResponseTypesConfig(OMElement oauthConfigElem) { } if (responseTypeName != null && !"".equals(responseTypeName) && responseTypeHandlerImplClass != null && !"".equals(responseTypeHandlerImplClass)) { + + // check for the configured hybrid response type + if (HYBRID_RESPONSE_TYPES.contains(responseTypeName)) { + configuredHybridResponseTypes.add(responseTypeName); + } supportedResponseTypeClassNames.put(responseTypeName, responseTypeHandlerImplClass); OMElement responseTypeValidatorClassNameElement = supportedResponseTypeElement .getFirstChildWithName( diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java index 43656059ba3..2b608c2ceaa 100755 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAO.java @@ -1992,14 +1992,41 @@ private void setSpOIDCProperties(Map> spOIDCProperties, OAu oauthApp.setSubjectTokenExpiryTime(Integer.parseInt(subjectTokenExpiryTime)); } - boolean hybridFlowEnabled = Boolean.parseBoolean(getFirstPropertyValue(spOIDCProperties, - HYBRID_FLOW_ENABLED)); - oauthApp.setHybridFlowEnabled(hybridFlowEnabled); + String hybridFlowEnabledProperty = getFirstPropertyValue(spOIDCProperties, HYBRID_FLOW_ENABLED); - String hybridFlowResponseType = getFirstPropertyValue(spOIDCProperties, - OAuthConstants.OIDCConfigProperties.HYBRID_FLOW_RESPONSE_TYPE); + // Check if the application has the `hybridFlowEnabled` property configured + if (hybridFlowEnabledProperty == null) { + // No hybridFlowEnabled property; use server's default behavior for hybrid flow. + if (LOG.isDebugEnabled()) { + LOG.debug(String.format("The application with consumer key %s does not have the 'hybridFlowEnabled' " + + "property configured. Using server default behavior to enable hybrid flow with all " + + "configured response types.", oauthApp.getOauthConsumerKey())); + } - oauthApp.setHybridFlowResponseType(hybridFlowResponseType); + List configuredHybridResponseTypes = OAuthServerConfiguration.getInstance() + .getConfiguredHybridResponseTypes(); + + if (configuredHybridResponseTypes.isEmpty()) { + // No configured hybrid response types; hybrid flow is disabled + oauthApp.setHybridFlowEnabled(false); + oauthApp.setHybridFlowResponseType(null); + } else { + // Enable hybrid flow with all configured response types + oauthApp.setHybridFlowEnabled(true); + String hybridFlowResponseType = String.join(", ", configuredHybridResponseTypes); + oauthApp.setHybridFlowResponseType(hybridFlowResponseType); + } + } else { + // Hybrid flow property is defined; parse and configure + boolean hybridFlowEnabled = Boolean.parseBoolean(hybridFlowEnabledProperty); + oauthApp.setHybridFlowEnabled(hybridFlowEnabled); + + String hybridFlowResponseType = getFirstPropertyValue(spOIDCProperties, + OAuthConstants.OIDCConfigProperties.HYBRID_FLOW_RESPONSE_TYPE); + + // Configure the hybrid flow response type (null if not explicitly set) + oauthApp.setHybridFlowResponseType(hybridFlowResponseType); + } } private String getFirstPropertyValue(Map> propertyMap, String key) { diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/validators/AbstractResponseTypeRequestValidator.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/validators/AbstractResponseTypeRequestValidator.java index d8a0f9a578e..cd5637f8f93 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/validators/AbstractResponseTypeRequestValidator.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/authz/validators/AbstractResponseTypeRequestValidator.java @@ -38,9 +38,7 @@ import java.util.ArrayList; import java.util.Arrays; -import java.util.HashSet; import java.util.List; -import java.util.Set; import javax.servlet.http.HttpServletRequest; @@ -208,6 +206,7 @@ private void validateHybridFlowRequest(HttpServletRequest request, OAuthAppDO ap String responseType = request.getParameter(OAuthConstants.OAuth20Params.RESPONSE_TYPE); boolean hybridFlowEnabled = appDO.isHybridFlowEnabled(); + // Check if the response type is a hybrid response type. if (OAuth2Util.isHybridResponseType(responseType)) { if (!hybridFlowEnabled) { if (log.isDebugEnabled()) { @@ -217,11 +216,14 @@ private void validateHybridFlowRequest(HttpServletRequest request, OAuthAppDO ap throw new InvalidOAuthClientException("Hybrid flow is not enabled for the application."); } - String configuredHybridFlowResponseType = appDO.getHybridFlowResponseType(); - if (!isRequestedResponseTypeConfigured(responseType, configuredHybridFlowResponseType)) { + // Retrieve the list of allowed hybrid response types + List hybridResponseTypeList = getHybridResponseType(appDO); + + // Validate the requested response type + if (!hybridResponseTypeList.contains(responseType)) { if (log.isDebugEnabled()) { - log.debug("Requested response type " + responseType + " is not configured for the hybrid flow " + - "for the application with client ID: " + appDO.getOauthConsumerKey()); + log.debug(String.format("Requested response type '%s' is not configured for the hybrid " + + "flow for the application with client ID: %s", responseType, appDO.getOauthConsumerKey())); } throw new InvalidOAuthClientException("Requested response type " + responseType + @@ -230,16 +232,18 @@ private void validateHybridFlowRequest(HttpServletRequest request, OAuthAppDO ap } } - private boolean isRequestedResponseTypeConfigured(String responseType, String configuredHybridFlowResponseType) { + private List getHybridResponseType(OAuthAppDO appDO) throws InvalidOAuthClientException { - Set configuredResponseTypes = new HashSet<>(Arrays.asList(configuredHybridFlowResponseType.split(" "))); - String[] requestedResponseTypes = responseType.split(" "); - for (String requestedType : requestedResponseTypes) { - if (!configuredResponseTypes.contains(requestedType)) { - return false; - } + String configuredHybridFlowResponseType = appDO.getHybridFlowResponseType(); + + // Validate if the configured response type string is null or empty + if (configuredHybridFlowResponseType == null || configuredHybridFlowResponseType.trim().isEmpty()) { + throw new InvalidOAuthClientException(String.format("No hybrid flow response types are configured " + + "for the application with client ID: %s.", appDO.getOauthConsumerKey())); } - return true; + + // Split the configured hybrid response types into a list + return Arrays.asList(configuredHybridFlowResponseType.split(",")); } private OAuth2ClientValidationResponseDTO validateCallBack(String clientId, String callbackURI, OAuthAppDO appDO) { diff --git a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java index 43313c7c55e..d6d830ff931 100644 --- a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java +++ b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/dao/OAuthAppDAOTest.java @@ -56,6 +56,7 @@ import java.sql.SQLException; import java.sql.Timestamp; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.UUID; @@ -1164,6 +1165,115 @@ public void testGetAppInformationByAppNameWithAppResidentOrgIdAndWithException() } } + @DataProvider(name = "testGetAppInformationForHybridFlowData") + public Object[][] testGetAppInformationForHybridFlowData() { + + return new Object[][]{ + {true, "code token", true, "code token"}, + {true, "code token, code id_token", true, "code token, code id_token"}, + {false, "code id_token", false, "code id_token"}, + }; + } + + @Test(dataProvider = "testGetAppInformationForHybridFlowData") + public void testGetAppInformationForHybridFlowTest + (boolean hybridFlowEnabled, String hybridFlowResponseType, + boolean hybridFlowEnabledExpected, String hybridFlowResponseTypeExpected) throws Exception { + + try (MockedStatic oAuthServerConfiguration = mockStatic( + OAuthServerConfiguration.class); + MockedStatic identityTenantUtil = mockStatic(IdentityTenantUtil.class); + MockedStatic identityUtil = mockStatic(IdentityUtil.class); + MockedStatic identityDatabaseUtil = mockStatic(IdentityDatabaseUtil.class); + MockedStatic oAuthComponentServiceHolder = + mockStatic(OAuthComponentServiceHolder.class)) { + setupMocksForTest(oAuthServerConfiguration, identityTenantUtil, identityUtil); + mockUserstore(oAuthComponentServiceHolder); + try (Connection connection = getConnection(DB_NAME)) { + mockIdentityUtilDataBaseConnection(connection, identityDatabaseUtil); + OAuthAppDO defaultOAuthAppDO = getDefaultOAuthAppDO(); + + // Add Impersonation OIDC properties. + defaultOAuthAppDO.setHybridFlowEnabled(hybridFlowEnabled); + defaultOAuthAppDO.setHybridFlowResponseType(hybridFlowResponseType); + addOAuthApplication(defaultOAuthAppDO, TENANT_ID); + + OAuthAppDAO appDAO = new OAuthAppDAO(); + OAuthAppDO oAuthAppDO = appDAO.getAppInformation(CONSUMER_KEY); + assertNotNull(oAuthAppDO); + assertEquals(oAuthAppDO.isHybridFlowEnabled(), hybridFlowEnabledExpected); + assertEquals(oAuthAppDO.getHybridFlowResponseType(), hybridFlowResponseTypeExpected); + appDAO.removeConsumerApplication(CONSUMER_KEY); + } + } finally { + resetPrivilegedCarbonContext(); + } + } + + @DataProvider(name = "testGetMigratedAppInformationForHybridFlowData") + public Object[][] testGetMigratedAppInformationForHybridFlowData() { + + return new Object[][]{ + {Arrays.asList("code token", "code id_token", "code id_token token"), true, + "code token, code id_token, code id_token token"}, + {Arrays.asList("code id_token"), true, "code id_token"}, + {Arrays.asList(), false, null}, + }; + } + + @Test(dataProvider = "testGetMigratedAppInformationForHybridFlowData") + public void testGetMigratedAppInformationForHybridFlowTest(List configuredHybridFlowResponseType, + boolean hybridFlowEnabledExpected, + String hybridFlowResponseTypeExpected) throws Exception { + + try (MockedStatic oAuthServerConfiguration = mockStatic( + OAuthServerConfiguration.class); + MockedStatic identityTenantUtil = mockStatic(IdentityTenantUtil.class); + MockedStatic identityUtil = mockStatic(IdentityUtil.class); + MockedStatic identityDatabaseUtil = mockStatic(IdentityDatabaseUtil.class); + MockedStatic oAuthComponentServiceHolder = + mockStatic(OAuthComponentServiceHolder.class)) { + mockUserstore(oAuthComponentServiceHolder); + final String deleteHybridFlowProperty = + createDeleteQuery(CONSUMER_KEY, "hybridFlowEnabled"); + final String deleteHybridFlowResponseTypeProperty = + createDeleteQuery(CONSUMER_KEY, "hybridFlowResponseType"); + setupMocksForTest(oAuthServerConfiguration, identityTenantUtil, identityUtil); + when(mockedServerConfig.getConfiguredHybridResponseTypes()).thenReturn(configuredHybridFlowResponseType); + try (Connection connection = getConnection(DB_NAME)) { + mockIdentityUtilDataBaseConnection(connection, identityDatabaseUtil); + OAuthAppDO defaultOAuthAppDO = getDefaultOAuthAppDO(); + + addOAuthApplication(defaultOAuthAppDO, TENANT_ID); + + executeDeleteQuery(connection, deleteHybridFlowProperty); + executeDeleteQuery(connection, deleteHybridFlowResponseTypeProperty); + + OAuthAppDAO appDAO = new OAuthAppDAO(); + OAuthAppDO oAuthAppDO = appDAO.getAppInformation(CONSUMER_KEY); + assertNotNull(oAuthAppDO); + assertEquals(oAuthAppDO.isHybridFlowEnabled(), hybridFlowEnabledExpected); + assertEquals(oAuthAppDO.getHybridFlowResponseType(), hybridFlowResponseTypeExpected); + appDAO.removeConsumerApplication(CONSUMER_KEY); + } + } finally { + resetPrivilegedCarbonContext(); + } + } + + private String createDeleteQuery(String consumerKey, String propertyKey) { + + return "DELETE FROM IDN_OIDC_PROPERTY WHERE CONSUMER_KEY='" + + consumerKey + "' AND PROPERTY_KEY ='" + propertyKey + "'"; + } + + private void executeDeleteQuery(Connection connection, String query) throws SQLException { + + try (PreparedStatement stmt = connection.prepareStatement(query)) { + stmt.execute(); + } + } + private OAuthAppDO getDefaultOAuthAppDO() { AuthenticatedUser authenticatedUser = new AuthenticatedUser(); authenticatedUser.setUserName(USER_NAME); diff --git a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/OAuth2ServiceTest.java b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/OAuth2ServiceTest.java index b569091c65f..c6be5e05e67 100644 --- a/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/OAuth2ServiceTest.java +++ b/components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/OAuth2ServiceTest.java @@ -37,6 +37,7 @@ import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils; import org.wso2.carbon.identity.common.testng.WithCarbonHome; import org.wso2.carbon.identity.core.util.IdentityTenantUtil; +import org.wso2.carbon.identity.core.util.IdentityUtil; import org.wso2.carbon.identity.oauth.OAuthUtil; import org.wso2.carbon.identity.oauth.cache.AppInfoCache; import org.wso2.carbon.identity.oauth.cache.OAuthCache; @@ -345,24 +346,84 @@ public void testValidateClientInfoWithEmptyGrantTypes() throws Exception { } } - @Test(dataProvider = "ValidateClientInfoDataProvider") - public void testValidateHybridFlowValidRequest(String clientId, String grantType, - String callbackUrl, String tenantDomain, - int tenantId, String callbackURI) throws Exception { + @DataProvider(name = "HybridFlowValidationDataProvider") + public Object[][] hybridFlowValidationDataProvider() { + + return new Object[][]{ + // Valid hybrid flow + {true, "code token", "code token", + true, null, null}, + + // Valid hybrid flow + {true, "code token,code id_token", "code token", + true, null, null}, + + // Hybrid flow disabled + {false, "code token", "code token", + false, OAuth2ErrorCodes.INVALID_CLIENT, "Hybrid flow is not enabled for the application."}, + + // Hybrid flow enabled but configured response type is null. + {true, null, "code id_token token", + false, OAuth2ErrorCodes.INVALID_CLIENT, "No hybrid flow response types are configured " + + "for the application with client ID: testClient."}, + + // Hybrid flow enabled but configured response type is empty. + {true, "", "code id_token token", + false, OAuth2ErrorCodes.INVALID_CLIENT, "No hybrid flow response types are configured " + + "for the application with client ID: testClient."}, + + // Hybrid flow enabled but configured response type is different than requested. + {true, "code id_token token", "code token", + false, OAuth2ErrorCodes.INVALID_CLIENT, "Requested response type code token is not " + + "configured for the hybrid flow for the application."}, + + {true, "code id_token token, code id_token", "code token", + false, OAuth2ErrorCodes.INVALID_CLIENT, "Requested response type code token is not " + + "configured for the hybrid flow for the application."} + }; + } + + @Test(dataProvider = "HybridFlowValidationDataProvider") + public void testValidateHybridFlow(boolean hybridFlowEnabled, String hybridFlowResponseType, String responseType, + boolean expectedValidClient, String expectedErrorCode, + String expectedErrorMsg) throws Exception { try (MockedStatic oAuth2Util = mockStatic(OAuth2Util.class); - MockedStatic identityTenantUtil = mockStatic(IdentityTenantUtil.class)) { - OAuthAppDO oAuthAppDO = getOAuthAppDO(clientId, grantType, callbackUrl, tenantDomain, - tenantId, identityTenantUtil, oAuth2Util); - oAuthAppDO.setHybridFlowEnabled(true); - oAuthAppDO.setHybridFlowResponseType("code token"); + MockedStatic identityTenantUtil = mockStatic(IdentityTenantUtil.class); + MockedStatic identityUtil = mockStatic(IdentityUtil.class)) { + + String clientId = "testClient"; + String callbackUrl = "dummyCallBackUrl"; + + // Set up the mocked OAuthAppDO + OAuthAppDO oAuthAppDO = getOAuthAppDO(clientId, "dummyGrantType", + callbackUrl, "carbon.super", -1234, identityTenantUtil, oAuth2Util); + oAuthAppDO.setHybridFlowEnabled(hybridFlowEnabled); + oAuthAppDO.setHybridFlowResponseType(hybridFlowResponseType); + oAuthAppDO.setOauthConsumerKey(clientId); + + // Mock static utility methods + identityUtil.when(() -> IdentityUtil.getProperty(OAuthConstants + .ENABLE_HYBRID_FLOW_APPLICATION_LEVEL_VALIDATION)).thenReturn("true"); + oAuth2Util.when(() -> OAuth2Util.isHybridResponseType(anyString())).thenCallRealMethod(); + + // Mock request parameters when(mockHttpServletRequest.getParameter(CLIENT_ID)).thenReturn(clientId); - when(mockHttpServletRequest.getParameter(REDIRECT_URI)).thenReturn(callbackURI); - when(mockHttpServletRequest.getParameter(RESPONSE_TYPE)).thenReturn("code token"); - OAuth2ClientValidationResponseDTO oAuth2ClientValidationResponseDTO = oAuth2Service. - validateClientInfo(mockHttpServletRequest); + when(mockHttpServletRequest.getParameter(REDIRECT_URI)).thenReturn(callbackUrl); + when(mockHttpServletRequest.getParameter(RESPONSE_TYPE)).thenReturn(responseType); + + // Call the service + OAuth2ClientValidationResponseDTO oAuth2ClientValidationResponseDTO = oAuth2Service + .validateClientInfo(mockHttpServletRequest); + + // Assert results assertNotNull(oAuth2ClientValidationResponseDTO); - assertTrue(oAuth2ClientValidationResponseDTO.isValidClient()); + assertEquals(oAuth2ClientValidationResponseDTO.isValidClient(), expectedValidClient); + + if (!expectedValidClient) { + assertEquals(oAuth2ClientValidationResponseDTO.getErrorCode(), expectedErrorCode); + assertEquals(oAuth2ClientValidationResponseDTO.getErrorMsg(), expectedErrorMsg); + } } }