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.

Sign up for GitHub

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

Bring auto-accept invite logic into Synapse #17147

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
3353f4c
Bring auto-accept invite logic into synapse
devonh May 2, 2024
3be3fb7
Remove TODO comments
devonh May 3, 2024
f612271
Add note to docs about not running same time as module
devonh May 3, 2024
657baec
Add changelog entry
devonh May 3, 2024
f82358d
Merge branch 'develop' into devon/auto-accept-invite
devonh May 3, 2024
0906a34
Replace unreachable check with assert
devonh May 3, 2024
0bdcf0d
Remove mutable default from test
devonh May 3, 2024
a50d304
Fix unrelated linter errors...
devonh May 3, 2024
c82e16e
Merge branch 'develop' into devon/auto-accept-invite
devonh May 3, 2024
9698bf4
Small tweaks from review
devonh May 7, 2024
f07a112
Parameterize auto-accept invites tests
devonh May 7, 2024
6db79fc
Revert "Fix unrelated linter errors..."
devonh May 7, 2024
396909f
Add back linter ignore
devonh May 7, 2024
821a74b
Address review comments
devonh May 8, 2024
aa6e20e
Merge branch 'develop' into devon/auto-accept-invite
devonh May 17, 2024
8874e58
Fix tests with latest develop changes
devonh May 17, 2024
41c851c
Update changelog entry to point at docs.
devonh May 21, 2024
d038118
Merge branch 'develop' into devon/auto-accept-invite
devonh May 21, 2024
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
12 changes: 6 additions & 6 deletions docs/usage/configuration/config_documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4565,15 +4565,15 @@ Automatically accepting invites controls whether users are presented with an inv
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. Set to true to change the default.
* `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. Set to true to change the default.
* `only_from_local_users`: Whether invites should be automatically accepted from all users, or only from users
on this homeserver. Defaults to false. Set to true to change the default.
* `worker_to_run_on`: Which worker to run this module on. Defaults to None (running on the main process).
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.
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
Expand Down
2 changes: 1 addition & 1 deletion synapse/config/auto_accept_invites.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,4 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
"only_from_local_users", False
)

self.worker_to_run_on = auto_accept_invites_config.get("worker_to_run_on", None)
self.worker_to_run_on = auto_accept_invites_config.get("worker_to_run_on")
28 changes: 24 additions & 4 deletions synapse/events/auto_accept_invites.py
View file
Open in desktop
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 •
edited
Loading

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
Expand Up @@ -19,8 +19,10 @@
#
#
import logging
from http import HTTPStatus
from typing import Any, Dict, Tuple

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

Expand All @@ -34,6 +36,10 @@ def __init__(self, config: AutoAcceptInvitesConfig, api: ModuleApi):
self._api = api
self._config = config

if not self._config.enabled:
logger.info("Not starting InviteAutoAccepter as it is not enabled")
devonh marked this conversation as resolved.
Show resolved Hide resolved
return

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

if not should_run_on_this_worker:
Expand Down Expand Up @@ -70,7 +76,6 @@ async def on_new_event(self, event: EventBase, *args: Any) -> None:

# Only accept invites for direct messages if the configuration mandates it.
is_direct_message = event.content.get("is_direct", False)
logger.info("Is Direct: %r", is_direct_message)
is_allowed_by_direct_message_rules = (
not self._config.accept_invites_only_for_direct_messages
or is_direct_message is True
Expand Down Expand Up @@ -112,6 +117,11 @@ async def _mark_room_as_direct_message(
"""
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
Expand Down Expand Up @@ -148,7 +158,7 @@ async def _retry_make_join(

Args:
sender: the user performing the membership change
target: the for whom the membership is changing
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")
"""
Expand All @@ -166,10 +176,20 @@ async def _retry_make_join(
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.info(
f"Update_room_membership raised the following exception: {e}"
logger.warn(
f"Update_room_membership raised the following unexpected exception: {e}"
)
finally:
sleep = 2**retries
retries += 1
devonh marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
13 changes: 4 additions & 9 deletions synapse/handlers/profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#
import logging
import random
from typing import TYPE_CHECKING, List, Optional, Union
from typing import TYPE_CHECKING, Optional, Union

from synapse.api.errors import (
AuthError,
Expand Down Expand Up @@ -64,10 +64,8 @@ def __init__(self, hs: "HomeServer"):
self.user_directory_handler = hs.get_user_directory_handler()
self.request_ratelimiter = hs.get_request_ratelimiter()

self.max_avatar_size: Optional[int] = hs.config.server.max_avatar_size
self.allowed_avatar_mimetypes: Optional[List[str]] = (
hs.config.server.allowed_avatar_mimetypes
)
self.max_avatar_size = hs.config.server.max_avatar_size
self.allowed_avatar_mimetypes = hs.config.server.allowed_avatar_mimetypes

self._is_mine_server_name = hs.is_mine_server_name

Expand Down Expand Up @@ -340,10 +338,7 @@ async def check_avatar_size_and_mime_type(self, mxc: str) -> bool:

if self.max_avatar_size:
# Ensure avatar does not exceed max allowed avatar size
if (
media_info.media_length
and media_info.media_length > self.max_avatar_size
):
if media_info.media_length > self.max_avatar_size:
logger.warning(
"Forbidding avatar change to %s: %d bytes is above the allowed size "
"limit",
Expand Down
Loading
Loading