Also find groups added through groupOfUniqueNames#78
Also find groups added through groupOfUniqueNames#78adamruzicka wants to merge 2 commits intotheforeman:masterfrom
Conversation
adamruzicka
force-pushed
the
unique-group-search
branch
3 times, most recently
from
88e5ed8 to
c827590
Compare
when looking up groups assigned to a user
adamruzicka
force-pushed
the
unique-group-search
branch
from
c827590 to
0667edb
Compare
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
We also have the times_in_groups method (and I wonder why it's there) which may need to be modified to support the same.
|
|
||
| def user_group_filter(uid, user_dn) | ||
| unique_filter = Net::LDAP::Filter.eq('uniquemember', user_dn) & | ||
| Net::LDAP::Filter.eq('objectClass', 'groupOfUniqueNames') |
There was a problem hiding this comment.
There's also groupOfNames. Should that also be supported? We search for it here:
ldap_fluff/lib/ldap_fluff/posix.rb
Line 24 in 77a387a
There was a problem hiding this comment.
There are multiple group-like object types in LDAP. I think it's a high level decision what we support (and document!)
| # return an ldap user with groups attached | ||
| # note : this method is not particularly fast for large ldap systems | ||
| def find_user_groups(uid) | ||
| user = find_user(uid).first |
There was a problem hiding this comment.
Should this somehow yell if there's more than 1 user?
There was a problem hiding this comment.
Can it happen? I was living under the impression that the uid we get should uniquely identify a single object within the subtree set by @base
There was a problem hiding this comment.
It wouldn't be a good practice, but if you organize your LDAP tree with different organization units it can happen.
Concrete example: let's say your base is dc=example,dc=com you can have cn=myuid,ou=People,dc=example,dc=com and cn=myuid,ou=Systems,dc=example,dc=com.
There was a problem hiding this comment.
There can be multiple entries with the same uid (as opposed to dn)
It doesn't seem to be used anywhere. It used to be used when checking whether an user belongs to a set of groups, but that particular implementation was dropped. I'd lean more towards dropping it, if possible. |
| @@ -16,14 +16,17 @@ def find_user(uid, base_dn = @base) | |||
| # return an ldap user with groups attached | |||
| # note : this method is not particularly fast for large ldap systems | |||
| def find_user_groups(uid) | |||
There was a problem hiding this comment.
I was digging deeper on what should be done for POSIX. If we assume this maps to the nss-pam-ldap implementation then according to https://arthurdejong.org/nss-pam-ldapd/README this is the spec:
Currently, two ways of specifying group membership are supported. The first, by using the memberUid attribute, is the simplest and by far the fastest (takes the least number of lookups). The attribute values are user names (same as the uid attribute for posixAccount entries) and are returned without further processing.
The second method is to use DN values in the member attribute (attribute names can be changed by using the attribute mapping options as described in the manual page). This is potentially a lot slower because in the worst case every DN has to be looked up in the LDAP server to find the proper value for the uid attribute.
If the LDAP server supports the deref control (provided by the deref overlay in OpenLDAP) the DN to uid expansing is performed by the LDAP server.
If the DN value already contains a uid value (e.g. uid=arthur, dc=example, dc=com) a further lookup is skipped and the uid value from the DN is used.
For other DN values an extra lookup is performed to expand it to a uid. These lookups are cached and are configurable with the cache dn2uid configuration option.
The member attribute may also contain the DN of another group entry. These nested groups are parsed recursively depending on the nss_nested_groups option.
Currently, the memberOf attribute in posixAccount entries is unsupported.
If we look at the method, then we see there is already a constructor parameter that determines @attr_login, which is not respected right now.
Reading the documentation it's not clear to me if memberUid or member is an XOR, but it looks like groupOfUniqueNames is not part of the posix spec at all.
In #72 (comment) I already noted that I think it's a different flavor we should support. Most likely an expansion of the FreeIPA flavor.
There was a problem hiding this comment.
How about instead of exposing this as a brand new flavor, we allow enabling the use of groupOfNames/groupOfUniqueNames in a similar way how we handle nis netgroups? Something along the lines of adamruzicka/ldap_fluff@unique-group-search...adamruzicka:ldap_fluff:unique-group-search-ext
There was a problem hiding this comment.
Looking at the code I think that could work. For LdapFluff::Posix::MemberService I was debating a subclass but it looks like it's not strictly needed.
Dropping unused code is (almost) always a good idea IMHO. |
|
|
Closing in favor of #88 |
3 participants
when looking up groups assigned to a user