* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox