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 5ABFF1FF0E3 for ; Tue, 21 Jul 2026 17:21:30 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 60CD321475; Tue, 21 Jul 2026 17:21:29 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 21 Jul 2026 17:20:53 +0200 Message-Id: Subject: Re: [PATCH access-control 2/2] test: api: add tests for ACL modification endpoint From: "Daniel Kral" To: "Elias Huhsovitz" , X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260720134535.136172-1-e.huhsovitz@proxmox.com> <20260720134535.136172-3-e.huhsovitz@proxmox.com> In-Reply-To: <20260720134535.136172-3-e.huhsovitz@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784647227267 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.064 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: YGF6MYHB2X7OHJIQ7VOFJLBFIJOETICW X-Message-ID-Hash: YGF6MYHB2X7OHJIQ7VOFJLBFIJOETICW X-MailFrom: d.kral@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: 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. > --- > 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-up= date-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-te= st.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_ut= f8 =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::AP= I2::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) { ..= . }, ); > + > +# 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'); > +} > + > +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?