From 8ce5afed357e04d8ed3a5724fcda9f3c431908dc Mon Sep 17 00:00:00 2001
From: Ioannis Bonatakis <ybonatakis@suse.com>
Date: Thu, 26 Sep 2024 17:06:40 +0200
Subject: [PATCH] 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 <ybonatakis@suse.com>
---
 lib/OpenQA/WebAPI/Auth/OpenID.pm | 13 ++++++++-----
 t/03-auth-openid.t               | 28 ++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/lib/OpenQA/WebAPI/Auth/OpenID.pm b/lib/OpenQA/WebAPI/Auth/OpenID.pm
index 39e5be7165f..082b34aab8f 100644
--- a/lib/OpenQA/WebAPI/Auth/OpenID.pm
+++ b/lib/OpenQA/WebAPI/Auth/OpenID.pm
@@ -108,8 +108,10 @@ 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);
@@ -117,9 +119,10 @@ sub auth_response ($c) {
 
             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
index 10bfdd42fd1..bcad6bdfe1d 100644
--- 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;