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

["Daniel Kral" <d.kral@proxmox.com>]

Thanks for adding some test cases for this!

See below for some comments inline.

The patch also needs a run of `make tidy`.

On Mon Jul 20, 2026 at 3:45 PM CEST, Elias Huhsovitz wrote:
> 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>

It might be nice to have these test cases precede the fixing patch so
that the change in logic is directly visible and verifiable in the
fixing patch through the change in the test outcomes.

> ---
>  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;

nit: missing empty newline between shebang and first use statement

> +
> +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;

I tend to not redefine functions here at all.

This could be rewritten as:

my $api_acl_module = Test::MockModule->new('PVE::API2::ACL');

$api_acl_module->noop('cfs_update');
$api_acl_module->mock(
    'cfs_read_file' => sub($filename) { ... },
    'cfs_write_file' => sub($filename, $data, $force_utf8 = undef) { ... },
);

> +
> +# 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';
> +    });

get_user doesn't need to be mocked every time, if you use

    $rpcenv->set_user('admin@pve');

> +}
> +
> +sub run_update_acl ($params) {
> +    my $result = eval { $handler->handle($handler_info, $params) };
> +    return ($result, $@);
> +}

Hm, I wonder whether the test cases would be a little more compact and
reviewable by following the usual test case structure which is

- defining an array full of hashes where each hash represents a test
  case
- a loop then goes through each test case

Each test case could then make a deep comparison between the resulting
$current_cfg->{acl_root}, which is reset for each test case entry.

As a proposal for a possible test case entry could be:

    {
        update_acl_params => { ... },
        # can be made empty for specific test cases, etc.
        permissions => { 'Permissions.Modify' => 1 },
        expected_acl_root => { ... },
    }

What do you think?