From f13468ea5c5858f4454b1c52f89a3d06c6dd4020 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 and minify swagger.json perl misc/devel/minifySwagger.pl -s api/v1/swagger/swagger.json -d api/v1/swagger/swagger.min.json 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.json | 410 ++++++++++++++++++++++++++++++++++++++ api/v1/swagger/paths.json | 3 + api/v1/swagger/paths/patrons.json | 60 ++++++ t/db_dependent/api/v1/patrons.t | 66 +++++- 6 files changed, 582 insertions(+), 6 deletions(-) create mode 100644 api/v1/swagger.json 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.json b/api/v1/swagger.json new file mode 100644 index 0000000..4612275 --- /dev/null +++ b/api/v1/swagger.json @@ -0,0 +1,410 @@ +{ + "swagger": "2.0", + "info": { + "title": "Koha REST API", + "version": "1", + "license": { + "name": "GPL v3", + "url": "http://www.gnu.org/licenses/gpl.txt" + }, + "contact": { + "name": "Koha Team", + "url": "http://koha-community.org/" + } + }, + "basePath": "/api/v1", + "paths": { + "/patrons": { + "get": { + "operationId": "listPatrons", + "tags": ["patrons"], + "produces": [ + "application/json" + ], + "responses": { + "200": { + "description": "A list of patrons", + "schema": { + "type": "array", + "items": { + "$ref": "#/definitions/patron" + } + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "#/definitions/error" + } + } + } + } + }, + "/patrons/{borrowernumber}": { + "get": { + "operationId": "getPatron", + "tags": ["patrons"], + "parameters": [ + { + "$ref": "#/parameters/borrowernumberPathParam" + } + ], + "produces": [ + "application/json" + ], + "responses": { + "200": { + "description": "A patron", + "schema": { + "$ref": "#/definitions/patron" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "#/definitions/error" + } + }, + "404": { + "description": "Patron not found", + "schema": { + "$ref": "#/definitions/error" + } + } + } + } + }, + "/patrons/{borrowernumber}/password": { + "patch": { + "operationId": "changepasswordPatron", + "tags": ["patrons"], + "parameters": [ + { "$ref": "#/parameters/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/error" + } + }, + "403": { + "description": "Access forbidden", + "schema": { + "$ref": "#/definitions/error" + } + }, + "404": { + "description": "Patron not found", + "schema": { + "$ref": "#/definitions/error" + } + } + } + } + }, + "/holds": { + "get": { + "operationId": "listHolds", + "tags": ["borrowers", "holds"], + "parameters": [ + { + "name": "reserve_id", + "in": "query", + "description": "Internal reserve identifier", + "type": "integer" + }, + { + "name": "borrowernumber", + "in": "query", + "description": "Internal borrower identifier", + "type": "integer" + }, + { + "name": "reservedate", + "in": "query", + "description": "Reserve date", + "type": "string" + }, + { + "name": "biblionumber", + "in": "query", + "description": "Internal biblio identifier", + "type": "integer" + }, + { + "name": "branchcode", + "in": "query", + "description": "Branch code", + "type": "string" + }, + { + "name": "notificationdate", + "in": "query", + "description": "Notification date", + "type": "string" + }, + { + "name": "reminderdate", + "in": "query", + "description": "Reminder date", + "type": "string" + }, + { + "name": "cancellationdate", + "in": "query", + "description": "Cancellation date", + "type": "string" + }, + { + "name": "reservenotes", + "in": "query", + "description": "Reserve notes", + "type": "string" + }, + { + "name": "priority", + "in": "query", + "description": "Priority", + "type": "integer" + }, + { + "name": "found", + "in": "query", + "description": "Found status", + "type": "string" + }, + { + "name": "timestamp", + "in": "query", + "description": "Time of latest update", + "type": "string" + }, + { + "name": "itemnumber", + "in": "query", + "description": "Internal item identifier", + "type": "integer" + }, + { + "name": "waitingdate", + "in": "query", + "description": "Date the item was marked as waiting for the patron", + "type": "string" + }, + { + "name": "expirationdate", + "in": "query", + "description": "Date the hold expires", + "type": "string" + }, + { + "name": "lowestPriority", + "in": "query", + "description": "Lowest priority", + "type": "integer" + }, + { + "name": "suspend", + "in": "query", + "description": "Suspended", + "type": "integer" + }, + { + "name": "suspend_until", + "in": "query", + "description": "Suspended until", + "type": "string" + } + ], + "produces": ["application/json"], + "responses": { + "200": { + "description": "A list of holds", + "schema": { "$ref": "#/definitions/holds" } + }, + "404": { + "description": "Borrower not found", + "schema": { "$ref": "#/definitions/error" } + } + } + }, + "post": { + "operationId": "addHold", + "tags": ["borrowers", "holds"], + "parameters": [ + { + "name": "body", + "in": "body", + "description": "A JSON object containing informations about the new hold", + "required": true, + "schema": { + "type": "object", + "properties": { + "borrowernumber": { + "description": "Borrower internal identifier", + "type": "integer" + }, + "biblionumber": { + "description": "Biblio internal identifier", + "type": "integer" + }, + "itemnumber": { + "description": "Item internal identifier", + "type": "integer" + }, + "branchcode": { + "description": "Pickup location", + "type": "string" + }, + "expirationdate": { + "description": "Hold end date", + "type": "string", + "format": "date" + } + } + } + } + ], + "consumes": ["application/json"], + "produces": ["application/json"], + "responses": { + "201": { + "description": "Created hold", + "schema": { "$ref": "#/definitions/hold" } + }, + "400": { + "description": "Missing or wrong parameters", + "schema": { "$ref": "#/definitions/error" } + }, + "403": { + "description": "Hold not allowed", + "schema": { "$ref": "#/definitions/error" } + }, + "404": { + "description": "Borrower not found", + "schema": { "$ref": "#/definitions/error" } + }, + "500": { + "description": "Internal error", + "schema": { "$ref": "#/definitions/error" } + } + } + } + }, + "/holds/{reserve_id}": { + "put": { + "operationId": "editHold", + "tags": ["holds"], + "parameters": [ + { "$ref": "#/parameters/holdIdPathParam" }, + { + "name": "body", + "in": "body", + "description": "A JSON object containing fields to modify", + "required": true, + "schema": { + "type": "object", + "properties": { + "priority": { + "description": "Position in waiting queue", + "type": "integer", + "minimum": 1 + }, + "branchcode": { + "description": "Pickup location", + "type": "string" + }, + "suspend_until": { + "description": "Suspend until", + "type": "string", + "format": "date" + } + } + } + } + ], + "consumes": ["application/json"], + "produces": ["application/json"], + "responses": { + "200": { + "description": "Updated hold", + "schema": { "$ref": "#/definitions/hold" } + }, + "400": { + "description": "Missing or wrong parameters", + "schema": { "$ref": "#/definitions/error" } + }, + "404": { + "description": "Hold not found", + "schema": { "$ref": "#/definitions/error" } + } + } + }, + "delete": { + "operationId": "deleteHold", + "tags": ["holds"], + "parameters": [ + { "$ref": "#/parameters/holdIdPathParam" } + ], + "produces": ["application/json"], + "responses": { + "200": { + "description": "Successful deletion", + "schema": { + "type": "object" + } + }, + "404": { + "description": "Hold not found", + "schema": { "$ref": "#/definitions/error" } + } + } + } + } + }, + "definitions": { + "$ref": "./definitions/index.json" + }, + "parameters": { + "borrowernumberPathParam": { + "name": "borrowernumber", + "in": "path", + "description": "Internal patron identifier", + "required": true, + "type": "integer" + }, + "holdIdPathParam": { + "name": "reserve_id", + "in": "path", + "description": "Internal hold identifier", + "required": true, + "type": "integer" + } + } +} 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