From b8ed66390aec1f5efbf7c9a49840da912358b12f Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 21 Apr 2019 00:58:53 +0100 Subject: [PATCH 01/29] add option to require an access_token to GET /profile on CS API --- synapse/config/server.py | 8 ++++++++ synapse/rest/client/v1/profile.py | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/synapse/config/server.py b/synapse/config/server.py index 08e4e45482e1..8ad42a8a6c8f 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -71,6 +71,10 @@ def read_config(self, config): # master, potentially causing inconsistency. self.enable_media_repo = config.get("enable_media_repo", True) + # whether to require users to authenticate in order to query /profile + # endpoints via CS API. this is a workaround in advance of MSC1301 landing + self.auth_profile_reqs = config.get("auth_profile_reqs", True) + # whether to enable search. If disabled, new entries will not be inserted # into the search tables and they will not be indexed. Users will receive # errors when attempting to search for messages. @@ -318,6 +322,10 @@ def default_config(self, server_name, data_dir_path, **kwargs): # #use_presence: false + # whether to require users to authenticate in order to query /profile + # endpoints via CS API. this is a workaround in advance of MSC1301 landing + #auth_profile_reqs: false + # The GC threshold parameters to pass to `gc.set_threshold`, if defined # #gc_thresholds: [700, 10, 10] diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index a23edd8fe5e7..c7d3d43d7c74 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -31,6 +31,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + if self.hs.config.auth_profile_reqs: + yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( @@ -74,6 +76,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + if self.hs.config.auth_profile_reqs: + yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) avatar_url = yield self.profile_handler.get_avatar_url( @@ -116,6 +120,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + if self.hs.config.auth_profile_reqs: + yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( From 32706f18f7e6a87c858d65f5ea338f4ccf066908 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 21 Apr 2019 01:01:40 +0100 Subject: [PATCH 02/29] changelog --- changelog.d/5083.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5083.feature diff --git a/changelog.d/5083.feature b/changelog.d/5083.feature new file mode 100644 index 000000000000..f6014c6b43e5 --- /dev/null +++ b/changelog.d/5083.feature @@ -0,0 +1 @@ +Adds auth_profile_reqs option to require access_token to GET /profile endpoints on CS API From 4838e47404bc214077a633a8d5d97002ef755d9b Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Sun, 21 Apr 2019 01:46:07 +0100 Subject: [PATCH 03/29] regenerate sample config --- docs/sample_config.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 4ada0fba0e68..5841b871807d 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -69,6 +69,10 @@ pid_file: DATADIR/homeserver.pid # #use_presence: false +# whether to require users to authenticate in order to query /profile +# endpoints via CS API. this is a workaround in advance of MSC1301 landing +#auth_profile_reqs: false + # The GC threshold parameters to pass to `gc.set_threshold`, if defined # #gc_thresholds: [700, 10, 10] From 8af9d7ef532af765a9224a4fe9102e50322d8ded Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 21 Apr 2019 01:21:08 +0100 Subject: [PATCH 04/29] don't auth profile reqs by default --- synapse/config/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 8ad42a8a6c8f..028695591f16 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -73,7 +73,7 @@ def read_config(self, config): # whether to require users to authenticate in order to query /profile # endpoints via CS API. this is a workaround in advance of MSC1301 landing - self.auth_profile_reqs = config.get("auth_profile_reqs", True) + self.auth_profile_reqs = config.get("auth_profile_reqs", False) # whether to enable search. If disabled, new entries will not be inserted # into the search tables and they will not be indexed. Users will receive From 2fd0b8f37af85292b692d0b3a0e3a67c2e297b83 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 21 Apr 2019 07:16:52 +0100 Subject: [PATCH 05/29] also add auth_public_rooms option to require auth for CS API /publicRooms --- docs/sample_config.yaml | 5 +++++ synapse/config/server.py | 10 ++++++++++ synapse/rest/client/v1/room.py | 5 +++++ 3 files changed, 20 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 5841b871807d..6849121d1f6a 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -73,6 +73,11 @@ pid_file: DATADIR/homeserver.pid # endpoints via CS API. this is a workaround in advance of MSC1301 landing #auth_profile_reqs: false +# whether to require users to authenticate in order to query /publicRooms +# endpoints via CS API. this is a workaround in advance of +# https://github.com/matrix-org/matrix-doc/issues/612 beinig solved +#auth_public_rooms: false + # The GC threshold parameters to pass to `gc.set_threshold`, if defined # #gc_thresholds: [700, 10, 10] diff --git a/synapse/config/server.py b/synapse/config/server.py index 028695591f16..ea8424590755 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -75,6 +75,11 @@ def read_config(self, config): # endpoints via CS API. this is a workaround in advance of MSC1301 landing self.auth_profile_reqs = config.get("auth_profile_reqs", False) + # whether to require users to authenticate in order to query /publicRooms + # endpoints via CS API. this is a workaround in advance of + # https://github.com/matrix-org/matrix-doc/issues/612 beinig solved + self.auth_public_rooms = config.get("auth_public_rooms", False) + # whether to enable search. If disabled, new entries will not be inserted # into the search tables and they will not be indexed. Users will receive # errors when attempting to search for messages. @@ -326,6 +331,11 @@ def default_config(self, server_name, data_dir_path, **kwargs): # endpoints via CS API. this is a workaround in advance of MSC1301 landing #auth_profile_reqs: false + # whether to require users to authenticate in order to query /publicRooms + # endpoints via CS API. this is a workaround in advance of + # https://github.com/matrix-org/matrix-doc/issues/612 beinig solved + #auth_public_rooms: false + # The GC threshold parameters to pass to `gc.set_threshold`, if defined # #gc_thresholds: [700, 10, 10] diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index 48da4d557f78..c23bf3988719 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -301,6 +301,11 @@ def on_GET(self, request): try: yield self.auth.get_user_by_req(request, allow_guest=True) except AuthError as e: + # option to allow servers in private federations to require auth + # when accessing /publicRooms via CS API + if self.hs.config.auth_public_rooms: + raise e + # We allow people to not be authed if they're just looking at our # room list, but require auth when we proxy the request. # In both cases we call the auth function, as that has the side From d3eeb755790f096d62e6bcfe7a7ce82435b6497a Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Sun, 21 Apr 2019 07:24:50 +0100 Subject: [PATCH 06/29] typo --- docs/sample_config.yaml | 2 +- synapse/config/server.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 6849121d1f6a..3f83f3410eee 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -75,7 +75,7 @@ pid_file: DATADIR/homeserver.pid # whether to require users to authenticate in order to query /publicRooms # endpoints via CS API. this is a workaround in advance of -# https://github.com/matrix-org/matrix-doc/issues/612 beinig solved +# https://github.com/matrix-org/matrix-doc/issues/612 being solved #auth_public_rooms: false # The GC threshold parameters to pass to `gc.set_threshold`, if defined diff --git a/synapse/config/server.py b/synapse/config/server.py index ea8424590755..696e39b37f11 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -77,7 +77,7 @@ def read_config(self, config): # whether to require users to authenticate in order to query /publicRooms # endpoints via CS API. this is a workaround in advance of - # https://github.com/matrix-org/matrix-doc/issues/612 beinig solved + # https://github.com/matrix-org/matrix-doc/issues/612 being solved self.auth_public_rooms = config.get("auth_public_rooms", False) # whether to enable search. If disabled, new entries will not be inserted @@ -333,7 +333,7 @@ def default_config(self, server_name, data_dir_path, **kwargs): # whether to require users to authenticate in order to query /publicRooms # endpoints via CS API. this is a workaround in advance of - # https://github.com/matrix-org/matrix-doc/issues/612 beinig solved + # https://github.com/matrix-org/matrix-doc/issues/612 being solved #auth_public_rooms: false # The GC threshold parameters to pass to `gc.set_threshold`, if defined From a60274c700834141b1d71b874cbcf867d98759fb Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 23 Apr 2019 17:03:30 +0100 Subject: [PATCH 07/29] Incorporate review --- synapse/config/server.py | 35 +++++++++++++++----------- synapse/federation/transport/server.py | 4 +++ synapse/rest/client/v1/profile.py | 6 ++--- synapse/rest/client/v1/room.py | 9 ++++--- 4 files changed, 32 insertions(+), 22 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 696e39b37f11..a1513038d697 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -71,14 +71,16 @@ def read_config(self, config): # master, potentially causing inconsistency. self.enable_media_repo = config.get("enable_media_repo", True) - # whether to require users to authenticate in order to query /profile - # endpoints via CS API. this is a workaround in advance of MSC1301 landing - self.auth_profile_reqs = config.get("auth_profile_reqs", False) - - # whether to require users to authenticate in order to query /publicRooms - # endpoints via CS API. this is a workaround in advance of - # https://github.com/matrix-org/matrix-doc/issues/612 being solved - self.auth_public_rooms = config.get("auth_public_rooms", False) + # Whether to require authentication to retrieve profile data (avatars, + # display names) of other users through the client API. + self.require_auth_for_profile_requests = config.get("require_auth_for_profile_requests", False) + + # If set to 'True', requires authentication to access the server's + # public rooms directory through the client API, and forbids any other + # homeserver to fetch it via federation. + self.restrict_public_rooms_to_local_users = config.get( + "restrict_public_rooms_to_local_users", False, + ) # whether to enable search. If disabled, new entries will not be inserted # into the search tables and they will not be indexed. Users will receive @@ -327,14 +329,17 @@ def default_config(self, server_name, data_dir_path, **kwargs): # #use_presence: false - # whether to require users to authenticate in order to query /profile - # endpoints via CS API. this is a workaround in advance of MSC1301 landing - #auth_profile_reqs: false + # Whether to require authentication to retrieve profile data (avatars, + # display names) of other users through the client API. Defaults to + # 'False'. + # + #require_auth_for_profile_requests: True - # whether to require users to authenticate in order to query /publicRooms - # endpoints via CS API. this is a workaround in advance of - # https://github.com/matrix-org/matrix-doc/issues/612 being solved - #auth_public_rooms: false + # If set to 'True', requires authentication to access the server's + # public rooms directory through the client API, and forbids any other + # homeserver to fetch it via federation. Defaults to 'False'. + # + #restrict_public_rooms_to_local_users: True # The GC threshold parameters to pass to `gc.set_threshold`, if defined # diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index efb6bdca488b..70f306438cec 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -98,6 +98,7 @@ def __init__(self, hs): self.server_name = hs.hostname self.store = hs.get_datastore() self.federation_domain_whitelist = hs.config.federation_domain_whitelist + self.deny_public_rooms = hs.config.restrict_public_rooms_to_local_users # A method just so we can pass 'self' as the authenticator to the Servlets @defer.inlineCallbacks @@ -133,6 +134,9 @@ def authenticate_request(self, request, content): ): raise FederationDeniedError(origin) + if "publicRooms" in request.path and self.deny_public_rooms: + raise FederationDeniedError(origin) + if not json_request["signatures"]: raise NoAuthenticationError( 401, "Missing Authorization headers", Codes.UNAUTHORIZED, diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index c7d3d43d7c74..bde687035202 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -31,7 +31,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - if self.hs.config.auth_profile_reqs: + if self.hs.config.require_auth_for_profile_requests: yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) @@ -76,7 +76,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - if self.hs.config.auth_profile_reqs: + if self.hs.config.require_auth_for_profile_requests: yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) @@ -120,7 +120,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - if self.hs.config.auth_profile_reqs: + if self.hs.config.require_auth_for_profile_requests: yield self.auth.get_user_by_req(request) user = UserID.from_string(user_id) diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py index c23bf3988719..fab04965cb08 100644 --- a/synapse/rest/client/v1/room.py +++ b/synapse/rest/client/v1/room.py @@ -301,10 +301,11 @@ def on_GET(self, request): try: yield self.auth.get_user_by_req(request, allow_guest=True) except AuthError as e: - # option to allow servers in private federations to require auth - # when accessing /publicRooms via CS API - if self.hs.config.auth_public_rooms: - raise e + # Option to allow servers to require auth when accessing + # /publicRooms via CS API. This is especially helpful in private + # federations. + if self.hs.config.restrict_public_rooms_to_local_users: + raise # We allow people to not be authed if they're just looking at our # room list, but require auth when we proxy the request. From b6d666fbe89f40586a99cd309826753d9acc80b8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 23 Apr 2019 17:04:49 +0100 Subject: [PATCH 08/29] Sample config --- docs/sample_config.yaml | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 3f83f3410eee..7f0efdca0ea6 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -69,14 +69,17 @@ pid_file: DATADIR/homeserver.pid # #use_presence: false -# whether to require users to authenticate in order to query /profile -# endpoints via CS API. this is a workaround in advance of MSC1301 landing -#auth_profile_reqs: false - -# whether to require users to authenticate in order to query /publicRooms -# endpoints via CS API. this is a workaround in advance of -# https://github.com/matrix-org/matrix-doc/issues/612 being solved -#auth_public_rooms: false +# Whether to require authentication to retrieve profile data (avatars, +# display names) of other users through the client API. Defaults to +# 'False'. +# +#require_auth_for_profile_requests: True + +# If set to 'True', requires authentication to access the server's +# public rooms directory through the client API, and forbids any other +# homeserver to fetch it via federation. Defaults to 'False'. +# +#restrict_public_rooms_to_local_users: True # The GC threshold parameters to pass to `gc.set_threshold`, if defined # From d791bfdd0f3f31a634ac5cd0d757eee6e04f248f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 23 Apr 2019 17:17:17 +0100 Subject: [PATCH 09/29] fix --- synapse/config/server.py | 4 +++- synapse/federation/transport/server.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index a1513038d697..48c3a7a13574 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -73,7 +73,9 @@ def read_config(self, config): # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. - self.require_auth_for_profile_requests = config.get("require_auth_for_profile_requests", False) + self.require_auth_for_profile_requests = config.get( + "require_auth_for_profile_requests", False, + ) # If set to 'True', requires authentication to access the server's # public rooms directory through the client API, and forbids any other diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 70f306438cec..ec64d64d62c6 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -134,7 +134,7 @@ def authenticate_request(self, request, content): ): raise FederationDeniedError(origin) - if "publicRooms" in request.path and self.deny_public_rooms: + if b"publicRooms" in request.path and self.deny_public_rooms: raise FederationDeniedError(origin) if not json_request["signatures"]: From 60fa555b98173c1f7958a75e27a6ff55adf2f92d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 10:46:05 +0100 Subject: [PATCH 10/29] Fix config --- docs/sample_config.yaml | 10 +++++----- synapse/config/server.py | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 7f0efdca0ea6..8a0be812a383 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -71,15 +71,15 @@ pid_file: DATADIR/homeserver.pid # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to -# 'False'. +# 'false'. # -#require_auth_for_profile_requests: True +#require_auth_for_profile_requests: true -# If set to 'True', requires authentication to access the server's +# If set to 'true', requires authentication to access the server's # public rooms directory through the client API, and forbids any other -# homeserver to fetch it via federation. Defaults to 'False'. +# homeserver to fetch it via federation. Defaults to 'false'. # -#restrict_public_rooms_to_local_users: True +#restrict_public_rooms_to_local_users: true # The GC threshold parameters to pass to `gc.set_threshold`, if defined # diff --git a/synapse/config/server.py b/synapse/config/server.py index 48c3a7a13574..9905d0b4188a 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -333,15 +333,15 @@ def default_config(self, server_name, data_dir_path, **kwargs): # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to - # 'False'. + # 'false'. # - #require_auth_for_profile_requests: True + #require_auth_for_profile_requests: true - # If set to 'True', requires authentication to access the server's + # If set to 'true', requires authentication to access the server's # public rooms directory through the client API, and forbids any other - # homeserver to fetch it via federation. Defaults to 'False'. + # homeserver to fetch it via federation. Defaults to 'false'. # - #restrict_public_rooms_to_local_users: True + #restrict_public_rooms_to_local_users: true # The GC threshold parameters to pass to `gc.set_threshold`, if defined # From 5b38e7f821b03a10adb018fe07d4e23bd3557b92 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 10:53:46 +0100 Subject: [PATCH 11/29] Add check to make sure we share a room with a user before allowing a profile lookup --- synapse/handlers/profile.py | 35 ++++++++++++++++++++++++++++--- synapse/rest/client/v1/profile.py | 17 ++++++++------- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index a65c98ff5c5a..e062f80ce4b1 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -51,8 +51,11 @@ def __init__(self, hs): self.user_directory_handler = hs.get_user_directory_handler() @defer.inlineCallbacks - def get_profile(self, user_id): + def get_profile(self, user_id, requester=None): + yield self.check_profile_query_allowed(user_id, requester) + target_user = UserID.from_string(user_id) + if self.hs.is_mine(target_user): try: displayname = yield self.store.get_profile_displayname( @@ -115,7 +118,9 @@ def get_profile_from_cache(self, user_id): defer.returnValue(profile or {}) @defer.inlineCallbacks - def get_displayname(self, target_user): + def get_displayname(self, target_user, requester=None): + yield self.check_profile_query_allowed(target_user, requester) + if self.hs.is_mine(target_user): try: displayname = yield self.store.get_profile_displayname( @@ -177,7 +182,9 @@ def set_displayname(self, target_user, requester, new_displayname, by_admin=Fals yield self._update_join_states(requester, target_user) @defer.inlineCallbacks - def get_avatar_url(self, target_user): + def get_avatar_url(self, target_user, requester=None): + yield self.check_profile_query_allowed(target_user, requester) + if self.hs.is_mine(target_user): try: avatar_url = yield self.store.get_profile_avatar_url( @@ -283,6 +290,28 @@ def _update_join_states(self, requester, target_user): room_id, str(e) ) + @defer.inlineCallbacks + def check_profile_query_allowed(self, target_user, requester=None): + # Implementation of MSC1301: don't allow looking up profiles if the + # requester isn't in the same room as the target. We expect requester to + # be None when this function is called outside of a profile query, e.g. + # when building a membership event. In this case, we must allow the + # lookup. + if self.hs.config.require_auth_for_profile_requests and requester: + try: + requester_rooms = yield self.store.get_rooms_for_user(requester) + target_user_rooms = yield self.store.get_rooms_for_user(target_user) + except StoreError as e: + if e.code == 404: + # This likely means that one of the users doesn't exist, + # so we act as if we couldn't find the profile. + raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) + raise + # Check if the length of the intersection between the room lists + # for both users is 0. + if not len(requester_rooms.intersection(target_user_rooms)): + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) + class MasterProfileHandler(BaseProfileHandler): PROFILE_UPDATE_MS = 60 * 1000 diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index bde687035202..b4d3e4cd502f 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -32,11 +32,12 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): if self.hs.config.require_auth_for_profile_requests: - yield self.auth.get_user_by_req(request) + requester = yield self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, + user, requester=requester, ) ret = {} @@ -77,11 +78,12 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): if self.hs.config.require_auth_for_profile_requests: - yield self.auth.get_user_by_req(request) + requester = yield self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) avatar_url = yield self.profile_handler.get_avatar_url( - user, + user, requester=requester, ) ret = {} @@ -121,14 +123,15 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): if self.hs.config.require_auth_for_profile_requests: - yield self.auth.get_user_by_req(request) + requester = yield self.auth.get_user_by_req(request) + user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, + user, requester=requester, ) avatar_url = yield self.profile_handler.get_avatar_url( - user, + user, requester=requester, ) ret = {} From a7779817b5baa723a07fc2b680691caebef16e10 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 13:34:16 +0100 Subject: [PATCH 12/29] test fix --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index ef543890f9c2..95b124d94b3d 100644 --- a/tox.ini +++ b/tox.ini @@ -24,6 +24,7 @@ deps = pip>=10 setenv = + PIP_USE_PEP517 = false PYTHONDONTWRITEBYTECODE = no_byte_code COVERAGE_PROCESS_START = {toxinidir}/.coveragerc From 3a42a38bb0621a8d48ce22564f5ed16877b91987 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 13:45:30 +0100 Subject: [PATCH 13/29] fix --- synapse/handlers/profile.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index e062f80ce4b1..a43d28caedf8 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -301,16 +301,17 @@ def check_profile_query_allowed(self, target_user, requester=None): try: requester_rooms = yield self.store.get_rooms_for_user(requester) target_user_rooms = yield self.store.get_rooms_for_user(target_user) + + # Check if the length of the intersection between the room lists + # for both users is 0. + if not len(requester_rooms.intersection(target_user_rooms)): + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) except StoreError as e: if e.code == 404: # This likely means that one of the users doesn't exist, # so we act as if we couldn't find the profile. raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) raise - # Check if the length of the intersection between the room lists - # for both users is 0. - if not len(requester_rooms.intersection(target_user_rooms)): - raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) class MasterProfileHandler(BaseProfileHandler): From 9b101c617f602df53733e229af1502ed916422a7 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 14:14:43 +0100 Subject: [PATCH 14/29] fix --- synapse/rest/client/v1/profile.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index b4d3e4cd502f..79264115c97a 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -31,6 +31,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + requester = None + if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) @@ -77,6 +79,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + requester = None + if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) @@ -122,6 +126,8 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): + requester = None + if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) From 8fa6337baa80e53637183c92f3d110689b5b1518 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 16:51:35 +0100 Subject: [PATCH 15/29] unfix --- tox.ini | 1 - 1 file changed, 1 deletion(-) diff --git a/tox.ini b/tox.ini index 95b124d94b3d..ef543890f9c2 100644 --- a/tox.ini +++ b/tox.ini @@ -24,7 +24,6 @@ deps = pip>=10 setenv = - PIP_USE_PEP517 = false PYTHONDONTWRITEBYTECODE = no_byte_code COVERAGE_PROCESS_START = {toxinidir}/.coveragerc From f3907afe59d19ea5cacd931574632161c27ccbb1 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 25 Apr 2019 16:53:15 +0100 Subject: [PATCH 16/29] fix --- changelog.d/5083.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/5083.feature b/changelog.d/5083.feature index f6014c6b43e5..55d114b3fe7e 100644 --- a/changelog.d/5083.feature +++ b/changelog.d/5083.feature @@ -1 +1 @@ -Adds auth_profile_reqs option to require access_token to GET /profile endpoints on CS API +Add an configuration option to require authentication on /publicRooms and /profile endpoints. From bb99538d4fb5e44defb80967a2e7a61ee8951e62 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 25 Apr 2019 15:11:27 +0100 Subject: [PATCH 17/29] Merge pull request #5098 from matrix-org/rav/fix_pep_517 Workarounds for pep-517 errors --- README.rst | 4 ++-- changelog.d/5098.misc | 1 + tox.ini | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changelog.d/5098.misc diff --git a/README.rst b/README.rst index 24afb93d7d7c..5409f0c563ef 100644 --- a/README.rst +++ b/README.rst @@ -173,7 +173,7 @@ Synapse offers two database engines: * `PostgreSQL `_ By default Synapse uses SQLite in and doing so trades performance for convenience. -SQLite is only recommended in Synapse for testing purposes or for servers with +SQLite is only recommended in Synapse for testing purposes or for servers with light workloads. Almost all installations should opt to use PostreSQL. Advantages include: @@ -272,7 +272,7 @@ to install using pip and a virtualenv:: virtualenv -p python3 env source env/bin/activate - python -m pip install -e .[all] + python -m pip install --no-pep-517 -e .[all] This will run a process of downloading and installing all the needed dependencies into a virtual env. diff --git a/changelog.d/5098.misc b/changelog.d/5098.misc new file mode 100644 index 000000000000..9cd83bf226d7 --- /dev/null +++ b/changelog.d/5098.misc @@ -0,0 +1 @@ +Add workarounds for pep-517 install errors. diff --git a/tox.ini b/tox.ini index ef543890f9c2..d0e519ce4606 100644 --- a/tox.ini +++ b/tox.ini @@ -24,6 +24,11 @@ deps = pip>=10 setenv = + # we have a pyproject.toml, but don't want pip to use it for building. + # (otherwise we get an error about 'editable mode is not supported for + # pyproject.toml-style projects'). + PIP_USE_PEP517 = false + PYTHONDONTWRITEBYTECODE = no_byte_code COVERAGE_PROCESS_START = {toxinidir}/.coveragerc From 99a9f58e97c71719636a0183f3b785d80aa5032e Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 26 Apr 2019 18:46:26 +0100 Subject: [PATCH 18/29] Fix types and add some doc --- synapse/handlers/profile.py | 28 ++++++++++++++++++++++------ synapse/rest/client/v1/profile.py | 8 ++++---- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index a43d28caedf8..9c57c098e492 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -51,9 +51,7 @@ def __init__(self, hs): self.user_directory_handler = hs.get_user_directory_handler() @defer.inlineCallbacks - def get_profile(self, user_id, requester=None): - yield self.check_profile_query_allowed(user_id, requester) - + def get_profile(self, user_id): target_user = UserID.from_string(user_id) if self.hs.is_mine(target_user): @@ -292,6 +290,20 @@ def _update_join_states(self, requester, target_user): @defer.inlineCallbacks def check_profile_query_allowed(self, target_user, requester=None): + """Checks whether a profile query is allowed. If the + 'require_auth_for_profile_requests' config flag is set to True and a + 'requester' is provided, the query is only allowed if the two users + share a room. + + Args: + target_user (UserID): The owner of the queried profile. + requester (None|UserID): The user querying for the profile. + + Raises: + SynapseError(403): The two users share no room, or ne user couldn't + be found to be in any room the server is in, and therefore the query + is denied. + """ # Implementation of MSC1301: don't allow looking up profiles if the # requester isn't in the same room as the target. We expect requester to # be None when this function is called outside of a profile query, e.g. @@ -299,8 +311,12 @@ def check_profile_query_allowed(self, target_user, requester=None): # lookup. if self.hs.config.require_auth_for_profile_requests and requester: try: - requester_rooms = yield self.store.get_rooms_for_user(requester) - target_user_rooms = yield self.store.get_rooms_for_user(target_user) + requester_rooms = yield self.store.get_rooms_for_user( + requester.to_string() + ) + target_user_rooms = yield self.store.get_rooms_for_user( + target_user.to_string(), + ) # Check if the length of the intersection between the room lists # for both users is 0. @@ -310,7 +326,7 @@ def check_profile_query_allowed(self, target_user, requester=None): if e.code == 404: # This likely means that one of the users doesn't exist, # so we act as if we couldn't find the profile. - raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) raise diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index 79264115c97a..b231acfd0b23 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -39,7 +39,7 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester, + user, requester=requester.user, ) ret = {} @@ -87,7 +87,7 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester, + user, requester=requester.user, ) ret = {} @@ -134,10 +134,10 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester, + user, requester=requester.user, ) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester, + user, requester=requester.user, ) ret = {} From 44314ba74143e808b78ebbfd6a779ea0d6446cec Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 26 Apr 2019 18:47:12 +0100 Subject: [PATCH 19/29] Add test suites --- tests/rest/client/v1/test_profile.py | 73 +++++++++++++++++++++++++++- tests/rest/client/v1/test_rooms.py | 32 ++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/v1/test_profile.py b/tests/rest/client/v1/test_profile.py index 1eab9c3bdba2..b5829c39a0a1 100644 --- a/tests/rest/client/v1/test_profile.py +++ b/tests/rest/client/v1/test_profile.py @@ -20,7 +20,7 @@ import synapse.types from synapse.api.errors import AuthError, SynapseError -from synapse.rest.client.v1 import profile +from synapse.rest.client.v1 import admin, login, profile, room from tests import unittest @@ -155,3 +155,74 @@ def test_set_my_avatar(self): self.assertEquals(mocked_set.call_args[0][0].localpart, "1234ABCD") self.assertEquals(mocked_set.call_args[0][1].user.localpart, "1234ABCD") self.assertEquals(mocked_set.call_args[0][2], "http://my.server/pic.gif") + + +class ProfilesRestrictedTestCase(unittest.HomeserverTestCase): + + servlets = [ + admin.register_servlets, + login.register_servlets, + profile.register_servlets, + room.register_servlets, + ] + + def make_homeserver(self, reactor, clock): + + config = self.default_config() + config.require_auth_for_profile_requests = True + self.hs = self.setup_test_homeserver(config=config) + + return self.hs + + def prepare(self, reactor, clock, hs): + # User owning the requested profile. + self.owner = self.register_user("owner", "pass") + self.owner_tok = self.login("owner", "pass") + self.profile_url = "/profile/%s" % (self.owner) + + # User requesting the profile. + self.requester = self.register_user("requester", "pass") + self.requester_tok = self.login("requester", "pass") + + self.room_id = self.helper.create_room_as(self.owner, tok=self.owner_tok) + + def test_no_auth(self): + self.try_fetch_profile(401) + + def test_not_in_shared_room(self): + self.ensure_requester_left_room() + + self.try_fetch_profile(403, self.requester_tok) + + def test_in_shared_room(self): + self.ensure_requester_left_room() + + self.helper.join( + room=self.room_id, + user=self.requester, + tok=self.requester_tok, + ) + + self.try_fetch_profile(200, self.requester_tok) + + def try_fetch_profile(self, expected_code, access_token=None): + request, channel = self.make_request( + "GET", + self.profile_url, + access_token=access_token, + ) + self.render(request) + self.assertEqual(channel.code, expected_code, channel.result) + + def ensure_requester_left_room(self): + try: + self.helper.leave( + room=self.room_id, + user=self.requester, + tok=self.requester_tok, + ) + except AssertionError: + # We don't care whether the leave request didn't return a 200 (e.g. + # if the user isn't already in the room), because we only want to + # make sure the user isn't in the room. + pass diff --git a/tests/rest/client/v1/test_rooms.py b/tests/rest/client/v1/test_rooms.py index 015c1442485c..cbe6bfbf8025 100644 --- a/tests/rest/client/v1/test_rooms.py +++ b/tests/rest/client/v1/test_rooms.py @@ -903,3 +903,35 @@ def test_include_context(self): self.assertEqual( context["profile_info"][self.other_user_id]["displayname"], "otheruser" ) + + +class PublicRoomsRestrictedTestCase(unittest.HomeserverTestCase): + + servlets = [ + admin.register_servlets, + room.register_servlets, + login.register_servlets, + ] + + def make_homeserver(self, reactor, clock): + + self.url = b"/_matrix/client/r0/publicRooms" + + config = self.default_config() + config.restrict_public_rooms_to_local_users = True + self.hs = self.setup_test_homeserver(config=config) + + return self.hs + + def test_restricted_no_auth(self): + request, channel = self.make_request("GET", self.url) + self.render(request) + self.assertEqual(channel.code, 401, channel.result) + + def test_restricted_auth(self): + self.register_user("user", "pass") + tok = self.login("user", "pass") + + request, channel = self.make_request("GET", self.url, access_token=tok) + self.render(request) + self.assertEqual(channel.code, 200, channel.result) From b051dfb726ff48aa3831659a07207adbe844497f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 26 Apr 2019 18:54:29 +0100 Subject: [PATCH 20/29] Fix require_auth_for_profile_requests = False --- synapse/rest/client/v1/profile.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index b231acfd0b23..917941bb25dc 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -39,7 +39,7 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester.user, + user, requester=requester, ) ret = {} @@ -87,7 +87,7 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester.user, + user, requester=requester, ) ret = {} @@ -129,15 +129,15 @@ def on_GET(self, request, user_id): requester = None if self.hs.config.require_auth_for_profile_requests: - requester = yield self.auth.get_user_by_req(request) + requester = yield self.auth.get_user_by_req(request).user user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester.user, + user, requester=requester, ) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester.user, + user, requester=requester, ) ret = {} From 12eec8e9f3bf696219675427259c8a8e1c64cd38 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 26 Apr 2019 19:17:39 +0100 Subject: [PATCH 21/29] fix --- synapse/rest/client/v1/profile.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index 917941bb25dc..8026f22f5daf 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -35,11 +35,12 @@ def on_GET(self, request, user_id): if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) + requester_user = requester.user user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester, + user, requester=requester_user, ) ret = {} @@ -83,11 +84,12 @@ def on_GET(self, request, user_id): if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) + requester_user = requester.user user = UserID.from_string(user_id) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester, + user, requester=requester_user, ) ret = {} @@ -129,15 +131,16 @@ def on_GET(self, request, user_id): requester = None if self.hs.config.require_auth_for_profile_requests: - requester = yield self.auth.get_user_by_req(request).user + requester = yield self.auth.get_user_by_req(request) + requester_user = requester.user user = UserID.from_string(user_id) displayname = yield self.profile_handler.get_displayname( - user, requester=requester, + user, requester=requester_user, ) avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester, + user, requester=requester_user, ) ret = {} From 53b389176ea5e2ed6cbb598c48e1bdacaf524c3f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 26 Apr 2019 20:42:18 +0100 Subject: [PATCH 22/29] fix --- synapse/rest/client/v1/profile.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index 8026f22f5daf..0913a0084250 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -31,7 +31,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - requester = None + requester_user = None if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) @@ -80,7 +80,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - requester = None + requester_user = None if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) @@ -128,7 +128,7 @@ def __init__(self, hs): @defer.inlineCallbacks def on_GET(self, request, user_id): - requester = None + requester_user = None if self.hs.config.require_auth_for_profile_requests: requester = yield self.auth.get_user_by_req(request) From e4fa719243107d797501e75651d4a3b54bbf980f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 1 May 2019 10:55:25 +0100 Subject: [PATCH 23/29] Incorporate review --- synapse/config/server.py | 4 ++- synapse/handlers/profile.py | 45 +++++++++++++++---------------- synapse/rest/client/v1/profile.py | 22 +++++++-------- 3 files changed, 34 insertions(+), 37 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 9905d0b4188a..24fdb99fb899 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -333,7 +333,9 @@ def default_config(self, server_name, data_dir_path, **kwargs): # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to - # 'false'. + # 'false'. Note that profile data is also available via the federation + # API, so this setting is of limited value if federation is enabled on + # the server. # #require_auth_for_profile_requests: true diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 9c57c098e492..4c8d74c371b4 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -116,9 +116,7 @@ def get_profile_from_cache(self, user_id): defer.returnValue(profile or {}) @defer.inlineCallbacks - def get_displayname(self, target_user, requester=None): - yield self.check_profile_query_allowed(target_user, requester) - + def get_displayname(self, target_user): if self.hs.is_mine(target_user): try: displayname = yield self.store.get_profile_displayname( @@ -180,9 +178,7 @@ def set_displayname(self, target_user, requester, new_displayname, by_admin=Fals yield self._update_join_states(requester, target_user) @defer.inlineCallbacks - def get_avatar_url(self, target_user, requester=None): - yield self.check_profile_query_allowed(target_user, requester) - + def get_avatar_url(self, target_user): if self.hs.is_mine(target_user): try: avatar_url = yield self.store.get_profile_avatar_url( @@ -309,25 +305,26 @@ def check_profile_query_allowed(self, target_user, requester=None): # be None when this function is called outside of a profile query, e.g. # when building a membership event. In this case, we must allow the # lookup. - if self.hs.config.require_auth_for_profile_requests and requester: - try: - requester_rooms = yield self.store.get_rooms_for_user( - requester.to_string() - ) - target_user_rooms = yield self.store.get_rooms_for_user( - target_user.to_string(), - ) + if not (self.hs.config.require_auth_for_profile_requests and requester): + return - # Check if the length of the intersection between the room lists - # for both users is 0. - if not len(requester_rooms.intersection(target_user_rooms)): - raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) - except StoreError as e: - if e.code == 404: - # This likely means that one of the users doesn't exist, - # so we act as if we couldn't find the profile. - raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) - raise + try: + requester_rooms = yield self.store.get_rooms_for_user( + requester.to_string() + ) + target_user_rooms = yield self.store.get_rooms_for_user( + target_user.to_string(), + ) + + # Check if the room lists have no elements in common. + if requester_rooms.isdisjoint(target_user_rooms): + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) + except StoreError as e: + if e.code == 404: + # This likely means that one of the users doesn't exist, + # so we act as if we couldn't find the profile. + raise SynapseError(403, "Profile isn't available", Codes.FORBIDDEN) + raise class MasterProfileHandler(BaseProfileHandler): diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index 0913a0084250..efafa45347ca 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -39,9 +39,9 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) - displayname = yield self.profile_handler.get_displayname( - user, requester=requester_user, - ) + yield self.profile_handler.check_profile_query_allowed(user, requester_user) + + displayname = yield self.profile_handler.get_displayname(user) ret = {} if displayname is not None: @@ -88,9 +88,9 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) - avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester_user, - ) + yield self.profile_handler.check_profile_query_allowed(user, requester_user) + + avatar_url = yield self.profile_handler.get_avatar_url(user, requester_user) ret = {} if avatar_url is not None: @@ -136,12 +136,10 @@ def on_GET(self, request, user_id): user = UserID.from_string(user_id) - displayname = yield self.profile_handler.get_displayname( - user, requester=requester_user, - ) - avatar_url = yield self.profile_handler.get_avatar_url( - user, requester=requester_user, - ) + yield self.profile_handler.check_profile_query_allowed(user, requester_user) + + displayname = yield self.profile_handler.get_displayname(user) + avatar_url = yield self.profile_handler.get_avatar_url(user) ret = {} if displayname is not None: From 2c960bcaf93c2603a723d5bcf85f5a8f9c24878d Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 1 May 2019 10:55:35 +0100 Subject: [PATCH 24/29] Fix test case --- tests/rest/client/v1/test_profile.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rest/client/v1/test_profile.py b/tests/rest/client/v1/test_profile.py index b5829c39a0a1..ea7d71a46287 100644 --- a/tests/rest/client/v1/test_profile.py +++ b/tests/rest/client/v1/test_profile.py @@ -42,6 +42,7 @@ def setUp(self): "set_displayname", "get_avatar_url", "set_avatar_url", + "check_profile_query_allowed", ] ) From 829f2a1c1446781d1c14dd4c4e8191cf9a591e17 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 1 May 2019 10:59:41 +0100 Subject: [PATCH 25/29] Update sample config --- docs/sample_config.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 7bdaf097c0a3..f1e1baa4ebd5 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -71,7 +71,9 @@ pid_file: DATADIR/homeserver.pid # Whether to require authentication to retrieve profile data (avatars, # display names) of other users through the client API. Defaults to -# 'false'. +# 'false'. Note that profile data is also available via the federation +# API, so this setting is of limited value if federation is enabled on +# the server. # #require_auth_for_profile_requests: true From e2d210599c6c84ca913526bba4d98263321e61e9 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 1 May 2019 11:10:34 +0100 Subject: [PATCH 26/29] Fix function call --- synapse/rest/client/v1/profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/rest/client/v1/profile.py b/synapse/rest/client/v1/profile.py index efafa45347ca..eac1966c5eb6 100644 --- a/synapse/rest/client/v1/profile.py +++ b/synapse/rest/client/v1/profile.py @@ -90,7 +90,7 @@ def on_GET(self, request, user_id): yield self.profile_handler.check_profile_query_allowed(user, requester_user) - avatar_url = yield self.profile_handler.get_avatar_url(user, requester_user) + avatar_url = yield self.profile_handler.get_avatar_url(user) ret = {} if avatar_url is not None: From 9121afcd345a1008a142783c7835c27dd6c00c59 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 1 May 2019 11:15:38 +0100 Subject: [PATCH 27/29] Test every profile endpoint --- tests/rest/client/v1/test_profile.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/v1/test_profile.py b/tests/rest/client/v1/test_profile.py index ea7d71a46287..5d13de11e686 100644 --- a/tests/rest/client/v1/test_profile.py +++ b/tests/rest/client/v1/test_profile.py @@ -193,7 +193,7 @@ def test_no_auth(self): def test_not_in_shared_room(self): self.ensure_requester_left_room() - self.try_fetch_profile(403, self.requester_tok) + self.try_fetch_profile(403, access_token=self.requester_tok) def test_in_shared_room(self): self.ensure_requester_left_room() @@ -207,9 +207,27 @@ def test_in_shared_room(self): self.try_fetch_profile(200, self.requester_tok) def try_fetch_profile(self, expected_code, access_token=None): + self.request_profile( + expected_code, + access_token=access_token + ) + + self.request_profile( + expected_code, + url_suffix="/displayname", + access_token=access_token, + ) + + self.request_profile( + expected_code, + url_suffix="/avatar_url", + access_token=access_token, + ) + + def request_profile(self, expected_code, url_suffix="", access_token=None): request, channel = self.make_request( "GET", - self.profile_url, + self.profile_url + url_suffix, access_token=access_token, ) self.render(request) From dc021a564cd66d8cc7cd3312b74cce07926af01c Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Wed, 8 May 2019 17:03:53 +0100 Subject: [PATCH 28/29] Update synapse/handlers/profile.py Co-Authored-By: babolivier --- synapse/handlers/profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index 4c8d74c371b4..91fc718ff833 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -305,7 +305,7 @@ def check_profile_query_allowed(self, target_user, requester=None): # be None when this function is called outside of a profile query, e.g. # when building a membership event. In this case, we must allow the # lookup. - if not (self.hs.config.require_auth_for_profile_requests and requester): + if not self.hs.config.require_auth_for_profile_requests or not requester: return try: From 5087bc6f05ca75ba95e7a5d3468034768796241f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Wed, 8 May 2019 17:20:44 +0100 Subject: [PATCH 29/29] Move check to servlet --- synapse/federation/transport/server.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/synapse/federation/transport/server.py b/synapse/federation/transport/server.py index 7097a769fbce..9030eb18c544 100644 --- a/synapse/federation/transport/server.py +++ b/synapse/federation/transport/server.py @@ -98,7 +98,6 @@ def __init__(self, hs): self.server_name = hs.hostname self.store = hs.get_datastore() self.federation_domain_whitelist = hs.config.federation_domain_whitelist - self.deny_public_rooms = hs.config.restrict_public_rooms_to_local_users # A method just so we can pass 'self' as the authenticator to the Servlets @defer.inlineCallbacks @@ -134,9 +133,6 @@ def authenticate_request(self, request, content): ): raise FederationDeniedError(origin) - if b"publicRooms" in request.path and self.deny_public_rooms: - raise FederationDeniedError(origin) - if not json_request["signatures"]: raise NoAuthenticationError( 401, "Missing Authorization headers", Codes.UNAUTHORIZED, @@ -720,8 +716,17 @@ class PublicRoomList(BaseFederationServlet): PATH = "/publicRooms" + def __init__(self, handler, authenticator, ratelimiter, server_name, deny_access): + super(PublicRoomList, self).__init__( + handler, authenticator, ratelimiter, server_name, + ) + self.deny_access = deny_access + @defer.inlineCallbacks def on_GET(self, origin, content, query): + if self.deny_access: + raise FederationDeniedError(origin) + limit = parse_integer_from_args(query, "limit", 0) since_token = parse_string_from_args(query, "since", None) include_all_networks = parse_boolean_from_args( @@ -1421,6 +1426,7 @@ def register_servlets(hs, resource, authenticator, ratelimiter, servlet_groups=N authenticator=authenticator, ratelimiter=ratelimiter, server_name=hs.hostname, + deny_access=hs.config.restrict_public_rooms_to_local_users, ).register(resource) if "group_server" in servlet_groups: