Improve OpenID auth handling on not_openid response

Enhanced error handling by including OpenID provider URI in error messages for invalid data. The error seems sporadic and as part of the research on https://progress.opensuse.org/issues/167266 we can consider this not a severe problem. Because of the sensitivity of the arguments, it is not feasible to add any info from the available data for later debbugging. But at least now we can tell which server cause problems. Signed-off-by: Ioannis Bonatakis <[email protected]>
os-autoinst · Oct 2, 2024 · 8ce5afe · 8ce5afe
1 parent 7a5fed2
commit 8ce5afe
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 9 deletions.
diff --git a/lib/OpenQA/WebAPI/Auth/OpenID.pm b/lib/OpenQA/WebAPI/Auth/OpenID.pm
@@ -108,18 +108,21 @@ 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 () {
+            my $op_uri = $params{'openid.op_endpoint'} // '';
+            $err_handler->('Failed to login', "OpenID provider '$op_uri' 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);
             $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) { _handle_verified($c, $vident) },
-        error => sub (@args) { $err_handler->(@args) },
+        # Do something appropriate when the user hits "cancel" at the OP
+        cancelled => sub () { },    # uncoverable statement
+        verified => sub ($vident) { _handle_verified($c, $vident) },    # uncoverable statement
+        error => sub (@args) { $err_handler->(@args) },    # uncoverable statement
     );
 
     return (redirect => decode_base64url($csr->args('return_page'), error => 0)) if $csr->args('return_page');

diff --git a/t/03-auth-openid.t b/t/03-auth-openid.t
@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 use Test::Most;
+use Mojo::Base -signatures;
 use Test::Warnings ':report_warnings';
 use Test::MockModule;
 use Test::MockObject;
@@ -26,11 +27,30 @@ ok OpenQA::WebAPI::Auth::OpenID::_handle_verified($c, $vident), 'can call _handl
 $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');
+    req => Test::MockObject->new->set_always(
+        params => Test::MockObject->new->set_always(pairs => ['openid.op_endpoint', 'https://www.opensuse.org/openid/'])
+    )->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')))
+  ->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');
 
+my $mock_openid_consumer = Test::MockModule->new('Net::OpenID::Consumer');
+$mock_openid_consumer->redefine(
+    'handle_server_response',
+    sub ($self, %res_handlers) {
+        return $res_handlers{setup_needed}
+          ? $res_handlers{setup_needed}->("https://www.opensuse.org/openid/setup")
+          : undef;
+    });
+$c->set_always(
+    req => Test::MockObject->new->set_always(
+        params => Test::MockObject->new->set_always(pairs => ['openid.op_endpoint', 'https://www.opensuse.org/openid/'])
+    )->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('debug')))
+  ->set_true('flash');
+is OpenQA::WebAPI::Auth::OpenID::auth_response($c), 0, 'can handle setup_needed response';
+$c->app->log->called_ok('debug', 'a debug messgae is logged when setup_needed respond');
 done_testing;