From d77efc22bf4e295f3537bb7c38b0676c16918490 Mon Sep 17 00:00:00 2001 From: Bruno Date: Wed, 18 May 2022 19:21:37 +0000 Subject: [PATCH 1/4] fix: User permissions on UI --- site/src/components/Navbar/Navbar.tsx | 2 +- site/src/components/UsersTable/UsersTable.tsx | 68 ++++++++++--------- site/src/pages/UsersPage/UsersPage.tsx | 40 +++++------ site/src/pages/UsersPage/UsersPageView.tsx | 3 + site/src/xServices/auth/authXService.ts | 7 ++ 5 files changed, 65 insertions(+), 55 deletions(-) diff --git a/site/src/components/Navbar/Navbar.tsx b/site/src/components/Navbar/Navbar.tsx index 64ca212dd9a78..90a3c4676794a 100644 --- a/site/src/components/Navbar/Navbar.tsx +++ b/site/src/components/Navbar/Navbar.tsx @@ -11,7 +11,7 @@ export const Navbar: React.FC = () => { const permissions = useSelector(xServices.authXService, selectPermissions) // When we have more options in the admin dropdown we may want to check this // for more permissions - const displayAdminDropdown = !!permissions?.readAllUsers + const displayAdminDropdown = !!permissions?.updateUsers const onSignOut = () => authSend("SIGN_OUT") return diff --git a/site/src/components/UsersTable/UsersTable.tsx b/site/src/components/UsersTable/UsersTable.tsx index c961c4b2f9aa8..648f589a32449 100644 --- a/site/src/components/UsersTable/UsersTable.tsx +++ b/site/src/components/UsersTable/UsersTable.tsx @@ -8,10 +8,8 @@ import React from "react" import * as TypesGen from "../../api/typesGenerated" import { EmptyState } from "../EmptyState/EmptyState" import { RoleSelect } from "../RoleSelect/RoleSelect" -import { TableHeaderRow } from "../TableHeaders/TableHeaders" import { TableLoader } from "../TableLoader/TableLoader" import { TableRowMenu } from "../TableRowMenu/TableRowMenu" -import { TableTitle } from "../TableTitle/TableTitle" import { UserCell } from "../UserCell/UserCell" export const Language = { @@ -28,6 +26,7 @@ export interface UsersTableProps { users?: TypesGen.User[] roles?: TypesGen.Role[] isUpdatingUserRoles?: boolean + canEditUsers?: boolean onSuspendUser: (user: TypesGen.User) => void onResetUserPassword: (user: TypesGen.User) => void onUpdateUserRoles: (user: TypesGen.User, roles: TypesGen.Role["name"][]) => void @@ -40,52 +39,57 @@ export const UsersTable: React.FC = ({ onResetUserPassword, onUpdateUserRoles, isUpdatingUserRoles, + canEditUsers, }) => { - const isLoading = !users || !roles + const isLoading = !users || (canEditUsers && roles) return ( - - - {Language.usernameLabel} - {Language.rolesLabel} + + {Language.usernameLabel} + {Language.rolesLabel} {/* 1% is a trick to make the table cell width fit the content */} - - + {canEditUsers && } + {isLoading && } - {users && - roles && + {!isLoading && users.map((u) => ( {" "} - onUpdateUserRoles(u, roles)} - /> - - - + {canEditUsers ? ( + onUpdateUserRoles(u, roles)} + /> + ) : ( + <>{u.roles.map((r) => r.display_name).join(", ")} + )} + {canEditUsers && ( + + + + )} ))} diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 579924832f9f6..690c2e9631c1f 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -1,8 +1,9 @@ -import { useActor } from "@xstate/react" +import { useActor, useSelector } from "@xstate/react" import React, { useContext, useEffect } from "react" import { useNavigate } from "react-router" import { ConfirmDialog } from "../../components/ConfirmDialog/ConfirmDialog" import { ResetPasswordDialog } from "../../components/ResetPasswordDialog/ResetPasswordDialog" +import { selectPermissions } from "../../xServices/auth/authSelectors" import { XServiceContext } from "../../xServices/StateContext" import { UsersPageView } from "./UsersPageView" @@ -12,39 +13,33 @@ export const Language = { suspendDialogMessagePrefix: "Do you want to suspend the user", } -const useRoles = () => { - const xServices = useContext(XServiceContext) - const [rolesState, rolesSend] = useActor(xServices.siteRolesXService) - const { roles } = rolesState.context - - /** - * Fetch roles on component mount - */ - useEffect(() => { - rolesSend({ - type: "GET_ROLES", - }) - }, [rolesSend]) - - return roles -} - export const UsersPage: React.FC = () => { const xServices = useContext(XServiceContext) const [usersState, usersSend] = useActor(xServices.usersXService) + const [rolesState, rolesSend] = useActor(xServices.siteRolesXService) const { users, getUsersError, userIdToSuspend, userIdToResetPassword, newUserPassword } = usersState.context const navigate = useNavigate() const userToBeSuspended = users?.find((u) => u.id === userIdToSuspend) const userToResetPassword = users?.find((u) => u.id === userIdToResetPassword) - const roles = useRoles() + const permissions = useSelector(xServices.authXService, selectPermissions) + const canEditUsers = permissions && permissions.updateUsers + const { roles } = rolesState.context - /** - * Fetch users on component mount - */ + // Fetch users on component mount useEffect(() => { usersSend("GET_USERS") }, [usersSend]) + // Fetch roles on component mount + useEffect(() => { + // Only fetch the roles if the user has permission for it + if (canEditUsers) { + rolesSend({ + type: "GET_ROLES", + }) + } + }, [canEditUsers, rolesSend]) + return ( <> { }} error={getUsersError} isUpdatingUserRoles={usersState.matches("updatingUserRoles")} + canEditUsers={canEditUsers} /> void onSuspendUser: (user: TypesGen.User) => void onResetUserPassword: (user: TypesGen.User) => void @@ -31,6 +32,7 @@ export const UsersPageView: React.FC = ({ onUpdateUserRoles, error, isUpdatingUserRoles, + canEditUsers, }) => { return ( @@ -46,6 +48,7 @@ export const UsersPageView: React.FC = ({ onResetUserPassword={onResetUserPassword} onUpdateUserRoles={onUpdateUserRoles} isUpdatingUserRoles={isUpdatingUserRoles} + canEditUsers={canEditUsers} /> )} diff --git a/site/src/xServices/auth/authXService.ts b/site/src/xServices/auth/authXService.ts index 6ca72d406ba58..deba485615995 100644 --- a/site/src/xServices/auth/authXService.ts +++ b/site/src/xServices/auth/authXService.ts @@ -11,6 +11,7 @@ export const Language = { export const checks = { readAllUsers: "readAllUsers", + updateUsers: "updateUsers", createTemplates: "createTemplates", } as const @@ -21,6 +22,12 @@ export const permissionsToCheck = { }, action: "read", }, + [checks.updateUsers]: { + object: { + resource_type: "user", + }, + action: "update", + }, [checks.createTemplates]: { object: { resource_type: "template", From 0d5b094eb3636c8756900af0660ee1e13964fbca Mon Sep 17 00:00:00 2001 From: Bruno Date: Wed, 18 May 2022 19:22:38 +0000 Subject: [PATCH 2/4] Fix loading --- site/src/components/UsersTable/UsersTable.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/UsersTable/UsersTable.tsx b/site/src/components/UsersTable/UsersTable.tsx index 648f589a32449..1bcda6f2d6f3a 100644 --- a/site/src/components/UsersTable/UsersTable.tsx +++ b/site/src/components/UsersTable/UsersTable.tsx @@ -41,7 +41,7 @@ export const UsersTable: React.FC = ({ isUpdatingUserRoles, canEditUsers, }) => { - const isLoading = !users || (canEditUsers && roles) + const isLoading = !users || (canEditUsers && !roles) return (
From f91069fd96afb1b1ab54923241b0659083b09167 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 19 May 2022 14:08:16 +0000 Subject: [PATCH 3/4] Fix tests and loading state --- site/src/components/Navbar/Navbar.test.tsx | 8 ++++---- site/src/components/UsersTable/UsersTable.tsx | 5 +++-- site/src/pages/UsersPage/UsersPage.tsx | 6 ++++++ site/src/pages/UsersPage/UsersPageView.tsx | 3 +++ 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/site/src/components/Navbar/Navbar.test.tsx b/site/src/components/Navbar/Navbar.test.tsx index 25c610de22d5b..3a014d415327a 100644 --- a/site/src/components/Navbar/Navbar.test.tsx +++ b/site/src/components/Navbar/Navbar.test.tsx @@ -11,10 +11,10 @@ beforeEach(() => { }) describe("Navbar", () => { - describe("when user has permission to read all users", () => { + describe("when user has permission to update users", () => { it("displays the admin menu", async () => { const checkUserPermissionsSpy = jest.spyOn(API, "checkUserPermissions").mockResolvedValueOnce({ - [checks.readAllUsers]: true, + [checks.updateUsers]: true, }) renderWithAuth() @@ -25,10 +25,10 @@ describe("Navbar", () => { }) }) - describe("when user has NO permission to read all users", () => { + describe("when user has NO permission to update users", () => { it("does not display the admin menu", async () => { const checkUserPermissionsSpy = jest.spyOn(API, "checkUserPermissions").mockResolvedValueOnce({ - [checks.readAllUsers]: false, + [checks.updateUsers]: false, }) renderWithAuth() diff --git a/site/src/components/UsersTable/UsersTable.tsx b/site/src/components/UsersTable/UsersTable.tsx index 1bcda6f2d6f3a..62075d66d51ef 100644 --- a/site/src/components/UsersTable/UsersTable.tsx +++ b/site/src/components/UsersTable/UsersTable.tsx @@ -27,6 +27,7 @@ export interface UsersTableProps { roles?: TypesGen.Role[] isUpdatingUserRoles?: boolean canEditUsers?: boolean + isLoading?: boolean onSuspendUser: (user: TypesGen.User) => void onResetUserPassword: (user: TypesGen.User) => void onUpdateUserRoles: (user: TypesGen.User, roles: TypesGen.Role["name"][]) => void @@ -40,9 +41,8 @@ export const UsersTable: React.FC = ({ onUpdateUserRoles, isUpdatingUserRoles, canEditUsers, + isLoading, }) => { - const isLoading = !users || (canEditUsers && !roles) - return (
@@ -56,6 +56,7 @@ export const UsersTable: React.FC = ({ {isLoading && } {!isLoading && + users && users.map((u) => ( diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 690c2e9631c1f..b52cf1c69aa62 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -24,6 +24,11 @@ export const UsersPage: React.FC = () => { const permissions = useSelector(xServices.authXService, selectPermissions) const canEditUsers = permissions && permissions.updateUsers const { roles } = rolesState.context + // Is loading if + // - permissions are not loaded or + // - users are not loaded or + // - the user can edit the users but the roles are not loaded yet + const isLoading = !permissions || !users || (canEditUsers && !roles) // Fetch users on component mount useEffect(() => { @@ -63,6 +68,7 @@ export const UsersPage: React.FC = () => { }} error={getUsersError} isUpdatingUserRoles={usersState.matches("updatingUserRoles")} + isLoading={isLoading} canEditUsers={canEditUsers} /> diff --git a/site/src/pages/UsersPage/UsersPageView.tsx b/site/src/pages/UsersPage/UsersPageView.tsx index 5f8447b07ac2e..256e4bc09b0e0 100644 --- a/site/src/pages/UsersPage/UsersPageView.tsx +++ b/site/src/pages/UsersPage/UsersPageView.tsx @@ -17,6 +17,7 @@ export interface UsersPageViewProps { error?: unknown isUpdatingUserRoles?: boolean canEditUsers?: boolean + isLoading?: boolean openUserCreationDialog: () => void onSuspendUser: (user: TypesGen.User) => void onResetUserPassword: (user: TypesGen.User) => void @@ -33,6 +34,7 @@ export const UsersPageView: React.FC = ({ error, isUpdatingUserRoles, canEditUsers, + isLoading, }) => { return ( @@ -49,6 +51,7 @@ export const UsersPageView: React.FC = ({ onUpdateUserRoles={onUpdateUserRoles} isUpdatingUserRoles={isUpdatingUserRoles} canEditUsers={canEditUsers} + isLoading={isLoading} /> )} From dcf598b04b55a365e473479ed703d74d1636456d Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 19 May 2022 14:16:39 +0000 Subject: [PATCH 4/4] Add editable flag in the UsersTable story --- site/src/components/UsersTable/UsersTable.stories.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/site/src/components/UsersTable/UsersTable.stories.tsx b/site/src/components/UsersTable/UsersTable.stories.tsx index ddd025b547bf3..bb806ea2f968f 100644 --- a/site/src/components/UsersTable/UsersTable.stories.tsx +++ b/site/src/components/UsersTable/UsersTable.stories.tsx @@ -14,6 +14,14 @@ export const Example = Template.bind({}) Example.args = { users: [MockUser, MockUser2], roles: MockSiteRoles, + canEditUsers: false, +} + +export const Editable = Template.bind({}) +Editable.args = { + users: [MockUser, MockUser2], + roles: MockSiteRoles, + canEditUsers: true, } export const Empty = Template.bind({})