[pve-devel] [PATCH access-control 1/4] domain sync: make options actually required

[Dominik Csapak]

On 4/6/20 1:52 PM, Thomas Lamprecht wrote:
> On 4/6/20 1:31 PM, Dominik Csapak wrote:
>> we want the api options to be optional, but only as long as there are
>> default values set in the realm config
>>
>> since they are all marked as optional (else they would be required in
>> the api) this check did not work as intended
>>
>> instead, check if there exists a 'default' instead (which we then use)
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   PVE/API2/Domains.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Domains.pm b/PVE/API2/Domains.pm
>> index 8ae1db0..b42d4f6 100644
>> --- a/PVE/API2/Domains.pm
>> +++ b/PVE/API2/Domains.pm
>> @@ -359,7 +359,7 @@ my $parse_sync_opts = sub {
>>   	} elsif (!exists $res->{$opt}) {
>>   	    raise_param_exc({
>>   		"$opt" => 'Not passed as parameter and not defined in realm default sync options.'
>> -	    }) if !$fmt->{optional};
>> +	    }) if !exists $fmt->{default};
>>   	    $res->{$opt} = $fmt->{default} if exists $fmt->{default};
> 
> The checks seems now redundant and possible incomplete, just because now all
> are always optional doesn't mean that it is the case in the future. We could do:
> 
> raise_param_exc(...) if !$fmt->{optional} || !exists $fmt->{default};
> 
> $res->{$opt} = $fmt->{default};
> 

but they always have to be optional, otherwise the code never
reaches the point were it is not set but we enter the api call?
we use the definitions both for the property string as well as the
api parameters

but yes, the !exists and exists a line after that makes little sense

> 
> Need to rethink this, as is it's not correct (thanks for noticing) but I think
> that I thought about something else here, damn brain..
>