re #270 - Added middleware to refresh access tokens #278
Conversation
daggaz
changed the title
daggaz
force-pushed
the
refresh-tokens
branch
from
91e3ce6
to
3d132a9
Compare
daggaz
force-pushed
the
refresh-tokens
branch
from
3d132a9
to
3de576c
Compare
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
========================================
+ Coverage 86.3% 87.1% +0.7%
========================================
Files 11 11
Lines 549 590 +41
========================================
+ Hits 474 514 +40
- Misses 75 76 +1
|
daggaz
force-pushed
the
refresh-tokens
branch
3 times, most recently
from
1922fff
to
412bec1
Compare
There was a problem hiding this comment.
Thanks for creating this PR! I like the idea of us supporting this out of the box. It seems relatively straightforward and a common use case. My preference is to have it be entirely optional so as to not break existing user's installations when upgrading. I could be convinced otherwise though.
| if request is None: | ||
| logger.debug("Authentication backend was called without request") | ||
| return |
There was a problem hiding this comment.
We shouldn't short-circuit this functionality if the request isn't provided. django_auth_adfs.rest_framework.AdfsAccessTokenAuthentication.authenticate() will call the authenticate backend without a request.
There was a problem hiding this comment.
This code is in AdfsAuthCodeBackend, not the base class.
In AdfsAuthCodeBackend, request is already required to not be None. I've just made it explicit.
Before these changes, If you attempt to use AdfsAuthCodeBackend without a request, you will get an exception when it tries to generate redirect_uri in exchange_auth_code().
On a separate point, I actually think exchange_auth_code() should be pulled up from the base class to AdfsAuthCodeBackend, as it's only called from there.
| def process_request(self, request): | ||
| now = datetime.now() + settings.REFRESH_THRESHOLD | ||
| expiry = datetime.fromisoformat(request.session['_adfs_token_expiry']) | ||
| if now > expiry: | ||
| try: | ||
| self._refresh_access_token(request, request.session['_adfs_refresh_token']) | ||
| except (PermissionDenied, HTTPError): | ||
| logout(request) |
There was a problem hiding this comment.
This logic should live in a middleware, not the authenticate backend.
There was a problem hiding this comment.
I went back and forward on this myself a few times, moving the functionality between the middleware and the backend.
In the end I opted for the backend because then the session keys (such as _adfs_refresh_token) only appeared in one place rather than being spread through multiple files.
I'm open to moving it back to the middleware though.
| return user | ||
|
|
||
| def _process_adfs_response(self, request, adfs_response): | ||
| user = self.process_access_token(adfs_response['access_token'], adfs_response) | ||
| request.session['_adfs_access_token'] = adfs_response['access_token'] |
There was a problem hiding this comment.
I don't see this session value being used anywhere. I think it can be removed.
There was a problem hiding this comment.
It's not used, however in my use case I do not use the auto-generated claim to group mapping, and instead directly use the data in the access token.
ADFS can be configured to spit out many different things in the access token.
I'm sure there are many other use cases where having access the to original JWT would be valuable.
There was a problem hiding this comment.
I agree with @tim-schilling here, I don't really want to just add the token to the session "for the sake of it".
| user = self.process_access_token(adfs_response['access_token'], adfs_response) | ||
| request.session['_adfs_access_token'] = adfs_response['access_token'] | ||
| expiry = datetime.now() + timedelta(seconds=adfs_response['expires_in']) | ||
| request.session['_adfs_token_expiry'] = expiry.isoformat() |
There was a problem hiding this comment.
This creates a dependency on django.contrib.session. I'm up in the air if we want to continue supporting that path or not.
We should add a system check to verify that the session app is installed to alert the developer sooner about a misconfiguration.
There was a problem hiding this comment.
The base django.contrib.auth app requires django.contrib.session already. It's not an optional dependency.
| if now > expiry: | ||
| try: | ||
| self._refresh_access_token(request, request.session['_adfs_refresh_token']) | ||
| except (PermissionDenied, HTTPError): |
There was a problem hiding this comment.
What do you think about using logger.debug() here to capture the exception's information?
|
I'll try to review ASAP, but I'm pretty busy these days. If we're afraid of breaking, we'll have to major bump - I don't mind that 😊 |
|
Thanks for the good PR! And for the good review, @tim-schilling 😊 I'd love to get this merged this or next week. |
|
Closed in favor of #343 |
This PR addresses #270