fix(rbac): enforce the storage rights on the content routes - #924
Open
shreyanshj10 wants to merge 1 commit into
Open
shreyanshj10 wants to merge 1 commit into
shreyanshj10 wants to merge 1 commit into
Conversation
shreyanshj10
marked this pull request as ready for review
September 11, 2026 08:53
shreyanshj10
force-pushed
the
fix/issue-920-storage-rights-enforcement
branch
from
September 11, 2026 09:03
12c58ce to
b5edd98
Compare
…pro#920) Browsing storage content gated on vm.view, while uploading, pulling an ISO from a URL and deleting a volume gated on connection.view, so clearing storage.content, storage.upload or storage.delete from a role changed nothing. Point each route at the right the catalogue advertises, and hide the delete control in the content browser unless the caller holds storage.delete. VM Admin gains storage.delete and Tenant Admin gains storage.content, storage.upload and storage.delete, so neither loses a capability it already exercised through connection.view. Tenant Admin needs storage.content in particular because the content listing feeds the disk and ISO pickers of the guest wizards.
shreyanshj10
force-pushed
the
fix/issue-920-storage-rights-enforcement
branch
from
September 20, 2026 05:57
b5edd98 to
9fb16ed
Compare
Author
|
Rebased onto the current main again so this stays mergeable. Let me know if you'd like anything changed or if the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The role catalogue defines
storage.content,storage.uploadandstorage.delete, and the built-in roles grant them, but the storage content routes checked other rights instead. Removing a storage right from a role had no effect:connection.viewwas enough to upload an ISO, fetch one from a URL or delete a volume, andvm.viewwas enough to browse storage content. The delete button in the content browser only looked atreadOnly, so an admin who withheldstorage.deletestill let users delete ISOs, templates and backups.Change
storage/[storage]/contentGETvm.viewstorage.contentstorage/[storage]/content/[volid]DELETEconnection.viewstorage.deletestorage/[storage]/download-urlPOSTconnection.viewstorage.uploadstorage/[storage]/uploadchunk + finalizeconnection.viewstorage.uploadStorageContentBrowsernow also requireshasPermission('storage.delete')before showing the delete button.The guest-perimeter fallback on the content listing from #262 is kept, just re-pointed at
storage.content, so vm/tag/pool scoped callers still reach the ISO picker. The vDC storage mask and tenant filename checks are unchanged.Two built-in roles pick up the rights they were already using via
connection.view, so nobody seeded from the catalogue loses anything: VM Admin gainsstorage.delete, Tenant Admin gainsstorage.content,storage.uploadandstorage.delete. Existing databases are not re-seeded, so operators with customised roles will want to add these.Not included
The issue also maps
connections/[id]/ceph/*andconnections/[id]/storagetostorage.admin. Those endpoints feed the VM/LXC wizards, hardware handlers, dashboard widgets and the Ceph views, so gating them on a dangerous right would hide unrelated pages from every role without it. That seems like a separate decision, happy to add it here if you prefer.Tests
content/route.test.tsandcontent/[volid]/route.test.tsnow assert the new rights. The upload and download-url routes had no tests, sostorage/[storage]/storageRights.test.tsdenies the permission and checks that all three call sites requirestorage.upload. Each of these fails against the old constants. eslint andtsc --noEmitare clean on the changed files.Fixes #920