[RFC PATCH access-control 2/4] api: acl: expose 'additive' flag via GET and PUT

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Expose the new 'additive' flag through the REST API so tools like the
web UI and pveum can read and set it. GET returns it per entry next
to 'propagate', and PUT gains a matching parameter.

Treat the PUT parameter as tri-state: leaving it out keeps the stored
flag unchanged. That way a client that does not know the option, or
that just re-saves an entry to change something else, cannot wipe an
additive flag that was configured earlier. Passing 'additive=0'
clears it.

Reject 'additive' without 'propagate', same as the parser does. Skip
that check when deleting, where both flags are ignored anyway, so a
cleanup call can reuse the same parameters as the matching add call.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/PVE/API2/ACL.pm      |  75 +++++++++++--
 src/test/Makefile        |   1 +
 src/test/api-acl-test.pl | 223 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 292 insertions(+), 7 deletions(-)
 create mode 100644 src/test/api-acl-test.pl

diff --git a/src/PVE/API2/ACL.pm b/src/PVE/API2/ACL.pm
index c57dd38..10116f9 100644
--- a/src/PVE/API2/ACL.pm
+++ b/src/PVE/API2/ACL.pm
@@ -17,12 +17,27 @@ use base qw(PVE::RESTHandler);
 register_standard_option(
     'acl-propagate',
     {
-        description => "Allow to propagate (inherit) permissions.",
+        description => "Allow to propagate (inherit) permissions. See also 'additive' to merge"
+            . " instead of replace at deeper paths.",
         type => 'boolean',
         optional => 1,
         default => 1,
     },
 );
+my $acl_additive_desc =
+    "Make permissions accumulate instead of being replaced at deeper paths: this entry's roles"
+    . " are merged with roles granted by more specific ACL entries below, rather than being"
+    . " overridden by them. A deeper entry can still revoke a carried role via 'NoAccess', but"
+    . " not by granting a narrower one. Requires 'propagate'.";
+register_standard_option(
+    'acl-additive',
+    {
+        description => $acl_additive_desc,
+        type => 'boolean',
+        optional => 1,
+        default => 0,
+    },
+);
 register_standard_option(
     'acl-path',
     {
@@ -52,6 +67,7 @@ __PACKAGE__->register_method({
             additionalProperties => 0,
             properties => {
                 propagate => get_standard_option('acl-propagate'),
+                additive => get_standard_option('acl-additive'),
                 path => get_standard_option('acl-path'),
                 type => { type => 'string', enum => ['user', 'group', 'token'] },
                 ugid => { type => 'string' },
@@ -83,7 +99,9 @@ __PACKAGE__->register_method({
                     my $d = $node->{"${type}s"};
                     next if !$d;
                     next if !($audit || $rpcenv->check_perm_modify($authuser, $path, 1));
+                    my $additive_members = $node->{"additive_${type}s"} // {};
                     foreach my $id (keys %$d) {
+                        my $additive_roles = $additive_members->{$id} // {};
                         foreach my $role (keys %{ $d->{$id} }) {
                             my $propagate = $d->{$id}->{$role};
                             push @$res,
@@ -93,6 +111,7 @@ __PACKAGE__->register_method({
                                     ugid => $id,
                                     roleid => $role,
                                     propagate => $propagate,
+                                    additive => $additive_roles->{$role} ? 1 : 0,
                                 };
                         }
                     }
@@ -117,6 +136,14 @@ __PACKAGE__->register_method({
         additionalProperties => 0,
         properties => {
             propagate => get_standard_option('acl-propagate'),
+            additive => get_standard_option(
+                'acl-additive',
+                {
+                    description => $acl_additive_desc
+                        . " Omit on update to keep the stored value; pass explicitly to set or"
+                        . " clear.",
+                },
+            ),
             path => get_standard_option('acl-path'),
             users => {
                 description => "List of users.",
@@ -166,6 +193,28 @@ __PACKAGE__->register_method({
             raise_param_exc({ path => "invalid ACL path '$param->{path}'" });
         }
 
+        my $propagate = 1;
+        if (defined($param->{propagate})) {
+            $propagate = $param->{propagate} ? 1 : 0;
+        }
+
+        # Tri-state: undef means "leave the stored flag alone", so a caller that does not send the
+        # parameter (older clients, UIs that predate the option) cannot silently clear a configured
+        # additive flag on an unrelated edit.
+        my $additive;
+        if (defined($param->{additive})) {
+            $additive = $param->{additive} ? 1 : 0;
+        }
+
+        # 'additive' on a non-propagating entry has nothing to merge down, so reject the combo up
+        # front. Delete ignores both flags, skip the check there to not block cleanup scripts.
+        raise_param_exc({ additive => "'additive' requires 'propagate'" })
+            if $additive && !$propagate && !$param->{delete};
+
+        # Also clear it when 'propagate' flips to 0 while 'additive' is omitted, so a GET reading
+        # this config before the writer strips it never sees the rejected combo.
+        $additive = 0 if !$propagate && !$param->{delete};
+
         PVE::AccessControl::lock_user_config(
             sub {
                 my $cfg = cfs_read_file("user.cfg");
@@ -174,12 +223,6 @@ __PACKAGE__->register_method({
                 my $authuser = $rpcenv->get_user();
                 my $auth_user_privs = $rpcenv->permissions($authuser, $path);
 
-                my $propagate = 1;
-
-                if (defined($param->{propagate})) {
-                    $propagate = $param->{propagate} ? 1 : 0;
-                }
-
                 my $node = PVE::AccessControl::find_acl_tree_node($cfg->{acl_root}, $path);
 
                 foreach my $role (split_list($param->{roles})) {
@@ -228,8 +271,14 @@ __PACKAGE__->register_method({
 
                         if ($param->{delete}) {
                             delete($node->{groups}->{$group}->{$role});
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_groups', $group, $role, 0,
+                            );
                         } else {
                             $node->{groups}->{$group}->{$role} = $propagate;
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_groups', $group, $role, $additive,
+                            ) if defined($additive);
                         }
                     }
 
@@ -241,8 +290,14 @@ __PACKAGE__->register_method({
 
                         if ($param->{delete}) {
                             delete($node->{users}->{$username}->{$role});
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_users', $username, $role, 0,
+                            );
                         } else {
                             $node->{users}->{$username}->{$role} = $propagate;
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_users', $username, $role, $additive,
+                            ) if defined($additive);
                         }
                     }
 
@@ -252,8 +307,14 @@ __PACKAGE__->register_method({
 
                         if ($param->{delete}) {
                             delete $node->{tokens}->{$tokenid}->{$role};
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_tokens', $tokenid, $role, 0,
+                            );
                         } else {
                             $node->{tokens}->{$tokenid}->{$role} = $propagate;
+                            PVE::AccessControl::update_acl_additive(
+                                $node, 'additive_tokens', $tokenid, $role, $additive,
+                            ) if defined($additive);
                         }
                     }
                 }
diff --git a/src/test/Makefile b/src/test/Makefile
index c41ee1d..db1e0c4 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -15,3 +15,4 @@ check:
 	perl -I.. perm-test9.pl
 	perl -I.. realm_sync_test.pl
 	perl -I.. api-tests.pl
+	perl -I.. api-acl-test.pl
diff --git a/src/test/api-acl-test.pl b/src/test/api-acl-test.pl
new file mode 100644
index 0000000..9aaabc7
--- /dev/null
+++ b/src/test/api-acl-test.pl
@@ -0,0 +1,223 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::AccessControl;
+use PVE::RPCEnvironment;
+use PVE::API2::ACL;
+
+# Fake the cluster file layer so we can drive the PUT handler against an in-process config
+# instead of /etc/pve.
+my $fake_cfg;
+my $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->noop('cfs_update');
+$cluster_module->mock('cfs_file_version', sub { return time(); });
+
+# AccessControl and API2::ACL import the cfs_* functions directly, so mocking the source package
+# does not affect their imported aliases. Install replacements in every namespace holding a ref.
+{
+    my $read = sub { return $fake_cfg; };
+    my $write = sub { $fake_cfg = $_[1]; };
+    my $lock = sub { my (undef, undef, $code) = @_; $code->(); };
+    no strict 'refs'; ## no critic
+    no warnings 'redefine'; ## no critic
+    for my $pkg ('PVE::Cluster', 'PVE::AccessControl', 'PVE::API2::ACL') {
+        *{"${pkg}::cfs_read_file"} = $read;
+        *{"${pkg}::cfs_write_file"} = $write;
+        *{"${pkg}::cfs_lock_file"} = $lock;
+    }
+}
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+# Seed $fake_cfg with a minimal starting config before init_request reads it.
+$fake_cfg = {};
+PVE::AccessControl::userconfig_force_defaults($fake_cfg);
+$fake_cfg->{users}->{'root@pam'} = { enable => 1, expire => 0 };
+
+$rpcenv->init_request();
+$rpcenv->set_user('root@pam');
+
+# Mock permission checks since root@pam bypasses them anyway; keep it explicit so the test does
+# not depend on subtle rpcenv state.
+my $rpcenv_module = Test::MockModule->new('PVE::RPCEnvironment');
+$rpcenv_module->mock(
+    'permissions', sub { return { 'Permissions.Modify' => 1 }; },
+);
+
+sub fresh_cfg {
+    my $cfg = {};
+    PVE::AccessControl::userconfig_force_defaults($cfg);
+    $cfg->{users}->{'root@pam'} = { enable => 1, expire => 0 };
+    $cfg->{users}->{'alice@pve'} = { enable => 1, expire => 0 };
+    return $cfg;
+}
+
+my ($handler, $info) = PVE::API2::ACL->find_handler('PUT', '');
+
+sub put_acl {
+    my ($p) = @_;
+    eval { $handler->handle($info, $p); };
+    return $@;
+}
+
+# PUT without 'additive' must preserve the existing flag
+
+$fake_cfg = fresh_cfg();
+put_acl({
+    path => '/vms/100',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+    additive => 1,
+});
+is(
+    $fake_cfg->{acl_root}->{children}->{vms}->{children}->{100}->{additive_users}->{'alice@pve'}
+        ->{'PVEAuditor'},
+    1,
+    'additive=1 stored',
+);
+
+# Unaware caller edits the same entry without passing additive; flag must
+# survive.
+put_acl({
+    path => '/vms/100',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+});
+is(
+    $fake_cfg->{acl_root}->{children}->{vms}->{children}->{100}->{additive_users}->{'alice@pve'}
+        ->{'PVEAuditor'},
+    1,
+    'additive preserved when caller omits the parameter',
+);
+
+# Explicit additive=0 clears the flag.
+put_acl({
+    path => '/vms/100',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+    additive => 0,
+});
+is(
+    exists($fake_cfg->{acl_root}->{children}->{vms}->{children}->{100}->{additive_users}),
+    '',
+    'additive=0 clears the flag (and scaffolding)',
+);
+
+# Delete + additive=1 must not error, even on non-propagating call
+
+$fake_cfg = fresh_cfg();
+put_acl({
+    path => '/vms/200',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+    additive => 1,
+});
+is(
+    put_acl({
+        path => '/vms/200',
+        users => 'alice@pve',
+        roles => 'PVEAuditor',
+        propagate => 0,
+        additive => 1,
+        delete => 1,
+    }),
+    '',
+    'delete accepts additive=1 with propagate=0',
+);
+is(
+    $fake_cfg->{acl_root}->{children}->{vms}->{children}->{200}->{users}->{'alice@pve'}
+        ->{'PVEAuditor'},
+    undef,
+    'role was actually removed',
+);
+
+# Non-delete with additive=1 + propagate=0 still errors
+
+$fake_cfg = fresh_cfg();
+my $err = put_acl({
+    path => '/vms/300',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 0,
+    additive => 1,
+});
+like(
+    "$err", qr/'additive' requires 'propagate'/, 'non-delete rejects additive + !propagate',
+);
+
+# roles() must not carry a propagate=0 role even if its additive flag is set. This drift state is
+# not reachable through the API (PUT clears it, see next test) or the parser (it strips the flag),
+# so construct it directly here to lock in the roles() guard regardless of how it might arise.
+
+$fake_cfg = fresh_cfg();
+my $node = PVE::AccessControl::find_acl_tree_node($fake_cfg->{acl_root}, '/vms/400');
+$node->{users}->{'alice@pve'}->{'PVEAuditor'} = 0; # propagate=0
+$node->{additive_users}->{'alice@pve'}->{'PVEAuditor'} = 1;
+my $drift_roles = PVE::AccessControl::roles($fake_cfg, 'alice@pve', '/vms/400/sub');
+is_deeply(
+    $drift_roles,
+    {},
+    'harvest_additive: propagate=0 in-memory drift does not carry to deeper paths',
+);
+
+# Flipping propagate to 0 clears a stale additive flag
+# A non-propagating entry cannot carry roles down, so setting propagate=0
+# must normalize the now-invalid 'additive' flag in-memory, even when the
+# caller omits 'additive' on that update.
+
+$fake_cfg = fresh_cfg();
+put_acl({
+    path => '/vms/500',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+    additive => 1,
+});
+put_acl({
+    path => '/vms/500',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 0,
+});
+is(
+    exists($fake_cfg->{acl_root}->{children}->{vms}->{children}->{500}->{additive_users}),
+    '',
+    'propagate=0 clears the additive flag even when additive is omitted',
+);
+
+# GET surfaces the additive flag per entry
+
+$fake_cfg = fresh_cfg();
+put_acl({
+    path => '/vms/600',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+    additive => 1,
+});
+put_acl({
+    path => '/vms/601',
+    users => 'alice@pve',
+    roles => 'PVEAuditor',
+    propagate => 1,
+});
+$rpcenv->{user_cfg} = $fake_cfg;
+my ($get_handler, $get_info) = PVE::API2::ACL->find_handler('GET', '');
+my $acls = $get_handler->handle($get_info, {});
+my %additive_by_path =
+    map { $_->{path} => $_->{additive} } grep { $_->{ugid} eq 'alice@pve' } @$acls;
+is($additive_by_path{'/vms/600'}, 1, 'GET reports additive=1 for the additive entry');
+is($additive_by_path{'/vms/601'}, 0, 'GET reports additive=0 for the plain entry');
+
+done_testing();
-- 
2.47.3