search

cschiewek / devise_ldap_authenticatable

Devise Module for LDAP
MIT License
594 stars 359 forks source link

Check User status in LDAP Group w/out admin auth #162

Closed grubernaut closed 10 years ago

grubernaut commented 10 years ago

Should allow a user to check group status without requiring admin authentication. Not including AD at this point.

Signed-off-by: "Jake Champlin" jake.champlin.27@gmail.com

stevenyxu commented 10 years ago

Thanks for the patch @jchampy. I can own this and power through merging it on Saturday. I like the interface and appreciate the contribution. I'll toss in an acceptance test with our LDAP mock server, but if you wanted a shot at that @jchampy or had something in the works, let me know, and I'll give you dibs on doing a pass at it. Also, sorry but a quick request: I'd love to bring the code into line with the existing code style (2 space indents rather than tab indents). I can do the formatting myself, but I'd rather have the lines blame directly to you so you get credit :). If you want to update this PR with a commit with the 2 space indents, that would be awesome; if not or if you don't care, that's cool too.

Thanks again!

grubernaut commented 10 years ago

Thanks for the kind words and help @cairo140!

I'll push a commit to fix the formatting styles later today.

I'll hack on it this weekend and attempt an acceptance test with the server, and I'll stay in touch with my progress.

stevenyxu commented 10 years ago

@jchampy sorry for the delay. I'm checking it out now and it appears there are a few dead ends in the code like calls to auth_user_groups. Also, the integration tests test a function that hasn't been added to the Devise model. I'll fill in these as best as I can. If you see something I missed let me know.

grubernaut commented 10 years ago

@cairo140 no worries. I had planned to incorporate the auth_user_groups function to return a list of all groups that a member was in without binding to an admin account, but didn't get around to it. I don't know if it is essential or not, but it is not for my application of the gem.

If you want to add the function to the Devise model I would appreciate that.

stevenyxu commented 10 years ago

@jchampy sounds good. I thought a bunch on the use of exposing two separate Devise model-level functions for the admin-powered group check and the non-admin-powered group check. I didn't see a use case that made sense to support that extra interface, so I stuck with the in_ldap_group? method on the Devise model and had it go through a flow that checked your new static config to determine if it should use the user's own LDAP binding or the admin binding. I left some semi-coherent remarks in the commit. If I missed some compelling use case you had in mind to support the dual user_in_ldap_group? + in_ldap_group? interface, let me know. Thanks for the patch!

grubernaut commented 10 years ago

Thanks for the fixes @cairo140. Your updates make sense to me. Less clutter. Thanks for the help!