[PM-18855] Add edit Cipher permission check to Cipher Authorization Service and use in Vault dialog (#18375)

Centralize edit permission checks in CipherAuthorizationService instead of using the disableForm parameter passed to VaultItemDialogComponent. This refactoring improves consistency with how delete and restore permissions are handled, establishes a single source of truth for authorization logic, and simplifies caller components. This change also fixes the bug in ticket, which allows Users to properly edit Ciphers inside of the various Admin Console report types.
2026-02-07 12:13:45 +00:00 · 2026-02-06 10:18:20 -08:00
parent 6a01e7436c
commit b6ff3a110e
10 changed files with 218 additions and 114 deletions
--- a/apps/web/src/app/admin-console/organizations/collections/vault.component.ts
+++ b/apps/web/src/app/admin-console/organizations/collections/vault.component.ts
@@ -920,14 +920,9 @@ export class VaultComponent implements OnInit, OnDestroy {
    cipher?: CipherView,
    activeCollectionId?: CollectionId,
  ) {
-    const organization = await firstValueFrom(this.organization$);
-    const disableForm = cipher ? !cipher.edit && !organization.canEditAllCiphers : false;
-    // If the form is disabled, force the mode into `view`
-    const dialogMode = disableForm ? "view" : mode;
    this.vaultItemDialogRef = VaultItemDialogComponent.open(this.dialogService, {
-      mode: dialogMode,
+      mode,
      formConfig,
-      disableForm,
      activeCollectionId,
      isAdminConsoleAction: true,
      restore: this.restore,
--- a/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/cipher-report.component.ts
@@ -186,13 +186,10 @@ export abstract class CipherReportComponent implements OnDestroy {
    cipher: CipherView,
    activeCollectionId?: CollectionId,
  ) {
-    const disableForm = cipher ? !cipher.edit && !this.organization?.canEditAllCiphers : false;
-
    this.vaultItemDialogRef = VaultItemDialogComponent.open(this.dialogService, {
      mode,
      formConfig,
      activeCollectionId,
-      disableForm,
      isAdminConsoleAction: this.organization != null,
    });

--- a/apps/web/src/app/dirt/reports/pages/organizations/exposed-passwords-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/organizations/exposed-passwords-report.component.ts
@@ -1,17 +1,13 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { Component, OnInit } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
-import { firstValueFrom } from "rxjs";
+import { firstValueFrom, takeUntil, tap } from "rxjs";

 import { AuditService } from "@bitwarden/common/abstractions/audit.service";
-import {
-  getOrganizationById,
-  OrganizationService,
-} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
+import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { getById } from "@bitwarden/common/platform/misc";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
 import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
@@ -51,7 +47,7 @@ export class ExposedPasswordsReportComponent
  extends BaseExposedPasswordsReportComponent
  implements OnInit
 {
-  manageableCiphers: Cipher[];
+  private manageableCiphers: Cipher[] = [];

  constructor(
    cipherService: CipherService,
@@ -82,20 +78,25 @@ export class ExposedPasswordsReportComponent

  async ngOnInit() {
    this.isAdminConsoleActive = true;
-    // eslint-disable-next-line rxjs-angular/prefer-takeuntil, rxjs/no-async-subscribe
-    this.route.parent.parent.params.subscribe(async (params) => {
-      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
-      this.organization = await firstValueFrom(
-        this.organizationService
-          .organizations$(userId)
-          .pipe(getOrganizationById(params.organizationId)),
-      );
-      this.manageableCiphers = await this.cipherService.getAll(userId);
-    });
+    this.route.parent?.parent?.params
+      .pipe(
+        tap(async (params) => {
+          const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+          this.organization = await firstValueFrom(
+            this.organizationService.organizations$(userId).pipe(getById(params.organizationId)),
+          );
+          this.manageableCiphers = await this.cipherService.getAll(userId);
+        }),
+        takeUntil(this.destroyed$),
+      )
+      .subscribe();
  }

-  getAllCiphers(): Promise<CipherView[]> {
-    return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+  async getAllCiphers(): Promise<CipherView[]> {
+    if (this.organization) {
+      return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+    }
+    return [];
  }

  canManageCipher(c: CipherView): boolean {
--- a/apps/web/src/app/dirt/reports/pages/organizations/inactive-two-factor-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/organizations/inactive-two-factor-report.component.ts
@@ -1,9 +1,10 @@
 import { ChangeDetectorRef, Component, OnInit, ChangeDetectionStrategy } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
-import { firstValueFrom, map, takeUntil } from "rxjs";
+import { firstValueFrom, takeUntil, tap } from "rxjs";

 import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { getById } from "@bitwarden/common/platform/misc";
@@ -81,27 +82,24 @@ export class InactiveTwoFactorReportComponent
    this.isAdminConsoleActive = true;

    this.route.parent?.parent?.params
-      ?.pipe(takeUntil(this.destroyed$))
-      // eslint-disable-next-line rxjs/no-async-subscribe
-      .subscribe(async (params) => {
-        const userId = await firstValueFrom(
-          this.accountService.activeAccount$.pipe(map((a) => a?.id)),
-        );
-
-        if (userId) {
+      .pipe(
+        tap(async (params) => {
+          const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
          this.organization = await firstValueFrom(
            this.organizationService.organizations$(userId).pipe(getById(params.organizationId)),
          );
          this.manageableCiphers = await this.cipherService.getAll(userId);
          await super.ngOnInit();
-        }
-        this.changeDetectorRef.markForCheck();
-      });
+          this.changeDetectorRef.markForCheck();
+        }),
+        takeUntil(this.destroyed$),
+      )
+      .subscribe();
  }

  async getAllCiphers(): Promise<CipherView[]> {
    if (this.organization) {
-      return await this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+      return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
    }
    return [];
  }
--- a/apps/web/src/app/dirt/reports/pages/organizations/reused-passwords-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/organizations/reused-passwords-report.component.ts
@@ -1,16 +1,12 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { Component, OnInit } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
-import { firstValueFrom } from "rxjs";
+import { firstValueFrom, takeUntil, tap } from "rxjs";

-import {
-  getOrganizationById,
-  OrganizationService,
-} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
+import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { getById } from "@bitwarden/common/platform/misc";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
 import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
@@ -50,7 +46,7 @@ export class ReusedPasswordsReportComponent
  extends BaseReusedPasswordsReportComponent
  implements OnInit
 {
-  manageableCiphers: Cipher[];
+  manageableCiphers: Cipher[] = [];

  constructor(
    cipherService: CipherService,
@@ -79,21 +75,27 @@ export class ReusedPasswordsReportComponent

  async ngOnInit() {
    this.isAdminConsoleActive = true;
-    // eslint-disable-next-line rxjs-angular/prefer-takeuntil, rxjs/no-async-subscribe
-    this.route.parent.parent.params.subscribe(async (params) => {
-      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
-      this.organization = await firstValueFrom(
-        this.organizationService
-          .organizations$(userId)
-          .pipe(getOrganizationById(params.organizationId)),
-      );
-      this.manageableCiphers = await this.cipherService.getAll(userId);
-      await super.ngOnInit();
-    });
+
+    this.route.parent?.parent?.params
+      .pipe(
+        tap(async (params) => {
+          const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+          this.organization = await firstValueFrom(
+            this.organizationService.organizations$(userId).pipe(getById(params.organizationId)),
+          );
+          this.manageableCiphers = await this.cipherService.getAll(userId);
+          await super.ngOnInit();
+        }),
+        takeUntil(this.destroyed$),
+      )
+      .subscribe();
  }

-  getAllCiphers(): Promise<CipherView[]> {
-    return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+  async getAllCiphers(): Promise<CipherView[]> {
+    if (this.organization) {
+      return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+    }
+    return [];
  }

  canManageCipher(c: CipherView): boolean {
--- a/apps/web/src/app/dirt/reports/pages/organizations/unsecured-websites-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/organizations/unsecured-websites-report.component.ts
@@ -1,16 +1,13 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { Component, OnInit } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
-import { firstValueFrom, map } from "rxjs";
+import { firstValueFrom, takeUntil, tap } from "rxjs";

 import { CollectionService } from "@bitwarden/admin-console/common";
-import {
-  getOrganizationById,
-  OrganizationService,
-} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
+import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
+import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { getById } from "@bitwarden/common/platform/misc";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
 import { Cipher } from "@bitwarden/common/vault/models/domain/cipher";
@@ -51,7 +48,7 @@ export class UnsecuredWebsitesReportComponent
  implements OnInit
 {
  // Contains a list of ciphers, the user running the report, can manage
-  private manageableCiphers: Cipher[];
+  private manageableCiphers: Cipher[] = [];

  constructor(
    cipherService: CipherService,
@@ -82,23 +79,26 @@ export class UnsecuredWebsitesReportComponent

  async ngOnInit() {
    this.isAdminConsoleActive = true;
-    // eslint-disable-next-line rxjs-angular/prefer-takeuntil, rxjs/no-async-subscribe
-    this.route.parent.parent.params.subscribe(async (params) => {
-      const userId = await firstValueFrom(
-        this.accountService.activeAccount$.pipe(map((a) => a?.id)),
-      );
-      this.organization = await firstValueFrom(
-        this.organizationService
-          .organizations$(userId)
-          .pipe(getOrganizationById(params.organizationId)),
-      );
-      this.manageableCiphers = await this.cipherService.getAll(userId);
-      await super.ngOnInit();
-    });
+    this.route.parent?.parent?.params
+      .pipe(
+        tap(async (params) => {
+          const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+          this.organization = await firstValueFrom(
+            this.organizationService.organizations$(userId).pipe(getById(params.organizationId)),
+          );
+          this.manageableCiphers = await this.cipherService.getAll(userId);
+          await super.ngOnInit();
+        }),
+        takeUntil(this.destroyed$),
+      )
+      .subscribe();
  }

-  getAllCiphers(): Promise<CipherView[]> {
-    return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+  async getAllCiphers(): Promise<CipherView[]> {
+    if (this.organization) {
+      return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+    }
+    return [];
  }

  protected canManageCipher(c: CipherView): boolean {
--- a/apps/web/src/app/dirt/reports/pages/organizations/weak-passwords-report.component.ts
+++ b/apps/web/src/app/dirt/reports/pages/organizations/weak-passwords-report.component.ts
@@ -1,16 +1,12 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { Component, OnInit } from "@angular/core";
 import { ActivatedRoute } from "@angular/router";
-import { firstValueFrom } from "rxjs";
+import { firstValueFrom, takeUntil, tap } from "rxjs";

-import {
-  getOrganizationById,
-  OrganizationService,
-} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
+import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
 import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
 import { getUserId } from "@bitwarden/common/auth/services/account.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
+import { getById } from "@bitwarden/common/platform/misc";
 import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
@@ -51,7 +47,7 @@ export class WeakPasswordsReportComponent
  extends BaseWeakPasswordsReportComponent
  implements OnInit
 {
-  manageableCiphers: Cipher[];
+  private manageableCiphers: Cipher[] = [];

  constructor(
    cipherService: CipherService,
@@ -82,22 +78,26 @@ export class WeakPasswordsReportComponent

  async ngOnInit() {
    this.isAdminConsoleActive = true;
-    // eslint-disable-next-line rxjs-angular/prefer-takeuntil, rxjs/no-async-subscribe
-    this.route.parent.parent.params.subscribe(async (params) => {
-      const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
-
-      this.organization = await firstValueFrom(
-        this.organizationService
-          .organizations$(userId)
-          .pipe(getOrganizationById(params.organizationId)),
-      );
-      this.manageableCiphers = await this.cipherService.getAll(userId);
-      await super.ngOnInit();
-    });
+    this.route.parent?.parent?.params
+      .pipe(
+        tap(async (params) => {
+          const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId));
+          this.organization = await firstValueFrom(
+            this.organizationService.organizations$(userId).pipe(getById(params.organizationId)),
+          );
+          this.manageableCiphers = await this.cipherService.getAll(userId);
+          await super.ngOnInit();
+        }),
+        takeUntil(this.destroyed$),
+      )
+      .subscribe();
  }

-  getAllCiphers(): Promise<CipherView[]> {
-    return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+  async getAllCiphers(): Promise<CipherView[]> {
+    if (this.organization) {
+      return this.cipherService.getAllFromApiForOrganization(this.organization.id, true);
+    }
+    return [];
  }

  canManageCipher(c: CipherView): boolean {
--- a/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.ts
+++ b/apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.ts
@@ -87,11 +87,6 @@ export interface VaultItemDialogParams {
   */
  formConfig: CipherFormConfig;

-  /**
-   * If true, the "edit" button will be disabled in the dialog.
-   */
-  disableForm?: boolean;
-
  /**
   * The ID of the active collection. This is know the collection filter selected by the user.
   */
@@ -273,7 +268,7 @@ export class VaultItemDialogComponent implements OnInit, OnDestroy {
  }

  protected get disableEdit() {
-    return this.params.disableForm;
+    return !this.canEdit;
  }

  protected get showEdit() {
@@ -314,6 +309,8 @@ export class VaultItemDialogComponent implements OnInit, OnDestroy {

  protected canDelete = false;

+  protected canEdit = false;
+
  protected attachmentsButtonDisabled = false;

  protected confirmedPremiumUpgrade = false;
@@ -372,6 +369,20 @@ export class VaultItemDialogComponent implements OnInit, OnDestroy {
        ),
      );

+      this.canEdit = await firstValueFrom(
+        this.cipherAuthorizationService.canEditCipher$(
+          this.cipher,
+          this.params.isAdminConsoleAction,
+        ),
+      );
+
+      // If user cannot edit and dialog opened in form mode, force to view mode
+      if (!this.canEdit && this.params.mode === "form") {
+        this.params.mode = "view";
+        this.loadForm = false;
+        this.updateTitle();
+      }
+
      await this.eventCollectionService.collect(
        EventType.Cipher_ClientViewed,
        this.cipher.id,
--- a/libs/common/src/vault/services/cipher-authorization.service.spec.ts
+++ b/libs/common/src/vault/services/cipher-authorization.service.spec.ts
@@ -205,6 +205,70 @@ describe("CipherAuthorizationService", () => {
    });
  });

+  describe("canEditCipher$", () => {
+    it("should return true if isAdminConsoleAction is true and cipher is unassigned", (done) => {
+      const cipher = createMockCipher("org1", []) as CipherView;
+      const organization = createMockOrganization({ canEditUnassignedCiphers: true });
+      mockOrganizationService.organizations$.mockReturnValue(
+        of([organization]) as Observable<Organization[]>,
+      );
+
+      cipherAuthorizationService.canEditCipher$(cipher, true).subscribe((result) => {
+        expect(result).toBe(true);
+        done();
+      });
+    });
+
+    it("should return true if isAdminConsoleAction is true and user can edit all ciphers in the org", (done) => {
+      const cipher = createMockCipher("org1", ["col1"]) as CipherView;
+      const organization = createMockOrganization({ canEditAllCiphers: true });
+      mockOrganizationService.organizations$.mockReturnValue(
+        of([organization]) as Observable<Organization[]>,
+      );
+
+      cipherAuthorizationService.canEditCipher$(cipher, true).subscribe((result) => {
+        expect(result).toBe(true);
+        expect(mockOrganizationService.organizations$).toHaveBeenCalledWith(mockUserId);
+        done();
+      });
+    });
+
+    it("should return false if isAdminConsoleAction is true but user does not have permission to edit unassigned ciphers", (done) => {
+      const cipher = createMockCipher("org1", []) as CipherView;
+      const organization = createMockOrganization({ canEditUnassignedCiphers: false });
+      mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
+
+      cipherAuthorizationService.canEditCipher$(cipher, true).subscribe((result) => {
+        expect(result).toBe(false);
+        done();
+      });
+    });
+
+    it("should return true if cipher.edit is true and is not an admin action", (done) => {
+      const cipher = createMockCipher("org1", [], true) as CipherView;
+      const organization = createMockOrganization();
+      mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
+
+      cipherAuthorizationService.canEditCipher$(cipher, false).subscribe((result) => {
+        expect(result).toBe(true);
+        expect(mockCollectionService.decryptedCollections$).not.toHaveBeenCalled();
+        done();
+      });
+    });
+
+    it("should return false if cipher.edit is false and is not an admin action", (done) => {
+      const cipher = createMockCipher("org1", [], false) as CipherView;
+      const organization = createMockOrganization();
+      mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
+
+      cipherAuthorizationService.canEditCipher$(cipher, false).subscribe((result) => {
+        expect(result).toBe(false);
+        expect(mockCollectionService.decryptedCollections$).not.toHaveBeenCalled();
+        done();
+      });
+    });
+  });
+
  describe("canCloneCipher$", () => {
    it("should return true if cipher has no organizationId", async () => {
      const cipher = createMockCipher(null, []) as CipherView;
--- a/libs/common/src/vault/services/cipher-authorization.service.ts
+++ b/libs/common/src/vault/services/cipher-authorization.service.ts
@@ -53,6 +53,19 @@ export abstract class CipherAuthorizationService {
    cipher: CipherLike,
    isAdminConsoleAction?: boolean,
  ) => Observable<boolean>;
+
+  /**
+   * Determines if the user can edit the specified cipher.
+   *
+   * @param {CipherLike} cipher - The cipher object to evaluate for edit permissions.
+   * @param {boolean} isAdminConsoleAction - Optional. A flag indicating if the action is being performed from the admin console.
+   *
+   * @returns {Observable<boolean>} - An observable that emits a boolean value indicating if the user can edit the cipher.
+   */
+  abstract canEditCipher$: (
+    cipher: CipherLike,
+    isAdminConsoleAction?: boolean,
+  ) => Observable<boolean>;
 }

 /**
@@ -118,6 +131,29 @@ export class DefaultCipherAuthorizationService implements CipherAuthorizationSer
    );
  }

+  /**
+   *
+   * {@link CipherAuthorizationService.canEditCipher$}
+   */
+  canEditCipher$(cipher: CipherLike, isAdminConsoleAction?: boolean): Observable<boolean> {
+    return this.organization$(cipher).pipe(
+      map((organization) => {
+        if (isAdminConsoleAction) {
+          // If the user is an admin, they can edit an unassigned cipher
+          if (!cipher.collectionIds || cipher.collectionIds.length === 0) {
+            return organization?.canEditUnassignedCiphers === true;
+          }
+
+          if (organization?.canEditAllCiphers) {
+            return true;
+          }
+        }
+
+        return !!cipher.edit;
+      }),
+    );
+  }
+
  /**
   * {@link CipherAuthorizationService.canCloneCipher$}
   */