-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[PM-31364] Discard edit bug fixes #20511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8be194a
3f19efb
4be9e6d
da83145
23e345d
a0ebeb8
15c5e4d
9d17b30
350aacc
8c7f83e
d4084b2
be57533
85c11fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,35 @@ | ||
| import { Injectable } from "@angular/core"; | ||
| import { CanDeactivate } from "@angular/router"; | ||
| import { firstValueFrom } from "rxjs"; | ||
|
|
||
| import { PoliciesComponent } from "./policies.component"; | ||
| import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; | ||
| import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; | ||
| import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; | ||
|
|
||
| // `import type` avoids a circular runtime dependency: the guard only needs | ||
| // PoliciesComponent as a TypeScript type (for the CanDeactivate generic and | ||
| // the canDeactivate() parameter), never as a runtime value. | ||
| import type { PoliciesComponent } from "./policies.component"; | ||
|
|
||
| @Injectable({ providedIn: "root" }) | ||
| export class PoliciesDeactivateGuard implements CanDeactivate<PoliciesComponent> { | ||
| canDeactivate(component: PoliciesComponent): Promise<boolean> { | ||
| constructor( | ||
| private readonly accountService: AccountService, | ||
| private readonly authService: AuthService, | ||
| ) {} | ||
|
|
||
| async canDeactivate(component: PoliciesComponent): Promise<boolean> { | ||
| // If the user is already locked or logged out (e.g. during a lock/logout flow), | ||
| // always allow navigation so the discard-edits dialog is never shown. | ||
| // Guard against a null active account (switchAccount(null) during logout). | ||
| const account = await firstValueFrom(this.accountService.activeAccount$); | ||
| if (account?.id == null) { | ||
| return true; | ||
| } | ||
| const status = await firstValueFrom(this.authService.authStatusFor$(account.id)); | ||
| if (status !== AuthenticationStatus.Unlocked) { | ||
| return true; | ||
| } | ||
| return component.canDeactivate(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,6 @@ import { | |
| ChangeDetectorRef, | ||
| Component, | ||
| DestroyRef, | ||
| HostListener, | ||
| Inject, | ||
| Signal, | ||
| ViewContainerRef, | ||
|
|
@@ -15,13 +14,15 @@ import { | |
| } from "@angular/core"; | ||
| import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; | ||
| import { FormBuilder } from "@angular/forms"; | ||
| import { map, firstValueFrom, switchMap, filter } from "rxjs"; | ||
| import { map, firstValueFrom, switchMap, filter, of } from "rxjs"; | ||
|
|
||
| import { PolicyApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/policy/policy-api.service.abstraction"; | ||
| import { PolicyType } from "@bitwarden/common/admin-console/enums"; | ||
| import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; | ||
| import { PolicyResponse } from "@bitwarden/common/admin-console/models/response/policy.response"; | ||
| import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; | ||
| import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; | ||
| import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; | ||
| import { getUserId } from "@bitwarden/common/auth/services/account.service"; | ||
| import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; | ||
| import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; | ||
|
|
@@ -86,6 +87,7 @@ export class PolicyEditDialogComponent implements AfterViewInit { | |
| protected readonly dialogService: DialogService, | ||
| protected readonly cdkDialogRef: CdkDialogRef, | ||
| protected readonly configService: ConfigService, | ||
| private readonly authService: AuthService, | ||
| ) {} | ||
|
|
||
| get policy(): BasePolicyEditDefinition { | ||
|
|
@@ -94,7 +96,10 @@ export class PolicyEditDialogComponent implements AfterViewInit { | |
|
|
||
| private isFormDirty(): boolean { | ||
| const component = this.policyComponent(); | ||
| return (component?.enabled?.dirty ?? false) || (component?.data?.dirty ?? false); | ||
| if (!component) { | ||
| return false; | ||
| } | ||
| return component.enabled.dirty || (component.data?.dirty ?? false); | ||
| } | ||
|
|
||
| private readonly discardDialogOptions = { | ||
|
|
@@ -143,8 +148,38 @@ export class PolicyEditDialogComponent implements AfterViewInit { | |
| if (result !== undefined || !this.isFormDirty()) { | ||
| return true; | ||
| } | ||
| return this.dialogService.openSimpleDialog(this.discardDialogOptions); | ||
| const confirmed = await this.dialogService.openSimpleDialog(this.discardDialogOptions); | ||
| if (confirmed) { | ||
| // Disarm the beforeunload handler immediately. Angular won't destroy this component | ||
| // until its next change-detection cycle, but a lock/logout can trigger | ||
| // window.location.reload() before that cycle runs. Clearing the flag here ensures | ||
| // the handler doesn't block the reload after the user has already confirmed discard. | ||
| this.discardGuardEnabled.set(false); | ||
| } | ||
|
Comment on lines
+152
to
+158
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ DEBT: Comment references a DetailsThis same PR removes the The same stale rationale appears at lines 162–166 ("so the browser's beforeunload dialog cannot block the lock/logout reload") for the auth-status subscription — worth aligning that comment too. |
||
| return confirmed; | ||
| }; | ||
|
|
||
| // When the vault is locked or the user is logged out, disarm both guards immediately | ||
| // so the browser's beforeunload dialog cannot block the lock/logout reload, and so | ||
| // the closePredicate won't show the discard dialog during the subsequent router teardown. | ||
| // If the active account becomes null (switchAccount(null) during logout), treat that | ||
| // as a non-Unlocked state and disarm as well. | ||
| this.accountService.activeAccount$ | ||
| .pipe( | ||
| switchMap((account) => { | ||
| if (account?.id == null) { | ||
| return of(null); // no active account — disarm immediately | ||
| } | ||
| return this.authService | ||
| .authStatusFor$(account.id) | ||
| .pipe(filter((status) => status !== AuthenticationStatus.Unlocked)); | ||
| }), | ||
| takeUntilDestroyed(this.destroyRef), | ||
| ) | ||
| .subscribe(() => { | ||
| this.discardGuardEnabled.set(false); | ||
| this.dialogRef.closePredicate = undefined; | ||
| }); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -161,14 +196,6 @@ export class PolicyEditDialogComponent implements AfterViewInit { | |
| } | ||
| }; | ||
|
|
||
| @HostListener("window:beforeunload", ["$event"]) | ||
| onBeforeUnload(event: BeforeUnloadEvent): void { | ||
| if (this.discardGuardEnabled() && this.isFormDirty()) { | ||
| event.preventDefault(); | ||
| event.returnValue = ""; | ||
| } | ||
| } | ||
|
|
||
| async ngAfterViewInit() { | ||
| const policyResponse = await this.load(); | ||
| this.loading.set(false); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AuthServiceconstructor dependency breaks existing tests.Details and fix
PolicyEditDialogComponentandMultiStepPolicyEditDialogComponentnow requireAuthServicevia constructor injection (not@Optional()), but neitherpolicy-edit-dialog.component.spec.tsnormulti-step-policy-edit-dialog.component.spec.tsprovides one inTestBed.configureTestingModule.TestBed.createComponent(...)in the existingbeforeEachblocks will throwNullInjectorError: No provider for AuthService!, failing every test in both spec files.Fix: add a mock provider in both spec files, e.g.:
Also consider provisioning
authService.authStatusFor$to return a non-emitting observable (e.g.NEVERorof(AuthenticationStatus.Unlocked)) so the new subscription insetupDiscardGuarddoesn't disrupt other tests ifngAfterViewInitruns.