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

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

Thank you for the review!
I added my thoughts down below.

On Tue Jul 21, 2026 at 5:20 PM CEST, Daniel Kral wrote:
> 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.
>

I agree, this is much better. I will do this in the future.

>> ---
>>  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) { ... },
> );
>

Thanks, this came about due to some strange error messages perl gave me,
but this is much cleaner.

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

Thansk I missed this functionality.

>> +}
>> +
>> +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?

I also like this kind of style.

I used the following guide as a base: https://perlmaven.com/testing
and it mentioned the subtest keyword here: https://perlmaven.com/subtest

There subtest keyword is also used in some other repositories, such as 
- pve-container/src/test/snapshot-test.pm
- qemu-server/src/test/snapshot-test.pm
- pve-container/src/test/idmap-test.pm

So i was not sure what the preferred approach currently is.

I did it this way, since I am more familiar with JUnit5 as a testing 
framework. I don't view the additional verbosity as a bad thing in
this case, since it provides a bit more description and makes it easier
to modify existing tests in the future.

I appreciate the simplicity of the approach you suggested
and can implement this style in v2.