-
-
Notifications
You must be signed in to change notification settings - Fork 794
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 access token 500 to properly handle expired or deleted refresh tokens #1337
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1337 +/- ##
=======================================
Coverage 97.54% 97.55%
=======================================
Files 32 32
Lines 2120 2123 +3
=======================================
+ Hits 2068 2071 +3
Misses 52 52
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@discobeta could you review your tests? It looks like the new change isn't covered. |
b458cc1
to
eddbfae
Compare
added additional tests |
@discobeta, It looks like https://github.com/jazzband/django-oauth-toolkit/pull/1337/checks?check_run_id=17910869256 still isn't covered. You probably need to setup a scenario where you make a valid AccessToken, delete the token, then make a request. |
Added a test for a deleted token |
The except branch added by your patch is still not covered, https://app.codecov.io/gh/jazzband/django-oauth-toolkit/pull/1337?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=jazzband |
I pushed an update that covers this change. Can you please approve the
workflow to recheck coverage?
…On Sat, Oct 21, 2023 at 12:42 PM dopry ***@***.***> wrote:
The except branch added by your patch is still not covered,
https://app.codecov.io/gh/jazzband/django-oauth-toolkit/pull/1337?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=jazzband
—
Reply to this email directly, view it on GitHub
<#1337 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRERCSQIQNPU6PITIYSTN3YAP3OLAVCNFSM6AAAAAA55WNPIKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZTHA2TINJYGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@dopry It looks like this is now covered. Thank you for your recommendation and support with this. |
I think the last thing we need is a changelog entry and adding yourself to Authors if you're not in there already. |
@dopry can you please take a look here and see if there are any further changes required? |
Fixes #1318
Description of the Change
We are now try / except when attempting to find a vali token to avoid a 500
oauth2_provider.models.AccessToken.DoesNotExist: AccessToken matching query does not exist
Checklist
CHANGELOG.md
updated (only for user relevant changes)AUTHORS