GetGroupsFromActiveDirectory() to look for the attribute set in claim types configuration instead of Name

[rurikon]

We have a set of groups in AD that have different DisplayName, Name and SamAccountName. I was looking into why the group rights didn't work and ran into this issue: #134 It basically describes the exact same issue we have.

In the comments, Yvand says "A possible alternative is to get the groups using method LDAPCP.GetGroupsFromActiveDirectory(), which should set the claim value of group with their samAccountName." However, if I look at the code in GetGroupsFromActiveDirectory(), it looks like it's only using adGroup.Name and not adGroup.SamAccountName. Would it be possible to change the code to look at whatever is set in the claim types configuration page instead? I would've made a pull request, but I'm not sure what is the variable for that configuration setting.

The relevant lines are:

LDAPCP/LDAPCP/LDAPCP.cs

Line 1368 in c0670eb

string claimValue = adGroup.Name;

LDAPCP/LDAPCP/LDAPCP.cs

Line 1378 in c0670eb

claimValue = groupCTConfig.ClaimValuePrefix.Replace(ClaimsProviderConstants.LDAPCPCONFIG_TOKENDOMAINNAME, groupDomainName) + adGroup.Name;

LDAPCP/LDAPCP/LDAPCP.cs

Line 1382 in c0670eb

claimValue = groupCTConfig.ClaimValuePrefix.Replace(ClaimsProviderConstants.LDAPCPCONFIG_TOKENDOMAINFQDN, groupDomainFqdn) + adGroup.Name;

(I first posted this as a comment on the closed issue, but I figured it's better to make a new one.)

[Yvand]

@rurikon thank you for taking the time to report this issue. Yes, you were right to open a new issue.
Indeed, it's quite arbitrary that in GetGroupsFromActiveDirectory, LDAPCP uses property Name.
I'll see how this can be improved, ideally using the attribute set in the config as you suggested.
I'll keep you posted on the progress

[Yvand]

@rurikon, the class System.DirectoryServices.AccountManagement.GroupPrincipal exposes only 3 properties which LDAPCP could use as claimValue for the group names in GetGroupsFromActiveDirectory():

Name                    Property   string Name {get;set;}
SamAccountName          Property   string SamAccountName {get;set;}
UserPrincipalName       Property   string UserPrincipalName {get;set;}

In your scenario, would using either SamAccountName or UserPrincipalName work instead of Name?

[rurikon]

When installing, LDAPCP automatically sets group claim type to query SamAccountName, so that would be a logical choice.

Another option, if it's not too complex, is to check if the setting matches one of the exposed ones (Name, SamAccountName or UserPrincipalName). If it does, use the setting, and if it doesn't, default to SamAccountName. This is probably what I would aim for, because it gives the end user some control over how they set up their claims.

[Yvand]

Understood. So in your case, properties Name and SamAccountName have different values?
The change completely makes sense, but I want to be very careful because if I do it, in most cases LDAPCP would switch without any warning to SamAccountName and I'm really concerned about the potential impacts after an update.

[rurikon]

Yes, Name and SamAccountName are different in our environment. According to my tests, when we have SamAccountName as the group claim type, people picker works "as intended" but augmentation doesn't work at all. People picker will add rights to c:0-.t|claimprovider|my-group-samaccountname (or any other property we define in settings), but the .NET augmentation will always add c:0-.t|claimprovider|my-group-name to the user, because it's locked into Name. I suppose for most environments SamAccountName and Name are the same, so the problem doesn't manifest, because for them "my-group-samaccountname" is the same as "my-group-name" and the claims strings end up being equal. In our environment, I have to select Name as the group claim type, so that the string people picker uses will match the string in augmentation.

If you don't go the complex route, it will be a breaking change to those (like me) who have changed the group claim type to Name in order to get augmentation to work. New users will have things easier because the default setting SamAccountName will match right away, but they can't choose any other property as the group claim. Users who have identical Name and SamAccountName won't notice a thing.

If you choose the complex route, in my understanding it won't affect current installations. The options are:

• if Name and SamAccountName are the same, no strings will be affected
• if someone is in the same situation as me and has changed the group claim type to Name, augmentation will continue to use it (until they decide to change the settings)
• if group claim type is set to something other than Name, SamAccountName or UPN, the augmentation hasn't worked before and will not work after the change
• if someone uses SamAccountName or UPN as the group claim type and relies on the augmentation not working, they will have a "positive breaking change" when it starts working, but this is a corner case that's depending on incorrect assumptions

[Yvand]

@rurikon I thought about it, and I completely agree with your summary. The impact can be only positive, regardless if default configuration of LDAPCP was changed or not.
I'll start working on this feature soon and update this thread when done.
Thank you for your inputs.

[Yvand]

@rurikon I implemented the changes in 219b660
I made extensive tests and I did not find any issue

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.