From 2bf88c50081c6b2b2f297d32707dfc50f62c6759 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Fri, 11 Apr 2025 15:51:59 -0700 Subject: [PATCH] add a flow to InputPasswordFlow enum, and a method to verify the flow and presence of a userId --- .../complete-trial-initiation.component.html | 4 +- .../complete-trial-initiation.component.ts | 2 +- apps/web/src/app/core/core.module.ts | 1 - .../src/services/jslib-services.module.ts | 1 - .../change-password.component.html | 2 +- .../change-password.component.ts | 4 +- .../input-password.component.html | 8 +- .../input-password.component.ts | 180 ++++++++++++------ .../registration-finish.component.html | 2 +- .../registration-finish.component.ts | 3 +- .../set-password-jit.component.html | 2 +- .../set-password-jit.component.ts | 2 +- .../change-password.service.abstraction.ts | 2 + .../default-change-password.service.ts | 12 +- 14 files changed, 140 insertions(+), 85 deletions(-) diff --git a/apps/web/src/app/billing/trial-initiation/complete-trial-initiation/complete-trial-initiation.component.html b/apps/web/src/app/billing/trial-initiation/complete-trial-initiation/complete-trial-initiation.component.html index 8118b1e512d..b5238e3361c 100644 --- a/apps/web/src/app/billing/trial-initiation/complete-trial-initiation/complete-trial-initiation.component.html +++ b/apps/web/src/app/billing/trial-initiation/complete-trial-initiation/complete-trial-initiation.component.html @@ -1,6 +1,6 @@
{{ "currentMasterPass" | i18n }} @@ -90,9 +90,7 @@ {{ "checkForBreaches" | i18n }} - + (); @Output() onSecondaryButtonClick = new EventEmitter(); - @Input({ required: true }) inputPasswordFlow!: InputPasswordFlow; - @Input({ required: true }) email!: string; - @Input({ required: true }) userId!: UserId; + @Input({ required: true }) flow!: InputPasswordFlow; + @Input({ required: true, transform: (val: string) => val.trim().toLowerCase() }) email!: string; + @Input() userId?: UserId; @Input() loading = false; @Input() masterPasswordPolicyOptions: MasterPasswordPolicyOptions | null = null; @@ -133,13 +139,24 @@ export class InputPasswordComponent implements OnInit { compareInputs( ValidationGoal.InputsShouldNotMatch, "newPassword", - "hint", + "newPasswordHint", this.i18nService.t("hintEqualsPassword"), ), ], }, ); + protected get minPasswordLengthMsg() { + if ( + this.masterPasswordPolicyOptions != null && + this.masterPasswordPolicyOptions.minLength > 0 + ) { + return this.i18nService.t("characterMinimum", this.masterPasswordPolicyOptions.minLength); + } else { + return this.i18nService.t("characterMinimum", this.minPasswordLength); + } + } + constructor( private auditService: AuditService, private cipherService: CipherService, @@ -155,9 +172,14 @@ export class InputPasswordComponent implements OnInit { ) {} ngOnInit(): void { + this.addFormFieldsIfNecessary(); + this.setButtonText(); + } + + private addFormFieldsIfNecessary() { if ( - this.inputPasswordFlow === InputPasswordFlow.ChangePassword || - this.inputPasswordFlow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation + this.flow === InputPasswordFlow.ChangePassword || + this.flow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation ) { // https://github.com/angular/angular/issues/48794 (this.formGroup as FormGroup).addControl( @@ -166,14 +188,16 @@ export class InputPasswordComponent implements OnInit { ); } - if (this.inputPasswordFlow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation) { + if (this.flow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation) { // https://github.com/angular/angular/issues/48794 (this.formGroup as FormGroup).addControl( "rotateUserKey", this.formBuilder.control(false), ); } + } + private setButtonText() { if (this.primaryButtonText) { this.primaryButtonTextStr = this.i18nService.t( this.primaryButtonText.key, @@ -189,22 +213,9 @@ export class InputPasswordComponent implements OnInit { } } - get minPasswordLengthMsg() { - if ( - this.masterPasswordPolicyOptions != null && - this.masterPasswordPolicyOptions.minLength > 0 - ) { - return this.i18nService.t("characterMinimum", this.masterPasswordPolicyOptions.minLength); - } else { - return this.i18nService.t("characterMinimum", this.minPasswordLength); - } - } - - getPasswordStrengthScore(score: PasswordStrengthScore) { - this.passwordStrengthScore = score; - } - protected submit = async () => { + this.verifyFlowAndUserId(); + this.formGroup.markAllAsTouched(); if (this.formGroup.invalid) { @@ -216,45 +227,50 @@ export class InputPasswordComponent implements OnInit { throw new Error("Email is required to create master key."); } - this.kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(this.userId)); - if (this.kdfConfig == null) { - throw new Error("KdfConfig is required to create master key."); - } - const currentPassword = this.formGroup.get("currentPassword")?.value || ""; const newPassword = this.formGroup.controls.newPassword.value; const newPasswordHint = this.formGroup.controls.newPasswordHint.value; const checkForBreaches = this.formGroup.controls.checkForBreaches.value; - // 1. Verify current password is correct (if necessary) + // 1. Determine kdfConfig + if (this.flow === InputPasswordFlow.AccountRegistration) { + this.kdfConfig = DEFAULT_KDF_CONFIG; + } else { + this.kdfConfig = await firstValueFrom(this.kdfConfigService.getKdfConfig$(this.userId)); + } + + if (this.kdfConfig == null) { + throw new Error("KdfConfig is required to create master key."); + } + + // 2. Verify current password is correct (if necessary) if ( - this.inputPasswordFlow === InputPasswordFlow.ChangePassword || - this.inputPasswordFlow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation + this.flow === InputPasswordFlow.ChangePassword || + this.flow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation ) { - const currentPasswordIsCorrect = await this.verifyCurrentPassword( + const currentPasswordVerified = await this.verifyCurrentPassword( currentPassword, - this.userId, this.kdfConfig, ); - if (!currentPasswordIsCorrect) { + if (!currentPasswordVerified) { return; } } - // 2. Evaluate new password - const newPasswordEvaluatedSuccessfully = await this.evaluateNewPassword( + // 3. Verify new password + const newPasswordVerified = await this.verifyNewPassword( newPassword, this.passwordStrengthScore, checkForBreaches, ); - if (!newPasswordEvaluatedSuccessfully) { + if (!newPasswordVerified) { return; } - // 3. Create cryptographic keys + // 4. Create cryptographic keys and build a PasswordInputResult object const newMasterKey = await this.keyService.makeMasterKey( newPassword, - this.email.trim().toLowerCase(), + this.email, this.kdfConfig, ); @@ -280,12 +296,12 @@ export class InputPasswordComponent implements OnInit { }; if ( - this.inputPasswordFlow === InputPasswordFlow.ChangePassword || - this.inputPasswordFlow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation + this.flow === InputPasswordFlow.ChangePassword || + this.flow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation ) { const currentMasterKey = await this.keyService.makeMasterKey( currentPassword, - this.email.trim().toLowerCase(), + this.email, this.kdfConfig, ); @@ -307,31 +323,66 @@ export class InputPasswordComponent implements OnInit { passwordInputResult.currentLocalMasterKeyHash = currentLocalMasterKeyHash; } - if (this.inputPasswordFlow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation) { + if (this.flow === InputPasswordFlow.ChangePasswordWithOptionalUserKeyRotation) { passwordInputResult.rotateUserKey = this.formGroup.get("rotateUserKey")?.value; } - // 4. Emit cryptographic keys and other password related properties + // 5. Emit cryptographic keys and other password related properties this.onPasswordFormSubmit.emit(passwordInputResult); }; /** - * Returns true if the current password is correct, false otherwise + * This method prevents a dev from passing down the wrong `InputPasswordFlow` + * from the parent component or from failing to pass down a `userId` for flows + * that require it. + * + * We cannot mark the `userId` `@Input` as required because in an account registration + * flow we will not have an active account `userId` to pass down. + */ + private verifyFlowAndUserId() { + /** + * There can be no active account (and thus no userId) in an account registration + * flow. If there is a userId, it means the dev passed down the wrong InputPasswordFlow + * from the parent component. + */ + if (this.flow === InputPasswordFlow.AccountRegistration) { + if (this.userId) { + throw new Error( + "There can be no userId in an account registration flow. Please pass down the appropriate InputPasswordFlow from the parent component.", + ); + } + } + + /** + * There MUST be an active account (and thus a userId) in all other flows. + * If no userId is passed down, it means the dev either: + * (a) passed down the wrong InputPasswordFlow, or + * (b) passed down the correct InputPasswordFlow but failed to pass down a userId + */ + if (this.flow !== InputPasswordFlow.AccountRegistration) { + if (!this.userId) { + throw new Error("The selected InputPasswordFlow requires that a userId be passed down"); + } + } + } + + /** + * Returns `true` if the current password is correct (it can be used to successfully decrypt + * the masterKeyEncrypedUserKey), `false` otherwise */ private async verifyCurrentPassword( currentPassword: string, - userId: UserId, kdfConfig: KdfConfig, ): Promise { const currentMasterKey = await this.keyService.makeMasterKey( currentPassword, - this.email.trim().toLowerCase(), + this.email, kdfConfig, ); const decryptedUserKey = await this.masterPasswordService.decryptUserKeyWithMasterKey( currentMasterKey, - userId, + this.userId, ); if (decryptedUserKey == null) { @@ -348,9 +399,10 @@ export class InputPasswordComponent implements OnInit { } /** - * Returns true if the new password passes all checks, false otherwise + * Returns `true` if the new password is not weak or breached and it passes + * any enforced org policy options, `false` otherwise */ - private async evaluateNewPassword( + private async verifyNewPassword( newPassword: string, passwordStrengthScore: PasswordStrengthScore, checkForBreaches: boolean, @@ -414,7 +466,7 @@ export class InputPasswordComponent implements OnInit { return true; } - async rotateUserKeyClicked() { + protected async rotateUserKeyClicked() { const rotateUserKeyCtrl = this.formGroup.get( "rotateUserKey", ) as unknown as FormControl; @@ -468,4 +520,8 @@ export class InputPasswordComponent implements OnInit { } } } + + protected getPasswordStrengthScore(score: PasswordStrengthScore) { + this.passwordStrengthScore = score; + } } diff --git a/libs/auth/src/angular/registration/registration-finish/registration-finish.component.html b/libs/auth/src/angular/registration/registration-finish/registration-finish.component.html index ccc502bd7b6..aa6b5c8edc3 100644 --- a/libs/auth/src/angular/registration/registration-finish/registration-finish.component.html +++ b/libs/auth/src/angular/registration/registration-finish/registration-finish.component.html @@ -5,7 +5,7 @@ (); - InputPasswordFlow = InputPasswordFlow; - + inputPasswordFlow = InputPasswordFlow.AccountRegistration; loading = true; submitting = false; email: string; diff --git a/libs/auth/src/angular/set-password-jit/set-password-jit.component.html b/libs/auth/src/angular/set-password-jit/set-password-jit.component.html index 3a4956ef7e9..f7c2b144cd1 100644 --- a/libs/auth/src/angular/set-password-jit/set-password-jit.component.html +++ b/libs/auth/src/angular/set-password-jit/set-password-jit.component.html @@ -13,7 +13,7 @@ ; } diff --git a/libs/auth/src/common/services/change-password/default-change-password.service.ts b/libs/auth/src/common/services/change-password/default-change-password.service.ts index 5377cdb0ccf..934940cd341 100644 --- a/libs/auth/src/common/services/change-password/default-change-password.service.ts +++ b/libs/auth/src/common/services/change-password/default-change-password.service.ts @@ -1,10 +1,8 @@ -import { firstValueFrom } from "rxjs"; - -import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { Account } from "@bitwarden/common/auth/abstractions/account.service"; import { MasterPasswordApiService } from "@bitwarden/common/auth/abstractions/master-password-api.service.abstraction"; import { PasswordRequest } from "@bitwarden/common/auth/models/request/password.request"; -import { getUserId } from "@bitwarden/common/auth/services/account.service"; import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction"; +import { UserId } from "@bitwarden/common/types/guid"; import { MasterKey } from "@bitwarden/common/types/key"; import { KeyService } from "@bitwarden/key-management"; @@ -12,7 +10,6 @@ import { ChangePasswordService } from "../../abstractions"; export class DefaultChangePasswordService implements ChangePasswordService { constructor( - private accountService: AccountService, private keyService: KeyService, private masterPasswordApiService: MasterPasswordApiService, private masterPasswordService: InternalMasterPasswordServiceAbstraction, @@ -40,8 +37,11 @@ export class DefaultChangePasswordService implements ChangePasswordService { newPasswordHint: string, newMasterKey: MasterKey, newServerMasterKeyHash: string, + userId: UserId, ) { - const userId = await firstValueFrom(getUserId(this.accountService.activeAccount$)); + if (!userId) { + throw new Error("The change password process requires a userId"); + } const decryptedUserKey = await this.masterPasswordService.decryptUserKeyWithMasterKey( currentMasterKey,