BB2-4781: Revoke prior tokens on new auth flow#1605
Conversation
There was a problem hiding this comment.
Thought there would need to be changes to this file, but in the ended it wound up just being using the HTTPStatus library rather than plain integer status codes.
There was a problem hiding this comment.
Ya I feel like using the HTTPStatus library should be a standard going forward
jimmyfagan
left a comment
jimmyfagan
left a comment
There was a problem hiding this comment.
Left a few comments, but more generally, we might want to review the mural again to make sure we're on the right track regarding whether this should be for v2 or for v2 and v3, and regarding whether this should take effect with the submission of the permission screen, or with the POST token call. I would think this should probably be for v2 and v3, and I would also think that it should happen on the permission screen rather than the post, but we could probably check to make sure that covers all of the cases we talked through previously.
|
|
||
| for access_token in prior_access_tokens: | ||
| try: | ||
| refresh_token = get_refresh_token_model().objects.get(access_token=access_token.id) |
There was a problem hiding this comment.
(nit) We already get the RefreshToken model above and should be able to reuse it here.
| access_token.expires = timezone.now() | ||
| access_token.save() |
There was a problem hiding this comment.
Do we want to match the above and still only do this if expires > now? Might be able to set the access_token.expires before trying to get the refresh token to avoid having to set it in two spots as well.
| return all(ord(char) <= 383 for char in text) and bool(text) | ||
|
|
||
|
|
||
| def revoke_prior_tokens_for_user_and_app_if_they_exist(user_id: int, app_id: int) -> None: |
There was a problem hiding this comment.
A more general comment, our method of doing this elsewhere is by using remove_application_user_pair_tokens_data_access. Is there much materially different about the two functions that justifies keeping both, or could we reuse the existing function and get rid of this altogether? They also seem to be utilized in different stages of the process, but getting closer to the existing behavior of remove_application_user_pair_tokens_data_access might have a little more precedent (I think it's during the permission screen processing rather than on POST token, since that's when the user's new preferences should take effect).
There was a problem hiding this comment.
It looks like remove_application_user_pair_tokens_data_access deletes the data_access_grant as well as the token records. The function I added just renders the prior tokens unusable, but does not delete them. The benefit there is we can see how many tokens a beneficiary may have had, though i'm sure how big of a benefit that really is. We certainly can use remove_application_user_pair_tokens_data_access instead if we prefer to delete the prior tokens and access grant, and then create a new access grant as well on subsequent authorizations.
I initially implemented it for all versions, and at the end added a check for just v3. The reason for that was AC #2 and 3 on the ticket. Would be easy to do it for all versions, the mural (section TOKENS 4/2026), does not mention anything about specific versions, so I will change it back to be for all versions? 
The mural also mentions that we should only delete old tokens if scopes are removed in a subsequent auth, but that is not in the ticket. Will raise these as an 11th at sync today. Shouldn't be difficult to remove prior tokens when they get to the permissions screen rather than after clicking connect if that's what we want. |
…w from post of TokenView. Revoke all previous tokens as the newest one will not have been created yet at that point
3 participants
JIRA Ticket:
BB2-4781
What Does This PR Do?
This change ensures that if there are any previously existing access tokens or refresh tokens for an user_id/app_id pair after going through a new v3 auth flow, that there are specific DB updates that take place upon a new auth flow for the same user_id/app_id pair:
oauth2_provider_accesstokenrecord has an expires value that is greater than the current time, mark the expiresoauth2_provider_refreshtokenrecord (there won't be if there is a CAN flow), associated with one of those previously existing oauth2_provider_accesstoken records, mark the access_token_id column to null, and the revoked column to the current time so the token can no longer be usedauthorization_dataaccessgrantrecords are handledWhat Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
Validation
select * from authorization_dataaccessgrant where beneficiary_id = {{user_id}} order by id descselect * from oauth2_provider_accesstoken where user_id = {{user_id}} order by id descselect * from oauth2_provider_refreshtoken where user_id = {{user_id}} order by id descWhat Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval.
Any Migrations?
etc)