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
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
