Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>tags/v1.60.0
@@ -0,0 +1 @@ | |||||
Fix a bug introduced in Synapse 1.60.0rc1 that would break some imports from `synapse.module_api`. |
@@ -11,29 +11,28 @@ The available spam checker callbacks are: | |||||
### `check_event_for_spam` | ### `check_event_for_spam` | ||||
_First introduced in Synapse v1.37.0_ | _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 | ```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. | 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 | 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` | ### `user_may_join_room` | ||||
@@ -177,11 +177,11 @@ has queries that can be used to check a database for this problem in advance. | |||||
</details> | </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. | 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. | 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 | # Event is spam, mark it as forbidden (you may use some more precise error | ||||
# code if it is useful). | # code if it is useful). | ||||
return synapse.module_api.errors.Codes.FORBIDDEN | 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 | # Upgrading to v1.59.0 | ||||
@@ -30,7 +30,7 @@ from typing import ( | |||||
from synapse.api.errors import Codes | from synapse.api.errors import Codes | ||||
from synapse.rest.media.v1._base import FileInfo | from synapse.rest.media.v1._base import FileInfo | ||||
from synapse.rest.media.v1.media_storage import ReadableFileWrapper | 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.types import RoomAlias, UserProfile | ||||
from synapse.util.async_helpers import delay_cancellation, maybe_awaitable | from synapse.util.async_helpers import delay_cancellation, maybe_awaitable | ||||
from synapse.util.metrics import Measure | from synapse.util.metrics import Measure | ||||
@@ -46,12 +46,9 @@ CHECK_EVENT_FOR_SPAM_CALLBACK = Callable[ | |||||
["synapse.events.EventBase"], | ["synapse.events.EventBase"], | ||||
Awaitable[ | Awaitable[ | ||||
Union[ | Union[ | ||||
Allow, | |||||
Codes, | |||||
str, | |||||
# Deprecated | # Deprecated | ||||
bool, | bool, | ||||
# Deprecated | |||||
str, | |||||
] | ] | ||||
], | ], | ||||
] | ] | ||||
@@ -178,6 +175,8 @@ def load_legacy_spam_checkers(hs: "synapse.server.HomeServer") -> None: | |||||
class SpamChecker: | class SpamChecker: | ||||
NOT_SPAM = "NOT_SPAM" | |||||
def __init__(self, hs: "synapse.server.HomeServer") -> None: | def __init__(self, hs: "synapse.server.HomeServer") -> None: | ||||
self.hs = hs | self.hs = hs | ||||
self.clock = hs.get_clock() | self.clock = hs.get_clock() | ||||
@@ -268,9 +267,7 @@ class SpamChecker: | |||||
if check_media_file_for_spam is not None: | if check_media_file_for_spam is not None: | ||||
self._check_media_file_for_spam_callbacks.append(check_media_file_for_spam) | 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. | """Checks if a given event is considered "spammy" by this server. | ||||
If the server considers an event spammy, then it will be rejected if | 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 | event: the event to be checked | ||||
Returns: | 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. | 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: | for callback in self._check_event_for_spam_callbacks: | ||||
with Measure( | with Measure( | ||||
self.clock, "{}.{}".format(callback.__module__, callback.__qualname__) | 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. | # This spam-checker accepts the event. | ||||
# Other spam-checkers may reject it, though. | # Other spam-checkers may reject it, though. | ||||
continue | continue | ||||
@@ -304,13 +299,23 @@ class SpamChecker: | |||||
# This spam-checker rejects the event with deprecated | # This spam-checker rejects the event with deprecated | ||||
# return value `True` | # return value `True` | ||||
return Codes.FORBIDDEN | 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: | 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. | # No spam-checker has rejected the event, let it pass. | ||||
return Allow.ALLOW | |||||
return self.NOT_SPAM | |||||
async def should_drop_federated_event( | async def should_drop_federated_event( | ||||
self, event: "synapse.events.EventBase" | self, event: "synapse.events.EventBase" | ||||
@@ -15,7 +15,6 @@ | |||||
import logging | import logging | ||||
from typing import TYPE_CHECKING | from typing import TYPE_CHECKING | ||||
import synapse | |||||
from synapse.api.constants import MAX_DEPTH, EventContentFields, EventTypes, Membership | from synapse.api.constants import MAX_DEPTH, EventContentFields, EventTypes, Membership | ||||
from synapse.api.errors import Codes, SynapseError | from synapse.api.errors import Codes, SynapseError | ||||
from synapse.api.room_versions import EventFormatVersions, RoomVersion | 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) | 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) | logger.warning("Event contains spam, soft-failing %s", pdu.event_id) | ||||
# we redact (to save disk space) as well as soft-failing (to stop | # we redact (to save disk space) as well as soft-failing (to stop | ||||
# using the event in prev_events). | # using the event in prev_events). | ||||
@@ -23,7 +23,6 @@ from canonicaljson import encode_canonical_json | |||||
from twisted.internet.interfaces import IDelayedCall | from twisted.internet.interfaces import IDelayedCall | ||||
import synapse | |||||
from synapse import event_auth | from synapse import event_auth | ||||
from synapse.api.constants import ( | from synapse.api.constants import ( | ||||
EventContentFields, | EventContentFields, | ||||
@@ -886,10 +885,22 @@ class EventCreationHandler: | |||||
event.sender, | 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( | 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( | ev = await self.handle_new_client_event( | ||||
@@ -35,7 +35,6 @@ from typing_extensions import ParamSpec | |||||
from twisted.internet import defer | from twisted.internet import defer | ||||
from twisted.web.resource import Resource | from twisted.web.resource import Resource | ||||
from synapse import spam_checker_api | |||||
from synapse.api.errors import SynapseError | from synapse.api.errors import SynapseError | ||||
from synapse.events import EventBase | from synapse.events import EventBase | ||||
from synapse.events.presence_router import ( | from synapse.events.presence_router import ( | ||||
@@ -55,6 +54,7 @@ from synapse.events.spamcheck import ( | |||||
USER_MAY_JOIN_ROOM_CALLBACK, | USER_MAY_JOIN_ROOM_CALLBACK, | ||||
USER_MAY_PUBLISH_ROOM_CALLBACK, | USER_MAY_PUBLISH_ROOM_CALLBACK, | ||||
USER_MAY_SEND_3PID_INVITE_CALLBACK, | USER_MAY_SEND_3PID_INVITE_CALLBACK, | ||||
SpamChecker, | |||||
) | ) | ||||
from synapse.events.third_party_rules import ( | from synapse.events.third_party_rules import ( | ||||
CHECK_CAN_DEACTIVATE_USER_CALLBACK, | CHECK_CAN_DEACTIVATE_USER_CALLBACK, | ||||
@@ -140,9 +140,7 @@ are loaded into Synapse. | |||||
""" | """ | ||||
PRESENCE_ALL_USERS = PresenceRouter.ALL_USERS | 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__ = [ | __all__ = [ | ||||
"errors", | "errors", | ||||
@@ -151,7 +149,7 @@ __all__ = [ | |||||
"respond_with_html", | "respond_with_html", | ||||
"run_in_background", | "run_in_background", | ||||
"cached", | "cached", | ||||
"Allow", | |||||
"NOT_SPAM", | |||||
"UserID", | "UserID", | ||||
"DatabasePool", | "DatabasePool", | ||||
"LoggingTransaction", | "LoggingTransaction", | ||||
@@ -12,9 +12,6 @@ | |||||
# See the License for the specific language governing permissions and | # See the License for the specific language governing permissions and | ||||
# limitations under the License. | # limitations under the License. | ||||
from enum import Enum | from enum import Enum | ||||
from typing import Union | |||||
from synapse.api.errors import Codes | |||||
class RegistrationBehaviour(Enum): | class RegistrationBehaviour(Enum): | ||||
@@ -25,25 +22,3 @@ class RegistrationBehaviour(Enum): | |||||
ALLOW = "allow" | ALLOW = "allow" | ||||
SHADOW_BAN = "shadow_ban" | SHADOW_BAN = "shadow_ban" | ||||
DENY = "deny" | 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`. | |||||
""" |