[Bug 17445] New: REST API: Generic handling of malformed query parameters
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Bug ID: 17445 Summary: REST API: Generic handling of malformed query parameters Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Web services Assignee: koha-bugs@lists.koha-community.org Reporter: martin.renvoize@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: claire_gravely@hotmail.com, jonathan.druart@bugs.koha-community.org, kyle@bywatersolutions.com, martin.renvoize@ptfs-europe.com, testopia@bugs.koha-community.org, tomascohen@gmail.com Depends on: 17425, 17428 The swagger plugin as is does not have the option to throw and error on badly formed query parameter keys (i.e extra or miss spelt keys). We should hook the around action to add this extra validation Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17425 [Bug 17425] Koha::Object should raise exceptions https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17428 [Bug 17428] REST api: CRUD endpoint for cities -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|17425, 17428 | Blocks| |17428 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17425 [Bug 17425] Koha::Object should raise exceptions https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17428 [Bug 17428] REST api: CRUD endpoint for cities -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #1 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 56487 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56487&action=edit BUG 17455: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56487|0 |1 is obsolete| | --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 56488 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56488&action=edit BUG 17455: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Tests coming in followup -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Lari Taskula <lari.taskula@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@jns.fi -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56488|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 56500 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56500&action=edit BUG 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 56546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56546&action=edit Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 56547 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56547&action=edit Bug 17445: Move the params check after the authentication check If the user is not authorised to call this route, we would prefer to raise a 403 instead of 400 Note that we wanted to submit tests for this change but the city code does not let use do that (we are allowed to list/show cities even without any permissions). The patrons.t is not complete enought and the holds.t tests do not pass... Tomas plans to submit tests but we reach the end of the hackfest ;) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #5)
Created attachment 56546 [details] [review] Bug 17445: Just some refactoring
The idea is to make the code more readable moving the code to its own subroutine
Totally agree with this.. I was intending on doing this one myself but wanted to get the quick and dirty patch up here as quick as possible to get feedback on the general method :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #6)
Created attachment 56547 [details] [review] Bug 17445: Move the params check after the authentication check
If the user is not authorised to call this route, we would prefer to raise a 403 instead of 400
Note that we wanted to submit tests for this change but the city code does not let use do that (we are allowed to list/show cities even without any permissions). The patrons.t is not complete enought and the holds.t tests do not pass...
Tomas plans to submit tests but we reach the end of the hackfest ;)
Also agree with this change.. it didn't even occur to me to think about error code presidency in this case.. Generically I think you tend to just work backwards down the error codes, so your checking for a 403 failure before a more generic 400 is perfect in this case. Good spot! :) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #9 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 56634 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56634&action=edit [SIGNED-OFF] BUG 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body https://bugs.koha-community.org/show_bug.cgi?id=17445 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56500|0 |1 is obsolete| | Attachment #56546|0 |1 is obsolete| | Attachment #56547|0 |1 is obsolete| | --- Comment #10 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 56635 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56635&action=edit [SIGNED-OFF] Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 --- Comment #11 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 56636 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56636&action=edit [SIGNED-OFF] Bug 17445: Move the params check after the authentication check If the user is not authorised to call this route, we would prefer to raise a 403 instead of 400 Note that we wanted to submit tests for this change but the city code does not let use do that (we are allowed to list/show cities even without any permissions). The patrons.t is not complete enought and the holds.t tests do not pass... Tomas plans to submit tests but we reach the end of the hackfest ;) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56634|0 |1 is obsolete| | --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 56659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56659&action=edit Bug 17445: Add 'malformed query' error response This patch adds to the x-mojo-around action code to give a meaningful error given a bad query parameter in the query string for a request. Test Plan 1) Submit an api request to an existing restful endpoint with no query parameters 2) Confirm the correct response is being given 3) Submit an api request to an existing restful endpoint with an allowed query parameter 4) Confimr the correct response is being given 5) Submit an api request to an existing restful endpoint with a malformed query paramter 6) Note the 400 response code and helpful json api body https://bugs.koha-community.org/show_bug.cgi?id=17445 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56635|0 |1 is obsolete| | --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 56660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56660&action=edit Bug 17445: Just some refactoring The idea is to make the code more readable moving the code to its own subroutine Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56636|0 |1 is obsolete| | --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 56661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56661&action=edit Bug 17445: Move the params check after the authentication check If the user is not authorised to call this route, we would prefer to raise a 403 instead of 400 Note that we wanted to submit tests for this change but the city code does not let use do that (we are allowed to list/show cities even without any permissions). The patrons.t is not complete enought and the holds.t tests do not pass... Tomas plans to submit tests but we reach the end of the hackfest ;) Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17445 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jonathan, Martin! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org