Skip to content

Commit

Permalink
Improve OpenID auth handling on not_openid response
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
b10n1k committed Oct 2, 2024
1 parent 7a5fed2 commit 8ce5afe
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 9 deletions.
13 changes: 8 additions & 5 deletions lib/OpenQA/WebAPI/Auth/OpenID.pm
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
28 changes: 24 additions & 4 deletions t/03-auth-openid.t
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

0 comments on commit 8ce5afe

Please sign in to comment.