Skip to content

BB2-4781: Revoke prior tokens on new auth flow #1605

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

Open
JamesDemeryNava wants to merge 12 commits into
base: master
Choose a base branch
from
+163 −34
Open

BB2-4781: Revoke prior tokens on new auth flow #1605

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6e8d9f3
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 21, 2026
599e102
BB2-4781: Revoke prior tokens on new auth flow
JamesDemeryNava May 21, 2026
de1fa4d
Ensure token revocation only happens on v3 auth flows
JamesDemeryNava May 22, 2026
b0ea5ae
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 22, 2026
4f2cffa
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 27, 2026
f570eed
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 27, 2026
8508c52
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 27, 2026
4335eb3
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 28, 2026
e89f80d
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 28, 2026
5a993e1
Move the revocation of prior tokens to form_valid of AuthorizationVie…
JamesDemeryNava May 28, 2026
605a6ce
Merge branch 'master' into jamesdemery/bb2-4781-revoke-prior-tokens-o…
JamesDemeryNava May 29, 2026
f86556e
Address PR feedback, remove no longer needed test
JamesDemeryNava May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 32 additions & 30 deletions apps/dot_ext/tests/test_beneficiary_demographic_scope_changes.py
View file
Open in desktop
Copy link
Copy Markdown
Contributor Author

@JamesDemeryNava JamesDemeryNava May 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@ryan-morosa ryan-morosa May 28, 2026

Choose a reason for hiding this comment

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

Ya I feel like using the HTTPStatus library should be a standard going forward

Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import json
from apps.test import BaseApiTest
from django.core.management import call_command
from django.http import HttpRequest
from django.urls import reverse
from http import HTTPStatus
from unittest import mock

# from oauth2_provider.compat import parse_qs, urlparse
from urllib.parse import parse_qs, urlparse

from django.core.management import call_command
from django.http import HttpRequest
from django.urls import reverse
from oauth2_provider.models import AccessToken, RefreshToken
from rest_framework.test import APIClient
from waffle.testutils import override_switch
from apps.authorization.models import DataAccessGrant, ArchivedDataAccessGrant
from apps.dot_ext.models import ArchivedToken, Application
from http import HTTPStatus
from unittest import mock

from apps.authorization.models import ArchivedDataAccessGrant, DataAccessGrant
from apps.dot_ext.models import Application, ArchivedToken
from apps.test import BaseApiTest


class TestBeneficiaryDemographicScopesChanges(BaseApiTest):
Expand Down Expand Up @@ -127,7 +129,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
)

# Assert auth request was successful
self.assertEqual(status_code, 200)
self.assertEqual(status_code, HTTPStatus.OK)

# Assert scope in response content
self.assertEqual(response_scopes, sorted(APPLICATION_SCOPES_FULL))
Expand All @@ -138,7 +140,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_1.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# ------ TEST #2: Test refresh of token_1
refresh_request_data = {
Expand All @@ -152,7 +154,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
content = json.loads(response.content.decode('utf-8'))

# Assert successful
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# Assert response scopes
response_scopes = sorted(content['scope'].split())
Expand All @@ -166,7 +168,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 1)
Expand Down Expand Up @@ -197,7 +199,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert NO access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_3.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 403)
self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 1)
Expand All @@ -215,7 +217,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Test access to userinfo end point? NO ACCESS!
response = client.get('/v1/connect/userinfo')
content = json.loads(response.content)
self.assertEqual(response.status_code, 401)
self.assertEqual(response.status_code, HTTPStatus.UNAUTHORIZED)
self.assertEqual(content.get('detail', None), 'Authentication credentials were not provided.')

# ------ TEST #5: Test token_1 from TEST #1 token refresh? NO ACCESS!
Expand All @@ -241,7 +243,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
)

# Assert auth request was successful
self.assertEqual(status_code, 200)
self.assertEqual(status_code, HTTPStatus.OK)

# Assert scope in response content
self.assertEqual(response_scopes, sorted(APPLICATION_SCOPES_FULL))
Expand All @@ -252,7 +254,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_6.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# ------ TEST #7: Test token_3 from TEST #3 again. It should still have access, but no permission with status=403.

Expand All @@ -262,7 +264,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Test access to userinfo end point?
response = client.get('/v1/connect/userinfo')
content = json.loads(response.content)
self.assertEqual(response.status_code, 403)
self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN)
self.assertEqual(content.get('detail', None), 'You do not have permission to perform this action.')

# Verify token counts expected.
Expand All @@ -280,15 +282,15 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):

# Perform partial authorization request, with out application getting an access token.
response = self.client.post(reverse('oauth2_provider:authorize'), data=payload)
self.assertEqual(response.status_code, 302)
self.assertEqual(response.status_code, HTTPStatus.FOUND)

# Setup token_3 in APIClient from previous step. It should be removed now?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_3.token)

# Test access to userinfo end point?
response = client.get('/v1/connect/userinfo')
content = json.loads(response.content)
self.assertEqual(response.status_code, 401)
self.assertEqual(response.status_code, HTTPStatus.UNAUTHORIZED)
self.assertEqual(content.get('detail', None), 'Authentication credentials were not provided.')

# Verify token counts expected.
Expand All @@ -309,7 +311,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
)

# Assert auth request was successful
self.assertEqual(status_code, 200)
self.assertEqual(status_code, HTTPStatus.OK)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 1)
Expand All @@ -323,14 +325,14 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_9.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# Beneficiary chooses the DENY button choice on consent page
payload['allow'] = False

# Perform partial authorization request, with out application getting an access token.
response = self.client.post(reverse('oauth2_provider:authorize'), data=payload)
self.assertEqual(response.status_code, 302)
self.assertEqual(response.status_code, HTTPStatus.FOUND)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 1)
Expand All @@ -346,7 +348,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# when the allow parameter is false
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_9.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# BB2-4270: Remove prior active tokens so tests below are not looking for multiple active tokens
# which is an impossible state
Expand All @@ -360,12 +362,12 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
payload['allow'] = True

# Perform authorization request
token_10, refresh_token_10, status_code, response_scopes, access_token_scopes = self._authorize_and_request_token(
payload, application
token_10, refresh_token_10, status_code, response_scopes, access_token_scopes = (
self._authorize_and_request_token(payload, application)
)

# Assert auth request was successful
self.assertEqual(status_code, 200)
self.assertEqual(status_code, HTTPStatus.OK)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 1)
Expand All @@ -379,15 +381,15 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_10.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 200)
self.assertEqual(response.status_code, HTTPStatus.OK)

# Application changes choice to require demographic scopes
application.require_demographic_scopes = False
application.save()

# Perform partial authorization request, with out application getting an access token.
response = self.client.post(reverse('oauth2_provider:authorize'), data=payload)
self.assertEqual(response.status_code, 302)
self.assertEqual(response.status_code, HTTPStatus.FOUND)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 0)
Expand All @@ -400,7 +402,7 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):

# Perform partial authorization request, with out application getting an access token.
response = self.client.post(reverse('oauth2_provider:authorize'), data=payload)
self.assertEqual(response.status_code, 302)
self.assertEqual(response.status_code, HTTPStatus.FOUND)

# Verify token counts expected.
self.assertEqual(AccessToken.objects.count(), 0)
Expand All @@ -414,5 +416,5 @@ def test_bene_demo_scopes_change(self, mock_get_and_update):
# Assert access to userinfo end point?
client.credentials(HTTP_AUTHORIZATION='Bearer ' + token_10.token)
response = client.get('/v1/connect/userinfo')
self.assertEqual(response.status_code, 401)
self.assertEqual(response.status_code, HTTPStatus.UNAUTHORIZED)
self.assertEqual(content.get('detail', None), 'Authentication credentials were not provided.')
100 changes: 98 additions & 2 deletions apps/dot_ext/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
from datetime import timedelta
from unittest.mock import MagicMock, patch

from django.test import TestCase
from django.utils import timezone

from apps.versions import VersionNotMatched
from apps.dot_ext.constants import SUPPORTED_VERSION_TEST_CASES
from apps.dot_ext.utils import get_api_version_number_from_url, validate_latin_extended_string
from apps.dot_ext.utils import (
get_api_version_number_from_url,
revoke_prior_tokens_for_user_and_app_if_they_exist,
validate_latin_extended_string,
)
from apps.versions import VersionNotMatched


class TestDOTUtils(TestCase):
Expand Down Expand Up @@ -33,3 +41,91 @@ def test_latin_extended_failure(self):

for text in invalid_inputs:
assert not validate_latin_extended_string(text)

@patch('apps.dot_ext.utils.get_refresh_token_model')
@patch('apps.dot_ext.utils.get_access_token_model')
def test_revoke_prior_tokens_for_user_and_app_if_they_exist_no_prior_tokens(
self, mock_get_access_token, mock_get_refresh_token
):
"""Confirm that if only one access token is returned, that we never query for refresh tokens"""
mock_access_token_model = MagicMock()
mock_access_token_model.objects.filter.return_value.order_by.return_value = [MagicMock()]
mock_get_access_token.return_value = mock_access_token_model

mock_refresh_token_model = MagicMock()
mock_get_refresh_token.return_value = mock_refresh_token_model

revoke_prior_tokens_for_user_and_app_if_they_exist(1, 1)

mock_get_refresh_token.objects.get.assert_not_called()

@patch('apps.dot_ext.utils.timezone')
@patch('apps.dot_ext.utils.get_refresh_token_model')
@patch('apps.dot_ext.utils.get_access_token_model')
def test_revoke_prior_tokens_for_user_and_app_if_they_exist_prior_tokens_exit(
self, mock_get_access_token, mock_get_refresh_token, mock_timezone
):
"""Confirm that if there are multiple access tokens, the prior ones will have their expires value updated.
Also, any associated refresh tokens will have their access_token_id set to null, and their revoked value
set to the current time (UTC)
"""
mock_timezone.now.return_value = timezone.now()

new_access_token = MagicMock(
id=1, user_id=1, expires=timezone.now() + timedelta(hours=2), created=timezone.now()
)
prior_access_token = MagicMock(
id=2, user_id=1, expires=timezone.now() + timedelta(minutes=30), created=timezone.now() - timedelta(days=10)
)
prior_access_token_two = MagicMock(
id=3, user_id=1, expires=timezone.now() - timedelta(days=20), created=timezone.now() - timedelta(days=20)
)

mock_access_token_model = MagicMock()
mock_access_token_model.objects.filter.return_value.order_by.return_value = [
new_access_token,
prior_access_token,
prior_access_token_two,
]
mock_get_access_token.return_value = mock_access_token_model

refresh_token = MagicMock(access_token_id=2, revoked=None)
mock_refresh_token_model = MagicMock()
mock_refresh_token_model.objects.get.return_value = refresh_token
mock_get_refresh_token.return_value = mock_refresh_token_model

revoke_prior_tokens_for_user_and_app_if_they_exist(1, 10)

prior_access_token.save.assert_called_once()
prior_access_token_two.save.assert_not_called()

assert refresh_token.revoked is not None
assert refresh_token.access_token_id is None
refresh_token.save.assert_called_once()

@patch('apps.dot_ext.utils.timezone')
@patch('apps.dot_ext.utils.get_access_token_model')
def test_revoke_prior_tokens_for_user_and_app_if_they_exist_no_associated_refresh_token(
self, mock_get_access_token, mock_timezone
):
"""Confirm that even if there is no associated refresh_token for an access_token, that the access_token
still has its expires value updated.
"""
mock_timezone.now.return_value = timezone.now()

new_access_token = MagicMock(
id=1, user_id=1, expires=timezone.now() + timedelta(hours=2), created=timezone.now()
)
prior_access_token = MagicMock(
id=2, user_id=1, expires=timezone.now() + timedelta(minutes=30), created=timezone.now() - timedelta(days=10)
)

mock_access_token_model = MagicMock()
mock_access_token_model.objects.filter.return_value.order_by.return_value = [
new_access_token,
prior_access_token,
]
mock_get_access_token.return_value = mock_access_token_model

revoke_prior_tokens_for_user_and_app_if_they_exist(1, 10)
prior_access_token.save.assert_called_once()
48 changes: 47 additions & 1 deletion apps/dot_ext/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@
from django.db import transaction
from django.http import HttpRequest
from django.http.response import JsonResponse
from oauth2_provider.models import AccessToken, RefreshToken, get_application_model
from django.utils import timezone
from oauth2_provider.models import (
AccessToken,
RefreshToken,
get_access_token_model,
get_application_model,
get_refresh_token_model,
)
from oauthlib.oauth2.rfc6749.errors import (
InvalidClientError,
InvalidGrantError,
Expand Down Expand Up @@ -326,3 +333,42 @@ def validate_latin_extended_string(text: str) -> bool:
bool: if all strings are encoded less than U+017F (383) and it is not empty
"""
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:
Copy link
Copy Markdown
Contributor

@jimmyfagan jimmyfagan May 28, 2026

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@JamesDemeryNava JamesDemeryNava May 28, 2026

Choose a reason for hiding this comment

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

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.

"""Revoke prior tokens for a user/app id pair to ensure that if a user has reauthorized
that prior tokens can't be used, in case any of those prior tokens have more scopes than
the newly created one

Args:
user_id (int): ID for the user who just re-authorized an app they have authorized previously
app_id (int): ID for the application the user just re-authorized for
"""
AccessToken = get_access_token_model()
RefreshToken = get_refresh_token_model()
prior_access_tokens = list(AccessToken.objects.filter(user=user_id, application=app_id).order_by('-created'))

# If there is only one access token for a user_id/app_id, we don't need to revoke any prior tokens
if len(prior_access_tokens) <= 1:
return

prior_access_tokens.pop(0)

for access_token in prior_access_tokens:
try:
refresh_token = get_refresh_token_model().objects.get(access_token=access_token.id)
Comment thread
JamesDemeryNava marked this conversation as resolved.
Outdated

# Only update the access token expires value if it is in the future
Comment thread
ryan-morosa marked this conversation as resolved.
Outdated
if access_token.expires > timezone.now():
access_token.expires = timezone.now()
access_token.save()

if refresh_token.revoked is None:
refresh_token.revoked = timezone.now()
refresh_token.access_token_id = None
refresh_token.save()

except RefreshToken.DoesNotExist:
# indicates it is a access token created via CAN flow, as it does not have an associated refresh token
access_token.expires = timezone.now()
access_token.save()
Comment on lines +367 to +368
Copy link
Copy Markdown
Contributor

@jimmyfagan jimmyfagan May 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@JamesDemeryNava JamesDemeryNava May 29, 2026

Choose a reason for hiding this comment

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

That makes sense to me!

Loading
Loading