feat: [WD-16894] Add bulk deletion and group modification of TLS Users #1008

Kxiru · 2024-11-26T02:11:11Z

Done

[✔] Allow selection of tls identities in the permission- > identity list
[✔] Use the new DELETE 1.0/auth/identities/tls/:id endpoint for the bulk delete
[✔] Unhide “delete” and “modify groups” button for tls users of the new type in the identities list
[✔] Modify bulk actions "delete" and "modify groups" on the identity list page to also allow inclusion of tls users
[✔] Legacy TLS users should be disabled, not deleted.

QA

Run the LXD-UI:

On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access.
With a local copy of this branch, build and run as described in the docs.

Perform the following QA steps:

Create "new" TLS Fine grained identities

To add a new TLS Identity Client (Pending) please follow the steps for creating a pending fine-grained TLS identity in the 'Authenticate with the LXD server' section of the LXD documentation.
Be sure to use the API instructions to create pending fine grained identities, as the CLI instructions require a remote to be specified.
Be sure to authenticate the client by following the instructions through step 2.

Delete TLS Users

Navigate to the Permissions > Identities
Attempt to delete individual TLS Client (Pending) identities using the inline delete identity button.
Attempt to delete bulk TLS Client (Pending) identities.
Verify that Legacy TLS identities (Client Certificate (Unrestricted) identities) cannot be selected via the checkbox or modified/deleted via inline buttons.

Add TLS users users to a group

Navigate to Permissions > Groups
Create a new group and add permissions to the group.
Navigate to the Permissions > Identities
Attempt to add an individual TLS identity to a group using the inline modify groups button.
Attempt to add several TLS identities (Pending) to a group.

Test TLS user permissions / Login as a TLS User

Concatenate the .key and .crt files to create a .pem file using the following command:
cat <KEY-FILE> <CRT-FILE> > <PEM-FILE-NAME>.pem
Change lines 11 and 22 of the haproxy-dev.cfg file to the following, respectively.
bind 0.0.0.0:8407 ssl verify optional crt <PEM-FILE-PATH> ca-file <CRT-FILE-PATH>
server lxd_https LXD_UI_BACKEND_IP:8443 ssl verify none crt <PEM-FILE-PATH>
TLS Fine grained identities have been tested with the following permissions:

Screenshots

webteam-app · 2024-11-26T02:11:18Z

Demo

Jenkins

demos.haus

edlerd

Some comments below, I think this is still missing to address the requirement "Legacy TLS users should be disabled, not deleted.". But maybe that is why it is still in draft state.

src/pages/permissions/PermissionIdentities.tsx

src/pages/permissions/actions/DeleteIdentityBtn.tsx

src/util/instanceBulkActions.tsx

mas-who · 2024-12-03T07:54:29Z

Good work so far @Kxiru for figuring out the difference between legacy and fine-grained tls identities. I think before conducting a proper review for the PR, there are a few items I'd like to see covered first:

Please would you adjust the PR title to reflect that group modification for tls identities are included in this PR
Please tidy up the PR description with respect to formatting, correctness and clarity (If you are not sure about this one, please reach out to me). When we merge the PR, the description becomes part of the merge commit message so we should keep it neat.
Since this PR includes modifying groups for a tls identity, have you QA'd locally if permissions work for those identities? If not, then you should do so by getting the actual certificate for the tls identity from the lxd server, add it to our haproxy configs and "log in" as the tls identity to test group permissions (can chat about this if you need direction for what to do).
Please also include detailed QA steps in the PR description for testing tls identity deletion and group modification.

mas-who

Nice update on the PR details 🔥

I've left some code related comments so far. Will QA tomorrow.

src/pages/permissions/PermissionIdentities.tsx

src/pages/permissions/actions/DeleteIdentityBtn.tsx

src/pages/permissions/panels/EditIdentitiesForm.tsx

mas-who · 2024-12-09T08:58:20Z

QA comments

When the screen is large enough, I think the link chip should show the full content and not truncate. Currently, the chip for tls certificate identities are truncated.
When bulk editing identity groups, upon completion, the tls identities are de-selected whereas the oidc identities remain selected. We should keep things consistent i.e. keep identities selected after modifying groups which corresponds to the original behaviour.
Screencast from 09-12-2024 10:48:20.webm
The table header seems incorrect when selecting multiple identities e.g. "3 identities selected. Select all 5 identities" (see example below). The correct heading should mention "Select all 3 identities" which corresponds to the maximum number of selectable identities in the table.
The notification message for deleting an identity still includes the auth method, it should be removed. Furthermore, the identity I deleted in the example below is a tls certificate, the icon included in the notification is incorrect.

mas-who

@Kxiru I think you didn't push the changes for the review comments. Please would you double check that. I have unresolved the comments for now.

src/pages/permissions/PermissionIdentities.tsx

Kxiru · 2024-12-09T11:00:28Z

@Kxiru I think you didn't push the changes for the review comments. Please would you double check that. I have unresolved the comments for now.

Apologies, you are right, I hadn't pushed the changes.
I will review push the changes now and address the QA comments (2 of which are already addressed within my changes) in a further commit amend.

Kxiru · 2024-12-09T11:34:53Z

QA comments

When the screen is large enough, I think the link chip should show the full content and not truncate. Currently, the chip for tls certificate identities are truncated.
...

Comments 3 and 4 have been addressed in my last commit. Comment no.2 can not be recreated on my machine and therefore I believe that I have addressed this in my last commit as well. I'll address the dynamic identity chips now :).

mas-who

Almost there, some small nits

src/pages/permissions/PermissionIdentities.tsx

src/pages/permissions/actions/DeleteIdentityBtn.tsx

src/pages/permissions/PermissionIdentities.tsx

mas-who · 2024-12-09T15:12:56Z

Found another minor QA issue, you may need to remove the margin below the buttons for a fine-grained identity. Currently it's causing the rows in the identities table to have unequal height.

mas-who · 2024-12-09T15:22:27Z

After completing group modification for mixed type identities, I am still seeing that tls identities gets de-selected.

Screencast.from.09-12-2024.17.20.37.webm

src/components/ResourceLabel.tsx

edlerd

Two ideas to avoid duplication and make the code a bit cleaner.

src/pages/permissions/PermissionIdentities.tsx

Kxiru · 2024-12-10T15:03:34Z

Addiitonal feature: Enable user feedback when the user is deleitng an identity such as a spinner.

src/components/IdentityResource.tsx

src/pages/permissions/actions/BulkDeleteIdentitiesBtn.tsx

mas-who · 2024-12-11T08:08:47Z

@Kxiru just noticed the tests are failing, looking at the failure, it's probably due to the change where we no longer reset identities selection after modifying their groups

edlerd

Thanks for the iteration on this one. It seems very good already, a handful of comments for things that I discovered that can be a bit simpler or improved slightly. Most are tiny changes.

src/components/ResourceLabel.tsx

src/pages/permissions/PermissionIdentities.tsx

src/pages/permissions/actions/DeleteIdentityBtn.tsx

src/pages/permissions/panels/EditIdentitiesForm.tsx

src/pages/permissions/actions/BulkDeleteIdentitiesBtn.tsx

Signed-off-by: Nkeiruka <[email protected]>

edlerd

QA and Code LGTM, good stuff :)

@webteam-app

## Done - Created "Create Identity" Button - Create modal to create new TLS fine grained identities. - A few new functions and a new LXDTrustToken type. ## User Management Spike [✔️] A user needs to be able to create a TLS user (Creates a pending user) [✔️] A user needs to be able to destroy a TLS user ([WD-16894](#1008)) Fixes: - Inability to create TLS fine grained tokens through the UI. ## QA 1. Run the LXD-UI: - On the demo server via the link posted by @webteam-app below. This is only available for PRs created by collaborators of the repo. Ask @mas-who or @edlerd for access. - With a local copy of this branch, [build and run as described in the docs](../CONTRIBUTING.md#setting-up-for-development). 2. Perform the following QA steps: - [List the steps to QA the new feature(s) or prove that a bug has been resolved] ## Screenshots ![image](https://github.com/user-attachments/assets/9549e7f1-c217-4d45-8afa-9ef2c1845f43) ![Screenshot from 2024-12-10 22-47-10](https://github.com/user-attachments/assets/71f60924-98ac-47a1-a0c4-7498792610a4) [WD-16894]: https://warthogs.atlassian.net/browse/WD-16894?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

Kxiru added Review: Code needed Review: QA needed labels Nov 26, 2024

Kxiru requested review from edlerd and mas-who November 26, 2024 02:11

Kxiru self-assigned this Nov 26, 2024

edlerd reviewed Nov 26, 2024

View reviewed changes

src/pages/permissions/PermissionIdentities.tsx Outdated Show resolved Hide resolved

src/pages/permissions/actions/DeleteIdentityBtn.tsx Outdated Show resolved Hide resolved

src/util/instanceBulkActions.tsx Show resolved Hide resolved

Kxiru marked this pull request as ready for review December 2, 2024 22:05

Kxiru requested a review from edlerd December 2, 2024 22:06

Kxiru force-pushed the add-bulk-tls-deletion branch from e8d3c8b to 6da6cf8 Compare December 2, 2024 22:07

Kxiru changed the title ~~feat: [WD-15663] Add bulk deletion of TLS Users~~ feat: [WD-16894] Add bulk deletion of TLS Users Dec 2, 2024

Kxiru changed the title ~~feat: [WD-16894] Add bulk deletion of TLS Users~~ feat: [WD-16894] Add bulk deletion and group modification of TLS Users Dec 3, 2024

mas-who reviewed Dec 4, 2024

View reviewed changes

Kxiru requested a review from mas-who December 5, 2024 16:54

mas-who reviewed Dec 9, 2024

View reviewed changes

Kxiru force-pushed the add-bulk-tls-deletion branch from 6da6cf8 to a4fcbd3 Compare December 9, 2024 11:09

mas-who reviewed Dec 9, 2024

View reviewed changes

Kxiru force-pushed the add-bulk-tls-deletion branch from a4fcbd3 to 8ed4442 Compare December 9, 2024 13:43

Kxiru requested a review from mas-who December 9, 2024 13:44

mas-who reviewed Dec 9, 2024

View reviewed changes

src/components/ResourceLabel.tsx Outdated Show resolved Hide resolved

edlerd reviewed Dec 9, 2024

View reviewed changes

src/pages/permissions/PermissionIdentities.tsx Outdated Show resolved Hide resolved

src/pages/permissions/PermissionIdentities.tsx Outdated Show resolved Hide resolved

Kxiru mentioned this pull request Dec 10, 2024

feat: [WD-16972] TLS user management spike. #1026

Merged

Kxiru force-pushed the add-bulk-tls-deletion branch from 8ed4442 to 4256bfb Compare December 11, 2024 00:58

Kxiru requested review from edlerd and mas-who December 11, 2024 01:12

mas-who reviewed Dec 11, 2024

View reviewed changes

src/components/IdentityResource.tsx Outdated Show resolved Hide resolved

src/pages/permissions/actions/BulkDeleteIdentitiesBtn.tsx Outdated Show resolved Hide resolved

edlerd reviewed Dec 12, 2024

View reviewed changes

feat: [WD-16894] Add bulk deletion of TLS Users

a33ba9f

Signed-off-by: Nkeiruka <[email protected]>

Kxiru force-pushed the add-bulk-tls-deletion branch from 4256bfb to a33ba9f Compare December 12, 2024 17:23

Kxiru requested review from mas-who and edlerd December 12, 2024 17:23

edlerd approved these changes Dec 12, 2024

View reviewed changes

Kxiru merged commit 87d33b5 into canonical:main Dec 12, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [WD-16894] Add bulk deletion and group modification of TLS Users #1008

feat: [WD-16894] Add bulk deletion and group modification of TLS Users #1008

Kxiru commented Nov 26, 2024 •

edited

Loading

webteam-app commented Nov 26, 2024

edlerd left a comment

mas-who commented Dec 3, 2024

mas-who left a comment

mas-who commented Dec 9, 2024

mas-who left a comment

Kxiru commented Dec 9, 2024 •

edited

Loading

Kxiru commented Dec 9, 2024

mas-who left a comment

mas-who commented Dec 9, 2024

mas-who commented Dec 9, 2024

edlerd left a comment

Kxiru commented Dec 10, 2024

mas-who commented Dec 11, 2024

edlerd left a comment

edlerd left a comment

feat: [WD-16894] Add bulk deletion and group modification of TLS Users #1008

feat: [WD-16894] Add bulk deletion and group modification of TLS Users #1008

Conversation

Kxiru commented Nov 26, 2024 • edited Loading

Done

QA

Screenshots

webteam-app commented Nov 26, 2024

Demo

Jenkins

demos.haus

edlerd left a comment

Choose a reason for hiding this comment

mas-who commented Dec 3, 2024

mas-who left a comment

Choose a reason for hiding this comment

mas-who commented Dec 9, 2024

mas-who left a comment

Choose a reason for hiding this comment

Kxiru commented Dec 9, 2024 • edited Loading

Kxiru commented Dec 9, 2024

mas-who left a comment

Choose a reason for hiding this comment

mas-who commented Dec 9, 2024

mas-who commented Dec 9, 2024

edlerd left a comment

Choose a reason for hiding this comment

Kxiru commented Dec 10, 2024

mas-who commented Dec 11, 2024

edlerd left a comment

Choose a reason for hiding this comment

edlerd left a comment

Choose a reason for hiding this comment

Kxiru commented Nov 26, 2024 •

edited

Loading

Kxiru commented Dec 9, 2024 •

edited

Loading