feat: add identity management with RBAC permission model#914
feat: add identity management with RBAC permission model#914lashinijay wants to merge 17 commits into
Conversation
- Add Users, Roles, and Groups pages under Identities section in console - Implement identity controller and routes proxying Thunder user/role/group APIs - Add invite user flow via Thunder USER_ONBOARDING flow (returns copyable invite link) - Add server-side pagination (offset/limit) with TablePagination component for all three pages - Fix Thunder list response decoding to use totalResults instead of total - Replace browser confirm() dialogs with themed ConfirmationDialog for delete actions - Wire RBAC permission model across all existing routes using HandleFuncWithValidationAndAuthz - Add RBAC permissions and predefined roles definitions - Add amp-system-client and related bootstrap scripts for Thunder OAuth app provisioning - Update Helm chart bootstrap to include email attribute in engineer user schema Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds typed RBAC permissions and middleware, applies RBAC enforcement to HTTP routes, integrates Thunder identity client and controller, registers authenticated identity routes, implements frontend identities UI (types, API, hooks, pages), and bootstraps AMP resource-server permissions with deployment scripts. ChangesRBAC Authorization System & Identity Management
Estimated code review effort: Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
console/apps/web-ui/public/config.js (1)
36-36: ⚡ Quick winRefactor the self-comparison to a clearer placeholder pattern.
The expression
'true' === 'true'compares a string literal to itself, which always evaluates totrue. While this appears to be a placeholder pattern where the first'true'would be replaced at build/runtime, the self-comparison creates confusion and triggers static analysis warnings. Consider using a more explicit placeholder pattern, such as a distinct template variable (e.g.,'%%DISABLE_AUTH%%' === 'true') or an environment-driven approach that doesn't rely on in-place string replacement of identical literals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/apps/web-ui/public/config.js` at line 36, The disableAuth assignment uses a tautological self-comparison ('true' === 'true') which is confusing and triggers linters; change the expression to use a clear placeholder or env-driven value such as comparing a distinct template token or environment variable (e.g., replace the left-hand literal with a build-time placeholder token like '%%DISABLE_AUTH%%' or read from process.env) so that disableAuth is computed as ('%%DISABLE_AUTH%%' === 'true') or an equivalent env check; update the config.js disableAuth line and any build/runtime replacement logic to ensure the placeholder is substituted correctly.deployments/scripts/register-amp-resources.sh (1)
40-51: 💤 Low valueSuppressed curl errors may hide network issues.
The
2>/dev/nullon the curl commands silently discards connection errors and SSL warnings. When debugging failures, this makes it harder to diagnose network problems vs API errors.💡 Suggested improvement
api_call() { local method="$1" endpoint="$2" data="${3:-}" + local curl_stderr if [[ -z "$data" ]]; then - curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \ + curl_stderr=$(mktemp) + result=$(curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \ -H "Content-Type: application/json" \ - -H "Authorization: Bearer $TOKEN" 2>/dev/null || echo -e "\n000" + -H "Authorization: Bearer $TOKEN" 2>"$curl_stderr") || { cat "$curl_stderr" >&2; rm -f "$curl_stderr"; echo -e "\n000"; return; } + rm -f "$curl_stderr" + echo "$result" else - curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \ + curl_stderr=$(mktemp) + result=$(curl -s -w "\n%{http_code}" -X "$method" "$THUNDER_URL$endpoint" \ -H "Content-Type: application/json" \ -H "Authorization: Bearer $TOKEN" \ - -d "$data" 2>/dev/null || echo -e "\n000" + -d "$data" 2>"$curl_stderr") || { cat "$curl_stderr" >&2; rm -f "$curl_stderr"; echo -e "\n000"; return; } + rm -f "$curl_stderr" + echo "$result" fi }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployments/scripts/register-amp-resources.sh` around lines 40 - 51, In the api_call function, stop suppressing curl stderr so network/SSL errors are visible: remove the "2>/dev/null" from both curl invocations and instead run curl with flags like --fail --show-error (or capture stderr into a variable) and surface that stderr when the call fails; update the two curl invocations inside api_call (the branches with and without -d) to preserve and/or log curl's error output and return a non-zero status or meaningful message instead of silently emitting "\n000".deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml (1)
159-159: 💤 Low valueFragile string check for schema attribute detection.
The grep
'"email"'may match email values elsewhere in the response (e.g., user emails in a paginated list), causing the patch to be skipped even when the schema lacks the attribute. Consider parsing the JSON schema structure specifically.💡 Suggested improvement
- if [[ -n "$SCHEMA_ID" ]] && ! echo "$BODY" | grep -q '"email"'; then + HAS_EMAIL_ATTR=$(echo "$BODY" | python3 -c "import sys,json; schemas=json.load(sys.stdin); schemas=schemas if isinstance(schemas,list) else schemas.get('schemas',[]); s=[s for s in schemas if s.get('name')=='{{ .Values.thunder.bootstrap.userSchema.name }}']; print('yes' if s and 'email' in s[0].get('schema',{}) else 'no')" 2>/dev/null || echo 'no') + if [[ -n "$SCHEMA_ID" ]] && [[ "$HAS_EMAIL_ATTR" != "yes" ]]; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml` at line 159, The current string check uses grep '"email"' against BODY which can match values elsewhere; instead parse the JSON in BODY to detect the schema attribute explicitly. Replace the grep check around SCHEMA_ID/BODY with a jq-based test, e.g. use echo "$BODY" | jq -e '(.attributes[]?.name == "email") or (.properties.email != null)' >/dev/null to determine presence of the email attribute and branch accordingly; update the conditional that references SCHEMA_ID and BODY to use that jq test so only the schema's attribute list/properties is inspected.console/workspaces/pages/identities/src/UsersPage.tsx (1)
67-75: ⚡ Quick winMake delete confirmation callback explicitly async-safe.
Returning
deleteUser(...)directly fromonConfirmcan leak rejected promises depending on dialog callback handling.Proposed fix
const handleDelete = (user: ThunderUser) => { addConfirmation({ @@ - onConfirm: () => deleteUser({ orgName: orgId, userId: user.id }), + onConfirm: async () => { + try { + await deleteUser({ orgName: orgId, userId: user.id }); + } catch { + // rely on global mutation error handler + } + }, }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/identities/src/UsersPage.tsx` around lines 67 - 75, The onConfirm currently returns deleteUser(...) directly which can leak rejected promises; update the onConfirm in handleDelete to be an explicit async function that awaits deleteUser({ orgName: orgId, userId: user.id }) and catches errors (e.g., async () => { try { await deleteUser(...) } catch (err) { /* handle/log/notify error */ } }) so the dialog callback is async-safe; reference handleDelete and the deleteUser call when making this change.console/workspaces/pages/identities/src/RolesPage.tsx (1)
67-75: ⚡ Quick winGuard delete mutation errors inside confirmation callback.
onConfirmcurrently returnsdeleteRole(...)directly. If the mutation rejects and the dialog utility does not await/catch it, this can surface as an unhandled rejection.Proposed fix
const handleDelete = (role: ThunderRole) => { addConfirmation({ title: "Delete Role", description: `Are you sure you want to delete "${role.name}"? This action cannot be undone.`, confirmButtonText: "Delete", confirmButtonColor: "error", confirmButtonIcon: <Trash size={16} />, - onConfirm: () => deleteRole({ orgName: orgId, roleId: role.id }), + onConfirm: async () => { + try { + await deleteRole({ orgName: orgId, roleId: role.id }); + } catch { + // let global mutation error handling surface this + } + }, }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/identities/src/RolesPage.tsx` around lines 67 - 75, The onConfirm callback passed to addConfirmation in handleDelete currently returns deleteRole(...) directly which can cause unhandled rejections; modify the onConfirm to call an async wrapper that awaits deleteRole({ orgName: orgId, roleId: role.id }) inside a try/catch (or attaches a .catch handler) and surface/log errors appropriately so any rejection is handled within the confirmation flow (update the onConfirm defined in handleDelete to be async () => { try { await deleteRole(...) } catch (err) { /* handle/log */ } } or equivalent).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Line 660: The code in doRequest ignores the error returned by
io.ReadAll(resp.Body) when reading the response into the variable body; change
this to capture and handle the error (e.g., body, err := io.ReadAll(resp.Body)),
close resp.Body if not already closed, and return or wrap a clear error
(including HTTP status and the read error) instead of proceeding to decoding;
update any callers to expect this error path so truncated/failed responses are
surfaced immediately.
In `@agent-manager-service/controllers/identity_controller.go`:
- Around line 439-443: Clamp negative offset values to zero in the paginated
handlers that currently only normalize limit: after calling getIntQueryParam(r,
"offset", 0) in the GetGroupMembers and ListRoles handlers (and any other
paginated handlers similar to the shown snippet), add a check like if offset < 0
{ offset = 0 } so negative offsets are normalized before use; keep the existing
limit validation (if limit <= 0 || limit > 100 { limit = 20 }) unchanged and
ensure you update every occurrence matching this pattern (the offset variable
returned from getIntQueryParam) so invalid negative offsets cannot propagate
upstream.
- Around line 180-181: Remove raw user identifiers from error logs in the
CreateUser handling code: change the log.Error calls that currently include
"username" (and "email" in the other occurrence) so they only log a generic
message plus the error (e.g., log.Error("CreateUser failed", "error", err));
update both the occurrence shown (log.Error("CreateUser failed", "username",
body.Username, "error", err)) and the similar one at the other location (lines
264-265) to drop body.Username/body.Email; if extra correlation is needed, log a
non-identifying request ID or hashed identifier instead of the raw
username/email.
- Around line 66-71: The current use of sync.Once (rootOUOnce) in resolveOuID
caches both successes and failures permanently; change the logic so only
successful OU lookups are cached. Replace sync.Once with a sync.Mutex (or
RWMutex) and fields rootOUID/rootOUErr such that resolveOuID acquires the lock,
returns cached rootOUID if non-empty, otherwise attempts the Thunder Identity
lookup; on success set rootOUID (and clear rootOUErr) and persist for future
calls, but on failure do NOT persist rootOUErr (or persist it only transiently
with retry logic) — simply return the error so callers can retry. Update any
code referencing rootOUOnce to use the new mutex-based pattern (identify
resolveOuID, rootOUID, rootOUErr, and rootOUOnce in the diff) and add minimal
unit tests to ensure a failed first lookup does not block subsequent successful
retries.
In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 179-193: The onSuccess handlers in useAddGroupMembers and
useRemoveGroupMembers only invalidate the 'identity-group-members' query,
leaving useGetUserGroups caches stale; update both handlers (the onSuccess in
the useApiMutation for mutationFn addGroupMembers and removeGroupMembers) to
also invalidate user-group caches by calling queryClient.invalidateQueries for
the user-groups key (e.g. 'identity-user-groups' or your useGetUserGroups
queryKey) for each affected userId from the mutation args (body.userIds) so both
group-member and per-user group lists are refreshed after add/remove operations.
In `@console/workspaces/pages/identities/package.json`:
- Around line 4-7: The package.json declares "type": "module" while also
including a "require" field that points to an ESM file, which causes
ERR_REQUIRE_ESM for CommonJS consumers; open the package.json where "type":
"module", "import": "./dist/index.js", "require": "./dist/index.js", and
"types": "dist/index.d.ts" are defined and remove the "require" entry (and any
other "require" mappings in similar page packages) so only "import" remains for
ESM packages, ensuring CommonJS consumers don't get an invalid require mapping.
In `@console/workspaces/pages/identities/src/GroupsPage.tsx`:
- Around line 107-112: The empty-state check should distinguish between "no
groups at all" and "no groups on the current page" so pagination is not hidden
when total > 0; update the render logic in GroupsPage to use both groups and
total (the variables named groups and total) and only show the "No groups yet"
ListingTable.EmptyState when total === 0, while when groups.length === 0 &&
total > 0 render a page-empty message (or a different ListingTable.EmptyState
variant) but keep the pagination component visible; adjust both occurrences that
currently check groups.length (the two ListingTable.EmptyState blocks) to follow
this rule so users can navigate back to previous pages.
In `@console/workspaces/pages/identities/src/RoleCreatePage.tsx`:
- Around line 52-65: handleSubmit currently awaits createRole directly which can
cause unhandled promise rejections; wrap the createRole call in a try/catch
inside handleSubmit so errors are caught and prevent bubbling, only call
navigate(rolesPath) on success, and keep existing name/description handling.
Specifically, in handleSubmit wrap await createRole({...}) in try { await
createRole(...) ; navigate(rolesPath); } catch (err) { /* no-op since
createError/snackbar already handles it or optionally log */ } so the mutation
error is caught and does not produce an unhandled rejection.
In `@console/workspaces/pages/identities/src/UserCreatePage.tsx`:
- Around line 64-83: The submit handler handleSubmit awaits createUser (the
mutateAsync call) but doesn't catch rejections, causing possible unhandled
promise rejections; wrap the await createUser(...) call in a try/catch so
failures are caught before calling navigate(usersPath) — on success navigate, on
error exit the handler (optionally log or let the mutation's onError/snackbar
handle user-visible errors) to prevent the rejected promise from escaping;
update handleSubmit accordingly so createUser, mutateAsync, and navigate are
referenced and the navigation only happens when createUser succeeds.
In `@console/workspaces/pages/identities/src/UserInvitePage.tsx`:
- Around line 61-77: handleSubmit currently awaits inviteUser directly which can
lead to unhandled promise rejections; wrap the inviteUser call in a try/catch
inside handleSubmit, keep the existing validation and
setEmailError/setInviteLink logic, and in the catch block set an appropriate
error state or log the error (and avoid swallowing it) so failures are handled
gracefully; reference the handleSubmit function and the inviteUser call where
setInviteLink is currently invoked.
---
Nitpick comments:
In `@console/apps/web-ui/public/config.js`:
- Line 36: The disableAuth assignment uses a tautological self-comparison
('true' === 'true') which is confusing and triggers linters; change the
expression to use a clear placeholder or env-driven value such as comparing a
distinct template token or environment variable (e.g., replace the left-hand
literal with a build-time placeholder token like '%%DISABLE_AUTH%%' or read from
process.env) so that disableAuth is computed as ('%%DISABLE_AUTH%%' === 'true')
or an equivalent env check; update the config.js disableAuth line and any
build/runtime replacement logic to ensure the placeholder is substituted
correctly.
In `@console/workspaces/pages/identities/src/RolesPage.tsx`:
- Around line 67-75: The onConfirm callback passed to addConfirmation in
handleDelete currently returns deleteRole(...) directly which can cause
unhandled rejections; modify the onConfirm to call an async wrapper that awaits
deleteRole({ orgName: orgId, roleId: role.id }) inside a try/catch (or attaches
a .catch handler) and surface/log errors appropriately so any rejection is
handled within the confirmation flow (update the onConfirm defined in
handleDelete to be async () => { try { await deleteRole(...) } catch (err) { /*
handle/log */ } } or equivalent).
In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Around line 67-75: The onConfirm currently returns deleteUser(...) directly
which can leak rejected promises; update the onConfirm in handleDelete to be an
explicit async function that awaits deleteUser({ orgName: orgId, userId: user.id
}) and catches errors (e.g., async () => { try { await deleteUser(...) } catch
(err) { /* handle/log/notify error */ } }) so the dialog callback is async-safe;
reference handleDelete and the deleteUser call when making this change.
In
`@deployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yaml`:
- Line 159: The current string check uses grep '"email"' against BODY which can
match values elsewhere; instead parse the JSON in BODY to detect the schema
attribute explicitly. Replace the grep check around SCHEMA_ID/BODY with a
jq-based test, e.g. use echo "$BODY" | jq -e '(.attributes[]?.name == "email")
or (.properties.email != null)' >/dev/null to determine presence of the email
attribute and branch accordingly; update the conditional that references
SCHEMA_ID and BODY to use that jq test so only the schema's attribute
list/properties is inspected.
In `@deployments/scripts/register-amp-resources.sh`:
- Around line 40-51: In the api_call function, stop suppressing curl stderr so
network/SSL errors are visible: remove the "2>/dev/null" from both curl
invocations and instead run curl with flags like --fail --show-error (or capture
stderr into a variable) and surface that stderr when the call fails; update the
two curl invocations inside api_call (the branches with and without -d) to
preserve and/or log curl's error output and return a non-zero status or
meaningful message instead of silently emitting "\n000".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aaa5ed89-e020-4388-9685-63104e336017
⛔ Files ignored due to path filters (1)
console/common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
agent-manager-service/api/agent_config_routes.goagent-manager-service/api/agent_routes.goagent-manager-service/api/agent_token_routes.goagent-manager-service/api/app.goagent-manager-service/api/catalog_routes.goagent-manager-service/api/environment_routes.goagent-manager-service/api/evaluator_routes.goagent-manager-service/api/gateway_routes.goagent-manager-service/api/git_secret_routes.goagent-manager-service/api/identity_routes.goagent-manager-service/api/infra_resource_routes.goagent-manager-service/api/llm_deployment_routes.goagent-manager-service/api/llm_provider_apikey_routes.goagent-manager-service/api/llm_proxy_apikey_routes.goagent-manager-service/api/llm_proxy_deployment_routes.goagent-manager-service/api/llm_routes.goagent-manager-service/api/monitor_routes.goagent-manager-service/api/repository_routes.goagent-manager-service/clients/thundersvc/client.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/clients/thundersvc/identity_types.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/identity_controller.goagent-manager-service/middleware/authorization.goagent-manager-service/middleware/path_params.goagent-manager-service/rbac/permissions.goagent-manager-service/rbac/predefined_roles.goagent-manager-service/utils/constants.goagent-manager-service/wiring/params.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/rush.jsonconsole/workspaces/core-ui/package.jsonconsole/workspaces/core-ui/src/Layouts/OxygenLayout/navigationItems.tsxconsole/workspaces/core-ui/src/Route/Route.tsxconsole/workspaces/core-ui/src/pages/index.tsxconsole/workspaces/libs/api-client/src/apis/identities.tsconsole/workspaces/libs/api-client/src/apis/index.tsconsole/workspaces/libs/api-client/src/hooks/identities.tsconsole/workspaces/libs/api-client/src/hooks/index.tsconsole/workspaces/libs/types/src/api/identities.tsconsole/workspaces/libs/types/src/api/index.tsconsole/workspaces/libs/types/src/routes/generated-route.map.tsconsole/workspaces/libs/types/src/routes/routes.map.tsconsole/workspaces/pages/identities/eslint.config.jsconsole/workspaces/pages/identities/package.jsonconsole/workspaces/pages/identities/src/GroupCreatePage.tsxconsole/workspaces/pages/identities/src/GroupsPage.tsxconsole/workspaces/pages/identities/src/Identities.Organization.tsxconsole/workspaces/pages/identities/src/RoleCreatePage.tsxconsole/workspaces/pages/identities/src/RolesPage.tsxconsole/workspaces/pages/identities/src/UserCreatePage.tsxconsole/workspaces/pages/identities/src/UserInvitePage.tsxconsole/workspaces/pages/identities/src/UsersPage.tsxconsole/workspaces/pages/identities/src/index.tsconsole/workspaces/pages/identities/tsconfig.jsonconsole/workspaces/pages/identities/tsconfig.lib.jsondeployments/docker-compose.ymldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamldeployments/scripts/register-amp-resources.sh
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
console/workspaces/pages/identities/src/UsersPage.tsx (1)
111-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep pagination visible when dataset exists but current page is empty.
When
total > 0and the current page has no rows, this renders a global empty state and hides pagination, so users can’t navigate back to a valid page.Suggested fix
+import React, { useEffect, useMemo, useState } from "react"; ... const users = useMemo(() => data?.users ?? [], [data]); const total = data?.total ?? 0; + const hasAnyUsers = total > 0; + + useEffect(() => { + if (page > 0 && page * rowsPerPage >= total) { + setPage(Math.max(0, Math.ceil(total / rowsPerPage) - 1)); + } + }, [page, rowsPerPage, total]); ... - {users.length === 0 ? ( + {!hasAnyUsers ? ( <ListingTable.EmptyState illustration={<Users size={64} />} title="No users yet" description='Click "Invite User" to invite one.' /> ) : ( ... - {users.length > 0 && ( + {hasAnyUsers && ( <TablePaginationAlso applies to: 150-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/identities/src/UsersPage.tsx` around lines 111 - 116, The current conditional hides pagination when the dataset exists but the current page is empty; change the EmptyState render guard to only show the global empty state when total === 0 (i.e., use if users.length === 0 && total === 0 or simply total === 0) so that when total > 0 but users.length === 0 the table still renders with pagination enabled; update the conditional around ListingTable.EmptyState (and the analogous block at the other location referenced) to check total before deciding to render the global empty state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-manager-service/api/identity_routes.go`:
- Line 46: Replace the OrgView permission with the role-read permission on the
group-role listing route: update the middleware.HandleFuncWithValidationAndAuthz
registration that wires ctrl.GetGroupRoles so it requires rbac.RoleRead instead
of rbac.OrgView, ensuring only callers with role-read privilege can enumerate
roles via GetGroupRoles.
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 309-319: The loop over resp.Members currently swallows GetUser
errors and returns a users slice smaller than resp.TotalResults; change this to
propagate failures instead of continue-ing: when c.GetUser(ctx, m.ID) returns an
error, return nil, 0 (or resp.TotalResults if you prefer) and that error
immediately so the caller sees the failure; update the loop in the function that
iterates resp.Members to return the error from GetUser rather than skipping
members, and adjust any callers/tests that assumed partial results if necessary.
- Around line 322-351: GetGroupRoles currently pages all roles via ListRoles
then calls GetRoleAssignments for each ThunderRole causing a fan-out; change
this to use a direct Thunder query or cached/indexed mapping for role
assignments (e.g., a new ListRoleAssignmentsByGroup or lookup that accepts
groupID) instead of per-role calls, and if you cannot implement the direct
lookup immediately, at minimum stop swallowing assignment fetch errors in
GetGroupRoles (do not continue on err from GetRoleAssignments — return the
error) to avoid returning partial/incomplete data; update references in
GetGroupRoles, ListRoles, GetRoleAssignments, and any cache/index to reflect the
new lookup approach.
In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 199-205: The new hook useGetGroupRoles registers queries under the
'identity-group-roles' key but role-changing mutation handlers still don't
invalidate that key, causing stale group role data; update the role rename, role
delete, and assignee add/remove mutation handlers to call the React Query client
invalidateQueries for ['identity-group-roles', { orgName, groupId }] (or the
closest shape used by useGetGroupRoles) so invalidation is scoped to the
affected group(s) when groupId is available, falling back to invalidating the
broader 'identity-group-roles' key if not; use the same key shape as
useGetGroupRoles and add these invalidateQueries calls in the mutation
success/settled handlers.
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 240-242: The UI currently treats rolesData == null as loading and
shows a spinner forever (including when the query fails); update the
GroupEditPage rendering to explicitly check the roles query status instead of
rolesData == null—use the query object's isLoading/isError (or error) flags
(e.g., rolesQuery.isLoading, rolesQuery.isError, rolesQuery.error) and render
the CircularProgress only when loading, render an error message or error
fallback when isError (including the actual error text), and only render the
empty-state UI when the query succeeded but roles.length === 0; update the
conditional around rolesData/roles to follow this explicit status check so
errors are not masked by the spinner.
- Around line 50-53: The member lists are hard-capped at 100 by the fixed {
offset: 0, limit: 100 } passed to useGetGroupMembers (and the other member list
call around the subsequent block), preventing edits to members beyond the first
page; change the calls to remove the fixed 100 limit and instead support
full/paginated fetching—either by passing a configurable limit/offset or by
implementing looped page fetching until no more results (update
useGetGroupMembers usage and its caller logic to accept/handle pagination
parameters or a “fetchAll” behavior so add/remove operations can see all
members).
In `@console/workspaces/pages/identities/src/UserEditPage.tsx`:
- Around line 57-60: The UserEditPage currently fetches only the first 100
groups via useListGroups({ orgName: orgId }, { offset: 0, limit: 100 }),
truncating group selection; update the fetching logic in UserEditPage to
retrieve all groups (e.g., remove the hardcoded limit and implement paginated
fetching using offset/limit or cursor until no more results) and merge results
into allGroupsData so the selector shows every group — adjust related loading
state (isLoadingAllGroups) and ensure duplicates are handled when concatenating
pages.
- Around line 75-77: The effect in UserEditPage that calls
setSelectedGroups(initialGroups) on every initialGroups change can overwrite
in-progress edits; change it to only initialize selectedGroups once or when the
user hasn't modified the selection: track user edits (e.g., a hasEditedGroups
boolean/ref toggled from the groups change handler) and in the useEffect for
initialGroups call setSelectedGroups(initialGroups) only if hasEditedGroups is
false (or when selectedGroups is undefined/uninitialized). Update handlers that
change selectedGroups to set the hasEditedGroups flag so background refetches
won't clobber unsaved edits; keep references to setSelectedGroups,
initialGroups, and the edit handler names to locate the changes.
---
Outside diff comments:
In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Around line 111-116: The current conditional hides pagination when the dataset
exists but the current page is empty; change the EmptyState render guard to only
show the global empty state when total === 0 (i.e., use if users.length === 0 &&
total === 0 or simply total === 0) so that when total > 0 but users.length === 0
the table still renders with pagination enabled; update the conditional around
ListingTable.EmptyState (and the analogous block at the other location
referenced) to check total before deciding to render the global empty state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36ee2f84-cc29-4357-8fd3-62c8fa683243
⛔ Files ignored due to path filters (1)
console/common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
agent-manager-service/api/identity_routes.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/clients/thundersvc/identity_types.goagent-manager-service/controllers/identity_controller.goagent-manager-service/wiring/wire.goagent-manager-service/wiring/wire_gen.goconsole/apps/web-ui/public/config.jsconsole/workspaces/core-ui/package.jsonconsole/workspaces/libs/api-client/src/apis/identities.tsconsole/workspaces/libs/api-client/src/hooks/identities.tsconsole/workspaces/pages/identities/src/GroupEditPage.tsxconsole/workspaces/pages/identities/src/GroupsPage.tsxconsole/workspaces/pages/identities/src/Identities.Organization.tsxconsole/workspaces/pages/identities/src/UserEditPage.tsxconsole/workspaces/pages/identities/src/UsersPage.tsx
✅ Files skipped from review due to trivial changes (2)
- console/workspaces/core-ui/package.json
- agent-manager-service/wiring/wire_gen.go
| export function useGetGroupRoles(params: GroupPathParams) { | ||
| const { getToken } = useAuthHooks(); | ||
| return useApiQuery<{ roles: ThunderRole[] }>({ | ||
| queryKey: ['identity-group-roles', params], | ||
| queryFn: () => getGroupRoles(params, getToken), | ||
| enabled: !!params.orgName && !!params.groupId, | ||
| }); |
There was a problem hiding this comment.
Invalidate identity-group-roles from role mutations.
This adds a new cache key, but role-changing mutations still only invalidate role-list / assignment queries. After a role rename, delete, or assignee add/remove, group detail views can keep showing stale assigned roles until a full refresh. Add an identity-group-roles invalidation in those mutation handlers, scoped to affected groupIds when available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/workspaces/libs/api-client/src/hooks/identities.ts` around lines 199
- 205, The new hook useGetGroupRoles registers queries under the
'identity-group-roles' key but role-changing mutation handlers still don't
invalidate that key, causing stale group role data; update the role rename, role
delete, and assignee add/remove mutation handlers to call the React Query client
invalidateQueries for ['identity-group-roles', { orgName, groupId }] (or the
closest shape used by useGetGroupRoles) so invalidation is scoped to the
affected group(s) when groupId is available, falling back to invalidating the
broader 'identity-group-roles' key if not; use the same key shape as
useGetGroupRoles and add these invalidateQueries calls in the mutation
success/settled handlers.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-manager-service/clients/thundersvc/identity_client.go`:
- Around line 447-460: The loop over resp.Assignments currently swallows
failures from GetUser/GetGroup (in the switch cases) by using continue,
producing partial role assignment results; change this to fail fast by returning
the error (or a wrapped error with context) whenever c.GetUser(ctx, a.ID) or
c.GetGroup(ctx, a.ID) returns an error so callers don't receive incomplete
data—update the switch in the loop that iterates resp.Assignments to return the
error immediately (including which assignment ID and type) instead of appending
nothing and continuing, and ensure any callers expect/handle that error from
this method.
In `@agent-manager-service/controllers/identity_controller_test.go`:
- Around line 167-169: The test's unbounded loop waiting for ctrl.rootOUID can
hang CI; modify the loop around ctrl.resolveOuID(newRequest()) so it is bounded
(e.g., add a maxRetries counter or timeout) and break with a clear test failure
if rootOUID is not set after the limit; also stop ignoring errors from
resolveOuID—capture its error return and fail the test immediately on unexpected
errors or on exceeding retries to avoid infinite hangs.
In `@console/workspaces/libs/api-client/src/hooks/identities.ts`:
- Around line 126-144: useAllGroups registers a distinct cache key
'identity-groups-all' that currently isn't invalidated by group mutations;
update the group mutation hooks (e.g., createGroup, updateGroup, deleteGroup or
whatever uses useApiMutation for group changes) to also invalidate the
'identity-groups-all' cache key for the affected org (same params shape used in
useAllGroups). Specifically, add invalidation of ['identity-groups-all', params]
(or include it in the existing invalidation list/options passed to
useApiMutation/queryClient.invalidateQueries) alongside any existing
'identity-groups' invalidation so pages using useAllGroups get refreshed after
mutations.
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 105-113: The dropdown filtering in the availableUsers useMemo only
excludes initialMembers from the current page which allows selecting members
that exist on other pages and leads handleSave to attempt duplicate adds; modify
the logic to exclude all existing member IDs by replacing/augmenting
initialMembers with a full list (e.g., allMemberIds) obtained from a
paginated-members hook, then build the excluded Set from
[...allMemberIds.filter(id not removed?), ...pendingAdds.map(u => u.id),
...removedIds?] and update the useMemo dependencies to include that full-list
variable; ensure handleSave deduplicates pendingAdds against this global member
ID set before submitting.
In `@console/workspaces/pages/identities/src/RoleEditPage.tsx`:
- Around line 60-67: The current calls to useListUsers and useListGroups in
RoleEditPage (allUsersData/allGroupsData) hard-code offset:0, limit:100 which
truncates results; update the autocomplete source to fetch/search/paginate
instead of loading only the first 100: remove the fixed offset/limit usage and
wire the autocomplete component (the assignee/user/group picker) to a
paginated/search API flow that calls useListUsers/useListGroups with a
search/query string and page/offset params (or use the hook’s provided
pagination callbacks), implement debounced query changes and "load more" or
virtualized pagination in the picker so users can find and select assignees
beyond the first page. Ensure useListUsers/useListGroups calls accept and pass
through search and pagination args, and replace any reliance on
allUsersData/allGroupsData as the single source for the autocomplete options.
- Around line 166-177: Currently the code loops and calls
addAssignees/removeAssignees for each individual ID (using pendingUserAdds,
removedUserIds, pendingGroupAdds, removedGroupIds), which is slow and fragile;
instead aggregate the IDs into arrays and call addAssignees once with body: {
userIds: [...] } (if any), call removeAssignees once with body: { userIds: [...]
} (if any), and do the same for groupIds, reusing the existing params object —
keep the existing function names addAssignees and removeAssignees and ensure you
only invoke them when the aggregated array is non-empty.
In `@console/workspaces/pages/identities/src/RolesPage.tsx`:
- Line 107: When server-side pagination deletes items and the current page
becomes empty despite total > 0, clamp the page index to the last valid page
instead of rendering the empty-table state; in RolesPage.tsx detect the
condition roles.length === 0 && total > 0 and call the page setter (e.g.,
setPage or setPageIndex) to Math.max(0, Math.ceil(total / pageSize) - 1) before
rendering, then return/loading or re-fetch so the UI displays the last non-empty
page. Apply the same guard/logic to the other conditional block around lines
146-158 that also checks total and renders the table.
In `@console/workspaces/pages/identities/src/UsersPage.tsx`:
- Line 114: When a paged response returns users.length === 0 but total > 0 the
table shows empty because the current page is out of range; add page clamping in
the UsersPage component: add a useEffect that watches total, users.length, page
and limit, compute lastPage = Math.max(1, Math.ceil(total / limit)) and if
users.length === 0 && total > 0 && page !== lastPage call the page setter (e.g.,
setPage(lastPage) or onPageChange(lastPage)) to move to the last valid page;
ensure the effect guards against infinite loops by only updating when page !==
lastPage. Also apply the same useEffect/clamping logic to the other paginated
list in this file (the block referenced around lines 153-165) using its
corresponding state variables (total, items length, page, limit and setter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3263313e-949a-4c8b-ace2-84a9d2dd375f
📒 Files selected for processing (34)
agent-manager-service/api/identity_routes.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/clients/thundersvc/identity_types.goagent-manager-service/controllers/identity_controller.goagent-manager-service/controllers/identity_controller_test.goagent-manager-service/wiring/wire_gen.goconsole/workspaces/libs/api-client/src/hooks/identities.tsconsole/workspaces/pages/add-new-agent/package.jsonconsole/workspaces/pages/add-new-project/package.jsonconsole/workspaces/pages/agent-kind/package.jsonconsole/workspaces/pages/agent-security/package.jsonconsole/workspaces/pages/build/package.jsonconsole/workspaces/pages/configure-agent/package.jsonconsole/workspaces/pages/deploy/package.jsonconsole/workspaces/pages/eval/package.jsonconsole/workspaces/pages/gateways/package.jsonconsole/workspaces/pages/identities/package.jsonconsole/workspaces/pages/identities/src/GroupCreatePage.tsxconsole/workspaces/pages/identities/src/GroupEditPage.tsxconsole/workspaces/pages/identities/src/GroupsPage.tsxconsole/workspaces/pages/identities/src/Identities.Organization.tsxconsole/workspaces/pages/identities/src/RoleCreatePage.tsxconsole/workspaces/pages/identities/src/RoleEditPage.tsxconsole/workspaces/pages/identities/src/RolesPage.tsxconsole/workspaces/pages/identities/src/UserCreatePage.tsxconsole/workspaces/pages/identities/src/UserEditPage.tsxconsole/workspaces/pages/identities/src/UserInvitePage.tsxconsole/workspaces/pages/identities/src/UsersPage.tsxconsole/workspaces/pages/llm-providers/package.jsonconsole/workspaces/pages/logs/package.jsonconsole/workspaces/pages/metrics/package.jsonconsole/workspaces/pages/overview/package.jsonconsole/workspaces/pages/test/package.jsonconsole/workspaces/pages/traces/package.json
✅ Files skipped from review due to trivial changes (2)
- console/workspaces/pages/eval/package.json
- agent-manager-service/wiring/wire_gen.go
62c010f to
9b9b285
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
console/workspaces/pages/identities/src/GroupEditPage.tsx (1)
151-157: ⚡ Quick winBatch add/remove member updates instead of sending one request per user.
Current per-user calls increase request volume and partial-update risk. Send aggregated
userIdsin a single add and single remove call.♻️ Suggested refactor
- const deduped = pendingAdds.filter((u) => !allMemberIds.has(u.id)); - for (const u of deduped) { - await addMembers({ params: { orgName: orgId, groupId }, body: { userIds: [u.id] } }); - } - for (const id of removedIds) { - await removeMembers({ params: { orgName: orgId, groupId }, body: { userIds: [id] } }); - } + const userIdsToAdd = [...new Set(pendingAdds.map((u) => u.id).filter((id) => !allMemberIds.has(id)))]; + const userIdsToRemove = [...removedIds]; + + if (userIdsToAdd.length > 0) { + await addMembers({ params: { orgName: orgId, groupId }, body: { userIds: userIdsToAdd } }); + } + if (userIdsToRemove.length > 0) { + await removeMembers({ params: { orgName: orgId, groupId }, body: { userIds: userIdsToRemove } }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@console/workspaces/pages/identities/src/GroupEditPage.tsx` around lines 151 - 157, The current loop sends one API call per user; instead collect IDs and call addMembers/removeMembers once: compute deduped = pendingAdds.filter(u => !allMemberIds.has(u.id)), build an array addIds = deduped.map(u => u.id) and if addIds.length > 0 call addMembers({ params: { orgName: orgId, groupId }, body: { userIds: addIds } }); similarly, if removedIds has items call removeMembers once with body: { userIds: removedIds } to perform batched updates and avoid per-user requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 58-60: The useAllGroupMemberIds query currently falls back to an
empty set on failure which makes members appear editable and weakens dedupe;
update the code where useAllGroupMemberIds is used (the call returning
allMemberIdsData and isLoadingAllMemberIds) to also read its error/isError state
and treat an error as a blocking state: when error/isError is true, do not
enable member edits or proceed with save, surface an explicit error UI/message
or disable the relevant controls, and avoid using an empty array/map as the
fallback for existingMemberIds; apply the same pattern to the other occurrences
of useAllGroupMemberIds usage (the similar variables and blocks referenced
elsewhere) so any query failure prevents edits and forces an explicit error
handling path.
---
Nitpick comments:
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx`:
- Around line 151-157: The current loop sends one API call per user; instead
collect IDs and call addMembers/removeMembers once: compute deduped =
pendingAdds.filter(u => !allMemberIds.has(u.id)), build an array addIds =
deduped.map(u => u.id) and if addIds.length > 0 call addMembers({ params: {
orgName: orgId, groupId }, body: { userIds: addIds } }); similarly, if
removedIds has items call removeMembers once with body: { userIds: removedIds }
to perform batched updates and avoid per-user requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f428f703-197d-42c8-b909-a7b3b4dd7814
⛔ Files ignored due to path filters (1)
console/common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
agent-manager-service/api/identity_routes.goagent-manager-service/clients/thundersvc/identity_client.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/identity_controller.goagent-manager-service/controllers/identity_controller_test.goconsole/workspaces/core-ui/src/Layouts/OxygenLayout/navigationItems.tsxconsole/workspaces/libs/api-client/src/apis/identities.tsconsole/workspaces/libs/api-client/src/hooks/identities.tsconsole/workspaces/pages/add-new-agent/package.jsonconsole/workspaces/pages/add-new-project/package.jsonconsole/workspaces/pages/agent-kind/package.jsonconsole/workspaces/pages/agent-security/package.jsonconsole/workspaces/pages/build/package.jsonconsole/workspaces/pages/configure-agent/package.jsonconsole/workspaces/pages/deploy/package.jsonconsole/workspaces/pages/eval/package.jsonconsole/workspaces/pages/gateways/package.jsonconsole/workspaces/pages/identities/package.jsonconsole/workspaces/pages/identities/src/GroupCreatePage.tsxconsole/workspaces/pages/identities/src/GroupEditPage.tsxconsole/workspaces/pages/identities/src/GroupsPage.tsxconsole/workspaces/pages/identities/src/RoleCreatePage.tsxconsole/workspaces/pages/identities/src/RoleEditPage.tsxconsole/workspaces/pages/identities/src/RolesPage.tsxconsole/workspaces/pages/identities/src/UserCreatePage.tsxconsole/workspaces/pages/identities/src/UserEditPage.tsxconsole/workspaces/pages/identities/src/UserInvitePage.tsxconsole/workspaces/pages/identities/src/UsersPage.tsxconsole/workspaces/pages/llm-providers/package.jsonconsole/workspaces/pages/logs/package.jsonconsole/workspaces/pages/metrics/package.jsonconsole/workspaces/pages/overview/package.jsonconsole/workspaces/pages/test/package.jsonconsole/workspaces/pages/traces/package.json
| const { data: allMemberIdsData, isLoading: isLoadingAllMemberIds } = useAllGroupMemberIds( | ||
| { orgName: orgId, groupId: groupId ?? "" }, | ||
| ); |
There was a problem hiding this comment.
Handle all-member-ids query failures explicitly before enabling member edits.
If this query fails, the fallback empty set makes existing members appear selectable and weakens dedupe during save. Treat this as a blocking/error state instead of silently continuing.
💡 Suggested fix
- const { data: allMemberIdsData, isLoading: isLoadingAllMemberIds } = useAllGroupMemberIds(
+ const {
+ data: allMemberIdsData,
+ isLoading: isLoadingAllMemberIds,
+ isError: isAllMemberIdsError,
+ } = useAllGroupMemberIds(
{ orgName: orgId, groupId: groupId ?? "" },
);
@@
- const isLoading = isLoadingMembers || isLoadingAllMemberIds || isLoadingUsers;
+ const isLoading = isLoadingMembers || isLoadingAllMemberIds || isLoadingUsers;
+ const hasBlockingError = isAllMemberIdsError;
@@
+ if (hasBlockingError) {
+ return (
+ <PageLayout title="Edit Group" disableIcon>
+ <Alert severity="error">Failed to load group membership data. Please refresh and try again.</Alert>
+ </PageLayout>
+ );
+ }Also applies to: 81-84, 112-119, 166-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@console/workspaces/pages/identities/src/GroupEditPage.tsx` around lines 58 -
60, The useAllGroupMemberIds query currently falls back to an empty set on
failure which makes members appear editable and weakens dedupe; update the code
where useAllGroupMemberIds is used (the call returning allMemberIdsData and
isLoadingAllMemberIds) to also read its error/isError state and treat an error
as a blocking state: when error/isError is true, do not enable member edits or
proceed with save, surface an explicit error UI/message or disable the relevant
controls, and avoid using an empty array/map as the fallback for
existingMemberIds; apply the same pattern to the other occurrences of
useAllGroupMemberIds usage (the similar variables and blocks referenced
elsewhere) so any query failure prevents edits and forces an explicit error
handling path.
…-permission-model
8428575 to
640c75e
Compare
…der set Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a3134cf to
22599c6
Compare
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
New Features
Chores