Merge pull request #4156 from okurz/enhance/openid

Simplify OpenQA::WebAPI::Auth::OpenID

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,
@@ -63,77 +63,63 @@ sub auth_login ($self) {
     return (error => $csr->err);
 }
 
-sub auth_response ($self) {
-    my %params = @{$self->req->params->pairs};
-    my $url = $self->app->config->{global}->{base_url} || $self->req->url->base;
+sub _first_last_name ($ax) { join(' ', $ax->{'value.firstname'} // '', $ax->{'value.lastname'} // '') }
+
+sub _create_user ($c, $id, $email, $nickname, $fullname) {
+    $c->schema->resultset('Users')->create_user($id, email => $email, nickname => $nickname, fullname => $fullname);
+}
+
+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');
+
+    my $email = $sreg->{email} || $ax->{'value.email'} || '[email protected]';
+    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;
+
+    _create_user($c, $vident->{identity}, $email, $nickname, $fullname);
+    $c->session->{user} = $vident->{identity};
+}
+
+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(' ', @_)); },
+        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 {
-        my ($err, $txt) = @_;
-        $self->app->log->error("OpenID: $err: $txt");
-        $self->flash(error => "$err: $txt");
+    my $err_handler = sub ($err, $txt) {
+        $c->app->log->error("OpenID: $err: $txt");
+        $c->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]});
+            $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 {
-            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'} || '[email protected]';
-            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) { _handle_verified($c, $vident) },
+        error => sub (@args) { $err_handler->(@args) },
     );
 
     return (redirect => decode_base64url($csr->args('return_page'), error => 0)) if $csr->args('return_page');

t/03-auth-openid.t

@@ -0,0 +1,36 @@
+# 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_res, %openid_res2);
+my $vident = Test::MockObject->new->set_series('signed_extension_fields', \%openid_res, \%openid_res2);
+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);
+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;

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