Fixes: #19540Fixes: #16290 (side effect of the proposed fix)
Closes: #12804 (side effect of the proposed fix)
Introduced in: https://github.com/matrix-org/synapse/pull/8932
---
This PR is a relatively simple simplification of the profile change on
deactivation that appears to remove multiple bugs.
This PR's **primary motivating fix** is #19540: when a user is
deactivated and erased, they would be kept in the user directory. This
bug appears to have been here since #8932 (previously
https://github.com/matrix-org/synapse/pull/8932) (v1.26.0).
The root cause of this bug is that after removing the user from the user
directory, we would immediately update their displayname and avatar to
empty strings (one at a time), which re-inserts
the user into the user directory.
With this PR, we now delete the entire `profiles` row upon user erasure,
which is cleaner (from a 'your database goes back to zero after
deactivating and erasing a user' point of view) and
only needs one database operation (instead of doing displayname then
avatar).
With this PR, we also no longer send the 2 (deferred) `m.room.member`
`join` events to every room to propagate the displayname and avatar_url
changes.
This is good for two reasons:
- the user is about to get parted from those rooms anyway, so this
reduces the number of state events sent per room from 3 to 1. (More
efficient for us in the moment and leaves less litter in the room DAG.)
- it is possible for the displayname/avatar update to be sent **after**
the user parting, which seems as though it could trigger the user to be
re-joined to a public room.
(With that said, although this sounds vaguely familiar in my lossy
memory, I can't find a ticket that actually describes this bug, so this
might be fictional. Edit: #16290 seems to describe this, although the
title is misleading.)
Additionally, as a side effect of the proposed fix (deleting the
`profiles` row), this PR also now deletes custom profile fields upon
user erasure, which is a new feature/bugfix (not sure which) in its own
right.
I do not see a ticket that corresponds to this feature gap, possibly
because custom profile fields are still a niche feature without
mainstream support (to the best of my knowledge).
Tests are included for the primary bugfix and for the cleanup of custom
profile fields.
### `set_displayname` module API change
This change includes a minor _technically_-breaking change to the module
API.
The change concerns `set_displayname` which is exposed to the module API
with a `deactivation: bool = False` flag, matching the internal handler
method it wraps.
I suspect that this is a mistake caused by overly-faithfully piping
through the args from the wrapped method (this Module API was introduced
in
https://github.com/matrix-org/synapse/pull/14629/changes#diff-0b449f6f95672437cf04f0b5512572b4a6a729d2759c438b7c206ea249619885R1592).
The linked PR did the same for `by_admin` originally before it was
changed.
The `deactivation` flag's only purpose is to be piped through to other
Module API callbacks when a module has registered to be notified about
profile changes.
My claim is that it makes no sense for the Module API to have this flag
because it is not the one doing the deactivation, thus it should never
be in a position to set this to `True`.
My proposed change keeps the flag (for function signature
compatibility), but turns it into a no-op (with a `ERROR` log when it's
set to True by the module).
The Module API callback notifying of the module-caused displayname
change will therefore now always have `deactivation = False`.
*Discussed in
[`#synapse-dev:matrix.org`](https://matrix.to/#/!i5D5LLct_DYG-4hQprLzrxdbZ580U9UB6AEgFnk6rZQ/$1f8N6G_EJUI_I_LvplnVAF2UFZTw_FzgsPfB6pbcPKk?via=element.io&via=matrix.org&via=beeper.com)*
---------
Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
This changes the arguments in clock functions to be `Duration` and
converts call sites and constants into `Duration`. There are still some
more functions around that should be converted (e.g.
`timeout_deferred`), but we leave that to another PR.
We also changes `.as_secs()` to return a float, as the rounding broke
things subtly. The only reason to keep it (its the same as
`timedelta.total_seconds()`) is for symmetry with `as_millis()`.
Follows on from https://github.com/element-hq/synapse/pull/19223
Introduce `Clock.call_when_running(...)` to wrap startup code in a
logcontext, ensuring we can identify which server generated the logs.
Background:
> Ideally, nothing from the Synapse homeserver would be logged against the `sentinel`
> logcontext as we want to know which server the logs came from. In practice, this is not
> always the case yet especially outside of request handling.
>
> Global things outside of Synapse (e.g. Twisted reactor code) should run in the
> `sentinel` logcontext. It's only when it calls into application code that a logcontext
> gets activated. This means the reactor should be started in the `sentinel` logcontext,
> and any time an awaitable yields control back to the reactor, it should reset the
> logcontext to be the `sentinel` logcontext. This is important to avoid leaking the
> current logcontext to the reactor (which would then get picked up and associated with
> the next thing the reactor does).
>
> *-- `docs/log_contexts.md`
Also adds a lint to prefer `Clock.call_when_running(...)` over
`reactor.callWhenRunning(...)`
Part of https://github.com/element-hq/synapse/issues/18905
This can be reviewed commit by commit
There are a few improvements over the experimental support:
- authorisation of Synapse <-> MAS requests is simplified, with a single
shared secret, removing the need for provisioning a client on the MAS
side
- the tests actually spawn a real server, allowing us to test the rust
introspection layer
- we now check that the device advertised in introspection actually
exist, making it so that when a user logs out, the tokens are
immediately invalidated, even if the cache doesn't expire
- it doesn't rely on discovery anymore, rather on a static endpoint
base. This means users don't have to override the introspection endpoint
to avoid internet roundtrips
- it doesn't depend on `authlib` anymore, as we simplified a lot the
calls done from Synapse to MAS
We still have to update the MAS documentation about the Synapse setup,
but that can be done later.
---------
Co-authored-by: reivilibre <oliverw@element.io>
Fix https://github.com/element-hq/synapse/issues/18735
In https://github.com/element-hq/synapse/pull/18670, we updated
`run_as_background_process` to add a `server_name` argument. Because
this function is directly exported from the Synapse module API, this is
a breaking change to any downstream Synapse modules that use
`run_as_background_process`.
This PR shims and deprecates the existing
`run_as_background_process(...)` for modules by providing a stub
`server_name` value and introduces a new
`ModuleApi.run_as_background_process(...)` that covers the `server_name`
logic automagically.
Part of https://github.com/element-hq/synapse/issues/18592
Separated out of https://github.com/element-hq/synapse/pull/18656
because it's a bigger, unique piece of the refactor
### Testing strategy
1. Add the `metrics` listener in your `homeserver.yaml`
```yaml
listeners:
# This is just showing how to configure metrics either way
#
# `http` `metrics` resource
- port: 9322
type: http
bind_addresses: ['127.0.0.1']
resources:
- names: [metrics]
compress: false
# `metrics` listener
- port: 9323
type: metrics
bind_addresses: ['127.0.0.1']
```
1. Start the homeserver: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Fetch `http://localhost:9322/_synapse/metrics` and/or
`http://localhost:9323/metrics`
1. Observe response includes the background processs metrics
(`synapse_background_process_start_count`,
`synapse_background_process_db_txn_count_total`, etc) with the
`server_name` label
The main goal of this PR is to handle device list changes onto multiple
writers, off the main process, so that we can have logins happening
whilst Synapse is rolling-restarting.
This is quite an intrusive change, so I would advise to review this
commit by commit; I tried to keep the history as clean as possible.
There are a few things to consider:
- the `device_list_key` in stream tokens becomes a
`MultiWriterStreamToken`, which has a few implications in sync and on
the storage layer
- we had a split between `DeviceHandler` and `DeviceWorkerHandler` for
master vs. worker process. I've kept this split, but making it rather
writer vs. non-writer worker, using method overrides for doing
replication calls when needed
- there are a few operations that need to happen on a single worker at a
time. Instead of using cross-worker locks, for now I made them run on
the first writer on the list
---------
Co-authored-by: Eric Eastwood <erice@element.io>
Refactor `Measure` block metrics to be homeserver-scoped (add
`server_name` label to block metrics).
Part of https://github.com/element-hq/synapse/issues/18592
### Testing strategy
#### See behavior of previous `metrics` listener
1. Add the `metrics` listener in your `homeserver.yaml`
```yaml
listeners:
- port: 9323
type: metrics
bind_addresses: ['127.0.0.1']
```
1. Start the homeserver: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Fetch `http://localhost:9323/metrics`
1. Observe response includes the block metrics
(`synapse_util_metrics_block_count`,
`synapse_util_metrics_block_in_flight`, etc)
#### See behavior of the `http` `metrics` resource
1. Add the `metrics` resource to a new or existing `http` listeners in
your `homeserver.yaml`
```yaml
listeners:
- port: 9322
type: http
bind_addresses: ['127.0.0.1']
resources:
- names: [metrics]
compress: false
```
1. Start the homeserver: `poetry run synapse_homeserver --config-path
homeserver.yaml`
1. Fetch `http://localhost:9322/_synapse/metrics` (it's just a `GET`
request so you can even do in the browser)
1. Observe response includes the block metrics
(`synapse_util_metrics_block_count`,
`synapse_util_metrics_block_in_flight`, etc)
This PR adds an additional `room_config` argument to the
`user_may_create_room` spam checker module API callback.
It will continue to work with implementations of `user_may_create_room`
that do not expect the additional parameter.
A side affect is that on a room upgrade the spam checker callback is
called *after* doing some work to calculate the state rather than
before. However, I hope that this is acceptable given the relative
infrequency of room upgrades.
Broke in #17916, as the signature inspection incorrectly looks at the
wrapper function. We fix this by setting the signature on the wrapper
function to that of the wrapped function via `@functools.wraps`.
Refactor `get_profile` to avoid returning "empty" (`None` / `null`)
fields. Currently this is not very important, but will be more useful
once #17488 lands. It does update the servlet to use this now which has
a minor change in behavior: additional fields served over federation
will now be properly sent back to clients.
It also adds constants for `avatar_url` / `displayname` although I did
not attempt to use it everywhere possible.
Since Synapse 1.76.0, any module which registers a `on_new_event`
callback would brick the ability to join remote rooms.
This is because this callback tried to get the full state of the room,
which would end up in a deadlock.
Related:
https://github.com/matrix-org/synapse-auto-accept-invite/issues/18
The following module would brick the ability to join remote rooms:
```python
from typing import Any, Dict, Literal, Union
import logging
from synapse.module_api import ModuleApi, EventBase
logger = logging.getLogger(__name__)
class MyModule:
def __init__(self, config: None, api: ModuleApi):
self._api = api
self._config = config
self._api.register_third_party_rules_callbacks(
on_new_event=self.on_new_event,
)
async def on_new_event(self, event: EventBase, _state_map: Any) -> None:
logger.info(f"Received new event: {event}")
@staticmethod
def parse_config(_config: Dict[str, Any]) -> None:
return None
```
This is technically a breaking change, as we are now passing partial
state on the `on_new_event` callback.
However, this callback was broken for federated rooms since 1.76.0, and
local rooms have full state anyway, so it's unlikely that it would
change anything.
During the migration the automated script to update the copyright
headers accidentally got rid of some of the existing copyright lines.
Reinstate them.
This adds a module API which allows a module to update a user's
presence state/status message. This is useful for controlling presence
from an external system.
To fully control presence from the module the presence.enabled config
parameter gains a new state of "untracked" which disables internal tracking
of presence changes via user actions, etc. Only updates from the module will
be persisted and sent down sync properly).
Also add restore of purge/shutdown rooms after a synapse restart.
Co-authored-by: Eric Eastwood <erice@matrix.org>
Co-authored-by: Erik Johnston <erikj@matrix.org>
* Allow user_id to be optional for room deletion
* Add module API method to delete a room
* Newsfile
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
* Don't worry about the case block=True && requester_user_id is None
---------
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
* Fix the method signature of `run_db_interaction` on the module API
* Newsfile
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
---------
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
* Add a module API function to provide `call_later`
* Newsfile
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
* Add comments
* Update version number
---------
Signed-off-by: Olivier Wilkinson (reivilibre) <oliverw@matrix.org>
Olivier 'reivilibre