Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Consistently exclude users from directory #10914

Closed
wants to merge 9 commits into from
Closed

Consistently exclude users from directory #10914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da02259
Consistently exclude support, deactivated, and appservice users
Sep 24, 2021
c9913cf
Test user dir rebuilding in storage, not handlers
Sep 24, 2021
ad7ea64
Fix test_disabling_room_list
Sep 24, 2021
acf94bf
Test that exclusions apply when rebuilding dir
Sep 24, 2021
556566f
Fix type hint for `user_directory_stream_pos`
Sep 24, 2021
969d0d1
Use patch rather than assigning mocks
Sep 24, 2021
ff89fb8
LintyMcLintFace
Sep 24, 2021
ff177a8
Ch-ch-ch-ch-changelog
Sep 24, 2021
2e32e1c
Fix failing unit tests
Sep 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/10914.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where rebuilding the user directory wouldn't exclude support and disabled users.
25 changes: 7 additions & 18 deletions synapse/handlers/user_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,7 @@ async def handle_local_profile_change(
# FIXME(#3714): We should probably do this in the same worker as all
# the other changes.

# Support users are for diagnostics and should not appear in the user directory.
is_support = await self.store.is_support_user(user_id)
# When change profile information of deactivated user it should not appear in the user directory.
is_deactivated = await self.store.get_user_deactivated_status(user_id)

if not (is_support or is_deactivated):
if not await self.store.is_excluded_from_user_dir(user_id):
Copy link
Member

Choose a reason for hiding this comment

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

I find the double-negative here a bit mind-bending. Perhaps we should instead have an should_include_in_user_dir ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad it's not just me. I was erring towards preserving the original logic below as much as possible, but think it'll be better this way round.

await self.store.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)
Expand Down Expand Up @@ -229,8 +224,8 @@ async def _handle_deltas(self, deltas: List[Dict[str, Any]]) -> None:
else:
logger.debug("Server is still in room: %r", room_id)

is_support = await self.store.is_support_user(state_key)
if not is_support:
excluded = await self.store.is_excluded_from_user_dir(state_key)
if not excluded:
if change is MatchChange.no_change:
# Handle any profile changes
await self._handle_profile_change(
Expand Down Expand Up @@ -357,12 +352,8 @@ async def _handle_new_user(
# First, if they're our user then we need to update for every user
if self.is_mine_id(user_id):
Copy link
Member

Choose a reason for hiding this comment

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

think this is redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we'll check to see if they're local within is_excluded_from_user_dir? I agree...

But. I think putting the localness check into is_excluded_from_user_dir makes things more confusing. To a first approximation, it's true that we'll never exclude a remote user from the user directory. With one exception: users_who_share_private_rooms. This contains triples (local_user_id, any_user_id, room_id) where the two users both belong to the given private room. Only local users live in the first entry of those tuples. (I think this is an optimisation?) To maintain that there's extra logic which needs to ensure we don't put remote users into that column.

I think it's going to be clearer if we have a function should_include_local_user_in_dir and make the local-vs-remote logic explicit.


is_appservice = self.store.get_if_app_services_interested_in_user(
user_id
)

# We don't care about appservice users.
if not is_appservice:
excluded = await self.store.is_excluded_from_user_dir(user_id)
if not excluded:
for other_user_id in other_users_in_room:
if user_id == other_user_id:
continue
Expand All @@ -374,10 +365,8 @@ async def _handle_new_user(
if user_id == other_user_id:
continue

is_appservice = self.store.get_if_app_services_interested_in_user(
other_user_id
)
if self.is_mine_id(other_user_id) and not is_appservice:
excluded = await self.store.is_excluded_from_user_dir(other_user_id)
if not excluded:
to_insert.add((other_user_id, user_id))

if to_insert:
Expand Down
38 changes: 28 additions & 10 deletions synapse/storage/databases/main/user_directory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,16 @@ def _get_next_batch(

# Update each user in the user directory.
for user_id, profile in users_with_profile.items():
await self.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)
if not await self.is_excluded_from_user_dir(user_id):
Copy link
Member

Choose a reason for hiding this comment

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

it's probably fine, but for the record: what's the logic behind adding this here, and at line 353, where there was no check before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consistency with _handle_deltas in the handlers code.

Rebuilding the user directory from scratch (here) should have the same outcome as starting with an empty directory and building it by listening for membership events (_handle_deltas). Before my changes, the latter excluded support and deactivated users, so I think the former should too.

await self.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)

to_insert = set()

if is_public:
for user_id in users_with_profile:
if self.get_if_app_services_interested_in_user(user_id):
if await self.is_excluded_from_user_dir(user_id):
continue

to_insert.add(user_id)
Expand All @@ -259,7 +260,7 @@ def _get_next_batch(
if not self.hs.is_mine_id(user_id):
continue

if self.get_if_app_services_interested_in_user(user_id):
if await self.is_excluded_from_user_dir(user_id):
continue

for other_user_id in users_with_profile:
Expand Down Expand Up @@ -349,10 +350,11 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]:
)

for user_id in users_to_work_on:
profile = await self.get_profileinfo(get_localpart_from_id(user_id))
await self.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)
if not await self.is_excluded_from_user_dir(user_id):
profile = await self.get_profileinfo(get_localpart_from_id(user_id))
await self.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)

# We've finished processing a user. Delete it from the table.
await self.db_pool.simple_delete_one(
Expand All @@ -369,6 +371,22 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]:

return len(users_to_work_on)

async def is_excluded_from_user_dir(self, user: str) -> bool:
"""Certain classes of local user are omitted from the user directory.
Is this user one of them?
"""
if self.hs.is_mine_id(user):
Copy link
Member

Choose a reason for hiding this comment

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

please can we do an early return rather than a big nested if statement?

# Support users are for diagnostics and should not appear in the user directory.
if await self.is_support_user(user):
return True
# Deactivated users aren't contactable, so should not appear in the user directory.
if await self.get_user_deactivated_status(user):
return True
# App service users aren't usually contactable, so exclude them.
if self.get_if_app_services_interested_in_user(user):
return True
Comment on lines +386 to +387
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure what to think about this.
I have some appservice-generated users in my homeserver's search directory. I can't confirm or deny whether I've done something wrong to wind up with that happening, but thought it was worth mentioning in case this should trigger the question of 'do we really intend to exclude AS users from the directory?'.

Both yes and no make sense here. No, on a lot of networks (IRC probably? Discord when using a guild-bot.), the users aren't contactable.
But yes, on some networks, users are contactable and there's nothing inherently wrong with letting you discover them when trying to start a DM ...

I would guess that the 'No' cases outweigh the 'Yes' cases, so I think what you're doing is sensible.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like most of the code currently excludes AS users from the directory, but there are enough bugs that they leak through.

At least we're making it consistent here. There might be scope for a future change where we make this configurable per-appservice, but let's leave it for now.

Comment on lines +386 to +387
Copy link
Member

Choose a reason for hiding this comment

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

wonder if we should do this test first, since it doesn't require a database hit and should be pretty quick.

return False

async def is_room_world_readable_or_publicly_joinable(self, room_id: str) -> bool:
"""Check if the room is either world_readable or publically joinable"""

Expand Down Expand Up @@ -527,7 +545,7 @@ async def get_user_in_directory(self, user_id: str) -> Optional[Dict[str, str]]:
desc="get_user_in_directory",
)

async def update_user_directory_stream_pos(self, stream_id: int) -> None:
async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> None:
await self.db_pool.simple_update_one(
table="user_directory_stream_pos",
keyvalues={},
Expand Down
Loading