UniStuttgart-VISUS / Visus.LdapAuthentication

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

Visus.DirectoryIdentity status #11

Open Maxhy opened 8 months ago

Maxhy commented 8 months ago

This is more a discussion about Identity implementation.

I see that you recently pushed Visus.DirectoryIdentity. Can you give some status update and TODO here? I believe you mainly focused on the UserStore whereas UserManager (mainly for password check) and UserClaimsPrincipalFactory implementation are required as well?

Thanks.

crowbar27 commented 8 months ago

This is in a very early stage and we plan to have it for DirectoryServices and Novell LDAP. I have pretty much decided on how the authentication hack will work, because I do not want to expose the password hashes from LDAP (newer versions of AD do not have these and if there are hashes you would need extremely high privileges to read them, which a web service should not have). I guess the password issue is why I was unable to find any existing implementation of ASP.NET Core Identity using LDAP.

The plan is to have the role store as well, but I have not yet thought through what a role would be. The obvious idea would be using the groups, but reading all groups recursively is very expensive, so there would need to be some filter. Another option would be having attributes as roles, but all of this could also be a claim. Long story short: I do not yet fully know what I want ...

The final thing I am still pondering is what the minimal implementation would be. It seems that most implementations are based on the EF core one, which offers a lot of flexibility in dynamically putting together whatever you want, but it seems to me a bit over-engineered for my purposes.

Maxhy commented 8 months ago

Thanks for the update. This is interesting.

At this moment I'm able to avoid using a ldap UserStore by just creating claims and avoiding calling 'uncompatible' methods from UserManager which do not rely on the current user claims. Having real support of UserStore may be a preferable option when using Identity on the long run. I guess I understand your comment in regards to password hash, but can't this be avoided by just overriding password check to perform a Bind instead on an inherited UserManager class?

I'm not sure what the issue would be with Roles, some new mapping will be required here as you cannot know in advance the expected Roles syntax, but group membership will be a good solution here as you said and as you already have it on the claims list. At this moment it is what I'm doing to deal with UserManager role check (which is kind of a hack but it works without having to implement UserStore / RoleStore as hopefully the check is being done on current user claims...):

if (claimsPrincipal.HasClaim(ClaimTypes.GroupSid, groupName))
{
   claims.Add(new Claim(ClaimTypes.Role, role));
}
[...]
if (claims.Count > 0)
{
    // We have to create a new identity because this is currently done in ad-hoc...
    claimsPrincipal.AddIdentity(new ClaimsIdentity(claims));
}

Be aware that for some application using the internal Identity EF store or Ldap would be a configuration up to the application administrator to decide. The less we have to deal with dedicated Ldap object from an API consumer point of view, the best it is for such use cases.

crowbar27 commented 8 months ago

I guess I understand your comment in regards to password hash, but can't this be avoided by just overriding password check to perform a Bind instead on an inherited UserManager class?

TBH, I did not consider UserManager at all, because from what I read in the documentation, it is optional. But CheckPasswordAsync could be a solution, although I am not completely sold on that it checks the password against the database. From what I read from the documentation, this could also be a simple complexity check. From your comments, I assume you work on some kind of custom Identity stuff. Do you happen to know what this actually does?

Maxhy commented 8 months ago

Yes, CheckPasswordAsync does check for password match (against the database here by default). It is called by SignInManager.PasswordSignInAsync btw.

crowbar27 commented 8 months ago

Yes, CheckPasswordAsync does check for password match (against the database here by default). It is called by SignInManager.PasswordSignInAsync btw.

This is actually a very valuable piece of information. I did not know about that, but only about the store, which must check against the hash.

crowbar27 commented 8 months ago

Be aware that for some application using the internal Identity EF store or Ldap would be a configuration up to the application administrator to decide. The less we have to deal with dedicated Ldap object from an API consumer point of view, the best it is for such use cases.

Just for clarification: you envisage a usage scenario where LDAP for the user store would be combined with an EF Core store for something else (the claims, for instance), and in this case, it would be probematic if LDAP would have requirements wrt the user object (like having to implement ILdapUser). I will consider using Attributes instead for DirectoryAuthentication, but I think it will be difficult for LdapAuthentication, because one of my primary goals is not breaking existing code. Removing ILdapUser would be a breaking change. I might consider it for version 2 though, which I plan to switch to async/await once Novell 4 is out.

crowbar27 commented 2 months ago

A quick update on that issue: I found the time to think a bit about this and I have a first implementation in the main branch. The one thing that is missing atm is the ability to retrieve users based on role (group) claims, bc the ILdapSearchService does not support expanding groups yet.