Detect appropriate membership validator strategy by default #58
Conversation
Detects ActiveDirectory capabilities, defaults to Recursive.
| # defaults to `:detect`. Otherwise the configured strategy is selected. | ||
| # | ||
| # Returns the selected membership validator strategy Symbol. | ||
| def configure_membership_validation_strategy(strategy = nil) |
There was a problem hiding this comment.
Why not default :detect in the method parameter?
def configure_membership_validation_strategy(strategy = :detect)
@membership_validator ||= strategy.to_sym
endThere was a problem hiding this comment.
@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.
|
I like how you defined this as another concrete implementation of the strategy base class. |
|
@jch added some tests and moved some things around. Thoughts? |
|
@jch feel free to take this over while I'm out. |
|
Doing some manual testing, but noticing that AD is not returning the control string Since there's no AD integration test, I have a local test file with the following: require_relative "./test_helper"
class DetectActiveDirectoryTest < Minitest::Test
def setup
@service = MockInstrumentationService.new
@ldap = GitHub::Ldap.new(
host: "172.28.128.3",
port: "389",
admin_user: "CN=Administrator,CN=Users,DC=ad,DC=ghe,DC=local",
admin_password: "superp@ssw0rd",
search_domains: ["CN=Users,DC=ad,DC=ghe,DC=local"],
service: @service
)
end
def test_capabilities
assert_includes @ldap.capabilities["supportedcontrol"], GitHub::Ldap::MembershipValidators::Detect::ACTIVE_DIRECTORY_V61_R2_OID
end
def test_active_directory_capability
validator = GitHub::Ldap::MembershipValidators::Detect.new(@ldap, [])
assert_predicate validator, :active_directory_capability?
end
def test_connection
result = @ldap.test_connection
assert_equal 0, result.code
end
def test_search
results = @ldap.search(base: "CN=Users,DC=ad,DC=ghe,DC=local")
assert_equal 1094, results.size
end
endI'll poke further to see if it's the particular version of AD I'm running that's causing this issue. cc @jameswhite |
|
I do see that capability when I connect with Apache Directory Studio though:
|
|
Figured it out. net-ldap wasn't requesting |
|
Depends on #59 to pull in latest changes from net-ldap. |
|
I've started another branch to allow local activedirectory testing #61. |
Detect appropriate membership validator strategy by default
|
@jch good catch RE: |
This adds a meta strategy to detect the best strategy based on the server capabilities. The only real detection is whether the server is an ActiveDirectory supporting the "in chain" matching rule, otherwise we just default to the new recursive strategy.
Also adds a
membership_validatorconfiguration option to force a specific strategy (one of:detect,:classic,:recursive, or:active_directory).See http://msdn.microsoft.com/en-us/library/cc223359.aspx for details on the ActiveDirectory capability detection. I'm struggling to find the reference now but from what I understand, the "in chain" matching rule is only supported as part of ActiveDirectory 2008 R2.
cc @jch @github/ldap