Bug 24631

+1

Having to load the class is also a potential security breach, as it allows disabled plugins to execute code.

Created attachment 103092 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
1) Apply this patch
2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.34/koha-plugin-kitchen-sink-v2.1.34.kpz
3) Browse to plugins home, note that nothing appears to have changed
4) Browse to your pluginsdir, find the META.yml file for the Kitchen Sink plugin, edit some of the metadata
5) Reload the plugins home page, note your changes to META.yml appear in the plugins table!

+1

Just getting started for the morning, but will put this to the side to look at later.

Thinking about doing more plugin work soon...

Thanks for jumping onto this one Kyle :)

Hi Kyle,

Can you comment on why you chose to name the file META.yml ? Is it related to https://metacpan.org/pod/CPAN::Meta::Spec ?

(In reply to Julian Maurice from comment #5)
> Hi Kyle,
> 
> Can you comment on why you chose to name the file META.yml ? Is it related
> to https://metacpan.org/pod/CPAN::Meta::Spec ?

You are essentially correct. The file name is inspired by CPAN's META.yml/META.json file.

> 3) Browse to plugins home, note that nothing appears to have changed

Except my screen turned orange :o
(first time installing a plugin, and kitchen sink, that was a surprise ^^")

More seriously: Is that actually expected to have the background changed to orange when the plugin is disabled?

> 5) Reload the plugins home page, note your changes to META.yml appear in the plugins table!

I don't see changes. Is the correct file to edit this one?
/var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/META.yml

I'm using koha-testing-docker, I even ran restart_all what could be missing? (git log from the container shows that HEAD is bug 24631)

(In reply to Kyle M Hall from comment #6)
> (In reply to Julian Maurice from comment #5)
> > Hi Kyle,
> > 
> > Can you comment on why you chose to name the file META.yml ? Is it related
> > to https://metacpan.org/pod/CPAN::Meta::Spec ?
> 
> You are essentially correct. The file name is inspired by CPAN's
> META.yml/META.json file.

Won't that be a problem if people want to publish their plugins on CPAN ?

(In reply to Julian Maurice from comment #8)
> (In reply to Kyle M Hall from comment #6)
> > (In reply to Julian Maurice from comment #5)
> > > Hi Kyle,
> > > 
> > > Can you comment on why you chose to name the file META.yml ? Is it related
> > > to https://metacpan.org/pod/CPAN::Meta::Spec ?
> > 
> > You are essentially correct. The file name is inspired by CPAN's
> > META.yml/META.json file.
> 
> Won't that be a problem if people want to publish their plugins on CPAN ?

I was wary of using META.yml for a name due to its CPAN significance, but publishing plugins on CPAN seems problematic.

It seems to me the plugins are their own packages and that the metadata file is part of that packaging scheme, so I don't really see the overlap. 

Like you could publish the module for a Koha plugin on CPAN, but you'd have to download it and package it into a KPZ and at that point you could provide the META.yml file.

That said, maybe META.yml is just too close for comfort. Maybe KMETA.yml or KPMETA.yml? Just to make it a bit more distinctive?

(In reply to David Cook from comment #9)
> That said, maybe META.yml is just too close for comfort. Maybe KMETA.yml or
> KPMETA.yml? Just to make it a bit more distinctive?

KPMETA.yml is just more confusing for me :)
If it is not intended to be used on CPAN, why not choose something completely different, like 'plugin.yml' ?

But there is a bigger problem IMO: in order to get the path of the yaml file, the main plugin module is loaded first, which defeats the purpose of this bug. Isn't there any other way to find the yaml file path ?

I'm happy with whatever name.. META.yml probably came out of a discussion Kyle and I had about using cpan's packaging system for inspiration.. meta.yml, plugin.yml. koha.yml.. I'm not too worried about a name so long as we document it.

As for requiring module load prior to reading the yml.. hmm, does feel like we should be able to get that path info from the plugin base code koha side somehow.. A chunk of this is actually about how we then expose the new meta file next to releases on github (and eventually gitlab) so we can grab that data via api's and display/use it prior to install entirely.

> But there is a bigger problem IMO: in order to get the path of the yaml
> file, the main plugin module is loaded first, which defeats the purpose of
> this bug. Isn't there any other way to find the yaml file path ?

I will submit a followup to avoid 'require'ing the plugin directory.

(In reply to Martin Renvoize from comment #11)
> I'm happy with whatever name.. META.yml probably came out of a discussion
> Kyle and I had about using cpan's packaging system for inspiration..
> meta.yml, plugin.yml. koha.yml.. I'm not too worried about a name so long as
> we document it.
> 
> As for requiring module load prior to reading the yml.. hmm, does feel like
> we should be able to get that path info from the plugin base code koha side
> somehow.. A chunk of this is actually about how we then expose the new meta
> file next to releases on github (and eventually gitlab) so we can grab that
> data via api's and display/use it prior to install entirely.

I'm doing my best to keep Koha backwards compatible with plugins that have no META.yml file. I definitely think we should consider ignoring plugin releases without a META.yml file for searching purposes.

Created attachment 103467 [details] [review]
Bug 24631: Remove the need to 'require' the plugin path

(In reply to Martin Renvoize from comment #11)
> I'm happy with whatever name.. META.yml probably came out of a discussion
> Kyle and I had about using cpan's packaging system for inspiration..
> meta.yml, plugin.yml. koha.yml.. I'm not too worried about a name so long as
> we document it.
> 
> As for requiring module load prior to reading the yml.. hmm, does feel like
> we should be able to get that path info from the plugin base code koha side
> somehow.. A chunk of this is actually about how we then expose the new meta
> file next to releases on github (and eventually gitlab) so we can grab that
> data via api's and display/use it prior to install entirely.

I will submit a followup to rename the file to PLUGIN.yml.

Created attachment 103468 [details] [review]
Bug 24631: Rename META.yml to PLUGIN.yml

(In reply to Martin Renvoize from comment #11)
> As for requiring module load prior to reading the yml.. hmm, does feel like
> we should be able to get that path info from the plugin base code koha side
> somehow.. A chunk of this is actually about how we then expose the new meta
> file next to releases on github (and eventually gitlab) so we can grab that
> data via api's and display/use it prior to install entirely.

The proposed process sounds like how Apt works (ie download and use the metadata before you even see the packaged software), but you're trying to shoehorn it into Github/Gitlab infrastructure.

Looking at https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases, I see the koha-plugin-kitchen-sink-v2.1.39.kpz
and PLUGIN.yml sitting next to each other. That means you could download and verify the metadata without the software, but that would make importing plugins a 2 step process, if you had to import the KPZ and YAML both. It would also be possible to load in the wrong YAML file for a KPZ. 

In terms of importing, it would be easier if the PLUGIN.yml were located within the KPZ file. However, that would require staging and unpacking the file to get the PLUGIN.yml metadata. Not very API friendly and also a bit fiddly at the server level. 

Ok I'm having a thought...

If PLUGIN.yml had a MD5 hash for the KPZ file, that would bind together a PLUGIN.yml file and a KPZ file. (Note that you could cryptographically sign the PLUGIN.yml file to increase security.) 

To import a plugin, you'd first import the PLUGIN.yml, and then you'd import the KPZ file (this could be manual or automated).

For backwards compatibility, you could still import a KPZ file without a PLUGIN.yml. Maybe we could add a system preference for this and slowly deprecate this functionality. 

For the Upload UI, you could noticeably prompt for the PLUGIN.yml, but have a small visible link to "skip" the metadata verification process. 

For the Plugin Management UI, maybe it would have to generate plugin metadata in the absence of the file. It would be less secure, but that's the world that we're living in at the moment, and we could deprecate this insecure functionality over time. 

Alternatively, maybe make Koha so that all new plugins for a system have to have PLUGIN.yml in order to import, but all existing plugins have metadata auto-generated for them (after all, they're already installed in the system). And maybe have a system preference to turn off the PLUGIN.yml requirement if necessary to import old plugins that don't have PLUGIN.yml? That would default to the safer option while allowing backwards compatibility optionally.

How does that sound?

(In reply to David Cook from comment #17)
> (In reply to Martin Renvoize from comment #11)
> > As for requiring module load prior to reading the yml.. hmm, does feel like
> > we should be able to get that path info from the plugin base code koha side
> > somehow.. A chunk of this is actually about how we then expose the new meta
> > file next to releases on github (and eventually gitlab) so we can grab that
> > data via api's and display/use it prior to install entirely.
> 
> The proposed process sounds like how Apt works (ie download and use the
> metadata before you even see the packaged software), but you're trying to
> shoehorn it into Github/Gitlab infrastructure.
> 
> Looking at
> https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases, I see
> the koha-plugin-kitchen-sink-v2.1.39.kpz
> and PLUGIN.yml sitting next to each other. That means you could download and
> verify the metadata without the software, but that would make importing
> plugins a 2 step process, if you had to import the KPZ and YAML both. It
> would also be possible to load in the wrong YAML file for a KPZ. 
> 
> In terms of importing, it would be easier if the PLUGIN.yml were located
> within the KPZ file. However, that would require staging and unpacking the
> file to get the PLUGIN.yml metadata. Not very API friendly and also a bit
> fiddly at the server level. 
> 
> Ok I'm having a thought...
> 
> If PLUGIN.yml had a MD5 hash for the KPZ file, that would bind together a
> PLUGIN.yml file and a KPZ file. (Note that you could cryptographically sign
> the PLUGIN.yml file to increase security.) 
> 
> To import a plugin, you'd first import the PLUGIN.yml, and then you'd import
> the KPZ file (this could be manual or automated).
> 
> For backwards compatibility, you could still import a KPZ file without a
> PLUGIN.yml. Maybe we could add a system preference for this and slowly
> deprecate this functionality. 
> 
> For the Upload UI, you could noticeably prompt for the PLUGIN.yml, but have
> a small visible link to "skip" the metadata verification process. 
> 
> For the Plugin Management UI, maybe it would have to generate plugin
> metadata in the absence of the file. It would be less secure, but that's the
> world that we're living in at the moment, and we could deprecate this
> insecure functionality over time. 
> 
> Alternatively, maybe make Koha so that all new plugins for a system have to
> have PLUGIN.yml in order to import, but all existing plugins have metadata
> auto-generated for them (after all, they're already installed in the
> system). And maybe have a system preference to turn off the PLUGIN.yml
> requirement if necessary to import old plugins that don't have PLUGIN.yml?
> That would default to the safer option while allowing backwards
> compatibility optionally.
> 
> How does that sound?

Those are some good ideas, a lot of which are more in the scope of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24632

Take a look at that bug and file a bug that depends on both of them both that would add the missing pieces!

Retested on latest patches.

> 3) Browse to plugins home, note that nothing appears to have changed

Except my screen turned orange :o

Is that actually expected to have the background changed to orange when the plugin is disabled?

> 5) Reload the plugins home page, note your changes to META.yml appear in the plugins table!

I don't see changes. When editing this file
/var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/
META.yml

But I see changes when editing this file
/var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm

For some reason it's still META.yml .
The container is fresh, patches where applied before starting it. KitchenSink was redownloaded now.

(In reply to Kyle M Hall from comment #18)
> Those are some good ideas, a lot of which are more in the scope of
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24632
> 
> Take a look at that bug and file a bug that depends on both of them both
> that would add the missing pieces!

I've already commented extensively on that bug. At some point, I'm thinking of submitting a patch using Crypt::OpenSSL::RSA or CryptX to handle the crypto side of things. 

Filing a separate bug that depends on both might be a good idea, although it seems prudent to plan the ideal workflow now, rather than trying to bolt more things on later. After all, it seems like we're already running into issues of trying to maintain backwards compatibility. 

But... I haven't gotten into Koha plugin development yet, so I don't have any real agenda at this point.

I'm hoping to develop a Koha plugin soon, but not quite there yet.

(In reply to Victor Grousset/tuxayo from comment #19)
> Retested on latest patches.
> 
> > 3) Browse to plugins home, note that nothing appears to have changed
> 
> Except my screen turned orange :o
> 
> Is that actually expected to have the background changed to orange when the
> plugin is disabled?
> 
> > 5) Reload the plugins home page, note your changes to META.yml appear in the plugins table!
> 
> I don't see changes. When editing this file
> /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/
> META.yml
> 
> But I see changes when editing this file
> /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm
> 
> For some reason it's still META.yml .
> The container is fresh, patches where applied before starting it.
> KitchenSink was redownloaded now.

If you aren't seeing the changes in META.yml, but *are* seeing changes to the metadata in KitchenSink.pm, that means Koha is failing to find the META.yml and is falling back to the original behavior. I'll double check my code and retest.

(In reply to Kyle M Hall from comment #21)
> (In reply to Victor Grousset/tuxayo from comment #19)
> > Retested on latest patches.
> > 
> > > 3) Browse to plugins home, note that nothing appears to have changed
> > 
> > Except my screen turned orange :o
> > 
> > Is that actually expected to have the background changed to orange when the
> > plugin is disabled?
> > 
> > > 5) Reload the plugins home page, note your changes to META.yml appear in the plugins table!
> > 
> > I don't see changes. When editing this file
> > /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/
> > META.yml
> > 
> > But I see changes when editing this file
> > /var/lib/koha/kohadev/plugins/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm
> > 
> > For some reason it's still META.yml .
> > The container is fresh, patches where applied before starting it.
> > KitchenSink was redownloaded now.
> 
> If you aren't seeing the changes in META.yml, but *are* seeing changes to
> the metadata in KitchenSink.pm, that means Koha is failing to find the
> META.yml and is falling back to the original behavior. I'll double check my
> code and retest.

Oof, I think this is because I forgot to update the download link and test plan after renaming from META.yml to PLUGIN.yml. I'll squash those patches and update the test plan.

Created attachment 103656 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
1) Apply this patch
2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
3) Browse to plugins home, note that nothing appears to have changed
4) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
5) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!

Why not do something like this in a plugin with an our $metadata definition:
sub new {
    my ($class, $params) = @_;
    return $class->SUPER::new($params, $metadata);
}
Or when a new style plugin has a manifest file, do:
sub new {
    my ($class, $params) = @_;
    return $class->SUPER::new($params, IO::File->new(FILENAME) );
}
And let Plugin::Base::new handle either the hashref or the filehandle?

This part of GetPlugins is rather debatable:
        load $plugin_class;
        my $plugin = $plugin_class->new({
            enable_plugins => $self->{'enable_plugins'}
                # loads even if plugins are disabled
                # FIXME: is this for testing without bothering to mock config?
        });
We should not execute it at all imo if we did not set enable_plugins. And the override flag enable_plugins here could well be a candidate for security problems.
When we install a plugin, we could save the metadata that the interface needs into plugin_data ? Why read it every time? When we upgrade the plugin, we replace it.
Why load every module on a plain GetPlugins call ?

Perhaps a bit out of scope, but could we have a Koha/Plugin/Example class that contains the absolute minimum code for a plugin to be usable in our codebase, controlled by the regular QA process ?
I know KitchenSink but it contains a lot of examples and is not in the codebase.

And as a final observation, I am seeing lots of methods in the plugin_methods table for an installed class with 2 actual methods.
We probably dont need all the imported stuff from Plugin::Base and its modules?

Created attachment 103722 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
1) Apply this patch
2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
3) Browse to plugins home, note that nothing appears to have changed
4) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
5) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Works :D

I might have found an unrelated bug. When putting a minimum version of Koha superior to the current one. The warning text is wrong.

>  Warning: This report was written for a newer version of Koha. Run at your own risk.

(In reply to Victor Grousset/tuxayo from comment #29)
> Works :D
> 
> I might have found an unrelated bug. When putting a minimum version of Koha
> superior to the current one. The warning text is wrong.
> 
> >  Warning: This report was written for a newer version of Koha. Run at your own risk.

Good catch! Can you file a bug report for that?

Yes, bug 25285

At least comment24 and comment25 need feedback (although I did not add the others for ignoring them ;)
Actually, duplicating the arguable code in GetPlugins to GetPluginsMetaData is no reason imo for automatically going thru QA.

(In reply to Marcel de Rooy from comment #24)
> Why not do something like this in a plugin with an our $metadata definition:
> sub new {
>     my ($class, $params) = @_;
>     return $class->SUPER::new($params, $metadata);
> }
> Or when a new style plugin has a manifest file, do:
> sub new {
>     my ($class, $params) = @_;
>     return $class->SUPER::new($params, IO::File->new(FILENAME) );
> }
> And let Plugin::Base::new handle either the hashref or the filehandle?

One of the purposes here is to allow Koha to get the metadata without instantiating the plugin. I think your suggestions is a good one though, as it would keep the metadata DRY. I'll file a followup bug for this!

(In reply to Marcel de Rooy from comment #25)
> This part of GetPlugins is rather debatable:
>         load $plugin_class;
>         my $plugin = $plugin_class->new({
>             enable_plugins => $self->{'enable_plugins'}
>                 # loads even if plugins are disabled
>                 # FIXME: is this for testing without bothering to mock
> config?
>         });
> We should not execute it at all imo if we did not set enable_plugins. And
> the override flag enable_plugins here could well be a candidate for security
> problems.
> When we install a plugin, we could save the metadata that the interface
> needs into plugin_data ? Why read it every time? When we upgrade the plugin,
> we replace it.
> Why load every module on a plain GetPlugins call ?

I was thinking about this, and the problem is the plugin metadata may change at any time. This is a more complicated problem than I had initially thought. We could A) update the metadata ever time the plugin is loaded, or B) Update the plugin metadata any time InstallPlugins is run. In any case this is outside the scope of this bug and requires a bit of thought before I implement anything.

I think I've changed my mind about making this a followup bug. The new code for based on Marcel's thoughts is much more elegant. I'm going to post the change there for posterity, then close the bug and merge the changes in here as well.

Created attachment 103982 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
1) Apply this patch
2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
3) Browse to plugins home, note that nothing appears to have changed
4) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
5) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!

Created attachment 103983 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
1) Apply this patch
2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
3) Browse to plugins home, note that nothing appears to have changed
4) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
5) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

For some reason I'm not getting any change.

Whether from PLUGIN.yml or KitchenSink.pm

On a fresh koha-testing-docker after restart_all and flush_memcached

The previous version of the patch (when I signed it off) works.

- change PLUGIN.yml and ../KitchenSink.pm in different ways
- checkout to b50d891c099d0f21f2528c5a6d5bbb586cf1612e (master at that time)
- apply the old patch with git am
- restart_all
- changes from PLUGIN.yml are visible :D
- checkout to current master
- apply this bug
- restart_all
- the original version of PLUGIN.yml is visible  ?!?!?!?!?!?!

Even though PLUGIN.yml and ../KitchenSink.pm are changed I still **again** the original version. This is very confusing.

I'm a bit confused.

So you're storing metadata in PLUGIN.yml *and* in plugin_data depending on the scenario? That seems suboptimal.

Why not just use the plugin_data? You could read PLUGIN.yml into plugin_data at install/upgrade time, and then treat the database as the source of truth.

I suppose plugin_data is really more of a key-value data store, so maybe it would be better to have a `plugins` table that stores 1 row per plugin for standard plugin metadata. Then you use that for all plugin management purposes. 

Doing it that way you would have 1 source of truth and you wouldn't have any arbitrary code execution. 

For new plugins, you use PLUGIN.yml at install/upgrade time (after which it becomes irrelevant). For old plugins, you use the insecure method of extracting metadata from the plugin code. Then you eventually deprecate the insecure method.

> So you're storing metadata in PLUGIN.yml *and* in plugin_data depending on the scenario? That seems suboptimal.

I just edited the two sources of data that for debugging.

> and then treat the database as the source of truth.

Oh wait that's also in the DB?! Good, nothing supernatural then ^^"

Confirmed! select * from plugin_data \G

> For new plugins, you use PLUGIN.yml at install/upgrade time (after which it becomes irrelevant)

Step 4 of test plan show that it's meant to be read every time.

So from what I understand, code is working now (before the DB caching was not) but it's the test plan that is wrong.

I'll edit PLUGIN.yml and ../KitchenSink.pm in the kpz to confirm.

It works :)

@Kyle: Is there another way to test than modifying the archive? (so the test plan can be fixed)
It's easy on Linux with file-roller, the GUI for archives that most people have.

Also, bad news, without the patch I can install and uninstall a plugin.
But with the patch, I can install but not uninstall.

> Can't locate .pm in @INC

>    92:     require $plugin_path;   # Require the plugin to have it's path listed in INC

So $plugin_path is undef?

(In reply to Victor Grousset/tuxayo from comment #42)
> It works :)
> 
> @Kyle: Is there another way to test than modifying the archive? (so the test
> plan can be fixed)
> It's easy on Linux with file-roller, the GUI for archives that most people
> have.
> 
> Also, bad news, without the patch I can install and uninstall a plugin.
> But with the patch, I can install but not uninstall.
> 
> > Can't locate .pm in @INC
> 
> >    92:     require $plugin_path;   # Require the plugin to have it's path listed in INC
> 
> So $plugin_path is undef?

Good catch! I'll check on that.

(In reply to David Cook from comment #40)
> I'm a bit confused.
> 
> So you're storing metadata in PLUGIN.yml *and* in plugin_data depending on
> the scenario? That seems suboptimal.
> 
> Why not just use the plugin_data? You could read PLUGIN.yml into plugin_data
> at install/upgrade time, and then treat the database as the source of truth.
> 
> I suppose plugin_data is really more of a key-value data store, so maybe it
> would be better to have a `plugins` table that stores 1 row per plugin for
> standard plugin metadata. Then you use that for all plugin management
> purposes. 
> 
> Doing it that way you would have 1 source of truth and you wouldn't have any
> arbitrary code execution. 
> 
> For new plugins, you use PLUGIN.yml at install/upgrade time (after which it
> becomes irrelevant). For old plugins, you use the insecure method of
> extracting metadata from the plugin code. Then you eventually deprecate the
> insecure method.

In a nutshell, GetPluginsMetadata will look for plugin metadata in the database, and if it's not there, get it from the plugin via get_metadata and store it in the db.

get_metadata looks in various places based on speed of access. First it looks in itself at the metadata key, then the metadata package var, then the database, then the PLUGIN.yml file.

Created attachment 104023 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
 1) Apply this patch
 2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
 3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it
 4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data
 5) Browse to plugins home, note that nothing appears to have changed
 6) Note the metadata in plugin_data has been regenerated
 7) Browse to your pluginsdir, find the KitchenSink.pm file for the Kitchen Sink plugin, edit some of the metadata
 8) Delete the plugin_data metadata again
 9) Reload the plugins home
10) Note the metdata has been updated
11) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead
12) Clear the metadata from plugin_data again
13) Reload the plugins home
14) Note again the metadata is regenerated in plugin_data
15) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
16) Clear the metadata from plugin_data again
17) Reload the plugins home
18) Note again the metadata is regenerated in plugin_data
19) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

I should have added to the test plan:
1) Test that you can run the plugin configuration, enable/disable, the plugin, run the tool and report methods, and uninstall the plugin!

Created attachment 104024 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
 1) Apply this patch
 2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
 3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it
 4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data
 5) Browse to plugins home, note that nothing appears to have changed
 6) Note the metadata in plugin_data has been regenerated
 7) Browse to your pluginsdir, find the KitchenSink.pm file for the Kitchen Sink plugin, edit some of the metadata
 8) Delete the plugin_data metadata again
 9) Reload the plugins home
10) Note the metdata has been updated
11) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead
12) Clear the metadata from plugin_data again
13) Reload the plugins home
14) Note again the metadata is regenerated in plugin_data
15) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
16) Clear the metadata from plugin_data again
17) Reload the plugins home
18) Note again the metadata is regenerated in plugin_data
19) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!
20) Test that you can enable/disable the plugin
21) Test that all the plugin methods ( configure, report, tool ) still work
22) Test that you can uninstall the plugin

By the way, for https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24632, I ended up signing a digest of the KPZ file, so I don't have any concerns about the metadata file affecting that functionality anymore.

>  3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it

Test note:
select plugin_value from plugin_data where plugin_key = '__METADATA__' \G


>  4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data

That's a valid way to do it right? :
delete from plugin_data where plugin_key = '__METADATA__';


> 5) Browse to plugins home, note that nothing appears to have changed

I can see my edits to KitchenSink.pm, that's not expected right?

I tried to continue.

> 6) Note the metadata in plugin_data has been regenerated

Should we expect the original data or the edited version? (from KitchenSink.pm)

>  9) Reload the plugins home

error!

> Can't use string ("HASH(0x55b5ca2beea8)") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Plugins.pm line 180

>   180:         $plugin_metadata->{can}->{ $_->plugin_method } = 1 foreach @plugin_methods;

I don't get what's different from before.

Ok, I have to restart_all after each change to Plugins.pm. I'm restarting over.

>  5) Browse to plugins home, note that nothing appears to have changed

I still the data from the .pm edited on step 4



> 11) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead

«this will cause» IIUC that only a hint about what should happen next. Not an actual check to perform now. Seems clear, moving on.


> 13) Reload the plugins home

error!

> Can't call method "isa" on an undefined value at /kohadevbox/koha/Koha/Plugins/Base.pm line 337

note: I restart_all after the KitchenSink.pm

I did restart_all again and now I'm getting endless of these errors in the logs.

> Use of uninitialized value $bundle_path in concatenation (.) or string at /kohadevbox/koha/Koha/Plugins/Base.pm line 193.
> Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Can't call method "isa" on an undefined value at /kohadevbox/koha/Koha/Plugins/Base.pm line 337.
> Compilation failed in require at (eval 402) line 1.
> Use of uninitialized value $bundle_path in concatenation (.) or string at /kohadevbox/koha/Koha/Plugins/Base.pm line 193.
> Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Can't call method "isa" on an undefined value at /kohadevbox/koha/Koha/Plugins/Base.pm line 337.
> Compilation failed in require at (eval 402) line 1.

Created attachment 104256 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
 1) Apply this patch
 2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
 3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it
 4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data
    Restart Plack
 5) Browse to plugins home, note that nothing appears to have changed
 6) Note the metadata in plugin_data has been regenerated
 7) Browse to your pluginsdir, find the KitchenSink.pm file for the Kitchen Sink plugin, edit some of the metadata
 8) Delete the plugin_data metadata again
    Restart Plack
 9) Reload the plugins home
10) Note the metdata has been updated
11) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead
12) Clear the metadata from plugin_data again
    Restart Plack
13) Reload the plugins home
14) Note again the metadata is regenerated in plugin_data
15) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
16) Clear the metadata from plugin_data again
    Restart Plack
17) Reload the plugins home
18) Note again the metadata is regenerated in plugin_data
19) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!
20) Test that you can enable/disable the plugin
21) Test that all the plugin methods ( configure, report, tool ) still work
22) Test that you can uninstall the plugin

(In reply to Victor Grousset/tuxayo from comment #52)
> I did restart_all again and now I'm getting endless of these errors in the
> logs.
> 
> > Use of uninitialized value $bundle_path in concatenation (.) or string at /kohadevbox/koha/Koha/Plugins/Base.pm line 193.
> > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Can't call method "isa" on an undefined value at /kohadevbox/koha/Koha/Plugins/Base.pm line 337.
> > Compilation failed in require at (eval 402) line 1.
> > Use of uninitialized value $bundle_path in concatenation (.) or string at /kohadevbox/koha/Koha/Plugins/Base.pm line 193.
> > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": Can't call method "isa" on an undefined value at /kohadevbox/koha/Koha/Plugins/Base.pm line 337.
> > Compilation failed in require at (eval 402) line 1.

I found an issue with my patch and my test plan was incomplete. Plack needs to be restarted after clearing the plugins data table because Plack keeps the plugin modules in memory. Deleting the plugin data and restarting Plack at the same time forces Koha to fetch the metadata from the original source, be it the plugin module or a PLUGIN.yml file.

Please give it another try!

 5) Browse to plugins home, note that nothing appears to have changed

Noooooo I see my changes from KitchenSink.pm . Which isn't right, right?

I applied the patch, restarted plack. I don't see much what could have been forgotten.

This bug is cursed >_<

Created attachment 104266 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
 1) Apply this patch
 2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
 3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it
 4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data
    Restart Plack
 5) Browse to your pluginsdir, find the KitchenSink.pm file for the Kitchen Sink plugin, edit some of the metadata
 6) Delete the plugin_data metadata again
    Restart Plack
 7) Reload the plugins home
 8) Note the metdata has been updated
 9) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead
10) Clear the metadata from plugin_data again
    Restart Plack
11) Reload the plugins home
12) Note again the metadata is regenerated in plugin_data
13) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
14) Clear the metadata from plugin_data again
    Restart Plack
15) Reload the plugins home
16) Note again the metadata is regenerated in plugin_data
17) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!
18) Test that you can enable/disable the plugin
19) Test that all the plugin methods ( configure, report, tool ) still work
20) Test that you can uninstall the plugin

(In reply to Victor Grousset/tuxayo from comment #55)
>  5) Browse to plugins home, note that nothing appears to have changed
> 
> Noooooo I see my changes from KitchenSink.pm . Which isn't right, right?
> 
> I applied the patch, restarted plack. I don't see much what could have been
> forgotten.
> 
> This bug is cursed >_<

Heh, I have no idea what I was thinking when I wrote that part of the test plan! If you delete the metadata in the db, and restart plack, *of course* the metadata will get updated from your altered plugin! I've updated the test plan to remove that part.

> Heh, I have no idea what I was thinking when I wrote that part of the test plan! If you delete the metadata in the db, and restart plack, *of course* the metadata will get updated from your altered plugin!

I expected it would be from PLUGIN.yml
Ok, so retesting!

Created attachment 104270 [details] [review]
Bug 24631: Plugin metadata should be outside the main class

Currently, plugin metadata is stored within the main plugin class file but this means one has to load the class to read it.  We should pull this data out into a manifest/metadata file in a standard format (JSON/YAML) to allow simpler external parsing.

Test Plan:
 1) Apply this patch
 2) Install the latest Kitchen Sink plugin: https://github.com/bywatersolutions/koha-plugin-kitchen-sink/releases/download/v2.1.39/koha-plugin-kitchen-sink-v2.1.39.kpz
 3) Look in plugin_data for the new "__METADATA__" key, note it has the metadata in it
 4) Alter the metadata in KitchenSink.pm, delete __METADATA__ from plugin_data
    Restart Plack
 5) Browse to your pluginsdir, find the KitchenSink.pm file for the Kitchen Sink plugin, edit some of the metadata
 6) Delete the plugin_data metadata again
    Restart Plack
 7) Reload the plugins home
 8) Note the metdata has been updated
 9) Remove lines 28-40, 47-49 from KitchenSink.pm, this will cause Koha to look at PLUGIN.yml instead
10) Clear the metadata from plugin_data again
    Restart Plack
11) Reload the plugins home
12) Note again the metadata is regenerated in plugin_data
13) Browse to your pluginsdir, find the PLUGIN.yml file for the Kitchen Sink plugin, edit some of the metadata
14) Clear the metadata from plugin_data again
    Restart Plack
15) Reload the plugins home
16) Note again the metadata is regenerated in plugin_data
17) Reload the plugins home page, note your changes to PLUGIN.yml appear in the plugins table!
18) Test that you can enable/disable the plugin
19) Test that all the plugin methods ( configure, report, tool ) still work
20) Test that you can uninstall the plugin

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

> 19) Test that all the plugin methods ( configure, report, tool ) still work

Report doesn't work or maybe I don't know how to use it.

Whatever, it has surely nothing to do with this.

In conclusion: ﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑

(In reply to Victor Grousset/tuxayo from comment #58)
> > Heh, I have no idea what I was thinking when I wrote that part of the test plan! If you delete the metadata in the db, and restart plack, *of course* the metadata will get updated from your altered plugin!
> 
> I expected it would be from PLUGIN.yml
> Ok, so retesting!

As an FYI, the idea here is to check the database for the metadata to avoid loading the plugin. If it's not in the database, we load the plugin and ask it for its' metadata. When we as a plugin for it's metadata it checks
A) For metadata in it's own Perl module ( fastest )
B) Checks the database ( second fastest, for convenience when the plugin is already loaded, is skipped when upgrading plugins )
C) The PLUGIN.yml file if it exists ( slowest, but these speeds are all relative to each other ;)

(In reply to Victor Grousset/tuxayo from comment #60)
> > 19) Test that all the plugin methods ( configure, report, tool ) still work
> 
> Report doesn't work or maybe I don't know how to use it.
> 
> Whatever, it has surely nothing to do with this.
> 
> In conclusion: ﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑﷐[U+1F389]﷑

Hooray! Thanks Victor!

(In reply to Julian Maurice from comment #8)
> Won't that be a problem if people want to publish their plugins on CPAN ?

I've been thinking about this more, and while I was skeptical at first... I actually kind of like the idea now.

For vendors, it could be easier to provide plugins via the system rather than at an instance basis.

Coupled with a whitelist or some other "enable/disable" system per instance, it could make plugin management easier.

(In reply to Kyle M Hall from comment #61) 
> As an FYI, the idea here is to check the database for the metadata to avoid
> loading the plugin. If it's not in the database, we load the plugin and ask
> it for its' metadata. When we as a plugin for it's metadata it checks
> A) For metadata in it's own Perl module ( fastest )
> B) Checks the database ( second fastest, for convenience when the plugin is
> already loaded, is skipped when upgrading plugins )
> C) The PLUGIN.yml file if it exists ( slowest, but these speeds are all
> relative to each other ;)

I am not sure if I understood that part correctly. You are saying that plugin's metadata can be in 3 different places (Perl module, database, and YAML file) now ? And in order to read the YAML file we need to load the module first ? What are the benefits ?

You are talking about performance of different methods. Have you measured it ? Do we really gain that much by storing metadata in MySQL ? Have you considered caching metadata in memcache ?

I thought that the yaml file would become the only source of truth and that it would allow us to avoid loading the Perl module when not needed (especially for disabled modules).

I'm sorry but I really don't see what we gain here. If this is for easing parsing by external tools, then you can just dump the Perl module's metadata into a YAML file when you create the .kpz

Moving to Failed QA to get feedback