From 25e0fc86c069440253e65b7a56355adec5d4b8de 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' --- Koha/Exceptions.pm | 5 ++++ Koha/Exceptions/Password.pm | 24 +++++++++++++++ Koha/Patron.pm | 11 +++++-- Koha/REST/V1/Patron.pm | 72 ++++++++++++++++++++++++++++++++------------- 4 files changed, 88 insertions(+), 24 deletions(-) create mode 100644 Koha/Exceptions/Password.pm diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index dd8647b..7385874 100644 --- a/Koha/Exceptions.pm +++ b/Koha/Exceptions.pm @@ -9,6 +9,11 @@ use Exception::Class ( 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 e557ab1..5337db2 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -27,6 +27,7 @@ use C4::Log; use Koha::AuthUtils; use Koha::Database; use Koha::DateUtils; +use Koha::Exceptions::Password; use Koha::Issues; use Koha::OldIssues; use Koha::Patron::Categories; @@ -283,10 +284,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 2f5c0a9..c5d6127 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,54 @@ 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; -- 1.9.1