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

Support mapping groups' IDs to names for Django groups

Open tim-schilling opened this issue 3 years ago • 8 comments

Moving this part of the conversation from #173 to here.

Looks like sAMAccountName is not available for groups entirely managed within Azure AD: https://docs.microsoft.com/en-us/azure/active-directory/hybrid/how-to-connect-fed-group-claims

sAMAccountName and On Premises Group SID attributes are only available on Group objects synced from Active Directory. They aren't available on groups created in Azure Active Directory or Office365. Applications configured in Azure Active Directory to get synced on-premises group attributes get them for synced groups only.

For configurations with only Azure Active Directory, the above means that the group claims will only include the id. This results in the group names being UUIDs which aren't reasonably maintainable. To mitigate that, I propose a setting is added that will handle the mapping of id to group name.

Setting: GROUPS_CLAIM_MAPPING A dictionary of Azure AD Group ID to Django Group Name mappings. When a groups claim contains one of these IDs, the corresponding Django group will be used (and created if needed) using the name from the mapping.

Fund with Polar

tim-schilling avatar Jan 24 '22 19:01 tim-schilling

I've implemented a workaround in my application by subclassing the backend and decided that I'd be better off creating the groups identified in the mapping on AppConfig.ready rather than key off of MIRROR_GROUPS. The justification here is that the application clearly expects these types of groups so it's fine to create them ahead of time. I assume the purpose of MIRROR_GROUPS is that a user's groups claim could be as high as 100 or 200 with some number of them the being unnecessary to the application so they shouldn't be created ahead of time.

tim-schilling avatar Jan 24 '22 20:01 tim-schilling

Hi. This seems reasonable, but normally I’d recommend implementing graph or roles for this purpose then.

In other words:

  • user authenticates
  • token is validated
  • fetch azure to get groups if groups hasn’t been updated for x time

or:

  • creating application roles and act on those.

JonasKs avatar Jan 29 '22 08:01 JonasKs

I'm a bit hesitant to add another round trip request to my authentication flow. I'll take a look at application roles to see if that'll work better for us.

tim-schilling avatar Jan 29 '22 13:01 tim-schilling

That's how we manage things now atleast, using app roles instead of groups.

JonasKs avatar Feb 01 '22 06:02 JonasKs

@JonasKs I couldn't get app roles to work. It continued to only use the ids. I'm happy to close this issue if the preferred approach is to manually manage it.

tim-schilling avatar Feb 08 '22 16:02 tim-schilling

@JonasKs, do you have details on how you've got roles handed back to your Django auth process? I can't seem to see where to add them to the response from AzureAD.

Support for groups in the way that @tim-schilling described would be my preferred option (being able to map names to groups instead of/alongside the UUIDs), but App Roles would be a reasonable substitute should there be documentation on how to make it work...

jameskirsop avatar Feb 17 '22 00:02 jameskirsop

I haven't used company groups for Azure AD, since we default on ADFS. Groups in Azure AD is not nested, which means if a user is member of x, and x is member of y, y is not in the token. For us, this don't work, and we default to application roles for Azure AD applications.

JonasKs avatar Feb 18 '22 06:02 JonasKs

Hi. This seems reasonable, but normally I’d recommend implementing graph or roles for this purpose then.

In other words:

* user authenticates

* token is validated

* fetch azure to get groups if groups hasn’t been updated for x time

or:

* creating application roles and act on those.

So the second option we would also like to do, are there any examples? Because we do not seem to find anything in the documentation about this?

brammittendorff avatar Jul 06 '22 13:07 brammittendorff