We use an OpenLDAP-server, which backs a samba-server.

I'm using our ldap-groups to grant drupal-roles. The setup is as follows:

ou=accounts,dc=kgs-brinkum,dc=dyndns,dc=org contains the accounts which are distinguished be their uid-attribute.
ou=groups,dc=kgs-brinkum,dc=dyndns,dc=org contains the groups, each group has a multivalued member memberUid which holds the uid of the associated members.

I habe no problem to map this into the configuration of the ldap-server, but when the authorization-module looks for the groups it fails to recognize the membership.

As far as I'm understanding it, ldap_servers_get_first_rdn_value_from_dn in LdapServer.class.php:1727 expects the id of the user as part of the groups dn...

I solved the problem one level above this, my path is attached to this issue.

I must confess, that I don't fully understand the nature of recursive groups, maybe this would open a more elegant solution...

CommentFileSizeAuthor
#17 ldap_servers-nesting-1965160-17.patch1.1 KBsi.mon
#12 ldap_servers-nesting-1965160-0.patch5.74 KBtYb
#11 1965160_attempt2.patch2.01 KBjens.kuespert
#3 1965163.patch3.81 KBjohnbarclay
ldap-server.patch1.04 KBjens.kuespert
ldap_config.zip1.81 KBjens.kuespert
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Title: Group-Membership is not recognized » LDAP Servers: Group-Membership not derived correctly for non nested groups
Status: Active » Needs review

Thanks. This looks good. This needs some more testing before I close the issue.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented

I committed this also.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Status: Needs review » Needs work
FileSize
1965163.patch3.81 KB

I reverted this. It failed the simpletests. The idea is that ->groupMembershipsFromEntryResursive is only called once when nested groups are disabled. The attached patch may be a better fix because it avoids the extra query that should only be performed if going another level deeper in the nesting. It passes all the simpletests.

Can you test it out and see if it meets your use case?

Its always possible that the simpletests are wrong also as they use a mock ldap server.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Priority: Normal » Major

I've merged #1879168: Ldap Authorization: Derive from Group-dn does not work with this.

jens.kuespert’s picture

jens.kuespert CreditAttribution: jens.kuespert commented
Status: Needs work » Fixed

Thanks for your work. I updated today and tested the authorization in my setup. Everything seems to work as expected now.

My first patch was nothing but a crude hack, I'm glad you solved the problem the right way.

Hope it's OK, if set this issue to fixed.

jens.kuespert’s picture

jens.kuespert CreditAttribution: jens.kuespert commented
Status: Fixed » Active

Today I got around to test more thoroughly. It seems that my patch got applied to the wrong code-segment.

I'll try to dig into this in the next few days. Maybe it would be a good idea to remove the patch from the wrong place (groupUserMembershipsFromUserAttr in LdapServer.class.php).

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented

My patch was different than yours. It fixed the recursive groups when nested was disabled. I thought that was what your patch was getting at.

jens.kuespert’s picture

jens.kuespert CreditAttribution: jens.kuespert commented

Not exactly. In our setup we don't have recursive or nested groups. The group-membership is determined by a multi-valued attribute of the group-entry.

In LdapServer::groupUserMembershipsFromEntry the group-membership is correctly queried, but in groupMembershipsFromEntryRecursive the entry isn't recocgnized and asigned to $member_id. So my patch tried to work around this whole recursive thing, since we don't need it...

In the next days I'll try to understand the test-cases and attempt to come up with a cleaner solution.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented

Think of the recursive function as improperty named. In the case where nested groups are not used, it only loops through once to avoid unnecessary duplication of code. I think your patch will work if put in the recursive function and not hard coding the attribute/dn used.

Some background can be found in #1879168: Ldap Authorization: Derive from Group-dn does not work

jens.kuespert’s picture

jens.kuespert CreditAttribution: jens.kuespert commented
Assigned: Unassigned » jens.kuespert

The cited issue seems to be exactly the same. At least the setup and all the property names are matching. Same goes for the reported behaviour. Even the mentioned simple solution does the same.

What I do not understand is, why groupMembershipsFromEntryResursive fails to add the groups of the first level... Hm... Maybe this an "one off" problem? I'll dig into this.

[Hope it's OK if I assign this one to me.]

jens.kuespert’s picture

jens.kuespert CreditAttribution: jens.kuespert commented
FileSize
1965160_attempt2.patch2.01 KB

OK, I think it's fixed now.

My first assumption about the "one-off" thing was wrong. But it seems to me, that the code in groupMembershipsFromEntryRecursive failed to recognize the group membership before descending into the recursion. A possible solution to this is another check, which checks if the group_entry in question looks like group we were looking for. My guess is, that this can be seen on the objectclass-attribute. If this matches, the entry is indeed a group the user belongs to.

This time I created a private branch to work on the changes, the attached patch branched from version 043a950950018c07e8a5afa33454b95154f84fe4 of 7.x-2.x.

Since I didn't understand how to run the tests, I'd like to submit this patch for further consideration. Please let me know, if there is anything else I could do to help testing.

tYb’s picture

tYb CreditAttribution: tYb commented
Version: 7.x-2.x-dev » 7.x-2.0-beta5
FileSize
ldap_servers-nesting-1965160-0.patch5.74 KB

Here the first patch ldap-server for 7.x-2.0-beta5, which corrected the issue for me.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Version: 7.x-2.0-beta5 » 7.x-2.x-dev
worldcitizen’s picture

worldcitizen CreditAttribution: worldcitizen commented

Dear all we've tested: 1965160_attempt2.patch it solved the issue for us.

We hoped it would be also incorporated in 7.x-2.0-beta6. It wasn't, as we didn't remember right away we've patched ldap users with ldap group rights couldn't login any more.

Can the patch please be interoperated in 7.x-2.0-beta6+ ?

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Status: Active » Needs review

I committed 1965160_attempt2.patch to 7.x-2.x-dev.

johnbarclay’s picture

johnbarclay CreditAttribution: johnbarclay commented
Issue summary: View changes
Status: Needs review » Closed (fixed)
si.mon’s picture

si.mon CreditAttribution: si.mon commented
FileSize
ldap_servers-nesting-1965160-17.patch1.1 KB

Patch suggested in #11 didn't work. Same for 7.x-2.x-dev
I've attached a patch which fixed it for me.