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

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

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.