From 3d3c7a1acc47919a234980a60050e55c8b3c3926 Mon Sep 17 00:00:00 2001 From: rivindum Date: Fri, 8 Sep 2023 15:56:35 +0530 Subject: [PATCH 1/9] Ignore claims outside req object for FAPI --- .../endpoint/authz/OAuth2AuthzEndpoint.java | 69 +++++++++++++------ 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java index 1b15a55a8f0..a8ce31b1bae 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java @@ -2479,6 +2479,13 @@ private void handleOIDCRequestObject(OAuthMessage oAuthMessage, OAuthAuthzReques } else if (isRequestParameter(oauthRequest)) { requestObjValue = oauthRequest.getParam(REQUEST); } + // Mandate request object for FAPI requests + if (isFapiConformant(oAuthMessage.getClientId())) { + if (StringUtils.isBlank(oauthRequest.getParam(REQUEST))) { + throw new InvalidRequestException("Request Object is mandatory for FAPI Conformant Applications.", + OAuth2ErrorCodes.INVALID_REQUEST, "Request object is missing."); + } + } if (StringUtils.isNotEmpty(requestObjValue)) { handleRequestObject(oAuthMessage, oauthRequest, parameters); @@ -2524,8 +2531,10 @@ private void handleRequestObject(OAuthMessage oAuthMessage, OAuthAuthzRequest oa When the request parameter is used, the OpenID Connect request parameter values contained in the JWT supersede those passed using the OAuth 2.0 request syntax */ + boolean isFapiConformant = isFapiConformant(oAuthMessage.getClientId()); + // If FAPI conformant, claims outside request object should be ignored. overrideAuthzParameters(oAuthMessage, parameters, oauthRequest.getParam(REQUEST), - oauthRequest.getParam(REQUEST_URI), requestObject); + oauthRequest.getParam(REQUEST_URI), requestObject, isFapiConformant); // If the redirect uri was not given in auth request the registered redirect uri will be available here, // so validating if the registered redirect uri is a single uri that can be properly redirected. @@ -2548,17 +2557,18 @@ private void handleRequestObject(OAuthMessage oAuthMessage, OAuthAuthzRequest oa private void overrideAuthzParameters(OAuthMessage oAuthMessage, OAuth2Parameters params, String requestParameterValue, - String requestURIParameterValue, RequestObject requestObject) { + String requestURIParameterValue, RequestObject requestObject, + boolean ignoreClaimsOutsideRequestObject) { if (StringUtils.isNotBlank(requestParameterValue) || StringUtils.isNotBlank(requestURIParameterValue)) { - replaceIfPresent(requestObject, REDIRECT_URI, params::setRedirectURI); - replaceIfPresent(requestObject, NONCE, params::setNonce); - replaceIfPresent(requestObject, STATE, params::setState); - replaceIfPresent(requestObject, DISPLAY, params::setDisplay); - replaceIfPresent(requestObject, RESPONSE_MODE, params::setResponseMode); - replaceIfPresent(requestObject, LOGIN_HINT, params::setLoginHint); - replaceIfPresent(requestObject, ID_TOKEN_HINT, params::setIDTokenHint); - replaceIfPresent(requestObject, PROMPT, params::setPrompt); + replaceIfPresent(requestObject, REDIRECT_URI, params::setRedirectURI, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, NONCE, params::setNonce, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, STATE, params::setState, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, DISPLAY, params::setDisplay, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, RESPONSE_MODE, params::setResponseMode, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, LOGIN_HINT, params::setLoginHint, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, ID_TOKEN_HINT, params::setIDTokenHint, ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, PROMPT, params::setPrompt, ignoreClaimsOutsideRequestObject); if (requestObject.getClaim(CLAIMS) instanceof net.minidev.json.JSONObject) { // Claims in the request object is in the type of net.minidev.json.JSONObject, @@ -2570,8 +2580,10 @@ private void overrideAuthzParameters(OAuthMessage oAuthMessage, OAuth2Parameters if (isPkceSupportEnabled()) { // If code_challenge and code_challenge_method is sent inside the request object then add them to // Oauth2 parameters. - replaceIfPresent(requestObject, CODE_CHALLENGE, params::setPkceCodeChallenge); - replaceIfPresent(requestObject, CODE_CHALLENGE_METHOD, params::setPkceCodeChallengeMethod); + replaceIfPresent(requestObject, CODE_CHALLENGE, params::setPkceCodeChallenge, + ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, CODE_CHALLENGE_METHOD, params::setPkceCodeChallengeMethod, + ignoreClaimsOutsideRequestObject); } if (StringUtils.isNotEmpty(requestObject.getClaimValue(SCOPE))) { @@ -2635,11 +2647,14 @@ private List getAcrValues(RequestObject requestObject) { return acrRequestedValues; } - private void replaceIfPresent(RequestObject requestObject, String claim, Consumer consumer) { + private void replaceIfPresent(RequestObject requestObject, String claim, Consumer consumer, + boolean ignoreClaimsOutsideRequestObject) { String claimValue = requestObject.getClaimValue(claim); if (StringUtils.isNotEmpty(claimValue)) { consumer.accept(claimValue); + } else if (ignoreClaimsOutsideRequestObject) { + consumer.accept(null); } } @@ -4069,15 +4084,17 @@ private OAuth2Parameters getOAuth2ParamsFromOAuthMessage(OAuthMessage oAuthMessa * @param params OAuth2 Parameters * @return PKCE code challenge. Priority will be given to the value inside the OAuth2Parameters. */ - private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params) { + private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params) + throws InvalidRequestException { - String pkceChallengeCode; + String pkceChallengeCode = null; + boolean isFapiConformantApp = isFapiConformant(params.getClientId()); // If the code_challenge is in the request object, then it is added to Oauth2 params before this point. if (params.getPkceCodeChallenge() != null) { // If Oauth2 params contains code_challenge get value from Oauth2 params. pkceChallengeCode = params.getPkceCodeChallenge(); - } else { - // Else retrieve from request query params. + } else if (!isFapiConformantApp) { + // Else retrieve from request query params if application is not FAPI compliant. pkceChallengeCode = oAuthMessage.getOauthPKCECodeChallenge(); } @@ -4093,15 +4110,17 @@ private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters * @param params OAuth2 Parameters * @return PKCE code challenge method. Priority will be given to the value inside the OAuth2Parameters. */ - private String getPkceCodeChallengeMethod(OAuthMessage oAuthMessage, OAuth2Parameters params) { + private String getPkceCodeChallengeMethod(OAuthMessage oAuthMessage, OAuth2Parameters params) + throws InvalidRequestException { - String pkceChallengeMethod; + String pkceChallengeMethod = null; + boolean isFapiConformantApp = isFapiConformant(params.getClientId()); // If the code_challenge_method is in the request object, then it is added to Oauth2 params before this point. if (params.getPkceCodeChallengeMethod() != null) { // If Oauth2 params contains code_challenge_method get value from Oauth2 params. pkceChallengeMethod = params.getPkceCodeChallengeMethod(); - } else { - // Else retrieve from request query params. + } else if (!isFapiConformantApp) { + // Else retrieve from request query params if application is not FAPI compliant. pkceChallengeMethod = oAuthMessage.getOauthPKCECodeChallengeMethod(); } @@ -4129,4 +4148,12 @@ private boolean isPromptSelectAccount(OAuth2Parameters oauth2Params) { return OAuthConstants.Prompt.SELECT_ACCOUNT.equals(oauth2Params.getPrompt()); } + + private boolean isFapiConformant(String clientId) throws InvalidRequestException { + try { + return OAuth2Util.isFapiConformantApp(clientId); + } catch (IdentityOAuth2Exception e) { + throw new InvalidRequestException(e.getMessage(), e.getErrorCode()); + } + } } From 99e94c741de934639fcdf00dda2ca0a89eb7a2ad Mon Sep 17 00:00:00 2001 From: rivindum Date: Fri, 15 Sep 2023 09:52:50 +0530 Subject: [PATCH 2/9] Mandate request object for FAPI --- .../identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java | 3 ++- .../carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java | 4 ++++ .../wso2/carbon/identity/oauth/par/common/ParConstants.java | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java index a8ce31b1bae..39742a29c5b 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java @@ -2480,8 +2480,9 @@ private void handleOIDCRequestObject(OAuthMessage oAuthMessage, OAuthAuthzReques requestObjValue = oauthRequest.getParam(REQUEST); } // Mandate request object for FAPI requests + // https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) if (isFapiConformant(oAuthMessage.getClientId())) { - if (StringUtils.isBlank(oauthRequest.getParam(REQUEST))) { + if (requestObjValue == null) { throw new InvalidRequestException("Request Object is mandatory for FAPI Conformant Applications.", OAuth2ErrorCodes.INVALID_REQUEST, "Request object is missing."); } diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java index f9b47e41b40..3cdea9527da 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java @@ -265,6 +265,10 @@ private void validateRequestObject(OAuthAuthzRequest oAuthAuthzRequest) throws P if (requestObject == null) { throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.INVALID_REQUEST_OBJECT); } + } else if (isFAPIConformantApp(oAuthAuthzRequest.getClientId())) { + // Mandate request object for FAPI requests + // https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) + throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.REQUEST_OBJECT_MISSING); } } catch (RequestObjectException e) { if (OAuth2ErrorCodes.SERVER_ERROR.equals(e.getErrorCode())) { diff --git a/components/org.wso2.carbon.identity.oauth.par/src/main/java/org/wso2/carbon/identity/oauth/par/common/ParConstants.java b/components/org.wso2.carbon.identity.oauth.par/src/main/java/org/wso2/carbon/identity/oauth/par/common/ParConstants.java index cc17604c13f..0fa817d45c1 100644 --- a/components/org.wso2.carbon.identity.oauth.par/src/main/java/org/wso2/carbon/identity/oauth/par/common/ParConstants.java +++ b/components/org.wso2.carbon.identity.oauth.par/src/main/java/org/wso2/carbon/identity/oauth/par/common/ParConstants.java @@ -43,6 +43,7 @@ public class ParConstants { public static final String INVALID_CLIENT_ERROR = "A valid OAuth client could not be found for client_id: "; public static final String INVALID_REQUEST_OBJECT = "Unable to build a valid Request Object from the" + " pushed authorization request."; + public static final String REQUEST_OBJECT_MISSING = "Request object is missing."; private ParConstants() { From 60e9492bc2c9430437da3ccbdc29131668c3fe4b Mon Sep 17 00:00:00 2001 From: rivindum Date: Fri, 15 Sep 2023 09:54:45 +0530 Subject: [PATCH 3/9] Retrieve state value inside request object --- .../oauth2/model/CarbonOAuthAuthzRequest.java | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java index df12865bc01..7520d8650cf 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java @@ -18,6 +18,7 @@ package org.wso2.carbon.identity.oauth2.model; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.oltu.oauth2.as.request.OAuthAuthzRequest; @@ -26,12 +27,16 @@ import org.apache.oltu.oauth2.common.exception.OAuthSystemException; import org.apache.oltu.oauth2.common.utils.OAuthUtils; import org.apache.oltu.oauth2.common.validators.OAuthValidator; +import org.json.JSONObject; import org.wso2.carbon.identity.central.log.mgt.utils.LogConstants; import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils; import org.wso2.carbon.identity.oauth.common.OAuthConstants; import org.wso2.carbon.identity.oauth.config.OAuthServerConfiguration; +import org.wso2.carbon.identity.openidconnect.model.Constants; import org.wso2.carbon.utils.DiagnosticLog; +import java.nio.charset.StandardCharsets; +import java.util.Base64; import javax.servlet.http.HttpServletRequest; @@ -80,4 +85,25 @@ protected OAuthValidator initValidator() throws OAuthProblem return OAuthUtils.instantiateClass(clazz); } + + @Override + public String getState() { + + /*If request object is present, get the state from the request object. + This state value was required to overridden from the request object in order to make sure the correct state + value(value inside the request object) is sent in error responses prior to building the request object.*/ + if (StringUtils.isNotBlank(getParam(Constants.REQUEST))) { + byte[] requestObject; + try { + requestObject = Base64.getDecoder().decode(getParam(Constants.REQUEST).split("\\.")[1]); + } catch (IllegalArgumentException e) { + // Decode if the requestObject is base64-url encoded. + requestObject = Base64.getUrlDecoder().decode(getParam(Constants.REQUEST).split("\\.")[1]); + } + JSONObject requestObjectJson = new JSONObject(new String(requestObject, StandardCharsets.UTF_8)); + return requestObjectJson.has(OAuth.OAUTH_STATE) ? requestObjectJson.getString(OAuth.OAUTH_STATE) : null; + } else { + return super.getState(); + } + } } From ef0e046274f92b950041433c237956dc64e81944 Mon Sep 17 00:00:00 2001 From: rivindum Date: Mon, 18 Sep 2023 18:24:57 +0530 Subject: [PATCH 4/9] Set state for error flows from request object --- .../endpoint/authz/OAuth2AuthzEndpoint.java | 9 ++-- .../oauth/endpoint/par/OAuth2ParEndpoint.java | 35 ++++++++------- .../oauth/endpoint/util/EndpointUtil.java | 44 ++++++++++++++++++- .../oauth/endpoint/util/EndpointUtilTest.java | 4 +- .../oauth2/model/CarbonOAuthAuthzRequest.java | 27 ------------ 5 files changed, 70 insertions(+), 49 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java index 39742a29c5b..a1beec7ab70 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java @@ -2399,7 +2399,7 @@ private String populateOauthParameters(OAuth2Parameters params, OAuthMessage oAu } if (isPkceSupportEnabled()) { - String pkceChallengeCode = getPkceCodeChallenge(oAuthMessage, params); + String pkceChallengeCode = getPkceCodeChallenge(oAuthMessage, params, validationResponse.isPkceMandatory()); String pkceChallengeMethod = getPkceCodeChallengeMethod(oAuthMessage, params); String redirectURI = validatePKCEParameters(oAuthMessage, validationResponse, pkceChallengeCode, @@ -4083,9 +4083,10 @@ private OAuth2Parameters getOAuth2ParamsFromOAuthMessage(OAuthMessage oAuthMessa * * @param oAuthMessage oAuthMessage * @param params OAuth2 Parameters + * @param isPkcemandatory is PKCE mandatory * @return PKCE code challenge. Priority will be given to the value inside the OAuth2Parameters. */ - private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params) + private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params, boolean isPkcemandatory) throws InvalidRequestException { String pkceChallengeCode = null; @@ -4094,8 +4095,8 @@ private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters if (params.getPkceCodeChallenge() != null) { // If Oauth2 params contains code_challenge get value from Oauth2 params. pkceChallengeCode = params.getPkceCodeChallenge(); - } else if (!isFapiConformantApp) { - // Else retrieve from request query params if application is not FAPI compliant. + } else if (!isFapiConformantApp || isPkcemandatory) { + // Else retrieve from request query params if application is not FAPI compliant or PKCE is mandatory. pkceChallengeCode = oAuthMessage.getOauthPKCECodeChallenge(); } diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java index f79380dc7d5..f94358ff930 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java @@ -255,23 +255,26 @@ private void validateInputParameters(HttpServletRequest request) throws ParClien private void validateRequestObject(OAuthAuthzRequest oAuthAuthzRequest) throws ParCoreException { try { - if (OAuth2Util.isOIDCAuthzRequest(oAuthAuthzRequest.getScopes()) && - StringUtils.isNotBlank(oAuthAuthzRequest.getParam(REQUEST))) { - - OAuth2Parameters parameters = new OAuth2Parameters(); - parameters.setClientId(oAuthAuthzRequest.getClientId()); - parameters.setRedirectURI(oAuthAuthzRequest.getRedirectURI()); - parameters.setResponseType(oAuthAuthzRequest.getResponseType()); - parameters.setTenantDomain(getSPTenantDomainFromClientId(oAuthAuthzRequest.getClientId())); - - RequestObject requestObject = OIDCRequestObjectUtil.buildRequestObject(oAuthAuthzRequest, parameters); - if (requestObject == null) { - throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.INVALID_REQUEST_OBJECT); + if (OAuth2Util.isOIDCAuthzRequest(oAuthAuthzRequest.getScopes())) { + if (StringUtils.isNotBlank(oAuthAuthzRequest.getParam(REQUEST))) { + + OAuth2Parameters parameters = new OAuth2Parameters(); + parameters.setClientId(oAuthAuthzRequest.getClientId()); + parameters.setRedirectURI(oAuthAuthzRequest.getRedirectURI()); + parameters.setResponseType(oAuthAuthzRequest.getResponseType()); + parameters.setTenantDomain(getSPTenantDomainFromClientId(oAuthAuthzRequest.getClientId())); + + RequestObject requestObject = + OIDCRequestObjectUtil.buildRequestObject(oAuthAuthzRequest, parameters); + if (requestObject == null) { + throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, + ParConstants.INVALID_REQUEST_OBJECT); + } + } else if (isFAPIConformantApp(oAuthAuthzRequest.getClientId())) { + /* Mandate request object for FAPI requests + https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) */ + throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.REQUEST_OBJECT_MISSING); } - } else if (isFAPIConformantApp(oAuthAuthzRequest.getClientId())) { - // Mandate request object for FAPI requests - // https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) - throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.REQUEST_OBJECT_MISSING); } } catch (RequestObjectException e) { if (OAuth2ErrorCodes.SERVER_ERROR.equals(e.getErrorCode())) { diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java index 0bd81b01140..ef2c0dca69b 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java @@ -91,6 +91,7 @@ import org.wso2.carbon.identity.oauth2.OAuth2Service; import org.wso2.carbon.identity.oauth2.OAuth2TokenValidationService; import org.wso2.carbon.identity.oauth2.Oauth2ScopeConstants; +import org.wso2.carbon.identity.oauth2.RequestObjectException; import org.wso2.carbon.identity.oauth2.bean.OAuthClientAuthnContext; import org.wso2.carbon.identity.oauth2.bean.Scope; import org.wso2.carbon.identity.oauth2.dto.OAuth2ClientValidationResponseDTO; @@ -100,7 +101,11 @@ import org.wso2.carbon.identity.oauth2.scopeservice.OAuth2Resource; import org.wso2.carbon.identity.oauth2.scopeservice.ScopeMetadataService; import org.wso2.carbon.identity.oauth2.util.OAuth2Util; +import org.wso2.carbon.identity.openidconnect.OIDCRequestObjectUtil; +import org.wso2.carbon.identity.openidconnect.RequestObjectBuilder; import org.wso2.carbon.identity.openidconnect.RequestObjectService; +import org.wso2.carbon.identity.openidconnect.RequestObjectValidator; +import org.wso2.carbon.identity.openidconnect.model.RequestObject; import org.wso2.carbon.identity.webfinger.DefaultWebFingerProcessor; import org.wso2.carbon.identity.webfinger.WebFingerProcessor; import org.wso2.carbon.idp.mgt.IdentityProviderManagementException; @@ -1712,7 +1717,18 @@ public static void setParAuthService(ParAuthService parAuthService) { public static String retrieveStateForErrorURL(HttpServletRequest request, OAuth2Parameters oAuth2Parameters) { String state = null; - if (oAuth2Parameters != null && oAuth2Parameters.getState() != null) { + + if (request.getParameter(OAuthConstants.OAuth20Params.REQUEST) != null) { + String stateInsideRequestObj = getStateFromRequestObject(request, oAuth2Parameters); + if (StringUtils.isNotBlank(stateInsideRequestObj)) { + state = stateInsideRequestObj; + if (log.isDebugEnabled()) { + log.debug("Retrieved state value " + state + " from request object."); + } + } + } + + if (StringUtils.isEmpty(state) && oAuth2Parameters != null && oAuth2Parameters.getState() != null) { state = oAuth2Parameters.getState(); if (log.isDebugEnabled()) { log.debug("Retrieved state value " + state + " from OAuth2Parameters."); @@ -1727,6 +1743,32 @@ public static String retrieveStateForErrorURL(HttpServletRequest request, OAuth2 return state; } + private static String getStateFromRequestObject(HttpServletRequest request, OAuth2Parameters oAuth2Parameters) { + + try { + RequestObjectValidator requestObjectValidator = OAuthServerConfiguration.getInstance() + .getRequestObjectValidator(); + RequestObjectBuilder requestObjectBuilder = OAuthServerConfiguration.getInstance() + .getRequestObjectBuilders().get("request_param_value_builder"); + RequestObject requestObject = + requestObjectBuilder.buildRequestObject(request.getParameter(OAuthConstants.OAuth20Params.REQUEST), + oAuth2Parameters); + if (StringUtils.isBlank(oAuth2Parameters.getClientId())) { + // Set client id and tenant domain required for signature validation if not already set. + String clientId = request.getParameter(PROP_CLIENT_ID); + oAuth2Parameters.setClientId(clientId); + oAuth2Parameters.setTenantDomain(getSPTenantDomainFromClientId(clientId)); + } + // Validate request object signature to ensure request object is not tampered. + OIDCRequestObjectUtil.validateRequestObjectSignature(oAuth2Parameters, requestObject, + requestObjectValidator); + return requestObject.getClaimValue(OAuthConstants.OAuth20Params.STATE); + } catch (RequestObjectException e) { + log.debug("Error while retrieving state from request object.", e); + } + return null; + } + /** * Return updated redirect URL. * diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtilTest.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtilTest.java index b6e8965493a..c69565d4f96 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtilTest.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtilTest.java @@ -103,6 +103,7 @@ import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; +import static org.mockito.ArgumentMatchers.contains; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.ArgumentMatchers.nullable; import static org.mockito.Matchers.any; @@ -208,6 +209,7 @@ public class EndpointUtilTest extends PowerMockIdentityBaseTest { private static final String REQUESTED_OIDC_SCOPES_KEY = "requested_oidc_scopes="; private static final String REQUESTED_OIDC_SCOPES_VALUES = "openid+profile"; private static final String EXTERNAL_CONSENTED_APP_NAME = "testApp"; + private static final String REDIRECT = "redirect"; private static final String EXTERNAL_CONSENT_URL = "https://localhost:9443/consent"; private String username; private String password; @@ -646,7 +648,7 @@ public void testGetErrorPageURL(boolean isImplicitResponse, boolean isHybridResp when(OAuth2Util.OAuthURL.getOAuth2ErrorPageUrl()).thenReturn(ERROR_PAGE_URL); when(mockedOAuthResponse.getLocationUri()).thenReturn("http://localhost:8080/location"); - when(mockedHttpServletRequest.getParameter(anyString())).thenReturn("http://localhost:8080/location"); + when(mockedHttpServletRequest.getParameter(contains(REDIRECT))).thenReturn("http://localhost:8080/location"); String url = EndpointUtil.getErrorPageURL(mockedHttpServletRequest, "invalid request", "invalid request object", "invalid request", "test", parameters); diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java index 7520d8650cf..793bf4053b2 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/CarbonOAuthAuthzRequest.java @@ -18,7 +18,6 @@ package org.wso2.carbon.identity.oauth2.model; -import org.apache.commons.lang3.StringUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.oltu.oauth2.as.request.OAuthAuthzRequest; @@ -27,17 +26,12 @@ import org.apache.oltu.oauth2.common.exception.OAuthSystemException; import org.apache.oltu.oauth2.common.utils.OAuthUtils; import org.apache.oltu.oauth2.common.validators.OAuthValidator; -import org.json.JSONObject; import org.wso2.carbon.identity.central.log.mgt.utils.LogConstants; import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils; import org.wso2.carbon.identity.oauth.common.OAuthConstants; import org.wso2.carbon.identity.oauth.config.OAuthServerConfiguration; -import org.wso2.carbon.identity.openidconnect.model.Constants; import org.wso2.carbon.utils.DiagnosticLog; -import java.nio.charset.StandardCharsets; -import java.util.Base64; - import javax.servlet.http.HttpServletRequest; /** @@ -85,25 +79,4 @@ protected OAuthValidator initValidator() throws OAuthProblem return OAuthUtils.instantiateClass(clazz); } - - @Override - public String getState() { - - /*If request object is present, get the state from the request object. - This state value was required to overridden from the request object in order to make sure the correct state - value(value inside the request object) is sent in error responses prior to building the request object.*/ - if (StringUtils.isNotBlank(getParam(Constants.REQUEST))) { - byte[] requestObject; - try { - requestObject = Base64.getDecoder().decode(getParam(Constants.REQUEST).split("\\.")[1]); - } catch (IllegalArgumentException e) { - // Decode if the requestObject is base64-url encoded. - requestObject = Base64.getUrlDecoder().decode(getParam(Constants.REQUEST).split("\\.")[1]); - } - JSONObject requestObjectJson = new JSONObject(new String(requestObject, StandardCharsets.UTF_8)); - return requestObjectJson.has(OAuth.OAUTH_STATE) ? requestObjectJson.getString(OAuth.OAUTH_STATE) : null; - } else { - return super.getState(); - } - } } From ba5de369c23e91a6c3c7eb2ccfd865cad282c847 Mon Sep 17 00:00:00 2001 From: rivindum Date: Fri, 22 Sep 2023 16:02:32 +0530 Subject: [PATCH 5/9] Fix review comments --- .../carbon/identity/oauth/endpoint/util/EndpointUtil.java | 7 +++++-- .../identity/openidconnect/OIDCRequestObjectUtil.java | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java index ef2c0dca69b..56906f730b7 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/EndpointUtil.java @@ -1728,7 +1728,7 @@ public static String retrieveStateForErrorURL(HttpServletRequest request, OAuth2 } } - if (StringUtils.isEmpty(state) && oAuth2Parameters != null && oAuth2Parameters.getState() != null) { + if (StringUtils.isBlank(state) && oAuth2Parameters != null && oAuth2Parameters.getState() != null) { state = oAuth2Parameters.getState(); if (log.isDebugEnabled()) { log.debug("Retrieved state value " + state + " from OAuth2Parameters."); @@ -1749,7 +1749,7 @@ private static String getStateFromRequestObject(HttpServletRequest request, OAut RequestObjectValidator requestObjectValidator = OAuthServerConfiguration.getInstance() .getRequestObjectValidator(); RequestObjectBuilder requestObjectBuilder = OAuthServerConfiguration.getInstance() - .getRequestObjectBuilders().get("request_param_value_builder"); + .getRequestObjectBuilders().get(OIDCRequestObjectUtil.REQUEST_PARAM_VALUE_BUILDER); RequestObject requestObject = requestObjectBuilder.buildRequestObject(request.getParameter(OAuthConstants.OAuth20Params.REQUEST), oAuth2Parameters); @@ -1764,6 +1764,9 @@ private static String getStateFromRequestObject(HttpServletRequest request, OAut requestObjectValidator); return requestObject.getClaimValue(OAuthConstants.OAuth20Params.STATE); } catch (RequestObjectException e) { + /* If request object signature validation fails, logs and return null from this method and the state value + will be overridden from oauth2 parameters or request parameters if present inside the + retrieveStateForErrorURL method. */ log.debug("Error while retrieving state from request object.", e); } return null; diff --git a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/OIDCRequestObjectUtil.java b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/OIDCRequestObjectUtil.java index 1ddac4da221..b6dcdc27e4b 100644 --- a/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/OIDCRequestObjectUtil.java +++ b/components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/openidconnect/OIDCRequestObjectUtil.java @@ -45,7 +45,7 @@ public class OIDCRequestObjectUtil { private static final Log log = LogFactory.getLog(OIDCRequestObjectUtil.class); private static final String REQUEST = "request"; private static final String REQUEST_URI = "request_uri"; - private static final String REQUEST_PARAM_VALUE_BUILDER = "request_param_value_builder"; + public static final String REQUEST_PARAM_VALUE_BUILDER = "request_param_value_builder"; private static final String REQUEST_URI_PARAM_VALUE_BUILDER = "request_uri_param_value_builder"; /** From 91e786f7e9e877fcee7b9379dda1bcd09d7eb34d Mon Sep 17 00:00:00 2001 From: rivindum Date: Fri, 22 Sep 2023 16:51:26 +0530 Subject: [PATCH 6/9] Fix review comments --- .../endpoint/authz/OAuth2AuthzEndpoint.java | 29 +++++++------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java index 05628599dbf..f0dca0bcd92 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java @@ -2409,7 +2409,7 @@ private String populateOauthParameters(OAuth2Parameters params, OAuthMessage oAu } if (isPkceSupportEnabled()) { - String pkceChallengeCode = getPkceCodeChallenge(oAuthMessage, params, validationResponse.isPkceMandatory()); + String pkceChallengeCode = getPkceCodeChallenge(oAuthMessage, params); String pkceChallengeMethod = getPkceCodeChallengeMethod(oAuthMessage, params); String redirectURI = validatePKCEParameters(oAuthMessage, validationResponse, pkceChallengeCode, @@ -2591,10 +2591,8 @@ private void overrideAuthzParameters(OAuthMessage oAuthMessage, OAuth2Parameters if (isPkceSupportEnabled()) { // If code_challenge and code_challenge_method is sent inside the request object then add them to // Oauth2 parameters. - replaceIfPresent(requestObject, CODE_CHALLENGE, params::setPkceCodeChallenge, - ignoreClaimsOutsideRequestObject); - replaceIfPresent(requestObject, CODE_CHALLENGE_METHOD, params::setPkceCodeChallengeMethod, - ignoreClaimsOutsideRequestObject); + replaceIfPresent(requestObject, CODE_CHALLENGE, params::setPkceCodeChallenge, false); + replaceIfPresent(requestObject, CODE_CHALLENGE_METHOD, params::setPkceCodeChallengeMethod, false); } if (StringUtils.isNotEmpty(requestObject.getClaimValue(SCOPE))) { @@ -4093,20 +4091,17 @@ private OAuth2Parameters getOAuth2ParamsFromOAuthMessage(OAuthMessage oAuthMessa * * @param oAuthMessage oAuthMessage * @param params OAuth2 Parameters - * @param isPkcemandatory is PKCE mandatory * @return PKCE code challenge. Priority will be given to the value inside the OAuth2Parameters. */ - private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params, boolean isPkcemandatory) - throws InvalidRequestException { + private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters params) { - String pkceChallengeCode = null; - boolean isFapiConformantApp = isFapiConformant(params.getClientId()); + String pkceChallengeCode; // If the code_challenge is in the request object, then it is added to Oauth2 params before this point. if (params.getPkceCodeChallenge() != null) { // If Oauth2 params contains code_challenge get value from Oauth2 params. pkceChallengeCode = params.getPkceCodeChallenge(); - } else if (!isFapiConformantApp || isPkcemandatory) { - // Else retrieve from request query params if application is not FAPI compliant or PKCE is mandatory. + } else { + // Else retrieve from request query params. pkceChallengeCode = oAuthMessage.getOauthPKCECodeChallenge(); } @@ -4122,17 +4117,15 @@ private String getPkceCodeChallenge(OAuthMessage oAuthMessage, OAuth2Parameters * @param params OAuth2 Parameters * @return PKCE code challenge method. Priority will be given to the value inside the OAuth2Parameters. */ - private String getPkceCodeChallengeMethod(OAuthMessage oAuthMessage, OAuth2Parameters params) - throws InvalidRequestException { + private String getPkceCodeChallengeMethod(OAuthMessage oAuthMessage, OAuth2Parameters params) { - String pkceChallengeMethod = null; - boolean isFapiConformantApp = isFapiConformant(params.getClientId()); + String pkceChallengeMethod; // If the code_challenge_method is in the request object, then it is added to Oauth2 params before this point. if (params.getPkceCodeChallengeMethod() != null) { // If Oauth2 params contains code_challenge_method get value from Oauth2 params. pkceChallengeMethod = params.getPkceCodeChallengeMethod(); - } else if (!isFapiConformantApp) { - // Else retrieve from request query params if application is not FAPI compliant. + } else { + // Else retrieve from request query params. pkceChallengeMethod = oAuthMessage.getOauthPKCECodeChallengeMethod(); } From e6a16ed597509956ecd93f6b767ebda6f0b3982b Mon Sep 17 00:00:00 2001 From: rivindum Date: Mon, 25 Sep 2023 21:57:28 +0530 Subject: [PATCH 7/9] Improve unit tests --- .../authz/OAuth2AuthzEndpointTest.java | 183 ++++++++++++++++++ .../resources/security/testkeystore.jks | Bin 0 -> 2217 bytes 2 files changed, 183 insertions(+) create mode 100644 components/org.wso2.carbon.identity.oauth.endpoint/src/test/resources/repository/resources/security/testkeystore.jks diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java index 894e75b8817..cd7cea3ca3f 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java @@ -17,12 +17,20 @@ */ package org.wso2.carbon.identity.oauth.endpoint.authz; +import com.nimbusds.jose.JOSEException; +import com.nimbusds.jose.JWSAlgorithm; +import com.nimbusds.jose.JWSHeader; +import com.nimbusds.jose.JWSSigner; +import com.nimbusds.jose.crypto.RSASSASigner; +import com.nimbusds.jose.crypto.bc.BouncyCastleProviderSingleton; import com.nimbusds.jwt.JWTClaimsSet; +import com.nimbusds.jwt.PlainJWT; import com.nimbusds.jwt.SignedJWT; import org.apache.axis2.transport.http.HTTPConstants; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.lang.StringUtils; import org.apache.commons.lang3.ArrayUtils; +import org.apache.commons.lang3.SerializationUtils; import org.apache.oltu.oauth2.as.request.OAuthAuthzRequest; import org.apache.oltu.oauth2.as.validator.CodeValidator; import org.apache.oltu.oauth2.as.validator.TokenValidator; @@ -40,6 +48,7 @@ import org.testng.Assert; import org.testng.annotations.AfterTest; import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import org.wso2.carbon.base.CarbonBaseConstants; @@ -67,6 +76,7 @@ import org.wso2.carbon.identity.central.log.mgt.utils.LoggerUtils; import org.wso2.carbon.identity.claim.metadata.mgt.ClaimMetadataHandler; import org.wso2.carbon.identity.claim.metadata.mgt.model.ExternalClaim; +import org.wso2.carbon.identity.common.testng.TestConstants; import org.wso2.carbon.identity.core.ServiceURL; import org.wso2.carbon.identity.core.ServiceURLBuilder; import org.wso2.carbon.identity.core.URLBuilderException; @@ -94,6 +104,7 @@ import org.wso2.carbon.identity.oauth2.IdentityOAuth2Exception; import org.wso2.carbon.identity.oauth2.OAuth2ScopeService; import org.wso2.carbon.identity.oauth2.OAuth2Service; +import org.wso2.carbon.identity.oauth2.RequestObjectException; import org.wso2.carbon.identity.oauth2.authz.AuthorizationHandlerManager; import org.wso2.carbon.identity.oauth2.authz.OAuthAuthzReqMessageContext; import org.wso2.carbon.identity.oauth2.device.api.DeviceAuthService; @@ -118,22 +129,33 @@ import org.wso2.carbon.identity.openidconnect.DefaultOIDCClaimsCallbackHandler; import org.wso2.carbon.identity.openidconnect.OIDCConstants; import org.wso2.carbon.identity.openidconnect.OpenIDConnectClaimFilterImpl; +import org.wso2.carbon.identity.openidconnect.RequestObjectBuilder; import org.wso2.carbon.identity.openidconnect.RequestObjectService; +import org.wso2.carbon.identity.openidconnect.RequestObjectValidator; +import org.wso2.carbon.identity.openidconnect.RequestObjectValidatorImpl; +import org.wso2.carbon.identity.openidconnect.RequestParamRequestObjectBuilder; import org.wso2.carbon.identity.openidconnect.model.RequestObject; import org.wso2.carbon.identity.openidconnect.model.RequestedClaim; import org.wso2.carbon.utils.CarbonUtils; +import java.io.FileInputStream; import java.io.IOException; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.nio.file.Path; import java.nio.file.Paths; +import java.security.Key; +import java.security.KeyStore; +import java.security.interfaces.RSAPrivateKey; import java.sql.Connection; import java.text.ParseException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Calendar; import java.util.Collections; +import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.Hashtable; @@ -182,6 +204,9 @@ import static org.testng.Assert.assertNotNull; import static org.testng.Assert.assertTrue; import static org.testng.FileAssert.fail; +import static org.wso2.carbon.identity.oauth2.util.OAuth2Util.EXP; +import static org.wso2.carbon.identity.oauth2.util.OAuth2Util.NBF; +import static org.wso2.carbon.identity.openidconnect.OIDCRequestObjectUtil.REQUEST_PARAM_VALUE_BUILDER; @PrepareForTest({OAuth2Util.class, SessionDataCache.class, OAuthServerConfiguration.class, IdentityDatabaseUtil.class, EndpointUtil.class, FrameworkUtils.class, EndpointUtil.class, OpenIDConnectUserRPStore.class, SignedJWT.class, @@ -310,6 +335,8 @@ public class OAuth2AuthzEndpointTest extends TestOAuthEndpointBase { private OAuth2ScopeConsentResponse oAuth2ScopeConsentResponse; private ServiceProvider dummySp; + private KeyStore clientKeyStore; + @BeforeClass public void setUp() throws Exception { @@ -2137,6 +2164,162 @@ public Object answer(InvocationOnMock invocation) { assertEquals(cacheEntry[0].getoAuth2Parameters().getDisplayName(), savedDisplayName); } + @BeforeMethod + public void setupKeystore() throws Exception { + + clientKeyStore = getKeyStoreFromFile("testkeystore.jks", "wso2carbon", + System.getProperty(CarbonBaseConstants.CARBON_HOME)); + } + + @DataProvider(name = "provideHandleRequestObjectData") + public Object[][] provideHandleRequestObjectData() throws Exception { + + OAuth2Parameters oAuth2Parameters = new OAuth2Parameters(); + oAuth2Parameters.setClientId(TestConstants.CLIENT_ID); + oAuth2Parameters.setRedirectURI(TestConstants.CALLBACK); + oAuth2Parameters.setTenantDomain(TestConstants.TENANT_DOMAIN); + oAuth2Parameters.setNonce("nonceInParams"); + oAuth2Parameters.setState("stateInParams"); + oAuth2Parameters.setPrompt("promptInParams"); + + Map defaultClaims = new HashMap<>(); + defaultClaims.put(OAuthConstants.OAuth20Params.REDIRECT_URI, TestConstants.CALLBACK); + defaultClaims.put(NBF, System.currentTimeMillis() / 1000); + defaultClaims.put(EXP, System.currentTimeMillis() / 1000 + 3000); + defaultClaims.put(OAuthConstants.OAuth20Params.SCOPE, TestConstants.SCOPE_STRING); + + Map claims1 = new HashMap<>(defaultClaims); + claims1.put(OAuthConstants.STATE, "stateInRequestObject"); + claims1.put(OAuthConstants.OAuth20Params.NONCE, "nonceInRequestObject"); + claims1.put(OAuthConstants.OAuth20Params.PROMPT, "promptInRequestObject"); + + return new Object[][]{ + {true, SerializationUtils.clone(oAuth2Parameters), claims1, + "Test override claims from request object."}, + {true, SerializationUtils.clone(oAuth2Parameters), defaultClaims, + "Test ignore claims outside request object."}, // no overridable claims sent in the req obj + {false, SerializationUtils.clone(oAuth2Parameters), defaultClaims, + "Test request without request object."} + }; + } + + @Test(dataProvider = "provideHandleRequestObjectData") + public void testHandleOIDCRequestObjectForFAPI(boolean withRequestObject, Object oAuth2ParametersObj, + Map claims, + String testName) throws Exception { + + OAuth2Parameters oAuth2Parameters = (OAuth2Parameters) oAuth2ParametersObj; + OAuth2Parameters originalOAuth2Parameters = SerializationUtils.clone(oAuth2Parameters); + + Key privateKey = clientKeyStore.getKey("wso2carbon", "wso2carbon".toCharArray()); + + if (withRequestObject) { + String jsonWebToken = + buildJWTWithExpiry(oAuth2Parameters.getClientId(), oAuth2Parameters.getClientId(), "1000", + "audience", + JWSAlgorithm.PS256.getName(), + privateKey, 0, claims, 3600 * 1000); + when(oAuthAuthzRequest.getParam(OAuthConstants.OAuth20Params.REQUEST)).thenReturn(jsonWebToken); + } + + Map requestObjectBuilderMap = new HashMap<>(); + requestObjectBuilderMap.put(REQUEST_PARAM_VALUE_BUILDER, new RequestParamRequestObjectBuilder()); + RequestObjectValidator requestObjectValidator = PowerMockito.spy(new RequestObjectValidatorImpl()); + doReturn(true).when(requestObjectValidator, "validateSignature", any(), any()); + doReturn(true).when(requestObjectValidator, "isValidAudience", any(), any()); + mockOAuthServerConfiguration(); + when((oAuthServerConfiguration.getRequestObjectBuilders())).thenReturn(requestObjectBuilderMap); + when((oAuthServerConfiguration.getRequestObjectValidator())).thenReturn(requestObjectValidator); + mockStatic(LoggerUtils.class); + when(LoggerUtils.isDiagnosticLogsEnabled()).thenReturn(false); + + OAuthAppDO appDO = new OAuthAppDO(); + appDO.setRequestObjectSignatureValidationEnabled(false); + spy(OAuth2Util.class); + doReturn(appDO).when(OAuth2Util.class, "getAppInformationByClientId", oAuth2Parameters.getClientId()); + doReturn(true).when(OAuth2Util.class, "isFapiConformantApp", any()); + mockEndpointUtil(false); + when(oAuth2Service.isPKCESupportEnabled()).thenReturn(false); + + Assert.assertEquals(oAuth2Parameters.getNonce(), originalOAuth2Parameters.getNonce()); + Assert.assertEquals(oAuth2Parameters.getState(), originalOAuth2Parameters.getState()); + Assert.assertEquals(oAuth2Parameters.getPrompt(), originalOAuth2Parameters.getPrompt()); + + Method handleOIDCRequestObject = authzEndpointObject.getClass().getDeclaredMethod( + "handleOIDCRequestObject", OAuthMessage.class, OAuthAuthzRequest.class, OAuth2Parameters.class); + handleOIDCRequestObject.setAccessible(true); + try { + handleOIDCRequestObject.invoke(authzEndpointObject, oAuthMessage, oAuthAuthzRequest, oAuth2Parameters); + Assert.assertEquals(oAuth2Parameters.getNonce(), claims.get(OAuthConstants.OAuth20Params.NONCE), testName); + Assert.assertEquals(oAuth2Parameters.getState(), claims.get(OAuthConstants.OAuth20Params.STATE), testName); + Assert.assertEquals(oAuth2Parameters.getPrompt(), claims.get(OAuthConstants.OAuth20Params.PROMPT), + testName); + } catch (InvocationTargetException e) { + Assert.assertEquals(e.getTargetException().getMessage(), + "Request Object is mandatory for FAPI Conformant Applications.", testName); + } + } + + private static KeyStore getKeyStoreFromFile(String keystoreName, String password, String home) throws Exception { + + Path tenantKeystorePath = Paths.get(home, "repository", "resources", "security", keystoreName); + FileInputStream file = new FileInputStream(tenantKeystorePath.toString()); + KeyStore keystore = KeyStore.getInstance(KeyStore.getDefaultType()); + keystore.load(file, password.toCharArray()); + return keystore; + } + + private static String buildJWTWithExpiry(String issuer, String subject, String jti, String audience, String + algorithm, Key privateKey, long notBeforeMillis, Map claims, long lifetimeInMillis) + throws RequestObjectException { + + JWTClaimsSet jwtClaimsSet = getJwtClaimsSet(issuer, subject, jti, audience, notBeforeMillis, claims, + lifetimeInMillis); + if (JWSAlgorithm.NONE.getName().equals(algorithm)) { + return new PlainJWT(jwtClaimsSet).serialize(); + } + + return signJWTWithRSA(jwtClaimsSet, privateKey, JWSAlgorithm.parse(algorithm)); + } + + private static String signJWTWithRSA(JWTClaimsSet jwtClaimsSet, Key privateKey, JWSAlgorithm jwsAlgorithm) + throws RequestObjectException { + + try { + JWSSigner signer = new RSASSASigner((RSAPrivateKey) privateKey); + SignedJWT signedJWT = new SignedJWT(new JWSHeader(jwsAlgorithm), jwtClaimsSet); + signer.getJCAContext().setProvider(BouncyCastleProviderSingleton.getInstance()); + signedJWT.sign(signer); + return signedJWT.serialize(); + } catch (JOSEException e) { + throw new RequestObjectException("error_signing_jwt", "Error occurred while signing JWT."); + } + } + + private static JWTClaimsSet getJwtClaimsSet(String issuer, String subject, String jti, String audience, long + notBeforeMillis, Map claims, long lifetimeInMillis) { + + long curTimeInMillis = Calendar.getInstance().getTimeInMillis(); + // Set claims to jwt token. + JWTClaimsSet.Builder jwtClaimsSetBuilder = new JWTClaimsSet.Builder(); + jwtClaimsSetBuilder.issuer(issuer); + jwtClaimsSetBuilder.subject(subject); + jwtClaimsSetBuilder.audience(Arrays.asList(audience)); + jwtClaimsSetBuilder.jwtID(jti); + jwtClaimsSetBuilder.expirationTime(new Date((curTimeInMillis + lifetimeInMillis))); + jwtClaimsSetBuilder.issueTime(new Date(curTimeInMillis)); + + if (notBeforeMillis > 0) { + jwtClaimsSetBuilder.notBeforeTime(new Date(curTimeInMillis + notBeforeMillis)); + } + if (claims != null && !claims.isEmpty()) { + for (Map.Entry entry : claims.entrySet()) { + jwtClaimsSetBuilder.claim(entry.getKey().toString(), entry.getValue()); + } + } + return jwtClaimsSetBuilder.build(); + } + @Test(dependsOnGroups = "testWithConnection") public void testIdentityOAuthAdminException() throws Exception { diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/resources/repository/resources/security/testkeystore.jks b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/resources/repository/resources/security/testkeystore.jks new file mode 100644 index 0000000000000000000000000000000000000000..062f0e33999bff9ee4fd10456b265be6ec28d4b8 GIT binary patch literal 2217 zcmcJQX*|@87RTp5n{{Z6U4|%GivJk9%A~~DAA1^EMwXPZFO_X%?dn06u`iYM5NSg* z*>f2}$u46rODTKOxO#5)#l7$Ei{Fdy`Mo%wvwY5Be`bG%Kp@b40e_1-G?1c7b`5Z& z`0j^X*06hc2m}EGsqj6J56P{B1mJ)o6b%3f2%HLEszaWN_aFHjd}G>yk3B%>DF2C- zY^_|3@XM;*x;RRKw+Cd;%MZR}J0_Wj-K-o8ceYQT3;W>FM8^bn&n!GSX}3O!G@M*0 zKRNwIbo1FxPs~)(9re~fjmqUYV-*uZA7H#cXKii296q5GZ4WT0sCcUdMx zsJ$W-Y6+U%!y&dEwsS4%b`x`}iJeMvm7F&6j>noJbVbeSYuClI5mNOGRG%SHIaH?Fz=1=gY+4;t`WmN?Lci{FE9HwbpO2)zz1w}Db~r`AM|E-8(J`jhop@;A zy)320v}m_^_;lvfx?Y3NgCls(QPb?F7dlqhW$($c*aq)+v3Y`=2!z=q&vnNmMcZC6 z&6*|ps|>l_ukcFQH+jJl(Z(i8nr(F!qJkhakMzT=y-KJCt6jG?EaEufD(m>kIVXgT1`5naAyWWe9aDZF zfk>U7vZ{?)HgQsz#5Nq^KQD%*2^j%_PQ!u6BVV4pHhC*o#b!Cx5c!d=AQf8IpBbv$ zXlmkT;MCyWX40!SdM)?*82Id=)3^IePi1xxVMILhijIYhj9gB2F=;ZDvn;;^w|t@? zU21v@_0-K|_=T#NM6kSPh^8t-hi6*t?x&?@3!%7e!s0;FF6*z6)Q!oj<>;P94IY@H zQkQ*~+A)T-ak7B&3YNW>nj25oSyXzfpHmuG%+^{yBi9BEuG*Q1JricVHbFx;&Xygp zFlrbizMfFgJ7#s3SCXk-!jo_%a-mD&hnN;Nl$$K+EpSWktY8Z%z0K;ji9@W-C%OEb zOtKxzuVflSl%es5y9FWp55_8>@;HV=Aigk+Q6-MZX7WyHUu&^NJ4*M+FZfkyBexGG0AXs#RffX5=hH)2 zt=?xZW>B387rG1m#;vv7;yEH_+laH%#S6Xjh~W&oT~Rko@&!VO-8GTH@VK5ERLzGr z1^)R}s~Q}LoHfRX^;nY%@nUjcm+TB$@HN+st^9*ALYDGL!9@2_2UKB3-dky1w}046 z40SjqB1+CTLJDVn&r#&c-M<;~7o_V5{kXJ#;?q%lilCM5YH=N@aIq*WQROBTdRX)H zD$|L6h}MnpkLxjx&M$x7FU5`SPiCZ5xGqbkVkGPqeiRr!(|I?pcbG)Znqmz#OX}$a zfovJGsnXo2i>;n#afbTz6msZJJY0V|o&!B7zIOOQTxLa6YJ6}q>aa;skN8owWX|M8 zvWanR-iyq!n{&}BPBGur2XnlC@U?r?jA?lC=c30?NuhNwl4ZMqhX&byRQoC_h6sqV zbnodV5ki|NzL@%TFt_Z>7W3nQYv8v;l8VInRutwoAC294H1tYReT&WC=MmwctPW~J zAg~J{6=n}op?bwIC;)}RuQjgAfG8xFV!ToO7774(5D<`r;{mylFk3hh13hPj;{~~Y z0Vf7#W@-8#;`leAK%QS7HwGSbDKH548+m>s21NgIVZ7Xm%D;SNGUY0W6+z*&KpajD zR9D4=ct;VG+CGAszw!T^oeKO{x%=+`Mg{mGAQj+-QUL(Ms!1iFjWWvD%{GKzg>iTJ z2MLaq)u7-l?T_S-R1}puc~BEQ)FTMArDf?o;SA_pxd?NX#k^el{^~Ua^`*Bh;dSt+ z!Hz|j!S997XXL&rEcDvUyT`dKe%bJPx8}__eSV2A;<1%`FMJ%OQ&`O%_m6jHu*nW~y!HVZi~*`s=f{{OB}= zX${?I>ux>9W{di5^E0ZIkFU6FI+n8D)JGwf+V`L^00P`S1WJLD`=gWOgA2n2b3#kB zj;D8Qw=xS~nISiZtLZCR|FbjuO#~pe4d>$T7?!ju?#gJTk92ES;E^9qJ)BwiILYf6 z?DW&%o?`7F(>UKco$>%kpUVWj)AXk69m{W^e^FqhKz4Ws>*SN;9~l*Wc4;1>c?HZB zXDx>(IhcjU16EjjeXHo71^~%hxUO<6?K12FLXl+vf=#={yD}Vi_lOErEjQAmi0j12 zevM;(oW}faBO!a!+#@xA;I4}J3H_X*y|$q3aI_XH4Wy`s02Ys&wP^&zQQr8@ORZ-M zYaYkAP(7c~dBY)a+FxA(rOiry6H*)I%1g)3uL@2`FQw}jRP}(}sw-l%sb0cD6yAJ_ r2-i3~0je9q%f6?$8Q5s$wbp$)patjO+In`|M9}9?zC#sRk(K`fgN(~! literal 0 HcmV?d00001 From 8d8880ec34c5e5d605d6abe99b61f8a8414044ac Mon Sep 17 00:00:00 2001 From: Chinthaka Jayatilake <37581983+ChinthakaJ98@users.noreply.github.com> Date: Tue, 3 Oct 2023 21:41:59 +0530 Subject: [PATCH 8/9] Resolving review comments Co-authored-by: Janak Amarasena --- .../identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java index cd7cea3ca3f..d12851d8bfa 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java @@ -2197,7 +2197,7 @@ public Object[][] provideHandleRequestObjectData() throws Exception { {true, SerializationUtils.clone(oAuth2Parameters), claims1, "Test override claims from request object."}, {true, SerializationUtils.clone(oAuth2Parameters), defaultClaims, - "Test ignore claims outside request object."}, // no overridable claims sent in the req obj + "Test ignore claims outside request object."}, // No overridable claims sent in the req obj. {false, SerializationUtils.clone(oAuth2Parameters), defaultClaims, "Test request without request object."} }; From d04102128137314d9c789e0d4a4bf9921b1bff01 Mon Sep 17 00:00:00 2001 From: Chinthaka Jayatilake <37581983+ChinthakaJ98@users.noreply.github.com> Date: Wed, 4 Oct 2023 13:44:20 +0530 Subject: [PATCH 9/9] Resolving review comments --- .../oauth/endpoint/authz/OAuth2AuthzEndpoint.java | 6 +++--- .../oauth/endpoint/par/OAuth2ParEndpoint.java | 12 +++++++++++- .../endpoint/authz/OAuth2AuthzEndpointTest.java | 6 ++++-- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java index f0dca0bcd92..a2a5a3b9136 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java @@ -2489,8 +2489,8 @@ private void handleOIDCRequestObject(OAuthMessage oAuthMessage, OAuthAuthzReques } else if (isRequestParameter(oauthRequest)) { requestObjValue = oauthRequest.getParam(REQUEST); } - // Mandate request object for FAPI requests - // https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) + /* Mandate request object for FAPI requests. + https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) */ if (isFapiConformant(oAuthMessage.getClientId())) { if (requestObjValue == null) { throw new InvalidRequestException("Request Object is mandatory for FAPI Conformant Applications.", @@ -4198,8 +4198,8 @@ private void addUserAttributesToCache(SessionDataCacheEntry sessionDataCacheEntr DeviceAuthorizationGrantCache.getInstance().addToCache(cacheKey, cacheEntry); } - private boolean isFapiConformant(String clientId) throws InvalidRequestException { + try { return OAuth2Util.isFapiConformantApp(clientId); } catch (IdentityOAuth2Exception e) { diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java index f94358ff930..0e1d83e1ce7 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/par/OAuth2ParEndpoint.java @@ -35,6 +35,7 @@ import org.wso2.carbon.identity.oauth.par.exceptions.ParClientException; import org.wso2.carbon.identity.oauth.par.exceptions.ParCoreException; import org.wso2.carbon.identity.oauth.par.model.ParAuthData; +import org.wso2.carbon.identity.oauth2.IdentityOAuth2Exception; import org.wso2.carbon.identity.oauth2.RequestObjectException; import org.wso2.carbon.identity.oauth2.bean.OAuthClientAuthnContext; import org.wso2.carbon.identity.oauth2.dto.OAuth2ClientValidationResponseDTO; @@ -270,7 +271,7 @@ private void validateRequestObject(OAuthAuthzRequest oAuthAuthzRequest) throws P throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.INVALID_REQUEST_OBJECT); } - } else if (isFAPIConformantApp(oAuthAuthzRequest.getClientId())) { + } else if (isFapiConformant(oAuthAuthzRequest.getClientId())) { /* Mandate request object for FAPI requests https://openid.net/specs/openid-financial-api-part-2-1_0.html#authorization-server (5.2.2-1) */ throw new ParClientException(OAuth2ErrorCodes.INVALID_REQUEST, ParConstants.REQUEST_OBJECT_MISSING); @@ -283,4 +284,13 @@ private void validateRequestObject(OAuthAuthzRequest oAuthAuthzRequest) throws P throw new ParClientException(e.getErrorCode(), e.getMessage(), e); } } + + private boolean isFapiConformant(String clientId) throws ParClientException { + + try { + return OAuth2Util.isFapiConformantApp(clientId); + } catch (IdentityOAuth2Exception e) { + throw new ParClientException(e.getMessage(), e.getErrorCode()); + } + } } diff --git a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java index d12851d8bfa..bd970f85a7a 100644 --- a/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java +++ b/components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/authz/OAuth2AuthzEndpointTest.java @@ -329,6 +329,8 @@ public class OAuth2AuthzEndpointTest extends TestOAuthEndpointBase { private static final String SP_NAME = "Name"; private static final String STATE = "JEZGpTb8IF"; private static final String OIDC_DIALECT = "http://wso2.org/oidc/claim"; + private static final int MILLISECONDS_PER_SECOND = 1000; + private static final int TIME_MARGIN_IN_SECONDS = 3000; private OAuth2AuthzEndpoint oAuth2AuthzEndpoint; private Object authzEndpointObject; @@ -2184,8 +2186,8 @@ public Object[][] provideHandleRequestObjectData() throws Exception { Map defaultClaims = new HashMap<>(); defaultClaims.put(OAuthConstants.OAuth20Params.REDIRECT_URI, TestConstants.CALLBACK); - defaultClaims.put(NBF, System.currentTimeMillis() / 1000); - defaultClaims.put(EXP, System.currentTimeMillis() / 1000 + 3000); + defaultClaims.put(NBF, System.currentTimeMillis() / MILLISECONDS_PER_SECOND); + defaultClaims.put(EXP, System.currentTimeMillis() / MILLISECONDS_PER_SECOND + TIME_MARGIN_IN_SECONDS); defaultClaims.put(OAuthConstants.OAuth20Params.SCOPE, TestConstants.SCOPE_STRING); Map claims1 = new HashMap<>(defaultClaims);