Bug 22071 - authenticate_api_request does not stash koha.user in the OAuth use case
Summary: authenticate_api_request does not stash koha.user in the OAuth use case
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 20568
Blocks: 21198 22132
  Show dependency treegraph
 
Reported: 2019-01-04 14:46 UTC by Tomás Cohen Arazi
Modified: 2020-01-06 20:15 UTC (History)
5 users (show)

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


Attachments
Bug 22071: Regression tests (5.66 KB, patch)
2019-01-04 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case (1.24 KB, patch)
2019-01-04 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: Regression tests (5.63 KB, patch)
2019-01-04 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case (1.24 KB, patch)
2019-01-04 15:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: Regression tests (5.69 KB, patch)
2019-01-04 19:12 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case (1.29 KB, patch)
2019-01-04 19:12 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22071: (follow-up) Simplify code (5.97 KB, patch)
2019-01-07 10:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: (follow-up) Add POD for validate_query_parameters (747 bytes, patch)
2019-01-07 10:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22071: Regression tests (5.76 KB, patch)
2019-01-07 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case (1.37 KB, patch)
2019-01-07 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22071: (follow-up) Simplify code (6.04 KB, patch)
2019-01-07 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22071: (follow-up) Add POD for validate_query_parameters (812 bytes, patch)
2019-01-07 14:16 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2019-01-04 14:46:41 UTC
The patron that made the request is not being stashed in OAuth authentication. Regression tests and fixes to be submitted.
Comment 1 Tomás Cohen Arazi 2019-01-04 15:50:29 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2019-01-04 15:50:32 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2019-01-04 15:58:20 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2019-01-04 15:58:23 UTC Comment hidden (obsolete)
Comment 5 Charles Farmer 2019-01-04 19:10:38 UTC
I was able to get the same behavior as described in the test plan: "not ok 1 - token-based tests" but "ok 2 - cookie-based tests"

The following patch made all tests successful.

Had to download Net::OAuth2::AuthorizationServer and activate the syspref RESTOAuth2ClientCredentials, if anybody needs additional instructions.
Comment 6 Charles Farmer 2019-01-04 19:12:36 UTC
Created attachment 83651 [details] [review]
Bug 22071: Regression tests

The authenticate_api_request() behaviour needs more tests. This patch
adds tests for the stashed Koha::Patron object. It highlights the bug in
the OAuth authentication case.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail in the OAuth case, pass in the cookie-based auth
case.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 7 Charles Farmer 2019-01-04 19:12:40 UTC
Created attachment 83652 [details] [review]
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case

This patch makes the OAuth code stash the requestion Koha::Patron.

To test:
- Apply the tests patch:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail :-(
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> SUCCESS: Tests pass :-D
- Sign off =D

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 8 Tomás Cohen Arazi 2019-01-07 10:39:15 UTC
Created attachment 83676 [details] [review]
Bug 22071: (follow-up) Simplify code

In order to add features to this method, the current code would force us
to do it for each authentication method.

There's duplicated code that could be simplified. This patch makes the
authentication code just set $user on each block (oauth and cookie
authentication) and moves the final permissions check to the end of the
authenticate_api_request method.

Overall, the behaviour remains unchanged.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
          t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass! Nothing changed!
- Sign off :-D
Comment 9 Tomás Cohen Arazi 2019-01-07 10:39:20 UTC
Created attachment 83677 [details] [review]
Bug 22071: (follow-up) Add POD for validate_query_parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Martin Renvoize 2019-01-07 14:16:27 UTC
Created attachment 83686 [details] [review]
Bug 22071: Regression tests

The authenticate_api_request() behaviour needs more tests. This patch
adds tests for the stashed Koha::Patron object. It highlights the bug in
the OAuth authentication case.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail in the OAuth case, pass in the cookie-based auth
case.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-01-07 14:16:31 UTC
Created attachment 83687 [details] [review]
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case

This patch makes the OAuth code stash the requestion Koha::Patron.

To test:
- Apply the tests patch:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> FAIL: Tests fail :-(
- Apply this patch
- Run:
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t
=> SUCCESS: Tests pass :-D
- Sign off =D

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-01-07 14:16:35 UTC
Created attachment 83688 [details] [review]
Bug 22071: (follow-up) Simplify code

In order to add features to this method, the current code would force us
to do it for each authentication method.

There's duplicated code that could be simplified. This patch makes the
authentication code just set $user on each block (oauth and cookie
authentication) and moves the final permissions check to the end of the
authenticate_api_request method.

Overall, the behaviour remains unchanged.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
          t/db_dependent/api/v1/oauth.t
=> SUCCESS: Tests pass! Nothing changed!
- Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-01-07 14:16:38 UTC
Created attachment 83689 [details] [review]
Bug 22071: (follow-up) Add POD for validate_query_parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-01-07 14:17:21 UTC
A solid improvement to the existing code and good additional of tests.  No regressions found and no qa failures found.

Passing QA
Comment 15 Nick Clemens 2019-01-09 01:15:07 UTC
Awesome work all!

Pushed to master for 19.05
Comment 16 Martin Renvoize 2019-01-10 14:24:29 UTC
Pushed to 18.11.x for 18.11.02
Comment 17 Jesse Maseto 2019-01-11 20:18:40 UTC
Pushed to 18.05.x for 18.05.08
Comment 18 Fridolin Somers 2019-01-16 09:04:09 UTC
Depends on Bug 20568 not in 17.11.x