Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow spam-checker modules to be async #8890

Merged
merged 14 commits into from
Dec 11, 2020
1 change: 1 addition & 0 deletions changelog.d/8890.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Spam-checkers may now define their methods as `async`.
19 changes: 13 additions & 6 deletions docs/spam_checker.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -32,28 +34,33 @@ 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
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):
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
Expand Down
55 changes: 39 additions & 16 deletions synapse/events/spamcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
# 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
from synapse.util.async_helpers import maybe_awaitable

if TYPE_CHECKING:
import synapse.events
Expand All @@ -39,7 +40,9 @@ 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"
) -> 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
Expand All @@ -50,15 +53,16 @@ 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 spam_checker.check_event_for_spam(event):
if await maybe_awaitable(spam_checker.check_event_for_spam(event)):
return True

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
Expand All @@ -75,14 +79,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.
Expand All @@ -94,12 +102,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.
Expand All @@ -112,12 +123,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.
Expand All @@ -130,12 +146,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
Expand All @@ -157,12 +178,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],
Expand All @@ -185,7 +206,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
Expand Down
7 changes: 6 additions & 1 deletion synapse/federation/federation_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ def _check_sigs_and_hashes(

ctx = current_context()

@defer.inlineCallbacks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned this in the commit, but it is sad to add an inlineCallbacks back in, but without this we need to convert a huge stack to async/await. I'd rather do that as a follow-up.

def callback(_, pdu: EventBase):
with PreserveLoggingContext(ctx):
if not check_event_content_hash(pdu):
Expand Down Expand Up @@ -105,7 +106,11 @@ def callback(_, pdu: EventBase):
)
return redacted_event

if self.spam_checker.check_event_for_spam(pdu):
result = yield defer.ensureDeferred(
self.spam_checker.check_event_for_spam(pdu)
)

if result:
logger.warning(
"Event contains spam, redacting %s: %s",
pdu.event_id,
Expand Down
8 changes: 4 additions & 4 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,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
Expand Down Expand Up @@ -58,6 +57,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

Expand Down Expand Up @@ -1638,6 +1638,6 @@ async def on_logged_out(

# This might return an awaitable, if it does block the log out
# until it completes.
result = g(user_id=user_id, device_id=device_id, access_token=access_token,)
if inspect.isawaitable(result):
await result
await maybe_awaitable(
g(user_id=user_id, device_id=device_id, access_token=access_token,)
)
4 changes: 3 additions & 1 deletion synapse/handlers/directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,7 @@ async def create_and_send_nonmember_event(
event.sender,
)

spam_error = 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"
Expand Down
7 changes: 2 additions & 5 deletions synapse/handlers/receipts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 [],
)

Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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")
Expand Down
10 changes: 5 additions & 5 deletions synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 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

Expand Down
9 changes: 2 additions & 7 deletions synapse/metrics/background_process_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Loading