From bd9a1079bc6fe2d0a2c75acce1771324a52e0ab7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 19 Dec 2024 08:00:50 -0600 Subject: [PATCH 01/13] Update reverse proxy docs with what we've learned from #17986 (#17994) Update reverse proxy docs with what we've learned from https://github.com/element-hq/synapse/pull/17986 Also vice versa and update our nginx config with what I learned from the reverse proxy docs. ### Pull Request Checklist * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). The entry should: - Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from `EventStore` to `EventWorkerStore`.". - Use markdown where necessary, mostly for `code blocks`. - End with either a period (.) or an exclamation mark (!). - Start with a capital letter. - Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --- changelog.d/17994.doc | 1 + docker/conf-workers/nginx.conf.j2 | 3 +++ docs/reverse_proxy.md | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog.d/17994.doc diff --git a/changelog.d/17994.doc b/changelog.d/17994.doc new file mode 100644 index 00000000000..54b7cf10008 --- /dev/null +++ b/changelog.d/17994.doc @@ -0,0 +1 @@ +Fix example in reverse proxy docs to include server port. diff --git a/docker/conf-workers/nginx.conf.j2 b/docker/conf-workers/nginx.conf.j2 index c3f9b584d29..95d2f760d2f 100644 --- a/docker/conf-workers/nginx.conf.j2 +++ b/docker/conf-workers/nginx.conf.j2 @@ -38,6 +38,9 @@ server { {% if using_unix_sockets %} proxy_pass http://unix:/run/main_public.sock; {% else %} + # note: do not add a path (even a single /) after the port in `proxy_pass`, + # otherwise nginx will canonicalise the URI and cause signature verification + # errors. proxy_pass http://localhost:8080; {% endif %} proxy_set_header X-Forwarded-For $remote_addr; diff --git a/docs/reverse_proxy.md b/docs/reverse_proxy.md index 7128af114e9..45de2b1f65b 100644 --- a/docs/reverse_proxy.md +++ b/docs/reverse_proxy.md @@ -74,7 +74,7 @@ server { proxy_pass http://localhost:8008; proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; - proxy_set_header Host $host; + proxy_set_header Host $host:$server_port; # Nginx by default only allows file uploads up to 1M in size # Increase client_max_body_size to match max_upload_size defined in homeserver.yaml From 234d07eb09946543546ce63463c3bfd40465e1d6 Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Thu, 19 Dec 2024 14:02:06 +0000 Subject: [PATCH 02/13] Disable statement timeout during room purge (#18017) This is already done for `purge_history` but seems to have been forgotten for `purge_room`. --- changelog.d/18017.misc | 1 + synapse/storage/databases/main/purge_events.py | 5 +++++ 2 files changed, 6 insertions(+) create mode 100644 changelog.d/18017.misc diff --git a/changelog.d/18017.misc b/changelog.d/18017.misc new file mode 100644 index 00000000000..6b943a5bed2 --- /dev/null +++ b/changelog.d/18017.misc @@ -0,0 +1 @@ +Disable DB statement timeout when doing a purge room since it can be quite long. diff --git a/synapse/storage/databases/main/purge_events.py b/synapse/storage/databases/main/purge_events.py index 08244153a39..c195685af83 100644 --- a/synapse/storage/databases/main/purge_events.py +++ b/synapse/storage/databases/main/purge_events.py @@ -376,6 +376,11 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: (room_id,), ) + if isinstance(self.database_engine, PostgresEngine): + # Disable statement timeouts for this transaction; purging rooms can + # take a while! + txn.execute("SET LOCAL statement_timeout = 0") + # First, fetch all the state groups that should be deleted, before # we delete that information. txn.execute( From 2d23250da719e5b81e5793e525171469958466f7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 19 Dec 2024 12:02:47 -0500 Subject: [PATCH 03/13] Remove support for PostgreSQL 11 and 12 (#18034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is essentially matrix-org/synapse#14392. I didn't see anything in there about updating sytest or complement. The main driver of this is so that I can use `jsonb_path_exists` in #17488. 😄 --- .ci/scripts/calculate_jobs.py | 2 +- .github/workflows/tests.yml | 2 +- changelog.d/18034.removal | 1 + docs/upgrade.md | 8 ++++++++ synapse/storage/engines/postgres.py | 4 ++-- 5 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 changelog.d/18034.removal diff --git a/.ci/scripts/calculate_jobs.py b/.ci/scripts/calculate_jobs.py index ea278173db3..5249acdc5d6 100755 --- a/.ci/scripts/calculate_jobs.py +++ b/.ci/scripts/calculate_jobs.py @@ -60,7 +60,7 @@ def set_output(key: str, value: str): { "python-version": "3.9", "database": "postgres", - "postgres-version": "11", + "postgres-version": "13", "extras": "all", } ] diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d91f9c29187..084b08b2492 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -581,7 +581,7 @@ jobs: matrix: include: - python-version: "3.9" - postgres-version: "11" + postgres-version: "13" - python-version: "3.13" postgres-version: "17" diff --git a/changelog.d/18034.removal b/changelog.d/18034.removal new file mode 100644 index 00000000000..303b442fd4a --- /dev/null +++ b/changelog.d/18034.removal @@ -0,0 +1 @@ +Remove support for PostgreSQL 11 and 12. Contributed by @clokep. diff --git a/docs/upgrade.md b/docs/upgrade.md index 45e63b0c5de..6c96cb91a31 100644 --- a/docs/upgrade.md +++ b/docs/upgrade.md @@ -117,6 +117,14 @@ each upgrade are complete before moving on to the next upgrade, to avoid stacking them up. You can monitor the currently running background updates with [the Admin API](usage/administration/admin_api/background_updates.html#status). +# Upgrading to v1.122.0 + +## Dropping support for PostgreSQL 11 and 12 + +In line with our [deprecation policy](deprecation_policy.md), we've dropped +support for PostgreSQL 11 and 12, as they are no longer supported upstream. +This release of Synapse requires PostgreSQL 13+. + # Upgrading to v1.120.0 ## Removal of experimental MSC3886 feature diff --git a/synapse/storage/engines/postgres.py b/synapse/storage/engines/postgres.py index 8c8c6d04144..e4cd359201b 100644 --- a/synapse/storage/engines/postgres.py +++ b/synapse/storage/engines/postgres.py @@ -99,8 +99,8 @@ def check_database( allow_unsafe_locale = self.config.get("allow_unsafe_locale", False) # Are we on a supported PostgreSQL version? - if not allow_outdated_version and self._version < 110000: - raise RuntimeError("Synapse requires PostgreSQL 11 or above.") + if not allow_outdated_version and self._version < 130000: + raise RuntimeError("Synapse requires PostgreSQL 13 or above.") with db_conn.cursor() as txn: txn.execute("SHOW SERVER_ENCODING") From d69c00b5a19ba45645665afa532421b25c74407a Mon Sep 17 00:00:00 2001 From: Colin Watson Date: Fri, 20 Dec 2024 10:57:59 +0000 Subject: [PATCH 04/13] Stop using twisted.internet.defer.returnValue (#18020) `defer.returnValue` was only needed in Python 2; in Python 3, a simple `return` is fine. `twisted.internet.defer.returnValue` is deprecated as of Twisted 24.7.0. Most uses of `returnValue` in synapse were removed a while back; this cleans up some remaining bits. --- changelog.d/18020.misc | 1 + contrib/cmdclient/console.py | 6 +++--- contrib/cmdclient/http.py | 8 ++++---- synapse/logging/scopecontextmanager.py | 19 +------------------ synapse/util/patch_inline_callbacks.py | 4 ++-- 5 files changed, 11 insertions(+), 27 deletions(-) create mode 100644 changelog.d/18020.misc diff --git a/changelog.d/18020.misc b/changelog.d/18020.misc new file mode 100644 index 00000000000..8d2dd883b91 --- /dev/null +++ b/changelog.d/18020.misc @@ -0,0 +1 @@ +Remove some remaining uses of `twisted.internet.defer.returnValue`. Contributed by Colin Watson. diff --git a/contrib/cmdclient/console.py b/contrib/cmdclient/console.py index ca2e72b5e8e..9b5d33d2b1f 100755 --- a/contrib/cmdclient/console.py +++ b/contrib/cmdclient/console.py @@ -245,7 +245,7 @@ def _check_can_login(self): if "flows" not in json_res: print("Failed to find any login flows.") - defer.returnValue(False) + return False flow = json_res["flows"][0] # assume first is the one we want. if "type" not in flow or "m.login.password" != flow["type"] or "stages" in flow: @@ -254,8 +254,8 @@ def _check_can_login(self): "Unable to login via the command line client. Please visit " "%s to login." % fallback_url ) - defer.returnValue(False) - defer.returnValue(True) + return False + return True def do_emailrequest(self, line): """Requests the association of a third party identifier diff --git a/contrib/cmdclient/http.py b/contrib/cmdclient/http.py index e6a10b5f329..54363e42592 100644 --- a/contrib/cmdclient/http.py +++ b/contrib/cmdclient/http.py @@ -78,7 +78,7 @@ def put_json(self, url, data): url, data, headers_dict={"Content-Type": ["application/json"]} ) body = yield readBody(response) - defer.returnValue((response.code, body)) + return response.code, body @defer.inlineCallbacks def get_json(self, url, args=None): @@ -88,7 +88,7 @@ def get_json(self, url, args=None): url = "%s?%s" % (url, qs) response = yield self._create_get_request(url) body = yield readBody(response) - defer.returnValue(json.loads(body)) + return json.loads(body) def _create_put_request(self, url, json_data, headers_dict: Optional[dict] = None): """Wrapper of _create_request to issue a PUT request""" @@ -134,7 +134,7 @@ def do_request( response = yield self._create_request(method, url) body = yield readBody(response) - defer.returnValue(json.loads(body)) + return json.loads(body) @defer.inlineCallbacks def _create_request( @@ -173,7 +173,7 @@ def _create_request( if self.verbose: print("Status %s %s" % (response.code, response.phrase)) print(pformat(list(response.headers.getAllRawHeaders()))) - defer.returnValue(response) + return response def sleep(self, seconds): d = defer.Deferred() diff --git a/synapse/logging/scopecontextmanager.py b/synapse/logging/scopecontextmanager.py index 581e6d6411f..feaadc4d87a 100644 --- a/synapse/logging/scopecontextmanager.py +++ b/synapse/logging/scopecontextmanager.py @@ -20,13 +20,10 @@ # import logging -from types import TracebackType -from typing import Optional, Type +from typing import Optional from opentracing import Scope, ScopeManager, Span -import twisted - from synapse.logging.context import ( LoggingContext, current_context, @@ -112,9 +109,6 @@ class _LogContextScope(Scope): """ A custom opentracing scope, associated with a LogContext - * filters out _DefGen_Return exceptions which arise from calling - `defer.returnValue` in Twisted code - * When the scope is closed, the logcontext's active scope is reset to None. and - if enter_logcontext was set - the logcontext is finished too. """ @@ -146,17 +140,6 @@ def __init__( self._finish_on_close = finish_on_close self._enter_logcontext = enter_logcontext - def __exit__( - self, - exc_type: Optional[Type[BaseException]], - value: Optional[BaseException], - traceback: Optional[TracebackType], - ) -> None: - if exc_type == twisted.internet.defer._DefGen_Return: - # filter out defer.returnValue() calls - exc_type = value = traceback = None - super().__exit__(exc_type, value, traceback) - def __str__(self) -> str: return f"Scope<{self.span}>" diff --git a/synapse/util/patch_inline_callbacks.py b/synapse/util/patch_inline_callbacks.py index 56bdf451dad..beea4d28881 100644 --- a/synapse/util/patch_inline_callbacks.py +++ b/synapse/util/patch_inline_callbacks.py @@ -162,7 +162,7 @@ def check_yield_points_inner( d = result.throwExceptionIntoGenerator(gen) else: d = gen.send(result) - except (StopIteration, defer._DefGen_Return) as e: + except StopIteration as e: if current_context() != expected_context: # This happens when the context is lost sometime *after* the # final yield and returning. E.g. we forgot to yield on a @@ -183,7 +183,7 @@ def check_yield_points_inner( ) ) changes.append(err) - # The `StopIteration` or `_DefGen_Return` contains the return value from the + # The `StopIteration` contains the return value from the # generator. return cast(T, e.value) From 7c2284b2f22def70e46448d035969951a0ce2f4c Mon Sep 17 00:00:00 2001 From: morguldir Date: Mon, 23 Dec 2024 12:19:35 +0100 Subject: [PATCH 05/13] Make admin api redactions use the requester to send the redaction (#18029) --- changelog.d/18029.bugfix | 1 + synapse/handlers/admin.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/18029.bugfix diff --git a/changelog.d/18029.bugfix b/changelog.d/18029.bugfix new file mode 100644 index 00000000000..f7036fe9fc0 --- /dev/null +++ b/changelog.d/18029.bugfix @@ -0,0 +1 @@ +Fix a bug preventing the admin redaction endpoint from working on messages from remote users. diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index d1194545aeb..f3e7790d435 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -473,7 +473,7 @@ async def _redact_all_events( "type": EventTypes.Redaction, "content": {"reason": reason} if reason else {}, "room_id": room, - "sender": user_id, + "sender": requester.user.to_string(), } if room_version.updated_redaction_rules: event_dict["content"]["redacts"] = event.event_id From b1d030a1079779d8441a48accbf66923e3ceb589 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 14:52:41 +0000 Subject: [PATCH 06/13] Bump serde_json from 1.0.133 to 1.0.134 (#18044) --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 34870644512..6752854d0cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,9 +451,9 @@ dependencies = [ [[package]] name = "serde_json" -version = "1.0.133" +version = "1.0.134" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c7fceb2473b9166b2294ef05efcb65a3db80803f0b03ef86a5fc88a2b85ee377" +checksum = "d00f4175c42ee48b15416f6193a959ba3a0d67fc699a0db9ad12df9f83991c7d" dependencies = [ "itoa", "memchr", From 2b59e738ee79287b731b8bd17f263e890d3a4c80 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 14:57:22 +0000 Subject: [PATCH 07/13] Bump authlib from 1.3.2 to 1.4.0 (#18048) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- poetry.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/poetry.lock b/poetry.lock index a0804b9b694..b594727df98 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 1.8.4 and should not be changed by hand. +# This file is automatically @generated by Poetry 1.8.5 and should not be changed by hand. [[package]] name = "annotated-types" @@ -32,13 +32,13 @@ tests-mypy = ["mypy (>=1.11.1)", "pytest-mypy-plugins"] [[package]] name = "authlib" -version = "1.3.2" +version = "1.4.0" description = "The ultimate Python library in building OAuth and OpenID Connect servers and clients." optional = true -python-versions = ">=3.8" +python-versions = ">=3.9" files = [ - {file = "Authlib-1.3.2-py2.py3-none-any.whl", hash = "sha256:ede026a95e9f5cdc2d4364a52103f5405e75aa156357e831ef2bfd0bc5094dfc"}, - {file = "authlib-1.3.2.tar.gz", hash = "sha256:4b16130117f9eb82aa6eec97f6dd4673c3f960ac0283ccdae2897ee4bc030ba2"}, + {file = "Authlib-1.4.0-py2.py3-none-any.whl", hash = "sha256:4bb20b978c8b636222b549317c1815e1fe62234fc1c5efe8855d84aebf3a74e3"}, + {file = "authlib-1.4.0.tar.gz", hash = "sha256:1c1e6608b5ed3624aeeee136ca7f8c120d6f51f731aa152b153d54741840e1f2"}, ] [package.dependencies] @@ -3138,4 +3138,4 @@ user-search = ["pyicu"] [metadata] lock-version = "2.0" python-versions = "^3.9.0" -content-hash = "170c8bc97d4cd16a38a98ed9d48bef0ef09c994d8b129160ef262306cf689db7" +content-hash = "d71159b19349fdc0b7cd8e06e8c8778b603fc37b941c6df34ddc31746783d94d" From 66b24d3d009e8d9e8507c5b778bd28ebaa0f674d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 15:03:10 +0000 Subject: [PATCH 08/13] Bump anyhow from 1.0.94 to 1.0.95 (#18045) --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6752854d0cf..50481282e61 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13,9 +13,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.94" +version = "1.0.95" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1fd03a028ef38ba2276dce7e33fcd6369c158a1bca17946c4b1b701891c1ff7" +checksum = "34ac096ce696dc2fcabef30516bb13c0a68a11d30131d3df6f04711467681b04" [[package]] name = "arc-swap" From e5a53819fcaf73bbd20e30310e6b72d5bcbdc543 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 15:03:55 +0000 Subject: [PATCH 09/13] Bump mypy-zope from 1.0.8 to 1.0.9 (#18047) --- poetry.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/poetry.lock b/poetry.lock index b594727df98..3811d384e4b 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1377,17 +1377,17 @@ files = [ [[package]] name = "mypy-zope" -version = "1.0.8" +version = "1.0.9" description = "Plugin for mypy to support zope interfaces" optional = false python-versions = "*" files = [ - {file = "mypy_zope-1.0.8-py3-none-any.whl", hash = "sha256:8794a77dae0c7e2f28b8ac48569091310b3ee45bb9d6cd4797dcb837c40f9976"}, - {file = "mypy_zope-1.0.8.tar.gz", hash = "sha256:854303a95aefc4289e8a0796808e002c2c7ecde0a10a8f7b8f48092f94ef9b9f"}, + {file = "mypy_zope-1.0.9-py3-none-any.whl", hash = "sha256:6666c1556891a3cb186137519dbd7a58cb30fb72b2504798cad47b35391921ba"}, + {file = "mypy_zope-1.0.9.tar.gz", hash = "sha256:37d6985dfb05a4c27b35cff47577fd5bad878db4893ddedf54d165f7389a1cdb"}, ] [package.dependencies] -mypy = ">=1.0.0,<1.13.0" +mypy = ">=1.0.0,<1.14.0" "zope.interface" = "*" "zope.schema" = "*" From ebc21a8c67ed7a05af010e329b6402cd41229ebe Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 15:09:51 +0000 Subject: [PATCH 10/13] Bump twine from 5.1.1 to 6.0.1 (#18049) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- poetry.lock | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/poetry.lock b/poetry.lock index 3811d384e4b..b5e5bcbd0ef 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2627,19 +2627,20 @@ docs = ["sphinx (<7.0.0)"] [[package]] name = "twine" -version = "5.1.1" +version = "6.0.1" description = "Collection of utilities for publishing packages on PyPI" optional = false python-versions = ">=3.8" files = [ - {file = "twine-5.1.1-py3-none-any.whl", hash = "sha256:215dbe7b4b94c2c50a7315c0275d2258399280fbb7d04182c7e55e24b5f93997"}, - {file = "twine-5.1.1.tar.gz", hash = "sha256:9aa0825139c02b3434d913545c7b847a21c835e11597f5255842d457da2322db"}, + {file = "twine-6.0.1-py3-none-any.whl", hash = "sha256:9c6025b203b51521d53e200f4a08b116dee7500a38591668c6a6033117bdc218"}, + {file = "twine-6.0.1.tar.gz", hash = "sha256:36158b09df5406e1c9c1fb8edb24fc2be387709443e7376689b938531582ee27"}, ] [package.dependencies] -importlib-metadata = ">=3.6" -keyring = ">=15.1" -pkginfo = ">=1.8.1,<1.11" +importlib-metadata = {version = ">=3.6", markers = "python_version < \"3.10\""} +keyring = {version = ">=15.1", markers = "platform_machine != \"ppc64le\" and platform_machine != \"s390x\""} +packaging = "*" +pkginfo = ">=1.8.1" readme-renderer = ">=35.0" requests = ">=2.20" requests-toolbelt = ">=0.8.0,<0.9.0 || >0.9.0" @@ -2647,6 +2648,9 @@ rfc3986 = ">=1.4.0" rich = ">=12.0.0" urllib3 = ">=1.26.0" +[package.extras] +keyring = ["keyring (>=15.1)"] + [[package]] name = "twisted" version = "24.7.0" From b5267678d250ea2f3b020de0c45206341e2da096 Mon Sep 17 00:00:00 2001 From: Shay Date: Fri, 3 Jan 2025 04:52:42 -0800 Subject: [PATCH 11/13] Add a test to verify remote user messages can be redacted via admin api redaction endpoint if requester is admin in room (#18043) --- changelog.d/18043.bugfix | 1 + tests/rest/admin/test_user.py | 59 +++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 changelog.d/18043.bugfix diff --git a/changelog.d/18043.bugfix b/changelog.d/18043.bugfix new file mode 100644 index 00000000000..05f82a44875 --- /dev/null +++ b/changelog.d/18043.bugfix @@ -0,0 +1 @@ +Fix a bug preventing the admin redaction endpoint from working on messages from remote users. \ No newline at end of file diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index b517aefd0c5..a35a250975f 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -60,6 +60,7 @@ from tests import unittest from tests.replication._base import BaseMultiWorkerStreamTestCase from tests.test_utils import SMALL_PNG +from tests.test_utils.event_injection import inject_event from tests.unittest import override_config @@ -5408,6 +5409,64 @@ def test_admin_redact_works_if_user_kicked_or_banned(self) -> None: # we redacted 6 messages self.assertEqual(len(matches), 6) + def test_redactions_for_remote_user_succeed_with_admin_priv_in_room(self) -> None: + """ + Test that if the admin requester has privileges in a room, redaction requests + succeed for a remote user + """ + + # inject some messages from remote user and collect event ids + original_message_ids = [] + for i in range(5): + event = self.get_success( + inject_event( + self.hs, + room_id=self.rm1, + type="m.room.message", + sender="@remote:remote_server", + content={"msgtype": "m.text", "body": f"nefarious_chatter{i}"}, + ) + ) + original_message_ids.append(event.event_id) + + # send a request to redact a remote user's messages in a room. + # the server admin created this room and has admin privilege in room + channel = self.make_request( + "POST", + "/_synapse/admin/v1/user/@remote:remote_server/redact", + content={"rooms": [self.rm1]}, + access_token=self.admin_tok, + ) + self.assertEqual(channel.code, 200) + id = channel.json_body.get("redact_id") + + # check that there were no failed redactions + channel = self.make_request( + "GET", + f"/_synapse/admin/v1/user/redact_status/{id}", + access_token=self.admin_tok, + ) + self.assertEqual(channel.code, 200) + self.assertEqual(channel.json_body.get("status"), "complete") + failed_redactions = channel.json_body.get("failed_redactions") + self.assertEqual(failed_redactions, {}) + + filter = json.dumps({"types": [EventTypes.Redaction]}) + channel = self.make_request( + "GET", + f"rooms/{self.rm1}/messages?filter={filter}&limit=50", + access_token=self.admin_tok, + ) + self.assertEqual(channel.code, 200) + + for event in channel.json_body["chunk"]: + for event_id in original_message_ids: + if event["type"] == "m.room.redaction" and event["redacts"] == event_id: + original_message_ids.remove(event_id) + break + # we originally sent 5 messages so 5 should be redacted + self.assertEqual(len(original_message_ids), 0) + class UserRedactionBackgroundTaskTestCase(BaseMultiWorkerStreamTestCase): servlets = [ From 6306de8e162479c51032cb8440ced9b125ed0927 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 3 Jan 2025 12:23:29 -0500 Subject: [PATCH 12/13] Refactor get_profile: do not return missing fields. (#18063) 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. --- changelog.d/18063.misc | 1 + synapse/api/constants.py | 5 +++++ synapse/handlers/profile.py | 14 +++++++++----- synapse/handlers/sso.py | 9 +++++---- synapse/handlers/user_directory.py | 16 +++++++++++++--- synapse/module_api/__init__.py | 18 ++++++++++-------- synapse/rest/client/profile.py | 9 +-------- 7 files changed, 44 insertions(+), 28 deletions(-) create mode 100644 changelog.d/18063.misc diff --git a/changelog.d/18063.misc b/changelog.d/18063.misc new file mode 100644 index 00000000000..424bf25408f --- /dev/null +++ b/changelog.d/18063.misc @@ -0,0 +1 @@ +Refactor `get_profile` to no longer include fields with a value of `None`. diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 1206d1e00f3..9806e2b0fe3 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -320,3 +320,8 @@ class ApprovalNoticeMedium: class Direction(enum.Enum): BACKWARDS = "b" FORWARDS = "f" + + +class ProfileFields: + DISPLAYNAME: Final = "displayname" + AVATAR_URL: Final = "avatar_url" diff --git a/synapse/handlers/profile.py b/synapse/handlers/profile.py index ac4544ca4c0..22eedcb54f6 100644 --- a/synapse/handlers/profile.py +++ b/synapse/handlers/profile.py @@ -22,6 +22,7 @@ import random from typing import TYPE_CHECKING, List, Optional, Union +from synapse.api.constants import ProfileFields from synapse.api.errors import ( AuthError, Codes, @@ -83,7 +84,7 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi Returns: A JSON dictionary. For local queries this will include the displayname and avatar_url - fields. For remote queries it may contain arbitrary information. + fields, if set. For remote queries it may contain arbitrary information. """ target_user = UserID.from_string(user_id) @@ -92,10 +93,13 @@ async def get_profile(self, user_id: str, ignore_backoff: bool = True) -> JsonDi if profileinfo.display_name is None and profileinfo.avatar_url is None: raise SynapseError(404, "Profile was not found", Codes.NOT_FOUND) - return { - "displayname": profileinfo.display_name, - "avatar_url": profileinfo.avatar_url, - } + # Do not include display name or avatar if unset. + ret = {} + if profileinfo.display_name is not None: + ret[ProfileFields.DISPLAYNAME] = profileinfo.display_name + if profileinfo.avatar_url is not None: + ret[ProfileFields.AVATAR_URL] = profileinfo.avatar_url + return ret else: try: result = await self.federation.make_query( diff --git a/synapse/handlers/sso.py b/synapse/handlers/sso.py index ee74289b6c4..cee2eefbb37 100644 --- a/synapse/handlers/sso.py +++ b/synapse/handlers/sso.py @@ -43,7 +43,7 @@ from twisted.web.iweb import IRequest from twisted.web.server import Request -from synapse.api.constants import LoginType +from synapse.api.constants import LoginType, ProfileFields from synapse.api.errors import Codes, NotFoundError, RedirectException, SynapseError from synapse.config.sso import SsoAttributeRequirement from synapse.handlers.device import DeviceHandler @@ -813,9 +813,10 @@ def is_allowed_mime_type(content_type: str) -> bool: # bail if user already has the same avatar profile = await self._profile_handler.get_profile(user_id) - if profile["avatar_url"] is not None: - server_name = profile["avatar_url"].split("/")[-2] - media_id = profile["avatar_url"].split("/")[-1] + if ProfileFields.AVATAR_URL in profile: + avatar_url_parts = profile[ProfileFields.AVATAR_URL].split("/") + server_name = avatar_url_parts[-2] + media_id = avatar_url_parts[-1] if self._is_mine_server_name(server_name): media = await self._media_repo.store.get_local_media(media_id) # type: ignore[has-type] if media is not None and upload_name == media.upload_name: diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 1281929d384..f88d39b38f0 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -26,7 +26,13 @@ from twisted.internet.interfaces import IDelayedCall import synapse.metrics -from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules, Membership +from synapse.api.constants import ( + EventTypes, + HistoryVisibility, + JoinRules, + Membership, + ProfileFields, +) from synapse.api.errors import Codes, SynapseError from synapse.handlers.state_deltas import MatchChange, StateDeltasHandler from synapse.metrics.background_process_metrics import run_as_background_process @@ -756,6 +762,10 @@ async def _unsafe_refresh_remote_profiles_for_remote_server( await self.store.update_profile_in_user_dir( user_id, - display_name=non_null_str_or_none(profile.get("displayname")), - avatar_url=non_null_str_or_none(profile.get("avatar_url")), + display_name=non_null_str_or_none( + profile.get(ProfileFields.DISPLAYNAME) + ), + avatar_url=non_null_str_or_none( + profile.get(ProfileFields.AVATAR_URL) + ), ) diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index f6bfd93d3ce..2a2f821427d 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -45,6 +45,7 @@ from twisted.web.resource import Resource from synapse.api import errors +from synapse.api.constants import ProfileFields from synapse.api.errors import SynapseError from synapse.api.presence import UserPresenceState from synapse.config import ConfigError @@ -1086,7 +1087,10 @@ async def update_room_membership( content = {} # Set the profile if not already done by the module. - if "avatar_url" not in content or "displayname" not in content: + if ( + ProfileFields.AVATAR_URL not in content + or ProfileFields.DISPLAYNAME not in content + ): try: # Try to fetch the user's profile. profile = await self._hs.get_profile_handler().get_profile( @@ -1095,8 +1099,8 @@ async def update_room_membership( except SynapseError as e: # If the profile couldn't be found, use default values. profile = { - "displayname": target_user_id.localpart, - "avatar_url": None, + ProfileFields.DISPLAYNAME: target_user_id.localpart, + ProfileFields.AVATAR_URL: None, } if e.code != 404: @@ -1109,11 +1113,9 @@ async def update_room_membership( ) # Set the profile where it needs to be set. - if "avatar_url" not in content: - content["avatar_url"] = profile["avatar_url"] - - if "displayname" not in content: - content["displayname"] = profile["displayname"] + for field_name in [ProfileFields.AVATAR_URL, ProfileFields.DISPLAYNAME]: + if field_name not in content and field_name in profile: + content[field_name] = profile[field_name] event_id, _ = await self._hs.get_room_member_handler().update_membership( requester=requester, diff --git a/synapse/rest/client/profile.py b/synapse/rest/client/profile.py index 7a95b9445d1..ef59582865f 100644 --- a/synapse/rest/client/profile.py +++ b/synapse/rest/client/profile.py @@ -227,14 +227,7 @@ async def on_GET( user = UserID.from_string(user_id) await self.profile_handler.check_profile_query_allowed(user, requester_user) - displayname = await self.profile_handler.get_displayname(user) - avatar_url = await self.profile_handler.get_avatar_url(user) - - ret = {} - if displayname is not None: - ret["displayname"] = displayname - if avatar_url is not None: - ret["avatar_url"] = avatar_url + ret = await self.profile_handler.get_profile(user_id) return 200, ret From b3ba501c526bf15240bb8cc635858d05cd702c2c Mon Sep 17 00:00:00 2001 From: Mathieu Velten Date: Mon, 6 Jan 2025 16:32:18 +0100 Subject: [PATCH 13/13] Properly purge state groups tables when purging a room (#18024) Currently purging a complex room can lead to a lot of orphaned rows left behind in the state groups tables. It seems it is because we are loosing track of state groups sometimes. This change uses the `room_id` indexed column of `state_groups` table to decide what to delete instead of doing an indirection through `event_to_state_groups`. Related to https://github.com/element-hq/synapse/issues/3364. ### Pull Request Checklist * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Erik Johnston --- changelog.d/18024.bugfix | 1 + synapse/storage/controllers/purge_events.py | 4 +- .../storage/databases/main/purge_events.py | 36 +++-------- synapse/storage/databases/state/store.py | 60 +++++++------------ tests/rest/admin/test_room.py | 2 +- 5 files changed, 34 insertions(+), 69 deletions(-) create mode 100644 changelog.d/18024.bugfix diff --git a/changelog.d/18024.bugfix b/changelog.d/18024.bugfix new file mode 100644 index 00000000000..956f43f0362 --- /dev/null +++ b/changelog.d/18024.bugfix @@ -0,0 +1 @@ +Properly purge state groups tables when purging a room with the admin API. diff --git a/synapse/storage/controllers/purge_events.py b/synapse/storage/controllers/purge_events.py index e794b370c25..15c04ffef89 100644 --- a/synapse/storage/controllers/purge_events.py +++ b/synapse/storage/controllers/purge_events.py @@ -42,8 +42,8 @@ async def purge_room(self, room_id: str) -> None: """Deletes all record of a room""" with nested_logging_context(room_id): - state_groups_to_delete = await self.stores.main.purge_room(room_id) - await self.stores.state.purge_room_state(room_id, state_groups_to_delete) + await self.stores.main.purge_room(room_id) + await self.stores.state.purge_room_state(room_id) async def purge_history( self, room_id: str, token: str, delete_local_events: bool diff --git a/synapse/storage/databases/main/purge_events.py b/synapse/storage/databases/main/purge_events.py index c195685af83..ebdeb8fbd70 100644 --- a/synapse/storage/databases/main/purge_events.py +++ b/synapse/storage/databases/main/purge_events.py @@ -20,7 +20,7 @@ # import logging -from typing import Any, List, Set, Tuple, cast +from typing import Any, Set, Tuple, cast from synapse.api.errors import SynapseError from synapse.storage.database import LoggingTransaction @@ -332,7 +332,7 @@ def _purge_history_txn( return referenced_state_groups - async def purge_room(self, room_id: str) -> List[int]: + async def purge_room(self, room_id: str) -> None: """Deletes all record of a room Args: @@ -348,7 +348,7 @@ async def purge_room(self, room_id: str) -> List[int]: # purge any of those rows which were added during the first. logger.info("[purge] Starting initial main purge of [1/2]") - state_groups_to_delete = await self.db_pool.runInteraction( + await self.db_pool.runInteraction( "purge_room", self._purge_room_txn, room_id=room_id, @@ -356,18 +356,15 @@ async def purge_room(self, room_id: str) -> List[int]: ) logger.info("[purge] Starting secondary main purge of [2/2]") - state_groups_to_delete.extend( - await self.db_pool.runInteraction( - "purge_room", - self._purge_room_txn, - room_id=room_id, - ), + await self.db_pool.runInteraction( + "purge_room", + self._purge_room_txn, + room_id=room_id, ) - logger.info("[purge] Done with main purge") - return state_groups_to_delete + logger.info("[purge] Done with main purge") - def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: + def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> None: # This collides with event persistence so we cannot write new events and metadata into # a room while deleting it or this transaction will fail. if isinstance(self.database_engine, PostgresEngine): @@ -381,19 +378,6 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: # take a while! txn.execute("SET LOCAL statement_timeout = 0") - # First, fetch all the state groups that should be deleted, before - # we delete that information. - txn.execute( - """ - SELECT DISTINCT state_group FROM events - INNER JOIN event_to_state_groups USING(event_id) - WHERE events.room_id = ? - """, - (room_id,), - ) - - state_groups = [row[0] for row in txn] - # Get all the auth chains that are referenced by events that are to be # deleted. txn.execute( @@ -513,5 +497,3 @@ def _purge_room_txn(self, txn: LoggingTransaction, room_id: str) -> List[int]: # periodically anyway (https://github.com/matrix-org/synapse/issues/5888) self._invalidate_caches_for_room_and_stream(txn, room_id) - - return state_groups diff --git a/synapse/storage/databases/state/store.py b/synapse/storage/databases/state/store.py index f7a59c8992d..9944f90015c 100644 --- a/synapse/storage/databases/state/store.py +++ b/synapse/storage/databases/state/store.py @@ -840,60 +840,42 @@ async def get_previous_state_groups( return dict(rows) - async def purge_room_state( - self, room_id: str, state_groups_to_delete: Collection[int] - ) -> None: - """Deletes all record of a room from state tables - - Args: - room_id: - state_groups_to_delete: State groups to delete - """ - - logger.info("[purge] Starting state purge") - await self.db_pool.runInteraction( + async def purge_room_state(self, room_id: str) -> None: + return await self.db_pool.runInteraction( "purge_room_state", self._purge_room_state_txn, room_id, - state_groups_to_delete, ) - logger.info("[purge] Done with state purge") def _purge_room_state_txn( self, txn: LoggingTransaction, room_id: str, - state_groups_to_delete: Collection[int], ) -> None: - # first we have to delete the state groups states - logger.info("[purge] removing %s from state_groups_state", room_id) - - self.db_pool.simple_delete_many_txn( - txn, - table="state_groups_state", - column="state_group", - values=state_groups_to_delete, - keyvalues={}, - ) - - # ... and the state group edges + # Delete all edges that reference a state group linked to room_id logger.info("[purge] removing %s from state_group_edges", room_id) + txn.execute( + """ + DELETE FROM state_group_edges AS sge WHERE sge.state_group IN ( + SELECT id FROM state_groups AS sg WHERE sg.room_id = ? + )""", + (room_id,), + ) - self.db_pool.simple_delete_many_txn( - txn, - table="state_group_edges", - column="state_group", - values=state_groups_to_delete, - keyvalues={}, + # state_groups_state table has a room_id column but no index on it, unlike state_groups, + # so we delete them by matching the room_id through the state_groups table. + logger.info("[purge] removing %s from state_groups_state", room_id) + txn.execute( + """ + DELETE FROM state_groups_state AS sgs WHERE sgs.state_group IN ( + SELECT id FROM state_groups AS sg WHERE sg.room_id = ? + )""", + (room_id,), ) - # ... and the state groups logger.info("[purge] removing %s from state_groups", room_id) - - self.db_pool.simple_delete_many_txn( + self.db_pool.simple_delete_txn( txn, table="state_groups", - column="id", - values=state_groups_to_delete, - keyvalues={}, + keyvalues={"room_id": room_id}, ) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 95ed7364510..99df5915290 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -3050,7 +3050,7 @@ def _block_room(self, room_id: str) -> None: "pusher_throttle", "room_account_data", "room_tags", - # "state_groups", # Current impl leaves orphaned state groups around. + "state_groups", "state_groups_state", "federation_inbound_events_staging", ]