feat: add clear_expired_tokens task to remove oauth expired tokens#3835
feat: add clear_expired_tokens task to remove oauth expired tokens#3835Anas12091101 wants to merge 8 commits intomasterfrom
Conversation
Anas12091101
force-pushed
the
anas/add-clear_expired_tokens-task
branch
from
46b5174 to
a4662cb
Compare
Anas12091101
force-pushed
the
anas/add-clear_expired_tokens-task
branch
from
39a54ac to
6af50ce
Compare
|
@dsubak, there are approximately ~1.8M access and refresh tokens in xPRO, and the COUNT(*) query returned almost instantly. Given that, it seems the table size isn’t causing performance issues. Do you think we still need the chunked_oauth_access_token_delete.py script in this case? |
@Anas12091101 I'd give a manual run of |
mitxpro/tasks.py
Outdated
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @app.task(acks_late=True) |
There was a problem hiding this comment.
It could be a shared task; it doesn't need to depend on an app.
mitxpro/settings.py
Outdated
| CRON_CLEAR_TOKENS_HOURS = get_string( | ||
| name="CRON_CLEAR_TOKENS_HOURS", | ||
| default=0, | ||
| description="'hours' value for the 'clear-expired-tokens' scheduled task (defaults to midnight)", | ||
| ) | ||
| CRON_CLEAR_TOKENS_DAYS = get_string( | ||
| name="CRON_CLEAR_TOKENS_DAYS", | ||
| default="*", | ||
| description="'day_of_month' value for 'clear-expired-tokens' scheduled task (default will run every day)", | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is more like a one-off setting, and I don't think we are going to tweak it often. We should probably remove these settings.
| "clear-expired-tokens": { | ||
| "task": "mitxpro.tasks.clear_expired_tokens", | ||
| "schedule": crontab( | ||
| minute=0, | ||
| hour=CRON_CLEAR_TOKENS_HOURS, | ||
| day_of_week="*", | ||
| day_of_month=CRON_CLEAR_TOKENS_DAYS, | ||
| month_of_year="*", | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Was there a decision on running it daily?
There was a problem hiding this comment.
The configurations can be shortened depending on the decision. Daily sounds too often for this task.
There was a problem hiding this comment.
@arslanashraf7 I’ve configured it similar to MITxOnline, so the task will run every Monday at 9 AM.
| "REFRESH_TOKEN_EXPIRE_SECONDS": { | ||
| "description": "Number of seconds until a refresh token expires", | ||
| "required": false | ||
| }, |
There was a problem hiding this comment.
Did you run generate_app_json command for this?
mitxpro/tasks.py
Outdated
| def clear_expired_tokens(): | ||
| """Clear expired OAuth2 access, refresh, and ID tokens via the cleartokens management command.""" | ||
| log.info("Starting clear_expired_tokens...") | ||
| call_command("cleartokens") |
There was a problem hiding this comment.
In mitodl/mitxonline#3248, we switched from the command to the direct method from oauth2_provider.models import clear_expired. Perhaps we should do the same.
mitxpro/tasks_test.py
Outdated
| def test_clear_tokens_does_not_delete_unexpired_tokens(settings): | ||
| """Test that cleartokens does not delete tokens that have not yet expired""" | ||
| settings.OAUTH2_PROVIDER = { | ||
| **settings.OAUTH2_PROVIDER, | ||
| "REFRESH_TOKEN_EXPIRE_SECONDS": 60 * 60 * 24 * 30, # 30 days | ||
| } | ||
|
|
||
| user = UserFactory.create() | ||
| application = Application.objects.create( | ||
| name="test-cleartokens-app", | ||
| client_type="confidential", | ||
| authorization_grant_type="authorization-code", | ||
| skip_authorization=True, | ||
| ) | ||
|
|
||
| now = timezone.now() | ||
|
|
||
| # Create an unexpired access token (expires 1 hour from now) | ||
| access_token = AccessToken.objects.create( | ||
| user=user, | ||
| application=application, | ||
| token=uuid.uuid4().hex, | ||
| expires=now + timedelta(hours=1), | ||
| scope="read write", | ||
| ) | ||
|
|
||
| # Create a non-revoked refresh token | ||
| refresh_token = RefreshToken.objects.create( | ||
| user=user, | ||
| application=application, | ||
| token=uuid.uuid4().hex, | ||
| access_token=access_token, | ||
| ) | ||
|
|
||
| tasks.clear_expired_tokens() | ||
|
|
||
| # Unexpired tokens should still exist | ||
| assert AccessToken.objects.filter(pk=access_token.pk).exists() | ||
| assert RefreshToken.objects.filter(pk=refresh_token.pk).exists() | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_clear_tokens_deletes_expired_tokens(settings): |
There was a problem hiding this comment.
Both of these could be converted into a single parameterized test. This will also reduce the code by a large extent.
for more information, see https://pre-commit.ci
3 participants
What are the relevant tickets?
https://github.com/mitodl/hq/issues/10115
Description (What does it do?)
This PR adds
clear_expired_tokenstask to remove OAuth expired tokens. The task is set to run every week.Screenshots (if appropriate):
How can this be tested?
./manage.py cleartokensand verify it deletes only expired tokensAdditional Context