all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Elias Huhsovitz <e.huhsovitz@proxmox.com>
To: pve-devel@lists.proxmox.com
Cc: Elias Huhsovitz <e.huhsovitz@proxmox.com>
Subject: [PATCH access-control v2 1/2] test: api: add tests for ACL modification endpoint
Date: Thu, 23 Jul 2026 13:09:38 +0200	[thread overview]
Message-ID: <20260723110939.84932-2-e.huhsovitz@proxmox.com> (raw)
In-Reply-To: <20260723110939.84932-1-e.huhsovitz@proxmox.com>

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





  reply	other threads:[~2026-07-23 11:10 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2026-07-23 11:09 ` [PATCH access-control v2 2/2] fix #6510: api: acl: allow deleting ACLs for non-existent entities Elias Huhsovitz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260723110939.84932-2-e.huhsovitz@proxmox.com \
    --to=e.huhsovitz@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal