handle oauthlib errors on create token and revoke token requests#1210
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Codecov Report
@@ Coverage Diff @@## master #1210 +/- ## ========================================== + Coverage 96.04% 96.20% +0.16% ========================================== Files 26 26 Lines 1314 1317 +3 ========================================== + Hits 1262 1267 +5 + Misses 52 50 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
n2ygk left a comment
n2ygk left a comment There was a problem hiding this comment.
@AndrejZbin Thanks for this and filling in the PR template.
Please add test case(s) to demonstrate the original bug and the fix.
Code coverage shows that this new code is not exercised by any tests:
https://github.com/jazzband/django-oauth-toolkit/pull/1210/checks?check_run_id=8701284936
AndrejZbinforce-pushed the fix-catch-oauth-errors branch 6 times, most recently from 8e9066f to 12c9de9Compareoauth2_provider/oauth2_backends.py Outdated
| headers, body, status=self.server.create_revocation_response(uri, http_method, body, headers) | ||
| uri=headers.get("Location", None) | ||
| returnuri, headers, body, status | ||
| exceptOAuth2Errorasexc: |
There was a problem hiding this comment.
According to https://github.com/jazzband/django-oauth-toolkit/pull/1210/checks?check_run_id=8744338670 the newly-added exception case is not tested. Please add some failure tests.
There was a problem hiding this comment.
So apparently create_revocation_response errors are handled by oauthlib and the added try/except block is redundant in this function. But I think it won't hurt to be on "safe" side and still catch errors from oautlib, at least for consistency across functions. I added test cases that cover the the "except" block in both functions even when the exception is handled by oauthlib. However, if this slight redundancy is not desired, I'm happy to remove it and change only create_token_response method.
There was a problem hiding this comment.
@AndrejZbin I don't think it's necessary to have extra lines of code that will never be used and mock patching the oauthlib response is inappropriate if oauthlib would never return such a response.
There was a problem hiding this comment.
Fair enough, I reverted changes to create_revocation_response function and removed tests with mock patching.
AndrejZbinforce-pushed the fix-catch-oauth-errors branch 2 times, most recently from 9f93ff1 to 13643a7Compare
AndrejZbinforce-pushed the fix-catch-oauth-errors branch from 13643a7 to 04cf3afCompare
2 participants
Fixes#1209
Description of the Change
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS