[PM-24304][PM-24305] - [Defect] Some fields are not disabled when editing an item from My Vault (#15982)

* disable all remaining form fields for editing personally owned My Items * fix failing tests * ensure collection field is also properly disabled * clean up logic * fix failing test * fix test * refactor variable to avoid using `is` prefix * directly reference parent form for status rather than subscribe to it * refactor subscription for form status changes * use observable as an Output * disable attachment button on desktop vault when the form * disable custom field components when custom fields already exist and parent form is disabled * disable attachments button in the browser when the edit form is disabled * grab icon button instance for disabled state --------- Co-authored-by: Nick Krantz <nick@livefront.com>
2025-12-06 00:13:28 +00:00 · 2025-08-25 15:48:00 -07:00
parent e10d13faa8
commit 9ed69ef4b8
22 changed files with 224 additions and 36 deletions
--- a/libs/vault/src/cipher-form/cipher-form-container.ts
+++ b/libs/vault/src/cipher-form/cipher-form-container.ts
@@ -78,8 +78,8 @@ export abstract class CipherFormContainer {
  abstract enableFormFields(): void;

  /**
-   * An observable that emits when the form status changes to enabled.
-   * This can be used to disable child forms when the parent form is enabled.
+   * An observable that emits when the form status changes between enabled/disabled.
+   * This can be used for child forms to react to changes in the form status.
   */
-  formEnabled$: Observable<void>;
+  formStatusChange$: Observable<"enabled" | "disabled">;
 }
--- a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.html
+++ b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.html
@@ -24,7 +24,7 @@
      bitLink
      type="button"
      linkType="primary"
-      *ngIf="!hasCustomFields && !isPartialEdit"
+      *ngIf="!hasCustomFields && !isPartialEdit && allowNewField"
      (click)="addCustomField()"
    >
      <i class="bwi bwi-plus tw-font-bold" aria-hidden="true"></i>
--- a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts
+++ b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts
@@ -29,11 +29,12 @@ describe("AdditionalOptionsSectionComponent", () => {
  let passwordRepromptEnabled$: BehaviorSubject<boolean>;

  const getInitialCipherView = jest.fn(() => null);
+  const formStatusChange$ = new BehaviorSubject<"enabled" | "disabled">("enabled");

  beforeEach(async () => {
    getInitialCipherView.mockClear();

-    cipherFormProvider = mock<CipherFormContainer>({ getInitialCipherView });
+    cipherFormProvider = mock<CipherFormContainer>({ getInitialCipherView, formStatusChange$ });
    passwordRepromptService = mock<PasswordRepromptService>();
    passwordRepromptEnabled$ = new BehaviorSubject(true);
    passwordRepromptService.enabled$ = passwordRepromptEnabled$;
--- a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts
+++ b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts
@@ -58,6 +58,11 @@ export class AdditionalOptionsSectionComponent implements OnInit {

  @Input() disableSectionMargin: boolean;

+  /** True when the form allows new fields to be added */
+  get allowNewField(): boolean {
+    return this.additionalOptionsForm.enabled;
+  }
+
  constructor(
    private cipherFormContainer: CipherFormContainer,
    private formBuilder: FormBuilder,
--- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts
+++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts
@@ -35,10 +35,11 @@ describe("AutofillOptionsComponent", () => {
  let autofillSettingsService: MockProxy<AutofillSettingsServiceAbstraction>;
  let platformUtilsService: MockProxy<PlatformUtilsService>;
  const getInitialCipherView = jest.fn(() => null);
+  const formStatusChange$ = new BehaviorSubject<"enabled" | "disabled">("enabled");

  beforeEach(async () => {
    getInitialCipherView.mockClear();
-    cipherFormContainer = mock<CipherFormContainer>({ getInitialCipherView });
+    cipherFormContainer = mock<CipherFormContainer>({ getInitialCipherView, formStatusChange$ });
    liveAnnouncer = mock<LiveAnnouncer>();
    platformUtilsService = mock<PlatformUtilsService>();
    domainSettingsService = mock<DomainSettingsService>();
--- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts
+++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts
@@ -72,6 +72,10 @@ export class AutofillOptionsComponent implements OnInit {
    return this.autofillOptionsForm.controls.uris.controls;
  }

+  protected get isPartialEdit() {
+    return this.cipherFormContainer.config.mode === "partial-edit";
+  }
+
  protected defaultMatchDetection$ = this.domainSettingsService.defaultUriMatchStrategy$.pipe(
    // The default match detection should only be shown when used on the browser
    filter(() => this.platformUtilsService.getClientType() == ClientType.Browser),
@@ -102,7 +106,7 @@ export class AutofillOptionsComponent implements OnInit {

    this.autofillOptionsForm.valueChanges.pipe(takeUntilDestroyed()).subscribe((value) => {
      this.cipherFormContainer.patchCipher((cipher) => {
-        cipher.login.uris = value.uris.map((uri: UriField) =>
+        cipher.login.uris = value.uris?.map((uri: UriField) =>
          Object.assign(new LoginUriView(), {
            uri: uri.uri,
            match: uri.matchDetection,
@@ -126,6 +130,15 @@ export class AutofillOptionsComponent implements OnInit {
      .subscribe(() => {
        this.uriOptions?.last?.focusInput();
      });
+
+    this.cipherFormContainer.formStatusChange$.pipe(takeUntilDestroyed()).subscribe((status) => {
+      // Disable adding new URIs when the cipher form is disabled
+      if (status === "disabled") {
+        this.autofillOptionsForm.disable();
+      } else if (!this.isPartialEdit) {
+        this.autofillOptionsForm.enable();
+      }
+    });
  }

  ngOnInit() {
@@ -136,7 +149,7 @@ export class AutofillOptionsComponent implements OnInit {
      this.initNewCipher();
    }

-    if (this.cipherFormContainer.config.mode === "partial-edit") {
+    if (this.isPartialEdit) {
      this.autofillOptionsForm.disable();
    }
  }
--- a/libs/vault/src/cipher-form/components/cipher-form.component.ts
+++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts
@@ -115,8 +115,8 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci
  /**
   * Emitted when the form is enabled
   */
-  private formEnabledSubject = new Subject<void>();
-  formEnabled$ = this.formEnabledSubject.asObservable();
+  private formStatusChangeSubject = new Subject<"enabled" | "disabled">();
+  @Output() formStatusChange$ = this.formStatusChangeSubject.asObservable();

  /**
   * The original cipher being edited or cloned. Null for add mode.
@@ -158,11 +158,12 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci

  disableFormFields(): void {
    this.cipherForm.disable({ emitEvent: false });
+    this.formStatusChangeSubject.next("disabled");
  }

  enableFormFields(): void {
    this.cipherForm.enable({ emitEvent: false });
-    this.formEnabledSubject.next();
+    this.formStatusChangeSubject.next("enabled");
  }

  /**
--- a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.html
+++ b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.html
@@ -93,6 +93,7 @@
          bitIconButton="bwi-pencil-square"
          class="tw-self-center tw-mt-2"
          data-testid="edit-custom-field-button"
+          [disabled]="parentFormDisabled"
          *ngIf="canEdit(field.value.type)"
        ></button>

@@ -104,6 +105,7 @@
          [label]="'reorderToggleButton' | i18n: field.value.name"
          (keydown)="handleKeyDown($event, field.value.name, i)"
          data-testid="reorder-toggle-button"
+          [disabled]="parentFormDisabled"
          *ngIf="canEdit(field.value.type)"
        ></button>
      </div>
@@ -113,7 +115,8 @@
        bitLink
        linkType="primary"
        (click)="openAddEditCustomFieldDialog()"
-        *ngIf="!isPartialEdit"
+        data-testid="add-field-button"
+        *ngIf="!isPartialEdit && !parentFormDisabled"
      >
        <i class="bwi bwi-plus tw-font-bold" aria-hidden="true"></i>
        {{ "addField" | i18n }}
--- a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts
+++ b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts
@@ -4,6 +4,7 @@ import { DebugElement } from "@angular/core";
 import { ComponentFixture, TestBed } from "@angular/core/testing";
 import { By } from "@angular/platform-browser";
 import { mock } from "jest-mock-extended";
+import { BehaviorSubject } from "rxjs";

 import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
@@ -16,7 +17,12 @@ import {
 } from "@bitwarden/common/vault/enums";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 import { FieldView } from "@bitwarden/common/vault/models/view/field.view";
-import { DialogRef, BitPasswordInputToggleDirective, DialogService } from "@bitwarden/components";
+import {
+  DialogRef,
+  BitPasswordInputToggleDirective,
+  DialogService,
+  BitIconButtonComponent,
+} from "@bitwarden/components";

 import { CipherFormConfig } from "../../abstractions/cipher-form-config.service";
 import { CipherFormContainer } from "../../cipher-form-container";
@@ -39,6 +45,7 @@ describe("CustomFieldsComponent", () => {
  let announce: jest.Mock;
  let patchCipher: jest.Mock;
  let config: CipherFormConfig;
+  const formStatusChange$ = new BehaviorSubject<"disabled" | "enabled">("enabled");

  beforeEach(async () => {
    open = jest.fn();
@@ -65,6 +72,7 @@ describe("CustomFieldsComponent", () => {
            registerChildForm: jest.fn(),
            config,
            getInitialCipherView: jest.fn(() => originalCipherView),
+            formStatusChange$,
          },
        },
        {
@@ -552,4 +560,54 @@ describe("CustomFieldsComponent", () => {
      expect(editButtons).toHaveLength(3);
    });
  });
+
+  describe("parent form disabled", () => {
+    beforeEach(() => {
+      originalCipherView!.fields = mockFieldViews;
+      formStatusChange$.next("disabled");
+      component.ngOnInit();
+
+      fixture.detectChanges();
+    });
+
+    afterEach(() => {
+      formStatusChange$.next("enabled");
+      fixture.detectChanges();
+    });
+
+    it("disables edit and reorder buttons", () => {
+      const reorderButtonQuery = By.directive(BitIconButtonComponent);
+      const editButtonQuery = By.directive(BitIconButtonComponent);
+
+      let reorderButton = fixture.debugElement.query(reorderButtonQuery);
+      let editButton = fixture.debugElement.query(editButtonQuery);
+
+      expect(reorderButton.componentInstance.disabled()).toBe(true);
+      expect(editButton.componentInstance.disabled()).toBe(true);
+
+      formStatusChange$.next("enabled");
+      fixture.detectChanges();
+
+      reorderButton = fixture.debugElement.query(reorderButtonQuery);
+      editButton = fixture.debugElement.query(editButtonQuery);
+
+      expect(reorderButton.componentInstance.disabled()).toBe(false);
+      expect(editButton.componentInstance.disabled()).toBe(false);
+    });
+
+    it("hides add field button", () => {
+      const query = By.css('button[data-testid="add-field-button"]');
+
+      let addFieldButton = fixture.debugElement.query(query);
+
+      expect(addFieldButton).toBeNull();
+
+      formStatusChange$.next("enabled");
+      fixture.detectChanges();
+
+      addFieldButton = fixture.debugElement.query(query);
+
+      expect(addFieldButton).not.toBeNull();
+    });
+  });
 });
--- a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts
+++ b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts
@@ -113,6 +113,9 @@ export class CustomFieldsComponent implements OnInit, AfterViewInit {
  /** Emits when a new custom field should be focused */
  private focusOnNewInput$ = new Subject<void>();

+  /** Tracks the disabled status of the edit cipher form */
+  protected parentFormDisabled: boolean = false;
+
  disallowHiddenField?: boolean;

  destroyed$: DestroyRef;
@@ -133,6 +136,10 @@ export class CustomFieldsComponent implements OnInit, AfterViewInit {
      // getRawValue ensures disabled fields are included
      this.updateCipher(this.fields.getRawValue());
    });
+
+    this.cipherFormContainer.formStatusChange$.pipe(takeUntilDestroyed()).subscribe((status) => {
+      this.parentFormDisabled = status === "disabled";
+    });
  }

  /** Fields form array, referenced via a getter to avoid type-casting in multiple places  */
--- a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.html
+++ b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.html
@@ -11,6 +11,7 @@
      [attr.aria-checked]="itemDetailsForm.value.favorite"
      [label]="'favorite' | i18n"
      (click)="toggleFavorite()"
+      [disabled]="favoriteButtonDisabled"
    ></button>
  </bit-section-header>
  <bit-card>
--- a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts
+++ b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts
@@ -82,6 +82,8 @@ export class ItemDetailsSectionComponent implements OnInit {

  protected userId: UserId;

+  protected favoriteButtonDisabled = false;
+
  @Input({ required: true })
  config: CipherFormConfig;

@@ -241,15 +243,19 @@ export class ItemDetailsSectionComponent implements OnInit {
  /**
   * When the cipher does not belong to an organization but the user's organization
   * requires all ciphers to be owned by an organization, disable the entire form
-   * until the user selects an organization.
+   * until the user selects an organization. Once the organization is set, enable the form.
+   * Ensure to properly set the collections control state when the form is enabled.
   */
  private setFormState() {
    if (this.config.originalCipher && !this.allowPersonalOwnership) {
      if (this.itemDetailsForm.controls.organizationId.value === null) {
        this.cipherFormContainer.disableFormFields();
        this.itemDetailsForm.controls.organizationId.enable();
+        this.favoriteButtonDisabled = true;
      } else {
        this.cipherFormContainer.enableFormFields();
+        this.favoriteButtonDisabled = false;
+        this.setCollectionControlState();
      }
    }
  }
@@ -305,7 +311,6 @@ export class ItemDetailsSectionComponent implements OnInit {
    });

    const orgId = this.itemDetailsForm.controls.organizationId.value as OrganizationId;
-    const organization = this.organizations.find((o) => o.id === orgId);
    const initializedWithCachedCipher = this.cipherFormContainer.initializedWithCachedCipher();

    // Configure form for clone mode.
@@ -327,9 +332,7 @@ export class ItemDetailsSectionComponent implements OnInit {

    await this.updateCollectionOptions(prefillCollections);

-    if (!organization?.canEditAllCiphers && !prefillCipher.canAssignToCollections) {
-      this.itemDetailsForm.controls.collectionIds.disable();
-    }
+    this.setCollectionControlState();

    if (this.partialEdit) {
      this.itemDetailsForm.disable();
@@ -344,22 +347,34 @@ export class ItemDetailsSectionComponent implements OnInit {
            c.readOnly &&
            this.originalCipherView.collectionIds.includes(c.id as CollectionId),
        );
-
-        // When Owners/Admins access setting is turned on.
-        // Disable Collections Options if Owner/Admin does not have Edit/Manage permissions on item
-        // Disable Collections Options if Custom user does not have Edit/Manage permissions on item
-        if (
-          (organization?.allowAdminAccessToAllCollectionItems &&
-            (!this.originalCipherView.viewPassword || !this.originalCipherView.edit)) ||
-          (organization?.type === OrganizationUserType.Custom &&
-            !this.originalCipherView.viewPassword)
-        ) {
-          this.itemDetailsForm.controls.collectionIds.disable();
-        }
      }
    }
  }

+  private setCollectionControlState() {
+    const initialCipherView = this.cipherFormContainer.getInitialCipherView();
+    const orgId = this.itemDetailsForm.controls.organizationId.value as OrganizationId;
+    const organization = this.organizations.find((o) => o.id === orgId);
+    if (!organization || !initialCipherView) {
+      return;
+    }
+    // Disable the collection control if either of the following apply:
+    // 1. The organization does not allow editing all ciphers and the existing cipher cannot be assigned to
+    // collections
+    // 2. When Owners/Admins access setting is turned on.
+    // AND either:
+    //   a. Disable Collections Options if Owner/Admin does not have Edit/Manage permissions on item
+    //   b. Disable Collections Options if Custom user does not have Edit/Manage permissions on item
+    if (
+      (!organization.canEditAllCiphers && !initialCipherView.canAssignToCollections) ||
+      (organization.allowAdminAccessToAllCollectionItems &&
+        (!initialCipherView.viewPassword || !initialCipherView.edit)) ||
+      (organization.type === OrganizationUserType.Custom && !initialCipherView.viewPassword)
+    ) {
+      this.itemDetailsForm.controls.collectionIds.disable();
+    }
+  }
+
  /**
   * Updates the collection options based on the selected organization.
   * @param startingSelection - Optional starting selection of collectionIds to be automatically selected.
--- a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts
+++ b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts
@@ -3,6 +3,7 @@ import { Component } from "@angular/core";
 import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing";
 import { By } from "@angular/platform-browser";
 import { mock, MockProxy } from "jest-mock-extended";
+import { BehaviorSubject } from "rxjs";

 import { AuditService } from "@bitwarden/common/abstractions/audit.service";
 import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service";
@@ -47,6 +48,7 @@ describe("LoginDetailsSectionComponent", () => {
    getInitialCipherView.mockClear();
    cipherFormContainer = mock<CipherFormContainer>({
      getInitialCipherView,
+      formStatusChange$: new BehaviorSubject<"enabled" | "disabled">("enabled"),
      website: "example.com",
    });

--- a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts
+++ b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts
@@ -1,7 +1,7 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
 import { DatePipe, NgIf } from "@angular/common";
-import { Component, inject, OnInit, Optional } from "@angular/core";
+import { Component, DestroyRef, inject, OnInit, Optional } from "@angular/core";
 import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { FormBuilder, ReactiveFormsModule } from "@angular/forms";
 import { map } from "rxjs";
@@ -81,6 +81,8 @@ export class LoginDetailsSectionComponent implements OnInit {
   */
  private existingFido2Credentials?: Fido2CredentialView[];

+  private destroyRef = inject(DestroyRef);
+
  get hasPasskey(): boolean {
    return this.existingFido2Credentials != null && this.existingFido2Credentials.length > 0;
  }
@@ -148,6 +150,19 @@ export class LoginDetailsSectionComponent implements OnInit {
    if (this.cipherFormContainer.config.mode === "partial-edit") {
      this.loginDetailsForm.disable();
    }
+
+    // If the form is enabled, ensure to disable password or TOTP
+    // for hidden password users
+    this.cipherFormContainer.formStatusChange$
+      .pipe(takeUntilDestroyed(this.destroyRef))
+      .subscribe((status) => {
+        if (status === "enabled") {
+          if (!this.viewHiddenFields) {
+            this.loginDetailsForm.controls.password.disable();
+            this.loginDetailsForm.controls.totp.disable();
+          }
+        }
+      });
  }

  private initFromExistingCipher(existingLogin: LoginView) {
--- a/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts
+++ b/libs/vault/src/cipher-form/components/sshkey-section/sshkey-section.component.ts
@@ -98,9 +98,13 @@ export class SshKeySectionComponent implements OnInit {

    // Disable the form if the cipher form container is enabled
    // to prevent user interaction
-    this.cipherFormContainer.formEnabled$
+    this.cipherFormContainer.formStatusChange$
      .pipe(takeUntilDestroyed(this.destroyRef))
-      .subscribe(() => this.sshKeyForm.disable());
+      .subscribe((status) => {
+        if (status === "enabled") {
+          this.sshKeyForm.disable();
+        }
+      });
  }

  /** Set form initial form values from the current cipher */