-
Notifications
You must be signed in to change notification settings - Fork 23
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
Oauth2 to OIDC #2448
Oauth2 to OIDC #2448
Changes from all commits
a9c56a8
2038437
8431e9b
798cb7a
4c752f4
f23bdf7
0bf3afb
13d6a7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
from django.core.exceptions import ImproperlyConfigured | ||
from oidc_provider.lib.utils.oauth2 import extract_access_token | ||
from oidc_provider.models import Token | ||
from rest_framework import authentication, exceptions | ||
from rest_framework.exceptions import PermissionDenied | ||
from rest_framework.permissions import BasePermission | ||
|
||
|
||
class OidcOauth2Auth(authentication.BaseAuthentication): | ||
|
@@ -20,3 +23,56 @@ def authenticate(self, request): | |
raise exceptions.AuthenticationFailed("The oauth2 token has expired") | ||
|
||
return oauth2_token.user, None | ||
|
||
|
||
class TokenHasScope(BasePermission): | ||
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. Is there a reason we are writing our own version of 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. AFAIK DRF does not have a built-in 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'm also a bit vary of this. Could you do some more research with regards to this? Could we depend on oauth2_provider for that function alone, or is it tightly coupled to its own OAuth2 provider? Do we want to depend on it, or do we want to run our own fork of the code? At least get some tests up if we do :) 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. Hm, I've been digging through the code for the OAuth2 provider, and the answer is a big I don't know. I guess I can continue testing some more, but it seems that if we are to use the TokenHasScope from oauth2_provider, we will need duplicates of settings, to ensure that the oidc_provider and oauth2_provider has the same settings, as TokenHasScope relies on settings for oauth2_provider. 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 class is by the way taken from this stale issue at oidc_provider's repo juanifioren/django-oidc-provider#342 Other issues related to this worth a read: |
||
""" | ||
The request is authenticated and the token used has the right scope | ||
""" | ||
|
||
def has_permission(self, request, view): | ||
|
||
try: | ||
token = Token.objects.get( | ||
access_token=extract_access_token(request) | ||
) # The Token object retrieved from access_token | ||
|
||
if token.has_expired(): | ||
self.message = { | ||
"detail": PermissionDenied.default_detail, | ||
"error": "Token has expired", | ||
} | ||
return False | ||
|
||
except Token.DoesNotExist: | ||
self.message = { | ||
"detail": PermissionDenied.default_detail, | ||
"error": "No token provided or token does not exist", | ||
} | ||
|
||
return False | ||
|
||
if hasattr(token, "scope"): | ||
required_scopes = self.get_scopes(request, view) | ||
|
||
if set(required_scopes).issubset(set(token.scope)): | ||
return True | ||
|
||
# Provide information about required scope | ||
self.message = { | ||
"detail": PermissionDenied.default_detail, | ||
"error": "Permission denied, required_scopes: " | ||
+ str(list(required_scopes)), | ||
} | ||
|
||
return False | ||
|
||
return False | ||
|
||
def get_scopes(self, request, view): | ||
try: | ||
return getattr(view, "required_scopes") | ||
except AttributeError: | ||
raise ImproperlyConfigured( | ||
"TokenHasScope requires the view to define the required_scopes attribute" | ||
) |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
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.
I don't think it's that smart to change migrations.
Removing the dependency on this migration doesn't remove the tables/columns from the database. A separate migration is required to do this. After that migration is done, a new change can be done where the requirement is removed, and migrations can be squashed to remove the dependency.
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.
I just saw that you've done it correctly with the sso app migration, good. But I still think this migration shouldn't be changed (and therefore all the sso migrations have to be kept), until it is squashed so that sso is fully removed. It's a bit of a long run, but I don't think it matters that much to keep the migrations in for a while. Maybe @duvholt could chime in on this?
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.
@sklirg I see! So it would be better to not involve this and perhaps add something like it in it's own PR for example?