Bug 17445 - REST API: Generic handling of malformed query parameters
Summary: REST API: Generic handling of malformed query parameters
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17428
  Show dependency treegraph
 
Reported: 2016-10-14 08:06 UTC by Martin Renvoize
Modified: 2017-06-14 22:16 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:


Attachments
BUG 17455: Add 'malformed query' error response (1.37 KB, patch)
2016-10-14 08:59 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUG 17455: Add 'malformed query' error response (1.78 KB, patch)
2016-10-14 09:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUG 17445: Add 'malformed query' error response (1.78 KB, patch)
2016-10-14 10:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17445: Just some refactoring (2.70 KB, patch)
2016-10-14 14:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17445: Move the params check after the authentication check (2.49 KB, patch)
2016-10-14 14:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] BUG 17445: Add 'malformed query' error response (1.84 KB, patch)
2016-10-18 21:19 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17445: Just some refactoring (2.77 KB, patch)
2016-10-18 21:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17445: Move the params check after the authentication check (2.55 KB, patch)
2016-10-18 21:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17445: Add 'malformed query' error response (1.89 KB, patch)
2016-10-19 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17445: Just some refactoring (2.81 KB, patch)
2016-10-19 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17445: Move the params check after the authentication check (2.60 KB, patch)
2016-10-19 13:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2016-10-14 08:06:01 UTC
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
Comment 1 Martin Renvoize 2016-10-14 08:59:35 UTC
Created attachment 56487 [details] [review]
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.
Comment 2 Martin Renvoize 2016-10-14 09:05:46 UTC
Created attachment 56488 [details] [review]
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
Comment 3 Martin Renvoize 2016-10-14 09:06:19 UTC
Tests coming in followup
Comment 4 Martin Renvoize 2016-10-14 10:04:59 UTC
Created attachment 56500 [details] [review]
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
Comment 5 Jonathan Druart 2016-10-14 14:23:33 UTC
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
Comment 6 Jonathan Druart 2016-10-14 14:23:36 UTC
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 ;)
Comment 7 Martin Renvoize 2016-10-14 14:30:44 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 56546 [details] [review] [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 :)
Comment 8 Martin Renvoize 2016-10-14 14:34:53 UTC
(In reply to Jonathan Druart from comment #6)
> Created attachment 56547 [details] [review] [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! :)
Comment 9 Josef Moravec 2016-10-18 21:19:49 UTC
Created attachment 56634 [details] [review]
[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>
Comment 10 Josef Moravec 2016-10-18 21:20:07 UTC
Created attachment 56635 [details] [review]
[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>
Comment 11 Josef Moravec 2016-10-18 21:20:15 UTC
Created attachment 56636 [details] [review]
[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>
Comment 12 Tomás Cohen Arazi 2016-10-19 13:20:06 UTC
Created attachment 56659 [details] [review]
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>
Comment 13 Tomás Cohen Arazi 2016-10-19 13:20:17 UTC
Created attachment 56660 [details] [review]
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>
Comment 14 Tomás Cohen Arazi 2016-10-19 13:20:26 UTC
Created attachment 56661 [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 ;)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Kyle M Hall 2016-10-21 17:15:46 UTC
Pushed to master for 16.11, thanks Jonathan, Martin!