From d42cd2b6298bf2765b6a609909526f774450c250 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 20 Jan 2022 14:43:42 +0000 Subject: [PATCH] Bug 29894: Add some exceptions to TwoFactorAuth module Test updated accordingly. Adding utf8 flag to CGI in staff script. Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart Signed-off-by: Fridolin Somers --- Koha/Auth/TwoFactorAuth.pm | 39 ++++++++++------ members/two_factor_auth.pl | 2 +- t/db_dependent/Koha/Auth/TwoFactorAuth.t | 59 ++++++++++++++++++------ 3 files changed, 71 insertions(+), 29 deletions(-) diff --git a/Koha/Auth/TwoFactorAuth.pm b/Koha/Auth/TwoFactorAuth.pm index e2cff4487f..efa9b9d829 100644 --- a/Koha/Auth/TwoFactorAuth.pm +++ b/Koha/Auth/TwoFactorAuth.pm @@ -16,10 +16,11 @@ package Koha::Auth::TwoFactorAuth; # along with Koha; if not, see . use Modern::Perl; -use Auth::GoogleAuth; use GD::Barcode; use MIME::Base64 qw( encode_base64 ); +use Koha::Exceptions; + use base qw( Auth::GoogleAuth ); =head1 NAME @@ -42,30 +43,42 @@ It's based on Auth::GoogleAuth $obj = Koha::Auth::TwoFactorAuth->new({ patron => $p, secret => $s }); + Patron is mandatory. + Secret is optional, defaults to patron's secret. + Passing secret32 overrules secret! Secret32 should be base32. + =cut sub new { my ($class, $params) = @_; my $patron = $params->{patron}; - my $secret = $params->{secret}; my $secret32 = $params->{secret32}; - - if (!$secret && !$secret32){ - $secret32 = $patron->secret; + my $secret = $params->{secret}; + + Koha::Exceptions::MissingParameter->throw("Mandatory patron parameter missing") + unless $patron && ref($patron) eq 'Koha::Patron'; + + my $type = 'secret32'; + if( $secret32 ) { + Koha::Exceptions::BadParameter->throw("Secret32 should be base32") + if $secret32 =~ /[^a-z2-7]/; + } elsif( $secret ) { + $type = 'secret'; + } elsif( $patron->secret ) { + $secret32 = $patron->secret; # saved already in base32 + } else { + Koha::Exceptions::MissingParameter->throw("No secret passed or patron has no secret"); } my $issuer = $patron->library->branchname; my $key_id = sprintf "%s_%s", $issuer, ( $patron->email || $patron->userid ); - return $class->SUPER::new( - { - ( $secret ? ( secret => $secret ) : () ), - ( $secret32 ? ( secret32 => $secret32 ) : () ), - issuer => $issuer, - key_id => $key_id, - } - ); + return $class->SUPER::new({ + $type => $secret32 || $secret, + issuer => $issuer, + key_id => $key_id, + }); } =head3 qr_code diff --git a/members/two_factor_auth.pl b/members/two_factor_auth.pl index b874a6f97d..0cf6843b84 100755 --- a/members/two_factor_auth.pl +++ b/members/two_factor_auth.pl @@ -17,7 +17,7 @@ use Modern::Perl; -use CGI; +use CGI qw(-utf8); use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_and_exit output_html_with_http_headers ); diff --git a/t/db_dependent/Koha/Auth/TwoFactorAuth.t b/t/db_dependent/Koha/Auth/TwoFactorAuth.t index 6f76cd0749..2bd0e9128b 100755 --- a/t/db_dependent/Koha/Auth/TwoFactorAuth.t +++ b/t/db_dependent/Koha/Auth/TwoFactorAuth.t @@ -1,5 +1,6 @@ use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 2; +use Test::Exception; use t::lib::Mocks; use t::lib::TestBuilder; @@ -10,31 +11,59 @@ use Koha::Auth::TwoFactorAuth; our $schema = Koha::Database->new->schema; our $builder = t::lib::TestBuilder->new; -subtest 'qr_code' => sub { - plan tests => 9; - +subtest 'new' => sub { + plan tests => 10; $schema->storage->txn_begin; t::lib::Mocks::mock_preference('TwoFactorAuthentication', 1); - my $patron = $builder->build_object({ class => 'Koha::Patrons' }); - # Testing without secret (might change later on) + # Trivial test: no patron, no object + throws_ok { Koha::Auth::TwoFactorAuth->new; } + 'Koha::Exceptions::MissingParameter', + 'Croaked on missing patron'; + throws_ok { Koha::Auth::TwoFactorAuth->new({ patron => 'Henk', secret => q<> }) } + 'Koha::Exceptions::MissingParameter', + 'Croaked on missing patron object'; + + # Testing without secret + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); is( $patron->secret, undef, 'Secret still undefined' ); + throws_ok { Koha::Auth::TwoFactorAuth->new({ patron => $patron }) } + 'Koha::Exceptions::MissingParameter', 'Croaks on missing secret'; + # Pass a wrong encoded secret + throws_ok { Koha::Auth::TwoFactorAuth->new({ patron => $patron, secret32 => '@' }) } + 'Koha::Exceptions::BadParameter', + 'Croaked on wrong encoding'; + + # Test passing secret or secret32 (converted to base32) + $patron->secret('nv4v65dpobpxgzldojsxiii'); # this is base32 already for 'my_top_secret!' my $auth = Koha::Auth::TwoFactorAuth->new({ patron => $patron }); - is( $auth->secret, undef, 'Still no secret yet as expected' ); - # Auth::GoogleAuth will generate a secret when calling qr_code - my $img_data = $auth->qr_code; - is( length($auth->secret32), 16, 'Secret of 16 base32 chars expected' ); - is( length($img_data) > 22, 1, 'Dataurl not empty too' ); # prefix is 22 - $auth->clear; + is( $auth->secret32, $patron->secret, 'Base32 secret as expected' ); + $auth->code( $patron->secret ); # trigger conversion by passing base32 to code + is( $auth->secret, 'my_top_secret!', 'Decoded secret fine too' ); + # The other way around + $auth = Koha::Auth::TwoFactorAuth->new({ patron => $patron, secret => 'my_top_secret!' }); + is( $auth->secret32, undef, 'GoogleAuth did not yet encode' ); + $auth->code; # this will trigger base32 encoding now + is( $auth->secret, 'my_top_secret!', 'Check secret' ); + is( $auth->secret32, 'nv4v65dpobpxgzldojsxiii', 'Check secret32' ); + + $schema->storage->txn_rollback; +}; + +subtest 'qr_code' => sub { + plan tests => 5; + + $schema->storage->txn_begin; - # Update patron data + t::lib::Mocks::mock_preference('TwoFactorAuthentication', 1); + my $patron = $builder->build_object({ class => 'Koha::Patrons' }); $patron->secret('you2wont2guess2it'); # this is base32 btw $patron->auth_method('two-factor'); $patron->store; - $auth = Koha::Auth::TwoFactorAuth->new({ patron => $patron }); - $img_data = $auth->qr_code; + my $auth = Koha::Auth::TwoFactorAuth->new({ patron => $patron }); + my $img_data = $auth->qr_code; is( substr($img_data, 0, 22), 'data:image/png;base64,', 'Checking prefix of dataurl' ); like( substr($img_data, 22), qr/^[a-zA-Z0-9\/=+]+$/, 'Contains base64 chars' ); is( $auth->qr_code, $img_data, 'Repeated call' ); -- 2.39.5