PM-13944 - Refactor canClone method to use CipherAuthorizationService

[theMickster]

🎟️ Tracking

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

📔 Objective

Follow-up tech debt work to #11660 to use the CipherAuthorizationService and the RestrictedItemTypesService to determine whether a user can clone a cipher. Leverages similar patterns found in item-more-options.component.ts and the canClone$.

In terms of unit testing the canClone$(), it contains no business logic—it simply orchestrates two well-tested services via RxJS pipes—so unit testing it would only verify trivial Observable piping while creating maintenance overhead without meaningful coverage gains. My general observation is that we don't unit test component glue code.

I found additional related services that needed covering unit tests.

📸 Screenshots

Please see the following screen recording that shows the testing following the refactor.
All appears to function as it did before the refactor.

Screen.Recording.2025-10-13.at.1.04.12.PM.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @theMickster's task —— View job

---

Code Review Complete ✅

Summary of Changes

This PR successfully refactors the canClone method in vault-items.component.ts to use the CipherAuthorizationService and RestrictedItemTypesService, following the established pattern from item-more-options.component.ts. The refactor:

• Converts synchronous canClone() to async canClone$() Observable pattern
• Removes custom authorization logic in favor of centralized services
• Removes the toSignal conversion and restrictedPolicies field
• Adds comprehensive unit tests for RestrictedItemTypesService.isCipherRestricted() (168 lines)

Overall Assessment: This is a solid tech debt refactor that improves code maintainability and consistency. The changes are functionally equivalent to the original implementation and follow Angular/RxJS best practices.

---

Critical Issues

None identified. ✅

---

Suggested Improvements

🎨 Consider consistency with reference implementation

apps/web/src/app/vault/components/vault-items/vault-items.component.ts:287-303

Your implementation correctly follows the pattern from item-more-options.component.ts, but there's a subtle difference worth noting:

// Your implementation (vault-items.component.ts:287-303)
protected canClone$(vaultItem: VaultItem<C>): Observable<boolean> {
  return this.restrictedItemTypesService.restricted$.pipe(
    switchMap((restrictedTypes) => {
      const isItemRestricted = restrictedTypes.some(
        (rt) => rt.cipherType === CipherViewLikeUtils.getType(vaultItem.cipher),
      );
      if (isItemRestricted) {
        return of(false);
      }
      return this.cipherAuthorizationService.canCloneCipher$(
        vaultItem.cipher,
        this.showAdminActions,
      );
    }),
  );
}

// Reference implementation (item-more-options.component.ts:76-91)
protected canClone$ = combineLatest([
  this._cipher$,
  this.restrictedItemTypesService.restricted$,
]).pipe(
  filter(([c]) => c != null),
  switchMap(([c, restrictedTypes]) => {
    const isItemRestricted = restrictedTypes.some(
      (restrictType) => restrictType.cipherType === CipherViewLikeUtils.getType(c),
    );
    if (!isItemRestricted) {  // Note: inverted condition
      return this.cipherAuthorizationService.canCloneCipher$(c);
    }
    return new BehaviorSubject(false);
  }),
);

Differences:

1. Your condition checks if (isItemRestricted) while the reference checks if (!isItemRestricted) - both are logically equivalent, but inverting yours would match the reference
2. Reference returns new BehaviorSubject(false) while you return of(false) - functionally equivalent, but of(false) is simpler and more appropriate here

Recommendation: Consider inverting the condition to match the reference pattern for consistency:

if (!isItemRestricted) {
  return this.cipherAuthorizationService.canCloneCipher$(
    vaultItem.cipher,
    this.showAdminActions,
  );
}
return of(false);

This is purely stylistic - your current implementation is correct.

---

🤔 Question: Missing unit tests for component glue code

apps/web/src/app/vault/components/vault-items/vault-items.component.ts:287-303

The PR description states:

"In terms of unit testing the canClone$(), it contains no business logic—it simply orchestrates two well-tested services via RxJS pipes—so unit testing it would only verify trivial Observable piping while creating maintenance overhead without meaningful coverage gains. My general observation is that we don't unit test component glue code."

While this is a reasonable position, the reference implementation in item-more-options.component.ts also lacks tests for its canClone$ property. However, there's a potential issue:

Consideration: The canClone$ method directly consumes restrictedItemTypesService.restricted$ on each call. While this works correctly, it means:

• Each call to canClone$(item) creates a new subscription to restricted$
• For a list with N items, there will be N subscriptions (though shareReplay in the service helps)
• The template uses canClone$(item) | async, which is efficient with Angular's async pipe

Question for consideration: Would a component-level test that verifies the integration of the two services (restricted items + authorization) provide value? For example:

• Test that restricted items are correctly blocked from cloning
• Test that non-restricted items correctly delegate to canCloneCipher$
• Test edge cases like null/undefined ciphers

This would ensure the orchestration logic remains correct even if service implementations change.

---

📝 Minor: Removed import cleanup

apps/web/src/app/vault/components/vault-items/vault-items.component.ts:5

Good catch removing the now-unused toSignal import. ✅

---

Good Practices Observed

• ✅ Consistent use of RxJS Observable patterns (ADR-0003)
• ✅ Proper use of takeUntilDestroyed() for manual subscription (line 127)
• ✅ Excellent unit test coverage for RestrictedItemTypesService.isCipherRestricted() with comprehensive edge cases
• ✅ Clear test descriptions that document expected behavior
• ✅ Proper use of shareReplay in the service to avoid duplicate subscriptions
• ✅ Removal of inline authorization logic in favor of centralized service

---

Action Items for Author

1. ⚠️ Ensure all CI tests pass - The refactor changes behavior from synchronous to asynchronous, so confirm E2E/integration tests cover clone functionality
2. 💭 Consider inverting the isItemRestricted condition for consistency with the reference implementation (optional, style preference)
3. 💭 Consider whether component-level integration tests would provide value for the canClone$ orchestration logic

---

Additional Context

The refactor successfully centralizes authorization logic, making it:

• Easier to maintain (single source of truth)
• More consistent across the codebase
• Better tested at the service level

The pattern matches item-more-options.component.ts:76-91 closely, which validates the approach. The comprehensive unit tests for isCipherRestricted() are particularly valuable, covering edge cases like personal ciphers, undefined org IDs, and various allowlist scenarios.

No blocking issues identified. This is ready for merge pending CI validation. 👍

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – 0909cd58-7e0f-43c6-8871-e9b38ac722df

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.93%. Comparing base (601c1cc) to head (d29de9b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...lt/components/vault-items/vault-items.component.ts	0.00%	7 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16849   +/-   ##
=======================================
  Coverage   38.92%   38.93%           
=======================================
  Files        3435     3435           
  Lines       97482    97473    -9     
  Branches    14660    14657    -3     
=======================================
+ Hits        37947    37952    +5     
+ Misses      57877    57863   -14     
  Partials     1658     1658           

☔ View full report in Codecov by Sentry.
📢 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.