From 7891256c516d0e5b3d50388582c5737d495575fd Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Thu, 13 Oct 2016 15:49:42 +0300 Subject: [PATCH] Bug 17006: (follow-up) Use Koha::Exceptions for change password endpoint Koha::Exceptions are a nice way to handle erros and they should be used much more than they are now. This patch adds Koha::Exceptions into Koha::Patron->change_password_to sub and catches the exceptions in the controller. 1. Apply patch 2. Run t/db_dependent/api/v1/patrons.t 3. Send PATCH request to http://library/api/v1/patrons/YYY/password where YYY is existing borrowernumber (borrowers flag required) 4. Make sure that password was changed. 5. Try also too short password, and wrong current password and observe that errors are displayed appropriately. curl -X PATCH http://library/api/v1/patrons/123/password \ --data '{"current_password":"123", "new_password":"1234"}' \ --cookie 'CGISESSID=055ca5a9a3ad3fb5052143bd015c1c10' Signed-off-by: Josef Moravec --- Koha/Exceptions.pm | 5 ++++ Koha/Exceptions/Password.pm | 24 +++++++++++++++ Koha/Patron.pm | 11 +++++-- Koha/REST/V1/Patron.pm | 73 ++++++++++++++++++++++++++++++++------------- 4 files changed, 89 insertions(+), 24 deletions(-) create mode 100644 Koha/Exceptions/Password.pm diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index 60b9e6a..7979489 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -8,6 +8,11 @@ use Exception::Class ( 'Koha::Exceptions::Exception' => { description => 'Something went wrong!', }, + 'Koha::Exceptions::BadSystemPreference' => { + isa => 'Koha::Exceptions::Exception', + description => 'System preference value is incomprehensible', + fields => ['preference'], + }, 'Koha::Exceptions::DuplicateObject' => { isa => 'Koha::Exceptions::Exception', description => 'Same object already exists', diff --git a/Koha/Exceptions/Password.pm b/Koha/Exceptions/Password.pm new file mode 100644 index 0000000..3fb5d04 --- /dev/null +++ b/Koha/Exceptions/Password.pm @@ -0,0 +1,24 @@ +package Koha::Exceptions::Password; + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Password' => { + description => 'Something went wrong!', + }, + 'Koha::Exceptions::Password::Invalid' => { + isa => 'Koha::Exceptions::Password', + description => 'Invalid password', + }, + 'Koha::Exceptions::Password::TooShort' => { + isa => 'Koha::Exceptions::Password', + description => 'Password is too short', + }, + 'Koha::Exceptions::Password::TrailingWhitespaces' => { + isa => 'Koha::Exceptions::Password', + description => 'Password contains trailing whitespace(s)', + } +); + +1; diff --git a/Koha/Patron.pm b/Koha/Patron.pm index d4ba052..17ca896 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -28,6 +28,7 @@ use Koha::AuthUtils; use Koha::Checkouts; use Koha::Database; use Koha::DateUtils; +use Koha::Exceptions::Password; use Koha::Holds; use Koha::Old::Checkouts; use Koha::Patron::Categories; @@ -670,10 +671,14 @@ sub change_password_to { my $min_length = C4::Context->preference("minPasswordLength"); if ($min_length > length($cleartext_password)) { - return (undef, "Password is too short. Minimum length: $min_length."); + Koha::Exceptions::Password::TooShort->throw( + "Password is too short. Minimum length: $min_length" + ); } - if ($cleartext_password =~ m|^\s+| or $cleartext_password =~ m|\s+$|) { - return (undef, "Password cannot contain trailing whitespaces."); + elsif ($cleartext_password =~ m|^\s+| or $cleartext_password =~ m|\s+$|) { + Koha::Exceptions::Password::TrailingWhitespaces->throw( + "Password cannot contain trailing whitespaces." + ); } my $hashed_password = Koha::AuthUtils::hash_password($cleartext_password); $self->set({ password => $hashed_password })->store; diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index dc32217..dff5208 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -21,8 +21,13 @@ use Mojo::Base 'Mojolicious::Controller'; use C4::Auth qw( haspermission checkpw_internal ); use C4::Context; +use Koha::Exceptions; +use Koha::Exceptions::Password; use Koha::Patrons; +use Scalar::Util qw(blessed); +use Try::Tiny; + sub list { my ($c, $args, $cb) = @_; @@ -49,29 +54,55 @@ sub get { sub changepassword { my ($c, $args, $cb) = @_; - my $user = $c->stash('koha.user'); - my $patron = Koha::Patrons->find($args->{borrowernumber}); - - my $OpacPasswordChange = C4::Context->preference("OpacPasswordChange"); - my $haspermission = haspermission($user->userid, {borrowers => 1}); - unless ($OpacPasswordChange && $user->borrowernumber == $args->{borrowernumber} - || $haspermission) { - return $c->$cb({ error => "OPAC password change is disabled" }, 403); + my $patron; + my $user; + try { + + $patron = Koha::Patrons->find($args->{borrowernumber}); + $user = $c->stash('koha.user'); + + my $OpacPasswordChange = C4::Context->preference("OpacPasswordChange"); + my $haspermission = haspermission($user->userid, {borrowers => 1}); + unless ($OpacPasswordChange && $user->borrowernumber == $args->{borrowernumber}) { + Koha::Exceptions::BadSystemPreference->throw( + preference => 'OpacPasswordChange' + ) unless $haspermission; + } + + my $pw = $args->{'body'}; + my $dbh = C4::Context->dbh; + unless ($haspermission || checkpw_internal($dbh, $patron->userid, $pw->{'current_password'})) { + Koha::Exceptions::Password::Invalid->throw; + } + $patron->change_password_to($pw->{'new_password'}); + return $c->$cb({}, 200); } - return $c->$cb({ error => "Patron not found." }, 404) unless $patron; - - my $pw = $args->{'body'}; - my $dbh = C4::Context->dbh; - unless ($haspermission - || checkpw_internal($dbh, $patron->userid, $pw->{'current_password'})) { - return $c->$cb({ error => "Wrong current password." }, 400); - } - - my ($success, $errmsg) = $patron->change_password_to($pw->{'new_password'}); - if ($errmsg) { - return $c->$cb({ error => $errmsg }, 400); + catch { + if (not defined $patron) { + return $c->$cb({ error => "Patron not found." }, 404); + } + elsif (not defined $user) { + return $c->$cb({ error => "User must be defined." }, 500); + } + + die $_ unless blessed $_ && $_->can('rethrow'); + if ($_->isa('Koha::Exceptions::Password::Invalid')) { + return $c->$cb({ error => "Wrong current password." }, 400); + } + elsif ($_->isa('Koha::Exceptions::Password::TooShort')) { + return $c->$cb({ error => $_->error }, 400); + } + elsif ($_->isa('Koha::Exceptions::Password::TrailingWhitespaces')) { + return $c->$cb({ error => $_->error }, 400); + } + elsif ($_->isa('Koha::Exceptions::BadSystemPreference') + && $_->preference eq 'OpacPasswordChange') { + return $c->$cb({ error => "OPAC password change is disabled" }, 403); + } + else { + return $c->$cb({ error => "Something went wrong. $_" }, 500); + } } - return $c->$cb({}, 200); } 1; -- 2.1.4