From b7f70ba39b7c2190a2d039b6a3eb7167c0b3c3ea Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 10 Aug 2022 19:49:33 +0000 Subject: [PATCH 1/5] pairing --- coderd/rbac/builtin.go | 1 + coderd/rbac/object.go | 6 + site/src/AppRouter.tsx | 230 +++++++++--------- site/src/components/NavbarView/NavbarView.tsx | 16 +- site/src/xServices/auth/authXService.ts | 22 +- 5 files changed, 160 insertions(+), 115 deletions(-) diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index cd51d88361636..61e5a8d544712 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -88,6 +88,7 @@ var ( // Should be able to read all template details, even in orgs they // are not in. ResourceTemplate: {ActionRead}, + ResourceAuditLog: {ActionRead}, }), } }, diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index 6106dd8079015..88f342d286e41 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -22,6 +22,12 @@ var ( Type: "workspace", } + // ResourceAuditLog + // read = access audit log + ResourceAuditLog = Object{ + Type: "audit_log", + } + // ResourceTemplate CRUD. Org owner only. // create/delete = Make or delete a new template // update = Update the template, make new template versions diff --git a/site/src/AppRouter.tsx b/site/src/AppRouter.tsx index b7b0d0e6b9cf0..25bd726d11341 100644 --- a/site/src/AppRouter.tsx +++ b/site/src/AppRouter.tsx @@ -1,5 +1,7 @@ -import { FC, lazy, Suspense } from "react" +import { useActor } from "@xstate/react" +import { FC, lazy, Suspense, useContext } from "react" import { Navigate, Route, Routes } from "react-router-dom" +import { XServiceContext } from "xServices/StateContext" import { AuthAndFrame } from "./components/AuthAndFrame/AuthAndFrame" import { RequireAuth } from "./components/RequireAuth/RequireAuth" import { SettingsLayout } from "./components/SettingsLayout/SettingsLayout" @@ -27,167 +29,173 @@ const WorkspacesPage = lazy(() => import("./pages/WorkspacesPage/WorkspacesPage" const CreateWorkspacePage = lazy(() => import("./pages/CreateWorkspacePage/CreateWorkspacePage")) const AuditPage = lazy(() => import("./pages/AuditPage/AuditPage")) -export const AppRouter: FC = () => ( - }> - - - - - } - /> +export const AppRouter: FC = () => { + const xServices = useContext(XServiceContext) + const [authState, authSend] = useActor(xServices.authXService) + const { me } = authState.context - } /> - } /> - - - - } - /> - - + return ( + }> + - - + + + } /> - - + } /> + } /> - - + + + } /> - + - + } /> - - - - } - /> - - - - - - - } - /> - - - - } - /> - - {/* REMARK: Route under construction - Eventually, we should gate this page - with permissions and licensing */} - - - ) : ( + + - + - ) - } - > - + } + /> - }> - } /> - } /> - } /> - + + + + + } + /> + + + + } + /> + + - - + - + } /> - + } /> + + {/* REMARK: Route under construction + Eventually, we should gate this page + with permissions and licensing */} + - - + process.env.NODE_ENV === "production" ? ( + + ) : ( + + + + ) } - /> + > + + + }> + } /> + } /> + } /> + - + + - + } /> - + + + + } + /> - - - - } - /> + + + + } + /> + + + + + + } + /> + + + + + + } + /> + - - {/* Using path="*"" means "match anything", so this route + {/* Using path="*"" means "match anything", so this route acts like a catch-all for URLs that we don't have explicit routes for. */} - } /> - - -) + } /> + + + ) +} diff --git a/site/src/components/NavbarView/NavbarView.tsx b/site/src/components/NavbarView/NavbarView.tsx index 98dc8dd985e44..ae63e425c9a87 100644 --- a/site/src/components/NavbarView/NavbarView.tsx +++ b/site/src/components/NavbarView/NavbarView.tsx @@ -24,9 +24,19 @@ export const Language = { audit: "Audit", } -const NavItems: React.FC<{ className?: string; linkClassName?: string }> = ({ className }) => { +enum AuditorRoles { + admin, + auditor, +} + +const NavItems: React.FC<{ className?: string; roles?: TypesGen.Role[] }> = ({ + className, + roles = [], +}) => { const styles = useStyles() const location = useLocation() + const userRoles = roles.map((role) => role.name) + const hasAuditPermissions = Object.keys(AuditorRoles).some((role) => userRoles.includes(role)) return ( @@ -49,7 +59,7 @@ const NavItems: React.FC<{ className?: string; linkClassName?: string }> = ({ cl {/* REMARK: the below link is under-construction */} - {process.env.NODE_ENV !== "production" && ( + {process.env.NODE_ENV !== "production" && hasAuditPermissions && ( {Language.audit} @@ -89,7 +99,7 @@ export const NavbarView: React.FC = ({ user, onSignOut }) => { - +
{user && } diff --git a/site/src/xServices/auth/authXService.ts b/site/src/xServices/auth/authXService.ts index cf0a9432ea33a..77a82b9143f7d 100644 --- a/site/src/xServices/auth/authXService.ts +++ b/site/src/xServices/auth/authXService.ts @@ -9,11 +9,17 @@ export const Language = { successRegenerateSSHKey: "SSH Key regenerated successfully", } +enum RolesWithAudit { + admin, + auditor, +} + export const checks = { readAllUsers: "readAllUsers", updateUsers: "updateUsers", createUser: "createUser", createTemplates: "createTemplates", + viewAuditLog: "viewAuditLog", } as const export const permissionsToCheck = { @@ -41,6 +47,12 @@ export const permissionsToCheck = { }, action: "write", }, + [checks.viewAuditLog]: { + object: { + resource_type: "audit_log", + }, + action: "read", + } } as const type Permissions = Record @@ -461,7 +473,15 @@ export const authMachine = assignPermissions: assign({ // Setting event.data as Permissions to be more stricted. So we know // what permissions we asked for. - permissions: (_, event) => event.data as Permissions, + permissions: (context, event) => { + const userRoles = context?.me?.roles.map((role) => role.name) ?? [] + const viewAuditLog = Object.keys(RolesWithAudit).some((role) => userRoles.includes(role)) + const assignedPermissions = { + ...event.data, + viewAuditLog + } + return assignedPermissions as Permissions + }, }), assignGetPermissionsError: assign({ checkPermissionsError: (_, event) => event.data, From 8c9eb0e39f6f42136fca8711631c1fe9094d5c19 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 10 Aug 2022 20:07:13 +0000 Subject: [PATCH 2/5] restricting audit route resolvees #3460 --- site/src/AppRouter.tsx | 8 ++++---- site/src/components/Navbar/Navbar.tsx | 10 ++++++++-- site/src/components/NavbarView/NavbarView.tsx | 20 +++++++------------ site/src/xServices/auth/authXService.ts | 17 ++-------------- 4 files changed, 21 insertions(+), 34 deletions(-) diff --git a/site/src/AppRouter.tsx b/site/src/AppRouter.tsx index 25bd726d11341..9135f7c472896 100644 --- a/site/src/AppRouter.tsx +++ b/site/src/AppRouter.tsx @@ -32,7 +32,7 @@ const AuditPage = lazy(() => import("./pages/AuditPage/AuditPage")) export const AppRouter: FC = () => { const xServices = useContext(XServiceContext) const [authState, authSend] = useActor(xServices.authXService) - const { me } = authState.context + const { permissions } = authState.context return ( }> @@ -118,13 +118,13 @@ export const AppRouter: FC = () => { {/* REMARK: Route under construction - Eventually, we should gate this page - with permissions and licensing */} + Eventually, we should gate this page + with permissions and licensing */} ) : ( diff --git a/site/src/components/Navbar/Navbar.tsx b/site/src/components/Navbar/Navbar.tsx index 0ac64ef7d1269..cbfdfd949dd19 100644 --- a/site/src/components/Navbar/Navbar.tsx +++ b/site/src/components/Navbar/Navbar.tsx @@ -6,8 +6,14 @@ import { NavbarView } from "../NavbarView/NavbarView" export const Navbar: React.FC = () => { const xServices = useContext(XServiceContext) const [authState, authSend] = useActor(xServices.authXService) - const { me } = authState.context + const { me, permissions } = authState.context const onSignOut = () => authSend("SIGN_OUT") - return + return ( + + ) } diff --git a/site/src/components/NavbarView/NavbarView.tsx b/site/src/components/NavbarView/NavbarView.tsx index ae63e425c9a87..0e26ea9a21b94 100644 --- a/site/src/components/NavbarView/NavbarView.tsx +++ b/site/src/components/NavbarView/NavbarView.tsx @@ -15,6 +15,7 @@ import { UserDropdown } from "../UserDropdown/UsersDropdown" export interface NavbarViewProps { user?: TypesGen.User onSignOut: () => void + canViewAuditLog: boolean } export const Language = { @@ -24,19 +25,12 @@ export const Language = { audit: "Audit", } -enum AuditorRoles { - admin, - auditor, -} - -const NavItems: React.FC<{ className?: string; roles?: TypesGen.Role[] }> = ({ +const NavItems: React.FC<{ className?: string; canViewAuditLog: boolean }> = ({ className, - roles = [], + canViewAuditLog, }) => { const styles = useStyles() const location = useLocation() - const userRoles = roles.map((role) => role.name) - const hasAuditPermissions = Object.keys(AuditorRoles).some((role) => userRoles.includes(role)) return ( @@ -59,7 +53,7 @@ const NavItems: React.FC<{ className?: string; roles?: TypesGen.Role[] }> = ({ {/* REMARK: the below link is under-construction */} - {process.env.NODE_ENV !== "production" && hasAuditPermissions && ( + {process.env.NODE_ENV !== "production" && canViewAuditLog && ( {Language.audit} @@ -70,7 +64,7 @@ const NavItems: React.FC<{ className?: string; roles?: TypesGen.Role[] }> = ({ ) } -export const NavbarView: React.FC = ({ user, onSignOut }) => { +export const NavbarView: React.FC = ({ user, onSignOut, canViewAuditLog }) => { const styles = useStyles() const [isDrawerOpen, setIsDrawerOpen] = useState(false) @@ -91,7 +85,7 @@ export const NavbarView: React.FC = ({ user, onSignOut }) => {
- +
@@ -99,7 +93,7 @@ export const NavbarView: React.FC = ({ user, onSignOut }) => { - +
{user && } diff --git a/site/src/xServices/auth/authXService.ts b/site/src/xServices/auth/authXService.ts index 77a82b9143f7d..f07660885b275 100644 --- a/site/src/xServices/auth/authXService.ts +++ b/site/src/xServices/auth/authXService.ts @@ -9,11 +9,6 @@ export const Language = { successRegenerateSSHKey: "SSH Key regenerated successfully", } -enum RolesWithAudit { - admin, - auditor, -} - export const checks = { readAllUsers: "readAllUsers", updateUsers: "updateUsers", @@ -52,7 +47,7 @@ export const permissionsToCheck = { resource_type: "audit_log", }, action: "read", - } + }, } as const type Permissions = Record @@ -473,15 +468,7 @@ export const authMachine = assignPermissions: assign({ // Setting event.data as Permissions to be more stricted. So we know // what permissions we asked for. - permissions: (context, event) => { - const userRoles = context?.me?.roles.map((role) => role.name) ?? [] - const viewAuditLog = Object.keys(RolesWithAudit).some((role) => userRoles.includes(role)) - const assignedPermissions = { - ...event.data, - viewAuditLog - } - return assignedPermissions as Permissions - }, + permissions: (_, event) => event.data as Permissions, }), assignGetPermissionsError: assign({ checkPermissionsError: (_, event) => event.data, From 49a53be8d3c713290e85dbb8f433e63f55238b9a Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 10 Aug 2022 20:15:25 +0000 Subject: [PATCH 3/5] updated tests --- .../components/NavbarView/NavbarView.test.tsx | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/site/src/components/NavbarView/NavbarView.test.tsx b/site/src/components/NavbarView/NavbarView.test.tsx index c0755dbda196e..a3c3c8861bfdd 100644 --- a/site/src/components/NavbarView/NavbarView.test.tsx +++ b/site/src/components/NavbarView/NavbarView.test.tsx @@ -1,5 +1,5 @@ import { screen } from "@testing-library/react" -import { MockUser } from "../../testHelpers/entities" +import { MockUser, MockUser2 } from "../../testHelpers/entities" import { render } from "../../testHelpers/renderHelpers" import { Language as navLanguage, NavbarView } from "./NavbarView" @@ -22,26 +22,26 @@ describe("NavbarView", () => { it("renders content", async () => { // When - render() + render() // Then await screen.findAllByText("Coder", { exact: false }) }) it("workspaces nav link has the correct href", async () => { - render() + render() const workspacesLink = await screen.findByText(navLanguage.workspaces) expect((workspacesLink as HTMLAnchorElement).href).toContain("/workspaces") }) it("templates nav link has the correct href", async () => { - render() + render() const templatesLink = await screen.findByText(navLanguage.templates) expect((templatesLink as HTMLAnchorElement).href).toContain("/templates") }) it("users nav link has the correct href", async () => { - render() + render() const userLink = await screen.findByText(navLanguage.users) expect((userLink as HTMLAnchorElement).href).toContain("/users") }) @@ -54,7 +54,7 @@ describe("NavbarView", () => { } // When - render() + render() // Then // There should be a 'B' avatar! @@ -63,7 +63,7 @@ describe("NavbarView", () => { }) it("audit nav link has the correct href", async () => { - render() + render() const auditLink = await screen.findByText(navLanguage.audit) expect((auditLink as HTMLAnchorElement).href).toContain("/audit") }) @@ -74,7 +74,13 @@ describe("NavbarView", () => { NODE_ENV: "production", } - render() + render() + const auditLink = screen.queryByText(navLanguage.audit) + expect(auditLink).not.toBeInTheDocument() + }) + + it("audit nav link is hidden for members", async () => { + render() const auditLink = screen.queryByText(navLanguage.audit) expect(auditLink).not.toBeInTheDocument() }) From c5afa048d51b38d46bb4cf3e2251d8ced706fbf6 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 10 Aug 2022 20:34:42 +0000 Subject: [PATCH 4/5] fixing lint --- site/src/AppRouter.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/AppRouter.tsx b/site/src/AppRouter.tsx index 9135f7c472896..0497b15007f2e 100644 --- a/site/src/AppRouter.tsx +++ b/site/src/AppRouter.tsx @@ -31,7 +31,7 @@ const AuditPage = lazy(() => import("./pages/AuditPage/AuditPage")) export const AppRouter: FC = () => { const xServices = useContext(XServiceContext) - const [authState, authSend] = useActor(xServices.authXService) + const [authState] = useActor(xServices.authXService) const { permissions } = authState.context return ( From 62f8fc074a31dfb7475ef55be24cba864ca413e1 Mon Sep 17 00:00:00 2001 From: Kira Pilot Date: Wed, 10 Aug 2022 21:59:37 +0000 Subject: [PATCH 5/5] useSelector instead of useActor --- site/src/AppRouter.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/site/src/AppRouter.tsx b/site/src/AppRouter.tsx index 0497b15007f2e..332f283505989 100644 --- a/site/src/AppRouter.tsx +++ b/site/src/AppRouter.tsx @@ -1,6 +1,7 @@ -import { useActor } from "@xstate/react" +import { useSelector } from "@xstate/react" import { FC, lazy, Suspense, useContext } from "react" import { Navigate, Route, Routes } from "react-router-dom" +import { selectPermissions } from "xServices/auth/authSelectors" import { XServiceContext } from "xServices/StateContext" import { AuthAndFrame } from "./components/AuthAndFrame/AuthAndFrame" import { RequireAuth } from "./components/RequireAuth/RequireAuth" @@ -31,8 +32,7 @@ const AuditPage = lazy(() => import("./pages/AuditPage/AuditPage")) export const AppRouter: FC = () => { const xServices = useContext(XServiceContext) - const [authState] = useActor(xServices.authXService) - const { permissions } = authState.context + const permissions = useSelector(xServices.authXService, selectPermissions) return ( }>