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

[PATCH access-control v3 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, API tokens, and roles.

Edge cases, such as manual edits, can leave orphaned ACLs in
the cluster configuration. Deletion of an orphaned ACL is currently
prevented and an error is thrown (e.g., "user 'foo@pve' does
not exist"). This happens because the API endpoint strictly
validates the existence of the target entity, before processing any
request.

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, validates input parameters, and specifically tests the
edge cases mentioned in the bug report.

Patch 2/2 modifies the `update_acl` API endpoint to bypass the
entity and role existence checks when the `delete` flag is set.
This allows administrators to clean up orphaned ACL entries.

Changes v2 -> v3:
-----------------
* Fix a bug that caused the API to crash or skip deletions
  when removing ACLs for non-existent roles.
* Remove dead `noop('cfs_update')` mock in the test suite.
* Rename test descriptions to consistently end with 'succeeds'
  or 'fails'.
* Add missing test cases for parameter validation, path
  validation, idempotent deletes, privilege regressions and
  deleting ACLs with non-existent roles.
* Reword commit message: Use plain prose instead of bullet lists.

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.

Previous Versions
-----------------
v2: https://lore.proxmox.com/pve-devel/20260723110939.84932-1-e.huhsovitz@proxmox.com/
v1: https://lore.proxmox.com/pve-devel/20260720134535.136172-1-e.huhsovitz@proxmox.com/

Summary of Changes
------------------

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

 src/PVE/API2/ACL.pm             |  13 +-
 src/test/api-tests.pl           |   2 +-
 src/test/api-update-acl-test.pl | 356 ++++++++++++++++++++++++++++++++
 3 files changed, 364 insertions(+), 7 deletions(-)
 create mode 100644 src/test/api-update-acl-test.pl

-- 
2.47.3

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

[Elias Huhsovitz]

The `PUT /access/acl` endpoint lacked automated test coverage. Introduce
`api-update-acl-test.pl` to verify the ACL modification logic.

Add tests for:  adding, modifying, and deleting ACLs for existing users,
groups, and tokens.

Add tests for: parameter validation, invalid paths, and idempotent
deletions to prevent future regressions.

Verify that: that requests to add ACLs for non-existent entities or
roles are rejected, privilege boundaries are enforced, and orphaned ACLs
for non-existent entities and roles can be successfully deleted.

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
 src/test/api-tests.pl           |   2 +-
 src/test/api-update-acl-test.pl | 356 ++++++++++++++++++++++++++++++++
 2 files changed, 357 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..600cce0
--- /dev/null
+++ b/src/test/api-update-acl-test.pl
@@ -0,0 +1,356 @@
+#!/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->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 => 'Add ACL with insufficient role privileges fails',
+        permissions => { 'VM.Audit' => 1 }, # Caller has some perms, but not PVEAdmin
+        update_acl_params => {
+            path => '/',
+            users => 'admin@pve',
+            roles => 'PVEAdmin',
+        },
+        expected_error => qr/requires 'Permissions.Modify' or superset of privileges/,
+        expected_acl_root => {},
+    },
+    {
+        description => 'Modify existing ACL to add a second role succeeds',
+        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 succeeds',
+        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 succeeds',
+        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 succeeds',
+        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 => 'Delete non-existent ACL entry succeeds',
+        setup_acl_root => {
+            users => { 'admin@pve' => { PVEAdmin => 1 } },
+        },
+        update_acl_params => {
+            path => '/',
+            users => 'admin@pve',
+            roles => 'NoAccess', # User does not actually have this role
+            delete => 1,
+        },
+        expected_acl_root => {
+            users => { 'admin@pve' => { PVEAdmin => 1 } }, # State remains unchanged
+        },
+    },
+    {
+        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 succeeds',
+        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 succeeds',
+        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 => 'Delete ACL for non-existing role succeeds',
+        setup_acl_root => {
+            users => { 'admin@pve' => { 'InvalidRole' => 1 } },
+        },
+        update_acl_params => {
+            path => '/',
+            users => 'admin@pve',
+            roles => 'InvalidRole',
+            delete => 1,
+        },
+        expected_acl_root => {
+            users => { 'admin@pve' => {} },
+        },
+    },
+    {
+        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 => {},
+    },
+    {
+        description => 'Add ACL without users, groups, or tokens fails',
+        update_acl_params => {
+            path => '/',
+            roles => 'PVEAdmin',
+        },
+        expected_error => qr/either 'users', 'groups' or 'tokens' is required/,
+        expected_acl_root => {},
+    },
+    {
+        description => 'Add ACL with invalid path fails',
+        update_acl_params => {
+            path => '/invalid/path',
+            users => 'admin@pve',
+            roles => 'PVEAdmin',
+        },
+        expected_error => qr/invalid ACL path/,
+        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

[PATCH access-control v3 2/2] fix #6510: api: acl: allow deletion 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, to allow
removal of orphaned ACLs.

Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
---
 src/PVE/API2/ACL.pm | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index c57dd38..80de548 100644
--- a/src/PVE/API2/ACL.pm
+++ b/src/PVE/API2/ACL.pm
@@ -183,14 +183,14 @@ __PACKAGE__->register_method({
                 my $node = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
 
                 foreach my $role (split_list($param->{roles})) {
+                    my $role_privs = $cfg->{roles}->{$role};
                     die "role '$role' does not exist\n"
-                        if !$cfg->{roles}->{$role};
+                        if !$role_privs && !$param->{delete};
 
                     # permissions() returns set privs as key, and propagate bit as value!
-                    if (!defined($auth_user_privs->{'Permissions.Modify'})) {
+                    if (!defined($auth_user_privs->{'Permissions.Modify'}) && $role_privs) {
                         # 'perm-modify' allows /vms/* with VM.Allocate and similar restricted use cases
                         # filter those to only allow handing out a subset of currently active privs
-                        my $role_privs = $cfg->{roles}->{$role};
                         my $verb = $param->{delete} ? 'remove' : 'add';
                         foreach my $priv (keys $role_privs->%*) {
                             raise_param_exc(
@@ -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