From 9c8f3261b91f980c7e7399652adedcd0a667c76f Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Thu, 21 Jul 2016 08:48:01 +0200
Subject: [PATCH] Bug 16929: [QA Follow-up] Shortcut methods and use
 statements
Content-Type: text/plain; charset=utf-8

Resolves the following comments:

I'd prefer to see a generate_csrf method than a CSRF flag.
It'd be better to use instead of require the 2 modules.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 Koha/Token.pm            |   43 ++++++++++++++++++++++++++++++++-----------
 opac/opac-memberentry.pl |   12 ++++--------
 t/Token.t                |    8 ++++----
 3 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/Koha/Token.pm b/Koha/Token.pm
index ddec453..9e26800 100644
--- a/Koha/Token.pm
+++ b/Koha/Token.pm
@@ -31,12 +31,12 @@ Koha::Token - Tokenizer
 
     # safely generate a CSRF token (nonblocking)
     my $csrf_token = $tokenizer->generate({
-        CSRF => 1, id => $id, secret => $secret,
+        type => 'CSRF', id => $id, secret => $secret,
     });
 
     # or check a CSRF token
-    my $result = $tokenizer->check({
-        CSRF => 1, id => $id, secret => $secret, token => $token,
+    my $result = $tokenizer->check_csrf({
+        id => $id, secret => $secret, token => $token,
     });
 
 =head1 DESCRIPTION
@@ -48,6 +48,9 @@ Koha::Token - Tokenizer
 =cut
 
 use Modern::Perl;
+use Bytes::Random::Secure ();
+use String::Random ();
+use WWW::CSRF ();
 use base qw(Class::Accessor);
 use constant HMAC_SHA1_LENGTH => 20;
 
@@ -68,7 +71,7 @@ sub new {
 
     my $token = $tokenizer->generate({ length => 20 });
     my $csrf_token = $tokenizer->generate({
-        CSRF => 1, id => $id, secret => $secret,
+        type => 'CSRF', id => $id, secret => $secret,
     });
 
     Generate several types of tokens. Now includes CSRF.
@@ -78,7 +81,7 @@ sub new {
 
 sub generate {
     my ( $self, $params ) = @_;
-    if( $params->{CSRF} ) {
+    if( $params->{type} && $params->{type} eq 'CSRF' ) {
         $self->{lasttoken} = _gen_csrf( $params );
     } else {
         $self->{lasttoken} = _gen_rand( $params );
@@ -86,10 +89,21 @@ sub generate {
     return $self->{lasttoken};
 }
 
+=head2 generate_csrf
+
+    Shortcut for: generate({ type => 'CSRF', ... })
+
+=cut
+
+sub generate_csrf {
+    my ( $self, $params ) = @_;
+    return $self->generate({ %$params, type => 'CSRF' });
+}
+
 =head2 check
 
     my $result = $tokenizer->check({
-        CSRF => 1, id => $id, secret => $secret, token => $token,
+        type => 'CSRF', id => $id, secret => $secret, token => $token,
     });
 
     Check several types of tokens. Now includes CSRF.
@@ -99,12 +113,23 @@ sub generate {
 
 sub check {
     my ( $self, $params ) = @_;
-    if( $params->{CSRF} ) {
+    if( $params->{type} && $params->{type} eq 'CSRF' ) {
         return _chk_csrf( $params );
     }
     return;
 }
 
+=head2 check_csrf
+
+    Shortcut for: check({ type => 'CSRF', ... })
+
+=cut
+
+sub check_csrf {
+    my ( $self, $params ) = @_;
+    return $self->check({ %$params, type => 'CSRF' });
+}
+
 # --- Internal routines ---
 
 sub _gen_csrf {
@@ -116,8 +141,6 @@ sub _gen_csrf {
     my ( $params ) = @_;
     return if !$params->{id} || !$params->{secret};
 
-    require Bytes::Random::Secure;
-    require WWW::CSRF;
 
     my $randomizer = Bytes::Random::Secure->new( NonBlocking => 1 );
         # this is most fundamental: do not use /dev/random since it is
@@ -134,7 +157,6 @@ sub _chk_csrf {
     my ( $params ) = @_;
     return if !$params->{id} || !$params->{secret} || !$params->{token};
 
-    require WWW::CSRF;
     my $csrf_status = WWW::CSRF::check_csrf_token(
         $params->{id},
         $params->{secret},
@@ -148,7 +170,6 @@ sub _gen_rand {
     my $length = $params->{length} || 1;
     $length = 1 unless $length > 0;
 
-    require String::Random;
     return String::Random::random_string( '.' x $length );
 }
 
diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl
index 399fea6..e4945e7 100755
--- a/opac/opac-memberentry.pl
+++ b/opac/opac-memberentry.pl
@@ -183,8 +183,7 @@ elsif ( $action eq 'update' ) {
 
     my $borrower = GetMember( borrowernumber => $borrowernumber );
     die "Wrong CSRF token"
-        unless Koha::Token->new->check({
-            CSRF   => 1,
+        unless Koha::Token->new->check_csrf({
             id     => $borrower->{userid},
             secret => md5_base64( C4::Context->config('pass') ),
             token  => scalar $cgi->param('csrf_token'),
@@ -202,8 +201,7 @@ elsif ( $action eq 'update' ) {
             empty_mandatory_fields => \@empty_mandatory_fields,
             invalid_form_fields    => $invalidformfields,
             borrower               => \%borrower,
-            csrf_token             => Koha::Token->new->generate({
-                CSRF   => 1,
+            csrf_token             => Koha::Token->new->generate_csrf({
                 id     => $borrower->{userid},
                 secret => md5_base64( C4::Context->config('pass') ),
             }),
@@ -238,8 +236,7 @@ elsif ( $action eq 'update' ) {
                 action => 'edit',
                 nochanges => 1,
                 borrower => GetMember( borrowernumber => $borrowernumber ),
-                csrf_token => Koha::Token->new->generate({
-                    CSRF   => 1,
+                csrf_token => Koha::Token->new->generate_csrf({
                     id     => $borrower->{userid},
                     secret => md5_base64( C4::Context->config('pass') ),
                 }),
@@ -262,8 +259,7 @@ elsif ( $action eq 'edit' ) {    #Display logged in borrower's data
         borrower  => $borrower,
         guarantor => scalar Koha::Patrons->find($borrowernumber)->guarantor(),
         hidden => GetHiddenFields( $mandatory, 'modification' ),
-        csrf_token => Koha::Token->new->generate({
-            CSRF   => 1,
+        csrf_token => Koha::Token->new->generate_csrf({
             id     => $borrower->{userid},
             secret => md5_base64( C4::Context->config('pass') ),
         }),
diff --git a/t/Token.t b/t/Token.t
index 642342f..dcc182e 100644
--- a/t/Token.t
+++ b/t/Token.t
@@ -30,16 +30,16 @@ is( length($token), 20, "Token $token has 20 chars" );
 
 my $id = $tokenizer->generate({ length => 8 });
 my $secr = $tokenizer->generate({ length => 32 });
-my $csrftoken = $tokenizer->generate({ CSRF => 1, id => $id, secret => $secr });
+my $csrftoken = $tokenizer->generate_csrf({ id => $id, secret => $secr });
 isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" );
 
 is( $tokenizer->check, undef, "Check without any parameters" );
-my $result = $tokenizer->check({
-    CSRF => 1, id => $id, secret => $secr, token => $csrftoken,
+my $result = $tokenizer->check_csrf({
+    id => $id, secret => $secr, token => $csrftoken,
 });
 is( $result, 1, "CSRF token verified" );
 
 $result = $tokenizer->check({
-    CSRF => 1, id => $id, secret => $secr, token => $token,
+    type => 'CSRF', id => $id, secret => $secr, token => $token,
 });
 isnt( $result, 1, "This token is no CSRF token" );
-- 
1.7.10.4