Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Move experimental features MSC3026, MSC3881, and MSC3967 to per-user flags #15345

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
fea933f
move experimental feature msc3026 (busy presence) to per-user flag
H-Shay Mar 28, 2023
0d61d3d
move msc3967 (Do not require UIA when first uploading cross signing k…
H-Shay Mar 28, 2023
1739ce6
move experimental feature msc3881 (remotely toggle push) to per-user …
H-Shay Mar 28, 2023
4aea2de
move experimental feature msc2654 (unread counts) to per-user flag
H-Shay Mar 28, 2023
4291c66
newsfragment
H-Shay Mar 28, 2023
d3cc11d
forgot to lint
H-Shay Mar 28, 2023
ca3e15b
add experimental features store to worker store
H-Shay Mar 29, 2023
51769a9
re-add parameters to test
H-Shay Mar 29, 2023
f9e7a0a
add a db function to tell if just one feature is enabled
H-Shay May 1, 2023
842eb40
update tests to use enum
H-Shay May 1, 2023
e5f33c5
fall back to default config setting if not enabled in table
H-Shay May 2, 2023
15dd372
stupid github web editor
H-Shay May 2, 2023
e53a8a5
change how config is checked
H-Shay May 2, 2023
e8c571b
remove support for per-user msc2654
H-Shay May 2, 2023
aea7cbd
move ExperimentalFeature definition to avoid circular import
H-Shay May 10, 2023
e156b84
consolidate logic checking config and db to one place
H-Shay May 10, 2023
7568c72
test activation both by config and admin api
H-Shay May 10, 2023
9155b82
remove changes to sync
H-Shay May 10, 2023
2cb4182
Merge branch 'develop' into shay/experimental_flags_part_2
H-Shay May 10, 2023
a767f1c
small fix
H-Shay May 10, 2023
b99dab3
Merge branch 'shay/experimental_flags_part_2' of https://github.com/m…
H-Shay May 10, 2023
86ec834
Update changelog.d/15345.feature
H-Shay May 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15345.feature
Original file line number Diff line number Diff line change
@@ -0,0 +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.
2 changes: 2 additions & 0 deletions synapse/app/generic_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -146,6 +147,7 @@ class GenericWorkerSlavedStore(
TransactionWorkerStore,
LockStore,
SessionStore,
ExperimentalFeaturesStore,
):
# Properties that multiple storage classes define. Tell mypy what the
# expected type is.
Expand Down
19 changes: 12 additions & 7 deletions synapse/handlers/presence.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -148,8 +149,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}

Expand Down Expand Up @@ -422,8 +421,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",
Expand Down Expand Up @@ -609,8 +606,12 @@ async def set_state(
PresenceState.BUSY,
)

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 (
presence == PresenceState.BUSY and not self._busy_presence_enabled
presence == PresenceState.BUSY and not busy_presence_enabled
):
raise SynapseError(400, "Invalid presence state")

Expand Down Expand Up @@ -1238,8 +1239,12 @@ async def set_state(
PresenceState.BUSY,
)

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 (
presence == PresenceState.BUSY and not self._busy_presence_enabled
presence == PresenceState.BUSY and not busy_presence_enabled
):
raise SynapseError(400, "Invalid presence state")

Expand All @@ -1257,7 +1262,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()

Expand Down
12 changes: 1 addition & 11 deletions synapse/rest/admin/experimental_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,20 @@
# limitations under the License.


from enum import Enum
from http import HTTPStatus
from typing import TYPE_CHECKING, Dict, Tuple

from synapse.api.errors import SynapseError
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):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved to the experimental features database class to avoid a circular import that occurred when using this type in the presence handler.

"""
Currently supported per-user features
"""

MSC3026 = "msc3026"
MSC3881 = "msc3881"
MSC3967 = "msc3967"


class ExperimentalFeaturesRestServlet(RestServlet):
"""
Enable or disable experimental features for a user or determine which features are enabled
Expand Down
7 changes: 6 additions & 1 deletion synapse/rest/client/keys.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -375,7 +376,11 @@ 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:
msc3967_enabled = await self.hs.get_datastores().main.get_feature_enabled(
user_id, ExperimentalFeature.MSC3967
)

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(
Expand Down
15 changes: 11 additions & 4 deletions synapse/rest/client/pusher.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -42,7 +43,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)
Expand All @@ -54,8 +54,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(), ExperimentalFeature.MSC3881
)

for pusher in pusher_dicts:
if self._msc3881_enabled:
if msc3881_enabled:
pusher["org.matrix.msc3881.enabled"] = pusher["enabled"]
pusher["org.matrix.msc3881.device_id"] = pusher["device_id"]
del pusher["enabled"]
Expand All @@ -73,7 +77,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)
Expand Down Expand Up @@ -113,7 +116,11 @@ 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:
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:
enabled = content["org.matrix.msc3881.enabled"]

if not append:
Expand Down
4 changes: 2 additions & 2 deletions synapse/rest/client/versions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines 98 to +99
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is really accurate -- don't we need to check this based on which user is requesting /versions?

For example, a client might use this to enable UI allowing users to say they're busy. The user would then receive an unexpected error when attempting to use that feature.

(Similar feedback to below for MSC3881)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think you're right - it would be True or False based on either the config or whether the feature is enabled in the DB, but how would that be checked here per user? Is it possible to extract the username from the GET request to /versions (my suspicion is no?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind I figured it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I thought I figured it out but since /versions doesn't require authentication there's no access token to extract the user id from. Is there a way to figure out the user id from a request that doesn't require authentication? If not, what's the best way to handle what /versions should return here if we can't determine the user (and thus cannot check if it is enabled per user)?

Copy link
Contributor

@reivilibre reivilibre May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I can think of is to make it optionally accept authentication and ask the clients to pretty please change their request code so it sends auth for /versions if they want to use unstable features... :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if we should start putting unstable features in /capabilities API (which is authed) instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if we should start putting unstable features in /capabilities API (which is authed) instead?

I think at some point I was told to prefer /versions over /capabilities, but after some searching I can't seem to find a reference to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to find a reference to this.

It's in the spec: https://spec.matrix.org/v1.6/client-server-api/#capabilities-negotiation
The relevant bit is:

This system should not be used to advertise unstable or experimental features - this is better done by the /versions endpoint.

Given this it seems like Oliver's suggestion is the way to go? Any other suggestions I should consider?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this it seems like Oliver's suggestion is the way to go? Any other suggestions I should consider?

I'd probably double check if clients would be willing to do this first?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSC4026 was written for this, it has now passed FCP.

# 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
Expand All @@ -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
Expand Down
50 changes: 49 additions & 1 deletion synapse/storage/databases/main/experimental_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
MSC3881 = "msc3881"
MSC3967 = "msc3967"


class ExperimentalFeaturesStore(CacheInvalidationWorkerStore):
def __init__(
self,
Expand Down Expand Up @@ -73,3 +83,41 @@ 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: "ExperimentalFeature"
) -> bool:
"""
Checks to see if a given feature is enabled for the user

Args:
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},
["enabled"],
allow_none=True,
)

# None and false are treated the same
db_enabled = bool(res)

return db_enabled
Loading