diff --git a/app/routes/inspections.py b/app/routes/inspections.py index 5750378..6af958a 100644 --- a/app/routes/inspections.py +++ b/app/routes/inspections.py @@ -489,10 +489,30 @@ def templates_for_project(project_id): """Forms usable on this contract — shared ones plus any attached to it. Powers the Contract -> Form cascade on the start-inspection page, the same - way facilities_for_project powers Contract -> Facility. Read-only, and the - real gate is still the POST validation in start(); this only keeps the - picker honest as the contract changes. + way facilities_for_project powers Contract -> Facility. + + **Scoped to the caller's own contracts.** The POST validation in start() is + what stops a form being *used* across contracts, but this endpoint would + otherwise happily list one customer's bespoke form NAMES to another + customer's inspector who simply asked for a contract id — the same leak + that rule 96 covers on the mobile API. Empty list rather than 403, so it + does not confirm whether the contract exists either. """ + if current_user.is_inspector: + fids = get_inspector_scope(current_user) or [] + allowed = { + f.project_id + for f in Facility.query.filter(Facility.id.in_(fids)).all() + } if fids else set() + if project_id not in allowed: + logger_msg = ('TEMPLATES_FOR_PROJECT | out-of-scope request | ' + 'user=%s | project_id=%s') + current_app.logger.warning(logger_msg, current_user.username, project_id) + return jsonify([]) + elif current_user.role == 'customer': + # Customers never start inspections; nothing here is theirs to see. + return jsonify([]) + templates = InspectionTemplate.available_query(project_id).all() return jsonify([ {'id': t.id, 'name': t.name, 'shared': t.is_shared} @@ -1480,15 +1500,25 @@ def bulk_action(): if action == 'delete': from app.utils import storage photo_paths = [] + # Snapshot (id, label) BEFORE deleting: the objects are expired after + # the commit, and the audit pass must run after it. log_action() + # commits internally (rule 41), so auditing inside this loop would + # commit the deletes one at a time — and a mid-loop failure would + # leave rows gone with the photo cleanup below never reached. + deleted = [] for insp in inspections: photo_paths.extend(_collect_inspection_photos(insp)) - log_action(ACTION_DELETE, 'Inspection', insp.id, - f'{insp.template.name if insp.template else "—"} @ ' - f'{insp.facility.name if insp.facility else "—"}', - f'bulk deleted by {current_user.username}') + deleted.append(( + insp.id, + f'{insp.template.name if insp.template else "—"} @ ' + f'{insp.facility.name if insp.facility else "—"}', + )) db.session.delete(insp) changed += 1 db.session.commit() + for insp_id, label in deleted: + log_action(ACTION_DELETE, 'Inspection', insp_id, label, + f'bulk deleted by {current_user.username}') # Files only after the rows are gone — an orphaned file is recoverable, # a deleted file belonging to a surviving row is not. for rel_path in photo_paths: @@ -1498,6 +1528,11 @@ def bulk_action(): # ── Request follow-up ──────────────────────────────────────────────── elif action == 'flag_followup': note = request.form.get('follow_up_note', '').strip() or None + # Only the rows this run actually flagged. Re-deriving it afterwards + # from `follow_up_requested_by == current_user.id` would also match + # inspections this same user flagged on an EARLIER run and that were + # skipped here as already-flagged — re-notifying their inspectors. + flagged = [] for insp in inspections: # Same two guards as the single-inspection route: nothing to follow # up on before submission, and a repeat request must not overwrite @@ -1509,12 +1544,11 @@ def bulk_action(): insp.follow_up_note = note insp.follow_up_requested_by = current_user.id insp.follow_up_requested_at = now_eastern() + flagged.append(insp) changed += 1 db.session.commit() - for insp in inspections: - if insp.follow_up_requested_by != current_user.id or not insp.follow_up_required: - continue + for insp in flagged: body = (f'{current_user.display_name} has requested a follow-up ' f're-inspection of "{insp.template.name if insp.template else "—"}" ' f'at {insp.facility.name if insp.facility else "—"}.' @@ -1543,15 +1577,18 @@ def bulk_action(): exclude_user_ids = {current_user.id, inspector.id if inspector else None} - {None}, ) + db.session.commit() # notify() does not commit — rule 70 + # Audited after the commit (rule 41) — log_action commits internally. + for insp in flagged: log_action(ACTION_UPDATE, 'Inspection', insp.id, f'{insp.template.name if insp.template else "—"}', f'bulk follow_up_required=True by {current_user.username}') - db.session.commit() # notify() does not commit — rule 70 _flash_bulk(changed, skipped, 'flagged for follow-up', skip_reason='not submitted, or already flagged') # ── Clear follow-up ────────────────────────────────────────────────── elif action == 'clear_followup': + cleared = [] for insp in inspections: if not insp.follow_up_required: skipped += 1 @@ -1560,11 +1597,13 @@ def bulk_action(): insp.follow_up_note = None insp.follow_up_requested_by = None insp.follow_up_requested_at = None + cleared.append(insp) changed += 1 + db.session.commit() + for insp in cleared: # after the commit — rule 41 log_action(ACTION_UPDATE, 'Inspection', insp.id, f'{insp.template.name if insp.template else "—"}', f'bulk follow_up cleared by {current_user.username}') - db.session.commit() _flash_bulk(changed, skipped, 'cleared of the follow-up flag', skip_reason='not flagged') diff --git a/app/routes/issues.py b/app/routes/issues.py index 374c897..894c562 100644 --- a/app/routes/issues.py +++ b/app/routes/issues.py @@ -1033,16 +1033,23 @@ def bulk_action(): flash('That user no longer exists.', 'danger') return redirect(back) + # Track what actually moved. Re-deriving this after the commit by + # testing `issue.assigned_to == user.id` would also match the issues + # that were ALREADY assigned to that person — they were counted as + # skipped, but would still be emailed "assigned to you" every time + # anyone ran a bulk assign over them. + newly_assigned = [] for issue in issues: if issue.assigned_to == (user.id if user else None): skipped += 1 continue issue.assigned_to = user.id if user else None + newly_assigned.append(issue) changed += 1 db.session.commit() - for issue in issues: - if issue.assigned_to == (user.id if user else None) and user: + if user: + for issue in newly_assigned: notify( recipient = user, title = f'Issue #{issue.id} assigned to you', @@ -1054,7 +1061,7 @@ def bulk_action(): event_type = EVENT_ISSUE_ASSIGNED, send_email = True, ) - db.session.commit() # notify() does not commit — rule 70 + db.session.commit() # notify() does not commit — rule 70 label = user.display_name if user else 'Unassigned' log_action(ACTION_UPDATE, 'Issue', None, f'bulk assign → {label}', @@ -1068,11 +1075,17 @@ def bulk_action(): flash('Please choose a status to set.', 'warning') return redirect(back) + # (issue, old_status) for the audit pass, which must run AFTER the + # commit — log_action() commits internally (rule 41), so calling it + # inside this loop would commit each row separately and lose the + # batch's atomicity. + moved = [] for issue in issues: if issue.status == new_status: skipped += 1 continue old = issue.status + moved.append((issue, old)) issue.status = new_status # Keep resolved_at consistent with the status, the same way the # single-issue update does — a resolved issue with no resolved_at @@ -1082,14 +1095,16 @@ def bulk_action(): elif new_status in ('open', 'in_progress'): issue.resolved_at = None changed += 1 + db.session.commit() + for issue, old in moved: log_action(ACTION_UPDATE, 'Issue', issue.id, f'#{issue.id}', f'bulk status {old} → {new_status} by {current_user.username}') - db.session.commit() _flash_bulk(changed, skipped, f'set to {new_status.replace("_", " ").title()}') # ── Verify & close ─────────────────────────────────────────────────── elif action == 'verify': + verified = [] for issue in issues: if issue.status not in ('resolved', 'pending_verification'): skipped += 1 @@ -1099,10 +1114,12 @@ def bulk_action(): issue.verified_at = now_eastern() if not issue.resolved_at: issue.resolved_at = now_eastern() + verified.append(issue) changed += 1 + db.session.commit() + for issue in verified: # after the commit — rule 41 log_action(ACTION_UPDATE, 'Issue', issue.id, f'#{issue.id}', f'bulk_verified_by={current_user.username}') - db.session.commit() _flash_bulk(changed, skipped, 'verified and closed', skip_reason='not awaiting verification') @@ -1110,17 +1127,25 @@ def bulk_action(): elif action == 'delete': from app.utils import storage photo_paths = [] + # Snapshot the ids BEFORE deleting — the objects are expired after the + # commit, and the audit pass has to run after it (rule 41: log_action + # commits internally, so auditing inside this loop would commit the + # deletes one at a time and, on a mid-loop failure, leave rows gone + # with the photo cleanup below never reached). + deleted_ids = [] for issue in issues: if issue.photo_path: photo_paths.append(issue.photo_path) for lst in (issue.mobile_photo_paths, issue.result_photos): if lst: photo_paths.extend(lst) - log_action(ACTION_DELETE, 'Issue', issue.id, f'#{issue.id}', - f'bulk deleted by {current_user.username}') + deleted_ids.append(issue.id) db.session.delete(issue) changed += 1 db.session.commit() + for issue_id in deleted_ids: + log_action(ACTION_DELETE, 'Issue', issue_id, f'#{issue_id}', + f'bulk deleted by {current_user.username}') # Files go only after the rows are safely gone — a failure here leaves # an orphaned file, which is recoverable; the reverse is not. for rel_path in photo_paths: