Skip to content

feat(roles): redesign org roles page and better perm multiselect#6027

Open
mathnogueira wants to merge 36 commits intomainfrom
chore/ENG-4631
Open

feat(roles): redesign org roles page and better perm multiselect#6027
mathnogueira wants to merge 36 commits intomainfrom
chore/ENG-4631

Conversation

@mathnogueira
Copy link
Copy Markdown
Contributor

@mathnogueira mathnogueira commented Apr 14, 2026

Context

We have updated the Project Roles page with a new UI experience, but the Organization page still had the old UI. This PR updates it to reuse the same components.

This also adds an improvement on the Permission MultiSelect component to allow it to filter by descriptions other than just the title. This was also applied to the Project Roles page.

Screenshots

New design matching the Project Roles UI

image

New Permission Multiselect component

This allows filtering by the action description as well as its title

image

Read-only (Platform roles)

image

Steps to verify the change

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

@linear
Copy link
Copy Markdown

linear Bot commented Apr 14, 2026

@maidul98
Copy link
Copy Markdown
Collaborator

maidul98 commented Apr 14, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@mathnogueira mathnogueira marked this pull request as ready for review April 14, 2026 19:52
actions: readonly TOrgPermissionAction[];
};

export const ORG_PERMISSION_OBJECT: Record<string, TOrgPermissionConfig> = {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this is an important part to be reviewed. It maps all permissions and respective actions and their descriptions.

@mathnogueira
Copy link
Copy Markdown
Contributor Author

@claude review this again

@mathnogueira
Copy link
Copy Markdown
Contributor Author

@claude review this again, please

@mathnogueira
Copy link
Copy Markdown
Contributor Author

@claude stop reviewing this PR

@akhilmhdh akhilmhdh removed their request for review April 15, 2026 19:53
@mathnogueira
Copy link
Copy Markdown
Contributor Author

@claude don't review this

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note from Matheus:

I don't think this is a problem. If user added a policy, but removed all permissions, no need to display that row anymore.

btw, if they remove all permissions, they are actually deleted from the DB. So, not a regression.


Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 frontend/src/pages/organization/RoleByIDPage/components/RolePermissionsSection/RolePermissionsSection.tsx:94-99 — After saving a policy that was added via 'Add Policy' but never given any actions, the accordion row for that subject persists showing 'No Access' until page reload. This happens because the new reset(el) call (introduced by this PR) resets the form with permissions[subject] = {} still present, and the render guard permissions?.[subject] \!== undefined treats {} as defined — so the row stays visible despite nothing being saved to the API.

    Extended reasoning...

    What the bug is and how it manifests

    When a user clicks 'Add Policy' and selects a subject from the popover, handleSelectPolicy in OrgPolicySelectionModal.tsx calls form.setValue with an empty object {} for that subject. If the user then clicks Save without selecting any actions, a phantom accordion row continues to display 'No Access' for that subject even though the save succeeded and no permissions were stored.

    The specific code path that triggers it

    1. OrgPolicySelectionPopover adds the subject by calling form.setValue('permissions.gateway', {}) — sets the value to an empty object.
    2. The user clicks Save without choosing any actions.
    3. formRolePermission2API iterates Object.entries(formVal || {}). At line ~860 the guard if (\!actions) return does not fire because {} is truthy. Object.entries({}) is empty, so no permissions are pushed for the subject. The API payload correctly omits the subject.
    4. The API saves successfully with no permissions for the subject.
    5. reset(el) is called (added by this PR at line 97 of RolePermissionsSection.tsx). el.permissions[subject] is still {} — the Zod schema for that field is .optional(), which passes empty objects through without stripping them.
    6. After the reset, permissions?.[subject] \!== undefined evaluates to true for {}, so the accordion row remains mounted in the JSX and renders 'No Access'.
    7. On page reload, rolePermission2Form(role.permissions) does not reconstruct the subject (nothing was saved), so the row disappears.

    Why existing code does not prevent it

    The render guard at every conditional in RolePermissionsSection.tsx is permissions?.[subject] \!== undefined. An empty object {} satisfies this check. There is no filter that treats {} as equivalent to undefined before calling reset. Prior to this PR, reset() was called with no arguments, which reset to the last-loaded query cache value (which would not include the empty-object subject), preventing the phantom row.

    Impact

    The UX consequence is that administrators see a 'No Access' row for a subject they never actually saved permissions for. The form reports isDirty = false after save, so there is no visual cue that the state is stale. The row only disappears on a full page reload. No security impact — no permissions are granted. The confusion can lead admins to believe their save failed or that a 'No Access' policy was explicitly applied.

    How to fix

    The simplest fix is to strip permission entries whose value is an empty object (or where all action values are false/undefined) before calling reset. For example, before reset(el), filter el.permissions to exclude entries where Object.values(v).every(b => \!b). Alternatively, call reset() after query invalidation and refetch, which restores the pre-reset(el) behavior for this edge case while still clearing dirty state for saved entries.

    Step-by-step proof

    1. On the Org Roles page, edit a custom role that has no existing permissions.
    2. Click 'Add Policies' and select e.g. 'Gateways' — permissions.gateway is now {}.
    3. Do not select any actions. Click 'Save'.
    4. Observe: the save notification appears, but the 'Gateways' accordion row remains visible showing 'No Access'. isDirty is false.
    5. Reload the page: the 'Gateways' row is gone, confirming nothing was persisted.
    6. The phantom row was caused by reset(el) preserving permissions.gateway = {} in form state.

Comment on lines +583 to +600
[OrgPermissionSubjects.Groups]: {
title: "Group Management",
description: "Organize users into groups for bulk permission management",
actions: [
{
value: OrgPermissionGroupActions.Read,
label: "Read Groups",
description: "View groups and their members"
},
{
value: OrgPermissionGroupActions.Create,
label: "Create Groups",
description: "Create new user groups"
},
{
value: OrgPermissionGroupActions.Edit,
label: "Edit Groups",
description: "Update group membership and settings"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The new ORG_PERMISSION_OBJECT[Groups].actions introduced by this PR lists only 7 actions, missing AddIdentities = 'add-identities' and RemoveIdentities = 'remove-identities' that exist in the backend enum (backend/src/ee/services/permission/org-permission.ts:96-106). Because groupPermissionSchema in OrgRoleModifySection.utils.ts has no keys for these two actions and Zod uses strip mode by default, any role with add-identities or remove-identities permissions will silently lose them the first time an admin opens and saves the role through the new UI. The backend actively enforces both actions in group-service.ts (lines 967, 978, 1135, 1145) and grants them to the admin role (lines 416, 419).

Extended reasoning...

What the bug is and how it manifests

The frontend OrgPermissionGroupActions enum (frontend/src/context/OrgPermissionContext/types.ts:122-130) defines only 7 actions: Read, Create, Edit, Delete, GrantPrivileges, AddMembers, RemoveMembers. The backend enum (backend/src/ee/services/permission/org-permission.ts:96-106) defines 9 actions, additionally including AddIdentities = 'add-identities' and RemoveIdentities = 'remove-identities'. These backend actions are not dead code — group-service.ts enforces them at lines 967 and 978 (adding machine identities to groups) and lines 1135 and 1145 (removing machine identities from groups), and the admin role builder grants them at lines 416 and 419.

The specific code path that triggers it

This PR introduces ORG_PERMISSION_OBJECT[OrgPermissionSubjects.Groups].actions (OrgRoleModifySection.utils.ts lines ~583–618) with exactly 7 entries — cementing the incomplete frontend list as the authoritative source for the new UI. The groupPermissionSchema (the permissions.groups Zod schema in formSchema) only contains keys for the 7 frontend actions. Zod's z.object() strips unknown keys by default (no .passthrough()), so when zodResolver runs during handleSubmit, any add-identities or remove-identities values are silently removed before formRolePermission2API serializes the payload. The resulting API call permanently deletes these permissions from the role.

Why existing code doesn't prevent it

The rolePermission2Form() function populates the form by iterating the raw API response without Zod validation, so the permissions are present in the initial form state. But on submit, zodResolver enforces the schema and strips the unknown keys. There is no warning or error — the stripped keys are simply absent from the submitted permissions array. The ORG_PERMISSION_OBJECT[Groups].actions list introduced by this PR also means the multiselect UI can never display or allow selection of these two actions, making it impossible for admins to grant them through the new UI at all.

What the impact is

Any organization role that was granted add-identities or remove-identities permissions (e.g., the built-in admin role or a custom role configured via API or the old UI) will permanently lose those permissions the first time an admin opens it in the new redesigned editor and clicks Save. The user receives a success notification with no indication that permissions were dropped. Subsequently, any machine identity added to a group via a role relying on these permissions will get an authorization error from the backend enforcer.

Step-by-step proof

  1. A role has groups.add-identities: true set (e.g., via API or the admin role).
  2. An admin opens the Org Roles page, clicks the role, and the new UI loads: rolePermission2Form() populates permissions.groups including add-identities: true. This value is in the React Hook Form state.
  3. The admin makes any change (or just clicks Save without changes, since the initial isDirty check aside, the form submits the zodResolver-validated value).
  4. handleSubmit runs zodResolver(formSchema)formSchema.permissions.groups is a Zod object with only 7 known keys. Zod strips add-identities and remove-identities.
  5. formRolePermission2API receives the stripped permissions; the resulting TPermission[] array omits both actions.
  6. updateRole is called with the stripped array. The backend saves the role without add-identities/remove-identities. No error is returned.
  7. The admin sees "Successfully updated role" — the permissions were silently deleted.

How to fix it

Add AddIdentities and RemoveIdentities to the frontend OrgPermissionGroupActions enum, add corresponding entries to ORG_PERMISSION_OBJECT[Groups].actions, and add the two keys to groupPermissionSchema (the permissions.groups Zod object). This ensures the new multiselect UI can display and set both actions, and that Zod no longer strips them on save.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the placement of the comment, general UI feedback:

Image

1 - Let's use v3 here with the organization variant
2 - Let's also use v3 select here
3 - We actually don't have delete buttons for org permissions. If we look at the old version we had all of them available all the time

Image

And I think that makes sense with the dropdown on the right

Image

Because you can easily know if the role has access or not without having to open the accordion.

But, I also see value in your proposal with the policy selection dropdown and the delete button, but in this case, the dropdown on the right loses most of its value, because if the access policy is not on the list, then you don't have access.

I would put this for voting in the UI/UX channel.

Backend-wise, this changes nothing. So either one works!

The only extra thing we would need to add is validation if you try to add an empty policy, as we have in the project permission.

4 - The background color here is not the same as the one in the project permissions

Image

And if we ended up going with the delete option on 3, then we could actually use the exact same component as we have in the project permissions, not only the filterable select.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 1, 2, and 4.

Kept 3 since we discussed the UX w/ Scott and decided to keep this behavior.

Comment on lines +105 to +107
const hasPermissions = Object.values(permissions || {}).some((v) => v !== undefined);

const invalidSubjectsForAddPolicy = isRootOrganization ? [] : INVALID_SUBORG_PERMISSIONS;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The hasPermissions check on line 105 of RolePermissionsSection.tsx counts all defined permission subjects regardless of isRootOrganization context, causing a non-root org role that has only root-org-specific permissions (Sso, Ldap, Scim, GithubOrgSync, GithubOrgSyncManual, Billing, SubOrganization) to render an empty accordion wrapper instead of the 'No policies applied' empty state. This requires an API-created role in a non-root org with exclusively root-org-specific permissions, and the UX impact is cosmetic only. Fix: change to Object.entries(permissions || {}).some(([k, v]) => v !== undefined && !invalidSubjectsForAddPolicy.includes(k)).

Extended reasoning...

What the bug is

In RolePermissionsSection.tsx (line 105), hasPermissions is computed as:

const hasPermissions = Object.values(permissions || {}).some((v) => v !== undefined);

This check does not account for which permission subjects are actually renderable given the isRootOrganization context.

The specific code path that triggers it

INVALID_SUBORG_PERMISSIONS contains exactly 7 root-org-only subjects: Sso, Ldap, Scim, GithubOrgSync, GithubOrgSyncManual, Billing, SubOrganization. For non-root orgs, all 7 are excluded from accordion rendering:

  • Ldap/Scim/GithubOrgSync: filtered by the SIMPLE_PERMISSION_SUBJECTS.filter check at line 163–164 via !INVALID_SUBORG_PERMISSIONS.includes(subject)
  • Sso/GithubOrgSyncManual/Billing/SubOrganization: gated by {isRootOrganization && ...} guards in JSX

However, hasPermissions on line 105 uses Object.values(...).some(v => v !== undefined), which checks ALL subjects including those 7. If a non-root org role has permissions set only for subjects in INVALID_SUBORG_PERMISSIONS (possible via API creation), hasPermissions=true causes the {hasPermissions && <UnstableAccordion>} branch to render — but zero rows render inside it.

Step-by-step proof

  1. A custom role is created via API in a non-root org with { sso: { read: true } } (or any other root-org-only subject).
  2. rolePermission2Form converts the API response, setting permissions.sso = { read: true } in form state.
  3. hasPermissions = Object.values({ sso: { read: true } }).some(v => v !== undefined) evaluates to true.
  4. The {hasPermissions && <UnstableAccordion>} branch renders the accordion wrapper.
  5. Inside the accordion, OrgPermissionSsoRow is gated by {isRootOrganization && ...} — false for non-root orgs — so no rows render.
  6. Result: an empty <UnstableAccordion> wrapper with no items, instead of the 'No policies applied' empty state component.

Why existing code does not prevent it

The invalidSubjectsForAddPolicy variable (line 110) is correctly computed as isRootOrganization ? [] : INVALID_SUBORG_PERMISSIONS, and is used to filter the 'Add Policy' popover. However, this same filtering logic is not applied to the hasPermissions computation on line 105, which was introduced by this PR as part of the new hide-if-undefined rendering model.

Impact

Low severity / nit. It is cosmetic only (empty accordion vs. empty state message), requires an uncommon edge case (API-created role with only root-org-specific permissions in a non-root org), and has no security or functional impact.

How to fix

Replace line 105 with:

const hasPermissions = Object.entries(permissions || {}).some(
  ([k, v]) => v !== undefined && !invalidSubjectsForAddPolicy.includes(k as OrgPermissionSubjects)
);

This ensures only permissions that will actually render a row contribute to the hasPermissions flag, so the empty state is correctly shown when a non-root org role has only root-org-specific permissions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this cache should be in the gitignore, I think you need to rebase this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore the placement of this comment:

Image

Let's make this blue, I think it's the "organization" variant.

Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to validate empty policies, the same way we do for project policies:

Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should aim to use the same components in both places and be consistent on the UI:

Image Image

I would go as far as removing the dropdown (now that we start empty) and showing the green border on the left. Make it the exact same view and component.

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional findings (outside current diff — PR may have been updated during review):

  • 🔴 frontend/src/pages/project/RoleDetailsBySlugPage/components/GeneralPermissionPolicies.tsx:97-103 — The visibleActions filter in GeneralPermissionPolicies.tsx (used by ActionsMultiSelect) contains project-specific logic that compares subject strings against ProjectPermissionSub.Identity ('identity') and ProjectPermissionSub.Groups ('groups') to conditionally hide grant-privileges. Because OrgPermissionSubjects.Identity and OrgPermissionSubjects.Groups share these exact same string values, the filter fires for org subjects too — permanently hiding the Grant Privileges action in the org role editor when it was not previously selected. Admins cannot grant grant-privileges for org Identity or org Groups roles through the new UI.

    Extended reasoning...

    What the bug is and how it manifests

    In the refactored GeneralPermissionPolicies.tsx, the ActionsMultiSelect component computes identityGrantPrivileges and groupsGrantPrivileges by reading rule?.[ProjectPermissionIdentityActions.GrantPrivileges] and rule?.[ProjectPermissionGroupActions.GrantPrivileges]. These values feed into a visibleActions filter (lines 129–139 in the file) that conditionally excludes the grant-privileges action from the dropdown unless the permission is already active.

    The critical defect is that this filter is a raw string comparison:

    • subject === ProjectPermissionSub.Identity (value: 'identity') matches OrgPermissionSubjects.Identity (also 'identity')
    • subject === ProjectPermissionSub.Groups (value: 'groups') matches OrgPermissionSubjects.Groups (also 'groups')
    • ProjectPermissionIdentityActions.GrantPrivileges === 'grant-privileges' matches OrgPermissionIdentityActions.GrantPrivileges (same string)
    • ProjectPermissionGroupActions.GrantPrivileges === 'grant-privileges' matches OrgPermissionGroupActions.GrantPrivileges (same string)

    The specific code path that triggers it

    When RolePermissionsSection.tsx (org) renders org Identity or Groups subjects via GeneralPermissionPolicies, the component receives subject='identity' or subject='groups'. Inside ActionsMultiSelect, the visibleActions filter checks if the subject equals ProjectPermissionSub.Identity — the string comparison succeeds because the enum values are identical. If identityGrantPrivileges is false (i.e., grant-privileges was not already set in the role), the filter excludes the action from the multiselect options. The action simply never appears in the dropdown.

    Why existing code does not prevent it

    The filter was written for the project roles context and relies on enum values that happen to be shared between project and org permission types. TypeScript enum members with the same underlying string values are indistinguishable at runtime. The refactoring that reused GeneralPermissionPolicies for org roles did not account for this collision.

    Impact

    Administrators creating or editing org custom roles cannot add the grant-privileges permission to org Identity or Groups subjects through the new UI. The option is permanently absent from the multiselect. Roles that already have this permission display it correctly (the filter checks rule?.['grant-privileges'] which is true), but any role that does not already hold the permission cannot have it added through the UI. The ORG_PERMISSION_OBJECT in the diff explicitly includes GrantPrivileges in both Identity and Groups actions, confirming this is an intentional permission.

    Step-by-step proof

    1. Navigate to Org Roles, edit a custom role, click 'Add Policies', add the Identity subject.
    2. Open the Identity accordion row. The multiselect renders action options from ORG_PERMISSION_OBJECT[Identity].actions, which includes GrantPrivileges = 'grant-privileges'.
    3. ActionsMultiSelect receives subject='identity' and runs the visibleActions filter: subject === ProjectPermissionSub.Identity'identity' === 'identity'true.
    4. The filter checks identityGrantPrivileges = Boolean(rule?.['grant-privileges']) = false (not yet selected).
    5. The condition \!identityGrantPrivileges is true, so grant-privileges is filtered out of the visible options.
    6. The multiselect shows all other Identity actions but never shows Grant Privileges. The admin cannot select it.
    7. The same logic fires for subject='groups' with groupsGrantPrivileges.

    How to fix it

    The visibleActions filter should only apply to project permission subjects. One approach is to guard the identity/groups checks with an explicit project-context check, e.g., only filter when the subject is known to be a project subject (by checking against ProjectPermissionSub enum values exclusively, or by passing an isProjectPermission prop). Alternatively, the filter logic could be moved out of GeneralPermissionPolicies into the project-specific caller.

  • 🔴 frontend/src/pages/organization/SettingsPage/components/ProjectTemplatesTab/components/EditProjectTemplateSection/components/ProjectTemplateEditRoleForm.tsx:200-206 — The PR changed the trash icon visibility in GeneralPermissionPolicies from !isDisabled to !isDisabled && (fields.length > 1 || isConditional || !!onRemoveLastRule), but ProjectTemplateEditRoleForm.tsx was not updated to pass onRemoveLastRule. For non-conditional subjects (Members, Settings, Integrations, etc.) with exactly one rule, all three conditions are false so the trash icon never renders — users editing project template roles can no longer delete individual permission rows for these subjects. Fix: pass onRemoveLastRule to GeneralPermissionPolicies in ProjectTemplateEditRoleForm.tsx, mirroring the pattern already applied in RolePermissionsSection.tsx.

    Extended reasoning...

    What the bug is and how it manifests

    This PR refactored GeneralPermissionPolicies.tsx so that the trash icon (delete rule button) is now conditionally rendered at line 354:

    {!isDisabled && (fields.length > 1 || isConditional || !!onRemoveLastRule) && (

    Before this PR, the condition was simply !isDisabled, so the trash icon was always visible when the form was editable. After the PR, showing the icon requires at least one of: multiple rules, a conditional subject, or an onRemoveLastRule callback.

    The specific code path that triggers it

    ProjectTemplateEditRoleForm.tsx (lines 200-211 in the diff) renders GeneralPermissionPolicies without isConditional or onRemoveLastRule. For non-conditional subjects (e.g., Members, Settings, Integrations, Environments) with exactly one rule: fields.length > 1 = false, isConditional = undefined, !!onRemoveLastRule = false. The guard evaluates to !false && (false || false || false) = false — the trash icon is never rendered.

    Why existing code does not prevent it

    RolePermissionsSection.tsx (the project roles page) was correctly updated in this same PR to pass onRemoveLastRule. TypeScript does not flag the missing optional prop as an error. The two callers of GeneralPermissionPolicies are structurally identical but only one was updated, introducing an asymmetry between the project roles editor and the project template role editor.

    Impact

    Admins editing project template roles can no longer remove individual permission rows for any non-conditional subject that has a single rule. The only workaround is to use the Add Policies toggle to remove the entire subject (discarding all settings). This is a direct functional regression introduced by this PR refactoring.

    Step-by-step proof

    1. Navigate to Organization Settings > Project Templates > Edit a template > Edit a role.
    2. Expand a non-conditional permission subject (e.g., Members) that has one rule.
    3. Before this PR: trash icon is visible and functional (!isDisabled was always sufficient).
    4. After this PR: GeneralPermissionPolicies receives isConditional=undefined, onRemoveLastRule=undefined, and fields.length=1. Line 354 evaluates to !false && (false || false || false) = false. The trash icon IconButton is never rendered.
    5. The user has no per-row delete affordance — rules appear locked in.

    How to fix

    Pass onRemoveLastRule to GeneralPermissionPolicies in ProjectTemplateEditRoleForm.tsx, following the exact pattern used in RolePermissionsSection.tsx:

    onRemoveLastRule={
    !isDisabled
    ? () => form.setValue(permissions.subject as any, [], { shouldDirty: true })
    : undefined
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants