[pve-devel] [PATCH 17/23] allow ticket in auth header as fallback

[Fabian Grünbichler]

On November 12, 2019 11:05 am, Thomas Lamprecht wrote:
> On 11/12/19 10:46 AM, Fabian Grünbichler wrote:
>> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
>>> On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
>>>> @@ -1232,7 +1232,10 @@ sub unshift_read_header {
>>>>  		} elsif ($path =~ m/^\Q$base_uri\E/) {
>>>>  		    my $token = $r->header('CSRFPreventionToken');
>>>>  		    my $cookie = $r->header('Cookie');
>>>> -		    my $ticket = PVE::APIServer::Formatter::extract_auth_cookie($cookie, $self->{cookie_name});
>>>> +		    my $auth_header = $r->header('Authorization');
>>>> +		    my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name});
>>>> +		    $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name})
>>>> +			if !$ticket;
>>>
>>> can we do this a bit more separate like:
>>>
>>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>>>     $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name});
>>> }
>> 
>> this would then (with the next patch) become:
>> 
>> my $api_token;
>> if (!$ticket && (my $auth_header = $r->header('Authorization')) {
>>     $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name});
>> 
>>     if (!$ticket) {
>>         $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name});
>>     }
>> }
>> 
> the inner (apitoken) "if" would be nicer to move a layer out below the other one.>>

it needs $auth_header though, which would then also move (back out 
again):

my $auth_header = $r->header('Authorization');
if (!$ticket) {
    $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name});
}
my $api_token;
if (!$ticket) {
    $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name});
}

which is basically the original version, modulo separate declaration of 
$api_token:

my $auth_header = $r->header('Authorization');

$ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name})
    if !$ticket;

my $api_token;
$api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name})
    if !$ticket;

>>> or if you prefer:
>>>
>>> if (!$ticket) {
>>>     my $auth_header = $r->header('Authorization');
>>>     $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name});
>>> }
>> 
>> my $api_token;
>>  if (!$ticket) {
>>      my $auth_header = $r->header('Authorization');
>>      $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name});
>> 
>>      if (!$ticket) {
>>         $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name});
>>      }
>>  }
>> 
>>>
>>> makes it slightly cleaner/easier to read, IMO
>> 
>> which makes it harder to parse IMHO, but if you still prefer it I will 
>> rewrite it with your suggestion for v2? we could of course also add 
>> comments, like so:
>> 
>> # check actual cookie
> s/check/prefer/
> 
>> my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name});
>> 
>> # fallback to cookie in 'Authorization' header
>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name})
>>     if !$ticket;
>> 
>> # fallback to API token
>> my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name})
>>     if !$ticket;
> NAK, do *not* declare variables guarded with a postfix if, that gets
> bad results.[0][1]

fair enough (I always forgot about this) - it's a separate issue from 
the question of whether to nest or not nest the if's though ;)

> 
> [0]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html
> [1]: https://perldoc.perl.org/perlsyn.html#Statement-Modifiers
> 
>