-
Notifications
You must be signed in to change notification settings - Fork 137
feat: support direct querying of AD group membership via LDAP #972
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
feat: support direct querying of AD group membership via LDAP #972
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #972 +/- ##
==========================================
- Coverage 70.15% 69.51% -0.65%
==========================================
Files 54 54
Lines 2205 2227 +22
Branches 248 248
==========================================
+ Hits 1547 1548 +1
- Misses 627 648 +21
Partials 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixed teh cypress test but am waiting on a review (in git proxy), will update monday with working cypress tests |
I'm not sure I can improve the test coverage on this - unless someone has a suggestion on mocking passport. We are however using it successfully this end. Configs were documented. Otherwise ready for review @JamieSlome |
@JamieSlome @grovesy @coopernetes Keen to get a review on this - also it should be tested by someone using a REST API to check group meembership. |
@kriswest - are we able to review merge conflicts, then I'll do a second take and merge 👍 |
…on' into 909-ldap-user-group-confirmation
@JamieSlome conflicts resolved, ready for review and merge |
chore: remove debug console log
Hi @kriswest please could you resolve any remaining merge conflicts. Then l can help approve the PR |
…on' into 909-ldap-user-group-confirmation
@sam-holmes2 update on its way. When resolving conflict I needed to regenerate the schema doc, a bunch of the docs for remote config loading then drops out as its not in the schema (manually added to generated doc I guess). See: https://github.com/finos/git-proxy/pull/935/files#r2102236009 |
@sam-holmes2 updates pushed and conflicts resolved. |
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.
LGTM - thanks for your contribution! 🥳
36f9ee6
to
4387684
Compare
Conflicts resolved |
Conflicts resolved again and tests passing. |
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.
LGTM! 🍰
resolves #909
Adds support for using LDAP directly to confirm AD group membership, which is enabled when the
apis.ls
config is not set. Also adds documentations for the AD related parts of the config.