fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service (#13292)

* fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service - Fixed location of retrieving the active user id in one component. * fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service - Fixed up type safety. * fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service - Removed unnessesary subscriptions. * fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service - Fixed test. * fix(active-user-state-refactor): [PM-18052] Jit Bug with SSO Service - Made code a little cleaner.
2025-12-06 00:13:28 +00:00 · 2025-02-06 16:06:26 -05:00
parent 1c2333ca5a
commit 516246eab8
7 changed files with 24 additions and 59 deletions
--- a/libs/angular/src/auth/components/sso.component.ts
+++ b/libs/angular/src/auth/components/sso.component.ts
@@ -1,7 +1,6 @@
 // FIXME: Update this file to be type safe and remove this and next line
 // @ts-strict-ignore
 import { Directive, OnInit } from "@angular/core";
-import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { ActivatedRoute, NavigationExtras, Router } from "@angular/router";
 import { firstValueFrom } from "rxjs";
 import { first } from "rxjs/operators";
@@ -28,7 +27,6 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service"
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
-import { UserId } from "@bitwarden/common/types/guid";
 import { ToastService } from "@bitwarden/components";
 import { PasswordGenerationServiceAbstraction } from "@bitwarden/generator-legacy";

@@ -57,7 +55,6 @@ export class SsoComponent implements OnInit {
  protected redirectUri: string;
  protected state: string;
  protected codeChallenge: string;
-  protected activeUserId: UserId;

  constructor(
    protected ssoLoginService: SsoLoginServiceAbstraction,
@@ -77,11 +74,7 @@ export class SsoComponent implements OnInit {
    protected masterPasswordService: InternalMasterPasswordServiceAbstraction,
    protected accountService: AccountService,
    protected toastService: ToastService,
-  ) {
-    this.accountService.activeAccount$.pipe(takeUntilDestroyed()).subscribe((account) => {
-      this.activeUserId = account?.id;
-    });
-  }
+  ) {}

  async ngOnInit() {
    // eslint-disable-next-line rxjs/no-async-subscribe
@@ -233,10 +226,8 @@ export class SsoComponent implements OnInit {
      // - TDE login decryption options component
      // - Browser SSO on extension open
      // Note: you cannot set this in state before 2FA b/c there won't be an account in state.
-      await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(
-        orgSsoIdentifier,
-        this.activeUserId,
-      );
+      const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
+      await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(orgSsoIdentifier, userId);

      // Users enrolled in admin acct recovery can be forced to set a new password after
      // having the admin set a temp password for them (affects TDE & standard users)
--- a/libs/angular/src/auth/components/two-factor-auth/two-factor-auth.component.ts
+++ b/libs/angular/src/auth/components/two-factor-auth/two-factor-auth.component.ts
@@ -2,7 +2,6 @@
 // @ts-strict-ignore
 import { CommonModule } from "@angular/common";
 import { Component, Inject, OnInit, ViewChild } from "@angular/core";
-import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { FormBuilder, ReactiveFormsModule, Validators } from "@angular/forms";
 import { ActivatedRoute, NavigationExtras, Router, RouterLink } from "@angular/router";
 import { Subject, takeUntil, lastValueFrom, first, firstValueFrom } from "rxjs";
@@ -32,7 +31,6 @@ import { EnvironmentService } from "@bitwarden/common/platform/abstractions/envi
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
-import { UserId } from "@bitwarden/common/types/guid";
 import {
  AsyncActionsModule,
  ButtonModule,
@@ -128,7 +126,6 @@ export class TwoFactorAuthComponent extends CaptchaProtectedComponent implements
  protected changePasswordRoute = "set-password";
  protected forcePasswordResetRoute = "update-temp-password";
  protected successRoute = "vault";
-  protected activeUserId: UserId;

  constructor(
    protected loginStrategyService: LoginStrategyServiceAbstraction,
@@ -151,10 +148,6 @@ export class TwoFactorAuthComponent extends CaptchaProtectedComponent implements
    protected toastService: ToastService,
  ) {
    super(environmentService, i18nService, platformUtilsService, toastService);
-
-    this.accountService.activeAccount$.pipe(takeUntilDestroyed()).subscribe((account) => {
-      this.activeUserId = account?.id;
-    });
  }

  async ngOnInit() {
@@ -269,10 +262,8 @@ export class TwoFactorAuthComponent extends CaptchaProtectedComponent implements
    // Save off the OrgSsoIdentifier for use in the TDE flows
    // - TDE login decryption options component
    // - Browser SSO on extension open
-    await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(
-      this.orgIdentifier,
-      this.activeUserId,
-    );
+    const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
+    await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(this.orgIdentifier, userId);
    this.loginEmailService.clearValues();

    // note: this flow affects both TDE & standard users
--- a/libs/angular/src/auth/components/two-factor.component.ts
+++ b/libs/angular/src/auth/components/two-factor.component.ts
@@ -35,7 +35,6 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
-import { UserId } from "@bitwarden/common/types/guid";
 import { ToastService } from "@bitwarden/components";

 import { CaptchaProtectedComponent } from "./captcha-protected.component";
@@ -74,8 +73,6 @@ export class TwoFactorComponent extends CaptchaProtectedComponent implements OnI
  protected successRoute = "vault";
  protected twoFactorTimeoutRoute = "authentication-timeout";

-  protected activeUserId: UserId;
-
  get isDuoProvider(): boolean {
    return (
      this.selectedProviderType === TwoFactorProviderType.Duo ||
@@ -108,10 +105,6 @@ export class TwoFactorComponent extends CaptchaProtectedComponent implements OnI

    this.webAuthnSupported = this.platformUtilsService.supportsWebAuthn(win);

-    this.accountService.activeAccount$.pipe(takeUntilDestroyed()).subscribe((account) => {
-      this.activeUserId = account?.id;
-    });
-
    // Add subscription to authenticationSessionTimeout$ and navigate to twoFactorTimeoutRoute if expired
    this.loginStrategyService.authenticationSessionTimeout$
      .pipe(takeUntilDestroyed())
@@ -295,10 +288,8 @@ export class TwoFactorComponent extends CaptchaProtectedComponent implements OnI
    // Save off the OrgSsoIdentifier for use in the TDE flows
    // - TDE login decryption options component
    // - Browser SSO on extension open
-    await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(
-      this.orgIdentifier,
-      this.activeUserId,
-    );
+    const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
+    await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(this.orgIdentifier, userId);
    this.loginEmailService.clearValues();

    // note: this flow affects both TDE & standard users
--- a/libs/auth/src/angular/sso/sso.component.ts
+++ b/libs/auth/src/angular/sso/sso.component.ts
@@ -36,7 +36,6 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service"
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service";
 import { Utils } from "@bitwarden/common/platform/misc/utils";
-import { UserId } from "@bitwarden/common/types/guid";
 import {
  AsyncActionsModule,
  ButtonModule,
@@ -90,7 +89,6 @@ export class SsoComponent implements OnInit {
  protected state: string | undefined;
  protected codeChallenge: string | undefined;
  protected clientId: SsoClientType | undefined;
-  protected activeUserId: UserId | undefined;

  formPromise: Promise<AuthResult> | undefined;
  initiateSsoFormPromise: Promise<SsoPreValidateResponse> | undefined;
@@ -132,8 +130,6 @@ export class SsoComponent implements OnInit {
  }

  async ngOnInit() {
-    this.activeUserId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
-
    const qParams: QueryParams = await firstValueFrom(this.route.queryParams);

    // This if statement will pass on the second portion of the SSO flow
@@ -388,10 +384,10 @@ export class SsoComponent implements OnInit {
      // - TDE login decryption options component
      // - Browser SSO on extension open
      // Note: you cannot set this in state before 2FA b/c there won't be an account in state.
-      await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(
-        orgSsoIdentifier,
-        this.activeUserId,
-      );
+
+      // Grabbing the active user id right before making the state set to ensure it exists.
+      const userId = (await firstValueFrom(this.accountService.activeAccount$))?.id;
+      await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier(orgSsoIdentifier, userId);

      // Users enrolled in admin acct recovery can be forced to set a new password after
      // having the admin set a temp password for them (affects TDE & standard users)
--- a/libs/common/src/auth/abstractions/sso-login.service.abstraction.ts
+++ b/libs/common/src/auth/abstractions/sso-login.service.abstraction.ts
@@ -1,5 +1,3 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { UserId } from "@bitwarden/common/types/guid";

 export abstract class SsoLoginServiceAbstraction {
@@ -13,7 +11,7 @@ export abstract class SsoLoginServiceAbstraction {
   * @see https://datatracker.ietf.org/doc/html/rfc7636
   * @returns The code verifier used for SSO.
   */
-  getCodeVerifier: () => Promise<string>;
+  abstract getCodeVerifier: () => Promise<string>;
  /**
   * Sets the code verifier used for SSO.
   *
@@ -23,7 +21,7 @@ export abstract class SsoLoginServiceAbstraction {
   * and verify it matches the one sent in the request for the `authorization_code`.
   * @see https://datatracker.ietf.org/doc/html/rfc7636
   */
-  setCodeVerifier: (codeVerifier: string) => Promise<void>;
+  abstract setCodeVerifier: (codeVerifier: string) => Promise<void>;
  /**
   * Gets the value of the SSO state.
   *
@@ -33,7 +31,7 @@ export abstract class SsoLoginServiceAbstraction {
   * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.1
   * @returns The SSO state.
   */
-  getSsoState: () => Promise<string>;
+  abstract getSsoState: () => Promise<string>;
  /**
   * Sets the value of the SSO state.
   *
@@ -42,7 +40,7 @@ export abstract class SsoLoginServiceAbstraction {
   * returns the `state` in the callback and the client verifies that the value returned matches the value sent.
   * @see https://datatracker.ietf.org/doc/html/rfc6749#section-4.1
   */
-  setSsoState: (ssoState: string) => Promise<void>;
+  abstract setSsoState: (ssoState: string) => Promise<void>;
  /**
   * Gets the value of the user's organization sso identifier.
   *
@@ -50,20 +48,20 @@ export abstract class SsoLoginServiceAbstraction {
   * Do not use this value outside of the SSO login flow.
   * @returns The user's organization identifier.
   */
-  getOrganizationSsoIdentifier: () => Promise<string>;
+  abstract getOrganizationSsoIdentifier: () => Promise<string>;
  /**
   * Sets the value of the user's organization sso identifier.
   *
   * This should only be used during the SSO flow to identify the organization that the user is attempting to log in to.
   * Do not use this value outside of the SSO login flow.
   */
-  setOrganizationSsoIdentifier: (organizationIdentifier: string) => Promise<void>;
+  abstract setOrganizationSsoIdentifier: (organizationIdentifier: string) => Promise<void>;
  /**
   * Gets the user's email.
   * Note: This should only be used during the SSO flow to identify the user that is attempting to log in.
   * @returns The user's email.
   */
-  getSsoEmail: () => Promise<string>;
+  abstract getSsoEmail: () => Promise<string>;
  /**
   * Sets the user's email.
   * Note: This should only be used during the SSO flow to identify the user that is attempting to log in.
@@ -71,20 +69,20 @@ export abstract class SsoLoginServiceAbstraction {
   * @returns A promise that resolves when the email has been set.
   *
   */
-  setSsoEmail: (email: string) => Promise<void>;
+  abstract setSsoEmail: (email: string) => Promise<void>;
  /**
   * Gets the value of the active user's organization sso identifier.
   *
   * This should only be used post successful SSO login once the user is initialized.
   * @param userId The user id for retrieving the org identifier state.
   */
-  getActiveUserOrganizationSsoIdentifier: (userId: UserId) => Promise<string>;
+  abstract getActiveUserOrganizationSsoIdentifier: (userId: UserId) => Promise<string | null>;
  /**
   * Sets the value of the active user's organization sso identifier.
   *
   * This should only be used post successful SSO login once the user is initialized.
   */
-  setActiveUserOrganizationSsoIdentifier: (
+  abstract setActiveUserOrganizationSsoIdentifier: (
    organizationIdentifier: string,
    userId: UserId | undefined,
  ) => Promise<void>;
--- a/libs/common/src/auth/services/sso-login.service.spec.ts
+++ b/libs/common/src/auth/services/sso-login.service.spec.ts
@@ -87,7 +87,7 @@ describe("SSOLoginService ", () => {
    const orgIdentifier = "test-active-org-identifier";
    await sut.setActiveUserOrganizationSsoIdentifier(orgIdentifier, undefined);

-    expect(mockLogService.warning).toHaveBeenCalledWith(
+    expect(mockLogService.error).toHaveBeenCalledWith(
      "Tried to set a user organization sso identifier with an undefined user id.",
    );
  });
--- a/libs/common/src/auth/services/sso-login.service.ts
+++ b/libs/common/src/auth/services/sso-login.service.ts
@@ -1,5 +1,3 @@
-// FIXME: Update this file to be type safe and remove this and next line
-// @ts-strict-ignore
 import { firstValueFrom } from "rxjs";

 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
@@ -107,7 +105,7 @@ export class SsoLoginService implements SsoLoginServiceAbstraction {
    await this.ssoEmailState.update((_) => email);
  }

-  getActiveUserOrganizationSsoIdentifier(userId: UserId): Promise<string> {
+  getActiveUserOrganizationSsoIdentifier(userId: UserId): Promise<string | null> {
    return firstValueFrom(this.userOrgSsoIdentifierState(userId).state$);
  }

@@ -116,7 +114,7 @@ export class SsoLoginService implements SsoLoginServiceAbstraction {
    userId: UserId | undefined,
  ): Promise<void> {
    if (userId === undefined) {
-      this.logService.warning(
+      this.logService.error(
        "Tried to set a user organization sso identifier with an undefined user id.",
      );
      return;