/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Feb 22, 2016

Conversation

Reviewers
No reviews
Assignees

@illicitonion illicitonion

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@richvdh @erikjohnston @illicitonion @NegativeMjark
@richvdh
Member

richvdh commented Feb 19, 2016

There should be no difference between guest users and non-guest users in terms
of access to messages. Define the semantics of the is_peeking argument to
filter_events_for_clients (slightly) better; interpret it appropriately, and
set it correctly from /sync.

(fixes vector-im/vector-web#949)
@richvdh
Allow guest users access to messages in rooms they have joined 
There should be no difference between guest users and non-guest users in terms
of access to messages. Define the semantics of the is_peeking argument to
filter_events_for_clients (slightly) better; interpret it appropriately, and
set it correctly from /sync.

Some checks were not successful

1 errored and 1 successful checks
@matrixbot
Synapse — Build #6032 failed in 4 min 57 sec
Details
@matrixbot
default — Build finished. 323 tests run, 0 skipped, 0 failed.
Details
b71ca2b

@richvdh richvdh assigned NegativeMjark and richvdh and unassigned NegativeMjark Feb 19, 2016

@richvdh richvdh assigned NegativeMjark and unassigned richvdh Feb 19, 2016

@richvdh richvdh referenced this pull request in matrix-org/sytest Feb 19, 2016

Merged

Improve tests for history visibility #189

@NegativeMjark NegativeMjark commented on an outdated diff Feb 19, 2016

View full outdated diff
synapse/handlers/_base.py
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.
@NegativeMjark

NegativeMjark Feb 19, 2016

Contributor

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
@richvdh
Interpret unknown visibilities the same as shared

All checks have passed

2 successful checks
@matrixbot
Synapse — Build #6062 succeeded in 4 min 41 sec
Details
@matrixbot
default — Build finished. 342 tests run, 0 skipped, 0 failed.
Details
6c5b147
@richvdh
Member

richvdh commented Feb 19, 2016

my bad. PTAL?

@richvdh richvdh assigned illicitonion and unassigned NegativeMjark Feb 22, 2016

@richvdh
Member

richvdh commented Feb 22, 2016

@illicitonion: please take a look
@richvdh
Member

richvdh commented Feb 22, 2016

@illicitonion: please take a look.

Any idea what the special-case for RoomHistoryVisibility which i've currently commented-out is for? it appears to have been introduced by @erikjohnston in 41938af

@richvdh richvdh referenced this pull request Feb 22, 2016

Closed

handlers/_base: allow peeking users to see events in rooms they are members of #593

@erikjohnston
Owner

erikjohnston commented Feb 22, 2016

Any idea what the special-case for RoomHistoryVisibility which i've currently commented-out is for? it appears to have been introduced by @erikjohnston in 41938af

I'm guessing that it was so that users can see when the visibility changes from joined to shared, etc, so that the UI can say something like "You don't have permission to view any messages from before this time" instead of the history suddenly stopping.

@illicitonion illicitonion commented on the diff Feb 22, 2016

View full changes
synapse/handlers/_base.py
""" 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
@illicitonion

illicitonion Feb 22, 2016

Contributor

Param style is wrong here; should be:

Args:
argname (type): Description

(and below)

@richvdh

richvdh Feb 22, 2016

Member

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.

@illicitonion illicitonion commented on an outdated diff Feb 22, 2016

View full outdated diff
synapse/handlers/_base.py
if membership == Membership.JOIN:
return True
if event.type == EventTypes.RoomHistoryVisibility:
- return not is_peeking
+ # XXX why are m.room.history_visibility events special?
@illicitonion

illicitonion Feb 22, 2016

Contributor

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.

@illicitonion illicitonion commented on an outdated diff Feb 22, 2016

View full outdated diff
synapse/handlers/_base.py
elif visibility == "invited":
+ # user can also see the event if he was *invited* at the time
@illicitonion

illicitonion Feb 22, 2016

Contributor

s/he was/they were/

@illicitonion illicitonion commented on an outdated diff Feb 22, 2016

View full outdated diff
synapse/handlers/_base.py
return membership == Membership.INVITE
- return True
+ else:
+ # visibility is shared: user can also see the event if he has
@illicitonion

illicitonion Feb 22, 2016

Contributor

s/he has/they have/
@richvdh
address review comments 
drop commented-out special casing for historyvisibility event
s/he/they/ for users

All checks have passed

2 successful checks
@matrixbot
Synapse — Build #6081 succeeded in 5 min 22 sec
Details
@matrixbot
default — Build finished. 342 tests run, 0 skipped, 0 failed.
Details
5be3944
@illicitonion
Contributor

illicitonion commented Feb 22, 2016

LGTM

richvdh added a commit that referenced this pull request Feb 22, 2016

@richvdh
Merge pull request #587 from matrix-org/rav/guest_access_after_room_join 
Allow guest users access to messages in rooms they have joined
f7e3de0

@richvdh richvdh merged commit f7e3de0 into develop Feb 22, 2016
2 checks passed

2 checks passed

Synapse Build #6081 succeeded in 5 min 22 sec
Details
default Build finished. 342 tests run, 0 skipped, 0 failed.
Details

@richvdh richvdh deleted the rav/guest_access_after_room_join branch Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment