plone / plone.app.ldap

Plone UI and integration of the functionality provided by LDAPMultiPlugins, LDAPUserFolder and PloneLDAP
3 stars 10 forks source link

AD & other misc. fixes #21

Closed adaugherity closed 9 years ago

adaugherity commented 10 years ago

First of all, I apologize for the size and scope of this pull request -- I started off with what I thought was a simple fix for #7, then I discovered that my change only fixed it for import, not new configuration, and that new configuration was broken for non-AD also. As I learned my way around the code I fixed about half a dozen other bugs along the way. I can split this into multiple PRs if you would prefer.

Summary of fixes:

Regarding Active Directory, I tried to make my fixes match what I thought the intent was. I'm not sure if a separate AD plugin is still needed -- from what I can tell, the major differences are (a) read-only access, and (b) it enumerates group membership via the memberOf attribute on user objects, rather than the member attribute on group objects.

However, the standard LDAP plugin works fine with my AD server for authentication and user information, and RW access works for modifying group membership. (Group addition fails with “server unwilling to perform”, and we want to limit user addition/modification/password change to the AD side, so I haven’t tested those -- it's not 100%, but having some write access is better than none, and reading works fine. Also, nested groups do not appear to work with either plugin, but I don't normally use them.)

My AD groups are presented via LDAP as group class objects, with member attributes referring to a user’s full DN, much like standard LDAP. The memberOf user attribute is still present, but appears to be computed rather than stored in the user object. If the other method was once required, it no longer appears to be.

jensens commented 9 years ago

this one is hanging and needs a review. Last authors with major changes here from commits are @mauritsvanrees and @hvelarde - so a nudge you a bit to have a look at this one please. Anyone else?

adaugherity commented 9 years ago

I actually have a few more fixes (including one for #10) in my import_export branch (which also contains all commits from this branch); I had made a different branch because I thought this PR was under review and maybe you didn't wan't so many commits/fixes in one PR, but I never got around to submitting another PR or mentioning it in this one.

Please let me know if should rebase/squash/fixup my commits and/or split them into multiple PRs, or if I need to sign a contributor agreement or anything. I realize it's a lot of commits, including a couple mistakes I made and then fixed later, so I'm willing to do the git rebase work if desired.

hvelarde commented 9 years ago

@adaugherity in general is a bad idea to mix the scope of a PR because it makes the life of the code reviewers hard.

as long as I can see this package is not being tested, so I will start adding (on another branch) support for Travis CI and Coveralls.

I'm not able to evaluate your changes, as I personally don't use the package, and I know @mauritsvanrees is on vacations.

ebrehault commented 9 years ago

@adaugherity the code looks quite good to me. By the way, I have tried it on Plone 5 and it does not work but master branch does work either, the reason is mainly (as far as I can see) that we depends on five.formlib. I think we should review and merge this PR quickly and then start from here to make it Plone 5 compliant. I think it is quite critical for Plone 5. @jensens we should discuss that at next FWT meeting, what do you think?

hvelarde commented 9 years ago

it's really scary that after 7 years this package doesn't have a single test; I'm working on that right now on #25.

ableeb commented 9 years ago

I don't see you in the plone developers team so please sign a Plone contributor agreement so that we can merge your changes. You can email it directly to andy@plone.org. Thanks!

jensens commented 9 years ago

@ebrehault yes, ldap on plone 5 should work. I will also test pas.plugins.ldap soon with plone5, my preferred solution for ldap with plone.

jensens commented 9 years ago

btw, pas.plugins.ldap has a complete ldap test layer with ldifs and the possibility for custom schemas and ldifs. we may want to factor that out into an own package to be shared by plone.app.ldap and pas.plugins.ldap.

jensens commented 9 years ago

I merge this one. Its good, cleans up things and it hangs for too long. Since plone.app.ldap is not part of core you have time to fix things if something breaks.