feat: [WD-19336] Storage volumes permission checks #1134

Kxiru · 2025-03-04T23:48:04Z

Done

Storage Volumes page

if instance view permission is missing we should disable linking to the instance
if image view permission is missing we should disable linking to the image list
disable create volume button if user does not have permission
disable add snapshot button if user does not have permission
disable button if user does not have permission to delete a SV (DeleteStorageVolumeComponent x2)
disable rename header if permission is missing
disable SV migrate button if user does not have permission
disable SV duplicate button if user does not have permission for any projects

See Storage Volume Permission Actions for full list of implemented / pending actions to permission check.

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:
- [List the steps to QA the new feature(s) or prove that a bug has been resolved]

Screenshots

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

webteam-app · 2025-03-04T23:48:10Z

Demo

Jenkins

demos.haus

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

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

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

edlerd

This is a good direction. Some comments on the draft below.

edlerd · 2025-03-05T08:09:00Z

src/api/instances.tsx

@@ -21,6 +21,7 @@ export const instanceEntitlements = [
  "can_delete",
  "can_edit",
  "can_exec",
+  "can_view",


Suggested change

"can_view",

We never check view permission explicitly. If a user knows an entity exists, they can view it. We can always infer this permission.

edlerd · 2025-03-05T08:10:53Z

src/pages/storage/MigrateVolumeBtn.tsx

@@ -157,7 +159,7 @@ const MigrateVolumeBtn: FC<Props> = ({
        type="button"
        className={classname}
        loading={isVolumeLoading}
-        disabled={isVolumeLoading}
+        disabled={!canEditVolume(storageVolume) || isVolumeLoading}


We should always add a title explaining why something is disabled. In this new case, add a copy about the missing permission. See other places in the codebase for example copy. The loading state has other cues for why the button is disabled, so we don't need a title for those.

edlerd · 2025-03-05T08:11:47Z

src/pages/storage/StorageVolumeHeader.tsx

@@ -123,7 +125,9 @@ const StorageVolumeHeader: FC<Props> = ({ volume, project }) => {
      renameDisabledReason={
        (volume.used_by?.length ?? 0) > 0
          ? "Can not rename, volume is currently in use."
-          : undefined
+          : !canEditVolume(volume)


Readability will benefit from extracting the logic into a function with early exits instead of nested ternaries.

edlerd · 2025-03-05T08:14:21Z

src/pages/storage/StorageVolumeNameLink.tsx


  return (
    <div className={classnames("u-flex", className)}>
      <div
        className={classnames("u-truncate", "volume-name-link")}
-        title={caption}
+        title={volume.name}


The title should match the truncated contents.

Suggested change

title={volume.name}

title={caption}

edlerd · 2025-03-05T08:17:33Z

src/pages/storage/StorageVolumeNameLink.tsx

+  const caption = overrideName
+    ? displayLink
+      ? overrideName
+      : ""
+    : volume.name;


If I understand correctly, this is to not render anything in the right-hand column on the volume list. Currently, we still render some markup with titles and empty string as content in the case that link should be hidden. Better simplify and avoid the empty markup, as suggested below?

Might be worth adding a comment or solving this in a more straight forward way to make it easy to understand.

Suggested change

const caption = overrideName

? displayLink

? overrideName

: ""

: volume.name;

const caption = overrideName ? overrideName : volume.name;

if (overrideName && !displayLink) {

return null;

}

edlerd · 2025-03-05T08:19:00Z

src/pages/storage/actions/CreateVolumeBtn.tsx

@@ -27,6 +35,7 @@ const CreateVolumeBtn: FC<Props> = ({ project, className, defaultPool }) => {
      hasIcon={!isSmallScreen}
      onClick={handleAdd}
      className={className}
+      disabled={!canCreateStorageVolumes(project)}


Please add a title.

edlerd · 2025-03-05T08:19:20Z

src/pages/storage/actions/DuplicateVolumeBtn.tsx

@@ -31,6 +33,7 @@ const DuplicateVolumeBtn: FC<Props> = ({ volume, className, onClose }) => {
        className={className}
        onClick={openPortal}
        title="Duplicate volume"
+        disabled={!canCreateStorageVolumes(volume)}


Title please

edlerd · 2025-03-05T08:21:07Z

src/pages/storage/actions/snapshots/VolumeAddSnapshotBtn.tsx

+              : !canManageStorageVolumeSnapshots(volume)
+                ? "You do not have permission to create or delete snapshots of this volume."
+                : "Add Snapshot"


Probably better to extract this into a function with early exits as well. Then the disabled check below can use that same function. See how DeleteStoragePoolBtn is doing it for an example.

Kxiru added 3 commits March 4, 2025 14:56

feat: [Permission Checks] SV -> Link to Instance

23d863f

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

feat: [Permission Checks] SV -> Link to Image

415e421

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

feat: [Permission Checks] SV -> Can create storage volume

e9d9c0c

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

Kxiru added Review: Code needed Review: QA needed labels Mar 4, 2025

Kxiru requested a review from edlerd March 4, 2025 23:48

Kxiru self-assigned this Mar 4, 2025

Kxiru changed the title ~~Storage volumes permission checks~~ feat: [WD-19336] Storage volumes permission checks Mar 5, 2025

Kxiru force-pushed the storage-volumes-permission-checks branch from 1322d75 to 64663ef Compare March 5, 2025 01:15

feat: [Permission Checks] SV -> Can manage storage volume snapshots

d3efcd7

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

Kxiru force-pushed the storage-volumes-permission-checks branch from 64663ef to d3efcd7 Compare March 5, 2025 01:22

Kxiru added 3 commits March 5, 2025 01:43

feat: [Permission Checks] SV -> Can edit Storage volume headers/names

32d69b8

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

feat: [Permission Checks] SV -> Disable migrate volume button based o…

7be5c86

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

feat: [Permission Checks] SV -> Can duplicate Storage volumes

e31ebd6

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

edlerd reviewed Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [WD-19336] Storage volumes permission checks #1134

feat: [WD-19336] Storage volumes permission checks #1134

Kxiru commented Mar 4, 2025 •

edited

Loading

webteam-app commented Mar 4, 2025

edlerd left a comment

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

edlerd Mar 5, 2025

feat: [WD-19336] Storage volumes permission checks #1134

Are you sure you want to change the base?

feat: [WD-19336] Storage volumes permission checks #1134

Conversation

Kxiru commented Mar 4, 2025 • edited Loading

Done

QA

Screenshots

webteam-app commented Mar 4, 2025

edlerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kxiru commented Mar 4, 2025 •

edited

Loading