Skip to content

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

Merged
merged 17 commits into from
Nov 4, 2014
Merged
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
40 changes: 40 additions & 0 deletions lib/github/ldap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class Ldap
def_delegator :@connection, :open

attr_reader :uid, :search_domains, :virtual_attributes,
:membership_validator,
:instrumentation_service

# Build a new GitHub::Ldap instance
Expand Down Expand Up @@ -87,6 +88,9 @@ def initialize(options = {})
# when a base is not explicitly provided.
@search_domains = Array(options[:search_domains])

# configure which strategy should be used to validate user membership
configure_membership_validation_strategy(options[:membership_validator])

# enables instrumenting queries
@instrumentation_service = options[:instrumentation_service]
end
Expand Down Expand Up @@ -182,6 +186,23 @@ def search(options, &block)
end
end

# Internal: Searches the host LDAP server's Root DSE for capabilities and
# extensions.
#
# Returns a Net::LDAP::Entry object.
def capabilities
@capabilities ||=
instrument "capabilities.github_ldap" do |payload|
begin
@connection.search_root_dse
rescue Net::LDAP::LdapError => error
payload[:error] = error
# stubbed result
Net::LDAP::Entry.new
end
end
end

# Internal - Determine whether to use encryption or not.
#
# encryption: is the encryption method, either 'ssl', 'tls', 'simple_tls' or 'start_tls'.
Expand Down Expand Up @@ -214,5 +235,24 @@ def configure_virtual_attributes(attributes)
VirtualAttributes.new(false)
end
end

# Internal: Configure the membership validation strategy.
#
# Used by GitHub::Ldap::MembershipValidators::Detect to force a specific
# strategy (instead of detecting host capabilities and deciding at runtime).
#
# If `strategy` is not provided, or doesn't match a known strategy,
# defaults to `:detect`. Otherwise the configured strategy is selected.
#
# Returns the selected membership validator strategy Symbol.
def configure_membership_validation_strategy(strategy = nil)
Copy link
Contributor

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?

def configure_membership_validation_strategy(strategy = :detect)
  @membership_validator ||= strategy.to_sym
end

Copy link
Member Author

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 that nil will be handled appropriately.

@membership_validator =
case strategy.to_s
when "classic", "recursive", "active_directory"
strategy.to_sym
else
:detect
end
end
end
end
10 changes: 9 additions & 1 deletion lib/github/ldap/membership_validators.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'github/ldap/membership_validators/base'
require 'github/ldap/membership_validators/detect'
require 'github/ldap/membership_validators/classic'
require 'github/ldap/membership_validators/recursive'
require 'github/ldap/membership_validators/active_directory'
Expand All @@ -13,6 +14,13 @@ class Ldap
# validator = GitHub::Ldap::MembershipValidators::Classic.new(ldap, groups)
# validator.perform(entry) #=> true
#
module MembershipValidators; end
module MembershipValidators
# Internal: Mapping of strategy name to class.
STRATEGIES = {
:classic => GitHub::Ldap::MembershipValidators::Classic,
:recursive => GitHub::Ldap::MembershipValidators::Recursive,
:active_directory => GitHub::Ldap::MembershipValidators::ActiveDirectory
}
end
end
end
69 changes: 69 additions & 0 deletions lib/github/ldap/membership_validators/detect.rb
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?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that we want all validators to do?

Copy link
Member Author

Choose a reason for hiding this comment

The 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).

Copy link
Contributor

Choose a reason for hiding this comment

The 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
8 changes: 8 additions & 0 deletions test/ldap_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ def test_instruments_search
assert_equal "(uid=user1)", payload[:filter].to_s
assert_equal "dc=github,dc=com", payload[:base]
end

def test_membership_validator_default
assert_equal :detect, @ldap.membership_validator
end

def test_capabilities
assert_kind_of Net::LDAP::Entry, @ldap.capabilities
end
end

class GitHubLdapTest < GitHub::Ldap::Test
Expand Down
49 changes: 49 additions & 0 deletions test/membership_validators/detect_test.rb
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