Merge branch 'master' into develop

10 files changed, 91 insertions, 86 deletions

diff --git a/CHANGES.md b/CHANGES.md
index 40e362e920..2bf8cdea75 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,5 +1,5 @@
-Synapse 1.60.0rc2 (2022-05-27)
-==============================
+Synapse 1.60.0 (2022-05-31)
+===========================
 
 This release of Synapse adds a unique index to the `state_group_edges` table, in
 order to prevent accidentally introducing duplicate information (for example,
@@ -14,6 +14,15 @@ should update their modules to use the new signature where possible.
 See [the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/docs/upgrade.md#upgrading-to-v1600)
 for more details.
 
+Bugfixes
+--------
+
+- Fix a bug introduced in Synapse 1.60.0rc1 that would break some imports from `synapse.module_api`. ([\#12918](https://github.com/matrix-org/synapse/issues/12918))
+
+
+Synapse 1.60.0rc2 (2022-05-27)
+==============================
+
 Features
 --------
 
@@ -60,6 +69,7 @@ Bugfixes
 - Fix a bug introduced in Synapse 1.30.0 where empty rooms could be automatically created if a monthly active users limit is set. ([\#12713](https://github.com/matrix-org/synapse/issues/12713))
 - Fix push to dismiss notifications when read on another client. Contributed by @SpiritCroc @ Beeper. ([\#12721](https://github.com/matrix-org/synapse/issues/12721))
 - Fix poor database performance when reading the cache invalidation stream for large servers with lots of workers. ([\#12747](https://github.com/matrix-org/synapse/issues/12747))
+- Fix a long-standing bug where the user directory background process would fail to make forward progress if a user included a null codepoint in their display name or avatar. ([\#12762](https://github.com/matrix-org/synapse/issues/12762))
 - Delete events from the `federation_inbound_events_staging` table when a room is purged through the admin API. ([\#12770](https://github.com/matrix-org/synapse/issues/12770))
 - Give a meaningful error message when a client tries to create a room with an invalid alias localpart. ([\#12779](https://github.com/matrix-org/synapse/issues/12779))
 - Fix a bug introduced in 1.43.0 where a file (`providers.json`) was never closed. Contributed by @arkamar. ([\#12794](https://github.com/matrix-org/synapse/issues/12794))
@@ -115,7 +125,6 @@ Internal Changes
 - Drop the logging level of status messages for the URL preview cache expiry job from INFO to DEBUG. ([\#12720](https://github.com/matrix-org/synapse/issues/12720))
 - Downgrade some OIDC errors to warnings in the logs, to reduce the noise of Sentry reports. ([\#12723](https://github.com/matrix-org/synapse/issues/12723))
 - Update configs used by Complement to allow more invites/3PID validations during tests. ([\#12731](https://github.com/matrix-org/synapse/issues/12731))
-- Fix a long-standing bug where the user directory background process would fail to make forward progress if a user included a null codepoint in their display name or avatar. ([\#12762](https://github.com/matrix-org/synapse/issues/12762))
 - Tweak the mypy plugin so that `@cached` can accept `on_invalidate=None`. ([\#12769](https://github.com/matrix-org/synapse/issues/12769))
 - Move methods that call `add_push_rule` to the `PushRuleStore` class. ([\#12772](https://github.com/matrix-org/synapse/issues/12772))
 - Make handling of federation Authorization header (more) compliant with RFC7230. ([\#12774](https://github.com/matrix-org/synapse/issues/12774))
@@ -222,7 +231,7 @@ Deprecations and Removals
 -------------------------
 
 - Remove unstable identifiers from [MSC3069](https://github.com/matrix-org/matrix-doc/pull/3069). ([\#12596](https://github.com/matrix-org/synapse/issues/12596))
-- Remove the unspecified `m.login.jwt` login type and the unstable `uk.half-shot.msc2778.login.application_service` from 
+- Remove the unspecified `m.login.jwt` login type and the unstable `uk.half-shot.msc2778.login.application_service` from
   [MSC2778](https://github.com/matrix-org/matrix-doc/pull/2778). ([\#12597](https://github.com/matrix-org/synapse/issues/12597))
 - Synapse now requires at least Python 3.7.1 (up from 3.7.0), for compatibility with the latest Twisted trunk. ([\#12613](https://github.com/matrix-org/synapse/issues/12613))
 
diff --git a/debian/changelog b/debian/changelog
index b1e61e7c8a..8f08972743 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,10 +1,16 @@
-matrix-synapse-py3 (1.60.0~rc2+nmu1) UNRELEASED; urgency=medium
+matrix-synapse-py3 (1.61.0~rc1+nmu1) UNRELEASED; urgency=medium
 
   * Non-maintainer upload.
   * Remove unused `jitsimeetbridge` experiment from `contrib` directory.
 
  -- Synapse Packaging team <packages@matrix.org>  Sun, 29 May 2022 14:44:45 +0100
 
+matrix-synapse-py3 (1.60.0) stable; urgency=medium
+
+  * New Synapse release 1.60.0.
+
+ -- Synapse Packaging team <packages@matrix.org>  Tue, 31 May 2022 13:41:22 +0100
+
 matrix-synapse-py3 (1.60.0~rc2) stable; urgency=medium
 
   * New Synapse release 1.60.0rc2.
diff --git a/docs/modules/spam_checker_callbacks.md b/docs/modules/spam_checker_callbacks.md
index 71f6f9f0ab..ad35e667ed 100644
--- a/docs/modules/spam_checker_callbacks.md
+++ b/docs/modules/spam_checker_callbacks.md
@@ -11,29 +11,28 @@ The available spam checker callbacks are:
 ### `check_event_for_spam`
 
 _First introduced in Synapse v1.37.0_
-_Signature extended to support Allow and Code in Synapse v1.60.0_
-_Boolean and string return value types deprecated in Synapse v1.60.0_
+
+_Changed in Synapse v1.60.0: `synapse.module_api.NOT_SPAM` and `synapse.module_api.errors.Codes` can be returned by this callback. Returning a boolean or a string is now deprecated._ 
 
 ```python
-async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.ALLOW", "synapse.module_api.error.Codes", str, bool]
+async def check_event_for_spam(event: "synapse.module_api.EventBase") -> Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes", str, bool]
 ```
 
-Called when receiving an event from a client or via federation. The callback must return either:
-  - `synapse.module_api.ALLOW`, to allow the operation. Other callbacks
-    may still decide to reject it.
-  - `synapse.api.Codes` to reject the operation with an error code. In case
-    of doubt, `synapse.api.error.Codes.FORBIDDEN` is a good error code.
-  - (deprecated) a `str` to reject the operation and specify an error message. Note that clients
+Called when receiving an event from a client or via federation. The callback must return one of:
+  - `synapse.module_api.NOT_SPAM`, to allow the operation. Other callbacks may still 
+    decide to reject it.
+  - `synapse.module_api.errors.Codes` to reject the operation with an error code. In case
+    of doubt, `synapse.module_api.errors.Codes.FORBIDDEN` is a good error code.
+  - (deprecated) a non-`Codes` `str` to reject the operation and specify an error message. Note that clients
     typically will not localize the error message to the user's preferred locale.
-  - (deprecated) on `False`, behave as `ALLOW`. Deprecated as confusing, as some
-    callbacks in expect `True` to allow and others `True` to reject.
-  - (deprecated) on `True`, behave as `synapse.api.error.Codes.FORBIDDEN`. Deprecated as confusing, as
-    some callbacks in expect `True` to allow and others `True` to reject.
+  - (deprecated) `False`, which is the same as returning `synapse.module_api.NOT_SPAM`.
+  - (deprecated) `True`, which is the same as returning `synapse.module_api.errors.Codes.FORBIDDEN`.
 
 If multiple modules implement this callback, they will be considered in order. If a
-callback returns `synapse.module_api.ALLOW`, Synapse falls through to the next one. The value of the
-first callback that does not return `synapse.module_api.ALLOW` will be used. If this happens, Synapse
-will not call any of the subsequent implementations of this callback.
+callback returns `synapse.module_api.NOT_SPAM`, Synapse falls through to the next one.
+The value of the first callback that does not return `synapse.module_api.NOT_SPAM` will
+be used. If this happens, Synapse will not call any of the subsequent implementations of
+this callback.
 
 ### `user_may_join_room`
 
diff --git a/docs/upgrade.md b/docs/upgrade.md
index e7eadadb64..e3c64da17f 100644
--- a/docs/upgrade.md
+++ b/docs/upgrade.md
@@ -177,11 +177,11 @@ has queries that can be used to check a database for this problem in advance.
 
 </details>
 
-## SpamChecker API's `check_event_for_spam` has a new signature.
+## New signature for the spam checker callback `check_event_for_spam`
 
 The previous signature has been deprecated.
 
-Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they should now return `Union["synapse.module_api.Allow", "synapse.module_api.errors.Codes"]`.
+Whereas `check_event_for_spam` callbacks used to return `Union[str, bool]`, they should now return `Union["synapse.module_api.NOT_SPAM", "synapse.module_api.errors.Codes"]`.
 
 This is part of an ongoing refactoring of the SpamChecker API to make it less ambiguous and more powerful.
 
@@ -204,8 +204,8 @@ async def check_event_for_spam(event):
         # Event is spam, mark it as forbidden (you may use some more precise error
         # code if it is useful).
         return synapse.module_api.errors.Codes.FORBIDDEN
-    # Event is not spam, mark it as `ALLOW`.
-    return synapse.module_api.ALLOW
+    # Event is not spam, mark it as such.
+    return synapse.module_api.NOT_SPAM
 ```
 
 # Upgrading to v1.59.0
diff --git a/pyproject.toml b/pyproject.toml
index 59cff590b5..75251c863d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -54,7 +54,7 @@ skip_gitignore = true
 
 [tool.poetry]
 name = "matrix-synapse"
-version = "1.60.0rc2"
+version = "1.60.0"
 description = "Homeserver for the Matrix decentralised comms protocol"
 authors = ["Matrix.org Team and Contributors <packages@matrix.org>"]
 license = "Apache-2.0"
diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py
index 82998ca490..d2e06c754e 100644
--- a/synapse/events/spamcheck.py
+++ b/synapse/events/spamcheck.py
@@ -31,7 +31,7 @@ from typing import (
 from synapse.api.errors import Codes
 from synapse.rest.media.v1._base import FileInfo
 from synapse.rest.media.v1.media_storage import ReadableFileWrapper
-from synapse.spam_checker_api import Allow, Decision, RegistrationBehaviour
+from synapse.spam_checker_api import RegistrationBehaviour
 from synapse.types import RoomAlias, UserProfile
 from synapse.util.async_helpers import delay_cancellation, maybe_awaitable
 from synapse.util.metrics import Measure
@@ -46,7 +46,7 @@ CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[
     ["synapse.events.EventBase"],
     Awaitable[
         Union[
-            Allow,
+            str,
             Codes,
             # Highly experimental, not officially part of the spamchecker API, may
             # disappear without warning depending on the results of ongoing
@@ -55,8 +55,6 @@ CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[
             Tuple[Codes, Dict],
             # Deprecated
             bool,
-            # Deprecated
-            str,
         ]
     ],
 ]
@@ -183,6 +181,8 @@ def load_legacy_spam_checkers(hs: "synapse.server.HomeServer") -> None:
 
 
 class SpamChecker:
+    NOT_SPAM = "NOT_SPAM"
+
     def __init__(self, hs: "synapse.server.HomeServer") -> None:
         self.hs = hs
         self.clock = hs.get_clock()
@@ -275,7 +275,7 @@ class SpamChecker:
 
     async def check_event_for_spam(
         self, event: "synapse.events.EventBase"
-    ) -> Union[Decision, Tuple[Codes, Dict], str]:
+    ) -> Union[Tuple[Codes, Dict], str]:
         """Checks if a given event is considered "spammy" by this server.
 
         If the server considers an event spammy, then it will be rejected if
@@ -286,22 +286,20 @@ class SpamChecker:
             event: the event to be checked
 
         Returns:
-            - on `ALLOW`, the event is considered good (non-spammy) and should
-                be let through. Other spamcheck filters may still reject it.
-            - on `Code`, the event is considered spammy and is rejected with a specific
+            - `NOT_SPAM` if the event is considered good (non-spammy) and should be let
+                through. Other spamcheck filters may still reject it.
+            - A `Code` if the event is considered spammy and is rejected with a specific
                 error message/code.
-            - on `str`, the event is considered spammy and the string is used as error
-                message. This usage is generally discouraged as it doesn't support
-                internationalization.
+            - A string that isn't `NOT_SPAM` if the event is considered spammy and the
+                string should be used as the client-facing error message. This usage is
+                generally discouraged as it doesn't support internationalization.
         """
         for callback in self._check_event_for_spam_callbacks:
             with Measure(
                 self.clock, "{}.{}".format(callback.__module__, callback.__qualname__)
             ):
-                res: Union[
-                    Decision, Tuple[Codes, Dict], str, bool
-                ] = await delay_cancellation(callback(event))
-                if res is False or res is Allow.ALLOW:
+                res = await delay_cancellation(callback(event))
+                if res is False or res == self.NOT_SPAM:
                     # This spam-checker accepts the event.
                     # Other spam-checkers may reject it, though.
                     continue
@@ -309,14 +307,23 @@ class SpamChecker:
                     # This spam-checker rejects the event with deprecated
                     # return value `True`
                     return Codes.FORBIDDEN
+                elif not isinstance(res, str):
+                    # mypy complains that we can't reach this code because of the
+                    # return type in CHECK_EVENT_FOR_SPAM_CALLBACK, but we don't know
+                    # for sure that the module actually returns it.
+                    logger.warning(
+                        "Module returned invalid value, rejecting message as spam"
+                    )
+                    res = "This message has been rejected as probable spam"
                 else:
-                    # This spam-checker rejects the event either with a `str`,
-                    # with a `Codes` or with a `Tuple[Codes, Dict]`. In either
-                    # case, we stop here.
-                    return res
+                    # The module rejected the event either with a `Codes`
+                    # or some other `str`. In either case, we stop here.
+                    pass
+
+                return res
 
         # No spam-checker has rejected the event, let it pass.
-        return Allow.ALLOW
+        return self.NOT_SPAM
 
     async def should_drop_federated_event(
         self, event: "synapse.events.EventBase"
diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py
index 1e866b19d8..7bc54b9988 100644
--- a/synapse/federation/federation_base.py
+++ b/synapse/federation/federation_base.py
@@ -15,7 +15,6 @@
 import logging
 from typing import TYPE_CHECKING
 
-import synapse
 from synapse.api.constants import MAX_DEPTH, EventContentFields, EventTypes, Membership
 from synapse.api.errors import Codes, SynapseError
 from synapse.api.room_versions import EventFormatVersions, RoomVersion
@@ -101,7 +100,7 @@ class FederationBase:
 
         spam_check = await self.spam_checker.check_event_for_spam(pdu)
 
-        if spam_check is not synapse.spam_checker_api.Allow.ALLOW:
+        if spam_check != self.spam_checker.NOT_SPAM:
             logger.warning("Event contains spam, soft-failing %s", pdu.event_id)
             # we redact (to save disk space) as well as soft-failing (to stop
             # using the event in prev_events).
diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py
index f377769071..cf7c2d1979 100644
--- a/synapse/handlers/message.py
+++ b/synapse/handlers/message.py
@@ -23,7 +23,6 @@ from canonicaljson import encode_canonical_json
 
 from twisted.internet.interfaces import IDelayedCall
 
-import synapse
 from synapse import event_auth
 from synapse.api.constants import (
     EventContentFields,
@@ -897,11 +896,11 @@ class EventCreationHandler:
                 event.sender,
             )
 
-            spam_check = await self.spam_checker.check_event_for_spam(event)
-            if spam_check is not synapse.spam_checker_api.Allow.ALLOW:
-                if isinstance(spam_check, tuple):
+            spam_check_result = await self.spam_checker.check_event_for_spam(event)
+            if spam_check_result != self.spam_checker.NOT_SPAM:
+                if isinstance(spam_check_result, tuple):
                     try:
-                        [code, dict] = spam_check
+                        [code, dict] = spam_check_result
                         raise SynapseError(
                             403,
                             "This message had been rejected as probable spam",
@@ -911,11 +910,24 @@ class EventCreationHandler:
                     except ValueError:
                         logger.error(
                             "Spam-check module returned invalid error value. Expecting [code, dict], got %s",
-                            spam_check,
+                            spam_check_result,
                         )
-                        spam_check = Codes.FORBIDDEN
+                        spam_check_result = Codes.FORBIDDEN
+
+                if isinstance(spam_check_result, Codes):
+                    raise SynapseError(
+                        403,
+                        "This message has been rejected as probable spam",
+                        spam_check_result,
+                    )
+
+                # Backwards compatibility: if the return value is not an error code, it
+                # means the module returned an error message to be included in the
+                # SynapseError (which is now deprecated).
                 raise SynapseError(
-                    403, "This message had been rejected as probable spam", spam_check
+                    403,
+                    spam_check_result,
+                    Codes.FORBIDDEN,
                 )
 
             ev = await self.handle_new_client_event(
diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py
index 6668f64c90..b7451fc870 100644
--- a/synapse/module_api/__init__.py
+++ b/synapse/module_api/__init__.py
@@ -35,7 +35,6 @@ from typing_extensions import ParamSpec
 from twisted.internet import defer
 from twisted.web.resource import Resource
 
-from synapse import spam_checker_api
 from synapse.api.errors import SynapseError
 from synapse.events import EventBase
 from synapse.events.presence_router import (
@@ -55,6 +54,7 @@ from synapse.events.spamcheck import (
     USER_MAY_JOIN_ROOM_CALLBACK,
     USER_MAY_PUBLISH_ROOM_CALLBACK,
     USER_MAY_SEND_3PID_INVITE_CALLBACK,
+    SpamChecker,
 )
 from synapse.events.third_party_rules import (
     CHECK_CAN_DEACTIVATE_USER_CALLBACK,
@@ -140,9 +140,7 @@ are loaded into Synapse.
 """
 
 PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS
-
-ALLOW = spam_checker_api.Allow.ALLOW
-# Singleton value used to mark a message as permitted.
+NOT_SPAM = SpamChecker.NOT_SPAM
 
 __all__ = [
     "errors",
@@ -151,7 +149,7 @@ __all__ = [
     "respond_with_html",
     "run_in_background",
     "cached",
-    "Allow",
+    "NOT_SPAM",
     "UserID",
     "DatabasePool",
     "LoggingTransaction",
diff --git a/synapse/spam_checker_api/__init__.py b/synapse/spam_checker_api/__init__.py
index 95132c80b7..75578270ac 100644
--- a/synapse/spam_checker_api/__init__.py
+++ b/synapse/spam_checker_api/__init__.py
@@ -12,9 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 from enum import Enum
-from typing import Union
-
-from synapse.api.errors import Codes
 
 
 class RegistrationBehaviour(Enum):
@@ -25,25 +22,3 @@ class RegistrationBehaviour(Enum):
     ALLOW = "allow"
     SHADOW_BAN = "shadow_ban"
     DENY = "deny"
-
-
-# We define the following singleton enum rather than a string to be able to
-# write `Union[Allow, ..., str]` in some of the callbacks for the spam-checker
-# API, where the `str` is required to maintain backwards compatibility with
-# previous versions of the API.
-class Allow(Enum):
-    """
-    Singleton to allow events to pass through in SpamChecker APIs.
-    """
-
-    ALLOW = "allow"
-
-
-Decision = Union[Allow, Codes]
-"""
-Union to define whether a request should be allowed or rejected.
-
-To accept a request, return `ALLOW`.
-
-To reject a request without any specific information, use `Codes.FORBIDDEN`.
-"""