https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27947 --- Comment #41 from Tomás Cohen Arazi <tomascohen@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.