django-auth-adfs icon indicating copy to clipboard operation
django-auth-adfs copied to clipboard

User has too many groups?

Open 777GE90 opened this issue 3 years ago • 3 comments

I am using a modified version of this plugin and have noticed a potential issue that I wanted to validate.

I have setup Azure to obtain the groups like below: image

All seems to work fine and I'm able to login. If I inspect the token returned from my user, I correctly get the groups back like below:

{
    ...
    "groups": [
         "my-ad-group-1",
         "my-ad-group-2"
    ]
    ...
}

However, I noticed an edge case where some users who have far too many groups, simply don't get the "groups" provided back in the JWT access token. My understanding is because the data is too large to fit in the token, so instead Azure returns back an API link which I guess it expects you to hit in a separate call to get that users groups. So the token has the below instead:

{
    ...
    "_claim_names": {
        "groups": "src1"
    },
    "_claim_sources": {
        "src1": {
            "endpoint": "https://graph.windows.net/<tenant_id>/users/<user_id>/getMemberObjects"
    }
    ...
}

On a brief initial glance, I couldn't see anywhere obvious where this plugin would make this subsequent API call to get the groups, it seems like when it gets the token back it will just fail with a DEBUG log message complaining the groups are missing?

    def update_user_groups(self, user, claims):
        """
        Updates user group memberships based on the GROUPS_CLAIM setting.

        Args:
            user (django.contrib.auth.models.User): User model instance
            claims (dict): Claims from the access token
        """
        if settings.GROUPS_CLAIM is not None:
            # Update the user's group memberships
            django_groups = [group.name for group in user.groups.all()]

            if settings.GROUPS_CLAIM in claims:
                claim_groups = claims[settings.GROUPS_CLAIM]
                if not isinstance(claim_groups, list):
                    claim_groups = [claim_groups, ]
            else:
                logger.debug("The configured groups claim '%s' was not found in the access token",
                             settings.GROUPS_CLAIM)
                claim_groups = []
           ...

Was curious if you have seen this scenario before / whether the plugin supports this scenario or not?

777GE90 avatar Apr 22 '22 10:04 777GE90

Hi! This plugin does not support that. I’m happy to accept a pull request for it.

You’re right, azure will give this URL back instead of groups if it’s too many. There’s many ways to do this, but OBO is probably most common for graph. I’ve written docs about that here, in FastAPI-Azure-Auth.

JonasKs avatar Apr 22 '22 11:04 JonasKs

Hi! This plugin does not support that. I’m happy to accept a pull request for it.

You’re right, azure will give this URL back instead of groups if it’s too many. There’s many ways to do this, but OBO is probably most common for graph. I’ve written docs about that here, in FastAPI-Azure-Auth.

Cool, I will need to look into this to fix it for us before we can migrate over to Azure, but looking at my schedule will probably be late next month. Will definitely consider making it a PR for this repo as that would be better for us too.

You are definitely more knowledgeable about ADFS / Azure than me, so are you saying you would recommend OBO to be the best way to do this additional API call in this plugin? Or do you have any other suggestions?

One thing I did notice is that the API link that is returned in the token is referring to graph.windows.net but I noticed a lot of MS documentation refers to graph.microsoft.com - not sure what the difference is but I guess they will plan to depreciate one of those so will probably need to be mindful of that in the implementation, unless getting and using the access token is the same procedure for both.

777GE90 avatar Apr 22 '22 14:04 777GE90

Ok finally got round to doing this, pull request here: https://github.com/snok/django-auth-adfs/pull/247

Would be great to merge this in.

777GE90 avatar Aug 03 '22 07:08 777GE90