Bug 24003

Summary: REST API should set C4::Context->userenv
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: aleisha, jonathan.druart, josef.moravec, kyle, lucas, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24016
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.01, 19.11.07
Bug Depends on: 22132, 25411    
Bug Blocks:    
Attachments: Bug 24003: Add a failing test for missing userenv for REST basic auth
Bug 24003: Create new userenv for REST basic authentication
Bug 24003: Regression tests
Bug 24003: Make the API set userenv on authentication
Bug 24003: (follow-up) Basic auth missing tests for stashed user
Bug 24003: Regression tests
Bug 24003: Make the API set userenv on authentication
Bug 24003: (follow-up) Move interface setting to a more readable place
Bug 24003: Regression tests
Bug 24003: Make the API set userenv on authentication
Bug 24003: (follow-up) Move interface setting to a more readable place
Bug 24003: (follow-up) Add all userenv parameters
Bug 24003: Regression tests
Bug 24003: Make the API set userenv on authentication
Bug 24003: (follow-up) Move interface setting to a more readable place
Bug 24003: (follow-up) Add all userenv parameters
[19.11.x] Bug 24003: Regression tests
[19.11.x] Bug 24003: Make the API set userenv on authentication
[19.11.x] Bug 24003: (follow-up) Move interface setting to a more readable place
[19.11.x] Bug 24003: (follow-up) Add all userenv parameters

Description Lari Taskula 2019-11-08 14:22:32 UTC

    
Comment 1 Lari Taskula 2019-11-08 14:50:45 UTC
Created attachment 95228 [details] [review]
Bug 24003: Add a failing test for missing userenv for REST basic auth

When using REST API via basic authentication method, C4::Context->userenv
returns undef.

This patch adds a failing test to prove it.

To test:
1. prove t/db_dependent/api/v1/auth_basic.t
2. Observe failures:

    not ok 5 - exact match for JSON Pointer "/userenv/number"
       Failed test 'exact match for JSON Pointer "/userenv/number"'
       at t/db_dependent/api/v1/auth_basic.t line 52.
              got: undef
         expected: '1054'
    not ok 6 - exact match for JSON Pointer "/userenv/id"
       Failed test 'exact match for JSON Pointer "/userenv/id"'
       at t/db_dependent/api/v1/auth_basic.t line 52.
              got: undef
         expected: 'tomasito'
Comment 2 Lari Taskula 2019-11-08 15:21:20 UTC
Created attachment 95230 [details] [review]
Bug 24003: Create new userenv for REST basic authentication

To test:
1. prove t/db_dependent/api/v1/auth_basic.t
2. Observe success
Comment 3 Lari Taskula 2019-11-08 16:35:29 UTC
After discussing this issue on IRC we came to a conclusion that we should use the Koha::Patron from koha.user stash instead, and fix Koha::Objects that rely on userenv alone.
Comment 4 Tomás Cohen Arazi 2020-06-04 15:54:33 UTC
There's been big changes on the API front, but it is not clear when we will be able to make a clean transition into using Mojolicious for all controlling code and thus making the stash accessible to Koha::* classes.

That said, I'm taking over this bug, and will make it take care of setting userenv on every authentication method, as we are now seeing action_logs with no borrowernumber responsible for changes that happen in the API.
Comment 5 Tomás Cohen Arazi 2020-06-05 12:28:25 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2020-06-05 12:28:31 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2020-06-05 12:28:34 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2020-06-05 12:49:33 UTC
Created attachment 105586 [details] [review]
Bug 24003: Regression tests

This patch adds regression tests for the different authentication
mechanisms Koha supports. It highlights the fact that Koha expects
userenv to be set on authentication.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 9 Tomás Cohen Arazi 2020-06-05 12:49:40 UTC
Created attachment 105587 [details] [review]
Bug 24003: Make the API set userenv on authentication

This patch makes the authentication step stash the user that got
authenticated so code outside the Mojo part of Koha can use it (i.e.
through the use of C4::Context->userenv).

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2020-06-05 12:49:46 UTC
Created attachment 105588 [details] [review]
Bug 24003: (follow-up) Move interface setting to a more readable place

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2020-06-05 12:53:01 UTC
I went on and added missing tests in basic auth, and also added a test for an untested feature (C4::Context->interface being set to 'api').
Comment 12 Kyle M Hall 2020-06-08 11:04:23 UTC
Created attachment 105633 [details] [review]
Bug 24003: Regression tests

This patch adds regression tests for the different authentication
mechanisms Koha supports. It highlights the fact that Koha expects
userenv to be set on authentication.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2020-06-08 11:04:44 UTC
Created attachment 105634 [details] [review]
Bug 24003: Make the API set userenv on authentication

This patch makes the authentication step stash the user that got
authenticated so code outside the Mojo part of Koha can use it (i.e.
through the use of C4::Context->userenv).

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2020-06-08 11:04:48 UTC
Created attachment 105635 [details] [review]
Bug 24003: (follow-up) Move interface setting to a more readable place

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Jonathan Druart 2020-06-08 11:40:50 UTC
Why are you only setting the borrowernumber?
Comment 16 Tomás Cohen Arazi 2020-06-08 12:03:45 UTC
(In reply to Jonathan Druart from comment #15)
> Why are you only setting the borrowernumber?

On the API, this is only relevant for action_logs and probably filtering in plural classes. If it is going to be used from the UI, then authentication would be cookie-based, which sets all the things anyway.
Comment 17 Jonathan Druart 2020-06-08 12:22:08 UTC
Say you are going to implement the DELETE route for items, you will need to call Koha::Item->safe_to_delete.

246     return "not_same_branch"                                                                         
247       if defined C4::Context->userenv                                                                
248       and !C4::Context->IsSuperLibrarian()                                                           
249       and C4::Context->preference("IndependentBranches")
250       and ( C4::Context->userenv->{branch} ne $self->homebranch );   

Don't you want the branch to be defined at this point? Isn't that the purpose of this patch?
Comment 18 Tomás Cohen Arazi 2020-06-08 13:00:22 UTC
(In reply to Jonathan Druart from comment #17)
> Say you are going to implement the DELETE route for items, you will need to
> call Koha::Item->safe_to_delete.
> 
> 246     return "not_same_branch"                                            
> 
> 247       if defined C4::Context->userenv                                   
> 
> 248       and !C4::Context->IsSuperLibrarian()                              
> 
> 249       and C4::Context->preference("IndependentBranches")
> 250       and ( C4::Context->userenv->{branch} ne $self->homebranch );   
> 
> Don't you want the branch to be defined at this point? Isn't that the
> purpose of this patch?

I'm fixing a current bug on stable branches. If we were to think this in terms of the project future devs, I would rather stash the full Koha::Patron for the logged user and get rid of all that cruft we've been porting to Koha::*
Comment 19 Jonathan Druart 2020-06-09 09:35:47 UTC
I have no idea which bug in stable we are trying to fix. Where is that describe?

We should think "regression proof", if the userenv exists then it should be reliable. Having only the borrowerumber defined seems very wrong to me.
Comment 20 Tomás Cohen Arazi 2020-06-09 15:47:25 UTC
Created attachment 105666 [details] [review]
Bug 24003: (follow-up) Add all userenv parameters

This patch makes the API set more userenv params than just the
borrowernumber. To test:

1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_*
=> SUCCESS: All tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2020-06-09 15:49:59 UTC
The bug in stable is that any API interaction (the route /patrons changing data from an external source) ends with action_logs having the responsible for the action not set => bad.
Comment 22 Martin Renvoize 2020-06-12 15:46:00 UTC
Created attachment 105844 [details] [review]
Bug 24003: Regression tests

This patch adds regression tests for the different authentication
mechanisms Koha supports. It highlights the fact that Koha expects
userenv to be set on authentication.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2020-06-12 15:46:04 UTC
Created attachment 105845 [details] [review]
Bug 24003: Make the API set userenv on authentication

This patch makes the authentication step stash the user that got
authenticated so code outside the Mojo part of Koha can use it (i.e.
through the use of C4::Context->userenv).

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-06-12 15:46:07 UTC
Created attachment 105846 [details] [review]
Bug 24003: (follow-up) Move interface setting to a more readable place

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-06-12 15:46:10 UTC
Created attachment 105847 [details] [review]
Bug 24003: (follow-up) Add all userenv parameters

This patch makes the API set more userenv params than just the
borrowernumber. To test:

1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_*
=> SUCCESS: All tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2020-06-12 15:47:11 UTC
This all works well and I can't see any regressions. The followup doesn't seem to do any harm and will future proof us somewhat.

Passing QA
Comment 27 Jonathan Druart 2020-06-15 09:59:20 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 28 Lucas Gass 2020-06-17 21:38:30 UTC
backported to 20.05.x for 20.05.01
Comment 29 Aleisha Amohia 2020-06-18 04:23:43 UTC
doesn't apply cleanly to 19.11.x. please rebase if we want this in 19.11.x
Comment 30 Tomás Cohen Arazi 2020-06-19 12:59:28 UTC
Created attachment 106087 [details] [review]
[19.11.x] Bug 24003: Regression tests

This patch adds regression tests for the different authentication
mechanisms Koha supports. It highlights the fact that Koha expects
userenv to be set on authentication.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Tomás Cohen Arazi 2020-06-19 12:59:34 UTC
Created attachment 106088 [details] [review]
[19.11.x] Bug 24003: Make the API set userenv on authentication

This patch makes the authentication step stash the user that got
authenticated so code outside the Mojo part of Koha can use it (i.e.
through the use of C4::Context->userenv).

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_authenticate_api_request.t \
           t/db_dependent/api/v1/auth_basic.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Tomás Cohen Arazi 2020-06-19 12:59:40 UTC
Created attachment 106089 [details] [review]
[19.11.x] Bug 24003: (follow-up) Move interface setting to a more readable place

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Tomás Cohen Arazi 2020-06-19 12:59:48 UTC
Created attachment 106090 [details] [review]
[19.11.x] Bug 24003: (follow-up) Add all userenv parameters

This patch makes the API set more userenv params than just the
borrowernumber. To test:

1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/auth_*
=> SUCCESS: All tests pass
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Tomás Cohen Arazi 2020-06-19 13:33:50 UTC
Aleisha, the dependencies, in order are:

- 24862
- 25045
- 25237
- 25411
- 24003

They should all apply on 19.11.x and their tests pass. Some (like this one) have their own patch versions.
Comment 35 Aleisha Amohia 2020-06-21 23:57:27 UTC

backported to 19.11.x for 19.11.07

(In reply to Tomás Cohen Arazi from comment #34)


> - 25237

Not this one, as it is still in Needs Signoff.
Comment 36 Tomás Cohen Arazi 2020-06-22 00:01:43 UTC
(In reply to Aleisha Amohia from comment #35)
> 
> backported to 19.11.x for 19.11.07
> 
> (In reply to Tomás Cohen Arazi from comment #34)
> 
> 
> > - 25237
> 
> Not this one, as it is still in Needs Signoff.

That was some typo, 25327
Comment 37 Victor Grousset/tuxayo 2020-07-02 22:35:32 UTC
Missing dependencies for 19.05.x, can't backport.