From 4b52f2f8d7d272b0d6c140a85ae80ffc9617e818 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Thu, 18 Aug 2016 18:33:56 +0300 Subject: [PATCH] Bug 17006: Add API route for changing patron's password PATCH /patrons/{borrowernumber}/password (change password) Required body params: - current_password - new_password To test: 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. You may find this useful for testing: curl -X PATCH http://library/api/v1/patrons/123/password --data '{"current_password":"123456", "new_password":"1234"}' --cookie 'CGISESSID=055ca5a9a3ad3fb5052143bd015c1c10' Signed-off-by: Aleisha Amohia --- Koha/Patron.pm | 27 ++++++++++++++++ Koha/REST/V1/Patron.pm | 22 +++++++++++++ api/v1/swagger/paths.json | 3 ++ api/v1/swagger/paths/patrons.json | 60 +++++++++++++++++++++++++++++++++++ t/db_dependent/api/v1/patrons.t | 66 +++++++++++++++++++++++++++++++++++---- 5 files changed, 172 insertions(+), 6 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index b5449a3..e557ab1 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -24,6 +24,7 @@ use Carp; use C4::Context; use C4::Log; +use Koha::AuthUtils; use Koha::Database; use Koha::DateUtils; use Koha::Issues; @@ -267,6 +268,32 @@ sub track_login { $self->lastseen( dt_from_string() )->store; } +=head2 change_password_to + +my $changed = $patron->change_password_to($cleartext_password); + +Changes patron's password to C<$cleartext_password>. This subroutine +also makes validations for new password, but does not check the old +one. + +=cut + +sub change_password_to { + my ($self, $cleartext_password) = @_; + + my $min_length = C4::Context->preference("minPasswordLength"); + if ($min_length > length($cleartext_password)) { + return (undef, "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."); + } + my $hashed_password = Koha::AuthUtils::hash_password($cleartext_password); + $self->set({ password => $hashed_password })->store; + logaction( "MEMBERS", "CHANGE PASS", $self->borrowernumber, "" ) if C4::Context->preference("BorrowersLog"); + return 1; +} + =head3 type =cut diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index b97a154..0bee309 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -19,6 +19,8 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; + +use C4::Auth qw( haspermission checkpw_internal ); use Koha::Patrons; sub list { @@ -44,4 +46,24 @@ sub get { return $c->$cb($patron->unblessed, 200); } +sub changepassword { + my ($c, $args, $cb) = @_; + + my $user = $c->stash('koha.user'); + my $patron = Koha::Patrons->find($args->{borrowernumber}); + return $c->$cb({ error => "Patron not found." }, 404) unless $patron; + + my $pw = $args->{'body'}; + my $dbh = C4::Context->dbh; + unless (checkpw_internal($dbh, $user->userid, $pw->{'current_password'})) { + return $c->$cb({ error => "Wrong current password." }, 400); + } + + my ($success, $errmsg) = $user->change_password_to($pw->{'new_password'}); + if ($errmsg) { + return $c->$cb({ error => $errmsg }, 400); + } + return $c->$cb({}, 200); +} + 1; diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index f00b1b4..331bc99 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -10,5 +10,8 @@ }, "/patrons/{borrowernumber}": { "$ref": "paths/patrons.json#/~1patrons~1{borrowernumber}" + }, + "/patrons/{borrowernumber}/password": { + "$ref": "paths/patrons.json#/~1patrons~1{borrowernumber}~1password" } } diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 67632e1..2f57115 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -69,5 +69,65 @@ } } } + }, + "/patrons/{borrowernumber}/password": { + "patch": { + "operationId": "changepasswordPatron", + "tags": ["patrons"], + "parameters": [{ + "$ref": "../parameters.json#/borrowernumberPathParam" + }, { + "name": "body", + "in": "body", + "description": "A JSON object containing informations about passwords", + "required": true, + "schema": { + "type": "object", + "properties": { + "current_password": { + "description": "Current password", + "type": "string" + }, + "new_password": { + "description": "New password", + "type": "string" + } + } + } + } + ], + "produces": [ + "application/json" + ], + "responses": { + "200": { + "description": "Password changed" + }, + "400": { + "description": "Bad request", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "../definitions.json#/error" + } + }, + "404": { + "description": "Patron not found", + "schema": { + "$ref": "../definitions.json#/error" + } + } + }, + "x-koha-authorization": { + "allow-owner": true, + "permissions": { + "borrowers": "1" + } + } + } } } diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index f4b9410..2f48cd1 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -17,27 +17,29 @@ use Modern::Perl; -use Test::More tests => 20; +use Test::More tests => 38; use Test::Mojo; use t::lib::TestBuilder; +use t::lib::Mocks; use C4::Auth; use C4::Context; +use Koha::AuthUtils; use Koha::Database; use Koha::Patron; my $builder = t::lib::TestBuilder->new(); -my $dbh = C4::Context->dbh; -$dbh->{AutoCommit} = 0; -$dbh->{RaiseError} = 1; +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; $ENV{REMOTE_ADDR} = '127.0.0.1'; my $t = Test::Mojo->new('Koha::REST::V1'); my $categorycode = $builder->build({ source => 'Category' })->{ categorycode }; my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; + my $guarantor = $builder->build({ source => 'Borrower', value => { @@ -46,6 +48,8 @@ my $guarantor = $builder->build({ flags => 0, } }); + +my $password = "secret"; my $borrower = $builder->build({ source => 'Borrower', value => { @@ -81,6 +85,7 @@ $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(403); + $tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . ($borrower->{ borrowernumber }-1)); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) @@ -100,12 +105,32 @@ $t->request_ok($tx) ->status_is(200) ->json_is('/guarantorid', $guarantor->{borrowernumber}); +my $password_obj = { + current_password => $password, + new_password => "new password", +}; + +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/-100/password' => json => $password_obj); +$t->request_ok($tx) + ->status_is(401); + +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/'.$borrower->{borrowernumber}.'/password'); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(400); + +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/'.$guarantor->{borrowernumber}.'/password' => json => $password_obj); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(403); + my $loggedinuser = $builder->build({ source => 'Borrower', value => { branchcode => $branchcode, categorycode => $categorycode, - flags => 16 # borrowers flag + flags => 16, # borrowers flag + password => Koha::AuthUtils::hash_password($password), } }); @@ -129,4 +154,33 @@ $t->request_ok($tx) ->json_is('/borrowernumber' => $borrower->{ borrowernumber }) ->json_is('/surname' => $borrower->{ surname }); -$dbh->rollback; +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/-100/password' => json => $password_obj); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(404); + +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/'.$loggedinuser->{borrowernumber}.'/password' => json => $password_obj); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(200); + +ok(C4::Auth::checkpw_hash($password_obj->{'new_password'}, Koha::Patrons->find($loggedinuser->{borrowernumber})->password), "New password in database."); +is(C4::Auth::checkpw_hash($password_obj->{'current_password'}, Koha::Patrons->find($loggedinuser->{borrowernumber})->password), "", "Old password is gone."); + +$password_obj->{'current_password'} = $password_obj->{'new_password'}; +$password_obj->{'new_password'} = "a"; +t::lib::Mocks::mock_preference("minPasswordLength", 5); +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/'.$loggedinuser->{borrowernumber}.'/password' => json => $password_obj); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(400) + ->json_like('/error', qr/Password is too short/, "Password too short"); + +$password_obj->{'new_password'} = " abcdef "; +$tx = $t->ua->build_tx(PATCH => '/api/v1/patrons/'.$loggedinuser->{borrowernumber}.'/password' => json => $password_obj); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); +$t->request_ok($tx) + ->status_is(400) + ->json_is('/error', "Password cannot contain trailing whitespaces."); + +$schema->storage->txn_rollback; -- 1.9.1