Description
José-Mario Monteiro-Santos
2019-03-15 15:15:04 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. 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. 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 on attachment 86756 [details] [review] Fix for newer Mojolicious/OpenAPI versions >From 9398e9ce1ddb545b031c4accad4148f743eddadd Mon Sep 17 00:00:00 2001 >From: Jose-Mario Monteiro-Santos <jose-mario.monteiro-santos@inLibro.com> >Date: Tue, 19 Mar 2019 11:55:45 -0400 >Subject: [PATCH] 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 >--- > Koha/REST/V1/Auth.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/Koha/REST/V1/Auth.pm b/Koha/REST/V1/Auth.pm >index 53c6bac..d0ecd13 100644 >--- a/Koha/REST/V1/Auth.pm >+++ b/Koha/REST/V1/Auth.pm >@@ -130,7 +130,7 @@ sub authenticate_api_request { > > my $user; > >- my $spec = $c->match->endpoint->pattern->defaults->{'openapi.op_spec'}; >+ my $spec = $c->openapi->spec; > my $authorization = $spec->{'x-koha-authorization'}; > > my $authorization_header = $c->req->headers->authorization; >-- >2.7.4 Slightly modifying Auth.pm restores the original functionnality. It may be useful as a temporary fix for those who have this problem. Looks like this should be “needs sign off”? 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."} Is 18.05 here correct or shoudl this be a higher version? I changed it to master. We applied the patch to our own 19.05 This fixes a major security problem, I'm upping the importance and changing the component from ILL to "REST API". *** Bug 23864 has been marked as a duplicate of this bug. *** Is this as bad as it is described? 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. (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? (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. (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? (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. (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! (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. (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 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.) 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.. 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. 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> 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. Created attachment 98372 [details] [review] Bug 22522: Fix route typo (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) (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 (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 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. And it seems we have misused json_has in several places. Fortunately this will be quite easy to fix in a back-compatible way. 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> Created attachment 98558 [details] [review] Bug 22522: Fix route typo 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. All the mentioned tests should now pass. I'll run a full test suite with Kohadevbox to verify. Verified. Well, I had some test failures, but they were the same with the both minimum required package versions and newest ones. 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> Created attachment 98666 [details] [review] Bug 22522: Fix route typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> (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 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. 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. *** Bug 23799 has been marked as a duplicate of this bug. *** 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> Created attachment 99182 [details] [review] Bug 22522: Fix route typo Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Rebased. Nice work everyone! Pushed to master for 20.05 (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? Ues, directly with CPAN. I don't really have enough experience with packaging. 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. 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. does not apply to 19.11.x branch. please rebase if needed. 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? (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. Created attachment 105591 [details] [review] [19.11.x] Bug 22522: Fail back to ->openapi->spec 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 I think we need all the Auth.pm changes in the first patch on this series: 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. 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. 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. 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. (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? 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. https://gitlab.com/mrenvoize/Koha/-/commits/bug_22522 is my 19.11 branch with backported patches applied if it helps. 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> 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> 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> 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? 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 :) 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' 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. Created attachment 105692 [details] [review] [19.11.x] Bug 22522: Adjust tests to previous changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (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. (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 :) Jenkins is happy now :) backported to 19.11.x for 19.11.07 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. (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. 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? 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 #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 (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 (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 (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) |