handle oauthlib errors on create token and revoke token requests #1210
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 |
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
AndrejZbin
force-pushed
the
fix-catch-oauth-errors
branch
6 times, most recently
from
8e9066f to
12c9de9
Compare
oauth2_provider/oauth2_backends.py
Outdated
| headers, body, status = self.server.create_revocation_response(uri, http_method, body, headers) | ||
| uri = headers.get("Location", None) | ||
| return uri, headers, body, status | ||
| except OAuth2Error as exc: |
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.
AndrejZbin
force-pushed
the
fix-catch-oauth-errors
branch
2 times, most recently
from
9f93ff1 to
13643a7
Compare
AndrejZbin
force-pushed
the
fix-catch-oauth-errors
branch
from
13643a7 to
04cf3af
Compare
Fixes #1209
Description of the Change
Checklist
CHANGELOG.mdupdated (only for user relevant changes)AUTHORS