fix(wizard): admin/owner-rolcontrole en bescherming privileged velden bij wizard-bewerken (#69)

refactor(security): centralize project-edit guards + immutable field check

Author: anneschuth

features/futures/cluster-editing.md

@@ -0,0 +1,21 @@
+# Cluster editing (future feature)
+
+Status: **niet ondersteund**. `clusters` op een bestaand project is op dit
+moment niet wijzigbaar; de keuze wordt gemaakt bij projectaanmaak en blijft
+daarna staan.
+
+Beperkingen vandaag:
+
+- Geen edit-mode form exposes het veld (`IDENTITY_EDIT_SECTION` in
+  `forms/visualizers/wizard_sections.py` toont alleen display-name +
+  description).
+- Geen add/remove-logica voor cluster aan een bestaand project.
+- `clusters` zit in `IMMUTABLE_PROJECT_FIELDS`
+  (`opi/web/project_edit_security.py`): een form die het toch zou
+  submitten krijgt 400 (geen edit-form exposed het, dus zien we het =
+  bug of tamper).
+
+Bij implementatie: form-veld toevoegen, `clusters` uit
+`IMMUTABLE_PROJECT_FIELDS` halen, en de master-OPI propagatie testen voor
+zowel add (nieuwe cluster pickt project op) als remove (vertrekkende OPI
+ruimt op).

features/futures/form-field-rbac.md

@@ -0,0 +1,22 @@
+# Form field-level RBAC (future refactor)
+
+Status: **niet geïmplementeerd**. Vandaag past elke save-handler
+zelf `require_project_edit_access` (rol-gate) toe via
+`opi/web/project_edit_security.py`. Vergeet één save-pad en de
+gate is daar afwezig.
+
+Het `Editable`/`FormSection`-systeem
+(`opi/forms/editables/editable.py`) heeft geen `requires_role`-
+attribuut. Er is een `AdminEnforcer` voor business-rules ("≥1 admin
+moet blijven bestaan") maar geen field-level "wie mag dit veld editen".
+
+Wenselijke eindstaat:
+
+- `requires_role: str | None` op `Editable` of `FormSection`
+- Centrale `form_submission_guard` matcht request-user-role tegen
+  field-metadata vóór de processor draait — niet-toegestane waardes
+  leveren een `FieldError`
+- Form-renderer disablet velden voor users zonder permission (UI-feedback
+  in plaats van stille rejection achteraf)
+- Eén bron van waarheid voor RBAC: definitie naast de field-definitie,
+  automatisch geldend op alle save-paden

operations-manager/python/opi/web/project_edit_security.py

@@ -0,0 +1,71 @@
+"""Shared guards for project-editing save handlers.
+
+- ``require_project_edit_access``: admin/owner role gate, re-check on every
+  mutating request (TOCTOU).
+- ``apply_form_data_to_project``: merge submitted form data into the stored
+  project; reject submissions that include immutable top-level fields.
+
+Role-based protection (e.g. only admin/owner may edit ``users``) lives in
+the role gate; this module does not duplicate that. See
+``features/futures/form-field-rbac.md`` for the proper field-level RBAC
+that would replace today's per-handler glue.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from fastapi import HTTPException, Request
+
+from opi.core.auth_decorators import get_current_user
+from opi.services.project_service import get_project_service
+
+
+def require_project_edit_access(request: Request, project_name: str):
+    """Require admin/owner role on the project. Returns (project, user_email).
+
+    Raises HTTPException 404 if the project does not exist, 403 if the user
+    is not authorized for the project or lacks the required role.
+    """
+    user = get_current_user(request)
+    project_service = get_project_service()
+    project = project_service.get_project(project_name)
+
+    if not project:
+        raise HTTPException(status_code=404, detail=f"Project '{project_name}' niet gevonden")
+
+    user_email = user.get("email", "").lower()
+    if not project_service.is_user_authorized_for_project(project_name, user_email):
+        raise HTTPException(status_code=403, detail="Geen toegang tot dit project")
+
+    user_role = project_service.get_user_role_for_project(project_name, user_email)
+    if user_role not in ("admin", "owner"):
+        raise HTTPException(status_code=403, detail="Onvoldoende rechten om dit project te bewerken")
+
+    return project, user_email
+
+
+# Top-level fields no form should ever submit. name=on-disk filename;
+# clusters=not yet editable (see features/futures/cluster-editing.md).
+IMMUTABLE_PROJECT_FIELDS: tuple[str, ...] = ("name", "clusters")
+
+
+def apply_form_data_to_project(
+    existing_data: dict[str, Any],
+    submitted_data: dict[str, Any],
+) -> dict[str, Any]:
+    """Merge submitted form data into the stored project data. Returns a new dict.
+
+    Raises HTTPException 400 if the submission tries to set any
+    ``IMMUTABLE_PROJECT_FIELDS``. That is a structural-integrity violation:
+    no form should expose these fields, so receiving one means either a
+    form bug or a tampered submission. Fail loud rather than silently
+    dropping the value.
+    """
+    offending = [field for field in IMMUTABLE_PROJECT_FIELDS if field in submitted_data]
+    if offending:
+        raise HTTPException(
+            status_code=400,
+            detail=f"Submitted form data contains immutable project fields: {', '.join(offending)}",
+        )
+    return {**existing_data, **submitted_data}

operations-manager/python/opi/web/router_detail_edit.py

@@ -39,6 +39,10 @@
     init_modal_state_tokenized,
     save_modal_state_by_token,
 )
+from opi.web.project_edit_security import (
+    apply_form_data_to_project,
+    require_project_edit_access,
+)
 from opi.web.router_wizard import (
     _empty_sequence_item,
     _extract_section_data,
@@ -169,28 +173,6 @@ def _render_section_html(
     return html
 
 
-def _require_project_edit_access(request: Request, project_name: str):
-    """Check auth and return (project, user_email). Raises on failure."""
-    from opi.services.project_service import get_project_service
-
-    user = get_current_user(request)
-    project_service = get_project_service()
-    project = project_service.get_project(project_name)
-
-    if not project:
-        raise HTTPException(status_code=404, detail=f"Project '{project_name}' niet gevonden")
-
-    user_email = user.get("email", "").lower()
-    if not project_service.is_user_authorized_for_project(project_name, user_email):
-        raise HTTPException(status_code=403, detail="Geen toegang tot dit project")
-
-    user_role = project_service.get_user_role_for_project(project_name, user_email)
-    if user_role not in ("admin", "owner"):
-        raise HTTPException(status_code=403, detail="Onvoldoende rechten om dit project te bewerken")
-
-    return project, user_email
-
-
 def _require_project_member_access(request: Request, project_name: str):
     """Check auth for project member access (any role). Returns (project, user_email)."""
     from opi.services.project_service import get_project_service
@@ -523,7 +505,7 @@ async def _start_deployment(
 @requires_sso
 async def sequence_action(request: Request, project_name: str, section_id: str) -> HTMLResponse:
     """Handle add/remove sequence item and re-render the section form."""
-    project, _user_email = _require_project_edit_access(request, project_name)
+    project, _user_email = require_project_edit_access(request, project_name)
 
     section = _get_edit_section(section_id)
     project_data = project.data or {}
@@ -580,7 +562,7 @@ async def modal_wizard_init(request: Request, project_name: str, flow_id: str) -
     if _is_backup_restore_flow(flow_id):
         project, _user_email = _require_project_member_access(request, project_name)
     else:
-        project, _user_email = _require_project_edit_access(request, project_name)
+        project, _user_email = require_project_edit_access(request, project_name)
 
     project_data = project.data or {}
 
@@ -709,7 +691,7 @@ async def modal_wizard_init(request: Request, project_name: str, flow_id: str) -
 @requires_sso
 async def modal_wizard_load_step(request: Request, project_name: str, flow_id: str, section_id: str) -> HTMLResponse:
     """Load a step (for back-navigation)."""
-    _require_project_edit_access(request, project_name)
+    require_project_edit_access(request, project_name)
 
     wizard_token = _get_wizard_token(request)
     state = get_modal_state_by_token(wizard_token)
@@ -737,7 +719,7 @@ async def modal_wizard_submit_step(request: Request, project_name: str, flow_id:
     if _is_backup_restore_flow(flow_id):
         _require_project_member_access(request, project_name)
     else:
-        _require_project_edit_access(request, project_name)
+        require_project_edit_access(request, project_name)
 
     wizard_token = _get_wizard_token(request)
     state = get_modal_state_by_token(wizard_token)
@@ -939,7 +921,7 @@ async def modal_wizard_skip(request: Request, project_name: str, flow_id: str) -
     if _is_backup_restore_flow(flow_id):
         _require_project_member_access(request, project_name)
     else:
-        _require_project_edit_access(request, project_name)
+        require_project_edit_access(request, project_name)
 
     wizard_token = await _get_wizard_token_with_body(request)
     state = get_modal_state_by_token(wizard_token)
@@ -957,7 +939,7 @@ async def modal_wizard_confirm(request: Request, project_name: str, flow_id: str
     if _is_backup_restore_flow(flow_id):
         _require_project_member_access(request, project_name)
     else:
-        _require_project_edit_access(request, project_name)
+        require_project_edit_access(request, project_name)
 
     wizard_token = await _get_wizard_token_with_body(request)
     state = get_modal_state_by_token(wizard_token)
@@ -1105,6 +1087,11 @@ async def _modal_do_submit(
     from opi.handlers.project_file_handler import save_project_file
     from opi.services.project_service import get_project_service
 
+    # TOCTOU recheck on the mutating request. Backup/restore has its own
+    # member-level gate further down.
+    if not _is_backup_restore_flow(flow_id):
+        require_project_edit_access(request, project_name)
+
     state = get_modal_state_by_token(wizard_token)
     if not state:
         raise HTTPException(status_code=400, detail=_SESSION_EXPIRED)
@@ -1179,7 +1166,7 @@ async def _modal_do_submit(
                         if field in existing_dep and field not in new_dep:
                             new_dep[field] = existing_dep[field]
 
-    existing_data.update(merged_data)
+    existing_data = apply_form_data_to_project(existing_data, merged_data)
 
     # Run post_merge hooks (e.g. distribute component refs to deployments)
     for section in active_sections:

operations-manager/python/opi/web/router_wizard.py

@@ -413,20 +413,16 @@ async def wizard_page(request: Request, flow_id: str) -> HTMLResponse:
 @requires_sso
 async def wizard_edit_page(request: Request, flow_id: str, project_name: str) -> HTMLResponse:
     """Render the wizard page pre-filled from an existing project."""
-    from opi.services.project_service import get_project_service
+    from opi.web.project_edit_security import require_project_edit_access
 
     flow = get_flow(flow_id)
     user = get_current_user(request)
     templates = get_templates()
 
-    project_service = get_project_service()
-    project = project_service.get_project(project_name)
-    if not project:
-        raise HTTPException(status_code=404, detail=f"Project '{project_name}' niet gevonden")
-
-    user_email = user.get("email", "").lower()
-    if not project_service.is_user_authorized_for_project(project_name, user_email):
-        raise HTTPException(status_code=403, detail="Geen toegang tot dit project")
+    # Enforce admin/owner role: the wizard edit flow exposes users/role and
+    # config fields as editable, so a plain member must not be able to enter
+    # it (project takeover).
+    project, _user_email = require_project_edit_access(request, project_name)
 
     project_data = project.data
     if not project_data:
@@ -740,6 +736,13 @@ async def submit_step(request: Request, flow_id: str, section_id: str) -> HTMLRe
     if not state or state.flow_id != flow_id:
         return RedirectResponse(url=f"/forms/wizard/{flow_id}", status_code=303)
 
+    # Edit-mode only: fail fast per step instead of stripping data at final
+    # save. Also covers any future step-driven side-effects (e.g. auto-save).
+    if state.project_name:
+        from opi.web.project_edit_security import require_project_edit_access
+
+        require_project_edit_access(request, state.project_name)
+
     section = _get_section_from_flow(flow_id, section_id)
     flow = get_flow(flow_id)
     templates = get_templates()
@@ -1877,15 +1880,14 @@ async def _save_existing_project(
     """Save updated data to an existing project."""
     from opi.handlers.project_file_handler import save_project_file
     from opi.services.project_service import get_project_service
+    from opi.web.project_edit_security import apply_form_data_to_project, require_project_edit_access
+
+    # TOCTOU recheck on the mutating request.
+    project, _user_email = require_project_edit_access(request, project_name)
 
     project_service = get_project_service()
-    project = project_service.get_project(project_name)
-    if not project:
-        raise HTTPException(status_code=404, detail=f"Project '{project_name}' niet gevonden")
 
-    # Merge with existing data (preserve system-managed fields)
-    existing_data = project.data or {}
-    existing_data.update(data)
+    existing_data = apply_form_data_to_project(project.data or {}, data)
 
     save_project_file(project.filename, existing_data)
     project_service.load_project_from_data(existing_data, project.filename)