-
Notifications
You must be signed in to change notification settings - Fork 27
Detect appropriate membership validator strategy by default #58
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
Changes from all commits
cd3934e
22d7995
838c8b1
724177b
fe976b4
941a577
60ee236
18cdf65
7803978
9462676
013bb28
963297f
f506dc3
ba0792c
69999cb
4a75932
56e1fd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
module GitHub | ||
class Ldap | ||
module MembershipValidators | ||
# Detects the LDAP host's capabilities and determines the appropriate | ||
# membership validation strategy at runtime. Currently detects for | ||
# ActiveDirectory in-chain membership validation. An explicit strategy can | ||
# also be defined via `GitHub::Ldap#membership_validator=`. See also | ||
# `GitHub::Ldap#configure_membership_validation_strategy`. | ||
class Detect < Base | ||
# Internal: The capability required to use the ActiveDirectory strategy. | ||
# See: http://msdn.microsoft.com/en-us/library/cc223359.aspx. | ||
ACTIVE_DIRECTORY_V61_R2_OID = "1.2.840.113556.1.4.2080".freeze | ||
|
||
def perform(entry) | ||
# short circuit validation if there are no groups to check against | ||
return true if groups.empty? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something that we want all validators to do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jch unsure, it's something I don't like and want to figure out the right solution to but going to punt on for now (which means cargo-culting here for now). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I don't see us building a ton of strategies, so some duplication is fine for me. Just an observation. |
||
|
||
strategy.perform(entry) | ||
end | ||
|
||
# Internal: Returns the membership validation strategy object. | ||
def strategy | ||
@strategy ||= begin | ||
strategy = detect_strategy | ||
strategy.new(ldap, groups) | ||
end | ||
end | ||
|
||
# Internal: Detects LDAP host's capabilities and chooses the best | ||
# strategy for the host. | ||
# | ||
# If the strategy has been set explicitly, skips detection and uses the | ||
# configured strategy instead. | ||
# | ||
# Returns the strategy class. | ||
def detect_strategy | ||
case | ||
when GitHub::Ldap::MembershipValidators::STRATEGIES.key?(strategy_config) | ||
GitHub::Ldap::MembershipValidators::STRATEGIES[strategy_config] | ||
when active_directory_capability? | ||
GitHub::Ldap::MembershipValidators::STRATEGIES[:active_directory] | ||
else | ||
GitHub::Ldap::MembershipValidators::STRATEGIES[:recursive] | ||
end | ||
end | ||
|
||
# Internal: Returns the configured membership validator strategy Symbol. | ||
def strategy_config | ||
ldap.membership_validator | ||
end | ||
|
||
# Internal: Detect whether the LDAP host is an ActiveDirectory server. | ||
# | ||
# See: http://msdn.microsoft.com/en-us/library/cc223359.aspx. | ||
# | ||
# Returns true if the host is an ActiveDirectory server, false otherwise. | ||
def active_directory_capability? | ||
capabilities[:supportedcapabilities].include?(ACTIVE_DIRECTORY_V61_R2_OID) | ||
end | ||
|
||
# Internal: Returns the Net::LDAP::Entry object describing the LDAP | ||
# host's capabilities (via the Root DSE). | ||
def capabilities | ||
ldap.capabilities | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
require_relative '../test_helper' | ||
|
||
# NOTE: Since this strategy is targeted at detecting ActiveDirectory | ||
# capabilities, and we don't have AD setup in CI, we stub out actual queries | ||
# and test against what AD *would* respond with. | ||
|
||
class GitHubLdapDetectMembershipValidatorsTest < 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') | ||
@validator = GitHub::Ldap::MembershipValidators::Detect | ||
end | ||
|
||
def make_validator(groups) | ||
groups = @domain.groups(groups) | ||
@validator.new(@ldap, groups) | ||
end | ||
|
||
def test_defers_to_configured_strategy | ||
@ldap.configure_membership_validation_strategy(:classic) | ||
validator = make_validator(%w(group)) | ||
|
||
assert_kind_of GitHub::Ldap::MembershipValidators::Classic, validator.strategy | ||
end | ||
|
||
def test_detects_active_directory | ||
caps = Net::LDAP::Entry.new | ||
caps[:supportedcapabilities] = | ||
[GitHub::Ldap::MembershipValidators::Detect::ACTIVE_DIRECTORY_V61_R2_OID] | ||
|
||
validator = make_validator(%w(group)) | ||
@ldap.stub :capabilities, caps do | ||
assert_kind_of GitHub::Ldap::MembershipValidators::ActiveDirectory, | ||
validator.strategy | ||
end | ||
end | ||
|
||
def test_falls_back_to_recursive | ||
caps = Net::LDAP::Entry.new | ||
caps[:supportedcapabilities] = [] | ||
|
||
validator = make_validator(%w(group)) | ||
@ldap.stub :capabilities, caps do | ||
assert_kind_of GitHub::Ldap::MembershipValidators::Recursive, | ||
validator.strategy | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not default
:detect
in the method parameter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jch the method is always called with an argument, which will be
nil
or""
if no explicit option has been set, so the default here doesn't really kick in in practice. If anything, the default value could be removed, but here it's used to communicate thatnil
will be handled appropriately.