From 27b3a4a81d380249255fe06bcdade18d85ece6d1 Mon Sep 17 00:00:00 2001 From: Spruce Weber-Milne Date: Mon, 6 Apr 2026 15:23:51 -0700 Subject: [PATCH 1/2] Let cluster admins admin --- .../src/Shared/RequireAupAgreement.tsx | 6 +++++- .../ClientApp/src/Shared/usePermissions.ts | 4 ++-- .../src/components/Account/Requests.test.tsx | 21 +++++++++++++++++++ Hippo.Web/ClientApp/src/test/mockData.ts | 19 +++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/Hippo.Web/ClientApp/src/Shared/RequireAupAgreement.tsx b/Hippo.Web/ClientApp/src/Shared/RequireAupAgreement.tsx index 230f9838..44b63a36 100644 --- a/Hippo.Web/ClientApp/src/Shared/RequireAupAgreement.tsx +++ b/Hippo.Web/ClientApp/src/Shared/RequireAupAgreement.tsx @@ -22,8 +22,11 @@ export const RequireAupAgreement = (props: Props) => { const hasSystemPermission = context.user.permissions.some( (p) => p.role === "System", ); + const hasClusterAdminPermission = context.user.permissions.some( + (p) => p.role === "ClusterAdmin" && p.cluster === clusterName, + ); const hasFinancialAdminPermission = context.user.permissions.some( - (p) => p.role === "FinancialAdmin", + (p) => p.role === "FinancialAdmin" && p.cluster === clusterName, ); const cluster = context.clusters.find((c) => c.name === clusterName); const account = context.accounts.find((a) => a.cluster === clusterName); @@ -77,6 +80,7 @@ export const RequireAupAgreement = (props: Props) => { if ( hasSystemPermission || + hasClusterAdminPermission || !cluster.acceptableUsePolicyUrl || !cluster.acceptableUsePolicyUpdatedOn ) { diff --git a/Hippo.Web/ClientApp/src/Shared/usePermissions.ts b/Hippo.Web/ClientApp/src/Shared/usePermissions.ts index ba35810d..4177fc69 100644 --- a/Hippo.Web/ClientApp/src/Shared/usePermissions.ts +++ b/Hippo.Web/ClientApp/src/Shared/usePermissions.ts @@ -12,8 +12,8 @@ export const usePermissions = () => { ); const canViewGroup = (groupName: string) => { - if (isSystemAdmin || isClusterAdmin) return true; if (!clusterName) return false; + if (isSystemAdmin || isClusterAdmin) return true; const account = accounts.find((a) => a.cluster === clusterName); if (!account) return false; if (account.adminOfGroups.some((g) => g.name === groupName)) return true; @@ -22,8 +22,8 @@ export const usePermissions = () => { }; const canManageGroup = (groupName: string) => { - if (isSystemAdmin) return true; if (!clusterName) return false; + if (isSystemAdmin || isClusterAdmin) return true; const account = accounts.find((a) => a.cluster === clusterName); if (!account) return false; if (account.adminOfGroups.some((g) => g.name === groupName)) return true; diff --git a/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx b/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx index e905ce2b..76801b31 100644 --- a/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx +++ b/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx @@ -2,6 +2,7 @@ import { MemoryRouter, Route, Routes } from "react-router-dom"; import { fakeAccounts, + fakeClusterAdminAppContextNoAccount, fakeGroupAdminAppContext, fakeGroups, fakeRequests, @@ -106,6 +107,26 @@ it("shows reject button for each pending account", async () => { ); }); +it("lets cluster admins without an account access approvals and approve requests", async () => { + (global as any).Hippo = fakeClusterAdminAppContextNoAccount; + + await act(async () => { + render( + + + , + ); + }); + + expect( + await screen.findByText("There are 2 request(s) awaiting approval"), + ).toBeVisible(); + expect( + await screen.findAllByRole("button", { name: "Approve" }), + ).toHaveLength(2); + expect(screen.queryByText("Acceptable Use Policy")).not.toBeInTheDocument(); +}); + it("displays dialog when reject is clicked", async () => { const user = userEvent.setup(); await act(async () => { diff --git a/Hippo.Web/ClientApp/src/test/mockData.ts b/Hippo.Web/ClientApp/src/test/mockData.ts index d4b81cba..2049ea68 100644 --- a/Hippo.Web/ClientApp/src/test/mockData.ts +++ b/Hippo.Web/ClientApp/src/test/mockData.ts @@ -216,6 +216,25 @@ export const fakeGroupAdminAppContext: AppContextShape = { featureFlags: fakeFeatureFlags, }; +export const fakeClusterAdminAppContextNoAccount: AppContextShape = { + antiForgeryToken: "fakeAntiForgeryToken", + user: { + detail: { + ...fakeUser, + }, + permissions: [ + { + role: "ClusterAdmin", + cluster: "caesfarm", + }, + ], + }, + accounts: [], + clusters: [fakeCluster], + openRequests: fakeRequests, + featureFlags: fakeFeatureFlags, +}; + export const fakeSetContext = vi.fn(); From f9a8fdc9a596991587cab5132bd44af3ce244eea Mon Sep 17 00:00:00 2001 From: Spruce Weber-Milne Date: Mon, 6 Apr 2026 15:57:37 -0700 Subject: [PATCH 2/2] Make tests more explicit --- .../src/components/Account/Requests.test.tsx | 38 ++++++++++++++----- Hippo.Web/ClientApp/src/test/mockData.ts | 20 ++++++++++ 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx b/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx index 76801b31..bf145e34 100644 --- a/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx +++ b/Hippo.Web/ClientApp/src/components/Account/Requests.test.tsx @@ -13,17 +13,17 @@ import { responseMap } from "../../test/testHelpers"; import App from "../../App"; import { Requests } from "./Requests"; import { ModalProvider } from "react-modal-hook"; -import { render, screen } from "@testing-library/react"; +import { render, screen, within } from "@testing-library/react"; import "@testing-library/jest-dom"; import userEvent from "@testing-library/user-event"; import { act } from "react"; -import { RequireAupAgreement } from "../../Shared/RequireAupAgreement"; import AppContext from "../../Shared/AppContext"; globalThis.IS_REACT_ACT_ENVIRONMENT = true; const testCluster = fakeAccounts[0].cluster; const approveUrl = `/${testCluster}/approve`; +const pendingRequestCountText = `There are ${fakeRequests.length} request(s) awaiting approval`; beforeEach(() => { const requestResponse = Promise.resolve({ @@ -75,11 +75,11 @@ it("shows pending approvals count", async () => { ); }); expect( - await screen.findByText("There are 2 request(s) awaiting approval"), + await screen.findByText(pendingRequestCountText), ).toBeVisible(); }); -it("shows approval button for each pending account", async () => { +it("shows approval buttons for each request the group admin can manage", async () => { await act(async () => { render( @@ -93,7 +93,7 @@ it("shows approval button for each pending account", async () => { ).toHaveLength(2); }); -it("shows reject button for each pending account", async () => { +it("shows reject buttons for each request the group admin can manage", async () => { await act(async () => { render( @@ -107,7 +107,7 @@ it("shows reject button for each pending account", async () => { ); }); -it("lets cluster admins without an account access approvals and approve requests", async () => { +it("shows action buttons for CreateGroup requests for cluster admins without an account", async () => { (global as any).Hippo = fakeClusterAdminAppContextNoAccount; await act(async () => { @@ -119,11 +119,29 @@ it("lets cluster admins without an account access approvals and approve requests }); expect( - await screen.findByText("There are 2 request(s) awaiting approval"), + await screen.findByText(pendingRequestCountText), ).toBeVisible(); + const createGroupCell = (await screen.findAllByText("Create Group")).find( + (element) => element.tagName === "TD", + ); + expect(createGroupCell).toBeDefined(); + const createGroupRow = createGroupCell?.closest("tr"); + expect(createGroupRow).not.toBeNull(); + if (!createGroupRow) { + throw new Error("Expected Create Group request row to be present"); + } expect( await screen.findAllByRole("button", { name: "Approve" }), - ).toHaveLength(2); + ).toHaveLength(fakeRequests.length); + expect( + await screen.findAllByRole("button", { name: "Reject" }), + ).toHaveLength(fakeRequests.length); + expect( + within(createGroupRow).getByRole("button", { name: "Approve" }), + ).toBeVisible(); + expect( + within(createGroupRow).getByRole("button", { name: "Reject" }), + ).toBeVisible(); expect(screen.queryByText("Acceptable Use Policy")).not.toBeInTheDocument(); }); @@ -144,7 +162,7 @@ it("displays dialog when reject is clicked", async () => { ); }); expect( - await screen.findByText("There are 2 request(s) awaiting approval"), + await screen.findByText(pendingRequestCountText), ).toBeVisible(); await act(async () => { @@ -170,7 +188,7 @@ it("calls approve and filters list when approve is clicked", async () => { ); }); expect( - await screen.findByText("There are 2 request(s) awaiting approval"), + await screen.findByText(pendingRequestCountText), ).toBeVisible(); await act(async () => { diff --git a/Hippo.Web/ClientApp/src/test/mockData.ts b/Hippo.Web/ClientApp/src/test/mockData.ts index 2049ea68..eaff4c5a 100644 --- a/Hippo.Web/ClientApp/src/test/mockData.ts +++ b/Hippo.Web/ClientApp/src/test/mockData.ts @@ -8,6 +8,7 @@ import { PuppetUserRecord, RequestModel, AccountRequestDataModel, + GroupRequestDataModel, RequestStatus, GroupAccountModel } from "../types"; @@ -153,6 +154,25 @@ export const fakeRequests: RequestModel[] = [ accessTypes: ["SshKey"], } as AccountRequestDataModel, }, + { + id: 3, + requesterEmail: fakeUser.email, + requesterName: fakeUser.name, + action: "CreateGroup", + groupModel: { + id: 3, + name: "group3", + displayName: "Group 3", + admins: [fakeAdminGroupAccount], + data: {} as PuppetGroupRecord, + }, + status: RequestStatus.PendingApproval, + cluster: "caesfarm", + data: { + name: "group3", + displayName: "Group 3", + } as GroupRequestDataModel, + }, ]; const fakeCluster: ClusterModel = {