-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support active directory #18
base: master
Are you sure you want to change the base?
Conversation
Cool, thanks Chris! I hope to get time to review this over the week. |
Hi, I've updated it again with another commit. We had a requirement to search ldap from multiple parts in the ldap tree. e.g. we have users under /Users but also under /nsl/Users so we want to search from / and not /Users. So I found to do that, I needed two things: Firstly I had to put The other thing I needed to do is pass I also made getLDAPContext a protected method rather than static so that users can potentially override it if they want to inherit it and override it with other parameters. Now for whatever obscure reason, if you pass a SearchControls class to the search() method, it doesn't support the BasicAttributes or Attributes parameters. So instead you have to construct a filter string of the kind "variableName=value", so that's a slight difference there in the search setup. |
Made another change to make env a member of the class and make getBaseLDAPEnvironment protected instead of private to give maximum flexibility for users of the class to set parameters and override if necessary. |
I forgot that I did this pull request before, looks like you never got around to it. Because I've worked off the same repository, looks like my new work is added to the same pull request.... Anyway, if you ever get around to looking at it, these are my comments on the changes as of now:
|
Thanks @chrisbitmead we'll have to look at the versioning. should be 5.x for Grails 5 support. I'll look through this now, sorry for letting the last one drop, I got stuck waiting on maven repo stuff IIRC. |
Looks like I need to do a bit of testing on this Chris and we need to update the Doco too. The major version number should match the Grails version supported. I've cloned your repo to have a good look and make some changes. It'd be good to do these as branches as opposed to main... we have a grails 3 branch , should have a grails 4 branch and then pull in the grails 5 branch... give me a call if you can, cheers, and thanks for your work! |
OK, because the grails commands and templates have gone, the tests that create test projects don't work, which is a bummer. |
OK, I'm not that familiar with how people normally do git versions and stuff with grails plugins, it seems like a bit of a can of worms. I'm sure you can import the code into a branch if that works for you. And yeah, sorry I didn't know what the version convention was. It's possible I've broken something in whatever LDAP version you originally used by way of supporting the Active Directory version of LDAP. I used pattern matching to extract the right details, on the theory that the global pattern could probably be changed one way or the other for different LDAPs, but then again I may be missing the real issues. And I don't know, you may have issues getting Active Directory to test with, I don't know. Not to mention time to look into it. What LDAP did you use originally, I don't know if I have time either to set that up. Not sure what you mean by grails commands and templates have gone, what that's about. I did notice there were some templates, but wasn't sure what the overall design was trying to achieve with those. |
@chrisbitmead it's all good. Some of the underlying changes in Grails mean that some stuff is out of date. I've fixed a few things and committed fixes up to Shiro 1.13.0 with Grails 4. Shiro 2.01 will require us to move to Grails 5 for the Java 11 requirements. I'm currently merging in your changes, first to get us up to Shiro 2.0.1 and Grails 5, then I'll merge in changes for Active Directory support (which i may need some more advice on from you). I'll do them as separate things, making sure that we have all the tests working. We'll need to add tests for the AD stuff. Currently the tests run up apacheds for testing with LDAP. |
OK upgraded to Grails 5 and Shiro 2.0.1 and tests now working. Updated some doco. Breaking change is that you need to include Shiro in your dependencies now (see doco). Next I'll apply the AD changes to the LDAP Realm. |
This is the simplest thing that could work for supporting active directory. Seemingly, the typical ldap setup looks for roles by starting with the group, and then looking up the users via uniqueMember or some such. Typically in Active Directory you start from the member, and use the memberof attribute to find groups they belong to. (This is the approach of ActiveDirectoryRealm for example). Going the other way is problematic because seemingly the 'member' attribute of group gives the user display name, not the username. Because ldap searches are quite flexible we can support this rather different approach with a small upgrade to the code. When we retrieve the groups from memberof, they will typically be in a ldap hierarchy... e.g. test vs prod and so forth. We only want to extract the ones from the place in the hierarchy we are interested in. We can do that by using the groupPattern with a capture group to only grab the groups that we care about.
This code has been tested in its native environment to work with both regular ldap and active directory.
There's probably some more sophisticated way to support the whole ldap / active directory thing, but this is the simplest thing that could work. In particular, the shiro ActiveDirectoryRealm... and the shiro DefaultLdapRealm, and using those rather than this class. But I haven't gone all the way down the rabbit hole of looking into that.