Description
Lari Taskula
2019-11-08 14:22:32 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' 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 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. 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. Created attachment 105579 [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> Created attachment 105580 [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> Created attachment 105581 [details] [review] Bug 24003: (follow-up) Basic auth missing tests for stashed user Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> 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> 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> 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'). 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> 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> 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> Why are you only setting the borrowernumber? (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. 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? (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::* 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. 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> 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. 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> 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> 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> 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> 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 Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.01 doesn't apply cleanly to 19.11.x. please rebase if we want this in 19.11.x 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> 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> 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> 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> 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. 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. (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 Missing dependencies for 19.05.x, can't backport. |