Oauth2 to OIDC #2448
Oauth2 to OIDC #2448
Conversation
apps/online_oidc_provider/claims.py
Outdated
| info_nibble = ( | ||
| _("Nibble"), | ||
| _( | ||
| "Informasjon om dine kjøp og saldo på Nibble", | ||
| ), | ||
| ) | ||
|
|
||
| def scope_nibble(self): | ||
| return {'test':'this data was returned'} |
There was a problem hiding this comment.
I am highly uncertain whether this is necessary or not, and if it is: what it really does..
There was a problem hiding this comment.
Learned recently that an empty dictionary should be the default value. Else, "test" is added as a claim to /userinfo with the returned value.
FluidSense
force-pushed
the
fix/oauth2-to-oidc
branch
from
4d22d32 to
f23bdf7
Compare
| @@ -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.
Is there a reason we are writing our own version of TokenHasScope, instead of just using DRF's built in one? If so that should be clearly stated in a comment here
There was a problem hiding this comment.
AFAIK DRF does not have a built-in TokenHasScope. We previously used oauth2_provider's TokenHasScope, made by Django OAuth Toolkit to provide a support layer for DRF. As we are no longer using oauth2_provider, we cannot use their TokenHasScope either.
There was a problem hiding this comment.
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.
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.
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:
juanifioren/django-oidc-provider#242
juanifioren/django-oidc-provider#78
| @@ -7,10 +7,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
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.
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.
@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?
There was a problem hiding this comment.
Looks very good aside from the points that sklirg has pointed out!
Also just a small heads up for that I added a few Nibble tests which still use the old Oauth2TestCase, because the API-was still using that at the time. It should be OK to just replace a few lines, but the change needs to be made
|
I am a bit conflicted about this seeing as https://github.com/juanifioren/django-oidc-provider is relatively stale and not well-documented. On the other hand, we are already using it and this would require less manual work (i.e. issuing new tokens and secrets to all current applications) while satisfying our current needs. |
I can agree in being a bit conflicted on it, but as of right now it still works. If we end up needing to change we could export to a completly separate OpenID provider without too much hassle I think? We may need everyone to change their passwords at worst? @Aborysa had a look into setting up KeyCloak earlier, which might still be an alternative. Is it much work to get working as a bare-minimum? |
What kind of a pull request is this?
Our OIDC-provider also provides Oauth2-authentication.
This ports the shop over to the oidc-provider so we avoid duplicate authentication modules.
Code Checklist
(Possible) Breaking Changes
This breaks all SSO-Clients which uses Oauth2, the endpoint will have to be updated and new keys generated.
Nibble until Change endpoints to use oidc at OW4 Nibble2#75 is merged
The changes
are backwards compatible
Description of changes
Screenshot(s) if appropriate