From 2f9c9d5ebaa6b53d23aba2ec140499ff007e2bab Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Wed, 2 Jul 2025 12:18:33 -0400 Subject: [PATCH] Forbid locked users from using `POST /login` (#18594) Discussed in the [Synapse Dev room](https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$K4UojQtvaSpxSe35TWFXtKWGoAuHwHFcKo8qn2lwxSs?via=matrix.org&via=element.io&via=envs.net) ### 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/18594.bugfix | 1 + synapse/api/auth/internal.py | 8 ++------ synapse/api/errors.py | 14 ++++++++++++++ synapse/rest/client/login.py | 16 ++++++++++------ tests/rest/admin/test_user.py | 10 ++++++++++ 5 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 changelog.d/18594.bugfix diff --git a/changelog.d/18594.bugfix b/changelog.d/18594.bugfix new file mode 100644 index 000000000..3293a731f --- /dev/null +++ b/changelog.d/18594.bugfix @@ -0,0 +1 @@ +Respond with 401 & `M_USER_LOCKED` when a locked user calls `POST /login`, as per the spec. diff --git a/synapse/api/auth/internal.py b/synapse/api/auth/internal.py index 9fd4db68e..afc7b5c4a 100644 --- a/synapse/api/auth/internal.py +++ b/synapse/api/auth/internal.py @@ -29,6 +29,7 @@ from synapse.api.errors import ( InvalidClientTokenError, MissingClientTokenError, UnrecognizedRequestError, + UserLockedError, ) from synapse.http.site import SynapseRequest from synapse.logging.opentracing import active_span, force_tracing, start_active_span @@ -162,12 +163,7 @@ class InternalAuth(BaseAuth): if not allow_locked and await self.store.get_user_locked_status( requester.user.to_string() ): - raise AuthError( - 401, - "User account has been locked", - errcode=Codes.USER_LOCKED, - additional_fields={"soft_logout": True}, - ) + raise UserLockedError() # Deny the request if the user account has expired. # This check is only done for regular users, not appservice ones. diff --git a/synapse/api/errors.py b/synapse/api/errors.py index 601a09efe..b832c2f6a 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -306,6 +306,20 @@ class UserDeactivatedError(SynapseError): ) +class UserLockedError(SynapseError): + """The error returned to the client when the user attempted to access an + authenticated endpoint, but the account has been locked. + """ + + def __init__(self) -> None: + super().__init__( + code=HTTPStatus.UNAUTHORIZED, + msg="User account has been locked", + errcode=Codes.USER_LOCKED, + additional_fields={"soft_logout": True}, + ) + + class FederationDeniedError(SynapseError): """An error raised when the server tries to federate with a server which is not on its federation whitelist. diff --git a/synapse/rest/client/login.py b/synapse/rest/client/login.py index 72b219447..8a781f759 100644 --- a/synapse/rest/client/login.py +++ b/synapse/rest/client/login.py @@ -42,6 +42,7 @@ from synapse.api.errors import ( NotApprovedError, SynapseError, UserDeactivatedError, + UserLockedError, ) from synapse.api.ratelimiting import Ratelimiter from synapse.api.urls import CLIENT_API_PREFIX @@ -313,7 +314,7 @@ class LoginRestServlet(RestServlet): should_issue_refresh_token=should_issue_refresh_token, # The user represented by an appservice's configured sender_localpart # is not actually created in Synapse. - should_check_deactivated=qualified_user_id != appservice.sender, + should_check_deactivated_or_locked=qualified_user_id != appservice.sender, request_info=request_info, ) @@ -367,7 +368,7 @@ class LoginRestServlet(RestServlet): auth_provider_id: Optional[str] = None, should_issue_refresh_token: bool = False, auth_provider_session_id: Optional[str] = None, - should_check_deactivated: bool = True, + should_check_deactivated_or_locked: bool = True, *, request_info: RequestInfo, ) -> LoginResponse: @@ -389,8 +390,8 @@ class LoginRestServlet(RestServlet): should_issue_refresh_token: True if this login should issue a refresh token alongside the access token. auth_provider_session_id: The session ID got during login from the SSO IdP. - should_check_deactivated: True if the user should be checked for - deactivation status before logging in. + should_check_deactivated_or_locked: True if the user should be checked for + deactivation or locked status before logging in. This exists purely for appservice's configured sender_localpart which doesn't have an associated user in the database. @@ -415,11 +416,14 @@ class LoginRestServlet(RestServlet): ) user_id = canonical_uid - # If the account has been deactivated, do not proceed with the login. - if should_check_deactivated: + # If the account has been deactivated or locked, do not proceed with the login. + if should_check_deactivated_or_locked: deactivated = await self._main_store.get_user_deactivated_status(user_id) if deactivated: raise UserDeactivatedError("This account has been deactivated") + locked = await self._main_store.get_user_locked_status(user_id) + if locked: + raise UserLockedError() device_id = login_submission.get("device_id") diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py index 5f73dbdc4..e9c631e2a 100644 --- a/tests/rest/admin/test_user.py +++ b/tests/rest/admin/test_user.py @@ -2846,6 +2846,16 @@ class UserRestTestCase(unittest.HomeserverTestCase): self.assertEqual(Codes.USER_LOCKED, channel.json_body["errcode"]) self.assertTrue(channel.json_body["soft_logout"]) + # User is not authorized to log in anymore + channel = self.make_request( + "POST", + "/_matrix/client/r0/login", + {"type": "m.login.password", "user": "user", "password": "pass"}, + ) + self.assertEqual(401, channel.code, msg=channel.json_body) + self.assertEqual(Codes.USER_LOCKED, channel.json_body["errcode"]) + self.assertTrue(channel.json_body["soft_logout"]) + @override_config({"user_directory": {"enabled": True, "search_all_users": True}}) def test_locked_user_not_in_user_dir(self) -> None: # User is available in the user dir