UniStuttgart-VISUS / Visus.LdapAuthentication

LDAP authentication middleware for ASP.NET Core
MIT License
24 stars 8 forks source link

GroupIdentityAttribute doesn't affect primary group #15

Closed mycroes closed 2 months ago

mycroes commented 5 months ago

Affected library

Environment

Summary In LdapUserBase the primary group is always added as GroupSid claim using the value of the PrimaryGroupAttribute. However, all other groups are added by applying the converter to the specified GroupIdentityAttribute.

What are you doing? Trying to use sAMAccountName as GroupIdentityAttribute by specifying a Mapping on LdapOptions.

What is the problem? Primary group is never added with it's sAMAccountName.

What behaviour did you expect? Primary group is added through it's sAMAccountName.

Additional context Solution based on comments in #13.

crowbar27 commented 4 months ago

TBH, I am not sure whether I fully understood your problem, but the fact that it mentions "primary group" raised a red alert for me, which I will explain shortly. However, could you provide me with a test case (sample JSON, sample user names like "user1" and groups like "primarygroup", "group1", etc.) to repro this?

I think, however, that this relates to an issue that took me two years to figure out and which is handled wrong by most software, including Microsoft Exchange. The problem is that the primary group is not a "normal group" stored in a list of DNs, but the primary group is in a special field, so if you query the groups of an object, you get all groups except for the primary group. This holds for ADDS as well as OpenLDAP, but ADDS is particularly nasty in that it stores the RID, which is the SID stripped of the domain identifier. You have probably asked why I am working with the SIDs and not just use the DNs of the groups an this is the reason why: In order to get the primary group, I retrieve the RID, concatenate it with the domain SID to obtain the group SID (that is what the converter of the mapping attribute does) and then query the group with that SID.

I think that the hack I gave you in the other ticket will not work for the primary group and I also think that there is no workaround for this at the moment. This issue will need some significant re-implementation, I fear.

crowbar27 commented 4 months ago

I have made several extension methods for the LDAP entries publicly accessible and added the option to register a custom user mapper (see README for details). You should be able to customise how the group claims are generated this way, but I haven't tested it yet.

crowbar27 commented 2 months ago

I think I have a solution for the problem now, which is mapping the group on an object (https://github.com/UniStuttgart-VISUS/Visus.LdapAuthentication/blob/2de7a85c307f8165ad79730223350cb2779d5b9d/Visus.DirectoryAuthentication/LdapGroupBase.cs#L13) like the user and adding these objects to the user (https://github.com/UniStuttgart-VISUS/Visus.LdapAuthentication/blob/2de7a85c307f8165ad79730223350cb2779d5b9d/Visus.DirectoryAuthentication/LdapUserBase.cs#L67). This would allow you to customise how groups are mapped to claims via the new ClaimsBuilder (https://github.com/UniStuttgart-VISUS/Visus.LdapAuthentication/blob/2de7a85c307f8165ad79730223350cb2779d5b9d/Visus.DirectoryAuthentication/ClaimsBuilder.cs#L42). Would that solve your problem?

crowbar27 commented 2 months ago

Testing as 0.15.0

crowbar27 commented 2 months ago

Version 0.16.0 adds sAMAccountName as System.Security.Claims.ClaimTypes.Role by default.