[PM-19273] [opaque] LoginStrategyService - refactor makeprepasswordloginmasterkey (#13877)

* Remove dead code from loginStrategyService * Move makePrePasswordLoginMasterKey to base strategy
2026-02-10 05:30:01 +00:00 · 2025-03-18 22:40:06 +10:00
parent d122bba3a9
commit fd5078a5b6
7 changed files with 33 additions and 81 deletions
--- a/libs/auth/src/common/abstractions/login-strategy.service.ts
+++ b/libs/auth/src/common/abstractions/login-strategy.service.ts
@@ -5,8 +5,6 @@ import { Observable } from "rxjs";
 import { AuthenticationType } from "@bitwarden/common/auth/enums/authentication-type";
 import { AuthResult } from "@bitwarden/common/auth/models/domain/auth-result";
 import { TokenTwoFactorRequest } from "@bitwarden/common/auth/models/request/identity-token/token-two-factor.request";
-import { MasterKey } from "@bitwarden/common/types/key";
-import { KdfConfig } from "@bitwarden/key-management";

 import {
  UserApiLoginCredentials,
@@ -70,20 +68,6 @@ export abstract class LoginStrategyServiceAbstraction {
    // TODO: PM-15162 - deprecate captchaResponse
    captchaResponse: string,
  ) => Promise<AuthResult>;
-
-  // TODO: PM-19273 - Refactor makePrePasswordLoginMasterKey to no longer be on the service
-  // once PM-18176 removes the Recover2faComponent dependency.
-  /**
-   * Creates a master key from the provided master password and email.
-   * If a KdfConfig is provided, it will be used to generate the key.
-   * Otherwise, the PrePasswordLogin endpoint will be used to retrieve the user's
-   * KdfConfig.
-   */
-  makePrePasswordLoginMasterKey: (
-    masterPassword: string,
-    email: string,
-    kdfConfig?: KdfConfig,
-  ) => Promise<MasterKey>;
  /**
   * Emits true if the authentication session has expired.
   */
--- a/libs/auth/src/common/login-strategies/base-login.strategy.spec.ts
+++ b/libs/auth/src/common/login-strategies/base-login.strategy.spec.ts
@@ -42,11 +42,10 @@ import {
 import { CsprngArray } from "@bitwarden/common/types/csprng";
 import { UserId } from "@bitwarden/common/types/guid";
 import { UserKey, MasterKey } from "@bitwarden/common/types/key";
-import { KdfConfigService, KeyService } from "@bitwarden/key-management";
+import { KdfConfigService, KeyService, PBKDF2KdfConfig } from "@bitwarden/key-management";

-import { LoginStrategyServiceAbstraction } from "../abstractions";
 import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction";
-import { PasswordLoginCredentials } from "../models";
+import { PasswordHashLoginCredentials } from "../models";
 import { UserDecryptionOptions } from "../models/domain/user-decryption-options";

 import { PasswordLoginStrategy, PasswordLoginStrategyData } from "./password-login.strategy";
@@ -102,12 +101,11 @@ export function identityTokenResponseFactory(
 }

 // TODO: add tests for latest changes to base class for TDE
-describe("LoginStrategy", () => {
+describe("BaseLoginStrategy", () => {
  let cache: PasswordLoginStrategyData;
  let accountService: FakeAccountService;
  let masterPasswordService: FakeMasterPasswordService;

-  let loginStrategyService: MockProxy<LoginStrategyServiceAbstraction>;
  let keyService: MockProxy<KeyService>;
  let encryptService: MockProxy<EncryptService>;
  let apiService: MockProxy<ApiService>;
@@ -127,13 +125,12 @@ describe("LoginStrategy", () => {
  let environmentService: MockProxy<EnvironmentService>;

  let passwordLoginStrategy: PasswordLoginStrategy;
-  let credentials: PasswordLoginCredentials;
+  let credentials: PasswordHashLoginCredentials;

  beforeEach(async () => {
    accountService = mockAccountServiceWith(userId);
    masterPasswordService = new FakeMasterPasswordService();

-    loginStrategyService = mock<LoginStrategyServiceAbstraction>();
    keyService = mock<KeyService>();
    encryptService = mock<EncryptService>();
    apiService = mock<ApiService>();
@@ -161,7 +158,6 @@ describe("LoginStrategy", () => {
      cache,
      passwordStrengthService,
      policyService,
-      loginStrategyService,
      accountService as unknown as AccountService,
      masterPasswordService,
      keyService,
@@ -180,7 +176,7 @@ describe("LoginStrategy", () => {
      kdfConfigService,
      environmentService,
    );
-    credentials = new PasswordLoginCredentials(email, masterPassword);
+    credentials = new PasswordHashLoginCredentials(email, masterPassword, new PBKDF2KdfConfig());
  });

  describe("base class", () => {
@@ -508,7 +504,6 @@ describe("LoginStrategy", () => {
        cache,
        passwordStrengthService,
        policyService,
-        loginStrategyService,
        accountService as AccountService,
        masterPasswordService,
        keyService,
@@ -572,7 +567,6 @@ describe("LoginStrategy", () => {
        cache,
        passwordStrengthService,
        policyService,
-        loginStrategyService,
        accountService as AccountService,
        masterPasswordService,
        keyService,
--- a/libs/auth/src/common/login-strategies/base-login.strategy.ts
+++ b/libs/auth/src/common/login-strategies/base-login.strategy.ts
@@ -35,12 +35,14 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { Account, AccountProfile } from "@bitwarden/common/platform/models/domain/account";
 import { UserId } from "@bitwarden/common/types/guid";
+import { MasterKey } from "@bitwarden/common/types/key";
 import {
  KeyService,
  Argon2KdfConfig,
  PBKDF2KdfConfig,
  KdfConfigService,
  KdfType,
+  KdfConfig,
 } from "@bitwarden/key-management";

 import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction";
@@ -412,4 +414,23 @@ export abstract class BaseLoginStrategy {
    this.cache.next({ ...this.cache.value, captchaBypassToken: response.captchaToken ?? null });
    return result;
  }
+
+  /**
+   * Creates a master key from the provided master password and email, using the provided kdfConfig.
+   */
+  protected async makePrePasswordLoginMasterKey(
+    masterPassword: string,
+    email: string,
+    kdfConfig: KdfConfig,
+  ): Promise<MasterKey> {
+    email = email.trim().toLowerCase();
+
+    if (!kdfConfig) {
+      throw new Error("KDF config is required");
+    }
+
+    kdfConfig.validateKdfConfigForPreLogin();
+
+    return this.keyService.makeMasterKey(masterPassword, email, kdfConfig);
+  }
 }
--- a/libs/auth/src/common/login-strategies/opaque-login.strategy.ts
+++ b/libs/auth/src/common/login-strategies/opaque-login.strategy.ts
@@ -21,7 +21,6 @@ import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/pass
 import { UserId } from "@bitwarden/common/types/guid";
 import { MasterKey } from "@bitwarden/common/types/key";

-import { LoginStrategyServiceAbstraction } from "../abstractions";
 import { OpaqueLoginCredentials } from "../models/domain/login-credentials";
 import { CacheData } from "../services/login-strategies/login-strategy.state";

@@ -77,7 +76,6 @@ export class OpaqueLoginStrategy extends BaseLoginStrategy {
    data: OpaqueLoginStrategyData,
    private passwordStrengthService: PasswordStrengthServiceAbstraction,
    private policyService: PolicyService,
-    private loginStrategyService: LoginStrategyServiceAbstraction,
    ...sharedDeps: ConstructorParameters<typeof BaseLoginStrategy>
  ) {
    super(...sharedDeps);
@@ -97,11 +95,7 @@ export class OpaqueLoginStrategy extends BaseLoginStrategy {

    // Even though we are completing OPAQUE authN and not logging in with password hash,
    // we still need to hash the master password for logged in user verification scenarios.
-    data.masterKey = await this.loginStrategyService.makePrePasswordLoginMasterKey(
-      masterPassword,
-      email,
-      kdfConfig,
-    );
+    data.masterKey = await this.makePrePasswordLoginMasterKey(masterPassword, email, kdfConfig);

    // Hash the password early (before authentication) so we don't persist it in memory in plaintext
    data.localMasterKeyHash = await this.keyService.hashMasterKey(
--- a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts
+++ b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts
@@ -36,7 +36,6 @@ import { UserId } from "@bitwarden/common/types/guid";
 import { MasterKey, UserKey } from "@bitwarden/common/types/key";
 import { KdfConfigService, KeyService, PBKDF2KdfConfig } from "@bitwarden/key-management";

-import { LoginStrategyServiceAbstraction } from "../abstractions";
 import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction";
 import { PasswordHashLoginCredentials } from "../models/domain/login-credentials";

@@ -64,7 +63,6 @@ describe("PasswordLoginStrategy", () => {
  let accountService: FakeAccountService;
  let masterPasswordService: FakeMasterPasswordService;

-  let loginStrategyService: MockProxy<LoginStrategyServiceAbstraction>;
  let keyService: MockProxy<KeyService>;
  let encryptService: MockProxy<EncryptService>;
  let apiService: MockProxy<ApiService>;
@@ -91,7 +89,6 @@ describe("PasswordLoginStrategy", () => {
    accountService = mockAccountServiceWith(userId);
    masterPasswordService = new FakeMasterPasswordService();

-    loginStrategyService = mock<LoginStrategyServiceAbstraction>();
    keyService = mock<KeyService>();
    encryptService = mock<EncryptService>();
    apiService = mock<ApiService>();
@@ -115,8 +112,9 @@ describe("PasswordLoginStrategy", () => {
      sub: userId,
    });

-    loginStrategyService.makePrePasswordLoginMasterKey.mockResolvedValue(masterKey);
-
+    keyService.makeMasterKey
+      .calledWith(masterPassword, email, expect.any(PBKDF2KdfConfig))
+      .mockResolvedValue(masterKey);
    keyService.hashMasterKey
      .calledWith(masterPassword, expect.anything(), undefined)
      .mockResolvedValue(hashedPassword);
@@ -130,7 +128,6 @@ describe("PasswordLoginStrategy", () => {
      cache,
      passwordStrengthService,
      policyService,
-      loginStrategyService,
      accountService,
      masterPasswordService,
      keyService,
--- a/libs/auth/src/common/login-strategies/password-login.strategy.ts
+++ b/libs/auth/src/common/login-strategies/password-login.strategy.ts
@@ -19,7 +19,6 @@ import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/pass
 import { UserId } from "@bitwarden/common/types/guid";
 import { MasterKey } from "@bitwarden/common/types/key";

-import { LoginStrategyServiceAbstraction } from "../abstractions";
 import { PasswordHashLoginCredentials } from "../models/domain/login-credentials";
 import { CacheData } from "../services/login-strategies/login-strategy.state";

@@ -66,7 +65,6 @@ export class PasswordLoginStrategy extends BaseLoginStrategy {
    data: PasswordLoginStrategyData,
    private passwordStrengthService: PasswordStrengthServiceAbstraction,
    private policyService: PolicyService,
-    private loginStrategyService: LoginStrategyServiceAbstraction,
    ...sharedDeps: ConstructorParameters<typeof BaseLoginStrategy>
  ) {
    super(...sharedDeps);
@@ -84,11 +82,7 @@ export class PasswordLoginStrategy extends BaseLoginStrategy {

    const data = new PasswordLoginStrategyData();

-    data.masterKey = await this.loginStrategyService.makePrePasswordLoginMasterKey(
-      masterPassword,
-      email,
-      kdfConfig,
-    );
+    data.masterKey = await this.makePrePasswordLoginMasterKey(masterPassword, email, kdfConfig);
    data.userEnteredEmail = email;

    // Hash the password early (before authentication) so we don't persist it in memory in plaintext
--- a/libs/auth/src/common/services/login-strategies/login-strategy.service.ts
+++ b/libs/auth/src/common/services/login-strategies/login-strategy.service.ts
@@ -36,8 +36,7 @@ import { StateService } from "@bitwarden/common/platform/abstractions/state.serv
 import { TaskSchedulerService, ScheduledTaskNames } from "@bitwarden/common/platform/scheduling";
 import { GlobalState, GlobalStateProvider } from "@bitwarden/common/platform/state";
 import { PasswordStrengthServiceAbstraction } from "@bitwarden/common/tools/password-strength";
-import { MasterKey } from "@bitwarden/common/types/key";
-import { KeyService, KdfConfig, KdfConfigService } from "@bitwarden/key-management";
+import { KeyService, KdfConfigService } from "@bitwarden/key-management";

 import { AuthRequestServiceAbstraction, LoginStrategyServiceAbstraction } from "../../abstractions";
 import { InternalUserDecryptionOptionsServiceAbstraction } from "../../abstractions/user-decryption-options.service.abstraction";
@@ -324,35 +323,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction {
    }
  }

-  async makePrePasswordLoginMasterKey(
-    masterPassword: string,
-    email: string,
-    kdfConfig?: KdfConfig,
-  ): Promise<MasterKey> {
-    email = email.trim().toLowerCase();
-
-    if (!kdfConfig) {
-      try {
-        const preloginResponse = await this.prePasswordLoginApiService.postPrePasswordLogin(
-          new PrePasswordLoginRequest(email),
-        );
-        kdfConfig = preloginResponse?.toKdfConfig();
-      } catch (e: any) {
-        if (e == null || e.statusCode !== 404) {
-          throw e;
-        }
-      }
-
-      if (!kdfConfig) {
-        throw new Error("KDF config is required");
-      }
-    }
-
-    kdfConfig.validateKdfConfigForPreLogin();
-
-    return this.keyService.makeMasterKey(masterPassword, email, kdfConfig);
-  }
-
  private async clearCache(): Promise<void> {
    await this.currentAuthnTypeState.update((_) => null);
    await this.loginStrategyCacheState.update((_) => null);
@@ -426,7 +396,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction {
          data?.password ?? new PasswordLoginStrategyData(),
          this.passwordStrengthService,
          this.policyService,
-          this,
          ...sharedDeps,
        );
      case AuthenticationType.Sso:
@@ -460,11 +429,10 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction {
          data?.opaque ?? new OpaqueLoginStrategyData(),
          this.passwordStrengthService,
          this.policyService,
-          this,
          ...sharedDeps,
        );
      default:
-        return null;
+        throw new Error("No matching strategy found for AuthenticationType " + strategy);
    }
  }
 }