Bug 24862 - Wrong behaviour on anonymous sessions
Summary: Wrong behaviour on anonymous sessions
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 25045
  Show dependency treegraph
 
Reported: 2020-03-12 22:29 UTC by David Cook
Modified: 2020-06-30 19:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07, 19.05.13


Attachments
Bug 24862: Regression tests (2.19 KB, patch)
2020-03-13 15:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24862: Handle annonymous sessions gracefuly (2.97 KB, patch)
2020-03-13 15:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24862: Regression tests (2.24 KB, patch)
2020-03-14 08:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 24862: Handle annonymous sessions gracefuly (3.02 KB, patch)
2020-03-14 08:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 24862: Regression tests (2.24 KB, patch)
2020-03-16 11:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24862: Handle annonymous sessions gracefuly (3.08 KB, patch)
2020-03-16 11:22 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-03-12 22:29:00 UTC
It looks like the REST API might not be handling cookies correctly.

To test:
0. Use koha-testing-docker
1. Login to http://localhost:8080/
2. Go to http://localhost:8080/api/v1/libraries
3. Observe a 200 OK HTTP code and JSON output
4. Logout from http://localhost:8080/
5. Go to http://localhost:8080/api/v1/libraries
6. Observe a 500 Internal Server Error and the following JSON output:
{"error":"Something went wrong, check the logs."}

It seems to me that really that should be returning a 401 and the following JSON output:
{"error":"Authentication failure."}
Comment 1 Tomás Cohen Arazi 2020-03-13 13:18:08 UTC
Good catch, David. Thanks for reporting it.
Comment 2 Tomás Cohen Arazi 2020-03-13 15:13:57 UTC
Created attachment 100683 [details] [review]
Bug 24862: Regression tests

This patch introduces tests for the expected behaviour on API routes
that expect a logged in user, but the request is made with an anonymous
session cookie.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail because the situation is not handled correctly in
the code

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-03-13 15:14:00 UTC
Created attachment 100684 [details] [review]
Bug 24862: Handle annonymous sessions gracefuly

This patch introduces code to detect (cookie) annonymous sessions and
act as expected.

Right now, as check_cookie_auth is not passed the required permissions
(because there aren't always required permissions, and the code to check
permissions is shared with other authentication mechanisms) it returns
'ok' and the session id. This use case was overlooked when this was
coded, and yeilds unexpected error codes (500) when the user logs out
and the annonymous session cookie is used to hit the API. The end result
doesn't pose any security issue (i.e. the resource access is rejected)
but the returned error code is not correct and should be fixed.

This patch verifies for an anonymous session (and avoids querying the
corresponding patron) and then verifies if there is an authorization
config on the route and if the patron object is defined.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail, 500 instead of the expected 401
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Repeat the original 'steps to reproduce' from the bug report using
   the browser
=> SUCCESS: Problem solved!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Sponsored-by: ByWater Solutions
Comment 4 David Nind 2020-03-14 08:47:11 UTC
Created attachment 100692 [details] [review]
Bug 24862: Regression tests

This patch introduces tests for the expected behaviour on API routes
that expect a logged in user, but the request is made with an anonymous
session cookie.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail because the situation is not handled correctly in
the code

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2020-03-14 08:47:14 UTC
Created attachment 100693 [details] [review]
Bug 24862: Handle annonymous sessions gracefuly

This patch introduces code to detect (cookie) annonymous sessions and
act as expected.

Right now, as check_cookie_auth is not passed the required permissions
(because there aren't always required permissions, and the code to check
permissions is shared with other authentication mechanisms) it returns
'ok' and the session id. This use case was overlooked when this was
coded, and yeilds unexpected error codes (500) when the user logs out
and the annonymous session cookie is used to hit the API. The end result
doesn't pose any security issue (i.e. the resource access is rejected)
but the returned error code is not correct and should be fixed.

This patch verifies for an anonymous session (and avoids querying the
corresponding patron) and then verifies if there is an authorization
config on the route and if the patron object is defined.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail, 500 instead of the expected 401
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Repeat the original 'steps to reproduce' from the bug report using
   the browser
=> SUCCESS: Problem solved!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Nick Clemens 2020-03-16 11:22:38 UTC
Created attachment 100734 [details] [review]
Bug 24862: Regression tests

This patch introduces tests for the expected behaviour on API routes
that expect a logged in user, but the request is made with an anonymous
session cookie.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail because the situation is not handled correctly in
the code

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Nick Clemens 2020-03-16 11:22:41 UTC
Created attachment 100735 [details] [review]
Bug 24862: Handle annonymous sessions gracefuly

This patch introduces code to detect (cookie) annonymous sessions and
act as expected.

Right now, as check_cookie_auth is not passed the required permissions
(because there aren't always required permissions, and the code to check
permissions is shared with other authentication mechanisms) it returns
'ok' and the session id. This use case was overlooked when this was
coded, and yeilds unexpected error codes (500) when the user logs out
and the annonymous session cookie is used to hit the API. The end result
doesn't pose any security issue (i.e. the resource access is rejected)
but the returned error code is not correct and should be fixed.

This patch verifies for an anonymous session (and avoids querying the
corresponding patron) and then verifies if there is an authorization
config on the route and if the patron object is defined.

To test:
1. Apply the tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail, 500 instead of the expected 401
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Repeat the original 'steps to reproduce' from the bug report using
   the browser
=> SUCCESS: Problem solved!
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Martin Renvoize 2020-03-16 14:56:07 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Aleisha Amohia 2020-06-21 22:04:01 UTC
backported to 19.11.x for 19.11.07
Comment 10 Victor Grousset/tuxayo 2020-06-30 19:23:20 UTC
Backported to 19.05.x branch for 19.05.13