feat: store remote session validation verdicts - #6293
Conversation
|
|
Running ultrareview automatically — This PR adds concurrency-sensitive verdict persistence with CAS on updated_at and 'unknown never overwrites valid' logic, clears verdicts on delete/refresh, and modifies core session state and token resolution paths—high-risk for subtle race conditions.. I'll post findings when complete. |
There was a problem hiding this comment.
Ultrareview completed in 5m 29s
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
bflad
left a comment
There was a problem hiding this comment.
Approving. Thanks for moving ValidationOutcome into remotesessions. Taking a string on Validation.Record to keep the metrics package dependency-free is the right call over inverting that import.
I checked the invariants this rests on rather than just reading the diff, so recording what I verified. The CAS token is accurate: resolveUpstreamToken rebinds sess through validateAndRefresh, so RemoteSessionUpdatedAt is the post-refresh value, TouchRemoteSessionLastUsed writes only last_used_at, and there are no triggers on the table. SetRemoteSessionValidation deliberately leaving updated_at alone is what keeps a verdict write from colliding with the refresh CAS.
The invalidation rule also has no gaps. Every query that writes access_token_encrypted clears the verdict: UpsertRemoteSession in its DO UPDATE SET, UpdateRemoteSessionTokensIfUnchanged, and the soft-delete paths. Taking probedAt before the probe runs rather than at write time is what makes the last_validated_at <= @last_validated_at guard mean "started later wins" instead of "finished later wins", which is the behavior you want for overlapping probes.
Two minor things.
ValidationOutcomeRevoked cannot be stored. The type is documented as the closed set remote_sessions.validation_status stores, but RecordRemoteSessionValidation refuses revoked and nothing else writes the column. In the PR that defines the storage layer, a member of the declared closed set that the writer rejects reads as a contradiction. I would drop it and reintroduce it with AIM-261, when something can actually write it.
The user_session_issuers binding is absent. The sibling queries at lines 584, 602 and 653 pair this exact client tenant clause with a usi binding, and this one omits it. For a platform client, where both project_id and organization_id are NULL, the tenant clause admits any caller, so the effective guard becomes the (id, subject_urn, remote_session_client_id, updated_at) tuple. I think that is fine, since the caller reached the row through an issuer-scoped read and the field is display-only, and TestRecordRemoteSessionValidation_PlatformClient shows the behavior is deliberate. Worth a line in the query comment so the divergence from its siblings reads as a decision rather than an oversight.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2558772 to
e4f17da
Compare
AIM-204, part 3 of 5, stacked on #6292.
remotesessions.RecordRemoteSessionValidation/SetRemoteSessionValidation: writeslast_validated_at,validation_status,validation_reason(columns from feat: capture upstream identity from ID tokens on remote session grants #6104) with a compare-and-swap onupdated_at, tenant-bound through the client row;unknownnever overwrites a storedvalid, enforced in the UPDATE.RemoteSessionStategains the row id, CAS token and verdict fields;UpstreamTokengains theRemoteSessionID/RemoteSessionUpdatedAtCAS pair.ValidationOutcomelives inremotesessionsnext toRemoteSessionValidation;remotesessionsalready importsremotesessionmetrics, so the metric records the outcome as a string rather than importing it back.gram.remote_session.validationby outcome and issuer;gram.remote_session.idon refresh-failure logs.Summary by cubic
Stores live validation verdicts for remote session credentials so the consent page can show whether a stored token still works upstream. Part 3 of AIM-204; the probe that produces these verdicts lands in part 4.
RecordRemoteSessionValidation, a tenant-bound writer with compare-and-swap onupdated_at; the client row's project or organization scopes the write.unknownnever overwrites a storedvalid, enforced in the UPDATE.RemoteSessionStateandUpstreamTokencarry the row ID and CAS token.gram.remote_session.validationby outcome and issuer.gram.remote_session.idattribute to refresh-failure logs.Written for commit e4f17da. Summary will update on new commits.