|
Description
Jonathan Druart
2020-05-13 11:22:06 UTC
Comment hidden (obsolete)
Found the issue. It took me a while....
The thing is, the JSON::Validator/openAPI plugin combination we have in D9 is very loose, and doesn't catch a problem in the Advanced editor macros routes spec. Basically, there are routes that have:
x-koha-permission: {
editcatalogue => 'advanced_editor',
editcatalogue => 'delete_shared_macros',
}
which is basically invalid, as you shouldn't be able to define a hash with two values for the same key.
My first attempt was to just to the expected thing:
x-koha-permission: {
"editcatalogue" => [
'advanced_editor',
'delete_shared_macros'
]
}
This should work, but it doesn't really. Plack would be back to work, but C4::Auth::haspermission validates the permissions when it shouldn't. At least that's what running
$ prove t/db_dependent/api/v1/advanced_editor_macros.t
tells.
I will review the tests tomorrow (too tired today). It is possible that the permissions assignment taking place in the tests is incorrect.
I was about to write a regression test for C4::Auth for this case, and see what's going on, but... to my dismay... couldn't find tests...
Oh, they are, on a separate file :-D t/db_dependent/Auth/haspermission.t Created attachment 104856 [details] [review] Bug 25482: Wrong permissions specification for adv editor macros The current permissions are defined as a hash, with a duplicate key. This is generally invalid, but Debian 9's Mojolicious::Plugin::OpenAPI lets it go through, even though the results are, random? This patch sets the required permissions right, and following the haspermissions() syntax correctly. Tests for searching macros are not passing. Still trying to figure. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #1) > Found the issue. It took me a while.... > > The thing is, the JSON::Validator/openAPI plugin combination we have in D9 > is very loose, and doesn't catch a problem in the Advanced editor macros > routes spec. Basically, there are routes that have: > > x-koha-permission: { > editcatalogue => 'advanced_editor', > editcatalogue => 'delete_shared_macros', > } > > which is basically invalid, as you shouldn't be able to define a hash with > two values for the same key. > > My first attempt was to just to the expected thing: > > x-koha-permission: { > "editcatalogue" => [ > 'advanced_editor', > 'delete_shared_macros' > ] > } > > This should work, but it doesn't really. That syntax is wrong, the attached patch puts the right one. Created attachment 104857 [details] [review] Bug 25482: Fix tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Nick, please check what's going on with the failing test! So in my understand this is a duplicate of bug 25481. The error was a bit cryptic however "Unable to load schema 'paths.json'". Tomas, I still get the issue describe in the bug report: start-stop-daemon: matching only on non-root pidfile /var/run/koha/kohadev/plack.pid is insecure [FAIL] Error: Plack not running for kohadev: failed! So I think what you fixed belonged to bug 25481 actually. Created attachment 104873 [details] [review] Bug 25482: Explicitly set macros as not shard during tests Created attachment 104874 [details] [review] Bug 25482: Explicitly set macros as not shard during tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Swapping this bug report with bug 25481. Comment 0 should be Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Unable to load schema 'paths.json' (paths.json) at /usr/local/share/perl/5.28.1/JSON/Validator.pm line 293. JSON::Validator::_load_schema(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), Mojo::URL=HASH(0x557d439f9570)) called at /usr/local/share/perl/5.28.1/JSON/Validator.pm line 398 JSON::Validator::_resolve(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), Mojo::URL=HASH(0x557d439f9570)) called at /usr/local/share/perl/5.28.1/JSON/Validator.pm line 485 JSON::Validator::_resolve_ref(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), HASH(0x557d43866008), Mojo::URL=HASH(0x557d435b5330)) called at /usr/local/share/perl/5.28.1/JSON/Validator/OpenAPI/Mojolicious.pm line 347 JSON::Validator::OpenAPI::Mojolicious::_resolve_ref(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), HASH(0x557d43866008), Mojo::URL=HASH(0x557d435b5330)) called at /usr/local/share/perl/5.28.1/JSON/Validator.pm line 446 JSON::Validator::_resolve(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), HASH(0x557d43990b70)) called at /usr/local/share/perl/5.28.1/JSON/Validator/OpenAPI/Mojolicious.pm line 61 JSON::Validator::OpenAPI::Mojolicious::load_and_validate_schema(JSON::Validator::OpenAPI::Mojolicious=HASH(0x557d435b0668), Mojo::Home=SCALAR(0x557d43997638), HASH(0x557d435b0f80)) called at /kohadevbox/koha/Koha/REST/V1.pm line 107 Koha::REST::V1::catch {...} ("Duplicate keys not allowed, at character offset 4789 (before "...) called at /usr/share/perl5/Try/Tiny.pm line 123 Try::Tiny::try(CODE(0x557d435b06b0), Try::Tiny::Catch=REF(0x557d3fe78d28)) called at /kohadevbox/koha/Koha/REST/V1.pm line 132 Koha::REST::V1::startup(Koha::REST::V1=HASH(0x557d425daba8)) called at /usr/share/perl5/Mojolicious.pm line 180 Mojolicious::new("Koha::REST::V1") called at /usr/share/perl5/Mojo/Server.pm line 17 Mojo::Server::build_app(Mojo::Server=HASH(0x557d425daa88), "Koha::REST::V1") called at /usr/share/perl5/Mojolicious/Commands.pm line 72 Mojolicious::Commands::start_app("Mojolicious::Commands", "Koha::REST::V1") called at /kohadevbox/koha/api/v1/app.pl line 21 require /kohadevbox/koha/api/v1/app.pl called at (eval 410) line 1 eval 'package Mojo::Server::Sandbox::6e5427f801a8552f3acbea43b397de68; require $path' called at /usr/share/perl5/Mojo/Server.pm line 54 Mojo::Server::load_app(Mojo::Server::PSGI=HASH(0x557d425da470), "/kohadevbox/koha/api/v1/app.pl") called at /etc/koha/sites/kohadev/plack.psgi line 64 Plack::Sandbox::_2fetc_2fkoha_2fsites_2fkohadev_2fplack_2epsgi::__ANON__() called at /usr/share/perl5/Plack/Builder.pm line 118 Plack::Builder::builder(CODE(0x557d3bd895a0)) called at /etc/koha/sites/kohadev/plack.psgi line 66 require /etc/koha/sites/kohadev/plack.psgi called at (eval 16) line 3 eval 'package Plack::Sandbox::_2fetc_2fkoha_2fsites_2fkohadev_2fplack_2epsgi; { my $app = do $_file; if ( !$app && ( my $error = $@ || $! )) { die $error; } $app; } ' called at /usr/share/perl5/Plack/Util.pm line 120 Plack::Util::_load_sandbox("/etc/koha/sites/kohadev/plack.psgi") called at /usr/share/perl5/Plack/Util.pm line 136 Plack::Util::load_psgi("/etc/koha/sites/kohadev/plack.psgi") called at /usr/share/perl5/Plack/Runner.pm line 179 Plack::Runner::__ANON__() called at /usr/share/perl5/Plack/Runner.pm line 24 Plack::Runner::__ANON__() called at /usr/share/perl5/Plack/Runner.pm line 24 Plack::Runner::__ANON__() called at /usr/share/perl5/Plack/Runner.pm line 24 Plack::Runner::__ANON__() called at /usr/share/perl5/Plack/Runner.pm line 24 Plack::Runner::__ANON__() called at /usr/share/perl5/Starman/Server.pm line 174 Starman::Server::child_init_hook(Starman::Server=HASH(0x557d3b6f3df8)) called at /usr/share/perl5/Net/Server/PreFork.pm line 218 Net::Server::PreFork::run_child(Starman::Server=HASH(0x557d3b6f3df8)) called at /usr/share/perl5/Net/Server/PreFork.pm line 183 Net::Server::PreFork::run_n_children(Starman::Server=HASH(0x557d3b6f3df8), 2) called at /usr/share/perl5/Net/Server/PreFork.pm line 107 Net::Server::PreFork::loop(Starman::Server=HASH(0x557d3b6f3df8)) called at /usr/share/perl5/Net/Server.pm line 58 Net::Server::run(Starman::Server=HASH(0x557d3b6f3df8), "port", ARRAY(0x557d3bcfd120), "host", "*", "proto", "tcp", "serialize", ...) called at /usr/share/perl5/Starman/Server.pm line 106 Starman::Server::run(Starman::Server=HASH(0x557d3b6f3df8), CODE(0x557d3b6f3ca8), HASH(0x557d3b6f3a50)) called at /usr/share/perl5/Plack/Handler/Starman.pm line 25 Plack::Handler::Starman::run(Plack::Handler::Starman=HASH(0x557d3b6f3cc0), CODE(0x557d3b6f3ca8)) called at /usr/share/perl5/Plack/Loader/Delayed.pm line 20 Plack::Loader::Delayed::run(Plack::Loader::Delayed=HASH(0x557d3b464db8), Plack::Handler::Starman=HASH(0x557d3b6f3cc0)) called at /usr/share/perl5/Plack/Runner.pm line 277 Plack::Runner::run(Plack::Runner=HASH(0x557d3b3ff348)) called at /usr/bin/starman line 38 Compilation failed in require at (eval 410) line 1. Child process 895 exited with status 2 Tomas, could we improve the error handling and provide a direct hint about which file it problematic? (In reply to Jonathan Druart from comment #12) > Tomas, could we improve the error handling and provide a direct hint about > which file it problematic? I will think about it. The problem is the reporting capabilities from the Mojolicious::Plugin::OpenAPI lib. So it will probably be something to talk to upstream. (In reply to Tomás Cohen Arazi from comment #13) > (In reply to Jonathan Druart from comment #12) > > Tomas, could we improve the error handling and provide a direct hint about > > which file it problematic? > > I will think about it. The problem is the reporting capabilities from the > Mojolicious::Plugin::OpenAPI lib. So it will probably be something to talk > to upstream. I'm all with you on this, it took me a while to bisect this and reach the actual error. *** Bug 25494 has been marked as a duplicate of this bug. *** Created attachment 104936 [details] [review] Bug 25482: Wrong permissions specification for adv editor macros The current permissions are defined as a hash, with a duplicate key. This is generally invalid, but Debian 9's Mojolicious::Plugin::OpenAPI lets it go through, even though the results are, random? This patch sets the required permissions right, and following the haspermissions() syntax correctly. Tests for searching macros are not passing. Still trying to figure. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 104937 [details] [review] Bug 25482: Fix tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 104938 [details] [review] Bug 25482: Explicitly set macros as not shard during tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Well done for getting to the bottom of this one.. D10 now starts up as expected for me and tests are looking good.. Passing QA Nice work everyone! Pushed to master for 20.05 does not apply cleanly to 19.11.x please rebase if needed |