Skip to content

Support referral chasing for Active Directory member validation#95

Merged
davesims merged 100 commits into
masterfrom
support_referral_chasing
Aug 5, 2016
Merged

Support referral chasing for Active Directory member validation#95
davesims merged 100 commits into
masterfrom
support_referral_chasing

Conversation

@davesims

@davesims davesims commented Aug 1, 2016

Copy link
Copy Markdown
Contributor

This PR adds support for chasing referrals during an Active Directory member validation. Added a ReferralChaser object that wraps GitHub::Ldap#search functionality with the ability to pursue any referred URLs that come back from the server. Will replace #91 and depends on #94.

/cc #94 #91
/cc @mtodd @jch @jatoben @lildude @sbryant
/cc @github/platform-iam

Dave Sims added 30 commits July 25, 2016 09:29
- Set empty search base in user? rather than in GitHub::Ldap
- Get rid of indirection in Domain to global_catalog_search
- Update test
@davesims

davesims commented Aug 4, 2016

Copy link
Copy Markdown
Contributor Author

Just a note: I've rebased #94 onto this branch. I'll need to merge #94 into master before merging this.

Comment thread test/referral_chaser_test.rb Outdated
admin_user: "Joe",
admin_password: "passworD1",
port: 389
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Want to use a mock here too? Or use the options hash to pull configuration for working connections?

Comment thread test/referral_chaser_test.rb Outdated
admin_user: "uid=admin,dc=github,dc=com",
admin_password: "passworD1",
port: 389
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still need to use the options defined in GitHub::Ldap::Test for the right host.

Note, though, that these failures are because capabilities are being checked to aggressively, causing a query to occur when creating a new object, rather than lazily loading the capabilities as needed. This is out-of-scope from this PR, but it's a constraint to getting the build to pass now.

mtodd added 4 commits August 5, 2016 09:31
Also use that port value, as it changes depending on the integration test
LDAP server in use.
Stylistic preference to be more consistent with other tests.
@mtodd

mtodd commented Aug 5, 2016

Copy link
Copy Markdown
Member

Looks like we're 🍏. Let's get this merged, @davesims.

@davesims davesims merged commit 52b78e2 into master Aug 5, 2016
@mtodd mtodd deleted the support_referral_chasing branch August 5, 2016 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants