From fea933ff1ea8e12f402e943510171e7438701151 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 14:42:54 -0700 Subject: [PATCH 01/20] move experimental feature msc3026 (busy presence) to per-user flag --- synapse/config/experimental.py | 3 --- synapse/handlers/presence.py | 18 +++++++++++------- synapse/rest/client/versions.py | 2 +- tests/handlers/test_presence.py | 16 +++++++--------- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 65996797312b..7b77352bbb0e 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -43,9 +43,6 @@ class ExperimentalConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: experimental = config.get("experimental_features") or {} - # MSC3026 (busy presence state) - self.msc3026_enabled: bool = experimental.get("msc3026_enabled", False) - # MSC2716 (importing historical messages) self.msc2716_enabled: bool = experimental.get("msc2716_enabled", False) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 4ad223357384..e841c1b1b818 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -148,8 +148,6 @@ def __init__(self, hs: "HomeServer"): self._federation_queue = PresenceFederationQueue(hs, self) - self._busy_presence_enabled = hs.config.experimental.msc3026_enabled - active_presence = self.store.take_presence_startup_info() self.user_to_current_state = {state.user_id: state for state in active_presence} @@ -422,8 +420,6 @@ def __init__(self, hs: "HomeServer"): self.send_stop_syncing, UPDATE_SYNCING_USERS_MS ) - self._busy_presence_enabled = hs.config.experimental.msc3026_enabled - hs.get_reactor().addSystemEventTrigger( "before", "shutdown", @@ -609,8 +605,12 @@ async def set_state( PresenceState.BUSY, ) + busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), "msc3026" + ) + if presence not in valid_presence or ( - presence == PresenceState.BUSY and not self._busy_presence_enabled + presence == PresenceState.BUSY and not busy_presence_enabled ): raise SynapseError(400, "Invalid presence state") @@ -1238,8 +1238,12 @@ async def set_state( PresenceState.BUSY, ) + busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), "msc3026" + ) + if presence not in valid_presence or ( - presence == PresenceState.BUSY and not self._busy_presence_enabled + presence == PresenceState.BUSY and not busy_presence_enabled ): raise SynapseError(400, "Invalid presence state") @@ -1257,7 +1261,7 @@ async def set_state( new_fields["status_msg"] = status_msg if presence == PresenceState.ONLINE or ( - presence == PresenceState.BUSY and self._busy_presence_enabled + presence == PresenceState.BUSY and busy_presence_enabled ): new_fields["last_active_ts"] = self.clock.time_msec() diff --git a/synapse/rest/client/versions.py b/synapse/rest/client/versions.py index 59aed66464e5..1606250e98b4 100644 --- a/synapse/rest/client/versions.py +++ b/synapse/rest/client/versions.py @@ -96,7 +96,7 @@ def on_GET(self, request: Request) -> Tuple[int, JsonDict]: "io.element.e2ee_forced.private": self.e2ee_forced_private, "io.element.e2ee_forced.trusted_private": self.e2ee_forced_trusted_private, # Supports the busy presence state described in MSC3026. - "org.matrix.msc3026.busy_presence": self.config.experimental.msc3026_enabled, + "org.matrix.msc3026.busy_presence": True, # Supports receiving private read receipts as per MSC2285 "org.matrix.msc2285.stable": True, # TODO: Remove when MSC2285 becomes a part of the spec # Supports filtering of /publicRooms by room type as per MSC3827 diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 19f5322317a1..20d79f995b9b 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -15,7 +15,6 @@ from typing import Optional, cast from unittest.mock import Mock, call -from parameterized import parameterized from signedjson.key import generate_signing_key from twisted.test.proto_helpers import MemoryReactor @@ -724,14 +723,6 @@ def test_set_presence_from_syncing_keeps_status(self) -> None: # our status message should be the same as it was before self.assertEqual(state.status_msg, status_msg) - @parameterized.expand([(False,), (True,)]) - @unittest.override_config( - { - "experimental_features": { - "msc3026_enabled": True, - }, - } - ) def test_set_presence_from_syncing_keeps_busy( self, test_with_workers: bool ) -> None: @@ -744,6 +735,13 @@ def test_set_presence_from_syncing_keeps_busy( user_id = "@test:server" status_msg = "I'm busy!" + # set busy state in db + self.get_success( + self.hs.get_datastores().main.set_feature_for_user( + user_id, "msc_3026", True + ) + ) + # By default, we call /sync against the main process. worker_to_sync_against = self.hs if test_with_workers: From 0d61d3d3bd8083d9fb41893f9500ac867eccb8c5 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 14:49:36 -0700 Subject: [PATCH 02/20] move msc3967 (Do not require UIA when first uploading cross signing keys) from config to per-user flag --- synapse/config/experimental.py | 3 --- synapse/rest/client/keys.py | 2 +- tests/rest/client/test_keys.py | 8 +++++++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 7b77352bbb0e..43f544c34edd 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -183,9 +183,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "msc3958_supress_edit_notifs", False ) - # MSC3967: Do not require UIA when first uploading cross signing keys - self.msc3967_enabled = experimental.get("msc3967_enabled", False) - # MSC2659: Application service ping endpoint self.msc2659_enabled = experimental.get("msc2659_enabled", False) diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 9bbab5e6241e..2f3058d3fb25 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -375,7 +375,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() body = parse_json_object_from_request(request) - if self.hs.config.experimental.msc3967_enabled: + if await self.hs.get_datastores().main.get_feature_enabled(user_id, "msc3967"): if await self.e2e_keys_handler.is_cross_signing_set_up_for_user(user_id): # If we already have a master key then cross signing is set up and we require UIA to reset await self.auth_handler.validate_user_via_ui_auth( diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 8ee548905704..713c7abec379 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -205,7 +205,6 @@ def test_device_signing_with_uia_session_timeout(self) -> None: @override_config( { - "experimental_features": {"msc3967_enabled": True}, "ui_auth": {"session_timeout": "15s"}, } ) @@ -216,6 +215,13 @@ def test_device_signing_with_msc3967(self) -> None: alice_id = self.register_user("alice", password) alice_token = self.login("alice", password, device_id=device_id) + # enable msc3967 in db + self.get_success( + self.hs.get_datastores().main.set_feature_for_user( + alice_id, "msc3967", True + ) + ) + keys1 = self.make_device_keys(alice_id, device_id) # Initial request should succeed as no existing keys are present. From 1739ce698a48e8abb11175c8c98397ce9c585b9d Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 14:56:14 -0700 Subject: [PATCH 03/20] move experimental feature msc3881 (remotely toggle push) to per-user flag --- synapse/config/experimental.py | 3 --- synapse/rest/client/pusher.py | 13 +++++++++---- synapse/rest/client/versions.py | 2 +- tests/push/test_http.py | 27 ++++++++++++++++++++++----- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 43f544c34edd..e0a84e56d356 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -124,9 +124,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: raw_msc3866_config = experimental.get("msc3866", {}) self.msc3866 = MSC3866Config(**raw_msc3866_config) - # MSC3881: Remotely toggle push notifications for another client - self.msc3881_enabled: bool = experimental.get("msc3881_enabled", False) - # MSC3882: Allow an existing session to sign in a new session self.msc3882_enabled: bool = experimental.get("msc3882_enabled", False) self.msc3882_ui_auth: bool = experimental.get("msc3882_ui_auth", True) diff --git a/synapse/rest/client/pusher.py b/synapse/rest/client/pusher.py index 1a8f5292ac5c..0cc63dc5de23 100644 --- a/synapse/rest/client/pusher.py +++ b/synapse/rest/client/pusher.py @@ -42,7 +42,6 @@ def __init__(self, hs: "HomeServer"): super().__init__() self.hs = hs self.auth = hs.get_auth() - self._msc3881_enabled = self.hs.config.experimental.msc3881_enabled async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) @@ -55,7 +54,9 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: pusher_dicts = [p.as_dict() for p in pushers] for pusher in pusher_dicts: - if self._msc3881_enabled: + if await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ): pusher["org.matrix.msc3881.enabled"] = pusher["enabled"] pusher["org.matrix.msc3881.device_id"] = pusher["device_id"] del pusher["enabled"] @@ -73,7 +74,6 @@ def __init__(self, hs: "HomeServer"): self.auth = hs.get_auth() self.notifier = hs.get_notifier() self.pusher_pool = self.hs.get_pusherpool() - self._msc3881_enabled = self.hs.config.experimental.msc3881_enabled async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: requester = await self.auth.get_user_by_req(request) @@ -113,7 +113,12 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: append = content["append"] enabled = True - if self._msc3881_enabled and "org.matrix.msc3881.enabled" in content: + if ( + await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ) + and "org.matrix.msc3881.enabled" in content + ): enabled = content["org.matrix.msc3881.enabled"] if not append: diff --git a/synapse/rest/client/versions.py b/synapse/rest/client/versions.py index 1606250e98b4..ae2101ce83e2 100644 --- a/synapse/rest/client/versions.py +++ b/synapse/rest/client/versions.py @@ -115,7 +115,7 @@ def on_GET(self, request: Request) -> Tuple[int, JsonDict]: # Adds support for login token requests as per MSC3882 "org.matrix.msc3882": self.config.experimental.msc3882_enabled, # Adds support for remotely enabling/disabling pushers, as per MSC3881 - "org.matrix.msc3881": self.config.experimental.msc3881_enabled, + "org.matrix.msc3881": True, # Adds support for filtering /messages by event relation. "org.matrix.msc3874": self.config.experimental.msc3874_enabled, # Adds support for simple HTTP rendezvous as per MSC3886 diff --git a/tests/push/test_http.py b/tests/push/test_http.py index 99cec0836b1d..67c1934eaa7f 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -819,7 +819,6 @@ def test_dont_notify_rule_overrides_message(self) -> None: self.helper.send(room, body="Hello", tok=access_token) self.assertEqual(len(self.push_attempts), 1) - @override_config({"experimental_features": {"msc3881_enabled": True}}) def test_disable(self) -> None: """Tests that disabling a pusher means it's not pushed to anymore.""" user_id, access_token = self._make_user_with_pusher("user") @@ -828,6 +827,11 @@ def test_disable(self) -> None: room = self.helper.create_room_as(user_id, tok=access_token) self.helper.join(room=room, user=other_user_id, tok=other_access_token) + # enable msc3881 per_user flag + self.get_success( + self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + ) + # Send a message and check that it generated a push. self.helper.send(room, body="Hi!", tok=other_access_token) self.assertEqual(len(self.push_attempts), 1) @@ -848,7 +852,6 @@ def test_disable(self) -> None: self.assertFalse(enabled) self.assertTrue(isinstance(enabled, bool)) - @override_config({"experimental_features": {"msc3881_enabled": True}}) def test_enable(self) -> None: """Tests that enabling a disabled pusher means it gets pushed to.""" # Create the user with the pusher already disabled. @@ -858,6 +861,11 @@ def test_enable(self) -> None: room = self.helper.create_room_as(user_id, tok=access_token) self.helper.join(room=room, user=other_user_id, tok=other_access_token) + # enable msc3881 per_user flag + self.get_success( + self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + ) + # Send a message and check that it did not generate a push. self.helper.send(room, body="Hi!", tok=other_access_token) self.assertEqual(len(self.push_attempts), 0) @@ -878,7 +886,7 @@ def test_enable(self) -> None: self.assertTrue(enabled) self.assertTrue(isinstance(enabled, bool)) - @override_config({"experimental_features": {"msc3881_enabled": True}}) + # @override_config({"experimental_features": {"msc3881_enabled": True}}) def test_null_enabled(self) -> None: """Tests that a pusher that has an 'enabled' column set to NULL (eg pushers created before the column was introduced) is considered enabled. @@ -887,6 +895,11 @@ def test_null_enabled(self) -> None: # database. user_id, access_token = self._make_user_with_pusher("user", enabled=None) # type: ignore[arg-type] + # enable msc3881 per_user flag + self.get_success( + self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + ) + channel = self.make_request("GET", "/pushers", access_token=access_token) self.assertEqual(channel.code, 200) self.assertEqual(len(channel.json_body["pushers"]), 1) @@ -922,14 +935,18 @@ def test_update_different_device_access_token_device_id(self) -> None: self.assertEqual(len(pushers), 1) self.assertEqual(pushers[0].device_id, device_id) - @override_config({"experimental_features": {"msc3881_enabled": True}}) def test_device_id(self) -> None: """Tests that a pusher created with a given device ID shows that device ID in GET /pushers requests. """ - self.register_user("user", "pass") + user = self.register_user("user", "pass") access_token = self.login("user", "pass") + # enable msc3881 per_user flag + self.get_success( + self.hs.get_datastores().main.set_feature_for_user(user, "msc3881", True) + ) + # We create the pusher with an HTTP request rather than with # _make_user_with_pusher so that we can test the device ID is correctly set when # creating a pusher via an API call. From 4aea2dee87eec58eede18edc684ad07d374dc8f2 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 15:01:12 -0700 Subject: [PATCH 04/20] move experimental feature msc2654 (unread counts) to per-user flag --- synapse/config/experimental.py | 6 --- synapse/push/bulk_push_rule_evaluator.py | 44 +++++++++++-------- synapse/rest/client/sync.py | 27 +++++++++--- .../databases/main/event_push_actions.py | 20 +++++---- .../replication/slave/storage/test_events.py | 7 ++- tests/rest/client/test_sync.py | 6 +-- 6 files changed, 65 insertions(+), 45 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index e0a84e56d356..573763f27055 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -96,12 +96,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC3720 (Account status endpoint) self.msc3720_enabled: bool = experimental.get("msc3720_enabled", False) - # MSC2654: Unread counts - # - # Note that enabling this will result in an incorrect unread count for - # previously calculated push actions. - self.msc2654_enabled: bool = experimental.get("msc2654_enabled", False) - # MSC2815 (allow room moderators to view redacted event content) self.msc2815_enabled: bool = experimental.get("msc2815_enabled", False) diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index 320084f5f58c..a2d00645f0c1 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -27,6 +27,7 @@ Union, ) +import attr from prometheus_client import Counter from synapse.api.constants import ( @@ -107,6 +108,17 @@ def _should_count_as_unread(event: EventBase, context: EventContext) -> bool: return False +@attr.s(slots=True, auto_attribs=True) +class ActionsForUser: + """ + A class to hold the actions for a given event and whether the event should + increment the unread count. + """ + + actions: Collection[Union[Mapping, str]] + count_as_unread: bool + + class BulkPushRuleEvaluator: """Calculates the outcome of push rules for an event for all users in the room at once. @@ -336,15 +348,8 @@ async def _action_for_event_by_user( # (historical messages persisted in reverse-chronological order). return - # Disable counting as unread unless the experimental configuration is - # enabled, as it can cause additional (unwanted) rows to be added to the - # event_push_actions table. - count_as_unread = False - if self.hs.config.experimental.msc2654_enabled: - count_as_unread = _should_count_as_unread(event, context) - rules_by_user = await self._get_rules_for_event(event) - actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {} + actions_by_user: Dict[str, ActionsForUser] = {} room_member_count = await self.store.get_number_joined_users_in_room( event.room_id @@ -429,17 +434,19 @@ async def _action_for_event_by_user( if not isinstance(display_name, str): display_name = None - if count_as_unread: - # Add an element for the current user if the event needs to be marked as - # unread, so that add_push_actions_to_staging iterates over it. - # If the event shouldn't be marked as unread but should notify the - # current user, it'll be added to the dict later. - actions_by_user[uid] = [] - actions = evaluator.run(rules, uid, display_name) - if "notify" in actions: - # Push rules say we should notify the user of this event - actions_by_user[uid] = actions + + # check whether unread counts are enabled for this user + unread_enabled = await self.store.get_feature_enabled(uid, "msc2654") + if unread_enabled: + count_as_unread = _should_count_as_unread(event, context) + else: + count_as_unread = False + + if "notify" in actions or count_as_unread: + # Push rules say we should notify the user of this event or the event should + # increment the unread count + actions_by_user[uid] = ActionsForUser(actions, count_as_unread) # If there aren't any actions then we can skip the rest of the # processing. @@ -467,7 +474,6 @@ async def _action_for_event_by_user( await self.store.add_push_actions_to_staging( event.event_id, actions_by_user, - count_as_unread, thread_id, ) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 03b05789456b..7ae96493cbcd 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -100,7 +100,6 @@ def __init__(self, hs: "HomeServer"): self.presence_handler = hs.get_presence_handler() self._server_notices_sender = hs.get_server_notices_sender() self._event_serializer = hs.get_event_client_serializer() - self._msc2654_enabled = hs.config.experimental.msc2654_enabled self._msc3773_enabled = hs.config.experimental.msc3773_enabled async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -261,7 +260,7 @@ async def encode_response( ) joined = await self.encode_joined( - sync_result.joined, time_now, serialize_options + sync_result.joined, time_now, serialize_options, requester ) invited = await self.encode_invited( @@ -273,7 +272,7 @@ async def encode_response( ) archived = await self.encode_archived( - sync_result.archived, time_now, serialize_options + sync_result.archived, time_now, serialize_options, requester ) logger.debug("building sync response dict") @@ -344,6 +343,7 @@ async def encode_joined( rooms: List[JoinedSyncResult], time_now: int, serialize_options: SerializeEventConfig, + requester: Requester, ) -> JsonDict: """ Encode the joined rooms in a sync result @@ -352,13 +352,18 @@ async def encode_joined( rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options + requester: The requester of the sync Returns: The joined rooms list, in our response format """ joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, time_now, joined=True, serialize_options=serialize_options + room, + time_now, + joined=True, + serialize_options=serialize_options, + requester=requester, ) return joined @@ -449,6 +454,7 @@ async def encode_archived( rooms: List[ArchivedSyncResult], time_now: int, serialize_options: SerializeEventConfig, + requester: Requester, ) -> JsonDict: """ Encode the archived rooms in a sync result @@ -457,13 +463,18 @@ async def encode_archived( rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options + requester: the requester of the sync Returns: The archived rooms list, in our response format """ joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, time_now, joined=False, serialize_options=serialize_options + room, + time_now, + joined=False, + serialize_options=serialize_options, + requester=requester, ) return joined @@ -474,6 +485,7 @@ async def encode_room( time_now: int, joined: bool, serialize_options: SerializeEventConfig, + requester: Requester, ) -> JsonDict: """ Args: @@ -486,6 +498,7 @@ async def encode_room( only_fields: Optional. The list of event fields to include. event_formatter: function to convert from federation format to client format + Requester: The requester of the sync Returns: The room, encoded in our response format """ @@ -539,7 +552,9 @@ async def encode_room( "org.matrix.msc3773.unread_thread_notifications" ] = room.unread_thread_notifications result["summary"] = room.summary - if self._msc2654_enabled: + if await self.store.get_feature_enabled( + requester.user.to_string(), "msc2654" + ): result["org.matrix.msc2654.unread_count"] = room.unread_count return result diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index eeccf5db2433..63f5a968099c 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -105,6 +105,7 @@ from synapse.util.caches.descriptors import cached if TYPE_CHECKING: + from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -1215,8 +1216,7 @@ def _get_if_maybe_push_in_range_for_user_txn(txn: LoggingTransaction) -> bool: async def add_push_actions_to_staging( self, event_id: str, - user_id_actions: Dict[str, Collection[Union[Mapping, str]]], - count_as_unread: bool, + user_id_actions: Dict[str, "ActionsForUser"], thread_id: str, ) -> None: """Add the push actions for the event to the push action staging area. @@ -1234,17 +1234,19 @@ async def add_push_actions_to_staging( # This is a helper function for generating the necessary tuple that # can be used to insert into the `event_push_actions_staging` table. def _gen_entry( - user_id: str, actions: Collection[Union[Mapping, str]] + user_id: str, actions_by_user: "ActionsForUser" ) -> Tuple[str, str, str, int, int, int, str, int]: - is_highlight = 1 if _action_has_highlight(actions) else 0 - notif = 1 if "notify" in actions else 0 + is_highlight = 1 if _action_has_highlight(actions_by_user.actions) else 0 + notif = 1 if "notify" in actions_by_user.actions else 0 return ( event_id, # event_id column user_id, # user_id column - _serialize_action(actions, bool(is_highlight)), # actions column + _serialize_action( + actions_by_user.actions, bool(is_highlight) + ), # actions column notif, # notif column is_highlight, # highlight column - int(count_as_unread), # unread column + int(actions_by_user.count_as_unread), # unread column thread_id, # thread_id column self._clock.time_msec(), # inserted_ts column ) @@ -1262,8 +1264,8 @@ def _gen_entry( "inserted_ts", ), values=[ - _gen_entry(user_id, actions) - for user_id, actions in user_id_actions.items() + _gen_entry(user_id, actions_by_user) + for user_id, actions_by_user in user_id_actions.items() ], desc="add_push_actions_to_staging", ) diff --git a/tests/replication/slave/storage/test_events.py b/tests/replication/slave/storage/test_events.py index b2125b1fea41..aea25ed6df7f 100644 --- a/tests/replication/slave/storage/test_events.py +++ b/tests/replication/slave/storage/test_events.py @@ -24,6 +24,7 @@ from synapse.events import EventBase, _EventInternalMetadata, make_event_from_dict from synapse.events.snapshot import EventContext from synapse.handlers.room import RoomEventSource +from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer from synapse.storage.databases.main.event_push_actions import ( NotifCounts, @@ -412,8 +413,10 @@ def build_event( self.get_success( self.master_store.add_push_actions_to_staging( event.event_id, - dict(push_actions), - False, + { + user_id: ActionsForUser(actions, False) + for user_id, actions in push_actions + }, "main", ) ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 9c876c7a3230..118b894773c3 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -538,9 +538,6 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def default_config(self) -> JsonDict: config = super().default_config() - config["experimental_features"] = { - "msc2654_enabled": True, - } return config def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: @@ -588,6 +585,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" + # add per-user flag to the DB + ex_handler = self.hs.get_experimental_features_manager() + self.get_success(ex_handler.set_feature_for_user(self.user_id, "msc2654", True)) # Check that our own messages don't increase the unread count. self.helper.send(self.room_id, "hello", tok=self.tok) From 4291c660dab5482f79a6c408e7cc43b25cdc9f2a Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 15:07:23 -0700 Subject: [PATCH 05/20] newsfragment --- changelog.d/15345.feature | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog.d/15345.feature diff --git a/changelog.d/15345.feature b/changelog.d/15345.feature new file mode 100644 index 000000000000..ebb317096196 --- /dev/null +++ b/changelog.d/15345.feature @@ -0,0 +1,3 @@ +Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence), +MSC2654 (unread counts), MSC3881 (remotely toggle push notifications for another client), and +MSC3967 (Do not require UIA when first uploading cross signing keys) from the experimental config to per-user flags. From d3cc11dbdf8f1b8bfc3394ec128f423434fdbf6e Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 28 Mar 2023 16:11:18 -0700 Subject: [PATCH 06/20] forgot to lint --- tests/rest/client/test_sync.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 118b894773c3..773765f789c8 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -543,6 +543,7 @@ def default_config(self) -> JsonDict: def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.url = "/sync?since=%s" self.next_batch = "s0" + self.hs = hs # Register the first user (used to check the unread counts). self.user_id = self.register_user("kermit", "monkey") @@ -586,8 +587,11 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" # add per-user flag to the DB - ex_handler = self.hs.get_experimental_features_manager() - self.get_success(ex_handler.set_feature_for_user(self.user_id, "msc2654", True)) + self.get_success( + self.hs.get_datastores().main.set_feature_for_user( + self.user_id, "msc2654", True + ) + ) # Check that our own messages don't increase the unread count. self.helper.send(self.room_id, "hello", tok=self.tok) From ca3e15bdd45180d02d51783773fd11a2921217dd Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 29 Mar 2023 11:35:03 -0700 Subject: [PATCH 07/20] add experimental features store to worker store --- synapse/app/generic_worker.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/app/generic_worker.py b/synapse/app/generic_worker.py index e17ce35b8e29..db5e8980df93 100644 --- a/synapse/app/generic_worker.py +++ b/synapse/app/generic_worker.py @@ -51,6 +51,7 @@ from synapse.rest.synapse.client import build_synapse_client_resource_tree from synapse.rest.well_known import well_known_resource from synapse.server import HomeServer +from synapse.storage.databases.main import ExperimentalFeaturesStore from synapse.storage.databases.main.account_data import AccountDataWorkerStore from synapse.storage.databases.main.appservice import ( ApplicationServiceTransactionWorkerStore, @@ -146,6 +147,7 @@ class GenericWorkerSlavedStore( TransactionWorkerStore, LockStore, SessionStore, + ExperimentalFeaturesStore, ): # Properties that multiple storage classes define. Tell mypy what the # expected type is. From 51769a9b70a3d60aebc70ab6f63b5d629e6520a2 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 29 Mar 2023 12:57:57 -0700 Subject: [PATCH 08/20] re-add parameters to test --- tests/handlers/test_presence.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 20d79f995b9b..7a71db3af6d1 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -15,6 +15,7 @@ from typing import Optional, cast from unittest.mock import Mock, call +from parameterized import parameterized from signedjson.key import generate_signing_key from twisted.test.proto_helpers import MemoryReactor @@ -723,6 +724,7 @@ def test_set_presence_from_syncing_keeps_status(self) -> None: # our status message should be the same as it was before self.assertEqual(state.status_msg, status_msg) + @parameterized.expand([(False,), (True,)]) def test_set_presence_from_syncing_keeps_busy( self, test_with_workers: bool ) -> None: From f9e7a0a3a4ccb918a9afaf8ed261350a02628f45 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 1 May 2023 14:24:49 -0700 Subject: [PATCH 09/20] add a db function to tell if just one feature is enabled --- .../databases/main/experimental_features.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index cf3226ae5a70..f88598794a85 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -73,3 +73,35 @@ async def set_features_for_user( ) await self.invalidate_cache_and_stream("list_enabled_features", (user,)) + + async def get_feature_enabled(self, user_id: str, feature: str) -> bool: + """ + Checks to see if a given feature is enabled for the user + Args: + user: + the user to be queried on + feature: + the feature in question + Returns: + True if the feature is enabled, False if it is not or if the feature was + not found. + """ + + res = await self.db_pool.simple_select_one( + "per_user_experimental_features", + {"user_id": user_id, "feature": feature}, + ["enabled"], + allow_none=True, + ) + + if not res: + res = {"enabled": False} + + # Deal with Sqlite boolean return values + if res["enabled"] == 0: + res["enabled"] = False + if res["enabled"] == 1: + res["enabled"] = True + + return res["enabled"] +>>>>>>> 6c2267f83d... add a db function to tell if just one feature is enabled From 842eb40e4517365e5eb2f48840dba561913352b4 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 1 May 2023 14:30:30 -0700 Subject: [PATCH 10/20] update tests to use enum --- .../databases/main/experimental_features.py | 2 +- tests/handlers/test_presence.py | 19 +++++++++++++------ tests/push/test_http.py | 17 +++++++++++++---- tests/rest/client/test_keys.py | 5 +++-- tests/rest/client/test_sync.py | 5 +++-- 5 files changed, 33 insertions(+), 15 deletions(-) diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index f88598794a85..e751f6860046 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -104,4 +104,4 @@ async def get_feature_enabled(self, user_id: str, feature: str) -> bool: res["enabled"] = True return res["enabled"] ->>>>>>> 6c2267f83d... add a db function to tell if just one feature is enabled + diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index 7a71db3af6d1..ab763d6284c8 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -36,6 +36,7 @@ handle_update, ) from synapse.rest import admin +from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import room from synapse.server import HomeServer from synapse.types import JsonDict, UserID, get_domain_from_id @@ -514,9 +515,14 @@ def test_last_active(self) -> None: class PresenceHandlerTestCase(BaseMultiWorkerStreamTestCase): + servlets = [ + admin.register_servlets, + ] + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.presence_handler = hs.get_presence_handler() self.clock = hs.get_clock() + self.user = self.register_user("test", "pass") def test_external_process_timeout(self) -> None: """Test that if an external process doesn't update the records for a while @@ -734,13 +740,12 @@ def test_set_presence_from_syncing_keeps_busy( test_with_workers: If True, check the presence state of the user by calling /sync against a worker, rather than the main process. """ - user_id = "@test:server" status_msg = "I'm busy!" # set busy state in db self.get_success( - self.hs.get_datastores().main.set_feature_for_user( - user_id, "msc_3026", True + self.hs.get_datastores().main.set_features_for_user( + self.user, {ExperimentalFeature.MSC3026: True} ) ) @@ -755,20 +760,22 @@ def test_set_presence_from_syncing_keeps_busy( ) # Set presence to BUSY - self._set_presencestate_with_status_msg(user_id, PresenceState.BUSY, status_msg) + self._set_presencestate_with_status_msg( + self.user, PresenceState.BUSY, status_msg + ) # Perform a sync with a presence state other than busy. This should NOT change # our presence status; we only change from busy if we explicitly set it via # /presence/*. self.get_success( worker_to_sync_against.get_presence_handler().user_syncing( - user_id, True, PresenceState.ONLINE + self.user, True, PresenceState.ONLINE ) ) # Check against the main process that the user's presence did not change. state = self.get_success( - self.presence_handler.get_state(UserID.from_string(user_id)) + self.presence_handler.get_state(UserID.from_string(self.user)) ) # we should still be busy self.assertEqual(state.state, PresenceState.BUSY) diff --git a/tests/push/test_http.py b/tests/push/test_http.py index 67c1934eaa7f..97bc48f24644 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -20,6 +20,7 @@ import synapse.rest.admin from synapse.logging.context import make_deferred_yieldable from synapse.push import PusherConfig, PusherConfigException +from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import login, push_rule, pusher, receipts, room from synapse.server import HomeServer from synapse.types import JsonDict @@ -829,7 +830,9 @@ def test_disable(self) -> None: # enable msc3881 per_user flag self.get_success( - self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + self.hs.get_datastores().main.set_features_for_user( + user_id, {ExperimentalFeature.MSC3881: True} + ) ) # Send a message and check that it generated a push. @@ -863,7 +866,9 @@ def test_enable(self) -> None: # enable msc3881 per_user flag self.get_success( - self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + self.hs.get_datastores().main.set_features_for_user( + user_id, {ExperimentalFeature.MSC3881: True} + ) ) # Send a message and check that it did not generate a push. @@ -897,7 +902,9 @@ def test_null_enabled(self) -> None: # enable msc3881 per_user flag self.get_success( - self.hs.get_datastores().main.set_feature_for_user(user_id, "msc3881", True) + self.hs.get_datastores().main.set_features_for_user( + user_id, {ExperimentalFeature.MSC3881: True} + ) ) channel = self.make_request("GET", "/pushers", access_token=access_token) @@ -944,7 +951,9 @@ def test_device_id(self) -> None: # enable msc3881 per_user flag self.get_success( - self.hs.get_datastores().main.set_feature_for_user(user, "msc3881", True) + self.hs.get_datastores().main.set_features_for_user( + user, {ExperimentalFeature.MSC3881: True} + ) ) # We create the pusher with an HTTP request rather than with diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index 713c7abec379..ef2800ee5457 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -23,6 +23,7 @@ from synapse.api.errors import Codes from synapse.rest import admin +from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import keys, login from synapse.types import JsonDict @@ -217,8 +218,8 @@ def test_device_signing_with_msc3967(self) -> None: # enable msc3967 in db self.get_success( - self.hs.get_datastores().main.set_feature_for_user( - alice_id, "msc3967", True + self.hs.get_datastores().main.set_features_for_user( + alice_id, {ExperimentalFeature.MSC3967: True} ) ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 773765f789c8..49cd4cee6047 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -28,6 +28,7 @@ ReceiptTypes, RelationTypes, ) +from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -588,8 +589,8 @@ def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" # add per-user flag to the DB self.get_success( - self.hs.get_datastores().main.set_feature_for_user( - self.user_id, "msc2654", True + self.hs.get_datastores().main.set_features_for_user( + self.user_id, {ExperimentalFeature.MSC2654: True} ) ) From e5f33c58cc5e928ab2e63f4e731534019780f0ff Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 1 May 2023 18:36:59 -0700 Subject: [PATCH 11/20] fall back to default config setting if not enabled in table --- synapse/config/experimental.py | 15 +++++++++++++++ synapse/handlers/presence.py | 4 ++++ synapse/push/bulk_push_rule_evaluator.py | 3 +++ synapse/rest/client/keys.py | 8 +++++++- synapse/rest/client/pusher.py | 23 ++++++++++++++--------- synapse/rest/client/sync.py | 9 +++++++-- 6 files changed, 50 insertions(+), 12 deletions(-) diff --git a/synapse/config/experimental.py b/synapse/config/experimental.py index 573763f27055..65996797312b 100644 --- a/synapse/config/experimental.py +++ b/synapse/config/experimental.py @@ -43,6 +43,9 @@ class ExperimentalConfig(Config): def read_config(self, config: JsonDict, **kwargs: Any) -> None: experimental = config.get("experimental_features") or {} + # MSC3026 (busy presence state) + self.msc3026_enabled: bool = experimental.get("msc3026_enabled", False) + # MSC2716 (importing historical messages) self.msc2716_enabled: bool = experimental.get("msc2716_enabled", False) @@ -96,6 +99,12 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: # MSC3720 (Account status endpoint) self.msc3720_enabled: bool = experimental.get("msc3720_enabled", False) + # MSC2654: Unread counts + # + # Note that enabling this will result in an incorrect unread count for + # previously calculated push actions. + self.msc2654_enabled: bool = experimental.get("msc2654_enabled", False) + # MSC2815 (allow room moderators to view redacted event content) self.msc2815_enabled: bool = experimental.get("msc2815_enabled", False) @@ -118,6 +127,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: raw_msc3866_config = experimental.get("msc3866", {}) self.msc3866 = MSC3866Config(**raw_msc3866_config) + # MSC3881: Remotely toggle push notifications for another client + self.msc3881_enabled: bool = experimental.get("msc3881_enabled", False) + # MSC3882: Allow an existing session to sign in a new session self.msc3882_enabled: bool = experimental.get("msc3882_enabled", False) self.msc3882_ui_auth: bool = experimental.get("msc3882_ui_auth", True) @@ -174,6 +186,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None: "msc3958_supress_edit_notifs", False ) + # MSC3967: Do not require UIA when first uploading cross signing keys + self.msc3967_enabled = experimental.get("msc3967_enabled", False) + # MSC2659: Application service ping endpoint self.msc2659_enabled = experimental.get("msc2659_enabled", False) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index e841c1b1b818..08649eaf7b16 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -608,6 +608,8 @@ async def set_state( busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( target_user.to_string(), "msc3026" ) + if not busy_presence_enabled: + busy_presence_enabled = self.hs.config.experimental.msc3026_enabled if presence not in valid_presence or ( presence == PresenceState.BUSY and not busy_presence_enabled @@ -1241,6 +1243,8 @@ async def set_state( busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( target_user.to_string(), "msc3026" ) + if not busy_presence_enabled: + busy_presence_enabled = self.hs.config.experimental.msc3026_enabled if presence not in valid_presence or ( presence == PresenceState.BUSY and not busy_presence_enabled diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index a2d00645f0c1..ec78e77c11b9 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -438,6 +438,9 @@ async def _action_for_event_by_user( # check whether unread counts are enabled for this user unread_enabled = await self.store.get_feature_enabled(uid, "msc2654") + if not unread_enabled: + unread_enabled = self.hs.config.experimental.msc2654_enabled + if unread_enabled: count_as_unread = _should_count_as_unread(event, context) else: diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 2f3058d3fb25..08bd38d7aa5e 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -375,7 +375,13 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() body = parse_json_object_from_request(request) - if await self.hs.get_datastores().main.get_feature_enabled(user_id, "msc3967"): + msc3967_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user_id, "msc3967" + ) + if not msc3967_enabled: + msc3967_enabled = self.hs.config.experimental.msc2654_enabled + + if msc3967_enabled: if await self.e2e_keys_handler.is_cross_signing_set_up_for_user(user_id): # If we already have a master key then cross signing is set up and we require UIA to reset await self.auth_handler.validate_user_via_ui_auth( diff --git a/synapse/rest/client/pusher.py b/synapse/rest/client/pusher.py index 0cc63dc5de23..d6c0f1bd166f 100644 --- a/synapse/rest/client/pusher.py +++ b/synapse/rest/client/pusher.py @@ -53,10 +53,14 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: pusher_dicts = [p.as_dict() for p in pushers] + msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ) + if not msc3881_enabled: + msc3881_enabled = self.hs.config.experimental.msc3881_enabled + for pusher in pusher_dicts: - if await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" - ): + if msc3881_enabled: pusher["org.matrix.msc3881.enabled"] = pusher["enabled"] pusher["org.matrix.msc3881.device_id"] = pusher["device_id"] del pusher["enabled"] @@ -113,12 +117,13 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: append = content["append"] enabled = True - if ( - await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" - ) - and "org.matrix.msc3881.enabled" in content - ): + msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ) + if not msc3881_enabled: + msc3881_enabled = self.hs.config.experimental.msc3881_enabled + + if msc3881_enabled and "org.matrix.msc3881.enabled" in content: enabled = content["org.matrix.msc3881.enabled"] if not append: diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 7ae96493cbcd..0a5e08875f2e 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -552,9 +552,14 @@ async def encode_room( "org.matrix.msc3773.unread_thread_notifications" ] = room.unread_thread_notifications result["summary"] = room.summary - if await self.store.get_feature_enabled( + + msc2654_enabled = await self.hs.get_datastores().main.get_feature_enabled( requester.user.to_string(), "msc2654" - ): + ) + if not msc2654_enabled: + msc2654_enabled = self.hs.config.experimental.msc2654_enabled + + if msc2654_enabled: result["org.matrix.msc2654.unread_count"] = room.unread_count return result From 15dd3727e09c6ca84bd6f9a4d77eec8cb3fc6060 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Mon, 1 May 2023 18:46:22 -0700 Subject: [PATCH 12/20] stupid github web editor --- synapse/storage/databases/main/experimental_features.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index e751f6860046..5e11295bd355 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -104,4 +104,3 @@ async def get_feature_enabled(self, user_id: str, feature: str) -> bool: res["enabled"] = True return res["enabled"] - From e53a8a5baf1af9482f195a8879671192e07d96b2 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 2 May 2023 11:39:37 -0700 Subject: [PATCH 13/20] change how config is checked --- synapse/handlers/presence.py | 18 ++++++++++-------- synapse/rest/client/keys.py | 7 +++---- synapse/rest/client/pusher.py | 18 ++++++++++-------- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index 08649eaf7b16..f681beb92e6b 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -605,11 +605,12 @@ async def set_state( PresenceState.BUSY, ) - busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( - target_user.to_string(), "msc3026" + busy_presence_enabled = ( + await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), "msc3026" + ) + or self.hs.config.experimental.msc3026_enabled ) - if not busy_presence_enabled: - busy_presence_enabled = self.hs.config.experimental.msc3026_enabled if presence not in valid_presence or ( presence == PresenceState.BUSY and not busy_presence_enabled @@ -1240,11 +1241,12 @@ async def set_state( PresenceState.BUSY, ) - busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( - target_user.to_string(), "msc3026" + busy_presence_enabled = ( + await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), "msc3026" + ) + or self.hs.config.experimental.msc3026_enabled ) - if not busy_presence_enabled: - busy_presence_enabled = self.hs.config.experimental.msc3026_enabled if presence not in valid_presence or ( presence == PresenceState.BUSY and not busy_presence_enabled diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 08bd38d7aa5e..5d8b9239a41c 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -375,11 +375,10 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() body = parse_json_object_from_request(request) - msc3967_enabled = await self.hs.get_datastores().main.get_feature_enabled( - user_id, "msc3967" + msc3967_enabled = ( + await self.hs.get_datastores().main.get_feature_enabled(user_id, "msc3967") + or self.hs.config.experimental.msc2654_enabled ) - if not msc3967_enabled: - msc3967_enabled = self.hs.config.experimental.msc2654_enabled if msc3967_enabled: if await self.e2e_keys_handler.is_cross_signing_set_up_for_user(user_id): diff --git a/synapse/rest/client/pusher.py b/synapse/rest/client/pusher.py index d6c0f1bd166f..40c4b6adc356 100644 --- a/synapse/rest/client/pusher.py +++ b/synapse/rest/client/pusher.py @@ -53,11 +53,12 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: pusher_dicts = [p.as_dict() for p in pushers] - msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" + msc3881_enabled = ( + await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ) + or self.hs.config.experimental.msc3881_enabled ) - if not msc3881_enabled: - msc3881_enabled = self.hs.config.experimental.msc3881_enabled for pusher in pusher_dicts: if msc3881_enabled: @@ -117,11 +118,12 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: append = content["append"] enabled = True - msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" + msc3881_enabled = ( + await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), "msc3881" + ) + or self.hs.config.experimental.msc3881_enabled ) - if not msc3881_enabled: - msc3881_enabled = self.hs.config.experimental.msc3881_enabled if msc3881_enabled and "org.matrix.msc3881.enabled" in content: enabled = content["org.matrix.msc3881.enabled"] From e8c571b1cab19bd5d647baeb51d68dbf9bd99db6 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Tue, 2 May 2023 11:51:40 -0700 Subject: [PATCH 14/20] remove support for per-user msc2654 --- changelog.d/15345.feature | 4 +- synapse/push/bulk_push_rule_evaluator.py | 47 ++++++++----------- synapse/rest/client/sync.py | 22 ++------- .../databases/main/event_push_actions.py | 20 ++++---- .../replication/slave/storage/test_events.py | 7 +-- tests/rest/client/test_sync.py | 11 ++--- 6 files changed, 39 insertions(+), 72 deletions(-) diff --git a/changelog.d/15345.feature b/changelog.d/15345.feature index ebb317096196..4662bb0fd382 100644 --- a/changelog.d/15345.feature +++ b/changelog.d/15345.feature @@ -1,3 +1,3 @@ Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence), -MSC2654 (unread counts), MSC3881 (remotely toggle push notifications for another client), and -MSC3967 (Do not require UIA when first uploading cross signing keys) from the experimental config to per-user flags. +MSC3881 (remotely toggle push notifications for another client), and MSC3967 (Do not require UIA when first uploading +cross signing keys) from the experimental config to per-user flags. diff --git a/synapse/push/bulk_push_rule_evaluator.py b/synapse/push/bulk_push_rule_evaluator.py index ec78e77c11b9..320084f5f58c 100644 --- a/synapse/push/bulk_push_rule_evaluator.py +++ b/synapse/push/bulk_push_rule_evaluator.py @@ -27,7 +27,6 @@ Union, ) -import attr from prometheus_client import Counter from synapse.api.constants import ( @@ -108,17 +107,6 @@ def _should_count_as_unread(event: EventBase, context: EventContext) -> bool: return False -@attr.s(slots=True, auto_attribs=True) -class ActionsForUser: - """ - A class to hold the actions for a given event and whether the event should - increment the unread count. - """ - - actions: Collection[Union[Mapping, str]] - count_as_unread: bool - - class BulkPushRuleEvaluator: """Calculates the outcome of push rules for an event for all users in the room at once. @@ -348,8 +336,15 @@ async def _action_for_event_by_user( # (historical messages persisted in reverse-chronological order). return + # Disable counting as unread unless the experimental configuration is + # enabled, as it can cause additional (unwanted) rows to be added to the + # event_push_actions table. + count_as_unread = False + if self.hs.config.experimental.msc2654_enabled: + count_as_unread = _should_count_as_unread(event, context) + rules_by_user = await self._get_rules_for_event(event) - actions_by_user: Dict[str, ActionsForUser] = {} + actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {} room_member_count = await self.store.get_number_joined_users_in_room( event.room_id @@ -434,22 +429,17 @@ async def _action_for_event_by_user( if not isinstance(display_name, str): display_name = None - actions = evaluator.run(rules, uid, display_name) - - # check whether unread counts are enabled for this user - unread_enabled = await self.store.get_feature_enabled(uid, "msc2654") - if not unread_enabled: - unread_enabled = self.hs.config.experimental.msc2654_enabled + if count_as_unread: + # Add an element for the current user if the event needs to be marked as + # unread, so that add_push_actions_to_staging iterates over it. + # If the event shouldn't be marked as unread but should notify the + # current user, it'll be added to the dict later. + actions_by_user[uid] = [] - if unread_enabled: - count_as_unread = _should_count_as_unread(event, context) - else: - count_as_unread = False - - if "notify" in actions or count_as_unread: - # Push rules say we should notify the user of this event or the event should - # increment the unread count - actions_by_user[uid] = ActionsForUser(actions, count_as_unread) + actions = evaluator.run(rules, uid, display_name) + if "notify" in actions: + # Push rules say we should notify the user of this event + actions_by_user[uid] = actions # If there aren't any actions then we can skip the rest of the # processing. @@ -477,6 +467,7 @@ async def _action_for_event_by_user( await self.store.add_push_actions_to_staging( event.event_id, actions_by_user, + count_as_unread, thread_id, ) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 0a5e08875f2e..cbcfacad9079 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -100,6 +100,7 @@ def __init__(self, hs: "HomeServer"): self.presence_handler = hs.get_presence_handler() self._server_notices_sender = hs.get_server_notices_sender() self._event_serializer = hs.get_event_client_serializer() + self._msc2654_enabled = hs.config.experimental.msc2654_enabled self._msc3773_enabled = hs.config.experimental.msc3773_enabled async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: @@ -260,7 +261,7 @@ async def encode_response( ) joined = await self.encode_joined( - sync_result.joined, time_now, serialize_options, requester + sync_result.joined, time_now, serialize_options ) invited = await self.encode_invited( @@ -272,7 +273,7 @@ async def encode_response( ) archived = await self.encode_archived( - sync_result.archived, time_now, serialize_options, requester + sync_result.archived, time_now, serialize_options ) logger.debug("building sync response dict") @@ -343,7 +344,6 @@ async def encode_joined( rooms: List[JoinedSyncResult], time_now: int, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Encode the joined rooms in a sync result @@ -352,7 +352,6 @@ async def encode_joined( rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options - requester: The requester of the sync Returns: The joined rooms list, in our response format """ @@ -363,7 +362,6 @@ async def encode_joined( time_now, joined=True, serialize_options=serialize_options, - requester=requester, ) return joined @@ -454,7 +452,6 @@ async def encode_archived( rooms: List[ArchivedSyncResult], time_now: int, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Encode the archived rooms in a sync result @@ -463,7 +460,6 @@ async def encode_archived( rooms: list of sync results for rooms this user is joined to time_now: current time - used as a baseline for age calculations serialize_options: Event serializer options - requester: the requester of the sync Returns: The archived rooms list, in our response format """ @@ -474,7 +470,6 @@ async def encode_archived( time_now, joined=False, serialize_options=serialize_options, - requester=requester, ) return joined @@ -485,7 +480,6 @@ async def encode_room( time_now: int, joined: bool, serialize_options: SerializeEventConfig, - requester: Requester, ) -> JsonDict: """ Args: @@ -498,7 +492,6 @@ async def encode_room( only_fields: Optional. The list of event fields to include. event_formatter: function to convert from federation format to client format - Requester: The requester of the sync Returns: The room, encoded in our response format """ @@ -552,14 +545,7 @@ async def encode_room( "org.matrix.msc3773.unread_thread_notifications" ] = room.unread_thread_notifications result["summary"] = room.summary - - msc2654_enabled = await self.hs.get_datastores().main.get_feature_enabled( - requester.user.to_string(), "msc2654" - ) - if not msc2654_enabled: - msc2654_enabled = self.hs.config.experimental.msc2654_enabled - - if msc2654_enabled: + if self._msc2654_enabled: result["org.matrix.msc2654.unread_count"] = room.unread_count return result diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 63f5a968099c..eeccf5db2433 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -105,7 +105,6 @@ from synapse.util.caches.descriptors import cached if TYPE_CHECKING: - from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer logger = logging.getLogger(__name__) @@ -1216,7 +1215,8 @@ def _get_if_maybe_push_in_range_for_user_txn(txn: LoggingTransaction) -> bool: async def add_push_actions_to_staging( self, event_id: str, - user_id_actions: Dict[str, "ActionsForUser"], + user_id_actions: Dict[str, Collection[Union[Mapping, str]]], + count_as_unread: bool, thread_id: str, ) -> None: """Add the push actions for the event to the push action staging area. @@ -1234,19 +1234,17 @@ async def add_push_actions_to_staging( # This is a helper function for generating the necessary tuple that # can be used to insert into the `event_push_actions_staging` table. def _gen_entry( - user_id: str, actions_by_user: "ActionsForUser" + user_id: str, actions: Collection[Union[Mapping, str]] ) -> Tuple[str, str, str, int, int, int, str, int]: - is_highlight = 1 if _action_has_highlight(actions_by_user.actions) else 0 - notif = 1 if "notify" in actions_by_user.actions else 0 + is_highlight = 1 if _action_has_highlight(actions) else 0 + notif = 1 if "notify" in actions else 0 return ( event_id, # event_id column user_id, # user_id column - _serialize_action( - actions_by_user.actions, bool(is_highlight) - ), # actions column + _serialize_action(actions, bool(is_highlight)), # actions column notif, # notif column is_highlight, # highlight column - int(actions_by_user.count_as_unread), # unread column + int(count_as_unread), # unread column thread_id, # thread_id column self._clock.time_msec(), # inserted_ts column ) @@ -1264,8 +1262,8 @@ def _gen_entry( "inserted_ts", ), values=[ - _gen_entry(user_id, actions_by_user) - for user_id, actions_by_user in user_id_actions.items() + _gen_entry(user_id, actions) + for user_id, actions in user_id_actions.items() ], desc="add_push_actions_to_staging", ) diff --git a/tests/replication/slave/storage/test_events.py b/tests/replication/slave/storage/test_events.py index aea25ed6df7f..b2125b1fea41 100644 --- a/tests/replication/slave/storage/test_events.py +++ b/tests/replication/slave/storage/test_events.py @@ -24,7 +24,6 @@ from synapse.events import EventBase, _EventInternalMetadata, make_event_from_dict from synapse.events.snapshot import EventContext from synapse.handlers.room import RoomEventSource -from synapse.push.bulk_push_rule_evaluator import ActionsForUser from synapse.server import HomeServer from synapse.storage.databases.main.event_push_actions import ( NotifCounts, @@ -413,10 +412,8 @@ def build_event( self.get_success( self.master_store.add_push_actions_to_staging( event.event_id, - { - user_id: ActionsForUser(actions, False) - for user_id, actions in push_actions - }, + dict(push_actions), + False, "main", ) ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 49cd4cee6047..9c876c7a3230 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -28,7 +28,6 @@ ReceiptTypes, RelationTypes, ) -from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync from synapse.server import HomeServer from synapse.types import JsonDict @@ -539,12 +538,14 @@ class UnreadMessagesTestCase(unittest.HomeserverTestCase): def default_config(self) -> JsonDict: config = super().default_config() + config["experimental_features"] = { + "msc2654_enabled": True, + } return config def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.url = "/sync?since=%s" self.next_batch = "s0" - self.hs = hs # Register the first user (used to check the unread counts). self.user_id = self.register_user("kermit", "monkey") @@ -587,12 +588,6 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: def test_unread_counts(self) -> None: """Tests that /sync returns the right value for the unread count (MSC2654).""" - # add per-user flag to the DB - self.get_success( - self.hs.get_datastores().main.set_features_for_user( - self.user_id, {ExperimentalFeature.MSC2654: True} - ) - ) # Check that our own messages don't increase the unread count. self.helper.send(self.room_id, "hello", tok=self.tok) From aea7cbd48c623057427b56583d7afc12a691ee46 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 08:53:35 -0700 Subject: [PATCH 15/20] move ExperimentalFeature definition to avoid circular import --- synapse/rest/admin/experimental_features.py | 13 +------------ .../storage/databases/main/experimental_features.py | 12 +++++++++++- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/synapse/rest/admin/experimental_features.py b/synapse/rest/admin/experimental_features.py index 1d409ac2b7b0..8b07e1dff6f6 100644 --- a/synapse/rest/admin/experimental_features.py +++ b/synapse/rest/admin/experimental_features.py @@ -13,7 +13,6 @@ # limitations under the License. -from enum import Enum from http import HTTPStatus from typing import TYPE_CHECKING, Dict, Tuple @@ -21,23 +20,13 @@ from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest from synapse.rest.admin import admin_patterns, assert_requester_is_admin +from synapse.storage.databases.main.experimental_features import ExperimentalFeature from synapse.types import JsonDict, UserID if TYPE_CHECKING: from synapse.server import HomeServer -class ExperimentalFeature(str, Enum): - """ - Currently supported per-user features - """ - - MSC3026 = "msc3026" - MSC2654 = "msc2654" - MSC3881 = "msc3881" - MSC3967 = "msc3967" - - class ExperimentalFeaturesRestServlet(RestServlet): """ Enable or disable experimental features for a user or determine which features are enabled diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index 5e11295bd355..197cdb21cea2 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from enum import Enum from typing import TYPE_CHECKING, Dict from synapse.storage.database import DatabasePool, LoggingDatabaseConnection @@ -20,10 +21,19 @@ from synapse.util.caches.descriptors import cached if TYPE_CHECKING: - from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.server import HomeServer +class ExperimentalFeature(str, Enum): + """ + Currently supported per-user features + """ + + MSC3026 = "msc3026" + MSC2654 = "msc2654" + MSC3881 = "msc3881" + MSC3967 = "msc3967" + class ExperimentalFeaturesStore(CacheInvalidationWorkerStore): def __init__( self, From e156b84c3f7156b91b1c59297463aa58c25f3a93 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 09:04:56 -0700 Subject: [PATCH 16/20] consolidate logic checking config and db to one place --- synapse/handlers/presence.py | 15 +++----- synapse/rest/client/keys.py | 6 ++-- synapse/rest/client/pusher.py | 15 +++----- .../databases/main/experimental_features.py | 36 +++++++++++-------- 4 files changed, 35 insertions(+), 37 deletions(-) diff --git a/synapse/handlers/presence.py b/synapse/handlers/presence.py index f681beb92e6b..9516d3fbf80e 100644 --- a/synapse/handlers/presence.py +++ b/synapse/handlers/presence.py @@ -63,6 +63,7 @@ from synapse.replication.tcp.commands import ClearUserSyncsCommand from synapse.replication.tcp.streams import PresenceFederationStream, PresenceStream from synapse.storage.databases.main import DataStore +from synapse.storage.databases.main.experimental_features import ExperimentalFeature from synapse.streams import EventSource from synapse.types import ( JsonDict, @@ -605,11 +606,8 @@ async def set_state( PresenceState.BUSY, ) - busy_presence_enabled = ( - await self.hs.get_datastores().main.get_feature_enabled( - target_user.to_string(), "msc3026" - ) - or self.hs.config.experimental.msc3026_enabled + busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), ExperimentalFeature.MSC3026 ) if presence not in valid_presence or ( @@ -1241,11 +1239,8 @@ async def set_state( PresenceState.BUSY, ) - busy_presence_enabled = ( - await self.hs.get_datastores().main.get_feature_enabled( - target_user.to_string(), "msc3026" - ) - or self.hs.config.experimental.msc3026_enabled + busy_presence_enabled = await self.hs.get_datastores().main.get_feature_enabled( + target_user.to_string(), ExperimentalFeature.MSC3026 ) if presence not in valid_presence or ( diff --git a/synapse/rest/client/keys.py b/synapse/rest/client/keys.py index 5d8b9239a41c..bc120d262acc 100644 --- a/synapse/rest/client/keys.py +++ b/synapse/rest/client/keys.py @@ -31,6 +31,7 @@ from synapse.logging.opentracing import log_kv, set_tag from synapse.replication.http.devices import ReplicationUploadKeysForUserRestServlet from synapse.rest.client._base import client_patterns, interactive_auth_handler +from synapse.storage.databases.main.experimental_features import ExperimentalFeature from synapse.types import JsonDict, StreamToken from synapse.util.cancellation import cancellable @@ -375,9 +376,8 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id = requester.user.to_string() body = parse_json_object_from_request(request) - msc3967_enabled = ( - await self.hs.get_datastores().main.get_feature_enabled(user_id, "msc3967") - or self.hs.config.experimental.msc2654_enabled + msc3967_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user_id, ExperimentalFeature.MSC3967 ) if msc3967_enabled: diff --git a/synapse/rest/client/pusher.py b/synapse/rest/client/pusher.py index 40c4b6adc356..5fcb19beb932 100644 --- a/synapse/rest/client/pusher.py +++ b/synapse/rest/client/pusher.py @@ -27,6 +27,7 @@ from synapse.push import PusherConfigException from synapse.rest.client._base import client_patterns from synapse.rest.synapse.client.unsubscribe import UnsubscribeResource +from synapse.storage.databases.main.experimental_features import ExperimentalFeature from synapse.types import JsonDict if TYPE_CHECKING: @@ -53,11 +54,8 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: pusher_dicts = [p.as_dict() for p in pushers] - msc3881_enabled = ( - await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" - ) - or self.hs.config.experimental.msc3881_enabled + msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), ExperimentalFeature.MSC3881 ) for pusher in pusher_dicts: @@ -118,11 +116,8 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]: append = content["append"] enabled = True - msc3881_enabled = ( - await self.hs.get_datastores().main.get_feature_enabled( - user.to_string(), "msc3881" - ) - or self.hs.config.experimental.msc3881_enabled + msc3881_enabled = await self.hs.get_datastores().main.get_feature_enabled( + user.to_string(), ExperimentalFeature.MSC3881 ) if msc3881_enabled and "org.matrix.msc3881.enabled" in content: diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index 197cdb21cea2..7a0a2caa0e27 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -34,6 +34,7 @@ class ExperimentalFeature(str, Enum): MSC3881 = "msc3881" MSC3967 = "msc3967" + class ExperimentalFeaturesStore(CacheInvalidationWorkerStore): def __init__( self, @@ -84,19 +85,32 @@ async def set_features_for_user( await self.invalidate_cache_and_stream("list_enabled_features", (user,)) - async def get_feature_enabled(self, user_id: str, feature: str) -> bool: + async def get_feature_enabled( + self, user_id: str, feature: "ExperimentalFeature" + ) -> bool: """ Checks to see if a given feature is enabled for the user + Args: - user: - the user to be queried on - feature: - the feature in question + user_id: the user to be queried on + feature: the feature in question Returns: True if the feature is enabled, False if it is not or if the feature was not found. """ + # check first if feature is enabled in the config + if feature == ExperimentalFeature.MSC3026: + globally_enabled = self.hs.config.experimental.msc3026_enabled + elif feature == ExperimentalFeature.MSC3881: + globally_enabled = self.hs.config.experimental.msc3881_enabled + else: + globally_enabled = self.hs.config.experimental.msc3967_enabled + + if globally_enabled: + return globally_enabled + + # if it's not enabled globally, check if it is enabled per-user res = await self.db_pool.simple_select_one( "per_user_experimental_features", {"user_id": user_id, "feature": feature}, @@ -104,13 +118,7 @@ async def get_feature_enabled(self, user_id: str, feature: str) -> bool: allow_none=True, ) - if not res: - res = {"enabled": False} - - # Deal with Sqlite boolean return values - if res["enabled"] == 0: - res["enabled"] = False - if res["enabled"] == 1: - res["enabled"] = True + # None and false are treated the same + db_enabled = bool(res) - return res["enabled"] + return db_enabled From 7568c726d34b4e8b3602a73acac45ef76aaa4117 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 09:05:19 -0700 Subject: [PATCH 17/20] test activation both by config and admin api --- tests/handlers/test_presence.py | 77 +++++++++++++++++++++++++++----- tests/push/test_http.py | 57 ++++++++++++++++++++---- tests/rest/client/test_keys.py | 79 +++++++++++++++++++++++++++++---- 3 files changed, 186 insertions(+), 27 deletions(-) diff --git a/tests/handlers/test_presence.py b/tests/handlers/test_presence.py index ab763d6284c8..ba47e801a497 100644 --- a/tests/handlers/test_presence.py +++ b/tests/handlers/test_presence.py @@ -36,8 +36,7 @@ handle_update, ) from synapse.rest import admin -from synapse.rest.admin.experimental_features import ExperimentalFeature -from synapse.rest.client import room +from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.types import JsonDict, UserID, get_domain_from_id from synapse.util import Clock @@ -515,14 +514,13 @@ def test_last_active(self) -> None: class PresenceHandlerTestCase(BaseMultiWorkerStreamTestCase): - servlets = [ - admin.register_servlets, - ] + servlets = [admin.register_servlets, login.register_servlets] def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.presence_handler = hs.get_presence_handler() self.clock = hs.get_clock() - self.user = self.register_user("test", "pass") + self.user = self.register_user("test", "pass", True) + self.admin_user_tok = self.login("test", "pass") def test_external_process_timeout(self) -> None: """Test that if an external process doesn't update the records for a while @@ -731,10 +729,11 @@ def test_set_presence_from_syncing_keeps_status(self) -> None: self.assertEqual(state.status_msg, status_msg) @parameterized.expand([(False,), (True,)]) - def test_set_presence_from_syncing_keeps_busy( + def test_set_presence_from_syncing_keeps_busy_via_admin( self, test_with_workers: bool ) -> None: - """Test that presence set by syncing doesn't affect busy status + """Test that presence set by syncing doesn't affect busy status, with the busy status + enabled via the admin api. Args: test_with_workers: If True, check the presence state of the user by calling @@ -742,13 +741,69 @@ def test_set_presence_from_syncing_keeps_busy( """ status_msg = "I'm busy!" - # set busy state in db + # activate busy state via admin api + url = f"/_synapse/admin/v1/experimental_features/{self.user}" + channel = self.make_request( + "PUT", + url, + content={ + "features": {"msc3026": True}, + }, + access_token=self.admin_user_tok, + ) + self.assertEqual(channel.code, 200) + + # By default, we call /sync against the main process. + worker_to_sync_against = self.hs + if test_with_workers: + # Create a worker and use it to handle /sync traffic instead. + # This is used to test that presence changes get replicated from workers + # to the main process correctly. + worker_to_sync_against = self.make_worker_hs( + "synapse.app.generic_worker", {"worker_name": "presence_writer"} + ) + + # Set presence to BUSY + self._set_presencestate_with_status_msg( + self.user, PresenceState.BUSY, status_msg + ) + + # Perform a sync with a presence state other than busy. This should NOT change + # our presence status; we only change from busy if we explicitly set it via + # /presence/*. self.get_success( - self.hs.get_datastores().main.set_features_for_user( - self.user, {ExperimentalFeature.MSC3026: True} + worker_to_sync_against.get_presence_handler().user_syncing( + self.user, True, PresenceState.ONLINE ) ) + # Check against the main process that the user's presence did not change. + state = self.get_success( + self.presence_handler.get_state(UserID.from_string(self.user)) + ) + # we should still be busy + self.assertEqual(state.state, PresenceState.BUSY) + + @parameterized.expand([(False,), (True,)]) + @unittest.override_config( + { + "experimental_features": { + "msc3026_enabled": True, + }, + } + ) + def test_set_presence_from_syncing_keeps_busy_via_config( + self, test_with_workers: bool + ) -> None: + """Test that presence set by syncing doesn't affect busy status, with the busy status + enabled via the config + + Args: + test_with_workers: If True, check the presence state of the user by calling + /sync against a worker, rather than the main process. + """ + status_msg = "I'm busy!" + # By default, we call /sync against the main process. worker_to_sync_against = self.hs if test_with_workers: diff --git a/tests/push/test_http.py b/tests/push/test_http.py index 97bc48f24644..42f24a530d05 100644 --- a/tests/push/test_http.py +++ b/tests/push/test_http.py @@ -20,9 +20,9 @@ import synapse.rest.admin from synapse.logging.context import make_deferred_yieldable from synapse.push import PusherConfig, PusherConfigException -from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import login, push_rule, pusher, receipts, room from synapse.server import HomeServer +from synapse.storage.databases.main.experimental_features import ExperimentalFeature from synapse.types import JsonDict from synapse.util import Clock @@ -37,6 +37,7 @@ class HTTPPusherTests(HomeserverTestCase): receipts.register_servlets, push_rule.register_servlets, pusher.register_servlets, + synapse.rest.admin.register_servlets, ] user_id = True hijack_auth = False @@ -820,20 +821,60 @@ def test_dont_notify_rule_overrides_message(self) -> None: self.helper.send(room, body="Hello", tok=access_token) self.assertEqual(len(self.push_attempts), 1) - def test_disable(self) -> None: - """Tests that disabling a pusher means it's not pushed to anymore.""" + @override_config({"experimental_features": {"msc3881_enabled": True}}) + def test_disable_via_config(self) -> None: + """Tests that disabling a pusher means it's not pushed to anymore, with the + ability to disable a pusher enabled via the config. + """ user_id, access_token = self._make_user_with_pusher("user") other_user_id, other_access_token = self._make_user_with_pusher("otheruser") room = self.helper.create_room_as(user_id, tok=access_token) self.helper.join(room=room, user=other_user_id, tok=other_access_token) - # enable msc3881 per_user flag - self.get_success( - self.hs.get_datastores().main.set_features_for_user( - user_id, {ExperimentalFeature.MSC3881: True} - ) + # Send a message and check that it generated a push. + self.helper.send(room, body="Hi!", tok=other_access_token) + self.assertEqual(len(self.push_attempts), 1) + + # Disable the pusher. + self._set_pusher(user_id, access_token, enabled=False) + + # Send another message and check that it did not generate a push. + self.helper.send(room, body="Hi!", tok=other_access_token) + self.assertEqual(len(self.push_attempts), 1) + + # Get the pushers for the user and check that it is marked as disabled. + channel = self.make_request("GET", "/pushers", access_token=access_token) + self.assertEqual(channel.code, 200) + self.assertEqual(len(channel.json_body["pushers"]), 1) + + enabled = channel.json_body["pushers"][0]["org.matrix.msc3881.enabled"] + self.assertFalse(enabled) + self.assertTrue(isinstance(enabled, bool)) + + def test_disable_via_admin(self) -> None: + """Tests that disabling a pusher means it's not pushed to anymore, + with the ability to disable a pusher enabled via the admin api. + """ + user_id, access_token = self._make_user_with_pusher("user") + other_user_id, other_access_token = self._make_user_with_pusher("otheruser") + self.register_user("admin", "pass", True) + admin_tok = self.login("admin", "pass") + + room = self.helper.create_room_as(user_id, tok=access_token) + self.helper.join(room=room, user=other_user_id, tok=other_access_token) + + # enable msc3881 per_user flag via the admin api + url = f"/_synapse/admin/v1/experimental_features/{user_id}" + channel = self.make_request( + "PUT", + url, + content={ + "features": {"msc3881": True}, + }, + access_token=admin_tok, ) + self.assertEqual(channel.code, 200) # Send a message and check that it generated a push. self.helper.send(room, body="Hi!", tok=other_access_token) diff --git a/tests/rest/client/test_keys.py b/tests/rest/client/test_keys.py index ef2800ee5457..46ab0b505f09 100644 --- a/tests/rest/client/test_keys.py +++ b/tests/rest/client/test_keys.py @@ -23,7 +23,6 @@ from synapse.api.errors import Codes from synapse.rest import admin -from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.rest.client import keys, login from synapse.types import JsonDict @@ -37,6 +36,7 @@ class KeyQueryTestCase(unittest.HomeserverTestCase): keys.register_servlets, admin.register_servlets_for_client_rest_resource, login.register_servlets, + admin.register_servlets, ] def test_rejects_device_id_ice_key_outside_of_list(self) -> None: @@ -207,21 +207,84 @@ def test_device_signing_with_uia_session_timeout(self) -> None: @override_config( { "ui_auth": {"session_timeout": "15s"}, + "experimental_features": {"msc3967_enabled": True}, } ) - def test_device_signing_with_msc3967(self) -> None: - """Device signing key follows MSC3967 behaviour when enabled.""" + def test_device_signing_with_msc3967_via_config(self) -> None: + """Device signing key follows MSC3967 behaviour when enabled in config.""" password = "wonderland" device_id = "ABCDEFGHI" alice_id = self.register_user("alice", password) alice_token = self.login("alice", password, device_id=device_id) - # enable msc3967 in db - self.get_success( - self.hs.get_datastores().main.set_features_for_user( - alice_id, {ExperimentalFeature.MSC3967: True} - ) + keys1 = self.make_device_keys(alice_id, device_id) + + # Initial request should succeed as no existing keys are present. + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys1, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + keys2 = self.make_device_keys(alice_id, device_id) + + # Subsequent request should require UIA as keys already exist even though session_timeout is set. + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys2, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.UNAUTHORIZED, channel.result) + + # Grab the session + session = channel.json_body["session"] + # Ensure that flows are what is expected. + self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"]) + + # add UI auth + keys2["auth"] = { + "type": "m.login.password", + "identifier": {"type": "m.id.user", "user": alice_id}, + "password": password, + "session": session, + } + + # Request should complete + channel = self.make_request( + "POST", + "/_matrix/client/v3/keys/device_signing/upload", + keys2, + alice_token, + ) + self.assertEqual(channel.code, HTTPStatus.OK, channel.result) + + @override_config( + { + "ui_auth": {"session_timeout": "15s"}, + } + ) + def test_device_signing_with_msc3967_via_admin(self) -> None: + """Device signing key follows MSC3967 behaviour when enabled for user via admin api.""" + password = "wonderland" + device_id = "ABCDEFGHI" + alice_id = self.register_user("alice", password) + alice_token = self.login("alice", password, device_id=device_id) + self.register_user("admin", "pass", True) + admin_tok = self.login("admin", "pass") + + url = f"/_synapse/admin/v1/experimental_features/{alice_id}" + channel = self.make_request( + "PUT", + url, + content={ + "features": {"msc3967": True}, + }, + access_token=admin_tok, ) + self.assertEqual(channel.code, 200) keys1 = self.make_device_keys(alice_id, device_id) From 9155b82c6433265b1bec6ea30544d6633a94da39 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 09:16:04 -0700 Subject: [PATCH 18/20] remove changes to sync --- synapse/rest/client/sync.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index cbcfacad9079..fc036c4dfa9b 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -358,10 +358,7 @@ async def encode_joined( joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, - time_now, - joined=True, - serialize_options=serialize_options, + room, time_now, joined=True, serialize_options=serialize_options, ) return joined @@ -466,10 +463,7 @@ async def encode_archived( joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, - time_now, - joined=False, - serialize_options=serialize_options, + room, time_now, joined=False, serialize_options=serialize_options, ) return joined From a767f1c8a9d7cda915650a682f42e7a8b60b7138 Mon Sep 17 00:00:00 2001 From: "H. Shay" Date: Wed, 10 May 2023 09:29:15 -0700 Subject: [PATCH 19/20] small fix --- synapse/rest/client/sync.py | 4 ++-- synapse/storage/databases/main/experimental_features.py | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index fc036c4dfa9b..03b05789456b 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -358,7 +358,7 @@ async def encode_joined( joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, time_now, joined=True, serialize_options=serialize_options, + room, time_now, joined=True, serialize_options=serialize_options ) return joined @@ -463,7 +463,7 @@ async def encode_archived( joined = {} for room in rooms: joined[room.room_id] = await self.encode_room( - room, time_now, joined=False, serialize_options=serialize_options, + room, time_now, joined=False, serialize_options=serialize_options ) return joined diff --git a/synapse/storage/databases/main/experimental_features.py b/synapse/storage/databases/main/experimental_features.py index 7a0a2caa0e27..d62da1160db9 100644 --- a/synapse/storage/databases/main/experimental_features.py +++ b/synapse/storage/databases/main/experimental_features.py @@ -30,7 +30,6 @@ class ExperimentalFeature(str, Enum): """ MSC3026 = "msc3026" - MSC2654 = "msc2654" MSC3881 = "msc3881" MSC3967 = "msc3967" From 86ec83456eb5dc127672d7deb371458f74c2238c Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 10 May 2023 09:40:35 -0700 Subject: [PATCH 20/20] Update changelog.d/15345.feature Co-authored-by: Patrick Cloke --- changelog.d/15345.feature | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/changelog.d/15345.feature b/changelog.d/15345.feature index 4662bb0fd382..834754a054db 100644 --- a/changelog.d/15345.feature +++ b/changelog.d/15345.feature @@ -1,3 +1 @@ -Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence), -MSC3881 (remotely toggle push notifications for another client), and MSC3967 (Do not require UIA when first uploading -cross signing keys) from the experimental config to per-user flags. +Follow-up to adding experimental feature flags per-user (#15344) which moves experimental features MSC3026 (busy presence), MSC3881 (remotely toggle push notifications for another client), and MSC3967 (Do not require UIA when first uploading cross signing keys) from the experimental config to per-user flags.