From c45610baf33dc4218fb4b3e2310aee209f4aa6ac Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 7 Jul 2022 15:05:59 -0500 Subject: [PATCH 01/17] most basic possible policy merge --- app/pages/project/access/ProjectAccessPage.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index 0ad95afe08..4f85683f1d 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -60,8 +60,22 @@ export function ProjectAccessPage() { const [editingUserRow, setEditingUserRow] = useState(null) const projectParams = useRequiredParams('orgName', 'projectName') const { data: policy } = useApiQuery('projectPolicyView', projectParams) + const { data: orgPolicy } = useApiQuery('organizationPolicyView', { + orgName: projectParams.orgName, + }) + + // user can also get roles from the silo (and possibly the fleet?) but the + // silo policy view endpoint is `/silos/:siloName/policy`, and we don't have + // the silo name, so we can't fetch it yet. need to think about this + + const combinedPolicy = { + roleAssignments: [ + ...(policy?.roleAssignments || []), + ...(orgPolicy?.roleAssignments || []), + ], + } - const rows = useUserAccessRows(policy, projectRoleOrder) + const rows = useUserAccessRows(combinedPolicy, projectRoleOrder) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('projectPolicyUpdate', { From 8273c344344974c3542b1adae4cd3101cabf829c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 11 Aug 2022 13:06:15 -0500 Subject: [PATCH 02/17] bump api to silo policy endpoint PR branch --- OMICRON_VERSION | 2 +- libs/api/__generated__/Api.ts | 39 ++++++++++++++++++++++---- libs/api/__generated__/OMICRON_VERSION | 2 +- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 150406fa82..7173534e8f 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -aff0d7ace4d92612619289b3e75c597269282166 +69ae4c1204d2ccfa93c4cd4d557becf75aa372b4 diff --git a/libs/api/__generated__/Api.ts b/libs/api/__generated__/Api.ts index acb1da5b6b..0bab28e3d7 100644 --- a/libs/api/__generated__/Api.ts +++ b/libs/api/__generated__/Api.ts @@ -2293,6 +2293,10 @@ export interface DeviceAuthConfirmParams {} export interface DeviceAccessTokenParams {} +export interface GlobalPolicyViewParams {} + +export interface GlobalPolicyUpdateParams {} + export interface RackListParams { limit?: number | null pageToken?: string | null @@ -3321,6 +3325,31 @@ export class Api extends HttpClient { ...params, }), + /** + * Fetch the top-level IAM policy + */ + globalPolicyView: (query: GlobalPolicyViewParams, params: RequestParams = {}) => + this.request({ + path: `/global/policy`, + method: 'GET', + ...params, + }), + + /** + * Update the top-level IAM policy + */ + globalPolicyUpdate: ( + query: GlobalPolicyUpdateParams, + body: FleetRolePolicy, + params: RequestParams = {} + ) => + this.request({ + path: `/global/policy`, + method: 'PUT', + body, + ...params, + }), + /** * List racks */ @@ -4573,24 +4602,24 @@ export class Api extends HttpClient { }), /** - * Fetch the top-level IAM policy + * Fetch the current silo's IAM policy */ policyView: (query: PolicyViewParams, params: RequestParams = {}) => - this.request({ + this.request({ path: `/policy`, method: 'GET', ...params, }), /** - * Update the top-level IAM policy + * Update the current silo's IAM policy */ policyUpdate: ( query: PolicyUpdateParams, - body: FleetRolePolicy, + body: SiloRolePolicy, params: RequestParams = {} ) => - this.request({ + this.request({ path: `/policy`, method: 'PUT', body, diff --git a/libs/api/__generated__/OMICRON_VERSION b/libs/api/__generated__/OMICRON_VERSION index 3b496e9d8c..cc08cfd056 100644 --- a/libs/api/__generated__/OMICRON_VERSION +++ b/libs/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -aff0d7ace4d92612619289b3e75c597269282166 +69ae4c1204d2ccfa93c4cd4d557becf75aa372b4 From b50c44a3d8e3f26421ccfc4f700f1691a7eaba2e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 11 Aug 2022 14:59:28 -0500 Subject: [PATCH 03/17] merge silo, org policies for org IAM; silo, org, project for project IAM --- app/pages/OrgAccessPage.tsx | 35 +++++++++------- app/pages/__tests__/org-access.e2e.ts | 24 ++++++----- app/pages/__tests__/project-access.e2e.ts | 41 ++++++++++++------- .../project/access/ProjectAccessPage.tsx | 34 +++++++-------- libs/api-mocks/msw/handlers.ts | 21 +++++++--- libs/api-mocks/role-assignment.ts | 20 ++++++--- libs/api-mocks/silo.ts | 13 ++++++ libs/api-mocks/user.ts | 12 +++++- 8 files changed, 133 insertions(+), 67 deletions(-) create mode 100644 libs/api-mocks/silo.ts diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 3a07efdb28..4150754b1a 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -42,6 +42,7 @@ const EmptyState = ({ onClick }: { onClick: () => void }) => ( OrgAccessPage.loader = async ({ params }: LoaderFunctionArgs) => { await Promise.all([ + apiQueryClient.prefetchQuery('policyView', {}), apiQueryClient.prefetchQuery('organizationPolicyView', requireOrgParams(params)), // used in useUserAccessRows to resolve user names apiQueryClient.prefetchQuery('userList', { limit: 200 }), @@ -56,9 +57,17 @@ export function OrgAccessPage() { const [addModalOpen, setAddModalOpen] = useState(false) const [editingUserRow, setEditingUserRow] = useState(null) const orgParams = useRequiredParams('orgName') - const { data: policy } = useApiQuery('organizationPolicyView', orgParams) + const { data: siloPolicy } = useApiQuery('policyView', {}) + const { data: orgPolicy } = useApiQuery('organizationPolicyView', orgParams) - const rows = useUserAccessRows(policy, orgRoleOrder) + const combinedPolicy = { + roleAssignments: [ + ...(siloPolicy?.roleAssignments || []), + ...(orgPolicy?.roleAssignments || []), + ], + } + + const rows = useUserAccessRows(combinedPolicy, orgRoleOrder) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('organizationPolicyUpdate', { @@ -90,13 +99,13 @@ export function OrgAccessPage() { updatePolicy.mutate({ ...orgParams, // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, policy!), + body: setUserRole(row.id, null, orgPolicy!), }) }, }, ]), ], - [policy, orgParams, updatePolicy] + [orgPolicy, orgParams, updatePolicy] ) const tableInstance = useReactTable({ @@ -116,20 +125,18 @@ export function OrgAccessPage() { Add user to organization - {policy && ( - setAddModalOpen(false)} - onSuccess={() => setAddModalOpen(false)} - policy={policy} - /> - )} - {policy && editingUserRow && ( + setAddModalOpen(false)} + onSuccess={() => setAddModalOpen(false)} + policy={combinedPolicy} + /> + {editingUserRow && ( setEditingUserRow(null)} onSuccess={() => setEditingUserRow(null)} - policy={policy} + policy={combinedPolicy} userId={editingUserRow.id} initialValues={{ roleName: editingUserRow.roleName }} /> diff --git a/app/pages/__tests__/org-access.e2e.ts b/app/pages/__tests__/org-access.e2e.ts index 582ab1739e..c8d3e8dd12 100644 --- a/app/pages/__tests__/org-access.e2e.ts +++ b/app/pages/__tests__/org-access.e2e.ts @@ -7,11 +7,12 @@ test('Click through org access page', async ({ page }) => { const table = page.locator('role=table') - // page is there, we see user 1 but not 2 + // page is there, we see user 1 and 2 but not 3 await page.click('role=link[name*="Access & IAM"]') await expectVisible(page, ['role=heading[name*="Access & IAM"]']) await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) - await expectNotVisible(page, ['role=cell[name="user-2"]']) + await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'viewer' }) + await expectNotVisible(page, ['role=cell[name="user-3"]']) // Add user 2 as collab await page.click('role=button[name="Add user to organization"]') @@ -19,10 +20,13 @@ test('Click through org access page', async ({ page }) => { await page.click('role=button[name="User"]') // only users not already on the org should be visible - await expectNotVisible(page, ['role=option[name="Hannah Arendt"]']) - await expectVisible(page, ['role=option[name="Hans Jonas"]']) + await expectNotVisible(page, [ + 'role=option[name="Hannah Arendt"]', + 'role=option[name="Hans Jonas"]', + ]) + await expectVisible(page, ['role=option[name="Jacob Klein"]']) - await page.click('role=option[name="Hans Jonas"]') + await page.click('role=option[name="Jacob Klein"]') await page.click('role=button[name="Role"]') await expectVisible(page, [ @@ -34,12 +38,12 @@ test('Click through org access page', async ({ page }) => { await page.click('role=option[name="Collaborator"]') await page.click('role=button[name="Add user"]') - // User 2 shows up in the table - await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'collaborator' }) + // User 3 shows up in the table + await expectRowVisible(table, { ID: 'user-3', Name: 'Jacob Klein', Role: 'collaborator' }) - // now change user 2's role from collab to viewer + // now change user 3's role from collab to viewer await page - .locator('role=row', { hasText: 'user-2' }) + .locator('role=row', { hasText: 'user-3' }) .locator('role=button[name="Row actions"]') .click() await page.click('role=menuitem[name="Change role"]') @@ -51,7 +55,7 @@ test('Click through org access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(table, { ID: 'user-2', Role: 'viewer' }) + await expectRowVisible(table, { ID: 'user-3', Role: 'viewer' }) // now delete user 2 await page diff --git a/app/pages/__tests__/project-access.e2e.ts b/app/pages/__tests__/project-access.e2e.ts index 177760d283..5798edd05d 100644 --- a/app/pages/__tests__/project-access.e2e.ts +++ b/app/pages/__tests__/project-access.e2e.ts @@ -6,22 +6,28 @@ test('Click through project access page', async ({ page }) => { await page.goto('/orgs/maze-war/projects/mock-project') await page.click('role=link[name*="Access & IAM"]') - // page is there, we see user 1 but not 2 + // page is there, we see user 1-3 but not 4 await expectVisible(page, ['role=heading[name*="Access & IAM"]']) const table = page.locator('table') await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) - await expectNotVisible(page, ['role=cell[name="user-2"]']) + await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'viewer' }) + await expectRowVisible(table, { ID: 'user-3', Name: 'Jacob Klein', Role: 'collaborator' }) + await expectNotVisible(page, ['role=cell[name="user-4"]']) - // Add user 2 as collab + // Add user 4 as collab await page.click('role=button[name="Add user to project"]') await expectVisible(page, ['role=heading[name*="Add user to project"]']) await page.click('role=button[name="User"]') // only users not already on the project should be visible - await expectNotVisible(page, ['role=option[name="Hannah Arendt"]']) - await expectVisible(page, ['role=option[name="Hans Jonas"]']) + await expectNotVisible(page, [ + 'role=option[name="Hannah Arendt"]', + 'role=option[name="Hans Jonas"]', + 'role=option[name="Jacob Klein"]', + ]) + await expectVisible(page, ['role=option[name="Simone de Beauvoir"]']) - await page.click('role=option[name="Hans Jonas"]') + await page.click('role=option[name="Simone de Beauvoir"]') await page.click('role=button[name="Role"]') await expectVisible(page, [ @@ -33,12 +39,16 @@ test('Click through project access page', async ({ page }) => { await page.click('role=option[name="Collaborator"]') await page.click('role=button[name="Add user"]') - // User 2 shows up in the table - await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'collaborator' }) + // User 4 shows up in the table + await expectRowVisible(table, { + ID: 'user-4', + Name: 'Simone de Beauvoir', + Role: 'collaborator', + }) - // now change user 2 role from collab to viewer + // now change user 4 role from collab to viewer await page - .locator('role=row', { hasText: 'user-2' }) + .locator('role=row', { hasText: 'user-4' }) .locator('role=button[name="Row actions"]') .click() await page.click('role=menuitem[name="Change role"]') @@ -50,14 +60,15 @@ test('Click through project access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(table, { ID: 'user-2', Role: 'viewer' }) + await expectRowVisible(table, { ID: 'user-4', Role: 'viewer' }) - // now delete user 2 + // now delete user 3. has to be 3 or 4 because they're the only ones that come + // from the project policy await page - .locator('role=row', { hasText: 'user-2' }) + .locator('role=row', { hasText: 'user-3' }) .locator('role=button[name="Row actions"]') .click() - await expectVisible(page, ['role=cell[name=user-2]']) + await expectVisible(page, ['role=cell[name=user-3]']) await page.click('role=menuitem[name="Delete"]') - await expectNotVisible(page, ['role=cell[name=user-2]']) + await expectNotVisible(page, ['role=cell[name=user-3]']) }) diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index 4f85683f1d..53199d9d14 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -44,8 +44,11 @@ const EmptyState = ({ onClick }: { onClick: () => void }) => ( ) ProjectAccessPage.loader = async ({ params }: LoaderFunctionArgs) => { + const { orgName, projectName } = requireProjectParams(params) await Promise.all([ - apiQueryClient.prefetchQuery('projectPolicyView', requireProjectParams(params)), + apiQueryClient.prefetchQuery('policyView', {}), + apiQueryClient.prefetchQuery('organizationPolicyView', { orgName }), + apiQueryClient.prefetchQuery('projectPolicyView', { orgName, projectName }), // used in useUserAccessRows to resolve user names apiQueryClient.prefetchQuery('userList', { limit: 200 }), ]) @@ -59,19 +62,16 @@ export function ProjectAccessPage() { const [addModalOpen, setAddModalOpen] = useState(false) const [editingUserRow, setEditingUserRow] = useState(null) const projectParams = useRequiredParams('orgName', 'projectName') - const { data: policy } = useApiQuery('projectPolicyView', projectParams) - const { data: orgPolicy } = useApiQuery('organizationPolicyView', { - orgName: projectParams.orgName, - }) - - // user can also get roles from the silo (and possibly the fleet?) but the - // silo policy view endpoint is `/silos/:siloName/policy`, and we don't have - // the silo name, so we can't fetch it yet. need to think about this + const { orgName } = projectParams + const { data: siloPolicy } = useApiQuery('policyView', {}) + const { data: orgPolicy } = useApiQuery('organizationPolicyView', { orgName }) + const { data: projectPolicy } = useApiQuery('projectPolicyView', projectParams) const combinedPolicy = { roleAssignments: [ - ...(policy?.roleAssignments || []), + ...(siloPolicy?.roleAssignments || []), ...(orgPolicy?.roleAssignments || []), + ...(projectPolicy?.roleAssignments || []), ], } @@ -107,13 +107,13 @@ export function ProjectAccessPage() { updatePolicy.mutate({ ...projectParams, // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, policy!), + body: setUserRole(row.id, null, projectPolicy!), }) }, }, ]), ], - [policy, projectParams, updatePolicy] + [projectPolicy, projectParams, updatePolicy] ) const tableInstance = useReactTable({ @@ -133,18 +133,18 @@ export function ProjectAccessPage() { Add user to project - {policy && ( + { setAddModalOpen(false)} - policy={policy} + policy={combinedPolicy} /> - )} - {policy && editingUserRow && ( + } + {editingUserRow && ( setEditingUserRow(null)} - policy={policy} + policy={combinedPolicy} userId={editingUserRow.id} initialValues={{ roleName: editingUserRow.roleName }} /> diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index f52ca0b509..9d97ad7438 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -6,6 +6,7 @@ import { pick, sortBy } from '@oxide/util' import type { Json } from '../json-type' import { serial } from '../serial' import { sessionMe } from '../session' +import { defaultSilo } from '../silo' import type { DiskParams, GlobalImageParams, @@ -20,16 +21,16 @@ import type { VpcRouterParams, VpcSubnetParams, } from './db' -import { lookupById } from './db' -import { lookupSshKey } from './db' -import { lookupDisk } from './db' -import { lookupGlobalImage } from './db' import { db, + lookupById, + lookupDisk, + lookupGlobalImage, lookupInstance, lookupNetworkInterface, lookupOrg, lookupProject, + lookupSshKey, lookupVpc, lookupVpcRouter, lookupVpcSubnet, @@ -129,6 +130,16 @@ export const handlers = [ } ), + rest.get | GetErr>('/api/policy', (req, res) => { + // assume we're in the default silo + const siloId = defaultSilo.id + const role_assignments = db.roleAssignments + .filter((r) => r.resource_type === 'silo' && r.resource_id === siloId) + .map((r) => pick(r, 'identity_id', 'identity_type', 'role_name')) + + return res(json({ role_assignments })) + }), + rest.get>( '/api/organizations', (req, res) => res(json(paginated(req.url.search, db.orgs))) @@ -184,7 +195,7 @@ export const handlers = [ } ), - rest.get | GetErr>( + rest.get | GetErr>( '/api/organizations/:orgName/policy', (req, res) => { const [org, err] = lookupOrg(req.params) diff --git a/libs/api-mocks/role-assignment.ts b/libs/api-mocks/role-assignment.ts index 17d12da7ac..744d832543 100644 --- a/libs/api-mocks/role-assignment.ts +++ b/libs/api-mocks/role-assignment.ts @@ -1,12 +1,14 @@ import type { OrganizationRoleRoleAssignment, ProjectRoleRoleAssignment, + SiloRoleRoleAssignment, } from 'libs/api/__generated__/Api' import type { Json } from './json-type' import { org } from './org' import { project } from './project' -import { user1 } from './user' +import { defaultSilo } from './silo' +import { user1, user2, user3 } from './user' // For most other resources, we can store the API types directly in the DB. But // in this case the API response doesn't have the resource ID on it, and we need @@ -17,21 +19,29 @@ import { user1 } from './user' type DbRoleAssignment = { resource_id: string } & ( | ({ resource_type: 'project' } & Json) | ({ resource_type: 'organization' } & Json) + | ({ resource_type: 'silo' } & Json) ) export const roleAssignments: DbRoleAssignment[] = [ { - resource_type: 'organization', - resource_id: org.id, + resource_type: 'silo', + resource_id: defaultSilo.id, identity_id: user1.id, identity_type: 'silo_user', role_name: 'admin', }, + { + resource_type: 'organization', + resource_id: org.id, + identity_id: user2.id, + identity_type: 'silo_user', + role_name: 'viewer', + }, { resource_type: 'project', resource_id: project.id, - identity_id: user1.id, + identity_id: user3.id, identity_type: 'silo_user', - role_name: 'admin', + role_name: 'collaborator', }, ] diff --git a/libs/api-mocks/silo.ts b/libs/api-mocks/silo.ts new file mode 100644 index 0000000000..563aba997c --- /dev/null +++ b/libs/api-mocks/silo.ts @@ -0,0 +1,13 @@ +import type { Silo } from '@oxide/api' + +import type { Json } from './json-type' + +export const defaultSilo: Json = { + id: 'default-silo-uuid', + name: 'default-silo', + description: 'a fake default silo', + time_created: new Date(2021, 3, 1).toISOString(), + time_modified: new Date(2021, 4, 2).toISOString(), + discoverable: true, + user_provision_type: 'jit', +} diff --git a/libs/api-mocks/user.ts b/libs/api-mocks/user.ts index c77665d0d4..e657e31111 100644 --- a/libs/api-mocks/user.ts +++ b/libs/api-mocks/user.ts @@ -12,4 +12,14 @@ export const user2: Json = { display_name: 'Hans Jonas', } -export const users = [user1, user2] +export const user3: Json = { + id: 'user-3', + display_name: 'Jacob Klein', +} + +export const user4: Json = { + id: 'user-4', + display_name: 'Simone de Beauvoir', +} + +export const users = [user1, user2, user3, user4] From 2e28933496b39ae7ca63c4d6cec5718e260b511e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 12:44:48 -0500 Subject: [PATCH 04/17] simplify IAM merge, don't group by user (will probably put it back) --- app/pages/OrgAccessPage.tsx | 46 ++++++++++------- .../project/access/ProjectAccessPage.tsx | 40 ++++++++------- libs/api/roles.ts | 50 +++++++++---------- 3 files changed, 74 insertions(+), 62 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 4150754b1a..2d8e3fbe89 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -5,11 +5,10 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, - orgRoleOrder, setUserRole, useApiMutation, useApiQueryClient, - useUserAccessRows, + useUserRows, } from '@oxide/api' import type { OrganizationRole, UserAccessRow } from '@oxide/api' import { useApiQuery } from '@oxide/api' @@ -24,6 +23,7 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' +import { sortBy } from '@oxide/util' import { OrgAccessAddUserSideModal, OrgAccessEditUserSideModal } from 'app/forms/org-access' import { requireOrgParams, useRequiredParams } from 'app/hooks' @@ -60,14 +60,12 @@ export function OrgAccessPage() { const { data: siloPolicy } = useApiQuery('policyView', {}) const { data: orgPolicy } = useApiQuery('organizationPolicyView', orgParams) - const combinedPolicy = { - roleAssignments: [ - ...(siloPolicy?.roleAssignments || []), - ...(orgPolicy?.roleAssignments || []), - ], - } - - const rows = useUserAccessRows(combinedPolicy, orgRoleOrder) + const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') + const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo') + const rows = useMemo( + () => sortBy(orgRows.concat(siloRows), (u) => u.id), + [orgRows, siloRows] + ) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('organizationPolicyUpdate', { @@ -84,7 +82,11 @@ export function OrgAccessPage() { colHelper.accessor('name', { header: 'Name' }), colHelper.accessor('roleName', { header: 'Role', - cell: (info) => {info.getValue()}, + cell: (info) => {info.getValue()}, + }), + colHelper.accessor('roleSource', { + header: 'Role source', + cell: (info) => {info.getValue()}, }), getActionsCol((row: UserRow) => [ { @@ -125,18 +127,24 @@ export function OrgAccessPage() { Add user to organization - setAddModalOpen(false)} - onSuccess={() => setAddModalOpen(false)} - policy={combinedPolicy} - /> - {editingUserRow && ( + {orgPolicy && ( + setAddModalOpen(false)} + onSuccess={() => setAddModalOpen(false)} + // has to be org policy and not combined because you can still add a + // user who's on the silo to the org policy + // TODO: compute user list explicitly here instead of doing it inside + // the modal + policy={orgPolicy} + /> + )} + {orgPolicy && editingUserRow && ( setEditingUserRow(null)} onSuccess={() => setEditingUserRow(null)} - policy={combinedPolicy} + policy={orgPolicy} userId={editingUserRow.id} initialValues={{ roleName: editingUserRow.roleName }} /> diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index 53199d9d14..f5c96ed2d8 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -5,11 +5,10 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, - projectRoleOrder, setUserRole, useApiMutation, useApiQueryClient, - useUserAccessRows, + useUserRows, } from '@oxide/api' import type { ProjectRole, UserAccessRow } from '@oxide/api' import { useApiQuery } from '@oxide/api' @@ -24,6 +23,7 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' +import { sortBy } from '@oxide/util' import { ProjectAccessAddUserSideModal, @@ -67,15 +67,13 @@ export function ProjectAccessPage() { const { data: orgPolicy } = useApiQuery('organizationPolicyView', { orgName }) const { data: projectPolicy } = useApiQuery('projectPolicyView', projectParams) - const combinedPolicy = { - roleAssignments: [ - ...(siloPolicy?.roleAssignments || []), - ...(orgPolicy?.roleAssignments || []), - ...(projectPolicy?.roleAssignments || []), - ], - } - - const rows = useUserAccessRows(combinedPolicy, projectRoleOrder) + const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo') + const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') + const projectRows = useUserRows(projectPolicy?.roleAssignments, 'project') + const rows = useMemo( + () => sortBy(siloRows.concat(orgRows, projectRows), (u) => u.id), + [siloRows, orgRows, projectRows] + ) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('projectPolicyUpdate', { @@ -92,7 +90,11 @@ export function ProjectAccessPage() { colHelper.accessor('name', { header: 'Name' }), colHelper.accessor('roleName', { header: 'Role', - cell: (info) => {info.getValue()}, + cell: (info) => {info.getValue()}, + }), + colHelper.accessor('roleSource', { + header: 'Role source', + cell: (info) => {info.getValue()}, }), getActionsCol((row: UserRow) => [ { @@ -133,18 +135,22 @@ export function ProjectAccessPage() { Add user to project - { + {projectPolicy && ( setAddModalOpen(false)} - policy={combinedPolicy} + // has to be project policy and not combined because you can still add a + // user who's on the silo or org to the project policy + // TODO: compute user list explicitly here instead of doing it inside + // the modal + policy={projectPolicy} /> - } - {editingUserRow && ( + )} + {projectPolicy && editingUserRow && ( setEditingUserRow(null)} - policy={combinedPolicy} + policy={projectPolicy} userId={editingUserRow.id} initialValues={{ roleName: editingUserRow.roleName }} /> diff --git a/libs/api/roles.ts b/libs/api/roles.ts index c605a82ac0..69d6b196a8 100644 --- a/libs/api/roles.ts +++ b/libs/api/roles.ts @@ -5,7 +5,7 @@ */ import { useMemo } from 'react' -import { groupBy, sortBy } from '@oxide/util' +import { sortBy } from '@oxide/util' import { useApiQuery } from '.' import type { IdentityType, OrganizationRole, ProjectRole } from './__generated__/Api' @@ -96,43 +96,41 @@ export type UserAccessRow = { id: string name: string roleName: Role + roleSource: string } /** * Role assignments come from the API in (user, role) pairs without display - * names. This groups those pairs into one row per user, picks the strongest - * role as that user's "main" role, and uses a fetched list of "all" users to - * add a display name for each user. It's a bit awkward, but the logic is + * names and without info about which resource the role came from. This tags + * each row with that info. It has to be a hook because it depends on the result + * of an API request for the list of users. It's a bit awkward, but the logic is * identical between projects and orgs so it is worth sharing. */ -export function useUserAccessRows( - // allow undefined because this is fetched with RQ - policy: Policy | undefined, - roleOrder: Record +export function useUserRows( + roleAssignments: RoleAssignment[] | undefined, + roleSource: string ): UserAccessRow[] { - // TODO: this hits /users, which returns system users, not silo users. We need - // an endpoint to list silo users. I'm hoping we might end up using /users for - // that. See https://github.com/oxidecomputer/omicron/issues/1235 - const { data: users } = useApiQuery('userList', { limit: 200 }) - // HACK: because the policy has no names, we are fetching ~all the users, // putting them in a dictionary, and adding the names to the rows - const usersDict = useMemo( + const usersDict = useUsersDict() + return useMemo( + () => + (roleAssignments || []).map((ra) => ({ + id: ra.identityId, + name: usersDict[ra.identityId]?.displayName || '', // placeholder until we get names, obviously + roleName: ra.roleName, + roleSource, + })), + [roleAssignments, roleSource, usersDict] + ) +} + +function useUsersDict() { + const { data: users } = useApiQuery('userList', { limit: 200 }) + return useMemo( () => Object.fromEntries((users?.items || []).map((u) => [u.id, u])), [users] ) - - return useMemo(() => { - const roleAssignments = policy?.roleAssignments || [] - const groups = groupBy(roleAssignments, (u) => u.identityId) - return Object.entries(groups).map(([userId, groupRoleAssignments]) => ({ - id: userId, - name: usersDict[userId]?.displayName || '', // placeholder until we get names, obviously - // assert non-null because we know there has to be one, otherwise there - // wouldn't be a group - roleName: getMainRole(roleOrder)(groupRoleAssignments.map((ra) => ra.roleName))!, - })) - }, [policy, usersDict, roleOrder]) } /** From f0da4f1e12e4506932f7547b81838b1263de0b6b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 13:36:23 -0500 Subject: [PATCH 05/17] bring the Object.entries inside groupBy --- .../form/fields/ImageSelectField.tsx | 8 +++---- libs/ui/lib/action-menu/ActionMenu.tsx | 8 +++---- libs/util/array.spec.ts | 24 ++++++++++++++++++- libs/util/array.ts | 2 +- 4 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/components/form/fields/ImageSelectField.tsx b/app/components/form/fields/ImageSelectField.tsx index 3883f2706c..eda9994d54 100644 --- a/app/components/form/fields/ImageSelectField.tsx +++ b/app/components/form/fields/ImageSelectField.tsx @@ -97,11 +97,9 @@ interface ImageSelectFieldProps extends Omit { export function ImageSelectField({ images, name, ...props }: ImageSelectFieldProps) { return ( - {Object.entries(groupBy(images, (i) => i.distribution)).map( - ([distroName, distroValues]) => ( - - ) - )} + {groupBy(images, (i) => i.distribution).map(([distroName, distroValues]) => ( + + ))} ) } diff --git a/libs/ui/lib/action-menu/ActionMenu.tsx b/libs/ui/lib/action-menu/ActionMenu.tsx index 681822d69a..d0cdf312a7 100644 --- a/libs/ui/lib/action-menu/ActionMenu.tsx +++ b/libs/ui/lib/action-menu/ActionMenu.tsx @@ -41,11 +41,9 @@ export function ActionMenu(props: ActionMenuProps) { const actions = items.filter((i) => !i.navGroup) // TODO: repent. this is horrible - const groupedItems = Object.entries( - groupBy( - items.filter((i) => i.navGroup), - (i) => i.navGroup! - ) + const groupedItems = groupBy( + items.filter((i) => i.navGroup), + (i) => i.navGroup! ) const allGroups: [string, QuickActionItem[]][] = diff --git a/libs/util/array.spec.ts b/libs/util/array.spec.ts index 5783f947c8..1c7b6f6fe2 100644 --- a/libs/util/array.spec.ts +++ b/libs/util/array.spec.ts @@ -1,4 +1,4 @@ -import { sortBy } from './array' +import { groupBy, sortBy } from './array' test('sortBy', () => { expect(sortBy(['d', 'b', 'c', 'a'])).toEqual(['a', 'b', 'c', 'd']) @@ -17,3 +17,25 @@ test('sortBy', () => { ) ).toEqual([{ x: [0] }, { x: [0, 0] }, { x: [0, 0, 0] }, { x: [0, 0, 0, 0] }]) }) + +test('groupBy', () => { + expect( + groupBy( + [ + { x: 'a', y: 1 }, + { x: 'b', y: 2 }, + { x: 'a', y: 3 }, + ], + (o) => o.x + ) + ).toEqual([ + [ + 'a', + [ + { x: 'a', y: 1 }, + { x: 'a', y: 3 }, + ], + ], + ['b', [{ x: 'b', y: 2 }]], + ]) +}) diff --git a/libs/util/array.ts b/libs/util/array.ts index fb5fe87ed1..74ebc93488 100644 --- a/libs/util/array.ts +++ b/libs/util/array.ts @@ -21,7 +21,7 @@ export function groupBy(arr: T[], by: (t: T) => GroupKey) { } groups[key].push(item) } - return groups + return Object.entries(groups) } type Truthy = T extends false | '' | 0 | null | undefined ? never : T From ba9f2b1517686f2aac164befa8b659dadaf5fd1f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 14:50:33 -0500 Subject: [PATCH 06/17] enter the matrix --- app/pages/OrgAccessPage.tsx | 86 ++++++++++-------- .../project/access/ProjectAccessPage.tsx | 90 +++++++++++-------- libs/api/roles.ts | 2 +- 3 files changed, 106 insertions(+), 72 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 2d8e3fbe89..09e69befe7 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -10,7 +10,7 @@ import { useApiQueryClient, useUserRows, } from '@oxide/api' -import type { OrganizationRole, UserAccessRow } from '@oxide/api' +import type { OrganizationRole, SiloRole } from '@oxide/api' import { useApiQuery } from '@oxide/api' import { Table, getActionsCol } from '@oxide/table' import { @@ -23,7 +23,7 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' -import { sortBy } from '@oxide/util' +import { groupBy, sortBy } from '@oxide/util' import { OrgAccessAddUserSideModal, OrgAccessEditUserSideModal } from 'app/forms/org-access' import { requireOrgParams, useRequiredParams } from 'app/hooks' @@ -44,12 +44,17 @@ OrgAccessPage.loader = async ({ params }: LoaderFunctionArgs) => { await Promise.all([ apiQueryClient.prefetchQuery('policyView', {}), apiQueryClient.prefetchQuery('organizationPolicyView', requireOrgParams(params)), - // used in useUserAccessRows to resolve user names + // used to resolve user names apiQueryClient.prefetchQuery('userList', { limit: 200 }), ]) } -type UserRow = UserAccessRow +type UserRow = { + id: string + name: string + siloRole: SiloRole | undefined + orgRole: OrganizationRole | undefined +} const colHelper = createColumnHelper() @@ -63,8 +68,17 @@ export function OrgAccessPage() { const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo') const rows = useMemo( - () => sortBy(orgRows.concat(siloRows), (u) => u.id), - [orgRows, siloRows] + () => + sortBy( + groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => ({ + id, + name: ras[0].name, + siloRole: ras.find((ra) => ra.roleSource === 'silo')?.roleName, + orgRole: ras.find((ra) => ra.roleSource === 'org')?.roleName, + })), + (row) => row.id + ), + [siloRows, orgRows] ) const queryClient = useApiQueryClient() @@ -80,32 +94,36 @@ export function OrgAccessPage() { () => [ colHelper.accessor('id', { header: 'ID' }), colHelper.accessor('name', { header: 'Name' }), - colHelper.accessor('roleName', { - header: 'Role', - cell: (info) => {info.getValue()}, + colHelper.accessor('siloRole', { + header: 'Silo role', + cell: (info) => (info.getValue() ? {info.getValue()} : null), }), - colHelper.accessor('roleSource', { - header: 'Role source', - cell: (info) => {info.getValue()}, + colHelper.accessor('orgRole', { + header: 'Org role', + cell: (info) => (info.getValue() ? {info.getValue()} : null), }), - getActionsCol((row: UserRow) => [ - { - label: 'Change role', - onActivate: () => setEditingUserRow(row), - }, - // TODO: only show if you have permission to do this - { - label: 'Delete', - onActivate() { - // TODO: confirm delete - updatePolicy.mutate({ - ...orgParams, - // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, orgPolicy!), - }) - }, - }, - ]), + getActionsCol((row: UserRow) => + row.orgRole + ? [ + { + label: 'Change role', + onActivate: () => setEditingUserRow(row), + }, + // TODO: only show if you have permission to do this + { + label: 'Delete', + onActivate() { + // TODO: confirm delete + updatePolicy.mutate({ + ...orgParams, + // we know policy is there, otherwise there's no row to display + body: setUserRole(row.id, null, orgPolicy!), + }) + }, + }, + ] + : [] + ), ], [orgPolicy, orgParams, updatePolicy] ) @@ -132,21 +150,17 @@ export function OrgAccessPage() { isOpen={addModalOpen} onDismiss={() => setAddModalOpen(false)} onSuccess={() => setAddModalOpen(false)} - // has to be org policy and not combined because you can still add a - // user who's on the silo to the org policy - // TODO: compute user list explicitly here instead of doing it inside - // the modal policy={orgPolicy} /> )} - {orgPolicy && editingUserRow && ( + {orgPolicy && editingUserRow?.orgRole && ( setEditingUserRow(null)} onSuccess={() => setEditingUserRow(null)} policy={orgPolicy} userId={editingUserRow.id} - initialValues={{ roleName: editingUserRow.roleName }} + initialValues={{ roleName: editingUserRow.orgRole }} /> )} {rows.length === 0 ? ( diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index f5c96ed2d8..f5de55c028 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -10,7 +10,7 @@ import { useApiQueryClient, useUserRows, } from '@oxide/api' -import type { ProjectRole, UserAccessRow } from '@oxide/api' +import type { OrganizationRole, ProjectRole, SiloRole } from '@oxide/api' import { useApiQuery } from '@oxide/api' import { Table, getActionsCol } from '@oxide/table' import { @@ -23,7 +23,7 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' -import { sortBy } from '@oxide/util' +import { groupBy, sortBy } from '@oxide/util' import { ProjectAccessAddUserSideModal, @@ -49,12 +49,18 @@ ProjectAccessPage.loader = async ({ params }: LoaderFunctionArgs) => { apiQueryClient.prefetchQuery('policyView', {}), apiQueryClient.prefetchQuery('organizationPolicyView', { orgName }), apiQueryClient.prefetchQuery('projectPolicyView', { orgName, projectName }), - // used in useUserAccessRows to resolve user names + // used to resolve user names apiQueryClient.prefetchQuery('userList', { limit: 200 }), ]) } -type UserRow = UserAccessRow +type UserRow = { + id: string + name: string + siloRole: SiloRole | undefined + orgRole: OrganizationRole | undefined + projectRole: ProjectRole | undefined +} const colHelper = createColumnHelper() @@ -71,7 +77,17 @@ export function ProjectAccessPage() { const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') const projectRows = useUserRows(projectPolicy?.roleAssignments, 'project') const rows = useMemo( - () => sortBy(siloRows.concat(orgRows, projectRows), (u) => u.id), + () => + sortBy( + groupBy(siloRows.concat(orgRows, projectRows), (u) => u.id).map(([id, ras]) => ({ + id, + name: ras[0].name, + siloRole: ras.find((ra) => ra.roleSource === 'silo')?.roleName, + orgRole: ras.find((ra) => ra.roleSource === 'org')?.roleName, + projectRole: ras.find((ra) => ra.roleSource === 'project')?.roleName, + })), + (row) => row.id + ), [siloRows, orgRows, projectRows] ) @@ -88,32 +104,40 @@ export function ProjectAccessPage() { () => [ colHelper.accessor('id', { header: 'ID' }), colHelper.accessor('name', { header: 'Name' }), - colHelper.accessor('roleName', { - header: 'Role', - cell: (info) => {info.getValue()}, + colHelper.accessor('siloRole', { + header: 'Silo role', + cell: (info) => (info.getValue() ? {info.getValue()} : null), + }), + colHelper.accessor('orgRole', { + header: 'Org role', + cell: (info) => (info.getValue() ? {info.getValue()} : null), }), - colHelper.accessor('roleSource', { - header: 'Role source', - cell: (info) => {info.getValue()}, + colHelper.accessor('projectRole', { + header: 'Project role', + cell: (info) => (info.getValue() ? {info.getValue()} : null), }), - getActionsCol((row: UserRow) => [ - { - label: 'Change role', - onActivate: () => setEditingUserRow(row), - }, - // TODO: only show if you have permission to do this - { - label: 'Delete', - onActivate() { - // TODO: confirm delete - updatePolicy.mutate({ - ...projectParams, - // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, projectPolicy!), - }) - }, - }, - ]), + getActionsCol((row: UserRow) => + row.projectRole + ? [ + { + label: 'Change role', + onActivate: () => setEditingUserRow(row), + }, + // TODO: only show if you have permission to do this + { + label: 'Delete', + onActivate() { + // TODO: confirm delete + updatePolicy.mutate({ + ...projectParams, + // we know policy is there, otherwise there's no row to display + body: setUserRole(row.id, null, projectPolicy!), + }) + }, + }, + ] + : [] + ), ], [projectPolicy, projectParams, updatePolicy] ) @@ -139,20 +163,16 @@ export function ProjectAccessPage() { setAddModalOpen(false)} - // has to be project policy and not combined because you can still add a - // user who's on the silo or org to the project policy - // TODO: compute user list explicitly here instead of doing it inside - // the modal policy={projectPolicy} /> )} - {projectPolicy && editingUserRow && ( + {projectPolicy && editingUserRow?.projectRole && ( setEditingUserRow(null)} policy={projectPolicy} userId={editingUserRow.id} - initialValues={{ roleName: editingUserRow.roleName }} + initialValues={{ roleName: editingUserRow.projectRole }} /> )} {rows.length === 0 ? ( diff --git a/libs/api/roles.ts b/libs/api/roles.ts index 69d6b196a8..f342579385 100644 --- a/libs/api/roles.ts +++ b/libs/api/roles.ts @@ -92,7 +92,7 @@ export function setUserRole( return { roleAssignments } } -export type UserAccessRow = { +type UserAccessRow = { id: string name: string roleName: Role From ca73e81d799c25c121f72ea8dbbdc6882a9ac3c0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 15:51:18 -0500 Subject: [PATCH 07/17] highlight effective role in green --- app/components/RoleBadgeCell.tsx | 29 +++++++++++++++ app/pages/OrgAccessPage.tsx | 34 ++++++++++++----- .../project/access/ProjectAccessPage.tsx | 37 +++++++++++++------ 3 files changed, 78 insertions(+), 22 deletions(-) create mode 100644 app/components/RoleBadgeCell.tsx diff --git a/app/components/RoleBadgeCell.tsx b/app/components/RoleBadgeCell.tsx new file mode 100644 index 0000000000..d1f1bde580 --- /dev/null +++ b/app/components/RoleBadgeCell.tsx @@ -0,0 +1,29 @@ +import type { CoreColumnDef } from '@tanstack/react-table' + +import type { OrganizationRole, ProjectRole, SiloRole } from '@oxide/api' +import { Badge } from '@oxide/ui' + +// I think there are changes in RT 8.4+ that make this less ridiculous +type CellInfo = Parameters< + Exclude['cell'], string | undefined> +>[0] + +type Role = SiloRole | OrganizationRole | ProjectRole + +/** + * Highlight the "effective" role in green, others gray. + * + * Example: User has collab on org and viewer on project. Collab supersedes + * because it is the "stronger" role, i.e., it strictly includes the perms on + * viewer. So collab is highlighted as the "effective" role. + */ +export const RoleBadgeCell = ( + info: CellInfo +) => { + const cellRole = info.getValue() + if (!cellRole) return null + const effectiveRole = info.row.original.effectiveRole + return ( + {cellRole} + ) +} diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 09e69befe7..b64a0b8cef 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -5,6 +5,7 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, + getOrgRole, setUserRole, useApiMutation, useApiQueryClient, @@ -15,7 +16,6 @@ import { useApiQuery } from '@oxide/api' import { Table, getActionsCol } from '@oxide/table' import { Access24Icon, - Badge, Button, EmptyMessage, PageHeader, @@ -23,8 +23,9 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' -import { groupBy, sortBy } from '@oxide/util' +import { groupBy, isTruthy, sortBy } from '@oxide/util' +import { RoleBadgeCell } from 'app/components/RoleBadgeCell' import { OrgAccessAddUserSideModal, OrgAccessEditUserSideModal } from 'app/forms/org-access' import { requireOrgParams, useRequiredParams } from 'app/hooks' @@ -54,6 +55,8 @@ type UserRow = { name: string siloRole: SiloRole | undefined orgRole: OrganizationRole | undefined + // all these types are the same but this is strictly more correct than using one + effectiveRole: SiloRole | OrganizationRole } const colHelper = createColumnHelper() @@ -70,12 +73,23 @@ export function OrgAccessPage() { const rows = useMemo( () => sortBy( - groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => ({ - id, - name: ras[0].name, - siloRole: ras.find((ra) => ra.roleSource === 'silo')?.roleName, - orgRole: ras.find((ra) => ra.roleSource === 'org')?.roleName, - })), + groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => { + const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName + const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName + const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName + + const roles = [siloRole, orgRole, projectRole].filter(isTruthy) + + return { + id, + name: ras[0].name, + siloRole, + orgRole, + projectRole, + // we know there has to be at least one + effectiveRole: getOrgRole(roles)!, + } + }), (row) => row.id ), [siloRows, orgRows] @@ -96,11 +110,11 @@ export function OrgAccessPage() { colHelper.accessor('name', { header: 'Name' }), colHelper.accessor('siloRole', { header: 'Silo role', - cell: (info) => (info.getValue() ? {info.getValue()} : null), + cell: RoleBadgeCell, }), colHelper.accessor('orgRole', { header: 'Org role', - cell: (info) => (info.getValue() ? {info.getValue()} : null), + cell: RoleBadgeCell, }), getActionsCol((row: UserRow) => row.orgRole diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index f5de55c028..4981fb56ed 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -5,6 +5,7 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, + getProjectRole, setUserRole, useApiMutation, useApiQueryClient, @@ -15,7 +16,6 @@ import { useApiQuery } from '@oxide/api' import { Table, getActionsCol } from '@oxide/table' import { Access24Icon, - Badge, Button, EmptyMessage, PageHeader, @@ -23,8 +23,9 @@ import { TableActions, TableEmptyBox, } from '@oxide/ui' -import { groupBy, sortBy } from '@oxide/util' +import { groupBy, isTruthy, sortBy } from '@oxide/util' +import { RoleBadgeCell } from 'app/components/RoleBadgeCell' import { ProjectAccessAddUserSideModal, ProjectAccessEditUserSideModal, @@ -60,6 +61,8 @@ type UserRow = { siloRole: SiloRole | undefined orgRole: OrganizationRole | undefined projectRole: ProjectRole | undefined + // all these types are the same but this is strictly more correct than using one + effectiveRole: SiloRole | OrganizationRole | ProjectRole } const colHelper = createColumnHelper() @@ -79,13 +82,23 @@ export function ProjectAccessPage() { const rows = useMemo( () => sortBy( - groupBy(siloRows.concat(orgRows, projectRows), (u) => u.id).map(([id, ras]) => ({ - id, - name: ras[0].name, - siloRole: ras.find((ra) => ra.roleSource === 'silo')?.roleName, - orgRole: ras.find((ra) => ra.roleSource === 'org')?.roleName, - projectRole: ras.find((ra) => ra.roleSource === 'project')?.roleName, - })), + groupBy(siloRows.concat(orgRows, projectRows), (u) => u.id).map(([id, ras]) => { + const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName + const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName + const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName + + const roles = [siloRole, orgRole, projectRole].filter(isTruthy) + + return { + id, + name: ras[0].name, + siloRole, + orgRole, + projectRole, + // we know there has to be at least one + effectiveRole: getProjectRole(roles)!, + } + }), (row) => row.id ), [siloRows, orgRows, projectRows] @@ -106,15 +119,15 @@ export function ProjectAccessPage() { colHelper.accessor('name', { header: 'Name' }), colHelper.accessor('siloRole', { header: 'Silo role', - cell: (info) => (info.getValue() ? {info.getValue()} : null), + cell: RoleBadgeCell, }), colHelper.accessor('orgRole', { header: 'Org role', - cell: (info) => (info.getValue() ? {info.getValue()} : null), + cell: RoleBadgeCell, }), colHelper.accessor('projectRole', { header: 'Project role', - cell: (info) => (info.getValue() ? {info.getValue()} : null), + cell: RoleBadgeCell, }), getActionsCol((row: UserRow) => row.projectRole From d9bcb23e50edb9e4bf467f51ab3467d296d8248f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 15:57:30 -0500 Subject: [PATCH 08/17] make e2e tests pass --- app/pages/__tests__/org-access.e2e.ts | 30 +++++++++++++++++------ app/pages/__tests__/project-access.e2e.ts | 24 ++++++++++++------ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/app/pages/__tests__/org-access.e2e.ts b/app/pages/__tests__/org-access.e2e.ts index c8d3e8dd12..50cc0306db 100644 --- a/app/pages/__tests__/org-access.e2e.ts +++ b/app/pages/__tests__/org-access.e2e.ts @@ -10,8 +10,18 @@ test('Click through org access page', async ({ page }) => { // page is there, we see user 1 and 2 but not 3 await page.click('role=link[name*="Access & IAM"]') await expectVisible(page, ['role=heading[name*="Access & IAM"]']) - await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) - await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'viewer' }) + await expectRowVisible(table, { + ID: 'user-1', + Name: 'Hannah Arendt', + 'Silo role': 'admin', + 'Org role': '', + }) + await expectRowVisible(table, { + ID: 'user-2', + Name: 'Hans Jonas', + 'Silo role': '', + 'Org role': 'viewer', + }) await expectNotVisible(page, ['role=cell[name="user-3"]']) // Add user 2 as collab @@ -20,11 +30,12 @@ test('Click through org access page', async ({ page }) => { await page.click('role=button[name="User"]') // only users not already on the org should be visible - await expectNotVisible(page, [ + await expectNotVisible(page, ['role=option[name="Hans Jonas"]']) + await expectVisible(page, [ 'role=option[name="Hannah Arendt"]', - 'role=option[name="Hans Jonas"]', + 'role=option[name="Jacob Klein"]', + 'role=option[name="Simone de Beauvoir"]', ]) - await expectVisible(page, ['role=option[name="Jacob Klein"]']) await page.click('role=option[name="Jacob Klein"]') @@ -39,7 +50,12 @@ test('Click through org access page', async ({ page }) => { await page.click('role=button[name="Add user"]') // User 3 shows up in the table - await expectRowVisible(table, { ID: 'user-3', Name: 'Jacob Klein', Role: 'collaborator' }) + await expectRowVisible(table, { + ID: 'user-3', + Name: 'Jacob Klein', + 'Silo role': '', + 'Org role': 'collaborator', + }) // now change user 3's role from collab to viewer await page @@ -55,7 +71,7 @@ test('Click through org access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(table, { ID: 'user-3', Role: 'viewer' }) + await expectRowVisible(table, { ID: 'user-3', 'Org role': 'viewer' }) // now delete user 2 await page diff --git a/app/pages/__tests__/project-access.e2e.ts b/app/pages/__tests__/project-access.e2e.ts index 5798edd05d..c4f33e2322 100644 --- a/app/pages/__tests__/project-access.e2e.ts +++ b/app/pages/__tests__/project-access.e2e.ts @@ -9,9 +9,17 @@ test('Click through project access page', async ({ page }) => { // page is there, we see user 1-3 but not 4 await expectVisible(page, ['role=heading[name*="Access & IAM"]']) const table = page.locator('table') - await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) - await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'viewer' }) - await expectRowVisible(table, { ID: 'user-3', Name: 'Jacob Klein', Role: 'collaborator' }) + await expectRowVisible(table, { + ID: 'user-1', + Name: 'Hannah Arendt', + 'Silo role': 'admin', + }) + await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', 'Org role': 'viewer' }) + await expectRowVisible(table, { + ID: 'user-3', + Name: 'Jacob Klein', + 'Project role': 'collaborator', + }) await expectNotVisible(page, ['role=cell[name="user-4"]']) // Add user 4 as collab @@ -20,12 +28,12 @@ test('Click through project access page', async ({ page }) => { await page.click('role=button[name="User"]') // only users not already on the project should be visible - await expectNotVisible(page, [ + await expectNotVisible(page, ['role=option[name="Jacob Klein"]']) + await expectVisible(page, [ 'role=option[name="Hannah Arendt"]', 'role=option[name="Hans Jonas"]', - 'role=option[name="Jacob Klein"]', + 'role=option[name="Simone de Beauvoir"]', ]) - await expectVisible(page, ['role=option[name="Simone de Beauvoir"]']) await page.click('role=option[name="Simone de Beauvoir"]') @@ -43,7 +51,7 @@ test('Click through project access page', async ({ page }) => { await expectRowVisible(table, { ID: 'user-4', Name: 'Simone de Beauvoir', - Role: 'collaborator', + 'Project role': 'collaborator', }) // now change user 4 role from collab to viewer @@ -60,7 +68,7 @@ test('Click through project access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(table, { ID: 'user-4', Role: 'viewer' }) + await expectRowVisible(table, { ID: 'user-4', 'Project role': 'viewer' }) // now delete user 3. has to be 3 or 4 because they're the only ones that come // from the project policy From 9e0095de53fa0ee32f55f1ba45ffb24d485aa9a8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 16:02:56 -0500 Subject: [PATCH 09/17] add test for new table behavior to e2e --- app/pages/__tests__/org-access.e2e.ts | 14 ++++++++++++ app/pages/__tests__/project-access.e2e.ts | 27 ++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/app/pages/__tests__/org-access.e2e.ts b/app/pages/__tests__/org-access.e2e.ts index 50cc0306db..c92cb87d05 100644 --- a/app/pages/__tests__/org-access.e2e.ts +++ b/app/pages/__tests__/org-access.e2e.ts @@ -81,4 +81,18 @@ test('Click through org access page', async ({ page }) => { await expectVisible(page, ['role=cell[name=user-2]']) await page.click('role=menuitem[name="Delete"]') await expectNotVisible(page, ['role=cell[name=user-2]']) + + // now add an org role to user 1, who currently only has silo role + await page.click('role=button[name="Add user to organization"]') + await page.click('role=button[name="User"]') + await page.click('role=option[name="Hannah Arendt"]') + await page.click('role=button[name="Role"]') + await page.click('role=option[name="Viewer"]') + await page.click('role=button[name="Add user"]') + await expectRowVisible(table, { + ID: 'user-1', + Name: 'Hannah Arendt', + 'Silo role': 'admin', + 'Org role': 'viewer', + }) }) diff --git a/app/pages/__tests__/project-access.e2e.ts b/app/pages/__tests__/project-access.e2e.ts index c4f33e2322..626700a899 100644 --- a/app/pages/__tests__/project-access.e2e.ts +++ b/app/pages/__tests__/project-access.e2e.ts @@ -13,11 +13,21 @@ test('Click through project access page', async ({ page }) => { ID: 'user-1', Name: 'Hannah Arendt', 'Silo role': 'admin', + 'Org role': '', + 'Project role': '', + }) + await expectRowVisible(table, { + ID: 'user-2', + Name: 'Hans Jonas', + 'Silo role': '', + 'Org role': 'viewer', + 'Project role': '', }) - await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', 'Org role': 'viewer' }) await expectRowVisible(table, { ID: 'user-3', Name: 'Jacob Klein', + 'Silo role': '', + 'Org role': '', 'Project role': 'collaborator', }) await expectNotVisible(page, ['role=cell[name="user-4"]']) @@ -79,4 +89,19 @@ test('Click through project access page', async ({ page }) => { await expectVisible(page, ['role=cell[name=user-3]']) await page.click('role=menuitem[name="Delete"]') await expectNotVisible(page, ['role=cell[name=user-3]']) + + // now add a project role to user 1, who currently only has silo role + await page.click('role=button[name="Add user to project"]') + await page.click('role=button[name="User"]') + await page.click('role=option[name="Hannah Arendt"]') + await page.click('role=button[name="Role"]') + await page.click('role=option[name="Viewer"]') + await page.click('role=button[name="Add user"]') + await expectRowVisible(table, { + ID: 'user-1', + Name: 'Hannah Arendt', + 'Silo role': 'admin', + 'Org role': '', + 'Project role': 'viewer', + }) }) From 3c7ef8ca3622c3186adbe3d682ca5f8ea1ee26a8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 16:42:01 -0500 Subject: [PATCH 10/17] point to omicron main now that the change is merged --- OMICRON_VERSION | 2 +- libs/api/__generated__/OMICRON_VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 7173534e8f..6e37e9eee9 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -69ae4c1204d2ccfa93c4cd4d557becf75aa372b4 +eef65672cf65c5585f3c0340cd0f719009acdcf2 diff --git a/libs/api/__generated__/OMICRON_VERSION b/libs/api/__generated__/OMICRON_VERSION index cc08cfd056..c7338b7668 100644 --- a/libs/api/__generated__/OMICRON_VERSION +++ b/libs/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -69ae4c1204d2ccfa93c4cd4d557becf75aa372b4 +eef65672cf65c5585f3c0340cd0f719009acdcf2 From 22d37b1d94fe0fe566a05624a18005a4d8b9bb80 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 12 Aug 2022 18:21:17 -0500 Subject: [PATCH 11/17] disable change and delete instead of removing --- app/pages/OrgAccessPage.tsx | 43 +++++++++---------- .../project/access/ProjectAccessPage.tsx | 43 +++++++++---------- 2 files changed, 42 insertions(+), 44 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index b64a0b8cef..86a44ebab1 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -116,28 +116,27 @@ export function OrgAccessPage() { header: 'Org role', cell: RoleBadgeCell, }), - getActionsCol((row: UserRow) => - row.orgRole - ? [ - { - label: 'Change role', - onActivate: () => setEditingUserRow(row), - }, - // TODO: only show if you have permission to do this - { - label: 'Delete', - onActivate() { - // TODO: confirm delete - updatePolicy.mutate({ - ...orgParams, - // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, orgPolicy!), - }) - }, - }, - ] - : [] - ), + // TODO: tooltips on disabled elements explaining why + getActionsCol((row: UserRow) => [ + { + label: 'Change role', + onActivate: () => setEditingUserRow(row), + disabled: !row.orgRole, + }, + // TODO: only show if you have permission to do this + { + label: 'Delete', + onActivate() { + // TODO: confirm delete + updatePolicy.mutate({ + ...orgParams, + // we know policy is there, otherwise there's no row to display + body: setUserRole(row.id, null, orgPolicy!), + }) + }, + disabled: !row.orgRole, + }, + ]), ], [orgPolicy, orgParams, updatePolicy] ) diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index 4981fb56ed..ed34421b8e 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -129,28 +129,27 @@ export function ProjectAccessPage() { header: 'Project role', cell: RoleBadgeCell, }), - getActionsCol((row: UserRow) => - row.projectRole - ? [ - { - label: 'Change role', - onActivate: () => setEditingUserRow(row), - }, - // TODO: only show if you have permission to do this - { - label: 'Delete', - onActivate() { - // TODO: confirm delete - updatePolicy.mutate({ - ...projectParams, - // we know policy is there, otherwise there's no row to display - body: setUserRole(row.id, null, projectPolicy!), - }) - }, - }, - ] - : [] - ), + // TODO: tooltips on disabled elements explaining why + getActionsCol((row: UserRow) => [ + { + label: 'Change role', + onActivate: () => setEditingUserRow(row), + disabled: !row.projectRole, + }, + // TODO: only show if you have permission to do this + { + label: 'Delete', + onActivate() { + // TODO: confirm delete + updatePolicy.mutate({ + ...projectParams, + // we know policy is there, otherwise there's no row to display + body: setUserRole(row.id, null, projectPolicy!), + }) + }, + disabled: !row.projectRole, + }, + ]), ], [projectPolicy, projectParams, updatePolicy] ) From ceabf51dabb9335dd4959d29dec934bcd8ee7b16 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 15 Aug 2022 16:36:44 -0500 Subject: [PATCH 12/17] use RT 8.5 to do less weird type stuff --- app/components/RoleBadgeCell.tsx | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/app/components/RoleBadgeCell.tsx b/app/components/RoleBadgeCell.tsx index d1f1bde580..f8a9a0ff17 100644 --- a/app/components/RoleBadgeCell.tsx +++ b/app/components/RoleBadgeCell.tsx @@ -1,13 +1,8 @@ -import type { CoreColumnDef } from '@tanstack/react-table' +import type { CellContext } from '@tanstack/react-table' import type { OrganizationRole, ProjectRole, SiloRole } from '@oxide/api' import { Badge } from '@oxide/ui' -// I think there are changes in RT 8.4+ that make this less ridiculous -type CellInfo = Parameters< - Exclude['cell'], string | undefined> ->[0] - type Role = SiloRole | OrganizationRole | ProjectRole /** @@ -17,8 +12,8 @@ type Role = SiloRole | OrganizationRole | ProjectRole * because it is the "stronger" role, i.e., it strictly includes the perms on * viewer. So collab is highlighted as the "effective" role. */ -export const RoleBadgeCell = ( - info: CellInfo +export const RoleBadgeCell = ( + info: CellContext ) => { const cellRole = info.getValue() if (!cellRole) return null From 7dae6dd8cddb8430e568a9b64b4d14b5dd5dffbe Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 15 Aug 2022 17:26:06 -0500 Subject: [PATCH 13/17] less confusing row processing, sort by name instead of id --- app/pages/OrgAccessPage.tsx | 44 ++++++++---------- .../project/access/ProjectAccessPage.tsx | 46 +++++++++---------- 2 files changed, 42 insertions(+), 48 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 86a44ebab1..6171fab637 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -70,30 +70,26 @@ export function OrgAccessPage() { const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo') - const rows = useMemo( - () => - sortBy( - groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => { - const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName - const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName - const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName - - const roles = [siloRole, orgRole, projectRole].filter(isTruthy) - - return { - id, - name: ras[0].name, - siloRole, - orgRole, - projectRole, - // we know there has to be at least one - effectiveRole: getOrgRole(roles)!, - } - }), - (row) => row.id - ), - [siloRows, orgRows] - ) + const rows = useMemo(() => { + const users = groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => { + const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName + const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName + const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName + + const roles = [siloRole, orgRole, projectRole].filter(isTruthy) + + return { + id, + name: ras[0].name, + siloRole, + orgRole, + projectRole, + // we know there has to be at least one + effectiveRole: getOrgRole(roles)!, + } + }) + return sortBy(users, (u) => u.id) + }, [siloRows, orgRows]) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('organizationPolicyUpdate', { diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index ed34421b8e..f26c2ab208 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -79,30 +79,28 @@ export function ProjectAccessPage() { const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo') const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org') const projectRows = useUserRows(projectPolicy?.roleAssignments, 'project') - const rows = useMemo( - () => - sortBy( - groupBy(siloRows.concat(orgRows, projectRows), (u) => u.id).map(([id, ras]) => { - const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName - const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName - const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName - - const roles = [siloRole, orgRole, projectRole].filter(isTruthy) - - return { - id, - name: ras[0].name, - siloRole, - orgRole, - projectRole, - // we know there has to be at least one - effectiveRole: getProjectRole(roles)!, - } - }), - (row) => row.id - ), - [siloRows, orgRows, projectRows] - ) + const rows = useMemo(() => { + const users = groupBy(siloRows.concat(orgRows, projectRows), (u) => u.id).map( + ([id, ras]) => { + const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName + const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName + const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName + + const roles = [siloRole, orgRole, projectRole].filter(isTruthy) + + return { + id, + name: ras[0].name, + siloRole, + orgRole, + projectRole, + // we know there has to be at least one + effectiveRole: getProjectRole(roles)!, + } + } + ) + return sortBy(users, (u) => u.name) + }, [siloRows, orgRows, projectRows]) const queryClient = useApiQueryClient() const updatePolicy = useApiMutation('projectPolicyUpdate', { From 2496fd08bbefde5bb3ede9b95581b2457ff9dc05 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 15 Aug 2022 17:28:38 -0500 Subject: [PATCH 14/17] whoops, orgs have no project role --- app/pages/OrgAccessPage.tsx | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 6171fab637..7e6276d7a4 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -74,21 +74,19 @@ export function OrgAccessPage() { const users = groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => { const siloRole = ras.find((ra) => ra.roleSource === 'silo')?.roleName const orgRole = ras.find((ra) => ra.roleSource === 'org')?.roleName - const projectRole = ras.find((ra) => ra.roleSource === 'project')?.roleName - const roles = [siloRole, orgRole, projectRole].filter(isTruthy) + const roles = [siloRole, orgRole].filter(isTruthy) return { id, name: ras[0].name, siloRole, orgRole, - projectRole, // we know there has to be at least one effectiveRole: getOrgRole(roles)!, } }) - return sortBy(users, (u) => u.id) + return sortBy(users, (u) => u.name) }, [siloRows, orgRows]) const queryClient = useApiQueryClient() From a3788c44ae2390e79d4ba4e35d1a427b153c4b9f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 15 Aug 2022 18:11:58 -0500 Subject: [PATCH 15/17] slightly easier to understand roles nonsense --- app/pages/OrgAccessPage.tsx | 4 +-- .../project/access/ProjectAccessPage.tsx | 4 +-- libs/api/roles.spec.ts | 34 +++++++------------ libs/api/roles.ts | 16 ++++----- 4 files changed, 23 insertions(+), 35 deletions(-) diff --git a/app/pages/OrgAccessPage.tsx b/app/pages/OrgAccessPage.tsx index 7e6276d7a4..04df4996e2 100644 --- a/app/pages/OrgAccessPage.tsx +++ b/app/pages/OrgAccessPage.tsx @@ -5,7 +5,7 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, - getOrgRole, + getEffectiveOrgRole, setUserRole, useApiMutation, useApiQueryClient, @@ -83,7 +83,7 @@ export function OrgAccessPage() { siloRole, orgRole, // we know there has to be at least one - effectiveRole: getOrgRole(roles)!, + effectiveRole: getEffectiveOrgRole(roles)!, } }) return sortBy(users, (u) => u.name) diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index f26c2ab208..f95c3a7348 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -5,7 +5,7 @@ import type { LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, - getProjectRole, + getEffectiveProjectRole, setUserRole, useApiMutation, useApiQueryClient, @@ -95,7 +95,7 @@ export function ProjectAccessPage() { orgRole, projectRole, // we know there has to be at least one - effectiveRole: getProjectRole(roles)!, + effectiveRole: getEffectiveProjectRole(roles)!, } } ) diff --git a/libs/api/roles.spec.ts b/libs/api/roles.spec.ts index 94a7bf1609..36d691e4e9 100644 --- a/libs/api/roles.spec.ts +++ b/libs/api/roles.spec.ts @@ -1,41 +1,31 @@ import type { ProjectRolePolicy } from './__generated__/Api' import { - getMainRole, - getOrgRole, - getProjectRole, + getEffectiveOrgRole, + getEffectiveProjectRole, orgRoleOrder, projectRoleOrder, setUserRole, } from './roles' -// not strictly necessary since we're testing the other functions, but it's nice -// to show how it works -describe('getMainRole', () => { - it('uses role order to choose which role to return', () => { - expect(getMainRole({ x: 0, y: 3 })(['x', 'y'])).toEqual('x') - expect(getMainRole({ x: 4, y: 1 })(['x', 'y'])).toEqual('y') - }) -}) - -describe('getMainProjectRole', () => { - it('returns null when the list of role assignments is empty', () => { - expect(getProjectRole([])).toBeNull() - expect(getOrgRole([])).toBeNull() +describe('getEffectiveProjectRole and getEffectiveOrgRole', () => { + it('returns falsy when the list of role assignments is empty', () => { + expect(getEffectiveProjectRole([])).toBeFalsy() + expect(getEffectiveOrgRole([])).toBeFalsy() }) it('returns the strongest role when there are multiple roles, regardless of policy order', () => { - expect(getProjectRole(['admin', 'collaborator'])).toEqual('admin') - expect(getProjectRole(['collaborator', 'admin'])).toEqual('admin') + expect(getEffectiveProjectRole(['admin', 'collaborator'])).toEqual('admin') + expect(getEffectiveProjectRole(['collaborator', 'admin'])).toEqual('admin') - expect(getOrgRole(['collaborator', 'viewer'])).toEqual('collaborator') - expect(getOrgRole(['viewer', 'collaborator'])).toEqual('collaborator') + expect(getEffectiveOrgRole(['collaborator', 'viewer'])).toEqual('collaborator') + expect(getEffectiveOrgRole(['viewer', 'collaborator'])).toEqual('collaborator') }) it("type errors when passed a role that's not in the enum", () => { // @ts-expect-error - getProjectRole(['fake!']) + getEffectiveProjectRole(['fake!']) // @ts-expect-error - getOrgRole(['fake!']) + getEffectiveOrgRole(['fake!']) }) }) diff --git a/libs/api/roles.ts b/libs/api/roles.ts index f342579385..e3c15daa29 100644 --- a/libs/api/roles.ts +++ b/libs/api/roles.ts @@ -10,15 +10,10 @@ import { sortBy } from '@oxide/util' import { useApiQuery } from '.' import type { IdentityType, OrganizationRole, ProjectRole } from './__generated__/Api' -/** Given a role order and a list of roles, get the one that sorts earliest */ -export const getMainRole = - (roleOrder: Record) => - (userRoles: Role[]): Role | null => - userRoles.length > 0 ? sortBy(userRoles, (r) => roleOrder[r])[0] : null - /** Turn a role order record into a sorted array of strings. */ +// used for displaying lists of roles, like in a