Add changes required for FAPI 2.0 compliance #2963
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
imesh94
changed the title
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
|
|
||
| return authorizationResponseIssParameterSupported; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| public void setAuthorizationResponseIssParameterSupported(Boolean authorizationResponseIssParameterSupported) { | |
| log.debug("Setting authorizationResponseIssParameterSupported to: " + authorizationResponseIssParameterSupported); |
| configMap.put(DiscoveryConstants.AUTHORIZATION_DETAILS_TYPES_SUPPORTED, | ||
| this.authorizationDetailsTypesSupported); | ||
| configMap.put(DiscoveryConstants.AUTHORIZATION_RESPONSE_ISS_PARAMETER_SUPPORTED, | ||
| this.authorizationResponseIssParameterSupported); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| this.authorizationResponseIssParameterSupported); | |
| this.authorizationResponseIssParameterSupported); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Configuration map created with " + configMap.size() + " entries"); | |
| } |
Comment on lines
160
to
161
| providerConfig | ||
| .setAuthorizationDetailsTypesSupported(authorizationDetailTypes.stream().toArray(String[]::new)); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| providerConfig | |
| .setAuthorizationDetailsTypesSupported(authorizationDetailTypes.stream().toArray(String[]::new)); | |
| providerConfig | |
| .setAuthorizationDetailsTypesSupported(authorizationDetailTypes.stream().toArray(String[]::new)); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Setting authorization details types supported: " + authorizationDetailTypes.size() + " types"); | |
| } |
Comment on lines
+163
to
+165
| if (OAuth2Util.isFapi2Enabled()) { | ||
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 4
Suggested change
| if (OAuth2Util.isFapi2Enabled()) { | |
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | |
| } | |
| if (OAuth2Util.isFapi2Enabled()) { | |
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | |
| log.info("FAPI 2.0 enabled: Setting authorization response ISS parameter support to true"); | |
| } |
Comment on lines
+327
to
328
| if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | ||
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(oauthParams.get(RESPONSE_TYPE), |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 5
Suggested change
| if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(oauthParams.get(RESPONSE_TYPE), | |
| if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| log.info("Processing FAPI 1.0 conformant authorization request for client: " + oAuthAuthzRequest.getClientId()); | |
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(oauthParams.get(RESPONSE_TYPE), |
Comment on lines
+406
to
+407
| } else if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | ||
| /* Mandate request object for FAPI 1.0 Advanced requests |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 6
Suggested change
| } else if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| /* Mandate request object for FAPI 1.0 Advanced requests | |
| } else if (isFapiConformant(oAuthAuthzRequest.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| log.warn("Request object is missing for FAPI 1.0 Advanced client: " + oAuthAuthzRequest.getClientId()); | |
| /* Mandate request object for FAPI 1.0 Advanced requests |
Comment on lines
2398
to
2401
|
|
||
| if (isFapiConformant(params.getClientId())) { | ||
| if (isFapiConformant(params.getClientId()) && OAuth2Util.isFapi1Enabled()) { | ||
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(params.getResponseType(), params.getResponseMode()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 7
Suggested change
| if (isFapiConformant(params.getClientId())) { | |
| if (isFapiConformant(params.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(params.getResponseType(), params.getResponseMode()); | |
| } | |
| if (isFapiConformant(params.getClientId()) && OAuth2Util.isFapi1Enabled()) { | |
| log.debug("FAPI 1.0 validation enabled for client: " + params.getClientId()); | |
| EndpointUtil.validateFAPIAllowedResponseTypeAndMode(params.getResponseType(), params.getResponseMode()); | |
| } |
Comment on lines
+2747
to
+2756
| if (isFapiConformant(oAuthMessage.getClientId()) && OAuth2Util.isFapi2Enabled()) { | ||
| /* For FAPI 2.0 compliance, state parameter sent in the par request should be returned in the | ||
| authorization response */ | ||
| Object parState = oAuthMessage.getRequest().getAttribute(ParConstants.PAR_STATE); | ||
| if (parState != null) { | ||
| params.setState(parState.toString()); | ||
| } | ||
| } else { | ||
| params.setState(oauthRequest.getState()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 8
Suggested change
| if (isFapiConformant(oAuthMessage.getClientId()) && OAuth2Util.isFapi2Enabled()) { | |
| /* For FAPI 2.0 compliance, state parameter sent in the par request should be returned in the | |
| authorization response */ | |
| Object parState = oAuthMessage.getRequest().getAttribute(ParConstants.PAR_STATE); | |
| if (parState != null) { | |
| params.setState(parState.toString()); | |
| } | |
| } else { | |
| params.setState(oauthRequest.getState()); | |
| } | |
| if (isFapiConformant(oAuthMessage.getClientId()) && OAuth2Util.isFapi2Enabled()) { | |
| log.debug("FAPI 2.0 compliance check for client: " + oAuthMessage.getClientId()); | |
| /* For FAPI 2.0 compliance, state parameter sent in the par request should be returned in the | |
| authorization response */ | |
| Object parState = oAuthMessage.getRequest().getAttribute(ParConstants.PAR_STATE); | |
| if (parState != null) { | |
| log.debug("Using PAR state parameter for FAPI 2.0 compliant client"); | |
| params.setState(parState.toString()); | |
| } | |
| } else { | |
| params.setState(oauthRequest.getState()); | |
| } |
Comment on lines
+1011
to
+1012
| @Test | ||
| public void testParseJsonTokenRequest() throws Exception { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 9
Suggested change
| @Test | |
| public void testParseJsonTokenRequest() throws Exception { | |
| @Test | |
| public void testParseJsonTokenRequest() throws Exception { | |
| log.info("Testing parseJsonTokenRequest method"); |
Comment on lines
+1047
to
+1048
| Assert.fail("Expected TokenEndpointBadRequestException was not thrown"); | ||
| } catch (TokenEndpointBadRequestException e) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 10
Suggested change
| Assert.fail("Expected TokenEndpointBadRequestException was not thrown"); | |
| } catch (TokenEndpointBadRequestException e) { | |
| } catch (TokenEndpointBadRequestException e) { | |
| log.debug("Expected exception caught for malformed JSON: " + e.getMessage()); | |
| assertTrue(e.getMessage().contains("Malformed or unsupported request payload")); |
Comment on lines
55
to
56
| params = ParAuthServiceComponentDataHolder.getInstance().getParAuthService() | ||
| .retrieveParams(uuid, request.getParameter(OAuthConstants.OAuth20Params.CLIENT_ID)); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 11
Suggested change
| params = ParAuthServiceComponentDataHolder.getInstance().getParAuthService() | |
| .retrieveParams(uuid, request.getParameter(OAuthConstants.OAuth20Params.CLIENT_ID)); | |
| params = ParAuthServiceComponentDataHolder.getInstance().getParAuthService() | |
| .retrieveParams(uuid, request.getParameter(OAuthConstants.OAuth20Params.CLIENT_ID)); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Successfully retrieved PAR parameters for request UUID: " + uuid); | |
| } |
Comment on lines
60
to
61
| } catch (ParClientException e) { | ||
| throw new ParAuthFailureException(e.getErrorCode(), e.getMessage(), e); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 12
Suggested change
| } catch (ParClientException e) { | |
| throw new ParAuthFailureException(e.getErrorCode(), e.getMessage(), e); | |
| } catch (ParClientException e) { | |
| log.error("PAR client error occurred while retrieving parameters: " + e.getMessage()); | |
| throw new ParAuthFailureException(e.getErrorCode(), e.getMessage(), e); |
Comment on lines
+3734
to
+3737
| useEntityIDAsIssuer = Boolean.parseBoolean(openIDConnectConfigElem.getFirstChildWithName( | ||
| getQNameWithIdentityNS(ConfigElements.OPENID_CONNECT_USE_ENTITY_ID_AS_ISSUER)). | ||
| getText().trim()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 13
Suggested change
| useEntityIDAsIssuer = Boolean.parseBoolean(openIDConnectConfigElem.getFirstChildWithName( | |
| getQNameWithIdentityNS(ConfigElements.OPENID_CONNECT_USE_ENTITY_ID_AS_ISSUER)). | |
| getText().trim()); | |
| } | |
| useEntityIDAsIssuer = Boolean.parseBoolean(openIDConnectConfigElem.getFirstChildWithName( | |
| getQNameWithIdentityNS(ConfigElements.OPENID_CONNECT_USE_ENTITY_ID_AS_ISSUER)). | |
| getText().trim()); | |
| log.info("UseEntityIdAsIssuer configuration loaded: " + useEntityIDAsIssuer); | |
| } |
Comment on lines
+3807
to
+3810
| if (fapiElem.getFirstChildWithName(getQNameWithIdentityNS(ConfigElements.FAPI_VERSION)) != null) { | ||
| fapiVersion = fapiElem.getFirstChildWithName(getQNameWithIdentityNS( | ||
| ConfigElements.FAPI_VERSION)).getText().trim(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 14
Suggested change
| if (fapiElem.getFirstChildWithName(getQNameWithIdentityNS(ConfigElements.FAPI_VERSION)) != null) { | |
| fapiVersion = fapiElem.getFirstChildWithName(getQNameWithIdentityNS( | |
| ConfigElements.FAPI_VERSION)).getText().trim(); | |
| } | |
| if (fapiElem.getFirstChildWithName(getQNameWithIdentityNS(ConfigElements.FAPI_VERSION)) != null) { | |
| fapiVersion = fapiElem.getFirstChildWithName(getQNameWithIdentityNS( | |
| ConfigElements.FAPI_VERSION)).getText().trim(); | |
| log.info("FAPI version configured: " + fapiVersion); | |
| } |
Comment on lines
638
to
639
| boolean isAuthorizedClient = authzHandler.isAuthorizedClient(authzReqMsgCtx); | ||
| if (!isAuthorizedClient) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 15
Suggested change
| boolean isAuthorizedClient = authzHandler.isAuthorizedClient(authzReqMsgCtx); | |
| if (!isAuthorizedClient) { | |
| boolean isAuthorizedClient = authzHandler.isAuthorizedClient(authzReqMsgCtx); | |
| if (!isAuthorizedClient) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Client is not authorized to use the requested authorization grant type. ClientId: " + authzReqDTO.getConsumerKey()); | |
| } |
Comment on lines
+647
to
+648
| } catch (InvalidOAuthClientException e) { | ||
| throw new IdentityOAuth2Exception("Error occurred while retrieving the fapi conformance of the " + |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 16
Suggested change
| } catch (InvalidOAuthClientException e) { | |
| throw new IdentityOAuth2Exception("Error occurred while retrieving the fapi conformance of the " + | |
| } catch (InvalidOAuthClientException e) { | |
| log.error("Error occurred while retrieving the FAPI conformance status for client: " + authzReqDTO.getConsumerKey(), e.getMessage()); | |
| throw new IdentityOAuth2Exception("Error occurred while retrieving the fapi conformance of the " + |
Comment on lines
+134
to
+136
| try { | ||
| if (OAuth2Util.isFapiConformantApp(authorizationResponseDTO.getClientId()) && | ||
| OAuth2Util.isFapi2Enabled()) { |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 17
Suggested change
| try { | |
| if (OAuth2Util.isFapiConformantApp(authorizationResponseDTO.getClientId()) && | |
| OAuth2Util.isFapi2Enabled()) { | |
| try { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Checking FAPI conformance for client: " + authorizationResponseDTO.getClientId()); | |
| } | |
| if (OAuth2Util.isFapiConformantApp(authorizationResponseDTO.getClientId()) && |
Comment on lines
+141
to
+143
| } catch (IdentityOAuth2Exception | InvalidOAuthClientException e) { | ||
| throw new OAuthRuntimeException("Error occurred while retrieving application details. ", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 18
Suggested change
| } catch (IdentityOAuth2Exception | InvalidOAuthClientException e) { | |
| throw new OAuthRuntimeException("Error occurred while retrieving application details. ", e); | |
| } | |
| } catch (IdentityOAuth2Exception | InvalidOAuthClientException e) { | |
| log.error("Error occurred while retrieving application details for client: " + authorizationResponseDTO.getClientId()); | |
| throw new OAuthRuntimeException("Error occurred while retrieving application details. ", e); |
Comment on lines
+4775
to
+4777
| if (OAuthServerConfiguration.getInstance().getIsUseEntityIDAsIssuerEnabled() || isFapi2Enabled()) { | ||
| return getResidentIdpEntityId(tenantDomain); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 19
Suggested change
| if (OAuthServerConfiguration.getInstance().getIsUseEntityIDAsIssuerEnabled() || isFapi2Enabled()) { | |
| return getResidentIdpEntityId(tenantDomain); | |
| } | |
| if (OAuthServerConfiguration.getInstance().getIsUseEntityIDAsIssuerEnabled() || isFapi2Enabled()) { | |
| String entityId = getResidentIdpEntityId(tenantDomain); | |
| if (log.isDebugEnabled()) { | |
| log.debug("Using entity ID as issuer for tenant domain: " + tenantDomain + ", entity ID: " + entityId); | |
| } | |
| return entityId; | |
| } |
Comment on lines
+5783
to
+5787
| */ | ||
| public static boolean isFapi2Enabled() { | ||
|
|
||
| return OAuthConstants.FAPIVersions.FAPI2.equals(OAuthServerConfiguration.getInstance().getFapiVersion()); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 20
Suggested change
| */ | |
| public static boolean isFapi2Enabled() { | |
| return OAuthConstants.FAPIVersions.FAPI2.equals(OAuthServerConfiguration.getInstance().getFapiVersion()); | |
| } | |
| public static boolean isFapi2Enabled() { | |
| boolean isFapi2 = OAuthConstants.FAPIVersions.FAPI2.equals(OAuthServerConfiguration.getInstance().getFapiVersion()); | |
| if (log.isDebugEnabled()) { | |
| log.debug("FAPI 2.0 enabled status: " + isFapi2); | |
| } | |
| return isFapi2; | |
| } |
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
There was a problem hiding this comment.
Pull Request Overview
This PR implements changes required for FAPI 2.0 compliance in WSO2 IS-7.1, adding support for FAPI version configuration and related functionality.
Key changes include:
- Added FAPI version configuration support with new helper methods to distinguish between FAPI 1.0 and 2.0
- Implemented FAPI 2.0 specific behaviors including issuer parameter in authorization responses and state parameter prioritization from PAR requests
- Updated test cases to accommodate the new FAPI version configurations
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| OAuth2Util.java | Added FAPI version check methods and updated issuer logic to support entity ID usage |
| QueryResponseModeProvider.java | Added issuer parameter to authorization responses for FAPI 2.0 compliance |
| AuthorizationHandlerManager.java | Updated error codes for FAPI 2.0 compliant applications |
| OAuthServerConfiguration.java | Added FAPI version and entity ID issuer configuration support |
| ParRequestBuilder.java | Added state parameter extraction from PAR requests |
| AuthzUtil.java | Updated FAPI checks to be version-specific and prioritized PAR state parameter |
| OAuth2ParEndpoint.java | Made FAPI request object validation specific to FAPI 1.0 |
| ProviderConfigBuilder.java | Added authorization response issuer parameter support for FAPI 2.0 |
| OIDProviderConfigResponse.java | Added property for authorization response issuer parameter |
| DiscoveryConstants.java | Added constant for authorization response issuer parameter |
| Various test files | Updated test configurations and mocks to support new FAPI version settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (parState != null) { | ||
| params.setState(parState.toString()); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
[nitpick] The else block only contains a single statement. Consider moving this outside the conditional block and only setting the PAR state when the condition is met, to improve code readability.
Suggested change
| } else { | |
| } | |
| if (params.getState() == null) { |
Comment on lines
+162
to
166
| // `authorization_response_iss` should be true for FAPI 2.0 compliance. | ||
| if (OAuth2Util.isFapi2Enabled()) { | ||
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | ||
| } | ||
| } |
There was a problem hiding this comment.
The FAPI 2.0 check is placed inside the authorization details types condition. This means the authorization response issuer parameter will only be set if authorization detail types are configured. Consider moving this check outside the authorization details condition to ensure it's always evaluated for FAPI 2.0.
Suggested change
| // `authorization_response_iss` should be true for FAPI 2.0 compliance. | |
| if (OAuth2Util.isFapi2Enabled()) { | |
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | |
| } | |
| } | |
| } | |
| // `authorization_response_iss` should be true for FAPI 2.0 compliance. | |
| if (OAuth2Util.isFapi2Enabled()) { | |
| providerConfig.setAuthorizationResponseIssParameterSupported(Boolean.TRUE); | |
| } |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (23.21%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2963 +/- ##
============================================
- Coverage 58.54% 58.38% -0.16%
+ Complexity 9349 9244 -105
============================================
Files 669 669
Lines 52226 51627 -599
Branches 11835 11734 -101
============================================
- Hits 30577 30145 -432
+ Misses 17495 17350 -145
+ Partials 4154 4132 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add changes required for FAPI 2.0 compliance
This PR contains the changes required to make WSO2 IS-7.1 FAPI 2.0 conformant.
Related Issues: wso2/product-is#25863
Changes Introduced
New Configurations
identity.xml