From c64a49981eeac6774cf5af7974bff4a66c28f990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Sun, 7 Jun 2026 14:44:31 +0200 Subject: [PATCH 01/10] refactor the auth for user role editing, clearer and explicitly about the roles we protect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/auth/policy.py | 47 ++++++++++++++++++---------- flexmeasures/data/models/user.py | 1 + flexmeasures/ui/views/users/views.py | 5 +-- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py index 1fa9f21417..b65b5e1f67 100644 --- a/flexmeasures/auth/policy.py +++ b/flexmeasures/auth/policy.py @@ -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,36 @@ 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 user.has_role(ADMIN_ROLE) and ADMIN_ROLE not in [role.name for role in roles]: + return True # admins can do all changes, aside from admin status + + for role in [r for r in roles if r is not None]: + if role.name == ADMIN_ROLE: + return False # nobody can do this here, only in CLI or directly in the DB + if role.name == ADMIN_READER_ROLE: + if not user.has_role(ADMIN_ROLE): + return False # only admins can change admin-reader status + if role.name == ACCOUNT_ADMIN_ROLE: # consultants can do this + if ( + modified_user.account.consultancy_account is None + or not user.has_role(CONSULTANT_ROLE) + or not user.account.id == modified_user.account.consultancy_account.id + ): + return False + if role.name == CONSULTANT_ROLE: # account-admins can do this + if ( + not user.has_role(ACCOUNT_ADMIN_ROLE) + or not user.account.id == modified_user.account.id + ): + return False + + return True diff --git a/flexmeasures/data/models/user.py b/flexmeasures/data/models/user.py index 2f0239dfec..687f8138f3 100644 --- a/flexmeasures/data/models/user.py +++ b/flexmeasures/data/models/user.py @@ -289,6 +289,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/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: From 2f9687bbdbc23e3988ad56966e0129a4239e096d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 9 Jun 2026 10:25:38 +0200 Subject: [PATCH 02/10] add changelog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- documentation/changelog.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index c2d79a3685..b5c49aabd8 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -23,6 +23,15 @@ Bugfixes ----------- +v0.33.1 | June XX, 2026 +============================ + +Bugfixes +----------- +* Improve auth checks on editing user roles [see `PR #2228 `_] + + + v0.33.0 | June 1, 2026 ============================ From fcf890e81a5a625d89dc3e35851636e3c2359338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 9 Jun 2026 14:00:03 +0200 Subject: [PATCH 03/10] reformulate with False as default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- documentation/changelog.rst | 1 + flexmeasures/api/v3_0/users.py | 2 +- flexmeasures/auth/policy.py | 49 ++++++++++--------- .../auth/tests/test_principal_matching.py | 37 ++++++++++++++ flexmeasures/ui/static/openapi-specs.json | 2 +- 5 files changed, 66 insertions(+), 25 deletions(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index b5c49aabd8..590dec13cb 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -29,6 +29,7 @@ v0.33.1 | June XX, 2026 Bugfixes ----------- * Improve auth checks on editing user roles [see `PR #2228 `_] +* Deny unsupported or unresolved user role changes by default [see `PR #XXXX `_] diff --git a/flexmeasures/api/v3_0/users.py b/flexmeasures/api/v3_0/users.py index 6f7857cf21..61cf53007f 100644 --- a/flexmeasures/api/v3_0/users.py +++ b/flexmeasures/api/v3_0/users.py @@ -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 diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py index b65b5e1f67..cb6d33d996 100644 --- a/flexmeasures/auth/policy.py +++ b/flexmeasures/auth/policy.py @@ -228,27 +228,30 @@ def can_modify_role( else: roles.append(role) - if user.has_role(ADMIN_ROLE) and ADMIN_ROLE not in [role.name for role in roles]: - return True # admins can do all changes, aside from admin status - - for role in [r for r in roles if r is not None]: + can_modify_roles = [] + for role in roles: + can_modify_this_role = False + if role is None: + can_modify_roles.append(can_modify_this_role) + continue if role.name == ADMIN_ROLE: - return False # nobody can do this here, only in CLI or directly in the DB - if role.name == ADMIN_READER_ROLE: - if not user.has_role(ADMIN_ROLE): - return False # only admins can change admin-reader status - if role.name == ACCOUNT_ADMIN_ROLE: # consultants can do this - if ( - modified_user.account.consultancy_account is None - or not user.has_role(CONSULTANT_ROLE) - or not user.account.id == modified_user.account.consultancy_account.id - ): - return False - if role.name == CONSULTANT_ROLE: # account-admins can do this - if ( - not user.has_role(ACCOUNT_ADMIN_ROLE) - or not user.account.id == modified_user.account.id - ): - return False - - return True + # Nobody can do this here, only in CLI or directly in the DB. + can_modify_this_role = False + elif role.name == ADMIN_READER_ROLE: + can_modify_this_role = user.has_role( + ADMIN_ROLE + ) # only admins can change admin-reader status + elif role.name == ACCOUNT_ADMIN_ROLE: # admins and consultants can do this + can_modify_this_role = user.has_role(ADMIN_ROLE) or ( + modified_user.account.consultancy_account is not None + and user.has_role(CONSULTANT_ROLE) + and user.account.id == modified_user.account.consultancy_account.id + ) + elif role.name == CONSULTANT_ROLE: # admins and account-admins can do this + can_modify_this_role = user.has_role(ADMIN_ROLE) or ( + user.has_role(ACCOUNT_ADMIN_ROLE) + and user.account.id == modified_user.account.id + ) + can_modify_roles.append(can_modify_this_role) + + return bool(can_modify_roles) and all(can_modify_roles) diff --git a/flexmeasures/auth/tests/test_principal_matching.py b/flexmeasures/auth/tests/test_principal_matching.py index f919cc01d2..0000c7a197 100644 --- a/flexmeasures/auth/tests/test_principal_matching.py +++ b/flexmeasures/auth/tests/test_principal_matching.py @@ -1,5 +1,7 @@ from __future__ import annotations +from types import SimpleNamespace + import pytest from flexmeasures.auth.policy import user_matches_principals, can_modify_role @@ -194,3 +196,38 @@ 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, []), + [4, SimpleNamespace(name="unsupported-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/ui/static/openapi-specs.json b/flexmeasures/ui/static/openapi-specs.json index 3f122566b7..10ebfc1661 100644 --- a/flexmeasures/ui/static/openapi-specs.json +++ b/flexmeasures/ui/static/openapi-specs.json @@ -2250,7 +2250,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": [] From 4b8d1efe447d34d004d313485cbf8f32a19a964e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 9 Jun 2026 14:17:01 +0200 Subject: [PATCH 04/10] remove extra changelog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- documentation/changelog.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 590dec13cb..b5c49aabd8 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -29,7 +29,6 @@ v0.33.1 | June XX, 2026 Bugfixes ----------- * Improve auth checks on editing user roles [see `PR #2228 `_] -* Deny unsupported or unresolved user role changes by default [see `PR #XXXX `_] From 1c2c391fca4ebaa7c57ec2f4f9bb377a59b071ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 30 Jun 2026 16:59:15 +0200 Subject: [PATCH 05/10] simplify the loop to check roles - no collection just returning False if we encounter a disallowed entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/auth/policy.py | 47 +++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py index cb6d33d996..850908e256 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, @@ -228,30 +228,41 @@ def can_modify_role( else: roles.append(role) - can_modify_roles = [] + if not roles: + return False + for role in roles: - can_modify_this_role = False if role is None: - can_modify_roles.append(can_modify_this_role) - continue + return False if role.name == ADMIN_ROLE: # Nobody can do this here, only in CLI or directly in the DB. - can_modify_this_role = False - elif role.name == ADMIN_READER_ROLE: - can_modify_this_role = user.has_role( - ADMIN_ROLE - ) # only admins can change admin-reader status - elif role.name == ACCOUNT_ADMIN_ROLE: # admins and consultants can do this - can_modify_this_role = user.has_role(ADMIN_ROLE) or ( + return False + if role.name == ADMIN_READER_ROLE: + # only admins can change admin-reader status + if not user.has_role(ADMIN_ROLE): + return False + continue + 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 - ) - elif role.name == CONSULTANT_ROLE: # admins and account-admins can do this - can_modify_this_role = user.has_role(ADMIN_ROLE) or ( + ): + 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 - ) - can_modify_roles.append(can_modify_this_role) + ): + continue + return False + return False - return bool(can_modify_roles) and all(can_modify_roles) + return True From a68a652f41fd30c763cb6ee114415b1d294e23b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 30 Jun 2026 16:59:29 +0200 Subject: [PATCH 06/10] move import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/api/v3_0/users.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/flexmeasures/api/v3_0/users.py b/flexmeasures/api/v3_0/users.py index 61cf53007f..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 @@ -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) From c658028f9490256a235fda2e42f9c4eeb400a1fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Tue, 30 Jun 2026 17:08:23 +0200 Subject: [PATCH 07/10] run a regression test case where the first new role is forbidden and the last new role is allowed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/auth/tests/test_principal_matching.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/flexmeasures/auth/tests/test_principal_matching.py b/flexmeasures/auth/tests/test_principal_matching.py index 0000c7a197..2746ad5bca 100644 --- a/flexmeasures/auth/tests/test_principal_matching.py +++ b/flexmeasures/auth/tests/test_principal_matching.py @@ -4,7 +4,11 @@ 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: @@ -223,7 +227,10 @@ def test_can_modify_role( ( make_mock_user(19, ["admin"], 1, []), make_mock_user(23, ["consultant"], 1, []), - [4, SimpleNamespace(name="unsupported-role")], + [ + SimpleNamespace(name="unsupported-role"), + SimpleNamespace(name=CONSULTANT_ROLE), + ], ), ], ) From 7b3fbc81f7219c0a10ee0f2ec919bb801d9e543b Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Wed, 1 Jul 2026 11:49:27 +0200 Subject: [PATCH 08/10] docs: merge changelog listings for v0.33.1 Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 99eaa6aa21..0d6061b455 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -40,6 +40,7 @@ v0.33.1 | July XX, 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,16 +48,6 @@ Infrastructure / Support * Filter handled untrusted-host ``SecurityError`` events out of Sentry, alongside the existing 404 filtering [see `PR #2257 `_] - -v0.33.1 | June XX, 2026 -============================ - -Bugfixes ------------ -* Improve auth checks on editing user roles [see `PR #2228 `_] - - - v0.33.0 | June 1, 2026 ============================ From d870cfcc90ed5c26804e104840af37b4e7ca385b Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Wed, 1 Jul 2026 11:49:45 +0200 Subject: [PATCH 09/10] docs: add release date for v0.33.1 Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 0d6061b455..e37cd914aa 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -34,7 +34,7 @@ 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 From 919ac546d03625cb5c33a3be6b57b80203016ce3 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Wed, 1 Jul 2026 11:58:14 +0200 Subject: [PATCH 10/10] refactor: consistent nesting Signed-off-by: F.N. Claessen --- flexmeasures/auth/policy.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py index 850908e256..8a962da3cd 100644 --- a/flexmeasures/auth/policy.py +++ b/flexmeasures/auth/policy.py @@ -239,9 +239,9 @@ def can_modify_role( # noqa: C901 return False if role.name == ADMIN_READER_ROLE: # only admins can change admin-reader status - if not user.has_role(ADMIN_ROLE): - return False - continue + 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):