Skip to content

feat(ldap-auth): make querying user groups possible#2585

Open
cchndl wants to merge 7 commits into
SciCatProject:masterfrom
cchndl:groups-from-ldap
Open

feat(ldap-auth): make querying user groups possible#2585
cchndl wants to merge 7 commits into
SciCatProject:masterfrom
cchndl:groups-from-ldap

Conversation

@cchndl
Copy link
Copy Markdown
Contributor

@cchndl cchndl commented Mar 3, 2026

The ldap authenticator did not query the groups from ldap until now.

Add the required config options for getting the groups from ldap. For this we just expose the existing options from node-ldapauth-fork/passport-ldapauth from the SciCat config.
Add a group extraction service for the LDAP response and register it to the servicefactory if enabled.

Additional fixes:
The AccessGroupFromPayloadService did not correctly check whether the elements in payload[accessGroupsProperty] are strings.

@cchndl cchndl requested a review from a team as a code owner March 3, 2026 16:32
@cfelder cfelder force-pushed the groups-from-ldap branch 3 times, most recently from 729b731 to 73195d3 Compare April 28, 2026 15:20
@cfelder
Copy link
Copy Markdown
Contributor

cfelder commented May 5, 2026

/ptal @nitrosx

Copy link
Copy Markdown
Member

@fpotier fpotier left a comment

Choose a reason for hiding this comment

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

I tried the feature with scicatlive but couldn't make it work out of the box.
It's maybe because we use a simplified ldap provider.
Left a few comments but I'm not sure it's worth implementing as you seem to be the only facility using this.

Comment thread src/auth/access-group-provider/access-group-from-ldap.service.ts Outdated
Comment thread src/auth/access-group-provider/access-group-from-ldap.service.ts Outdated
@cchndl cchndl force-pushed the groups-from-ldap branch 2 times, most recently from eec086f to ef58a5f Compare May 12, 2026 08:22
Alexander Zaft and others added 4 commits May 12, 2026 11:33
add config options and use correct accessGroupProperty and add group
extracting service

Change-Id: Iddf3f95201fb2c7915fee5b0483afdb6232902e8
Change-Id: If1930b0a94e9c8cc14d2dbee629e2f534363fcaf
'_groups' in the ldap payload is from the library and won't change
unless that is switched out. The field in the ldap response which
represents the group name can vary however, so make that the
configurable one (could e.g. be 'cn' or 'ou').
@Junjiequan
Copy link
Copy Markdown
Member

Can we merge it or is there any blocker for this to be merged?

@cchndl
Copy link
Copy Markdown
Contributor Author

cchndl commented May 12, 2026

I missed that the environment variables were also in docs/index.md, but otherwise nothing from my side.

@cfelder
Copy link
Copy Markdown
Contributor

cfelder commented May 12, 2026

Besides making the ldap attribute for determining the group name configurable we also need this for determining the username. For non-ad setups e.g. using uid instead of displayName is still a local fix on our end.

Imho we can do it as a small addition within this PR. If you want to have it separate please let us know.

@cchndl cchndl force-pushed the groups-from-ldap branch from 2088636 to a3028b2 Compare May 12, 2026 12:02
@Junjiequan
Copy link
Copy Markdown
Member

Imho we can do it as a small addition within this PR. If you want to have it separate please let us know.

It looks find to me to have them in this PR. This PR is quite small and the implementation is straightforward, I'm happy for it to be merged

@cchndl cchndl force-pushed the groups-from-ldap branch from 73ae96f to a668d93 Compare May 12, 2026 13:53
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.

4 participants