Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring auto-accept invite logic into Synapse #17147

Merged
merged 18 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/17147.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Bring auto-accept invite logic into Synapse.
devonh marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 29 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -4595,3 +4595,32 @@ background_updates:
min_batch_size: 10
default_batch_size: 50
```
---
## Auto Accept Invites
Configuration settings related to automatically accepting invites.

---
### `auto_accept_invites`

Automatically accepting invites controls whether users are presented with an invite request or if they
are instead automatically joined to a room when receiving an invite. Set the `enabled` sub-option to true to
enable auto-accepting invites. Defaults to false.
This setting has the following sub-options:
* `enabled`: Whether to run the auto-accept invites logic. Defaults to false.
* `only_for_direct_messages`: Whether invites should be automatically accepted for all room types, or only
for direct messages. Defaults to false.
* `only_from_local_users`: Whether to only automatically accept invites from users on this homeserver. Defaults to false.
* `worker_to_run_on`: Which worker to run this module on. This must match the "worker_name".

NOTE: Care should be taken not to enable this setting if the `synapse_auto_accept_invite` module is enabled and installed.
The two modules will compete to perform the same task and may result in undesired behaviour. For example, multiple join
events could be generated from a single invite.

Example configuration:
```yaml
auto_accept_invites:
enabled: true
only_for_direct_messages: true
only_from_local_users: true
worker_to_run_on: "worker_1"
```
6 changes: 6 additions & 0 deletions synapse/app/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
from synapse.config.homeserver import HomeServerConfig
from synapse.config.server import ListenerConfig, ManholeConfig, TCPListenerConfig
from synapse.crypto import context_factory
from synapse.events.auto_accept_invites import InviteAutoAccepter
from synapse.events.presence_router import load_legacy_presence_router
from synapse.handlers.auth import load_legacy_password_auth_providers
from synapse.http.site import SynapseSite
Expand Down Expand Up @@ -582,6 +583,11 @@ def run_sighup(*args: Any, **kwargs: Any) -> None:
m = module(config, module_api)
logger.info("Loaded module %s", m)

if hs.config.auto_accept_invites.enabled:
# Start the local auto_accept_invites module.
m = InviteAutoAccepter(hs.config.auto_accept_invites, module_api)
logger.info("Loaded local module %s", m)

load_legacy_spam_checkers(hs)
load_legacy_third_party_event_rules(hs)
load_legacy_presence_router(hs)
Expand Down
2 changes: 2 additions & 0 deletions synapse/config/_base.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ from synapse.config import ( # noqa: F401
api,
appservice,
auth,
auto_accept_invites,
background_updates,
cache,
captcha,
Expand Down Expand Up @@ -120,6 +121,7 @@ class RootConfig:
federation: federation.FederationConfig
retention: retention.RetentionConfig
background_updates: background_updates.BackgroundUpdateConfig
auto_accept_invites: auto_accept_invites.AutoAcceptInvitesConfig

config_classes: List[Type["Config"]] = ...
config_files: List[str]
Expand Down
43 changes: 43 additions & 0 deletions synapse/config/auto_accept_invites.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#
# This file is licensed under the Affero General Public License (AGPL) version 3.
#
# Copyright (C) 2024 New Vector, Ltd
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# See the GNU Affero General Public License for more details:
# <https://www.gnu.org/licenses/agpl-3.0.html>.
#
# Originally licensed under the Apache License, Version 2.0:
# <http://www.apache.org/licenses/LICENSE-2.0>.
#
# [This file includes modifications made by New Vector Limited]
#
#
from typing import Any

from synapse.types import JsonDict

from ._base import Config


class AutoAcceptInvitesConfig(Config):
section = "auto_accept_invites"

def read_config(self, config: JsonDict, **kwargs: Any) -> None:
auto_accept_invites_config = config.get("auto_accept_invites") or {}

self.enabled = auto_accept_invites_config.get("enabled", False)

self.accept_invites_only_for_direct_messages = auto_accept_invites_config.get(
"only_for_direct_messages", False
)

self.accept_invites_only_from_local_users = auto_accept_invites_config.get(
"only_from_local_users", False
)

self.worker_to_run_on = auto_accept_invites_config.get("worker_to_run_on")
2 changes: 2 additions & 0 deletions synapse/config/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from .api import ApiConfig
from .appservice import AppServiceConfig
from .auth import AuthConfig
from .auto_accept_invites import AutoAcceptInvitesConfig
from .background_updates import BackgroundUpdateConfig
from .cache import CacheConfig
from .captcha import CaptchaConfig
Expand Down Expand Up @@ -105,4 +106,5 @@ class HomeServerConfig(RootConfig):
RedisConfig,
ExperimentalConfig,
BackgroundUpdateConfig,
AutoAcceptInvitesConfig,
]
196 changes: 196 additions & 0 deletions synapse/events/auto_accept_invites.py
Copy link
Member

Choose a reason for hiding this comment

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

Generally only out-of-tree modules will use the Module API, so it feels a bit odd to me to see in-tree code making use of it. But then again, it allows the code to be self-contained, and easy to extract to a third-party module if we ever wanted to do so in the future.

I wonder if instead of having an explicit config section for this module, we instead just have it installed by default into your venv. Then, just like a third-party module, a sysadmin would just configure it under modules as if it were installed separately.

This cuts down on the number of config sections, specialised code in the module config loader, and makes migrating this code to an out-of-tree module even easier if desired.

We would just need to be careful not to integrate the code too heavily, thus making it difficult to unpick later. One way to encourage this would be to put this code under a separate directory, say /synapse/modules, and tests under /tests/modules. We can then treat code in those directories as separate, intended to interact with the rest of Synapse only through the Module API, as an external module would.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the general idea.

Specifically, would we create a separate pyproject.toml file for each module? (ie. in /synapse/modules/my_module/)

And how would we go about versioning these modules?
If versioning them, would we need to remember both to update the module version itself, as well as the overall synapse dependency version?

When installing them, would we just add them to the main pyproject.toml as a path dependency? Would this be enough to ensure they are installed in each of the various docker containers, deb package, local install, etc.?

Hopefully this makes sense to you. I ran into these things while trying this out.
These will need to be sorted out if this is to be a viable long term path forward.

Copy link
Member

@erikjohnston erikjohnston May 8, 2024

Choose a reason for hiding this comment

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

I would heavily encourage that they're not separate projects, as you lose a bunch of benefits of it being in tree (e.g. being able to use private APIs etc).

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, that is one of the downsides. While this code doesn't actually need any private APIs, it is inevitably handy.

I can actually add another downside. While we wouldn't end up adding to Synapse's config if we made this a module... it would beg the question of how we'd actually document the config of this module. We wouldn't be able to put it in https://element-hq.github.io/synapse/latest/usage/configuration/config_documentation.html, unless we added a new section for each in-tree module... and then you've ended up making the user-visible config larger anyhow.

That leads me to think that the only benefit of keeping the modules separate would be if we ever wanted to move them out-of-tree again in future. But I think the times we'll actually do that are minimal. And if we really need to do so, then untangling it from deep within Synapse isn't impossible, just slightly more fiddly.

The initial reason for me suggesting that we keep this code separate is that internal code using the module API felt weird. But after reflection I don't think it's really an issue. It doesn't block us from modifying the API since the code is internal and can change. I also don't believe we have any assumptions in the code that all consumers of the API are external.

So all in all, I'm OK with leaving this code how it is and where it is.

Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
#
# This file is licensed under the Affero General Public License (AGPL) version 3.
#
# Copyright 2021 The Matrix.org Foundation C.I.C
# Copyright (C) 2024 New Vector, Ltd
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
# published by the Free Software Foundation, either version 3 of the
# License, or (at your option) any later version.
#
# See the GNU Affero General Public License for more details:
# <https://www.gnu.org/licenses/agpl-3.0.html>.
#
# Originally licensed under the Apache License, Version 2.0:
# <http://www.apache.org/licenses/LICENSE-2.0>.
#
# [This file includes modifications made by New Vector Limited]
#
#
import logging
from http import HTTPStatus
from typing import Any, Dict, Tuple

from synapse.api.constants import AccountDataTypes, EventTypes, Membership
from synapse.api.errors import SynapseError
from synapse.config.auto_accept_invites import AutoAcceptInvitesConfig
from synapse.module_api import EventBase, ModuleApi, run_as_background_process

logger = logging.getLogger(__name__)


class InviteAutoAccepter:
def __init__(self, config: AutoAcceptInvitesConfig, api: ModuleApi):
# Keep a reference to the Module API.
self._api = api
self._config = config

if not self._config.enabled:
return

should_run_on_this_worker = config.worker_to_run_on == self._api.worker_name

if not should_run_on_this_worker:
logger.info(
"Not accepting invites on this worker (configured: %r, here: %r)",
config.worker_to_run_on,
self._api.worker_name,
)
return

logger.info(
"Accepting invites on this worker (here: %r)", self._api.worker_name
)

# Register the callback.
self._api.register_third_party_rules_callbacks(
on_new_event=self.on_new_event,
)

async def on_new_event(self, event: EventBase, *args: Any) -> None:
"""Listens for new events, and if the event is an invite for a local user then
automatically accepts it.

Args:
event: The incoming event.
"""
# Check if the event is an invite for a local user.
is_invite_for_local_user = (
event.type == EventTypes.Member
and event.is_state()
and event.membership == Membership.INVITE
and self._api.is_mine(event.state_key)
)

# Only accept invites for direct messages if the configuration mandates it.
is_direct_message = event.content.get("is_direct", False)
is_allowed_by_direct_message_rules = (
not self._config.accept_invites_only_for_direct_messages
or is_direct_message is True
)

# Only accept invites from remote users if the configuration mandates it.
is_from_local_user = self._api.is_mine(event.sender)
is_allowed_by_local_user_rules = (
not self._config.accept_invites_only_from_local_users
or is_from_local_user is True
)

if (
is_invite_for_local_user
and is_allowed_by_direct_message_rules
and is_allowed_by_local_user_rules
):
# Make the user join the room. We run this as a background process to circumvent a race condition
# that occurs when responding to invites over federation (see https://github.com/matrix-org/synapse-auto-accept-invite/issues/12)
run_as_background_process(
"retry_make_join",
self._retry_make_join,
event.state_key,
event.state_key,
event.room_id,
"join",
bg_start_span=False,
)

if is_direct_message:
# Mark this room as a direct message!
await self._mark_room_as_direct_message(
event.state_key, event.sender, event.room_id
)

async def _mark_room_as_direct_message(
self, user_id: str, dm_user_id: str, room_id: str
) -> None:
"""
Marks a room (`room_id`) as a direct message with the counterparty `dm_user_id`
from the perspective of the user `user_id`.

Args:
user_id: the user for whom the membership is changing
dm_user_id: the user performing the membership change
room_id: room id of the room the user is invited to
"""

# This is a dict of User IDs to tuples of Room IDs
# (get_global will return a frozendict of tuples as it freezes the data,
# but we should accept either frozen or unfrozen variants.)
# Be careful: we convert the outer frozendict into a dict here,
# but the contents of the dict are still frozen (tuples in lieu of lists,
# etc.)
dm_map: Dict[str, Tuple[str, ...]] = dict(
await self._api.account_data_manager.get_global(
user_id, AccountDataTypes.DIRECT
)
or {}
)

if dm_user_id not in dm_map:
dm_map[dm_user_id] = (room_id,)
else:
dm_rooms_for_user = dm_map[dm_user_id]
assert isinstance(dm_rooms_for_user, (tuple, list))

dm_map[dm_user_id] = tuple(dm_rooms_for_user) + (room_id,)

await self._api.account_data_manager.put_global(
user_id, AccountDataTypes.DIRECT, dm_map
)

async def _retry_make_join(
self, sender: str, target: str, room_id: str, new_membership: str
) -> None:
"""
A function to retry sending the `make_join` request with an increasing backoff. This is
implemented to work around a race condition when receiving invites over federation.

Args:
sender: the user performing the membership change
target: the user for whom the membership is changing
room_id: room id of the room to join to
new_membership: the type of membership event (in this case will be "join")
"""

sleep = 0
retries = 0
join_event = None

while retries < 5:
try:
await self._api.sleep(sleep)
join_event = await self._api.update_room_membership(
sender=sender,
target=target,
room_id=room_id,
new_membership=new_membership,
)
except SynapseError as e:
if e.code == HTTPStatus.FORBIDDEN:
logger.debug(
f"Update_room_membership was forbidden. This can sometimes be expected for remote invites. Exception: {e}"
)
else:
logger.warn(
f"Update_room_membership raised the following unexpected (SynapseError) exception: {e}"
)
except Exception as e:
logger.warn(
f"Update_room_membership raised the following unexpected exception: {e}"
)

sleep = 2**retries
retries += 1

if join_event is not None:
break
2 changes: 1 addition & 1 deletion synapse/handlers/sso.py
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ def is_allowed_mime_type(content_type: str) -> bool:
server_name = profile["avatar_url"].split("/")[-2]
media_id = profile["avatar_url"].split("/")[-1]
if self._is_mine_server_name(server_name):
media = await self._media_repo.store.get_local_media(media_id)
media = await self._media_repo.store.get_local_media(media_id) # type: ignore[has-type]
Copy link
Member

Choose a reason for hiding this comment

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

Should this not go in #17166 instead? How come it was moved back to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

haha because when I put it in that PR, the linter complained about an unnecessary ignore....
So rather than fight a circle of nonsense, I feel like it's easier just to keep it here :)

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit cautious about introducing more ignore for types without trying to spend some time in understanding what has happened. By the looks of it the type inference has failed somehow, so am wondering if we've managed to introduce a loop in the typing maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure this parcitular lint is related to this issue: matrix-org/synapse#11165
Which seems like quite the can of worms.

Copy link
Member Author

Choose a reason for hiding this comment

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

The particular error is:
synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type]

Copy link
Member Author

Choose a reason for hiding this comment

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

For further reference, this is the only instance of using self._media_repo.store outside of the media_repo source.
That probably has something to do with the linter error only showing up here.

Since this code is completely unrelated to the changes in this PR, I suggest we allow the addition of the ignore here until the larger issue of #11165 is brought to a resolution. Adding an ignore keeps things in effectively the same state as before since mypy should have been treating this as an error, but it was silently ignoring it. At least with the ignore added, now the issue is explicit and visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

synapse/handlers/sso.py:820: error: Cannot determine type of "store" [has-type] this error sounds like one where we could hopefully add a type annotation instead?

Copy link
Member Author

@devonh devonh May 15, 2024

Choose a reason for hiding this comment

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

The type is generic & is specified at the following locations:

DATASTORE_CLASS = DataStore # type: ignore

DATASTORE_CLASS = GenericWorkerStore # type: ignore

DATASTORE_CLASS = AdminCmdStore # type: ignore

All of which also have an explicit type ignore on them.
I can't seem to come up with any annotation that would make mypy happy - maybe someone else knows a way, but this seems like a much deeper issue around DataStore typing.

if media is not None and upload_name == media.upload_name:
logger.info("skipping saving the user avatar")
return True
Expand Down
Loading
Loading