Bug 18124: [Follow-up] Handle default parameters in a sub

Adds a internal routine to handle default values for the parameters
id and secret.
Also adds a parameter session_id for generate_csrf and check_csrf. This
session parameter is combined with the id parameter when generating or
checking a token.

Test plan:
Run t/Token.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/Koha/Token.pm b/Koha/Token.pm
index c560fce..d0f7a54 100644 (file)
--- a/Koha/Token.pm
+++ b/Koha/Token.pm
@@ -34,9 +34,10 @@ Koha::Token - Tokenizer
         type => 'CSRF', id => $id, secret => $secret,
     });
 
-    # or check a CSRF token
+    # generate/check CSRF token with defaults and session id
+    my $csrf_token = $tokenizer->generate_csrf({ session_id => $x });
     my $result = $tokenizer->check_csrf({
-        id => $id, secret => $secret, token => $token,
+        session_id => $x, token => $token,
     });
 
 =head1 DESCRIPTION
@@ -74,7 +75,7 @@ sub new {
 
     my $token = $tokenizer->generate({ length => 20 });
     my $csrf_token = $tokenizer->generate({
-        type => 'CSRF', id => $id,
+        type => 'CSRF', id => $id, secret => $secret,
     });
 
     Generate several types of tokens. Now includes CSRF.
@@ -85,10 +86,7 @@ sub new {
 sub generate {
     my ( $self, $params ) = @_;
     if( $params->{type} && $params->{type} eq 'CSRF' ) {
-        $self->{lasttoken} = _gen_csrf( {
-            id     => Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} ),
-            secret => md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) ),
-        });
+        $self->{lasttoken} = _gen_csrf( $params );
     } else {
         $self->{lasttoken} = _gen_rand( $params );
     }
@@ -97,13 +95,16 @@ sub generate {
 
 =head2 generate_csrf
 
-    Shortcut for: generate({ type => 'CSRF', ... })
+    Like: generate({ type => 'CSRF', ... })
+    Note: id defaults to userid from context, secret to database password.
+    session_id is mandatory; it is combined with id.
 
 =cut
 
 sub generate_csrf {
     my ( $self, $params ) = @_;
-    return unless $params->{id};
+    return if !$params->{session_id};
+    $params = _add_default_csrf_params( $params );
     return $self->generate({ %$params, type => 'CSRF' });
 }
 
@@ -128,17 +129,35 @@ sub check {
 
 =head2 check_csrf
 
-    Shortcut for: check({ type => 'CSRF', ... })
+    Like: check({ type => 'CSRF', ... })
+    Note: id defaults to userid from context, secret to database password.
+    session_id is mandatory; it is combined with id.
 
 =cut
 
 sub check_csrf {
     my ( $self, $params ) = @_;
+    return if !$params->{session_id};
+    $params = _add_default_csrf_params( $params );
     return $self->check({ %$params, type => 'CSRF' });
 }
 
 # --- Internal routines ---
 
+sub _add_default_csrf_params {
+    my ( $params ) = @_;
+    $params->{session_id} //= '';
+    if( !$params->{id} ) {
+        $params->{id} = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{session_id} );
+    } else {
+        $params->{id} .= $params->{session_id};
+    }
+    $params->{id} //= Encode::encode( 'UTF-8', C4::Context->userenv->{id} );
+    my $pw = C4::Context->config('pass');
+    $params->{secret} //= md5_base64( Encode::encode( 'UTF-8', $pw ) ),
+    return $params;
+}
+
 sub _gen_csrf {
 
 # Since WWW::CSRF::generate_csrf_token does not use the NonBlocking
@@ -162,12 +181,12 @@ sub _gen_csrf {
 
 sub _chk_csrf {
     my ( $params ) = @_;
-    return if !$params->{id} || !$params->{token};
+    return if !$params->{id} || !$params->{secret} || !$params->{token};
 
-    my $id = Encode::encode( 'UTF-8', C4::Context->userenv->{id} . $params->{id} );
-    my $secret = md5_base64( Encode::encode( 'UTF-8', C4::Context->config('pass') ) );
     my $csrf_status = WWW::CSRF::check_csrf_token(
-        $id, $secret, $params->{token},
+        $params->{id},
+        $params->{secret},
+        $params->{token},
         { MaxAge => $params->{MaxAge} // ( CSRF_EXPIRY_HOURS * 3600 ) },
     );
     return $csrf_status == WWW::CSRF::CSRF_OK();

diff --git a/t/Token.t b/t/Token.t
index 3a971a5..2314d2e 100644 (file)
--- a/t/Token.t
+++ b/t/Token.t
@@ -33,13 +33,13 @@ is( length( $tokenizer->generate ), 1, "Generate without parameters" );
 my $token = $tokenizer->generate({ length => 20 });
 is( length($token), 20, "Token $token has 20 chars" );
 
-my $id = $tokenizer->generate({ lyyGength => 8 });
-my $csrftoken = $tokenizer->generate_csrf({ id => $id });
+my $id = $tokenizer->generate({ length => 8 });
+my $csrftoken = $tokenizer->generate_csrf({ session_id => $id });
 isnt( length($csrftoken), 0, "Token $csrftoken should not be empty" );
 
 is( $tokenizer->check, undef, "Check without any parameters" );
 my $result = $tokenizer->check_csrf({
-    id => $id, token => $csrftoken,
+    session_id => $id, token => $csrftoken,
 });
 is( $result, 1, "CSRF token verified" );
 
@@ -51,25 +51,25 @@ isnt( $result, 1, "This token is no CSRF token" );
 # Test MaxAge parameter
 my $age = 1; # 1 second
 $result = $tokenizer->check_csrf({
-    id => $id, token => $csrftoken, MaxAge => $age,
+    session_id => $id, token => $csrftoken, MaxAge => $age,
 });
 is( $result, 1, "CSRF token still valid within one second" );
 usleep $age * 1000000 * 2; # micro (millionth) seconds + 100%
 $result = $tokenizer->check_csrf({
-    id => $id, token => $csrftoken, MaxAge => $age,
+    session_id => $id, token => $csrftoken, MaxAge => $age,
 });
 isnt( $result, 1, "CSRF token expired after one second" );
 
 subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub {
     plan tests => 2;
-    $csrftoken = $tokenizer->generate_csrf({ id => $id });
+    $csrftoken = $tokenizer->generate_csrf({ session_id => $id });
     $result = $tokenizer->check_csrf({
-        id => $id, token => $csrftoken,
+        session_id => $id, token => $csrftoken,
     });
     is( $result, 1, "CSRF token verified" );
     C4::Context->set_userenv(0,43,0,'firstname','surname', 'CPL', 'Library 1', 0, ', ');
     $result = $tokenizer->check_csrf({
-        id => $id, token => $csrftoken,
+        session_id => $id, token => $csrftoken,
     });
     is( $result, '', "CSRF token is not verified if another logged in user is using the same id" );
 };
@@ -77,15 +77,15 @@ subtest 'Same id (cookie CGISESSID) with an other logged in user' => sub {
 subtest 'Same logged in user with another session (cookie CGISESSID)' => sub {
     plan tests => 2;
     C4::Context->set_userenv(0,42,0,'firstname','surname', 'CPL', 'Library 1', 0, ', ');
-    $csrftoken = $tokenizer->generate_csrf({ id => $id });
+    $csrftoken = $tokenizer->generate_csrf({ session_id => $id });
     $result = $tokenizer->check_csrf({
-        id => $id, token => $csrftoken,
+        session_id => $id, token => $csrftoken,
     });
     is( $result, 1, "CSRF token verified" );
     # Get another session id
-    $id = $tokenizer->generate({ lyyGength => 8 });
+    $id = $tokenizer->generate({ length => 8 });
     $result = $tokenizer->check_csrf({
-        id => $id, token => $csrftoken,
+        session_id => $id, token => $csrftoken,
     });
     is( $result, '', "CSRF token is not verified if another session is used" );
 };