Give every PAM failure a stable, machine-readable code - #8235
Conversation
`TypedResults.BitwardenValidationProblem` hard-coded 400. A failure can be carried by the same body without being bad input — a state conflict is a 409 — and a caller that reads only the status should still learn that much before it reads the codes. Additive: the parameter defaults to 400, so every existing call site is unchanged.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the conversion of the PAM surface from thrown exceptions to Code Review Details
Considered and dismissed:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// </summary> | ||
| Task<AccessRequestResult> SubmitAsync(Guid userId, Guid cipherId, AccessRequestSubmission submission); | ||
| /// <returns> | ||
| /// The submitted request, or the failure that stopped it: <see cref="Errors.AccessRequestNotFound"/> when the |
There was a problem hiding this comment.
🎨 SUGGESTED: The cref names AccessRequestNotFound, but SubmitAsync returns CipherNotFound for this case.
Details and fix
SubmitAccessRequestCommand.SubmitAsync returns new CipherNotFound() when _cipherRepository.GetByIdAsync(cipherId, userId) comes back null — and CipherNotFound is documented in AccessRequestErrors.cs as exactly this case ("The cipher does not exist, or the caller cannot see it").
| /// The submitted request, or the failure that stopped it: <see cref="Errors.AccessRequestNotFound"/> when the | |
| /// The submitted request, or the failure that stopped it: <see cref="Errors.CipherNotFound"/> when the |
No behavioural difference (both are NotFoundError and render the same 404 envelope), but this <returns> block is the contract an SDK author reads to enumerate the error surface, so the wrong record name is worth correcting.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## pam/uat #8235 +/- ##
==========================================
Coverage ? 63.88%
==========================================
Files ? 2431
Lines ? 106107
Branches ? 9571
==========================================
Hits ? 67786
Misses ? 36029
Partials ? 2292 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PAM rejected with `ErrorResponseModel`, which carries a human-readable message and no discriminant. Every failure a client must act on differently was therefore identified by matching the server's English sentence, and the web client had grown two catalogs of eighteen sentences matched with `String.includes()` to do it. Three of those are not failures at all — already-active, already-approved and already-pending mean the requester has what they asked for, and the UI reconciles rather than reporting an error — so rewording one sentence would silently turn a reconciliation into a red toast, with no test failing on either side. PAM now answers with RFC 7807 problem responses in the shape the Admin Console's invite-link confirm endpoint already ships: the stable code is `errors.<property>[].type`, and the property is the request field a form should mark invalid, or `code` when no single field is at fault. The message is unchanged and travels as `detail`. To get there, PAM commands stop throwing for expected failures and return `CommandResult<T>` carrying an `Error` from `Bit.Services.Pam.Errors` — one record per failure, each with the code and the property on it. Handlers render them through the single `PamErrorResult` arm of their `Results<…>` return type, which keeps the success schema in the generated OpenAPI. A coded failure is a 400 unless it is a `ConflictError`, which is a 409. The model-state filter is deliberately left alone. A DataAnnotations rejection means the request never bound cleanly, which leaves a client nothing to act on differently, so those failures carry no code and keep the `ErrorResponseModel` 400 the controllers produced. The guards behind those attributes that do carry a code — a blank name, a non-positive duration — are the commands defending their own contract, not a second user-facing validation layer. Codes are for the semantic failures: state a request conflicts with, a bound it exceeds, a rule that denies it. A `NotFoundError` deliberately keeps the `ErrorResponseModel` 404 that a thrown `NotFoundException` produces — PAM's read paths still throw those, so all its 404s stay identical, and a 404 needs no code because the status already tells it apart. Codes exist for the failures a status cannot separate. The exception filter stays as the safety net for genuinely unexpected throws. Contract: a code is never localized and never reworded once shipped; adding one is additive, since a client treats an unknown code as a generic failure.
0a579ef to
8d44cb9
Compare
1 participant
🎟️ Tracking
Companion to the client-side ask in
docs/pam-server-asks.md(clientspam/uat). Base branch ispam/uat, notmain.📔 Objective
PAM rejected with
ErrorResponseModel— a human-readablemessageand no machine-readable discriminant. Every failure a client must act on differently was therefore identified by matching the server's English sentence. The web client had grown two such catalogs, eighteen sentences matched withString.includes(), written weeks apart by different people, both carrying the same note in their own words: "When the server grows a code, this catalog is the single place to retire."Three of the eighteen are not failures at all.
access_already_active,access_request_already_approvedandaccess_request_already_pendingmean the requester already has what they asked for; the UI reconciles — collapses the form, re-reads the access state, shows an informational toast. Reword one of those sentences today and a reconciliation silently becomes a red error toast, with no test failing on either side. Status codes don't disambiguate: they were all400.This gives every PAM failure a stable code, in RFC 7807 problem responses.
The shape
The same shape the Admin Console's invite-link confirm endpoint already ships (
TypedResults.BitwardenValidationProblem), so clients learn one parser:errors.<property>[].type— stable, never localized, never reworded once shipped.reason,durationSeconds,collections,name, …), named exactly as the request model serializes it. A failure no single field caused — a state conflict, a request shaped for the wrong approval mode, a denial by the governing rule — is keyed bycode.detailis today's message, unchanged."type": "conflict_error"). The code is still what a client switches on; the status is a coarse hint for anything that reads no further. This is a status change for the three reconcile cases, which were 400.How
PAM commands stop throwing for expected failures and return
CommandResult<T>carrying anError— reusing the Admin Console's v2Error/IValidationError/CommandResulttypes rather than inventing a parallel set. One record per failure inBit.Services.Pam.Errors, with the code and the property on it:Handlers render them through the single
PamErrorResultarm of theirResults<…>return type. Declaring the union rather than returning bareIResultis what keeps the success schema in the generated OpenAPI, and so in the SDK's bindings.The model-state filter converted too, with the failing DataAnnotations attribute as the code (
required,range,string_length) — otherwise a client would still need a second parser for anything rejected before a handler ran, and[Required] Reasonon the extension model would shadowextension_reason_requiredentirely.Deliberately unchanged
ErrorResponseModel. PAM's read paths still throwNotFoundException, so all its 404s remain identical however they were reached. A 404 needs no code — the status already tells it apart. Codes exist for the failures a status cannot separate.Scope
All ~35 expected failures across the PAM surface, not just the eighteen the clients match today: submit, activate, cancel, decide, extend, revoke, the access-rule writes, and the three rule-engine denial reasons (which the catalogs never covered, so they were indistinguishable prose). One error style across the whole surface means the leasing UI's next prose-matching catalog never gets written.
TypedResults.BitwardenValidationProblemgained an optionalstatusCode(defaults to 400) for the 409s — additive, every existing call site unchanged.Tests
PamErrorResultTestsasserts the bytes, not the result type — the code and its placement in the body are the promise.PamErrorCatalogTestsreflects over the catalog: codes are snake_case, a code reused across two errors keys the same property, the set of deliberately-shared codes is written down, and the eighteen the clients match today are all present. Two integration tests prove the shape survives the real pipeline end-to-end.427 unit + 31 integration tests pass.
Downstream
The SDK maps codes onto typed error variants (
AccessRequestError::AlreadyPending, …) and both client catalogs are deleted rather than copied into the next client. Those are bitwarden/sdk-internal#1400 and bitwarden/clients#22544, also targetingpam/uat.