From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 7DE861FF0E0 for ; Thu, 23 Jul 2026 11:00:42 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DB37E21488; Thu, 23 Jul 2026 11:00:41 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 23 Jul 2026 11:00:35 +0200 Message-Id: Subject: Re: [PATCH access-control 2/2] test: api: add tests for ACL modification endpoint From: "Elias Huhsovitz" To: "Daniel Kral" , X-Mailer: aerc 0.20.0 References: <20260720134535.136172-1-e.huhsovitz@proxmox.com> <20260720134535.136172-3-e.huhsovitz@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784797206318 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.013 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: GQ5X3QTVWWAYQAACASLP7SOZRC3AYZEI X-Message-ID-Hash: GQ5X3QTVWWAYQAACASLP7SOZRC3AYZEI X-MailFrom: e.huhsovitz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > > 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; >> =20 >> my $harness =3D TAP::Harness->new({ verbosity =3D> -1 }); >> =20 >> -my $result =3D $harness->runtests('api-get-permissions-test.pl'); >> +my $result =3D $harness->runtests('api-get-permissions-test.pl', 'api-u= pdate-acl-test.pl'); >> =20 >> exit -1 if $result->{failed}; >> diff --git a/src/test/api-update-acl-test.pl b/src/test/api-update-acl-t= est.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 =3D 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_u= tf8 =3D undef) { >> + die "unknown file '$filename'\n" if $filename ne 'user.cfg'; >> + $current_cfg =3D $data; >> +}); >> + >> +# Re-assign sub-routines to our mocked versions if imported into PVE::A= PI2::ACL. >> +no warnings 'redefine'; >> +*PVE::API2::ACL::cfs_read_file =3D \&PVE::Cluster::cfs_read_file; >> +*PVE::API2::ACL::cfs_write_file =3D \&PVE::Cluster::cfs_write_file; > > I tend to not redefine functions here at all. > > This could be rewritten as: > > my $api_acl_module =3D Test::MockModule->new('PVE::API2::ACL'); > > $api_acl_module->noop('cfs_update'); > $api_acl_module->mock( > 'cfs_read_file' =3D> sub($filename) { ... }, > 'cfs_write_file' =3D> sub($filename, $data, $force_utf8 =3D 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 =3D Test::MockModule->new('PVE::AccessControl'); >> +$acl_module->mock('lock_user_config', sub ($code, @args) { >> + return $code->(@args); >> +}); >> + >> +my $rpcenv_module =3D Test::MockModule->new('PVE::RPCEnvironment'); >> + >> +my $rpcenv =3D PVE::RPCEnvironment->init('cli'); >> + >> +my ($handler, $handler_info) =3D PVE::API2::ACL->find_handler('PUT', ''= ); >> + >> +# --- HELPER FUNCTIONS --- >> + >> +sub get_base_cfg () { >> + return { >> + users =3D> { >> + 'admin@pve' =3D> { >> + enable =3D> 1, >> + tokens =3D> { >> + 'mytoken' =3D> { privsep =3D> 1 }, >> + }, >> + }, >> + }, >> + groups =3D> { 'admin_group' =3D> {} }, >> + roles =3D> { >> + 'PVEAdmin' =3D> { 'Permissions.Modify' =3D> 1 }, >> + 'NoAccess' =3D> {}, >> + }, >> + acl_root =3D> {}, >> + }; >> +} >> + >> +sub before_each () { >> + $current_cfg =3D get_base_cfg(); >> + >> + $rpcenv_module->mock('permissions', sub { >> + return { 'Permissions.Modify' =3D> 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 =3D 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 =3D> { ... }, > # can be made empty for specific test cases, etc. > permissions =3D> { 'Permissions.Modify' =3D> 1 }, > expected_acl_root =3D> { ... }, > } > > 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=20 - 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=20 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.