From 05d22326d373e10c34214508e4298329b3c82c83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Thu, 10 Sep 2026 17:14:58 +0200 Subject: [PATCH 1/2] fix: make organisation audit changes and affected users explicit --- documentation/changelog.rst | 2 + flexmeasures/api/v3_0/accounts.py | 49 +++--- .../tests/test_accounts_audit_fresh_db.py | 139 ++++++++++++++++++ .../api/v3_0/tests/test_api_v3_0_users.py | 5 +- .../tests/test_api_v3_0_users_fresh_db.py | 57 ++++++- flexmeasures/api/v3_0/users.py | 16 +- 6 files changed, 239 insertions(+), 29 deletions(-) create mode 100644 flexmeasures/api/v3_0/tests/test_accounts_audit_fresh_db.py diff --git a/documentation/changelog.rst b/documentation/changelog.rst index 813fb1f7c1..af1ce76983 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -48,6 +48,8 @@ Infrastructure / Support Bugfixes ----------- +* Organisation audit logs now show only changed fields with their previous and new values, and user role and active-status changes identify the affected user + * Where several data sources report the same event, which one a search keeps is now decided the same way every time: a source version only counts against other versions of that source, and a caller that lists its sources gets the order it asked for [see `PR #2494 `_] * A KPI on the asset page counted an event once per data source that reported it, so a total could come out higher than any source reported; it now reduces one value per event, from the latest source version and the most recent belief within it [see `PR #2472 `_] * ``flexmeasures add schedule --dry-run`` no longer saves a schedule when it is combined with ``--as-job``, where the flag used to be dropped without a word and the queued job stored its schedule anyway; that combination is now rejected, and a dry run says how many beliefs it would have saved and which events they cover [see `PR #2483 `_] diff --git a/flexmeasures/api/v3_0/accounts.py b/flexmeasures/api/v3_0/accounts.py index 96aaca538d..c8ce75df74 100644 --- a/flexmeasures/api/v3_0/accounts.py +++ b/flexmeasures/api/v3_0/accounts.py @@ -14,7 +14,7 @@ ) from flexmeasures.auth.decorators import permission_required_for_context from flexmeasures.data.models.annotations import Annotation, get_or_create_annotation -from flexmeasures.data.models.audit_log import AuditLog +from flexmeasures.data.models.audit_log import AuditLog, truncate_string from flexmeasures.data.models.user import Account, AccountRole, Plan, User from flexmeasures.data.models.generic_assets import GenericAsset from flexmeasures.data.services.accounts import get_accounts, get_audit_log_records @@ -386,30 +386,37 @@ def patch(self, account_data: dict, id: int, account: Account): "account_roles", ] - modified_fields = { - field: getattr(account, field) - for field in fields_to_check - if account_data.get(field) != getattr(account, field) - } - - # Compile modified fields string - modified_fields_str = ", ".join(modified_fields.keys()) + modified_fields = {} + for field in fields_to_check: + if field not in account_data: + continue + old_value = getattr(account, field) + new_value = account_data[field] + if field == "account_roles": + old_value = sorted(role.name for role in old_value) + new_value = sorted(role.name for role in new_value) + if old_value != new_value: + modified_fields[field] = (old_value, new_value) + + # Compile modified fields with their old and new values before updating. + changes = "; ".join( + f"{field}: {old_value!r} -> {new_value!r}" + for field, (old_value, new_value) in modified_fields.items() + ) for k, v in account_data.items(): setattr(account, k, v) - event_message = f"Account Updated, Field: {modified_fields_str}" - - # Add Audit log - account_audit_log = AuditLog( - event_datetime=server_now(), - event=event_message, - active_user_id=current_user.id, - active_user_name=current_user.username, - affected_user_id=current_user.id, - affected_account_id=account.id, - ) - db.session.add(account_audit_log) + # Add Audit log only when values actually changed. + if modified_fields: + account_audit_log = AuditLog( + event_datetime=server_now(), + event=truncate_string(f"Organisation updated: {changes}", 500), + active_user_id=current_user.id, + active_user_name=current_user.username, + affected_account_id=account.id, + ) + db.session.add(account_audit_log) db.session.commit() return account_schema.dump(account), 200 diff --git a/flexmeasures/api/v3_0/tests/test_accounts_audit_fresh_db.py b/flexmeasures/api/v3_0/tests/test_accounts_audit_fresh_db.py new file mode 100644 index 0000000000..89c2cb49d6 --- /dev/null +++ b/flexmeasures/api/v3_0/tests/test_accounts_audit_fresh_db.py @@ -0,0 +1,139 @@ +import json + +from flask import url_for +import pytest +from sqlalchemy import select + +from flexmeasures.data.models.audit_log import AuditLog +from flexmeasures.data.models.user import Account +from flexmeasures.data.services.users import find_user_by_email + +pytestmark = pytest.mark.parametrize( + "requesting_user", ["test_admin_user@seita.nl"], indirect=True +) + + +def account_events(db, account_id): + return db.session.scalars( + select(AuditLog).filter_by(affected_account_id=account_id) + ).all() + + +@pytest.mark.parametrize("resubmit_unchanged_fields", [False, True]) +def test_account_attribute_audit_reports_only_changes( + fresh_db, + client, + setup_roles_users_fresh_db, + requesting_user, + resubmit_unchanged_fields, +): + """Attribute edits record old and new values without listing unchanged fields.""" + account = find_user_by_email("test_prosumer_user@seita.nl").account + account.attributes = {"max_power_kw": 10} + fresh_db.session.commit() + previous_ids = {log.id for log in account_events(fresh_db, account.id)} + payload = {"attributes": json.dumps({"max_power_kw": 50})} + if resubmit_unchanged_fields: + payload.update( + name=account.name, + logo_url=account.logo_url, + account_roles=[role.id for role in account.account_roles], + ) + + response = client.patch(url_for("AccountAPI:patch", id=account.id), json=payload) + + assert response.status_code == 200, response.json + logs = [ + log + for log in account_events(fresh_db, account.id) + if log.id not in previous_ids + ] + assert len(logs) == 1 + event = logs[0].event + assert "attributes" in event + assert "max_power_kw" in event + assert "10" in event and "50" in event + for unchanged_field in ("name", "logo_url", "account_roles", "plan_id"): + assert unchanged_field not in event + assert logs[0].active_user_id == requesting_user.id + + +@pytest.mark.parametrize("payload_type", ["empty", "same", "reordered_roles"]) +def test_account_noop_patch_does_not_add_audit_log( + fresh_db, client, setup_roles_users_fresh_db, requesting_user, payload_type +): + """Unchanged values and reordered role lists produce no audit entry.""" + account = fresh_db.session.execute( + select(Account).filter_by(name="Multi Role Account") + ).scalar_one() + payload = {} + if payload_type == "same": + payload = {"name": account.name, "attributes": json.dumps(account.attributes)} + elif payload_type == "reordered_roles": + payload = { + "account_roles": [role.id for role in reversed(account.account_roles)] + } + previous_ids = {log.id for log in account_events(fresh_db, account.id)} + + response = client.patch(url_for("AccountAPI:patch", id=account.id), json=payload) + + assert response.status_code == 200, response.json + assert {log.id for log in account_events(fresh_db, account.id)} == previous_ids + + +def test_account_large_attribute_audit_fits_storage( + fresh_db, client, setup_roles_users_fresh_db, requesting_user +): + """Large attribute values can be saved with bounded audit events.""" + account = find_user_by_email("test_prosumer_user@seita.nl").account + account.attributes = {"description": "before" * 200} + fresh_db.session.commit() + previous_ids = {log.id for log in account_events(fresh_db, account.id)} + attributes = {"description": "after" * 200} + + response = client.patch( + url_for("AccountAPI:patch", id=account.id), + json={"attributes": json.dumps(attributes)}, + ) + + assert response.status_code == 200, response.json + assert json.loads(response.json["attributes"]) == attributes + logs = [ + log + for log in account_events(fresh_db, account.id) + if log.id not in previous_ids + ] + assert len(logs) == 1 + assert len(logs[0].event) <= 500 + assert "attributes" in logs[0].event + assert "before" in logs[0].event and "after" in logs[0].event + + +def test_account_role_audit_uses_role_names( + fresh_db, client, setup_roles_users_fresh_db, requesting_user +): + """Role edits display meaningful old and new role names.""" + account = fresh_db.session.execute( + select(Account).filter_by(name="Multi Role Account") + ).scalar_one() + remaining_role = next( + role for role in account.account_roles if role.name == "Prosumer" + ) + previous_ids = {log.id for log in account_events(fresh_db, account.id)} + + response = client.patch( + url_for("AccountAPI:patch", id=account.id), + json={"account_roles": [remaining_role.id]}, + ) + + assert response.status_code == 200, response.json + logs = [ + log + for log in account_events(fresh_db, account.id) + if log.id not in previous_ids + ] + assert len(logs) == 1 + assert "account_roles" in logs[0].event + for role_name in ("Prosumer", "Supplier", "Dummy"): + assert role_name in logs[0].event + assert "object at" not in logs[0].event 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 09086c375a..879f39654f 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 @@ -222,7 +222,10 @@ def test_edit_user( assert db.session.execute( select(AuditLog).filter_by( affected_user_id=user.id, - event="Active status set to 'False'.", + event=( + f"Updated user {user.username!r} (ID: {user.id}): " + "Active status changed from 'True' to 'False'." + ), active_user_id=requesting_user.id, affected_account_id=user.account_id, ) 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 433190d188..894ae1be03 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 @@ -5,7 +5,7 @@ from flexmeasures.api.tests.utils import UserContext from flexmeasures.data.services.users import find_user_by_email from flexmeasures.data.models.audit_log import AuditLog -from flexmeasures.data.models.user import Account +from flexmeasures.data.models.user import Account, User @pytest.mark.parametrize( @@ -75,6 +75,7 @@ def test_user_reset_password( indirect=["requesting_user"], ) def test_user_role_successful_modification_permission( + fresh_db, client, setup_roles_users_fresh_db, requesting_user, @@ -82,6 +83,9 @@ def test_user_role_successful_modification_permission( user_to_update, expected_role, ): + user = fresh_db.session.get(User, user_to_update) + username, account_id = user.username, user.account_id + previous_ids = set(fresh_db.session.scalars(select(AuditLog.id)).all()) patch_user_response = client.patch( url_for("UserAPI:patch", id=user_to_update), json={"flexmeasures_roles": expected_role}, @@ -89,6 +93,57 @@ def test_user_role_successful_modification_permission( print("Server responded with:\n%s" % patch_user_response.data) assert patch_user_response.status_code == expected_status_code + logs = fresh_db.session.scalars( + select(AuditLog).filter_by(affected_user_id=user_to_update) + ).all() + logs = [log for log in logs if log.id not in previous_ids] + assert len(logs) == 1 + event = logs[0].event + assert username in event + assert str(user_to_update) in event + assert ("Added role(s)" if expected_role else "Removed role(s)") in event + assert logs[0].active_user_id == requesting_user.id + assert logs[0].affected_account_id == account_id + + account_audit_response = client.get(url_for("AccountAPI:auditlog", id=account_id)) + assert account_audit_response.status_code == 200 + assert event in [log["event"] for log in account_audit_response.json] + + +@pytest.mark.parametrize("requesting_user", ["test_admin_user@seita.nl"], indirect=True) +@pytest.mark.parametrize( + "initial_active, active", [(True, True), (True, False), (False, True)] +) +def test_user_active_status_audit( + fresh_db, + client, + setup_roles_users_fresh_db, + requesting_user, + initial_active, + active, +): + """Status changes identify the user and values; unchanged status adds no entry.""" + user = find_user_by_email("test_prosumer_user@seita.nl") + user.active = initial_active + fresh_db.session.commit() + previous_ids = set(fresh_db.session.scalars(select(AuditLog.id)).all()) + + response = client.patch( + url_for("UserAPI:patch", id=user.id), json={"active": active} + ) + + assert response.status_code == 200, response.json + logs = fresh_db.session.scalars( + select(AuditLog).filter_by(affected_user_id=user.id) + ).all() + logs = [log for log in logs if log.id not in previous_ids] + if active == initial_active: + assert not logs + else: + assert len(logs) == 1 + assert user.username in logs[0].event + assert str(user.id) in logs[0].event + assert f"from '{initial_active}' to '{active}'" in logs[0].event @pytest.mark.parametrize( diff --git a/flexmeasures/api/v3_0/users.py b/flexmeasures/api/v3_0/users.py index f9a446d2e1..7d6da53155 100644 --- a/flexmeasures/api/v3_0/users.py +++ b/flexmeasures/api/v3_0/users.py @@ -11,7 +11,7 @@ from werkzeug.exceptions import Forbidden from flexmeasures.auth.policy import can_modify_role, check_access -from flexmeasures.data.models.audit_log import AuditLog +from flexmeasures.data.models.audit_log import AuditLog, truncate_string from flexmeasures.data.models.user import User as UserModel, Account from flexmeasures.api.common.schemas.users import AccountIdField, UserIdField from flexmeasures.api.common.utils.api_utils import get_accessible_accounts @@ -476,7 +476,7 @@ def patch(self, id: int, user: UserModel, **user_data): # noqa C901 f"You are not allowed to remove ({role.name}) role from this user." ) if roles_being_removed: - audit_event += f"Removed role(s): [{','.join([r.name for r in roles_being_removed])}]." + audit_event += f"Removed role(s): [{','.join(sorted(r.name for r in roles_being_removed))}]. " roles_being_added = new_roles - current_roles for role in roles_being_added: @@ -485,13 +485,14 @@ def patch(self, id: int, user: UserModel, **user_data): # noqa C901 f"You are not allowed to add ({role.name}) role to this user." ) if roles_being_added: - audit_event += f"Added role(s): [{','.join([r.name for r in roles_being_added])}]." + audit_event += f"Added role(s): [{','.join(sorted(r.name for r in roles_being_added))}]. " + old_value = getattr(user, k) setattr(user, k, v) if k == "active" and v is False: remove_cookie_and_token_access(user) - if k == "active": - audit_event += f"Active status set to '{v}'." + if k == "active" and old_value != v: + audit_event += f"Active status changed from '{old_value}' to '{v}'. " if audit_event: user_audit_log = create_user_audit_log(audit_event, user) db.session.add(user_audit_log) @@ -690,7 +691,10 @@ def create_user_audit_log(audit_event: str, user: UserModel): ) return AuditLog( event_datetime=server_now(), - event=audit_event, + event=truncate_string( + f"Updated user {user.username!r} (ID: {user.id}): {audit_event.strip()}", + 500, + ), active_user_id=active_user_id, active_user_name=active_user_name, affected_user_id=user.id, From 06d87c255dd7f88f2c8c738c427c9ea1853e2255 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Thu, 10 Sep 2026 18:02:36 +0200 Subject: [PATCH 2/2] add PR Nr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- documentation/changelog.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index af1ce76983..ef94e5dc7c 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -48,8 +48,7 @@ Infrastructure / Support Bugfixes ----------- -* Organisation audit logs now show only changed fields with their previous and new values, and user role and active-status changes identify the affected user - +* Organisation audit logs now show only changed fields with their previous and new values, and user role and active-status changes identify the affected user [see `PR #2522 `_] * Where several data sources report the same event, which one a search keeps is now decided the same way every time: a source version only counts against other versions of that source, and a caller that lists its sources gets the order it asked for [see `PR #2494 `_] * A KPI on the asset page counted an event once per data source that reported it, so a total could come out higher than any source reported; it now reduces one value per event, from the latest source version and the most recent belief within it [see `PR #2472 `_] * ``flexmeasures add schedule --dry-run`` no longer saves a schedule when it is combined with ``--as-job``, where the flag used to be dropped without a word and the queued job stored its schedule anyway; that combination is now rejected, and a dry run says how many beliefs it would have saved and which events they cover [see `PR #2483 `_]