From 2029909b013f8a5063e390913d0f39782a5cc608 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 17 May 2023 16:54:45 +0100 Subject: [PATCH] Bug 23336: (QA follow-up) Minor fixes This patch deals with some QA script warnings, and also makes some changes in line with bug 33556. We also adds current user id and checkout item id into the confirmation token to improve security and fix the failing tests. Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize --- Koha/REST/V1/Checkouts.pm | 22 +++++++++++---------- t/db_dependent/api/v1/checkouts.t | 32 +++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/Koha/REST/V1/Checkouts.pm b/Koha/REST/V1/Checkouts.pm index a657c8ac23..2fd756e3ae 100644 --- a/Koha/REST/V1/Checkouts.pm +++ b/Koha/REST/V1/Checkouts.pm @@ -110,11 +110,12 @@ Controller function that handles retrieval of Checkout availability sub get_availability { my $c = shift->openapi->valid_input or return; + my $user = $c->stash('koha.user'); - my $patron = Koha::Patrons->find( $c->validation->param('patron_id') ); + my $patron = Koha::Patrons->find( $c->param('patron_id') ); my $inprocess = 0; # What does this do? my $ignore_reserves = 0; # Don't ignore reserves - my $item = Koha::Items->find( $c->validation->param('item_id') ); + my $item = Koha::Items->find( $c->param('item_id') ); my $params = { item => $item }; @@ -123,9 +124,9 @@ sub get_availability { C4::Circulation::CanBookBeIssued( $patron, undef, undef, $inprocess, $ignore_reserves, $params ); - my $confirm_keys = join( /:/, sort keys %{$confirmation} ); - my $tokenizer = Koha::Token->new; - my $token = $tokeniser->generate_jwt({ id => $confirm_keys }); + my $confirm_keys = join( ":", sort keys %{$confirmation} ); + $confirm_keys = $user->id . ":" . $item->id . ":" . $confirm_keys; + my $token = Koha::Token->new->generate_jwt( { id => $confirm_keys } ); my $response = { blockers => $impossible, @@ -145,8 +146,9 @@ Add a new checkout sub add { my $c = shift->openapi->valid_input or return; + my $user = $c->stash('koha.user'); - my $body = $c->validation->param('body'); + my $body = $c->req->json; my $item_id = $body->{item_id}; my $patron_id = $body->{patron_id}; my $onsite = $body->{onsite_checkout}; @@ -205,10 +207,10 @@ sub add { # Check for existance of confirmation token # and if exists check validity - if ( my $token = $c->validation->param('confirmation') ) { - my $confirm_keys = join( /:/, sort keys %{$confirmation} ); - my $tokenizer = Koha::Token->new; - $confirmed = $tokenizer->check_jwt( + if ( my $token = $c->param('confirmation') ) { + my $confirm_keys = join( ":", sort keys %{$confirmation} ); + $confirm_keys = $user->id . ":" . $item->id . ":" . $confirm_keys; + $confirmed = Koha::Token->new->check_jwt( { id => $confirm_keys, token => $token } ); } diff --git a/t/db_dependent/api/v1/checkouts.t b/t/db_dependent/api/v1/checkouts.t index 89ebb5862b..2e749aa874 100755 --- a/t/db_dependent/api/v1/checkouts.t +++ b/t/db_dependent/api/v1/checkouts.t @@ -30,6 +30,7 @@ use C4::Circulation qw( AddIssue AddReturn CanBookBeIssued ); use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); +use Koha::Token; my $schema = Koha::Database->schema; my $builder = t::lib::TestBuilder->new; @@ -237,7 +238,7 @@ $schema->storage->txn_rollback; subtest 'get_availability' => sub { - plan tests => 27; + plan tests => 28; $schema->storage->txn_begin; my $librarian = $builder->build_object( @@ -294,7 +295,7 @@ subtest 'get_availability' => sub { "//$userid:$password@/api/v1/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" )->status_is(200)->json_is( '/blockers' => {} ) ->json_is( '/confirms' => {} )->json_is( '/warnings' => {} ) - ->json_is( '/confirmation_token' => undef ); + ->json_has( '/confirmation_token'); # Blocked %issuingimpossible = ( GNA => 1 ); @@ -302,7 +303,7 @@ subtest 'get_availability' => sub { "//$userid:$password@/api/v1/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" )->status_is(200)->json_is( '/blockers' => { GNA => 1 } ) ->json_is( '/confirms' => {} )->json_is( '/warnings' => {} ) - ->json_is( '/confirmation_token' => undef ); + ->json_has( '/confirmation_token'); %issuingimpossible = (); # Warnings/Info @@ -314,18 +315,31 @@ subtest 'get_availability' => sub { ->json_is( '/confirms' => {} ) ->json_is( '/warnings' => { alert1 => "this is an alert", message1 => "this is a message" } ) - ->json_is( '/confirmation_token' => undef ); + ->json_has( '/confirmation_token'); %alerts = (); %messages = (); # Needs confirm %needsconfirmation = ( confirm1 => 1, confirm2 => 'please' ); + my $token = Koha::Token->new->generate_jwt( { id => $librarian->id . ":" . $item1_id . ":confirm1:confirm2:please" }); $t->get_ok( "//$userid:$password@/api/v1/checkouts/availability?item_id=$item1_id&patron_id=$patron_id" )->status_is(200)->json_is( '/blockers' => {} ) ->json_is( '/confirms' => { confirm1 => 1, confirm2 => 'please' } ) ->json_is( '/warnings' => {} ) - ->json_is( '/confirmation_token' => 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjb25maXJtMSI6MSwiY29uZmlybTIiOjF9.4QBpITwnIGOAfohyKjaFDoeBWnGmQTdyJrPn9pavArw' ); + ->json_has( '/confirmation_token'); + my $confirmation_token = $t->tx->res->json('/confirmation_token'); + ok( + Koha::Token->new->check_jwt( + { + id => $librarian->id . ":" + . $item1_id + . ":confirm1:confirm2:please", + token => $confirmation_token + } + ), + 'Correct token' + ); $schema->storage->txn_rollback; }; @@ -397,7 +411,13 @@ subtest 'add checkout' => sub { } )->status_is(412); - my $token = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjb25maXJtMSI6MSwiY29uZmlybTIiOjF9.4QBpITwnIGOAfohyKjaFDoeBWnGmQTdyJrPn9pavArw"; + my $token = Koha::Token->new->generate_jwt( + { + id => $librarian->id . ":" + . $item1_id + . ":confirm1:confirm2:please" + } + ); $t->post_ok( "//$userid:$password@/api/v1/checkouts?confirmation=$token" => json => { item_id => $item1_id, -- 2.40.1