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