diff --git a/changelog.d/12918.bugfix b/changelog.d/12918.bugfix
new file mode 100644
index 0000000000..38bdd80700
--- /dev/null
+++ b/changelog.d/12918.bugfix
@@ -0,0 +1 @@
+Fix a bug introduced in Synapse 1.60.0rc1 that would break some imports from `synapse.module_api`.
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/synapse/events/spamcheck.py b/synapse/events/spamcheck.py
index 7984874e21..1048b4c825 100644
--- a/synapse/events/spamcheck.py
+++ b/synapse/events/spamcheck.py
@@ -30,7 +30,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,12 +46,9 @@ CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[
["synapse.events.EventBase"],
Awaitable[
Union[
- Allow,
- Codes,
+ str,
# Deprecated
bool,
- # Deprecated
- str,
]
],
]
@@ -178,6 +175,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()
@@ -268,9 +267,7 @@ class SpamChecker:
if check_media_file_for_spam is not None:
self._check_media_file_for_spam_callbacks.append(check_media_file_for_spam)
- async def check_event_for_spam(
- self, event: "synapse.events.EventBase"
- ) -> Union[Decision, str]:
+ async def check_event_for_spam(self, event: "synapse.events.EventBase") -> 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
@@ -281,22 +278,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, 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
@@ -304,13 +299,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( # type: ignore[unreachable]
+ "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`
- # or with a `Codes`. 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 cb1bc4c06f..22cdad3f33 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,
@@ -886,10 +885,22 @@ 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:
+ 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, 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 95f3b27927..c44e9da121 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`.
-"""
|
