Skip to content

Additional query attributes option for Member search strategy #68

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
mtodd merged 1 commit into from
Dec 3, 2014
Merged

Additional query attributes option for Member search strategy #68

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
Take attrs option for Member search strategy
  • Loading branch information
@mtodd
mtodd committed Dec 3, 2014
commit 8056ecd74133bfc7938176ebc0f51067bf63f405
10 changes: 7 additions & 3 deletions lib/github/ldap/members/recursive.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@ class Recursive
include Filter

DEFAULT_MAX_DEPTH = 9
ATTRS = %w(member uniqueMember memberUid)
DEFAULT_ATTRS = %w(member uniqueMember memberUid)

@jch jch Dec 3, 2014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This existed before, so I'm not asking for a change, but are these standard LDAP attributes? Or are they attributes we expect when using this strategy?

@mtodd mtodd Dec 3, 2014

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Both, really. member and uniqueMember are attributes of the groupOfNames and groupOfUniqueNames object classes (and Active Directory's group category), and memberUid is for the posixGroup object class.

Hardcoding these here (and elsewhere) is a smell. Addressing that smell is a fairly large engineering undertaking that isn't possible right now. I have some ideas I'd like to run by you, though, for the future.


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

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

# Internal: The attributes to search for.
attr_reader :attrs

# Public: Instantiate new search strategy.
#
# - ldap: GitHub::Ldap object
Expand All @@ -25,6 +28,7 @@ def initialize(ldap, options = {})
@ldap = ldap
@options = options
@depth = options[:depth] || DEFAULT_MAX_DEPTH
@attrs = Array(options[:attrs]).concat DEFAULT_ATTRS
end

# Public: Performs search for group members, including groups and
Expand Down Expand Up @@ -95,7 +99,7 @@ def member_entries(entry)
# Returns an Array of Net::LDAP::Entry objects.
def entries_by_dn(members)
members.map do |dn|
ldap.domain(dn).bind(attributes: ATTRS)
ldap.domain(dn).bind(attributes: attrs)
end.compact
end
private :entries_by_dn
Expand All @@ -106,7 +110,7 @@ def entries_by_dn(members)
def entries_by_uid(members)
filter = members.map { |uid| Net::LDAP::Filter.eq(ldap.uid, uid) }.reduce(:|)
domains.each_with_object([]) do |domain, entries|
entries.concat domain.search(filter: filter, attributes: ATTRS)
entries.concat domain.search(filter: filter, attributes: attrs)
end.compact
end
private :entries_by_uid
Expand Down