Implements tasks T001-T020 from specs/002-saas-integration/tasks.md (Setup, Foundational, and User Story 1 - the P1 MVP: every inbound request is authenticated and trusted before anything happens). User Story 2 (admin onboarding/rotation/revocation) and User Story 3 (rate limiting) are not yet implemented (T021-T032 remain). Schema (prisma/schema.prisma + initial migration): - Replace the placeholder Product model (leftover starter-template scaffolding: code/description/ProductStatus enum) with the real docs/06-database-schema.md shape (externalProductId, supportEnabled, status). - Add ProductIntegration (credential ref, rotation/revocation state, allowed scope, per-integration/per-user rate limits) and CustomerReference models. - Align AuditLog to docs/06's shape (actor/actorType/entityType/ entityId/reason/metadata) -- the placeholder shape had no fields to satisfy this feature's audit requirements. Auth: - HMAC-signed short-lived tokens (issue/verify) with jti-based replay defense via Redis and a bounded clock-skew tolerance. - Credential secrets are AES-256-GCM encrypted at rest (new required INTEGRATION_CREDENTIAL_ENCRYPTION_KEY env var) since no secret manager exists in this stack yet -- see research.md "Credential storage". - New product-integration-auth.plugin.ts Fastify plugin runs the validation order in contracts/inbound-request-contract.md and populates request.reqContext only on full success; every attempt (success or failure) is audit-logged without ever persisting the raw token/credential. Unregistered product and invalid credential return an identical response (FR-010). - New POST /v1/support/requests endpoint exercises the boundary end-to-end (ticket creation itself is a future feature). Also: - Fix docker-compose.test.yml's container_name collisions -- discovered while testing this change concurrently is now covered by an app-level regression test (separate commit). - Fix test:unit to scope to tests/unit only (it was running the entire tests/** glob including integration tests) -- this feature's new integration test makes real Prisma/Redis calls, unlike the prior instantiation-only checks, so the existing glob-scoping gap became actually harmful. - Update Jenkinsfile with the new required credential. Verified: full quality gate (typecheck/lint/format/architecture/ unit tests) passes; all of User Story 1's quickstart scenarios manually verified end-to-end against a live server + Postgres + Redis; the new integration test suite verified against a live database (not run as part of `npm test`, matches existing test:integration convention). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
179 lines
13 KiB
Markdown
179 lines
13 KiB
Markdown
# Phase 0 Research: SaaS Product Integration & Inbound Request Trust
|
|
|
|
## Decision: Signed short-lived token format
|
|
|
|
- **Decision**: HMAC-SHA256-signed token, structured as a compact JWT
|
|
(`header.payload.signature`), signed with the `ProductIntegration`'s own per-product secret
|
|
(never a shared/global secret). Payload carries `productId` (SupportHub's internal id, not the
|
|
raw external one), `tenantId`, `userId`, `iat`, `exp` (short — target 60s, generous enough for
|
|
clock skew tolerance below), and a `jti` (nonce) for replay detection.
|
|
- **Rationale**: User picked "signed short-lived tokens" over OAuth2/mTLS for this plan (lowest
|
|
operational overhead, no token-issuance service to build, straightforward per-product secret
|
|
rotation). JWT is chosen over a bespoke signed-string format purely because Node's ecosystem
|
|
already has well-reviewed JWT libraries and it's a format integrating teams will recognize —
|
|
but the *validation* logic is fully custom (see below), it doesn't defer trust decisions to a
|
|
JWT library's defaults.
|
|
- **Alternatives considered**: OAuth2 client-credentials (rejected per user decision — adds a
|
|
token-issuance/introspection surface this plan doesn't need); mTLS (rejected — heavier
|
|
operationally, reserved as a future per-integration option since `ProductIntegration
|
|
.authMechanism` is already a free-text field in docs/06, not an enum, so nothing here blocks
|
|
adding mTLS support to a specific high-trust integration later without a schema change).
|
|
|
|
## Decision: Replay resistance
|
|
|
|
- **Decision**: Reject a token whose `jti` has been seen before within its own validity window.
|
|
Track seen `jti`s in Redis (already an infrastructure dependency — `infrastructure/cache`) with
|
|
a TTL matching the token's `exp`, so the tracking set never grows unbounded.
|
|
- **Rationale**: A 60-second token expiry alone bounds the replay window but doesn't close it — a
|
|
captured token is still valid for up to 60s. `jti`-tracking closes it to "exactly once."
|
|
- **Alternatives considered**: Expiry-only (no `jti` tracking) — rejected, doesn't satisfy
|
|
spec.md's Edge Cases requirement that a replayed token "MUST be rejected," only that it
|
|
eventually stops being accepted.
|
|
|
|
## Decision: Clock skew tolerance
|
|
|
|
- **Decision**: Accept a token up to 5 seconds past its `exp` and up to 5 seconds before its `iat`
|
|
(both configurable, not hardcoded — Constitution Principle II).
|
|
- **Rationale**: Small enough that it doesn't meaningfully widen the replay window beyond what
|
|
`jti` tracking already closes, generous enough to absorb realistic NTP drift between two
|
|
independently-operated systems.
|
|
- **Alternatives considered**: Zero tolerance — rejected as operationally fragile; large tolerance
|
|
(e.g. 60s) — rejected as unnecessarily widening the token's effective lifetime.
|
|
|
|
## Decision: Credential rotation mechanism
|
|
|
|
- **Decision**: `ProductIntegration` gains a nullable `previousCredentialRef` and
|
|
`previousCredentialExpiresAt` alongside the existing `credentialRef`. On rotation: the current
|
|
`credentialRef` moves to `previousCredentialRef` with `previousCredentialExpiresAt` set to
|
|
"now + transition window," and a new `credentialRef` is issued. Token validation tries the
|
|
current secret first, then the previous one (if `previousCredentialExpiresAt` hasn't passed).
|
|
- **Rationale**: This is what makes rotation zero-downtime (SC-003) without a distributed
|
|
"atomic cutover" — both secrets are simultaneously valid for a bounded window, which directly
|
|
satisfies Constitution Principle VII's concurrency requirement without inventing new
|
|
coordination infrastructure.
|
|
- **Alternatives considered**: Versioned credential list (unbounded history) — rejected as more
|
|
than the spec requires (only *one* prior credential needs to remain valid, per spec.md User
|
|
Story 2's "old and new credential both valid during a transition window").
|
|
|
|
## Decision: Credential storage (no secret manager exists yet in this repo)
|
|
|
|
- **Decision**: `docs/06-database-schema.md` describes `credentialRef` as "a pointer into secret
|
|
manager, never the raw secret" — but no secret-manager integration exists anywhere in this
|
|
codebase or `docs/07-backend-architecture.md`'s stack today, and HMAC signature verification
|
|
needs the actual secret value at verify time, not just a hash of it (unlike a password, which
|
|
only ever needs comparison). Pragmatic resolution for this feature: `credentialRef` /
|
|
`previousCredentialRef` store the secret **encrypted at rest** with AES-256-GCM, using a new
|
|
required env var `INTEGRATION_CREDENTIAL_ENCRYPTION_KEY` (32-byte key, added to
|
|
`src/config/env.ts`'s Zod schema). The plaintext secret is generated once at registration/
|
|
rotation time, returned to the admin caller exactly once (never retrievable again — matches
|
|
spec.md's credential lifecycle), and only its ciphertext is persisted. Decryption happens
|
|
in-process, only inside the token-verification path.
|
|
- **Rationale**: This satisfies the spirit of "never the raw secret in the database" (plaintext
|
|
is never at rest) without inventing a dependency on an external secret-manager service this
|
|
repo doesn't have. It's flagged here explicitly as a placeholder: if/when a real secret manager
|
|
(Vault, AWS Secrets Manager, etc.) is adopted, `credentialRef` becomes a genuine external
|
|
reference and this encryption layer is removed — that migration is out of scope for this
|
|
feature and should be called out as a follow-up, not silently implied as "done."
|
|
- **Alternatives considered**: Storing the secret in plaintext — rejected outright, directly
|
|
contradicts docs/06 and the constitution's secrets-handling governance. Requiring an actual
|
|
secret-manager integration before this feature can ship — rejected as disproportionate scope
|
|
for what Phase 2 needs; no other part of the roadmap currently requires one either.
|
|
|
|
## Decision: Rate limiting design (per-integration and per-user)
|
|
|
|
- **Decision**: Apply `@fastify/rate-limit` at the route level (not the single global registration
|
|
currently in `rate-limit.plugin.ts`) for the inbound SaaS-facing route, with a custom
|
|
`keyGenerator` returning `` `integration:${productIntegrationId}` `` for the integration-level
|
|
limit and a second, stricter per-route rate-limit instance keyed by
|
|
`` `integration:${productIntegrationId}:user:${externalUserId}` `` for the per-user limit. Both
|
|
read their max/window from `ProductIntegration`-linked configuration (new fields, see
|
|
data-model.md), not a hardcoded value.
|
|
- **Rationale**: The existing global `rate-limit.plugin.ts` registration (`max: 1000, timeWindow:
|
|
'1 minute'`, ungated) stays as a blanket floor for the whole API; it doesn't get removed, just
|
|
supplemented — this feature's per-integration/per-user limits are strictly tighter and specific
|
|
to the inbound SaaS route. `keyGenerator` needs the validated `productIntegrationId`/
|
|
`externalUserId`, so the rate-limit check runs in the route's handler chain *after* the
|
|
`product-integration-auth` plugin's preHandler populates `request.reqContext`.
|
|
- **Alternatives considered**: A single global per-IP limit — rejected, doesn't satisfy FR-009's
|
|
"per integration and independently per end user" requirement; a product's shared outbound IP
|
|
would incorrectly throttle every user behind it together.
|
|
|
|
## Decision: Unknown-field rejection (FR-008)
|
|
|
|
- **Decision**: The inbound Zod schema uses `.strict()` (rejects any key not explicitly defined),
|
|
not the Zod default of silently stripping unknown keys.
|
|
- **Rationale**: FR-008 requires the *entire request* rejected on an unknown field, not a
|
|
best-effort parse — `.strict()` is exactly this behavior in Zod; the default `.parse()`
|
|
behavior (strip unknown keys silently) would violate FR-008.
|
|
- **Alternatives considered**: None — this is a direct, unambiguous mapping from requirement to
|
|
Zod API.
|
|
|
|
## Decision: Error response shape without leaking registration status (FR-010)
|
|
|
|
- **Decision**: "Unregistered product" and "invalid credential for a registered product" return
|
|
the *same* generic `401 INVALID_INTEGRATION_CREDENTIAL` response body and status code. "Product
|
|
suspended" returns a distinguishable `403 PRODUCT_INTEGRATION_SUSPENDED` (this one is safe to
|
|
distinguish — a suspended product's own registered caller already knows it's registered).
|
|
"Out-of-scope request" (valid credential, but tenant/user outside `allowedScope`) returns
|
|
`403 REQUEST_OUT_OF_SCOPE`.
|
|
- **Rationale**: This satisfies both FR-010 requirements at once: distinguishable where doing so
|
|
is safe (suspended vs. scope), identical where distinguishing would leak whether an arbitrary
|
|
product ID is registered at all (invalid credential vs. unregistered product).
|
|
- **Alternatives considered**: Fully distinguishing all four cases — rejected, directly
|
|
contradicts FR-010's "without leaking whether an unregistered product ID exists."
|
|
|
|
## Decision: Reconciling the placeholder Prisma schema
|
|
|
|
- **Decision**: The current `Product` model in `prisma/schema.prisma` (`code`, `name`,
|
|
`description`, `status: ProductStatus` enum) is starter-template scaffolding, not the real
|
|
domain model — it doesn't match `docs/06-database-schema.md`'s `Product` shape at all
|
|
(`externalProductId`, `supportEnabled`, `status: String`). This feature replaces it with the
|
|
doc 06 shape. New models (`ProductIntegration`, `CustomerReference`) use `cuid()` ids matching
|
|
doc 06 exactly. The existing `Category`, `User`, and `AuditLog` placeholder models are left
|
|
alone (out of scope for this feature — `Category`'s real shape belongs to whichever future
|
|
feature builds the catalog domain properly; `User`/`AuditLog` aren't touched by this feature's
|
|
requirements beyond *reusing* `AuditLog` for FR-007).
|
|
- **Rationale**: Phase 2 is explicitly where `docs/10-implementation-roadmap.md` places
|
|
"Product/ProductIntegration models" — this is the correct feature to fix `Product`, not a
|
|
scope-creep addition. Leaving `Category`/`User` alone keeps the change bounded to what this
|
|
feature actually needs.
|
|
- **Alternatives considered**: Adding `ProductIntegration` pointing at the old placeholder
|
|
`Product` shape and deferring the `Product` fix — rejected, would mean building
|
|
`ProductIntegration.product` against a model with no `externalProductId` to validate inbound
|
|
requests against, defeating the feature's own purpose.
|
|
|
|
## Decision: Aligning `AuditLog` to doc 06's shape (discovered during implementation)
|
|
|
|
- **Decision**: The placeholder `AuditLog` model (`userId`, `action`, `resource`, `payload`) is
|
|
replaced with `docs/06-database-schema.md`'s real shape (`actor`, `actorType`, `action`,
|
|
`entityType`, `entityId`, `oldValue`, `newValue`, `reason`, `metadata`, `createdAt`) — dropping
|
|
its foreign key to `User`. `actor` becomes a plain string identifier (a `ProductIntegration.id`,
|
|
an agent id, `"system"`, etc.), not a relation.
|
|
- **Rationale**: Originally planned to leave `AuditLog` untouched and just "reuse" it (see the
|
|
"Where `ProductIntegration` lifecycle endpoints live" decision below and plan.md's Constitution
|
|
Check), but the placeholder shape has no `entityType`/`entityId`/`reason` fields at all — FR-007
|
|
("record which integration/credential was involved," "reason: invalid_credential | suspended |
|
|
...") literally cannot be satisfied by the old shape. This isn't scope creep into unrelated
|
|
future work — it's a direct, minimal prerequisite for this feature's own FR-007, discovered
|
|
while wiring up data-model.md's `AuditLog` field mapping against the real schema. The dropped
|
|
`User` relation also better matches Constitution Principle I: an audit `actor` shouldn't require
|
|
a local `User` row to exist, since most actors (product integrations, external users via
|
|
`externalUserId`, "ai") never have one.
|
|
- **Alternatives considered**: Keep the placeholder shape and encode `entityType`/`entityId`/
|
|
`reason` inside the existing free-text `resource` field and `payload` JSON — rejected as exactly
|
|
the kind of unstructured workaround Constitution Principle VI's audit requirement exists to
|
|
prevent; it would make audit rows unqueryable by entity without parsing `payload` first.
|
|
|
|
## Decision: Where `ProductIntegration` lifecycle endpoints live
|
|
|
|
- **Decision**: Extend the existing `src/modules/catalog/products/` module (already scaffolded
|
|
with controller/service/repository/routes/schema/mapper/types) with integration lifecycle
|
|
operations, rather than creating a new top-level module.
|
|
- **Rationale**: `docs/07-backend-architecture.md`'s module list has no separate
|
|
"product-integrations" module; `docs/06-database-schema.md` groups `ProductIntegration` under
|
|
the same "Domain: Integration / Catalog" heading as `Product`, and it's a 1:1 relation.
|
|
- **Alternatives considered**: A new `src/modules/platform/integrations/` addition — rejected;
|
|
that module already exists for outbound webhook delivery (docs/11 gap A2, a different, future
|
|
concern), and conflating inbound-trust management with outbound-webhook delivery in one module
|
|
would blur a module boundary the constitution requires to stay clear (Principle III).
|