Bug 25482

Summary: Wrong permissions in spec break Plack on Debian 10
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Command-line UtilitiesAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P5 - low CC: 1joynelson, kyle, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25481
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25504
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 24167, 25502    
Attachments: Bug 25482: Wrong permissions specification for adv editor macros
Bug 25482: Fix tests
Bug 25482: Explicitly set macros as not shard during tests
Bug 25482: Explicitly set macros as not shard during tests
Bug 25482: Wrong permissions specification for adv editor macros
Bug 25482: Fix tests
Bug 25482: Explicitly set macros as not shard during tests

Description Jonathan Druart 2020-05-13 11:22:06 UTC Comment hidden (obsolete)
Comment 1 Tomás Cohen Arazi 2020-05-13 21:58:05 UTC
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...
Comment 2 Tomás Cohen Arazi 2020-05-13 21:59:45 UTC
Oh, they are, on a separate file :-D
t/db_dependent/Auth/haspermission.t
Comment 3 Tomás Cohen Arazi 2020-05-13 22:12:33 UTC
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>
Comment 4 Tomás Cohen Arazi 2020-05-13 22:13:58 UTC
(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.
Comment 5 Tomás Cohen Arazi 2020-05-13 22:34:50 UTC
Created attachment 104857 [details] [review]
Bug 25482: Fix tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2020-05-13 22:35:35 UTC
Nick, please check what's going on with the failing test!
Comment 7 Jonathan Druart 2020-05-14 08:52:08 UTC
So in my understand this is a duplicate of bug 25481.

The error was a bit cryptic however "Unable to load schema 'paths.json'".
Comment 8 Jonathan Druart 2020-05-14 09:02:10 UTC
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.
Comment 9 Nick Clemens 2020-05-14 10:42:31 UTC
Created attachment 104873 [details] [review]
Bug 25482: Explicitly set macros as not shard during tests
Comment 10 Tomás Cohen Arazi 2020-05-14 11:32:52 UTC
Created attachment 104874 [details] [review]
Bug 25482: Explicitly set macros as not shard during tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Jonathan Druart 2020-05-14 12:24:34 UTC
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
Comment 12 Jonathan Druart 2020-05-14 12:26:09 UTC
Tomas, could we improve the error handling and provide a direct hint about which file it problematic?
Comment 13 Tomás Cohen Arazi 2020-05-14 12:43:35 UTC
(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.
Comment 14 Tomás Cohen Arazi 2020-05-14 12:44:07 UTC
(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.
Comment 15 Nick Clemens 2020-05-15 11:33:04 UTC
*** Bug 25494 has been marked as a duplicate of this bug. ***
Comment 16 Martin Renvoize 2020-05-15 11:45:46 UTC
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>
Comment 17 Martin Renvoize 2020-05-15 11:45:50 UTC
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>
Comment 18 Martin Renvoize 2020-05-15 11:45:53 UTC
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>
Comment 19 Martin Renvoize 2020-05-15 11:46:29 UTC
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
Comment 20 Martin Renvoize 2020-05-15 11:47:35 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-05-19 22:27:49 UTC
does not apply cleanly to 19.11.x  please rebase if needed