diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 7e2b802d34..8de3014fb2 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -8,7 +8,6 @@ import { AuthRequestServiceAbstraction, AuthRequestService, LoginEmailServiceAbstraction, - LoginEmailService, LogoutReason, } from "@bitwarden/auth/common"; import { ApiService as ApiServiceAbstraction } from "@bitwarden/common/abstractions/api.service"; @@ -708,8 +707,6 @@ export default class MainBackground { this.stateProvider, ); - this.loginEmailService = new LoginEmailService(this.stateProvider); - this.ssoLoginService = new SsoLoginService(this.stateProvider); this.userVerificationApiService = new UserVerificationApiService(this.apiService); @@ -1255,9 +1252,6 @@ export default class MainBackground { clearCaches(); if (userId == null) { - this.loginEmailService.setRememberEmail(false); - await this.loginEmailService.saveEmailSettings(); - await this.refreshBadge(); await this.refreshMenu(); await this.overlayBackground?.updateOverlayCiphers(); // null in popup only contexts diff --git a/apps/desktop/src/app/layout/account-switcher.component.ts b/apps/desktop/src/app/layout/account-switcher.component.ts index 92cfebfd60..ff27dacd96 100644 --- a/apps/desktop/src/app/layout/account-switcher.component.ts +++ b/apps/desktop/src/app/layout/account-switcher.component.ts @@ -165,13 +165,10 @@ export class AccountSwitcherComponent { async addAccount() { this.close(); - this.loginEmailService.setRememberEmail(false); - await this.loginEmailService.saveEmailSettings(); - - await this.router.navigate(["/login"]); const activeAccount = await firstValueFrom(this.accountService.activeAccount$); await this.stateService.clearDecryptedData(activeAccount?.id as UserId); await this.accountService.switchAccount(null); + await this.router.navigate(["/login"]); } private async createInactiveAccounts(baseAccounts: { diff --git a/libs/angular/src/auth/components/login.component.ts b/libs/angular/src/auth/components/login.component.ts index 4466a106a5..0d3c3a8707 100644 --- a/libs/angular/src/auth/components/login.component.ts +++ b/libs/angular/src/auth/components/login.component.ts @@ -126,7 +126,7 @@ export class LoginComponent extends CaptchaProtectedComponent implements OnInit, let rememberEmail = this.loginEmailService.getRememberEmail(); - if (rememberEmail == null) { + if (!rememberEmail) { rememberEmail = (await firstValueFrom(this.loginEmailService.storedEmail$)) != null; } diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 86ccce4e00..880488cfcf 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -970,7 +970,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: LoginEmailServiceAbstraction, useClass: LoginEmailService, - deps: [StateProvider], + deps: [AccountServiceAbstraction, AuthServiceAbstraction, StateProvider], }), safeProvider({ provide: OrgDomainInternalServiceAbstraction, diff --git a/libs/auth/src/common/abstractions/login-email.service.ts b/libs/auth/src/common/abstractions/login-email.service.ts index 89165af543..d4fbbaff84 100644 --- a/libs/auth/src/common/abstractions/login-email.service.ts +++ b/libs/auth/src/common/abstractions/login-email.service.ts @@ -2,36 +2,40 @@ import { Observable } from "rxjs"; export abstract class LoginEmailServiceAbstraction { /** - * An observable that monitors the storedEmail + * An observable that monitors the storedEmail on disk. + * This will return null if an account is being added. */ - storedEmail$: Observable; + storedEmail$: Observable; /** - * Gets the current email being used in the login process. + * Gets the current email being used in the login process from memory. * @returns A string of the email. */ getEmail: () => string; /** - * Sets the current email being used in the login process. + * Sets the current email being used in the login process in memory. * @param email The email to be set. */ setEmail: (email: string) => void; /** - * Gets whether or not the email should be stored on disk. + * 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 whether or not the email should be stored on disk. + * Sets in memory whether or not the email should be stored on disk when + * `saveEmailSettings` is called. */ setRememberEmail: (value: boolean) => void; /** - * Sets the email and rememberEmail properties to null. + * Sets the email and rememberEmail properties in memory to null. */ clearValues: () => void; /** - * - If rememberEmail is true, sets the storedEmail on disk to the current email. - * - If rememberEmail is false, sets the storedEmail on disk to null. - * - Then sets the email and rememberEmail properties to null. + * Saves or clears the email on disk + * - If an account is being added, only changes the stored email when rememberEmail is true. + * - If rememberEmail is true, sets the email on disk to the current email. + * - If rememberEmail is false, sets the email on disk to null. + * Always clears the email and rememberEmail properties from memory. * @returns A promise that resolves once the email settings are saved. */ saveEmailSettings: () => Promise; diff --git a/libs/auth/src/common/services/login-email/login-email.service.spec.ts b/libs/auth/src/common/services/login-email/login-email.service.spec.ts new file mode 100644 index 0000000000..190c9ecf16 --- /dev/null +++ b/libs/auth/src/common/services/login-email/login-email.service.spec.ts @@ -0,0 +1,150 @@ +import { MockProxy, mock } from "jest-mock-extended"; +import { BehaviorSubject, firstValueFrom } from "rxjs"; + +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { + FakeAccountService, + FakeGlobalState, + FakeStateProvider, + mockAccountServiceWith, +} from "@bitwarden/common/spec"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { LoginEmailService, STORED_EMAIL } from "./login-email.service"; + +describe("LoginEmailService", () => { + let sut: LoginEmailService; + + let accountService: FakeAccountService; + let authService: MockProxy; + let stateProvider: FakeStateProvider; + + const userId = "USER_ID" as UserId; + let storedEmailState: FakeGlobalState; + let mockAuthStatuses$: BehaviorSubject>; + + beforeEach(() => { + accountService = mockAccountServiceWith(userId); + authService = mock(); + stateProvider = new FakeStateProvider(accountService); + + storedEmailState = stateProvider.global.getFake(STORED_EMAIL); + + mockAuthStatuses$ = new BehaviorSubject>({}); + authService.authStatuses$ = mockAuthStatuses$; + + sut = new LoginEmailService(accountService, authService, stateProvider); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe("storedEmail$", () => { + it("returns the stored email when not adding an account", async () => { + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + const result = await firstValueFrom(sut.storedEmail$); + + expect(result).toEqual("userEmail@bitwarden.com"); + }); + + it("returns the stored email when not adding an account and the user has just logged in", async () => { + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + mockAuthStatuses$.next({ [userId]: AuthenticationStatus.Unlocked }); + // account service already initialized with userId as active user + + const result = await firstValueFrom(sut.storedEmail$); + + expect(result).toEqual("userEmail@bitwarden.com"); + }); + + it("returns null when adding an account", async () => { + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + mockAuthStatuses$.next({ + [userId]: AuthenticationStatus.Unlocked, + ["OtherUserId" as UserId]: AuthenticationStatus.Locked, + }); + + const result = await firstValueFrom(sut.storedEmail$); + + expect(result).toBeNull(); + }); + }); + + describe("saveEmailSettings", () => { + it("saves the email when not adding an account", async () => { + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + const result = await firstValueFrom(storedEmailState.state$); + + expect(result).toEqual("userEmail@bitwarden.com"); + }); + + 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"); + sut.setRememberEmail(false); + await sut.saveEmailSettings(); + + const result = await firstValueFrom(storedEmailState.state$); + + expect(result).toBeNull(); + }); + + it("saves the email when adding an account", async () => { + mockAuthStatuses$.next({ + [userId]: AuthenticationStatus.Unlocked, + ["OtherUserId" as UserId]: AuthenticationStatus.Locked, + }); + + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + const result = await firstValueFrom(storedEmailState.state$); + + expect(result).toEqual("userEmail@bitwarden.com"); + }); + + it("does not clear the email when adding an account and rememberEmail is false", async () => { + storedEmailState.stateSubject.next("initialEmail@bitwarden.com"); + + mockAuthStatuses$.next({ + [userId]: AuthenticationStatus.Unlocked, + ["OtherUserId" as UserId]: AuthenticationStatus.Locked, + }); + + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(false); + await sut.saveEmailSettings(); + + const result = await firstValueFrom(storedEmailState.state$); + + // result should not be null + expect(result).toEqual("initialEmail@bitwarden.com"); + }); + + it("clears the email and rememberEmail after saving", async () => { + sut.setEmail("userEmail@bitwarden.com"); + sut.setRememberEmail(true); + await sut.saveEmailSettings(); + + const result = sut.getEmail(); + + expect(result).toBeNull(); + }); + }); +}); diff --git a/libs/auth/src/common/services/login-email/login-email.service.ts b/libs/auth/src/common/services/login-email/login-email.service.ts index 171af07430..3babc6f43c 100644 --- a/libs/auth/src/common/services/login-email/login-email.service.ts +++ b/libs/auth/src/common/services/login-email/login-email.service.ts @@ -1,4 +1,8 @@ -import { Observable } from "rxjs"; +import { Observable, firstValueFrom, switchMap } from "rxjs"; + +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 { GlobalState, @@ -8,20 +12,49 @@ import { } from "../../../../../common/src/platform/state"; import { LoginEmailServiceAbstraction } from "../../abstractions/login-email.service"; -const STORED_EMAIL = new KeyDefinition(LOGIN_EMAIL_DISK, "storedEmail", { +export const STORED_EMAIL = new KeyDefinition(LOGIN_EMAIL_DISK, "storedEmail", { deserializer: (value: string) => value, }); export class LoginEmailService implements LoginEmailServiceAbstraction { - private email: string; + private email: string | null; private rememberEmail: boolean; - private readonly storedEmailState: GlobalState; - storedEmail$: Observable; + // True if an account is currently being added through account switching + private readonly addingAccount$: Observable; - constructor(private stateProvider: StateProvider) { + private readonly storedEmailState: GlobalState; + storedEmail$: Observable; + + constructor( + private accountService: AccountService, + private authService: AuthService, + private stateProvider: StateProvider, + ) { this.storedEmailState = this.stateProvider.getGlobal(STORED_EMAIL); - this.storedEmail$ = this.storedEmailState.state$; + + // In order to determine if an account is being added, we check if any account is not logged out + this.addingAccount$ = this.authService.authStatuses$.pipe( + switchMap(async (statuses) => { + // We don't want to consider the active account since it may have just changed auth status to logged in + // which would make this observable think an account is being added + const activeUser = await firstValueFrom(this.accountService.activeAccount$); + if (activeUser) { + delete statuses[activeUser.id]; + } + return Object.values(statuses).some((status) => status !== AuthenticationStatus.LoggedOut); + }), + ); + + this.storedEmail$ = this.storedEmailState.state$.pipe( + switchMap(async (storedEmail) => { + // When adding an account, we don't show the stored email + if (await firstValueFrom(this.addingAccount$)) { + return null; + } + return storedEmail; + }), + ); } getEmail() { @@ -37,16 +70,31 @@ export class LoginEmailService implements LoginEmailServiceAbstraction { } setRememberEmail(value: boolean) { - this.rememberEmail = value; + this.rememberEmail = value ?? false; } clearValues() { this.email = null; - this.rememberEmail = null; + this.rememberEmail = false; } async saveEmailSettings() { - await this.storedEmailState.update(() => (this.rememberEmail ? this.email : null)); + const addingAccount = await firstValueFrom(this.addingAccount$); + 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 storedEmail; + } + + // Logging in with rememberEmail set to false will clear the stored email + if (this.rememberEmail) { + return this.email; + } + return null; + }); this.clearValues(); } }