diff --git a/documentation/changelog.rst b/documentation/changelog.rst index c6f902d316..e37cd914aa 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -34,12 +34,13 @@ Bugfixes * Allow root assets belonging to different accounts to share the same name, while keeping asset names unique among root assets within the same account and among children of the same parent [see `PR #2226 `_] -v0.33.1 | July XX, 2026 +v0.33.1 | July 1, 2026 ============================ Bugfixes ----------- * Allow flex-model and flex-context to be missing from scheduling requests, because by now the whole flex-config can be defined on assets (in the db) instead [see `PR #2237 `_] +* Improve auth checks on editing user roles [see `PR #2228 `_] * Fix Chart Point sessions chart [see `PR #2259 `_] Infrastructure / Support @@ -47,7 +48,6 @@ Infrastructure / Support * Filter handled untrusted-host ``SecurityError`` events out of Sentry, alongside the existing 404 filtering [see `PR #2257 `_] - v0.33.0 | June 1, 2026 ============================ diff --git a/flexmeasures/api/v3_0/users.py b/flexmeasures/api/v3_0/users.py index 6f7857cf21..034f4f2e3f 100644 --- a/flexmeasures/api/v3_0/users.py +++ b/flexmeasures/api/v3_0/users.py @@ -9,7 +9,7 @@ from flask_security.recoverable import send_reset_password_instructions from flask_json import as_json from werkzeug.exceptions import Forbidden -from flexmeasures.auth.policy import check_access +from flexmeasures.auth.policy import can_modify_role, check_access from flexmeasures.data.models.audit_log import AuditLog from flexmeasures.data.models.user import User as UserModel, Account @@ -393,7 +393,7 @@ def patch(self, id: int, user: UserModel, **user_data): # noqa C901 It has to be used by the user themselves, admins, consultant or account-admins (of the same account). Any subset of user fields can be sent. If the user is not an (account-)admin, they can only edit a few of their own fields. - User roles cannot be updated by everyone - it requires certain access levels (roles, account), with the general rule that you need a higher access level than the role being updated. + User role updates require explicit permission for every role being added or removed, generally by a user with a higher access level than the role being updated. Unsupported, unknown, or otherwise unresolved roles are denied. The following fields are not allowed to be updated at all: - id @@ -460,8 +460,6 @@ def patch(self, id: int, user: UserModel, **user_data): # noqa C901 ) # if flexmeasures_roles is not empty, check if the user can modify the role if k == "flexmeasures_roles" and (v or len(v) == 0): - from flexmeasures.auth.policy import can_modify_role - current_roles = set(user.flexmeasures_roles) new_roles = set(v) diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py index 1fa9f21417..8a962da3cd 100644 --- a/flexmeasures/auth/policy.py +++ b/flexmeasures/auth/policy.py @@ -200,7 +200,7 @@ def check_account_role(user, principal: str) -> bool: return False -def can_modify_role( +def can_modify_role( # noqa: C901 user, roles_to_modify, modified_user, @@ -210,7 +210,7 @@ def can_modify_role( :param user: The current attempting to modify a role. :param roles_to_modify: A list of roles to modify - can be a Role or a role ID. :param modified_user: The user whose roles are being modified. - :return: True if the user can modify the roles, False otherwise. + :return: True if the user can modify each of the roles, False otherwise. The roles are: - admin: can only be changed in CLI / directly in the DB, so not here @@ -219,23 +219,50 @@ def can_modify_role( - consultant: can be added and removed by admins and account-admins (in same account) """ - + roles = [] for role in roles_to_modify: if isinstance(role, int): from flexmeasures.data.models.user import Role - role = current_app.db.session.get(Role, role) - - if role is not None: - if role.name != ADMIN_ROLE and user.has_role(ADMIN_ROLE): - return True # admin can do all changes, aside from admin status - if role.name == ACCOUNT_ADMIN_ROLE and user.has_role(CONSULTANT_ROLE): - if modified_user.account.consultancy_account is not None: - if user.account.id == modified_user.account.consultancy_account.id: - return True - if role.name == CONSULTANT_ROLE and user.has_role(ACCOUNT_ADMIN_ROLE): - if user.account.id and modified_user.account.id: - if user.account.id == modified_user.account.id: - return True - - return False + roles.append(current_app.db.session.get(Role, role)) + else: + roles.append(role) + + if not roles: + return False + + for role in roles: + if role is None: + return False + if role.name == ADMIN_ROLE: + # Nobody can do this here, only in CLI or directly in the DB. + return False + if role.name == ADMIN_READER_ROLE: + # only admins can change admin-reader status + if user.has_role(ADMIN_ROLE): + continue + return False + if role.name == ACCOUNT_ADMIN_ROLE: + # admins and consultants can do this + if user.has_role(ADMIN_ROLE): + continue + if ( + modified_user.account.consultancy_account is not None + and user.has_role(CONSULTANT_ROLE) + and user.account.id == modified_user.account.consultancy_account.id + ): + continue + return False + if role.name == CONSULTANT_ROLE: + # admins and account-admins can do this + if user.has_role(ADMIN_ROLE): + continue + if ( + user.has_role(ACCOUNT_ADMIN_ROLE) + and user.account.id == modified_user.account.id + ): + continue + return False + return False + + return True diff --git a/flexmeasures/auth/tests/test_principal_matching.py b/flexmeasures/auth/tests/test_principal_matching.py index f919cc01d2..2746ad5bca 100644 --- a/flexmeasures/auth/tests/test_principal_matching.py +++ b/flexmeasures/auth/tests/test_principal_matching.py @@ -1,8 +1,14 @@ from __future__ import annotations +from types import SimpleNamespace + import pytest -from flexmeasures.auth.policy import user_matches_principals, can_modify_role +from flexmeasures.auth.policy import ( + CONSULTANT_ROLE, + can_modify_role, + user_matches_principals, +) class MockAccount: @@ -194,3 +200,41 @@ def test_can_modify_role( assert ( can_modify_role(mock_user, roles_to_modify, modified_user) == can_modify_roles ) + + +@pytest.mark.parametrize( + "mock_user, modified_user, roles_to_modify", + [ + # Empty role changes are not explicitly allowed. + ( + make_mock_user(19, ["consultant"], 1, []), + make_mock_user(20, ["account-admin"], 2, [], 1), + [], + ), + # None is not an explicitly supported role. + ( + make_mock_user(19, ["consultant"], 1, []), + make_mock_user(21, ["account-admin"], 2, [], 1), + [None], + ), + # Unsupported roles must not be allowed by falling through supported checks. + ( + make_mock_user(19, ["consultant"], 1, []), + make_mock_user(22, [], 2, [], 1), + [SimpleNamespace(name="unsupported-role")], + ), + # Every requested role must be explicitly allowed, so one unsupported role denies the whole change. + ( + make_mock_user(19, ["admin"], 1, []), + make_mock_user(23, ["consultant"], 1, []), + [ + SimpleNamespace(name="unsupported-role"), + SimpleNamespace(name=CONSULTANT_ROLE), + ], + ), + ], +) +def test_can_modify_role_denies_unexplicit_role_changes( + db, setup_roles_users, mock_user, modified_user, roles_to_modify +): + assert can_modify_role(mock_user, roles_to_modify, modified_user) is False diff --git a/flexmeasures/data/models/user.py b/flexmeasures/data/models/user.py index 82e045149f..826fa9a295 100644 --- a/flexmeasures/data/models/user.py +++ b/flexmeasures/data/models/user.py @@ -290,6 +290,7 @@ def __acl__(self): """ Within the same account, everyone can read. Consultants as well. Only the user themselves, consultants or account-admins can edit their user record. + Check policy.can_modify_role() for special treatment of roles. Creation and deletion are left to site admins in CLI. """ return { diff --git a/flexmeasures/ui/static/openapi-specs.json b/flexmeasures/ui/static/openapi-specs.json index af48a5cc66..b352dc217f 100644 --- a/flexmeasures/ui/static/openapi-specs.json +++ b/flexmeasures/ui/static/openapi-specs.json @@ -2251,7 +2251,7 @@ }, "patch": { "summary": "Update a user.", - "description": "This endpoint sets data for an existing user.\nIt has to be used by the user themselves, admins, consultant or account-admins (of the same account).\nAny subset of user fields can be sent.\nIf the user is not an (account-)admin, they can only edit a few of their own fields.\nUser roles cannot be updated by everyone - it requires certain access levels (roles, account), with the general rule that you need a higher access level than the role being updated.\n\nThe following fields are not allowed to be updated at all:\n- id\n- account_id\n", + "description": "This endpoint sets data for an existing user.\nIt has to be used by the user themselves, admins, consultant or account-admins (of the same account).\nAny subset of user fields can be sent.\nIf the user is not an (account-)admin, they can only edit a few of their own fields.\nUser role updates require explicit permission for every role being added or removed, generally by a user with a higher access level than the role being updated. Unsupported, unknown, or otherwise unresolved roles are denied.\n\nThe following fields are not allowed to be updated at all:\n- id\n- account_id\n", "security": [ { "ApiKeyAuth": [] diff --git a/flexmeasures/ui/views/users/views.py b/flexmeasures/ui/views/users/views.py index f3867dfa2e..0dfdd823fc 100644 --- a/flexmeasures/ui/views/users/views.py +++ b/flexmeasures/ui/views/users/views.py @@ -6,7 +6,7 @@ from werkzeug.exceptions import Forbidden, Unauthorized from sqlalchemy import select -from flexmeasures.auth.policy import check_access +from flexmeasures.auth.policy import check_access, ADMIN_ROLE from flexmeasures.data import db from flexmeasures.data.models.audit_log import AuditLog from flexmeasures.data.models.user import User, Role, Account @@ -43,7 +43,8 @@ def render_user(user: User | None, msg: str | None = None): roles = {} for role in db.session.scalars(select(Role)).all(): - roles[role.name] = role.id + if role.name != ADMIN_ROLE: + roles[role.name] = role.id user_roles = [] if user is not None: