search

UniStuttgart-VISUS / Visus.LdapAuthentication

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

Visus.DirectoryAuthentication.LdapOptions.SearchBase should probably be pluralized #13

Closed mycroes closed 7 months ago

mycroes commented 7 months ago

Affected library

Environment

Summary LdapOptions in ...DirectoryAuthentication is still using SearchBase as the property name for what is now SearchBases (plural) in both ...LdapAuthentication and the ...DIrectoryAuthentication README.

What behaviour did you expect? I assume this property should be renamed to SearchBases to match the fact that it supports multiple search bases and to align with ...LdapAuthentication.

mycroes commented 7 months ago

Thanks for fixing! Good to see you're spending some time on the libraries recently, I'll be rolling out my first production deployment soon.

crowbar27 commented 7 months ago

In package 0.13.0, which should be out now.

I am very close to deploying DirectoryAuthentication in production for the first time as well, which will be when I publish 1.0.0. Please note that there might be breaking changes between 0. and 1.. I am still reworking how the user is mapped in preparation of an implementation that integrates with ASP.NET Core Identity. If you are using the built-in LdapUser, you should be unaffected though.

mycroes commented 7 months ago

I accept the API isn't stable, our usage is very limited so I don't expect much issues there. I'm indeed using built-in LdapUser only. In all honesty the only thing that would be an improvement for me is to have the group names instead of SIDs, but I didn't want to cheat the mapping to have the group names stored in the SIDs. Because I need to map 2 of the AD groups to internal role names I just resolved this by doing the mapping based on the SID instead of the group name (the mapping is stored in AppSettings).

crowbar27 commented 7 months ago

If you are binding against AD, it should be safe to set GroupIdentityAttribute to "sAMAccountName" and GroupIdentityConverter to null.

mycroes commented 7 months ago

Yes, that was my alternate solution, but then they do end up in GroupSid claims, right?

crowbar27 commented 7 months ago

At the moment, they do. However, once I have refactored the user object mapping, everything will be configurable via ILdapUserMapper.