Attachment 129689 Details for Bug 29894 – Bug 29894: Add some exceptions to TwoFactorAuth module

[patch] Bug 29894: Add some exceptions to TwoFactorAuth module

Bug-29894-Add-some-exceptions-to-TwoFactorAuth-mod.patch (text/plain), 6.79 KB, created by Marcel de Rooy on 2022-01-21 10:29:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Marcel de Rooy

Created: 2022-01-21 10:29:13 UTC

Size: 6.79 KB

patch

obsolete

>From c3fe286a1a25c02c972efecbec8d95430835ee68 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Thu, 20 Jan 2022 14:43:42 +0000
>Subject: [PATCH] Bug 29894: Add some exceptions to TwoFactorAuth module
>Content-Type: text/plain; charset=utf-8
>
>Test updated accordingly.
>Adding utf8 flag to CGI in staff script.
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>---
> 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 9f6639396e..193df447f8 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 <http://www.gnu.org/licenses>.
> 
> use Modern::Perl;
>-use Auth::GoogleAuth;
> use Imager::QRCode;
> 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.20.1

Actions: View | Diff | Splinter Review

Attachments on bug 29894: 129538 | 129539 | 129669 | 129670 | 129689 | 129690 | 129691 | 129692 | 133286 | 133287 | 133288 | 133289 | 133562 | 133563 | 133564 | 133565 | 133566 | 133618 | 133661 | 133662 | 133663 | 133664 | 133665 | 133666 | 133667