Skip to content

feat(pam): explain request-gated access when viewing a governed collection - #22553

Open
maxkpower wants to merge 5 commits into
pam/collections-gated-indicatorfrom
pam/gated-collection-banner
Open

feat(pam): explain request-gated access when viewing a governed collection#22553
maxkpower wants to merge 5 commits into
pam/collections-gated-indicatorfrom
pam/gated-collection-banner

Conversation

@maxkpower

@maxkpower maxkpower commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40088

📔 Objective

A member filtering to a gated collection saw request-gated rows with no collection-level explanation.
Adds a banner above the item list stating the restriction, and lifts the shared gating check into
services/gated-collection.ts so the sidebar indicator, this banner and the collection-dialog callout
make one claim from one source.

Icon is bwi-lock-encrypted, matching the sidebar indicator.

📸 Screenshots

01-before-gated-collection 02-after-gated-collection

⚠️ Known limitations

  • The banner names the restriction but offers no action; requesting still happens per item, from the
    cipher-view banner.
  • It appears only when a single collection is the active filter. Browsing "All items" or an
    organization root shows governed and ungated rows mixed, with the per-row column as the only signal.
  • GovernedCollectionsService is root-provided so four surfaces share one cached per-org rules read.
    That cache's invalidation is not addressed here: a rule enabled in another tab is not reflected until
    the read refreshes.
  • Now that the collection read carries hasEnabledAccessRule, this surface and the sidebar indicator
    could drop GovernedCollectionsService entirely. Not done here; it is a design change to two
    milestones rather than part of this one.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.48936% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.55%. Comparing base (57c2e17) to head (dbcd240).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...twarden_license/bit-web/src/app/pam/provide-pam.ts 0.00% 2 Missing ⚠️
...e/bit-web/src/app/pam/services/gated-collection.ts 92.30% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@                         Coverage Diff                         @@
##           pam/collections-gated-indicator   #22553      +/-   ##
===================================================================
+ Coverage                            54.54%   54.55%   +0.01%     
===================================================================
  Files                                 4382     4385       +3     
  Lines                               138868   138890      +22     
  Branches                             21917    21916       -1     
===================================================================
+ Hits                                 75750    75778      +28     
+ Misses                               57619    57613       -6     
  Partials                              5499     5499              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maxkpower maxkpower added the t:feature Change Type - Feature Development label Aug 20, 2026
@maxkpower
maxkpower marked this pull request as ready for review August 20, 2026 16:19
@maxkpower
maxkpower requested review from a team as code owners August 20, 2026 16:19
@maxkpower
maxkpower requested a review from gbubemismith August 20, 2026 16:19
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed at head dbcd240, against the stacked base pam/collections-gated-indicator. The new VAULT_GATED_COLLECTION_BANNER seam follows the established SafeInjectionToken<Type<unknown>> + inject(..., { optional: true }) + *ngComponentOutlet pattern already used by VAULT_FILTER_GATED_COLLECTION_INDICATOR and VAULT_ROW_LEASE_BADGE, so apps/web gains no PAM dependency and renders unchanged when the token is unprovided; the outlet is additionally gated on selectedCollection, which selectedCollection$ already resolves to undefined for All, Unassigned, and an absent collectionId. The extraction into services/gated-collection.ts is a faithful move of the indicator's stream — same FeatureFlag.Pam gate, same getOptionalUserId signed-out guard, same PAM-org narrowing before any rules$(organizationId) read, same rulesGoverningCollection predicate — and the helper is only ever invoked from field initializers, a valid injection context for inject/toObservable/toSignal. The banner derives its claim from access-rule metadata the member's own organization already exposes: no Vault Data, keys, or PII cross a boundary, and the server-side gate remains authoritative; bit-callout's type/icon/title/accessibleName contracts, the bwi-lock-encrypted glyph, and the en-only i18n key addition all check out.

Code Review Details

No findings.

The earlier pass's ♻️ DEBT finding — the indicator and helper TSDoc claiming the collection-row badge and the collection-dialog callout also route through gatedCollection() — is resolved by 0276dcc and dbcd240. The docs in services/gated-collection.ts, gated-collection-filter-indicator.component.ts, provide-pam.ts, and pam/CLAUDE.md now agree that only the sidebar lock and the banner share the helper, and each states why the other two surfaces cannot.

@maxkpower maxkpower added the ai-review Request a Claude code review label Aug 20, 2026
@maxkpower
maxkpower force-pushed the pam/gated-collection-banner branch from 9ff7385 to b7b9a14 Compare August 20, 2026 17:23
The helper has two callers, the sidebar lock and the vault banner. The
collection-dialog callout reads GovernedCollectionsService and
rulesGoverningCollection directly, and the collection-row badge reads the
collection's own hasEnabledAccessRule, so neither is kept in step by a change
to the helper. The module guide said otherwise in one place and contradicted
itself in another.
…d check

`gatedCollection` counts rules rather than naming them, so the reason it cannot
take the collection-row badge's `hasEnabledAccessRule` shortcut is not "it needs
the rules themselves" - it is that neither caller has a usable flag. The sidebar's
nodes are rebuilt through `new CollectionView(...)`, which resets the flag to
`false`, and the banner is handed ids alone.
@maxkpower
maxkpower force-pushed the pam/gated-collection-banner branch from b7b9a14 to dbcd240 Compare August 20, 2026 18:13

@gbubemismith gbubemismith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxkpower I just left one comment, every other thing looks good. Nice job!

* vault never depends on the feature library that implements it. Unprovided, the vault is
* unchanged.
*/
export const VAULT_GATED_COLLECTION_BANNER = new SafeInjectionToken<Type<unknown>>(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 Using Type<Unknown> here defeats the purpose of SafeInjectionToken. The tokens value is a compile time check on the provider. From comments I think we can do this

Suggested change
export const VAULT_GATED_COLLECTION_BANNER = new SafeInjectionToken<Type<unknown>>(
export interface VaultGatedCollectionBanner {
readonly organizationId: InputSignal<OrganizationId | undefined>;
readonly collectionId: InputSignal<CollectionId | undefined>;
}
export const VAULT_GATED_COLLECTION_BANNER = new SafeInjectionToken<VaultGatedCollectionBanner>(
"VaultGatedCollectionBanner",
);

That way in vault.component.ts we can have something like this

protected readonly gatedCollectionBanner: Type<VaultGatedCollectionBanner> | null = inject(
    VAULT_GATED_COLLECTION_BANNER,
    { optional: true },
  );

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants