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.

Sign up for GitHub

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

Jump to bottom

Validating BNR permission for consent revocation #66

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Validating BNR permission for consent revocation #66

merged 6 commits into from
Dec 11, 2024

Conversation

anjuchamantha
Copy link
Contributor

@anjuchamantha anjuchamantha commented Dec 9, 2024 •
edited
Loading

Validating BNR permission for consent revocation

With this PR, the following validation is added to check when a BNR(Business Nominated Representative) user tries to revoke a consent.

  • For all the active accounts in the consent, if the BNR user has AUTHORIZE or REVOKE permission, then the user can revoke that consent.
    (i.e. For any active account in consent mappings, if the user does not have AUTHORIZE or REVOKE permission, he/she cannot revoke the consent)

Also a new can_revoke (boolean) variable is sent in the /admin/search response, which is used in consent manager UI to decide showing the Stop Sharing button.
(i.e. BNR users who does not meet the above validation will not see a Stop Sharing button.

Issue:

Applicable Labels: OB3 CDS Toolkit

Development Checklist

  1. Built complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verify the PR does't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Documented test scenarios(link available in guides).
  3. Written automation tests (link available in guides).
  4. Verified tests in multiple database environments (if applicable).
  5. Verified tests in multiple deployed specifications (if applicable).
  6. Tested with OBBI enabled (if applicable).
  7. Tested with specification regulatory conformance suites (if applicable).

Automation Test Details

Test Suite Test Script IDs
Integration Suite TCXXXXX, TCXXXX

Conformance Tests Details

Test Suite Name Test Suite Version Scenarios Result
Security Suite VX.X Foo, Bar Passed

Resources

Knowledge Base: https://sites.google.com/wso2.com/open-banking/

Guides: https://sites.google.com/wso2.com/open-banking/developer-guides

@anjuchamantha anjuchamantha added the Ready-to-Build Adding this label will trigger a new build job. label Dec 9, 2024
@anjuchamantha anjuchamantha changed the title [WIP] Validating BNR permission for consent revocation Validating BNR permission for consent revocation Dec 10, 2024
imesh94
imesh94 reviewed Dec 10, 2024
View reviewed changes
...main/java/org/wso2/openbanking/cds/consent/extensions/admin/impl/CDSConsentAdminHandler.java Outdated
AccountMetadataServiceImpl accountMetadataService = AccountMetadataServiceImpl.getInstance();

/*
* For all the active accounts, if the user has AUTHORIZE or REVOKE permission for BNR, then the user can
Copy link
Contributor

Choose a reason for hiding this comment

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

User can revoke the consent only if he has authorize permission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with e7670fa

@anjuchamantha anjuchamantha added Ready-to-Build Adding this label will trigger a new build job. and removed Ready-to-Build Adding this label will trigger a new build job. labels Dec 10, 2024
aka4rKO
aka4rKO reviewed Dec 11, 2024
View reviewed changes
...main/java/org/wso2/openbanking/cds/consent/extensions/admin/impl/CDSConsentAdminHandler.java Outdated
ArrayList<String> userIDs = (ArrayList<String>) consentAdminData.getQueryParams()
.get(CDSConsentExtensionConstants.USER_ID_KEY_NAME);
// userIDs can be null or empty when the request comes from a CustomerCareOfficer
if (userIDs != null && !userIDs.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

when can there be a list of userIDs? because usually revoke is done by a single user right? If there is a scenario when a list of userIDs can come shall we add that as a comment as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is because consentAdminData.getQueryParams() returns a Map, and and .get(<key>) returns an array list. That doesn't mean that we can have multiple userIDs for revoke. Since ConsentAdminData is a common class we cannot change that behaviour for revoke only.
For this scenario (revoking a consent from consent manager) we only have 1 user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was already a validateAndGetQueryParam method to get the first element from query params. Modified with bc8b123

@anjuchamantha anjuchamantha added Ready-to-Build Adding this label will trigger a new build job. and removed Ready-to-Build Adding this label will trigger a new build job. labels Dec 11, 2024
@aka4rKO aka4rKO merged commit c6c9c0d into wso2:main Dec 11, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imesh94 imesh94 imesh94 left review comments

@aka4rKO aka4rKO aka4rKO approved these changes

Assignees
No one assigned
Labels
Ready-to-Build Adding this label will trigger a new build job.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anjuchamantha @imesh94 @aka4rKO