https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27947
--- Comment #41 from Tomás Cohen Arazi <[email protected]> --- (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. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
