From 3b531d8701ef9737a69cf66f823dc2295380b743 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 9d7f397..e629195 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -13,6 +13,11 @@ use Exception::Class ( description => 'Bad parameter was given', fields => ["parameter"], }, + '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 3052300..410ede9 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; @@ -725,10 +726,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 5c849a8..73e16ef 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 = shift->openapi->valid_input or return; @@ -46,29 +51,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