Skip to content
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

LDAP: rework configuration and move to a common module #2708

Merged
merged 7 commits into from
Apr 11, 2022

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Mar 31, 2022

This reworks the LDAP configuration of the user- and groups-providers.
As the LDAP groups provider needs to be aware of users (e.g. for
looking up groups members) and the LDAP users provider needs to be aware
of groups (e.g. for looking up groupmembership of a user) this commit
basically unifies the configuration of both providers.

Additionally the LDAP configuration is reworked to no longer rely on
templating LDAP filters in the configuration with can be error prone and
confusing.

This commit also move all code executing LDAP queries into the common
utlis/ldap module for being able to share more code between both
providers.

Instead of open a new connection with every single requests. We now use
a long-lived connection per service with a simple mechanism to
automatically reconnect in case of failures.

@rhafer rhafer self-assigned this Mar 31, 2022
@update-docs
Copy link

update-docs bot commented Mar 31, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@rhafer rhafer force-pushed the share-ldap-conn branch 8 times, most recently from d227818 to 449f676 Compare March 31, 2022 14:42
@cs3org cs3org deleted a comment from lgtm-com bot Mar 31, 2022
@rhafer rhafer marked this pull request as ready for review March 31, 2022 15:59
@rhafer rhafer requested review from labkode, ishank011, glpatcern and a team as code owners March 31, 2022 15:59
pkg/utils/ldap.go Show resolved Hide resolved
pkg/utils/ldap.go Show resolved Hide resolved
pkg/user/manager/ldap/ldap.go Outdated Show resolved Hide resolved
pkg/utils/ldap/identity.go Outdated Show resolved Hide resolved
pkg/utils/ldap/identity.go Show resolved Hide resolved
pkg/utils/ldap/identity.go Outdated Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Unfortunately I still have some questions, sorry...

pkg/group/manager/ldap/ldap.go Outdated Show resolved Hide resolved
pkg/user/manager/ldap/ldap.go Outdated Show resolved Hide resolved
pkg/utils/ldap/identity.go Outdated Show resolved Hide resolved
pkg/utils/ldap/identity.go Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Outdated Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Outdated Show resolved Hide resolved
pkg/utils/ldap/reconnect.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

👍

Instead of open a new connection with every single requests. We now use
a long-lived connection per service with a simple mechanism to
automatically reconnect in case of failures.

Fixes cs3org#2122
This reworks the LDAP configuration of the user- and groups-providers.
As the LDAP groups provider needs to be aware of users (e.g. for
looking up groups members) and the LDAP users provider needs to	be aware
of groups (e.g. for looking up groupmembership of a user) this commit
basically unifies the configuration of both providers.

Additionally the LDAP configuration is reworked to no longer rely on
templating LDAP filters in the configuration with can be error prone and
confusing.

This commit also move all code executing LDAP queries into the common
utlis/ldap module for being able to share more code between both
providers.
Depending on the configured group_objectclass we can no resolve group
members by DN as well.

Closes: 2124
GetUser() was used to lookup users by name or ID, however it is only
supposed to work for ID based lookups. If the ID based lookup fails
fallback to an explicit GetUserByClaim fall for trying a name based
lookup.
rhafer added a commit to rhafer/ocis that referenced this pull request Apr 11, 2022
webUISharingInternalGroupsEdgeCases/shareWithGroupsEdgeCases.feature:41
no longer fails as cs3org/reva#2708 fixed some
issue with LDAP filter escaping.
@rhafer rhafer merged commit 134ed5a into cs3org:edge Apr 11, 2022
rhafer added a commit to rhafer/ocis that referenced this pull request Apr 11, 2022
webUISharingInternalGroupsEdgeCases/shareWithGroupsEdgeCases.feature:41
no longer fails as cs3org/reva#2708 fixed some
issue with LDAP filter escaping.
@kobergj kobergj mentioned this pull request Dec 19, 2022
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.

3 participants