Skip to content

feat: [WD-34915] Make Admin groups immutable#1898

Open
Kxiru wants to merge 1 commit intocanonical:mainfrom
Kxiru:lxd-admin-group-immutable
Open

feat: [WD-34915] Make Admin groups immutable#1898
Kxiru wants to merge 1 commit intocanonical:mainfrom
Kxiru:lxd-admin-group-immutable

Conversation

@Kxiru
Copy link
Copy Markdown
Contributor

@Kxiru Kxiru commented Mar 24, 2026

Done

  • Make admin groups immutable
  • Disable deletion of admins row, with explanatory hover text.
  • Disable selection of new permissions
  • Split Group + Edit Identity API calls so that you can still edit the admins inside the admins group. I do not think this applies anymore

Note, my admin group is called "admins". Confirm whether this is the default group name or if I have a custom one.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Navigate to Permissions > Groups
    • Observe that the above checkbox interactions behave as expected.

Screenshots

image

Fixes canonical/lxd#17986

@Kxiru Kxiru self-assigned this Mar 24, 2026
@webteam-app
Copy link
Copy Markdown

@Kxiru Kxiru force-pushed the lxd-admin-group-immutable branch 2 times, most recently from 9e8d3db to daea130 Compare March 24, 2026 19:05
@Kxiru Kxiru marked this pull request as ready for review March 24, 2026 19:06
Copy link
Copy Markdown
Contributor

@kimanhou kimanhou left a comment

Choose a reason for hiding this comment

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

Could you please include the QA steps in the description ?

Maybe we should also disable the name input and the description input in the edit side panel, instead of letting the user modify the name then receiving an error:

Image

I also don't like being able to click on Edit permissions and then not being able to do anything. Maybe we should disable the button from the side panel and displaying what the permissions are underneath. Or change the label of the button to See permissions and completely remove the Add permissions section (and even the search bar ?)

@omarelkashef
Copy link
Copy Markdown
Contributor

Should we also disable the edit button or the side panel input fields instead of having the user run into these errors?

image image

@Kxiru
Copy link
Copy Markdown
Contributor Author

Kxiru commented Mar 26, 2026

Should we also disable the edit button or the side panel input fields instead of having the user run into these errors?

@omarelkashef Yes, thank you for bringing this up

@Kxiru
Copy link
Copy Markdown
Contributor Author

Kxiru commented Mar 26, 2026

@kimanhou I think the Permissions button should stay but can be renamed to See Permissions. Pushing a new commit that addresses all changes and changes "Edit permissions" to "View permissions".

@Kxiru Kxiru force-pushed the lxd-admin-group-immutable branch from daea130 to 04715d6 Compare March 26, 2026 11:57
@Kxiru Kxiru requested a review from kimanhou March 26, 2026 11:57
Copy link
Copy Markdown
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Let's still allow changing memberships in the admins group via the group form. There shouldn't be an error below, but the change should go through by skipping the admin group update and directly altering the group members:

Image

@Kxiru Kxiru force-pushed the lxd-admin-group-immutable branch from 04715d6 to 6139e25 Compare March 26, 2026 16:18
@kimanhou
Copy link
Copy Markdown
Contributor

It might not be related to this PR, but in the "Add permissions" section in the edit side panel, even if the dropdown lists are disabled, the cursor is still pointer. It would be better to have a cursor not-allowed.

Tests fail.

Same remark as David: when you modify the identities, an error is thrown while it shouldn't.

@kimanhou
Copy link
Copy Markdown
Contributor

Related bug github issue on LXD repo: canonical/lxd#17986

@Kxiru
Copy link
Copy Markdown
Contributor Author

Kxiru commented Mar 27, 2026

Same remark as David: when you modify the identities, an error is thrown while it shouldn't.
Thanks for flagging, I couldn't reproduce this until I realised I was using 5.21. Working on this now.

Signed-off-by: Nkeiruka <nkeiruka.whenu@canonical.com>
@Kxiru Kxiru force-pushed the lxd-admin-group-immutable branch from 6139e25 to b0b73a2 Compare March 27, 2026 20:47
@Kxiru Kxiru requested a review from edlerd March 27, 2026 20:48
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.

Could not parse group: The admins group cannot be modified

5 participants