From 9c8f3261b91f980c7e7399652adedcd0a667c76f Mon Sep 17 00:00:00 2001 From: Marcel de Rooy 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 --- 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