1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-11 13:53:34 +00:00

refactor(auth-guard): [PM-22822] Update AuthGuard to explicitly handle each forceSetPasswordReason (#15252)

Update the `authGuard` to explicitly handle each `ForceSetPasswordReason`
This commit is contained in:
rr-bw
2025-06-23 11:45:27 -07:00
committed by GitHub
parent e291e2df0a
commit 5bd4d1691e
4 changed files with 226 additions and 74 deletions

View File

@@ -13,8 +13,10 @@ import {
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; 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 { 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 { 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 { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
import { UserId } from "@bitwarden/common/types/guid"; import { UserId } from "@bitwarden/common/types/guid";
@@ -25,12 +27,14 @@ describe("AuthGuard", () => {
authStatus: AuthenticationStatus, authStatus: AuthenticationStatus,
forceSetPasswordReason: ForceSetPasswordReason, forceSetPasswordReason: ForceSetPasswordReason,
keyConnectorServiceRequiresAccountConversion: boolean = false, keyConnectorServiceRequiresAccountConversion: boolean = false,
featureFlag: FeatureFlag | null = null,
) => { ) => {
const authService: MockProxy<AuthService> = mock<AuthService>(); const authService: MockProxy<AuthService> = mock<AuthService>();
authService.getAuthStatus.mockResolvedValue(authStatus); authService.getAuthStatus.mockResolvedValue(authStatus);
const messagingService: MockProxy<MessagingService> = mock<MessagingService>(); const messagingService: MockProxy<MessagingService> = mock<MessagingService>();
const keyConnectorService: MockProxy<KeyConnectorService> = mock<KeyConnectorService>(); const keyConnectorService: MockProxy<KeyConnectorService> = mock<KeyConnectorService>();
keyConnectorService.convertAccountRequired$ = of(keyConnectorServiceRequiresAccountConversion); keyConnectorService.convertAccountRequired$ = of(keyConnectorServiceRequiresAccountConversion);
const configService: MockProxy<ConfigService> = mock<ConfigService>();
const accountService: MockProxy<AccountService> = mock<AccountService>(); const accountService: MockProxy<AccountService> = mock<AccountService>();
const activeAccountSubject = new BehaviorSubject<Account | null>(null); const activeAccountSubject = new BehaviorSubject<Account | null>(null);
accountService.activeAccount$ = activeAccountSubject; 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>( const forceSetPasswordReasonSubject = new BehaviorSubject<ForceSetPasswordReason>(
forceSetPasswordReason, forceSetPasswordReason,
); );
@@ -59,7 +69,10 @@ describe("AuthGuard", () => {
{ path: "guarded-route", component: EmptyComponent, canActivate: [authGuard] }, { path: "guarded-route", component: EmptyComponent, canActivate: [authGuard] },
{ path: "lock", component: EmptyComponent }, { path: "lock", component: EmptyComponent },
{ path: "set-password", 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: "update-temp-password", component: EmptyComponent },
{ path: "change-password", component: EmptyComponent },
{ path: "remove-password", component: EmptyComponent }, { path: "remove-password", component: EmptyComponent },
]), ]),
], ],
@@ -69,6 +82,7 @@ describe("AuthGuard", () => {
{ provide: KeyConnectorService, useValue: keyConnectorService }, { provide: KeyConnectorService, useValue: keyConnectorService },
{ provide: AccountService, useValue: accountService }, { provide: AccountService, useValue: accountService },
{ provide: MasterPasswordServiceAbstraction, useValue: masterPasswordService }, { provide: MasterPasswordServiceAbstraction, useValue: masterPasswordService },
{ provide: ConfigService, useValue: configService },
], ],
}); });
@@ -110,70 +124,152 @@ describe("AuthGuard", () => {
expect(router.url).toBe("/remove-password"); 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 () => { describe("given user is Unlocked", () => {
const { router } = setup( describe("given the PM16117_SetInitialPasswordRefactor feature flag is ON", () => {
AuthenticationStatus.Unlocked, const tests = [
ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, ForceSetPasswordReason.SsoNewJitProvisionedUser,
); ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission,
ForceSetPasswordReason.TdeOffboarding,
];
await router.navigate(["guarded-route"]); describe("given user attempts to navigate to an auth guarded route", () => {
expect(router.url).toContain("/set-password"); 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 () => { await router.navigate(["guarded-route"]);
const { router } = setup( expect(router.url).toContain("/set-initial-password");
AuthenticationStatus.Unlocked, });
ForceSetPasswordReason.AdminForcePasswordReset, });
); });
await router.navigate(["guarded-route"]); describe("given user attempts to navigate to /set-initial-password", () => {
expect(router.url).toContain("/update-temp-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 () => { await router.navigate(["/set-initial-password"]);
const { router } = setup( expect(router.url).toContain("/set-initial-password");
AuthenticationStatus.Unlocked, });
ForceSetPasswordReason.WeakMasterPassword, });
); });
});
await router.navigate(["guarded-route"]); describe("given the PM16117_SetInitialPasswordRefactor feature flag is OFF", () => {
expect(router.url).toContain("/update-temp-password"); 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 () => { describe("given user attempts to navigate to an auth guarded route", () => {
const { router } = setup( tests.forEach(({ reason, url }) => {
AuthenticationStatus.Unlocked, it(`should redirect to ${url} when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => {
ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission, const { router } = setup(AuthenticationStatus.Unlocked, reason);
);
await router.navigate(["/set-password"]); await router.navigate(["/guarded-route"]);
expect(router.url).toContain("/set-password"); 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 () => { describe("given user attempts to navigate to the set- or update- password route itself", () => {
const { router } = setup( tests.forEach(({ reason, url }) => {
AuthenticationStatus.Unlocked, it(`should allow navigation to continue to ${url} when user has ForceSetPasswordReason.${ForceSetPasswordReason[reason]}`, async () => {
ForceSetPasswordReason.AdminForcePasswordReset, const { router } = setup(AuthenticationStatus.Unlocked, reason);
);
await router.navigate(["/update-temp-password"]); await router.navigate([url]);
expect(router.url).toContain("/update-temp-password"); expect(router.url).toContain(url);
}); });
});
});
});
it("should allow navigation to update-temp-password when the user is unlocked and has weak password", async () => { describe("given the PM16117_ChangeExistingPasswordRefactor feature flag is ON", () => {
const { router } = setup( const tests = [
AuthenticationStatus.Unlocked, ForceSetPasswordReason.AdminForcePasswordReset,
ForceSetPasswordReason.WeakMasterPassword, ForceSetPasswordReason.WeakMasterPassword,
); ];
await router.navigate(["/update-temp-password"]); describe("given user attempts to navigate to an auth guarded route", () => {
expect(router.url).toContain("/update-temp-password"); 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 () => { await router.navigate(["guarded-route"]);
const { router } = setup(AuthenticationStatus.Unlocked, ForceSetPasswordReason.None); expect(router.url).toContain("/change-password");
});
});
});
await router.navigate(["/remove-password"]); describe("given user attempts to navigate to /change-password", () => {
expect(router.url).toContain("/remove-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");
});
});
});
});
}); });
}); });

View File

@@ -14,8 +14,10 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv
import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; 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 { 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 { 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 { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
export const authGuard: CanActivateFn = async ( export const authGuard: CanActivateFn = async (
@@ -28,6 +30,7 @@ export const authGuard: CanActivateFn = async (
const keyConnectorService = inject(KeyConnectorService); const keyConnectorService = inject(KeyConnectorService);
const accountService = inject(AccountService); const accountService = inject(AccountService);
const masterPasswordService = inject(MasterPasswordServiceAbstraction); const masterPasswordService = inject(MasterPasswordServiceAbstraction);
const configService = inject(ConfigService);
const authStatus = await authService.getAuthStatus(); const authStatus = await authService.getAuthStatus();
@@ -57,19 +60,53 @@ export const authGuard: CanActivateFn = async (
masterPasswordService.forceSetPasswordReason$(userId), 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 ( if (
forceSetPasswordReason === forceSetPasswordReason ===
ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission && 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 ( if (
forceSetPasswordReason !== ForceSetPasswordReason.None && forceSetPasswordReason === ForceSetPasswordReason.TdeOffboarding &&
!routerState.url.includes("update-temp-password") !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; return true;

View File

@@ -1,41 +1,58 @@
/* /**
* This enum is used to determine if a user should be forced to initially set or reset their password * This enum is used to determine if a user should be forced to set an initial password or
* on login (server flag) or unlock via MP (client evaluation). * 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 // FIXME: update to use a const object instead of a typescript enum
// eslint-disable-next-line @bitwarden/platform/no-enums // eslint-disable-next-line @bitwarden/platform/no-enums
export enum ForceSetPasswordReason { export enum ForceSetPasswordReason {
/** /**
* A password reset should not be forced. * A password set/change should not be forced.
*/ */
None, None,
/** /*--------------------------
* Occurs when an organization admin forces a user to reset their password. Set Initial Password
* Communicated via server flag. ---------------------------*/
*/
AdminForcePasswordReset,
/** /**
* Occurs when a user logs in / unlocks their vault with a master password that does not meet an organization's * Occurs when a user JIT provisions into a master-password-encryption org via SSO and must set their initial password.
* master password policy that is enforced on login/unlock.
* Only set client side b/c server can't evaluate MP.
*/ */
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). * Set post login & decryption client side and by server in sync (to catch logged in users).
*/ */
TdeUserWithoutPasswordHasPasswordResetPermission, 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, 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,
} }

View File

@@ -15,6 +15,7 @@ export enum FeatureFlag {
OptimizeNestedTraverseTypescript = "pm-21695-optimize-nested-traverse-typescript", OptimizeNestedTraverseTypescript = "pm-21695-optimize-nested-traverse-typescript",
/* Auth */ /* Auth */
PM16117_SetInitialPasswordRefactor = "pm-16117-set-initial-password-refactor",
PM16117_ChangeExistingPasswordRefactor = "pm-16117-change-existing-password-refactor", PM16117_ChangeExistingPasswordRefactor = "pm-16117-change-existing-password-refactor",
PM9115_TwoFactorExtensionDataPersistence = "pm-9115-two-factor-extension-data-persistence", PM9115_TwoFactorExtensionDataPersistence = "pm-9115-two-factor-extension-data-persistence",
@@ -101,6 +102,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.RemoveCardItemTypePolicy]: FALSE, [FeatureFlag.RemoveCardItemTypePolicy]: FALSE,
/* Auth */ /* Auth */
[FeatureFlag.PM16117_SetInitialPasswordRefactor]: FALSE,
[FeatureFlag.PM16117_ChangeExistingPasswordRefactor]: FALSE, [FeatureFlag.PM16117_ChangeExistingPasswordRefactor]: FALSE,
[FeatureFlag.PM9115_TwoFactorExtensionDataPersistence]: FALSE, [FeatureFlag.PM9115_TwoFactorExtensionDataPersistence]: FALSE,