Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

…am_cache

.github/workflows/release-artifacts.yml

@@ -212,7 +212,8 @@ jobs:
           mv debs*/* debs/
           tar -cvJf debs.tar.xz debs
       - name: Attach to release
-        uses: softprops/action-gh-release@v2
+        # Pinned to work around https://github.com/softprops/action-gh-release/issues/445
+        uses: softprops/[email protected]
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
         with:

changelog.d/17962.misc

@@ -0,0 +1 @@
+Fix new scheduled tasks jumping the queue.

changelog.d/17965.feature

@@ -0,0 +1 @@
+Use stable `M_USER_LOCKED` error code for locked accounts, as per [Matrix 1.12](https://spec.matrix.org/v1.12/client-server-api/#account-locking).

changelog.d/17970.bugfix

@@ -0,0 +1 @@
+Fix release process to not create duplicate releases.

changelog.d/17972.misc

@@ -0,0 +1 @@
+Consolidate SSO redirects through `/_matrix/client/v3/login/sso/redirect(/{idpId})`.

synapse/api/errors.py

@@ -87,8 +87,7 @@ class Codes(str, Enum):
     WEAK_PASSWORD = "M_WEAK_PASSWORD"
     INVALID_SIGNATURE = "M_INVALID_SIGNATURE"
     USER_DEACTIVATED = "M_USER_DEACTIVATED"
-    # USER_LOCKED = "M_USER_LOCKED"
-    USER_LOCKED = "ORG_MATRIX_MSC3939_USER_LOCKED"
+    USER_LOCKED = "M_USER_LOCKED"
     NOT_YET_UPLOADED = "M_NOT_YET_UPLOADED"
     CANNOT_OVERWRITE_MEDIA = "M_CANNOT_OVERWRITE_MEDIA"
 

synapse/api/urls.py

@@ -23,7 +23,8 @@
 
 import hmac
 from hashlib import sha256
-from urllib.parse import urlencode
+from typing import Optional
+from urllib.parse import urlencode, urljoin
 
 from synapse.config import ConfigError
 from synapse.config.homeserver import HomeServerConfig
@@ -66,3 +67,42 @@ def build_user_consent_uri(self, user_id: str) -> str:
             urlencode({"u": user_id, "h": mac}),
         )
         return consent_uri
+
+
+class LoginSSORedirectURIBuilder:
+    def __init__(self, hs_config: HomeServerConfig):
+        self._public_baseurl = hs_config.server.public_baseurl
+
+    def build_login_sso_redirect_uri(
+        self, *, idp_id: Optional[str], client_redirect_url: str
+    ) -> str:
+        """Build a `/login/sso/redirect` URI for the given identity provider.
+
+        Builds `/_matrix/client/v3/login/sso/redirect/{idpId}?redirectUrl=xxx` when `idp_id` is specified.
+        Otherwise, builds `/_matrix/client/v3/login/sso/redirect?redirectUrl=xxx` when `idp_id` is `None`.
+
+        Args:
+            idp_id: Optional ID of the identity provider
+            client_redirect_url: URL to redirect the user to after login
+
+        Returns
+            The URI to follow when choosing a specific identity provider.
+        """
+        base_url = urljoin(
+            self._public_baseurl,
+            f"{CLIENT_API_PREFIX}/v3/login/sso/redirect",
+        )
+
+        serialized_query_parameters = urlencode({"redirectUrl": client_redirect_url})
+
+        if idp_id:
+            resultant_url = urljoin(
+                # We have to add a trailing slash to the base URL to ensure that the
+                # last path segment is not stripped away when joining with another path.
+                f"{base_url}/",
+                f"{idp_id}?{serialized_query_parameters}",
+            )
+        else:
+            resultant_url = f"{base_url}?{serialized_query_parameters}"
+
+        return resultant_url

synapse/config/cas.py

@@ -20,7 +20,7 @@
 #
 #
 
-from typing import Any, List
+from typing import Any, List, Optional
 
 from synapse.config.sso import SsoAttributeRequirement
 from synapse.types import JsonDict
@@ -46,7 +46,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
 
             # TODO Update this to a _synapse URL.
             public_baseurl = self.root.server.public_baseurl
-            self.cas_service_url = public_baseurl + "_matrix/client/r0/login/cas/ticket"
+            self.cas_service_url: Optional[str] = (
+                public_baseurl + "_matrix/client/r0/login/cas/ticket"
+            )
 
             self.cas_protocol_version = cas_config.get("protocol_version")
             if (

synapse/config/server.py

@@ -332,8 +332,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
             logger.info("Using default public_baseurl %s", public_baseurl)
         else:
             self.serve_client_wellknown = True
+            # Ensure that public_baseurl ends with a trailing slash
             if public_baseurl[-1] != "/":
                 public_baseurl += "/"
+
+        # Scrutinize user-provided config
+        if not isinstance(public_baseurl, str):
+            raise ConfigError("Must be a string", ("public_baseurl",))
+
         self.public_baseurl = public_baseurl
 
         # check that public_baseurl is valid

synapse/replication/tcp/commands.py

@@ -495,7 +495,7 @@ def to_line(self) -> str:
 
 
 class NewActiveTaskCommand(_SimpleCommand):
-    """Sent to inform instance handling background tasks that a new active task is available to run.
+    """Sent to inform instance handling background tasks that a new task is ready to run.
 
     Format::
 

synapse/replication/tcp/handler.py

@@ -727,7 +727,7 @@ def on_NEW_ACTIVE_TASK(
     ) -> None:
         """Called when get a new NEW_ACTIVE_TASK command."""
         if self._task_scheduler:
-            self._task_scheduler.launch_task_by_id(cmd.data)
+            self._task_scheduler.on_new_task(cmd.data)
 
     def new_connection(self, connection: IReplicationConnection) -> None:
         """Called when we have a new connection."""

synapse/rest/synapse/client/pick_idp.py

@@ -21,6 +21,7 @@
 import logging
 from typing import TYPE_CHECKING
 
+from synapse.api.urls import LoginSSORedirectURIBuilder
 from synapse.http.server import (
     DirectServeHtmlResource,
     finish_request,
@@ -49,32 +50,32 @@ def __init__(self, hs: "HomeServer"):
             hs.config.sso.sso_login_idp_picker_template
         )
         self._server_name = hs.hostname
+        self._public_baseurl = hs.config.server.public_baseurl
+        self._login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config)
 
     async def _async_render_GET(self, request: SynapseRequest) -> None:
         client_redirect_url = parse_string(
             request, "redirectUrl", required=True, encoding="utf-8"
         )
         idp = parse_string(request, "idp", required=False)
 
-        # if we need to pick an IdP, do so
+        # If we need to pick an IdP, do so
         if not idp:
             return await self._serve_id_picker(request, client_redirect_url)
 
-        # otherwise, redirect to the IdP's redirect URI
-        providers = self._sso_handler.get_identity_providers()
-        auth_provider = providers.get(idp)
-        if not auth_provider:
-            logger.info("Unknown idp %r", idp)
-            self._sso_handler.render_error(
-                request, "unknown_idp", "Unknown identity provider ID"
+        # Otherwise, redirect to the login SSO redirect endpoint for the given IdP
+        # (which will in turn take us to the the IdP's redirect URI).
+        #
+        # We could go directly to the IdP's redirect URI, but this way we ensure that
+        # the user goes through the same logic as normal flow. Additionally, if a proxy
+        # needs to intercept the request, it only needs to intercept the one endpoint.
+        sso_login_redirect_url = (
+            self._login_sso_redirect_url_builder.build_login_sso_redirect_uri(
+                idp_id=idp, client_redirect_url=client_redirect_url
             )
-            return
-
-        sso_url = await auth_provider.handle_redirect_request(
-            request, client_redirect_url.encode("utf8")
         )
-        logger.info("Redirecting to %s", sso_url)
-        request.redirect(sso_url)
+        logger.info("Redirecting to %s", sso_login_redirect_url)
+        request.redirect(sso_login_redirect_url)
         finish_request(request)
 
     async def _serve_id_picker(

synapse/util/task_scheduler.py

@@ -174,9 +174,10 @@ async def schedule_task(
             The id of the scheduled task
         """
         status = TaskStatus.SCHEDULED
+        start_now = False
         if timestamp is None or timestamp < self._clock.time_msec():
             timestamp = self._clock.time_msec()
-            status = TaskStatus.ACTIVE
+            start_now = True
 
         task = ScheduledTask(
             random_string(16),
@@ -190,9 +191,11 @@ async def schedule_task(
         )
         await self._store.insert_scheduled_task(task)
 
-        if status == TaskStatus.ACTIVE:
+        # If the task is ready to run immediately, run the scheduling algorithm now
+        # rather than waiting
+        if start_now:
             if self._run_background_tasks:
-                await self._launch_task(task)
+                self._launch_scheduled_tasks()
             else:
                 self._hs.get_replication_command_handler().send_new_active_task(task.id)
 
@@ -300,23 +303,13 @@ async def delete_task(self, id: str) -> None:
             raise Exception(f"Task {id} is currently ACTIVE and can't be deleted")
         await self._store.delete_scheduled_task(id)
 
-    def launch_task_by_id(self, id: str) -> None:
-        """Try launching the task with the given ID."""
-        # Don't bother trying to launch new tasks if we're already at capacity.
-        if len(self._running_tasks) >= TaskScheduler.MAX_CONCURRENT_RUNNING_TASKS:
-            return
-
-        run_as_background_process("launch_task_by_id", self._launch_task_by_id, id)
-
-    async def _launch_task_by_id(self, id: str) -> None:
-        """Helper async function for `launch_task_by_id`."""
-        task = await self.get_task(id)
-        if task:
-            await self._launch_task(task)
+    def on_new_task(self, task_id: str) -> None:
+        """Handle a notification that a new ready-to-run task has been added to the queue"""
+        # Just run the scheduler
+        self._launch_scheduled_tasks()
 
-    @wrap_as_background_process("launch_scheduled_tasks")
-    async def _launch_scheduled_tasks(self) -> None:
-        """Retrieve and launch scheduled tasks that should be running at that time."""
+    def _launch_scheduled_tasks(self) -> None:
+        """Retrieve and launch scheduled tasks that should be running at this time."""
         # Don't bother trying to launch new tasks if we're already at capacity.
         if len(self._running_tasks) >= TaskScheduler.MAX_CONCURRENT_RUNNING_TASKS:
             return
@@ -326,20 +319,26 @@ async def _launch_scheduled_tasks(self) -> None:
 
         self._launching_new_tasks = True
 
-        try:
-            for task in await self.get_tasks(
-                statuses=[TaskStatus.ACTIVE], limit=self.MAX_CONCURRENT_RUNNING_TASKS
-            ):
-                await self._launch_task(task)
-            for task in await self.get_tasks(
-                statuses=[TaskStatus.SCHEDULED],
-                max_timestamp=self._clock.time_msec(),
-                limit=self.MAX_CONCURRENT_RUNNING_TASKS,
-            ):
-                await self._launch_task(task)
-
-        finally:
-            self._launching_new_tasks = False
+        async def inner() -> None:
+            try:
+                for task in await self.get_tasks(
+                    statuses=[TaskStatus.ACTIVE],
+                    limit=self.MAX_CONCURRENT_RUNNING_TASKS,
+                ):
+                    # _launch_task will ignore tasks that we're already running, and
+                    # will also do nothing if we're already at the maximum capacity.
+                    await self._launch_task(task)
+                for task in await self.get_tasks(
+                    statuses=[TaskStatus.SCHEDULED],
+                    max_timestamp=self._clock.time_msec(),
+                    limit=self.MAX_CONCURRENT_RUNNING_TASKS,
+                ):
+                    await self._launch_task(task)
+
+            finally:
+                self._launching_new_tasks = False
+
+        run_as_background_process("launch_scheduled_tasks", inner)
 
     @wrap_as_background_process("clean_scheduled_tasks")
     async def _clean_scheduled_tasks(self) -> None:

tests/api/test_urls.py

@@ -0,0 +1,55 @@
+#
+# 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>.
+#
+
+
+from twisted.test.proto_helpers import MemoryReactor
+
+from synapse.api.urls import LoginSSORedirectURIBuilder
+from synapse.server import HomeServer
+from synapse.util import Clock
+
+from tests.unittest import HomeserverTestCase
+
+# a (valid) url with some annoying characters in.  %3D is =, %26 is &, %2B is +
+TRICKY_TEST_CLIENT_REDIRECT_URL = 'https://x?<ab c>&q"+%3D%2B"="fö%26=o"'
+
+
+class LoginSSORedirectURIBuilderTestCase(HomeserverTestCase):
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
+        self.login_sso_redirect_url_builder = LoginSSORedirectURIBuilder(hs.config)
+
+    def test_no_idp_id(self) -> None:
+        self.assertEqual(
+            self.login_sso_redirect_url_builder.build_login_sso_redirect_uri(
+                idp_id=None, client_redirect_url="http://example.com/redirect"
+            ),
+            "https://test/_matrix/client/v3/login/sso/redirect?redirectUrl=http%3A%2F%2Fexample.com%2Fredirect",
+        )
+
+    def test_explicit_idp_id(self) -> None:
+        self.assertEqual(
+            self.login_sso_redirect_url_builder.build_login_sso_redirect_uri(
+                idp_id="oidc-github", client_redirect_url="http://example.com/redirect"
+            ),
+            "https://test/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=http%3A%2F%2Fexample.com%2Fredirect",
+        )
+
+    def test_tricky_redirect_uri(self) -> None:
+        self.assertEqual(
+            self.login_sso_redirect_url_builder.build_login_sso_redirect_uri(
+                idp_id="oidc-github",
+                client_redirect_url=TRICKY_TEST_CLIENT_REDIRECT_URL,
+            ),
+            "https://test/_matrix/client/v3/login/sso/redirect/oidc-github?redirectUrl=https%3A%2F%2Fx%3F%3Cab+c%3E%26q%22%2B%253D%252B%22%3D%22f%C3%B6%2526%3Do%22",
+        )