Re: [PATCH pve-access-control v4 1/5] fix: #7520: sdn: prune orphaned ACLs on resource deletion

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

On Fri Jun 26, 2026 at 12:52 PM CEST, David Riley wrote:
> Add a helper routine to prune ACL entries under the '/sdn/' path when
> the corresponding resources are deleted. Compare the running
> configuration against the newly compiled state during config commit.
>
> This prevents dangling permission states and ensures configuration
> consistency across manual API/UI applies as well as automatic reloads
> during system boot.

The patch message does describe functionality which is only implemented
in one of the following patches (#5).

If a patch implements something that is going to be used in the
following patches, it is usually fine to state this, e.g.
"Add <some> in preparation of <thing> in the following patches".

It might make sense to introduce the lookup_acl_node() (or another
variant, see below for more) in a separate commit, so those concerns are
separated from both adding lookup_acl_node() for both
remove_sdn_resource_access() and the VNet Migration.

Usually, the patches which only introduce the helpers do not need to be
prefixed with 'fix #XYZ:', but only the patches, which actually fix the
behavior, i.e., the last patch.

>
> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=7520
> Signed-off-by: David Riley <d.riley@proxmox.com>
> ---
>  src/PVE/AccessControl.pm | 46 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 0d632b3..10526db 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1974,6 +1974,52 @@ sub remove_vm_from_pool {
>      lock_user_config($delVMfromPoolFn, "pool cleanup for VM $vmid failed");
>  }
>  
> +sub remove_sdn_resource_access {
> +    my ($paths) = @_; # [ ['zones', '<zone>'], ['zones', '<zone>', '<vnet>'] ]

$paths might be confusing here as in pve-access-control variables named
'path' are usually the string representation, e.g. /zones/<zone>.

> +
> +    my $delete_resource_fn = sub {
> +        my $usercfg = cfs_read_file("user.cfg");
> +        my $modified = 0;
> +
> +        for my $segments (@$paths) {
> +            my @full_path = ('sdn', @$segments);
> +            my $current = $usercfg->{acl_root};
> +            my ($parent, $last_key) = lookup_acl_node($current, \@full_path);
> +
> +            if ($parent && $last_key) {
> +                delete $parent->{children}->{$last_key};
> +                $modified = 1;
> +            }
> +        }
> +
> +        if ($modified) {
> +            cfs_write_file("user.cfg", $usercfg);
> +        }
> +    };
> +
> +    lock_user_config($delete_resource_fn, "SDN ACL cleanup failed");
> +}
> +
> +sub lookup_acl_node {
> +    my ($root, $path) = @_;
> +
> +    my $current = $root;
> +    my $parent = undef;
> +    my $last_key = undef;
> +
> +    for my $step (@$path) {

nit: another patch uses $segment (instead of $step) for the same
     concept, so it might also make more sense to use the same term here
     (if this is still needed with the comment below)

> +        if (defined($current->{children}) && defined($current->{children}->{$step})) {
> +            $parent = $current;
> +            $last_key = $step;
> +            $current = $current->{children}->{$step};
> +        } else {
> +            return (undef, undef, undef);
> +        }
> +    }
> +
> +    return ($parent, $last_key, $current);
> +}

If $path is a string, then this could reuse the code from the existing
find_acl_tree_node() subroutine. Either adapt it with a

    return wantarray ? ($node, $parent, $last_key) : $node;

or a wrapper

    sub find_acl_tree_node_full($root, $path) { ... }

    sub find_acl_tree_node($root, $path) {
        my ($node) = find_acl_tree_node_full($root, $path);

        return $node;
    }

No hard feelings here though.

> +
>  my $USER_CONTROLLED_TFA_TYPES = {
>      u2f => 1,
>      oath => 1,