Do not filter users post search

[maxmalek]

Signed-off-by: Maximilian Malek maximilian.malek@uni-graz.at

Fixes element-hq/element-web#24422

Checklist

• Tests written for new code (and old code if feasible)
• Linter and other CI checks pass
• Sign-off given on the changes (see CONTRIBUTING.md)

Notes: Make sure users returned by the homeserver search API are displayed. Don't silently drop any.

Currently, Element only displays search results that contain the search term as substring, which is IMHO an anti-feature. This one-line change removes the filter for the people search and displays all results sent by the homeserver.

Pros:

• Makes fuzzy search, regex search, etc possible (if handled by the server)
• Actually displays what was returned by the API; does not silently drop legitimate search results
• More control for homeservers

Cons:

• Malicious homeservers may send garbage that is then actually shown

Ideally those other substring filters nearby should be removed entirely, but the user search is the biggest problem on my end right now so I've kept this PR to the minimum.

Background: I'm working on an identity server & search provider similar to ma1sd that handles the _matrix/client/r0/user_directory/search endpoint redirected via reverse proxy.
This means I'm able to inject users into the search that are not returned by synapse.
We've been using ma1sd's LDAP-based search extensively, but I ended up writing my own since ma1sd is too slow. Added fuzzy search, then realized element silently drops results it doesn't like.

---

Here's what your changelog entry will look like:

✨ Features

• Make sure users returned by the homeserver search API are displayed. Don't silently drop any. (#9556). Fixes The user search does not work. element-hq/element-web#24422. Contributed by @maxmalek.

[SimonBrandner]

This is going to require a design review to check we really want this behaviour

[SimonBrandner]

I would prefer to remove the if statement completely

[HarHarLinks]

Without digging into this code to deep, the file name indicates this is only the Spotlight search (i.e. what appears when you press Ctrl+K or the top left "fake input box button").

Would this not need to be changed for the "Start new chat" and "Invite to room" modals, too?

[daniellekirkwood]

I've tried comparing the netlify build with the product and can't see the difference here - could you give me an example of where and how this changes the user's experience? Thank you!

[t3chguy]

This comment needs to say WHY (because we rely on the server to filter DIRECTORY results for us)

[HarHarLinks]

Can you elaborate why clientside filtering is done in the first place? I think the spec suggests nothing of the kind, and I think additional validation seems superfluous since the server should be trustworthy enough.

[t3chguy]

Probably a copy-pasta, but you'd need to ask whomever wrote it

[HarHarLinks]

I thought I was, @t3chguy :) https://github.com/matrix-org/matrix-react-sdk/pull/7804/files#diff-bd33652e7c8d0c14883372ef5febdaeb8a8ce8a53220d72b13a0c953132265feR261

[dhenneke]

@HarHarLinks and me investigated it further and found the following:

The component has different sources for the users: (1) the results from the user profile endpoint and (2) all users that joined any room that I am in.

(1) was already server-side filtered using the user entered query:

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Line 343 in 7a250f4

useDebouncedCallback(filter === Filter.People, searchPeople, searchParams);

(2) is not pre-filtered, but every user is added to the possible user results:

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Lines 358 to 364 in 7a250f4

	for (const user of [...findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), ...users]) {
	// Make sure we don't have any user more than once
	if (alreadyAddedUserIds.has(user.userId)) continue;
	alreadyAddedUserIds.add(user.userId);
	userResults.push(toMemberResult(user));
	}

The conditional in discussion is thus required to filter the users in (2):

matrix-react-sdk/src/components/views/dialogs/spotlight/SpotlightDialog.tsx

Lines 414 to 415 in 7a250f4

	} else if (isMemberResult(entry)) {
	if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query

However, this line will also filter the results that were already filtered by the server. This shouldn't be necessary because I would assume that we can trust that the homeserver returns proper results.

Proposal

We would propose some refactorings to remove the double-filtering of the server results:

  // ...
+ const localUsers = findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor).filter(... apply the filtering by trimmedQuery ...);
+ for (const user of [...localUsers, ...users]) {
- for (const user of [...findVisibleRoomMembers(cli, msc3946ProcessDynamicPredecessor), ...users]) {
      // Make sure we don't have any user more than once
      if (alreadyAddedUserIds.has(user.userId)) continue;
      alreadyAddedUserIds.add(user.userId);
  
      userResults.push(toMemberResult(user));
  }
  
  // ...

With this change, the filtering of the possibleResults can be skipped:

- } else if (isMemberResult(entry)) {
-   if (!entry.query?.some((q) => q.includes(lcQuery))) return; // bail, does not match query

[HarHarLinks]

Further, we suggest to add our use-case to the tests.

[pmaier1]

From a product POV, I think we want to move on with this as it makes progress towards better user discoverability. There are some unknowns which we'd like to clarify, though:

could you give me an example of where and how this changes the user's experience

Yes, that would be appreciated to better understand the change in behavior.

Would this not need to be changed for the "Start new chat" and "Invite to room" modals, too?

1. I agree. The behavior of these components should be consistent. Please clarify.

Malicious homeservers may send garbage that is then actually shown

2. From an engineering POV, are there risks from this change we might be overseeing? Is there a good reason for the current behavior?
3. How do other clients behave? We'd want this behavior to be consistent cross-platform.

[HatoNordeck]

O.K., I try to give the use case, here, and why we need this change:
We want to implement a fuzzy search, which also enables search by user attributes in the LDAP, e.g. search by role in the Organization. This is an extension of the functionality at the Homeserver level. Therefor the clients should allow search results, which differ from the search string. A clientside validation is not meaningful and blocks a better serverside  search implementation.

Example fuzzy search:
Search String: Mathew Hodgson
Search Result: Matthew Hodgson, ID, Avatar 

Example search by attribute: 
Search String: COO
Search Result: Amandine Le Pape, ID, Avatar
Search Result 2: Christian Hauff, ID, Avatar 

Out of scope here, but next stage would be, to also allow more complex search result objects in the client, which contain also the found attribute and give transparency, why the person was found:
Search String: COO
Search Result: Amandine Le Pape, ID, Avatar, Role: COO
Search Result 2: Christian Hauff, ID, Avatar, Organisation: Coop

[daniellekirkwood]

O.K., I try to give the use case, here, and why we need this change: We want to implement a fuzzy search, which also enables search by user attributes in the LDAP, e.g. search by role in the Organization.

This was super helpful -- thank you!

[daniellekirkwood]

@SimonBrandner & @pmaier1 I think we're good to remove the Design Reviewer and instead add a Product one.

I'll do that now but feel free to comment if you think it still needs design input.

[pmaier1]

From a product POV this goes in the right direction 👍
Please still let us know whether there are other locations to be made consistent.

[t3chguy]

The comment/approach needs improving as per the earlier request

[HarHarLinks]

Would this not need to be changed for the "Start new chat" and "Invite to room" modals, too?

Those appear to not filter the results in the same way it is done in spotlight and hence already work for the use-case described by @HatoNordeck.

The android app "works" regarding this use-case from my point of view (it matches web's invite behavior, and to my knowledge it does not have a secondary "spotlight" search as web does).

iOS remains untested by myself as I don't have Apple hardware.

[MTRNord]

iOS remains untested by myself as I don't have Apple hardware.

I tested it with the Bundesmessenger fork (It has no changes in this UI part) and it seems to work just fine for the User invite dialogs. There don't seem to be any other searches that accept User searches in the first place. They are not filtering any of the results.

[HarHarLinks]

Progress update: I talked to OP out of band about following up on this request. We decided that as soon as we (Nordeck) feel confident enough about having tested our proposed amendment (#9556 (comment)) we are going to open our own PR which is to supersede this one.