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

Allow searching for users by displayname in the database group backend #17800

Closed
wants to merge 1 commit into from

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Nov 4, 2019

When searching for users in a group by displayname only the uid was matched before:

$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);

With this PR usersInGroup will also check for matching displaynames when called with a $search parameter

@blizzz
Copy link
Member

blizzz commented Nov 5, 2019

tests :)

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl juliushaertl force-pushed the bugfix/noid/search-in-group-displayname branch from d966388 to 5822e13 Compare November 12, 2019 20:51
@juliushaertl juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 12, 2019
@juliushaertl
Copy link
Member Author

Hmm needs more adjustments since this change will no longer find users in other backends then

@MorrisJobke
Copy link
Member

I also stumbled over this.

In the database we search over userID, displayname and email:

$query->select('uid', 'displayname')
->from($this->table, 'u')
->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
$query->expr()->eq('userid', 'uid'),
$query->expr()->eq('appid', $query->expr()->literal('settings')),
$query->expr()->eq('configkey', $query->expr()->literal('email')))
)
// sqlite doesn't like re-using a single named parameter here
->where($query->expr()->iLike('uid', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
->orWhere($query->expr()->iLike('displayname', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
->orWhere($query->expr()->iLike('configvalue', $query->createPositionalParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')))
->orderBy($query->func()->lower('displayname'), 'ASC')
->orderBy('uid_lower', 'ASC')

I'm coming here, because searching in groups (in the user management) only searches user IDs, while searching the "all" tab it checks for user ID, display name and email:

$users = $group->searchUsers($search, $limit, $offset);

One can also see that the searchUsers searches for user ID, but there is also searchDisplayName (the method that was changed in here), searches also only for the user ID, because it uses the same backend method. The idea here would be then to provide a new group backend interface that adds then the "search display name as well" feature. The big question then still is: do we then search only display name or userID/displayname/email like in the other methods?

I just checked the userbackend which has a getUsers():

public function getUsers($search = '', $limit = null, $offset = null);

There multiple backends implement it in different ways:

... what a mess, because our public interface only says "search" and doesn't specify anything.

What now?

@blizzz
Copy link
Member

blizzz commented May 11, 2020

LDAP searches display name and user ID:

negative, the first filters for valid users, the second ensures the displayname attribute is present, the third pulls in the search attributes, which are configurable.

@blizzz
Copy link
Member

blizzz commented May 11, 2020

... what a mess, because our public interface only says "search" and doesn't specify anything.

It depends in the user case. For end user search, this fuzzyness is ok and good, as long as the desired results are shown. There are use cases where API-wise you like to search for something specific, e.g. when user_saml gets an id and wants to ensure the corresponding LDAP user is imported. At the moment this is also treated with search.

@MorrisJobke
Copy link
Member

I tried porting the function from the User/Database.php in #21138

@MorrisJobke
Copy link
Member

Let's close this here in favor of #21138

@MorrisJobke MorrisJobke closed this Aug 5, 2020
@MorrisJobke MorrisJobke deleted the bugfix/noid/search-in-group-displayname branch August 5, 2020 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants