Re: [PATCH manager v2] fix #6735: api: pci: allow mdevscan access via mapping permissions.

[Dominik Csapak <d.csapak@proxmox.com>]

On 9/2/26 10:51 AM, Elias Huhsovitz wrote:
> Thanks for the feedback! Just one small stylistic question below.
> 
> On Tue Sep 1, 2026 at 2:27 PM CEST, Dominik Csapak wrote:
>> some comments inline
>>
> 
> [...]
>>> +
>>> +        return PVE::QemuServer::PCI::Mdev::get_mdev_types($id) if ($is_pci_id);
>>> +
>>> +        if (!$rpcenv->check_any($authuser, '/', ['Sys.Audit', 'Sys.Modify'], 1)) {
>>
>> isn't this just $has_global_perms again?
> 
> Sorry, these shouldn't have been here, i formatted the wrong branch by
> mistake. The check is indeeed redundant.
> 
>>> +            $rpcenv->check_any(
>>> +                $authuser,
>>> +                "/mapping/pci/$id",
>>> +                ['Mapping.Use', 'Mapping.Modify', 'Mapping.Audit'],
>>> +            );
>>
>> this was also already checked?
>>
>>
>> IMO these happen because the code flow is not very obvious. Instead
>> of having a bunch of postif clauses, I'd prefer a simpler approach:
>>
>>
>> my $id = ...
>> my $rpcenv = ...
>> my $authuser = ...
>>
>> my $has_global_perms = ...
>> my $is_pci_id = ...
>>
>>
>> if ($is_pci_id)  {
>>      raise_perm_exc if !$has_global_perms;
>>      ...
>> } else {
>>      raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
>>      ...
>> }
>>
>> with such code, the existing flow and indentation stays the same,
>> and it's clear which branch takes which permissions
>>
>> does that make sense?
> 
> 
> I just have a small question here:
> 
> Do you propse A or B:
> 
> A:
> 
>   if ($is_pci_id)  {
>       raise_perm_exc if !$has_global_perms;
>       # business logic ....
>   } else {
>       raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
>       # business logic ...
>   }
> 
>   # END OF SUBROUTINE
> 
> B:
> 
>   if ($is_pci_id)  {
>       raise_perm_exc if !$has_global_perms;
>   } else {
>       raise_perm_exc if !$has_global_perms && !check_any(mapping_perms);
>   }
>   
>   # business logic ...
> 
>   # END OF SUBROUTINE
> 
> Becuase i prefer B. It seems much simpler to me since the permission logic and
> business logic aren't intertwined. I would even like to go as far as
> factoring the logic into its own subroutine, since the business logic
> should be completely separate from the authorization.
> 
> C:
> 
>   check_custom_perm($is_pci_id);
> 
>   # business logic ...
> 
>   # END OF SUBROUTINE
> 
> 
> This also doesnt change the business logic, and i can keep the if-else
> statement if you prefer.

Actually i would prefer A, because the two branches are by definition
very different (aside from one permission check), so having it split
up early makes sense to me.

With B and C, we at least have to duplicate one if/else (namely
on the 'is it a pci id or not' and that has a good chance
to diverge in the future (e.g. imagine we introduce a 3rd branch
that needs again different permissions, with A this is
more or less automatic since each branch needs it's own check.

with B and C two different parts of the code have to argue
about the same branching.