Add 'Permissions' main page - #1145
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues, and left some high level feedback:
- In SelectMultiTypeaheadCheckbox, avoid calling setLocalOptions directly during render when props change and remove the console.log statements; instead sync localOptions from options via a dedicated useEffect to prevent unnecessary re-renders and potential state update warnings.
- The getPermissionsFullData and searchPermissionsEntries endpoints in rpcPermissions share almost identical two-step find/show logic; consider extracting this into a shared helper to reduce duplication and make future changes to the query behavior easier to maintain.
- In AddPermissionModal and permissionsUtils, the Permission fields extratargetfilter and memberof are treated as arrays in some places and as strings in the Permission type; align the Permission interface and helpers with their actual usage (arrays vs. strings) to avoid type confusion and potential runtime issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In SelectMultiTypeaheadCheckbox, avoid calling setLocalOptions directly during render when props change and remove the console.log statements; instead sync localOptions from options via a dedicated useEffect to prevent unnecessary re-renders and potential state update warnings.
- The getPermissionsFullData and searchPermissionsEntries endpoints in rpcPermissions share almost identical two-step find/show logic; consider extracting this into a shared helper to reduce duplication and make future changes to the query behavior easier to maintain.
- In AddPermissionModal and permissionsUtils, the Permission fields extratargetfilter and memberof are treated as arrays in some places and as strings in the Permission type; align the Permission interface and helpers with their actual usage (arrays vs. strings) to avoid type confusion and potential runtime issues.
## Individual Comments
### Comment 1
<location path="src/utils/datatypes/globalDataTypes.ts" line_range="233-242" />
<code_context>
description: string;
}
+export interface Permission {
+ cn: string;
+ dn: string;
+ ipapermright: string[];
+ ipapermbindruletype: string;
+ attrs: string[];
+ ipapermlocation: string;
+ extratargetfilter: string;
+ ipapermtargetfilter: string;
+ type: string;
+ memberof: string;
+ targetgroup: string;
+ memberof_privilege: string[];
+ ipapermissiontype: string[];
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Permission interface field types are inconsistent with usage, leading to type/runtime mismatches.
Several fields in `Permission` don’t match how they’re used elsewhere:
- `cn` is typed as `string` but accessed as `permission.cn[0]`, and `cnType` indicates it should be an array.
- `extratargetfilter` and `memberof` are typed as `string`, but `AddPermissionModal`/`rpcPermissions`/`PermissionAddPayload` use `string[]`.
- `attrs` is required `string[]`, but callers pass `undefined` when empty and `apiToPermission` treats it as optional.
These mismatches will lead to `any` casts or runtime errors. Please update the interface to match usage (e.g. `cn: string[]`, `extratargetfilter?: string[]`, `memberof?: string[]`, `attrs?: string[]`) and adjust helpers like `createEmptyPermission` accordingly.
</issue_to_address>
### Comment 2
<location path="src/components/layouts/SimpleSelector.tsx" line_range="37-39" />
<code_context>
const onSelect = (
_event: React.MouseEvent<Element, MouseEvent> | undefined,
- value: string | number | undefined
+ value: SelectOptionProps
) => {
- props.onSelectedChange(value as string);
- setSelected(value as string);
+ props.onSelectedChange(value[props.returnedProperty ?? "value"]);
+ setSelected(value.value);
setIsOpen(false);
</code_context>
<issue_to_address>
**issue (bug_risk):** SimpleSelector now passes/receives full option objects but still uses a string `selected` value, which is likely incompatible with the underlying Select component.
Previously `SelectOption` used a primitive `value` (`string | number`), and `SimpleSelector.selected` matched that. Now `SelectOption` passes the full option object as `value`, but `SimpleSelector.selected` is still a `string`. Since PatternFly `Select` expects `selected` to match the option `value` (now `SelectOptionProps`), this mismatch can break selection highlighting and controlled behavior.
`onSelectedChange` also returns `value[returnedProperty]` (e.g. `key`), while internal state uses `value.value` (the label), so the component stores one representation and emits another, which can cause desync between displayed selection and the actual selected key.
Please either keep `value` as a primitive (`option.value`) with `onSelect(value: string | number)`, or make `selected` consistently use `SelectOptionProps` (or the same type as `value`) and update callers accordingly.
</issue_to_address>
### Comment 3
<location path="src/pages/Permissions/Permissions.tsx" line_range="251-260" />
<code_context>
+ if (isSelected) {
+ newSelectedPermissions = JSON.parse(JSON.stringify(selectedPermissions));
+ for (let i = 0; i < permissions.length; i++) {
+ if (selectedPermissions.find((s) => s.cn[0] === permissions[i].cn[0])) {
+ continue;
+ }
+ newSelectedPermissions.push(permissions[i]);
+ }
+ } else {
+ for (let i = 0; i < selectedPermissions.length; i++) {
+ let found = false;
+ for (let ii = 0; ii < permissions.length; ii++) {
+ if (selectedPermissions[i].cn[0] === permissions[ii].cn[0]) {
+ found = true;
+ break;
</code_context>
<issue_to_address>
**issue (bug_risk):** Selection logic assumes `Permission.cn` is indexable (`cn[0]`), which contradicts the interface and can break equality checks.
`updateSelectedPermissions` compares `s.cn[0]` to `permissions[i].cn[0]`, while `Permission.cn` is typed and used elsewhere as a `string` (e.g. `permission.cn` in `deletePermissions`). This mismatch means you’re either only comparing the first character, or the type definition is wrong. Please align usage and types: either compare full strings (`s.cn === permissions[i].cn`) if `cn` is a string, or change `Permission` (and related helpers) to use `cn: string[]` and update the deletion/selection logic accordingly.
</issue_to_address>
### Comment 4
<location path="src/pages/Permissions/Permissions.tsx" line_range="188-197" />
<code_context>
+ searchPermissions({
</code_context>
<issue_to_address>
**issue (bug_risk):** Search mutation only re-enables the search UI when `data` is present, so errors can leave the search disabled indefinitely.
In `submitSearchValue`, `setSearchIsDisabled(true)` is called before the mutation, but `setSearchIsDisabled(false)` only runs when `"data" in result`. If the mutation fails or returns an error-only shape, the handler never re-enables the search, leaving `isSearchActive` true and the UI effectively locked. Please handle the error path explicitly (e.g., `"error" in result` or a `.catch`) and move re-enabling the input into a `.finally` so it always runs.
</issue_to_address>
### Comment 5
<location path="src/components/Form/SelectMultiTypeaheadCheckbox.tsx" line_range="35-44" />
<code_context>
+ allowCreation,
+}: SelectMultiTypeaheadCheckboxProps) => {
+ // This is to allow mutations to options property
+ const [localOptions, setLocalOptions] =
+ useState<SelectOptionProps[]>(options);
+ console.log("LOCAL", localOptions);
+ console.log("OPTIONS", options);
+ // This one is actually shown, it can contain Create New options and no results
+ const [availableOptions, setAvailableOptions] =
+ useState<SelectOptionProps[]>(localOptions);
+ const [isOpen, setIsOpen] = useState(false);
+ const [inputValue, setInputValue] = useState<string>("");
+ const [focusedItemIndex, setFocusedItemIndex] = useState<number | null>(null);
+ const [activeItemId, setActiveItemId] = useState<string | null>(null);
+ const textInputRef = useRef<HTMLInputElement>(undefined);
+
+ if (options !== localOptions) {
+ setLocalOptions(options);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Directly calling `setLocalOptions` in render based on `options !== localOptions` risks unnecessary re-renders or loops.
Because this check runs in the render path, `options !== localOptions` will be true whenever a new array instance is passed, even if its contents are unchanged. Calling `setLocalOptions` during render can lead to repeated renders or render loops and makes state harder to reason about. Instead, mirror `options` into state in an effect:
```ts
useEffect(() => {
setLocalOptions(options);
}, [options]);
```
so updates are applied after render.
</issue_to_address>
### Comment 6
<location path="src/utils/permissionsUtils.tsx" line_range="20-29" />
<code_context>
+ return { ipaObject, recordOnChange };
+};
+
+const simpleValues = new Set([
+ "cn",
+ "dn",
+ "ipapermbindruletype",
+ "ipapermlocation",
+ "extratargetfilter",
+ "ipapermtargetfilter",
+ "type",
+ "memberof",
+ "targetgroup",
+]);
+const dateValues = new Set([]);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Conversion helpers mark `extratargetfilter` and `memberof` as simple scalar values, but other code uses them as arrays.
Because `simpleValues` includes `extratargetfilter` and `memberof`, `convertApiObj` will treat them as scalars, while `PermissionAddPayload`, `AddPermissionModal`, and `apiToPermission` all treat them as `string[]`. This inconsistency can lead to objects flipping between string and array shapes depending on which helper ran last. Please either:
- Remove these keys from `simpleValues` and treat them as arrays in `convertApiObj`, or
- Normalize them to a single agreed shape in `apiToPermission`/`createEmptyPermission` and adjust the UI code accordingly.
Suggested implementation:
```typescript
const simpleValues = new Set([
"cn",
"dn",
"ipapermbindruletype",
"ipapermlocation",
"ipapermtargetfilter",
"type",
"targetgroup",
]);
```
If there is any remaining code that assumes `extratargetfilter` or `memberof` are plain strings (e.g. direct string operations without indexing into an array), those usages should be updated to work with `string[]` instead. Based on your comment, `PermissionAddPayload`, `AddPermissionModal`, and `createEmptyPermission`/`apiToPermission` are already aligned to `string[]`, so no further changes should be needed there.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I've considered reusing some of the already in use typeaheads, however decided not to, as I would end up breaking some stuff and I need checkboxes. |
There was a problem hiding this comment.
hi, i have some comments just from checking the UI, I will review the code next.
Behaviour:
When adding a new permission:
1.1. I select Custom in the Type attribute
1.2. I write some non-xistent subtree in the Subtree field
1.3. I switch the Type attribute to Automount
1.4. The text in the subtree field remains there and blocks the creation of the permission
Expected: The text is removed upon changing the Type
See:
Screencast.From.2026-07-30.12-38-05.mp4
Another one:
Issue: Unclickable “Create new” option in the Attributes field
Actual behavior:
When typing a value in the Attributes field that does not match any existing attribute, a “Create new” option appears. However, the option is not clickable.
Expected behavior:
Either the “Create new” option should be clickable and create a new attribute, or it should not be displayed if creating new attributes is not supported.
See:
Screencast.From.2026-07-30.12-34-17.mp4
Another one is, please add rules to the Input field in the Permission name ( the very first one). Otherwise, its failing on submission.
See:
|
Another suggestions I have for the add modal: When Some fields are failing upon submission, for example the |
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
Please see my other comments as well, some are connected to the UI behaviour I already mentioned
|
As
I understand where you're coming from, but I think this should be done server-side if at all, otherwise we will show different data than the server does, which would be confusing. You may add all for testing purposes and remove it later, without you having to go and write down all the previous options. Yes, it's confusing, that you can have all and others at the same time, but I really don't want to show different data than what is provided through backend. |
|
@veronnicka, please re-visit, the tests are failing, but those are the common flaky ones. |
okay, thanks for explaining |
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
Hi, thanks for the changes, I checked the UI as well, works as expected.
One thing that is still unclear to me, why create the create new attribute, when upon creation, I cannot add the new permission because of it. See:
Is it a typing problem? From the user perspective there is not enough information for what Im doing wrong.
Also, are we waiting for the search fix before merging or just merge it as-is and deal with it in the #1139 PR?
Other than this, LGTM.
|
This one should get merged first, but please ignore broken search. |
|
Hi @veronnicka, the add option is there, because LDAP schema may contain whatever and may differ from what FreeIPA provides us as the information. Say you expand your schema, don't write your own plugin but still want to add a permission there, that is exactly the use-case where you'd use this. You can try this out, in my case I've expanded the schema by location attribute:
ldapmodify -D "cn=Directory Manager" -W -f location.ldif
Now you're able to create location attribute to anything, but others will fail. 
(don't worry about privilege in the example, it works for all) |
carma12
left a comment
carma12
left a comment
There was a problem hiding this comment.
Please check my comments below. I'm still wrapping my head around this approach: it is functional, but probably there are some things that need to be reviewed further due to the missing parameters. I took the reference from the 'API browser' utility in old WebUI, but please double check this as it may be some inconsistencies.
| ipapermbindruletype: string; | ||
| attrs: string[]; | ||
| ipapermlocation: string; | ||
| extratargetfilter: string; |
There was a problem hiding this comment.
I think this one and some other parameters (ipapermtargetfilter, and memberof) should be string[].
| description: string; | ||
| } | ||
|
|
||
| export interface Permission { |
There was a problem hiding this comment.
Missing parameters: ipapermincludedattr (string[]), ipapermexcludedattr (string[]), ipapermdefaultattr (string[]), ipapermtargetto (string), ipapermtargetfrom (string), targetgroup (string), ipapermlocation (string, I think).
There was a problem hiding this comment.
The list differs between mod and add functions..., but yes, for some reason it's completely wrong, without me raising an eyebrow...
There was a problem hiding this comment.
There are still a lot of missing parameters from the list, but not sure if it would make sense to still add them in case those are needed in the future? Personally, I find this lack of consistency (and documentation) quite difficult to handle: there is no way to determine which parameters are really needed and which not.
| for (let i = 0; i < permissionsListSize; i++) { | ||
| permissions.push(permissionsListResult[i].result); | ||
| } | ||
|
|
||
| return { | ||
| elementsList: permissions, | ||
| totalCount: batchResponse.result.totalCount, | ||
| }; |
There was a problem hiding this comment.
I think there is a missing step here: ideally, the data should be parsed using the apiToPermission function. This will take the data received from the API call and will parse each parameter according to their corresponding types (e.g. if xyz parameter can be received as string[] but treated in the code as string or number).
Related to this, apiToPermission is not used anywhere, but for some reason knip is not detecting this.
There was a problem hiding this comment.
This would explain, as knip goes, this is in exceptions.
There was a problem hiding this comment.
There was a problem hiding this comment.
The apiToPermission also needs to be added in the search, as it currently still pushes raw API results directly without conversion. I.e.:
permissions.push(apiToPermission(permissionsListResult[i].result));
| <Select | ||
| data-cy={`${dataCy}-multi-typeahead-checkbox-select`} | ||
| role="menu" | ||
| id="multi-typeahead-checkbox-select" |
There was a problem hiding this comment.
This is hardcoded. Maybe we can be ${dataCy}-multi-typeahead-checkbox-select as in data-cy?
There was a problem hiding this comment.
id should be passed, will change
carma12
left a comment
carma12
left a comment
There was a problem hiding this comment.
Just some other details that I found in another round of review.
| if (isSelected) { | ||
| newSelectedPermissions = JSON.parse(JSON.stringify(selectedPermissions)); | ||
| for (let i = 0; i < permissions.length; i++) { | ||
| if (selectedPermissions.find((s) => s.cn[0] === permissions[i].cn[0])) { |
There was a problem hiding this comment.
permission.cn is typed as string and apiToPermission converts it to a plain string via simpleValues/convertToString. So I think permission.cn[0] would access the first character of the string, not the first element of an array. This needs to be fixed.
| ipapermbindruletype: string; | ||
| type?: string; | ||
| ipapermlocation?: string; | ||
| extratargetfilter?: string[]; |
There was a problem hiding this comment.
extratargetfilter should be string.
There was a problem hiding this comment.
The other way around, it should be an array in the other places.
| const [extraTargetFilter, setExtraTargetFilter] = React.useState<string[]>( | ||
| [] | ||
| ); |
There was a problem hiding this comment.
This one should be string only.
| {...(activeItemId && { "aria-activedescendant": activeItemId })} | ||
| role="combobox" | ||
| isExpanded={isOpen} | ||
| aria-controls="select-multi-typeahead-checkbox-listbox" |
There was a problem hiding this comment.
I think this one should match the id format (i.e.: id={${id}-select-multi-typeahead-checkbox-listbox}), so it should be:
aria-controls={`${id}-select-multi-typeahead-checkbox-listbox`}
| <TypeAheadWithCheckbox | ||
| id="modal-form-memberof" | ||
| dataCy="modal-select-memberof" | ||
| options={( | ||
| groupsQuery.data?.result.result as unknown as Record< | ||
| string, | ||
| unknown[] | ||
| >[] | ||
| )?.map((group) => ({ | ||
| children: group.cn[0] as string, | ||
| value: group.cn[0] as string, | ||
| "data-cy": `modal-select-attrs-${group.cn[0]}`, |
There was a problem hiding this comment.
If groupsQuery.data is undefined (during loading or on error), the optional chaining ?.map(...) will return undefined as well. Please add a fallback: options={... || []} or guard inside TypeAheadWithCheckbox with useState(options ?? []).
There was a problem hiding this comment.
Good catch, should be fixed now
Thank you for this explanation. Then I think it makes sense how the UI works now. |
Add the 'Permissions' page under the 'Role-based access control' section, allowing users to list, search, add, and delete permissions. Changes: - Create `Permissions` main page with table, search, and pagination - Add `AddPermissionModal` and `DeletePermissionsModal` components - Add permissions navigation route and menu item - Add `rpcPermissions` service with find/show/add/delete/search endpoints - Add `Permission` data type and related utilities - Add reusable `SelectMultiTypeaheadCheckbox` form component - Extend `SimpleSelector` to support returning a custom property - Add `findGroups` query to `rpcUserGroups` service Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: David Hanina <dhanina@redhat.com>
Not having this would throw, to not throw we prepend the id we pass. Signed-off-by: David Hanina <dhanina@redhat.com>
|
Hi, @veronnicka, @carma12 I've updated the PR. Apart from the mentioned changes I've also rebased the PR on top of current main, which simplifies the code a bit. |
|
|
||
| const hasType = type.trim() !== ""; | ||
| const hasTarget = | ||
| hasType || subtree.trim() !== "" || extraTargetFilter.length > 0; |
There was a problem hiding this comment.
hasTarget is a bit off vs what the server actually checks. subtree on its own isn't enough, if you pick Custom and only fill Subtree, Add is enabled but permission_add fails asking for a target. On the flip side, target DN / memberof / attrs alone leave Add disabled even though that should be fine (basedn is used by default). Can we treat type, memberof, target DN/targetgroup, filter and attrs as the real targets, and not count subtree by itself?
| { | ||
| id: "valid-chars", | ||
| message: | ||
| "May only contain letters, numbers, -, _, ., :, /, and space", |
There was a problem hiding this comment.
On create, FreeIPA doesn’t use the general permission cn pattern. permission_add replaces it with a stricter one via _disallow_colon, so names can’t include : or / — only letters, numbers, -, _, ., and space. The modal still allows : and /, so it looks fine here and fails on submit. Can we use the add rule instead?
Ref: https://github.com/freeipa/freeipa/blob/master/ipaserver/plugins/permission.py (_disallow_colon, and permission_add.get_args)
There was a problem hiding this comment.
Thanks, it would be much nicer to simply grab these patterns from the backend response, but that is kind of refactoring I'm not ready for today.
veronnicka
left a comment
veronnicka
left a comment
There was a problem hiding this comment.
I have no other comments. If the checks pass, Im fine with merging
4 participants
Add the 'Permissions' page under the 'Role-based access control' section, allowing users to list, search, add, and delete permissions.
Changes:
Permissionsmain page with table, search, and paginationAddPermissionModalandDeletePermissionsModalcomponentsrpcPermissionsservice with find/show/add/delete/search endpointsPermissiondata type and related utilitiesSelectMultiTypeaheadCheckboxform componentSimpleSelectorto support returning a custom propertyfindGroupsquery torpcUserGroupsserviceAssisted-by: Cursor cursoragent@cursor.com
Summary by Sourcery
Introduce a new Permissions management page under role-based access control, backed by RPC services and utilities for listing, searching, adding, and deleting permissions.
New Features:
Enhancements: