From aa348ce4793935f6ef4cd979629f6d5128ecb42f Mon Sep 17 00:00:00 2001 From: Hugh Nimmo-Smith Date: Fri, 15 May 2026 17:06:26 +0100 Subject: [PATCH] Update for MSC4335 landing in the spec --- changelog.d/18876.feature | 2 +- .../configuration/config_documentation.md | 9 +- schema/synapse-config.schema.yaml | 20 +- synapse/api/errors.py | 12 +- synapse/config/repository.py | 28 ++- synapse/media/media_repository.py | 29 +-- .../callbacks/media_repository_callbacks.py | 5 +- synapse/rest/admin/experimental_features.py | 3 - tests/rest/client/test_media.py | 194 +++++++----------- 9 files changed, 129 insertions(+), 173 deletions(-) diff --git a/changelog.d/18876.feature b/changelog.d/18876.feature index a976987a56..b068068b74 100644 --- a/changelog.d/18876.feature +++ b/changelog.d/18876.feature @@ -1 +1 @@ -Add support for experimental [MSC4335](https://github.com/matrix-org/matrix-spec-proposals/pull/4335) M_USER_LIMIT_EXCEEDED error code for media upload limits. +Return M_USER_LIMIT_EXCEEDED error code for media upload limits from [MSC4335](https://github.com/matrix-org/matrix-spec-proposals/pull/4335). diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md index 0d93a9f450..4aa44eeefd 100644 --- a/docs/usage/configuration/config_documentation.md +++ b/docs/usage/configuration/config_documentation.md @@ -2215,19 +2215,20 @@ Options for each entry include: * `max_size` (byte size): Amount of data that can be uploaded in the time period by the user. Required. -* `msc4335_info_uri` (string): Experimental MSC4335 URI to where the user can find information about the upload limit. Optional. +* `info_uri` (string): URI return to the client for where the user can find information about the upload limit. Optional. If not set then a static `data:text/html` URI is returned with a simple message. It is recommended to provide an `info_uri` that points to a page with more information about the upload limit and how users can reduce their upload usage or request an upload limit increase. -* `msc4335_can_upgrade` (boolean): Experimental MSC4335 value to say if the limit can be increased. Optional. +* `can_upgrade` (boolean): Value returned to the client for whether the limit can be increased. Optional default `false`. Defaults to `false`. Example configuration: ```yaml media_upload_limits: - time_period: 1h max_size: 100M + info_uri: https://example.com/quota#hour - time_period: 1w max_size: 500M - msc4335_info_uri: https://example.com/quota - msc4335_can_upgrade: true + info_uri: https://example.com/quota + can_upgrade: true ``` --- ### `max_image_pixels` diff --git a/schema/synapse-config.schema.yaml b/schema/synapse-config.schema.yaml index f24679c9bc..51388b7899 100644 --- a/schema/synapse-config.schema.yaml +++ b/schema/synapse-config.schema.yaml @@ -2475,21 +2475,29 @@ properties: description: >- Amount of data that can be uploaded in the time period by the user. Required. - msc4335_info_uri: + info_uri: type: string description: >- - Experimental MSC4335 URI to where the user can find information about the upload limit. Optional. - msc4335_can_upgrade: + URI return to the client for where the user can find information + about the upload limit. Optional. + If not set then a static `data:text/html` URI is returned with a + simple message. + It is recommended to provide an `info_uri` that points to a page + with more information about the upload limit and how users can + reduce their upload usage or request an upload limit increase. + can_upgrade: type: boolean description: >- - Experimental MSC4335 value to say if the limit can be increased. Optional. + Value returned to the client for whether the limit can be increased. Optional default `false`. + default: false examples: - - time_period: 1h max_size: 100M + info_uri: https://example.com/quota#hour - time_period: 1w max_size: 500M - msc4335_info_uri: https://example.com/quota - msc4335_can_upgrade: true + info_uri: https://example.com/quota + can_upgrade: true max_image_pixels: $ref: "#/$defs/bytes" description: Maximum number of pixels that will be thumbnailed. diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 841b68b7c2..6848525288 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -152,7 +152,7 @@ class Codes(str, Enum): # Part of MSC4326 UNKNOWN_DEVICE = "ORG.MATRIX.MSC4326.M_UNKNOWN_DEVICE" - MSC4335_USER_LIMIT_EXCEEDED = "ORG.MATRIX.MSC4335_USER_LIMIT_EXCEEDED" + USER_LIMIT_EXCEEDED = "M_USER_LIMIT_EXCEEDED" class CodeMessageException(RuntimeError): @@ -515,9 +515,9 @@ class ResourceLimitError(SynapseError): ) -class MSC4335UserLimitExceededError(SynapseError): +class UserLimitExceededError(SynapseError): """ - Experimental implementation of MSC4335 M_USER_LIMIT_EXCEEDED error + Implementation of M_USER_LIMIT_EXCEEDED error """ def __init__( @@ -528,16 +528,16 @@ class MSC4335UserLimitExceededError(SynapseError): can_upgrade: bool = False, ): additional_fields: dict[str, Union[str, bool]] = { - "org.matrix.msc4335.info_uri": info_uri, + "info_uri": info_uri, } if can_upgrade: - additional_fields["org.matrix.msc4335.can_upgrade"] = can_upgrade + additional_fields["can_upgrade"] = can_upgrade super().__init__( code, msg, - Codes.MSC4335_USER_LIMIT_EXCEEDED, + Codes.USER_LIMIT_EXCEEDED, additional_fields=additional_fields, ) diff --git a/synapse/config/repository.py b/synapse/config/repository.py index a9411a0258..6058eaf4de 100644 --- a/synapse/config/repository.py +++ b/synapse/config/repository.py @@ -21,7 +21,7 @@ import logging import os -from typing import Any, Optional +from typing import Any import attr @@ -134,11 +134,11 @@ class MediaUploadLimit: time_period_ms: int """The time period in milliseconds.""" - msc4335_info_uri: Optional[str] = None - """Used for experimental MSC4335 error code feature""" + info_uri: str + """The URI to return with the M_USER_LIMIT_EXCEEDED error.""" - msc4335_can_upgrade: Optional[bool] = None - """Used for experimental MSC4335 error code feature""" + can_upgrade: bool = False + """Whether the user can upgrade their plan to increase the limit. This is returned in the M_USER_LIMIT_EXCEEDED error.""" class ContentRepositoryConfig(Config): @@ -314,23 +314,21 @@ class ContentRepositoryConfig(Config): for limit_config in config.get("media_upload_limits", []): time_period_ms = self.parse_duration(limit_config["time_period"]) max_bytes = self.parse_size(limit_config["max_size"]) - msc4335_info_uri = limit_config.get("msc4335_info_uri", None) - msc4335_can_upgrade = limit_config.get("msc4335_can_upgrade", None) + info_uri = limit_config.get("info_uri", "") + can_upgrade = bool(limit_config.get("can_upgrade", False)) - if (msc4335_info_uri is not None or msc4335_can_upgrade is not None) and ( - not (msc4335_info_uri and msc4335_can_upgrade is not None) - ): - raise ConfigError( - "If any of msc4335_info_uri or msc4335_can_upgrade are set, then both msc4335_info_uri and " - "msc4335_can_upgrade must be set." + if info_uri == "": + logger.warning( + "Empty info_uri provided for media upload limit, using static fallback value instead. You should specify an info_uri that points to more information about the upload limits imposed." ) + info_uri = "data:text/html,

You have exceeded a media upload limit. Ask your server administrator for more information.

" self.media_upload_limits.append( MediaUploadLimit( max_bytes, time_period_ms, - msc4335_info_uri, - msc4335_can_upgrade, + info_uri, + can_upgrade, ) ) diff --git a/synapse/media/media_repository.py b/synapse/media/media_repository.py index 52ccc8c8ff..391d70c50a 100644 --- a/synapse/media/media_repository.py +++ b/synapse/media/media_repository.py @@ -37,10 +37,10 @@ from synapse.api.errors import ( Codes, FederationDeniedError, HttpResponseException, - MSC4335UserLimitExceededError, NotFoundError, RequestSendFailed, SynapseError, + UserLimitExceededError, cs_error, ) from synapse.api.ratelimiting import Ratelimiter @@ -71,7 +71,6 @@ from synapse.media.storage_provider import ( ) from synapse.media.thumbnailer import Thumbnailer, ThumbnailError from synapse.media.url_previewer import UrlPreviewer -from synapse.rest.admin.experimental_features import ExperimentalFeature from synapse.storage.databases.main.media_repository import LocalMedia, RemoteMedia from synapse.types import UserID from synapse.util.async_helpers import Linearizer @@ -398,26 +397,12 @@ class MediaRepository: sent_bytes=uploaded_media_size, attempted_bytes=content_length, ) - # If the MSC4335 experimental feature is enabled and the media limit - # has the info_uri configured then we raise the MSC4335 error - msc4335_enabled = await self.store.is_feature_enabled( - auth_user.to_string(), ExperimentalFeature.MSC4335 - ) - if ( - msc4335_enabled - and limit.msc4335_info_uri - and limit.msc4335_can_upgrade is not None - ): - raise MSC4335UserLimitExceededError( - 403, - "Media upload limit exceeded", - limit.msc4335_info_uri, - limit.msc4335_can_upgrade, - ) - # Otherwise we use the current behaviour albeit not spec compliant - # See: https://github.com/element-hq/synapse/issues/18749 - raise SynapseError( - 400, "Media upload limit exceeded", Codes.RESOURCE_LIMIT_EXCEEDED + + raise UserLimitExceededError( + 403, + "Media upload limit exceeded", + limit.info_uri, + limit.can_upgrade, ) if is_new_media: diff --git a/synapse/module_api/callbacks/media_repository_callbacks.py b/synapse/module_api/callbacks/media_repository_callbacks.py index f1e6ea4c38..5b58229b77 100644 --- a/synapse/module_api/callbacks/media_repository_callbacks.py +++ b/synapse/module_api/callbacks/media_repository_callbacks.py @@ -150,7 +150,10 @@ class MediaRepositoryModuleApiCallbacks: ): # Use a copy of the data in case the module modifies it limit_copy = MediaUploadLimit( - max_bytes=limit.max_bytes, time_period_ms=limit.time_period_ms + max_bytes=limit.max_bytes, + time_period_ms=limit.time_period_ms, + info_uri=limit.info_uri, + can_upgrade=limit.can_upgrade, ) await delay_cancellation( callback(user_id, limit_copy, sent_bytes, attempted_bytes) diff --git a/synapse/rest/admin/experimental_features.py b/synapse/rest/admin/experimental_features.py index 652bf6e1fd..abdb937793 100644 --- a/synapse/rest/admin/experimental_features.py +++ b/synapse/rest/admin/experimental_features.py @@ -44,7 +44,6 @@ class ExperimentalFeature(str, Enum): MSC3881 = "msc3881" MSC3575 = "msc3575" MSC4222 = "msc4222" - MSC4335 = "msc4335" def is_globally_enabled(self, config: "HomeServerConfig") -> bool: if self is ExperimentalFeature.MSC3881: @@ -53,8 +52,6 @@ class ExperimentalFeature(str, Enum): return config.experimental.msc3575_enabled if self is ExperimentalFeature.MSC4222: return config.experimental.msc4222_enabled - if self is ExperimentalFeature.MSC4335: - return config.experimental.msc4335_enabled assert_never(self) diff --git a/tests/rest/client/test_media.py b/tests/rest/client/test_media.py index 273e0e2dd4..b79153f7e3 100644 --- a/tests/rest/client/test_media.py +++ b/tests/rest/client/test_media.py @@ -44,9 +44,8 @@ from twisted.web.http_headers import Headers from twisted.web.iweb import UNKNOWN_LENGTH, IResponse from twisted.web.resource import Resource -from synapse.api.errors import Codes, HttpResponseException +from synapse.api.errors import HttpResponseException from synapse.api.ratelimiting import Ratelimiter -from synapse.config import ConfigError from synapse.config._base import Config from synapse.config.homeserver import HomeServerConfig from synapse.config.oembed import OEmbedEndpointConfig @@ -2938,8 +2937,16 @@ class MediaUploadLimits(unittest.HomeserverTestCase): # These are the limits that we are testing unless overridden if config.get("media_upload_limits") is None: config["media_upload_limits"] = [ - {"time_period": "1d", "max_size": "1K"}, - {"time_period": "1w", "max_size": "3K"}, + { + "time_period": "1d", + "max_size": "1K", + "info_uri": "https://example.com/limits#daily", + }, + { + "time_period": "1w", + "max_size": "3K", + "info_uri": "https://example.com/limits#weekly", + }, ] return self.setup_test_homeserver(config=config) @@ -2978,7 +2985,7 @@ class MediaUploadLimits(unittest.HomeserverTestCase): self.assertEqual(channel.code, 200) channel = self.upload_media(800) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 403) def test_under_daily_limit(self) -> None: """Test that uploading media under the daily limit fails.""" @@ -3016,7 +3023,7 @@ class MediaUploadLimits(unittest.HomeserverTestCase): # This will fail as the weekly limit has been exceeded channel = self.upload_media(900) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 403) # Reset the weekly limit by advancing a week self.reactor.advance(7 * 60 * 60 * 24) # Advance by 7 days @@ -3025,12 +3032,10 @@ class MediaUploadLimits(unittest.HomeserverTestCase): channel = self.upload_media(900) self.assertEqual(channel.code, 200) - def test_msc4335_requires_config(self) -> None: + def test_logs_warning_with_no_info_uri_in_config(self) -> None: config_dict = default_config("test") - # msc4335_info_uri and msc4335_can_upgrade are required - - with self.assertRaises(ConfigError): + with self.assertLogs("synapse.config.repository", level="WARNING"): HomeServerConfig().parse_config_dict( { "media_upload_limits": [ @@ -3046,111 +3051,71 @@ class MediaUploadLimits(unittest.HomeserverTestCase): "", ) - with self.assertRaises(ConfigError): - HomeServerConfig().parse_config_dict( - { - "media_upload_limits": [ - { - "time_period": "1d", - "max_size": "1K", - "msc4335_can_upgrade": False, - } - ], - **config_dict, - }, - "", - "", - ) - - with self.assertRaises(ConfigError): - HomeServerConfig().parse_config_dict( - { - "media_upload_limits": [ - { - "time_period": "1d", - "max_size": "1K", - "msc4335_can_upgrade": True, - } - ], - **config_dict, - }, - "", - "", - ) - @override_config( { - "experimental_features": {"msc4335_enabled": True}, "media_upload_limits": [ { "time_period": "1d", "max_size": "1K", - "msc4335_info_uri": "https://example.com", - "msc4335_can_upgrade": False, + "info_uri": "https://example.com", + "can_upgrade": False, } ], } ) - def test_msc4335_returns_hard_user_limit_exceeded(self) -> None: - """Test that the MSC4335 error is returned with can_upgrade False when experimental feature is enabled.""" + def test_returns_hard_user_limit_exceeded(self) -> None: + """Test that the error is returned with can_upgrade False.""" channel = self.upload_media(500) self.assertEqual(channel.code, 200) channel = self.upload_media(800) self.assertEqual(channel.code, 403) - self.assertEqual( - channel.json_body["errcode"], "ORG.MATRIX.MSC4335_USER_LIMIT_EXCEEDED" - ) - self.assertEqual( - channel.json_body["org.matrix.msc4335.info_uri"], "https://example.com" - ) + self.assertEqual(channel.json_body["errcode"], "M_USER_LIMIT_EXCEEDED") + self.assertEqual(channel.json_body["info_uri"], "https://example.com") @override_config( { - "experimental_features": {"msc4335_enabled": True}, "media_upload_limits": [ { "time_period": "1d", "max_size": "1K", - "msc4335_info_uri": "https://example.com", - "msc4335_can_upgrade": True, + "info_uri": "https://example.com", } ], } ) - def test_msc4335_returns_soft_user_limit_exceeded(self) -> None: + def test_returns_hard_user_limit_exceeded_by_default(self) -> None: + """Test that the error is returned with can_upgrade False by default.""" + channel = self.upload_media(500) + self.assertEqual(channel.code, 200) + + channel = self.upload_media(800) + self.assertEqual(channel.code, 403) + self.assertEqual(channel.json_body["errcode"], "M_USER_LIMIT_EXCEEDED") + self.assertEqual(channel.json_body["info_uri"], "https://example.com") + + @override_config( + { + "media_upload_limits": [ + { + "time_period": "1d", + "max_size": "1K", + "info_uri": "https://example.com", + "can_upgrade": True, + } + ], + } + ) + def test_returns_soft_user_limit_exceeded(self) -> None: """Test that the MSC4335 error is returned with can_upgrade True when experimental feature is enabled.""" channel = self.upload_media(500) self.assertEqual(channel.code, 200) channel = self.upload_media(800) self.assertEqual(channel.code, 403) - self.assertEqual( - channel.json_body["errcode"], "ORG.MATRIX.MSC4335_USER_LIMIT_EXCEEDED" - ) - self.assertEqual( - channel.json_body["org.matrix.msc4335.info_uri"], "https://example.com" - ) - self.assertEqual(channel.json_body["org.matrix.msc4335.can_upgrade"], True) - - @override_config( - { - "experimental_features": {"msc4335_enabled": True}, - "media_upload_limits": [ - { - "time_period": "1d", - "max_size": "1K", - } - ], - } - ) - def test_msc4335_requires_info_uri(self) -> None: - """Test that the MSC4335 error is not used if info_uri is not provided.""" - channel = self.upload_media(500) - self.assertEqual(channel.code, 200) - - channel = self.upload_media(800) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.json_body["errcode"], "M_USER_LIMIT_EXCEEDED") + self.assertEqual(channel.json_body["info_uri"], "https://example.com") + self.assertEqual(channel.json_body["can_upgrade"], True) class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): @@ -3186,8 +3151,16 @@ class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): # default limits to use if config.get("media_upload_limits") is None: config["media_upload_limits"] = [ - {"time_period": "1d", "max_size": "1K"}, - {"time_period": "1w", "max_size": "3K"}, + { + "time_period": "1d", + "max_size": "1K", + "info_uri": "https://example.com/limits#daily", + }, + { + "time_period": "1w", + "max_size": "3K", + "info_uri": "https://example.com/limits#weekly", + }, ] return self.setup_test_homeserver(config=config) @@ -3201,10 +3174,14 @@ class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): # n.b. we return these in increasing duration order and Synapse will need to sort them correctly return [ MediaUploadLimit( - time_period_ms=Config.parse_duration("1d"), max_bytes=5000 + time_period_ms=Config.parse_duration("1d"), + max_bytes=5000, + info_uri="https://override.example.com/limits#daily", ), MediaUploadLimit( - time_period_ms=Config.parse_duration("1w"), max_bytes=15000 + time_period_ms=Config.parse_duration("1w"), + max_bytes=15000, + info_uri="https://override.example.com/limits#weekly", ), ] # user2 has no limits @@ -3285,13 +3262,16 @@ class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): # User 1 attempts to upload 4000 bytes taking it over the limit channel = self.upload_media(4000, self.tok1) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 403) assert self.last_media_upload_limit_exceeded is not None self.assertEqual(self.last_media_upload_limit_exceeded["user_id"], self.user1) self.assertEqual( self.last_media_upload_limit_exceeded["limit"], MediaUploadLimit( - max_bytes=5000, time_period_ms=Config.parse_duration("1d") + max_bytes=5000, + time_period_ms=Config.parse_duration("1d"), + info_uri="https://override.example.com/limits#daily", + can_upgrade=False, ), ) self.assertEqual(self.last_media_upload_limit_exceeded["sent_bytes"], 3000) @@ -3300,13 +3280,16 @@ class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): # User 1 attempts to upload 20000 bytes which is over the weekly limit # This tests that the limits have been sorted as expected channel = self.upload_media(20000, self.tok1) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 403) assert self.last_media_upload_limit_exceeded is not None self.assertEqual(self.last_media_upload_limit_exceeded["user_id"], self.user1) self.assertEqual( self.last_media_upload_limit_exceeded["limit"], MediaUploadLimit( - max_bytes=15000, time_period_ms=Config.parse_duration("1w") + max_bytes=15000, + time_period_ms=Config.parse_duration("1w"), + info_uri="https://override.example.com/limits#weekly", + can_upgrade=False, ), ) self.assertEqual(self.last_media_upload_limit_exceeded["sent_bytes"], 3000) @@ -3329,36 +3312,17 @@ class MediaUploadLimitsModuleOverrides(unittest.HomeserverTestCase): # User 3 uploads 800 bytes which is over the limit channel = self.upload_media(800, self.tok3) - self.assertEqual(channel.code, 400) + self.assertEqual(channel.code, 403) assert self.last_media_upload_limit_exceeded is not None self.assertEqual(self.last_media_upload_limit_exceeded["user_id"], self.user3) self.assertEqual( self.last_media_upload_limit_exceeded["limit"], MediaUploadLimit( - max_bytes=1024, time_period_ms=Config.parse_duration("1d") + max_bytes=1024, + time_period_ms=Config.parse_duration("1d"), + info_uri="https://example.com/limits#daily", + can_upgrade=False, ), ) self.assertEqual(self.last_media_upload_limit_exceeded["sent_bytes"], 500) self.assertEqual(self.last_media_upload_limit_exceeded["attempted_bytes"], 800) - - @override_config( - { - "media_upload_limits": [ - { - "time_period": "1d", - "max_size": "1K", - "msc4335_info_uri": "https://example.com", - "msc4335_can_upgrade": False, - }, - ] - } - ) - def test_msc4335_defaults_disabled(self) -> None: - """Test that the MSC4335 is not used unless experimental feature is enabled.""" - channel = self.upload_media(500, self.tok3) - self.assertEqual(channel.code, 200) - - channel = self.upload_media(800, self.tok3) - # n.b. this response is not spec compliant as described at: https://github.com/element-hq/synapse/issues/18749 - self.assertEqual(channel.code, 400) - self.assertEqual(channel.json_body["errcode"], Codes.RESOURCE_LIMIT_EXCEEDED)