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

Commit

Permalink
Consistently exclude from user_directory (#10960)
Browse files Browse the repository at this point in the history
* Introduce `should_include_local_users_in_dir`

We exclude three kinds of local users from the user_directory tables. At
present we don't consistently exclude all three in the same places. This
commit introduces a new function to gather those exclusion conditions
together. Because we have to handle local and remote users in different
ways, I've made that function only consider the case of remote users.
It's the caller's responsibility to make the local versus remote
distinction clear and correct.

A test fixup is required. The test now hits a path which makes db
queries against the users table. The expected rows were missing, because
we were using a dummy user that hadn't actually been registered.

We also add new test cases to covert the exclusion logic.

----

By my reading this makes these changes:

* When an app service user registers or changes their profile, they will
  _not_ be added to the user directory. (Previously only support and
  deactivated users were excluded). This is consistent with the logic that
  rebuilds the user directory. See also [the discussion
  here](#10914 (comment)).
* When rebuilding the directory, exclude support and disabled users from
  room sharing tables. Previously only appservice users were excluded.
* Exclude all three categories of local users when rebuilding the
  directory. Previously `_populate_user_directory_process_users` didn't do
  any exclusion.

Co-authored-by: Richard van der Hoff <[email protected]>
  • Loading branch information
David Robertson and richvdh authored Oct 4, 2021
1 parent a0f48ee commit f7b034a
Show file tree
Hide file tree
Showing 7 changed files with 409 additions and 57 deletions.
1 change: 1 addition & 0 deletions changelog.d/10960.bugfix
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.
27 changes: 9 additions & 18 deletions synapse/handlers/user_directory.py
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 await self.store.should_include_local_user_in_dir(user_id):
await self.store.update_profile_in_user_dir(
user_id, profile.display_name, profile.avatar_url
)
Expand Down Expand Up @@ -229,8 +224,10 @@ 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:
include_in_dir = not self.is_mine_id(
state_key
) or await self.store.should_include_local_user_in_dir(state_key)
if include_in_dir:
if change is MatchChange.no_change:
# Handle any profile changes
await self._handle_profile_change(
Expand Down Expand Up @@ -356,13 +353,7 @@ 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):

is_appservice = self.store.get_if_app_services_interested_in_user(
user_id
)

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

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

if to_insert:
Expand Down
46 changes: 33 additions & 13 deletions synapse/storage/databases/main/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,10 @@

logger = logging.getLogger(__name__)


TEMP_TABLE = "_temp_populate_user_directory"


class UserDirectoryBackgroundUpdateStore(StateDeltasStore):

# How many records do we calculate before sending it to
# add_users_who_share_private_rooms?
SHARE_PRIVATE_WORKING_SET = 500
Expand Down Expand Up @@ -235,6 +233,13 @@ def _get_next_batch(
)

users_with_profile = await self.get_users_in_room_with_profiles(room_id)
# Throw away users excluded from the directory.
users_with_profile = {
user_id: profile
for user_id, profile in users_with_profile.items()
if not self.hs.is_mine_id(user_id)
or await self.should_include_local_user_in_dir(user_id)
}

# Update each user in the user directory.
for user_id, profile in users_with_profile.items():
Expand All @@ -246,22 +251,19 @@ def _get_next_batch(

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

to_insert.add(user_id)

if to_insert:
await self.add_users_in_public_rooms(room_id, to_insert)
to_insert.clear()
else:
for user_id in users_with_profile:
# We want the set of pairs (L, M) where L and M are
# in `users_with_profile` and L is local.
# Do so by looking for the local user L first.
if not self.hs.is_mine_id(user_id):
continue

if self.get_if_app_services_interested_in_user(user_id):
continue

for other_user_id in users_with_profile:
if user_id == other_user_id:
continue
Expand Down Expand Up @@ -349,10 +351,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 await self.should_include_local_user_in_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 +372,24 @@ def _get_next_batch(txn: LoggingTransaction) -> Optional[List[str]]:

return len(users_to_work_on)

async def should_include_local_user_in_dir(self, user: str) -> bool:
"""Certain classes of local user are omitted from the user directory.
Is this user one of them?
"""
# App service users aren't usually contactable, so exclude them.
if self.get_if_app_services_interested_in_user(user):
# TODO we might want to make this configurable for each app service
return False

# Support users are for diagnostics and should not appear in the user directory.
if await self.is_support_user(user):
return False

# Deactivated users aren't contactable, so should not appear in the user directory.
if await self.get_user_deactivated_status(user):
return False
return True

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 @@ -537,7 +558,6 @@ async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> No


class UserDirectoryStore(UserDirectoryBackgroundUpdateStore):

# How many records do we calculate before sending it to
# add_users_who_share_private_rooms?
SHARE_PRIVATE_WORKING_SET = 500
Expand Down
Loading

0 comments on commit f7b034a

Please sign in to comment.