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

[Thomas Lamprecht]

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.>>
>> 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]

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