Support for multiple IDPs?

[peterfarrell]

Right now this library is limited to a single IDP. I have need where users could authenticate from 2 different IDPs.

Django SAML2 library has the ability to configure multiple IDPs and offers a WAYF page if the IDP is unknown.
https://github.com/knaperek/djangosaml2#pysaml2-specific-files-and-configuration

https://app.example.com/oauth2/login?next=/&idp=adfs.example.com
https://app.example.com/oauth2/login?next=/&idp=adfs.different.com

If only one IDP is configured, then nothing changes. If multiple IDPs are configured and the IDP is not in the login request then a WAYF view would be served. The value in the IDP would be the current SERVER value

Would there be interest in a PR to add support for this? If I had time, I'd work on one.

[JonasKs]

I would be happy to include this as a new feature, but this is not something I personally have time to look at in the short term. If you (or someone reading this) is willing to create a PR I would be happy to review it!

[peterfarrell]

I'd be happy to help if I can find the time. Is there any documentation on how to setup and run the test suite locally? I see a Vagrant file, but I think some simple documentation in the contributions files on how to get this setup be very helpful.

[JonasKs]

I’ve actually only used our company ADFS setup, so never used the Vagrant file personally. I’d have to look into that.

PRs for docs on how to use it are very welcome. 😄

[JonasKs]

As for the test suite you can look at this file.

[peterfarrell]

@JonasKs that file link is helpful. I'm not a poetry user.

FYI, I figured out that I can't install this library as editable using pip.

-e git+https://github.com/atdsaa/django-auth-adfs.git@setuptools_edittable#egg=django_auth_adfs

Installing requirements causes this error:

  Running setup.py develop for django-auth-adfs
    ERROR: Command errored out with exit status 1:
     command: /usr/local/bin/python -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/usr/local/src/django-auth-adfs/setup.py'"'"'; __file__='"'"'/usr/local/src/django-auth-adfs/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps
         cwd: /usr/local/src/django-auth-adfs/
    Complete output (3 lines):
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    ModuleNotFoundError: No module named 'setuptools'
    ----------------------------------------
ERROR: Command errored out with exit status 1: /usr/local/bin/python -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/usr/local/src/django-auth-adfs/setup.py'"'"'; __file__='"'"'/usr/local/src/django-auth-adfs/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' develop --no-deps Check the logs for full command output.

Poetry fails to include setuptools in the generated setup.py file per this bug:

Ref: python-poetry/poetry#2956

[JonasKs]

I tend to just edit files directly in the venv when I’m testing. I’m not on a PC tonight so can’t really look into it. :/ you could go back a few commits and restore the setup.py locally while you test, tho.

[peterfarrell]

@JonasKs Any suggestions on how you would like to the additional settings to be broken out? Right now the DJANGO_ADFS_AUTH dict is flat and I'm trying to figure out how backwards compatible you want to be. There's already some shims in place for older settings keys that are deprecated.

I was thinking something like this. We could continue to support the current configuration or allow people to setup the IDPS dict. Also, most of the config should be configurable by IDP as mappings et al may differ between IDPs.

AUTH_ADFS = {
    'IDPS': {
        # Key for WAYF, WAYF is not used if only one IDP is defined
        'internal': {
            'SERVER': 'adfs.example.com',
            'CLIENT_ID': 'xxxxx',
            'RELYING_PARTY_ID': 'internal-example-com',
            'AUDIENCE': 'microsoft:identityserver:internal-example-com',
            'CLAIM_MAPPING': {
                'first_name': 'FirstName',
                'last_name': 'LastName',
                'email': 'Email',
                'phone_number': 'TelephoneNumber',
                'ad_object_guid': 'objectGUID',
            },
            'CA_BUNDLE': False,
            'USERNAME_CLAIM': 'UserName',
            'GROUPS_CLAIM': None,
        }
    },
    'CREATE_NEW_USERS': False,
    'DISABLE_SSO': True,
    'LOGIN_EXEMPT_URLS': [
        '^api',  # Assuming you API is available at /api
    ],
    'CUSTOM_FAILED_RESPONSE_VIEW': 'security.views.user_denied_openid'
}

The key for the WAYF is used for a WAYF page (if more than one IDP is defined) and also in the OAuth2CallbackView as a querystring parameter as part of the callback so we know which IDP to use the auth code against.

Thoughts?

[JonasKs]

We could continue to support the current configuration or allow people to setup the IDPS dict

I'd like for both to be supported, as breaking it would require a major version bump.

Your suggestion seems clean to me. 👍

[peterfarrell]

@JonasKs Thanks for the feedback. In order to make things easier, I would like to propose a dev could continue using the current config key or the new IDPS dict but not both at the same time.

[JonasKs]

Yes, that's perfect. 👍

[JonasKs]

I'd be happy to help if I can find the time. Is there any documentation on how to setup and run the test suite locally? I see a Vagrant file, but I think some simple documentation in the contributions files on how to get this setup be very helpful.

As I mentioned earlier in the ticket, you can run all tests by running poetry run coverage run manage.py test -v 2. It's not required to have the Vagrant files up and running, they're mocked in the tests.
As for the Vagrant files I've looked at them, but can't really help you out on how to run them, as I'm using a Linux workstation. There are docs, though. Not sure if you've seen them?

[peterfarrell]

@JonasKs Thanks for pointing out the Vagrant stuff in the demo section. I never thought to look there -- I was specifically looking on how to run tests in the Contributing readme:

https://github.com/snok/django-auth-adfs/blob/master/CONTRIBUTING.rst

Maybe the comment you made could be used to have a section in Contributing on how to run the tests?

[JonasKs]

Agree! I'll submit it with my next PR, unless someone beats me to it 👍

[fabianallendorf]

What is the state of this issue? At our company we implemented a workaround for now, but would greatly appreciate if this was supported. I can share the basic idea of our solution, perhaps that's helpful.

AUTH_ADFS = {"SETTINGS_CLASS": "path.to.MainAuthAdfsSettings"}  # set a main settings class as entry-point

class MainSettings:
    def __init__(self):
        self.deferred_settings = DefaultSettings()

    def __getattr__(self, item):
        return getattr(self.deferred_settings, item)

There is one main settings class which holds a reference to some "actual" settings class. Initially the default settings are set.
There is one settings class for each new IDP, which extends the DefaultSettings class and overwrites certain values.
__getattr__ is overwritten so each call to a settings attribute on MainSettings is deferred to the actual settings class.

# Custom settings class for one IDP
class CustomIDPSettings(DefaultSettings):
  def __init__(self):
    self.SERVER = ...
    self.CALLBACK_URL = ...  # a custom setting we added to define a callback url for each IDP

class CustomOAuth2LoginView(OAuth2LoginView):
    def get(self, request):
        _patch_django_auth_adfs(CustomIDPSettings)
        return redirect(django_auth_adfs.config.provider_config.build_authorization_endpoint(request))

class CustomOAuth2LogoutView(OAuth2LogoutView):
    def get(self, request):
        _patch_django_auth_adfs(CustomIDPSettings)
        return super().get(request)

class CustomOAuth2CallbackView(OAuth2CallbackView):
    def get(self, request):
        _patch_django_auth_adfs(CustomIDPSettings)
        return super().get(request)

We have overwritten Login, Logout and Callback view classes. They dont do much, except calling the super class. However before that, the settings are patched.

from functools import partial

def _patch_django_auth_adfs(settings_cls):
    settings = settings_cls()
    # deferred settings object is patched
    django_auth_adfs.config.settings.deferred_settings = settings_cls()
    # provider config timestamp is set to None, to invalidate the current config
    django_auth_adfs.config.provider_config._config_timestamp = None
    # redirect uri is patched so the request contains the correct `redirect_uri`
    django_auth_adfs.config.provider_config.redirect_uri = partial(
        _redirect_uri,  # custom method
        callback_url_name=settings.CALLBACK_URL, # a custom setting
        provider_config=django_auth_adfs.config.provider_config,
    )

def _redirect_uri(request, provider_config, callback_url_name: str):
    provider_config.load_config()
    return request.build_absolute_uri(reverse(callback_url_name))

We added CALLBACK_URL to the settings class, so each IDP can have their own callback url. That was necessary because ProviderConfig.redirect_uri uses hardcoded url name django_auth_adfs:callback.
_redirect_uri is a custom method which behaves like ProviderConfig.redirect_uri expect it takes callback_url_name as parameter.

I hope this is helpful to someone how needs a quick and dirty workaround until this feature is implemented.

[JonasKs]

Hi! Thanks for providing a workaround. State is that well tested and documented pull requests are welcome, but I am not going to implement it myself.

[peterfarrell]

Hi all, I'm actively working on a PR for this feature. Watch this space.

[peterfarrell]

Just an update on the PR - we are currently blocked by jpadilla/pyjwt#433

Right now I could get the authentication portion to work, but tokens would be broken because tokens are checked against valid issuers and right now PyJWT only allows one valid issuer. We could do a big try/catch and try other issuers, but it seems like support in PyJWT is coming soon.

[JonasKs]

Multi-tenant scenarios:

• No validation of iss - this is allowed in multi-tenant applications
• You have a set of tenants that you want to allow, this requires a list of issuers

Single-tenant scenarios:

• Always validate to a specific iss

I'm also the author of the FastAPI-Azure-Auth package, which allow both multi-tenant scenarios described above by taking an iss_callable. The reason I wanted this to be a callable instead of a static list provided on server start is because I don't want to re-deploy or have this manually written in the application. The list of accepted tenants will change from day to day, based on which customers we have.

In the documentation, I provide an example on how to configure such a iss_callable, as well as a more sophisticated example with memory-caching which is updated every hour.

There's multiple solutions here:

• Swap to python-jose which is simply just a cleaner version of pyJWT imo
  • A list of issuers are supported there
• Wait for pyJWT to release this feature
• Do as I do in FastAPI-Azure-Auth

Personally I like doing as I do in FastAPI-Azure-Auth.

[JonasKs]

Also, thank you for the PR! I didn't have time to check it out before now (I was in the US for thanksgiving 🦃).

[ChegeBryan]

@peterfarrell what workaround do you currently have for this jpadilla/pyjwt/433?

Allowing multiple issuers issue is currently stale. ☠️

[JonasKs]

A PR to swap out pyJWT with python-jose welcome.