Skip to content

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

Merged
merged 26 commits into from
Jun 16, 2025

Conversation

kriswest
Copy link
Contributor

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.

Copy link

netlify bot commented Apr 11, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 6a3146d
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68500c858c10b80008e548a3

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 13.33333% with 26 lines in your changes missing coverage. Please review.

Project coverage is 69.51%. Comparing base (72d43e4) to head (6a3146d).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/service/passport/activeDirectory.js 0.00% 14 Missing ⚠️
src/service/passport/ldaphelper.js 26.66% 11 Missing ⚠️
src/config/index.ts 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kriswest
Copy link
Contributor Author

Fixed teh cypress test but am waiting on a review (in git proxy), will update monday with working cypress tests

@kriswest
Copy link
Contributor Author

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

@kriswest
Copy link
Contributor Author

@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.

@JamieSlome
Copy link
Member

@kriswest - are we able to review merge conflicts, then I'll do a second take and merge 👍

@kriswest kriswest requested a review from JamieSlome May 8, 2025 06:35
@kriswest
Copy link
Contributor Author

@JamieSlome conflicts resolved, ready for review and merge

chore: remove debug console log
@sam-holmes2
Copy link

Hi @kriswest please could you resolve any remaining merge conflicts. Then l can help approve the PR

@kriswest
Copy link
Contributor Author

@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

@kriswest
Copy link
Contributor Author

@sam-holmes2 updates pushed and conflicts resolved.

Copy link

@sam-holmes2 sam-holmes2 left a 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! 🥳

@kriswest kriswest closed this Jun 3, 2025
@kriswest kriswest force-pushed the 909-ldap-user-group-confirmation branch from 36f9ee6 to 4387684 Compare June 3, 2025 16:50
@kriswest kriswest reopened this Jun 3, 2025
@kriswest
Copy link
Contributor Author

kriswest commented Jun 3, 2025

Conflicts resolved

@kriswest
Copy link
Contributor Author

Conflicts resolved again and tests passing.

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome merged commit 9d96614 into finos:main Jun 16, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LDAP Authentication - Validating Groups of a user
3 participants