Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix consent page for mandatory attributes #2685

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Thumimku
Copy link
Contributor

Proposed changes in this pull request

Issue: wso2/product-is#21888

We have two claim categories: Required and Mandatory. These claims can be configured in the User Attributes section. Before filtering the claims for pre-consent, the claims are returned by the framework. The requested scopes are then filtered, and the consent page is displayed.

Over time, we implemented several fixes that inadvertently disrupted this flow.

In PR #1698, we introduced a change to pass the requested scopes first to the framework. The framework would then filter the claims and return both Required and Mandatory claims. However, this approach had a flaw where essential claims were missed.

To address this, in PR #2405, we reverted the behavior to stop passing scopes to the framework, restoring the previous logic while filtering essential claims. Unfortunately, this introduced another issue.

As mentioned in Issue #21888, even if the requested scopes do not include mandatory attributes, these attributes still appear on the consent page. This happens because filtering was removed. Although this is an edge case (e.g., why would an attribute be configured as mandatory but not requested by the client?), it still needs to be addressed to ensure consent is only provided for requested attributes.

This PR resolves the issue, ensuring that the consent page reflects only the requested scopes and associated attributes, maintaining expected behavior.

I tested all three issues to avoid any regressions.

  1. When using OIDC scopes the claims displayed in the consent screen is not inline with the scope wso2/product-is#11331
    Consent should be given only requested attributes.
1.mp4
  1. Pre-consent handling fails to respect essential claims specified in the claims parameter in the oauth2/authorize call wso2/product-is#19817
    Essential claims request and response should work as expected with proper consent screen
2.mp4
  1. OIDC Requests Consent for All Mandatory Attributes Instead of Requested Scopes wso2/product-is#21888
3.mp4

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12906695623

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 16.66667% with 10 lines in your changes missing coverage. Please review.

Project coverage is 55.42%. Comparing base (0c03aa3) to head (fe8d49f).
Report is 28 commits behind head on master.

Files with missing lines Patch % Lines
...tity/oauth/endpoint/authz/OAuth2AuthzEndpoint.java 16.66% 6 Missing and 4 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2685      +/-   ##
============================================
- Coverage     55.67%   55.42%   -0.26%     
+ Complexity     8484     8378     -106     
============================================
  Files           643      631      -12     
  Lines         49589    49544      -45     
  Branches       9395     9549     +154     
============================================
- Hits          27611    27458     -153     
- Misses        18054    18166     +112     
+ Partials       3924     3920       -4     
Flag Coverage Δ
unit 38.72% <16.66%> (-0.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12906695623
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/12906695623

@Thumimku Thumimku merged commit d937c36 into wso2-extensions:master Jan 23, 2025
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants