Aug 17 - Fix customer inspector out-of-scope issue assign
This commit is contained in:
@@ -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 `<input type="file">` change. XHR to `POST /<id>/upload-photo`. On success, the server path is written to `<input type="hidden" id="field_<fid>_server_path">` and a `<img id="thumb_<fid>">` 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. `<form id="issuesBulkForm">` 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/<id>/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. |
|
||||
|
||||
|
||||
+113
-9
@@ -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('/<int:inspection_id>/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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user