From fbf9c5abfadd2f57179ce9ee84f3c37aef716192 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Mon, 26 Aug 2024 12:05:22 -0700 Subject: [PATCH] [PM-10993] Browser Refresh - Fix duplicate password generation emissions in Firefox (#10704) * [PM-10993] Avoid saving the generation options to state when toggling the password type * [PM-10993] Fix tests --- .../cipher-form-generator.component.spec.ts | 19 ++++--- .../cipher-form-generator.component.ts | 52 ++++++++++++++----- 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.spec.ts b/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.spec.ts index 5b65c6da24..85ace2f0ac 100644 --- a/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.spec.ts +++ b/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.spec.ts @@ -126,15 +126,22 @@ describe("CipherFormGeneratorComponent", () => { expect(fixture.nativeElement.querySelector("bit-toggle-group")).toBeTruthy(); }); - it("should save password options when the password type is updated", async () => { - mockLegacyPasswordGenerationService.generatePassword.mockResolvedValue("generated-password"); + it("should update the generated value when the password type is updated", fakeAsync(async () => { + mockLegacyPasswordGenerationService.generatePassword + .mockResolvedValueOnce("first-password") + .mockResolvedValueOnce("second-password"); + + component.ngOnChanges(); + tick(); + + expect(component["generatedValue"]).toBe("first-password"); await component["updatePasswordType"]("passphrase"); + tick(); - expect(mockLegacyPasswordGenerationService.saveOptions).toHaveBeenCalledWith({ - type: "passphrase", - }); - }); + expect(component["generatedValue"]).toBe("second-password"); + expect(mockLegacyPasswordGenerationService.generatePassword).toHaveBeenCalledTimes(2); + })); it("should update the password history when a new password is generated", fakeAsync(() => { mockLegacyPasswordGenerationService.generatePassword.mockResolvedValue("new-password"); diff --git a/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.ts b/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.ts index 2d24194d29..7d93ca20d9 100644 --- a/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-generator/cipher-form-generator.component.ts @@ -1,7 +1,18 @@ import { CommonModule } from "@angular/common"; import { Component, DestroyRef, EventEmitter, Input, OnChanges, Output } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { firstValueFrom, map, startWith, Subject, Subscription, switchMap, tap } from "rxjs"; +import { + combineLatest, + map, + merge, + shareReplay, + startWith, + Subject, + Subscription, + switchMap, + take, + tap, +} from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -61,18 +72,13 @@ export class CipherFormGeneratorComponent implements OnChanges { protected regenerateButtonTitle: string; protected regenerate$ = new Subject(); + protected passwordTypeSubject$ = new Subject(); /** * The currently generated value displayed to the user. * @protected */ protected generatedValue: string = ""; - /** - * The current password generation options. - * @private - */ - private passwordOptions$ = this.legacyPasswordGenerationService.getOptions$(); - /** * The current username generation options. * @private @@ -80,10 +86,30 @@ export class CipherFormGeneratorComponent implements OnChanges { private usernameOptions$ = this.legacyUsernameGenerationService.getOptions$(); /** - * The current password type specified by the password generation options. + * The current password type selected in the UI. Starts with the saved value from the service. * @protected */ - protected passwordType$ = this.passwordOptions$.pipe(map(([options]) => options.type)); + protected passwordType$ = merge( + this.legacyPasswordGenerationService.getOptions$().pipe( + take(1), + map(([options]) => options.type), + ), + this.passwordTypeSubject$, + ).pipe(shareReplay({ bufferSize: 1, refCount: false })); + + /** + * The current password generation options. + * @private + */ + private passwordOptions$ = combineLatest([ + this.legacyPasswordGenerationService.getOptions$(), + this.passwordType$, + ]).pipe( + map(([[options], type]) => { + options.type = type; + return options; + }), + ); /** * Tracks the regenerate$ subscription @@ -121,7 +147,7 @@ export class CipherFormGeneratorComponent implements OnChanges { .pipe( startWith(null), switchMap(() => this.passwordOptions$), - switchMap(([options]) => this.legacyPasswordGenerationService.generatePassword(options)), + switchMap((options) => this.legacyPasswordGenerationService.generatePassword(options)), tap(async (password) => { await this.legacyPasswordGenerationService.addHistory(password); }), @@ -148,12 +174,10 @@ export class CipherFormGeneratorComponent implements OnChanges { } /** - * Switch the password generation type and save the options (generating a new password automatically). + * Switch the password generation type. * @param value The new password generation type. */ protected updatePasswordType = async (value: GeneratorType) => { - const [currentOptions] = await firstValueFrom(this.passwordOptions$); - currentOptions.type = value; - await this.legacyPasswordGenerationService.saveOptions(currentOptions); + this.passwordTypeSubject$.next(value); }; }