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

Allow guest users access to messages in rooms they have joined #587

Merged
merged 4 commits into from
Feb 22, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
52 changes: 41 additions & 11 deletions synapse/handlers/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,15 @@ def __init__(self, hs):
self.event_builder_factory = hs.get_event_builder_factory()

@defer.inlineCallbacks
def _filter_events_for_clients(self, user_tuples, events, event_id_to_state):
def filter_events_for_clients(self, user_tuples, events, event_id_to_state):
""" Returns dict of user_id -> list of events that user is allowed to
see.

:param (str, bool) user_tuples: (user id, is_peeking) for each
Copy link
Contributor

Choose a reason for hiding this comment

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

Param style is wrong here; should be:

Args:
argname (type): Description

(and below)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. At least, that is not the convention we agreed last time it was discussed, which was https://www.jetbrains.com/pycharm/help/type-hinting-in-pycharm.html#legacy.

user to be checked. is_peeking should be true if:
* the user is not currently a member of the room, and:
* the user has not been a member of the room since the given
events
"""
forgotten = yield defer.gatherResults([
self.store.who_forgot_in_room(
Expand All @@ -72,18 +78,20 @@ def _filter_events_for_clients(self, user_tuples, events, event_id_to_state):
def allowed(event, user_id, is_peeking):
state = event_id_to_state[event.event_id]

# get the room_visibility at the time of the event.
visibility_event = state.get((EventTypes.RoomHistoryVisibility, ""), None)
if visibility_event:
visibility = visibility_event.content.get("history_visibility", "shared")
else:
visibility = "shared"

# if it was world_readable, it's easy: everyone can read it
if visibility == "world_readable":
return True

if is_peeking:
return False

# get the user's membership at the time of the event. (or rather,
# just *after* the event. Which means that people can see their
# own join events, but not (currently) their own leave events.)
membership_event = state.get((EventTypes.Member, user_id), None)
if membership_event:
if membership_event.event_id in event_id_forgotten:
Expand All @@ -93,20 +101,32 @@ def allowed(event, user_id, is_peeking):
else:
membership = None

# if the user was a member of the room at the time of the event,
# they can see it.
if membership == Membership.JOIN:
return True

if event.type == EventTypes.RoomHistoryVisibility:
return not is_peeking
# XXX why are m.room.history_visibility events special?
Copy link
Contributor

Choose a reason for hiding this comment

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

I've talked with Erik, and we believe the reason for this is that it allows clients to actually see that history visibility changes happened, partially because they can't see the event which opens up history visibility itself.

I contend that this is bad, partially because it leaks membership information about otherwise restricted rooms.

I'm fairly convinced that what we want to do is either:

  1. Stop special casing it completely. This means that they necessarily won't see a room become shared.
  2. Special case it by allowing it to be returned if the state before or after the event would allow you to see it, rather than just before as we currently do.

# return True
pass

if visibility == "shared":
return True
elif visibility == "joined":
return membership == Membership.JOIN
# user can also see the event if he has become a member since
# the event
#
# XXX: if the user has subsequently joined and then left again,
# ideally we would share history up to the point they left. But
# we don't know when they left.
return not is_peeking
elif visibility == "invited":
# user can also see the event if he was *invited* at the time
# of the event.
return membership == Membership.INVITE

return True
# presumably visibility is "joined"; we weren't a member at the
# time of the event, so we're done.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the server is supposed to assume that the room is "shared" if it doesn't understand the value of history_visibility.

https://matrix.org/speculator/spec/HEAD/client_server.html#id31

return False

defer.returnValue({
user_id: [
Expand All @@ -119,7 +139,17 @@ def allowed(event, user_id, is_peeking):

@defer.inlineCallbacks
def _filter_events_for_client(self, user_id, events, is_peeking=False):
# Assumes that user has at some point joined the room if not is_guest.
"""
Check which events a user is allowed to see

:param str user_id: user id to be checked
:param [synapse.events.EventBase] events: list of events to be checked
:param bool is_peeking should be True if:
* the user is not currently a member of the room, and:
* the user has not been a member of the room since the given
events
:rtype [synapse.events.EventBase]
"""
types = (
(EventTypes.RoomHistoryVisibility, ""),
(EventTypes.Member, user_id),
Expand All @@ -128,7 +158,7 @@ def _filter_events_for_client(self, user_id, events, is_peeking=False):
frozenset(e.event_id for e in events),
types=types
)
res = yield self._filter_events_for_clients(
res = yield self.filter_events_for_clients(
[(user_id, is_peeking)], events, event_id_to_state
)
defer.returnValue(res.get(user_id, []))
Expand Down
2 changes: 0 additions & 2 deletions synapse/handlers/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,6 @@ def load_filtered_recents(self, room_id, sync_config, now_token,
recents = yield self._filter_events_for_client(
sync_config.user.to_string(),
recents,
is_peeking=sync_config.is_guest,
)
else:
recents = []
Expand All @@ -667,7 +666,6 @@ def load_filtered_recents(self, room_id, sync_config, now_token,
loaded_recents = yield self._filter_events_for_client(
sync_config.user.to_string(),
loaded_recents,
is_peeking=sync_config.is_guest,
)
loaded_recents.extend(recents)
recents = loaded_recents
Expand Down
2 changes: 1 addition & 1 deletion synapse/push/bulk_push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def action_for_event_by_user(self, event, handler, current_state):

users_dict = yield self.store.are_guests(self.rules_by_user.keys())

filtered_by_user = yield handler._filter_events_for_clients(
filtered_by_user = yield handler.filter_events_for_clients(
users_dict.items(), [event], {event.event_id: current_state}
)

Expand Down