Skip to content

Recursive memberOf group search strategy #66

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.

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions lib/github/ldap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class Ldap
require 'github/ldap/virtual_group'
require 'github/ldap/virtual_attributes'
require 'github/ldap/instrumentation'
require 'github/ldap/member_of'
require 'github/ldap/members'
require 'github/ldap/membership_validators'

Expand Down
22 changes: 22 additions & 0 deletions lib/github/ldap/member_of.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'github/ldap/member_of/classic'
require 'github/ldap/member_of/recursive'

module GitHub
class Ldap
# Provides various strategies to search for groups a user is a member of.
#
# For example:
#
# group = domain.groups(%w(Engineering)).first
# strategy = GitHub::Ldap::MemberOf::Recursive.new(ldap)
# strategy.perform(user) #=> [#<Net::LDAP::Entry>]
#
module MemberOf
# Internal: Mapping of strategy name to class.
STRATEGIES = {
:classic => GitHub::Ldap::MemberOf::Classic,
:recursive => GitHub::Ldap::MemberOf::Recursive
}
end
end
end
46 changes: 46 additions & 0 deletions lib/github/ldap/member_of/classic.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
module GitHub
class Ldap
module MemberOf
# Look up the groups an entry is a member of.
class Classic
include Filter

# Internal: The GitHub::Ldap object to search domains with.
attr_reader :ldap

# Public: Instantiate new search strategy.
#
# - ldap: GitHub::Ldap object
# - options: Hash of options (unused)
def initialize(ldap, options = {})
@ldap = ldap
@options = options
end

# Public: Performs search for groups an entry is a member of, including
# subgroups.
#
# Returns Array of Net::LDAP::Entry objects.
def perform(entry)
filter = member_filter(entry)

if ldap.posix_support_enabled? && !entry[ldap.uid].empty?
filter |= posix_member_filter(entry, ldap.uid)
end

domains.each_with_object([]) do |domain, entries|
entries.concat domain.search(filter: filter)
end
end

# Internal: Domains to search through.
#
# Returns an Array of GitHub::Ldap::Domain objects.
def domains
@domains ||= ldap.search_domains.map { |base| ldap.domain(base) }
end
private :domains
end
end
end
end
70 changes: 70 additions & 0 deletions lib/github/ldap/member_of/recursive.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
module GitHub
class Ldap
module MemberOf
# Look up the groups an entry is a member of, including nested subgroups.
#
# NOTE: this strategy is network and performance intensive.
class Recursive
include Filter

# Internal: The GitHub::Ldap object to search domains with.
attr_reader :ldap

# Internal: The maximum depth to search for subgroups.
attr_reader :depth

# Public: Instantiate new search strategy.
#
# - ldap: GitHub::Ldap object
# - options: Hash of options
def initialize(ldap, options = {})
@ldap = ldap
@options = options
@depth = options[:depth]
end

# Public: Performs search for groups an entry is a member of, including
# subgroups.
#
# Returns Array of Net::LDAP::Entry objects.
def perform(entry)
filter = member_filter(entry)

if ldap.posix_support_enabled? && !entry[ldap.uid].empty?
filter |= posix_member_filter(entry, ldap.uid)
end

entries = domains.each_with_object([]) do |domain, entries|
entries.concat domain.search(filter: filter)
end

entries.each_with_object(entries.dup) do |entry, entries|
entries.concat search_strategy.perform(entry)
end.select { |entry| group?(entry) }
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks pretty innocuous, but it can result in N_M *recursive_ queries and exceptionally bad performance for large data sets.

It might make more sense to inline the relevant logic from the search strategy in order to recursively search for multiple entries at a given time (instead of individually). This warrants more consideration and testing.

end

# Internal: Domains to search through.
#
# Returns an Array of GitHub::Ldap::Domain objects.
def domains
@domains ||= ldap.search_domains.map { |base| ldap.domain(base) }
end
private :domains

# Internal: The search strategy to recursively search for nested
# subgroups with.
def search_strategy
@search_strategy ||=
GitHub::Ldap::Members::Recursive.new ldap,
depth: depth,
attrs: %w(objectClass)
end

# Internal: Returns true if the entry is a group.
def group?(entry)
GitHub::Ldap::Group.group?(entry[:objectclass])
end
end
end
end
end
31 changes: 31 additions & 0 deletions test/member_of/classic_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require_relative '../test_helper'

class GitHubLdapClassicMemberOfTest < GitHub::Ldap::Test
def setup
@ldap = GitHub::Ldap.new(options.merge(search_domains: %w(dc=github,dc=com)))
@domain = @ldap.domain("dc=github,dc=com")
@entry = @domain.user?('user1')
@strategy = GitHub::Ldap::MemberOf::Classic.new(@ldap)
end

def find_group(cn)
@domain.groups([cn]).first
end

def test_finds_groups_entry_is_a_direct_member_of
member_of = @strategy.perform(@entry)
assert_includes member_of.map(&:dn), find_group("nested-group1").dn
end

def test_finds_subgroups_entry_is_a_member_of
skip "Classic strategy does not support nested subgroups"
member_of = @strategy.perform(@entry)
assert_includes member_of.map(&:dn), find_group("head-group").dn
assert_includes member_of.map(&:dn), find_group("tail-group").dn
end

def test_excludes_groups_entry_is_not_a_member_of
member_of = @strategy.perform(@entry)
refute_includes member_of.map(&:dn), find_group("ghe-admins").dn
end
end
30 changes: 30 additions & 0 deletions test/member_of/recursive_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
require_relative '../test_helper'

class GitHubLdapRecursiveMemberOfTest < GitHub::Ldap::Test
def setup
@ldap = GitHub::Ldap.new(options.merge(search_domains: %w(dc=github,dc=com)))
@domain = @ldap.domain("dc=github,dc=com")
@entry = @domain.user?('user1')
@strategy = GitHub::Ldap::MemberOf::Recursive.new(@ldap)
end

def find_group(cn)
@domain.groups([cn]).first
end

def test_finds_groups_entry_is_a_direct_member_of
member_of = @strategy.perform(@entry)
assert_includes member_of.map(&:dn), find_group("nested-group1").dn
end

def test_finds_subgroups_entry_is_a_member_of
member_of = @strategy.perform(@entry)
assert_includes member_of.map(&:dn), find_group("head-group").dn
assert_includes member_of.map(&:dn), find_group("tail-group").dn
end

def test_excludes_groups_entry_is_not_a_member_of
member_of = @strategy.perform(@entry)
refute_includes member_of.map(&:dn), find_group("ghe-admins").dn
end
end