From 2bec29c1d0b30b73813c440d95d066fbb7720e2d Mon Sep 17 00:00:00 2001 From: David Teller Date: Mon, 7 Dec 2020 17:39:04 +0100 Subject: [PATCH 01/13] Make check_event_for_spam async Method `check_event_for_spam` in both `class SpamCheck` and in plug-in spam checkers is currently sync. This makes it impossible to write spam checkers that need to e.g. check with a database before responding. Signed-off-by: Your Name --- changelog.d/8890.feature | 1 + docs/spam_checker.md | 2 +- synapse/events/spamcheck.py | 5 +++-- synapse/federation/federation_base.py | 30 +++++++++++++++++++-------- synapse/handlers/message.py | 6 ++++-- 5 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 changelog.d/8890.feature diff --git a/changelog.d/8890.feature b/changelog.d/8890.feature new file mode 100644 index 000000000000..0c6afb3b1a42 --- /dev/null +++ b/changelog.d/8890.feature @@ -0,0 +1 @@ +Spam-checkers may now define their method `check_event_for_spam` as `async`. This will simplify the work of spam-checkers that need to e.g. request data from a database without blocking the server. \ No newline at end of file diff --git a/docs/spam_checker.md b/docs/spam_checker.md index 7fc08f1b7021..26d5e8e4fb06 100644 --- a/docs/spam_checker.md +++ b/docs/spam_checker.md @@ -37,7 +37,7 @@ class ExampleSpamChecker: self.config = config self.api = api - def check_event_for_spam(self, foo): + async def check_event_for_spam(self, foo): return False # allow all events def user_may_invite(self, inviter_userid, invitee_userid, room_id): diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 936896656ac2..90a1143fce25 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -19,6 +19,7 @@ from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import Collection +from synapse.util.async_helpers import maybe_awaitable if TYPE_CHECKING: import synapse.events @@ -39,7 +40,7 @@ def __init__(self, hs: "synapse.server.HomeServer"): else: self.spam_checkers.append(module(config=config)) - def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: + async def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: """Checks if a given event is considered "spammy" by this server. If the server considers an event spammy, then it will be rejected if @@ -53,7 +54,7 @@ def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: True if the event is spammy. """ for spam_checker in self.spam_checkers: - if spam_checker.check_event_for_spam(event): + if await maybe_awaitable(spam_checker.check_event_for_spam(event)): return True return False diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 38aa47963f50..8344ebbba81f 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -18,7 +18,7 @@ from typing import Iterable, List from twisted.internet import defer -from twisted.internet.defer import Deferred, DeferredList +from twisted.internet.defer import Deferred, DeferredList, ensureDeferred from twisted.python.failure import Failure from synapse.api.constants import MAX_DEPTH, EventTypes, Membership @@ -35,6 +35,7 @@ make_deferred_yieldable, ) from synapse.types import JsonDict, get_domain_from_id +from synapse.util.async_helpers import maybe_awaitable logger = logging.getLogger(__name__) @@ -74,11 +75,11 @@ def _check_sigs_and_hashes( * throws a SynapseError if the signature check failed. The deferreds run their callbacks in the sentinel """ - deferreds = _check_sigs_on_pdus(self.keyring, room_version, pdus) + initial_deferreds = _check_sigs_on_pdus(self.keyring, room_version, pdus) ctx = current_context() - def callback(_, pdu: EventBase): + async def callback(_, pdu: EventBase): with PreserveLoggingContext(ctx): if not check_event_content_hash(pdu): # let's try to distinguish between failures because the event was @@ -105,7 +106,7 @@ def callback(_, pdu: EventBase): ) return redacted_event - if self.spam_checker.check_event_for_spam(pdu): + if await maybe_awaitable(self.spam_checker.check_event_for_spam(pdu)): logger.warning( "Event contains spam, redacting %s: %s", pdu.event_id, @@ -125,12 +126,23 @@ def errback(failure: Failure, pdu: EventBase): ) return failure - for deferred, pdu in zip(deferreds, pdus): - deferred.addCallbacks( - callback, errback, callbackArgs=[pdu], errbackArgs=[pdu] - ) + # Here, we require a little gymnastics to return + # deferreds that accept awaitables as callbacks + deferred_with_callbacks = [] + for deferred, pdu in zip(initial_deferreds, pdus): + + async def awaitable(): + result = None + try: + result = await deferred + except Exception as e: + return errback(e, pdu) + else: + return await callback(result, pdu) + + deferred_with_callbacks.append(ensureDeferred(awaitable)) - return deferreds + return deferred_with_callbacks class PduToCheckSig( diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index 96843338aefd..a2ff40af268e 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -51,7 +51,7 @@ from synapse.storage.state import StateFilter from synapse.types import Requester, RoomAlias, StreamToken, UserID, create_requester from synapse.util import json_decoder, json_encoder -from synapse.util.async_helpers import Linearizer +from synapse.util.async_helpers import Linearizer, maybe_awaitable from synapse.util.metrics import measure_func from synapse.visibility import filter_events_for_client @@ -744,7 +744,9 @@ async def create_and_send_nonmember_event( event.sender, ) - spam_error = self.spam_checker.check_event_for_spam(event) + spam_error = await maybe_awaitable( + self.spam_checker.check_event_for_spam(event) + ) if spam_error: if not isinstance(spam_error, str): spam_error = "Spam is not permitted here" From 25a44b61a027bc511c9a9f7ae1ff34c74e1520a2 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 8 Dec 2020 11:13:37 -0500 Subject: [PATCH 02/13] Use inlineCallbacks to potentially handle an async spam check method. Note that this would ideally switch the entire callstack to async/await instead, but the callstack is complicated by using many callback functions and a DeferredList. --- synapse/federation/federation_base.py | 36 +++++++++++---------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 8344ebbba81f..386a75b3c5f0 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -13,12 +13,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +import inspect import logging from collections import namedtuple from typing import Iterable, List from twisted.internet import defer -from twisted.internet.defer import Deferred, DeferredList, ensureDeferred +from twisted.internet.defer import Deferred, DeferredList from twisted.python.failure import Failure from synapse.api.constants import MAX_DEPTH, EventTypes, Membership @@ -35,7 +36,6 @@ make_deferred_yieldable, ) from synapse.types import JsonDict, get_domain_from_id -from synapse.util.async_helpers import maybe_awaitable logger = logging.getLogger(__name__) @@ -75,11 +75,12 @@ def _check_sigs_and_hashes( * throws a SynapseError if the signature check failed. The deferreds run their callbacks in the sentinel """ - initial_deferreds = _check_sigs_on_pdus(self.keyring, room_version, pdus) + deferreds = _check_sigs_on_pdus(self.keyring, room_version, pdus) ctx = current_context() - async def callback(_, pdu: EventBase): + @defer.inlineCallbacks + def callback(_, pdu: EventBase): with PreserveLoggingContext(ctx): if not check_event_content_hash(pdu): # let's try to distinguish between failures because the event was @@ -106,7 +107,11 @@ async def callback(_, pdu: EventBase): ) return redacted_event - if await maybe_awaitable(self.spam_checker.check_event_for_spam(pdu)): + result = self.spam_checker.check_event_for_spam(pdu) + if inspect.isawaitable(result): + result = yield defer.ensureDeferred(result) + + if result: logger.warning( "Event contains spam, redacting %s: %s", pdu.event_id, @@ -126,23 +131,12 @@ def errback(failure: Failure, pdu: EventBase): ) return failure - # Here, we require a little gymnastics to return - # deferreds that accept awaitables as callbacks - deferred_with_callbacks = [] - for deferred, pdu in zip(initial_deferreds, pdus): - - async def awaitable(): - result = None - try: - result = await deferred - except Exception as e: - return errback(e, pdu) - else: - return await callback(result, pdu) - - deferred_with_callbacks.append(ensureDeferred(awaitable)) + for deferred, pdu in zip(deferreds, pdus): + deferred.addCallbacks( + callback, errback, callbackArgs=[pdu], errbackArgs=[pdu] + ) - return deferred_with_callbacks + return deferreds class PduToCheckSig( From d8c5eab28d608cd4141acfc29ab1fc6363ec40e4 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:15:55 -0500 Subject: [PATCH 03/13] Allow spam methods to be async. --- synapse/events/spamcheck.py | 43 +++++++++++++++++++-------- synapse/federation/federation_base.py | 7 ++--- synapse/handlers/directory.py | 4 ++- synapse/handlers/federation.py | 2 +- synapse/handlers/message.py | 6 ++-- synapse/handlers/receipts.py | 7 ++--- synapse/handlers/register.py | 2 +- synapse/handlers/room.py | 4 +-- synapse/handlers/user_directory.py | 2 +- synapse/server.py | 2 +- synapse/util/async_helpers.py | 31 +++++-------------- 11 files changed, 54 insertions(+), 56 deletions(-) diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 90a1143fce25..2507ae1b495e 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -59,7 +59,7 @@ async def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: return False - def user_may_invite( + async def user_may_invite( self, inviter_userid: str, invitee_userid: str, room_id: str ) -> bool: """Checks if a given user may send an invite @@ -76,14 +76,18 @@ def user_may_invite( """ for spam_checker in self.spam_checkers: if ( - spam_checker.user_may_invite(inviter_userid, invitee_userid, room_id) + await maybe_awaitable( + spam_checker.user_may_invite( + inviter_userid, invitee_userid, room_id + ) + ) is False ): return False return True - def user_may_create_room(self, userid: str) -> bool: + async def user_may_create_room(self, userid: str) -> bool: """Checks if a given user may create a room If this method returns false, the creation request will be rejected. @@ -95,12 +99,15 @@ def user_may_create_room(self, userid: str) -> bool: True if the user may create a room, otherwise False """ for spam_checker in self.spam_checkers: - if spam_checker.user_may_create_room(userid) is False: + if ( + await maybe_awaitable(spam_checker.user_may_create_room(userid)) + is False + ): return False return True - def user_may_create_room_alias(self, userid: str, room_alias: str) -> bool: + async def user_may_create_room_alias(self, userid: str, room_alias: str) -> bool: """Checks if a given user may create a room alias If this method returns false, the association request will be rejected. @@ -113,12 +120,17 @@ def user_may_create_room_alias(self, userid: str, room_alias: str) -> bool: True if the user may create a room alias, otherwise False """ for spam_checker in self.spam_checkers: - if spam_checker.user_may_create_room_alias(userid, room_alias) is False: + if ( + await maybe_awaitable( + spam_checker.user_may_create_room_alias(userid, room_alias) + ) + is False + ): return False return True - def user_may_publish_room(self, userid: str, room_id: str) -> bool: + async def user_may_publish_room(self, userid: str, room_id: str) -> bool: """Checks if a given user may publish a room to the directory If this method returns false, the publish request will be rejected. @@ -131,12 +143,17 @@ def user_may_publish_room(self, userid: str, room_id: str) -> bool: True if the user may publish the room, otherwise False """ for spam_checker in self.spam_checkers: - if spam_checker.user_may_publish_room(userid, room_id) is False: + if ( + await maybe_awaitable( + spam_checker.user_may_publish_room(userid, room_id) + ) + is False + ): return False return True - def check_username_for_spam(self, user_profile: Dict[str, str]) -> bool: + async def check_username_for_spam(self, user_profile: Dict[str, str]) -> bool: """Checks if a user ID or display name are considered "spammy" by this server. If the server considers a username spammy, then it will not be included in @@ -158,12 +175,12 @@ def check_username_for_spam(self, user_profile: Dict[str, str]) -> bool: if checker: # Make a copy of the user profile object to ensure the spam checker # cannot modify it. - if checker(user_profile.copy()): + if await maybe_awaitable(checker(user_profile.copy())): return True return False - def check_registration_for_spam( + async def check_registration_for_spam( self, email_threepid: Optional[dict], username: Optional[str], @@ -186,7 +203,9 @@ def check_registration_for_spam( # spam checker checker = getattr(spam_checker, "check_registration_for_spam", None) if checker: - behaviour = checker(email_threepid, username, request_info) + behaviour = await maybe_awaitable( + checker(email_threepid, username, request_info) + ) assert isinstance(behaviour, RegistrationBehaviour) if behaviour != RegistrationBehaviour.ALLOW: return behaviour diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index 386a75b3c5f0..383737520afa 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -13,7 +13,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import inspect import logging from collections import namedtuple from typing import Iterable, List @@ -107,9 +106,9 @@ def callback(_, pdu: EventBase): ) return redacted_event - result = self.spam_checker.check_event_for_spam(pdu) - if inspect.isawaitable(result): - result = yield defer.ensureDeferred(result) + result = yield defer.ensureDeferred( + self.spam_checker.check_event_for_spam(pdu) + ) if result: logger.warning( diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index ad5683d25197..06a6c22feda5 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -133,7 +133,9 @@ async def create_association( 403, "You must be in the room to create an alias for it" ) - if not self.spam_checker.user_may_create_room_alias(user_id, room_alias): + if not await self.spam_checker.user_may_create_room_alias( + user_id, room_alias + ): raise AuthError(403, "This user is not permitted to create this alias") if not self.config.is_alias_creation_allowed( diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index b9799090f784..5e52c7e2c665 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1593,7 +1593,7 @@ async def on_invite_request( if self.hs.config.block_non_admin_invites: raise SynapseError(403, "This server does not accept room invites") - if not self.spam_checker.user_may_invite( + if not await self.spam_checker.user_may_invite( event.sender, event.state_key, event.room_id ): raise SynapseError( diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index a2ff40af268e..2b8aa9443d29 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -51,7 +51,7 @@ from synapse.storage.state import StateFilter from synapse.types import Requester, RoomAlias, StreamToken, UserID, create_requester from synapse.util import json_decoder, json_encoder -from synapse.util.async_helpers import Linearizer, maybe_awaitable +from synapse.util.async_helpers import Linearizer from synapse.util.metrics import measure_func from synapse.visibility import filter_events_for_client @@ -744,9 +744,7 @@ async def create_and_send_nonmember_event( event.sender, ) - spam_error = await maybe_awaitable( - self.spam_checker.check_event_for_spam(event) - ) + spam_error = await self.spam_checker.check_event_for_spam(event) if spam_error: if not isinstance(spam_error, str): spam_error = "Spam is not permitted here" diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 153cbae7b912..e850e45e46c7 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -18,7 +18,6 @@ from synapse.appservice import ApplicationService from synapse.handlers._base import BaseHandler from synapse.types import JsonDict, ReadReceipt, get_domain_from_id -from synapse.util.async_helpers import maybe_awaitable logger = logging.getLogger(__name__) @@ -98,10 +97,8 @@ async def _handle_new_receipts(self, receipts): self.notifier.on_new_event("receipt_key", max_batch_id, rooms=affected_room_ids) # Note that the min here shouldn't be relied upon to be accurate. - await maybe_awaitable( - self.hs.get_pusherpool().on_new_receipts( - min_batch_id, max_batch_id, affected_room_ids - ) + await self.hs.get_pusherpool().on_new_receipts( + min_batch_id, max_batch_id, affected_room_ids ) return True diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 0d85fd08684f..94b5610acd4c 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -187,7 +187,7 @@ async def register_user( """ self.check_registration_ratelimit(address) - result = self.spam_checker.check_registration_for_spam( + result = await self.spam_checker.check_registration_for_spam( threepid, localpart, user_agent_ips or [], ) diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py index 930047e730e2..3354c0f45d5c 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -358,7 +358,7 @@ async def clone_existing_room( """ user_id = requester.user.to_string() - if not self.spam_checker.user_may_create_room(user_id): + if not await self.spam_checker.user_may_create_room(user_id): raise SynapseError(403, "You are not permitted to create rooms") creation_content = { @@ -608,7 +608,7 @@ async def create_room( 403, "You are not permitted to create rooms", Codes.FORBIDDEN ) - if not is_requester_admin and not self.spam_checker.user_may_create_room( + if not is_requester_admin and not await self.spam_checker.user_may_create_room( user_id ): raise SynapseError(403, "You are not permitted to create rooms") diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index afbebfc20058..485c3e448333 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -84,7 +84,7 @@ async def search_users(self, user_id, search_term, limit): results["results"] = [ user for user in results["results"] - if not self.spam_checker.check_username_for_spam(user) + if not await self.spam_checker.check_username_for_spam(user) ] return results diff --git a/synapse/server.py b/synapse/server.py index b017e3489faf..02f2df32c57a 100644 --- a/synapse/server.py +++ b/synapse/server.py @@ -595,7 +595,7 @@ def get_stats_handler(self) -> StatsHandler: return StatsHandler(self) @cache_in_self - def get_spam_checker(self): + def get_spam_checker(self) -> SpamChecker: return SpamChecker(self) @cache_in_self diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index 382f0cf3f0d2..f39510bdb67f 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -15,10 +15,12 @@ # limitations under the License. import collections +import inspect import logging from contextlib import contextmanager from typing import ( Any, + Awaitable, Callable, Dict, Hashable, @@ -525,28 +527,9 @@ def failure_cb(val): return new_d -@attr.s(slots=True, frozen=True) -class DoneAwaitable: - """Simple awaitable that returns the provided value. +async def maybe_awaitable(value: Union[Awaitable[R], R]) -> R: + """Awaits an awaitable and returns the value, otherwise just returns the input. """ - - value = attr.ib() - - def __await__(self): - return self - - def __iter__(self): - return self - - def __next__(self): - raise StopIteration(self.value) - - -def maybe_awaitable(value): - """Convert a value to an awaitable if not already an awaitable. - """ - - if hasattr(value, "__await__"): - return value - - return DoneAwaitable(value) + if inspect.isawaitable(value): + return await value + return value From c812fa49117d5f5db9ced06b848014643ac25eb9 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:16:04 -0500 Subject: [PATCH 04/13] Update documentation. --- docs/spam_checker.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/docs/spam_checker.md b/docs/spam_checker.md index 26d5e8e4fb06..326b8a75d239 100644 --- a/docs/spam_checker.md +++ b/docs/spam_checker.md @@ -32,6 +32,8 @@ call back into the homeserver internals. ### Example ```python +from synapse.spam_checker_api import RegistrationBehaviour + class ExampleSpamChecker: def __init__(self, config, api): self.config = config @@ -40,20 +42,23 @@ class ExampleSpamChecker: async def check_event_for_spam(self, foo): return False # allow all events - def user_may_invite(self, inviter_userid, invitee_userid, room_id): + async def user_may_invite(self, inviter_userid, invitee_userid, room_id): return True # allow all invites - def user_may_create_room(self, userid): + async def user_may_create_room(self, userid): return True # allow all room creations - def user_may_create_room_alias(self, userid, room_alias): + async def user_may_create_room_alias(self, userid, room_alias): return True # allow all room aliases - def user_may_publish_room(self, userid, room_id): + async def user_may_publish_room(self, userid, room_id): return True # allow publishing of all rooms - def check_username_for_spam(self, user_profile): + async def check_username_for_spam(self, user_profile): return False # allow all usernames + + async def check_registration_for_spam(self, email_threepid, username, request_info): + return RegistrationBehaviour.ALLOW # allow all registrations ``` ## Configuration From 2dc2de42adbc9a183c6248d5c5a486291e05f9b0 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:25:51 -0500 Subject: [PATCH 05/13] Use maybe_awaitable consistently. --- synapse/handlers/auth.py | 14 +++++++------- synapse/metrics/background_process_metrics.py | 9 ++------- synapse/rest/media/v1/storage_provider.py | 16 ++++++---------- synapse/util/distributor.py | 7 ++----- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 588d3a60df44..5418431f1601 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -13,7 +13,6 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import inspect import logging import time import unicodedata @@ -54,6 +53,7 @@ from synapse.module_api import ModuleApi from synapse.types import JsonDict, Requester, UserID from synapse.util import stringutils as stringutils +from synapse.util.async_helpers import maybe_awaitable from synapse.util.msisdn import phone_number_to_msisdn from synapse.util.threepids import canonicalise_email @@ -1019,13 +1019,13 @@ async def delete_access_token(self, access_token: str): if hasattr(provider, "on_logged_out"): # This might return an awaitable, if it does block the log out # until it completes. - result = provider.on_logged_out( - user_id=user_info.user_id, - device_id=user_info.device_id, - access_token=access_token, + await maybe_awaitable( + provider.on_logged_out( + user_id=user_info.user_id, + device_id=user_info.device_id, + access_token=access_token, + ) ) - if inspect.isawaitable(result): - await result # delete pushers associated with this access token if user_info.token_id is not None: diff --git a/synapse/metrics/background_process_metrics.py b/synapse/metrics/background_process_metrics.py index 658f6ecd72a3..76b7decf260d 100644 --- a/synapse/metrics/background_process_metrics.py +++ b/synapse/metrics/background_process_metrics.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import inspect import logging import threading from functools import wraps @@ -25,6 +24,7 @@ from synapse.logging.context import LoggingContext, PreserveLoggingContext from synapse.logging.opentracing import noop_context_manager, start_active_span +from synapse.util.async_helpers import maybe_awaitable if TYPE_CHECKING: import resource @@ -206,12 +206,7 @@ async def run(): if bg_start_span: ctx = start_active_span(desc, tags={"request_id": context.request}) with ctx: - result = func(*args, **kwargs) - - if inspect.isawaitable(result): - result = await result - - return result + return await maybe_awaitable(func(*args, **kwargs)) except Exception: logger.exception( "Background process '%s' threw an exception", desc, diff --git a/synapse/rest/media/v1/storage_provider.py b/synapse/rest/media/v1/storage_provider.py index 18c9ed48d6e8..67f67efde725 100644 --- a/synapse/rest/media/v1/storage_provider.py +++ b/synapse/rest/media/v1/storage_provider.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. -import inspect import logging import os import shutil @@ -21,6 +20,7 @@ from synapse.config._base import Config from synapse.logging.context import defer_to_thread, run_in_background +from synapse.util.async_helpers import maybe_awaitable from ._base import FileInfo, Responder from .media_storage import FileResponder @@ -91,16 +91,14 @@ async def store_file(self, path, file_info): if self.store_synchronous: # store_file is supposed to return an Awaitable, but guard # against improper implementations. - result = self.backend.store_file(path, file_info) - if inspect.isawaitable(result): - return await result + return await maybe_awaitable(self.backend.store_file(path, file_info)) else: # TODO: Handle errors. async def store(): try: - result = self.backend.store_file(path, file_info) - if inspect.isawaitable(result): - return await result + return await maybe_awaitable( + self.backend.store_file(path, file_info) + ) except Exception: logger.exception("Error storing file") @@ -110,9 +108,7 @@ async def store(): async def fetch(self, path, file_info): # store_file is supposed to return an Awaitable, but guard # against improper implementations. - result = self.backend.fetch(path, file_info) - if inspect.isawaitable(result): - return await result + return await maybe_awaitable(self.backend.fetch(path, file_info)) class FileStorageProviderBackend(StorageProvider): diff --git a/synapse/util/distributor.py b/synapse/util/distributor.py index f73e95393cbe..a6ee9edaec90 100644 --- a/synapse/util/distributor.py +++ b/synapse/util/distributor.py @@ -12,13 +12,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -import inspect import logging from twisted.internet import defer from synapse.logging.context import make_deferred_yieldable, run_in_background from synapse.metrics.background_process_metrics import run_as_background_process +from synapse.util.async_helpers import maybe_awaitable logger = logging.getLogger(__name__) @@ -105,10 +105,7 @@ def fire(self, *args, **kwargs): async def do(observer): try: - result = observer(*args, **kwargs) - if inspect.isawaitable(result): - result = await result - return result + return await maybe_awaitable(observer(*args, **kwargs)) except Exception as e: logger.warning( "%s signal observer %s failed: %r", self.name, observer, e, From 4ea59736642d38825026092da90f49d8872ddd92 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:31:17 -0500 Subject: [PATCH 06/13] Note that check_event_for_spam can return a string. --- docs/spam_checker.md | 2 ++ synapse/events/spamcheck.py | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/docs/spam_checker.md b/docs/spam_checker.md index 326b8a75d239..5b4f6428e658 100644 --- a/docs/spam_checker.md +++ b/docs/spam_checker.md @@ -22,6 +22,8 @@ well as some specific methods: * `user_may_create_room` * `user_may_create_room_alias` * `user_may_publish_room` +* `check_username_for_spam` +* `check_registration_for_spam` The details of the each of these methods (as well as their inputs and outputs) are documented in the `synapse.events.spamcheck.SpamChecker` class. diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 2507ae1b495e..e7e3a7b9a45a 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -15,7 +15,7 @@ # limitations under the License. import inspect -from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple, Union from synapse.spam_checker_api import RegistrationBehaviour from synapse.types import Collection @@ -40,7 +40,9 @@ def __init__(self, hs: "synapse.server.HomeServer"): else: self.spam_checkers.append(module(config=config)) - async def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: + async def check_event_for_spam( + self, event: "synapse.events.EventBase" + ) -> Union[bool, 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 @@ -51,7 +53,8 @@ async def check_event_for_spam(self, event: "synapse.events.EventBase") -> bool: event: the event to be checked Returns: - True if the event is spammy. + True or a string if the event is spammy. If a string is returned it + will be used as the error message returned to the user. """ for spam_checker in self.spam_checkers: if await maybe_awaitable(spam_checker.check_event_for_spam(event)): From c02ebc604918224d8b76f87e7d0597b87db4c1db Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:33:44 -0500 Subject: [PATCH 07/13] Fix type hints. --- synapse/util/async_helpers.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index f39510bdb67f..6fe14b597411 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -531,5 +531,7 @@ async def maybe_awaitable(value: Union[Awaitable[R], R]) -> R: """Awaits an awaitable and returns the value, otherwise just returns the input. """ if inspect.isawaitable(value): + assert isinstance(value, Awaitable) return await value + assert not isinstance(value, Awaitable) return value From fdd9731a57338422867a620eb61e308ed889eaef Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:35:00 -0500 Subject: [PATCH 08/13] Update changelog. --- changelog.d/8890.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8890.feature b/changelog.d/8890.feature index 0c6afb3b1a42..97aa72a76e0b 100644 --- a/changelog.d/8890.feature +++ b/changelog.d/8890.feature @@ -1 +1 @@ -Spam-checkers may now define their method `check_event_for_spam` as `async`. This will simplify the work of spam-checkers that need to e.g. request data from a database without blocking the server. \ No newline at end of file +Spam-checkers may now define their methods as `async`. From 0f96a96fd166c28a0717cddf7c293ffd5a475996 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 11:51:18 -0500 Subject: [PATCH 09/13] Fix incompatible syntax. --- synapse/handlers/user_directory.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 485c3e448333..057507b8492c 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -81,11 +81,11 @@ async def search_users(self, user_id, search_term, limit): results = await self.store.search_user_dir(user_id, search_term, limit) # Remove any spammy users from the results. - results["results"] = [ - user - for user in results["results"] - if not await self.spam_checker.check_username_for_spam(user) - ] + results = [] + for user in results["results"]: + if not await self.spam_checker.check_username_for_spam(user): + results.append(user) + results["results"] = results return results From c2b54b12e3ef98b0997050c6dd59aa9a16943e39 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 12:14:15 -0500 Subject: [PATCH 10/13] Don't override variable. --- synapse/handlers/user_directory.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 057507b8492c..f263a638f802 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -81,11 +81,11 @@ async def search_users(self, user_id, search_term, limit): results = await self.store.search_user_dir(user_id, search_term, limit) # Remove any spammy users from the results. - results = [] + non_spammy_users = [] for user in results["results"]: if not await self.spam_checker.check_username_for_spam(user): - results.append(user) - results["results"] = results + non_spammy_users.append(user) + results["results"] = non_spammy_users return results From f78ba2b90756914482aa6d44be984bd74dd9471e Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 10 Dec 2020 12:17:35 -0500 Subject: [PATCH 11/13] Add a few missing calls. --- synapse/handlers/directory.py | 2 +- synapse/handlers/room_member.py | 2 +- tests/handlers/test_user_directory.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 06a6c22feda5..abcf86352dad 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -411,7 +411,7 @@ async def edit_published_room_list( """ user_id = requester.user.to_string() - if not self.spam_checker.user_may_publish_room(user_id, room_id): + if not await self.spam_checker.user_may_publish_room(user_id, room_id): raise AuthError( 403, "This user is not permitted to publish rooms to the room list" ) diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index c00288632480..9614ab758cf9 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -408,7 +408,7 @@ async def update_membership_locked( ) block_invite = True - if not self.spam_checker.user_may_invite( + if not await self.spam_checker.user_may_invite( requester.user.to_string(), target.to_string(), room_id ): logger.info("Blocking invite due to spam checker") diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 98e5af207248..647a17cb901f 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -270,7 +270,7 @@ def test_spam_checker(self): spam_checker = self.hs.get_spam_checker() class AllowAll: - def check_username_for_spam(self, user_profile): + async def check_username_for_spam(self, user_profile): # Allow all users. return False @@ -283,7 +283,7 @@ def check_username_for_spam(self, user_profile): # Configure a spam checker that filters all users. class BlockAll: - def check_username_for_spam(self, user_profile): + async def check_username_for_spam(self, user_profile): # All users are spammy. return True From 5e8fadc4d3e4d3315594ff3a99819c5ac7dc2cb7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 11 Dec 2020 12:15:46 -0500 Subject: [PATCH 12/13] Revert changes to maybe_awaitable. --- synapse/util/async_helpers.py | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index 6fe14b597411..f57ab4d46ab0 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -527,11 +527,28 @@ def failure_cb(val): return new_d -async def maybe_awaitable(value: Union[Awaitable[R], R]) -> R: +@attr.s(slots=True, frozen=True) +class DoneAwaitable: + """Simple awaitable that returns the provided value. + """ + + value = attr.ib() + + def __await__(self): + return self + + def __iter__(self): + return self + + def __next__(self): + raise StopIteration(self.value) + + +def maybe_awaitable(value: Union[Awaitable[R], R]) -> Awaitable[R]: """Awaits an awaitable and returns the value, otherwise just returns the input. """ if inspect.isawaitable(value): assert isinstance(value, Awaitable) - return await value - assert not isinstance(value, Awaitable) - return value + return value + + return DoneAwaitable(value) From 9a21585eff348e9001e2b09e295656c91fc14c42 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 11 Dec 2020 12:16:41 -0500 Subject: [PATCH 13/13] Revert comment. --- synapse/util/async_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index f57ab4d46ab0..9a873c8e8e49 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -545,7 +545,7 @@ def __next__(self): def maybe_awaitable(value: Union[Awaitable[R], R]) -> Awaitable[R]: - """Awaits an awaitable and returns the value, otherwise just returns the input. + """Convert a value to an awaitable if not already an awaitable. """ if inspect.isawaitable(value): assert isinstance(value, Awaitable)