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

fix(refresh_tokens): ensure deletion of expired tokens from the database #1050

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

Avantol13
Copy link
Contributor

New Features

Breaking Changes

Bug Fixes

Improvements

  • ensure deletion of expired tokens from the database

Dependency updates

Deployment changes

@coveralls
Copy link

coveralls commented Oct 3, 2022

Pull Request Test Coverage Report for Build 13129

  • 0 of 6 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 73.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/resources/openid/idp_oauth2.py 0 6 0.0%
Totals Coverage Status
Change from base Build 13114: -0.02%
Covered Lines: 6890
Relevant Lines: 9321

💛 - Coveralls

@@ -191,10 +191,17 @@ def get_access_token(self, user, token_endpoint, db_session=None):
refresh_token = row.refresh_token
Copy link
Contributor

Choose a reason for hiding this comment

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

should we sort the tokens from oldest to newest to make sure the old ones get cleaned up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't break on the first one it finds, it goes through all of them. So either way they'll all get cleaned up

Copy link
Contributor

Choose a reason for hiding this comment

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

but it resets refresh_token to None, so if it finds a valid token and then an expired token, it will raise the AuthError when it shouldn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I misunderstood what you were saying. Good catch, yeah I'll need to make another PR. Ideally there's only ever 1 expired at a time but this might be problematic for the current state

@Avantol13 Avantol13 merged commit 77eb5a4 into master Nov 15, 2022
@Avantol13 Avantol13 deleted the fix/delete_expired_refresh branch November 15, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants