Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC2197: Search Filtering in Federation /publicRooms #2197

Merged
merged 8 commits into from Aug 20, 2019

Conversation

@reivilibre
Copy link
Contributor

commented Jul 29, 2019

This MSC proposes introducing the POST method to the /publicRooms Federation API endpoint, including a filter argument which allows server-side filtering of rooms.

We are motivated by the opportunity to make searching the public Room Directory more efficient over Federation.

Rendered

MSC for Search Filtering in Federation /publicRooms
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>

At current, remote homeservers do not learn about what a user has searched for.

However, under this proposal, in the context of using the Federation API to forward on queries from the Client-Server

This comment has been minimized.

Copy link
@Half-Shot

Half-Shot Jul 29, 2019

Contributor

I can certainly see this being an issue for some users, although I would put forward that you'd expect your search request to be forwarded to a remote homeserver if you are searching within it. There probably needs to be a warning within clients that a remote search will take your data outside the jurisdiction of your own homeserver.

Hopefully something like this wouldn't prompt the need for a full blown consent screen.

@turt2live turt2live self-requested a review Jul 30, 2019

@turt2live
Copy link
Member

left a comment

looks pretty straightforward to me. Good to see that this fixes vector-im/riot-web#3327 too :)

Rewrap lines in MSC2917 to 80 chars wide
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

As requested, I have rewrapped the lines to 80 chars wide.

@lampholder

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

There probably needs to be a warning within clients that a remote search will take your data outside the jurisdiction of your own homeserver.

Can we add a line to the spec to client devs telling them they SHOULD display such a warning?

Hopefully something like this wouldn't prompt the need for a full blown consent screen.

Yeah, this definitely feels like it would be too much. The data that's being shared is:

  • only covered by GDPR if:
    • the search terms contain personal data, or
    • the user's homeserver IP is uniquely identifying (because it's a single-person homeserver, perhaps)
  • likely to be expected to be shared with the remote homeserver anyway

But just to remove any ambiguity (both in the users' mind and if we're ever asked to demonstrate our legitimate processing of these search terms on matrix.org) let's advise client devs to implement that warning (and implement that warning in Riots when we can).

@ara4n

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

this looks generally sane but istr there was a reason why erik didn’t implement it like this in the first place. can’t think of what that would be though, so: lgtm.

we should consider how this interacts with the whole “don’t show public rooms to non-local users” logic though.

@reivilibre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Sorry @ara4n, please can you clarify what you mean by

don’t show public rooms to non-local users

If you mean the m.federate (in m.room.create state event) stuff, then I don't see any problem.

@ara4n

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

i meant matrix-org/synapse#5534 - having reviewed, i think we’re fine. ignore me.

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

barring @lampholder's feedback, I think this is largely ready to go and people seem to agree:

@mscbot fcp merge

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@mscbot concern @lampholder has feedback

MSC2197: update with privacy perspective
Includes recommendations for client developers.

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

I have updated the MSC with your perspective on privacy, @lampholder – I hope you don't mind. Does this seem reasonable now?

I take it to mean that this MSC will lead to us adding this recommendation in the C-S API /publicRooms (perhaps in the documentation for the server parameter?). Am I understanding correctly?

@turt2live turt2live self-requested a review Aug 8, 2019

Addresses some of Andrew's comments
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>

- only covered by GDPR if:
- the search terms contain personal data, or
- the user's homeserver IP address is uniquely identifying (because it's a

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 9, 2019

Member

The origin of the homeserver is also shared because of the auth the API requires. If I run a travisralston.me homeserver, it is identified as me. However this is a problem for MSC1228 to solve.

This comment has been minimized.

Copy link
@reivilibre

reivilibre Aug 14, 2019

Author Contributor

@turt2live Is this resolved now? :)

@turt2live
Copy link
Member

left a comment

(still)

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

I think @lampholder's concerns are resolved, and if they aren't then he can add his own concerns. I only really added the concern to make sure that the concerns got addressed before this entered FCP, if that were to happen while @lampholder was on vacation.

However, even if this were to enter FCP today it would still give @lampholder time to make his own concerns (if he wanted to). So, because I haven't pinged @lampholder enough in this comment:

@mscbot resolve @lampholder has feedback

Domain name is potentially personally-identifying
Thanks to @turt2live

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
reivilibre added 2 commits Aug 14, 2019
Acknowledge other potential error responses for fallback
Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
Drop the hard SHOULD
Adopts @turt2live's phrasing

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>

@reivilibre reivilibre requested a review from anoadragon453 Aug 14, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

Looks good to me :)

@mscbot reviewed

@richvdh
Copy link
Member

left a comment

generally looks good to me, a few comments


* `limit` (`integer`): Limit the number of search results returned.
* `since` (`string`): A pagination token from a previous request, allowing
clients to get the next (or previous) batch of rooms. The direction of

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 15, 2019

Member

🤔 why does the API let us paginate in both directions?

This comment has been minimized.

Copy link
@reivilibre

reivilibre Aug 19, 2019

Author Contributor

Don't ask me! It's how the C-S API is, however…

@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 15, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@richvdh richvdh referenced this pull request Aug 16, 2019
3 of 3 tasks complete
@mscbot

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@turt2live turt2live merged commit 53a2ffb into matrix-org:master Aug 20, 2019

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

@turt2live turt2live self-assigned this Aug 26, 2019

@turt2live

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Implementation is currently at matrix-org/synapse#5859

@turt2live turt2live removed their assignment Aug 26, 2019

@turt2live turt2live added this to Implementation stages in Client-server r0.6 proposals Aug 26, 2019

@turt2live turt2live moved this from Implementation stages to Needs spec in Client-server r0.6 proposals Aug 30, 2019

@turt2live turt2live self-assigned this Aug 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.