Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implements admin API to lock an user (MSC3939) #15870

Merged
merged 19 commits into from
Aug 10, 2023
1 change: 1 addition & 0 deletions changelog.d/15870.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implements an admin API to lock an user without deactivating them. Based on [MSC3939](https://github.com/matrix-org/matrix-spec-proposals/pull/3939).
1 change: 1 addition & 0 deletions docs/admin_api/user_admin_api.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ Body parameters:
- `admin` - **bool**, optional, defaults to `false`. Whether the user is a homeserver administrator,
granting them access to the Admin API, among other things.
- `deactivated` - **bool**, optional. If unspecified, deactivation state will be left unchanged.
- `locked` - **bool**, optional. If unspecified, locked state will be left unchanged.

Note: the `password` field must also be set if both of the following are true:
- `deactivated` is set to `false` and the user was previously deactivated (you are reactivating this user)
Expand Down
2 changes: 2 additions & 0 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -3631,13 +3631,15 @@ This option has the following sub-options:
* `prefer_local_users`: Defines whether to prefer local users in search query results.
If set to true, local users are more likely to appear above remote users when searching the
user directory. Defaults to false.
* `show_locked_users`: Defines whether to show locked users in search query results. Defaults to false.

Example configuration:
```yaml
user_directory:
enabled: false
search_all_users: true
prefer_local_users: true
show_locked_users: true
```
---
### `user_consent`
Expand Down
2 changes: 1 addition & 1 deletion synapse/_scripts/synapse_port_db.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
"redactions": ["have_censored"],
"room_stats_state": ["is_federatable"],
"rooms": ["is_public", "has_auth_chain_index"],
"users": ["shadow_banned", "approved"],
"users": ["shadow_banned", "approved", "locked"],
"un_partial_stated_event_stream": ["rejection_status_changed"],
"users_who_share_rooms": ["share_private"],
"per_user_experimental_features": ["enabled"],
Expand Down
1 change: 1 addition & 0 deletions synapse/api/auth/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ async def get_user_by_req(
request: SynapseRequest,
allow_guest: bool = False,
allow_expired: bool = False,
allow_locked: bool = False,
) -> Requester:
"""Get a registered user's ID.

Expand Down
15 changes: 14 additions & 1 deletion synapse/api/auth/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ async def get_user_by_req(
request: SynapseRequest,
allow_guest: bool = False,
allow_expired: bool = False,
allow_locked: bool = False,
) -> Requester:
"""Get a registered user's ID.

Expand All @@ -79,7 +80,7 @@ async def get_user_by_req(
parent_span = active_span()
with start_active_span("get_user_by_req"):
requester = await self._wrapped_get_user_by_req(
request, allow_guest, allow_expired
request, allow_guest, allow_expired, allow_locked
)

if parent_span:
Expand Down Expand Up @@ -107,6 +108,7 @@ async def _wrapped_get_user_by_req(
request: SynapseRequest,
allow_guest: bool,
allow_expired: bool,
allow_locked: bool,
) -> Requester:
"""Helper for get_user_by_req

Expand All @@ -126,6 +128,17 @@ async def _wrapped_get_user_by_req(
access_token, allow_expired=allow_expired
)

# Deny the request if the user account is locked.
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},
)

# Deny the request if the user account has expired.
# This check is only done for regular users, not appservice ones.
if not allow_expired:
Expand Down
13 changes: 13 additions & 0 deletions synapse/api/auth/msc3861_delegated.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from synapse.api.auth.base import BaseAuth
from synapse.api.errors import (
AuthError,
Codes,
HttpResponseException,
InvalidClientTokenError,
OAuthInsufficientScopeError,
Expand Down Expand Up @@ -196,6 +197,7 @@ async def get_user_by_req(
request: SynapseRequest,
allow_guest: bool = False,
allow_expired: bool = False,
allow_locked: bool = False,
) -> Requester:
access_token = self.get_access_token_from_request(request)

Expand All @@ -205,6 +207,17 @@ async def get_user_by_req(
# so that we don't provision the user if they don't have enough permission:
requester = await self.get_user_by_access_token(access_token, allow_expired)

# Deny the request if the user account is locked.
if not allow_locked and await self.store.get_user_locked_status(
Copy link
Contributor Author

@MatMaul MatMaul Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandhose @hughns do you have an insight about if we should do that in the MSC3861 case too ?

I feel that it is perhaps something that should be dealt with by the OIDC provider, but I also think that it doesn't hurt to have the synapse layer provides an extra mechanism on top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we keep the synapse layer here, if a mechanism exists to signal a locked user in OIDC (does it ?), we should probably convert that to the proper synapse API error code (introduced by MSC3939).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently implemented account locking in MAS. This should be implemented when Synapse introspects the token ; there is no standard on the introspection API to signal that, but we can do something somewhat custom. It's already on my radar

requester.user.to_string()
):
raise AuthError(
401,
"User account has been locked",
errcode=Codes.USER_LOCKED,
additional_fields={"soft_logout": True},
)

if not allow_guest and requester.is_guest:
raise OAuthInsufficientScopeError([SCOPE_MATRIX_API])

Expand Down
2 changes: 2 additions & 0 deletions synapse/api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ 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"

# Part of MSC3848
# https://github.com/matrix-org/matrix-spec-proposals/pull/3848
Expand Down
1 change: 1 addition & 0 deletions synapse/config/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,4 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
self.user_directory_search_prefer_local_users = user_directory_config.get(
"prefer_local_users", False
)
self.show_locked_users = user_directory_config.get("show_locked_users", False)
1 change: 1 addition & 0 deletions synapse/handlers/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]:
"name",
"admin",
"deactivated",
"locked",
"shadow_banned",
"creation_ts",
"appservice_id",
Expand Down
5 changes: 4 additions & 1 deletion synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def __init__(self, hs: "HomeServer"):
self.is_mine_id = hs.is_mine_id
self.update_user_directory = hs.config.worker.should_update_user_directory
self.search_all_users = hs.config.userdirectory.user_directory_search_all_users
self.show_locked_users = hs.config.userdirectory.show_locked_users
self._spam_checker_module_callbacks = hs.get_module_api_callbacks().spam_checker
self._hs = hs

Expand Down Expand Up @@ -144,7 +145,9 @@ async def search_users(
]
}
"""
results = await self.store.search_user_dir(user_id, search_term, limit)
results = await self.store.search_user_dir(
user_id, search_term, limit, self.show_locked_users
)

# Remove any spammy users from the results.
non_spammy_users = []
Expand Down
17 changes: 17 additions & 0 deletions synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,17 @@ async def on_PUT(
HTTPStatus.BAD_REQUEST, "'deactivated' parameter is not of type boolean"
)

lock = body.get("locked", False)
if not isinstance(lock, bool):
raise SynapseError(
HTTPStatus.BAD_REQUEST, "'locked' parameter is not of type boolean"
)

if deactivate and lock:
raise SynapseError(
HTTPStatus.BAD_REQUEST, "An user can't be deactivated and locked"
)

approved: Optional[bool] = None
if "approved" in body and self._msc3866_enabled:
approved = body["approved"]
Expand Down Expand Up @@ -397,6 +408,12 @@ async def on_PUT(
target_user.to_string()
)

if "locked" in body:
if lock and not user["locked"]:
await self.store.set_user_locked_status(user_id, True)
elif not lock and user["locked"]:
await self.store.set_user_locked_status(user_id, False)

if "user_type" in body:
await self.store.set_user_type(target_user, user_type)

Expand Down
8 changes: 6 additions & 2 deletions synapse/rest/client/logout.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ def __init__(self, hs: "HomeServer"):
self._device_handler = handler

async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request, allow_expired=True)
requester = await self.auth.get_user_by_req(
request, allow_expired=True, allow_locked=True
)

if requester.device_id is None:
# The access token wasn't associated with a device.
Expand All @@ -67,7 +69,9 @@ def __init__(self, hs: "HomeServer"):
self._device_handler = handler

async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request, allow_expired=True)
requester = await self.auth.get_user_by_req(
request, allow_expired=True, allow_locked=True
)
user_id = requester.user.to_string()

# first delete all of the user's devices
Expand Down
62 changes: 58 additions & 4 deletions synapse/storage/databases/main/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,8 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]:
name, password_hash, is_guest, admin, consent_version, consent_ts,
consent_server_notice_sent, appservice_id, creation_ts, user_type,
deactivated, COALESCE(shadow_banned, FALSE) AS shadow_banned,
COALESCE(approved, TRUE) AS approved
COALESCE(approved, TRUE) AS approved,
COALESCE(locked, FALSE) AS locked
FROM users
WHERE name = ?
""",
Expand All @@ -230,10 +231,15 @@ def get_user_by_id_txn(txn: LoggingTransaction) -> Optional[Dict[str, Any]]:
# want to make sure we're returning the right type of data.
# Note: when adding a column name to this list, be wary of NULLable columns,
# since NULL values will be turned into False.
boolean_columns = ["admin", "deactivated", "shadow_banned", "approved"]
boolean_columns = [
"admin",
"deactivated",
"shadow_banned",
"approved",
"locked",
]
for column in boolean_columns:
if not isinstance(row[column], bool):
row[column] = bool(row[column])
row[column] = bool(row[column])

return row

Expand Down Expand Up @@ -1116,6 +1122,27 @@ async def get_user_deactivated_status(self, user_id: str) -> bool:
# Convert the integer into a boolean.
return res == 1

@cached()
async def get_user_locked_status(self, user_id: str) -> bool:
"""Retrieve the value for the `locked` property for the provided user.

Args:
user_id: The ID of the user to retrieve the status for.

Returns:
True if the user was locked, false if the user is still active.
"""

res = await self.db_pool.simple_select_one_onecol(
table="users",
keyvalues={"name": user_id},
retcol="locked",
desc="get_user_locked_status",
)

# Convert the potential integer into a boolean.
return bool(res)

async def get_threepid_validation_session(
self,
medium: Optional[str],
Expand Down Expand Up @@ -2111,6 +2138,33 @@ def set_user_deactivated_status_txn(
self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,))
txn.call_after(self.is_guest.invalidate, (user_id,))

async def set_user_locked_status(self, user_id: str, locked: bool) -> None:
"""Set the `locked` property for the provided user to the provided value.

Args:
user_id: The ID of the user to set the status for.
locked: The value to set for `locked`.
"""

await self.db_pool.runInteraction(
"set_user_locked_status",
self.set_user_locked_status_txn,
user_id,
locked,
)

def set_user_locked_status_txn(
self, txn: LoggingTransaction, user_id: str, locked: bool
) -> None:
self.db_pool.simple_update_one_txn(
txn=txn,
table="users",
keyvalues={"name": user_id},
updatevalues={"locked": locked},
)
self._invalidate_cache_and_stream(txn, self.get_user_locked_status, (user_id,))
self._invalidate_cache_and_stream(txn, self.get_user_by_id, (user_id,))

def update_user_approval_status_txn(
self, txn: LoggingTransaction, user_id: str, approved: bool
) -> None:
Expand Down
11 changes: 10 additions & 1 deletion synapse/storage/databases/main/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,11 @@ async def get_user_directory_stream_pos(self) -> Optional[int]:
)

async def search_user_dir(
self, user_id: str, search_term: str, limit: int
self,
user_id: str,
search_term: str,
limit: int,
show_locked_users: bool = False,
) -> SearchResult:
"""Searches for users in directory

Expand Down Expand Up @@ -1029,6 +1033,9 @@ async def search_user_dir(
)
"""

if not show_locked_users:
where_clause += " AND (u.locked IS NULL OR u.locked = FALSE)"

# We allow manipulating the ranking algorithm by injecting statements
# based on config options.
additional_ordering_statements = []
Expand Down Expand Up @@ -1060,6 +1067,7 @@ async def search_user_dir(
SELECT d.user_id AS user_id, display_name, avatar_url
FROM matching_users as t
INNER JOIN user_directory AS d USING (user_id)
LEFT JOIN users AS u ON t.user_id = u.name
WHERE
%(where_clause)s
ORDER BY
Expand Down Expand Up @@ -1115,6 +1123,7 @@ async def search_user_dir(
SELECT d.user_id AS user_id, display_name, avatar_url
FROM user_directory_search as t
INNER JOIN user_directory AS d USING (user_id)
LEFT JOIN users AS u ON t.user_id = u.name
WHERE
%(where_clause)s
AND value MATCH ?
Expand Down
16 changes: 16 additions & 0 deletions synapse/storage/schema/main/delta/80/01_users_alter_locked.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/* Copyright 2023 The Matrix.org Foundation C.I.C.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

ALTER TABLE users ADD locked BOOLEAN DEFAULT FALSE NOT NULL;
3 changes: 3 additions & 0 deletions tests/api/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def test_get_user_by_req_user_valid_token(self) -> None:
)
self.store.get_user_by_access_token = simple_async_mock(user_info)
self.store.mark_access_token_as_used = simple_async_mock(None)
self.store.get_user_locked_status = simple_async_mock(False)

request = Mock(args={})
request.args[b"access_token"] = [self.test_token]
Expand Down Expand Up @@ -293,6 +294,7 @@ def test_get_user_by_req__puppeted_token__not_tracking_puppeted_mau(self) -> Non
)
self.store.insert_client_ip = simple_async_mock(None)
self.store.mark_access_token_as_used = simple_async_mock(None)
self.store.get_user_locked_status = simple_async_mock(False)
request = Mock(args={})
request.getClientAddress.return_value.host = "127.0.0.1"
request.args[b"access_token"] = [self.test_token]
Expand All @@ -311,6 +313,7 @@ def test_get_user_by_req__puppeted_token__tracking_puppeted_mau(self) -> None:
token_used=True,
)
)
self.store.get_user_locked_status = simple_async_mock(False)
self.store.insert_client_ip = simple_async_mock(None)
self.store.mark_access_token_as_used = simple_async_mock(None)
request = Mock(args={})
Expand Down
Loading
Loading