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

Directly lookup local membership instead of getting all members in a room first (get_users_in_room mis-use) #13608

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 5 additions & 2 deletions synapse/handlers/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,11 @@ async def get_event(
if not event:
return None

users = await self.store.get_users_in_room(event.room_id)
is_peeking = user.to_string() not in users
is_user_in_room = await self.store.check_local_user_in_room(
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
user_id=user.to_string(), room_id=event.room_id
)
# The user is peeking if they aren't in the room already
is_peeking = not is_user_in_room

filtered = await filter_events_for_client(
self._storage_controllers, user.to_string(), [event], is_peeking=is_peeking
Expand Down
6 changes: 4 additions & 2 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -761,8 +761,10 @@ async def _is_exempt_from_privacy_policy(
async def _is_server_notices_room(self, room_id: str) -> bool:
if self.config.servernotices.server_notices_mxid is None:
return False
user_ids = await self.store.get_users_in_room(room_id)
return self.config.servernotices.server_notices_mxid in user_ids
is_server_notices_room = await self.store.check_local_user_in_room(
user_id=self.config.servernotices.server_notices_mxid, room_id=room_id
)
return is_server_notices_room
Comment on lines 761 to +767
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several of these is_server_notices_room kind of functions but this PR doesn't deduplicate them.


async def assert_accepted_privacy_policy(self, requester: Requester) -> None:
"""Check if a user has accepted the privacy policy
Expand Down
7 changes: 5 additions & 2 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1284,8 +1284,11 @@ async def get_event_context(
before_limit = math.floor(limit / 2.0)
after_limit = limit - before_limit

users = await self.store.get_users_in_room(room_id)
is_peeking = user.to_string() not in users
is_user_in_room = await self.store.check_local_user_in_room(
user_id=user.to_string(), room_id=room_id
)
# The user is peeking if they aren't in the room already
is_peeking = not is_user_in_room

async def filter_evts(events: List[EventBase]) -> List[EventBase]:
if use_admin_priviledge:
Expand Down
6 changes: 4 additions & 2 deletions synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -1620,8 +1620,10 @@ async def _is_host_in_room(self, current_state_ids: StateMap[str]) -> bool:
async def _is_server_notice_room(self, room_id: str) -> bool:
if self._server_notices_mxid is None:
return False
user_ids = await self.store.get_users_in_room(room_id)
return self._server_notices_mxid in user_ids
is_server_notices_room = await self.store.check_local_user_in_room(
user_id=self._server_notices_mxid, room_id=room_id
)
return is_server_notices_room


class RoomMemberMasterHandler(RoomMemberHandler):
Expand Down
10 changes: 8 additions & 2 deletions synapse/server_notices/server_notices_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ async def maybe_get_notice_room_for_user(self, user_id: str) -> Optional[str]:
Returns:
The room's ID, or None if no room could be found.
"""
# If there is no server notices MXID, then there is no server notices room
if self.server_notices_mxid is None:
return None
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved

rooms = await self._store.get_rooms_for_local_user_where_membership_is(
user_id, [Membership.INVITE, Membership.JOIN]
)
Expand All @@ -111,8 +115,10 @@ async def maybe_get_notice_room_for_user(self, user_id: str) -> Optional[str]:
# be joined. This is kinda deliberate, in that if somebody somehow
# manages to invite the system user to a room, that doesn't make it
# the server notices room.
user_ids = await self._store.get_users_in_room(room.room_id)
if len(user_ids) <= 2 and self.server_notices_mxid in user_ids:
is_server_notices_room = await self._store.check_local_user_in_room(
user_id=self.server_notices_mxid, room_id=room.room_id
)
if is_server_notices_room:
# we found a room which our user shares with the system notice
# user
return room.room_id
Expand Down
26 changes: 26 additions & 0 deletions synapse/storage/databases/main/roommember.py
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,32 @@ async def get_local_users_in_room(self, room_id: str) -> List[str]:
desc="get_local_users_in_room",
)

async def check_local_user_in_room(self, user_id: str, room_id: str) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is similar to check_user_in_room that is in synapse/api/auth.py but that one has some specific logic and can check for leave so I decided to leave it as-is.

async def check_user_in_room(
self,
room_id: str,
requester: Requester,
allow_departed_users: bool = False,
) -> Tuple[str, Optional[str]]:
"""Check if the user is in the room, or was at some point.
Args:
room_id: The room to check.
requester: The user making the request, according to the access token.
current_state: Optional map of the current state of the room.
If provided then that map is used to check whether they are a
member of the room. Otherwise the current membership is
loaded from the database.
allow_departed_users: if True, accept users that were previously
members but have now departed.
Raises:
AuthError if the user is/was not in the room.
Returns:
The current membership of the user in the room and the
membership event ID of the user.
"""
user_id = requester.user.to_string()
(
membership,
member_event_id,
) = await self.store.get_local_current_membership_for_user_in_room(
user_id=user_id,
room_id=room_id,
)
if membership:
if membership == Membership.JOIN:
return membership, member_event_id
# XXX this looks totally bogus. Why do we not allow users who have been banned,
# or those who were members previously and have been re-invited?
if allow_departed_users and membership == Membership.LEAVE:
forgot = await self.store.did_forget(user_id, room_id)
if not forgot:
return membership, member_event_id
raise UnstableSpecAuthError(
403,
"User %s not in room %s" % (user_id, room_id),
errcode=Codes.NOT_JOINED,
)

"""
Check whether a given local user is currently joined to the given room.

Returns:
A boolean indicating whether the user is currently joined to the room

Raises:
Exeption when called with a non-local user to this homeserver
"""
if not self.hs.is_mine_id(user_id):
raise Exception(
"Cannot call 'check_local_user_in_room' on "
"non-local user %s" % (user_id,),
)

(
membership,
member_event_id,
) = await self.get_local_current_membership_for_user_in_room(
user_id=user_id,
room_id=room_id,
)

return membership == Membership.JOIN

async def get_local_current_membership_for_user_in_room(
self, user_id: str, room_id: str
) -> Tuple[Optional[str], Optional[str]]:
Expand Down