Roles > 'Privileges' page

[carma12]

This PR has been created at the top of #1126 and must be merged first.

Summary by Sourcery

Add full role detail view with editable settings, member management, and privilege assignment, plus shared contextual help handling and support for ID override users and system accounts.

New Features:

• Expose role detail routes with tabs for settings, members, and privileges.
• Allow editing and saving role settings via role_mod.
• Enable viewing and managing role members across users, groups, hosts, host groups, services, user ID overrides, and system accounts.
• Provide a privileges tab to search, add, and remove privileges associated with a role.
• Add search endpoints and UI support for ID override users and system accounts when assigning them to roles.

Bug Fixes:

• Prevent RPC ID override queries from crashing when no results are returned by safely defaulting to empty arrays.

Enhancements:

• Extend membership tables to handle non-object member lists (external, sysaccount, idoverrideuser, privileges) without links and extra columns.
• Augment role data model and conversion utilities with member arrays and privileges to support richer role operations.
• Refactor contextual help panel usage into a dedicated hook and make the panel robust to missing fromPage values.
• Wire role memberships into existing member management components so they can be reused for role-related operations.

[sourcery-ai]

Hey - I've found 4 issues, and left some high level feedback:

• MembershipTable now accepts entityList as EntryDataTypes[] but several new callers pass string[] (e.g. MembersUserIdOverride/MembersSystemAccount), which relies on casting inside the component; consider widening the prop type and removing the internal as string casts so the string-array use case is explicitly supported and type-safe.
• RolesTabs, RolesMembers, and RolesPrivileges all fetch role data separately (useRoleSettings, useGetRoleByIdQuery), which can result in redundant RPC calls and state divergence; consider lifting the role query to a single owner (e.g. RolesTabs) and passing the resolved role down as props.
• In useRoleSettings, resetValues currently updates originalRole to the current role rather than reverting role to the original data, which makes the naming confusing and affects how it’s used on save error; consider renaming this function or adjusting the behavior so revert/reset semantics are clearly separated.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- MembershipTable now accepts `entityList` as `EntryDataTypes[]` but several new callers pass `string[]` (e.g. MembersUserIdOverride/MembersSystemAccount), which relies on casting inside the component; consider widening the prop type and removing the internal `as string` casts so the string-array use case is explicitly supported and type-safe.
- RolesTabs, RolesMembers, and RolesPrivileges all fetch role data separately (`useRoleSettings`, `useGetRoleByIdQuery`), which can result in redundant RPC calls and state divergence; consider lifting the role query to a single owner (e.g. RolesTabs) and passing the resolved role down as props.
- In `useRoleSettings`, `resetValues` currently updates `originalRole` to the current `role` rather than reverting `role` to the original data, which makes the naming confusing and affects how it’s used on save error; consider renaming this function or adjusting the behavior so revert/reset semantics are clearly separated.

## Individual Comments

### Comment 1
<location path="src/hooks/useRolesSettingsData.tsx" line_range="83-84" />
<code_context>
+    setModified(isModified);
+  }, [role, originalRole]);
+
+  const onResetValues = () => {
+    setOriginalRole({ ...role });
+    setModified(false);
+  };
</code_context>
<issue_to_address>
**issue (bug_risk):** Reset handler updates the originalRole to the current (possibly invalid) state, which breaks "revert" behavior and modified tracking.

In `onResetValues`, updating `originalRole` from the current `role` means a failed save will replace the last server-sourced role with the unsaved/invalid one. As a result, "Revert" in `RolesSettings` no longer restores the true original role, and the modified tracking baseline is incorrect.

If `resetValues` is meant to roll back to the last known-good value, it should restore `role` from `originalRole` while leaving `originalRole` unchanged, for example:
```ts
const onResetValues = () => {
  setRole({ ...originalRole });
  setModified(false);
};
```
If a different behavior is intended, consider renaming `resetValues` and updating `RolesSettings` so error handling does not overwrite the original snapshot.
</issue_to_address>

### Comment 2
<location path="src/pages/Roles/RolesSettings.tsx" line_range="69-78" />
<code_context>
+    modifiedValues.cn = props.role.cn;
+    setSaving(true);
+
+    saveRole(modifiedValues).then((response) => {
+      if ("data" in response) {
+        if (response.data?.result) {
+          dispatch(
+            addAlert({
+              name: "save-success",
+              title: "Role modified",
+              variant: "success",
+            })
+          );
+          props.onRefresh();
+        } else if (response.data?.error) {
+          const errorMessage = response.data.error as ErrorResult;
+          dispatch(
+            addAlert({
+              name: "save-error",
+              title: errorMessage.message,
+              variant: "danger",
+            })
+          );
+          props.onResetValues();
+        }
+        setSaving(false);
+      }
+    });
</code_context>
<issue_to_address>
**issue (bug_risk):** Save handler assumes a fulfilled promise; rejected RTK Query promises will leave the saving state stuck and skip alert handling.

The handler only handles fulfilled promises from `saveRole`. If the mutation rejects (e.g., network/serialization error), `setSaving(false)` is never called and no alert is dispatched, leaving the UI stuck in a "Saving" state.

Add failure handling (via `.catch` or `unwrap()` + try/catch) so rejection also clears `isSaving` and shows an error alert, for example:
```ts
setSaving(true);
saveRole(modifiedValues)
  .then((response) => { /* existing logic */ })
  .catch(() => {
    setSaving(false);
    dispatch(addAlert({
      name: "save-error",
      title: "Failed to save role due to a network or unexpected error",
      variant: "danger",
    }));
  });
```
</issue_to_address>

### Comment 3
<location path="src/components/tables/MembershipTable.tsx" line_range="98" />
<code_context>
-              }}
-            />
-          )}
-          <Td>
-            {props.from === "roles" ? (
-              // Temporary until Roles are implemented
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the link/no-link decision and URL construction into small helper functions to simplify the table cell rendering logic.

You can reduce the complexity around link/no-link handling without changing behavior by pulling the `props.from`/`isStringArray` branching into a small helper and reusing it in the cell rendering.

Right now the logic is embedded directly in the JSX:

```tsx
<Td>
  {props.from === "roles" ||
   props.from === "privileges" ||
   isStringArray ? (
    // String arrays, roles, and privileges don't have links
    itemId
  ) : (
    <Link
      to={
        "/" +
        props.from +
        "/" +
        (props.from === "services"
          ? encodeURIComponent(itemId)
          : itemId)
      }
    >
      {itemId}
    </Link>
  )}
</Td>
```

You can encapsulate both decisions (“is this item linkable?” and “what is the link target?”) in small helpers. This keeps the table body logic linear and makes future additions (e.g. more non-link types) less invasive.

For example:

```tsx
// keep near MemberTable for now
const shouldRenderLink = (from: FromTypes, isStringArray: boolean) => {
  return !isStringArray && from !== "roles" && from !== "privileges";
};

const getItemLink = (from: FromTypes, itemId: string) => {
  if (from === "services") {
    return `/${from}/${encodeURIComponent(itemId)}`;
  }
  return `/${from}/${itemId}`;
};
```

Then the cell becomes:

```tsx
<Td>
  {shouldRenderLink(props.from, isStringArray) ? (
    <Link to={getItemLink(props.from, itemId)}>{itemId}</Link>
  ) : (
    itemId
  )}
</Td>
```

This preserves the current behavior (including the new `privileges` case) but makes the control flow easier to follow and localizes the domain-specific special-casing.
</issue_to_address>

### Comment 4
<location path="src/pages/Roles/RolesTabs.tsx" line_range="61" />
<code_context>
+    ? role.member_sysaccount.length
+    : 0;
+
+  const handleTabClick = (
+    _event: React.MouseEvent<HTMLElement, MouseEvent>,
+    tabIndex: number | string
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the mapping between tab keys, routes, and sections into shared helpers to avoid duplicated branching logic and make future tabs easier to add.

You’re extending an already-branchy mapping between route `section` and tab keys/URLs; adding `"privileges"` makes the indirection a bit worse. You can reduce complexity by centralizing this mapping in a single place and reusing it both for `handleTabClick` and for deriving `activeTabKey` from `section`.

For example:

```ts
// Central mapping between tab keys and routes
const TAB_ROUTES: Record<string, (cn: string) => string> = {
  settings: (cn) => `/roles/${cn}`,
  member: (cn) => `/roles/${cn}/member_user`,
  privileges: (cn) => `/roles/${cn}/privileges`,
};

// Normalize section -> tab key in one helper
const getTabKeyFromSection = (section?: string): string => {
  if (!section) return "settings";
  if (section.startsWith("member_")) return "member";
  if (section === "privileges") return "privileges";
  return section;
};
```

Then use it consistently:

```ts
const [activeTabKey, setActiveTabKey] = useState(() =>
  getTabKeyFromSection(section)
);

const handleTabClick = (
  _event: React.MouseEvent<HTMLElement, MouseEvent>,
  tabIndex: string | number
) => {
  const tabKey = String(tabIndex);
  const toPath = TAB_ROUTES[tabKey];
  if (toPath) navigate(toPath(cn));
};
```

And in the effect:

```ts
useEffect(() => {
  const tabKey = getTabKeyFromSection(section);
  setActiveTabKey(tabKey);

  if (!section) {
    navigate(URL_PREFIX + TAB_ROUTES.settings(cn));
  }
}, [section, cn, navigate]);
```

This keeps all behavior (including `member_` subsections and the new `privileges` route) but removes duplicated `if/else` chains and makes it easier to add future tabs without spreading mapping logic across the component.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[carma12]

Amended Sourcery suggestions.

[b3lix]

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

[carma12]

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

Yes, but first the #1126 PR needs to be merged. This is also indicated in the PR description.

[b3lix]

@carma12 Can this be rebased before my review? I see a 6k+ LOC changes, but I think it will be less after rebase.

Yes, but first the #1126 PR needs to be merged. This is also indicated in the PR description.

I overlooked, I see it now, thanks for info ... looking into 1126 already.

[veronnicka]

Hi, please see my comment. I ran the code, I noticed the help panel not working but Im not sure if this is expected as it needs rebase, right? Was not sure so im just letting you know.

[carma12]

Hi, please see my comment. I ran the code, I noticed the help panel not working but Im not sure if this is expected as it needs rebase, right? Was not sure so im just letting you know.

Thank you for the review. Correct, the changes of #1124 needs to be merged first to be shown here. So you can ignore that in this PR :)

[veronnicka]

Thanks for the changes. I ran the code and everything works as expected. Im providing an ack

[carma12]

@veronnicka @b3lix - This PR has been rebased and its ready to review now.

[carma12]

@veronnicka - Didn't realized you already provided an ack until I removed it, sorry 😅 Please add it again if you want to keep it. The code hasn't changed at all, just multiple commits were removed when rebased.

[veronnicka]

providing the ack again

[veronnicka]

Found a bug in privileges page that is also present here, will give the details soon

[veronnicka]

Found a bug in privileges page that is also present here, will give the details soon

Please see #1134 (review)

[carma12]

Found a bug in privileges page that is also present here, will give the details soon

Please see #1134 (review)

I created this PR to fix the issue related to the limited amount of entry results shown when searching. The other issue related to the search results being shown only when the "search" button is clicked needs to be addressed in another PR.

[veronnicka]

If the bug is going to be solved in a different PR, thats fine by me then.