Skip to content

Check memberships recursively #40

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.

Sign up for GitHub

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

Merged
MikeMcQuaid merged 6 commits into from
Aug 28, 2014
Merged

Check memberships recursively #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be83e88
Add failing test case for deeply nested membership checks
mtodd Aug 27, 2014
fc34566
Adjust group count
mtodd Aug 28, 2014
a9db2f7
Fix LDIF entry
mtodd Aug 28, 2014
68b2451
Disable PosixGroup validation unless enabled
mtodd Aug 28, 2014
d29f991
Downcase group classes to match Net::LDAP behavior
mtodd Aug 28, 2014
055ac02
Test Group#group? checking
mtodd Aug 28, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/github/ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def group(base_name)
def load_group(group_entry)
if @virtual_attributes.enabled?
VirtualGroup.new(self, group_entry)
elsif PosixGroup.valid?(group_entry)
elsif posix_support_enabled? && PosixGroup.valid?(group_entry)
Copy link
Member Author

@mtodd mtodd Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skips trying PosixGroup unless it's enabled. It's on by default, so this doesn't result in changed behavior, just means we can disable this behavior at will.

PosixGroup.new(self, group_entry)
else
Group.new(self, group_entry)
Expand Down
5 changes: 4 additions & 1 deletion lib/github/ldap/group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,12 @@ def member_names
# Internal - Check if an object class includes the member names
# Use `&` rathen than `include?` because both are arrays.
#
# NOTE: object classes are downcased by default in Net::LDAP, so this
# will fail to match correctly unless we also downcase our group classes.
#
# Returns true if the object class includes one of the group class names.
def group?(object_class)
!(GROUP_CLASS_NAMES & object_class).empty?
!(GROUP_CLASS_NAMES.map(&:downcase) & object_class.map(&:downcase)).empty?
Copy link
Member Author

@mtodd mtodd Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary to properly partition groups from members here:

github-ldap/lib/github/ldap/group.rb

Lines 109 to 114 in 744b141

# Internal - Divide members of a group in user and subgroups.
#
# Returns two arrays, the first one with subgroups and the second one with users.
def groups_and_members
member_entries.partition {|e| group?(e[:objectclass])}
end

As documented, Net::LDAP downcases object classes (et al) by default.

Copy link
Contributor

@sbryant sbryant Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

end

# Internal - Generate a hash with all the group DNs for caching purposes.
Expand Down
7 changes: 7 additions & 0 deletions test/domain_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,13 @@ def test_membership_in_subgroups
assert @domain.is_member?(user, %w(enterprise-ops)),
"Expected `enterprise-ops` to include the member `#{user.dn}`"
end

def test_membership_in_deeply_nested_subgroups
assert user = @ldap.domain('uid=user1.1.1.1,ou=users,dc=github,dc=com').bind

assert @domain.is_member?(user, %w(group1)),
"Expected `group1` to include the member `#{user.dn}` via deep recursion"
end
end

class GitHubLdapPosixGroupsWithRecursionFallbackTest < GitHub::Ldap::Test
Expand Down
55 changes: 55 additions & 0 deletions test/fixtures/github-with-subgroups.ldif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,29 @@ objectClass: groupOfNames
member: uid=calavera,ou=users,dc=github,dc=com
member: uid=rubiojr,ou=users,dc=github,dc=com

dn: cn=group1,ou=groups,dc=github,dc=com
cn: group1
objectClass: groupOfNames
member: uid=user1,ou=users,dc=github,dc=com
member: cn=group1.1,ou=groups,dc=github,dc=com

dn: cn=group1.1,ou=groups,dc=github,dc=com
cn: group1
objectClass: groupOfNames
member: uid=user1.1,ou=users,dc=github,dc=com
member: cn=group1.1.1,ou=groups,dc=github,dc=com

dn: cn=group1.1.1,ou=groups,dc=github,dc=com
cn: group1
objectClass: groupOfNames
member: uid=user1.1.1,ou=users,dc=github,dc=com
member: cn=group1.1.1.1,ou=groups,dc=github,dc=com

dn: cn=group1.1.1.1,ou=groups,dc=github,dc=com
cn: group1
objectClass: groupOfNames
member: uid=user1.1.1.1,ou=users,dc=github,dc=com

# Users

dn: ou=users,dc=github,dc=com
Expand Down Expand Up @@ -89,3 +112,35 @@ uid: mtodd
userPassword: passworD1
mail: mtodd@github.com
objectClass: inetOrgPerson

dn: uid=user1,ou=users,dc=github,dc=com
uid: user1
sn: user1
cn: user1
userPassword: passworD1
mail: user1@github.com
objectClass: inetOrgPerson

dn: uid=user1.1,ou=users,dc=github,dc=com
uid: user1.1
sn: user1.1
cn: user1.1
userPassword: passworD1
mail: user1.1@github.com
objectClass: inetOrgPerson

dn: uid=user1.1.1,ou=users,dc=github,dc=com
uid: user1.1.1
sn: user1.1.1
cn: user1.1.1
userPassword: passworD1
mail: user1.1.1@github.com
objectClass: inetOrgPerson

dn: uid=user1.1.1.1,ou=users,dc=github,dc=com
uid: user1.1.1.1
sn: user1.1.1.1
cn: user1.1.1.1
userPassword: passworD1
mail: user1.1.1.1@github.com
objectClass: inetOrgPerson
8 changes: 7 additions & 1 deletion test/group_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ def setup
@group = @ldap.group("cn=enterprise,ou=groups,dc=github,dc=com")
end

def test_group?
object_classes = %w(groupOfNames)
assert @group.group?(object_classes)
assert @group.group?(object_classes.map(&:downcase))
end
Copy link
Member Author

@mtodd mtodd Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second assertion fails without explicitly downcasing GROUP_CLASS_NAMES in Group#group?.


def test_subgroups
assert_equal 3, @group.subgroups.size
end
Expand All @@ -24,7 +30,7 @@ def test_members_from_subgroups

def test_all_domain_groups
groups = groups_domain.all_groups
assert_equal 4, groups.size
assert_equal 8, groups.size
Copy link
Member Author

@mtodd mtodd Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to fixture changes.

end

def test_filter_domain_groups
Expand Down