diff --git a/CLAUDE.md b/CLAUDE.md index 1cfd042..e2c5ceb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1484,6 +1484,24 @@ A **PDF Summary** button was added to `reports/scorecard.html` alongside the exi - If `GROQ_API_KEY` is absent, input is disabled and a fallback "Submit to Support" link is shown (the reply is still persisted). - "Submit to Support" modal POSTs to `POST /support/tickets`; subject pre-filled from last user message in history. +### Flag-issue assignee scoping (Aug 2026) + +The "Assign to" dropdown in the flag-issue offcanvas was built from an **org-wide** query (`role in [director, inspector, external_inspector, ...]`), so a Customer Inspector could assign an issue to anyone in the system — including **another client's** Customer Inspector, who was then emailed the facility name and issue description. A cross-customer data leak, and the same leak in reverse whenever one of our own inspectors picked the wrong name. + +`_assignable_staff_for(inspection, actor)` in `routes/inspections.py` is now the single source for that list: + +| Role group | Scope | +|---|---| +| `inspector`, `external_inspector` | only those holding an `InspectorAssignment` on **this inspection's contract** — the same rows `get_inspector_scope()` reads, so the offered assignee can always open what they were given | +| `director`, `project_manager`, `auditor` | org-wide (they hold no `InspectorAssignment`, so contract-scoping would remove them entirely and break escalation) — but offered **only to our own staff** | +| inactive accounts | never offered | + +So a **Customer Inspector sees only the inspectors on their own contracts** — their colleagues plus ours — and never our internal org chart. A facility with no contract yields no contract-scoped candidates: fail-closed, leaving "Unassigned" as the only option. + +**Both call sites must use it.** `execute()` renders the dropdown; `flag_issue()` builds `form.assigned_to.choices`, which is what actually **validates the POST** — that is the security boundary, since the dropdown is only a UI hint. They were previously two hand-maintained queries that had already drifted: the offcanvas offered `project_manager` and `auditor` while the choices rejected them, so picking one **silently discarded the issue** (see below). One helper, one list, no drift. + +**A failed flag-issue POST now returns 400, not 200.** The offcanvas JS treats `res.ok` as success and reloads the page, so a 200 on a validation failure means the inspector watches the panel close and believes the issue was logged when nothing was saved — rule 60's failure mode, reachable through the PM/auditor drift above and through any rejected assignee. The route returns the re-rendered form with **400** so the JS error branch fires, and flashes a specific message for an out-of-contract assignee rather than "Not a valid choice". + ### Inspection Execute Page — UX Patterns - **Photo upload-on-select**: `uploadPhotoField(input)` fires immediately on `` change. XHR to `POST //upload-photo`. On success, the server path is written to `` and a `` is shown. @@ -1615,9 +1633,11 @@ timeout = 30 | 85 | **`next_due_date` is mutable state, `end_date` is a fixed boundary — never conflate them** | `fulfill()` rewrites `next_due_date` after every completed inspection; `end_date` is set by the manager and never touched by the app. The old single label "Start / Due Date" said both at once, which is what users reported as confusing. The label now follows context — `form.next_due_date.label.text` is set to "Start Date" in `create()` and "Next Due Date" in `edit()`. Do not rename the `next_due_date` column to match a label: it is indexed, it is the API payload key the iPad decodes, and the reminder cron filters on it. | | 87 | **Never write `role == 'inspector'` — use `user.is_inspector` (`User.INSPECTOR_ROLES`)** | phase49 added `external_inspector`, which must behave as an inspector everywhere. An equality check silently drops it into the *privileged* branch of every `if inspector: scope … else: org-wide` block — i.e. a third-party inspector would see **every contract in the system**. This is a fail-OPEN mistake: nothing errors, the data just leaks. The sweep converted ~44 Python sites and 7 template sites; the only surviving `== 'inspector'` literals are the matrix docstring, the `MATRIX_DEFAULTS` mirror comprehension, and the default-checked box in `admin/broadcast.html`. Query-level checks use `User.role.in_(User.INSPECTOR_ROLES)` (never `filter_by(role='inspector')`). A **new** `app/api/*` blueprint's `_ALLOWED_ROLES` must include `external_inspector`, same as rule 79 requires for `auditor`. | | 88 | **`app/enrollment/` writes no DB row and has exactly ONE read — keep the vertical slice sealed** | The enrollment form describes accounts that do NOT exist yet (no contract, facility or user to key a row against), so it stores flat JSON in `ENROLLMENT_DIR` and owns its own templates. The single permitted model access is `mailer._admin_recipients()` reading active `admin` users to address the new-enrollment alert — function-local, read-only, and guarded so a DB failure cannot break a submission. Adding a model/migration for enrollment, or letting the public POST **create** Users, would couple an unauthenticated endpoint to the account system — the exact thing the separation buys. If enrollment must ever provision accounts, do it as a separate admin-triggered action that reads a stored submission. Submission ids are filesystem paths: validate against `_ID_RE` before every open (path traversal). See §24. | +| 93 | **The flag-issue assignee list is contract-scoped, and BOTH call sites must use `_assignable_staff_for()`** | `execute()` renders the dropdown, `flag_issue()` builds the choices that validate the POST — the choices are the security boundary. Two separate queries had already drifted (offcanvas offered project_manager/auditor, choices rejected them), which silently discarded issues. Contract scoping applies to the two inspector roles for EVERY actor, not just customer ones: an org-wide list let anyone assign another client's Customer Inspector, who was then emailed that facility's name and issue description. Never widen this back to an unscoped `User.query.filter(role.in_(...))`. | +| 94 | **A failed flag-issue POST must return a non-2xx** | The offcanvas JS branches on `res.ok`, so a 200 re-render of the invalid form reads as success: the panel closes, the page reloads, and no issue exists — with nothing in the logs and no message to the user. `flag_issue()` returns 400 on a failed POST for exactly this reason. Any future AJAX-submitted form needs the same treatment (rule 60 is the same failure seen from the other end). | | 91 | **A bulk-action form must live OUTSIDE the table; row checkboxes join it via the HTML5 `form=` attribute** | Wrapping the table in the bulk form nests the per-row delete/unfollow forms inside it, and browsers **silently discard** nested forms (rule 9) — the row buttons would post nothing, with no console error and no server log. `
` sits above the table and each checkbox carries `form="issuesBulkForm"`. Same for `inspectionsBulkForm`. Applies to all four list templates (classic + modern). | | 92 | **Bulk deletes: DB rows first, storage files second** | Collect the keys, `db.session.delete()` every row, `commit()`, and only then `storage.delete()`. Deleting files first means a failed/rolled-back commit leaves surviving rows pointing at missing photos. `_collect_inspection_photos()` is shared by the single and bulk inspection delete paths precisely so the two cannot drift — a key missed there is an invisible permanent storage leak. | -| 89 | **`User.CUSTOMER_ROLES` is for ACCOUNT MANAGEMENT; `role == 'customer'` is for CAPABILITY — never swap them** | The inverse of rule 87, and it fails in both directions. Widening a capability check to `CUSTOMER_ROLES` hands a third-party Customer Inspector the customer portal (fail-OPEN, nothing errors). Narrowing an account-management check back to `'customer'` strands every Customer Inspector in a page that no longer lists or edits them (fail-closed, but invisible until someone looks for a missing account). `CUSTOMER_ROLES` / `is_customer_account` appear ONLY in: the `/customers` list query, its route guards, and the `auth.list_users` exclusion. Everything else — portal gates, `@customer_required`, `get_customer_scope()`, support chat, `notify_customers_for_facility()`, the customer branch of every `app/api/*` scope check — keeps the equality test, because a Customer Inspector is an **inspector** there (rule 87 already routes it correctly). | +| 89 | **`User.CUSTOMER_ROLES` is for ACCOUNT MANAGEMENT; `role == 'customer'` is for CAPABILITY — never swap them** | The inverse of rule 87, and it fails in both directions. Widening a capability check to `CUSTOMER_ROLES` hands a third-party Customer Inspector the customer portal (fail-OPEN, nothing errors). Narrowing an account-management check back to `'customer'` strands every Customer Inspector in a page that no longer lists or edits them (fail-closed, but invisible until someone looks for a missing account). `CUSTOMER_ROLES` / `is_customer_account` appear ONLY in: the `/customers` list query, its route guards, the `auth.list_users` exclusion, and **narrowing** uses that WITHHOLD something from an external account (`_assignable_staff_for()` uses it to hide our internal staff — safe direction, and commented as such). Everything else — portal gates, `@customer_required`, `get_customer_scope()`, support chat, `notify_customers_for_facility()`, the customer branch of every `app/api/*` scope check — keeps the equality test, because a Customer Inspector is an **inspector** there (rule 87 already routes it correctly). | | 90 | **A per-account notification opt-IN must survive a globally-OFF column** | `notify_by_matrix()` skips a role column early when the matrix says off. For the two customer columns that early `continue` has to also ask whether anyone opted in (`any(overrides.values())`), or the override saves, displays as on, and never sends — a silent failure with no error anywhere. Equally, `notify_customers_for_facility()` re-queries recipients from assignment rows, so `notify_by_matrix()` must hand it `allowed_user_ids` or the facility-scoped path bypasses every override. Both halves are needed; either one alone leaves a hole. See §11. | | 81 | **Photo timestamp/geo overlay is burned at UPLOAD, never on `PATCH /issues//photos`** | That PATCH receives only path strings — the bytes are already in storage and the payload carries no capture metadata. Burning there would need a read-modify-write per key plus an overwrite-in-place primitive (`storage.save()` mints a NEW uuid key, and §22 requires key == DB path), and would risk a **double burn** since the endpoint is deliberately idempotent/retry-safe (rule 45). Stamp in `POST /photos/upload`, where the raw bytes + EXIF are in hand and each call writes exactly one already-stamped object. Stamping failures must always fall back to storing the ORIGINAL bytes — never lose a photo to a stamping bug. See §23. | diff --git a/app/routes/inspections.py b/app/routes/inspections.py index 96244d5..bf15130 100644 --- a/app/routes/inspections.py +++ b/app/routes/inspections.py @@ -608,11 +608,10 @@ def execute(inspection_id): flash('Draft saved. You can continue filling in the form later.', 'success') return redirect(url_for('inspections.execute', inspection_id=inspection_id)) - staff_for_flag_issue = User.query.filter( - User.role.in_(['director', 'inspector', 'external_inspector', - 'project_manager', 'auditor']), - User.active == True, - ).order_by(User.full_name, User.username).all() + # Scoped to this inspection's contract — see _assignable_staff_for(). + # Must match flag_issue()'s choices exactly or the offcanvas silently + # fails to save (rule 60). + staff_for_flag_issue = _assignable_staff_for(inspection, current_user) return render_template('inspections/execute.html', inspection=inspection, @@ -923,6 +922,95 @@ def view(inspection_id): # ── Flag issue during inspection ────────────────────────────────────────────── +#: Internal roles that are NOT contract-scoped — they work across the whole +#: organisation, so they are offered regardless of which contract the +#: inspection belongs to. Only ever shown to our own people. +_ORG_WIDE_ASSIGNEE_ROLES = ('director', 'project_manager', 'auditor') + + +def _assignable_staff_for(inspection, actor): + """Users `actor` may assign an issue to, for THIS inspection. + + The candidate list is scoped by the inspection's CONTRACT, not taken + org-wide. Two distinct problems this fixes: + + 1. **Cross-customer leak.** A Customer Inspector could assign an issue to + anyone in the system — including another client's Customer Inspector. + The assignee is notified by email and in-app with the facility name and + issue description, so this handed one customer's data to another. It is + a leak whoever flags the issue, so the contract scope is applied to the + two inspector roles for EVERY actor, not just customer ones. + + 2. An external account should not see our internal org chart at all. For a + customer-side actor the list is their co-workers on shared contracts — + inspectors assigned to this inspection's contract — and nothing else. + + Rules applied: + * inspector / external_inspector -> only those holding an + InspectorAssignment on this inspection's contract (the same rows + get_inspector_scope() reads, so the list can never disagree with what + the assignee can actually open). + * director / project_manager / auditor -> org-wide, but offered ONLY to + our own staff. These roles carry no InspectorAssignment rows, so + contract-scoping them would remove them entirely and break the normal + "escalate to the contract manager" flow. + * inactive accounts are never offered. + + A facility with no contract yields no contract-scoped candidates; that is + fail-closed and correct — an external actor then gets an empty list and can + only leave the issue unassigned. + + Used by BOTH the offcanvas dropdown in execute() and the choices that + validate the POST in flag_issue(). They MUST stay identical: a value the UI + offers but the choices reject fails `validate_on_submit()`, and the + offcanvas JS treats the resulting 200 as success — the issue is silently + never saved (rule 60's failure mode, which is exactly what the two + hand-maintained lists were already doing to project_manager and auditor). + """ + from app.models.inspector_assignment import InspectorAssignment + + # `is_customer_account` is used here to WITHHOLD internal staff from an + # external account — the narrowing direction, which rule 89 permits. It + # must never be used to grant a customer-side account anything. + actor_is_external = bool(actor) and actor.is_customer_account + + project_id = inspection.facility.project_id if inspection.facility else None + + candidates = [] + if project_id: + candidates = ( + User.query + .join(InspectorAssignment, InspectorAssignment.user_id == User.id) + .filter( + InspectorAssignment.project_id == project_id, + User.role.in_(User.INSPECTOR_ROLES), + User.active == True, + ) + .order_by(User.full_name, User.username) + .all() + ) + + if not actor_is_external: + candidates += ( + User.query + .filter( + User.role.in_(_ORG_WIDE_ASSIGNEE_ROLES), + User.active == True, + ) + .order_by(User.full_name, User.username) + .all() + ) + + # The join can repeat a user across assignment rows; dedupe by id, keeping + # a stable display order. + seen, out = set(), [] + for u in candidates: + if u.id not in seen: + seen.add(u.id) + out.append(u) + out.sort(key=lambda u: (u.display_name or '').lower()) + return out + @bp.route('//flag-issue', methods=['GET', 'POST']) @login_required def flag_issue(inspection_id): @@ -935,13 +1023,14 @@ def flag_issue(inspection_id): return redirect(url_for('inspections.index')) form = IssueForm() - staff = User.query.filter( - User.role.in_(['director', 'inspector', 'external_inspector']) - ).order_by(User.username).all() + # SAME list the offcanvas rendered — this is what actually validates the + # POST, so it is also the security boundary: a crafted assigned_to for + # someone outside this contract fails validation rather than being stored. + staff = _assignable_staff_for(inspection, current_user) form.facility_id.choices = [(inspection.facility_id, inspection.facility.name)] form.assigned_to.choices = [(0, '— Unassigned —')] + [ - (u.id, u.username + (' (Customer)' if u.is_external_inspector else '')) + (u.id, u.display_name + (' (Customer)' if u.is_external_inspector else '')) for u in staff ] @@ -1008,6 +1097,21 @@ def flag_issue(inspection_id): flash('Issue logged successfully.', 'success') return redirect(url_for('inspections.execute', inspection_id=inspection_id)) + # A failed POST must NOT come back 200. The flag-issue offcanvas treats + # `res.ok` as success and reloads the page, so a 200 here means the issue + # is silently discarded with the user believing it was logged — the exact + # failure rule 60 describes. Returning 400 routes it to the JS error branch + # so the reason is shown and the form stays open with its input intact. + if request.method == 'POST': + if form.assigned_to.errors: + # Most likely an assignee outside this inspection's contract: + # either a stale page rendered before the assignment changed, or a + # crafted id. Say something actionable rather than "invalid choice". + flash('That person cannot be assigned to an issue on this contract. ' + 'Reopen the panel to refresh the list.', 'danger') + return render_template('inspections/flag_issue.html', + form=form, inspection=inspection), 400 + return render_template('inspections/flag_issue.html', form=form, inspection=inspection)