From 9551691fde0e7ebd2f19b2260184d680de698a10 Mon Sep 17 00:00:00 2001 From: Addison Beck Date: Mon, 1 Jul 2024 11:54:00 -0400 Subject: [PATCH] Restructure the `org-redirect` guard to be Angular 17+ compliant (#9552) * Document the `org-redirect` guard in code * Make assertions about the way the `org-redirect` guard should behave * Restructure the `org-redirect` guard to be Angular 17+ compliant * Convert data parameter to function parameter * Convert a data parameter to a function parameter that was missed * Pass redirect function to default organization route --- .../guards/org-redirect.guard.spec.ts | 123 ++++++++++++++++++ .../guards/org-redirect.guard.ts | 48 ++++--- .../organization-routing.module.ts | 7 +- .../organization-reporting-routing.module.ts | 7 +- .../organization-settings-routing.module.ts | 7 +- 5 files changed, 158 insertions(+), 34 deletions(-) create mode 100644 apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.spec.ts diff --git a/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.spec.ts b/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.spec.ts new file mode 100644 index 0000000000..576a9dde19 --- /dev/null +++ b/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.spec.ts @@ -0,0 +1,123 @@ +import { Component } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; +import { provideRouter } from "@angular/router"; +import { RouterTestingHarness } from "@angular/router/testing"; +import { MockProxy, mock } from "jest-mock-extended"; + +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; + +import { organizationRedirectGuard } from "./org-redirect.guard"; + +@Component({ + template: "

This is the home screen!

", +}) +export class HomescreenComponent {} + +@Component({ + template: "

This is the admin console!

", +}) +export class AdminConsoleComponent {} + +@Component({ + template: "

This is a subroute of the admin console!

", +}) +export class AdminConsoleSubrouteComponent {} + +const orgFactory = (props: Partial = {}) => + Object.assign( + new Organization(), + { + id: "myOrgId", + enabled: true, + type: OrganizationUserType.Admin, + }, + props, + ); + +describe("Organization Redirect Guard", () => { + let organizationService: MockProxy; + let routerHarness: RouterTestingHarness; + + beforeEach(async () => { + organizationService = mock(); + + TestBed.configureTestingModule({ + providers: [ + { provide: OrganizationService, useValue: organizationService }, + provideRouter([ + { + path: "", + component: HomescreenComponent, + }, + { + path: "organizations/:organizationId", + component: AdminConsoleComponent, + }, + { + path: "organizations/:organizationId/stringCallback/success", + component: AdminConsoleSubrouteComponent, + }, + { + path: "organizations/:organizationId/arrayCallback/exponential/success", + component: AdminConsoleSubrouteComponent, + }, + { + path: "organizations/:organizationId/noCallback", + component: AdminConsoleComponent, + canActivate: [organizationRedirectGuard()], + }, + { + path: "organizations/:organizationId/stringCallback", + component: AdminConsoleComponent, + canActivate: [organizationRedirectGuard(() => "success")], + }, + { + path: "organizations/:organizationId/arrayCallback", + component: AdminConsoleComponent, + canActivate: [organizationRedirectGuard(() => ["exponential", "success"])], + }, + ]), + ], + }); + + routerHarness = await RouterTestingHarness.create(); + }); + + it("redirects to `/` if the organization id provided is not found", async () => { + const org = orgFactory(); + organizationService.get.calledWith(org.id).mockResolvedValue(null); + await routerHarness.navigateByUrl(`organizations/${org.id}/noCallback`); + expect(routerHarness.routeNativeElement?.querySelector("h1")?.textContent?.trim() ?? "").toBe( + "This is the home screen!", + ); + }); + + it("redirects to `/organizations/{id}` if no custom redirect is supplied but the user can access the admin onsole", async () => { + const org = orgFactory(); + organizationService.get.calledWith(org.id).mockResolvedValue(org); + await routerHarness.navigateByUrl(`organizations/${org.id}/noCallback`); + expect(routerHarness.routeNativeElement?.querySelector("h1")?.textContent?.trim() ?? "").toBe( + "This is the admin console!", + ); + }); + + it("redirects properly when the redirect callback returns a single string", async () => { + const org = orgFactory(); + organizationService.get.calledWith(org.id).mockResolvedValue(org); + await routerHarness.navigateByUrl(`organizations/${org.id}/stringCallback`); + expect(routerHarness.routeNativeElement?.querySelector("h1")?.textContent?.trim() ?? "").toBe( + "This is a subroute of the admin console!", + ); + }); + + it("redirects properly when the redirect callback returns an array of strings", async () => { + const org = orgFactory(); + organizationService.get.calledWith(org.id).mockResolvedValue(org); + await routerHarness.navigateByUrl(`organizations/${org.id}/arrayCallback`); + expect(routerHarness.routeNativeElement?.querySelector("h1")?.textContent?.trim() ?? "").toBe( + "This is a subroute of the admin console!", + ); + }); +}); diff --git a/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.ts b/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.ts index bbfb51ed94..1ab73195e4 100644 --- a/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.ts +++ b/apps/web/src/app/admin-console/organizations/guards/org-redirect.guard.ts @@ -1,35 +1,45 @@ -import { Injectable } from "@angular/core"; -import { ActivatedRouteSnapshot, CanActivate, Router, RouterStateSnapshot } from "@angular/router"; +import { inject } from "@angular/core"; +import { + ActivatedRouteSnapshot, + CanActivateFn, + Router, + RouterStateSnapshot, +} from "@angular/router"; import { canAccessOrgAdmin, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -@Injectable({ - providedIn: "root", -}) -export class OrganizationRedirectGuard implements CanActivate { - constructor( - private router: Router, - private organizationService: OrganizationService, - ) {} +/** + * + * `CanActivateFn` that returns a URL Tree redirecting to a caller provided + * sub route of `/organizations/{id}/`. If no sub route is provided the URL + * tree returned will redirect to `/organizations/{id}` if possible, or `/` if + * the user does not have permission to access `organizations/{id}`. + */ +export function organizationRedirectGuard( + customRedirect?: (org: Organization) => string | string[], +): CanActivateFn { + return async (route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => { + const router = inject(Router); + const organizationService = inject(OrganizationService); - async canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot) { - const org = await this.organizationService.get(route.params.organizationId); + const org = await organizationService.get(route.params.organizationId); - const customRedirect = route.data?.autoRedirectCallback; - if (customRedirect) { + if (customRedirect != null) { let redirectPath = customRedirect(org); if (typeof redirectPath === "string") { redirectPath = [redirectPath]; } - return this.router.createUrlTree([state.url, ...redirectPath]); + return router.createUrlTree([state.url, ...redirectPath]); } - if (canAccessOrgAdmin(org)) { - return this.router.createUrlTree(["/organizations", org.id]); + if (org != null && canAccessOrgAdmin(org)) { + return router.createUrlTree(["/organizations", org.id]); } - return this.router.createUrlTree(["/"]); - } + + return router.createUrlTree(["/"]); + }; } 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 7abee6b0d0..518a3db5b1 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 @@ -13,7 +13,7 @@ import { import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { OrganizationPermissionsGuard } from "../../admin-console/organizations/guards/org-permissions.guard"; -import { OrganizationRedirectGuard } from "../../admin-console/organizations/guards/org-redirect.guard"; +import { organizationRedirectGuard } from "../../admin-console/organizations/guards/org-redirect.guard"; import { OrganizationLayoutComponent } from "../../admin-console/organizations/layouts/organization-layout.component"; import { GroupsComponent } from "../../admin-console/organizations/manage/groups.component"; import { deepLinkGuard } from "../../auth/guards/deep-link.guard"; @@ -31,10 +31,7 @@ const routes: Routes = [ { path: "", pathMatch: "full", - canActivate: [OrganizationRedirectGuard], - data: { - autoRedirectCallback: getOrganizationRoute, - }, + canActivate: [organizationRedirectGuard(getOrganizationRoute)], children: [], // This is required to make the auto redirect work, }, }, { diff --git a/apps/web/src/app/admin-console/organizations/reporting/organization-reporting-routing.module.ts b/apps/web/src/app/admin-console/organizations/reporting/organization-reporting-routing.module.ts index 1ab386298c..fafce75e73 100644 --- a/apps/web/src/app/admin-console/organizations/reporting/organization-reporting-routing.module.ts +++ b/apps/web/src/app/admin-console/organizations/reporting/organization-reporting-routing.module.ts @@ -11,7 +11,7 @@ import { UnsecuredWebsitesReportComponent } from "../../../admin-console/organiz import { WeakPasswordsReportComponent } from "../../../admin-console/organizations/tools/weak-passwords-report.component"; import { IsPaidOrgGuard } from "../guards/is-paid-org.guard"; import { OrganizationPermissionsGuard } from "../guards/org-permissions.guard"; -import { OrganizationRedirectGuard } from "../guards/org-redirect.guard"; +import { organizationRedirectGuard } from "../guards/org-redirect.guard"; import { EventsComponent } from "../manage/events.component"; import { ReportsHomeComponent } from "./reports-home.component"; @@ -25,10 +25,7 @@ const routes: Routes = [ { path: "", pathMatch: "full", - canActivate: [OrganizationRedirectGuard], - data: { - autoRedirectCallback: getReportRoute, - }, + canActivate: [organizationRedirectGuard(getReportRoute)], children: [], // This is required to make the auto redirect work, }, { diff --git a/apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts b/apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts index cc65bef8c7..31e2140497 100644 --- a/apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts +++ b/apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts @@ -5,7 +5,7 @@ import { canAccessSettingsTab } from "@bitwarden/common/admin-console/abstractio import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { OrganizationPermissionsGuard } from "../../organizations/guards/org-permissions.guard"; -import { OrganizationRedirectGuard } from "../../organizations/guards/org-redirect.guard"; +import { organizationRedirectGuard } from "../../organizations/guards/org-redirect.guard"; import { PoliciesComponent } from "../../organizations/policies"; import { AccountComponent } from "./account.component"; @@ -20,10 +20,7 @@ const routes: Routes = [ { path: "", pathMatch: "full", - canActivate: [OrganizationRedirectGuard], - data: { - autoRedirectCallback: getSettingsRoute, - }, + canActivate: [organizationRedirectGuard(getSettingsRoute)], children: [], // This is required to make the auto redirect work, }, { path: "account", component: AccountComponent, data: { titleId: "organizationInfo" } },