Bug 22071

Summary: authenticate_api_request does not stash koha.user in the OAuth use case
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: charles.farmer, fridolin.somers, jesse, nick, oleonard
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 20568    
Bug Blocks: 21198, 22132    
Attachments: Bug 22071: Regression tests
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case
Bug 22071: Regression tests
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case
Bug 22071: Regression tests
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case
Bug 22071: (follow-up) Simplify code
Bug 22071: (follow-up) Add POD for validate_query_parameters
Bug 22071: Regression tests
Bug 22071: Make authenticate_api_request stash koha.user in OAuth use case
Bug 22071: (follow-up) Simplify code
Bug 22071: (follow-up) Add POD for validate_query_parameters

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