Bug 17445

Summary: REST API: Generic handling of malformed query parameters
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Web servicesAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: claire_gravely, jonathan.druart, josef.moravec, kyle, lari.taskula, martin.renvoize, testopia, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17428    
Attachments: BUG 17455: Add 'malformed query' error response
BUG 17455: Add 'malformed query' error response
BUG 17445: Add 'malformed query' error response
Bug 17445: Just some refactoring
Bug 17445: Move the params check after the authentication check
[SIGNED-OFF] BUG 17445: Add 'malformed query' error response
[SIGNED-OFF] Bug 17445: Just some refactoring
[SIGNED-OFF] Bug 17445: Move the params check after the authentication check
Bug 17445: Add 'malformed query' error response
Bug 17445: Just some refactoring
Bug 17445: Move the params check after the authentication check

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!