From 965a84ce7027dfbbc6b962c9d4b61d2559b413d9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Mon, 24 Feb 2020 14:42:06 -0500 Subject: [PATCH 1/6] Add typing information to delete methods. --- synapse/handlers/directory.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 0b23ca919ade..daf047c569c2 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -147,15 +147,15 @@ def create_association( yield self._create_association(room_alias, room_id, servers, creator=user_id) @defer.inlineCallbacks - def delete_association(self, requester, room_alias): + def delete_association(self, requester: Requester, room_alias: RoomAlias): """Remove an alias from the directory (this is only meant for human users; AS users should call delete_appservice_association) Args: - requester (Requester): - room_alias (RoomAlias): + requester + room_alias Returns: Deferred[unicode]: room id that the alias used to point to @@ -192,7 +192,7 @@ def delete_association(self, requester, room_alias): try: yield self._update_canonical_alias( - requester, requester.user.to_string(), room_id, room_alias + requester, user_id, room_id, room_alias ) except AuthError as e: logger.info("Failed to update alias events: %s", e) @@ -210,7 +210,7 @@ def delete_appservice_association(self, service, room_alias): yield self._delete_association(room_alias) @defer.inlineCallbacks - def _delete_association(self, room_alias): + def _delete_association(self, room_alias: RoomAlias): if not self.hs.is_mine(room_alias): raise SynapseError(400, "Room alias must be local") @@ -284,7 +284,9 @@ def on_directory_query(self, args): ) @defer.inlineCallbacks - def _update_canonical_alias(self, requester, user_id, room_id, room_alias): + def _update_canonical_alias( + self, requester: Requester, user_id: str, room_id: str, room_alias: RoomAlias + ): """ Send an updated canonical alias event if the removed alias was set as the canonical alias or listed in the alt_aliases field. @@ -339,7 +341,7 @@ def get_association_from_room_alias(self, room_alias): result = yield as_handler.query_room_alias_exists(room_alias) return result - def can_modify_alias(self, alias, user_id=None): + def can_modify_alias(self, alias: RoomAlias, user_id: str = None): # Any application service "interested" in an alias they are regexing on # can modify the alias. # Users can only modify the alias if ALL the interested services have @@ -360,7 +362,7 @@ def can_modify_alias(self, alias, user_id=None): return defer.succeed(True) @defer.inlineCallbacks - def _user_can_delete_alias(self, alias, user_id): + def _user_can_delete_alias(self, alias: RoomAlias, user_id: str): creator = yield self.store.get_room_alias_creator(alias.to_string()) if creator is not None and creator == user_id: From 4b699578a16a773f4b70c3daa4c5788e21146d9c Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 25 Feb 2020 08:14:48 -0500 Subject: [PATCH 2/6] Augument delete alias behavior to allow for users with sufficient power level. --- synapse/handlers/directory.py | 41 +++++++++-- tests/handlers/test_directory.py | 119 +++++++++++++++++++++++++------ 2 files changed, 135 insertions(+), 25 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index daf047c569c2..7695e53d28ab 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -21,6 +21,7 @@ from twisted.internet import defer +from synapse import event_auth from synapse.api.constants import MAX_ALIAS_LENGTH, EventTypes from synapse.api.errors import ( AuthError, @@ -191,9 +192,7 @@ def delete_association(self, requester: Requester, room_alias: RoomAlias): room_id = yield self._delete_association(room_alias) try: - yield self._update_canonical_alias( - requester, user_id, room_id, room_alias - ) + yield self._update_canonical_alias(requester, user_id, room_id, room_alias) except AuthError as e: logger.info("Failed to update alias events: %s", e) @@ -363,13 +362,47 @@ def can_modify_alias(self, alias: RoomAlias, user_id: str = None): @defer.inlineCallbacks def _user_can_delete_alias(self, alias: RoomAlias, user_id: str): + """Determine with a user can delete an alias. + + One of the following must be true: + + 1. The user created the alias. + 2. The user is a server administrator. + 3. The user has a power-level sufficient to send a canonical alias event + for the current room. + + """ creator = yield self.store.get_room_alias_creator(alias.to_string()) if creator is not None and creator == user_id: return True is_admin = yield self.auth.is_server_admin(UserID.from_string(user_id)) - return is_admin + if is_admin: + return True + + # Resolve the alias to the corresponding room. + room_mapping = yield self.get_association(alias) + room_id = room_mapping["room_id"] + if not room_id: + return False + + # Check if the user has sufficient power-level to send a canonical alias + # event. + power_level_event = yield self.state.get_current_state( + room_id, EventTypes.PowerLevels, "" + ) + + auth_events = {} + if power_level_event: + auth_events[(EventTypes.PowerLevels, "")] = power_level_event + + send_level = event_auth.get_send_level( + EventTypes.CanonicalAlias, "", power_level_event + ) + user_level = event_auth.get_user_power_level(user_id, auth_events) + + return user_level >= send_level @defer.inlineCallbacks def edit_published_room_list(self, requester, room_id, visibility): diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index 27b916aed43f..d7b8443bc18d 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -18,6 +18,7 @@ from twisted.internet import defer +import synapse import synapse.api.errors from synapse.api.constants import EventTypes from synapse.config.room_directory import RoomDirectoryConfig @@ -87,50 +88,126 @@ def test_get_remote_association(self): ignore_backoff=True, ) - def test_delete_alias_not_allowed(self): - room_id = "!8765qwer:test" + def test_incoming_fed_query(self): self.get_success( - self.store.create_room_alias_association(self.my_room, room_id, ["test"]) + self.store.create_room_alias_association( + self.your_room, "!8765asdf:test", ["test"] + ) + ) + + response = self.get_success( + self.handler.on_directory_query({"room_alias": "#your-room:test"}) + ) + + self.assertEquals({"room_id": "!8765asdf:test", "servers": ["test"]}, response) + + +class TestDeleteAlias(unittest.HomeserverTestCase): + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + room.register_servlets, + directory.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + self.handler = hs.get_handlers().directory_handler + self.state_handler = hs.get_state_handler() + + # Create user + self.admin_user = self.register_user("admin", "pass", admin=True) + self.admin_user_tok = self.login("admin", "pass") + + # Create a test room + self.room_id = self.helper.create_room_as( + self.admin_user, tok=self.admin_user_tok ) + self.test_alias = "#test:test" + self.room_alias = RoomAlias.from_string(self.test_alias) + + def _create_alias(self, user): + # Create a new alias to this room. + self.get_success( + self.store.create_room_alias_association( + self.room_alias, self.room_id, ["test"], user + ) + ) + + def test_delete_alias_not_allowed(self): + """A user that doesn't meet the expected guidelines cannot delete an alias.""" + self._create_alias(self.admin_user) self.get_failure( self.handler.delete_association( - create_requester("@user:test"), self.my_room + create_requester("@user:test"), self.room_alias ), synapse.api.errors.AuthError, ) - def test_delete_alias(self): - room_id = "!8765qwer:test" + def test_delete_alias_creator(self): + """An alias creator can delete the alias.""" + # Create an alias from a different user. user_id = "@user:test" - self.get_success( - self.store.create_room_alias_association( - self.my_room, room_id, ["test"], user_id - ) - ) + self._create_alias(user_id) + # Deleting the alias completes successfully. result = self.get_success( - self.handler.delete_association(create_requester(user_id), self.my_room) + self.handler.delete_association(create_requester(user_id), self.room_alias) ) - self.assertEquals(room_id, result) + self.assertEquals(self.room_id, result) # The alias should not be found. self.get_failure( - self.handler.get_association(self.my_room), synapse.api.errors.SynapseError + self.handler.get_association(self.room_alias), + synapse.api.errors.SynapseError, ) - def test_incoming_fed_query(self): - self.get_success( - self.store.create_room_alias_association( - self.your_room, "!8765asdf:test", ["test"] + def test_delete_alias_admin(self): + """A server admin can delete an alias.""" + # Create an alias from a different user. + user_id = "@user:test" + self._create_alias(user_id) + + # Deleting the alias as the admin completes successfully. + result = self.get_success( + self.handler.delete_association( + create_requester(self.admin_user), self.room_alias ) ) + self.assertEquals(self.room_id, result) - response = self.get_success( - self.handler.on_directory_query({"room_alias": "#your-room:test"}) + # The alias should not be found. + self.get_failure( + self.handler.get_association(self.room_alias), + synapse.api.errors.SynapseError, ) - self.assertEquals({"room_id": "!8765asdf:test", "servers": ["test"]}, response) + def test_delete_alias_sufficient_power(self): + """A user with a sufficient power level should be able to delete an alias.""" + self._create_alias(self.admin_user) + + # A user with sufficient power levels should be able to delete an alias. + other_user_id = "@other:test" + self.helper.send_state( + self.room_id, + "m.room.power_levels", + {"users": {other_user_id: 100}}, + tok=self.admin_user_tok, + ) + + result = self.get_success( + self.handler.delete_association( + create_requester(other_user_id), self.room_alias + ) + ) + self.assertEquals(self.room_id, result) + + # The alias should not be found. + self.get_failure( + self.handler.get_association(self.room_alias), + synapse.api.errors.SynapseError, + ) class CanonicalAliasTestCase(unittest.HomeserverTestCase): From b92b8d3adf970712cceb6af4e7e6c8d61c165643 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 25 Feb 2020 08:53:52 -0500 Subject: [PATCH 3/6] Type the rest of the directory handler. --- synapse/handlers/directory.py | 57 ++++++++++++++++++++++------------- tox.ini | 1 + 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 7695e53d28ab..38764c7d2888 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -17,7 +17,7 @@ import collections import logging import string -from typing import List +from typing import Iterable, List, Optional from twisted.internet import defer @@ -31,6 +31,7 @@ StoreError, SynapseError, ) +from synapse.appservice import ApplicationService from synapse.types import Requester, RoomAlias, UserID, get_domain_from_id from ._base import BaseHandler @@ -58,7 +59,13 @@ def __init__(self, hs): self.spam_checker = hs.get_spam_checker() @defer.inlineCallbacks - def _create_association(self, room_alias, room_id, servers=None, creator=None): + def _create_association( + self, + room_alias: RoomAlias, + room_id: str, + servers: Optional[Iterable[str]] = None, + creator: Optional[str] = None, + ): # general association creation for both human users and app services for wchar in string.whitespace: @@ -84,17 +91,21 @@ def _create_association(self, room_alias, room_id, servers=None, creator=None): @defer.inlineCallbacks def create_association( - self, requester, room_alias, room_id, servers=None, check_membership=True, + self, + requester: Requester, + room_alias: RoomAlias, + room_id: str, + servers: Optional[List[str]] = None, + check_membership: bool = True, ): """Attempt to create a new alias Args: - requester (Requester) - room_alias (RoomAlias) - room_id (str) - servers (list[str]|None): List of servers that others servers - should try and join via - check_membership (bool): Whether to check if the user is in the room + requester + room_alias + room_id + servers: Iterable of servers that others servers should try and join via + check_membership: Whether to check if the user is in the room before the alias can be set (if the server's config requires it). Returns: @@ -199,7 +210,9 @@ def delete_association(self, requester: Requester, room_alias: RoomAlias): return room_id @defer.inlineCallbacks - def delete_appservice_association(self, service, room_alias): + def delete_appservice_association( + self, service: ApplicationService, room_alias: RoomAlias + ): if not service.is_interested_in_alias(room_alias.to_string()): raise SynapseError( 400, @@ -218,7 +231,7 @@ def _delete_association(self, room_alias: RoomAlias): return room_id @defer.inlineCallbacks - def get_association(self, room_alias): + def get_association(self, room_alias: RoomAlias): room_id = None if self.hs.is_mine(room_alias): result = yield self.get_association_from_room_alias(room_alias) @@ -332,7 +345,7 @@ def _update_canonical_alias( ) @defer.inlineCallbacks - def get_association_from_room_alias(self, room_alias): + def get_association_from_room_alias(self, room_alias: RoomAlias): result = yield self.store.get_association_from_room_alias(room_alias) if not result: # Query AS to see if it exists @@ -340,7 +353,7 @@ def get_association_from_room_alias(self, room_alias): result = yield as_handler.query_room_alias_exists(room_alias) return result - def can_modify_alias(self, alias: RoomAlias, user_id: str = None): + def can_modify_alias(self, alias: RoomAlias, user_id: Optional[str] = None): # Any application service "interested" in an alias they are regexing on # can modify the alias. # Users can only modify the alias if ALL the interested services have @@ -405,12 +418,14 @@ def _user_can_delete_alias(self, alias: RoomAlias, user_id: str): return user_level >= send_level @defer.inlineCallbacks - def edit_published_room_list(self, requester, room_id, visibility): + def edit_published_room_list( + self, requester: Requester, room_id: str, visibility: str + ): """Edit the entry of the room in the published room list. requester - room_id (str) - visibility (str): "public" or "private" + room_id + visibility: "public" or "private" """ user_id = requester.user.to_string() @@ -456,16 +471,16 @@ def edit_published_room_list(self, requester, room_id, visibility): @defer.inlineCallbacks def edit_published_appservice_room_list( - self, appservice_id, network_id, room_id, visibility + self, appservice_id: str, network_id: str, room_id: str, visibility: str ): """Add or remove a room from the appservice/network specific public room list. Args: - appservice_id (str): ID of the appservice that owns the list - network_id (str): The ID of the network the list is associated with - room_id (str) - visibility (str): either "public" or "private" + appservice_id: ID of the appservice that owns the list + network_id: The ID of the network the list is associated with + room_id + visibility: either "public" or "private" """ if visibility not in ["public", "private"]: raise SynapseError(400, "Invalid visibility setting") diff --git a/tox.ini b/tox.ini index 097ebb877404..7622aa19f18d 100644 --- a/tox.ini +++ b/tox.ini @@ -185,6 +185,7 @@ commands = mypy \ synapse/federation/federation_client.py \ synapse/federation/sender \ synapse/federation/transport \ + synapse/handlers/directory.py \ synapse/handlers/presence.py \ synapse/handlers/sync.py \ synapse/handlers/ui_auth \ From 590b063888fddaf23f4bfdb19aa43263d873d547 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 25 Feb 2020 08:56:27 -0500 Subject: [PATCH 4/6] Add a changelog entry. --- changelog.d/6986.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6986.feature diff --git a/changelog.d/6986.feature b/changelog.d/6986.feature new file mode 100644 index 000000000000..16dea8bd7f84 --- /dev/null +++ b/changelog.d/6986.feature @@ -0,0 +1 @@ +Users with a power level sufficient to modify the canonical alias of a room can now delete room aliases. From 0aaf63eed459c56947c96a1ea5327062125ea983 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Wed, 26 Feb 2020 11:57:40 -0500 Subject: [PATCH 5/6] Clarify docstring. Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- synapse/handlers/directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 38764c7d2888..0728fa6b578e 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -375,7 +375,7 @@ def can_modify_alias(self, alias: RoomAlias, user_id: Optional[str] = None): @defer.inlineCallbacks def _user_can_delete_alias(self, alias: RoomAlias, user_id: str): - """Determine with a user can delete an alias. + """Determine whether a user can delete an alias. One of the following must be true: From 3a7888364a77ccdf764bf6ae796665c6784a0568 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 27 Feb 2020 08:41:04 -0500 Subject: [PATCH 6/6] Share logic between being able to list rooms and delete aliases. --- synapse/api/auth.py | 9 ++------ synapse/handlers/directory.py | 33 ++++++++++----------------- tests/handlers/test_directory.py | 39 ++++++++++++++++++-------------- 3 files changed, 36 insertions(+), 45 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 5ca18b4301a8..c1ade1333b04 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -539,7 +539,7 @@ def compute_auth_events( @defer.inlineCallbacks def check_can_change_room_list(self, room_id: str, user: UserID): - """Check if the user is allowed to edit the room's entry in the + """Determine whether the user is allowed to edit the room's entry in the published room list. Args: @@ -570,12 +570,7 @@ def check_can_change_room_list(self, room_id: str, user: UserID): ) user_level = event_auth.get_user_power_level(user_id, auth_events) - if user_level < send_level: - raise AuthError( - 403, - "This server requires you to be a moderator in the room to" - " edit its room list entry", - ) + return user_level >= send_level @staticmethod def has_access_token(request): diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 0728fa6b578e..7845ee6686a2 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -21,7 +21,6 @@ from twisted.internet import defer -from synapse import event_auth from synapse.api.constants import MAX_ALIAS_LENGTH, EventTypes from synapse.api.errors import ( AuthError, @@ -390,32 +389,16 @@ def _user_can_delete_alias(self, alias: RoomAlias, user_id: str): if creator is not None and creator == user_id: return True - is_admin = yield self.auth.is_server_admin(UserID.from_string(user_id)) - if is_admin: - return True - # Resolve the alias to the corresponding room. room_mapping = yield self.get_association(alias) room_id = room_mapping["room_id"] if not room_id: return False - # Check if the user has sufficient power-level to send a canonical alias - # event. - power_level_event = yield self.state.get_current_state( - room_id, EventTypes.PowerLevels, "" - ) - - auth_events = {} - if power_level_event: - auth_events[(EventTypes.PowerLevels, "")] = power_level_event - - send_level = event_auth.get_send_level( - EventTypes.CanonicalAlias, "", power_level_event + res = yield self.auth.check_can_change_room_list( + room_id, UserID.from_string(user_id) ) - user_level = event_auth.get_user_power_level(user_id, auth_events) - - return user_level >= send_level + return res @defer.inlineCallbacks def edit_published_room_list( @@ -450,7 +433,15 @@ def edit_published_room_list( if room is None: raise SynapseError(400, "Unknown room") - yield self.auth.check_can_change_room_list(room_id, requester.user) + can_change_room_list = yield self.auth.check_can_change_room_list( + room_id, requester.user + ) + if not can_change_room_list: + raise AuthError( + 403, + "This server requires you to be a moderator in the room to" + " edit its room list entry", + ) making_public = visibility == "public" if making_public: diff --git a/tests/handlers/test_directory.py b/tests/handlers/test_directory.py index d7b8443bc18d..f5db89940a97 100644 --- a/tests/handlers/test_directory.py +++ b/tests/handlers/test_directory.py @@ -127,6 +127,11 @@ def prepare(self, reactor, clock, hs): self.test_alias = "#test:test" self.room_alias = RoomAlias.from_string(self.test_alias) + # Create a test user. + self.test_user = self.register_user("user", "pass", admin=False) + self.test_user_tok = self.login("user", "pass") + self.helper.join(room=self.room_id, user=self.test_user, tok=self.test_user_tok) + def _create_alias(self, user): # Create a new alias to this room. self.get_success( @@ -140,36 +145,36 @@ def test_delete_alias_not_allowed(self): self._create_alias(self.admin_user) self.get_failure( self.handler.delete_association( - create_requester("@user:test"), self.room_alias + create_requester(self.test_user), self.room_alias ), synapse.api.errors.AuthError, ) def test_delete_alias_creator(self): - """An alias creator can delete the alias.""" + """An alias creator can delete their own alias.""" # Create an alias from a different user. - user_id = "@user:test" - self._create_alias(user_id) + self._create_alias(self.test_user) - # Deleting the alias completes successfully. + # Delete the user's alias. result = self.get_success( - self.handler.delete_association(create_requester(user_id), self.room_alias) + self.handler.delete_association( + create_requester(self.test_user), self.room_alias + ) ) self.assertEquals(self.room_id, result) - # The alias should not be found. + # Confirm the alias is gone. self.get_failure( self.handler.get_association(self.room_alias), synapse.api.errors.SynapseError, ) def test_delete_alias_admin(self): - """A server admin can delete an alias.""" + """A server admin can delete an alias created by another user.""" # Create an alias from a different user. - user_id = "@user:test" - self._create_alias(user_id) + self._create_alias(self.test_user) - # Deleting the alias as the admin completes successfully. + # Delete the user's alias as the admin. result = self.get_success( self.handler.delete_association( create_requester(self.admin_user), self.room_alias @@ -177,7 +182,7 @@ def test_delete_alias_admin(self): ) self.assertEquals(self.room_id, result) - # The alias should not be found. + # Confirm the alias is gone. self.get_failure( self.handler.get_association(self.room_alias), synapse.api.errors.SynapseError, @@ -187,23 +192,23 @@ def test_delete_alias_sufficient_power(self): """A user with a sufficient power level should be able to delete an alias.""" self._create_alias(self.admin_user) - # A user with sufficient power levels should be able to delete an alias. - other_user_id = "@other:test" + # Increase the user's power level. self.helper.send_state( self.room_id, "m.room.power_levels", - {"users": {other_user_id: 100}}, + {"users": {self.test_user: 100}}, tok=self.admin_user_tok, ) + # They can now delete the alias. result = self.get_success( self.handler.delete_association( - create_requester(other_user_id), self.room_alias + create_requester(self.test_user), self.room_alias ) ) self.assertEquals(self.room_id, result) - # The alias should not be found. + # Confirm the alias is gone. self.get_failure( self.handler.get_association(self.room_alias), synapse.api.errors.SynapseError,