From 92f027af5e88a900473ef82282a8a4c6ddac273a Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 3 Mar 2025 12:09:35 -0500 Subject: [PATCH] fix(LoginComp + LoginStrategies): [Auth/PM-18654] Refreshed UI - Desktop TDE JIT provisioned user creation errors with missing org SSO id (#13619) * PM-18654 - State Service & Login Strategy Refactor - move env seeding into login strategy so that new accounts always load w/ the correct environment * PM-18654 - SSO Comp - just use user id from auth result * PM-18654 - Config Service - (1) don't allow cascading calls to the renewConfig by using a private promise (2) Replace shareReplay with share configured with manual timer * PM-18654 - LoginComponents - detail issue and possible fix * PM-18654 - DesktopLoginV1Comp - use correct destroy hook * PM-18654 - LoginComp - clean up no longer correct comment * PM-18654 - New Device Verification Component - Remove unused PasswordLoginStrategy dependency * PM-18654 - Browser Home Component - fix qParam logic * PM-18654 - DefaultConfigService - revert changes as they aren't necessary to fix the bug. * PM-18654 - DefaultConfigService - remove commented code * PM-18654 - LoginStrategy - add comment * PM-18654 - Fix login strat tests --- apps/browser/src/auth/popup/home.component.ts | 4 ++- .../src/auth/login/login-v1.component.ts | 9 ++--- .../src/services/jslib-services.module.ts | 33 ------------------- .../auth/src/angular/login/login.component.ts | 3 +- .../new-device-verification.component.ts | 2 -- libs/auth/src/angular/sso/sso.component.ts | 8 ++--- .../auth-request-login.strategy.spec.ts | 4 +++ .../login-strategies/login.strategy.spec.ts | 6 ++++ .../common/login-strategies/login.strategy.ts | 6 ++++ .../password-login.strategy.spec.ts | 4 +++ .../sso-login.strategy.spec.ts | 4 +++ .../user-api-login.strategy.spec.ts | 2 +- .../user-api-login.strategy.ts | 2 -- .../webauthn-login.strategy.spec.ts | 4 +++ .../login-strategy.service.ts | 2 +- .../src/platform/services/state.service.ts | 1 - 16 files changed, 44 insertions(+), 50 deletions(-) diff --git a/apps/browser/src/auth/popup/home.component.ts b/apps/browser/src/auth/popup/home.component.ts index dbdcf7d5aa8..0c4510204d1 100644 --- a/apps/browser/src/auth/popup/home.component.ts +++ b/apps/browser/src/auth/popup/home.component.ts @@ -81,8 +81,10 @@ export class HomeComponent implements OnInit, OnDestroy { tap(async (flag) => { // If the flag is turned ON, we must force a reload to ensure the correct UI is shown if (flag) { + const qParams = await firstValueFrom(this.route.queryParams); + const uniqueQueryParams = { - ...this.route.queryParams, + ...qParams, // adding a unique timestamp to the query params to force a reload t: new Date().getTime().toString(), }; diff --git a/apps/desktop/src/auth/login/login-v1.component.ts b/apps/desktop/src/auth/login/login-v1.component.ts index 5d1a1d818d5..ff8688353be 100644 --- a/apps/desktop/src/auth/login/login-v1.component.ts +++ b/apps/desktop/src/auth/login/login-v1.component.ts @@ -3,7 +3,7 @@ import { Component, NgZone, OnDestroy, OnInit, ViewChild, ViewContainerRef } from "@angular/core"; import { FormBuilder } from "@angular/forms"; import { ActivatedRoute, Router } from "@angular/router"; -import { Subject, takeUntil, tap } from "rxjs"; +import { Subject, firstValueFrom, takeUntil, tap } from "rxjs"; import { LoginComponentV1 as BaseLoginComponent } from "@bitwarden/angular/auth/components/login-v1.component"; import { FormValidationErrorsService } from "@bitwarden/angular/platform/abstractions/form-validation-errors.service"; @@ -143,10 +143,11 @@ export class LoginComponentV1 extends BaseLoginComponent implements OnInit, OnDe .getFeatureFlag$(FeatureFlag.UnauthenticatedExtensionUIRefresh) .pipe( tap(async (flag) => { - // If the flag is turned ON, we must force a reload to ensure the correct UI is shown if (flag) { + const qParams = await firstValueFrom(this.route.queryParams); + const uniqueQueryParams = { - ...this.route.queryParams, + ...qParams, // adding a unique timestamp to the query params to force a reload t: new Date().getTime().toString(), }; @@ -156,7 +157,7 @@ export class LoginComponentV1 extends BaseLoginComponent implements OnInit, OnDe }); } }), - takeUntil(this.destroy$), + takeUntil(this.componentDestroyed$), ) .subscribe(); } diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 3fb578ff7a9..dc6c049101a 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -45,8 +45,6 @@ import { DefaultAuthRequestApiService, DefaultLoginSuccessHandlerService, LoginSuccessHandlerService, - PasswordLoginStrategy, - PasswordLoginStrategyData, LoginApprovalComponentServiceAbstraction, } from "@bitwarden/auth/common"; import { ApiService as ApiServiceAbstraction } from "@bitwarden/common/abstractions/api.service"; @@ -1460,37 +1458,6 @@ const safeProviders: SafeProvider[] = [ useClass: DefaultLoginSuccessHandlerService, deps: [SyncService, UserAsymmetricKeysRegenerationService], }), - safeProvider({ - provide: PasswordLoginStrategy, - useClass: PasswordLoginStrategy, - deps: [ - PasswordLoginStrategyData, - PasswordStrengthServiceAbstraction, - PolicyServiceAbstraction, - LoginStrategyServiceAbstraction, - AccountServiceAbstraction, - InternalMasterPasswordServiceAbstraction, - KeyService, - EncryptService, - ApiServiceAbstraction, - TokenServiceAbstraction, - AppIdServiceAbstraction, - PlatformUtilsServiceAbstraction, - MessagingServiceAbstraction, - LogService, - StateServiceAbstraction, - TwoFactorServiceAbstraction, - InternalUserDecryptionOptionsServiceAbstraction, - BillingAccountProfileStateService, - VaultTimeoutSettingsService, - KdfConfigService, - ], - }), - safeProvider({ - provide: PasswordLoginStrategyData, - useClass: PasswordLoginStrategyData, - deps: [], - }), safeProvider({ provide: TaskService, useClass: DefaultTaskService, diff --git a/libs/auth/src/angular/login/login.component.ts b/libs/auth/src/angular/login/login.component.ts index a84fb93bd23..c291a64a8c5 100644 --- a/libs/auth/src/angular/login/login.component.ts +++ b/libs/auth/src/angular/login/login.component.ts @@ -161,8 +161,9 @@ export class LoginComponent implements OnInit, OnDestroy { tap(async (flag) => { // If the flag is turned OFF, we must force a reload to ensure the correct UI is shown if (!flag) { + const qParams = await firstValueFrom(this.activatedRoute.queryParams); const uniqueQueryParams = { - ...this.activatedRoute.queryParams, + ...qParams, // adding a unique timestamp to the query params to force a reload t: new Date().getTime().toString(), // Adding a unique timestamp as a query parameter }; diff --git a/libs/auth/src/angular/new-device-verification/new-device-verification.component.ts b/libs/auth/src/angular/new-device-verification/new-device-verification.component.ts index 6e0f9eec05e..6c48a471d08 100644 --- a/libs/auth/src/angular/new-device-verification/new-device-verification.component.ts +++ b/libs/auth/src/angular/new-device-verification/new-device-verification.component.ts @@ -20,7 +20,6 @@ import { import { LoginEmailServiceAbstraction } from "../../common/abstractions/login-email.service"; import { LoginStrategyServiceAbstraction } from "../../common/abstractions/login-strategy.service"; -import { PasswordLoginStrategy } from "../../common/login-strategies/password-login.strategy"; /** * Component for verifying a new device via a one-time password (OTP). @@ -58,7 +57,6 @@ export class NewDeviceVerificationComponent implements OnInit, OnDestroy { constructor( private router: Router, private formBuilder: FormBuilder, - private passwordLoginStrategy: PasswordLoginStrategy, private apiService: ApiService, private loginStrategyService: LoginStrategyServiceAbstraction, private logService: LogService, diff --git a/libs/auth/src/angular/sso/sso.component.ts b/libs/auth/src/angular/sso/sso.component.ts index cc9e5f83c01..ce63769ffca 100644 --- a/libs/auth/src/angular/sso/sso.component.ts +++ b/libs/auth/src/angular/sso/sso.component.ts @@ -427,7 +427,6 @@ export class SsoComponent implements OnInit { ); this.formPromise = this.loginStrategyService.logIn(credentials); const authResult = await this.formPromise; - if (authResult.requiresTwoFactor) { return await this.handleTwoFactorRequired(orgSsoIdentifier); } @@ -441,9 +440,10 @@ export class SsoComponent implements OnInit { // - Browser SSO on extension open // Note: you cannot set this in state before 2FA b/c there won't be an account in state. - // 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); + await this.ssoLoginService.setActiveUserOrganizationSsoIdentifier( + orgSsoIdentifier, + authResult.userId, + ); // must come after 2fa check since user decryption options aren't available if 2fa is required const userDecryptionOpts = await firstValueFrom( diff --git a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts index ec0918b4197..0646da4862b 100644 --- a/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts @@ -14,6 +14,7 @@ import { VaultTimeoutSettingsService, } from "@bitwarden/common/key-management/vault-timeout"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -53,6 +54,7 @@ describe("AuthRequestLoginStrategy", () => { let billingAccountProfileStateService: MockProxy; let vaultTimeoutSettingsService: MockProxy; let kdfConfigService: MockProxy; + let environmentService: MockProxy; const mockUserId = Utils.newGuid() as UserId; let accountService: FakeAccountService; @@ -88,6 +90,7 @@ describe("AuthRequestLoginStrategy", () => { billingAccountProfileStateService = mock(); vaultTimeoutSettingsService = mock(); kdfConfigService = mock(); + environmentService = mock(); accountService = mockAccountServiceWith(mockUserId); masterPasswordService = new FakeMasterPasswordService(); @@ -117,6 +120,7 @@ describe("AuthRequestLoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); tokenResponse = identityTokenResponseFactory(); diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index e1558a3de8b..b4a1e6a77d9 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -25,6 +25,7 @@ import { VaultTimeoutSettingsService, } from "@bitwarden/common/key-management/vault-timeout"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -123,6 +124,7 @@ describe("LoginStrategy", () => { let billingAccountProfileStateService: MockProxy; let vaultTimeoutSettingsService: MockProxy; let kdfConfigService: MockProxy; + let environmentService: MockProxy; let passwordLoginStrategy: PasswordLoginStrategy; let credentials: PasswordLoginCredentials; @@ -147,6 +149,7 @@ describe("LoginStrategy", () => { policyService = mock(); passwordStrengthService = mock(); billingAccountProfileStateService = mock(); + environmentService = mock(); vaultTimeoutSettingsService = mock(); @@ -175,6 +178,7 @@ describe("LoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); credentials = new PasswordLoginCredentials(email, masterPassword); }); @@ -496,6 +500,7 @@ describe("LoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); apiService.postIdentityToken.mockResolvedValue(identityTokenResponseFactory()); @@ -559,6 +564,7 @@ describe("LoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); const result = await passwordLoginStrategy.logIn(credentials); diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index a7299c7f0d0..89802c609c0 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -27,6 +27,7 @@ import { } from "@bitwarden/common/key-management/vault-timeout"; import { KeysRequest } from "@bitwarden/common/models/request/keys.request"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -93,6 +94,7 @@ export abstract class LoginStrategy { protected billingAccountProfileStateService: BillingAccountProfileStateService, protected vaultTimeoutSettingsService: VaultTimeoutSettingsService, protected KdfConfigService: KdfConfigService, + protected environmentService: EnvironmentService, ) {} abstract exportCache(): CacheData; @@ -196,6 +198,10 @@ export abstract class LoginStrategy { emailVerified: accountInformation.email_verified ?? false, }); + // User env must be seeded from currently set env before switching to the account + // to avoid any incorrect emissions of the global default env. + await this.environmentService.seedUserEnvironment(userId); + await this.accountService.switchAccount(userId); await this.stateService.addAccount( diff --git a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts index 78fad4443c3..0821405e535 100644 --- a/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/password-login.strategy.spec.ts @@ -18,6 +18,7 @@ import { VaultTimeoutSettingsService, } from "@bitwarden/common/key-management/vault-timeout"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -80,6 +81,7 @@ describe("PasswordLoginStrategy", () => { let billingAccountProfileStateService: MockProxy; let vaultTimeoutSettingsService: MockProxy; let kdfConfigService: MockProxy; + let environmentService: MockProxy; let passwordLoginStrategy: PasswordLoginStrategy; let credentials: PasswordLoginCredentials; @@ -106,6 +108,7 @@ describe("PasswordLoginStrategy", () => { billingAccountProfileStateService = mock(); vaultTimeoutSettingsService = mock(); kdfConfigService = mock(); + environmentService = mock(); appIdService.getAppId.mockResolvedValue(deviceId); tokenService.decodeAccessToken.mockResolvedValue({ @@ -144,6 +147,7 @@ describe("PasswordLoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); credentials = new PasswordLoginCredentials(email, masterPassword); tokenResponse = identityTokenResponseFactory(masterPasswordPolicy); diff --git a/libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts index 5c508dd0c56..6efb17a8d26 100644 --- a/libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts @@ -19,6 +19,7 @@ import { } from "@bitwarden/common/key-management/vault-timeout"; import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; @@ -63,6 +64,7 @@ describe("SsoLoginStrategy", () => { let billingAccountProfileStateService: MockProxy; let vaultTimeoutSettingsService: MockProxy; let kdfConfigService: MockProxy; + let environmentService: MockProxy; let ssoLoginStrategy: SsoLoginStrategy; let credentials: SsoLoginCredentials; @@ -98,6 +100,7 @@ describe("SsoLoginStrategy", () => { billingAccountProfileStateService = mock(); vaultTimeoutSettingsService = mock(); kdfConfigService = mock(); + environmentService = mock(); tokenService.getTwoFactorToken.mockResolvedValue(null); appIdService.getAppId.mockResolvedValue(deviceId); @@ -142,6 +145,7 @@ describe("SsoLoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); credentials = new SsoLoginCredentials(ssoCode, ssoCodeVerifier, ssoRedirectUrl, ssoOrgId); }); diff --git a/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts index de411c06b6b..c0c7e828b68 100644 --- a/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts @@ -97,7 +97,6 @@ describe("UserApiLoginStrategy", () => { apiLogInStrategy = new UserApiLoginStrategy( cache, - environmentService, keyConnectorService, accountService, masterPasswordService, @@ -115,6 +114,7 @@ describe("UserApiLoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); credentials = new UserApiLoginCredentials(apiClientId, apiClientSecret); diff --git a/libs/auth/src/common/login-strategies/user-api-login.strategy.ts b/libs/auth/src/common/login-strategies/user-api-login.strategy.ts index 4f6352b5d74..0bff20b4a65 100644 --- a/libs/auth/src/common/login-strategies/user-api-login.strategy.ts +++ b/libs/auth/src/common/login-strategies/user-api-login.strategy.ts @@ -7,7 +7,6 @@ import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-con import { UserApiTokenRequest } from "@bitwarden/common/auth/models/request/identity-token/user-api-token.request"; import { IdentityTokenResponse } from "@bitwarden/common/auth/models/response/identity-token.response"; import { VaultTimeoutAction } from "@bitwarden/common/key-management/vault-timeout"; -import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { UserId } from "@bitwarden/common/types/guid"; import { UserApiLoginCredentials } from "../models/domain/login-credentials"; @@ -31,7 +30,6 @@ export class UserApiLoginStrategy extends LoginStrategy { constructor( data: UserApiLoginStrategyData, - private environmentService: EnvironmentService, private keyConnectorService: KeyConnectorService, ...sharedDeps: ConstructorParameters ) { diff --git a/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts b/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts index f4282a4b4df..837c6a2a910 100644 --- a/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts @@ -16,6 +16,7 @@ import { VaultTimeoutSettingsService, } from "@bitwarden/common/key-management/vault-timeout"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; +import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -52,6 +53,7 @@ describe("WebAuthnLoginStrategy", () => { let billingAccountProfileStateService: MockProxy; let vaultTimeoutSettingsService: MockProxy; let kdfConfigService: MockProxy; + let environmentService: MockProxy; let webAuthnLoginStrategy!: WebAuthnLoginStrategy; @@ -95,6 +97,7 @@ describe("WebAuthnLoginStrategy", () => { billingAccountProfileStateService = mock(); vaultTimeoutSettingsService = mock(); kdfConfigService = mock(); + environmentService = mock(); tokenService.getTwoFactorToken.mockResolvedValue(null); appIdService.getAppId.mockResolvedValue(deviceId); @@ -120,6 +123,7 @@ describe("WebAuthnLoginStrategy", () => { billingAccountProfileStateService, vaultTimeoutSettingsService, kdfConfigService, + environmentService, ); // Create credentials diff --git a/libs/auth/src/common/services/login-strategies/login-strategy.service.ts b/libs/auth/src/common/services/login-strategies/login-strategy.service.ts index 20410c76f1f..4068c09338b 100644 --- a/libs/auth/src/common/services/login-strategies/login-strategy.service.ts +++ b/libs/auth/src/common/services/login-strategies/login-strategy.service.ts @@ -402,6 +402,7 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction { this.billingAccountProfileStateService, this.vaultTimeoutSettingsService, this.kdfConfigService, + this.environmentService, ]; return source.pipe( @@ -430,7 +431,6 @@ export class LoginStrategyService implements LoginStrategyServiceAbstraction { case AuthenticationType.UserApiKey: return new UserApiLoginStrategy( data?.userApiKey ?? new UserApiLoginStrategyData(), - this.environmentService, this.keyConnectorService, ...sharedDeps, ); diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index d452732aa3d..a78a9b37a8c 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -134,7 +134,6 @@ export class StateService< } async addAccount(account: TAccount) { - await this.environmentService.seedUserEnvironment(account.profile.userId as UserId); await this.updateState(async (state) => { state.accounts[account.profile.userId] = account; return state;