Merge pull request #119 from magfest/feature/audit-role-changes

Audit role grants and revokes

Author: CarensirA-MF

app/routes/admin/data_upload.py

@@ -44,6 +44,14 @@
     UI_GROUP_HOTEL_SERVICES,
 )
 from app.routes import h
+from app.routes.admin.users import _role_code
+from app.security_audit import (
+    log_security_event,
+    EVENT_USER_MODIFY,
+    CATEGORY_ADMIN,
+    SEVERITY_ALERT,
+    SEVERITY_INFO,
+)
 from .helpers import (
     require_super_admin,
     render_admin_config_page,
@@ -1282,6 +1290,7 @@ def user_roles_upload():
 
     created = 0
     skipped = 0
+    granted_by_user_id: dict[str, dict] = {}
 
     for idx, row in df.iterrows():
         user_identifier = _get_cell_value(row, user_col)
@@ -1332,7 +1341,41 @@ def user_roles_upload():
                 approval_group_id=approval_group_id,
             )
             db.session.add(role)
+            # UserRole.approval_group has load_on_pending=False (the
+            # SQLAlchemy default). A lazy load on a pending, unflushed
+            # object returns None without querying; it does not wait for
+            # autoflush. Flush now so _role_code() below reads the real
+            # approval group instead of silently dropping it.
+            db.session.flush()
             created += 1
+            entry = granted_by_user_id.setdefault(
+                user.id, {"user": user, "codes": [], "raw_codes": []}
+            )
+            entry["codes"].append(_role_code(role))
+            entry["raw_codes"].append(role_code)
+
+    # One audit entry per user touched, logged before the commit so a failure
+    # between the two can never record a grant that did not happen.
+    for entry in granted_by_user_id.values():
+        user = entry["user"]
+        granted = entry["codes"]
+        # Severity is computed from the raw role codes, not the rendered
+        # ones. approval_group_id is set on the row for any role, not just
+        # APPROVER, so a SUPER_ADMIN row with a group column filled would
+        # render "SUPER_ADMIN:TECH" and silently fail a membership test
+        # against ROLE_SUPER_ADMIN.
+        log_security_event(
+            EVENT_USER_MODIFY,
+            category=CATEGORY_ADMIN,
+            severity=SEVERITY_ALERT if ROLE_SUPER_ADMIN in entry["raw_codes"] else SEVERITY_INFO,
+            details={
+                "target_user_id": user.id,
+                "target_email": user.email,
+                "granted": sorted(granted),
+                "revoked": [],
+                "source": "bulk_upload",
+            },
+        )
 
     db.session.commit()
 

app/routes/admin/users.py

@@ -23,6 +23,13 @@
     CONFIG_AUDIT_RESTORE,
 )
 from app.routes import h
+from app.security_audit import (
+    log_security_event,
+    EVENT_USER_MODIFY,
+    CATEGORY_ADMIN,
+    SEVERITY_ALERT,
+    SEVERITY_INFO,
+)
 from .helpers import (
     require_super_admin,
     render_admin_config_page,
@@ -306,6 +313,24 @@ def restore_user(user_id: str):
     return redirect(url_for(".list_users"))
 
 
+def _role_code(role: UserRole) -> str:
+    """One role as a readable code, never a database ID."""
+    if role.work_type_id and role.work_type:
+        return f"{role.role_code}:{role.work_type.code}"
+    if role.approval_group_id and role.approval_group:
+        return f"{role.role_code}:{role.approval_group.code}"
+    return role.role_code
+
+
+def _role_codes(user: User) -> set[str]:
+    """Current roles as readable codes, never database IDs.
+
+    An ID is meaningless in an audit entry read a year later, and it breaks if
+    the work type or approval group is renamed or removed.
+    """
+    return {_role_code(r) for r in user.roles}
+
+
 def _update_user_roles(user: User, form_data) -> None:
     """
     Update user roles based on form data.
@@ -314,7 +339,13 @@ def _update_user_roles(user: User, form_data) -> None:
     - role_super_admin: "1" if checked
     - role_worktype_admin_<work_type_id>: "1" if checked
     - role_approver_<approval_group_id>: "1" if checked
+
+    Snapshots the role set before clearing it, and logs a security audit
+    entry naming what was granted and revoked. The snapshot must come first;
+    clear() plus flush() below leaves nothing to diff against afterward.
     """
+    before = _role_codes(user)
+
     # Clear existing roles
     user.roles.clear()
     db.session.flush()
@@ -348,3 +379,34 @@ def _update_user_roles(user: User, form_data) -> None:
                 approval_group_id=ag.id,
             )
             db.session.add(role)
+
+    # New roles are added via db.session.add(), not user.roles.append(). The
+    # already-loaded `roles` collection on `user` (cached when `before` ran)
+    # does not see them. flush() persists the FK rows; it does not refresh a
+    # collection already loaded in memory. Expire it so the next access
+    # requeries instead of returning the stale list.
+    db.session.flush()
+    db.session.expire(user, ["roles"])
+    after = _role_codes(user)
+
+    granted = sorted(after - before)
+    revoked = sorted(before - after)
+    if not granted and not revoked:
+        return
+
+    log_security_event(
+        EVENT_USER_MODIFY,
+        category=CATEGORY_ADMIN,
+        severity=(
+            SEVERITY_ALERT
+            if ROLE_SUPER_ADMIN in granted or ROLE_SUPER_ADMIN in revoked
+            else SEVERITY_INFO
+        ),
+        details={
+            "source": "admin_form",
+            "target_user_id": user.id,
+            "target_email": user.email,
+            "granted": granted,
+            "revoked": revoked,
+        },
+    )

app/security_audit.py

@@ -9,10 +9,14 @@
 from __future__ import annotations
 
 import json
+import logging
 from datetime import datetime
 from flask import request, session, has_request_context
 from app import db
 from app.models import SecurityAuditLog
+from app.services.slack import send_slack_message
+
+logger = logging.getLogger(__name__)
 
 # Event categories
 CATEGORY_AUTH = "AUTH"
@@ -38,6 +42,23 @@
 SEVERITY_ALERT = "ALERT"
 
 
+def _format_security_alert(event_type: str, user_id: str | None, details: dict | None) -> str:
+    """One line of Slack text for an ALERT-severity security event."""
+    actor = user_id or "unknown user"
+    parts = [f"Security alert: {event_type} by {actor}"]
+    if details:
+        target = details.get("target_email") or details.get("target_user_id")
+        if target:
+            parts.append(f"target {target}")
+        granted = details.get("granted")
+        if granted:
+            parts.append(f"granted {', '.join(granted)}")
+        revoked = details.get("revoked")
+        if revoked:
+            parts.append(f"revoked {', '.join(revoked)}")
+    return ". ".join(parts)
+
+
 def log_security_event(
     event_type: str,
     category: str,
@@ -83,6 +104,30 @@ def log_security_event(
         details=json.dumps(details) if details else None,
     )
     db.session.add(event)
+
+    # ALERT posts to Slack synchronously, with a 10 second timeout on the
+    # request path. Keep ALERT for rare, deliberate events. Raising a
+    # high-frequency event such as ACCESS_DENIED to ALERT would attempt a post
+    # on every occurrence. send_slack_message never raises and returns False
+    # when Slack is disabled, unconfigured, or circuit-broken, so a Slack
+    # problem cannot lose the audit row or fail the request.
+    #
+    # A caller that logs ALERT events inside a per-row loop before a single
+    # commit multiplies this cost. Twenty rows with Slack unreachable is
+    # twenty sequential ten-second waits, holding the transaction open.
+    if severity == SEVERITY_ALERT:
+        try:
+            send_slack_message(
+                text=_format_security_alert(event_type, user_id, details),
+                template_key=f"security_{event_type}",
+                work_item_id=None,
+            )
+        except Exception:
+            # send_slack_message documents that it never raises. This guard is
+            # here so that if that ever stops being true, a Slack problem
+            # cannot swallow the audit row, which is the actual control.
+            logger.exception("Slack post failed for %s security alert", event_type)
+
     return event