Bugzilla – Attachment 54144 Details for
Bug 16929
Prevent opac-memberentry waiting for random chars
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 16929: [QA Follow-up] Shortcut methods and use statements
Bug-16929-QA-Follow-up-Shortcut-methods-and-use-st.patch (text/plain), 6.96 KB, created by
Marc Véron
on 2016-08-08 08:40:14 UTC
(
hide
)
Description:
Bug 16929: [QA Follow-up] Shortcut methods and use statements
Filename:
MIME Type:
Creator:
Marc Véron
Created:
2016-08-08 08:40:14 UTC
Size:
6.96 KB
patch
obsolete
>From 210472a6327faebac5b703b573ead7d061f0e870 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 > >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> > >Signed-off-by: Marc <veron@veron.ch> >--- > 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" ); >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 16929
:
53418
|
53487
|
53552
|
53809
|
54140
|
54141
|
54142
|
54143
|
54144
|
54145
|
54151
|
54152
|
54153
|
54154
|
54216