[PATCH access-control 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests

[PATCH access-control 0/2] fix: #6510: Allow deleting ACLs for non-existent entities & add API tests

[Elias Huhsovitz]

This series fixes Bug #6510, which prevents the deletion of ACLs
for non-existent users, groups, or API tokens.

Patch 1/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.

Patch 2/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.

Elias Huhsovitz (2):
  fix #6510: api: acl: allow deleting ACLs for non-existent entities
  test: api: add tests for ACL modification endpoint

 src/PVE/API2/ACL.pm             |   6 +-
 src/test/api-tests.pl           |   2 +-
 src/test/api-update-acl-test.pl | 301 ++++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+), 4 deletions(-)
 create mode 100644 src/test/api-update-acl-test.pl

-- 
2.47.3

[PATCH access-control 1/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities

[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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index c57dd38..01329f7 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,7 @@ __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

[PATCH access-control 2/2] test: api: add tests for ACL modification endpoint

[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 | 301 ++++++++++++++++++++++++++++++++
 2 files changed, 302 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..0157533
--- /dev/null
+++ b/src/test/api-update-acl-test.pl
@@ -0,0 +1,301 @@
+#!/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 $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->noop('cfs_update');
+$cluster_module->mock('cfs_read_file', sub ($filename) {
+    die "unknown file '$filename'\n" if $filename ne 'user.cfg';
+    return $current_cfg;
+});
+$cluster_module->mock('cfs_write_file', sub ($filename, $data, $force_utf8 = undef) {
+    die "unknown file '$filename'\n" if $filename ne 'user.cfg';
+    $current_cfg = $data;
+});
+
+# Re-assign sub-routines to our mocked versions if imported into PVE::API2::ACL.
+no warnings 'redefine';
+*PVE::API2::ACL::cfs_read_file = \&PVE::Cluster::cfs_read_file;
+*PVE::API2::ACL::cfs_write_file = \&PVE::Cluster::cfs_write_file;
+
+# Mock lock_user_config to execute the callback immediately.
+my $acl_module = Test::MockModule->new('PVE::AccessControl');
+$acl_module->mock('lock_user_config', sub ($code, @args) {
+    return $code->(@args);
+});
+
+my $rpcenv_module = Test::MockModule->new('PVE::RPCEnvironment');
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+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->mock('permissions', sub {
+        return { 'Permissions.Modify' => 1 };
+    });
+    $rpcenv_module->mock('get_user', sub {
+        return 'admin@pve';
+    });
+}
+
+sub run_update_acl ($params) {
+    my $result = eval { $handler->handle($handler_info, $params) };
+    return ($result, $@);
+}
+
+# --- TESTS ---
+
+subtest 'Add ACL for existing user succeeds' => sub {
+    before_each();
+
+    my ($res, $err) = run_update_acl({
+        path  => '/',
+        users => 'admin@pve',
+        roles => 'PVEAdmin',
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(exists($current_cfg->{acl_root}->{users}->{'admin@pve'}->{PVEAdmin}),
+       'admin@pve ACL was added to config');
+};
+
+subtest 'Delete ACL for existing user succeeds' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{users} = { 'admin@pve' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        users  => 'admin@pve',
+        roles  => 'PVEAdmin',
+        delete => 1,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(!exists($current_cfg->{acl_root}->{users}->{'admin@pve'}->{PVEAdmin}),
+       'admin@pve ACL was removed from config');
+};
+
+subtest 'Add ACL without Permissions.Modify fails' => sub {
+    before_each();
+
+    $rpcenv_module->mock('permissions', sub { return {}; });
+
+    my ($res, $err) = run_update_acl({
+        path  => '/',
+        users => 'admin@pve',
+        roles => 'PVEAdmin',
+    });
+
+    like($err, qr/requires 'Permissions.Modify'/, 'API call fails due to missing privileges');
+    is($res, undef, 'API call returns undef on failure');
+};
+
+subtest 'Modify existing ACL to add a second role' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{users} = { 'admin@pve' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path  => '/',
+        users => 'admin@pve',
+        roles => 'NoAccess',
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(exists($current_cfg->{acl_root}->{users}->{'admin@pve'}->{PVEAdmin}),
+       'Original PVEAdmin role is preserved');
+    ok(exists($current_cfg->{acl_root}->{users}->{'admin@pve'}->{NoAccess}),
+       'New NoAccess role was added successfully');
+};
+
+subtest 'Modify existing ACL to change propagation flag' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{users} = { 'admin@pve' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path      => '/',
+        users     => 'admin@pve',
+        roles     => 'PVEAdmin',
+        propagate => 0,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    is($current_cfg->{acl_root}->{users}->{'admin@pve'}->{PVEAdmin}, 0,
+       'Propagation flag was updated from 1 to 0');
+};
+
+subtest 'Modify existing ACL for group and token' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{groups} = { 'admin_group' => { PVEAdmin => 1 } };
+    $current_cfg->{acl_root}->{tokens} = { 'admin@pve!mytoken' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path      => '/',
+        groups    => 'admin_group',
+        tokens    => 'admin@pve!mytoken',
+        roles     => 'NoAccess',
+        propagate => 0,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+
+    is($current_cfg->{acl_root}->{groups}->{'admin_group'}->{PVEAdmin}, 1,
+       'Group PVEAdmin role is preserved');
+    ok(exists($current_cfg->{acl_root}->{groups}->{'admin_group'}->{NoAccess}),
+       'Group NoAccess role was added');
+
+    is($current_cfg->{acl_root}->{tokens}->{'admin@pve!mytoken'}->{PVEAdmin}, 1,
+       'Token PVEAdmin role is preserved');
+    ok(exists($current_cfg->{acl_root}->{tokens}->{'admin@pve!mytoken'}->{NoAccess}),
+       'Token NoAccess role was added');
+};
+
+subtest 'Delete ACL for non-existing user' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{users} = { 'ghost@pve' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        users  => 'ghost@pve',
+        roles  => 'PVEAdmin',
+        delete => 1,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(!exists($current_cfg->{acl_root}->{users}->{'ghost@pve'}->{PVEAdmin}),
+       'ghost@pve ACL was removed from config');
+};
+
+subtest 'Add ACL for non-existing user fails' => sub {
+    before_each();
+
+    my ($res, $err) = run_update_acl({
+        path  => '/',
+        users => 'ghost@pve',
+        roles => 'PVEAdmin',
+    });
+
+    like($err, qr/user 'ghost\@pve' does not exist/, 'API call fails with correct error message');
+    is($res, undef, 'API call returns undef on failure');
+};
+
+subtest 'Delete ACL for non-existing group' => sub {
+    before_each();
+    $current_cfg->{acl_root}->{groups} = { 'ghost_group' => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        groups => 'ghost_group',
+        roles  => 'PVEAdmin',
+        delete => 1,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(!exists($current_cfg->{acl_root}->{groups}->{'ghost_group'}->{PVEAdmin}),
+       'ghost_group ACL was removed from config');
+};
+
+subtest 'Add ACL for non-existing group fails' => sub {
+    before_each();
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        groups => 'ghost_group',
+        roles  => 'PVEAdmin',
+    });
+
+    like($err, qr/group 'ghost_group' does not exist/, 'API call fails with correct error message');
+    is($res, undef, 'API call returns undef on failure');
+};
+
+subtest 'Delete ACL for non-existing token' => sub {
+    before_each();
+    my $token_id = 'ghost@pve!ghost_token';
+    $current_cfg->{acl_root}->{tokens} = { $token_id => { PVEAdmin => 1 } };
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        tokens => $token_id,
+        roles  => 'PVEAdmin',
+        delete => 1,
+    });
+
+    is($err, '', 'API call succeeds without throwing an error');
+    ok(!exists($current_cfg->{acl_root}->{tokens}->{$token_id}->{PVEAdmin}),
+       'ghost token ACL was removed from config');
+};
+
+subtest 'Add ACL for non-existing token fails' => sub {
+    before_each();
+
+    my ($res, $err) = run_update_acl({
+        path   => '/',
+        tokens => 'admin@pve!ghost_token',
+        roles  => 'PVEAdmin',
+    });
+
+    like($err, qr/no such token/, 'API call fails with correct error message');
+    is($res, undef, 'API call returns undef on failure');
+};
+
+subtest 'Add ACL with non-existing role fails' => sub {
+    before_each();
+
+    my ($res, $err) = run_update_acl({
+        path  => '/',
+        users => 'admin@pve',
+        roles => 'InvalidRole',
+    });
+
+    like($err, qr/role 'InvalidRole' does not exist/,
+         'API call fails with correct error message for invalid role');
+    is($res, undef, 'API call returns undef on failure');
+};
+
+done_testing();
-- 
2.47.3