-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-18721][PM-21272] Integrate InputPasswordComponent in AccountRecoveryDialogComponent #14662
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: auth/pm-18721/integrate-input-password-component-in-dialogs
Are you sure you want to change the base?
Changes from all commits
9606947
0c17789
5c80984
787fddf
aa64267
7ca7918
42e56d0
db7a401
9360bb0
9b13ff5
5d7ceac
53d4a14
09128e2
1fccdde
60e2bdc
801f5b1
03415ea
45f8c19
1ae47e4
d562acc
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 |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<bit-dialog [title]="'recoverAccount' | i18n" [subtitle]="dialogData.name"> | ||
<ng-container bitDialogContent> | ||
<bit-callout type="warning" | ||
>{{ "resetPasswordLoggedOutWarning" | i18n: loggedOutWarningName }} | ||
</bit-callout> | ||
|
||
<auth-input-password | ||
[flow]="inputPasswordFlow" | ||
[masterPasswordPolicyOptions]="masterPasswordPolicyOptions$ | async" | ||
(onPasswordFormSubmit)="handlePasswordFormSubmit($event)" | ||
(isSubmitting)="handleIsSubmittingChange($event)" | ||
></auth-input-password> | ||
</ng-container> | ||
|
||
<ng-container bitDialogFooter> | ||
<button | ||
type="button" | ||
bitButton | ||
buttonType="primary" | ||
[disabled]="submitting$ | async" | ||
(click)="handlePrimaryButtonClick()" | ||
> | ||
{{ "save" | i18n }} | ||
</button> | ||
<button type="button" bitButton buttonType="secondary" bitDialogClose> | ||
{{ "cancel" | i18n }} | ||
</button> | ||
</ng-container> | ||
</bit-dialog> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
import { CommonModule } from "@angular/common"; | ||
import { Component, Inject, ViewChild } from "@angular/core"; | ||
import { BehaviorSubject, combineLatest, map, switchMap } from "rxjs"; | ||
Check warning on line 3 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
import { | ||
Check warning on line 5 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
InputPasswordComponent, | ||
InputPasswordFlow, | ||
PasswordInputResult, | ||
} from "@bitwarden/auth/angular"; | ||
import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.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"; | ||
Check warning on line 14 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
import { OrganizationId } from "@bitwarden/common/types/guid"; | ||
import { | ||
Check warning on line 16 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
ButtonModule, | ||
CalloutModule, | ||
DIALOG_DATA, | ||
DialogConfig, | ||
DialogModule, | ||
DialogRef, | ||
DialogService, | ||
ToastService, | ||
} from "@bitwarden/components"; | ||
import { I18nPipe } from "@bitwarden/ui-common"; | ||
Check warning on line 26 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
import { OrganizationUserResetPasswordService } from "../../services/organization-user-reset-password/organization-user-reset-password.service"; | ||
Check warning on line 28 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
/** | ||
* Encapsulates a few key data inputs needed to initiate an account recovery | ||
* process for the organization user in question. | ||
*/ | ||
export type AccountRecoveryDialogData = { | ||
/** | ||
* The organization user's full name | ||
*/ | ||
name: string; | ||
|
||
/** | ||
* The organization user's email address | ||
*/ | ||
email: string; | ||
|
||
/** | ||
* The `organizationUserId` for the user | ||
*/ | ||
organizationUserId: string; | ||
|
||
/** | ||
* The organization's `organizationId` | ||
*/ | ||
organizationId: OrganizationId; | ||
}; | ||
|
||
export const AccountRecoveryDialogResultType = { | ||
Check warning on line 56 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
Ok: "ok", | ||
} as const; | ||
|
||
export type AccountRecoveryDialogResultType = | ||
(typeof AccountRecoveryDialogResultType)[keyof typeof AccountRecoveryDialogResultType]; | ||
|
||
/** | ||
* Used in a dialog for initiating the account recovery process against a | ||
* given organization user. An admin will access this form when they want to | ||
* reset a user's password and log them out of sessions. | ||
*/ | ||
@Component({ | ||
standalone: true, | ||
selector: "app-account-recovery-dialog", | ||
templateUrl: "account-recovery-dialog.component.html", | ||
imports: [ | ||
ButtonModule, | ||
CalloutModule, | ||
CommonModule, | ||
DialogModule, | ||
I18nPipe, | ||
InputPasswordComponent, | ||
], | ||
}) | ||
export class AccountRecoveryDialogComponent { | ||
Check warning on line 81 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
@ViewChild(InputPasswordComponent) | ||
inputPasswordComponent: InputPasswordComponent | undefined = undefined; | ||
Check warning on line 83 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
private parentSubmittingBehaviorSubject = new BehaviorSubject(false); | ||
parentSubmitting$ = this.parentSubmittingBehaviorSubject.asObservable(); | ||
Check warning on line 86 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
private childSubmittingBehaviorSubject = new BehaviorSubject(false); | ||
childSubmitting$ = this.childSubmittingBehaviorSubject.asObservable(); | ||
Check warning on line 89 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
submitting$ = combineLatest([this.parentSubmitting$, this.childSubmitting$]).pipe( | ||
Check warning on line 91 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
map(([parentIsSubmitting, childIsSubmitting]) => parentIsSubmitting || childIsSubmitting), | ||
); | ||
|
||
masterPasswordPolicyOptions$ = this.accountService.activeAccount$.pipe( | ||
Check warning on line 95 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
getUserId, | ||
switchMap((userId) => this.policyService.masterPasswordPolicyOptions$(userId)), | ||
Check warning on line 97 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
); | ||
|
||
inputPasswordFlow = InputPasswordFlow.ChangePasswordDelegation; | ||
Check warning on line 100 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
get loggedOutWarningName() { | ||
return this.dialogData.name != null ? this.dialogData.name : this.i18nService.t("thisUser"); | ||
} | ||
|
||
constructor( | ||
@Inject(DIALOG_DATA) protected dialogData: AccountRecoveryDialogData, | ||
private accountService: AccountService, | ||
private dialogRef: DialogRef<AccountRecoveryDialogResultType>, | ||
private i18nService: I18nService, | ||
private logService: LogService, | ||
private policyService: PolicyService, | ||
private resetPasswordService: OrganizationUserResetPasswordService, | ||
private toastService: ToastService, | ||
Check warning on line 114 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
) {} | ||
|
||
handlePrimaryButtonClick = async () => { | ||
Check warning on line 117 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
if (!this.inputPasswordComponent) { | ||
throw new Error("InputPasswordComponent is not initialized"); | ||
Check warning on line 119 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
} | ||
|
||
await this.inputPasswordComponent.submit(); | ||
Check warning on line 122 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
}; | ||
|
||
async handlePasswordFormSubmit(passwordInputResult: PasswordInputResult) { | ||
this.parentSubmittingBehaviorSubject.next(true); | ||
Check warning on line 126 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
|
||
try { | ||
await this.resetPasswordService.resetMasterPassword( | ||
Check warning on line 129 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
passwordInputResult.newPassword, | ||
this.dialogData.email, | ||
this.dialogData.organizationUserId, | ||
this.dialogData.organizationId, | ||
); | ||
|
||
this.toastService.showToast({ | ||
Check warning on line 136 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
variant: "success", | ||
title: "", | ||
message: this.i18nService.t("resetPasswordSuccess"), | ||
}); | ||
} catch (e) { | ||
this.logService.error(e); | ||
Check warning on line 142 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
} finally { | ||
this.parentSubmittingBehaviorSubject.next(false); | ||
Check warning on line 144 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
} | ||
|
||
this.dialogRef.close(AccountRecoveryDialogResultType.Ok); | ||
Check warning on line 147 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
} | ||
|
||
protected handleIsSubmittingChange(isSubmitting: boolean) { | ||
this.childSubmittingBehaviorSubject.next(isSubmitting); | ||
Check warning on line 151 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
} | ||
|
||
/** | ||
* Strongly typed helper to open an `AccountRecoveryDialogComponent` | ||
* @param dialogService Instance of the dialog service that will be used to open the dialog | ||
* @param dialogConfig Configuration for the dialog | ||
*/ | ||
static open = ( | ||
Check warning on line 159 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
dialogService: DialogService, | ||
dialogConfig: DialogConfig< | ||
AccountRecoveryDialogData, | ||
DialogRef<AccountRecoveryDialogResultType, unknown> | ||
>, | ||
) => { | ||
return dialogService.open<AccountRecoveryDialogResultType, AccountRecoveryDialogData>( | ||
Check warning on line 166 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/account-recovery-dialog.component.ts
|
||
AccountRecoveryDialogComponent, | ||
dialogConfig, | ||
); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from "./account-recovery-dialog.component"; | ||
Check warning on line 1 in apps/web/src/app/admin-console/organizations/members/components/account-recovery/index.ts
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ import { EncryptService } from "@bitwarden/common/key-management/crypto/abstract | |
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; | ||
import { Utils } from "@bitwarden/common/platform/misc/utils"; | ||
import { EncryptedString, EncString } from "@bitwarden/common/platform/models/domain/enc-string"; | ||
import { UserId } from "@bitwarden/common/types/guid"; | ||
import { OrganizationId, UserId } from "@bitwarden/common/types/guid"; | ||
import { UserKey } from "@bitwarden/common/types/key"; | ||
import { | ||
Argon2KdfConfig, | ||
|
@@ -96,7 +96,7 @@ export class OrganizationUserResetPasswordService | |
newMasterPassword: string, | ||
email: string, | ||
orgUserId: string, | ||
orgId: string, | ||
orgId: OrganizationId, | ||
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. When I updated the Since I'd rather changes not leak outside the feature flag, would it be better for me to set the param type in |
||
): Promise<void> { | ||
const response = await this.organizationUserApiService.getOrganizationUserResetPasswordDetails( | ||
orgId, | ||
|
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.
You've fixed the race condition, but I think this is still a lot of boilerplate just to handle a form submission. I assume this would have to be repeated in every parent component that wants to use this child component.
Does the child need to know about the form submit state at all? The submission process starts when
handlePrimaryButtonClick
is called, and it ends whenhandlePasswordFormSubmit
is finished, or if the child emits an error. If that's the case, the parent should be able to handle all of that by itself, which is also better design because the parent owns the form.๐ญ I also like the suggestion in Slack:
It does feel a bit unusual that the child is handling the parent's submit action. There's quite a bit of logic in that child component, what if it were extracted to a service that the parent could call (thereby handling its own form submission), and the child is more presentational, simply emitting form values? That's a fairly large change so it doesn't need to be done in this PR (if at all), just mentioning it as feedback.
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.
The submit process can also end if the child returns early, without emitting. That means
handlePasswordFormSubmit
would never get called in the parent, leaving is in a perpetual "submitting" state.That is, if we merely set
submitting
totrue
inhandlePrimaryButtonClick
, and then set it tofalse
inhandlePasswordFormSubmit
, then thatfalse
set would never get called in a case whereInputPasswordComponent.submit()
returns early without emitting. That's the case I was trying to solve in my previous implementation here (but had the race condition).cc: @JaredSnider-Bitwarden let me know if you have any other thoughts on this.
Uh oh!
There was an error while loading. Please reload this page.
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.
Yea the
InputPasswordComponent
has grown in scope over time. I will do a retro soon to investigate if/how we could potentially refactor things.