fix(pam): share approval-method label logic between the access-rules table and the collection callout - #22565
Open
maxkpower wants to merge 37 commits into
Open
fix(pam): share approval-method label logic between the access-rules table and the collection callout#22565maxkpower wants to merge 37 commits into
maxkpower wants to merge 37 commits into
Conversation
…pivot)
Route PAM-gated ("partial") ciphers through the SDK and surface them only in
the web vault, where they render read-only with a "Controlled access" badge. A
partial cipher ships a reduced `partialData` envelope in place of its full
payload; the SDK decrypts it into a view marked `partial`. Everywhere outside
the web vault list, partials are excluded — they never reach autofill, export,
reports, Fido2, key rotation, or the CLI, and are never modifiable.
Model plumbing: `CipherResponse.partialData` flows verbatim through `CipherData`
to domain `Cipher.partialData`, round-tripping losslessly through
`toSdkCipher`/`fromSdkCipher`.
Excluded-by-default streams: `getAllDecrypted()` and `cipherViews$` /
`cipherListViews$` exclude partials, derived from private partials-inclusive
sources so decryption stays single-pass. The web list consumes the opt-in
`cipherListViewsWithPartials$`. `CipherViewLikeUtils.isPartial` centralizes the
flag read.
Read-only rendering + badge seam: a gated cipher opens read-only in the
vault-item dialog and cipher-view; a host-provided "Controlled access" badge
column (`VAULT_ROW_LEASE_BADGE`) appears only when the `Pam` flag is on, a
`usePam` org is in view, and a host provides the badge.
Non-modifiable in web: gated rows are non-selectable and expose no modify menu
actions; both bulk-action paths filter them defensively; encrypted export
excludes them. The web filter tree includes partials so a type/folder present
only as gated rows still surfaces.
Inert until a PAM provider binds the badge seam and the server emits
`partialData`; with PAM off, every added filter is a no-op.
Add an end-user "My access" page (commercial web) where a member views and manages their own PAM access, a faithful port of the pam/poc "My requests / active leases" view, backed by the Rust SDK's leasing sub-clients (commercial().pam().access_requests() / .leases()). - SDK-backed service layer mirroring AccessRulesSdkService: AccessRequestSdkService / AccessLeaseSdkService (+ impls), wired in provide-pam. - my-access page: Active leases / Pending / History sections with favicons, collection subtext, live countdown, Extended badges, resolver + comment columns, window-gated Start/Cancel, End-access confirm; optimistic cancel/end with rollback; name resolution from local vault state. - Request-detail deep link at /pam/requests/:id (decision log, lease section); requester-facing actions only. - Additive commercial-only user-layout /pam route + feature-flag-gated nav slot. History "ended by you" vs "revoked" is derived from the decision log, since the real AccessLeaseStatus has no cancelled value. Extend stays on the (future) in-vault banner, not this page. Consumes the SDK surface in bitwarden/sdk-internal#1310; requires that published and the sdk-internal versions bumped before CI compiles. Second PR of the PAM leasing SDK-first migration (stacked on pam/access-rules-ui).
- Gate the user nav slot on membership in a PAM-enabled (usePam) org, not the feature flag alone - Wrap the SDK isLeasingError guard behind an injectable LeasingErrorService so the type-only barrel stays wasm-free and callers can mock it - Add a remainingTime pipe (mirroring relativeTime) and use it in templates; drop the component label helpers/computeds - Use `implements` rather than `extends` for the SDK service contracts - Trim doc and routing comments per review
Restructure /pam into the tabbed "Access requests" shell (Approvals /
My requests / History) with live berry counts, sharing a single
MyAccessService load across tabs. The My requests tab groups the caller's
Pending / Extension requests / Currently checked out access into accordions
behind a search + collection filter, keeping the start/cancel/end lifecycle
actions and live countdown. History is read-only; Approvals is a placeholder
until an approver-scoped SDK read exists.
Adapt the leasing decision-log to the SDK's nested AccessRequestDecisionView
(decider: "automatic" | { human: AccessApprover }) at sdk-internal
0.2.0-main.956, replacing the flattened deciderKind/id/name/email shape and
normalizing the branded CipherId in the request-detail template.
Address review feedback: - Rename pam/my-access to pam/access-requests (the page is "Access requests") and update the lazy route import + nav-slot doc. - Make the user nav-slot bit-nav-item self-closing.
Replace the two-state, icon-only vault-row lease badge with the unified access-state badge from the "Unifying access-state badges" spec (Figma 88-1699): a bit-badge pill with per-state colour + icon + copy, mutually exclusive precedence, and the 5-minute danger "Ending soon" escalation, rendered by a shared AccessStateBadgeComponent (reusable by the modal and Requests page). - AccessStateBadgeComponent + cipherAccessBadgeState resolver + 7 i18n keys - AccessRequestSdkService.getCipherAccessState (binds the SDK's commercial().pam().access_requests().cipher_access_state) - provide-pam binds VAULT_ROW_LEASE_BADGE to the component "Unavailable / held by another user" is modelled but not yet produced: CipherAccessStateView is caller-scoped and never reports another user's lease (needs an SDK + server signal). (cherry picked from commit 7ac3da6)
…rnal The partial-cipher pivot maps `Cipher.partialData` and `CipherView.partial` across the SDK boundary, but no published `sdk-internal` (or `commercial-sdk-internal`) declares either field — latest is 0.2.0-main.968 — so the three mapper sites failed to type-check. Declare local intersection aliases over `SdkCipher` / `SdkCipherView` that add the fields as optional members. Plain SDK objects stay assignable, so none of the callers (`CipherRecordMapper`, `cipher-sdk.service`, `default-cipher-encryption.service`) change. This restores the type-check only. The SDK still drops both fields at runtime, so a gated row's marker does not survive a round trip through `CipherRecordMapper`; `CipherView.partial` continues to be populated from the domain path (`partialData != null`). Collapse both aliases once the Rust side ships the fields.
The access-requests shell was mounted at `path: ""` with `canActivate: [deepLinkGuard(), authGuard]`. Because AppRoutingModule is registered before OssRoutingModule, that empty-path route won the match for `/`, so authGuard ran instead of the root redirectGuard in oss-routing.module. For an anonymous user that produced an infinite navigation loop: authGuard sends `authBlocked`, AppComponent handles it with `router.navigate(["/"])`, and the same route matches again. The router never committed a navigation (~100 transitions/second, main thread pegged), so the web vault never rendered past the static loading spinner in index.html. Mount the shell on "pam" instead and fold the feature-flag guard onto the same route. Guard order (deep link, auth, feature flag) and the covered URLs (/pam, /pam/**) are unchanged.
Rebuilds the PAM surfaces that existed on the pam/poc-rebased branch but were
missing here. The poc was reference only: it ran on direct HTTP against an older
main, so nothing was portable as written and every file is new work against the
Rust SDK (client.commercial().pam()).
Request access. A gated cipher offered no way to ask for access, because
CIPHER_VIEW_BANNER was declared and rendered by libs/vault but never provided.
Binds it to a new CipherViewBannerComponent showing one of four states off
cipher_access_state(): active lease (countdown, extend, end), approved request
(start, withdraw), pending request (withdraw), or nothing in play (request
access, folding out an inline form whose shape comes from pre_check —
a duration on the automatic path, a window plus a reason on the human one).
Extends AccessRequestSdkService with preCheck()/submitAccessRequest(), which the
pinned SDK already exposed but nothing called.
A submit the server rejects because the caller ALREADY holds what they asked for
is not a failure: classifyRequestAccessError recognises the AlreadyActive,
AlreadyApproved and AlreadyPending cases, collapses the fold-out, and lets the
re-read drive the banner into the state that exists.
Reveal in place. CipherView.leaseGated was declared with a comment naming a
producer that did not exist, so an open item stayed partial even after access
started. Ports the GATED_CIPHER_RELOADER seam, which exchanges a plain
Observable rather than a component class so libs/vault needs no dependency on
the implementing library. The dialog uses concatMap, not switchMap: switchMap
would drop an in-flight reveal's subscription while its promise kept running, so
a reveal could land after a re-lock and leave secrets on screen. Re-lock also
unmounts the form, whose own state still holds the decrypted cipher, and
originalCipher moves with the view or a later save would write the partial
copy's blanks over the fields the server suppressed.
The full cipher is read through the STANDARD single-cipher endpoint, not the
poc's GET /leases/ciphers/{id}/cipher, which is deprecated and scheduled for
removal. That is the point of the partial-cipher pivot: the server already
decides per caller whether a payload is restricted or complete. The result is
never written to the local cipher cache, so the cache stays partial and a lapsed
lease cannot leave decryptable secrets in local state.
Live refresh. Every surface was read-once, so an approver's decision only
appeared on reload. Adds NotificationType.RefreshAccessRequest = 29 (the value
the server sends; 28 is reserved there, so this is deliberately not the next free
slot locally) and AccessEventService, which filters the notification stream to it.
It reads ServerNotificationsService.notifications$ directly — deprecated in
favour of a case in processNotification, but adding one would put a commercial
PAM concern in libs/common, and DefaultTaskService filters the same stream for
RefreshSecurityTasks. AccessRefreshService merges that push with this client's
own mutations and fans it out per cipher, so a local change and a remote one
drive the UI through one path; it is needed at all because cipher_access_state()
is a one-shot read. concatMap throughout, so two pushes cannot interleave their
loads and leave several subjects describing different moments.
Approver inbox. The Approvals tab was a 21-line placeholder. Adds
ApprovalApiService, the module's one HTTP-backed contract, for exactly three
routes (inbox, history, decision) that the server implements but the SDK does not
expose; bound in provide-pam.ts so the eventual swap is one provider line. The
exception is narrower than the poc's: approver-side revoke and cancel-approval go
through the SDK (leases().end(), access_requests().cancel()), which reach the very
endpoints the poc called by hand.
AccessRequestDetailsResponse mirrors AccessRequestView field for field, so the
existing row builders all work on it unchanged instead of a parallel stack —
which is why this needed 40 new i18n keys rather than the ~70 estimated. Wire
statuses are normalised on the way in: cancelled to canceled, and a lease
cancelled collapses to revoked since the SDK has no such value and the decision
log already distinguishes the two.
The Approvals tab is hidden entirely from a member with no approval privileges
rather than shown empty, and canViewApprovalsGuard redirects the deep link to
match; an empty inbox reads as "nothing to do today", which is the wrong message
for someone who will never have anything there. History gains a scope toggle
rather than one merged table, because merging "a request I raised" with "a
request I decided" would put rows with different available actions in the same
columns, so a row's capabilities would depend on something invisible.
Collection callout. Adds COLLECTION_ACCESS_RULE_CALLOUT so the collection edit
dialog names the rules gating its items — otherwise the member list reads as the
whole story. Informational rather than a gate, so a failed read hides it. Names
every governing rule, not just the first as the poc did: a collection can be
governed by more than one, and showing one would understate the gating an admin
is about to change access to.
Nav badge. PamNavBadgeService as the OSS seam plus an SDK-backed implementation.
The poc's badge counted the approver inbox; the user nav slot links to the
caller's OWN page, so this counts their own actionable requests instead. The
approver-facing count is the Approvals tab berry.
Specs and docs. Adds the specs this module lacked (request-detail service and
route component, name resolver, shared decision builders). Rewrites the
per-directory CLAUDE.md, which described only the access-rules admin UI, and
corrects pam.allium where it had drifted from the code.
Out of scope and not built: governance dashboard, kill switch, org-wide audit
log, the bit-pam package layout, the poc's duplicate access-rule stack,
leased-cipher-fetcher, cipher-open-gate, cipher-lease-badge, and the poc's
FeatureFlag.Pam = TRUE demo hack. No changes to sdk-internal or the server repo.
The three approver routes could not be exercised end to end: their server
handlers are NotImplementedException scaffolds today, so they return HTTP 500.
Built against the documented contract.
…cessor The Access Requests lists rendered raw uuids instead of item names. Every id those lists name is a PAM-gated cipher, but AccessNameResolverService read them via CipherService.getAllDecryptedForIds, which is built on cipherViews$ — the stream that strips partials (added by the partial-cipher pivot, 5060ea7). So the lookup resolved nothing for exactly the rows it exists to name, and the templates fell back to row.cipherName ?? row.cipherId. Favicons were missing for the same reason: cipherById was empty too. Add getAllDecryptedForIdsIncludingPartials, the id-scoped counterpart of cipherListViewsWithPartials$, and read it from the resolver. Fixes all three tabs and /pam/requests/:id, which share the resolver. Verified against the UAT stack: the gated cipher is present in the sync with partialData set and decrypts to "AWS Root Account" via cipherViewsWithPartials$, while cipherViews$ dropped it (24 encrypted, 21 views, 0 decryption failures).
* feat(pam): serve the approver surface from the SDK The approver's inbox, history, and decision write were the module's one raw-HTTP seam, and deliberately temporary: the server implemented the three routes but the SDK exposed only requester-scoped operations, so there was no call to make. The SDK now exposes them, so the exception goes away. `ApprovalSdkService` joins the other three abstract contracts, backed by `ApprovalsSdkService` over `commercial().pam().approvals()`. Deletes ~730 lines: the HTTP service and its default, the decision request class, and the response classes. All of that was PascalCase wire parsing, status spelling normalisation, verdict int/string translation and branded-id widening - work the SDK's `TryFrom<AccessRequestDetailsResponseModel>` now does once, in one place, for every client. `ApproverInboxService` keeps its shape; only the injected service and the decide argument change, since the SDK's `AccessDecisionRequest` is a plain type rather than a class that translates verdicts. `inbox-request-filter.ts` now reads `expiredAt` off `AccessRequestView` itself, which the SDK carries as of this change. Its rule stays here for now: it needs a clock, and Tsify views cross the wasm boundary as plain objects, so an SDK-side method would not be callable from TypeScript. Requires bitwarden/sdk-internal#1384. Verified against a local WASM build of that branch; the version pin still has to be bumped once it publishes. * fix(pam): migrate off the SDK's removed LeasingError and "activated" status sdk-internal's leasing refactor replaced `LeasingError` and its generated `isLeasingError` guard with one error per client, and dropped `Activated` from `AccessRequestStatus` — activation no longer changes the status, it is observed through `producedLeaseId`. `abstractions/access-lease.ts` re-exports `AccessRequestError`, `ApprovalError` and `AccessLeaseError` and unions them as `LeasingError`, so the single `LeasingErrorService` seam still spans every leasing failure. The UI only reads `variant` — `"Api"` carries the server's message — so splitting the seam per client would buy nothing. Every site that relied on `"activated"` being distinct from `"approved"` now tests `producedLeaseId` instead. Only three of them named the status and failed to compile; the rest changed meaning silently, and left as-is an activated grant would read as a plain approval — the nav badge counting it, the Pending list showing it again with Start and Cancel offered, and History dropping it altogether. Also drops the CLAUDE.md claim that `AccessLeaseStatus` has no `cancelled` value: it now carries a distinct `canceled` alongside `revoked`, and `AccessLeaseView` carries `termination`. `historyDisplayStatus` consumes neither yet, so it still derives a self-end from the decision log.
`AccessLeaseStatus` carries a distinct `canceled` for a lease its requester ended, as against `revoked` for one an operator ended, so `historyDisplayStatus` now reads the label straight off `producedLeaseStatus`. That retires the decision-log scan it relied on while both collapsed into `revoked` — a human "deny" whose decider was the requester themself — along with the `decisions` and `requesterId` fields it needed on the way in. `endLease` optimistically wrote `revoked` when the holder ended their own lease, the closest value available at the time, leaving the row reading "Revoked" until a reload corrected it from the decision log. It now writes `canceled`, so the optimistic label already matches what the server will confirm. An approver's `revokeLease` keeps writing `revoked`. The audit trail in `access-request-route.component.ts` still attributes each deny decision to the requester or to an operator from the log. That is a per-decision label, which a single lease-wide status cannot replace.
`inject()` cannot infer an `InjectionToken`'s type argument, because the token only
uses it phantomly, so `leaseBadge` came out as `unknown`. The template's `@if` then
narrowed that to `{}`, which `NgComponentOutlet` rejects against its `Type<any>`
input. Annotate the field as `vault-cipher-row` already does for the same token.
`SafeInjectionToken`'s private tag guards against mismatched providers but does not
reach `inject()`, since the parameter is matched against the base `InjectionToken`.
Only the Angular compiler catches this. `test:types` runs per-library `tsc`, which
never type-checks templates, so it passes either way.
Brings back the governance audit view for the trail the server now records: the organization's access-audit log at pam/audit, with free-text and event-kind filters over the fetched window. Placed as an org-scoped route beside Access rules and guarded by canAccessEventLogs, mirroring the endpoint's own authorization. The nav group previously gated wholly on canManageAccessRules; each item now gates itself and the group appears when either is reachable, so neither permission rides in on the other's. Cipher and collection names are resolved from local vault state rather than the response's denormalized copies, which are encrypted EncStrings this client can only decrypt for items already in the viewer's own vault. An auditor who never held the item sees no item name, by design. No drill-down, unlike the poc it comes from. The request-detail page is authorized for the request's requester or a managing approver, which is a different permission from the one that opens this trail -- so a link there would send an auditor holding only canAccessEventLogs into a 404. The poc's trail was collection-scoped for approvers, where the link always resolved. The trail is read over HTTP, which leaves AuditApiService as the module's only exception to the SDK rule now that the approver surface has moved to the SDK: the pinned commercial SDK's pam() client still has no audit surface at all. It is bound behind an abstraction so the swap is one provider line once the SDK gains one; access-audit/README.md records the module shape that would take. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e SDK Three pieces of PAM logic the SDK already owns, which this module was still keeping its own copy of. `cipherAccessBadgeState` no longer ranks active lease -> ready -> pending -> privileged. It reads `CipherAccessStateView.badgeState`, where the SDK applies that precedence once at conversion, and only adapts the shape onto the presentation model (a `kind` discriminant, a parsed `Date`). The local union keeps `unavailable` and `expired`, which the SDK deliberately does not model because the per-cipher state response is caller-scoped. `AccessRuleError` is now the SDK's exported type rather than a hand-written mirror. The mirror had drifted: it carried `BadRequest` and `NotFound` variants the SDK never produced, so `isAccessRuleNotFound` could never fire and the rule-edit page's two "rule not found" branches were unreachable. The guard stays local and structural, because the SDK's own `isAccessRuleError` is a runtime wasm import while `abstractions/` stays type-only so jest never resolves the wasm package. `AccessRuleErrorVariant` bridges `NotFound` on until a published sdk-internal carries it - the Rust side now maps the server's 404 on the by-id calls - after which the alias collapses and the branches start working. `MAX_REQUEST_ACCESS_WINDOW_SECONDS` keeps its value but now records that the SDK owns the number and exposes it as `max_request_access_window_seconds()`, plus where that call belongs once it ships. Also carries an unrelated import-order fix in cipher-view-banner.component.ts that `npm run lint:fix` applied.
* fix(pam): simplify access-rules table badges and status labels * fix(pam): keep the IP-restricted signal in the approval method column The plain-text approval method column resolved a single label key, so a rule gated only by an ip_allowlist condition rendered as "Auto-approved" — not merely less detail, but a false statement about how the rule grants access. Return the applicable keys in display order instead, and let the pipe resolve and comma-join them, matching the design's three states: human approval + ip allowlist -> "Requires approval, IP restricted" ip allowlist only -> "IP restricted" neither -> "Auto-approved" Reuses the existing pamAccessRuleConditionIpRestricted string rather than adding a key, so no translations are orphaned. * fix(pam): address review on the access-rules table simplification Introduce new i18n keys instead of editing existing ones — messages.json entries are immutable once Crowdin has them, so changed copy needs a new key: pamAccessRuleInactiveHint and pamAccessRuleCreateNew replace the in-place edits to pamAccessRuleEnabledHint and pamAccessRuleNew. Read the "Maximum duration" column off maxLeaseDurationSeconds directly. accessRuleWindow collapsed an absent cap and a cap equal to the default into the same null, so an uncapped rule (the default for a new rule) was rendering its default duration as if it were a ceiling. Uncapped rules now show "No cap". The window helper and pipe were its only consumers, so both are removed. * fix(pam): correct import order in the cipher view banner Pre-existing lint error on pam/uat that blocks CI for every PR targeting it. * fix(pam): drop the duplicated startIcon on the create button
* fix(pam): align the access rules table and lifecycle vocabulary * fix(pam): address review findings in the access rules table alignment * fix(pam): share the duration formatter and de-duplicate its specs
* fix(pam): align the access rule form copy with the design * chore(pam): drop narrating comments from the access-rules specs * fix(pam): address review findings in the access rule form copy * fix(pam): address review findings in the access rule form copy * fix(pam): restore the page-type breadcrumb without an inert button * fix(pam): restore the default-duration and CIDR hints * fix(pam): align the page-type breadcrumb with the crumb baseline * fix(pam): reuse shared lookups and keep the duration message keys * fix(pam): type the shared test providers for provideRouter
…2508) * feat(pam): add discard, save-error, and validation summary states * fix(pam): address review findings in the access rule form states * fix(pam): hide the maximum-duration hint when no cap is set * fix(pam): use the design's discard-rule copy when creating a rule * fix(pam): drive discard state from the form and focus save errors on retry
* fix(pam): map access-rule save failures onto our own copy
The SDK surfaces a rejected write as an AccessRuleError whose message is the
whole serialized 400 — JSON envelope, exception message, and a server-side
stack trace carrying absolute paths into the server repo — and the edit page
piped it straight into the danger callout while the list page toasted it.
Classify the failure instead, in the shape of request-access-error.ts: the
write path's six known validator messages and the NotFound variant map onto
i18n'd copy, everything else falls back to the design's generic system-error
sentence with a "Try again" that re-fires the form. A mapped failure the admin
can act on is reported on the field it names, with no retry offered, since
resending the same values would fail identically.
The design's inline empty-field copy ("Enter a name." / "Select a collection.")
is deliberately not implemented. Reaching it means dropping Validators.required
for a custom-keyed validator, and that validator is also what bit-form-field
reads to render the required asterisk and its sr-only "(required)". Nothing
else in that template sets aria-required, so the copy would have cost these
fields their announcement to screen readers. DECISIONS.md records the two
workarounds that were built and measured, and why both fail.
* fix(pam): finish the access-rule error rename and drop duplicate copy
Rewire the save-failure callout onto the classified outcome: the message
comes from `saveErrorMessage()` and the retry is offered only for a
`generic` outcome, since a mapped failure needs the admin to change
something before resending.
The retry is now a plain submit button, which reaches `bitSubmit` through
the form without a template ref. Reuse the existing `tryAgain` string and
retire `pamAccessRuleSaveErrorTryAgain`, `pamAccessRuleSaveErrorTitle`
and `pamAccessRuleCollectionConflict`, all now orphaned.
The picker offered a hardcoded 15m-24h preset list and pre-selected an hour whatever the governing rule allowed, so a rule configured for 15-30 minutes still offered a 1 hour lease (PM-39858). Both numbers now come from the pre-check's bounds. requestDurationOptions() narrows the presets to the rule's cap, then widens the result to include the cap and the default themselves -- which keeps the picker non-empty for a cap below the smallest preset, and guarantees the pre-selected default is a real option so the select cannot render blank. Every cap an admin can set today coincides with a preset, but one written straight to the API need not, so entries the presets do not cover are formatted from their value instead of an i18n key. The human path seeds its window from the rule's default and validates the window against the rule's cap, rather than only the global 24h ceiling that let an over-cap window look valid until submit rejected it. Its validator became a factory reading the cap through a callback, so re-opening the fold-out against a different rule cannot leave a stale cap behind. requestAccessModalWindowExceedsMax hardcoded "24 hours" and is now parameterised with the cap actually in force. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The web client now carries two catalogs of server prose - twelve sentences in `request-access-error.ts`, six in `access-rule-error.ts` - matched with `includes()` because `ErrorResponseModel` has no machine-readable discriminant and every failure is a 400. Both catalogs independently note that they exist only until the server grows a code. Three of the sentences do not mean failure at all: they say the requester already holds what they asked for, and the UI reconciles rather than reporting an error. A rewording turns a reconciliation into a red toast with nothing failing on either side. The unmatched fallback is no better - the `Api` message is the whole serialized response body, stack trace and filesystem paths included, so it can be neither shown nor logged. Also records that the SDK now has `leases().leased_cipher()`, which retires this module's last raw-HTTP call once a published sdk-internal carries it.
…22543) The vault list's "Controlled access" column decided whether a collection row is privileged by fetching the organization's access rules and matching collection ids. That works for a member, but the access-rules endpoint requires organization membership by design, so a provider browsing a client organization's Admin Console gets nothing back and every collection cell renders empty -- the column is there, the rows never fill in. The server now derives the answer per collection, so read it off the collection. Bridge hasEnabledAccessRule through CollectionDetailsResponse, CollectionData, Collection, CollectionView and CollectionAdminView, and have the vault-row badge render the resting "Privileged" pill straight from it. A collection row now costs no request, cannot disagree with the list it is drawn beside, and is right for any viewer who can see the collection at all. GovernedCollectionsService stays for the collection-dialog callout, which names the governing rules and summarises what they enforce -- a boolean cannot say that. Both surfaces still agree on what "governed" means: the server computes the flag as "associated with a rule that is enabled", exactly what rulesGoverningCollection filters for. The flag rides alongside the SDK rather than through it. The SDK's Collection and CollectionView are #[serde(deny_unknown_fields)] and declare no such field, so putting it in toSdkCollection() would throw at runtime rather than fail to compile; it is restored from the source collection in fromSdkCollectionView instead, the same carry-forward defaultUserCollectionEmail already relies on. Tests in collection-sdk-mapping.spec.ts pin both halves of that, since no type can. Requires the matching server change (bitwarden/server "Tell clients which collections a PAM access rule governs"). Without it the field is absent, the badge reads false, and collection rows go back to being empty.
The access-state badge is the one pill recipe shared by the vault row, the cipher-view modal and the Requests page, so its colour/icon/copy mapping is exactly the kind of thing a visual-regression snapshot should hold still -- but it was the only purely presentational PAM component with no stories at all. All five existing PAM story files sit under access-rules/. Eleven stories: one per static recipe, the two active variants either side of the five-minute danger escalation, and a gallery rendering every recipe together, which is the view to check when changing a colour or the threshold. active is the only state that depends on the clock, so its expiresAt is built inside render() rather than at module load. A module-level Date.now() would leave a story that stays open drifting past its own expiry and silently falling back to the "Session ended" recipe. formatRemaining ceils, so the 4m story renders a stable label for a full minute and snapshots fine. Under a minute the countdown changes on every tick, so only EndingSoonSeconds carries chromatic.disableSnapshot; the other ten stay snapshotted. LapsedLease and NotGated pin two behaviours the type does not spell out: an active lease whose expiresAt has already passed locally falls back to the resting expired recipe instead of rendering a negative countdown, and a null state renders nothing rather than an empty pill. Expired and Unavailable note in their docstrings that neither is reachable through cipherAccessBadgeState today, matching the comment already in access-badge-state.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The populated table's "Create access rule" button previously jumped straight to the blank form, leaving the starter templates reachable only from the empty state. It now opens a menu offering a blank Custom rule and, as a labeled group, the three starter templates (just-in-time, approval-required, IP-restricted), each routing into the existing create flows.
Neither dialog had stories, so the states an approver and a lease holder actually see -- the approve/deny split, the summary a decision is made from, the extension form's validation -- were only reachable by driving the app. The decide dialog reads DIALOG_DATA once at construction and has no inputs, so args cannot drive it; each story supplies its own params through a withParams() decorator instead. Its row is built through the real toApprovalRow with a fixed `now`, mirroring the spec, so the summary renders the same precomputed labels as the inbox row behind the dialog -- the point of repeating the request inside the dialog is that the two cannot disagree -- and the window labels do not drift with the clock. Both use PreloadedEnglishI18nModule rather than I18nMockService: these templates pull around fifteen keys each, and the real copy is what the layout has to hold. The extend-lease dialog's validation story blurs the reason field rather than clicking Extend. Extend is [disabled] while the form is invalid, so a click would do nothing -- touch is what surfaces the required error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two OSS seam components -- the badge bound to VAULT_ROW_LEASE_BADGE and the callout bound to COLLECTION_ACCESS_RULE_CALLOUT -- render inside surfaces that know nothing about PAM, which makes them awkward to reach in the app and worth pinning here instead. The badge's three no-badge branches fail for different reasons and are pinned separately: a cipher that is not gated (no request is issued at all), the feature flag off, and a rejected access-state read swallowed by catchError. The badge is decoration on someone else's list, so failing quiet is the contract. Its SDK stub takes a state factory rather than a value, so an active lease's expiresAt is relative to render time; a module-load timestamp would resolve straight to the "Session ended" recipe. The callout stubs GovernedCollectionsService rather than layering it over a stubbed SDK the way the spec does. The real service caches per organization for 30s, which would carry one story's rules into the next. The component still runs the real rulesGoverningCollection filter over whatever the stub returns, so the disabled-rule and not-targeted cases exercise the filter rather than being handed an empty list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deactivating a rule wrote immediately from both the row menu and the bulk actions bar. Add the speedbump design specified: a warning Simple Dialog stating that the rule stops applying to new access requests. Activation stays one click — it only ever adds gating back. The bulk bar confirms over only the rules that will actually move, since setManyEnabled skips any already inactive; that skip rule now lives in one place (rulesChangingEnabled) rather than being re-derived in the component. One rule takes the singular copy whichever surface asked. Does not cover the per-rule active-lease / approved / pending counts from the ticket's AC 5 — no server, SDK or API surface exposes them today.
…table and the collection callout
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed a two-file PAM change that replaces the inline approval/IP key derivation in Code Review DetailsNo findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pam/uat #22565 +/- ##
===========================================
- Coverage 54.54% 54.47% -0.07%
===========================================
Files 4381 4381
Lines 138958 138830 -128
Branches 21942 21900 -42
===========================================
- Hits 75800 75634 -166
- Misses 57663 57716 +53
+ Partials 5495 5480 -15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(pam): share approval-method label logic between the access-rules table and the collection callout
This PR targets
pam/uat, not the default branch, and is part of a stack rooted onpam/uat. It follows the access-rules table and badge work already in the stack (the PR that introducedapprovalMethodLabelKeysinbitwarden_license/bit-web/src/app/pam/helpers/approval-method.ts); read that PR first, since this one only changes a second caller of the helper it added.🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-42252
📔 Objective
access-rule-summary.ts(used by the collection access-rule callout) had its own inline derivation of the approval-method and IP-restriction i18n keys, duplicating the logic already centralized inapprovalMethodLabelKeys()for the access-rules table. This PR switchesaccessRuleSummaryKeys()to call that shared helper instead of re-deriving the same keys locally, so the callout and the table can no longer drift out of sync on how a rule's access method is described.The behavior changes in one respect: previously, a rule with only an
ip_allowlistcondition (nohuman_approvalcondition) reported bothpamAccessRuleConditionAutoApprovedandpamAccessRuleConditionIpRestricted. The shared helper only falls back to the auto-approved key when neitherhuman_approvalnorip_allowlistis present, so such a rule now reportspamAccessRuleConditionIpRestrictedalone, matching how the access-rules table already described the same rule. The spec in this PR is updated to assert that corrected behavior.The single-active-user addition (
pamAccessRuleSingleActiveUser) stays local toaccessRuleSummaryKeys(), since it is specific to the collection callout and not part of the shared helper's contract.📸 Screenshots
Known gap: screenshots could not be captured for this PR. Storybook was started locally on port 6100 and progressed through a healthy webpack build (build events observed up to roughly the halfway point), but did not finish compiling before this check had to conclude, so no before/after screenshots were captured for
web-pam-collection-access-rule-callout--multiple-rulesorweb-pam-collection-access-rule-callout--all-conditions. This was a build-time issue only, not a code or environment failure; a re-run with more time budget should succeed.Known gaps
access-rule-summary.tsandaccess-rule-summary.spec.tsdirectly, but visual confirmation of the callout rendering is still outstanding.darkThemeOk=false, 2 failed assertions) because chrome-devtools-mcp's browser lock was held by sibling agents running concurrently in this session, not because of any problem found in the change. Storybook itself was confirmed up and freshly built from this HEAD during that check. Recommend re-running Visual QA once the sibling agents release the shared browser profile, or with a dedicated--isolatedprofile.