Bug 22522

Summary: API authentication breaks with updated Mojolicious version
Product: Koha Reporter: José-Mario Monteiro-Santos <jose-mario.monteiro-santos>
Component: Architecture, internals, and plumbingAssignee: Ere Maijala <ere.maijala>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: critical    
Priority: P1 - high CC: 1joynelson, aleisha, caroline.cyr-la-rose, dcook, ere.maijala, fridolin.somers, jonathan.druart, josef.moravec, julian.maurice, katrin.fischer, kyle, lucas, magnus, martin.renvoize, mirko, mtj, nick, philippe.blouin, tomascohen, victor
Version: Main   
Hardware: All   
OS: Linux   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19735
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23128
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23653
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07
Bug Depends on:    
Bug Blocks: 24167    
Attachments: Fix for newer Mojolicious/OpenAPI versions
Bug 22522 - Update API specs' access in Auth.pm
Bug 22522: Add support for current Mojolicious and related packages
Bug 22522: Add support for current Mojolicious and related packages
Bug 22522: Fix route typo
Bug 22522: Add support for current Mojolicious and related packages
Bug 22522: Fix route typo
Bug 22522: Fix several REST API tests
Bug 22522: Add support for current Mojolicious and related packages
Bug 22522: Fix route typo
Bug 22522: Fix several REST API tests
Bug 22522: Add support for current Mojolicious and related packages
Bug 22522: Fix route typo
Bug 22522: Fix several REST API tests
[19.11.x] Bug 22522: Fail back to ->openapi->spec
Bug 22522: Add support for current Mojolicious in 19.11
Bug 22522: [19.11.x] Add support for current Mojolicious and related packages
Bug 22522: [19.11.x] Fix route typo
Bug 22522: [19.11.x] Fix several REST API tests
[19.11.x] Bug 22522: Adjust tests to previous changes

Description José-Mario Monteiro-Santos 2019-03-15 15:15:04 UTC
When we tried to setup the ILL module using FreeForm as its backend, the ILL requests page in our staff client would not load any of the requests. We would get an error message saying that there was a problem with missing content in the Datable.

Upon investigation, we discovered that the calls made to the API were returning "Malformed query" errors. When validating the query, Auth.pm checks for authorized parameters in openapi.op_spec, which wasn't defined anywhere in our case. After a quick discussion with Andrew Isherwood on IRC, we downgraded Mojolicious (from 8.02 to 7.21), JSON::Validator (from 3.02 to 0.97) and Mojolicious::Plugin::OpenAPI (from 2.08 to 1.15) and everything worked.

Isn't this problematic ? If these modules aren't installed or don't meet the minimum version required by Koha, cpan will install the most recent versions and the API won't work. While this was experienced with the ILL module, I assume that other API calls can and will break in the same way.
Comment 1 Andrew Isherwood 2019-03-15 16:37:09 UTC
I agree with José-Mario, if Mojolicious is introducing breaking changes like this we should be pinning the version to be installed.

Looking at the release changelog (https://github.com/mojolicious/mojo/blob/master/Changes) 7.21 is now over two years old, so we should probably be making gradual moves forward on that too.

It seems to be a very fast moving project, so pinning the max version is probably the best thing to do in the short term.
Comment 2 José-Mario Monteiro-Santos 2019-03-18 21:15:22 UTC
I've noticed today that with newer Mojolicious versions, the authentication is basically skipped as x-koha-authorization is never defined. It seems that this makes it so all endpoints do not require authorization to be accessed. This is a major security flaw, since anybody can for example access patron's information.
Comment 3 José-Mario Monteiro-Santos 2019-03-19 17:51:47 UTC
Created attachment 86756 [details] [review]
Fix for newer Mojolicious/OpenAPI versions

With newer versions of Mojolicious and its plugins, endpoints' specs
could no longer be accessed, thus bypassing authorization checks
and failing to validate query parameters.

Test plan:
1. Without being logged in to Koha, access an endpoint directly
   (such as /api/v1/patrons/{patron_id})
2. Notice results are received (which is bad since we're not authenticated)
3. Try again with an endpoint that accepts query parameters
   (such as /api/v1/patrons?firstname=something)
4. Notice that the query is not accepted (even with correct parameters)

5. Apply the patch

6. Repeat step 1
7. Notice that the access is denied
8. Login as a user with proper access rights
9. Repeat step 1
10. Notice that you can now get results
11. Repeat step 3
12. Notice that the query is now accepted
13. Repeat step 3 but with an absurd parameter
14. Notice the query is correctly rejected

15. Ideally, check if other API calls were not broken
Comment 4 José-Mario Monteiro-Santos 2019-03-19 17:53:53 UTC Comment hidden (obsolete)
Comment 5 José-Mario Monteiro-Santos 2019-03-19 17:57:45 UTC
Slightly modifying Auth.pm restores the original functionnality. It may be useful as a temporary fix for those who have this problem.
Comment 6 Magnus Enger 2019-07-26 06:06:17 UTC
Looks like this should be “needs sign off”?
Comment 7 Magnus Enger 2019-10-03 13:07:33 UTC
Created attachment 93592 [details] [review]
Bug 22522 - Update API specs' access in Auth.pm

With newer versions of Mojolicious and its plugins, endpoints' specs
could no longer be accessed, thus bypassing authorization checks
and failing to validate query parameters.

Test plan:
1. Without being logged in to Koha, access an endpoint directly
   (such as /api/v1/patrons/{patron_id})
2. Notice results are received (which is bad since we're not authenticated)
3. Try again with an endpoint that accepts query parameters
   (such as /api/v1/patrons?firstname=something)
4. Notice that the query is not accepted (even with correct parameters)

5. Apply the patch

6. Repeat step 1
7. Notice that the access is denied
8. Login as a user with proper access rights
9. Repeat step 1
10. Notice that you can now get results
11. Repeat step 3
12. Notice that the query is now accepted
13. Repeat step 3 but with an absurd parameter
14. Notice the query is correctly rejected

15. Ideally, check if other API calls were not broken

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Upgraded modules with:
$ sudo cpanm Mojolicious JSON::Validator Mojolicious::Plugin::OpenAPI@2.14
Before the patch data is returned without being logged in. Not good!
After the patch: {"error":"Authentication failure."}
Comment 8 Katrin Fischer 2019-10-14 06:09:55 UTC
Is 18.05 here correct or shoudl this be a higher version?
Comment 9 Blou 2019-10-23 14:33:06 UTC
I changed it to master.

We applied the patch to our own 19.05
Comment 10 Magnus Enger 2019-10-24 07:19:16 UTC
This fixes a major security problem, I'm upping the importance and changing the component from ILL to "REST API".
Comment 11 Magnus Enger 2019-10-24 07:21:11 UTC
*** Bug 23864 has been marked as a duplicate of this bug. ***
Comment 12 Jonathan Druart 2019-10-24 10:58:53 UTC
Is this as bad as it is described?
Comment 13 Magnus Enger 2019-10-24 11:44:21 UTC
When I tested this for signing off I manually upgraded the relevant Perl modules and then I could access private borrowers data without being authenticated.
Comment 14 Tomás Cohen Arazi 2019-10-24 13:06:48 UTC
(In reply to Magnus Enger from comment #13)
> When I tested this for signing off I manually upgraded the relevant Perl
> modules and then I could access private borrowers data without being
> authenticated.

Does this break older versions of Mojolicious::Plugin::OpenAPI?
Comment 15 Magnus Enger 2019-10-24 13:08:24 UTC
(In reply to Tomás Cohen Arazi from comment #14)
> (In reply to Magnus Enger from comment #13)
> > When I tested this for signing off I manually upgraded the relevant Perl
> > modules and then I could access private borrowers data without being
> > authenticated.
> 
> Does this break older versions of Mojolicious::Plugin::OpenAPI?

As far as I have understood, older versions are OK. Newer versions cause problems.
Comment 16 Tomás Cohen Arazi 2019-10-24 13:11:02 UTC
(In reply to Magnus Enger from comment #15)
> (In reply to Tomás Cohen Arazi from comment #14)
> > (In reply to Magnus Enger from comment #13)
> > > When I tested this for signing off I manually upgraded the relevant Perl
> > > modules and then I could access private borrowers data without being
> > > authenticated.
> > 
> > Does this break older versions of Mojolicious::Plugin::OpenAPI?
> 
> As far as I have understood, older versions are OK. Newer versions cause
> problems.

The question is: does this change break current versions?
Comment 17 Magnus Enger 2019-10-24 13:22:56 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> The question is: does this change break current versions?

I did not test that, sorry.
Comment 18 Tomás Cohen Arazi 2019-10-24 13:23:33 UTC
(In reply to Magnus Enger from comment #17)
> (In reply to Tomás Cohen Arazi from comment #16)
> > The question is: does this change break current versions?
> 
> I did not test that, sorry.

I will!
Comment 19 Tomás Cohen Arazi 2019-10-24 18:01:39 UTC
(In reply to Magnus Enger from comment #17)
> (In reply to Tomás Cohen Arazi from comment #16)
> > The question is: does this change break current versions?
> 
> I did not test that, sorry.

Well, this produces the opposite results with older Mojo & friends, everything gets opened.
Lets think of a solution that works in both scenarios. Hint: something should be done on startup time only, like storing a function reference to the right one and using that reference in authenticate_api_request.
Comment 20 Tomás Cohen Arazi 2019-10-24 18:02:31 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> (In reply to Magnus Enger from comment #17)
> > (In reply to Tomás Cohen Arazi from comment #16)
> > > The question is: does this change break current versions?
> > 
> > I did not test that, sorry.
> 
> Well, this produces the opposite results with older Mojo & friends,
> everything gets opened.
> Lets think of a solution that works in both scenarios. Hint: something
> should be done on startup time only, like storing a function reference to
> the right one and using that reference in authenticate_api_request.

To reproduce, have a clean koha-testing-docker, get into the container, and run this on the CLI:
$ GET http://localhost:8080/api/v1/patrons
=> FAIL: You get all the patrons
Comment 21 Martin Renvoize 2019-11-07 13:47:05 UTC
I think we're going about this entirely the wrong way.. we shouldn't be adapting our code to work with a broad range of Mojo and OpenAPI versions.. we should pick a target range and stick to it.. then upgrade as a mini project each time we want to update the dependency.

I actually stated this way back when we first introduced Mojo and OpenAPI.. they're both very fast-moving projects (though they've slowed down a fair bit since I first made that comment).

We should indeed open a bug to update these dependant libraries at some point soon, but I don't think that should be done inline here. (remember, we also package the plugin ourselves.)
Comment 22 Ere Maijala 2020-01-27 12:14:25 UTC
I can see a couple of different viewpoints with all valid reasons. I'll just add mine to the mix:

I think everything should just work if koha_perl_deps.pl doesn't complain. If there's a hard requirement no not use a too recent version of a package, koha_perl_deps.pl should complain about it. I know, it would mean a max_ver would be needed in PerlDependencies.pl, but checking that Perl deps are ok would be a logical step when troubleshooting something like this, and currently it's not very useful if it can't say that something is too recent. We've been here before e.g. with HTTP::OAI.


In this case, though, wouldn't it suffice to change the code to read:

# TODO: remove the support for the latter path when the minimum version of XYZ gets bumped to at least X.Y.Z.
my $spec = $c->openapi->spec || $c->match->endpoint->pattern->defaults->{'openapi.op_spec'};

And file a follow-up bug about bumping the minimum version?

That's not like the most elegant resolution ever, but this bug is soon reaching its first anniversary, and I'm probably not the only other person being bitten by this. And I thought for a good while it was something wrong with my API plugin and didn't realize all the endpoints were affected..
Comment 23 Ere Maijala 2020-01-29 11:25:12 UTC
Created attachment 98061 [details] [review]
Bug 22522: Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.
Comment 24 Martin Renvoize 2020-01-29 16:42:28 UTC
Created attachment 98089 [details] [review]
Bug 22522: Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-01-29 16:43:58 UTC
Great work Ere, all seems to work for me in testing.. Signing off.

I agree with your comment that we should drop the support for the older module versions.
Comment 26 Mason James 2020-02-04 11:07:06 UTC
Created attachment 98372 [details] [review]
Bug 22522: Fix route typo
Comment 27 Mason James 2020-02-04 11:14:49 UTC
(In reply to Martin Renvoize from comment #25)
> Great work Ere, all seems to work for me in testing.. Signing off.
> 
> I agree with your comment that we should drop the support for the older
> module versions.

i've tested this on a deb11 bullseye system, as it has the following package versions by default...

JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

i tested the patch applied to a koha repo, on master branch at commit 6502482

a few tests failed, you can see the results here...

http://jen1.kohaaloha.com/job/Koha_master_D11-ka-mojo8/2/
http://jen1.kohaaloha.com/job/Koha_master_D11-ka-mojo8/2/testReport/

despite the failed tests, it looks promising :0)
Comment 28 Mason James 2020-02-04 11:27:40 UTC
(In reply to Mason James from comment #27)
> (In reply to Martin Renvoize from comment #25)
> > Great work Ere, all seems to work for me in testing.. Signing off.
> > 
> > I agree with your comment that we should drop the support for the older
> > module versions.
> 
> i've tested this on a deb11 bullseye system, as it has the following package
> versions by default...
> 
> JSON::Validator 3.18
> Mojolicious 8.32
> Mojolicious::Plugin::OpenAPI 2.21

fyi: specific bullseye package versions are...

libjson-validator-perl                 3.18+dfsg-1
libmojolicious-perl                    8.32+dfsg-1
libmojolicious-plugin-openapi-perl     2.21-1
Comment 29 Mason James 2020-02-04 21:24:32 UTC
(In reply to Ere Maijala from comment #23)
> Created attachment 98061 [details] [review] [review]
> Bug 22522: Add support for current Mojolicious and related packages
> 
> This patch allows tests to succeed with the following versions:
> JSON::Validator 3.18
> Mojolicious 8.32
> Mojolicious::Plugin::OpenAPI 2.21
> 
> Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now
> work.


hi Ere
can you confirm that the following test passes on your system...

 t/db_dependent/api/v1/patrons.t


i had the following error on deb-11...
----------------
        ok 17 - similar match for JSON Pointer "/conflict"
        ok 18
        ok 19 - PUT //CFyJuDAABmxNU9std4teRuXYRQtYjpdaIdQYRxufPH_g55vwFEqumaDbjY8PApSA:thePassword123@/api/v1/patrons/70
        ok 20 - Patron updated successfully
        not ok 21 - has value for JSON Pointer "HASH(0x56040d5d72e8)"  <<<<<<<<<<<
        ok 22 - Patron is really updated!
----------------

you can see the full results here ->  https://bit.ly/2Utsmqf
Comment 30 Ere Maijala 2020-02-06 12:37:50 UTC
Right, our definitions don't quite match reality. E.g. patron's surname can be null while the spec defines it as a string. That's just one, and I'm seeing other errors too, so it seems I failed to test this properly. I'll see what I can do.
Comment 31 Ere Maijala 2020-02-06 13:55:33 UTC
And it seems we have misused json_has in several places. Fortunately this will be quite easy to fix in a back-compatible way.
Comment 32 Ere Maijala 2020-02-07 11:51:13 UTC
Created attachment 98557 [details] [review]
Bug 22522: Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Ere Maijala 2020-02-07 11:51:17 UTC
Created attachment 98558 [details] [review]
Bug 22522: Fix route typo
Comment 34 Ere Maijala 2020-02-07 11:51:21 UTC
Created attachment 98559 [details] [review]
Bug 22522: Fix several REST API tests

Fixes among others the invalid use of json_has() which caused broken tests to pass with older Mojolicious versions.
Comment 35 Ere Maijala 2020-02-07 12:32:40 UTC
All the mentioned tests should now pass. I'll run a full test suite with Kohadevbox to verify.
Comment 36 Ere Maijala 2020-02-10 09:18:34 UTC
Verified. Well, I had some test failures, but they were the same with the both minimum required package versions and newest ones.
Comment 37 Martin Renvoize 2020-02-10 15:36:49 UTC
Created attachment 98665 [details] [review]
Bug 22522: Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-02-10 15:36:53 UTC
Created attachment 98666 [details] [review]
Bug 22522: Fix route typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-02-10 15:36:57 UTC
Created attachment 98667 [details] [review]
Bug 22522: Fix several REST API tests

Fixes among others the invalid use of json_has() which caused broken tests to pass with older Mojolicious versions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Mason James 2020-02-12 03:40:37 UTC
(In reply to Ere Maijala from comment #35)
> All the mentioned tests should now pass. I'll run a full test suite with
> Kohadevbox to verify.

hi folks
all tests are passing for me on debian 11,10,9,8

debian 9 and 8 are using mojo-7 and openapi-1 modules
debian 11 and 10 are using mojo-8 and openapi-2 modules

great work Ere!!! :p
Comment 41 Martin Renvoize 2020-02-12 07:29:33 UTC
That's great news.. given I signed off and checked the code myself too.  I think that's enough to call us PQA. Thanks for all your efforts guys, I'm really excited to push this one.
Comment 42 Martin Renvoize 2020-02-13 07:59:26 UTC
With the adoption of cpanfile in bug 19735 we introduce the ability to set max version and excluded versions.

Let's get this patch in as is, but follow-up quickly with a bug to set these rapidly changing libraries more strictly.
Comment 43 Martin Renvoize 2020-02-14 10:57:26 UTC
*** Bug 23799 has been marked as a duplicate of this bug. ***
Comment 44 Ere Maijala 2020-02-18 12:44:54 UTC
Created attachment 99181 [details] [review]
Bug 22522: Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Ere Maijala 2020-02-18 12:44:58 UTC
Created attachment 99182 [details] [review]
Bug 22522: Fix route typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Ere Maijala 2020-02-18 12:45:02 UTC
Created attachment 99183 [details] [review]
Bug 22522: Fix several REST API tests

Fixes among others the invalid use of json_has() which caused broken tests to pass with older Mojolicious versions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Ere Maijala 2020-02-18 12:46:47 UTC
Rebased.
Comment 48 Martin Renvoize 2020-02-25 13:44:51 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 49 Martin Renvoize 2020-02-25 17:09:50 UTC
(In reply to Mason James from comment #40)
> (In reply to Ere Maijala from comment #35)
> > All the mentioned tests should now pass. I'll run a full test suite with
> > Kohadevbox to verify.
> 
> hi folks
> all tests are passing for me on debian 11,10,9,8
> 
> debian 9 and 8 are using mojo-7 and openapi-1 modules
> debian 11 and 10 are using mojo-8 and openapi-2 modules
> 
> great work Ere!!! :p

So... where are we at with D10 packaging.. I'm confused..

How did you install Mojolicious::Plugin::OpenAPI on Buster for testing the above scenario's.. directly with cpan?
Comment 50 Ere Maijala 2020-02-26 07:15:00 UTC
Ues, directly with CPAN. I don't really have enough experience with packaging.
Comment 51 Martin Renvoize 2020-02-26 08:37:45 UTC
Brill, me too.

For now I've added it to the cpan list in our koha-testing-docker Dockerfile for Buster.  We should look at packaging options now I think as it's not great to require people to pull dependancies via cpan.. I'll open a big for both that and more formally fixing our dependancy versions.
Comment 52 David Cook 2020-03-12 07:18:48 UTC
Cheers Ere and Martin! 

(In reply to Blou from comment #9)
> I changed it to master.
> 
> We applied the patch to our own 19.05

I've patched our own 18.11 and 19.11 with Ere's fix, so I wouldn't be surprised if it affects 18.05 as well. 

(How far back are we supporting these days?)

It would be good to see this get pushed to the stable branches ASAP. While this problem shouldn't affect anyone using Debian packages, I'm sure there are lots of people around the world who aren't using them.
Comment 53 Joy Nelson 2020-03-31 22:20:53 UTC
does not apply to 19.11.x branch.  please rebase if needed.
Comment 54 Jonathan Druart 2020-06-03 10:36:15 UTC
All the API related tests are failing for 18.11 and 19.05 (maybe 19.11 as well, I have just requested a new run 19.11_D9 #156).

Could that be related to this change?
Comment 55 Kyle M Hall 2020-06-03 12:23:43 UTC
(In reply to Jonathan Druart from comment #54)
> All the API related tests are failing for 18.11 and 19.05 (maybe 19.11 as
> well, I have just requested a new run 19.11_D9 #156).
> 
> Could that be related to this change?

I think so. At least, they are failing for me on koha-testing-docker, presumably because the Mojo version is too new.

-    my $spec = $c->match->endpoint->pattern->defaults->{'openapi.op_spec'};
+    my $spec = $c->openapi->spec || $c->match->endpoint->pattern->defaults->{'openapi.op_spec'};

Fixes it for me.
Comment 56 Kyle M Hall 2020-06-05 14:47:40 UTC
Created attachment 105591 [details] [review]
[19.11.x] Bug 22522: Fail back to ->openapi->spec
Comment 57 Jonathan Druart 2020-06-08 09:39:34 UTC
Kyle, using master-bionic and 19.11.x + your patch I get:

t/db_dependent/api/v1/acquisitions_funds.t ............. 1/13                                                                                                                                                     
#   Failed test '401 Unauthorized'                                                                                                                                                                                
#   at t/db_dependent/api/v1/acquisitions_funds.t line 67.                                                                                                                                                        
#          got: '200'                                                                                                                                                                                             
#     expected: '401'
Use of uninitialized value $_[2] in hash element at /usr/share/perl5/Mojolicious/Plugin/OpenAPI.pm line 275.                                                                                                      
                                                                                                                         

And lot of similar failures
Comment 58 Martin Renvoize 2020-06-08 09:56:56 UTC
I think we need all the Auth.pm changes in the first patch on this series:
Comment 59 Martin Renvoize 2020-06-08 12:13:49 UTC
Created attachment 105639 [details] [review]
Bug 22522: Add support for current Mojolicious in 19.11

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions
now work.

Calling valid_input in under() would cause ' Use of uninitialized value
$_[2] ' in more recent OpenAPI plugins, so that was changed too. As far
as I can see this does not affect authorization.
Comment 60 Martin Renvoize 2020-06-08 12:14:49 UTC
I believe my alternative to Kyle's should do the trick.. it's literally the REST/V1/Auth.pm changes from the first patch here, just without all the test updates included.
Comment 61 Jonathan Druart 2020-06-08 12:49:09 UTC
still not working but it's better.

Only 3 are failing now:
t/db_dependent/api/v1/libraries.t
t/db_dependent/api/v1/patrons.t
t/db_dependent/api/v1/patrons_password.t

with things like
      #   Failed test 'has value for JSON Pointer "ARRAY(0x558849c58998)"'
        #   at t/db_dependent/api/v1/libraries.t line 92.                           
        # Looks like you failed 17 tests of 51.
Comment 62 Martin Renvoize 2020-06-08 14:08:28 UTC
I've been looking at this and in reality.. I think what we really need is to just backport the whole set correctly.. I'm not sure what stopped it from being backported in the first place to be honest.

I've had a go at backporting locally and it applies with minor conflicts to resolve on the 19.11 branch pretty easily. (all of it, including all the tests fixes).. The problem I'm having however, is that running all the tests to check they're now working against a KOHA_IMAGE=19.11-bionic koha-testing-docker crashes my dev server entirely for unknown reasons at the moment.. hence I've not submitted the set on mass yet.
Comment 63 Tomás Cohen Arazi 2020-06-08 15:12:42 UTC
(In reply to Martin Renvoize from comment #62)
> I've been looking at this and in reality.. I think what we really need is to
> just backport the whole set correctly.. I'm not sure what stopped it from
> being backported in the first place to be honest.
> 
> I've had a go at backporting locally and it applies with minor conflicts to
> resolve on the 19.11 branch pretty easily. (all of it, including all the
> tests fixes).. The problem I'm having however, is that running all the tests
> to check they're now working against a KOHA_IMAGE=19.11-bionic
> koha-testing-docker crashes my dev server entirely for unknown reasons at
> the moment.. hence I've not submitted the set on mass yet.

Are you by chance running it with CPAN and COVERAGE?
Comment 64 Martin Renvoize 2020-06-08 15:25:25 UTC
Nope.. 

`KOHA_IMAGE=19.11-bionic docker-compose -f docker-compose.yml -f docker-compose.mariadb_d9.yml -p koha up --force-recreate`

Then `kshell`, `kshell`, `prove -v t/db_dependant/api/v1/*`

It get's about 2/3rds through then the host OS runs out of memory and starts OOMing, starting with ssh.. and finally it all just falls over and I have to restart my dev box.
Comment 65 Martin Renvoize 2020-06-08 15:27:13 UTC
https://gitlab.com/mrenvoize/Koha/-/commits/bug_22522 is my 19.11 branch with backported patches applied if it helps.
Comment 66 Jonathan Druart 2020-06-09 09:31:51 UTC
Created attachment 105657 [details] [review]
Bug 22522: [19.11.x] Add support for current Mojolicious and related packages

This patch allows tests to succeed with the following versions:
JSON::Validator 3.18
Mojolicious 8.32
Mojolicious::Plugin::OpenAPI 2.21

Also Mojolicious::Plugin::OpenAPI version 1.17 and later 1.x versions now work.

Calling valid_input in under() would cause ' Use of uninitialized value $_[2] ' in more recent OpenAPI plugins, so that was changed too. As far as I can see this does not affect authorization.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 67 Jonathan Druart 2020-06-09 09:31:56 UTC
Created attachment 105658 [details] [review]
Bug 22522: [19.11.x] Fix route typo

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 68 Jonathan Druart 2020-06-09 09:32:01 UTC
Created attachment 105659 [details] [review]
Bug 22522: [19.11.x] Fix several REST API tests

Fixes among others the invalid use of json_has() which caused broken tests to pass with older Mojolicious versions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 69 Jonathan Druart 2020-06-09 09:32:16 UTC
I have tested Martin's remote branch on master-bionic and 19.11-bionic: all tests pass!

Aleisha, could you push those 3 patches (the 19.11 ones) and see how behave Jenkins?
Comment 70 Martin Renvoize 2020-06-09 10:38:57 UTC
Awesome.. glad to hear it worked well for your Jonathan.. I think my OOM's were local.. seems my dev server was due of a little maintenance and I can confirm that test now pass for me too :)
Comment 71 Aleisha Amohia 2020-06-10 03:32:12 UTC
Hi all, the tests fail in Jenkins in 19.11. Output below: 

10:41:09 koha_1       | DBD::mysql::st execute failed: Unknown column 'name' in 'where clause' [for Statement "SELECT COUNT( * ) FROM `cities` `me` WHERE ( `name` LIKE ? )" with ParamValues: 0="manuel%"] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test '200 OK'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       |     #          got: '500'
10:41:09 koha_1       |     #     expected: '200'
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'has value for JSON Pointer "/0"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'has value for JSON Pointer "/1"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'has value for JSON Pointer "/2"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'exact match for JSON Pointer "/0/name"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       |     #          got: undef
10:41:09 koha_1       |     #     expected: 'Manuel'
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'exact match for JSON Pointer "/1/name"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       |     #          got: undef
10:41:09 koha_1       |     #     expected: 'Manuela'
10:41:09 koha_1       | 
10:41:09 koha_1       |     #   Failed test 'exact match for JSON Pointer "/2/name"'
10:41:09 koha_1       |     #   at t/db_dependent/Koha/REST/Plugin/Objects.t line 155.
10:41:09 koha_1       |     #          got: undef
10:41:09 koha_1       |     #     expected: 'Manuelab'
Comment 72 Jonathan Druart 2020-06-10 08:03:03 UTC
19.11: There is something wrong in the paths, and so plack does not start correctly.
I have cherry-picked a07c5c153aaecc5cc8b0d1de5c53f184da5971b9
  Bug 25279: Add general query parameters to the /cities route

But there is still something wrong in the tests, name is not reversed with city_name but I did not find what was missing.
Comment 73 Tomás Cohen Arazi 2020-06-10 13:12:47 UTC
Created attachment 105692 [details] [review]
[19.11.x] Bug 22522: Adjust tests to previous changes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Jonathan Druart 2020-06-10 13:25:31 UTC
(In reply to Jonathan Druart from comment #72)
> 19.11: There is something wrong in the paths, and so plack does not start
> correctly.
> I have cherry-picked a07c5c153aaecc5cc8b0d1de5c53f184da5971b9
>   Bug 25279: Add general query parameters to the /cities route

Forget about this.
Comment 75 Jonathan Druart 2020-06-10 13:26:13 UTC
(In reply to Tomás Cohen Arazi from comment #73)
> Created attachment 105692 [details] [review] [review]
> [19.11.x] Bug 22522: Adjust tests to previous changes
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Aleisha, this one is read for you :)
Comment 76 Aleisha Amohia 2020-06-11 21:01:32 UTC
Jenkins is happy now :) backported to 19.11.x for 19.11.07
Comment 77 Victor Grousset/tuxayo 2020-06-12 18:16:25 UTC
I got a conflict when trying to backport to 19.05. (note the tests pass without these patches, no API issues on 19.05)

I'm not sure about InstallPlugins, it doesn't exist in 19.05 vs 19.11. So I don't know how valid that is if I don't use it.

About `Koha::Plugins::Methods->search->delete;` there is also uncertainly.

(note that HEAD means 19.05.x,
parent of 6eef54e940 means 19.11.x before the patch that I'm backporting,
and the last section is the patch that I'm backporting)
<<<<<<< HEAD
    my @plugins = Koha::Plugins->new->GetPlugins( { all => 1 } );
||||||| parent of 6eef54e940... [SIGNED-OFF] Bug 22522: [19.11.x] Add support for current Mojolicious and related packages
    my $plugins = Koha::Plugins->new;
    $plugins->InstallPlugins;

    my @plugins = $plugins->GetPlugins( { all => 1 } );
=======
    # remove any existing plugins that might interfere
    Koha::Plugins::Methods->search->delete;
    my $plugins = Koha::Plugins->new;
    $plugins->InstallPlugins;

    my @plugins = $plugins->GetPlugins( { all => 1 } );
>>>>>>> 6eef54e940... [SIGNED-OFF] Bug 22522: [19.11.x] Add support for current Mojolicious and related packages


It's only in PluginRoutes.t I could just try and see which tests pass...



Next conflict!
(note that HEAD means 19.05.x,
parent of 095948c43f means 19.11.x before the patch that I'm backporting,
and the last section is the patch that I'm backporting)
t/db_dependent/api/v1/patrons.t
<<<<<<< HEAD
        $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . $patron_2->id => json => $newpatron);
        $tx->req->cookies({name => 'CGISESSID', value => $session_id});
        $t->request_ok($tx)
          ->status_is(200, 'Patron updated successfully')
          ->json_has($newpatron);
||||||| parent of 095948c43f... [SIGNED-OFF] Bug 22522: [19.11.x] Fix several REST API tests
        $t->put_ok( "//$userid:$password@/api/v1/patrons/" . $patron_2->borrowernumber => json => $newpatron )
          ->status_is(200, 'Patron updated successfully')
          ->json_has($newpatron);
=======
        my $result = $t->put_ok( "//$userid:$password@/api/v1/patrons/" . $patron_2->borrowernumber => json => $newpatron )
          ->status_is(200, 'Patron updated successfully');

        # Put back the RO attributes
        $newpatron->{patron_id} = $unauthorized_patron->to_api->{patron_id};
        $newpatron->{restricted} = $unauthorized_patron->to_api->{restricted};
        $newpatron->{anonymized} = $unauthorized_patron->to_api->{anonymized};
        is_deeply($result->tx->res->json, $newpatron, 'Returned patron from update matches expected');

>>>>>>> 095948c43f... [SIGNED-OFF] Bug 22522: [19.11.x] Fix several REST API tests

I don't know if 19.11 supports that way to authenticate.
The tests will tell.

Another similar conflict.


Tests: it doesn't work. That was to be expected, that would have been so lucky to just hammer it like that.
Comment 78 Jonathan Druart 2020-06-15 08:19:17 UTC
(In reply to Aleisha Amohia from comment #76)
> Jenkins is happy now :) backported to 19.11.x for 19.11.07

It seems that there is something wrong for D10 however:

19.11_D10 (#14)

18:34:00 koha_1       | #   at t/00-load.t line 62.
18:34:00 koha_1       | #     Tried to use 'Koha::REST::V1'.
18:34:00 koha_1       | #     Error:  Can't locate JSON/Validator/OpenAPI/Mojolicious.pm in @INC (you may need to install the JSON::Validator::OpenAPI::Mojolicious module) (@INC contains: /kohadevbox/koha /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.28.1 /usr/local/share/perl/5.28.1 /usr/lib/x86_64-linux-gnu/perl5/5.28 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.28 /usr/share/perl/5.28 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base /var/lib/koha/kohadev/plugins) at /kohadevbox/koha/Koha/REST/V1.pm line 23.
Comment 79 Martin Renvoize 2020-06-15 09:34:45 UTC
I'm confused.. in the 19.11 D10 package we seem to be getting the following Mojolicious::Plugin::OpenAPI version:

kohadev-koha@ecc849b0f6ab:/kohadevbox/koha$ apt-cache policy libmojolicious-plugin-openapi-perl
libmojolicious-plugin-openapi-perl:
  Installed: 1.15-1~kohadev1
  Candidate: 1.15-1~kohadev1
  Version table:
 *** 1.15-1~kohadev1 500
        500 http://debian.koha-community.org/koha 19.11/main amd64 Packages
        100 /var/lib/dpkg/status

This version predates the movement of code from the core plugin into JSON::Validator::OpenAPI::Mojolicious as as such we don't get that module installed.. which leads to the errors Jonathan mentions above.. did something go weird with the packaging here?
Comment 80 Martin Renvoize 2020-06-15 10:03:25 UTC
Even more confused.. seems we're using those versions elsewhere too.. so I'm not understanding how the tests are passing elsewhere but not no D10.
Comment 81 Mason James 2020-06-17 00:03:45 UTC
(In reply to Martin Renvoize from comment #80)
> Even more confused.. seems we're using those versions elsewhere too.. so I'm
> not understanding how the tests are passing elsewhere but not no D10.

(In reply to Martin Renvoize from comment #79)
> I'm confused.. in the 19.11 D10 package we seem to be getting the following
> Mojolicious::Plugin::OpenAPI version:
> 
> kohadev-koha@ecc849b0f6ab:/kohadevbox/koha$ apt-cache policy
> libmojolicious-plugin-openapi-perl
> libmojolicious-plugin-openapi-perl:
>   Installed: 1.15-1~kohadev1
>   Candidate: 1.15-1~kohadev1
>   Version table:
>  *** 1.15-1~kohadev1 500
>         500 http://debian.koha-community.org/koha 19.11/main amd64 Packages
>         100 /var/lib/dpkg/status
> 
> This version predates the movement of code from the core plugin into
> JSON::Validator::OpenAPI::Mojolicious as as such we don't get that module
> installed.. which leads to the errors Jonathan mentions above.. did
> something go weird with the packaging here?

hi Martin
this behaviour is actually expected

the 'koha on debian-10' APT fixes have currently only been applied to the koha dev and 20.05 suites, and not the 19.11 and 19.05 suites
appling the fixes for koha 19.11 on debian 10, will actually break koha 19.11 on debian 8


i recommend that we fix koha 19.11 on debian 10, with the acceptance that koha 19.11 on debian 8 will break


the following patches are required to be applied to the 19.11.x branch for this to happen

	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25633
	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25591
	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25633


NOTE: users running 19.11 on older debian versions (oldoldstable and older) can continue to use the koha 19.11 packages by using the following /etc/apt/sources.d/koha line...

	deb http://debian.koha-community.org/koha 19.11 main OS_VERSION

replacing OS_VERSION with jessie, xenial, bionic
Comment 82 Aleisha Amohia 2020-06-17 21:55:42 UTC
(In reply to Mason James from comment #81)
> the following patches are required to be applied to the 19.11.x branch for
> this to happen
> 
> 	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25633
> 	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25591

Thanks all I have backported these two to 19.11.x for 19.11.07
Comment 83 Aleisha Amohia 2020-06-17 23:27:44 UTC
(In reply to Mason James from comment #81)

> hi Martin
> this behaviour is actually expected
> 
> the 'koha on debian-10' APT fixes have currently only been applied to the
> koha dev and 20.05 suites, and not the 19.11 and 19.05 suites
> appling the fixes for koha 19.11 on debian 10, will actually break koha
> 19.11 on debian 8
> 
> 
> i recommend that we fix koha 19.11 on debian 10, with the acceptance that
> koha 19.11 on debian 8 will break
> 

Noting that, as expected, the 19.11_D8 node is broken
Comment 84 Victor Grousset/tuxayo 2020-06-30 02:39:20 UTC
(In reply to Magnus Enger from comment #10)
> This fixes a major security problem, I'm upping the importance and changing
> the component from ILL to "REST API".

Wow, I missed that.

After a quick test I confirm that I can't do this (on 19.05 and 18.11):

> Test plan:
> 1. Without being logged in to Koha, access an endpoint directly
   (such as /api/v1/patrons/{patron_id})
> 2. Notice results are received (which is bad since we're not authenticated)