lahirujayathilake commented on code in PR #524: URL: https://github.com/apache/airavata-custos/pull/524#discussion_r3591138799
########## web/src/features/core/roles/queries.ts: ########## @@ -0,0 +1,81 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +"use client"; + +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { + createRole, + listPrivilegeCatalog, + listRoleRows, + reconcileRolePrivileges, + updateRole, +} from "./api"; +import type { RoleInput, RoleRow } from "./schemas"; + +export const roleKeys = { + all: ["roles"] as const, + rows: () => [...roleKeys.all, "rows"] as const, + privileges: () => [...roleKeys.all, "privileges"] as const, +}; + +const DEFAULTS = { + staleTime: 30_000, + gcTime: 300_000, + refetchOnWindowFocus: false, +} as const; + +export function useRoleRows() { + return useQuery({ + queryKey: roleKeys.rows(), + queryFn: listRoleRows, + ...DEFAULTS, + }); +} + +export function usePrivilegeCatalog() { + return useQuery({ + queryKey: roleKeys.privileges(), + queryFn: listPrivilegeCatalog, + ...DEFAULTS, + }); +} + +export function useCreateRole() { + const client = useQueryClient(); + return useMutation({ + mutationFn: async (input: RoleInput) => { + const role = await createRole(input); + if (role.id) await reconcileRolePrivileges(role.id, [], input.privileges); + return role; + }, + onSuccess: () => client.invalidateQueries({ queryKey: roleKeys.all }), Review Comment: Use `onSettled` instead. `reconcileRolePrivileges` is not atomic. Partial failures leave cache stale. Valid for line 79 as well ########## web/src/features/core/roles/schemas.ts: ########## Review Comment: Lines - 21,23,29,37 These four already exist in features/core/identity/schemas.ts, import them instead ########## web/src/app/(portal)/admin/users/roles/RoleCard.tsx: ########## @@ -45,30 +42,19 @@ export function RoleCard({ role, memberCount }: { role: RoleRow; memberCount: nu <p className="mt-1 text-sm text-muted-foreground">{role.description}</p> </div> <Badge variant="secondary" className="shrink-0"> - {memberCount} {memberCount === 1 ? "member" : "members"} + {role.memberCount} {role.memberCount === 1 ? "member" : "members"} </Badge> </div> </CardHeader> <CardContent className="space-y-4"> <div className="border-t border-border" /> - <div> - <h4 className="mb-2 text-xs font-semibold uppercase tracking-wide text-muted-foreground"> - Effective Privileges - </h4> - {rwPermissions.length === 0 ? ( - <p className="text-sm text-muted-foreground">No privileges granted.</p> - ) : ( - <ul className="space-y-2"> - {rwPermissions.map((p) => ( - <li key={p.section} className="flex items-center justify-between text-sm"> - <span className="font-mono text-foreground">{p.section}</span> - <PermissionRW read={p.read} write={p.write} /> - </li> - ))} - </ul> - )} - </div> + <PermissionMatrixEditor + permissions={role.privileges} + catalog={role.privileges} + onTogglePermission={() => undefined} + editable={false} Review Comment: catalog is redundant here (defaults to permissions). Make the `onTogglePermission` optional in the `PermissionMatrixEditor` and drop it from here because read only card doesn't need a click handler ########## web/tests/fixtures/auth.ts: ########## @@ -79,6 +78,7 @@ export async function signInAs(page: Page, persona: Persona = "admin") { throw new Error("NEXTAUTH_SECRET must be set for the cookie-injection fixture"); } + const { encode } = await import("next-auth/jwt"); Review Comment: Why dynamic import here? ########## web/src/app/(portal)/admin/users/roles/RoleFormDialog.tsx: ########## @@ -140,57 +125,17 @@ export function RoleFormDialog({ <PermissionMatrixEditor permissions={permissions} + catalog={catalog} onTogglePermission={(key) => setPermissions((prev) => togglePermission(prev, key))} /> - - <div className="border-t border-border" /> - - <div className="space-y-2"> - <Label htmlFor="role-user-search">Assign to users (optional)</Label> - <Input - id="role-user-search" - type="search" - value={userSearch} - onChange={(e) => setUserSearch(e.target.value)} - placeholder="Search by username or email" - /> - <ul className="max-h-40 space-y-1 overflow-y-auto rounded-md border p-2"> - {matchingUsers.length === 0 ? ( - <li className="px-1 py-1 text-sm text-muted-foreground">No users match.</li> - ) : ( - matchingUsers.map((u) => { - const id = u.id ?? u.email ?? ""; - return ( - <li key={id}> - <label className="flex cursor-pointer items-center gap-2 rounded-sm px-1 py-1 text-sm hover:bg-muted"> - <input - type="checkbox" - checked={selectedUserIds.has(id)} - onChange={() => toggleUserSelected(id)} - className="size-4 rounded border-input" - /> - <span className="font-medium text-foreground">{fullNameFor(u)}</span> - <span className="text-xs text-muted-foreground">{u.email}</span> - </label> - </li> - ); - }) - )} - </ul> Review Comment: Was dropping "Assign to users" intentional? ########## web/src/shared/users-admin/permissions.ts: ########## @@ -15,38 +15,69 @@ // specific language governing permissions and limitations // under the License. -// Permission sections mirror the backend's privilege scopes (domain:resource, -// e.g. "core:allocations"), so each section can be granted read/write -// independently. Displayed verbatim as the effective-privilege key. -export const PERMISSION_SECTIONS = [ - "amie:packets", - "amie:replies", - "amie:unmapped", - "core:allocations", - "core:clusters", - "core:organizations", - "core:projects", - "core:traces", - "core:users", - "temp-account:accounts", -] as const; -export type PermissionSection = (typeof PERMISSION_SECTIONS)[number]; -export type PermissionKey = `${PermissionSection}:read` | `${PermissionSection}:write`; +export type PermissionKey = string; -export function rwStateFor(permissions: PermissionKey[]) { +type PermissionParts = { + section: string; + action: string; +}; + +function splitPermission(key: PermissionKey): PermissionParts { + const parts = key.split(":"); + const action = parts.pop() ?? key; + return { section: parts.join(":") || key, action }; +} + +export function permissionRowsFor( + permissions: PermissionKey[], + catalog: readonly PermissionKey[] = permissions, +) { const held = new Set(permissions); - return PERMISSION_SECTIONS.map((section) => ({ - section, - read: held.has(`${section}:read` as PermissionKey), - write: held.has(`${section}:write` as PermissionKey), + const keys = Array.from(new Set([...catalog, ...permissions])).sort(); + const rows = new Map< + string, + { + section: string; + actions: Array<{ action: string; key: PermissionKey; active: boolean }>; + } + >(); + + for (const key of keys) { + const { section, action } = splitPermission(key); + const row = rows.get(section) ?? { section, actions: [] }; + row.actions.push({ action, key, active: held.has(key) }); + rows.set(section, row); + } + + return Array.from(rows.values()).map((row) => ({ + ...row, + actions: row.actions.sort((a, b) => { + const order = ["read", "write", "grant", "manage"]; Review Comment: Move `order` out of the comparator. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
