From 5bd4d1691e303ffca16fc8979ff9287ed9d2cf9f Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 23 Jun 2025 11:45:27 -0700 Subject: [PATCH] refactor(auth-guard): [PM-22822] Update AuthGuard to explicitly handle each forceSetPasswordReason (#15252) Update the `authGuard` to explicitly handle each `ForceSetPasswordReason` --- .../src/auth/guards/auth.guard.spec.ts | 200 +++++++++++++----- libs/angular/src/auth/guards/auth.guard.ts | 47 +++- .../domain/force-set-password-reason.ts | 51 +++-- libs/common/src/enums/feature-flag.enum.ts | 2 + 4 files changed, 226 insertions(+), 74 deletions(-) diff --git a/libs/angular/src/auth/guards/auth.guard.spec.ts b/libs/angular/src/auth/guards/auth.guard.spec.ts index db08553749a..f64d6cf769d 100644 --- a/libs/angular/src/auth/guards/auth.guard.spec.ts +++ b/libs/angular/src/auth/guards/auth.guard.spec.ts @@ -13,8 +13,10 @@ import { import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service"; import { MasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { UserId } from "@bitwarden/common/types/guid"; @@ -25,12 +27,14 @@ describe("AuthGuard", () => { authStatus: AuthenticationStatus, forceSetPasswordReason: ForceSetPasswordReason, keyConnectorServiceRequiresAccountConversion: boolean = false, + featureFlag: FeatureFlag | null = null, ) => { const authService: MockProxy = mock(); authService.getAuthStatus.mockResolvedValue(authStatus); const messagingService: MockProxy = mock(); const keyConnectorService: MockProxy = mock(); keyConnectorService.convertAccountRequired$ = of(keyConnectorServiceRequiresAccountConversion); + const configService: MockProxy = mock(); const accountService: MockProxy = mock(); const activeAccountSubject = new BehaviorSubject(null); accountService.activeAccount$ = activeAccountSubject; @@ -45,6 +49,12 @@ describe("AuthGuard", () => { ), ); + if (featureFlag) { + configService.getFeatureFlag.mockResolvedValue(true); + } else { + configService.getFeatureFlag.mockResolvedValue(false); + } + const forceSetPasswordReasonSubject = new BehaviorSubject( forceSetPasswordReason, ); @@ -59,7 +69,10 @@ describe("AuthGuard", () => { { path: "guarded-route", component: EmptyComponent, canActivate: [authGuard] }, { path: "lock", component: EmptyComponent }, { path: "set-password", component: EmptyComponent }, + { path: "set-password-jit", component: EmptyComponent }, + { path: "set-initial-password", component: EmptyComponent }, { path: "update-temp-password", component: EmptyComponent }, + { path: "change-password", component: EmptyComponent }, { path: "remove-password", component: EmptyComponent }, ]), ], @@ -69,6 +82,7 @@ describe("AuthGuard", () => { { provide: KeyConnectorService, useValue: keyConnectorService }, { provide: AccountService, useValue: accountService }, { provide: MasterPasswordServiceAbstraction, useValue: masterPasswordService }, + { provide: ConfigService, useValue: configService }, ], }); @@ -110,70 +124,152 @@ describe("AuthGuard", () => { expect(router.url).toBe("/remove-password"); }); - it("should redirect to set-password when user is TDE user without password and has password reset permission", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, - ); + describe("given user is Unlocked", () => { + describe("given the PM16117_SetInitialPasswordRefactor feature flag is ON", () => { + const tests = [ + ForceSetPasswordReason.SsoNewJitProvisionedUser, + ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, + ForceSetPasswordReason.TdeOffboarding, + ]; - await router.navigate(["guarded-route"]); - expect(router.url).toContain("/set-password"); - }); + describe("given user attempts to navigate to an auth guarded route", () => { + tests.forEach((reason) => { + it(`should redirect to /set-initial-password when the user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup( + AuthenticationStatus.Unlocked, + reason, + false, + FeatureFlag.PM16117_SetInitialPasswordRefactor, + ); - it("should redirect to update-temp-password when user has force set password reason", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.AdminForcePasswordReset, - ); + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/set-initial-password"); + }); + }); + }); - await router.navigate(["guarded-route"]); - expect(router.url).toContain("/update-temp-password"); - }); + describe("given user attempts to navigate to /set-initial-password", () => { + tests.forEach((reason) => { + it(`should allow navigation to continue to /set-initial-password when the user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup( + AuthenticationStatus.Unlocked, + reason, + false, + FeatureFlag.PM16117_SetInitialPasswordRefactor, + ); - it("should redirect to update-temp-password when user has weak password", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.WeakMasterPassword, - ); + await router.navigate(["/set-initial-password"]); + expect(router.url).toContain("/set-initial-password"); + }); + }); + }); + }); - await router.navigate(["guarded-route"]); - expect(router.url).toContain("/update-temp-password"); - }); + describe("given the PM16117_SetInitialPasswordRefactor feature flag is OFF", () => { + const tests = [ + { + reason: ForceSetPasswordReason.SsoNewJitProvisionedUser, + url: "/set-password-jit", + }, + { + reason: ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, + url: "/set-password", + }, + { + reason: ForceSetPasswordReason.TdeOffboarding, + url: "/update-temp-password", + }, + ]; - it("should allow navigation to set-password when the user is unlocked, is a TDE user without password, and has password reset permission", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, - ); + describe("given user attempts to navigate to an auth guarded route", () => { + tests.forEach(({ reason, url }) => { + it(`should redirect to ${url} when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup(AuthenticationStatus.Unlocked, reason); - await router.navigate(["/set-password"]); - expect(router.url).toContain("/set-password"); - }); + await router.navigate(["/guarded-route"]); + expect(router.url).toContain(url); + }); + }); + }); - it("should allow navigation to update-temp-password when the user is unlocked and has admin force password reset permission", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.AdminForcePasswordReset, - ); + describe("given user attempts to navigate to the set- or update- password route itself", () => { + tests.forEach(({ reason, url }) => { + it(`should allow navigation to continue to ${url} when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup(AuthenticationStatus.Unlocked, reason); - await router.navigate(["/update-temp-password"]); - expect(router.url).toContain("/update-temp-password"); - }); + await router.navigate([url]); + expect(router.url).toContain(url); + }); + }); + }); + }); - it("should allow navigation to update-temp-password when the user is unlocked and has weak password", async () => { - const { router } = setup( - AuthenticationStatus.Unlocked, - ForceSetPasswordReason.WeakMasterPassword, - ); + describe("given the PM16117_ChangeExistingPasswordRefactor feature flag is ON", () => { + const tests = [ + ForceSetPasswordReason.AdminForcePasswordReset, + ForceSetPasswordReason.WeakMasterPassword, + ]; - await router.navigate(["/update-temp-password"]); - expect(router.url).toContain("/update-temp-password"); - }); + describe("given user attempts to navigate to an auth guarded route", () => { + tests.forEach((reason) => { + it(`should redirect to /change-password when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup( + AuthenticationStatus.Unlocked, + reason, + false, + FeatureFlag.PM16117_ChangeExistingPasswordRefactor, + ); - it("should allow navigation to remove-password when the user is unlocked and has 'none' password reset permission", async () => { - const { router } = setup(AuthenticationStatus.Unlocked, ForceSetPasswordReason.None); + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/change-password"); + }); + }); + }); - await router.navigate(["/remove-password"]); - expect(router.url).toContain("/remove-password"); + describe("given user attempts to navigate to /change-password", () => { + tests.forEach((reason) => { + it(`should allow navigation to /change-password when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup( + AuthenticationStatus.Unlocked, + ForceSetPasswordReason.AdminForcePasswordReset, + false, + FeatureFlag.PM16117_ChangeExistingPasswordRefactor, + ); + + await router.navigate(["/change-password"]); + expect(router.url).toContain("/change-password"); + }); + }); + }); + }); + + describe("given the PM16117_ChangeExistingPasswordRefactor feature flag is OFF", () => { + const tests = [ + ForceSetPasswordReason.AdminForcePasswordReset, + ForceSetPasswordReason.WeakMasterPassword, + ]; + + describe("given user attempts to navigate to an auth guarded route", () => { + tests.forEach((reason) => { + it(`should redirect to /update-temp-password when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup(AuthenticationStatus.Unlocked, reason); + + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/update-temp-password"); + }); + }); + }); + + describe("given user attempts to navigate to /update-temp-password", () => { + tests.forEach((reason) => { + it(`should allow navigation to continue to /update-temp-password when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => { + const { router } = setup(AuthenticationStatus.Unlocked, reason); + + await router.navigate(["/update-temp-password"]); + expect(router.url).toContain("/update-temp-password"); + }); + }); + }); + }); }); }); diff --git a/libs/angular/src/auth/guards/auth.guard.ts b/libs/angular/src/auth/guards/auth.guard.ts index 690db37d090..a172c45d6f9 100644 --- a/libs/angular/src/auth/guards/auth.guard.ts +++ b/libs/angular/src/auth/guards/auth.guard.ts @@ -14,8 +14,10 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service"; import { MasterPasswordServiceAbstraction } from "@bitwarden/common/key-management/master-password/abstractions/master-password.service.abstraction"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; export const authGuard: CanActivateFn = async ( @@ -28,6 +30,7 @@ export const authGuard: CanActivateFn = async ( const keyConnectorService = inject(KeyConnectorService); const accountService = inject(AccountService); const masterPasswordService = inject(MasterPasswordServiceAbstraction); + const configService = inject(ConfigService); const authStatus = await authService.getAuthStatus(); @@ -57,19 +60,53 @@ export const authGuard: CanActivateFn = async ( masterPasswordService.forceSetPasswordReason$(userId), ); + const isSetInitialPasswordFlagOn = await configService.getFeatureFlag( + FeatureFlag.PM16117_SetInitialPasswordRefactor, + ); + const isChangePasswordFlagOn = await configService.getFeatureFlag( + FeatureFlag.PM16117_ChangeExistingPasswordRefactor, + ); + + // User JIT provisioned into a master-password-encryption org + if ( + forceSetPasswordReason === ForceSetPasswordReason.SsoNewJitProvisionedUser && + !routerState.url.includes("set-password-jit") && + !routerState.url.includes("set-initial-password") + ) { + const route = isSetInitialPasswordFlagOn ? "/set-initial-password" : "/set-password-jit"; + return router.createUrlTree([route]); + } + + // TDE org user has "manage account recovery" permission if ( forceSetPasswordReason === ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission && - !routerState.url.includes("set-password") + !routerState.url.includes("set-password") && + !routerState.url.includes("set-initial-password") ) { - return router.createUrlTree(["/set-password"]); + const route = isSetInitialPasswordFlagOn ? "/set-initial-password" : "/set-password"; + return router.createUrlTree([route]); } + // TDE Offboarding if ( - forceSetPasswordReason !== ForceSetPasswordReason.None && - !routerState.url.includes("update-temp-password") + forceSetPasswordReason === ForceSetPasswordReason.TdeOffboarding && + !routerState.url.includes("update-temp-password") && + !routerState.url.includes("set-initial-password") ) { - return router.createUrlTree(["/update-temp-password"]); + const route = isSetInitialPasswordFlagOn ? "/set-initial-password" : "/update-temp-password"; + return router.createUrlTree([route]); + } + + // Post- Account Recovery or Weak Password on login + if ( + forceSetPasswordReason === ForceSetPasswordReason.AdminForcePasswordReset || + (forceSetPasswordReason === ForceSetPasswordReason.WeakMasterPassword && + !routerState.url.includes("update-temp-password") && + !routerState.url.includes("change-password")) + ) { + const route = isChangePasswordFlagOn ? "/change-password" : "/update-temp-password"; + return router.createUrlTree([route]); } return true; diff --git a/libs/common/src/auth/models/domain/force-set-password-reason.ts b/libs/common/src/auth/models/domain/force-set-password-reason.ts index 68392125281..4a8ec8529cf 100644 --- a/libs/common/src/auth/models/domain/force-set-password-reason.ts +++ b/libs/common/src/auth/models/domain/force-set-password-reason.ts @@ -1,41 +1,58 @@ -/* - * This enum is used to determine if a user should be forced to initially set or reset their password - * on login (server flag) or unlock via MP (client evaluation). +/** + * This enum is used to determine if a user should be forced to set an initial password or + * change their existing password upon login (communicated via server flag) or upon unlocking + * with their master password (set via client evaluation). */ // FIXME: update to use a const object instead of a typescript enum // eslint-disable-next-line @bitwarden/platform/no-enums export enum ForceSetPasswordReason { /** - * A password reset should not be forced. + * A password set/change should not be forced. */ None, - /** - * Occurs when an organization admin forces a user to reset their password. - * Communicated via server flag. - */ - AdminForcePasswordReset, + /*-------------------------- + Set Initial Password + ---------------------------*/ /** - * Occurs when a user logs in / unlocks their vault with a master password that does not meet an organization's - * master password policy that is enforced on login/unlock. - * Only set client side b/c server can't evaluate MP. + * Occurs when a user JIT provisions into a master-password-encryption org via SSO and must set their initial password. */ - WeakMasterPassword, + SsoNewJitProvisionedUser, /** - * Occurs when a TDE user without a password obtains the password reset permission. + * Occurs when a TDE org user without a password obtains the password reset ("manage account recovery") + * permission, which requires the TDE user to have/set a password. + * * Set post login & decryption client side and by server in sync (to catch logged in users). */ TdeUserWithoutPasswordHasPasswordResetPermission, /** - * Occurs when TDE is disabled and master password has to be set. + * Occurs when an org admin switches the org from trusted-device-encryption to master-password-encryption, + * which forces the org user to set an initial password. User must not already have a master password, + * and they must be on a previously trusted device. + * + * Communicated via server flag. */ TdeOffboarding, + /*---------------------------- + Change Existing Password + -----------------------------*/ + /** - * Occurs when a new SSO user is JIT provisioned and needs to set their master password. + * Occurs when an org admin forces a user to change their password via Account Recovery. + * + * Communicated via server flag. */ - SsoNewJitProvisionedUser, + AdminForcePasswordReset, + + /** + * Occurs when a user logs in / unlocks their vault with a master password that does not meet an org's + * master password policy that is enforced on login/unlock. + * + * Only set client side b/c server can't evaluate MP. + */ + WeakMasterPassword, } diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 9cec89d9d5d..85821c6fe90 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -15,6 +15,7 @@ export enum FeatureFlag { OptimizeNestedTraverseTypescript = "pm-21695-optimize-nested-traverse-typescript", /* Auth */ + PM16117_SetInitialPasswordRefactor = "pm-16117-set-initial-password-refactor", PM16117_ChangeExistingPasswordRefactor = "pm-16117-change-existing-password-refactor", PM9115_TwoFactorExtensionDataPersistence = "pm-9115-two-factor-extension-data-persistence", @@ -101,6 +102,7 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.RemoveCardItemTypePolicy]: FALSE, /* Auth */ + [FeatureFlag.PM16117_SetInitialPasswordRefactor]: FALSE, [FeatureFlag.PM16117_ChangeExistingPasswordRefactor]: FALSE, [FeatureFlag.PM9115_TwoFactorExtensionDataPersistence]: FALSE,