iMerica / dj-rest-auth

Authentication for Django Rest Framework
https://dj-rest-auth.readthedocs.io/en/latest/index.html
MIT License
1.67k stars 311 forks source link

allauth's openid connect #463

Closed bufke closed 1 year ago

bufke commented 1 year ago

Hello, django-allauth now support openid connect. This allows one provider class to support theoretically any OIDC compliant service. It could even perhaps replace every other provider.

Is supporting this something the project would want? If so, may have time to help. The challenge is that this provider can be added multiple times using a SERVERS array and then generates a "dynamic instance for each configured server". This is very different as other providers can only be used once and have a static provider ID ("google"). Example:

Provider class id: openid_connect Provider server id: google_oidc Provider server id: another_oidc

Without changes, various places get the wrong ID. For example a callback url ends up having openid_connect when it really should have google_oidc. I got a terrible version working already by forcing in the dynamic provider name into various places. I would need to rework it for it to be of acceptable quality for this project.

Let me know if you think this is worth my time to create a pull request for. IMO OIDC is nice because I can forget about every provider and use just 1 for everything. I no longer would need to make a custom provider class just to support yet another oauth2 service. I can also add multiple providers - for example I can add two Azure tenants to one site (impossible previously with allauth). That said, I would understand if supporting every allauth socialaccount feature isn't appetizing here.

kiraware commented 1 year ago

This should be supported, many developer out there build their own way to solve OIDC in dj-rest-auth. It will make developer life a lot easier!!! I hope this will get PR soon and of course merged to the project

bufke commented 1 year ago

FYI @kiraware I am reasonably pleased with my mixin that can be used with both SocialLoginView and SocialConnectView. It works with and without oidc. I override the callback_url and adapter_class to set provider_id based on the kwarg provider (which is the provider_id as seen in the URL param). I use a mapping (feel free to tell me there is a better way!) to get the right adapter. SOCIAL_ADAPTER_MAP.

class OIDCMixin:
    client_class = OAuth2Client  # Only OAuth2 client is supported

    @property
    def callback_url(self):
        # Set dynamic OIDC provider ID
        provider_id = self.kwargs.get("provider", self.adapter_class.provider_id)
        # I assume a fixed domain, perhaps one would prefer to call super and replace the provider_id instead
        return DOMAIN + "/auth/" + provider_id

    @property
    def adapter_class(self):
        provider = self.kwargs.get("provider")

        adapter_class = SOCIAL_ADAPTER_MAP.get(
            provider, SOCIAL_ADAPTER_MAP["openid_connect"]
        )
        # Set dynamic OIDC provider ID
        adapter_class.provider_id = provider
        return adapter_class

I'm sure there are other ways - feel free to comment. Hopefully this could help get someone started with OIDC. If this was ever to be a PR, we'd probably want to ditch SOCIAL_ADAPTER_MAP as that's really my own thing to reduce boilerplate code. The idea with that is to have just 1 view that works with all oauth2 based providers.

If of any interest to a maintainer, the organization that I'm working with would be happy to discuss funding this feature/code review/consultation.

iMerica commented 1 year ago

Hi @bufke,

Thanks for raising this issue. I completely support OIDC as a means of providing the widest array of authentication use cases. However, if this package were to add support for it, it would have to satisfy this criteria:

bufke commented 1 year ago

Currently there are some imports from allauth in dj-rest-auth. Is that acceptable to continue doing, as long as they don't break when allauth isn't available?

django-oidc-provider I believe acts as a server. Not a consumer of an existing OAuth service. mozilla-django-oidc only supports one server making it non-viable for multi tenant apps or single organizations that use multiple auth providers. allauth allows adding multiple oidc servers - for example Google Apps for an organization and a separate "Sign in with GMail" that isn't locked down to one organization.

The user story for this in dj-rest-auth is that you want a simple and non-provider specific way to add OIDC log in to a Django app with a rest API.

kiraware commented 1 year ago

Hello @iMerica, TBH i'm agreed with @bufke about OIDC should be supported by dj-rest-auth. I wanna ask why this package couldn't implement OIDC if still rely on django-allauth? I thought dj-rest-auth is the REST API way, while django-allauth provide the internal and django templates way. Also i see that dj-rest-auth not rely on django-allauth is on the project.

However juanifioren/django-oidc-provider looks like not well mantained, and mozilla/mozilla-django-oidc as far as i know only allow one provider as bufke said.

I hope dj-rest-auth would be able to this, because this feature is very nice! Thanks for your attention!

iMerica commented 1 year ago

@kiraware

I support OIDC in general and have been using it in my career for many years now.

OIDC support is bigger than DjangoAllAuth and makes it redundant in a few ways. So feel free to submit PRs to this repo for OIDC as long as they don't depend on Django-AllAuth. My goal is to reduce the footprint of Django-Allauth in this repo and keep it as a optional dependency.

iMerica commented 1 year ago

Thank you for bringing up this topic. I'm going to close this issue, but PRS for OIDC are very welcome.

kiraware commented 1 year ago

@iMerica Thanks for your response. @bufke My pleasure sir if you could do PR on OIDC

bufke commented 1 year ago

Understood, the change I had in mind would have been allauth specific. Anyone else looking for this could override the provider_id the way I did.

bufke commented 1 year ago

Hello, I noticed that django-allauth's OIDC support has gotten much better in the 0.56+ versions. It's now possible to use multiple OIDC providers and configure them in django admin. They removed some providers like keycloak in favor of the oidc provider. If I was already using keycloak with dj-rest-auth, I would find that the migration path breaks support for rest-auth, as I am now required to use oidc.

IMO it would be a terrific workflow to use dj-rest-auth with django-allauth with OIDC. If you wanted to revisit this I would be happy to help and submit a PR. I believe the main change is that SocialLoginSerializer would need a few tweaks in how it gets the provider id. The way I linked above no longer works due to code refactoring in allauth.

kiraware commented 1 year ago

Would love if its really happen and implemented by your PR

bufke commented 1 year ago

I've updated my code here to support the dynamic provider id.

        if adapter_class == OpenIDConnectAdapter:
            provider = request.resolver_match.captured_kwargs.get("provider")
            adapter = adapter_class(request, provider)
        else:
            adapter = adapter_class(request)
        app = adapter.get_provider().app

I'm happy to submit a PR if @iMerica would like. As before, it depends on allauth. I can understand how it can be annoying to have allauth breaking changes affect this project. I don't have interest to build OIDC into dj-rest-auth without using allauth. But I'm happy to keep posting any personal changes I make here as needed.

Another option, I could break up SocialLoginSerializer.validate so that it's easier to extend. As you can see, I had to copy a lot of code so that I could make a few minor tweaks to this function.