Repository: FlexMeasures/flexmeasures
Pull request #1465 (2025-05-05 10:19:31)

feat: implement role modification permission checks for users
## Description

This PR adds an extra validation logic to prevent user's without proper permission from modifying other user's roles

## Look & Feel

None. A backend addition(API)

## How to test

admin: can only be changed in CLI / directly in the DB
admin-reader: can be added and removed by admins
account-admin: can be added and removed by admins and consultants
consultant: can be added and removed by admins and account-admins

To test, try to update any user roles that conflict with the above-listed definitions. You should expect to get a 403 HTTP error.

API: `http://localhost:5000/api/v3_0/users/<user_id>`

## Further Improvements

Adding a test case, these predefined roles to be used across the API's are not yet defined in the test case contest. @nhoening  how can we go about this, create new test accounts with these roles?

## Related Items

This PR closes [issue](https://github.com/SeitaBV/ems/issues/67)

---

- [x] I agree to contribute to the project under Apache 2 License. 
- [x] To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures


joshuaunity: @Flix6x It seems I'm having a DB session issue with my tests, although after looking at it for a while I don't really understand why that's the case. I believe after the first test case it flushes and moves on, but it's complaining of a duplicate right on the first test case.
Flix6x: > @Flix6x It seems I'm having a DB session issue with my tests, although after looking at it for a while I don't really understand why that's the case. I believe after the first test case it flushes and moves on, but it's complaining of a duplicate right on the first test case.
> 
> @nhoening 

What is the error? Can I reproduce it by just running the test suite on this branch?
joshuaunity: > > @Flix6x It seems I'm having a DB session issue with my tests, although after looking at it for a while I don't really understand why that's the case. I believe after the first test case it flushes and moves on, but it's complaining of a duplicate right on the first test case.
> > @nhoening
> 
> What is the error? Can I reproduce it by just running the test suite on this branch?

Yes you can, pull the latest update and run `pytest -s -k test_api_v3_0_users_fresh_db`

But this is the error it shows

![image](https://github.com/user-attachments/assets/e726a6cb-4f6c-4047-aae5-3023ebf36c6c)

joshuaunity: Thanks very much for the modifications @Flix6x 
joshuaunity: > Thanks very much for the modifications @Flix6x

So was the issue a case of use the wrong DB session?
joshuaunity: @Flix6x although the entire test is runnign together I surprisingly got a 401 error in my new test
Flix6x: This appears to be another case of tests influencing each other. Specifically, there is a test case in `test_auth_edit_user` that successfully deactivates `test_prosumer_user_2`, so your test case then fails with a 401 for that user.

Furthermore, I ran `pytest` and got 21 additional  tests failing in the CLI.

```
Results (412.27s (0:06:52)):
     822 passed
      22 failed
         - flexmeasures/api/v3_0/tests/test_api_v3_0_users.py:287 test_user_role_failed_modification_permission[test_prosumer_user_2@seita.nl-403-8-expected_role0]
         - flexmeasures/cli/tests/test_data_add.py:27 test_add_annotation
         - flexmeasures/cli/tests/test_data_add.py:63 test_add_holidays
         - flexmeasures/cli/tests/test_data_add.py:344 test_add_process[INFLEXIBLE]
         - flexmeasures/cli/tests/test_data_add.py:344 test_add_process[SHIFTABLE]
         - flexmeasures/cli/tests/test_data_add.py:344 test_add_process[BREAKABLE]
         - flexmeasures/cli/tests/test_data_add.py:385 test_add_sensor[PT20M-ONE-True]
         - flexmeasures/cli/tests/test_data_add.py:385 test_add_sensor[15-TWO-True]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[sensor-sensor]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[sensor-quantity]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[sensor-None]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[quantity-sensor]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[quantity-quantity]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[quantity-None]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[None-sensor]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[None-quantity]
         - flexmeasures/cli/tests/test_data_add.py:452 test_add_storage_schedule[None-None]
         - flexmeasures/cli/tests/test_data_edit.py:14 test_add_one_sensor_attribute
         - flexmeasures/cli/tests/test_data_edit.py:49 test_update_one_asset_attribute
         - flexmeasures/cli/tests/test_data_edit.py:77 test_resample_sensor_data[-]
         - flexmeasures/cli/tests/test_data_edit.py:77 test_resample_sensor_data[2021-03-28 15:00:00+00:00-2021-03-28 16:00:00+00:00]
         - flexmeasures/cli/tests/test_data_show.py:95 test_plot_beliefs
       3 xfailed
```

joshuaunity: >Furthermore, I ran pytest and got 21 additional tests failing in the CLI.

Are these being caused by my new tests?
Flix6x: > Are these being caused by my new tests?

Did you try running the CLI test suite (`pytest -k cli`) locally on `main` and on your branch?
nhoening: > > Are these being caused by my new tests?
> 
> Did you try running the CLI test suite (`pytest -k cli`) locally on `main` and on your branch?

I did that and get 22 failures on both. So this indicates that cli test failures are not a problem of this PR.

Here is a sample output:

```
>       assert "Beliefs for Sensor 'epex_da'" in result.output
E       assert "Beliefs for Sensor 'epex_da'" in ''
E        +  where '' = <Result TypeError("argument of type 'bool' is not iterable")>.output
```

Didn't that occur due to a Click version upgrade?

I have Click 8.2.0 installed. With 8.1.0, my CLI tests pass.


Flix6x: Okay, good. Then those are unrelated indeed.
nhoening: > Coverage increased (+0.04%) to 67.11%

nice

nhoening: @joshuaunity Please add a change log entry.

Patch:
diff --git a/documentation/changelog.rst b/documentation/changelog.rst
index 1f5af611..bfe99165 100644
--- a/documentation/changelog.rst
+++ b/documentation/changelog.rst
@@ -20,12 +20,13 @@ New features
 * Extending sensor CRUD functionality to the UI [see `PR #1394 <https://github.com/FlexMeasures/flexmeasures/pull/1394>`_ and `PR #1413 <https://github.com/FlexMeasures/flexmeasures/pull/1413>`_]
 * Marker clusters on the dashboard map expand in a tree to show the hierarchical relationship of the assets they represent [see `PR #1410 <https://github.com/FlexMeasures/flexmeasures/pull/1410>`_]
 * Load the sensors individually on the Sensors status page. Reload the jobs table using Ajax calls. Improve page performance and avoid timeouts. [see `PR #1425 <https://github.com/FlexMeasures/flexmeasures/pull/1425>`_ and `PR #1466 <https://github.com/FlexMeasures/flexmeasures/pull/1466>`_]
-* New pages for `Properties`, `Graphs`, `Context`, `Status` and `Audit Log`. Simplified the main asset page. [see `PR #1416 <https://github.com/FlexMeasures/flexmeasures/pull/1416>`_, `PR #1387 <https://github.com/FlexMeasures/flexmeasures/pull/1387>`_, `PR #1442 <https://github.com/FlexMeasures/flexmeasures/pull/1442>`_, `PR #1470 <https://github.com/FlexMeasures/flexmeasures/pull/1470>`_, `PR #1473 <https://github.com/FlexMeasures/flexmeasures/pull/1473>`_, `PR #1478 <https://github.com/FlexMeasures/flexmeasures/pull/1478>`_, `PR #1480<https://github.com/FlexMeasures/flexmeasures/pull/1480>`_] and `PR #1482 <https://github.com/FlexMeasures/flexmeasures/pull/1482>`_]
+* New pages for `Properties`, `Graphs`, `Context`, `Status` and `Audit Log`. Simplified the main asset page. [see `PR #1416 <https://github.com/FlexMeasures/flexmeasures/pull/1416>`_, `PR #1387 <https://github.com/FlexMeasures/flexmeasures/pull/1387>`_, `PR #1442 <https://github.com/FlexMeasures/flexmeasures/pull/1442>`_, `PR #1470 <https://github.com/FlexMeasures/flexmeasures/pull/1470>`_, `PR #1473 <https://github.com/FlexMeasures/flexmeasures/pull/1473>`_, `PR #1478 <https://github.com/FlexMeasures/flexmeasures/pull/1478>`_, `PR #1480<https://github.com/FlexMeasures/flexmeasures/pull/1480>`_ and `PR #1482 <https://github.com/FlexMeasures/flexmeasures/pull/1482>`_]
 * Only show important sensors statuses (flex-context and graph sensors) on the status page [see `PR #1439 <https://github.com/FlexMeasures/flexmeasures/pull/1439>`_]
 * Let the user interact with the breadcrumbs on asset graphs page when the graphs are loading [see `PR #1472 <https://github.com/FlexMeasures/flexmeasures/pull/1472>`_]
 * Added DB migrations to apply server defaults to ``generic_asset`` and ``data_sources`` tables [see `PR #1488 <https://github.com/FlexMeasures/flexmeasures/pull/1488>`_]
 * Show visually distinct siblings and children in the assets dropdown [see `PR #1504 <https://github.com/FlexMeasures/flexmeasures/pull/1504>`_]
 * Add resolution back to the CLI belief plotting [see `PR #1500 <https://github.com/FlexMeasures/flexmeasures/pull/1500>`_]
+* Improved permission validation for user role updates. [see `PR #1465 <https://github.com/FlexMeasures/flexmeasures/pull/1465>`_]
 
 Infrastructure / Support
 ----------------------
@@ -36,7 +37,7 @@ Infrastructure / Support
 * Describe four supported user roles explicitly (docs and code) [see `PR #1451 <https://github.com/FlexMeasures/flexmeasures/pull/1451>`_]
 * Updated the directory structure for crud operations for assets, sensors, users and accounts [see `PR #1467 <https://github.com/FlexMeasures/flexmeasures/pull/1467>`_ and `PR #1471 <https://github.com/FlexMeasures/flexmeasures/pull/1471>`_]
 * Docs: Add tutorial scripts [see `PR #1505 <https://github.com/FlexMeasures/flexmeasures/pull/1505>`_]
-* Docs: fix graphs page for assets in toy tutorial, display plotted sensors when CLI shows asset info [see `PR #1501 <https://github.com/FlexMeasures/flexmeasures/pull/1501>`_
+* Docs: fix graphs page for assets in toy tutorial, display plotted sensors when CLI shows asset info [see `PR #1501 <https://github.com/FlexMeasures/flexmeasures/pull/1501>`_]
 
 Bugfixes
 -----------
diff --git a/flexmeasures/api/v3_0/users.py b/flexmeasures/api/v3_0/users.py
index 7b9dc3eb..447e11c8 100644
--- a/flexmeasures/api/v3_0/users.py
+++ b/flexmeasures/api/v3_0/users.py
@@ -242,9 +242,10 @@ class UserAPI(FlaskView):
         .. :quickref: User; Patch data for an existing user
 
         This endpoint sets data for an existing user.
-        It has to be used by the user themselves, admins or account-admins (of the same account).
+        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.
 
         The following fields are not allowed to be updated at all:
          - id
@@ -295,6 +296,27 @@ class UserAPI(FlaskView):
                 raise Forbidden(
                     "Users who edit themselves cannot edit security-sensitive fields."
                 )
+            # if flexmeasures_roles is not empty, check if the user can modify the role
+            if k == "flexmeasures_roles" and v:
+                from flexmeasures.auth.policy import can_modify_role
+
+                current_roles = set(user.roles)
+                new_roles = set(v)
+
+                roles_being_removed = current_roles - new_roles
+                for role in roles_being_removed:
+                    if not can_modify_role(current_user, [role]):
+                        raise Forbidden(
+                            f"You are not allowed to remove ({role.name}) role from this user."
+                        )
+
+                roles_being_added = new_roles - current_roles
+                for role in roles_being_added:
+                    if not can_modify_role(current_user, [role]):
+                        raise Forbidden(
+                            f"You are not allowed to add ({role.name}) role to this user."
+                        )
+
             setattr(user, k, v)
             if k == "active" and v is False:
                 remove_cookie_and_token_access(user)
diff --git a/flexmeasures/auth/policy.py b/flexmeasures/auth/policy.py
index 1e96212b..3ab96bec 100644
--- a/flexmeasures/auth/policy.py
+++ b/flexmeasures/auth/policy.py
@@ -197,3 +197,42 @@ def check_account_role(user, principal: str) -> bool:
         if user.account.has_role(account_role):
             return True
     return False
+
+
+def can_modify_role(user, roles_to_modify) -> bool:
+    """For a set of supported roles, check if the current user can modify the roles.
+
+    :param user: The current user.
+    :param roles_to_modify: A list of roles to modify.
+    :return: True if the user can modify the roles, False otherwise.
+
+    The roles are:
+    - admin: can only be changed in CLI / directly in the DB
+    - admin-reader: can be added and removed by admins
+    - account-admin: can be added and removed by admins and consultants
+    - consultant: can be added and removed by admins and account-admins
+
+    """
+
+    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 not role:
+            return False
+        if role.name == ADMIN_ROLE:
+            return False
+        if role.name == ADMIN_READER_ROLE and not user.has_role(ADMIN_ROLE):
+            return False
+        if role.name == ACCOUNT_ADMIN_ROLE and not (
+            user.has_role(ADMIN_ROLE) or user.has_role(CONSULTANT_ROLE)
+        ):
+            return False
+        if role.name == CONSULTANT_ROLE and not (
+            user.has_role(ADMIN_ROLE) or user.has_role(ACCOUNT_ADMIN_ROLE)
+        ):
+            return False
+
+    return True
diff --git a/flexmeasures/data/models/user.py b/flexmeasures/data/models/user.py
index 48a2d143..59defad4 100644
--- a/flexmeasures/data/models/user.py
+++ b/flexmeasures/data/models/user.py
@@ -257,7 +257,7 @@ class User(db.Model, UserMixin, AuthModelMixin):
     def __acl__(self):
         """
         Within same account, everyone can read.
-        Only the user themselves or account-admins can edit their user record.
+        Only the user themselves, consultants or account-admins can edit their user record.
         Creation and deletion are left to site admins in CLI.
         """
         return {
@@ -265,6 +265,10 @@ class User(db.Model, UserMixin, AuthModelMixin):
             "update": [
                 f"user:{self.id}",
                 (f"account:{self.account_id}", f"role:{ACCOUNT_ADMIN_ROLE}"),
+                (
+                    f"account:{self.account.consultancy_account_id}",
+                    f"role:{CONSULTANT_ROLE}",
+                ),
             ],
         }
 
diff --git a/flexmeasures/data/schemas/users.py b/flexmeasures/data/schemas/users.py
index e3cc61ab..6d15a8fd 100644
--- a/flexmeasures/data/schemas/users.py
+++ b/flexmeasures/data/schemas/users.py
@@ -1,17 +1,24 @@
 from marshmallow import validates, ValidationError, validate
 from pytz import all_timezones
 
-from flexmeasures.data import ma
+from flexmeasures.data import db, ma
 from flexmeasures.data.models.user import User as UserModel
 from flexmeasures.data.schemas.times import AwareDateTimeField
 
 
-class UserSchema(ma.SQLAlchemySchema):
+class SQLAlchemySchema(ma.SQLAlchemySchema):
+    """Workaround from https://github.com/marshmallow-code/flask-marshmallow/issues/44#issuecomment-1002740778"""
+
+    class Meta(ma.SQLAlchemySchema.Meta):
+        sqla_session = db.session
+
+
+class UserSchema(SQLAlchemySchema):
     """
     This schema lists fields we support through this API (e.g. no password).
     """
 
-    class Meta:
+    class Meta(SQLAlchemySchema.Meta):
         model = UserModel
 
     @validates("timezone")


Test patch:
diff --git a/flexmeasures/api/v3_0/tests/conftest.py b/flexmeasures/api/v3_0/tests/conftest.py
index a8c534e1..ca842571 100644
--- a/flexmeasures/api/v3_0/tests/conftest.py
+++ b/flexmeasures/api/v3_0/tests/conftest.py
@@ -50,6 +50,20 @@ def setup_inactive_user(db, setup_accounts, setup_roles_users):
     """
     Set up one inactive user and one inactive admin.
     """
+    add_inactive_users(db, setup_accounts)
+
+
+@pytest.fixture(scope="function")
+def setup_inactive_user_fresh_db(
+    fresh_db, setup_accounts_fresh_db, setup_roles_users_fresh_db
+):
+    """
+    Set up one inactive user and one inactive admin.
+    """
+    add_inactive_users(fresh_db, setup_accounts_fresh_db)
+
+
+def add_inactive_users(db, setup_accounts):
     user_datastore = SQLAlchemySessionUserDatastore(db.session, User, UserRole)
     user_datastore.create_user(
         username="inactive test user",
diff --git a/flexmeasures/api/v3_0/tests/test_api_v3_0_users.py b/flexmeasures/api/v3_0/tests/test_api_v3_0_users.py
index dc4addd0..1f58430a 100644
--- a/flexmeasures/api/v3_0/tests/test_api_v3_0_users.py
+++ b/flexmeasures/api/v3_0/tests/test_api_v3_0_users.py
@@ -51,7 +51,7 @@ def test_get_users_bad_auth(requesting_user, client, setup_api_test_data):
     ],
     indirect=["requesting_user"],
 )
-def test_get_users_inatest_get_users_inactivective(
+def test_get_users_inactive(
     requesting_user,
     client,
     setup_api_test_data,
@@ -282,3 +282,32 @@ def test_logout(client, setup_api_test_data, requesting_user):
     assert logout_response.status_code == 302
 
     assert current_user.is_anonymous
+
+
+@pytest.mark.parametrize(
+    "requesting_user, expected_status_code, user_to_update, expected_role",
+    [
+        # Admin-reader tries to update user 5 (initially an account admin) to become admin-reader
+        ("test_admin_reader_user@seita.nl", 403, 5, [3]),
+        # Consultant tries to updates user 8 (initially consultant) to become admin-reader
+        ("test_consultant@seita.nl", 403, 8, [3]),
+        # Admin-reader tries to remove all roles of user 5 (initially an account admin)
+        ("test_admin_reader_user@seita.nl", 403, 5, []),
+    ],
+    indirect=["requesting_user"],
+)
+def test_user_role_failed_modification_permission(
+    client,
+    setup_api_test_data,
+    requesting_user,
+    expected_status_code,
+    user_to_update,
+    expected_role,
+):
+    patch_user_response = client.patch(
+        url_for("UserAPI:patch", id=user_to_update),
+        json={"flexmeasures_roles": expected_role},
+    )
+
+    print("Server responded with:\n%s" % patch_user_response.data)
+    assert patch_user_response.status_code == expected_status_code
diff --git a/flexmeasures/api/v3_0/tests/test_api_v3_0_users_fresh_db.py b/flexmeasures/api/v3_0/tests/test_api_v3_0_users_fresh_db.py
index b5a759b5..9d60c32c 100644
--- a/flexmeasures/api/v3_0/tests/test_api_v3_0_users_fresh_db.py
+++ b/flexmeasures/api/v3_0/tests/test_api_v3_0_users_fresh_db.py
@@ -20,7 +20,7 @@ from flexmeasures.data.models.audit_log import AuditLog
     indirect=["requesting_user"],
 )
 def test_user_reset_password(
-    db, app, client, setup_inactive_user, requesting_user, status_code
+    fresh_db, app, client, setup_inactive_user_fresh_db, requesting_user, status_code
 ):
     """
     Reset the password of User 2.
@@ -40,7 +40,7 @@ def test_user_reset_password(
         if status_code != 200:
             return
 
-        assert db.session.execute(
+        assert fresh_db.session.execute(
             select(AuditLog).filter_by(
                 affected_user_id=user2.id,
                 event=f"Password reset for user {user2.username}",
@@ -58,3 +58,33 @@ def test_user_reset_password(
             "reset your password:\n\n%sreset/" % request.host_url
             in pwd_reset_instructions.body
         )
+
+
+#  This test can also be found in test_api_v3_0_users.py, the difference is
+#  that this makes modifications to the db(and thats best to be in a seperate db session,
+#  in order not to affect other tests) while the other doesn't
+@pytest.mark.parametrize(
+    "requesting_user, expected_status_code, user_to_update, expected_role",
+    [
+        # Admin updates user 4 (initially no roles) to become admin-reader & consultant
+        ("test_admin_user@seita.nl", 200, 4, [3, 4]),
+        # Admin updates user 5 (initially an account-admin), removing the account-admin role
+        ("test_admin_user@seita.nl", 200, 5, []),
+    ],
+    indirect=["requesting_user"],
+)
+def test_user_role_successful_modification_permission(
+    client,
+    setup_roles_users_fresh_db,
+    requesting_user,
+    expected_status_code,
+    user_to_update,
+    expected_role,
+):
+    patch_user_response = client.patch(
+        url_for("UserAPI:patch", id=user_to_update),
+        json={"flexmeasures_roles": expected_role},
+    )
+
+    print("Server responded with:\n%s" % patch_user_response.data)
+    assert patch_user_response.status_code == expected_status_code
diff --git a/flexmeasures/auth/tests/test_principal_matching.py b/flexmeasures/auth/tests/test_principal_matching.py
index c81dc537..ebffdb45 100644
--- a/flexmeasures/auth/tests/test_principal_matching.py
+++ b/flexmeasures/auth/tests/test_principal_matching.py
@@ -2,7 +2,7 @@ from __future__ import annotations
 
 import pytest
 
-from flexmeasures.auth.policy import user_matches_principals
+from flexmeasures.auth.policy import user_matches_principals, can_modify_role
 
 
 class MockAccount:
@@ -95,3 +95,36 @@ def make_mock_user(
 )
 def test_principals_match(mock_user, principals, should_match):
     assert user_matches_principals(mock_user, principals) == should_match
+
+
+@pytest.mark.parametrize(
+    "mock_user, roles_to_modify, can_modify_roles",
+    [
+        # Admin user should be able to modify (admin-reader & consultant) roles
+        (make_mock_user(19, ["admin"], 1, []), [3, 4], True),
+        # Consultant user should not be able to modify (admin-reader) role
+        (make_mock_user(19, ["consultant"], 1, []), [3], False),
+        # Admin-reader user should not be able to modify (admin-reader) role
+        (
+            make_mock_user(19, ["admin-reader"], 1, []),
+            [3],
+            False,
+        ),
+        # Account-admin user should not be able to modify (admin-reader) role
+        (
+            make_mock_user(19, ["account-admin"], 1, []),
+            [3],
+            False,
+        ),
+        # Account-admin user should be able to modify (consultant) role
+        (
+            make_mock_user(19, ["account-admin"], 1, []),
+            [4],
+            True,
+        ),
+    ],
+)
+def test_can_modify_role(
+    db, setup_roles_users, mock_user, roles_to_modify, can_modify_roles
+):
+    assert can_modify_role(mock_user, roles_to_modify) == can_modify_roles