-
-
Notifications
You must be signed in to change notification settings - Fork 99
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
re #270 - Added middleware to refresh access tokens #278
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import logging | ||
from datetime import datetime, timedelta | ||
|
||
import jwt | ||
from django.contrib.auth import get_user_model | ||
from django.contrib.auth import get_user_model, logout | ||
from django.contrib.auth.backends import ModelBackend | ||
from django.contrib.auth.models import Group | ||
from django.core.exceptions import (ImproperlyConfigured, ObjectDoesNotExist, | ||
PermissionDenied) | ||
from requests import HTTPError | ||
|
||
from django_auth_adfs import signals | ||
from django_auth_adfs.config import provider_config, settings | ||
|
@@ -394,14 +396,47 @@ def authenticate(self, request=None, authorization_code=None, **kwargs): | |
logger.debug("Authentication backend was called but no authorization code was received") | ||
return | ||
|
||
# If there's no request object, we pass control to the next authentication backend | ||
if request is None: | ||
logger.debug("Authentication backend was called without request") | ||
return | ||
|
||
# If loaded data is too old, reload it again | ||
provider_config.load_config() | ||
|
||
adfs_response = self.exchange_auth_code(authorization_code, request) | ||
access_token = adfs_response["access_token"] | ||
user = self.process_access_token(access_token, adfs_response) | ||
user = self._process_adfs_response(request, adfs_response) | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this session value being used anywhere. I think it can be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @tim-schilling here, I don't really want to just add the token to the session "for the sake of it". |
||
expiry = datetime.now() + timedelta(seconds=adfs_response['expires_in']) | ||
request.session['_adfs_token_expiry'] = expiry.isoformat() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This creates a dependency on 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The base |
||
if 'refresh_token' in adfs_response: | ||
request.session['_adfs_refresh_token'] = adfs_response['refresh_token'] | ||
request.session.save() | ||
return user | ||
|
||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree |
||
logout(request) | ||
Comment on lines
+421
to
+428
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic should live in a middleware, not the authenticate backend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I'm open to moving it back to the middleware though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @tim-schilling 😊 |
||
|
||
def _refresh_access_token(self, request, refresh_token): | ||
provider_config.load_config() | ||
response = provider_config.session.post( | ||
provider_config.token_endpoint, | ||
data=f'grant_type=refresh_token&refresh_token={refresh_token}' | ||
) | ||
response.raise_for_status() | ||
adfs_response = response.json() | ||
self._process_adfs_response(request, adfs_response) | ||
|
||
|
||
class AdfsAccessTokenBackend(AdfsBaseBackend): | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is in
AdfsAuthCodeBackend
, not the base class.In
AdfsAuthCodeBackend
,request
is already required to not beNone
. 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 generateredirect_uri
inexchange_auth_code()
.On a separate point, I actually think
exchange_auth_code()
should be pulled up from the base class toAdfsAuthCodeBackend
, as it's only called from there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree