[PM-11136] Convert LoginEmailService email property to state provider (#10624)

* convert email property to state provider

* update tests

* assign loginEmail to variable before passing in

* remove nav logic in ngOnInit
This commit is contained in:
rr-bw 2024-09-03 09:40:11 -07:00 committed by GitHub
parent ea618c682a
commit b27dc44298
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 58 additions and 54 deletions

View File

@ -41,7 +41,7 @@ export class HomeComponent implements OnInit, OnDestroy {
) {}
async ngOnInit(): Promise<void> {
const email = this.loginEmailService.getEmail();
const email = await firstValueFrom(this.loginEmailService.loginEmail$);
const rememberEmail = this.loginEmailService.getRememberEmail();
if (email != null) {
@ -93,7 +93,7 @@ export class HomeComponent implements OnInit, OnDestroy {
async setLoginEmailValues() {
// Note: Browser saves email settings here instead of the login component
this.loginEmailService.setRememberEmail(this.formGroup.value.rememberEmail);
this.loginEmailService.setEmail(this.formGroup.value.email);
await this.loginEmailService.setLoginEmail(this.formGroup.value.email);
await this.loginEmailService.saveEmailSettings();
}
}

View File

@ -1,4 +1,4 @@
import { Component, NgZone } from "@angular/core";
import { Component, NgZone, OnInit } from "@angular/core";
import { FormBuilder } from "@angular/forms";
import { ActivatedRoute, Router } from "@angular/router";
import { firstValueFrom } from "rxjs";
@ -31,7 +31,7 @@ import { flagEnabled } from "../../platform/flags";
selector: "app-login",
templateUrl: "login.component.html",
})
export class LoginComponent extends BaseLoginComponent {
export class LoginComponent extends BaseLoginComponent implements OnInit {
showPasswordless = false;
constructor(
devicesApiService: DevicesApiServiceAbstraction,
@ -83,13 +83,14 @@ export class LoginComponent extends BaseLoginComponent {
};
super.successRoute = "/tabs/vault";
this.showPasswordless = flagEnabled("showPasswordless");
}
async ngOnInit(): Promise<void> {
if (this.showPasswordless) {
this.formGroup.controls.email.setValue(this.loginEmailService.getEmail());
const loginEmail = await firstValueFrom(this.loginEmailService.loginEmail$);
this.formGroup.controls.email.setValue(loginEmail);
this.formGroup.controls.rememberEmail.setValue(this.loginEmailService.getRememberEmail());
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.validateEmail();
await this.validateEmail();
}
}

View File

@ -44,8 +44,8 @@ export class HintComponent extends BaseHintComponent implements OnInit {
);
}
ngOnInit(): void {
super.ngOnInit();
async ngOnInit(): Promise<void> {
await super.ngOnInit();
this.emailFormControl.setValue(this.email);
}

View File

@ -251,12 +251,12 @@ export class BaseLoginDecryptionOptionsComponent implements OnInit, OnDestroy {
return;
}
this.loginEmailService.setEmail(this.data.userEmail);
this.loginEmailService.setLoginEmail(this.data.userEmail);
await this.router.navigate(["/login-with-device"]);
}
async requestAdminApproval() {
this.loginEmailService.setEmail(this.data.userEmail);
this.loginEmailService.setLoginEmail(this.data.userEmail);
await this.router.navigate(["/admin-approval-requested"]);
}

View File

@ -1,5 +1,6 @@
import { Directive, OnInit } from "@angular/core";
import { Router } from "@angular/router";
import { firstValueFrom } from "rxjs";
import { LoginEmailServiceAbstraction } from "@bitwarden/auth/common";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
@ -27,8 +28,8 @@ export class HintComponent implements OnInit {
protected toastService: ToastService,
) {}
ngOnInit(): void {
this.email = this.loginEmailService.getEmail() ?? "";
async ngOnInit(): Promise<void> {
this.email = (await firstValueFrom(this.loginEmailService.loginEmail$)) ?? "";
}
async submit() {

View File

@ -93,13 +93,6 @@ export class LoginViaAuthRequestComponent
) {
super(environmentService, i18nService, platformUtilsService, toastService);
// TODO: I don't know why this is necessary.
// Why would the existence of the email depend on the navigation?
const navigation = this.router.getCurrentNavigation();
if (navigation) {
this.email = this.loginEmailService.getEmail();
}
// Gets signalR push notification
// Only fires on approval to prevent enumeration
this.authRequestService.authRequestPushNotification$
@ -118,6 +111,7 @@ export class LoginViaAuthRequestComponent
}
async ngOnInit() {
this.email = await firstValueFrom(this.loginEmailService.loginEmail$);
this.userAuthNStatus = await this.authService.getAuthStatus();
const matchOptions: IsActiveMatchOptions = {
@ -165,7 +159,7 @@ export class LoginViaAuthRequestComponent
} else {
// Standard auth request
// TODO: evaluate if we can remove the setting of this.email in the constructor
this.email = this.loginEmailService.getEmail();
this.email = await firstValueFrom(this.loginEmailService.loginEmail$);
if (!this.email) {
this.toastService.showToast({

View File

@ -304,7 +304,7 @@ export class LoginComponent extends CaptchaProtectedComponent implements OnInit,
private async loadEmailSettings() {
// Try to load from memory first
const email = this.loginEmailService.getEmail();
const email = await firstValueFrom(this.loginEmailService.loginEmail$);
const rememberEmail = this.loginEmailService.getRememberEmail();
if (email) {
this.formGroup.controls.email.setValue(email);
@ -321,7 +321,7 @@ export class LoginComponent extends CaptchaProtectedComponent implements OnInit,
}
protected async saveEmailSettings() {
this.loginEmailService.setEmail(this.formGroup.value.email);
this.loginEmailService.setLoginEmail(this.formGroup.value.email);
this.loginEmailService.setRememberEmail(this.formGroup.value.rememberEmail);
await this.loginEmailService.saveEmailSettings();
}

View File

@ -1,29 +1,28 @@
import { Observable } from "rxjs";
export abstract class LoginEmailServiceAbstraction {
/**
* An observable that monitors the loginEmail in memory.
* The loginEmail is the email that is being used in the current login process.
*/
loginEmail$: Observable<string | null>;
/**
* An observable that monitors the storedEmail on disk.
* This will return null if an account is being added.
*/
storedEmail$: Observable<string | null>;
/**
* Gets the current email being used in the login process from memory.
* @returns A string of the email.
* Sets the loginEmail in memory.
* The loginEmail is the email that is being used in the current login process.
*/
getEmail: () => string;
/**
* Sets the current email being used in the login process in memory.
* @param email The email to be set.
*/
setEmail: (email: string) => void;
setLoginEmail: (email: string) => Promise<void>;
/**
* Gets from memory whether or not the email should be stored on disk when `saveEmailSettings` is called.
* @returns A boolean stating whether or not the email should be stored on disk.
*/
getRememberEmail: () => boolean;
/**
* Sets in memory whether or not the email should be stored on disk when
* `saveEmailSettings` is called.
* Sets in memory whether or not the email should be stored on disk when `saveEmailSettings` is called.
*/
setRememberEmail: (value: boolean) => void;
/**

View File

@ -43,7 +43,7 @@ describe("LoginEmailService", () => {
describe("storedEmail$", () => {
it("returns the stored email when not adding an account", async () => {
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
@ -53,7 +53,7 @@ describe("LoginEmailService", () => {
});
it("returns the stored email when not adding an account and the user has just logged in", async () => {
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
@ -66,7 +66,7 @@ describe("LoginEmailService", () => {
});
it("returns null when adding an account", async () => {
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
@ -83,7 +83,7 @@ describe("LoginEmailService", () => {
describe("saveEmailSettings", () => {
it("saves the email when not adding an account", async () => {
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
@ -95,7 +95,7 @@ describe("LoginEmailService", () => {
it("clears the email when not adding an account and rememberEmail is false", async () => {
storedEmailState.stateSubject.next("initialEmail@bitwarden.com");
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(false);
await sut.saveEmailSettings();
@ -110,7 +110,7 @@ describe("LoginEmailService", () => {
["OtherUserId" as UserId]: AuthenticationStatus.Locked,
});
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
@ -127,7 +127,7 @@ describe("LoginEmailService", () => {
["OtherUserId" as UserId]: AuthenticationStatus.Locked,
});
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(false);
await sut.saveEmailSettings();
@ -140,11 +140,11 @@ describe("LoginEmailService", () => {
it("does not clear the email and rememberEmail after saving", async () => {
// Browser uses these values to maintain the email between login and 2fa components so
// we do not want to clear them too early.
sut.setEmail("userEmail@bitwarden.com");
await sut.setLoginEmail("userEmail@bitwarden.com");
sut.setRememberEmail(true);
await sut.saveEmailSettings();
const result = sut.getEmail();
const result = await firstValueFrom(sut.loginEmail$);
expect(result).toBe("userEmail@bitwarden.com");
});

View File

@ -8,21 +8,28 @@ import {
GlobalState,
KeyDefinition,
LOGIN_EMAIL_DISK,
LOGIN_EMAIL_MEMORY,
StateProvider,
} from "../../../../../common/src/platform/state";
import { LoginEmailServiceAbstraction } from "../../abstractions/login-email.service";
export const LOGIN_EMAIL = new KeyDefinition<string>(LOGIN_EMAIL_MEMORY, "loginEmail", {
deserializer: (value: string) => value,
});
export const STORED_EMAIL = new KeyDefinition<string>(LOGIN_EMAIL_DISK, "storedEmail", {
deserializer: (value: string) => value,
});
export class LoginEmailService implements LoginEmailServiceAbstraction {
private email: string | null;
private rememberEmail: boolean;
// True if an account is currently being added through account switching
private readonly addingAccount$: Observable<boolean>;
private readonly loginEmailState: GlobalState<string>;
loginEmail$: Observable<string | null>;
private readonly storedEmailState: GlobalState<string>;
storedEmail$: Observable<string | null>;
@ -31,6 +38,7 @@ export class LoginEmailService implements LoginEmailServiceAbstraction {
private authService: AuthService,
private stateProvider: StateProvider,
) {
this.loginEmailState = this.stateProvider.getGlobal(LOGIN_EMAIL);
this.storedEmailState = this.stateProvider.getGlobal(STORED_EMAIL);
// In order to determine if an account is being added, we check if any account is not logged out
@ -46,6 +54,8 @@ export class LoginEmailService implements LoginEmailServiceAbstraction {
}),
);
this.loginEmail$ = this.loginEmailState.state$;
this.storedEmail$ = this.storedEmailState.state$.pipe(
switchMap(async (storedEmail) => {
// When adding an account, we don't show the stored email
@ -57,12 +67,8 @@ export class LoginEmailService implements LoginEmailServiceAbstraction {
);
}
getEmail() {
return this.email;
}
setEmail(email: string) {
this.email = email;
async setLoginEmail(email: string) {
await this.loginEmailState.update((_) => email);
}
getRememberEmail() {
@ -76,25 +82,27 @@ export class LoginEmailService implements LoginEmailServiceAbstraction {
// Note: only clear values on successful login or you are sure they are not needed.
// Browser uses these values to maintain the email between login and 2fa components so
// we do not want to clear them too early.
clearValues() {
this.email = null;
async clearValues() {
await this.setLoginEmail(null);
this.rememberEmail = false;
}
async saveEmailSettings() {
const addingAccount = await firstValueFrom(this.addingAccount$);
const email = await firstValueFrom(this.loginEmail$);
await this.storedEmailState.update((storedEmail) => {
// If we're adding an account, only overwrite the stored email when rememberEmail is true
if (addingAccount) {
if (this.rememberEmail) {
return this.email;
return email;
}
return storedEmail;
}
// Saving with rememberEmail set to false will clear the stored email
if (this.rememberEmail) {
return this.email;
return email;
}
return null;
});

View File

@ -53,6 +53,7 @@ export const KEY_CONNECTOR_DISK = new StateDefinition("keyConnector", "disk");
export const LOGIN_EMAIL_DISK = new StateDefinition("loginEmail", "disk", {
web: "disk-local",
});
export const LOGIN_EMAIL_MEMORY = new StateDefinition("loginEmail", "memory");
export const LOGIN_STRATEGY_MEMORY = new StateDefinition("loginStrategy", "memory");
export const MASTER_PASSWORD_DISK = new StateDefinition("masterPassword", "disk");
export const MASTER_PASSWORD_MEMORY = new StateDefinition("masterPassword", "memory");