feat(user_ldap): add function to find a user in ldap by thpiron · Pull Request #56721 · nextcloud/server

thpiron · 2025-11-26T18:38:14Z

linked to: Flexibilize matching of external user ids and Nextcloud users (SAML, OIDC to LDAP) #55284

Summary

Add a findOneUser function to LDAPProvider

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

github-actions · 2025-12-11T02:12:22Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

blizzz

Thank you for the draft and the work on it @thpiron! I have a few remarks, the foundation is good!

blizzz · 2025-12-12T14:08:22Z

lib/public/LDAP/ILDAPProvider.php

+	 *
+	 * @return IUser|null Returns a IUser if found in ldap using the configured ldap filter
+	 * @throws \Exception if multiple users has been found (search query should not allow this)
+	 */


On the API, we note down @since annotations

Suggested change

*/

* @since 33.0.0

*/

blizzz · 2025-12-12T14:10:26Z

lib/public/LDAP/ILDAPProvider.php

+	 * Search for a single user in ldap
+	 *
+	 * @return IUser|null Returns a IUser if found in ldap using the configured ldap filter
+	 * @throws \Exception if multiple users has been found (search query should not allow this)


Please don't throw the general \Exception but create a specific one, e.g. MultipleUsersReturnedException in the same namespace.

blizzz · 2025-12-12T14:12:50Z

apps/user_ldap/lib/LDAPProvider.php

+	/**
+	 * Search for a single user in ldap
+	 *
+	 * @return IUser|null Returns a IUser if found in ldap using the configured ldap filter
+	 * @throws \Exception if multiple users has been found (search query should not allow this)
+	 */


this docblock can be left out as it is provided by the interface.

blizzz · 2025-12-12T14:14:07Z

apps/user_ldap/lib/User_LDAP.php

+	 * @return IUser |null Returns an IUser if found in LDAP using the configured LDAP filter, or null if no user is found.
+	 */
+	public function getUserFromCustomAttribute(string $filter, string $attribute, string $searchTerm): ?IUser {
+		if ($filter === null || $filter === '') {


$filter cannot be null, as the signature demands a string.

Noted, I'll change this when reworking the interface function(s), keeping this as unresolved for now.

blizzz · 2025-12-12T14:17:02Z

apps/user_ldap/lib/User_LDAP.php

+	public function getUserFromCustomAttribute(string $filter, string $attribute, string $searchTerm): ?IUser {
+		if ($filter === null || $filter === '') {
+			$searchTerm = $this->access->escapeFilterPart($searchTerm);
+			$filter = "($attribute=$searchTerm)";


The filter should always be an and-combination of the user list filter, otherwise it might pull in users without access.

(Technically the login filter would be more correct semantically, but it might not resolve on such setups by configuration to avoid direct login.)

Noted, I'll change this when reworking the interface function(s), keeping this as unresolved for now.

blizzz · 2025-12-12T14:23:00Z

apps/user_ldap/lib/User_Proxy.php

+		foreach ($this->backends as $backend) {
+			if (method_exists($backend, 'getUserFromCustomAttribute')) {
+				$user = $backend->getUserFromCustomAttribute($filter, $attribute, $searchTerm);
+				return $user;


Only the first LDAP configuration would be considered, as there is an immediate return statement. Also, the method_exists should not be necessary?

This can be solved in two ways:

return the first result, that is not null. This might be faster, especially when there is not hit. But it might ignore users from other LDAP configurations, that then will not result in the exception.

Run through all backends, and return that one user, if found, and throw the MultipleUsersReturnedException as soon as there are two results.

The broad search operations goes against all configurations as well, so going with #2 should be fine and not impacting the performance compared to the status quo.

2 is safer indeed, I only throw an exception if the the user are different, but I'm not sure if this makes sense, Maybe I should always throw the exception if two users found even if it's the same on nextcloud? I don't know if there is a safeguard somewhere to make this case impossible.

b430068

blizzz · 2025-12-12T14:31:41Z

lib/public/LDAP/ILDAPProvider.php

+	 * @return IUser|null Returns a IUser if found in ldap using the configured ldap filter
+	 * @throws \Exception if multiple users has been found (search query should not allow this)
+	 */
+	public function findOneUser(string $filter, string $attribute, string $searchTerm): ?IUser;


The signature requires all parameters be set, contrary to the spec (it could have been written more precise, i take that) and I think contrary to your suggestion.

Two ways how to solve it:

Use nullable and optional parameters: ?string $filter = null, ?string $attribute = null, ?string $searchTerm = null. The downside is that the usage does not become clear by the parameters and it is easy to use the methods wrongly.

Split it into two methods: findOneUserByLdapFilter(string $filter): ?IUser and findOneUserByAttributeValue(string $attribute, string $searchTerm): ?IUser.

I prefer the latter.

We can also go a step back and agree on one solution, either the filter (which actually is a filter part, could pronounce this as well and clarify that this is and-connected to the user list filter, as per my comment further above), or the attribute-value pair.

Due to

Other login backends that may rely on LDAP, like the SAML/SSO or OIDC backend, will get an additional configuration option to specify an attribute to which the received user id is being compared.

leaving out the filter (I am also a bit nervous about that: to easy to make a mistake) and only accepting the attribute name and the search value. What do you think?

I'm not happy with this function signature either, splitting it in to function is indeed better I think.

For most cases I think that only the attribute/searchTerm part would be enough, but maybe an having an "expert" config with the filter could be useful in complex configurations.
This would also be consistent with the way user_ldap config works, where it is almost always possible to edit the filter manually.

Maybe adding in the findOneUserByLdapFilter docstring that it should be used carefully by the other apps (as an advanced setting)?

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

thpiron marked this pull request as ready for review November 26, 2025 19:02

thpiron requested a review from a team as a code owner November 26, 2025 19:02

thpiron requested review from Altahrim, come-nc, leftybournes and salmart-dev and removed request for a team November 26, 2025 19:02

thpiron changed the title ~~WIP: feat(user_ldap): add function to find a user in ldap~~ feat(user_ldap): add function to find a user in ldap Nov 26, 2025

thpiron marked this pull request as draft November 27, 2025 02:50

blizzz self-requested a review December 3, 2025 20:58

github-actions bot added the feedback-requested label Dec 11, 2025

blizzz reviewed Dec 12, 2025

View reviewed changes

thpiron added 6 commits December 13, 2025 13:10

feat(user_ldap): add function to find a user in ldap

2f25d0b

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

fix: apply linter

c2d6df2

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

fix: remove useless doctstring

bdeb14b

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

fix: use a specific exception defined in public API

fe25e82

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

fix: add since version

375a69d

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

fix: go through all ldap backends to check user

b430068

Signed-off-by: Thibault Piron <thibault.a.piron@gmail.com>

thpiron force-pushed the feat/add_findOneUser_func_to_LDAPProvider branch from 084e1ab to b430068 Compare December 13, 2025 12:12

Uh oh!

Conversation

thpiron commented Nov 26, 2025

Summary

Checklist

Uh oh!

github-actions bot commented Dec 11, 2025

Uh oh!

blizzz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants