diff --git a/.gitignore b/.gitignore index 444232f6..5d34fcfd 100644 --- a/.gitignore +++ b/.gitignore @@ -248,3 +248,4 @@ frontend-typescript/tsconfig.tsbuildinfo uploads/ *.db .codex/ +frontend-typescript/tsconfig.tsbuildinfo diff --git a/openspec/changes/budget-excel-export/.openspec.yaml b/openspec/changes/budget-excel-export/.openspec.yaml new file mode 100644 index 00000000..374c0fbf --- /dev/null +++ b/openspec/changes/budget-excel-export/.openspec.yaml @@ -0,0 +1,4 @@ +schema: spec-driven +created: 2026-09-20 +author: Norair Arutshyan +priority: medium diff --git a/openspec/changes/budget-excel-export/design.md b/openspec/changes/budget-excel-export/design.md new file mode 100644 index 00000000..75267a7c --- /dev/null +++ b/openspec/changes/budget-excel-export/design.md @@ -0,0 +1,42 @@ +# Design + +## Context + +See proposal.md - Why. Relevant existing data model (all in `services/budget/app/models/`): +- `BudgetModel`/`BudgetLineModel`/`BudgetCategoryModel` (`budget.py`): budget lines store `amount` in the budget's `local_currency`; `actual_currency` is the donor's currency; `estimated_exchange_rate` (local ÷ donor, e.g. AMD per EUR) is a planning-time estimate, never a stored per-line figure — existing code (donor dashboard, budget-line currency toggle) already treats it as a derived, render-time-only conversion, never persisted as a second amount. +- `FundingReceiptModel`/`CurrencyConversionModel`/`ReportLineConversionAllocationModel` (`currency_ledger.py`): a receipt (donor currency landed) and a conversion (one real bank FX event, rate = `local_amount ÷ donor_amount`) are **not linked 1:1** — only aggregate-balanced. A report-line expense (`ReportLineModel.amount`, in `local_currency`) is allocated FIFO across unconsumed conversion lots (`currency_ledger_services.allocate_fifo_service`), producing zero or more allocation rows per expense. +- No existing per-budget-line planned-vs-actual rollup query exists; `dashboard_crud.budget_breakdown` is the closest precedent but aggregates cross-budget, not per-line. +- `openpyxl==3.1.5` is already a dependency (currently import-only, via `excel_import_service.py`); no new package needed for writing. +- `attachment_routes.py` already establishes the `StreamingResponse` file-download pattern for this service. + +## Goals / Non-Goals + +**Goals:** +- Generate one workbook per request, entirely from live data, with no new tables or stored artifacts. +- Reuse the currency-ledger's real allocation data wherever it exists; only fall back to the planning-time estimate for the genuinely unresolved gap, and mark that gap visibly. + +**Non-Goals:** +- Donor-template-shaped round-trip export (deferred; see proposal). +- Caching, background generation, or emailing the file — synchronous request/response only, matching the size of a single budget's data. +- Any change to how receipts, conversions, or allocations are recorded (`ledger-budget`'s in-flight edit/delete/reset work is unrelated and unaffected). + +## Decisions + +**1. New per-budget-line rollup query, not a reuse of `dashboard_crud.budget_breakdown`.** +That function sums conversions/expenses per *budget*, across all budgets a customer owns — the export needs the same shape of number (converted, spent) but grouped by *budget_line_id* within one budget, plus the allocation-level rate detail `budget_breakdown` never needed. New function(s) live in a new `services/budget/app/crud/excel_export_crud.py`, following the same subquery pattern (`group_by` + `outerjoin`) rather than N+1 per-line queries. + +**2. Converted-expense figure blends real allocation rates with `estimated_exchange_rate` for the unsatisfied remainder, computed per report line then summed per budget line.** +For each report line: sum `allocation.amount_allocated × (conversion.donor_amount ÷ conversion.local_amount)` across its allocations (real rate, since `amount_allocated` is in `local_currency`), then add `(report_line.amount − Σallocation.amount_allocated) ÷ estimated_exchange_rate` for any remainder. This mirrors the existing precedent of using `estimated_exchange_rate` as an approximate stand-in (donor dashboard, budget-line toggle) rather than inventing a new conversion rule. A budget line is flagged "includes estimate" in the export if any of its report lines have a non-zero unsatisfied remainder. +*Alternative considered*: omit the unsatisfied remainder entirely (leave it unconverted/blank). Rejected per explicit product decision — donors expect the dashboard total to foot to something close to the full spend, and GrandFlow already accepts approximate figures elsewhere rather than showing gaps. + +**3. Workbook generated synchronously in the request/response cycle via `openpyxl.Workbook()`, streamed with `StreamingResponse` (mirroring `attachment_routes.py`), not written to storage first.** +A single budget's data (lines, ledger, report lines) is small; no need for the async/background pattern used elsewhere (e.g. Celery) for larger jobs. + +**4. Sheet 2's estimated-portion flag is a cell style (e.g. italic + light fill + a legend row), not a separate column.** +Keeps the sheet's column count matching the user's I/J/K scope instead of doubling columns for a rare partial-estimate case. + +## Risks / Trade-offs + +- [A budget with many report lines/allocations could make generation slow] → Out of scope for a "simple export" of one budget; single-budget expense volume in practice is small (tens to low hundreds of lines), revisit only if real usage shows otherwise. +- [`estimated_exchange_rate` unset on an older or draft budget leaves Sheet 1's donor-currency column and Sheet 2's deviation column blank for that budget] → Matches existing GrandFlow convention (donor dashboard already excludes rather than fabricates); the export's local-currency figures are still fully populated. +- [Category subtotal rows in Sheet 2 need the same rollup as Sheet 1's category grouping] → Reuse the same category-grouping logic/order between Sheet 1 and Sheet 2 rather than deriving it twice, to avoid the two sheets silently disagreeing on category order or membership. diff --git a/openspec/changes/budget-excel-export/proposal.md b/openspec/changes/budget-excel-export/proposal.md new file mode 100644 index 00000000..65ed7539 --- /dev/null +++ b/openspec/changes/budget-excel-export/proposal.md @@ -0,0 +1,32 @@ +# Proposal + +## Why + +A grantee owner has no way to hand a single budget's numbers to a donor, auditor, or board as a spreadsheet — every figure (budget lines, real-currency ledger, expenses) currently only exists inside the app. Donors overwhelmingly expect a multi-sheet Excel report (per the existing `KTK 2012.xls` example this proposal is grounded in), and GrandFlow already has all the underlying data (`budget_lines`, `funding_receipts`, `currency_conversions`, `report_lines`, and their FIFO allocations) — it just isn't exportable yet. + +## What Changes + +- Add `GET /budgets/{budget_id}/export.xlsx` (budget service): generates and streams a 3-sheet workbook for one budget, on demand (not stored), using `openpyxl` (already a dependency). +- **Sheet 1 — Original Budget**: budget lines grouped by category with subtotals, mirroring the example's layout. Each line shows its amount in the budget's `local_currency` plus a derived donor-currency estimate column (`amount ÷ estimated_exchange_rate`), consistent with how GrandFlow already treats `estimated_exchange_rate` elsewhere as an approximate, non-stored conversion (donor dashboard, budget-line toggle). +- **Sheet 2 — Budget vs. Report Dashboard**: per budget-line (and category subtotal) rows with only 3 result columns (matching the example's `I`/`J`/`K`; its interim/final-report date-range split in `E–H` is out of scope): Total Expenses (`local_currency`, direct sum of `report_lines.amount`), Total Expenses Converted (donor currency — real ledger rate for the portion covered by `ReportLineConversionAllocation`, falling back to `estimated_exchange_rate` for any unsatisfied remainder, with that estimated portion visually flagged so it's never mistaken for a real bank rate), and Deviation (budgeted-in-donor-currency minus converted actual). An income section lists every recorded `CurrencyConversion` as its own row (date, donor amount, local amount, implied rate) rather than one row per receipt, since one receipt can convert across several dates/rates. +- **Sheet 3 — List of Expenses**: one row per report-line expense; an expense whose payment was funded by more than one currency-conversion lot gets one row per allocation (subline), each carrying its own conversion date, rate, and converted amount, directly mirroring the real `ReportLineConversionAllocation` data with no aggregation. +- Frontend: an "Export to Excel" button on the single-budget view that downloads the generated file. +- Wire the new route into all three gateway configs (`nginx-dev.conf`, `nginx.conf`, `Caddyfile`). + +**Explicitly out of scope**: regenerating a budget back into a *donor's own* Excel layout (the round-trip feature already deferred by the archived `budget-export-from-excel` — actually an import feature — proposal). This is a new, fixed GrandFlow-authored export format, not a donor-template replay. + +## Capabilities + +### New Capabilities +- `budget-excel-export`: backend generation of the 3-sheet workbook from a budget's lines, ledger, and report-line data. +- `budget-excel-export-ui`: the frontend entry point (button + download) that triggers the export on a single-budget view. + +### Modified Capabilities +(none — this only reads existing `budget-currency-ledger`, `budget-reports`, and `budget-categories` data; no requirement in those specs changes) + +## Impact + +- **Backend**: new `services/budget/app/services/excel_export_service.py` (workbook generation), new per-budget-line rollup query (join `budget_lines`→`report_lines`, no existing precedent — closest is `dashboard_crud.budget_breakdown`, which is cross-budget, not per-line), new route in `budget_routes.py` (`GET /{budget_id}/export.xlsx`), reusing the `StreamingResponse` pattern from `attachment_routes.py`. +- **Frontend**: one button + fetch/download handler on the budget detail view; no new page. +- **Gateway**: route addition to `nginx-dev.conf`, `nginx.conf`, `Caddyfile`. +- **No schema/migration changes** — pure read of existing tables. diff --git a/openspec/changes/budget-excel-export/specs/budget-excel-export-ui/spec.md b/openspec/changes/budget-excel-export/specs/budget-excel-export-ui/spec.md new file mode 100644 index 00000000..dccfa0c0 --- /dev/null +++ b/openspec/changes/budget-excel-export/specs/budget-excel-export-ui/spec.md @@ -0,0 +1,25 @@ +# Spec Delta + +## Purpose + +Gives a budget owner or funder viewer a visible way to trigger and download the single-budget Excel export from the budget detail view. + +## ADDED Requirements + +### Requirement: Export button on budget detail view +The frontend SHALL show an "Export to Excel" action on the single-budget detail view, visible to anyone with read access to that budget (owner or funder), that downloads the generated workbook via `GET /budgets/{budget_id}/export.xlsx`. + +#### Scenario: Owner triggers export +- **WHEN** the budget owner clicks "Export to Excel" on their budget's detail view +- **THEN** the frontend requests the export endpoint and saves the returned file with a filename derived from the budget's name + +#### Scenario: Export unavailable to unauthorized viewers +- **WHEN** a user without read access to the budget views a page that would otherwise show the button +- **THEN** the frontend does not show the "Export to Excel" action + +### Requirement: Export failure feedback +The frontend SHALL show an inline error, without navigating away from the budget detail view, when the export request fails. + +#### Scenario: Backend error surfaced +- **WHEN** the export endpoint returns an error +- **THEN** the frontend shows an inline error message and the user remains on the budget detail view diff --git a/openspec/changes/budget-excel-export/specs/budget-excel-export/spec.md b/openspec/changes/budget-excel-export/specs/budget-excel-export/spec.md new file mode 100644 index 00000000..0a30064f --- /dev/null +++ b/openspec/changes/budget-excel-export/specs/budget-excel-export/spec.md @@ -0,0 +1,70 @@ +# Spec Delta + +## Purpose + +Lets a budget owner (or funder viewer) generate a 3-sheet Excel workbook of a single budget's plan, real-currency ledger, and expense history, for sharing outside the app. + +## ADDED Requirements + +### Requirement: Single-budget Excel export endpoint +The system SHALL provide an endpoint that generates and streams a `.xlsx` workbook for exactly one budget, computed on demand from current data (not persisted or cached), accessible to anyone permitted to view that budget (owner or funder, matching existing budget-read authorization). + +#### Scenario: Owner exports a budget +- **WHEN** the budget's owner requests the export for a budget they own +- **THEN** the system returns a `.xlsx` file streamed in the response, reflecting the budget's current lines, ledger, and report data + +#### Scenario: Funder views a funded budget's export +- **WHEN** a funder who funds the budget (`funding_customer_id`) requests its export +- **THEN** the system returns the same workbook a viewer with read access would see + +#### Scenario: Unauthorized user is rejected +- **WHEN** a user who is neither the budget's owner nor its funder requests the export +- **THEN** the system rejects the request without generating a workbook + +### Requirement: Sheet 1 — Original Budget +The workbook's first sheet SHALL list every budget line grouped by category, with a subtotal row per category and a grand-total row, each line showing its amount in the budget's `local_currency` and a derived estimate in the budget's `actual_currency` (`amount ÷ estimated_exchange_rate`) when `estimated_exchange_rate` is set. + +#### Scenario: Categories and subtotals shown +- **WHEN** the budget has lines across more than one category +- **THEN** Sheet 1 groups lines under their category, with a subtotal per category and a grand total for the whole budget + +#### Scenario: No donor-currency estimate available +- **WHEN** the budget has no `estimated_exchange_rate` set +- **THEN** Sheet 1 shows the local-currency amount only, leaving the donor-currency estimate column blank for that budget rather than showing a fabricated figure + +### Requirement: Sheet 2 — Budget vs. Report Dashboard, income section +The workbook's second sheet SHALL open with an income section listing every recorded currency conversion for the budget as its own row (converted date, donor-currency amount, local-currency amount, implied rate), plus a total row, rather than one row per funding receipt. + +#### Scenario: Multiple conversions from one receipt +- **WHEN** a budget has one funding receipt but several currency conversions recorded against it +- **THEN** the income section lists each conversion as a separate row with its own date and implied rate, not one blended row + +### Requirement: Sheet 2 — Budget vs. Report Dashboard, expense columns +For each budget line (and category subtotal), Sheet 2 SHALL show: total expenses in `local_currency` (sum of that line's report-line amounts); total expenses converted to `actual_currency`, using the real per-allocation conversion rate for the portion covered by a `CurrencyConversion` and the budget's `estimated_exchange_rate` for any unsatisfied remainder; and a deviation column (budgeted amount converted to `actual_currency` via `estimated_exchange_rate`, minus the converted total expenses). The workbook SHALL visually distinguish a converted-expense figure that includes an estimated (not real-rate) portion from one derived entirely from real conversions. + +#### Scenario: Expense fully covered by real conversions +- **WHEN** a budget line's report-line expenses are fully covered by allocated currency conversions +- **THEN** its converted-expense figure uses only real per-allocation rates, with no estimated-rate flag + +#### Scenario: Expense partially unconverted +- **WHEN** a budget line has report-line expenses whose allocation to currency-conversion lots is incomplete +- **THEN** its converted-expense figure combines the real-rate portion with the `estimated_exchange_rate`-derived portion for the remainder, and is flagged as including an estimate + +#### Scenario: No report-line expenses yet +- **WHEN** a budget line has no report-line expenses recorded +- **THEN** its expense and converted-expense figures show zero, and its deviation equals its full budgeted amount + +### Requirement: Sheet 3 — List of Expenses with per-allocation sublines +The workbook's third sheet SHALL list every report-line expense across all of the budget's reports, one row per report line when its full amount is covered by a single currency-conversion lot, or one row per allocation (subline) when a report line's amount is funded by more than one lot — each subline row carrying that allocation's own conversion date, converted amount, and implied rate. + +#### Scenario: Expense funded by a single lot +- **WHEN** a report-line expense is fully allocated to exactly one currency-conversion lot +- **THEN** Sheet 3 shows it as a single row with that lot's date and rate + +#### Scenario: Expense funded by multiple lots +- **WHEN** a report-line expense straddles more than one currency-conversion lot (e.g., one receipt converted across four separate events) +- **THEN** Sheet 3 shows one row per allocation, each with its own conversion date and implied rate, and the rows' amounts sum to the expense's full amount + +#### Scenario: Expense not yet allocated +- **WHEN** a report-line expense has no currency-conversion allocation at all +- **THEN** Sheet 3 shows it as a single row with its local-currency amount and no conversion date or rate diff --git a/openspec/changes/budget-excel-export/tasks.md b/openspec/changes/budget-excel-export/tasks.md new file mode 100644 index 00000000..98f7f494 --- /dev/null +++ b/openspec/changes/budget-excel-export/tasks.md @@ -0,0 +1,33 @@ +# Tasks + +Workflow rule: one task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. + +## 1. Export endpoint scaffolding + Sheet 1 (Original Budget) + +- [ ] 1.0 Run `scripts/start-group.sh budget-excel-export 1` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 1.1 Add `services/budget/app/services/excel_export_service.py` with a `generate_budget_export_workbook(budget, categories, lines, ...)` entry point that creates an `openpyxl.Workbook()` and returns bytes; verify with a unit test that it returns a valid `.xlsx` (round-trips through `openpyxl.load_workbook`) +- [ ] 1.2 Implement Sheet 1 generation: budget lines grouped by category (reuse `BudgetCategoryModel` ordering), subtotal row per category, grand-total row, local-currency amount column, and a donor-currency estimate column (`amount ÷ estimated_exchange_rate`) left blank when `estimated_exchange_rate` is unset; verify with a unit test asserting cell values for a budget with 2 categories and a budget with no `estimated_exchange_rate` +- [ ] 1.3 Add `GET /budgets/{budget_id}/export.xlsx` to `services/budget/app/api/budget_routes.py`, authorized the same way as `GET /budgets/{budget_id}` (owner or funder), returning a `StreamingResponse` (mirroring `attachment_routes.py`'s pattern) with the correct `Content-Type`/`Content-Disposition`; verify with an integration test that owner and funder both get 200 and a non-owner/non-funder gets rejected +- [ ] 1.4 Wire the new route into `nginx-dev.conf`, `nginx.conf`, and `Caddyfile`; verify by confirming the route pattern matches the existing `/budgets/{budget_id}/...` entries in all three files +- [ ] 1.5 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) + +## 2. Sheet 2 — Budget vs. Report Dashboard — depends on 1 + +- [ ] 2.1 Add `services/budget/app/crud/excel_export_crud.py` with a per-budget-line rollup query (join `budget_lines` → `report_lines`, grouped by `budget_line_id`) returning each line's total local-currency expenses and its allocations' `(amount_allocated, conversion.donor_amount, conversion.local_amount)` tuples; verify with a unit test against a seeded budget with lines spanning fully-allocated, partially-allocated, and zero-expense cases +- [ ] 2.2 Implement the converted-expense calculation in `excel_export_service.py`: real per-allocation rate for allocated amounts plus `estimated_exchange_rate` for any unsatisfied remainder, flagging a line as "includes estimate" when a remainder exists; verify with a unit test covering fully-allocated (no flag), partially-allocated (flagged, blended figure), and fully-unallocated (fully estimated, flagged) cases +- [ ] 2.3 Implement Sheet 2's income section: one row per `CurrencyConversion` for the budget (converted date, donor amount, local amount, implied rate) plus a total row; verify with a unit test using a budget with one funding receipt and multiple conversions, asserting one row per conversion +- [ ] 2.4 Implement Sheet 2's per-budget-line and category-subtotal rows (expenses local, expenses converted, deviation), reusing Sheet 1's category grouping/order and applying the estimated-portion cell style (italic + fill, per design.md Decision 4) where flagged; verify with a unit test asserting column values and that flagged cells carry the style +- [ ] 2.5 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) + +## 3. Sheet 3 — List of Expenses with per-allocation sublines — depends on 1 + +- [ ] 3.1 Extend `excel_export_crud.py` with a query returning every report line across all of the budget's reports, each with its ordered list of allocations (or none) +- [ ] 3.2 Implement Sheet 3 generation: one row per report line when zero or one allocation exists, one row per allocation (subline) when a report line has more than one, each subline carrying its own conversion date/rate/converted amount; verify with a unit test covering an unallocated expense, a single-lot expense, and a multi-lot expense (asserting the multi-lot rows sum to the expense's full amount) +- [ ] 3.3 Run backend lint/tests clean for `services/budget`; PR merged (`Closes` this group's sub-issue) + +## 4. Frontend export button — depends on 1, 2, 3 + +- [ ] 4.1 Add an "Export to Excel" button to the single-budget detail view in `frontend-typescript`, visible whenever the viewer has read access to the budget (owner or funder), following existing button/permission conventions on that view +- [ ] 4.2 Wire the button to `GET /budgets/{budget_id}/export.xlsx` and trigger a browser download of the response with a filename derived from the budget's name; verify manually that a downloaded file opens in Excel/LibreOffice with all 3 sheets populated +- [ ] 4.3 Add inline error handling that shows a message without navigating away when the request fails; verify with a frontend test that simulates a failed request and asserts the user stays on the budget detail view with an error shown +- [ ] 4.4 Run frontend lint/tests clean; manually verify end-to-end against a real budget with lines, receipts, multi-lot conversions, and report expenses (owner and funder logins); PR merged (`Closes` this group's sub-issue) diff --git a/openspec/changes/ci-compliance-guardrails/.openspec.yaml b/openspec/changes/ci-compliance-guardrails/.openspec.yaml new file mode 100644 index 00000000..a7e1fdee --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/.openspec.yaml @@ -0,0 +1,4 @@ +schema: spec-driven +created: 2026-09-21 +author: Norair Arutshyan +priority: medium diff --git a/openspec/changes/ci-compliance-guardrails/design.md b/openspec/changes/ci-compliance-guardrails/design.md new file mode 100644 index 00000000..ce7ba92a --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/design.md @@ -0,0 +1,51 @@ +# Design + +## Context + +Six independently path-filtered GitHub Actions workflows (`ai`, `budget`, `chat`, `users`, `worker`, `frontend`, plus `shared` and `e2e`) each run `black`/`mypy`/`flake8` then `pytest` for Python services (see `.github/workflows/budget.yml` for the reference shape). No pre-commit hooks and no security tooling exist today. Root `pyproject.toml` only configures `black`. See proposal.md - Why for the motivation. + +## Goals / Non-Goals + +**Goals:** +- Every new scan is enforced by CI, not by an agent or developer remembering a rule. +- Advisory rollout is observable: findings are visible per-PR, and there's a documented, low-ceremony way to tell when a check is ready to become required. +- The eventual required-check list composes cleanly with checks landing from other changes (`gdpr-iso27001-priority-2`'s Dependabot scan, `audit-mixin-coverage-guard`'s guard tests) without this change needing to touch those changes' code. + +**Non-Goals:** +- Not building the Dependabot config or the AuditMixin guard tests themselves — those belong to their own changes; this change only accounts for them in the required-check inventory once they exist. +- Not covering the frontend's TypeScript/JS-specific SAST needs beyond secret scanning — `npm audit` is already scoped separately in `gdpr-iso27001-priority-2`. +- Not attempting perfect precision on the custom Semgrep tenant-scoping rule in this change — see Risks below. + +## Decisions + +**gitleaks over trufflehog for secret scanning.** Both are viable; gitleaks has a simpler single-binary GitHub Action, a pre-commit hook maintained upstream, and a smaller default ruleset that's easier to tune for this repo's `.env.*-secrets` file patterns. TruffleHog's verification-against-live-API feature is more useful at much larger scale than this repo needs. + +**Bandit for Python SAST, not a broader multi-language SAST suite.** All backend services are Python/FastAPI; Bandit integrates directly into the existing per-service lint step with no new infra. A heavier tool (e.g. Snyk Code) is deferred — it would duplicate Semgrep's role here and adds a vendor dependency this change doesn't need. + +**Semgrep for the custom GDPR rules, not hand-written pytest guard tests.** The AuditMixin coverage guard (a separate change) uses pytest because it needs SQLAlchemy model-registry introspection. The PII-logging, raw-SQL, and tenant-scoping checks are pattern-matches over source text/AST across arbitrary files, which is exactly Semgrep's use case, and its rules are declarative YAML reviewable independent of the codebase. Custom rules live in `.semgrep/rules/*.yml`, one file per rule family, run via `semgrep --config .semgrep/`. + +**Advisory mode implemented as `continue-on-error: true` on each new job, not a separate non-blocking workflow.** Keeping advisory checks in the same workflow file as the eventual required version means promoting a check is a one-line diff (remove the flag), not a workflow restructure. Findings still post as CI annotations and appear in the PR checks list, just don't block merge. + +**Pilot on `budget` service first, then roll out to the rest.** Matches how AuditMixin tiers were rolled out incrementally rather than all-at-once. `budget` is chosen because it has the most active CI usage right now, giving the fastest signal on false-positive rate. + +**Branch protection changes are a manual GitHub settings step, not code.** GitHub branch protection rules aren't stored in-repo (no Terraform/IaC for repo settings exists in this project). Tasks.md will call out the exact settings to change and who needs admin access to change them. + +## Risks / Trade-offs + +- **[Risk]** The tenant/`customer_id`-scoping Semgrep rule is the highest false-positive-risk rule in this set — many legitimate queries (lookups by primary key, superuser/admin paths already reviewed under `customer-impersonation`) don't include tenant scoping and shouldn't. → **Mitigation**: ship this specific rule with a narrower initial pattern (only flag queries against an explicit list of known multi-tenant tables) and treat it as the last rule promoted to required, after the longest advisory period. +- **[Risk]** Secret scanning against full git history (not just the diff) could surface pre-existing leaked secrets that need rotation, not just a CI failure to fix. → **Mitigation**: scope the initial gitleaks CI job to the PR diff only; run a one-time full-history scan separately (task in tasks.md) so any historical findings are triaged deliberately, not discovered mid-PR. +- **[Risk]** Advisory-mode checks that nobody looks at provide no real signal for promotion. → **Mitigation**: tasks.md includes a step to track findings per check (e.g. a running count in the PR template or a weekly export) so the promotion criteria in the `branch-protection-enforcement` spec has real data behind it, not a guess. +- **[Trade-off]** Running Bandit/Semgrep/gitleaks adds CI time to every PR. → Accepted: these are fast static tools (seconds, not minutes) relative to the existing test suite; not mitigated further in this change. + +## Migration Plan + +1. Add gitleaks pre-commit hook + CI job (diff-scoped) to `budget` service only; advisory. +2. Add Bandit to `budget`'s lint step; advisory. +3. Add Semgrep with the PII-logging and raw-SQL rules to `budget`; advisory. Tenant-scoping rule follows once the other two show acceptable false-positive rates. +4. Roll the same three additions out to `chat`, `ai`, `users`, `worker`, `shared`, `frontend`, `e2e` workflows. +5. Run the one-time full-history gitleaks scan; triage any findings (rotate/redact) before treating secret scanning as trustworthy. +6. Document promotion criteria and the required-check inventory in `docs/security/ci-compliance-guardrails.md`. +7. Once each check's advisory period and false-positive threshold are met, remove `continue-on-error` and add it to `main`'s required status checks in GitHub repo settings. +8. Repeat step 7 as `gdpr-iso27001-priority-2` (Dependabot) and `audit-mixin-coverage-guard` land, updating the same doc. + +Rollback: each check is independently removable by deleting its CI step (advisory) or unchecking it in branch protection (required) — no data migration or schema involved, so rollback is low-risk at any stage. diff --git a/openspec/changes/ci-compliance-guardrails/proposal.md b/openspec/changes/ci-compliance-guardrails/proposal.md new file mode 100644 index 00000000..e85390ff --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/proposal.md @@ -0,0 +1,33 @@ +# Proposal + +## Why + +Compliance-relevant code quality (secret handling, injection risk, PII in logs, missing tenant scoping) today depends entirely on the author — human or AI agent — remembering to apply it; nothing in CI catches a violation, and prompt-based guidance (e.g. CLAUDE.md rules) is not a control an auditor can point to. This change adds CI-enforced scanning that fires regardless of who or what wrote the code, is not bypassable by omission, and gives ISO 27001/GDPR posture something durable to stand on. + +## What Changes + +- Add gitleaks secret scanning: a new repo-root `.pre-commit-config.yaml` for local feedback, plus a CI job across all 6 service workflows and the frontend workflow (pre-commit alone is bypassable with `--no-verify`; CI is the real gate). +- Add Bandit to each Python service's existing lint step (alongside `black`/`mypy`/`flake8`) for SAST coverage of common Python security bugs. +- Add Semgrep with a custom ruleset for GDPR-relevant anti-patterns not caught by generic SAST: PII-shaped values (email, token, password, ssn) passed to logger calls, raw SQL string interpolation, and ORM queries missing tenant/`customer_id` scoping. +- Run all new checks in advisory mode first (report findings as CI annotations, do not fail the build); document explicit, measurable promotion criteria (e.g. a bounded false-positive rate over N PRs) for flipping each check to required. +- Enable GitHub branch protection on `main` requiring the promoted checks to pass before merge. This is the layer that makes every check — the new ones here, plus the Dependabot scan from `gdpr-iso27001-priority-2` and the AuditMixin coverage guard from `audit-mixin-coverage-guard` once those land — actually non-bypassable rather than advisory. +- Document the required-check list and promotion/waiver process in `docs/`. + +## Capabilities + +### New Capabilities +- `ci-security-scanning`: secret scanning (gitleaks) and SAST (Bandit, Semgrep custom rules) running per-service in CI, advisory-first with a defined path to required. +- `branch-protection-enforcement`: required GitHub status checks on `main`, the criteria for promoting an advisory check to required, and the waiver process for an exception. + +### Modified Capabilities +(none — additive only, no existing spec's requirements change) + +## Impact + +- `.github/workflows/{budget,chat,ai,users,worker,frontend,e2e}.yml` and `shared.yml`: new scan steps. +- New `.pre-commit-config.yaml` at repo root (none exists today). +- New `.semgrep/` custom rules directory. +- New `docs/security/ci-compliance-guardrails.md` documenting the required-check list, promotion criteria, and waiver process. +- GitHub repository settings: branch protection rule on `main` (configuration, not code, but in scope for this change). +- Depends on `gdpr-iso27001-priority-2` and `audit-mixin-coverage-guard` for two of the checks eventually added to the required list; this change's own checks (secrets, SAST) are independent and can land first. + diff --git a/openspec/changes/ci-compliance-guardrails/specs/branch-protection-enforcement/spec.md b/openspec/changes/ci-compliance-guardrails/specs/branch-protection-enforcement/spec.md new file mode 100644 index 00000000..6610f035 --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/specs/branch-protection-enforcement/spec.md @@ -0,0 +1,42 @@ +# Spec Delta + +## Purpose + +Defines how an advisory CI check gets promoted to a required, merge-blocking check, and how GitHub branch protection on `main` enforces that gate so compliance controls cannot be silently bypassed by any author, human or AI. + +## ADDED Requirements + +### Requirement: Documented promotion criteria +The project SHALL maintain a written, measurable criterion for promoting a CI check from advisory to required (e.g. a minimum advisory period and a maximum false-positive rate observed over that period). + +#### Scenario: A check meets the documented criterion +- **WHEN** an advisory check has run for the documented minimum period with a false-positive rate at or below the documented threshold +- **THEN** the check is eligible to be promoted to a required status check + +### Requirement: Required status checks block merge to main +Once a check is promoted, GitHub branch protection on `main` SHALL require it to pass before a pull request can be merged. + +#### Scenario: A required check fails +- **WHEN** a pull request targeting `main` has a failing required status check +- **THEN** the merge button is disabled until the check passes or an approved waiver is recorded + +### Requirement: No silent bypass of required checks +Required checks SHALL NOT be bypassable via force-push or administrator merge override without an explicit, recorded waiver. + +#### Scenario: Merge attempted without a waiver +- **WHEN** a pull request with a failing required check is merged without a recorded waiver +- **THEN** the merge is rejected by branch protection settings + +### Requirement: Documented waiver process +The project SHALL document a waiver process for exempting a specific pull request from a required check, including who may approve the waiver and where it is recorded. + +#### Scenario: A required check produces a confirmed false positive +- **WHEN** a required check fails on a finding confirmed to be a false positive +- **THEN** the documented waiver process is followed and the approval is recorded before the pull request merges + +### Requirement: Required-check inventory stays current +The documented required-check list SHALL be kept current as checks from other changes land — including the dependency-vulnerability scan and the AuditMixin coverage guard tests — not only the checks introduced directly by this change. + +#### Scenario: A dependency from another change lands +- **WHEN** a check originating from a different change (e.g. the Dependabot scan or the AuditMixin coverage guard) is merged and ready for promotion +- **THEN** the required-check documentation and branch protection configuration are updated to include it diff --git a/openspec/changes/ci-compliance-guardrails/specs/ci-security-scanning/spec.md b/openspec/changes/ci-compliance-guardrails/specs/ci-security-scanning/spec.md new file mode 100644 index 00000000..c1a90a35 --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/specs/ci-security-scanning/spec.md @@ -0,0 +1,50 @@ +# Spec Delta + +## Purpose + +Defines the automated secret-scanning and SAST checks that run in CI against every service and the frontend, so security-relevant code properties are checked mechanically rather than relying on the author — human or AI — to remember to apply them. + +## ADDED Requirements + +### Requirement: Secret scanning in CI +Every pull request, regardless of target service, SHALL be scanned by an automated secret-detection tool (e.g. gitleaks) covering the full diff. + +#### Scenario: PR introduces a credential-shaped string +- **WHEN** a pull request adds a string matching a known secret pattern (API key, private key, connection string with embedded credentials, etc.) +- **THEN** the CI secret-scanning job reports the finding, including file and line + +#### Scenario: PR contains no secrets +- **WHEN** a pull request's diff contains no secret-shaped strings +- **THEN** the CI secret-scanning job completes with no findings + +### Requirement: Local secret-scanning feedback before push +The repository SHALL provide a pre-commit hook that runs the same secret-detection tool used in CI, so a contributor sees a finding before pushing. + +#### Scenario: Developer stages a credential-shaped string locally +- **WHEN** a developer runs `git commit` on a change containing a secret-shaped string +- **THEN** the pre-commit hook flags the file and line before the commit completes + +### Requirement: SAST coverage for Python services +Each Python service's CI lint step SHALL include a static-analysis security scan (e.g. Bandit) covering common insecure patterns (insecure deserialization, use of `eval`/`exec` on untrusted input, weak cryptographic primitives, hardcoded bind-all network interfaces). + +#### Scenario: PR introduces an insecure pattern +- **WHEN** a pull request adds code matching a known insecure pattern covered by the SAST tool +- **THEN** the CI job reports the finding with file, line, and rule identifier + +### Requirement: Custom rules for GDPR-relevant anti-patterns +CI SHALL run an additional rule set (e.g. Semgrep) covering patterns not caught by generic SAST: PII-shaped values passed to logging calls, raw SQL built via string interpolation instead of parameterized queries, and ORM queries against multi-tenant tables that omit tenant/`customer_id` scoping. + +#### Scenario: PII-shaped value passed to a logger +- **WHEN** a pull request adds a logging call whose argument is a variable or field named after a known PII category (email, ssn, password, token, etc.) +- **THEN** the CI job reports the finding + +#### Scenario: Query on a multi-tenant table omits tenant scoping +- **WHEN** a pull request adds a database query against a table known to require tenant/`customer_id` scoping, without a filter on that column +- **THEN** the CI job reports the finding + +### Requirement: Advisory-first rollout +Each check introduced by this capability SHALL run in advisory (non-blocking) mode when first introduced: findings are surfaced as CI output but do not fail the build or block merge. + +#### Scenario: A newly introduced check finds an issue +- **WHEN** a check that has not yet been promoted to required finds an issue on a PR +- **THEN** the finding is visible in CI output and the PR remains mergeable on that basis alone diff --git a/openspec/changes/ci-compliance-guardrails/tasks.md b/openspec/changes/ci-compliance-guardrails/tasks.md new file mode 100644 index 00000000..cc844c8a --- /dev/null +++ b/openspec/changes/ci-compliance-guardrails/tasks.md @@ -0,0 +1,55 @@ +# Tasks + +One task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. + +## 1. Secret scanning pilot (gitleaks) on budget + repo-wide pre-commit hook + +- [ ] 1.0 Run `scripts/start-group.sh ci-compliance-guardrails 1` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 1.1 Add repo-root `.pre-commit-config.yaml` with a gitleaks hook; verify `pre-commit run --all-files` flags a locally-added test secret, then remove the test secret. +- [ ] 1.2 Add a gitleaks CI job (diff-scoped, `continue-on-error: true`) to `.github/workflows/budget.yml`; verify it runs on a test PR and reports a planted secret finding without failing the build, then remove the planted secret. +- [ ] 1.3 Run a one-time full-history gitleaks scan against the repo; record any findings in a tracked triage list and rotate/redact confirmed secrets before closing this task. +- [ ] 1.4 Run budget service tests/lint clean; PR merged. + +## 2. Bandit SAST pilot on budget — depends on 1 + +- [ ] 2.0 Run `scripts/start-group.sh ci-compliance-guardrails 2` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 2.1 Add Bandit to budget's lint CI step (`continue-on-error: true`); verify it flags a planted insecure pattern (e.g. `eval` on untrusted input) in a throwaway commit, then remove the planted pattern. +- [ ] 2.2 Run budget service tests/lint clean; PR merged. + +## 3. Semgrep custom rules (PII-in-logs, raw-SQL) pilot on budget — depends on 1 + +- [ ] 3.0 Run `scripts/start-group.sh ci-compliance-guardrails 3` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 3.1 Create `.semgrep/rules/pii-logging.yml`; verify it flags a planted logger call whose argument is named after a known PII category (email, ssn, password, token), then remove the planted call. +- [ ] 3.2 Create `.semgrep/rules/raw-sql-interpolation.yml`; verify it flags a planted f-string/`.format()`-built SQL query, then remove the planted query. +- [ ] 3.3 Add a Semgrep CI job (`continue-on-error: true`) to budget.yml running `.semgrep/rules/`. +- [ ] 3.4 Run budget service tests/lint clean; PR merged. + +## 4. Roll out gitleaks + Bandit + Semgrep (PII/SQL rules) to remaining services — depends on 1, 2, 3 + +- [ ] 4.0 Run `scripts/start-group.sh ci-compliance-guardrails 4` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 4.1 Add the three advisory CI jobs to `chat.yml`, `ai.yml`, `users.yml`, `worker.yml`, `shared.yml`, `frontend.yml`, and `e2e.yml`, mirroring budget's configuration. +- [ ] 4.2 Verify each workflow completes successfully (green, findings advisory-only) on a real PR touching that service. +- [ ] 4.3 Run the full test suite across all affected services clean; PR merged. + +## 5. Tenant-scoping Semgrep rule (narrow scope, all services) — depends on 4 + +- [ ] 5.0 Run `scripts/start-group.sh ci-compliance-guardrails 5` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 5.1 Enumerate the known multi-tenant tables/models requiring `customer_id` scoping, cross-referencing the `model-audit-trail` spec and current AuditMixin inventory. +- [ ] 5.2 Create `.semgrep/rules/tenant-scoping.yml` scoped to that table list; verify it flags a planted unscoped query against a known multi-tenant table and does not flag existing superuser/impersonation code paths (`customer-impersonation` capability), then remove the planted query. +- [ ] 5.3 Add the rule to all 8 workflows in advisory mode (`continue-on-error: true`). +- [ ] 5.4 Run the full test suite clean; PR merged. + +## 6. Promotion tracking + documentation — depends on 4, 5 + +- [ ] 6.0 Run `scripts/start-group.sh ci-compliance-guardrails 6` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 6.1 Add a lightweight findings-tracking mechanism (e.g. a periodic export of CI annotation counts per check) so the promotion criteria has real data behind it. +- [ ] 6.2 Write `docs/security/ci-compliance-guardrails.md` documenting: each check, its advisory start date, promotion criteria, the current required-check inventory (including checks pending from `gdpr-iso27001-priority-2` and `audit-mixin-coverage-guard`), and the waiver process. +- [ ] 6.3 Run lint clean on the new docs; PR merged. + +## 7. Promote checks to required + enable branch protection — depends on 6, and on each check's documented advisory period having elapsed + +- [ ] 7.0 Run `scripts/start-group.sh ci-compliance-guardrails 7` to create/link this group's sub-issue and branch before starting any other work in this group. +- [ ] 7.1 Confirm each check meets its documented promotion criterion (false-positive rate, advisory duration) using the tracking data from group 6. +- [ ] 7.2 Remove `continue-on-error` from each promoted check's CI step across all workflows. +- [ ] 7.3 Enable GitHub branch protection on `main` requiring the promoted checks (repo admin action); verify a test PR with a deliberately failing check is blocked from merging, then revert the test change. +- [ ] 7.4 Update `docs/security/ci-compliance-guardrails.md`'s required-check list to reflect what's now enforced; PR merged. diff --git a/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md b/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md index 5caa7ba1..4182011d 100644 --- a/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md +++ b/openspec/changes/shared-feat-299-audit-mixin-rollout-tier3/tasks.md @@ -8,12 +8,12 @@ One task group = one GitHub ticket = one PR, merged before the next group starts - [x] 1.4 Add/update tests confirming `created_by` is populated on creation for each of the 5 models, and `updated_by` behaves per the model's mutability (populated on update for mutable models, stays `NULL` for the append-only `AIAuditLog`/`PrivilegedAccessLog`). - [x] 1.5 Run `services/ai`'s test suite clean; PR merged. -## 2. chat service — depends on 1 +## 2. chat service — depends on 1 — Issue #303 -- [ ] 2.1 Add Alembic migration adding nullable `created_by`/`updated_by` to `Conversation`, `Message`, `PrivilegedAccessLog`. -- [ ] 2.2 Update each model class to inherit `AuditMixin`. -- [ ] 2.3 Repeat the `PrivilegedAccessLog` actor-field check from task 1.3 for chat's copy. -- [ ] 2.4 Add/update tests confirming `created_by`/`updated_by` population for `Conversation`/`Message`, and `created_by`-only for `PrivilegedAccessLog`. +- [x] 2.1 Add Alembic migration adding nullable `created_by`/`updated_by` to `Conversation`, `Message`, `PrivilegedAccessLog`. +- [x] 2.2 Update each model class to inherit `AuditMixin`. +- [x] 2.3 Repeat the `PrivilegedAccessLog` actor-field check from task 1.3 for chat's copy. +- [x] 2.4 Add/update tests confirming `created_by`/`updated_by` population for `Conversation`/`Message`, and `created_by`-only for `PrivilegedAccessLog`. - [ ] 2.5 Run `services/chat`'s test suite clean; PR merged. ## 3. budget service — depends on 1 diff --git a/services/ai/tests/test_tier3_audit_columns.py b/services/ai/tests/test_tier3_audit_columns.py index 8f6d7561..fe0cd95a 100644 --- a/services/ai/tests/test_tier3_audit_columns.py +++ b/services/ai/tests/test_tier3_audit_columns.py @@ -41,19 +41,23 @@ def test_created_by_populated_on_insert(self, db): assert log.created_by == user_id def test_updated_by_stays_null_with_no_update_path(self, db): - log = AIAuditLog( - customer_id=str(uuid.uuid4()), - user_id=str(uuid.uuid4()), - prompt_version="v1", - input_text="text", - provider="anthropic", - model="claude", - success=True, - duration_ms=1, - created_at=_now(), - ) - db.add(log) - db.commit() + token = set_current_user_id(uuid.uuid4()) + try: + log = AIAuditLog( + customer_id=str(uuid.uuid4()), + user_id=str(uuid.uuid4()), + prompt_version="v1", + input_text="text", + provider="anthropic", + model="claude", + success=True, + duration_ms=1, + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) assert log.updated_by is None @@ -198,14 +202,18 @@ def test_created_by_matches_existing_actor_field(self, db): assert str(log.actor_user_id) == str(actor_id) def test_updated_by_stays_null_with_no_update_path(self, db): - log = PrivilegedAccessLog( - actor_user_id=str(uuid.uuid4()), - customer_id=str(uuid.uuid4()), - method="GET", - path="/api/v1/ai/settings", - created_at=_now(), - ) - db.add(log) - db.commit() + token = set_current_user_id(uuid.uuid4()) + try: + log = PrivilegedAccessLog( + actor_user_id=str(uuid.uuid4()), + customer_id=str(uuid.uuid4()), + method="GET", + path="/api/v1/ai/settings", + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) assert log.updated_by is None diff --git a/services/chat/app/crud/conversation.py b/services/chat/app/crud/conversation.py index 90d22577..16882a39 100644 --- a/services/chat/app/crud/conversation.py +++ b/services/chat/app/crud/conversation.py @@ -159,18 +159,18 @@ async def get_conversation_messages( if not _is_valid_uuid(conversation_id): return None - result = await db.execute( + owner_check = await db.execute( select(Conversation.id).where( Conversation.id == conversation_id, Conversation.customer_id == customer_id ) ) - if result.scalar_one_or_none() is None: + if owner_check.scalar_one_or_none() is None: return None - result = await db.execute( + messages_result = await db.execute( select(Message) .where(Message.conversation_id == conversation_id) .order_by(Message.created_at.desc()) .limit(limit) ) - return list(reversed(result.scalars().all())) + return list(reversed(messages_result.scalars().all())) diff --git a/services/chat/app/models/conversation.py b/services/chat/app/models/conversation.py index 20e0b05f..ed1921a2 100644 --- a/services/chat/app/models/conversation.py +++ b/services/chat/app/models/conversation.py @@ -3,15 +3,14 @@ from sqlalchemy import DateTime, Integer, String from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class Conversation(Base): +class Conversation(Base, AuditMixin): __tablename__ = "conversations" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) customer_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) user_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) title: Mapped[str | None] = mapped_column(String(255), nullable=True) diff --git a/services/chat/app/models/message.py b/services/chat/app/models/message.py index c47e85fc..71edd7ba 100644 --- a/services/chat/app/models/message.py +++ b/services/chat/app/models/message.py @@ -3,16 +3,15 @@ from sqlalchemy import DateTime, ForeignKey, Index, JSON, String, Text from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class Message(Base): +class Message(Base, AuditMixin): __tablename__ = "messages" __table_args__ = (Index("ix_messages_conversation_created", "conversation_id", "created_at"),) - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) conversation_id: Mapped[t.GUID] = mapped_column( t.GUID(), ForeignKey("conversations.id", ondelete="CASCADE"), nullable=False, index=True ) diff --git a/services/chat/app/models/privileged_access_log.py b/services/chat/app/models/privileged_access_log.py index bcd4f5d8..ebddb4c2 100644 --- a/services/chat/app/models/privileged_access_log.py +++ b/services/chat/app/models/privileged_access_log.py @@ -5,17 +5,16 @@ from sqlalchemy.orm import Mapped, mapped_column from app.models.base import Base +from shared.db.audit_mixin import AuditMixin import shared.db.type_decorators as t -class PrivilegedAccessLog(Base): +class PrivilegedAccessLog(Base, AuditMixin): """Append-only — no update/delete path exists anywhere in the app.""" __tablename__ = "privileged_access_logs" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) + id: Mapped[uuid.UUID] = mapped_column(t.GUID(), primary_key=True, default=lambda: uuid.uuid4()) actor_user_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) customer_id: Mapped[t.GUID] = mapped_column(t.GUID(), nullable=False, index=True) method: Mapped[str] = mapped_column(String, nullable=False) diff --git a/services/chat/migrations/versions/003_tier3_audit_columns.py b/services/chat/migrations/versions/003_tier3_audit_columns.py new file mode 100644 index 00000000..fd3eb736 --- /dev/null +++ b/services/chat/migrations/versions/003_tier3_audit_columns.py @@ -0,0 +1,42 @@ +"""Add created_by/updated_by (audit-mixin-rollout-tier3, group 2) + +Revision ID: 003_tier3_audit_columns +Revises: 002_add_privileged_access_logs +Create Date: 2026-09-20 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +from shared.db.type_decorators import GUID + +revision: str = "003_tier3_audit_columns" +down_revision: Union[str, Sequence[str], None] = "002_add_privileged_access_logs" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# None of these tables had updated_at before — every AuditMixin column is new here. +_TABLES = ["conversations", "messages", "privileged_access_logs"] + + +def _column(name: str) -> sa.Column: + if name == "updated_at": + return sa.Column(name, sa.DateTime(timezone=True), nullable=True) + return sa.Column(name, GUID(), nullable=True) + + +def upgrade() -> None: + for table in _TABLES: + op.add_column(table, _column("created_by")) + op.add_column(table, _column("updated_by")) + op.add_column(table, _column("updated_at")) + + +def downgrade() -> None: + for table in _TABLES: + op.drop_column(table, "updated_at") + op.drop_column(table, "updated_by") + op.drop_column(table, "created_by") diff --git a/services/chat/tests/conftest.py b/services/chat/tests/conftest.py index 2d23f522..01fe5130 100644 --- a/services/chat/tests/conftest.py +++ b/services/chat/tests/conftest.py @@ -12,19 +12,21 @@ from app.api.chat_routes import get_validated_user # noqa: E402 from app.models.base import Base # noqa: E402 +from app.models.conversation import Conversation # noqa: E402 +from app.models.message import Message # noqa: E402 from app.models.privileged_access_log import PrivilegedAccessLog # noqa: E402 from main import app # noqa: E402,F401 from tests.factories.user import ValidUserFactory # noqa: E402 +_DB_TABLES = [PrivilegedAccessLog.__table__, Conversation.__table__, Message.__table__] + @pytest.fixture def db(): - """Real in-memory sqlite session covering PrivilegedAccessLog — sync, - matching this sink's deliberately sync design (see - app/services/privileged_access_audit.py). Add tables here as more tests - need a real DB session for this service.""" + """Real in-memory sqlite session — sync, matching this service's sync + audit sinks. Add tables to _DB_TABLES as more tests need a real DB session.""" engine = create_engine("sqlite:///:memory:") - Base.metadata.create_all(engine, tables=[PrivilegedAccessLog.__table__]) + Base.metadata.create_all(engine, tables=_DB_TABLES) return sessionmaker(bind=engine)() diff --git a/services/chat/tests/test_tier3_audit_columns.py b/services/chat/tests/test_tier3_audit_columns.py new file mode 100644 index 00000000..c469171d --- /dev/null +++ b/services/chat/tests/test_tier3_audit_columns.py @@ -0,0 +1,110 @@ +"""audit-mixin-rollout-tier3 group 2: created_by/updated_by population for +Conversation, Message, PrivilegedAccessLog.""" + +import uuid +from datetime import datetime, timezone + +from app.models.privileged_access_log import PrivilegedAccessLog +from shared.security.current_user_context import reset_current_user_id, set_current_user_id +from tests.factories.conversation import ConversationFactory, MessageFactory + + +def _now(): + return datetime.now(timezone.utc) + + +class TestConversationMutable: + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + conversation = ConversationFactory() + db.add(conversation) + db.commit() + finally: + reset_current_user_id(token) + + assert conversation.created_by == user_id + + def test_updated_by_populated_on_update(self, db): + conversation = ConversationFactory() + db.add(conversation) + db.commit() + + updater_id = uuid.uuid4() + token = set_current_user_id(updater_id) + try: + conversation.message_count = 2 + conversation.last_activity_at = _now() + db.commit() + finally: + reset_current_user_id(token) + + assert conversation.updated_by == updater_id + + +class TestMessageAppendOnly: + # No update code path exists for Message anywhere in the app (see app/crud/conversation.py). + + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + message = MessageFactory() + db.add(message) + db.commit() + finally: + reset_current_user_id(token) + + assert message.created_by == user_id + + def test_updated_by_stays_null_with_no_update_path(self, db): + token = set_current_user_id(uuid.uuid4()) + try: + message = MessageFactory() + db.add(message) + db.commit() + finally: + reset_current_user_id(token) + + assert message.updated_by is None + + +class TestPrivilegedAccessLogAppendOnly: + def test_created_by_matches_existing_actor_field(self, db): + """actor_user_id already captures the acting user (task 1.3) — the + automatically-populated created_by should equal it, not diverge.""" + actor_id = uuid.uuid4() + token = set_current_user_id(actor_id) + try: + log = PrivilegedAccessLog( + actor_user_id=str(actor_id), + customer_id=str(uuid.uuid4()), + method="PUT", + path="/api/v1/chat", + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) + + assert log.created_by == actor_id + assert str(log.actor_user_id) == str(actor_id) + + def test_updated_by_stays_null_with_no_update_path(self, db): + token = set_current_user_id(uuid.uuid4()) + try: + log = PrivilegedAccessLog( + actor_user_id=str(uuid.uuid4()), + customer_id=str(uuid.uuid4()), + method="GET", + path="/api/v1/chat", + created_at=_now(), + ) + db.add(log) + db.commit() + finally: + reset_current_user_id(token) + + assert log.updated_by is None diff --git a/services/users/tests/test_audit_trail_routes.py b/services/users/tests/test_audit_trail_routes.py index 84b7f918..04e5e8d1 100644 --- a/services/users/tests/test_audit_trail_routes.py +++ b/services/users/tests/test_audit_trail_routes.py @@ -86,7 +86,7 @@ async def test_created_by_populated_via_real_auth_chain(self, bug_reports_db): ) ).scalar_one() assert str(bug_report.created_by) == user_id - assert str(bug_report.updated_by) == user_id + assert bug_report.updated_by is None class TestDonorGranteeAuditTrail: @@ -117,4 +117,4 @@ async def test_created_by_populated_via_real_auth_chain(self, db): ) ).scalar_one() assert str(donor_grantee.created_by) == user_id - assert str(donor_grantee.updated_by) == user_id + assert donor_grantee.updated_by is None diff --git a/shared/db/audit_mixin.py b/shared/db/audit_mixin.py index 52d15fd8..8f37c0aa 100644 --- a/shared/db/audit_mixin.py +++ b/shared/db/audit_mixin.py @@ -31,11 +31,10 @@ class AuditMixin(AuditColumnsMixin): @event.listens_for(AuditColumnsMixin, "before_insert", propagate=True) def _set_created_by_and_updated_by_on_insert(mapper, connection, target: AuditColumnsMixin) -> None: - # Never clobber a value a caller already set explicitly (e.g. manual created_by=/updated_by=). + # updated_by stays NULL here so it can prove append-only rows were never modified. user_id = get_current_user_id() if user_id is not None: target.created_by = user_id - target.updated_by = user_id @event.listens_for(AuditColumnsMixin, "before_update", propagate=True) diff --git a/shared/tests/test_audit_mixin.py b/shared/tests/test_audit_mixin.py index fdc00d50..baf0dd0f 100644 --- a/shared/tests/test_audit_mixin.py +++ b/shared/tests/test_audit_mixin.py @@ -48,7 +48,7 @@ def session(): class TestAuditMixinListener: - def test_created_by_and_updated_by_set_on_insert_when_context_set(self, session): + def test_created_by_set_and_updated_by_stays_null_on_insert_when_context_set(self, session): user_id = uuid.uuid4() token = set_current_user_id(user_id) try: @@ -59,7 +59,7 @@ def test_created_by_and_updated_by_set_on_insert_when_context_set(self, session) reset_current_user_id(token) assert widget.created_by == user_id - assert widget.updated_by == user_id + assert widget.updated_by is None def test_created_by_stays_null_when_context_unset(self, session): widget = _WidgetModel() @@ -141,11 +141,11 @@ def test_relationship_only_mutation_does_not_update_updated_by(self, session): finally: reset_current_user_id(token) - assert widget.updated_by == creator_id + assert widget.updated_by is None class TestAuditColumnsMixinListener: - def test_created_by_set_on_insert_when_context_set(self, session): + def test_created_by_set_and_updated_by_stays_null_on_insert_when_context_set(self, session): user_id = uuid.uuid4() token = set_current_user_id(user_id) try: @@ -156,7 +156,7 @@ def test_created_by_set_on_insert_when_context_set(self, session): reset_current_user_id(token) assert tag.created_by == user_id - assert tag.updated_by == user_id + assert tag.updated_by is None def test_created_by_stays_null_when_context_unset(self, session): tag = _TagModel(slug="acme")