refactor the auth for user role editing

documentation/changelog.rst

@@ -34,20 +34,20 @@ 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 <https://www.github.com/FlexMeasures/flexmeasures/pull/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 <https://www.github.com/FlexMeasures/flexmeasures/pull/2237>`_]
+* Improve auth checks on editing user roles [see `PR #2228 <https://www.github.com/FlexMeasures/flexmeasures/pull/2228>`_]
 * Fix Chart Point sessions chart [see `PR #2259 <https://www.github.com/FlexMeasures/flexmeasures/pull/2259>`_]
 
 Infrastructure / Support
 ------------------------
 * Filter handled untrusted-host ``SecurityError`` events out of Sentry, alongside the existing 404 filtering [see `PR #2257 <https://www.github.com/FlexMeasures/flexmeasures/pull/2257>`_]
 
 
-
 v0.33.0 | June 1, 2026
 ============================
 

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)
 

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

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

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 {

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": []

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: