From 29268938e7c416f8a0fe6e32946bcd7b7a98baa5 Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Tue, 23 Nov 2021 10:24:29 +0100 Subject: [PATCH 1/6] t: Fix typo in 03-auth.t --- t/03-auth.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/03-auth.t b/t/03-auth.t index e44d67d789a..bfa3cd45410 100644 --- a/t/03-auth.t +++ b/t/03-auth.t @@ -59,7 +59,7 @@ subtest OpenID => sub { handle_server_response => sub ($self, %args) { }, args => sub ($self, $query) { {return_page => encode_base64url('/tests/42')}->{$query} }); $t->get_ok('/response')->status_is(302); - $t->header_is('Location', '/tests/42', 'redirect to original papge after login'); + $t->header_is('Location', '/tests/42', 'redirect to original page after login'); $t->get_ok('/api_keys')->status_is(302); $t->header_is('Location', '/login?return_page=%2Fapi_keys', 'remember return_page for ensure_operator'); From fc3642b3234c28e78207e48ecf0a87195f1aa83c Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Mon, 30 Aug 2021 12:30:50 +0200 Subject: [PATCH 2/6] Simplify OpenQA::WebAPI::Auth::OpenID --- lib/OpenQA/WebAPI/Auth/OpenID.pm | 75 +++++++++++++------------------- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/lib/OpenQA/WebAPI/Auth/OpenID.pm b/lib/OpenQA/WebAPI/Auth/OpenID.pm index 9d4a09ac12e..b5913aa6b72 100644 --- a/lib/OpenQA/WebAPI/Auth/OpenID.pm +++ b/lib/OpenQA/WebAPI/Auth/OpenID.pm @@ -63,6 +63,29 @@ sub auth_login ($self) { return (error => $csr->err); } +sub _first_last_name ($ax) { join(' ', $ax->{'value.firstname'} // '', $ax->{'value.lastname'} // '') } + +sub _create_user ($self, $id, $email, $nickname, $fullname) { + $self->schema->resultset('Users')->create_user($id, email => $email, nickname => $nickname, fullname => $fullname); +} + +sub _handle_verified ($self, $vident) { + my $sreg = $vident->signed_extension_fields('http://openid.net/extensions/sreg/1.1'); + my $ax = $vident->signed_extension_fields('http://openid.net/srv/ax/1.0'); + + my $email = $sreg->{email} || $ax->{'value.email'} || 'nobody@example.com'; + my $nickname = $sreg->{nickname} || $ax->{'value.nickname'} || $ax->{'value.firstname'}; + unless ($nickname) { + my @a = split(/\/([^\/]+)$/, $vident->{identity}); + $nickname = $a[1]; + } + + my $fullname = $sreg->{fullname} || $ax->{'value.fullname'} || _first_last_name($ax) || $nickname; + + $self->_create_user($vident->{identity}, $email, $nickname, $fullname); + $self->session->{user} = $vident->{identity}; +} + sub auth_response ($self) { my %params = @{$self->req->params->pairs}; my $url = $self->app->config->{global}->{base_url} || $self->req->url->base; @@ -71,69 +94,31 @@ sub auth_response ($self) { %params = map { $_ => URI::Escape::uri_unescape($params{$_}) } keys %params; my $csr = Net::OpenID::Consumer->new( - debug => sub { $self->app->log->debug('Net::OpenID::Consumer: ' . join(' ', @_)); }, + debug => sub { $self->app->log->debug('Net::OpenID::Consumer: ' . join(' ', @args)) }, ua => LWP::UserAgent->new, required_root => $url, consumer_secret => $self->app->config->{_openid_secret}, args => \%params, ); - my $err_handler = sub { - my ($err, $txt) = @_; + my $err_handler = sub ($err, $txt) { $self->app->log->error("OpenID: $err: $txt"); $self->flash(error => "$err: $txt"); return (error => 0); }; $csr->handle_server_response( - not_openid => sub { - return $err_handler->('Failed to login', 'OpenID provider returned invalid data. Please retry again'); - }, - setup_needed => sub { - my $setup_url = shift; - + not_openid => sub () { $err_handler->('Failed to login', 'OpenID provider returned invalid data. Please retry again') }, + setup_needed => sub ($setup_url) { # Redirect the user to $setup_url $setup_url = URI::Escape::uri_unescape($setup_url); $self->app->log->debug(qq{setup_url[$setup_url]}); return (redirect => $setup_url, error => 0); }, - cancelled => sub { }, # Do something appropriate when the user hits "cancel" at the OP - verified => sub { - my $vident = shift; - my $sreg = $vident->signed_extension_fields('http://openid.net/extensions/sreg/1.1'); - my $ax = $vident->signed_extension_fields('http://openid.net/srv/ax/1.0'); - - my $email = $sreg->{email} || $ax->{'value.email'} || 'nobody@example.com'; - my $nickname = $sreg->{nickname} || $ax->{'value.nickname'} || $ax->{'value.firstname'}; - unless ($nickname) { - my @a = split(/\/([^\/]+)$/, $vident->{identity}); - $nickname = $a[1]; - } - my $fullname = $sreg->{fullname} || $ax->{'value.fullname'}; - unless ($fullname) { - if ($ax->{'value.firstname'}) { - $fullname = $ax->{'value.firstname'}; - if ($ax->{'value.lastname'}) { - $fullname .= ' ' . $ax->{'value.lastname'}; - } - } - else { - $fullname = $nickname; - } - } - - my $user = $self->schema->resultset('Users')->create_user( - $vident->{identity}, - email => $email, - nickname => $nickname, - fullname => $fullname - ); - $self->session->{user} = $vident->{identity}; - }, - error => sub { - return $err_handler->(@_); - }, + cancelled => sub () { }, # Do something appropriate when the user hits "cancel" at the OP + verified => sub ($vident) { $self->_handle_verified($vident) }, + error => sub (@args) { $err_handler->(@args) }, ); return (redirect => decode_base64url($csr->args('return_page'), error => 0)) if $csr->args('return_page'); From 9653aea26aae3038ed29eb0c1da80bf7c8c703df Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Tue, 23 Nov 2021 15:19:10 +0100 Subject: [PATCH 3/6] Clarify how OpenQA::WebAPI::Auth::OpenID uses controller argument --- lib/OpenQA/WebAPI/Auth/OpenID.pm | 42 ++++++++++++++++---------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/lib/OpenQA/WebAPI/Auth/OpenID.pm b/lib/OpenQA/WebAPI/Auth/OpenID.pm index b5913aa6b72..9a21e481047 100644 --- a/lib/OpenQA/WebAPI/Auth/OpenID.pm +++ b/lib/OpenQA/WebAPI/Auth/OpenID.pm @@ -9,19 +9,19 @@ use LWP::UserAgent; use Net::OpenID::Consumer; use MIME::Base64 qw(encode_base64url decode_base64url); -sub auth_login ($self) { - my $url = $self->app->config->{global}->{base_url} || $self->req->url->base->to_string; +sub auth_login ($c) { + my $url = $c->app->config->{global}->{base_url} || $c->req->url->base->to_string; # force secure connection after login - $url =~ s,^http://,https://, if $self->app->config->{openid}->{httpsonly}; + $url =~ s,^http://,https://, if $c->app->config->{openid}->{httpsonly}; my $csr = Net::OpenID::Consumer->new( ua => LWP::UserAgent->new, required_root => $url, - consumer_secret => $self->app->config->{_openid_secret}, + consumer_secret => $c->app->config->{_openid_secret}, ); - my $claimed_id = $csr->claimed_identity($self->config->{openid}->{provider}); + my $claimed_id = $csr->claimed_identity($c->config->{openid}->{provider}); if (!defined $claimed_id) { log_error("Claiming OpenID identity for URL '$url' failed: " . $csr->err); return; @@ -47,7 +47,7 @@ sub auth_login ($self) { ); my $return_url = Mojo::URL->new(qq{$url/response}); - if (my $return_page = $self->param('return_page') || $self->req->headers->referrer) { + if (my $return_page = $c->param('return_page') || $c->req->headers->referrer) { $return_page = Mojo::URL->new($return_page)->path_query; # return_page is encoded using base64 (in a version that avoids / and + symbol) # as any special characters like / or ? when urlencoded via % symbols, @@ -65,11 +65,11 @@ sub auth_login ($self) { sub _first_last_name ($ax) { join(' ', $ax->{'value.firstname'} // '', $ax->{'value.lastname'} // '') } -sub _create_user ($self, $id, $email, $nickname, $fullname) { - $self->schema->resultset('Users')->create_user($id, email => $email, nickname => $nickname, fullname => $fullname); +sub _create_user ($c, $id, $email, $nickname, $fullname) { + $c->schema->resultset('Users')->create_user($id, email => $email, nickname => $nickname, fullname => $fullname); } -sub _handle_verified ($self, $vident) { +sub _handle_verified ($c, $vident) { my $sreg = $vident->signed_extension_fields('http://openid.net/extensions/sreg/1.1'); my $ax = $vident->signed_extension_fields('http://openid.net/srv/ax/1.0'); @@ -82,28 +82,28 @@ sub _handle_verified ($self, $vident) { my $fullname = $sreg->{fullname} || $ax->{'value.fullname'} || _first_last_name($ax) || $nickname; - $self->_create_user($vident->{identity}, $email, $nickname, $fullname); - $self->session->{user} = $vident->{identity}; + _create_user($c, $vident->{identity}, $email, $nickname, $fullname); + $c->session->{user} = $vident->{identity}; } -sub auth_response ($self) { - my %params = @{$self->req->params->pairs}; - my $url = $self->app->config->{global}->{base_url} || $self->req->url->base; +sub auth_response ($c) { + my %params = @{$c->req->params->pairs}; + my $url = $c->app->config->{global}->{base_url} || $c->req->url->base; return (error => 'Got response on http but https is forced. MOJO_REVERSE_PROXY not set?') - if ($self->app->config->{openid}->{httpsonly} && $url !~ /^https:\/\//); + if ($c->app->config->{openid}->{httpsonly} && $url !~ /^https:\/\//); %params = map { $_ => URI::Escape::uri_unescape($params{$_}) } keys %params; my $csr = Net::OpenID::Consumer->new( - debug => sub { $self->app->log->debug('Net::OpenID::Consumer: ' . join(' ', @args)) }, + debug => sub (@args) { $c->app->log->debug('Net::OpenID::Consumer: ' . join(' ', @args)) }, ua => LWP::UserAgent->new, required_root => $url, - consumer_secret => $self->app->config->{_openid_secret}, + consumer_secret => $c->app->config->{_openid_secret}, args => \%params, ); my $err_handler = sub ($err, $txt) { - $self->app->log->error("OpenID: $err: $txt"); - $self->flash(error => "$err: $txt"); + $c->app->log->error("OpenID: $err: $txt"); + $c->flash(error => "$err: $txt"); return (error => 0); }; @@ -112,12 +112,12 @@ sub auth_response ($self) { setup_needed => sub ($setup_url) { # Redirect the user to $setup_url $setup_url = URI::Escape::uri_unescape($setup_url); - $self->app->log->debug(qq{setup_url[$setup_url]}); + $c->app->log->debug(qq{setup_url[$setup_url]}); return (redirect => $setup_url, error => 0); }, cancelled => sub () { }, # Do something appropriate when the user hits "cancel" at the OP - verified => sub ($vident) { $self->_handle_verified($vident) }, + verified => sub ($vident) { _handle_verified($c, $vident) }, error => sub (@args) { $err_handler->(@args) }, ); From 8796794c916c4f2ea69755cb69a425021d9bad88 Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Tue, 23 Nov 2021 15:14:27 +0100 Subject: [PATCH 4/6] t: Add test for OpenQA::WebAPI::Auth::OpenID --- t/03-auth-openid.t | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 t/03-auth-openid.t diff --git a/t/03-auth-openid.t b/t/03-auth-openid.t new file mode 100644 index 00000000000..49489220256 --- /dev/null +++ b/t/03-auth-openid.t @@ -0,0 +1,23 @@ +# Copyright SUSE LLC +# SPDX-License-Identifier: GPL-2.0-or-later + +use Test::Most; +use Test::Warnings ':report_warnings'; +use Test::MockModule; +use Test::MockObject; +use FindBin; +use lib "$FindBin::Bin/lib", "$FindBin::Bin/../external/os-autoinst-common/lib"; +use OpenQA::Test::TimeLimit '10'; +use OpenQA::WebAPI::Auth::OpenID; + +is OpenQA::WebAPI::Auth::OpenID::_first_last_name({'value.firstname' => 'Mordred'}), 'Mordred ', + '_first_last_name concats also with empty fields'; +my $openid = OpenQA::WebAPI::Auth::OpenID->new(); +my (%openid_res, %openid_res2); +my $vident = Test::MockObject->new->set_series('signed_extension_fields', \%openid_res, \%openid_res2); +$vident->{identity} = 'mordred'; +my $mock = Test::MockModule->new('OpenQA::WebAPI::Auth::OpenID'); +$mock->noop('_create_user'); +$mock->mock('session', {}); +ok $openid->_handle_verified($vident), 'can call _handle_verified'; +done_testing; From d5d599457c46201e4c71b82616be20d265512d80 Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Tue, 23 Nov 2021 17:07:16 +0100 Subject: [PATCH 5/6] t: Extend unit-test for WebAPI::Auth::OpenID Tested with ``` rm -rf cover_db/ && cover -coverage stmt -test -make 'prove -Ilib t/03-auth-openid.t; echo 0' && html2text cover_db/lib-OpenQA-WebAPI-Auth-OpenID-pm.html ``` this can be used to extend further and cover more paths. --- t/03-auth-openid.t | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/t/03-auth-openid.t b/t/03-auth-openid.t index 49489220256..54426804933 100644 --- a/t/03-auth-openid.t +++ b/t/03-auth-openid.t @@ -10,14 +10,23 @@ use lib "$FindBin::Bin/lib", "$FindBin::Bin/../external/os-autoinst-common/lib"; use OpenQA::Test::TimeLimit '10'; use OpenQA::WebAPI::Auth::OpenID; + is OpenQA::WebAPI::Auth::OpenID::_first_last_name({'value.firstname' => 'Mordred'}), 'Mordred ', '_first_last_name concats also with empty fields'; -my $openid = OpenQA::WebAPI::Auth::OpenID->new(); my (%openid_res, %openid_res2); my $vident = Test::MockObject->new->set_series('signed_extension_fields', \%openid_res, \%openid_res2); $vident->{identity} = 'mordred'; -my $mock = Test::MockModule->new('OpenQA::WebAPI::Auth::OpenID'); -$mock->noop('_create_user'); -$mock->mock('session', {}); -ok $openid->_handle_verified($vident), 'can call _handle_verified'; +my $users = Test::MockObject->new->set_always('create_user', 1); +my $schema = Test::MockObject->new->set_always(resultset => $users); +my %session; +my $c + = Test::MockObject->new->set_always(schema => $schema)->set_always(session => \%session)->set_true('_create_user'); +ok OpenQA::WebAPI::Auth::OpenID::_handle_verified($c, $vident), 'can call _handle_verified'; +$c->set_always( + req => Test::MockObject->new->set_always(params => Test::MockObject->new->set_always(pairs => [1, 2])) + ->set_always(url => Test::MockObject->new->set_always(base => 'openqa'))) + ->set_always(app => Test::MockObject->new->set_always(config => {}) + ->set_always(log => Test::MockObject->new->set_true('error', 'debug')))->set_true('flash'); +is OpenQA::WebAPI::Auth::OpenID::auth_response($c), 0, 'can call auth_response'; + done_testing; From f3c5f75186c6250ea7e28574d589c8bf5f7b1d4c Mon Sep 17 00:00:00 2001 From: Oliver Kurz Date: Wed, 24 Nov 2021 22:06:03 +0100 Subject: [PATCH 6/6] t: Extend 03-auth-openid.t for _create_user and call checks --- lib/OpenQA/WebAPI/Auth/OpenID.pm | 3 ++- t/03-auth-openid.t | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/OpenQA/WebAPI/Auth/OpenID.pm b/lib/OpenQA/WebAPI/Auth/OpenID.pm index 9a21e481047..39e5be7165f 100644 --- a/lib/OpenQA/WebAPI/Auth/OpenID.pm +++ b/lib/OpenQA/WebAPI/Auth/OpenID.pm @@ -108,7 +108,8 @@ sub auth_response ($c) { }; $csr->handle_server_response( - not_openid => sub () { $err_handler->('Failed to login', 'OpenID provider returned invalid data. Please retry again') }, + not_openid => + sub () { $err_handler->('Failed to login', 'OpenID provider returned invalid data. Please retry again') }, setup_needed => sub ($setup_url) { # Redirect the user to $setup_url $setup_url = URI::Escape::uri_unescape($setup_url); diff --git a/t/03-auth-openid.t b/t/03-auth-openid.t index 54426804933..10bfdd42fd1 100644 --- a/t/03-auth-openid.t +++ b/t/03-auth-openid.t @@ -15,18 +15,22 @@ is OpenQA::WebAPI::Auth::OpenID::_first_last_name({'value.firstname' => 'Mordred '_first_last_name concats also with empty fields'; my (%openid_res, %openid_res2); my $vident = Test::MockObject->new->set_series('signed_extension_fields', \%openid_res, \%openid_res2); -$vident->{identity} = 'mordred'; +my $user = $vident->{identity} = 'mordred'; my $users = Test::MockObject->new->set_always('create_user', 1); my $schema = Test::MockObject->new->set_always(resultset => $users); my %session; -my $c - = Test::MockObject->new->set_always(schema => $schema)->set_always(session => \%session)->set_true('_create_user'); +my $c = Test::MockObject->new->set_always(schema => $schema); +ok OpenQA::WebAPI::Auth::OpenID::_create_user($c, $user, 'nobody\@example.com', $user, $user), 'can call _create_user'; +$c->set_always(session => \%session); ok OpenQA::WebAPI::Auth::OpenID::_handle_verified($c, $vident), 'can call _handle_verified'; +$users->called_ok('create_user', 'new user is created for initial login'); +is(($users->call_args(2))[1], 'mordred', 'new user created with details'); $c->set_always( req => Test::MockObject->new->set_always(params => Test::MockObject->new->set_always(pairs => [1, 2])) ->set_always(url => Test::MockObject->new->set_always(base => 'openqa'))) ->set_always(app => Test::MockObject->new->set_always(config => {}) ->set_always(log => Test::MockObject->new->set_true('error', 'debug')))->set_true('flash'); is OpenQA::WebAPI::Auth::OpenID::auth_response($c), 0, 'can call auth_response'; +$c->app->log->called_ok('error', 'an error was logged for call without proper config'); done_testing;