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.

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
merged 17 commits into from
Nov 4, 2014
Merged

Detect appropriate membership validator strategy by default #58

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
cd3934e
Add membership_validator config option, default to detect
mtodd Oct 24, 2014
22d7995
Add Detect membership validation strategy
mtodd Oct 24, 2014
838c8b1
Move strategies map to GitHub::Ldap::MembershipValidators
mtodd Oct 24, 2014
724177b
Update required capability OID to match 2008 R2 v61
mtodd Oct 24, 2014
fe976b4
Proactively handle capapbility query errors, instrument
mtodd Oct 24, 2014
941a577
Fix detect_strategy doco
mtodd Oct 24, 2014
60ee236
Ensure detect_strategy returns the strategy class
mtodd Oct 24, 2014
18cdf65
s/if/whether/
mtodd Oct 24, 2014
7803978
Move capabilities lookup to GitHub::Ldap
mtodd Oct 24, 2014
9462676
Only instrument when we query for capabilities
mtodd Oct 24, 2014
013bb28
Server returns supportedCapabilities
mtodd Oct 24, 2014
963297f
Test Detect meta-strategy
mtodd Oct 24, 2014
f506dc3
fix typo
Oct 28, 2014
ba0792c
add test for validator detection
Oct 28, 2014
69999cb
Merge remote-tracking branch 'origin/master' into default-membership-…
Oct 28, 2014
4a75932
additional docs on AD membership validator
Oct 28, 2014
56e1fd5
Merge remote-tracking branch 'origin/master' into default-membership-…
Nov 4, 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
Next Next commit
Add membership_validator config option, default to detect
  • Loading branch information
@mtodd
mtodd committed Oct 24, 2014
commit cd3934ec79f2d13d76c095ec769045e5e74687f0
23 changes: 23 additions & 0 deletions lib/github/ldap.rb
View file
Open in desktop
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 @@ -214,5 +218,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