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

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

2019-11-12 Thread Thomas Lamprecht

On 11/12/19 11:17 AM, Fabian Grünbichler wrote:
> 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;
> 

Above with line spacing and comments would already be a big
improvement. I feel that the real-nice-thing is still missing though,
but no better idea myself ^^


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

2019-11-12 Thread 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
> 
> 

___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

2019-11-12 Thread 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


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

2019-11-12 Thread Fabian Grünbichler

On October 17, 2019 5:33 pm, Thomas Lamprecht wrote:
> On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
>> based on idea & RFC by Tim Marx, incorporating feedback by Thomas
>> Lamprecht. this will be extended to support API tokens in the
>> Authorization header as well, so make it generic.
>> 
>> Signed-off-by: Fabian Grünbichler 
>> ---
>> 
>> Notes:
>> semi-independent, could also leave extract_auth_cookie as alias/wrapper 
>> to
>> avoid a change in PMG. but since we need to change other method 
>> signatures
>> anyway for the token part, we could change this as well.
>> 
>> as-is, needs a versioned breaks/depends on pve-manager and some part of 
>> PMG?
>> 
>>  PVE/APIServer/AnyEvent.pm  |  5 -
>>  PVE/APIServer/Formatter.pm | 12 ++--
>>  2 files changed, 10 insertions(+), 7 deletions(-)
>> 
>> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
>> index 9aba27d..c0b90ab 100644
>> --- a/PVE/APIServer/AnyEvent.pm
>> +++ b/PVE/APIServer/AnyEvent.pm
>> @@ -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});
}
}

> 
> 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
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;

> 
>>  
>>  my ($rel_uri, $format) = &$split_abs_uri($path, 
>> $self->{base_uri});
>>  if (!$format) {
>> diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm
>> index 0c459bd..def1932 100644
>> --- a/PVE/APIServer/Formatter.pm
>> +++ b/PVE/APIServer/Formatter.pm
>> @@ -75,16 +75,16 @@ sub get_login_formatter {
>>  
>>  # some helper functions
>>  
>> -sub extract_auth_cookie {
>> -my ($cookie, $cookie_name) = @_;
>> +sub extract_auth_value {
>> +my ($header, $key) = @_;
>>  
>> -return undef if !$cookie;
>> +return undef if !$header;
>>  
>> -my $ticket = ($cookie =~ /(?:^|\s)\Q$cookie_name\E=([^;]*)/)[0];
>> +my $value = ($header =~ /(?:^|\s)\Q$key\E(?:=| )([^;]*)/)[0];
>>  
>> -$ticket = uri_unescape($ticket) if $ticket;
>> +$value = uri_unescape($value) if $value;
>>  
>> -return $ticket;
>> +return $value;
>>  }
>>  
>>  sub create_auth_cookie {
>> 
> 
> 
> 

___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

2019-10-17 Thread Thomas Lamprecht

On 10/17/19 3:14 PM, Fabian Grünbichler wrote:
> based on idea & RFC by Tim Marx, incorporating feedback by Thomas
> Lamprecht. this will be extended to support API tokens in the
> Authorization header as well, so make it generic.
> 
> Signed-off-by: Fabian Grünbichler 
> ---
> 
> Notes:
> semi-independent, could also leave extract_auth_cookie as alias/wrapper to
> avoid a change in PMG. but since we need to change other method signatures
> anyway for the token part, we could change this as well.
> 
> as-is, needs a versioned breaks/depends on pve-manager and some part of 
> PMG?
> 
>  PVE/APIServer/AnyEvent.pm  |  5 -
>  PVE/APIServer/Formatter.pm | 12 ++--
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
> index 9aba27d..c0b90ab 100644
> --- a/PVE/APIServer/AnyEvent.pm
> +++ b/PVE/APIServer/AnyEvent.pm
> @@ -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});
}

or if you prefer:

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

makes it slightly cleaner/easier to read, IMO

>  
>   my ($rel_uri, $format) = &$split_abs_uri($path, 
> $self->{base_uri});
>   if (!$format) {
> diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm
> index 0c459bd..def1932 100644
> --- a/PVE/APIServer/Formatter.pm
> +++ b/PVE/APIServer/Formatter.pm
> @@ -75,16 +75,16 @@ sub get_login_formatter {
>  
>  # some helper functions
>  
> -sub extract_auth_cookie {
> -my ($cookie, $cookie_name) = @_;
> +sub extract_auth_value {
> +my ($header, $key) = @_;
>  
> -return undef if !$cookie;
> +return undef if !$header;
>  
> -my $ticket = ($cookie =~ /(?:^|\s)\Q$cookie_name\E=([^;]*)/)[0];
> +my $value = ($header =~ /(?:^|\s)\Q$key\E(?:=| )([^;]*)/)[0];
>  
> -$ticket = uri_unescape($ticket) if $ticket;
> +$value = uri_unescape($value) if $value;
>  
> -return $ticket;
> +return $value;
>  }
>  
>  sub create_auth_cookie {
> 



___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

2019-10-17 Thread Fabian Grünbichler

based on idea & RFC by Tim Marx, incorporating feedback by Thomas
Lamprecht. this will be extended to support API tokens in the
Authorization header as well, so make it generic.

Signed-off-by: Fabian Grünbichler 
---

Notes:
semi-independent, could also leave extract_auth_cookie as alias/wrapper to
avoid a change in PMG. but since we need to change other method signatures
anyway for the token part, we could change this as well.

as-is, needs a versioned breaks/depends on pve-manager and some part of PMG?

 PVE/APIServer/AnyEvent.pm  |  5 -
 PVE/APIServer/Formatter.pm | 12 ++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm
index 9aba27d..c0b90ab 100644
--- a/PVE/APIServer/AnyEvent.pm
+++ b/PVE/APIServer/AnyEvent.pm
@@ -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;
 
my ($rel_uri, $format) = &$split_abs_uri($path, 
$self->{base_uri});
if (!$format) {
diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm
index 0c459bd..def1932 100644
--- a/PVE/APIServer/Formatter.pm
+++ b/PVE/APIServer/Formatter.pm
@@ -75,16 +75,16 @@ sub get_login_formatter {
 
 # some helper functions
 
-sub extract_auth_cookie {
-my ($cookie, $cookie_name) = @_;
+sub extract_auth_value {
+my ($header, $key) = @_;
 
-return undef if !$cookie;
+return undef if !$header;
 
-my $ticket = ($cookie =~ /(?:^|\s)\Q$cookie_name\E=([^;]*)/)[0];
+my $value = ($header =~ /(?:^|\s)\Q$key\E(?:=| )([^;]*)/)[0];
 
-$ticket = uri_unescape($ticket) if $ticket;
+$value = uri_unescape($value) if $value;
 
-return $ticket;
+return $value;
 }
 
 sub create_auth_cookie {
-- 
2.20.1


___
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information