From 821779af0809110e8d5be4792db01ef0a9b9f926 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 | 17 ++++++++++++ Koha/Patron.pm | 11 +++++--- Koha/REST/V1/Patron.pm | 71 +++++++++++++++++++++++++++++++++++--------------- 3 files changed, 75 insertions(+), 24 deletions(-) diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm index dd8647b..234a421 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', @@ -21,6 +26,18 @@ use Exception::Class ( isa => 'Koha::Exceptions::Exception', description => 'A required parameter is missing' }, + 'Koha::Exceptions::Password::Invalid' => { + isa => 'Koha::Exceptions::Exception', + description => 'Invalid password', + }, + 'Koha::Exceptions::Password::TooShort' => { + isa => 'Koha::Exceptions::Exception', + description => 'Password is too short', + }, + 'Koha::Exceptions::Password::TrailingWhitespaces' => { + isa => 'Koha::Exceptions::Exception', + description => 'Password contains trailing whitespace(s)', + }, # Virtualshelves exceptions 'Koha::Exceptions::Virtualshelves::DuplicateObject' => { isa => 'Koha::Exceptions::DuplicateObject', diff --git a/Koha/Patron.pm b/Koha/Patron.pm index e557ab1..454af5a 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; 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..a374d29 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -21,8 +21,12 @@ use Mojo::Base 'Mojolicious::Controller'; use C4::Auth qw( haspermission checkpw_internal ); use C4::Context; +use Koha::Exceptions; use Koha::Patrons; +use Scalar::Util qw(blessed); +use Try::Tiny; + sub list { my ($c, $args, $cb) = @_; @@ -49,29 +53,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