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

Prepend wildcard to the LDAP search term #14845

Closed
wants to merge 1 commit into from

Conversation

Toilal
Copy link

@Toilal Toilal commented Mar 25, 2019

It makes the LDAP user/group search behaves consistently with database backend

See

if ($search !== '') {
$query->where($query->expr()->iLike('gid', $query->createNamedParameter(
'%' . $this->dbConn->escapeLikeParameter($search) . '%'
)));
}

It makes the LDAP user/group search behaves consistently with database backend

See https://github.com/nextcloud/server/blob/3f4941e48aead48bacc7077e2819b492d1394778/lib/private/Group/Database.php#L270-L274

Signed-off-by: Rémi Alvergnat <remi.alvergnat@gfi.fr>
@Toilal Toilal force-pushed the ldap-search-term branch from 6446604 to e8ecddd Compare March 25, 2019 17:01
@Toilal
Copy link
Author

Toilal commented Mar 26, 2019

Where are functional tests behaviors implemented ? I can't find them.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

this kills performance on halfway big LDAP setups as indices cannot work.

@Toilal
Copy link
Author

Toilal commented Mar 27, 2019

this kills performance on halfway big LDAP setups as indices cannot work.

I don't know how to mitigate this performance issue, I'm a beginner with LDAP, but functionally speaking it's a problem that LDAP filtering and SQL filtering doesn't behave the same way.

Could you tell me why indices may not work with this change ?

@rullzer rullzer mentioned this pull request Mar 27, 2019
9 tasks
@blizzz
Copy link
Member

blizzz commented Mar 27, 2019

@Toilal Since the start of the string is unknown, the whole index has to be gone from top to bottom as anything can match. That's the reason why it is, as it is. The local user management is not intended for a big number of users.

@jgribonvald
Copy link

Could this feature be optional by configuration ?
Like that it permits to each one to make his choice ?

@blizzz
Copy link
Member

blizzz commented Mar 29, 2019

@jgorgulho @Toilal let's take a step back: What do you need it for? I would doubt it is common only to look at a part of a first, or second name. For the local backend it makes sense, because first and last names are not split, but this is totally doable in LDAP.

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 16, Nextcloud 17 Apr 1, 2019
@Toilal
Copy link
Author

Toilal commented Apr 5, 2019

In our use case, groups are prefixed in LDAP with some stuff that categories the groups, separated by semicolons.

esco:Applications:Folios:DE L IROISE_0290009C

But we need those kind of groups to be returned with search input like IROISE.

@blizzz
Copy link
Member

blizzz commented Apr 5, 2019

@Toilal don't you have an attribute on the groups that contains the name without those prefixes?

@jgribonvald
Copy link

No there isn't a such attribute as the unique names depends on the complete name. Also the tree representation (the complete name) is needed to find the rigth group.

@blizzz
Copy link
Member

blizzz commented Apr 5, 2019

Also the tree representation (the complete name) is needed to find the rigth group.

Sorry, could you rephrase it? Do you refer to the DN?

Imho it makes sense in general to have an attribute that just carries the plain name, which can then be added to the group search attributes (and you can add an index to your LDAP server). Having the wildcard prepended just kills performance for everybody.

@jgribonvald
Copy link

jgribonvald commented Apr 5, 2019

We are managing around 45 000 groups, and the representation/structuring of all theses groups is like a tree. But in our ldap all these groups are at the same level and the name is the whole path in the tree (like root:node1:subnode0:sub-subnodeA:leaf). So we consider that the name is the display name like the CN and also the DN is CN + LDAP branch information.
We can't have a display name that will mean something based on the leaf of the tree group (so the tail part) as it can be found several times (when I say several time it will be more than a dozen times). So to show groups to users we need to provide them the whole path else they won't be able to find the rigth group.
Also users won't make search on the whole path as only the last parts of the cn will be used.
The LDAP search in our LDAP is really efficient with wilcard prepended (as we have an index on the cn) so I don't understand why we can't have this behaviour.

On an other side this behaviour can't be configurable and by default disabled ?

@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
@MorrisJobke
Copy link
Member

@blizzz Mind to reply here?

@blizzz
Copy link
Member

blizzz commented Jul 17, 2019

@MorrisJobke thanks for bringing it to my attention again.

On an other side this behaviour can't be configurable and by default disabled ?

@jgribonvald this we can do, although I am not fan of adding more switches and code paths. I would accept it however.

@jgribonvald
Copy link

Thanks @blizzz.

@DanScharon
Copy link

so the actual use cases are only for groups then, right? We would only need it there.

@blizzz
Copy link
Member

blizzz commented Aug 8, 2019

Somewhere else someone has had also use cases with a number of and order of names. So in its entirety there a cases for both users and groups.

@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 8, 2019
@blizzz blizzz modified the milestones: Nextcloud 17, Nextcloud 18 Aug 8, 2019
@blizzz blizzz added enhancement and removed bug labels Aug 8, 2019
@blizzz
Copy link
Member

blizzz commented Aug 8, 2019

I moved the milestone to 18 since we're in feature freeze.

@DanScharon
Copy link

Somewhere else someone has had also use cases with a number of and order of names. So in its entirety there a cases for both users and groups.

hmm, interesting. When searching for users from LDAP, substring match already works (at least in our setup). So if I am looking for Mr. Miller I can find him by entering "iller". That doesn't work for groups. So in our case we would only need it for groups and can't say for sure what the performance impact on our LDAP search for users would be (~ 19.000 accounts).

This was referenced Dec 11, 2019
@rullzer rullzer mentioned this pull request Dec 19, 2019
18 tasks
@rullzer rullzer modified the milestones: Nextcloud 18, Nextcloud 19 Dec 19, 2019
This was referenced Apr 4, 2020
This was referenced Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
@blizzz blizzz modified the milestones: Nextcloud 19, Nextcloud 20 Apr 24, 2020
@blizzz
Copy link
Member

blizzz commented Apr 24, 2020

Moved to 20, @Toilal are still in for it? Otherwise I'd say let's close it. Someone else can pick it up still.

hmm, interesting. When searching for users from LDAP, substring match already works (at least in our setup). So if I am looking for Mr. Miller I can find him by entering "iller".

Yes, I think this is your setup :)

That doesn't work for groups. So in our case we would only need it for groups and can't say for sure what the performance impact on our LDAP search for users would be (~ 19.000 accounts).

Perhaps you can make your LDAP behave there similarly, too? That's more save than have a feeling-lucky asterisk up front in general.

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@MorrisJobke
Copy link
Member

Moved to 20, @Toilal are still in for it? Otherwise I'd say let's close it. Someone else can pick it up still.

As there was no feedback on this I will close it for now. It can be picked up at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants