From 72411a81e99b0fc0a4bc2fe5c968f2bff2c2820c Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Tue, 11 Jan 2022 13:24:22 -0500 Subject: [PATCH 1/8] Test of models.clear_tokens covers this. --- oauth2_provider/management/commands/cleartokens.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oauth2_provider/management/commands/cleartokens.py b/oauth2_provider/management/commands/cleartokens.py index 3fb1827f6..9d58361bc 100644 --- a/oauth2_provider/management/commands/cleartokens.py +++ b/oauth2_provider/management/commands/cleartokens.py @@ -3,7 +3,7 @@ from ...models import clear_expired -class Command(BaseCommand): +class Command(BaseCommand): # pragma: no cover help = "Can be run as a cronjob or directly to clean out expired tokens" def handle(self, *args, **options): From 378a87ed16f8b161e4e5f628832717654210c33f Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Tue, 11 Jan 2022 13:59:27 -0500 Subject: [PATCH 2/8] Update clear_expired tests as had been done in #1084. --- tests/test_models.py | 70 +++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 7b37486ca..396cf9df6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -1,3 +1,5 @@ +from datetime import timedelta + import pytest from django.contrib.auth import get_user_model from django.core.exceptions import ImproperlyConfigured, ValidationError @@ -294,7 +296,10 @@ def test_str(self): class TestClearExpired(BaseTestModels): def setUp(self): super().setUp() - # Insert two tokens on database. + # Insert many tokens, both expired and not, and grants. + now = timezone.now() + earlier = now - timedelta(seconds=100) + later = now + timedelta(seconds=100) app = Application.objects.create( name="test_app", redirect_uris="http://localhost http://example.com http://example.org", @@ -302,24 +307,37 @@ def setUp(self): client_type=Application.CLIENT_CONFIDENTIAL, authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE, ) - AccessToken.objects.create( - token="555", - expires=timezone.now(), - scope=2, - application=app, - user=self.user, - created=timezone.now(), - updated=timezone.now(), - ) - AccessToken.objects.create( - token="666", - expires=timezone.now(), - scope=2, - application=app, - user=self.user, - created=timezone.now(), - updated=timezone.now(), - ) + for i in range(100): + old = AccessToken.objects.create(token="old access token {}".format(i), expires=earlier) + new = AccessToken.objects.create(token="current access token {}".format(i), expires=later) + # make half of these Access Tokens have related Refresh Tokens, which prevent their deletion. + if i % 2: + RefreshToken.objects.create( + token="old refresh token {}".format(i), + application=app, + access_token=old, + user=self.user, + ) + RefreshToken.objects.create( + token="current refresh token {}".format(i), + application=app, + access_token=new, + user=self.user, + ) + Grant.objects.create( + user=self.user, + code="old grant code {}".format(i), + application=app, + expires=earlier, + redirect_uri="https://localhost/redirect", + ) + Grant.objects.create( + user=self.user, + code="new grant code {}".format(i), + application=app, + expires=later, + redirect_uri="https://localhost/redirect", + ) def test_clear_expired_tokens(self): self.oauth2_settings.REFRESH_TOKEN_EXPIRE_SECONDS = 60 @@ -333,15 +351,13 @@ def test_clear_expired_tokens_incorect_timetype(self): assert result == "ImproperlyConfigured" def test_clear_expired_tokens_with_tokens(self): - self.client.login(username="test_user", password="123456") - self.oauth2_settings.REFRESH_TOKEN_EXPIRE_SECONDS = 0 - ttokens = AccessToken.objects.count() - expiredt = AccessToken.objects.filter(expires__lte=timezone.now()).count() - assert ttokens == 2 - assert expiredt == 2 + assert AccessToken.objects.count() == 200 + assert RefreshToken.objects.count() == 100 + assert Grant.objects.count() == 200 clear_expired() - expiredt = AccessToken.objects.filter(expires__lte=timezone.now()).count() - assert expiredt == 0 + assert AccessToken.objects.count() == 150 + assert RefreshToken.objects.count() == 100 + assert Grant.objects.count() == 100 @pytest.mark.django_db From a23d8b982645f4d216252f2467dfd2aedfb6da78 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Tue, 11 Jan 2022 14:07:06 -0500 Subject: [PATCH 3/8] Exercise batch_delete and reduce sleep() to zero to speed up test. --- tests/test_models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_models.py b/tests/test_models.py index 396cf9df6..9beba8e2d 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -351,6 +351,8 @@ def test_clear_expired_tokens_incorect_timetype(self): assert result == "ImproperlyConfigured" def test_clear_expired_tokens_with_tokens(self): + self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_SIZE = 10 + self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_INTERVAL = 0.0 assert AccessToken.objects.count() == 200 assert RefreshToken.objects.count() == 100 assert Grant.objects.count() == 200 From 6ddb3d7a2b55568cd38712e2698b4f31349506a7 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Tue, 11 Jan 2022 17:00:09 -0500 Subject: [PATCH 4/8] Broken update to demonstrate problem with bulk_create. --- tests/test_models.py | 57 ++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 9beba8e2d..3e228da08 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -307,37 +307,52 @@ def setUp(self): client_type=Application.CLIENT_CONFIDENTIAL, authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE, ) - for i in range(100): - old = AccessToken.objects.create(token="old access token {}".format(i), expires=earlier) - new = AccessToken.objects.create(token="current access token {}".format(i), expires=later) - # make half of these Access Tokens have related Refresh Tokens, which prevent their deletion. - if i % 2: - RefreshToken.objects.create( - token="old refresh token {}".format(i), - application=app, - access_token=old, - user=self.user, - ) - RefreshToken.objects.create( - token="current refresh token {}".format(i), - application=app, - access_token=new, - user=self.user, - ) - Grant.objects.create( + + expired_access_tokens = AccessToken.objects.bulk_create( + AccessToken(token="expired AccessToken {}".format(i), expires=earlier) for i in range(100) + ) + current_access_tokens = AccessToken.objects.bulk_create( + AccessToken(token="current AccessToken {}".format(i), expires=later) for i in range(100) + ) + + RefreshToken.objects.bulk_create( + RefreshToken( + token="expired AT's refresh token {}".format(i), + application=app, + access_token=expired_access_tokens[i], + user=self.user, + ) + for i in range(100, 2) + ) + RefreshToken.objects.bulk_create( + RefreshToken( + token="current AT's refresh token {}".format(i), + application=app, + access_token=expired_access_tokens[i], + user=self.user, + ) + for i in range(49, 100, 2) + ) + Grant.objects.bulk_create( + Grant( user=self.user, code="old grant code {}".format(i), application=app, - expires=earlier, + expires=expired_access_tokens[i], redirect_uri="https://localhost/redirect", ) - Grant.objects.create( + for i in range(100) + ) + Grant.objects.bulk_create( + Grant( user=self.user, code="new grant code {}".format(i), application=app, - expires=later, + expires=current_access_tokens[i], redirect_uri="https://localhost/redirect", ) + for i in range(100) + ) def test_clear_expired_tokens(self): self.oauth2_settings.REFRESH_TOKEN_EXPIRE_SECONDS = 60 From 4a2cebb8716f3693b22e7ed82ce6d224ec3a85b8 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Wed, 12 Jan 2022 08:46:59 -0500 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Andrew Chen Wang <60190294+Andrew-Chen-Wang@users.noreply.github.com> --- tests/test_models.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 3e228da08..63bb1de8e 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -312,23 +312,23 @@ def setUp(self): AccessToken(token="expired AccessToken {}".format(i), expires=earlier) for i in range(100) ) current_access_tokens = AccessToken.objects.bulk_create( - AccessToken(token="current AccessToken {}".format(i), expires=later) for i in range(100) + AccessToken(token=f"current AccessToken {i}", expires=later) for i in range(100) ) RefreshToken.objects.bulk_create( RefreshToken( - token="expired AT's refresh token {}".format(i), + token=f"expired AT's refresh token {i}", application=app, - access_token=expired_access_tokens[i], + access_token_pk=expired_access_tokens[i].pk, user=self.user, ) for i in range(100, 2) ) RefreshToken.objects.bulk_create( RefreshToken( - token="current AT's refresh token {}".format(i), + token=f"current AT's refresh token {i}", application=app, - access_token=expired_access_tokens[i], + access_token_pk=expired_access_tokens[i].pk, user=self.user, ) for i in range(49, 100, 2) @@ -336,9 +336,9 @@ def setUp(self): Grant.objects.bulk_create( Grant( user=self.user, - code="old grant code {}".format(i), + code=f"old grant code {i}", application=app, - expires=expired_access_tokens[i], + expires=expired_access_tokens[i].expires, redirect_uri="https://localhost/redirect", ) for i in range(100) @@ -346,9 +346,9 @@ def setUp(self): Grant.objects.bulk_create( Grant( user=self.user, - code="new grant code {}".format(i), + code=f"new grant code {i}", application=app, - expires=current_access_tokens[i], + expires=current_access_tokens[i].expires, redirect_uri="https://localhost/redirect", ) for i in range(100) From ab3a1b6b0063f9da1c001bd6f19260f6571d6875 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Wed, 12 Jan 2022 12:40:31 -0500 Subject: [PATCH 6/8] Fix the review comment fix. --- tests/test_models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 63bb1de8e..69ba27f52 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -319,7 +319,7 @@ def setUp(self): RefreshToken( token=f"expired AT's refresh token {i}", application=app, - access_token_pk=expired_access_tokens[i].pk, + access_token=expired_access_tokens[i].pk, user=self.user, ) for i in range(100, 2) @@ -328,7 +328,7 @@ def setUp(self): RefreshToken( token=f"current AT's refresh token {i}", application=app, - access_token_pk=expired_access_tokens[i].pk, + access_token=expired_access_tokens[i].pk, user=self.user, ) for i in range(49, 100, 2) From edc39274104de5e0a608b6b11f55b210d4f5b6c1 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Wed, 12 Jan 2022 13:13:12 -0500 Subject: [PATCH 7/8] Make assert errors more readable. --- tests/test_models.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 69ba27f52..1d546dce5 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -368,13 +368,19 @@ def test_clear_expired_tokens_incorect_timetype(self): def test_clear_expired_tokens_with_tokens(self): self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_SIZE = 10 self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_INTERVAL = 0.0 - assert AccessToken.objects.count() == 200 - assert RefreshToken.objects.count() == 100 - assert Grant.objects.count() == 200 + at_count = AccessToken.objects.count() + assert at_count == 200 + rt_count = RefreshToken.objects.count() + assert rt_count == 100 + gt_count = Grant.objects.count() + assert gt_count == 200 clear_expired() - assert AccessToken.objects.count() == 150 - assert RefreshToken.objects.count() == 100 - assert Grant.objects.count() == 100 + at_count = AccessToken.objects.count() + assert at_count == 150 + rt_count = RefreshToken.objects.count() + assert rt_count == 100 + gt_count = Grant.objects.count() + assert gt_count == 100 @pytest.mark.django_db From 40dba48b5acf1d4999e055c3cab947dc5e623d31 Mon Sep 17 00:00:00 2001 From: Alan Crosswell Date: Wed, 12 Jan 2022 17:32:14 -0500 Subject: [PATCH 8/8] Parameterize the number of tokens to create and improve the assertion failure messages. --- tests/test_models.py | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/tests/test_models.py b/tests/test_models.py index 1d546dce5..9ce1e5eb7 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -297,6 +297,7 @@ class TestClearExpired(BaseTestModels): def setUp(self): super().setUp() # Insert many tokens, both expired and not, and grants. + self.num_tokens = 100 now = timezone.now() earlier = now - timedelta(seconds=100) later = now + timedelta(seconds=100) @@ -307,14 +308,16 @@ def setUp(self): client_type=Application.CLIENT_CONFIDENTIAL, authorization_grant_type=Application.GRANT_AUTHORIZATION_CODE, ) - + # make 200 access tokens, half current and half expired. expired_access_tokens = AccessToken.objects.bulk_create( - AccessToken(token="expired AccessToken {}".format(i), expires=earlier) for i in range(100) + AccessToken(token="expired AccessToken {}".format(i), expires=earlier) + for i in range(self.num_tokens) ) current_access_tokens = AccessToken.objects.bulk_create( - AccessToken(token=f"current AccessToken {i}", expires=later) for i in range(100) + AccessToken(token=f"current AccessToken {i}", expires=later) for i in range(self.num_tokens) ) - + # Give the first half of the access tokens a refresh token, + # alternating between current and expired ones. RefreshToken.objects.bulk_create( RefreshToken( token=f"expired AT's refresh token {i}", @@ -322,36 +325,37 @@ def setUp(self): access_token=expired_access_tokens[i].pk, user=self.user, ) - for i in range(100, 2) + for i in range(0, len(expired_access_tokens) // 2, 2) ) RefreshToken.objects.bulk_create( RefreshToken( token=f"current AT's refresh token {i}", application=app, - access_token=expired_access_tokens[i].pk, + access_token=current_access_tokens[i].pk, user=self.user, ) - for i in range(49, 100, 2) + for i in range(1, len(current_access_tokens) // 2, 2) ) + # Make some grants, half of which are expired. Grant.objects.bulk_create( Grant( user=self.user, code=f"old grant code {i}", application=app, - expires=expired_access_tokens[i].expires, + expires=earlier, redirect_uri="https://localhost/redirect", ) - for i in range(100) + for i in range(self.num_tokens) ) Grant.objects.bulk_create( Grant( user=self.user, code=f"new grant code {i}", application=app, - expires=current_access_tokens[i].expires, + expires=later, redirect_uri="https://localhost/redirect", ) - for i in range(100) + for i in range(self.num_tokens) ) def test_clear_expired_tokens(self): @@ -369,18 +373,18 @@ def test_clear_expired_tokens_with_tokens(self): self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_SIZE = 10 self.oauth2_settings.CLEAR_EXPIRED_TOKENS_BATCH_INTERVAL = 0.0 at_count = AccessToken.objects.count() - assert at_count == 200 + assert at_count == 2 * self.num_tokens, f"{2 * self.num_tokens} access tokens should exist." rt_count = RefreshToken.objects.count() - assert rt_count == 100 + assert rt_count == self.num_tokens // 2, f"{self.num_tokens // 2} refresh tokens should exist." gt_count = Grant.objects.count() - assert gt_count == 200 + assert gt_count == self.num_tokens * 2, f"{self.num_tokens * 2} grants should exist." clear_expired() at_count = AccessToken.objects.count() - assert at_count == 150 + assert at_count == self.num_tokens, "Half the access tokens should not have been deleted." rt_count = RefreshToken.objects.count() - assert rt_count == 100 + assert rt_count == self.num_tokens // 2, "Half of the refresh tokens should have been deleted." gt_count = Grant.objects.count() - assert gt_count == 100 + assert gt_count == self.num_tokens, "Half the grants should have been deleted." @pytest.mark.django_db