From ad26f0890a1d6c80a852a3fde3746675d77e466e Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 25 Jul 2024 23:00:29 +0200 Subject: [PATCH] [PM-4348] Migrate AuthGuards to functions (#9595) * Migrate auth guards * Fix remaining auth guard migration * Fix unauth guard usage * Add unit tests for auth guard and unauth guard * Remove unused angular DI code * Move auth related logic out fo sm guard * Add tests * Add more tests for unauth guard * Fix incorrect merge --- apps/browser/src/auth/popup/services/index.ts | 1 - .../popup/services/unauth-guard.service.ts | 5 - apps/browser/src/popup/app-routing.module.ts | 74 ++++---- .../src/popup/services/services.module.ts | 8 - apps/desktop/src/app/app-routing.module.ts | 10 +- .../organization-routing.module.ts | 4 +- apps/web/src/app/oss-routing.module.ts | 19 +- .../tools/reports/reports-routing.module.ts | 4 +- .../organizations-routing.module.ts | 6 +- .../providers/providers-routing.module.ts | 8 +- .../app/secrets-manager/guards/sm.guard.ts | 9 - .../app/secrets-manager/sm-routing.module.ts | 6 +- .../src/auth/guards/auth.guard.spec.ts | 177 ++++++++++++++++++ libs/angular/src/auth/guards/auth.guard.ts | 114 +++++------ .../src/auth/guards/unauth.guard.spec.ts | 89 +++++++++ libs/angular/src/auth/guards/unauth.guard.ts | 30 +-- .../src/services/jslib-services.module.ts | 4 - 17 files changed, 392 insertions(+), 176 deletions(-) delete mode 100644 apps/browser/src/auth/popup/services/index.ts delete mode 100644 apps/browser/src/auth/popup/services/unauth-guard.service.ts create mode 100644 libs/angular/src/auth/guards/auth.guard.spec.ts create mode 100644 libs/angular/src/auth/guards/unauth.guard.spec.ts diff --git a/apps/browser/src/auth/popup/services/index.ts b/apps/browser/src/auth/popup/services/index.ts deleted file mode 100644 index 63563f61fd..0000000000 --- a/apps/browser/src/auth/popup/services/index.ts +++ /dev/null @@ -1 +0,0 @@ -export { UnauthGuardService } from "./unauth-guard.service"; diff --git a/apps/browser/src/auth/popup/services/unauth-guard.service.ts b/apps/browser/src/auth/popup/services/unauth-guard.service.ts deleted file mode 100644 index 0fbb4ac9ba..0000000000 --- a/apps/browser/src/auth/popup/services/unauth-guard.service.ts +++ /dev/null @@ -1,5 +0,0 @@ -import { UnauthGuard as BaseUnauthGuardService } from "@bitwarden/angular/auth/guards"; - -export class UnauthGuardService extends BaseUnauthGuardService { - protected homepage = "tabs/current"; -} diff --git a/apps/browser/src/popup/app-routing.module.ts b/apps/browser/src/popup/app-routing.module.ts index 57195564c7..47d451cc01 100644 --- a/apps/browser/src/popup/app-routing.module.ts +++ b/apps/browser/src/popup/app-routing.module.ts @@ -2,7 +2,7 @@ import { Injectable, NgModule } from "@angular/core"; import { ActivatedRouteSnapshot, RouteReuseStrategy, RouterModule, Routes } from "@angular/router"; import { - AuthGuard, + authGuard, lockGuard, redirectGuard, tdeDecryptionRequiredGuard, @@ -191,7 +191,7 @@ const routes: Routes = [ { path: "remove-password", component: RemovePasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "remove-password" }, }, { @@ -215,162 +215,162 @@ const routes: Routes = [ { path: "ciphers", component: VaultItemsComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "ciphers" }, }, ...extensionRefreshSwap(ViewComponent, ViewV2Component, { path: "view-cipher", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "view-cipher" }, }), { path: "cipher-password-history", component: PasswordHistoryComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "cipher-password-history" }, }, ...extensionRefreshSwap(AddEditComponent, AddEditV2Component, { path: "add-cipher", - canActivate: [AuthGuard, debounceNavigationGuard()], + canActivate: [authGuard, debounceNavigationGuard()], data: { state: "add-cipher" }, runGuardsAndResolvers: "always", }), ...extensionRefreshSwap(AddEditComponent, AddEditV2Component, { path: "edit-cipher", - canActivate: [AuthGuard, debounceNavigationGuard()], + canActivate: [authGuard, debounceNavigationGuard()], data: { state: "edit-cipher" }, runGuardsAndResolvers: "always", }), { path: "share-cipher", component: ShareComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "share-cipher" }, }, { path: "collections", component: CollectionsComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "collections" }, }, ...extensionRefreshSwap(AttachmentsComponent, AttachmentsV2Component, { path: "attachments", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "attachments" }, }), { path: "generator", component: GeneratorComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "generator" }, }, { path: "generator-history", component: PasswordGeneratorHistoryComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "generator-history" }, }, ...extensionRefreshSwap(ImportBrowserComponent, ImportBrowserV2Component, { path: "import", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "import" }, }), ...extensionRefreshSwap(ExportBrowserComponent, ExportBrowserV2Component, { path: "export", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "export" }, }), ...extensionRefreshSwap(AutofillV1Component, AutofillComponent, { path: "autofill", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "autofill" }, }), { path: "account-security", component: AccountSecurityComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "account-security" }, }, ...extensionRefreshSwap(NotificationsSettingsV1Component, NotificationsSettingsComponent, { path: "notifications", component: NotificationsSettingsV1Component, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "notifications" }, }), ...extensionRefreshSwap(VaultSettingsComponent, VaultSettingsV2Component, { path: "vault-settings", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "vault-settings" }, }), { path: "folders", component: FoldersComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "folders" }, }, { path: "add-folder", component: FolderAddEditComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "add-folder" }, }, { path: "edit-folder", component: FolderAddEditComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "edit-folder" }, }, { path: "sync", component: SyncComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "sync" }, }, ...extensionRefreshSwap(ExcludedDomainsV1Component, ExcludedDomainsComponent, { path: "excluded-domains", component: ExcludedDomainsV1Component, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "excluded-domains" }, }), { path: "premium", component: PremiumComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "premium" }, }, { path: "appearance", component: AppearanceComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "appearance" }, }, ...extensionRefreshSwap(AddEditComponent, AddEditV2Component, { path: "clone-cipher", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "clone-cipher" }, }), { path: "send-type", component: SendTypeComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "send-type" }, }, { path: "add-send", component: SendAddEditComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "add-send" }, }, { path: "edit-send", component: SendAddEditComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "edit-send" }, }, { path: "update-temp-password", component: UpdateTempPasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "update-temp-password" }, }, { @@ -429,12 +429,12 @@ const routes: Routes = [ }, ...extensionRefreshSwap(AboutPageComponent, AboutPageV2Component, { path: "about", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "about" }, }), ...extensionRefreshSwap(MoreFromBitwardenPageComponent, MoreFromBitwardenPageV2Component, { path: "more-from-bitwarden", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "moreFromBitwarden" }, }), ...extensionRefreshSwap(TabsComponent, TabsV2Component, { @@ -449,30 +449,30 @@ const routes: Routes = [ { path: "current", component: CurrentTabComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], canMatch: [extensionRefreshRedirect("/tabs/vault")], data: { state: "tabs_current" }, runGuardsAndResolvers: "always", }, ...extensionRefreshSwap(VaultFilterComponent, VaultV2Component, { path: "vault", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "tabs_vault" }, }), { path: "generator", component: GeneratorComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "tabs_generator" }, }, ...extensionRefreshSwap(SettingsComponent, SettingsV2Component, { path: "settings", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "tabs_settings" }, }), ...extensionRefreshSwap(SendGroupingsComponent, SendV2Component, { path: "send", - canActivate: [AuthGuard], + canActivate: [authGuard], data: { state: "tabs_send" }, }), ], diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 01470f4d11..2c7129db29 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -1,8 +1,6 @@ import { APP_INITIALIZER, NgModule, NgZone } from "@angular/core"; -import { Router } from "@angular/router"; import { Subject, merge, of } from "rxjs"; -import { UnauthGuard as BaseUnauthGuardService } from "@bitwarden/angular/auth/guards"; import { AngularThemingService } from "@bitwarden/angular/platform/services/theming/angular-theming.service"; import { SafeProvider, safeProvider } from "@bitwarden/angular/platform/utils/safe-provider"; import { @@ -84,7 +82,6 @@ import { TotpService } from "@bitwarden/common/vault/services/totp.service"; import { DialogService, ToastService } from "@bitwarden/components"; import { PasswordRepromptService } from "@bitwarden/vault"; -import { UnauthGuardService } from "../../auth/popup/services"; import { AutofillService as AutofillServiceAbstraction } from "../../autofill/services/abstractions/autofill.service"; import AutofillService from "../../autofill/services/autofill.service"; import MainBackground from "../../background/main.background"; @@ -163,11 +160,6 @@ const safeProviders: SafeProvider[] = [ deps: [InitService], multi: true, }), - safeProvider({ - provide: BaseUnauthGuardService, - useClass: UnauthGuardService, - deps: [AuthService, Router], - }), safeProvider({ provide: CryptoFunctionService, useFactory: () => new WebCryptoFunctionService(window), diff --git a/apps/desktop/src/app/app-routing.module.ts b/apps/desktop/src/app/app-routing.module.ts index a93299e45a..2376eb3844 100644 --- a/apps/desktop/src/app/app-routing.module.ts +++ b/apps/desktop/src/app/app-routing.module.ts @@ -2,7 +2,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; import { - AuthGuard, + authGuard, lockGuard, redirectGuard, tdeDecryptionRequiredGuard, @@ -91,7 +91,7 @@ const routes: Routes = [ { path: "vault", component: VaultComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], }, { path: "accessibility-cookie", component: AccessibilityCookieComponent }, { path: "hint", component: HintComponent }, @@ -100,17 +100,17 @@ const routes: Routes = [ { path: "send", component: SendComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], }, { path: "update-temp-password", component: UpdateTempPasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], }, { path: "remove-password", component: RemovePasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { titleId: "removeMasterPassword" }, }, { diff --git a/apps/web/src/app/admin-console/organizations/organization-routing.module.ts b/apps/web/src/app/admin-console/organizations/organization-routing.module.ts index 7427bbb481..2c8c52efc6 100644 --- a/apps/web/src/app/admin-console/organizations/organization-routing.module.ts +++ b/apps/web/src/app/admin-console/organizations/organization-routing.module.ts @@ -1,7 +1,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; +import { authGuard } from "@bitwarden/angular/auth/guards"; import { featureFlaggedRoute } from "@bitwarden/angular/platform/utils/feature-flagged-route"; import { canAccessOrgAdmin, @@ -26,7 +26,7 @@ const routes: Routes = [ { path: ":organizationId", component: OrganizationLayoutComponent, - canActivate: [deepLinkGuard(), AuthGuard, organizationPermissionsGuard(canAccessOrgAdmin)], + canActivate: [deepLinkGuard(), authGuard, organizationPermissionsGuard(canAccessOrgAdmin)], children: [ { path: "", diff --git a/apps/web/src/app/oss-routing.module.ts b/apps/web/src/app/oss-routing.module.ts index 034f65366a..65414317cb 100644 --- a/apps/web/src/app/oss-routing.module.ts +++ b/apps/web/src/app/oss-routing.module.ts @@ -2,11 +2,10 @@ import { NgModule } from "@angular/core"; import { Route, RouterModule, Routes } from "@angular/router"; import { - AuthGuard, + authGuard, lockGuard, redirectGuard, tdeDecryptionRequiredGuard, - UnauthGuard, unauthGuardFn, } from "@bitwarden/angular/auth/guards"; import { canAccessFeature } from "@bitwarden/angular/platform/guard/feature-flag.guard"; @@ -105,7 +104,7 @@ const routes: Routes = [ { path: "register", component: TrialInitiationComponent, - canActivate: [UnauthGuard], + canActivate: [unauthGuardFn()], data: { titleId: "createAccount" } satisfies DataProperties, }, { @@ -135,13 +134,13 @@ const routes: Routes = [ { path: "verify-recover-delete-org", component: VerifyRecoverDeleteOrgComponent, - canActivate: [UnauthGuard], + canActivate: [unauthGuardFn()], data: { titleId: "deleteOrganization" }, }, { path: "verify-recover-delete-provider", component: VerifyRecoverDeleteProviderComponent, - canActivate: [UnauthGuard], + canActivate: [unauthGuardFn()], data: { titleId: "deleteAccount" } satisfies DataProperties, }, { @@ -152,13 +151,13 @@ const routes: Routes = [ { path: "update-temp-password", component: UpdateTempPasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { titleId: "updateTempPassword" } satisfies DataProperties, }, { path: "update-password", component: UpdatePasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { titleId: "updatePassword" } satisfies DataProperties, }, { @@ -395,7 +394,7 @@ const routes: Routes = [ { path: "remove-password", component: RemovePasswordComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], data: { pageTitle: "removeMasterPassword", titleId: "removeMasterPassword", @@ -406,7 +405,7 @@ const routes: Routes = [ { path: "", component: UserLayoutComponent, - canActivate: [deepLinkGuard(), AuthGuard], + canActivate: [deepLinkGuard(), authGuard], children: [ { path: "vault", @@ -486,7 +485,7 @@ const routes: Routes = [ }, { path: "tools", - canActivate: [AuthGuard], + canActivate: [authGuard], children: [ { path: "", pathMatch: "full", redirectTo: "generator" }, { diff --git a/apps/web/src/app/tools/reports/reports-routing.module.ts b/apps/web/src/app/tools/reports/reports-routing.module.ts index 0733a57564..cad6586bb8 100644 --- a/apps/web/src/app/tools/reports/reports-routing.module.ts +++ b/apps/web/src/app/tools/reports/reports-routing.module.ts @@ -1,7 +1,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; +import { authGuard } from "@bitwarden/angular/auth/guards"; import { hasPremiumGuard } from "../../core/guards/has-premium.guard"; @@ -18,7 +18,7 @@ const routes: Routes = [ { path: "", component: ReportsLayoutComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], children: [ { path: "", diff --git a/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts b/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts index 413ced840d..8e5860833c 100644 --- a/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts +++ b/bitwarden_license/bit-web/src/app/admin-console/organizations/organizations-routing.module.ts @@ -1,7 +1,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; +import { authGuard } from "@bitwarden/angular/auth/guards"; import { canAccessSettingsTab } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { isEnterpriseOrgGuard } from "@bitwarden/web-vault/app/admin-console/organizations/guards/is-enterprise-org.guard"; import { organizationPermissionsGuard } from "@bitwarden/web-vault/app/admin-console/organizations/guards/org-permissions.guard"; @@ -16,7 +16,7 @@ const routes: Routes = [ { path: "organizations/:organizationId", component: OrganizationLayoutComponent, - canActivate: [AuthGuard, organizationPermissionsGuard()], + canActivate: [authGuard, organizationPermissionsGuard()], children: [ { path: "settings", @@ -61,7 +61,7 @@ const routes: Routes = [ }, { path: "reporting/reports", - canActivate: [AuthGuard, organizationPermissionsGuard((org) => org.canAccessReports)], + canActivate: [authGuard, organizationPermissionsGuard((org) => org.canAccessReports)], children: [ { path: "member-access-report", diff --git a/bitwarden_license/bit-web/src/app/admin-console/providers/providers-routing.module.ts b/bitwarden_license/bit-web/src/app/admin-console/providers/providers-routing.module.ts index f2e890afe2..21fa863d2a 100644 --- a/bitwarden_license/bit-web/src/app/admin-console/providers/providers-routing.module.ts +++ b/bitwarden_license/bit-web/src/app/admin-console/providers/providers-routing.module.ts @@ -1,7 +1,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; +import { authGuard } from "@bitwarden/angular/auth/guards"; import { featureFlaggedRoute } from "@bitwarden/angular/platform/utils/feature-flagged-route"; import { AnonLayoutWrapperComponent } from "@bitwarden/auth/angular"; import { Provider } from "@bitwarden/common/admin-console/models/domain/provider"; @@ -32,12 +32,12 @@ import { SetupComponent } from "./setup/setup.component"; const routes: Routes = [ { path: "", - canActivate: [AuthGuard], + canActivate: [authGuard], component: UserLayoutComponent, children: [ { path: "", - canActivate: [AuthGuard], + canActivate: [authGuard], component: ProvidersComponent, data: { titleId: "providers" }, }, @@ -70,7 +70,7 @@ const routes: Routes = [ }, { path: "", - canActivate: [AuthGuard], + canActivate: [authGuard], children: [ { path: "setup", diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/guards/sm.guard.ts b/bitwarden_license/bit-web/src/app/secrets-manager/guards/sm.guard.ts index d8cda60d9b..2a36bf1cbb 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/guards/sm.guard.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/guards/sm.guard.ts @@ -6,10 +6,7 @@ import { RouterStateSnapshot, } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; -import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; -import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; /** @@ -20,19 +17,13 @@ export const canActivateSM: CanActivateFn = async ( state: RouterStateSnapshot, ) => { const syncService = inject(SyncService); - const authService = inject(AuthService); const orgService = inject(OrganizationService); - const authGuard = inject(AuthGuard); /** Workaround to avoid service initialization race condition. */ if ((await syncService.getLastSync()) == null) { await syncService.fullSync(false); } - if ((await authService.getAuthStatus()) !== AuthenticationStatus.Unlocked) { - return authGuard.canActivate(route, state); - } - const orgs = await orgService.getAll(); const smOrg = orgs.find((o) => o.canAccessSecretsManager); if (smOrg) { diff --git a/bitwarden_license/bit-web/src/app/secrets-manager/sm-routing.module.ts b/bitwarden_license/bit-web/src/app/secrets-manager/sm-routing.module.ts index 00ec259a12..bcc4869c89 100644 --- a/bitwarden_license/bit-web/src/app/secrets-manager/sm-routing.module.ts +++ b/bitwarden_license/bit-web/src/app/secrets-manager/sm-routing.module.ts @@ -1,7 +1,7 @@ import { NgModule } from "@angular/core"; import { RouterModule, Routes } from "@angular/router"; -import { AuthGuard } from "@bitwarden/angular/auth/guards"; +import { authGuard } from "@bitwarden/angular/auth/guards"; import { organizationEnabledGuard } from "./guards/sm-org-enabled.guard"; import { canActivateSM } from "./guards/sm.guard"; @@ -22,14 +22,14 @@ const routes: Routes = [ children: [ { path: "", - canActivate: [canActivateSM], + canActivate: [authGuard, canActivateSM], pathMatch: "full", children: [], }, { path: ":organizationId", component: LayoutComponent, - canActivate: [AuthGuard], + canActivate: [authGuard], children: [ { path: "", diff --git a/libs/angular/src/auth/guards/auth.guard.spec.ts b/libs/angular/src/auth/guards/auth.guard.spec.ts new file mode 100644 index 0000000000..8d024b6b2b --- /dev/null +++ b/libs/angular/src/auth/guards/auth.guard.spec.ts @@ -0,0 +1,177 @@ +import { TestBed } from "@angular/core/testing"; +import { Router } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { MockProxy, mock } from "jest-mock-extended"; +import { BehaviorSubject } from "rxjs"; + +import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec"; +import { AccountInfo, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { MasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { authGuard } from "./auth.guard"; + +describe("AuthGuard", () => { + const setup = ( + authStatus: AuthenticationStatus, + forceSetPasswordReason: ForceSetPasswordReason, + keyConnectorServiceRequiresAccountConversion: boolean = false, + ) => { + const authService: MockProxy = mock(); + authService.getAuthStatus.mockResolvedValue(authStatus); + const messagingService: MockProxy = mock(); + const keyConnectorService: MockProxy = mock(); + keyConnectorService.getConvertAccountRequired.mockResolvedValue( + keyConnectorServiceRequiresAccountConversion, + ); + const accountService: MockProxy = mock(); + const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>(null); + accountService.activeAccount$ = activeAccountSubject; + activeAccountSubject.next( + Object.assign( + { + name: "Test User 1", + email: "test@email.com", + emailVerified: true, + } as AccountInfo, + { id: "test-id" as UserId }, + ), + ); + + const forceSetPasswordReasonSubject = new BehaviorSubject( + forceSetPasswordReason, + ); + const masterPasswordService: MockProxy = + mock(); + masterPasswordService.forceSetPasswordReason$.mockReturnValue(forceSetPasswordReasonSubject); + + const testBed = TestBed.configureTestingModule({ + imports: [ + RouterTestingModule.withRoutes([ + { path: "", component: EmptyComponent }, + { path: "guarded-route", component: EmptyComponent, canActivate: [authGuard] }, + { path: "lock", component: EmptyComponent }, + { path: "set-password", component: EmptyComponent }, + { path: "update-temp-password", component: EmptyComponent }, + { path: "remove-password", component: EmptyComponent }, + ]), + ], + providers: [ + { provide: AuthService, useValue: authService }, + { provide: MessagingService, useValue: messagingService }, + { provide: KeyConnectorService, useValue: keyConnectorService }, + { provide: AccountService, useValue: accountService }, + { provide: MasterPasswordServiceAbstraction, useValue: masterPasswordService }, + ], + }); + + return { + router: testBed.inject(Router), + }; + }; + + it("should be created", () => { + const { router } = setup(AuthenticationStatus.LoggedOut, ForceSetPasswordReason.None); + expect(router).toBeTruthy(); + }); + + it("should return allow access to the guarded route when user is logged in & unlocked", async () => { + const { router } = setup(AuthenticationStatus.Unlocked, ForceSetPasswordReason.None); + + await router.navigate(["guarded-route"]); + expect(router.url).toBe("/guarded-route"); + }); + + it("should redirect to /lock when user is locked", async () => { + const { router } = setup(AuthenticationStatus.Locked, ForceSetPasswordReason.None); + + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/lock"); + }); + + it("should redirect to / when user is logged out", async () => { + const { router } = setup(AuthenticationStatus.LoggedOut, ForceSetPasswordReason.None); + + await router.navigate(["guarded-route"]); + expect(router.url).toBe("/"); + }); + + it("should redirect to /remove-password if keyconnector service requires account conversion", async () => { + const { router } = setup(AuthenticationStatus.Unlocked, ForceSetPasswordReason.None, true); + + await router.navigate(["guarded-route"]); + 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, + ); + + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/set-password"); + }); + + 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("/update-temp-password"); + }); + + it("should redirect to update-temp-password when user has weak password", async () => { + const { router } = setup( + AuthenticationStatus.Unlocked, + ForceSetPasswordReason.WeakMasterPassword, + ); + + await router.navigate(["guarded-route"]); + expect(router.url).toContain("/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, + ); + + await router.navigate(["/set-password"]); + expect(router.url).toContain("/set-password"); + }); + + 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, + ); + + await router.navigate(["/update-temp-password"]); + expect(router.url).toContain("/update-temp-password"); + }); + + 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, + ); + + await router.navigate(["/update-temp-password"]); + expect(router.url).toContain("/update-temp-password"); + }); + + 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(["/remove-password"]); + expect(router.url).toContain("/remove-password"); + }); +}); diff --git a/libs/angular/src/auth/guards/auth.guard.ts b/libs/angular/src/auth/guards/auth.guard.ts index b8e37d0af3..b54f114d3d 100644 --- a/libs/angular/src/auth/guards/auth.guard.ts +++ b/libs/angular/src/auth/guards/auth.guard.ts @@ -1,5 +1,11 @@ -import { Injectable } from "@angular/core"; -import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from "@angular/router"; +import { inject } from "@angular/core"; +import { + ActivatedRouteSnapshot, + CanActivateFn, + Router, + RouterStateSnapshot, + UrlTree, +} from "@angular/router"; import { firstValueFrom } from "rxjs"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -10,59 +16,57 @@ import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authenticatio import { ForceSetPasswordReason } from "@bitwarden/common/auth/models/domain/force-set-password-reason"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; -@Injectable() -export class AuthGuard implements CanActivate { - constructor( - private authService: AuthService, - private router: Router, - private messagingService: MessagingService, - private keyConnectorService: KeyConnectorService, - private accountService: AccountService, - private masterPasswordService: MasterPasswordServiceAbstraction, - ) {} +export const authGuard: CanActivateFn = async ( + route: ActivatedRouteSnapshot, + routerState: RouterStateSnapshot, +): Promise => { + const authService = inject(AuthService); + const router = inject(Router); + const messagingService = inject(MessagingService); + const keyConnectorService = inject(KeyConnectorService); + const accountService = inject(AccountService); + const masterPasswordService = inject(MasterPasswordServiceAbstraction); - async canActivate(route: ActivatedRouteSnapshot, routerState: RouterStateSnapshot) { - const authStatus = await this.authService.getAuthStatus(); + const authStatus = await authService.getAuthStatus(); - if (authStatus === AuthenticationStatus.LoggedOut) { - this.messagingService.send("authBlocked", { url: routerState.url }); - return false; - } - - if (authStatus === AuthenticationStatus.Locked) { - if (routerState != null) { - this.messagingService.send("lockedUrl", { url: routerState.url }); - } - return this.router.createUrlTree(["lock"], { queryParams: { promptBiometric: true } }); - } - - if ( - !routerState.url.includes("remove-password") && - (await this.keyConnectorService.getConvertAccountRequired()) - ) { - return this.router.createUrlTree(["/remove-password"]); - } - - const userId = (await firstValueFrom(this.accountService.activeAccount$)).id; - const forceSetPasswordReason = await firstValueFrom( - this.masterPasswordService.forceSetPasswordReason$(userId), - ); - - if ( - forceSetPasswordReason === - ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission && - !routerState.url.includes("set-password") - ) { - return this.router.createUrlTree(["/set-password"]); - } - - if ( - forceSetPasswordReason !== ForceSetPasswordReason.None && - !routerState.url.includes("update-temp-password") - ) { - return this.router.createUrlTree(["/update-temp-password"]); - } - - return true; + if (authStatus === AuthenticationStatus.LoggedOut) { + messagingService.send("authBlocked", { url: routerState.url }); + return false; } -} + + if (authStatus === AuthenticationStatus.Locked) { + if (routerState != null) { + messagingService.send("lockedUrl", { url: routerState.url }); + } + return router.createUrlTree(["lock"], { queryParams: { promptBiometric: true } }); + } + + if ( + !routerState.url.includes("remove-password") && + (await keyConnectorService.getConvertAccountRequired()) + ) { + return router.createUrlTree(["/remove-password"]); + } + + const userId = (await firstValueFrom(accountService.activeAccount$)).id; + const forceSetPasswordReason = await firstValueFrom( + masterPasswordService.forceSetPasswordReason$(userId), + ); + + if ( + forceSetPasswordReason === + ForceSetPasswordReason.TdeUserWithoutPasswordHasPasswordResetPermission && + !routerState.url.includes("set-password") + ) { + return router.createUrlTree(["/set-password"]); + } + + if ( + forceSetPasswordReason !== ForceSetPasswordReason.None && + !routerState.url.includes("update-temp-password") + ) { + return router.createUrlTree(["/update-temp-password"]); + } + + return true; +}; diff --git a/libs/angular/src/auth/guards/unauth.guard.spec.ts b/libs/angular/src/auth/guards/unauth.guard.spec.ts new file mode 100644 index 0000000000..6d8619f4d4 --- /dev/null +++ b/libs/angular/src/auth/guards/unauth.guard.spec.ts @@ -0,0 +1,89 @@ +import { TestBed } from "@angular/core/testing"; +import { Router } from "@angular/router"; +import { RouterTestingModule } from "@angular/router/testing"; +import { MockProxy, mock } from "jest-mock-extended"; +import { BehaviorSubject } from "rxjs"; + +import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec"; +import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; +import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; + +import { unauthGuardFn } from "./unauth.guard"; + +describe("UnauthGuard", () => { + const setup = (authStatus: AuthenticationStatus) => { + const authService: MockProxy = mock(); + authService.getAuthStatus.mockResolvedValue(authStatus); + const activeAccountStatusObservable = new BehaviorSubject(authStatus); + authService.activeAccountStatus$ = activeAccountStatusObservable; + + const testBed = TestBed.configureTestingModule({ + imports: [ + RouterTestingModule.withRoutes([ + { path: "", component: EmptyComponent }, + { + path: "unauth-guarded-route", + component: EmptyComponent, + canActivate: [unauthGuardFn()], + }, + { path: "vault", component: EmptyComponent }, + { path: "lock", component: EmptyComponent }, + { path: "testhomepage", component: EmptyComponent }, + { path: "testlocked", component: EmptyComponent }, + { + path: "testOverrides", + component: EmptyComponent, + canActivate: [ + unauthGuardFn({ homepage: () => "/testhomepage", locked: "/testlocked" }), + ], + }, + ]), + ], + providers: [{ provide: AuthService, useValue: authService }], + }); + + return { + router: testBed.inject(Router), + }; + }; + + it("should be created", () => { + const { router } = setup(AuthenticationStatus.LoggedOut); + expect(router).toBeTruthy(); + }); + + it("should redirect to /vault for guarded routes when logged in and unlocked", async () => { + const { router } = setup(AuthenticationStatus.Unlocked); + + await router.navigateByUrl("unauth-guarded-route"); + expect(router.url).toBe("/vault"); + }); + + it("should allow access to guarded routes when logged out", async () => { + const { router } = setup(AuthenticationStatus.LoggedOut); + + await router.navigateByUrl("unauth-guarded-route"); + expect(router.url).toBe("/unauth-guarded-route"); + }); + + it("should redirect to /lock for guarded routes when locked", async () => { + const { router } = setup(AuthenticationStatus.Locked); + + await router.navigateByUrl("unauth-guarded-route"); + expect(router.url).toBe("/lock"); + }); + + it("should redirect to /testhomepage for guarded routes when testOverrides are provided and the account is unlocked", async () => { + const { router } = setup(AuthenticationStatus.Unlocked); + + await router.navigateByUrl("testOverrides"); + expect(router.url).toBe("/testhomepage"); + }); + + it("should redirect to /testlocked for guarded routes when testOverrides are provided and the account is locked", async () => { + const { router } = setup(AuthenticationStatus.Locked); + + await router.navigateByUrl("testOverrides"); + expect(router.url).toBe("/testlocked"); + }); +}); diff --git a/libs/angular/src/auth/guards/unauth.guard.ts b/libs/angular/src/auth/guards/unauth.guard.ts index 9e1bca98ca..f96668773e 100644 --- a/libs/angular/src/auth/guards/unauth.guard.ts +++ b/libs/angular/src/auth/guards/unauth.guard.ts @@ -1,36 +1,10 @@ -import { Injectable, inject } from "@angular/core"; -import { CanActivate, CanActivateFn, Router, UrlTree } from "@angular/router"; +import { inject } from "@angular/core"; +import { CanActivateFn, Router, UrlTree } from "@angular/router"; import { Observable, map } from "rxjs"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; -/** - * @deprecated use unauthGuardFn function instead - */ -@Injectable() -export class UnauthGuard implements CanActivate { - protected homepage = "vault"; - constructor( - private authService: AuthService, - private router: Router, - ) {} - - async canActivate() { - const authStatus = await this.authService.getAuthStatus(); - - if (authStatus === AuthenticationStatus.LoggedOut) { - return true; - } - - if (authStatus === AuthenticationStatus.Locked) { - return this.router.createUrlTree(["lock"]); - } - - return this.router.createUrlTree([this.homepage]); - } -} - type UnauthRoutes = { homepage: () => string; locked: string; diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 8ad1e7d20c..a0fec2ad05 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -269,8 +269,6 @@ import { IndividualVaultExportServiceAbstraction, } from "@bitwarden/vault-export-core"; -import { AuthGuard } from "../auth/guards/auth.guard"; -import { UnauthGuard } from "../auth/guards/unauth.guard"; import { FormValidationErrorsService as FormValidationErrorsServiceAbstraction } from "../platform/abstractions/form-validation-errors.service"; import { FormValidationErrorsService } from "../platform/services/form-validation-errors.service"; import { LoggingErrorHandler } from "../platform/services/logging-error-handler"; @@ -306,8 +304,6 @@ import { ModalService } from "./modal.service"; * If you need help please ask for it, do NOT change the type of this array. */ const safeProviders: SafeProvider[] = [ - safeProvider(AuthGuard), - safeProvider(UnauthGuard), safeProvider(ModalService), safeProvider(PasswordRepromptService), safeProvider({ provide: WINDOW, useValue: window }),