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

Allow admins to require a manual approval process before new accounts can be used (using MSC3866) #13556

Merged
merged 24 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f7c9743
Add experimental config options and constant for MSC3866
babolivier Aug 18, 2022
1eaff08
Add storage support for checking and updating a user's approval status
babolivier Aug 18, 2022
685f76f
Block new accounts after registering if configured to do so
babolivier Aug 18, 2022
5d08fe2
Block login if a user requires approval and the server is configured …
babolivier Aug 18, 2022
7b532a9
Change admin APIs to support checking and updating the approval statu…
babolivier Aug 18, 2022
eedaed1
Changelog
babolivier Aug 18, 2022
ffaea1e
Use a boolean in the database schema
babolivier Aug 30, 2022
0230200
Incorporate review
babolivier Aug 31, 2022
562aa7a
Merge branch 'develop' of github.com:matrix-org/synapse into babolivi…
babolivier Sep 21, 2022
868ab64
Incorporate review
babolivier Sep 21, 2022
836aa32
Merge branch 'develop' of github.com:matrix-org/synapse into babolivi…
babolivier Sep 21, 2022
8d091b4
Correctly filter on bools, not ints
babolivier Sep 22, 2022
116fc53
Merge branch 'develop' of github.com:matrix-org/synapse into babolivi…
babolivier Sep 22, 2022
a87d2f7
Don't create a new device if the new user needs approval
babolivier Sep 22, 2022
08d85f5
Test that we raise the error on SSO logins
babolivier Sep 22, 2022
7585098
Test that we don't register devices for users needing approval
babolivier Sep 22, 2022
75cf999
Lint
babolivier Sep 22, 2022
f4a7f16
Merge branch 'develop' of github.com:matrix-org/synapse into babolivi…
babolivier Sep 26, 2022
df0c887
Incorporate review
babolivier Sep 27, 2022
3f93dda
Fix test
babolivier Sep 29, 2022
577967c
Lint
babolivier Sep 29, 2022
7a5425a
Incorporate review
babolivier Sep 29, 2022
560e160
Incorporate latest change in the MSC
babolivier Sep 29, 2022
7d71712
Add comment to try to catch bool()ing NULLs in the future
babolivier Sep 29, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13556.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow server admins to require a manual approval process before new accounts can be used (using [MSC3866](https://github.com/matrix-org/matrix-spec-proposals/pull/3866)).
2 changes: 2 additions & 0 deletions synapse/api/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ class Codes(str, Enum):

UNREDACTED_CONTENT_DELETED = "FI.MAU.MSC2815_UNREDACTED_CONTENT_DELETED"

USER_AWAITING_APPROVAL = "ORG.MATRIX.MSC3866_USER_AWAITING_APPROVAL"


class CodeMessageException(RuntimeError):
"""An exception with integer code and message string attributes.
Expand Down
19 changes: 19 additions & 0 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,25 @@

from typing import Any

import attr

from synapse.config._base import Config
from synapse.types import JsonDict


@attr.s(auto_attribs=True, frozen=True, slots=True)
class MSC3866Config:
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

wonder if we should be using the Pydantic models instead of attrs here so you get the validation? For some reason I'm under the impression that this is already done in some of the config, but feel free to punt it for now if it's too much faff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we're only doing it in http handlers. Also I'm not sure I see the point of validation to this extent for experimental, if-you-use-this-without-knowing-exactly-what-you're-doing-you're-on-your-own options.

Copy link
Contributor

Choose a reason for hiding this comment

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

We're free to use it wherever we want to. The original suggestion was to use it for config and I even prototyped this: #12651 (comment)

We don't have many examples of using Pydantic yet so I wouldn't force it on anyone. We would probably need some glue code for config too?

But I think it is pretty good at what it does, gives better error messages earlier and gives us an opportunity to propagate more types through Synapse.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion but presumably this won't always be an experimental option, at which point it will just get renamed into something unexperimental? So I don't really buy that we should do things less properly for experimental options, but it's only a minor thought

"""Configuration for MSC3866 (mandating approval for new users)"""

# Whether the base support for the approval process is enabled. This includes the
# ability for administrators to check and update the approval of users, even if no
# approval is currently required.
enabled: bool = False
# Whether to require that new users are approved by an admin before their account
# can be used. Note that this setting is ignored if 'enabled' is false.
require_approval_for_new_accounts: bool = False


class ExperimentalConfig(Config):
"""Config section for enabling experimental features"""

Expand Down Expand Up @@ -93,3 +108,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:

# MSC3852: Expose last seen user agent field on /_matrix/client/v3/devices.
self.msc3852_enabled: bool = experimental.get("msc3852_enabled", False)

# MSC3866: M_USER_AWAITING_APPROVAL error code
raw_msc3866_config = experimental.get("msc3866", {})
self.msc3866 = MSC3866Config(**raw_msc3866_config)
5 changes: 5 additions & 0 deletions synapse/handlers/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ def __init__(self, hs: "HomeServer"):
self.store = hs.get_datastores().main
self._storage_controllers = hs.get_storage_controllers()
self._state_storage_controller = self._storage_controllers.state
self._msc3866_enabled = hs.config.experimental.msc3866.enabled

async def get_whois(self, user: UserID) -> JsonDict:
connections = []
Expand Down Expand Up @@ -74,6 +75,10 @@ async def get_user(self, user: UserID) -> Optional[JsonDict]:
"is_guest",
}

if self._msc3866_enabled:
# Only include the approved flag if support for MSC3866 is enabled.
user_info_to_return.add("approved")

# Restrict returned keys to a known set.
user_info_dict = {
key: value
Expand Down
11 changes: 11 additions & 0 deletions synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,17 @@ async def check_user_exists(self, user_id: str) -> Optional[str]:
return res[0]
return None

async def is_user_approved(self, user_id: str) -> bool:
"""Checks if a user is approved and therefore can be allowed to log in.

Args:
user_id: the user to check the approval status of.

Returns:
A boolean that is True if the user is approved, False otherwise.
"""
return await self.store.is_user_approved(user_id)

async def _find_user_id_and_pwd_hash(
self, user_id: str
) -> Optional[Tuple[str, str]]:
Expand Down
8 changes: 8 additions & 0 deletions synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ async def register_user(
by_admin: bool = False,
user_agent_ips: Optional[List[Tuple[str, str]]] = None,
auth_provider_id: Optional[str] = None,
approved: bool = False,
) -> str:
"""Registers a new client on the server.

Expand All @@ -243,6 +244,8 @@ async def register_user(
user_agent_ips: Tuples of user-agents and IP addresses used
during the registration process.
auth_provider_id: The SSO IdP the user used, if any.
approved: True if the new user should be considered already
approved by an administrator.
Returns:
The registered user_id.
Raises:
Expand Down Expand Up @@ -304,6 +307,7 @@ async def register_user(
user_type=user_type,
address=address,
shadow_banned=shadow_banned,
approved=approved,
)

profile = await self.store.get_profileinfo(localpart)
Expand Down Expand Up @@ -692,6 +696,7 @@ async def register_with_store(
user_type: Optional[str] = None,
address: Optional[str] = None,
shadow_banned: bool = False,
approved: bool = False,
) -> None:
"""Register user in the datastore.

Expand All @@ -710,6 +715,7 @@ async def register_with_store(
api.constants.UserTypes, or None for a normal user.
address: the IP address used to perform the registration.
shadow_banned: Whether to shadow-ban the user
approved: Whether to mark the user as approved by an administrator
"""
if self.hs.config.worker.worker_app:
await self._register_client(
Expand All @@ -723,6 +729,7 @@ async def register_with_store(
user_type=user_type,
address=address,
shadow_banned=shadow_banned,
approved=approved,
)
else:
await self.store.register_user(
Expand All @@ -735,6 +742,7 @@ async def register_with_store(
admin=admin,
user_type=user_type,
shadow_banned=shadow_banned,
approved=approved,
)

# Only call the account validity module(s) on the main process, to avoid
Expand Down
5 changes: 5 additions & 0 deletions synapse/replication/http/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ async def _serialize_payload( # type: ignore[override]
user_type: Optional[str],
address: Optional[str],
shadow_banned: bool,
approved: bool,
) -> JsonDict:
"""
Args:
Expand All @@ -68,6 +69,8 @@ async def _serialize_payload( # type: ignore[override]
or None for a normal user.
address: the IP address used to perform the regitration.
shadow_banned: Whether to shadow-ban the user
approved: Whether the user should be considered already approved by an
administrator.
"""
return {
"password_hash": password_hash,
Expand All @@ -79,6 +82,7 @@ async def _serialize_payload( # type: ignore[override]
"user_type": user_type,
"address": address,
"shadow_banned": shadow_banned,
"approved": approved,
}

async def _handle_request( # type: ignore[override]
Expand All @@ -99,6 +103,7 @@ async def _handle_request( # type: ignore[override]
user_type=content["user_type"],
address=content["address"],
shadow_banned=content["shadow_banned"],
approved=content["approved"],
)

return 200, {}
Expand Down
43 changes: 42 additions & 1 deletion synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ def __init__(self, hs: "HomeServer"):
self.store = hs.get_datastores().main
self.auth = hs.get_auth()
self.admin_handler = hs.get_admin_handler()
self._msc3866_enabled = hs.config.experimental.msc3866.enabled

async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
await assert_requester_is_admin(self.auth, request)
Expand All @@ -95,6 +96,13 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
guests = parse_boolean(request, "guests", default=True)
deactivated = parse_boolean(request, "deactivated", default=False)

# If support for MSC3866 is not enabled, apply no filtering based on the
# `approved` column.
if self._msc3866_enabled:
approved = parse_boolean(request, "approved", default=True)
else:
approved = True

babolivier marked this conversation as resolved.
Show resolved Hide resolved
order_by = parse_string(
request,
"order_by",
Expand All @@ -115,8 +123,22 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
direction = parse_string(request, "dir", default="f", allowed_values=("f", "b"))

users, total = await self.store.get_users_paginate(
start, limit, user_id, name, guests, deactivated, order_by, direction
start,
limit,
user_id,
name,
guests,
deactivated,
order_by,
direction,
approved,
)

# If support for MSC3866 is not enabled, don't show the approval flag.
if not self._msc3866_enabled:
for user in users:
del user["approved"]

ret = {"users": users, "total": total}
if (start + limit) < total:
ret["next_token"] = str(start + len(users))
Expand Down Expand Up @@ -163,6 +185,7 @@ def __init__(self, hs: "HomeServer"):
self.deactivate_account_handler = hs.get_deactivate_account_handler()
self.registration_handler = hs.get_registration_handler()
self.pusher_pool = hs.get_pusherpool()
self._msc3866_enabled = hs.config.experimental.msc3866.enabled

async def on_GET(
self, request: SynapseRequest, user_id: str
Expand Down Expand Up @@ -239,6 +262,15 @@ async def on_PUT(
HTTPStatus.BAD_REQUEST, "'deactivated' parameter is not of type boolean"
)

approved: Optional[bool] = None
if "approved" in body and self._msc3866_enabled:
approved = body["approved"]
if not isinstance(approved, bool):
raise SynapseError(
HTTPStatus.BAD_REQUEST,
"'approved' parameter is not of type boolean",
)

# convert List[Dict[str, str]] into List[Tuple[str, str]]
if external_ids is not None:
new_external_ids = [
Expand Down Expand Up @@ -343,6 +375,9 @@ async def on_PUT(
if "user_type" in body:
await self.store.set_user_type(target_user, user_type)

if approved is not None:
await self.store.update_user_approval_status(target_user, approved)

user = await self.admin_handler.get_user(target_user)
assert user is not None

Expand All @@ -355,13 +390,18 @@ async def on_PUT(
if password is not None:
password_hash = await self.auth_handler.hash(password)

new_user_approved = False
if self._msc3866_enabled and approved is True:
new_user_approved = True
babolivier marked this conversation as resolved.
Show resolved Hide resolved

user_id = await self.registration_handler.register_user(
localpart=target_user.localpart,
password_hash=password_hash,
admin=set_admin_to,
default_display_name=displayname,
user_type=user_type,
by_admin=True,
approved=new_user_approved,
)

if threepids is not None:
Expand Down Expand Up @@ -550,6 +590,7 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
user_type=user_type,
default_display_name=displayname,
by_admin=True,
approved=True,
)

result = await register._create_registration_details(user_id, body)
Expand Down
15 changes: 15 additions & 0 deletions synapse/rest/client/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ def __init__(self, hs: "HomeServer"):
hs.config.registration.refreshable_access_token_lifetime is not None
)

# Whether we need to check if the user has been approved or not.
self._require_approval = (
hs.config.experimental.msc3866.enabled
and hs.config.experimental.msc3866.require_approval_for_new_accounts
)

self.auth = hs.get_auth()

self.clock = hs.get_clock()
Expand Down Expand Up @@ -220,6 +226,15 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, LoginResponse]:
except KeyError:
raise SynapseError(400, "Missing JSON keys.")

if self._require_approval:
approved = await self.auth_handler.is_user_approved(result["user_id"])
if not approved:
raise SynapseError(
code=403,
errcode=Codes.USER_AWAITING_APPROVAL,
msg="This account is pending approval by a server administrator.",
)

well_known_data = self._well_known_builder.get_well_known()
if well_known_data:
result["well_known"] = well_known_data
Expand Down
12 changes: 12 additions & 0 deletions synapse/rest/client/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,11 @@ def __init__(self, hs: "HomeServer"):
hs.config.registration.inhibit_user_in_use_error
)

self._require_approval = (
hs.config.experimental.msc3866.enabled
and hs.config.experimental.msc3866.require_approval_for_new_accounts
)

self._registration_flows = _calculate_registration_flows(
hs.config, self.auth_handler
)
Expand Down Expand Up @@ -756,6 +761,13 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
access_token=return_dict.get("access_token"),
)

if self._require_approval:
raise SynapseError(
code=403,
errcode=Codes.USER_AWAITING_APPROVAL,
msg="This account needs to be approved by an administrator before it can be used.",
)
babolivier marked this conversation as resolved.
Show resolved Hide resolved

return 200, return_dict

async def _do_appservice_registration(
Expand Down
9 changes: 8 additions & 1 deletion synapse/storage/databases/main/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ async def get_users_paginate(
deactivated: bool = False,
order_by: str = UserSortOrder.USER_ID.value,
direction: str = "f",
approved: bool = True,
babolivier marked this conversation as resolved.
Show resolved Hide resolved
) -> Tuple[List[JsonDict], int]:
"""Function to retrieve a paginated list of users from
users list. This will return a json list of users and the
Expand All @@ -217,6 +218,7 @@ async def get_users_paginate(
deactivated: whether to include deactivated users
order_by: the sort order of the returned list
direction: sort ascending or descending
approved: whether to include approved users
Returns:
A tuple of a list of mappings from user to information and a count of total users.
"""
Expand Down Expand Up @@ -249,6 +251,11 @@ def get_users_paginate_txn(
if not deactivated:
filters.append("deactivated = 0")

if not approved:
# We ignore NULL values for the approved flag because these should only
# be already existing users that we consider as already approved.
filters.append("approved = 0")

where_clause = "WHERE " + " AND ".join(filters) if len(filters) > 0 else ""

sql_base = f"""
Expand All @@ -262,7 +269,7 @@ def get_users_paginate_txn(

sql = f"""
SELECT name, user_type, is_guest, admin, deactivated, shadow_banned,
displayname, avatar_url, creation_ts * 1000 as creation_ts
displayname, avatar_url, creation_ts * 1000 as creation_ts, approved
{sql_base}
ORDER BY {order_by_column} {order}, u.name ASC
LIMIT ? OFFSET ?
Expand Down
Loading