feat(npm): remote-repository update with credential clearing (S15 Step 7, part 3/3)

David Fernandezrequested to merge
10io/s15-step-7-part3of3 into main

📚 Stacked MRs

This change is split into 3 stacked MRs to keep each within the review size limit (≈800 reviewable LoC). Each part targets the previous one (part 1 targets main); review and merge proceed bottom-up.

📦 What this MR does

Part 3 of 3 of npm remote plan — Step 7: repo service layer — credentials + cache-validity:

Adds UpdateNpmRemoteRepository, the patch update beneath the management layer's future update endpoint:

  • Credential clearing on URL change (S13 rule, in-store): when the supplied URL normalizes to a different value than the stored one and no new token accompanies it, tmp_plaintext_auth_token is zeroed in the same UPDATE statement as the url write and the update reports remote.CredentialClear in its result — an old bearer never survives to a re-pointed upstream, and a caller renders credentials_cleared from that action. A new token always replaces; a respelled URL (host case, default port, trailing slash) is not a change and cannot clear credentials spuriously (URLs are stored normalized).
  • Transaction requirement: the method rejects any handle that is not a *sql.Tx. Its FOR UPDATE read-compare-write is only serialized while the caller's transaction holds the lock; on a pool handle the lock dies with the SELECT's autocommit, leaving a race that could silently destroy a concurrently rotated credential.
  • Row gates: the lock's parent gate carries the read paths' full predicate set (npm format, remote kind, soft-delete, namespace isolation), so the update path can never mutate a binding the reads report as ErrNotFound.
  • Result shape: the update returns UpdateNpmRemoteRepositoryResult{Changed bool, CredentialAction remote.CredentialAction} rather than a bare cleared bool. A patch that resolved entirely to the stored state (a respelled url and nothing else) reports Changed: false with a nil action — a success that writes nothing, no longer indistinguishable from a url change that cleared nothing — and a credential write reports the set/replace/clear action the audit payload already defines (remote.CredentialNone for a write that touched no credential). Changed reports whether the UPDATE executed; only url is compared against stored state, so a same-value token or cache-window write still reports true.
  • Credential-write audit events (AC 3 audit clause): every credential write the store performs emits one remote.CredentialWriteEvent through a remote.AuditSink the constructor now takes — set or replace for a supplied token (told apart by a credential-presence bit read under the same FOR UPDATE lock, so the distinction cannot race a concurrent token write), clear for the url-change zeroing. The event carries the action and the namespace and parent-repository keys, never a credential value, and the token plaintext still never leaves the database on this path: the presence bit is computed in SQL. The create path emits a set when a token is supplied.
  • Create-only cache validity: updates write cache_validity_hours only when an explicit value is supplied — the create-time resolution is never re-run, so a stored value survives a url re-point unless the operator overrides it. The plan's Step 7 wording is corrected to match (drift correction, ships in the implementing MR per the workflow's carve-out).

✅ Spec coverage

🎯 Acceptance criteria

Criterion Tests
S15 Credentials and health AC 3 — url update zeroes the token in the same transaction unless a new token is supplied, emits an audit event, surfaces credentials_cleared TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository (integration: clear, replace, respelled no-op, rollback atomicity, FOR UPDATE serialization on both the url comparison and the credential-presence bit), TestNpmRemoteRepositoryStore_CredentialWriteAudit (integration: clear, clear on a token-less row, set, replace, empty stored token counts as absent, no event on a non-credential write, a respelled no-op, a refused write, create with and without a token), TestUpdateNpmRemoteRepositoryStmt (unit: the SET list and the reported action asserted together per patch shape)
Create-only cache validity — updates never re-run the create-time resolution TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository (integration: cache validity survives a url change; an explicit value writes)

⚠️ Error cases

Case Tests
Row and parent gates: missing binding, soft-deleted / maven / docker / oci / hosted / virtual parent, cross-namespace TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository_RowGates (integration; each seeded case also asserts the stored token survived)
Argument guards: nil context, nil handle, zero keys, empty patch, empty and over-cap token, malformed url TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository_ArgumentGuards (unit), TestNewNpmRemoteRepositoryStore_NilAuditSinkPanics (unit)
Schema refusal classified to ErrNpmRemoteRepositoryCheckViolation, and a transient failure never misreported as ErrNotFound TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository_RowGates (integration)

🔒 Security considerations

Consideration Tests
A refused write never leaks the failing row: the driver error is replaced, not wrapped, on classified and unclassified paths alike TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository_RowGates (integration: requireNoPgErrorInChain on the CHECK refusal), TestNpmRemoteRepositoryWriteError (unit: an unclassified PgError carrying a token in Detail is dropped from the chain; a cancellation stays matchable)
Credentials scoped to the old host never reach a new one, and the clear cannot be split from the url write TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository (integration: same-transaction clearing, rollback atomicity, lock serialization)
The stored plaintext never leaves the database on the update path — the locked read projects a presence bit, not the token TestNpmRemoteRepositoryStore_UpdateNpmRemoteRepository (integration: set versus replace decided from the presence bit), TestNpmRemoteRepositoryStore_LockNpmRemoteRepository_PrunesToOnePartition (integration: the statement the store executes)

🔍 Notes for reviewers

  • AC 3 audit clause is satisfied here: an earlier revision deferred the audit event to the management layer on the premise that the store never sees the acting principal. remote.CredentialWriteEvent carries no principal — S13 emits the payload, S17 adds the actor — so the store emits it. Converging this emission onto the tx-threaded remote.CredentialRepo seam and remote.CredentialManager, sequenced with S17, is tracked in #487. The emission sits inside the caller's transaction, so a caller that rolls back afterwards has already recorded the event — the same property credentials_cleared has, and part of what work item 487 converges. A url change on a token-less row still emits clear: the event describes the write performed (the SET NULL is unconditional), matching remote.CredentialManager.ClearCredentials. The plan's AC 1 (bearer-attach, Step 9) and AC 4 (probe, S13/S17) carve-outs are unaffected.
  • Commit sequence: the two commits that introduce the feature (4784e3f4 and ced25d90) each carry tests and implementation together, rather than the test-first pair the workflow asks for. The other commits on the branch are clean test(/refactor(/docs( splits. Noted rather than repaired — fixing it means rewriting pushed history that review replies already cite by SHA.
  • No e2e scenario catalog update: datastore service layer beneath the not-yet-shipped management CRUD; no HTTP surface changes.

Related to #287

:bricks: Stacked MRs (review/merge bottom-up)

Edited by David Fernandez

Merge request reports