* [PATCH access-control v2 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests
@ 2026-07-23 11:09 Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 1/2] test: api: add tests for ACL modification endpoint Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 2/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities Elias Huhsovitz
0 siblings, 2 replies; 3+ messages in thread
From: Elias Huhsovitz @ 2026-07-23 11:09 UTC (permalink / raw)
To: pve-devel; +Cc: Elias Huhsovitz
v1: https://lore.proxmox.com/pve-devel/20260720134535.136172-1-e.huhsovitz@proxmox.com/
This series fixes Bug #6510, which prevents the deletion of ACLs
for non-existent users, groups, or API tokens.
Patch 1/2 introduces automated tests for the
`PUT /access/acl` endpoint. The test suite verifies standard creation,
modification, and deletion workflows, enforces privilege boundaries,
and specifically tests the edge case resolved in the first patch.
Patch 2/2 modifies the `update_acl` API endpoint to bypass the
entity existence check when the `delete` flag is set. This change
allows administrators to clean up orphaned ACLs without weakening the
strict validation applied when adding new permissions.
changes v1->v2:
- declare all tests up front in $tests.
- run all tests using the same logic in loop.
- evaluate test results using `Test::More::is_deeply`.
- use redefine() instead of mock().
- use `no_auto => 1` for MockModule to prvent auto loading.
- run make tidy.
Elias Huhsovitz (2):
test: api: add tests for ACL modification endpoint
fix #6510: api: acl: allow deleting ACLs for non-existent entities
src/PVE/API2/ACL.pm | 7 +-
src/test/api-tests.pl | 2 +-
src/test/api-update-acl-test.pl | 297 ++++++++++++++++++++++++++++++++
3 files changed, 302 insertions(+), 4 deletions(-)
create mode 100644 src/test/api-update-acl-test.pl
--
2.47.3
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH access-control v2 1/2] test: api: add tests for ACL modification endpoint
2026-07-23 11:09 [PATCH access-control v2 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests Elias Huhsovitz
@ 2026-07-23 11:09 ` Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 2/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities Elias Huhsovitz
1 sibling, 0 replies; 3+ messages in thread
From: Elias Huhsovitz @ 2026-07-23 11:09 UTC (permalink / raw)
To: pve-devel; +Cc: Elias Huhsovitz
The `PUT /access/acl` endpoint previously lacked automated test
coverage. Introduce `api-update-acl-test.pl` to verify the ACL
modification logic.
The tests cover the following scenarios:
- Adding, modifying, and deleting ACLs for existing users, groups, and
tokens.
- Rejecting requests to add ACLs for non-existent entities.
- Enforcing privilege boundaries by rejecting calls lacking the
`Permissions.Modify` privilege.
- Deleting ACLs for non-existent entities.
Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
src/test/api-tests.pl | 2 +-
src/test/api-update-acl-test.pl | 297 ++++++++++++++++++++++++++++++++
2 files changed, 298 insertions(+), 1 deletion(-)
create mode 100644 src/test/api-update-acl-test.pl
diff --git a/src/test/api-tests.pl b/src/test/api-tests.pl
index a1a987a..e9c400a 100755
--- a/src/test/api-tests.pl
+++ b/src/test/api-tests.pl
@@ -7,6 +7,6 @@ use TAP::Harness;
my $harness = TAP::Harness->new({ verbosity => -1 });
-my $result = $harness->runtests('api-get-permissions-test.pl');
+my $result = $harness->runtests('api-get-permissions-test.pl', 'api-update-acl-test.pl');
exit -1 if $result->{failed};
diff --git a/src/test/api-update-acl-test.pl b/src/test/api-update-acl-test.pl
new file mode 100644
index 0000000..7099abf
--- /dev/null
+++ b/src/test/api-update-acl-test.pl
@@ -0,0 +1,297 @@
+#!/usr/bin/env perl
+
+use v5.36;
+
+use lib qw(..);
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::AccessControl;
+use PVE::RPCEnvironment;
+use PVE::API2::ACL;
+
+# This test suite verifies the `PUT /access/acl` API endpoint.
+#
+# It mocks the cluster filesystem and the RPC environment to isolate
+# the API logic from external dependencies. The tests pass mock
+# configuration hashes directly to the API handler and verify the
+# resulting state changes and error messages.
+#
+# The `get_base_cfg` function generates a clean, default configuration
+# hash. The `before_each` helper resets the configuration and all mock
+# behaviors before each test to isolate the test cases.
+
+my $current_cfg;
+
+# --- MOCKS ---
+
+my $api_acl_module = Test::MockModule->new('PVE::API2::ACL', no_auto => 1);
+$api_acl_module->noop('cfs_update');
+$api_acl_module->redefine(
+ 'cfs_read_file' => sub($filename) {
+ die "unknown file '$filename'\n" if $filename ne 'user.cfg';
+ return $current_cfg;
+ },
+ 'cfs_write_file' => sub($filename, $data, $force_utf8 = undef) {
+ die "unknown file '$filename'\n" if $filename ne 'user.cfg';
+ $current_cfg = $data;
+ },
+);
+
+my $acl_module = Test::MockModule->new('PVE::AccessControl', no_auto => 1);
+$acl_module->redefine(
+ 'lock_user_config' => sub($code, @args) {
+ return $code->(@args);
+ },
+);
+
+my $rpcenv_module = Test::MockModule->new('PVE::RPCEnvironment', no_auto => 1);
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+$rpcenv->set_user('admin@pve');
+
+my ($handler, $handler_info) = PVE::API2::ACL->find_handler('PUT', '');
+
+# --- HELPER FUNCTIONS ---
+
+sub get_base_cfg () {
+ return {
+ users => {
+ 'admin@pve' => {
+ enable => 1,
+ tokens => {
+ 'mytoken' => { privsep => 1 },
+ },
+ },
+ },
+ groups => { 'admin_group' => {} },
+ roles => {
+ 'PVEAdmin' => { 'Permissions.Modify' => 1 },
+ 'NoAccess' => {},
+ },
+ acl_root => {},
+ };
+}
+
+sub before_each () {
+ $current_cfg = get_base_cfg();
+
+ $rpcenv_module->redefine(
+ 'permissions' => sub {
+ return { 'Permissions.Modify' => 1 };
+ },
+ );
+}
+
+sub run_update_acl($params) {
+ my $result = eval { $handler->handle($handler_info, $params) };
+ return ($result, $@);
+}
+
+# --- TEST CASES ---
+
+my $tests = [
+ {
+ description => 'Add ACL for existing user succeeds',
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'PVEAdmin',
+ },
+ expected_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 1 } },
+ },
+ },
+ {
+ description => 'Delete ACL for existing user succeeds',
+ setup_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'PVEAdmin',
+ delete => 1,
+ },
+ expected_acl_root => {
+ users => { 'admin@pve' => {} },
+ },
+ },
+ {
+ description => 'Add ACL without Permissions.Modify fails',
+ permissions => {},
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'PVEAdmin',
+ },
+ expected_error => qr/requires 'Permissions.Modify'/,
+ expected_acl_root => {},
+ },
+ {
+ description => 'Modify existing ACL to add a second role',
+ setup_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'NoAccess',
+ },
+ expected_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 1, NoAccess => 1 } },
+ },
+ },
+ {
+ description => 'Modify existing ACL to change propagation flag',
+ setup_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'PVEAdmin',
+ propagate => 0,
+ },
+ expected_acl_root => {
+ users => { 'admin@pve' => { PVEAdmin => 0 } },
+ },
+ },
+ {
+ description => 'Modify existing ACL for group and token',
+ setup_acl_root => {
+ groups => { 'admin_group' => { PVEAdmin => 1 } },
+ tokens => { 'admin@pve!mytoken' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ groups => 'admin_group',
+ tokens => 'admin@pve!mytoken',
+ roles => 'NoAccess',
+ propagate => 0,
+ },
+ expected_acl_root => {
+ groups => { 'admin_group' => { PVEAdmin => 1, NoAccess => 0 } },
+ tokens => { 'admin@pve!mytoken' => { PVEAdmin => 1, NoAccess => 0 } },
+ },
+ },
+ {
+ description => 'Delete ACL for non-existing user',
+ setup_acl_root => {
+ users => { 'ghost@pve' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ users => 'ghost@pve',
+ roles => 'PVEAdmin',
+ delete => 1,
+ },
+ expected_acl_root => {
+ users => { 'ghost@pve' => {} },
+ },
+ },
+ {
+ description => 'Add ACL for non-existing user fails',
+ update_acl_params => {
+ path => '/',
+ users => 'ghost@pve',
+ roles => 'PVEAdmin',
+ },
+ expected_error => qr/user 'ghost\@pve' does not exist/,
+ expected_acl_root => {},
+ },
+ {
+ description => 'Delete ACL for non-existing group',
+ setup_acl_root => {
+ groups => { 'ghost_group' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ groups => 'ghost_group',
+ roles => 'PVEAdmin',
+ delete => 1,
+ },
+ expected_acl_root => {
+ groups => { 'ghost_group' => {} },
+ },
+ },
+ {
+ description => 'Add ACL for non-existing group fails',
+ update_acl_params => {
+ path => '/',
+ groups => 'ghost_group',
+ roles => 'PVEAdmin',
+ },
+ expected_error => qr/group 'ghost_group' does not exist/,
+ expected_acl_root => {},
+ },
+ {
+ description => 'Delete ACL for non-existing token',
+ setup_acl_root => {
+ tokens => { 'ghost@pve!ghost_token' => { PVEAdmin => 1 } },
+ },
+ update_acl_params => {
+ path => '/',
+ tokens => 'ghost@pve!ghost_token',
+ roles => 'PVEAdmin',
+ delete => 1,
+ },
+ expected_acl_root => {
+ tokens => { 'ghost@pve!ghost_token' => {} },
+ },
+ },
+ {
+ description => 'Add ACL for non-existing token fails',
+ update_acl_params => {
+ path => '/',
+ tokens => 'admin@pve!ghost_token',
+ roles => 'PVEAdmin',
+ },
+ expected_error => qr/no such token/,
+ expected_acl_root => {},
+ },
+ {
+ description => 'Add ACL with non-existing role fails',
+ update_acl_params => {
+ path => '/',
+ users => 'admin@pve',
+ roles => 'InvalidRole',
+ },
+ expected_error => qr/role 'InvalidRole' does not exist/,
+ expected_acl_root => {},
+ },
+];
+
+# --- TEST EXECUTION ---
+
+for my $case ($tests->@*) {
+ subtest $case->{description} => sub {
+ before_each();
+
+ if (exists $case->{setup_acl_root}) {
+ $current_cfg->{acl_root} = $case->{setup_acl_root};
+ }
+
+ if (exists $case->{permissions}) {
+ $rpcenv_module->redefine('permissions' => sub { return $case->{permissions} });
+ }
+
+ my ($res, $err) = run_update_acl($case->{update_acl_params});
+
+ if (exists $case->{expected_error}) {
+ like($err, $case->{expected_error}, 'API call fails with correct error message');
+ is($res, undef, 'API call returns undef on failure');
+ } else {
+ is($err, '', 'API call succeeds without throwing an error');
+ }
+
+ is_deeply(
+ $current_cfg->{acl_root},
+ $case->{expected_acl_root},
+ 'ACL root matches expected state',
+ );
+ };
+}
+
+done_testing();
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH access-control v2 2/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities
2026-07-23 11:09 [PATCH access-control v2 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 1/2] test: api: add tests for ACL modification endpoint Elias Huhsovitz
@ 2026-07-23 11:09 ` Elias Huhsovitz
1 sibling, 0 replies; 3+ messages in thread
From: Elias Huhsovitz @ 2026-07-23 11:09 UTC (permalink / raw)
To: pve-devel; +Cc: Elias Huhsovitz
When a user, group, or API token is deleted, the system usually cleans
up the associated ACLs. However, edge cases like
manual configuration edits can leave orphaned ACL entries in the
cluster configuration.
Currently, the `update_acl` API endpoint strictly validates the
existence of the target entity before processing any request. If the
entity is missing, the API throws an error (e.g., "user 'foo@pve' does
not exist") and aborts the operation. This prevents administrators from
cleaning up leftover ACLs through the API.
Bypass the existence check when the `delete` flag is set. This change
allows the API to remove orphaned ACL entries.
Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
src/PVE/API2/ACL.pm | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index c57dd38..77a9bbb 100644
--- a/src/PVE/API2/ACL.pm
+++ b/src/PVE/API2/ACL.pm
@@ -224,7 +224,7 @@ __PACKAGE__->register_method({
foreach my $group (split_list($param->{groups})) {
die "group '$group' does not exist\n"
- if !$cfg->{groups}->{$group};
+ if !$param->{delete} && !$cfg->{groups}->{$group};
if ($param->{delete}) {
delete($node->{groups}->{$group}->{$role});
@@ -237,7 +237,7 @@ __PACKAGE__->register_method({
my $username = PVE::AccessControl::verify_username($userid);
die "user '$username' does not exist\n"
- if !$cfg->{users}->{$username};
+ if !$param->{delete} && !$cfg->{users}->{$username};
if ($param->{delete}) {
delete($node->{users}->{$username}->{$role});
@@ -248,7 +248,8 @@ __PACKAGE__->register_method({
foreach my $tokenid (split_list($param->{tokens})) {
my ($username, $token) = PVE::AccessControl::split_tokenid($tokenid);
- PVE::AccessControl::check_token_exist($cfg, $username, $token);
+ PVE::AccessControl::check_token_exist($cfg, $username, $token)
+ if !$param->{delete};
if ($param->{delete}) {
delete $node->{tokens}->{$tokenid}->{$role};
--
2.47.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-23 11:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-23 11:09 [PATCH access-control v2 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 1/2] test: api: add tests for ACL modification endpoint Elias Huhsovitz
2026-07-23 11:09 ` [PATCH access-control v2 2/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities Elias Huhsovitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.