[Koha-bugs] [Bug 27947] Add default cancellation reasons to article requests

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27947

--- Comment #41 from Tomás Cohen Arazi <tomascohen at gmail.com> ---
(In reply to Martin Renvoize from comment #39)
> Sorry guys.. little more needed here.
> 
> My first followup drops the 'reserveforothers' permission requirement as I
> don't think that relates to this functionality.. but it makes the API tests
> fail.. and I can't see why.. code blind on a Friday.

The permissions were wrong.

You just missed the fact that privileged routes require at least one permission
(catalogue: 1 being the bare minimum). I think we never fixed this:
https://gitlab.com/koha-community/qa-test-tools/-/issues/11

> My second followup highlights an issue with the public route.  Although
> moving the route under /public/patrons/{patron_id} ensure we do a patron
> identity check.. there isn't actually a later check anywhere that the
> article your trying to delete actually belongs to the patron ;)

There's an implicit check by doing:

   $article_requests = $patron->article_requests->find( $article_request_id );

I clarified this with a comment in the code on the latest patch. I agree with
returning 404 there, as 403 would 'leak' the fact that the ID exists... Not
sure how important it is, but I think it is correctly tested to avoid security
issues.

> This final one is actually why I preferred the original
> /article_requests/{request_id} approach.. though of course that would
> require the addition of a routine to handle checking borrowernumber in the
> article request against the user as per the other routines for checking
> allow-owner.

allow-owner falls short. It feels like with routes with multiple ids in the
path we need to think a bit more [1]. I think object 'ownership' validation
should be baked into the objects in a declarative way. But that's for another
day/bug.

[1] I tried adding a validator for article requests in Koha::REST::V1::Auth,
but it failed randomly depending on which 'key' was picked first (patron_id vs.
article_request_id). It feels like a big refactoring is needed there. Or just
keep the patron_id from the path and leave the rest to  the controllers, as in
this bug.

-- 
You are receiving this mail because:
You are watching all bug changes.