Skip to content

Conversation

@JamesDemeryNava
Copy link
Contributor

…ecords are not deleted when a user has already authorized, and clicks 'Cancel' on the permissions screen

JIRA Ticket:
BB2-4270

What Does This PR Do?

Ensures that when a user who has already granted access to an app is on the permission screen, and clicks 'Cancel', that the data_access_grant, refresh_token, and access_token associated with that user are not deleted.

What Should Reviewers Watch For?

  • Have we preserved how deleting data_access_grant, refresh_token, and access_token works when the user chooses to not share demographic information>
  • Do the new/modified unit tests adequately cover this change?

If you're reviewing this PR, please check for these things in particular:

Validation

  • Ensure all unit/integration tests pass
  • Go through the auth flow with a user (ideally one you have not logged in with before), note their user_id. Run the following queries and ensure they return a result:
    • select * from authorization_dataaccessgrant where beneficiary_id ={{user_id from auth flow}} and application_id = 1; (Note: Use application_id = 1 if using TestApp)
    • select * from oauth2_provider_accesstoken where user_id = {{user_id from auth flow}} and application_id = 1 order by created desc;
    • select * from oauth2_provider_refreshtoken where user_id = {{user_id from auth flow}} and application_id = 1 order by created desc;
  • Then, go through the auth flow again, but on the permissions screen, click Cancel.
  • Run the queries above again and ensure they still return a result

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

  • Adds any new software dependencies
  • Modifies any security controls
  • Adds new transmission or storage of data
  • Any other changes that could possibly affect security?
  • Yes, one or more of the above security implications apply. This PR must not be merged without the ISSO or team
    security engineer's approval.

Any Migrations?

  • Yes, there are migrations
    • The migrations should be run PRIOR to the code being deployed
    • The migrations should be run AFTER the code is deployed
    • There is a more complicated migration plan (downtime,
      etc)
  • No migrations

…ecords are not deleted when a user has already authorized, and clicks 'Cancel' on the permissions screen
@jimmyfagan jimmyfagan self-assigned this Dec 8, 2025
@JamesDemeryNava
Copy link
Contributor Author

Confirmed this also works with coverage only apps

@jadudm
Copy link
Contributor

jadudm commented Dec 9, 2025

I also checked that none of the values changed when cancelling. The timestamps remain consistent; the tables/rows indicated suggest nothing changes.

Copy link
Contributor

@jadudm jadudm left a comment

Choose a reason for hiding this comment

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

LGTM. The mocking in the test is a nice bit of work, and helps assert the right thing is happening.

@JamesDemeryNava JamesDemeryNava merged commit c247b7a into master Dec 9, 2025
8 checks passed
@JamesDemeryNava JamesDemeryNava deleted the jamesdemery/BB2-4270-cancel-button-prevent-revoke branch December 9, 2025 19:01
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.

4 participants