diff --git a/openspec/changes/audit-mixin-rollout-tier4/tasks.md b/openspec/changes/audit-mixin-rollout-tier4/tasks.md deleted file mode 100644 index 265f445b..00000000 --- a/openspec/changes/audit-mixin-rollout-tier4/tasks.md +++ /dev/null @@ -1,24 +0,0 @@ -One task group = one GitHub ticket = one PR, merged before the next group starts. - -## 1. ai service catalog models — depends on audit-mixin-auto-population being merged - -- [ ] 1.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `AIProvider`, `AIProviderModel`. -- [ ] 1.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions are unaffected (mixin attribute override, per design.md decision 1). -- [ ] 1.3 Add/update tests confirming the new columns populate correctly for authenticated admin-driven creation, and stay `NULL` for unauthenticated seed/migration inserts. -- [ ] 1.4 Run `services/ai`'s test suite clean; PR merged. - -## 2. budget service DonorTemplateModel — depends on 1 - -- [ ] 2.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `DonorTemplateModel`. -- [ ] 2.2 Update the model class to inherit `AuditMixin`. -- [ ] 2.3 Add/update a test confirming `created_by` is populated on template upload via the authenticated route. -- [ ] 2.4 Run `services/budget`'s test suite clean; PR merged. - -## 3. users service CustomerModel and UserModel — depends on 1, 2 - -- [ ] 3.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `CustomerModel` and `UserModel`. -- [ ] 3.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions (including `UserModel`'s `str(uuid.uuid4())` default) are unaffected. -- [ ] 3.3 Audit admin-management and company-onboarding flows to confirm they pass an authenticated actor context so `created_by` populates correctly there (per design.md's open question); self-registration is expected to leave `created_by` `NULL`. -- [ ] 3.4 Add/update tests: self-registration leaves `created_by` `NULL`; admin-created accounts get a non-NULL `created_by`; updating a user/customer sets `updated_by`. -- [ ] 3.5 Manually verify login, self-registration, and admin-management flows on staging before merge, per this repo's standard practice for auth-adjacent schema changes. -- [ ] 3.6 Run `services/users`'s test suite clean; PR merged. diff --git a/openspec/changes/budget-fix-money-integrity/.openspec.yaml b/openspec/changes/budget-fix-money-integrity/.openspec.yaml new file mode 100644 index 00000000..4d71b525 --- /dev/null +++ b/openspec/changes/budget-fix-money-integrity/.openspec.yaml @@ -0,0 +1,4 @@ +schema: spec-driven +created: 2026-09-22 +author: Norair Arutshyan +priority: high diff --git a/openspec/changes/budget-fix-money-integrity/proposal.md b/openspec/changes/budget-fix-money-integrity/proposal.md new file mode 100644 index 00000000..76bda18c --- /dev/null +++ b/openspec/changes/budget-fix-money-integrity/proposal.md @@ -0,0 +1,29 @@ +## Why + +Two related money-correctness gaps in the budget service, both confirmed but deliberately not blocking prior ships: + +1. **`Budget.total_amount` recalculation is fragile.** From the donor-dashboard code review (#133): `create_budget_with_lines_service`'s rollback path bypasses `recalculate_budget_total` entirely (calls raw `delete_budget_line` CRUD directly, currently masked because the parent budget is deleted right after); the line write and total recalculation commit separately with no wrapping transaction; money is stored as `Float` (not `Decimal`), so binary floating-point drift is now surfaced through the API; a missed recalculation is silently swallowed with no log; `total_amount` is client-writable on `BudgetCreate`/`BudgetUpdate` but silently dropped; the `COALESCE(SUM(amount), 0)` formula is implemented twice (migration backfill SQL + SQLAlchemy) with no single source of truth. **The `Float` problem is wider than `total_amount` alone** — a repo-wide check found the same `Float` type on `BudgetLineModel.amount`, `ReportLine.amount` (`report.py`), and the entire currency-ledger table set (`currency_ledger.py`: `FundingReceipt.amount`, `CurrencyConversion.donor_amount`/`local_amount`, `CurrencyConversionAllocation.amount_allocated`) — every money column in the budget service is `Float`. +2. **Currency codes are never validated.** `shared/services/currency_service.py` has a working ISO 4217 module (`validate_currency`, `get_currency_list`, etc.) re-exported by both budget and users utils, but no actual call site validates `Budget.actual_currency`/`local_currency` or the currency-ledger's `record_receipt_service`/`record_conversion_service` inputs. The frontend currency dropdown is a hardcoded 11-code stand-in (`frontend-typescript/src/utils/currency.ts`) for the same reason — no endpoint exposes the real list. + +Bundled together because both are "money can silently be wrong" gaps in the same service, even though they're different mechanisms (recalculation integrity vs. input validation) — real money starts flowing through this table via the currency ledger, so both get harder to fix the longer they wait. + +## What Changes + +- Fix the rollback-path gap: route `create_budget_with_lines_service`'s exception handlers through `recalculate_budget_total` (or a CRUD-layer/ORM event listener so no future line-mutation path can skip it), not raw `delete_budget_line`. +- Wrap line-write + recalculation in a single transaction so a second-commit failure can't leave a stale total persisted. +- Migrate all budget-service money columns from `Float` to `Decimal`/`Numeric` — `budget.py` (`BudgetModel`, `BudgetLineModel`), `report.py` (`ReportLine.amount`), `currency_ledger.py` (`FundingReceipt.amount`, `CurrencyConversion.donor_amount`/`local_amount`, `CurrencyConversionAllocation.amount_allocated`) — including a data migration for existing rows. Doing this before the active `ledger-budget` change adds more update/delete surface on these same currency-ledger columns avoids compounding the Float-drift problem in new code. +- Either enforce or reject `total_amount` on `BudgetCreate`/`BudgetUpdate` instead of silently dropping it; log when a recalculation is skipped (budget not found). +- De-duplicate the `COALESCE(SUM(amount), 0)` formula to one source of truth. +- Wire `validate_currency()` into `update_budget_service` (`actual_currency`/`local_currency`) and into `record_receipt_service`/`record_conversion_service`. +- Expose `get_currency_list()` via a small endpoint (budget or users service) so the frontend dropdown can replace its hardcoded 11-code stand-in with the real ISO 4217 list. + +## Capabilities + +### New Capabilities +- `currency-code-validation`: server-side rejection of invalid ISO 4217 currency codes on budget and currency-ledger writes. + +## Status + +Proposal only — no design.md/tasks.md yet. Recommend investigating the `Float`→`Decimal` migration path (data migration for existing rows) before task breakdown, since it's the piece most likely to get harder the longer it waits. + +**Dependency note:** `currency_ledger_services.py` (`record_receipt_service`/`record_conversion_service`) is also touched by the active, not-yet-started `ledger-budget` change (adds update/delete for funding receipts and currency conversions on the same file). No hard blocker either order, but landing this change first — schema types and validation in place before more code is added against `Float` columns — avoids rework in `ledger-budget` and a same-file merge conflict if both are in flight at once. diff --git a/openspec/changes/ledger-budget/.openspec.yaml b/openspec/changes/ledger-budget/.openspec.yaml index e08b5f89..2aea6b73 100644 --- a/openspec/changes/ledger-budget/.openspec.yaml +++ b/openspec/changes/ledger-budget/.openspec.yaml @@ -1,2 +1,4 @@ schema: spec-driven created: 2026-08-03 +depends_on: + - budget-fix-money-integrity diff --git a/openspec/changes/shared-feat-138-status-history/.openspec.yaml b/openspec/changes/shared-feat-138-status-history/.openspec.yaml new file mode 100644 index 00000000..22525f43 --- /dev/null +++ b/openspec/changes/shared-feat-138-status-history/.openspec.yaml @@ -0,0 +1,7 @@ +schema: spec-driven +created: 2026-09-22 +author: Norair Arutshyan +priority: medium +depends_on: + - shared-feat-307-audit-mixin-rollout-tier4 + - async-privileged-access-audit diff --git a/openspec/changes/shared-feat-138-status-history/proposal.md b/openspec/changes/shared-feat-138-status-history/proposal.md new file mode 100644 index 00000000..fadc2fa7 --- /dev/null +++ b/openspec/changes/shared-feat-138-status-history/proposal.md @@ -0,0 +1,24 @@ +## Why + +`BudgetModel.status` (`ai_draft`/`draft`/`confirmed`/`archived`) is a plain field with no transition history: there's no way to answer "was this budget ever confirmed before it was archived," and no `AuditLog`-style table exists for budget mutations (only `services/ai` has one). Status is mutated via a generic PATCH-style path (`budget_crud.py`, `budget.status = status or budget.status`) with no record of who changed it or when. The identical need has now surfaced independently in a second domain: admin/superuser actions (invite user, remove user, update company, deactivate company) in `admin-management-page`, which today logs nothing for a company admin acting within their own tenant (only impersonation sessions get `privileged_access_logs` coverage). Two independent domains wanting the same shape (actor/target/from→to/timestamp) is a strong signal this should be one general mechanism, not a budget-only table. + +## What Changes + +- Add a general-purpose transition/action audit-log table (or `shared/db/` mixin), not scoped to budgets specifically — shape: actor, target entity/id, from-state, to-state, changed_at. +- Wire budget status transitions (`budget_crud.py`'s status-update choke point) through this mechanism. +- Wire admin-management actions (invite, remove, promote/demote, deactivate) through the same mechanism, including company-admin-acting-within-own-tenant (not just superuser impersonation, which already has separate `privileged_access_logs` coverage). +- Design the table to double as the source of truth for "was this budget ever confirmed" (needed by the donor dashboard's grantee status breakdown), rather than adding a separate `confirmed_at` high-water-mark column later. +- Decide whether an intermediate `submitted` status (grantee → donor handoff, discussed but not yet added to `BudgetStatus`) belongs in this same piece of work, since it would be another transition the table needs to capture. + +## Capabilities + +### New Capabilities +- `transition-audit-log`: general actor/target/from→to/timestamp audit trail, applied to budget status transitions and admin-management actions. + +## Status + +Proposal only — scope and design (single shared table vs. per-domain tables via a common mixin, exact schema, whether `submitted` status is in scope) need deeper investigation before task breakdown. Parent tracking: [GitHub issue #138](https://github.com/arutsh/GrantFlow/issues/138). + +**Dependency note:** two audit-infrastructure changes are already in flight and this should sequence behind both, not run in parallel: +- `shared-feat-307-audit-mixin-rollout-tier4` (current branch) — finishing it first avoids context-switching mid-audit-buildout and any incidental conflict in `shared/db/`. +- `async-privileged-access-audit` (active, 0/9 tasks done) — makes `log_privileged_access`/`PrivilegedAccessSink` async and removes the per-service dedicated sync engine each service was hand-copying for audit writes. This new transition-audit-log's write path should reuse that same async sink pattern once it lands, rather than becoming a 5th hand-duplicated audit-write mechanism (see `[[project_privileged_access_log_model_duplication]]`). diff --git a/openspec/changes/audit-mixin-rollout-tier4/.openspec.yaml b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/.openspec.yaml similarity index 72% rename from openspec/changes/audit-mixin-rollout-tier4/.openspec.yaml rename to openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/.openspec.yaml index b4b3ece7..f66047ac 100644 --- a/openspec/changes/audit-mixin-rollout-tier4/.openspec.yaml +++ b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/.openspec.yaml @@ -1,2 +1,3 @@ schema: spec-driven created: 2026-09-01 +priority: high diff --git a/openspec/changes/audit-mixin-rollout-tier4/design.md b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/design.md similarity index 69% rename from openspec/changes/audit-mixin-rollout-tier4/design.md rename to openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/design.md index 47a1755c..11137b5a 100644 --- a/openspec/changes/audit-mixin-rollout-tier4/design.md +++ b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/design.md @@ -1,6 +1,6 @@ ## Context -The 5 Tier 4 models each already declare their own `id` primary key column (`UserModel.id` uses `default=lambda: str(uuid.uuid4())`, notably a `str` default rather than `AuditMixin`'s `uuid.UUID` default — a pre-existing minor inconsistency, not something this change needs to reconcile). `AuditMixin` also declares an `id` column. In SQLAlchemy declarative mixins, an attribute defined directly on the concrete model class takes precedence over the same-named attribute inherited from a mixin — so mixing in `AuditMixin` on these models does **not** require touching their existing `id` definitions; only the unset `created_at`/`updated_at`/`created_by`/`updated_by` attributes get pulled in from the mixin. This significantly de-risks adopting `AuditMixin` on `UserModel`/`CustomerModel`: no PK change, no id-format migration. +The 5 Tier 4 models each already declare their own `id` primary key column. `AuditMixin` also declares an `id` column. In SQLAlchemy declarative mixins, an attribute defined directly on the concrete model class takes precedence over the same-named attribute inherited from a mixin — so mixing in `AuditMixin` on these models does **not** require touching their existing `id` definitions; only the unset `created_at`/`updated_at`/`created_by`/`updated_by` attributes get pulled in from the mixin. This significantly de-risks adopting `AuditMixin` on `UserModel`/`CustomerModel`: no PK change, no id-format migration. `UserModel`/`CustomerModel` sit on the auth/tenancy hot path (login, registration, JWT issuance, company onboarding, admin management) with a large existing test surface — this is the highest-risk tier of the whole rollout, ordered last deliberately. @@ -13,7 +13,7 @@ The 5 Tier 4 models each already declare their own `id` primary key column (`Use **Non-Goals:** - No backfill of `created_at` for existing users/customers (historical creation time is genuinely unknown). -- No change to how `UserModel.id`/`CustomerModel.id` are generated (stays `str(uuid.uuid4())`, not unified with `AuditMixin`'s `uuid.uuid4()` — cosmetic inconsistency, out of scope). +- No change to how `UserModel.id`/`CustomerModel.id` are generated — both already use `uuid.uuid4()` directly, same as `AuditMixin`'s default, so there's nothing to reconcile. - No requirement that `created_by` be non-NULL — self-registration has no authenticated actor, so NULL is a valid, expected value here (unlike, say, budget models where every row has a clear creator). ## Decisions @@ -39,4 +39,5 @@ Standard Alembic migration per service (`ai`, `budget`, `users`), additive/nulla ## Open Questions -- Should admin-created accounts (company-onboarding, admin-invite flows) be audited to confirm they already pass an actor context that would populate `created_by` correctly, or do those flows also run unauthenticated in some cases? +- ~~Should admin-created accounts (company-onboarding, admin-invite flows) be audited to confirm they already pass an actor context that would populate `created_by` correctly, or do those flows also run unauthenticated in some cases?~~ + **Resolved:** both `POST /users/invite` and `POST /customers/` require `Depends(get_validated_user)` (`services/users/app/api/user_routes.py:265`, `services/users/app/api/customer_routes.py:36`), which sets the actor contextvar (`shared/security/dependencies.py:54`) before the row is inserted — covered by `TestAdminInviteAuditTrail`/`TestCustomerCreationAuditTrail` in `test_tier4_audit_columns.py`. "Company-onboarding" (a new company's founder self-registering) goes through the unauthenticated self-registration path instead, where `created_by` staying `NULL` is the documented, tested (`TestUserSelfRegistrationAuditTrail`), expected behavior per decision 2 above — not a gap. diff --git a/openspec/changes/audit-mixin-rollout-tier4/proposal.md b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/proposal.md similarity index 100% rename from openspec/changes/audit-mixin-rollout-tier4/proposal.md rename to openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/proposal.md diff --git a/openspec/changes/audit-mixin-rollout-tier4/specs/model-audit-trail/spec.md b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/specs/model-audit-trail/spec.md similarity index 100% rename from openspec/changes/audit-mixin-rollout-tier4/specs/model-audit-trail/spec.md rename to openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/specs/model-audit-trail/spec.md diff --git a/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/tasks.md b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/tasks.md new file mode 100644 index 00000000..3431d4e0 --- /dev/null +++ b/openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/tasks.md @@ -0,0 +1,24 @@ +One task group = one GitHub ticket = one PR, merged before the next group starts. + +## 1. ai service catalog models — depends on audit-mixin-auto-population being merged — Issue #308 + +- [x] 1.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `AIProvider`, `AIProviderModel`. +- [x] 1.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions are unaffected (mixin attribute override, per design.md decision 1). +- [x] 1.3 Add/update tests confirming the new columns populate correctly for authenticated admin-driven creation, and stay `NULL` for unauthenticated seed/migration inserts. +- [ ] 1.4 Run `services/ai`'s test suite clean; PR merged. (Test suite clean: 136 passed. PR not yet opened.) + +## 2. budget service DonorTemplateModel — depends on 1 — Issue #309 + +- [x] 2.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `DonorTemplateModel`. +- [x] 2.2 Update the model class to inherit `AuditMixin`. +- [x] 2.3 Add/update a test confirming `created_by` is populated on template upload via the authenticated route. +- [ ] 2.4 Run `services/budget`'s test suite clean; PR merged. (Test suite clean: 374 passed. PR not yet opened.) + +## 3. users service CustomerModel and UserModel — depends on 1, 2 — Issue #310 + +- [x] 3.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `CustomerModel` and `UserModel`. +- [x] 3.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions (including `UserModel`'s `str(uuid.uuid4())` default) are unaffected. +- [x] 3.3 Audit admin-management and company-onboarding flows to confirm they pass an authenticated actor context so `created_by` populates correctly there (per design.md's open question); self-registration is expected to leave `created_by` `NULL`. +- [x] 3.4 Add/update tests: self-registration leaves `created_by` `NULL`; admin-created accounts get a non-NULL `created_by`; updating a user/customer sets `updated_by`. +- [ ] 3.5 Manually verify login, self-registration, and admin-management flows on staging before merge, per this repo's standard practice for auth-adjacent schema changes. (Login, self-registration, and customer creation already verified locally; staging still pending.) +- [ ] 3.6 Run `services/users`'s test suite clean; PR merged. (Test suite clean: 202 passed. PR not yet opened.) diff --git a/openspec/changes/users-fix-auth-claims-hardening/.openspec.yaml b/openspec/changes/users-fix-auth-claims-hardening/.openspec.yaml new file mode 100644 index 00000000..4d99c9bc --- /dev/null +++ b/openspec/changes/users-fix-auth-claims-hardening/.openspec.yaml @@ -0,0 +1,4 @@ +schema: spec-driven +created: 2026-09-22 +author: Norair Arutshyan +priority: medium diff --git a/openspec/changes/users-fix-auth-claims-hardening/proposal.md b/openspec/changes/users-fix-auth-claims-hardening/proposal.md new file mode 100644 index 00000000..8e2dcedc --- /dev/null +++ b/openspec/changes/users-fix-auth-claims-hardening/proposal.md @@ -0,0 +1,22 @@ +## Why + +Two independent findings on the same auth surface (`services/users/app/api/auth_routes.py`, `services/users/app/crud/user_crud.py`): + +1. **No shared claims builder.** `register_endpoint`, `login`, and `refresh_token` each independently build their own inline JWT claims dict literal, with no shared schema pinning claim key names/types across the Python producer, the hand-written `TokenClaims` TS interface in `AuthContext.tsx`, and the budget service's `require_donor` check that reads the same decoded payload. Only 3 call sites exist today, but nothing enforces staying in sync as more are added (admin impersonation, magic-link login, password-reset auto-login), and a claim-name typo would silently break consumers rather than erroring. +2. **Cross-tenant email enumeration.** `create_invited_user` checks `UserModel.email == email` with no company/deleted_at/status scoping — an authenticated company admin can probe any email via the invite form and learn from the 400/success split whether it's registered anywhere on the platform, including at a company they have no relationship to. + +Grouped together because they're the same file area and both auth-hardening debt, not because they're the same kind of fix. + +## What Changes + +- Centralize JWT claims construction into one `_build_access_token_claims(db, user, session)` function called from all token-issuance sites; consider a shared schema/type pinning the claim shape between backend and frontend. +- Decide the desired behavior for invite-time uniqueness checking: scope to same-company only (and give a generic non-committal response for other-company/global collisions), or explicitly accept and document the current cross-tenant signal. **This is a product/security tradeoff, not just a bug — needs an explicit decision recorded in design.md before implementation, not a silent fix.** + +## Capabilities + +### New Capabilities +(none — this is a hardening/refactor of the existing auth-claims and invite-uniqueness mechanisms) + +## Status + +Proposal only — no design.md/tasks.md yet. The claims-builder centralization is a straightforward refactor; the enumeration item needs a product decision first. diff --git a/services/ai/app/models/ai_provider.py b/services/ai/app/models/ai_provider.py index 00c58025..d6489dae 100644 --- a/services/ai/app/models/ai_provider.py +++ b/services/ai/app/models/ai_provider.py @@ -1,18 +1,13 @@ -import uuid - from sqlalchemy import Boolean, String from sqlalchemy.orm import mapped_column, Mapped from app.models.base import Base -import shared.db.type_decorators as t +from shared.db.audit_mixin import AuditMixin -class AIProvider(Base): +class AIProvider(Base, AuditMixin): __tablename__ = "ai_providers" - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) name: Mapped[str] = mapped_column(String, nullable=False, unique=True, index=True) display_name: Mapped[str] = mapped_column(String, nullable=False) key_prefix: Mapped[str | None] = mapped_column(String, nullable=True) diff --git a/services/ai/app/models/ai_provider_model.py b/services/ai/app/models/ai_provider_model.py index 1123812e..8fc34db9 100644 --- a/services/ai/app/models/ai_provider_model.py +++ b/services/ai/app/models/ai_provider_model.py @@ -1,13 +1,12 @@ -import uuid - from sqlalchemy import Boolean, ForeignKey, String, UniqueConstraint 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 AIProviderModel(Base): +class AIProviderModel(Base, AuditMixin): """Catalog of models valid for a given provider — keeps a model like claude-haiku-4-5 from being selectable against provider ollama.""" @@ -16,9 +15,6 @@ class AIProviderModel(Base): UniqueConstraint("provider_id", "name", name="uq_ai_provider_models_provider_name"), ) - id: Mapped[t.GUID] = mapped_column( - t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4()) - ) provider_id: Mapped[t.GUID] = mapped_column( t.GUID(), ForeignKey("ai_providers.id"), nullable=False ) diff --git a/services/ai/migrations/versions/018_tier4_audit_columns.py b/services/ai/migrations/versions/018_tier4_audit_columns.py new file mode 100644 index 00000000..541ee50c --- /dev/null +++ b/services/ai/migrations/versions/018_tier4_audit_columns.py @@ -0,0 +1,29 @@ +"""Add created_at/updated_at/created_by/updated_by (audit-mixin-rollout-tier4, group 1) + +Revision ID: 018_tier4_audit_columns +Revises: 017_tier3_audit_columns +Create Date: 2026-09-21 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from shared.db.migration_helpers import add_audit_columns, drop_audit_columns + +revision: str = "018_tier4_audit_columns" +down_revision: Union[str, Sequence[str], None] = "017_tier3_audit_columns" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# None of these tables had any audit columns before — every AuditMixin column is new here. +_TABLES = ["ai_providers", "ai_provider_models"] + + +def upgrade() -> None: + for table in _TABLES: + add_audit_columns(table) + + +def downgrade() -> None: + for table in _TABLES: + drop_audit_columns(table) diff --git a/services/ai/tests/conftest.py b/services/ai/tests/conftest.py index 86658cc2..dbe2d980 100644 --- a/services/ai/tests/conftest.py +++ b/services/ai/tests/conftest.py @@ -26,12 +26,14 @@ from app.models.user_provider_key import UserProviderKey # noqa: E402 from app.models.customer_ai_defaults import CustomerAiDefaults # noqa: E402 from app.models.ai_provider import AIProvider # noqa: E402 +from app.models.ai_provider_model import AIProviderModel # noqa: E402 _DB_TABLES = [ PrivilegedAccessLog.__table__, AIAuditLog.__table__, AIPrompt.__table__, AIProvider.__table__, + AIProviderModel.__table__, UserProviderKey.__table__, CustomerAiDefaults.__table__, ] diff --git a/services/ai/tests/test_tier4_audit_columns.py b/services/ai/tests/test_tier4_audit_columns.py new file mode 100644 index 00000000..1218a6da --- /dev/null +++ b/services/ai/tests/test_tier4_audit_columns.py @@ -0,0 +1,77 @@ +"""audit-mixin-rollout-tier4 group 1: created_by/updated_by population for +AIProvider, AIProviderModel.""" + +import uuid + +from app.models.ai_provider import AIProvider +from app.models.ai_provider_model import AIProviderModel +from shared.security.current_user_context import reset_current_user_id, set_current_user_id + + +class TestAIProviderMutable: + def test_created_by_populated_on_insert(self, db): + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + provider = AIProvider(name="anthropic", display_name="Anthropic") + db.add(provider) + db.commit() + finally: + reset_current_user_id(token) + + assert provider.created_by == user_id + + def test_created_by_stays_null_for_unauthenticated_seed_insert(self, db): + provider = AIProvider(name="ollama", display_name="Ollama") + db.add(provider) + db.commit() + + assert provider.created_by is None + + def test_updated_by_populated_on_update(self, db): + provider = AIProvider(name="anthropic", display_name="Anthropic") + db.add(provider) + db.commit() + + updater_id = uuid.uuid4() + token = set_current_user_id(updater_id) + try: + provider.is_active = False + db.commit() + finally: + reset_current_user_id(token) + + assert provider.updated_by == updater_id + + +class TestAIProviderModelMutable: + def _make_provider(self, db): + provider = AIProvider(name="anthropic", display_name="Anthropic") + db.add(provider) + db.commit() + return provider + + def test_created_by_populated_on_insert(self, db): + provider = self._make_provider(db) + user_id = uuid.uuid4() + token = set_current_user_id(user_id) + try: + model = AIProviderModel( + provider_id=provider.id, name="claude-haiku-4-5", display_name="Claude Haiku 4.5" + ) + db.add(model) + db.commit() + finally: + reset_current_user_id(token) + + assert model.created_by == user_id + + def test_created_by_stays_null_for_unauthenticated_seed_insert(self, db): + provider = self._make_provider(db) + model = AIProviderModel( + provider_id=provider.id, name="claude-haiku-4-5", display_name="Claude Haiku 4.5" + ) + db.add(model) + db.commit() + + assert model.created_by is None diff --git a/services/budget/app/models/mapping.py b/services/budget/app/models/mapping.py index 31499261..e7418684 100644 --- a/services/budget/app/models/mapping.py +++ b/services/budget/app/models/mapping.py @@ -2,9 +2,10 @@ from sqlalchemy.orm import Mapped, mapped_column from sqlalchemy import String, Integer, JSON from app.models.base import Base +from shared.db.audit_mixin import AuditColumnsMixin -class DonorTemplateModel(Base): +class DonorTemplateModel(Base, AuditColumnsMixin): __tablename__ = "donor_templates" id: Mapped[int] = mapped_column(Integer, primary_key=True, index=True) diff --git a/services/budget/migrations/versions/000016_tier4_audit_columns.py b/services/budget/migrations/versions/000016_tier4_audit_columns.py new file mode 100644 index 00000000..fc25ba4e --- /dev/null +++ b/services/budget/migrations/versions/000016_tier4_audit_columns.py @@ -0,0 +1,29 @@ +"""Add created_at/updated_at/created_by/updated_by (audit-mixin-rollout-tier4, group 2) + +Revision ID: 000016 +Revises: 000015 +Create Date: 2026-09-21 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from shared.db.migration_helpers import add_audit_columns, drop_audit_columns + +revision: str = "000016" +down_revision: Union[str, Sequence[str], None] = "000015" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# donor_templates had no audit columns before — every AuditMixin column is new here. +_TABLES = ["donor_templates"] + + +def upgrade() -> None: + for table in _TABLES: + add_audit_columns(table) + + +def downgrade() -> None: + for table in _TABLES: + drop_audit_columns(table) diff --git a/services/budget/tests/conftest.py b/services/budget/tests/conftest.py index 9e373aac..33615580 100644 --- a/services/budget/tests/conftest.py +++ b/services/budget/tests/conftest.py @@ -6,6 +6,7 @@ """ import os +from uuid import uuid4 os.environ.setdefault("OTEL_SDK_DISABLED", "true") @@ -27,6 +28,7 @@ ) from app.models.privileged_access_log import PrivilegedAccessLog # noqa: E402 from shared.security.dependencies import get_validated_user # noqa: E402 +from shared.security.jwt_utils import create_access_token # noqa: E402 from tests.factories.user import ValidUserFactory # noqa: E402 @@ -77,6 +79,19 @@ async def db(): await engine.dispose() +def _token_for(user_id: str, **extra_claims) -> str: + """A real signed JWT, for tests that need get_validated_user's contextvar side effect.""" + return create_access_token( + { + "user_id": user_id, + "session_id": str(uuid4()), + "role": "user", + "email_verified": True, + **extra_claims, + } + ) + + @pytest.fixture def make_client(): """Build a TestClient authenticated as a fresh fake user. diff --git a/services/budget/tests/test_email_verified_gate.py b/services/budget/tests/test_email_verified_gate.py index d4823f74..3d230f81 100644 --- a/services/budget/tests/test_email_verified_gate.py +++ b/services/budget/tests/test_email_verified_gate.py @@ -12,21 +12,13 @@ from app.api.budget_routes import get_db from app.models.base import Base from app.models.budget import BudgetModel -from shared.security.jwt_utils import create_access_token +from tests.conftest import _token_for pytestmark = pytest.mark.anyio def _token(email_verified: bool) -> str: - return create_access_token( - { - "user_id": str(uuid4()), - "session_id": str(uuid4()), - "role": "user", - "customer_id": str(uuid4()), - "email_verified": email_verified, - } - ) + return _token_for(str(uuid4()), customer_id=str(uuid4()), email_verified=email_verified) @pytest.fixture diff --git a/services/budget/tests/test_tier4_audit_columns.py b/services/budget/tests/test_tier4_audit_columns.py new file mode 100644 index 00000000..d418feaf --- /dev/null +++ b/services/budget/tests/test_tier4_audit_columns.py @@ -0,0 +1,42 @@ +"""audit-mixin-rollout-tier4 group 2: created_by/updated_by for DonorTemplateModel. +Real-JWT route test — overriding get_validated_user would skip the contextvar.""" + +from uuid import uuid4 + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from app.api.mapping_routes import get_db as mapping_get_db +from app.models.mapping import DonorTemplateModel +from main import app +from tests.conftest import _token_for + +pytestmark = pytest.mark.anyio + + +class TestDonorTemplateAuditTrail: + async def test_created_by_populated_via_real_auth_chain(self, db): + app.dependency_overrides[mapping_get_db] = lambda: db + try: + user_id = str(uuid4()) + client = TestClient(app) + + response = client.post( + "/api/v1/donor-mapping/templates", + headers={"Authorization": f"Bearer {_token_for(user_id)}"}, + json={"name": "Sample Donor Template"}, + ) + finally: + del app.dependency_overrides[mapping_get_db] + + assert response.status_code == 200 + template = ( + await db.execute( + select(DonorTemplateModel).where( + DonorTemplateModel.id == response.json()["id"] + ) + ) + ).scalar_one() + assert str(template.created_by) == user_id + assert template.updated_by is None diff --git a/services/users/app/models/customer.py b/services/users/app/models/customer.py index 45a3f017..adf1df88 100644 --- a/services/users/app/models/customer.py +++ b/services/users/app/models/customer.py @@ -7,8 +7,9 @@ from shared.db.audit_mixin import AuditMixin -class CustomerModel(Base): +class CustomerModel(Base, AuditMixin): __tablename__ = "customers" + __audit_actor_table__ = "users" id: Mapped[uuid.UUID] = mapped_column( GUID(), @@ -25,7 +26,9 @@ class CustomerModel(Base): # login/token-issuance for this company's users. Not a hard delete — # cross-service enforcement in budget/reports is a follow-on. deactivated_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) - users = relationship("UserModel", back_populates="customer") + users = relationship( + "UserModel", back_populates="customer", foreign_keys="UserModel.customer_id" + ) class DonorGranteeModel(Base, AuditMixin): diff --git a/services/users/app/models/user.py b/services/users/app/models/user.py index 3326cf26..0efdb055 100644 --- a/services/users/app/models/user.py +++ b/services/users/app/models/user.py @@ -7,11 +7,13 @@ import uuid from datetime import datetime from app.utils.db import GUID +from shared.db.audit_mixin import AuditMixin from shared.schemas.user_schema import UserStatus, UserRole -class UserModel(Base): +class UserModel(Base, AuditMixin): __tablename__ = "users" + __audit_actor_table__ = "users" id: Mapped[uuid.UUID] = mapped_column( GUID(), primary_key=True, default=lambda: uuid.uuid4(), nullable=False, index=True @@ -71,5 +73,5 @@ class UserModel(Base): deletion_requested_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) deleted_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) - customer = relationship("CustomerModel", lazy="joined") + customer = relationship("CustomerModel", lazy="joined", foreign_keys=[customer_id]) sessions = relationship("SessionModel", back_populates="user", cascade="all, delete-orphan") diff --git a/services/users/migrations/versions/000015_tier4_audit_columns.py b/services/users/migrations/versions/000015_tier4_audit_columns.py new file mode 100644 index 00000000..16028972 --- /dev/null +++ b/services/users/migrations/versions/000015_tier4_audit_columns.py @@ -0,0 +1,30 @@ +"""Add created_at/updated_at/created_by/updated_by (audit-mixin-rollout-tier4, group 3) + +Revision ID: 000015 +Revises: 000014 +Create Date: 2026-09-21 00:00:00.000000 + +""" + +from typing import Sequence, Union + +from shared.db.migration_helpers import add_audit_columns, drop_audit_columns + +revision: str = "000015" +down_revision: Union[str, Sequence[str], None] = "000014" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + +# customers/users had no audit columns before — every AuditMixin column is new here. +_TABLES = ["customers", "users"] + + +def upgrade() -> None: + # ai/budget/chat reference users cross-database and stay unconstrained. + for table in _TABLES: + add_audit_columns(table, actor_table="users") + + +def downgrade() -> None: + for table in _TABLES: + drop_audit_columns(table) diff --git a/services/users/tests/conftest.py b/services/users/tests/conftest.py index 9385bfc1..89cf29a8 100644 --- a/services/users/tests/conftest.py +++ b/services/users/tests/conftest.py @@ -8,6 +8,7 @@ import os from contextlib import ExitStack from unittest.mock import AsyncMock, patch +from uuid import uuid4 os.environ.setdefault("OTEL_SDK_DISABLED", "true") @@ -27,8 +28,10 @@ from app.models.base import Base # noqa: E402 from app.models.customer import CustomerModel, DonorGranteeModel # noqa: E402 from app.models.privileged_access_log import PrivilegedAccessLog # noqa: E402 +from app.models.user import UserModel # noqa: E402 from app.utils.security import get_current_user # noqa: E402 from shared.security.dependencies import get_validated_user # noqa: E402 +from shared.security.jwt_utils import create_access_token # noqa: E402 from tests.factories.user import ValidUserFactory # noqa: E402 @@ -40,7 +43,7 @@ def anyio_backend(): @pytest.fixture async def db(): - """Real in-memory async sqlite session (Customer/DonorGrantee/PrivilegedAccessLog); + """Real in-memory async sqlite session (Customer/DonorGrantee/PrivilegedAccessLog/User); mirrors services/ai/tests/test_email_verified_gate.py's TestClient+real-session pattern.""" engine = create_async_engine( "sqlite+aiosqlite://", @@ -54,6 +57,7 @@ async def db(): CustomerModel.__table__, DonorGranteeModel.__table__, PrivilegedAccessLog.__table__, + UserModel.__table__, ], ) maker = async_sessionmaker(engine, expire_on_commit=False) @@ -62,6 +66,19 @@ async def db(): await engine.dispose() +def _token_for(user_id: str, **extra_claims) -> str: + """A real signed JWT, for tests that need get_validated_user's contextvar side effect.""" + return create_access_token( + { + "user_id": user_id, + "session_id": str(uuid4()), + "role": "user", + "email_verified": True, + **extra_claims, + } + ) + + @pytest.fixture def make_client(): """Build a TestClient with a fake authenticated user and (optionally) a diff --git a/services/users/tests/test_audit_trail_routes.py b/services/users/tests/test_audit_trail_routes.py index 04e5e8d1..45a543e0 100644 --- a/services/users/tests/test_audit_trail_routes.py +++ b/services/users/tests/test_audit_trail_routes.py @@ -18,7 +18,7 @@ from app.models.customer import DonorGranteeModel from main import app from shared.security import session_revocation -from shared.security.jwt_utils import create_access_token +from tests.conftest import _token_for from tests.factories.user import CustomerFactory pytestmark = pytest.mark.anyio @@ -45,18 +45,6 @@ def fake_redis(monkeypatch): monkeypatch.setattr(session_revocation, "_redis_client", fakeredis.FakeStrictRedis()) -def _token_for(user_id: str, **extra_claims) -> str: - return create_access_token( - { - "user_id": user_id, - "session_id": str(uuid4()), - "role": "user", - "email_verified": True, - **extra_claims, - } - ) - - class TestBugReportAuditTrail: async def test_created_by_populated_via_real_auth_chain(self, bug_reports_db): app.dependency_overrides[bug_report_get_db] = lambda: bug_reports_db diff --git a/services/users/tests/test_tier4_audit_columns.py b/services/users/tests/test_tier4_audit_columns.py new file mode 100644 index 00000000..2c4c55f0 --- /dev/null +++ b/services/users/tests/test_tier4_audit_columns.py @@ -0,0 +1,137 @@ +"""audit-mixin-rollout-tier4 group 3: created_by/updated_by for CustomerModel/UserModel. +Real-JWT route tests — overriding get_validated_user would skip the contextvar.""" + +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from app.db.session import get_db as users_get_db +from app.models.customer import CustomerModel +from app.models.user import UserModel +from main import app +from tests.conftest import _token_for +from tests.factories.user import CustomerFactory, UserModelFactory + +pytestmark = pytest.mark.anyio + + +class TestUserSelfRegistrationAuditTrail: + async def test_created_by_stays_null_with_no_authenticated_actor(self, db): + app.dependency_overrides[users_get_db] = lambda: db + try: + with ( + patch("app.api.auth_routes.enqueue_verification_email"), + patch( + "app.api.auth_routes.set_email_verification_token", + AsyncMock(return_value="tok"), + ), + ): + client = TestClient(app) + response = client.post( + "/api/register", + json={ + "email": "new-signup@example.com", + "password": "Correct-Horse-1", + "consent_data_processing": True, + }, + ) + finally: + del app.dependency_overrides[users_get_db] + + assert response.status_code == 200 + user = ( + await db.execute( + select(UserModel).where(UserModel.email == "new-signup@example.com") + ) + ).scalar_one() + assert user.created_by is None + + +class TestAdminInviteAuditTrail: + async def test_created_by_populated_via_real_auth_chain(self, db): + customer = CustomerFactory.build() + admin = UserModelFactory.build(role="admin", customer_id=customer.id) + db.add_all([customer, admin]) + await db.commit() + + app.dependency_overrides[users_get_db] = lambda: db + try: + token = _token_for(str(admin.id), role="admin", customer_id=str(customer.id)) + with patch("app.api.user_routes.enqueue_invite_email"): + client = TestClient(app) + response = client.post( + "/api/users/invite", + headers={"Authorization": f"Bearer {token}"}, + json={"email": "invitee@example.com", "role": "user"}, + ) + finally: + del app.dependency_overrides[users_get_db] + + assert response.status_code == 200 + invited = ( + await db.execute( + select(UserModel).where(UserModel.email == "invitee@example.com") + ) + ).scalar_one() + assert str(invited.created_by) == str(admin.id) + + +class TestCustomerCreationAuditTrail: + async def test_created_by_populated_via_real_auth_chain(self, db): + actor = UserModelFactory.build() + db.add(actor) + await db.commit() + + app.dependency_overrides[users_get_db] = lambda: db + try: + user_id = str(actor.id) + token = _token_for(user_id) + client = TestClient(app) + response = client.post( + "/api/customers/", + headers={"Authorization": f"Bearer {token}"}, + json={ + "name": "New Org", + "country": "GB", + "currency": "GBP", + "is_ngo": True, + "is_donor": False, + }, + ) + finally: + del app.dependency_overrides[users_get_db] + + assert response.status_code == 200 + customer = ( + await db.execute( + select(CustomerModel).where(CustomerModel.id == response.json()["id"]) + ) + ).scalar_one() + assert str(customer.created_by) == user_id + + +class TestCustomerUpdateAuditTrail: + async def test_updated_by_populated_on_update(self, db): + customer = CustomerFactory.build() + admin = UserModelFactory.build(role="admin", customer_id=customer.id) + db.add_all([customer, admin]) + await db.commit() + + app.dependency_overrides[users_get_db] = lambda: db + try: + admin_id = str(admin.id) + token = _token_for(admin_id, role="admin", customer_id=str(customer.id)) + client = TestClient(app) + response = client.patch( + f"/api/customers/{customer.id}", + headers={"Authorization": f"Bearer {token}"}, + json={"name": "Renamed Org"}, + ) + finally: + del app.dependency_overrides[users_get_db] + + assert response.status_code == 200 + await db.refresh(customer) + assert str(customer.updated_by) == admin_id diff --git a/shared/db/audit_mixin.py b/shared/db/audit_mixin.py index 8f37c0aa..98f1ff91 100644 --- a/shared/db/audit_mixin.py +++ b/shared/db/audit_mixin.py @@ -3,16 +3,20 @@ from datetime import datetime, timezone from typing import Optional -from sqlalchemy.orm import Mapped, mapped_column -from sqlalchemy import DateTime, event, inspect +from sqlalchemy.orm import Mapped, declared_attr, mapped_column +from sqlalchemy import DateTime, ForeignKey, event, inspect from shared.db.type_decorators import GUID from shared.security.current_user_context import get_current_user_id class AuditColumnsMixin: - """created_at/updated_at/created_by/updated_by with no primary key — - for models whose PK isn't named/shaped like AuditMixin's `id`.""" + """created_at/updated_at/created_by/updated_by; set __audit_actor_table__ to FK + created_by/updated_by to that same-database table instead of leaving them unconstrained.""" + + # An existing relationship() to this table needs foreign_keys= on both sides once this + # FK exists, or SQLAlchemy raises AmbiguousForeignKeysError. + __audit_actor_table__: Optional[str] = None created_at: Mapped[datetime] = mapped_column( DateTime(timezone=True), default=lambda: datetime.now(timezone.utc), nullable=False @@ -20,9 +24,19 @@ class AuditColumnsMixin: updated_at: Mapped[Optional[datetime]] = mapped_column(DateTime(timezone=True)) - created_by: Mapped[Optional[uuid.UUID]] = mapped_column(GUID(), nullable=True) + @declared_attr + def created_by(cls) -> Mapped[Optional[uuid.UUID]]: + return mapped_column(GUID(), *cls._audit_actor_fk(), nullable=True) + + @declared_attr + def updated_by(cls) -> Mapped[Optional[uuid.UUID]]: + return mapped_column(GUID(), *cls._audit_actor_fk(), nullable=True) - updated_by: Mapped[Optional[uuid.UUID]] = mapped_column(GUID(), nullable=True) + @classmethod + def _audit_actor_fk(cls) -> tuple: + if not cls.__audit_actor_table__: + return () + return (ForeignKey(f"{cls.__audit_actor_table__}.id", ondelete="SET NULL"),) class AuditMixin(AuditColumnsMixin): diff --git a/shared/db/migration_helpers.py b/shared/db/migration_helpers.py new file mode 100644 index 00000000..4f1a754e --- /dev/null +++ b/shared/db/migration_helpers.py @@ -0,0 +1,55 @@ +"""Reusable Alembic ops for adding AuditColumnsMixin's columns to a table.""" + +from __future__ import annotations + +import sqlalchemy as sa +from alembic import op + +from shared.db.type_decorators import GUID + + +def add_audit_columns(table: str, *, actor_table: str | None = None) -> None: + """Add created_at/updated_at/created_by/updated_by, matching AuditColumnsMixin's + typing. Pass actor_table to FK created_by/updated_by to it — same-database only.""" + op.add_column( + table, + sa.Column( + "created_at", + sa.DateTime(timezone=True), + nullable=False, + server_default=sa.func.now(), + ), + ) + op.add_column(table, sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True)) + op.add_column(table, sa.Column("created_by", GUID(), nullable=True)) + op.add_column(table, sa.Column("updated_by", GUID(), nullable=True)) + if actor_table: + op.create_foreign_key( + f"fk_{table}_created_by_{actor_table}", + table, + actor_table, + ["created_by"], + ["id"], + ondelete="SET NULL", + ) + op.create_foreign_key( + f"fk_{table}_updated_by_{actor_table}", + table, + actor_table, + ["updated_by"], + ["id"], + ondelete="SET NULL", + ) + + +def drop_audit_columns(table: str) -> None: + """Drop created_at/updated_at/created_by/updated_by, discovering any FK + constraints on created_by/updated_by via inspection instead of an actor_table arg.""" + inspector = sa.inspect(op.get_bind()) + for fk in inspector.get_foreign_keys(table): + if set(fk["constrained_columns"]) & {"created_by", "updated_by"}: + op.drop_constraint(fk["name"], table, type_="foreignkey") + op.drop_column(table, "updated_by") + op.drop_column(table, "created_by") + op.drop_column(table, "updated_at") + op.drop_column(table, "created_at")