Skip to content
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.

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

Expired ID tokens are not removed #1222

Closed
2 tasks
Pankrat opened this issue Nov 16, 2022 · 1 comment · Fixed by #1223
Closed
2 tasks

Expired ID tokens are not removed #1222

Pankrat opened this issue Nov 16, 2022 · 1 comment · Fixed by #1223
Labels

Comments

@Pankrat
Copy link
Contributor

Pankrat commented Nov 16, 2022

Describe the bug

When running django-admin cleartokens, access tokens and refresh tokens are cleared as documented, but expired ID tokens stick around in the DB until they are removed manually.

To Reproduce

  1. Set OIDC_ENABLED = True
  2. Set REFRESH_TOKEN_EXPIRE_SECONDS to a low value
  3. Set ACCESS_TOKEN_EXPIRE_SECONDS to a low value
  4. Set ID_TOKEN_EXPIRE_SECONDS to a low value
  5. Get a set of tokens
  6. Wait until all of the tokens expired
  7. Run django-admin cleartokens
  8. The access and refresh token will be removed, the expired ID token is still in the DB

Expected behavior

I'd expect the ID token to be removed alongside the access token which holds the reference OR the ID token to be removed from the database when it expired.

Version

Tested with version 2.1.0.

  • I have tested with the latest published release and it's still a problem.
  • I have tested with the master branch and it's still a problem.

Additional context

More generally, I wonder if revoking an access token should clear the ID token (if there is one), similar to how revoking a refresh token clears the associated access token?

I'd be willing to contribute a test and patch to the issue if a patch would be welcome for this issue (please let me know if that's the case).

Thanks for your consideration!

@Pankrat Pankrat added the bug label Nov 16, 2022
@n2ygk
Copy link
Member

n2ygk commented Nov 16, 2022

@Pankrat Thanks for noticing this. Since OIDC was a late addition to DOT, I suspect the requisite changes to cleartokens was missed -- it's got a very short commit history.

Regarding the automatic clearing of an ID_token when an access_token is revoked, that "feels" right. If the access_token was revoked I would think validate_jwt_bearer_token should fail to validate the ID token based on that access token.

Please do submit a PR! Or maybe two, in order to separate the two concerns.

Pankrat added a commit to AbletonAG/django-oauth-toolkit that referenced this issue Nov 17, 2022
The `cleartokens` removed expired refresh tokens and associated access
tokens but kept expired ID tokens in the database.

Remove ID tokens when the associated access and refresh tokens are
cleared.

Preserve expired tokens until the associated access token is deleted to
keep relationships intact and not trigger delete cascades.

Fixes jazzband#1222
Pankrat added a commit to AbletonAG/django-oauth-toolkit that referenced this issue Nov 18, 2022
The `cleartokens` management command removed expired refresh tokens and
associated access tokens but kept expired ID tokens in the database.

Remove ID tokens when the associated access and refresh tokens are
cleared. Preserve expired ID tokens until the associated access token is
deleted to keep relationships intact and not trigger delete cascades.

Fixes jazzband#1222
n2ygk pushed a commit that referenced this issue Nov 18, 2022
The `cleartokens` management command removed expired refresh tokens and
associated access tokens but kept expired ID tokens in the database.

Remove ID tokens when the associated access and refresh tokens are
cleared. Preserve expired ID tokens until the associated access token is
deleted to keep relationships intact and not trigger delete cascades.

Fixes #1222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants