public inbox for pve-devel@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal