Prevent local user profiles leaking when a "per-room profile" is changed by DMRobertson · Pull Request #10695 · matrix-org/synapse

DMRobertson · 2021-08-25T14:43:21Z

The plan is described here. In a nutshell, I want to make it so that local users' entries in the user directory only ever refer to their "public" profile, and completely ignore any per-room nicknames.

This goes some way towards fixing #5677, but we still have the problem of how to handle remote users' profiles.

I don't think this will change any behaviour, but it makes PyCharm not highlight them as warnings which is less distracting.

* type hint _get_next_batch * Introduce an enum MatchChange so there is a name rather than an `Optional[bool]`

so that search_all_users config flag only controls searching. Cost: extra rows in user_directory and user_directory_search for users who are in no rooms. I think this is negligible since we already have rows for these users in the profiles and users tables.

fixes a unit test. And we all seem to think that profiles should refer to users, even if that isn't enforced as a foreign key constraint in the db.

Feels like it's less bad

I've made profile changes update the user directory, which in turn expects the corresponding user to actually exist.

DMRobertson · 2021-09-01T13:23:09Z

Tested this locally against element-web. It offered some kind of client-side suggestions which seemed to take into account per-room nicknames. However I confirmed the underlying response to /user_directory/search was returning the expected information: it did not yield results when searching for the per-room name, and it yielded the public name when searching by mxid.

DMRobertson · 2021-09-02T13:51:46Z

There is a bit of a hole here. We still use the event displayname and avatar when handling someone joining a room. I don't think many clients will offer the chance to set a per-room nickname before you join, but it's possible from the protocol's POV.

reivilibre · 2021-09-02T14:03:03Z

There is a bit of a hole here. We still use the event displayname and avatar when handling someone joining a room.

out of interest, why is this the case? Is it difficult for some reason?

DMRobertson · 2021-09-02T14:07:16Z

There is a bit of a hole here. We still use the event displayname and avatar when handling someone joining a room.

out of interest, why is this the case? Is it difficult for some reason?

Just an oversight on my part. Until I started looking at this for remote users, I hadn't properly considered that local users could go down this code path (the branch with elif joined is MatchChange.now_true) with an event whose name didn't match their profile.

changelog.d/10695.bugfix

synapse/handlers/user_directory.py

reivilibre · 2021-09-02T14:38:01Z

my review got ninja'd so hopefully things didn't get all tangled up and stop making sense

DMRobertson · 2021-09-03T10:45:28Z

There's a lot of duplication between UserDirectoryBackgroundUpdateStore.populate_user_directory_process_rooms and UserDirectoryHandler._track_user_joined_room.

I'd like to introduce free functions in the handler's module that I can import in the store. (Or maybe the other way round?). I was going to say "this can be a separate PR", but...

I've just noticed that _populate_user_directory_process_rooms in UserDirectoryBackgroundUpdateStore will read from room_memberships and so will populate the directory with per-room data. So maybe it is worth introducing those free functions now.

DMRobertson · 2021-09-03T12:14:23Z

I've just noticed that _populate_user_directory_process_rooms in UserDirectoryBackgroundUpdateStore will read from room_memberships and so will populate the directory with per-room data. So maybe it is worth introducing those free functions now.

Nah, this PR is long enough as it is. Let's consider this "don't leak local nicknames on updates". I'll open a separate PR for "don't leak local nicknames on rebuild".

reivilibre

Some substantial work has been done here; thanks for taking it on.

Happy that you're cleaning it up as you go!

docs/user_directory.md

synapse/handlers/user_directory.py

tests/handlers/test_user_directory.py

changelog.d/10695.bugfix

Co-authored-by: reivilibre <38398653+reivilibre@users.noreply.github.com>

reivilibre

LGTM, though I'm still new to reviewing so would like to get another pair of eyes on this (both to check that I haven't missed anything, and so that I can see what else I should be looking out for as a reviewer).

synapse/handlers/user_directory.py

* Fix reactivated users not being added to directory Co-authored-by: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>

I meant to merge this to develop, but screwed my git-fu. This reverts commit 401c1e8.

DMRobertson · 2021-09-09T18:43:15Z

@reivilibre thank you for reviewing this. @richvdh took a look at this too but struggled to follow. I offered to chop this up into lots of smaller PRs to make it easier to follow. With that in mind, I'll close this.

David Robertson added 3 commits August 31, 2021 11:48

Fix passing None to DB helpers

2b42abb

I don't think this will change any behaviour, but it makes PyCharm not highlight them as warnings which is less distracting.

Misc refactors

ef0a9c5

* type hint _get_next_batch * Introduce an enum MatchChange so there is a name rather than an `Optional[bool]`

changelog stub

038a9c5

DMRobertson force-pushed the dmr/user-directory-nickname-leaks branch from 500cea7 to 038a9c5 Compare August 31, 2021 10:48

David Robertson added 12 commits August 31, 2021 13:22

Docstrings, method reorder and rename

9366436

Remove unnecessary set() construction

1819999

Only update remote users dir. entries from events

bf97497

Refactor to remove a level of indentation

b348f0b

test_profile: register a user, not just a profile

13d6b6e

fixes a unit test. And we all seem to think that profiles should refer to users, even if that isn't enforced as a foreign key constraint in the db.

Use test-level "register_user", not the store

0d0f033

Feels like it's less bad

Missed an await GRR

cca4d10

Register user in RoomJoinRatelimitTestCase

137ecc8

I've made profile changes update the user directory, which in turn expects the corresponding user to actually exist.

Need the admin servlet to register_user

dc25098

Fix test: handler assumes lowercase username

90dcd65

Grrrrrr lint

d99eedb

DMRobertson changed the title ~~Draft: Dmr/user directory nickname leaks~~ Prevent local user profiles leaking when a "per-room profile" is changed Sep 1, 2021

DMRobertson marked this pull request as ready for review September 1, 2021 12:54

DMRobertson requested a review from a team September 1, 2021 12:54

reivilibre self-assigned this Sep 2, 2021

David Robertson added 2 commits September 2, 2021 15:15

Rename to clarify local versus remote name changes

ee72d78

Don't leak local users' per-room names on join

61d3e88

reivilibre suggested changes Sep 2, 2021

View reviewed changes

changelog.d/10695.bugfix Outdated Show resolved Hide resolved

synapse/handlers/user_directory.py Outdated Show resolved Hide resolved

synapse/handlers/user_directory.py Outdated Show resolved Hide resolved

synapse/handlers/user_directory.py Outdated Show resolved Hide resolved

reivilibre reviewed Sep 2, 2021

View reviewed changes

synapse/handlers/user_directory.py Outdated Show resolved Hide resolved

reivilibre reviewed Sep 2, 2021

View reviewed changes

synapse/handlers/user_directory.py Outdated Show resolved Hide resolved

Comment where we still have work TODO

359de94

David Robertson added 7 commits September 3, 2021 11:46

Typing in storage module for user_directory

474c212

Comments about leakages

38bbde8

db user dir: pull out processing a room to method

a3c023d

Exit early if process_single_room if not in room

90794cb

Describe data model in docs

e90524c

Additional comments on the bg process

bc9780f

Linttttttt

bbacf47

David Robertson added 2 commits September 3, 2021 13:18

Update changelog

6cca7c6

Rationale for synapse test, not complement

d50b492

DMRobertson requested review from reivilibre and removed request for reivilibre September 3, 2021 12:49

reivilibre suggested changes Sep 3, 2021

View reviewed changes

David Robertson and others added 4 commits September 3, 2021 19:51

Oliver's suggestions from code review.

0a217b9

Co-authored-by: reivilibre <38398653+reivilibre@users.noreply.github.com>

Fixup docs as per review

9c4b7ef

Fix coment and extra check, as per review

8662e5f

Improve docstring for _handle_remove_user

8269250

DMRobertson requested a review from reivilibre September 3, 2021 19:07

DMRobertson mentioned this pull request Sep 6, 2021

Add test case reproducing matrix-org/synapse#5677 for local users matrix-org/complement#199

Merged

reivilibre reviewed Sep 7, 2021

View reviewed changes

synapse/handlers/user_directory.py Show resolved Hide resolved

reivilibre requested a review from a team September 7, 2021 10:11

David Robertson and others added 2 commits September 8, 2021 13:56

Fix reactivated users not being added to directory (#10762)

401c1e8

* Fix reactivated users not being added to directory Co-authored-by: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>

Revert "Fix reactivated users not being added to directory (#10762)"

2b0d714

I meant to merge this to develop, but screwed my git-fu. This reverts commit 401c1e8.

This was referenced Sep 9, 2021

Fix leaking per-room profiles for local users when rebuilding directory #10761

Closed

Easy refactors of the user directory #10789

Merged

DMRobertson closed this Sep 9, 2021

DMRobertson deleted the dmr/user-directory-nickname-leaks branch October 8, 2021 14:16

Uh oh!

Conversation

DMRobertson commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DMRobertson commented Sep 1, 2021

Uh oh!

DMRobertson commented Sep 2, 2021

Uh oh!

reivilibre commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DMRobertson commented Sep 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reivilibre commented Sep 2, 2021

Uh oh!

DMRobertson commented Sep 3, 2021

Uh oh!

DMRobertson commented Sep 3, 2021

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DMRobertson commented Sep 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DMRobertson commented Aug 25, 2021 •

edited

Loading

reivilibre commented Sep 2, 2021 •

edited

Loading