+1, love this.

To Test:
Testing will be easiest if you perform an installation from scratch. This patch creates a new directory ( for a dev install, it's koha-dev/var/plugins ), and modifies both the koha-httpd.conf and koha-conf.xml files. The plugins directory also needs to be writable by the apache user ( www-data ), or uploading of plugins will fail. Make sure to restart apache so the changes will take effect.

1) Go to the Plugins page via the More pulldown on the nav bar
2) Add the plugins permissions for your account
3) Upload the example plugins
4) Test the plugins
5) Check the plugins links on the reports page and the tools page

A couple more comments: I mis-stated the plugins dir for a dev install, it is koha-dev/var/lib/plugins.

Second, I have uploaded to example plugins, on is a tool, the other is a report. Both should show up on the plugins home page, but only the tool show show in the tool plugins page, and only the report in the report plugins page, respectively.

Updated the patch to add plugin versioning. Adds the fields version for the plugin version and maximum_version for the max version of Koha the plugin will work with. Probably won't be used often, but it doesn't hurt to have it.

QA Comment:
A lot of tabulation characters are present in these 2 patches.
Could you resubmit them by replacing tab with 4 spaces please ?

I've done some more thinking about the security implications of the plugin system. I think the following are required for this to be sufficiently secure for use:
* Both a syspref *and* a configuration setting in koha-conf.xml should be required to enable plugins, and both should be disabled by default.
* Even when plugins are enabled, a separate koha-conf.xml configuration setting should be required to enable upload from the staff client.
* The plugins should be loaded with Module::Load::Conditional, and each type of plugin (Tools, Reports, etc.) should use a different interface. e.g. Koha::Plugin::Foobar->run_report() and Koha::Plugin::Foobar->run_tool()
* The plugin host should enable mandatory taint checking.
* Under no circumstances should the plugin feature add a +Exec directory.

(In reply to comment #21)
> I've done some more thinking about the security implications of the plugin
> system. I think the following are required for this to be sufficiently
> secure for use:
> * Both a syspref *and* a configuration setting in koha-conf.xml should be
> required to enable plugins, and both should be disabled by default.
> * Even when plugins are enabled, a separate koha-conf.xml configuration
> setting should be required to enable upload from the staff client.
> * The plugins should be loaded with Module::Load::Conditional, and each type
> of plugin (Tools, Reports, etc.) should use a different interface. e.g.
> Koha::Plugin::Foobar->run_report() and Koha::Plugin::Foobar->run_tool()
> * The plugin host should enable mandatory taint checking.
> * Under no circumstances should the plugin feature add a +Exec directory.

I can understand the configuration setting requirements. However, I cannot understand how to use Module::Load::Conditional, even after looking at the example patch you pointed me to. I don't feel it is quite fair to require the use of a poorly documented perl module. Can you please justify these requirements?

(In reply to comment #22)
> (In reply to comment #21)
> > I've done some more thinking about the security implications of the plugin
> > system. I think the following are required for this to be sufficiently
> > secure for use:
> > * Both a syspref *and* a configuration setting in koha-conf.xml should be
> > required to enable plugins, and both should be disabled by default.
> > * Even when plugins are enabled, a separate koha-conf.xml configuration
> > setting should be required to enable upload from the staff client.
> > * The plugins should be loaded with Module::Load::Conditional, and each type
> > of plugin (Tools, Reports, etc.) should use a different interface. e.g.
> > Koha::Plugin::Foobar->run_report() and Koha::Plugin::Foobar->run_tool()
> > * The plugin host should enable mandatory taint checking.
> > * Under no circumstances should the plugin feature add a +Exec directory.
> 
> I can understand the configuration setting requirements. However, I cannot
> understand how to use Module::Load::Conditional, even after looking at the
> example patch you pointed me to. I don't feel it is quite fair to require
> the use of a poorly documented perl module. Can you please justify these
> requirements?

You have read  http://search.cpan.org/dist/Module-Load-Conditional/lib/Module/Load/Conditional.pm eh? I don't find it particularly badly documented. ABd it is part of Perl core, so is not a new dependency

Module::Load::Conditional is but one way of solving a loading modules on the fly. I am sure you understand why an +Exec directory is a bad idea, and why using taint is a good one. As well as being off by default and unable to be turned on via the web is a good one.

(In reply to comment #22)
> (In reply to comment #21)
> > I've done some more thinking about the security implications of the plugin
> > system. I think the following are required for this to be sufficiently
> > secure for use:
> > * Both a syspref *and* a configuration setting in koha-conf.xml should be
> > required to enable plugins, and both should be disabled by default.
> > * Even when plugins are enabled, a separate koha-conf.xml configuration
> > setting should be required to enable upload from the staff client.
> > * The plugins should be loaded with Module::Load::Conditional, and each type
> > of plugin (Tools, Reports, etc.) should use a different interface. e.g.
> > Koha::Plugin::Foobar->run_report() and Koha::Plugin::Foobar->run_tool()
> > * The plugin host should enable mandatory taint checking.
> > * Under no circumstances should the plugin feature add a +Exec directory.
> 
> I can understand the configuration setting requirements. However, I cannot
> understand how to use Module::Load::Conditional, even after looking at the
> example patch you pointed me to. I don't feel it is quite fair to require
> the use of a poorly documented perl module. Can you please justify these
> requirements?

You can also use Module::Load if you'd prefer to handle error conditions yourself.

As for why I think this is necessary, consider a plugin which is nothing but a one-line forkbomb. Having an executable file doesn't even require someone to follow an API. They can simply download one of the gazillion of examples of how to take down your server with one line (just, what, 9 characters if you're using bash?), zip it up with your example ini file, and bring down the server. Or a plugin which actually just contains a command line script for reinitializing your Koha database for testing. Accidentally zip that up with your plugin, have someone connect to it (and there's no need for authentication to access a plugin, notice!), and your production server is pristine. Like the cheese shop, it is very clean.

> As for why I think this is necessary, consider a plugin which is nothing but
> a one-line forkbomb. Having an executable file doesn't even require someone
> to follow an API. They can simply download one of the gazillion of examples
> of how to take down your server with one line (just, what, 9 characters if
> you're using bash?), zip it up with your example ini file, and bring down
> the server. Or a plugin which actually just contains a command line script
> for reinitializing your Koha database for testing. Accidentally zip that up
> with your plugin, have someone connect to it (and there's no need for
> authentication to access a plugin, notice!), and your production server is
> pristine. Like the cheese shop, it is very clean.

I understand your examples, but I feel like this is more of a buyer beware issue. If you are uploading random plugins to your system without vetting them first, then of course you will have problems, Module::Load::Conditional or not!

The design is meant to have plugins deal with their own authentication. My examples do indeed use C4::Auth::checkauth for authentication.

After examining the issues involved, I just feel that this requirement is very very onerous, and would require huge amounts of time to understand and implement.

If you could please show me how Module::Load::Conditional would prevent any of your scenarios from taking place, I may reconsider my position. However, as it stands, even with said implementation, I could easily write a plugin to perform any of the damaging tasks you have mentioned.

What I would like to see is a plugin repository that contains only plugins that have been uploaded to bugs.koha-community.com so that they may be vetted and qa'ed just like any other modification to Koha. If a library wishes to use  plugins from elsewhere, it is up to themselves to vet those plugins before use, or to trust the source itself.

I understand that time is a limited resource for everyone ( including myself ), but if you feel strongly enough about this issue, I would welcome a followup patch to add this.

I've been thinking about this issue all night. I can understand your motivations and thinking behind the necessity of using Module::Load::Conditional. Could you describe the changes that would need to be made in order to implement it correctly? Here is the current workflow:

1) A plugin is uploaded to the plugins directory.
2) The plugins.ini is parsed, this tells which which about the starting file only.
3) Koha gives a link to the plugin starting file.
4) The file is executed, is authenticates itself, and loads its template files independently of the internal Koha templating system ( TT is used for templates only because we can be sure TT is available, and also it is simply better than other templating systems, though a plugin could just as easily output html from the perl file or use HTML::Template ).
5) At this point, control is passed completely to the plugin, which is responsible for any subsequent perl file execution.

Step 5 is where things will definitely change. As it stands, step 4 can be modified to use M:L:C easily, but step 5, not so much. I think step 5 will involve the plugin itself calling M:L:C via Koha::Plugins to continue its execution.

If you could describe the workflow necessary for implementing M:L:C, it would help me out greatly.

(In reply to comment #26)
> I've been thinking about this issue all night. I can understand your
> motivations and thinking behind the necessity of using
> Module::Load::Conditional. Could you describe the changes that would need to
> be made in order to implement it correctly? Here is the current workflow:

Let's assume that we're going to be handling a reports plugin called "CoolStuff". I'm going to describe the workflow using Module::Load::Conditional. Everything could be done with just Module::Load, but that would require more work.

> 1) A plugin is uploaded to the plugins directory.
This will be unchanged, except the plugins directory wouldn't be +Exec, or even directly accessible via Apache.

> 2) The plugins.ini is parsed, this tells which which about the starting file
> only.
There are two options here. For option 1, this would work basically the same, though the exact meaning of of the "start" parameter would change slightly. Option 2 might be better though. In this case, we don't actually need the start parameter at all, because we require a consistent Perl interface, so our plugin implements Koha::Plugin::CoolStuff::Dispatcher, and the plugin manager knows that we always start with the dispatcher. I'm going to assume we're going with option 2 because it's easier to implement.

> 3) Koha gives a link to the plugin starting file.
This is where things start *really* changing. Koha gives a link to /plugin/run_report.pl?plugin=CoolStuff, and when run_report.pl is called, it uses Module::Load::Conditional to load Koha::Plugin::CoolStuff::Main, and create
my $plugin = Koha::Plugin::CoolStuff::Main->new()

[this is where initialization code could get called]

> 4) The file is executed, is authenticates itself, and loads its template
> files independently of the internal Koha templating system ( TT is used for
> templates only because we can be sure TT is available, and also it is simply
> better than other templating systems, though a plugin could just as easily
> output html from the perl file or use HTML::Template ).
Rather than executing the file directly, /plugin/run_report.pl runs plugin->run_report(). This means that the plugin author doesn't have to worry about authentication. If it's required, it's already been done by the plugin host. The best way to handle non-Perl resources would be to have everything pulled from /plugin/resource.pl?plugin=CoolStuff&resource=image.png [and a TT plugin could make translating "image.png" to "/plugin/resource.pl?plugin=CoolStuff&resource=image.png" invisible to the user... this step would be optional, but it'd be a lot easier to manage things this way]

> 5) At this point, control is passed completely to the plugin, which is
> responsible for any subsequent perl file execution.
> 
> Step 5 is where things will definitely change. As it stands, step 4 can be
> modified to use M:L:C easily, but step 5, not so much. I think step 5 will
> involve the plugin itself calling M:L:C via Koha::Plugins to continue its
> execution.
Yeah, I think this would be the easiest way to deal with things. The plugin will extend Koha::Plugin::Report (and if a plugin implemented both a Report and a Tool, it could extend Koha::Plugin::Tool, too) but use Koha::Plugin::Host, so that it could do something like
my $cooler = Koha::Plugin::Host->load('CoolStuff', 'CoolerClass');
Then it makes use of $cooler in whatever way is necessary to implement the report functionality. For simple plugins, everything could be implemented in Koha::Plugin::CoolStuff::Main, and there wouldn't be any need for using Koha::Plugin::Host->load() from within the plugin. For fiendishly complex plugins, you could have a massive number of classes.

> If you could describe the workflow necessary for implementing M:L:C, it
> would help me out greatly.

Does this help clarify? In response to your question about how using M:L:C would reduce security issues, there is obviously no way to prevent someone installing dangerous code, but by using M:L:C we close a few of the easiest vectors and make it so that bugs in a plugin are less likely to give an attacker complete access to the server.

It sounds like the basic problem is that with plugins, we don't have any control over what the heck happens.  A plugin could do anything, from a nice fancy report to emailing every patron's personal information to a remote server, then deleting the database.  The only way to prevent this is to block plugins from running arbitrary code, and only accessing things through a controlled and authenticated API.  Even then, unintended stuff can happen (like deleting all your records, or emailing someone something you shouldn't).

It comes down to trust; who do you trust to put code on your system?  Right now, the Koha community and your sysadmin are the folks with that power.  This patch opens that up to anyone with the 'upload plugins' permission.  First order of business is confirming that the patch only goes that far, and doesn't have any holes that can allow other people to run code.

One possible example: if a user has permission to edit permissions, they can basically do anything, including edit themselves to be able to upload and run plugins.  Your circle of trust now has to expand to everyone that has this power, too.

What about making the GUI upload function of the plugin interface something that can be deactivated?  Then, only the sysadmin could upload and install plugins.  This is a person you already trust, and who (likely) is concerned with security, and will do some of that vetting for you before installing something that could be potentially harmful.

I like the idea of being able to create new Reports and Tools pages to do various cool things without having to make them part of Koha proper.  It gets us around a lot of the Quote123 problem.  But until we can provide a clear, secure API for Koha functions, or a serious of hooks/filters like Wordpress, I think user-installable code is going to be a risky proposition.  But, with a small modification to restrict code uploads to sysadmins only, we can bypass a lot of the risk with less overall work than a complete M:V:C reimplementation.

(In reply to comment #25)
> > As for why I think this is necessary, consider a plugin which is nothing but
> > a one-line forkbomb. Having an executable file doesn't even require someone
> > to follow an API. They can simply download one of the gazillion of examples
> > of how to take down your server with one line (just, what, 9 characters if
> > you're using bash?), zip it up with your example ini file, and bring down
> > the server. Or a plugin which actually just contains a command line script
> > for reinitializing your Koha database for testing. Accidentally zip that up
> > with your plugin, have someone connect to it (and there's no need for
> > authentication to access a plugin, notice!), and your production server is
> > pristine. Like the cheese shop, it is very clean.
> 
> I understand your examples, but I feel like this is more of a buyer beware
> issue. If you are uploading random plugins to your system without vetting
> them first, then of course you will have problems, Module::Load::Conditional
> or not!

Speaking as a sysadmin, I never run software with known security holes such as this one. Its just another hole-in-the-wall I have to try to be conscious of amongst the zillion or so I'm not even aware of. Producing code with security holes in ignorance is forgivable; producing code with security holes in full knowledge borders on the unethical imho.

(In reply to comment #27)

So, from your description, it seems that plugins will be entirely OOP. For M:L:C to work, we will need to add the plugins directory to the perl include path, correct? What would be the safest way of doing that?

Let take the Patrons List report, it's "step 1" class name would be Koha::Plugins::Org::CCFLS::PatronsList::Step1 and would be stored at $pluginsdir/Koha/Plugins/Org/CCFLS/PatronsList/Step1.pm, with templates and additional file in sub-directories along side Step1.pm

Also, how can we pass cgi variables between the steps? I imagine since everything will pass through run_report.pl, it can capture the variables, pass them to Koha::Plugins which can then pass them to the plugin via the new() method. Does that make sense?

Example Skeleton Code:

package Koha::Plugins::Org::CCFLS::PatronsList::Step1;

use Modern::Perl;

sub new {
    my $class = shift;
    my $self = {};
    bless $self, $class;
    return $self;
}

sub main {
  # Get data
  # Do something useful
  # Output template
}

1;

(In reply to comment #30)
> (In reply to comment #27)
> 
> So, from your description, it seems that plugins will be entirely OOP. For
> M:L:C to work, we will need to add the plugins directory to the perl include
> path, correct? What would be the safest way of doing that?

The solution here is probably to use File::Find (http://perldoc.perl.org/File/Find.html) or File::Find::Rule (http://search.cpan.org/~rclamp/File-Find-Rule-0.33/lib/File/Find/Rule.pm)

Once we capture the subdir structure, the include path can be modified on the fly.

(In reply to comment #30)
> (In reply to comment #27)
> 
> So, from your description, it seems that plugins will be entirely OOP. For
> M:L:C to work, we will need to add the plugins directory to the perl include
> path, correct? What would be the safest way of doing that?

That's the way I'd go with it, yeah. I believe to add a directory to the Perl include path we just have to push the directory to @INC: push @INC, $pluginpath;

> Let take the Patrons List report, it's "step 1" class name would be
> Koha::Plugins::Org::CCFLS::PatronsList::Step1 and would be stored at
> $pluginsdir/Koha/Plugins/Org/CCFLS/PatronsList/Step1.pm, with templates and
> additional file in sub-directories along side Step1.pm
> 
> Also, how can we pass cgi variables between the steps? I imagine since
> everything will pass through run_report.pl, it can capture the variables,
> pass them to Koha::Plugins which can then pass them to the plugin via the
> new() method. Does that make sense?

Yeah, we can just store everything in $self, and have run_report/run_tool pass all the CGI variables through.

> Example Skeleton Code:
> 
> package Koha::Plugins::Org::CCFLS::PatronsList::Step1;
> 
> use Modern::Perl;
> 
> sub new {
>     my $class = shift;
>     my $self = {};
>     bless $self, $class;
>     return $self;
> }
> 
> sub main {
>   # Get data
>   # Do something useful
>   # Output template
> }
> 
> 1;

That's exactly what I had in mind.

The latest iteration looks a lot better- it closes a couple of the obvious security holes, though there's an unnecessary ScriptAlias line in the httpd.conf that needs to be removed.

Here are a couple of comments:
* Plugins should extend the Koha::Plugins::Base class, so the run() routine you have in Koha::Plugins::Base should probably be moved to a new class, like Koha::Plugins::Handler
* Koha::Plugins::Base needs to save the contents of %args- my inclination would be to just do something simple like: my $self = bless %args, $class;
* When Koha::Plugins::Handler passes control over to the plugin, it should also pass a reference to itself, so that the plugin could call Koha::Plugins::Handler->load_plugin($pluginname), or any other helper functions that we might find are needed
* checkauth should be run in Koha::Plugins::Handler->run($plugin), which means that one of the parameters passed to every plugin should be a CGI object (this also takes care of getting CGI params to the plugin)
* There still need to be separate flags in koha-conf.xml to enable the plugin system generally and uploading plugins from the staff client. The default should be disabled for both.

Addressing these six points should take us much of the way toward a safe implementation

My work on the theming code has gotten me thinking about this again. I really like the idea, and I had a thought about how it might be possible to use plugins as a route to inclusion in Koha proper: have the plugin host export routines with the same signatures as C4::Output, then use those routines in the plugins (but still run checkauth prior to passing control over to the plugin, and capture STDOUT).

Any new, non-core functionality could be implemented as a plugin, and then incorporated into master once the kinks have been worked out. It would also be fairly trivial to (based on the work in bug 7417 and bug 8209) add suggestion and filter plugins at some point in the future.

(In reply to comment #35)
> My work on the theming code has gotten me thinking about this again. I
> really like the idea, and I had a thought about how it might be possible to
> use plugins as a route to inclusion in Koha proper: have the plugin host
> export routines with the same signatures as C4::Output, then use those
> routines in the plugins (but still run checkauth prior to passing control
> over to the plugin, and capture STDOUT).
> 
> Any new, non-core functionality could be implemented as a plugin, and then
> incorporated into master once the kinks have been worked out. It would also
> be fairly trivial to (based on the work in bug 7417 and bug 8209) add
> suggestion and filter plugins at some point in the future.

That sounds like a pretty awesome idea! I need to revisit this patch and get it shored up. Feel free to contribute whatever would be necessary to get your concept to work.

Here is a new edition of the Koha plugin system!

Test plan:
1) Apply patch
2) Run updatedatabase.pl
3) Create the directory /var/lib/koha/plugins
4) Add the line <pluginsdir>/var/lib/koha/plugins</pluginsdir>
5) Access the plugins system from the "More" pulldown
6) Upload the example plugin file provided here
7) Try it out!

Things that still need to be done:
1) Add a use_plugins parameter to the koha-conf.xml file
2) Add some authentication so that only authorized users can use tools and/or reports
3) Add another permission for configuring plugins
4) Add a way to uninstall plugins

Also, delete unused Perl dependencies from the patch. The revamp of this feature ended with me not using many of the modules the previous iteration used.

(In reply to comment #40)
> Test plan:

Don't forget to put the test plan in the commit message.

Kyle,

Great Job! I really like the idea behind this patch and this new rewrite is more elegant.

Just 2 short remarks after my first tests:

- Module::Bundled::Files is required (installed via cpan) and is not in the perl deps
- The enable_plugins entry in the koha-conf file is not in the test plan

Some little stuffs (but this feature is in discussion so I don't know if it is relevant):
- 8 spaces in tool-plugins.tt
- the ERROR_NO_TOOLS in never set
- $version in Plugins.pm is never used
- Plugins.pm:
            if ( defined($method) && $plugin->can($method) ) {
                push( @plugins, $plugin );
            } else {
                push( @plugins, $plugin );
            }

the tools/tool-plugins is completely broken (TOOLS_LOOP in tt vs plugins in pl), I suspect you did not commit them on purpose.

qa tools complains on 3 small fails:
 * Koha/Plugins.pm                                                          FAIL
  pod                         FAIL
    *** WARNING: Verbatim paragraph in NAME section  in file Koha/Plugins.pm
    *** ERROR: =item without previous =over  in file Koha/Plugins.pm
 * Koha/Plugins/Base.pm                                                     FAIL
  pod                         FAIL
    *** ERROR: =back without previous =over  in file Koha/Plugins/Base.pm
    *** ERROR: empty =head2  in file Koha/Plugins/Base.pm
 * plugins/plugins-upload.pl                                                FAIL
  critic                      FAIL
    Variable declared in conditional statement at line 62, column 5. Declare variables outside of the condition.

(In reply to comment #49)
> - The enable_plugins entry in the koha-conf file is not in the test plan

"1) Add a use_plugins parameter to the koha-conf.xml file"

Yes it is!

Created attachment 14531 [details]
Example Koha Plugin file

Jonathan, thanks for the input, it's all been taken care of. This latest patch is, I believe, feature complete and ready for testing!

Also, there are 4 new plugin related permissions. One for installing/uninstalling plugins, one for configuring plugins and one each for running tool plugins and running report plugins.

(In reply to comment #54)
> Also, there are 4 new plugin related permissions. One for
> installing/uninstalling plugins, one for configuring plugins and one each
> for running tool plugins and running report plugins.

Now it runs. But you must set permissions even as superlibrarian.

Install/Uninstall/Configure ok.

Image in example is not showing (/plugin/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/logo.png gives error)

When run tool/report or configure it mangles (example) branch name:
  Institut Protestant de Théologie
becomes
  Institut Protestant de Théologie

(In reply to comment #54)
> Also, there are 4 new plugin related permissions. One for
> installing/uninstalling plugins, one for configuring plugins and one each
> for running tool plugins and running report plugins.

The problem with permissions was my error, only need to save permissions again.

> Image in example is not showing
> (/plugin/Koha/Plugin/Com/ByWaterSolutions/KitchenSink/logo.png gives error)

Did you add the code to your apache config and copy over the .htaccess file?
 
> When run tool/report or configure it mangles (example) branch name:
>   Institut Protestant de Théologie
> becomes
>   Institut Protestant de Théologie

That is something we've run into before. I wish I could remember on which bug...

Applied last patch 
(perhaps you could put INSERT IGNORE in updatedatabase.pl)

Install/Upload/Uninstall works
ByWater logo shows, my new name is Bob, patron list is correct.

But branch name still mangled, and koha-qa still complains.

(In reply to comment #60)
> Applied last patch 
> (perhaps you could put INSERT IGNORE in updatedatabase.pl)
> 
> Install/Upload/Uninstall works
> ByWater logo shows, my new name is Bob, patron list is correct.
> 
> But branch name still mangled, and koha-qa still complains.

I should not that the name mangling is a problem with the example plugin, and not the plugin system itself. I'll see what I can do about it though.

(In reply to comment #61)
> (In reply to comment #60)
> > Applied last patch 
> > (perhaps you could put INSERT IGNORE in updatedatabase.pl)
> > 
> > Install/Upload/Uninstall works
> > ByWater logo shows, my new name is Bob, patron list is correct.
> > 
> > But branch name still mangled, and koha-qa still complains.
> 
> I should not that the name mangling is a problem with the example plugin,
> and not the plugin system itself. I'll see what I can do about it though.

At this point I cannot quite explain the name mangling. The strings should be properly encoded by C4::Template::output

(In reply to comment #67)
> Created attachment 14545 [details] [review]
> [SIGNED-OFF] Bug 7804 - Add Koha Plugin System
Missed comment:

Plugin system works as described.
Example plugin works
No error using koha-qa.pl and unit tests.

We are definitely getting closer. However, I have found some issues that will need to be addressed:
1) Koha::Plugins cannot be used without a koha-conf.xml file that is configured to enable plugins. That's a problem! The check needs to be done at run-time rather than compile time.
2) The unit test in t/Plugins.t is data dependent. It would be much better to mock the configuration data. Barring that, the test should be moved to t/db_dependent.
3) The test adds '.' to @INC. Unfortunately, tests are not usually run from t/, so it always fails when I run prove t/. The solution is to get the directory component from __FILE__
4) The test fails spectacularly for me:
jcamins@kohadev:~/kohaclone/t$ perl Plugins.t                                                      
1..15
ok 1 - use Koha::Plugins;
ok 2 - use Koha::Plugins::Handler;
not ok 3 - use Koha::Plugins::Base;
#   Failed test 'use Koha::Plugins::Base;'
#   at Plugins.t line 18.#     Tried to use 'Koha::Plugins::Base'.
#     Error:  Undefined subroutine &main::/home/jcamins/koha-dev/plugins called at (eval 37) line 2.
#       ...propagated at /usr/share/perl/5.14/base.pm line 93.
# BEGIN failed--compilation aborted at /usr/share/perl5/Authen/CAS/Client/Response.pm line 68.
# Compilation failed in require at /usr/share/perl5/Authen/CAS/Client.pm line 8.
# BEGIN failed--compilation aborted at /usr/share/perl5/Authen/CAS/Client.pm line 8.
# Compilation failed in require at /home/jcamins/kohaclone/C4/Auth_with_cas.pm line 26.
# BEGIN failed--compilation aborted at /home/jcamins/kohaclone/C4/Auth_with_cas.pm line 26.
# Compilation failed in require at /home/jcamins/kohaclone/C4/Auth.pm line 54.
# BEGIN failed--compilation aborted at /home/jcamins/kohaclone/C4/Auth.pm line 63.
# Compilation failed in require at /home/jcamins/kohaclone/Koha/Plugins/Base.pm line 27.
# BEGIN failed--compilation aborted at /home/jcamins/kohaclone/Koha/Plugins/Base.pm line 27.
# Compilation failed in require at (eval 34) line 2.
# BEGIN failed--compilation aborted at (eval 34) line 2.
not ok 4 - use Koha::Plugin::Test;
#   Failed test 'use Koha::Plugin::Test;'
#   at Plugins.t line 19.
#     Tried to use 'Koha::Plugin::Test'.
#     Error:  Attempt to reload Koha/Plugins/Base.pm aborted.
# Compilation failed in require at (eval 39) line 2.
#       ...propagated at /usr/share/perl/5.14/base.pm line 93.
# BEGIN failed--compilation aborted at Koha/Plugin/Test.pm line 7.
# Compilation failed in require at (eval 38) line 2.
# BEGIN failed--compilation aborted at (eval 38) line 2.
not ok 5 - Test can_load
#   Failed test 'Test can_load'
#   at Plugins.t line 22.
Can't locate object method "new" via package "Koha::Plugin::Test" at Plugins.t line 24.
# Looks like you planned 15 tests but ran 5.
# Looks like you failed 3 tests of 5 run.
# Looks like your test exited with 2 just after 5.

> 1) Koha::Plugins cannot be used without a koha-conf.xml file that is
> configured to enable plugins. That's a problem! The check needs to be done
> at run-time rather than compile time.

Fixed!

> 2) The unit test in t/Plugins.t is data dependent. It would be much better
> to mock the configuration data. Barring that, the test should be moved to
> t/db_dependent.

Moved to db_dependent

> 3) The test adds '.' to @INC. Unfortunately, tests are not usually run from
> t/, so it always fails when I run prove t/. The solution is to get the
> directory component from __FILE__

Fixed!

> 4) The test fails spectacularly for me:

Assuming you have all the required perl dependencies install, I'm stumped on this one. All the tests pass just fine for me. Do you have CAS enabled? It seems to be related to that.

QA comment:

The patch does not add permissions and userflags in the .sql files.

+ the datepicker module does not appear with your last patch (in my previous test, it worked!).

Marked as Failed QA.

> The patch does not add permissions and userflags in the .sql files.

I will address this
 
> + the datepicker module does not appear with your last patch (in my previous
> test, it worked!).

That's odd, it's still working for me. Do you have the line
   Alias /plugin/ "/var/lib/koha/plugins/"
is you koha-http.conf file?

Also, you'll need
  <pluginsdir>/var/lib/koha/plugins</pluginsdir>
  <enable_plugins>1</enable_plugins>
in your koha-conf.xml file, so the plugins dir needs to match between the two.

(In reply to comment #77)
> > + the datepicker module does not appear with your last patch (in my previous
> > test, it worked!).
> 
> That's odd, it's still working for me. Do you have the line
>    Alias /plugin/ "/var/lib/koha/plugins/"
> is you koha-http.conf file?

No, I forgot that! Now it works.


QA comment:

I think last patches take into account all previous remarks.

Tests passed with success.

I tested once more without finding any issue.
Code looks good to me

Kyle, do you  have some examples of plugins ? Do you have an idea of the kind of repository we can provide?
A wiki page with some information (about installation, how to develop a plugin, what's the use, etc.) would be great.

Marked as Passed QA.

I still get that same error message from the unit test. I am not using CAS.

Also, I have two questions about the unit test:
1) What are you doing with the can_load test?
2) Doesn't the Koha::Plugins::Handler module handle running plugins?

Finally, I am a little bit concerned about your use of "die." If someone navigates to a plugins page when plugins are disabled, they will get a nasty Perl error rather than a styled page that informs them plugins are disabled.

(In reply to comment #85)
> I still get that same error message from the unit test. I am not using CAS.
> 
> Also, I have two questions about the unit test:
> 1) What are you doing with the can_load test?

It's just testing the Module::Load::Conditional::can_load. Not necessary, but it doesn't hurt to test.

> 2) Doesn't the Koha::Plugins::Handler module handle running plugins?

Yes, it does. We can't actually run a plugin from the unit test though.


> Finally, I am a little bit concerned about your use of "die." If someone
> navigates to a plugins page when plugins are disabled, they will get a nasty
> Perl error rather than a styled page that informs them plugins are disabled.

I thought you wanted it to be that way. I can add a followup to change that.

(In reply to comment #86)
> (In reply to comment #85)
> > I still get that same error message from the unit test. I am not using CAS.
> > 
> > Also, I have two questions about the unit test:
> > 1) What are you doing with the can_load test?
> 
> It's just testing the Module::Load::Conditional::can_load. Not necessary,
> but it doesn't hurt to test.

Okay. I was trying to figure out wizardry you were trying to test by re-loading the module.

> > 2) Doesn't the Koha::Plugins::Handler module handle running plugins?
> 
> Yes, it does. We can't actually run a plugin from the unit test though.

Why not (other than the fact that the test is failing with the bizarre Auth_with_cas error)?

> > Finally, I am a little bit concerned about your use of "die." If someone
> > navigates to a plugins page when plugins are disabled, they will get a nasty
> > Perl error rather than a styled page that informs them plugins are disabled.
> 
> I thought you wanted it to be that way. I can add a followup to change that.

Thanks. The plugin code should refuse to run, but using die is not necessary (you could, for example, simply return, and check that the plugin object exists before using it).

> > > 2) Doesn't the Koha::Plugins::Handler module handle running plugins?
> > 
> > Yes, it does. We can't actually run a plugin from the unit test though.
> 
> Why not (other than the fact that the test is failing with the bizarre
> Auth_with_cas error)?

That's a good enough point ; )

I've amended the followup so that plugins can return a value that is in return returned by the Plugins::Handler::run. That way we can test run a plugin, and it will also be useful in the future for other plugin types ( such as a barcode modifying plugin ).

The last patch looks pretty good, but I still have that error message (comment #71) from the unit test that has to be solved before I can test this fully and perhaps push it. Any thoughts about where the problem might come from?

Also, a very small change to make to the last follow-up: you have a typo on this line:
+    return "Koha::Plugin::Test::intstall";

(In reply to comment #92)
> The last patch looks pretty good, but I still have that error message
> (comment #71) from the unit test that has to be solved before I can test
> this fully and perhaps push it. Any thoughts about where the problem might
> come from?

I'll do whatever it takes to get this into 3.12! I have a ton of ideas for useful plugins.

I noticed this error in your log:
#Error:  Undefined subroutine &main::/home/jcamins/koha-dev/plugins called at (eval 37) line 2.

I'm wondering if there are any 'crufty' files left over in your plugins directy. Maybe you should try deleting everything in there and then run the unit test.

Kyle

> Also, a very small change to make to the last follow-up: you have a typo on
> this line:
> +    return "Koha::Plugin::Test::intstall";
Fixed!

(In reply to comment #97)
> I noticed this error in your log:
> #Error:  Undefined subroutine &main::/home/jcamins/koha-dev/plugins called
> at (eval 37) line 2.
> 
> I'm wondering if there are any 'crufty' files left over in your plugins
> directy. Maybe you should try deleting everything in there and then run the
> unit test.

Sorry, you responded at the same time you rebased so I didn't notice the comment before. I have nothing in that directory.

Marking as "In Discussion" until we get this figured out.

Brendan,

This doesn't need another sign-off. Just more ideas on how to fix the problem I'm having with unit tests.

Created attachment 16220 [details] [review]
Bug 7804 - Add Koha Plugin System

Adds support for custom plugins. At the moment the Plugins
feature supports two types of plugins, reports and tools.

Plugins are installed by uploading KPZ ( Koha Plugin Zip )
packages. A KPZ file is just a zip file containing the
perl files, template files, and any other files neccessary
to make the plugin work.

Test plan:
1) Apply patch
2) Run updatedatabase.pl
3) Create the directory /var/lib/koha/plugins
4) Add the lines
      <pluginsdir>/var/lib/koha/plugins</pluginsdir>
      <enable_plugins>1</enable_plugins>"
   to your koha-conf.xml file
5) Add the line
       Alias /plugin/ "/var/lib/koha/plugins/"
   to your koha-httpd.conf file
6) Restart your webserver
7) Access the plugins system from the "More" pulldown
8) Upload the example plugin file provided here
9) Try it out!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 16221 [details] [review]
Bug 7804 - Add Koha Plugin System - Unit Tests

Tests Ok
~/kohaclone$ perl t/db_dependent/Plugins.t
1..15
ok 1 - use Koha::Plugins;
ok 2 - use Koha::Plugins::Handler;
ok 3 - use Koha::Plugins::Base;
ok 4 - use Koha::Plugin::Test;
ok 5 - Test can_load
ok 6 - Test plugin class isa Koha::Plugin::Test
ok 7 - Test plugin parent class isa Koha::Plugins::Base
ok 8 - Test plugin can report
ok 9 - Test plugin can tool
ok 10 - Test plugin can configure
ok 11 - Test plugin can install
ok 12 - Test plugin can install
ok 13 - Test $plugin->get_metadata()
ok 14 - Test $plugin->get_qualified_table_name()
ok 15 - Test $plugin->get_plugin_http_path()

(and all others as well)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 16222 [details] [review]
Bug 7804 - Add Koha Plugin System - QA Followup - Add flags and permissions sql

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 16223 [details] [review]
Bug 7804 - Add Koha Plugin System - QA Followup 2

* Add "Plugins disabled" screen instead of error
* Allow plugins to return a value, add a test run that checks the return value

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

When this unit test fails for you, are you running any other unit tests? I'm wondering if the issue is further up the chain, and perhaps some other unit tests may give you the same error.

(In reply to comment #71)
> We are definitely getting closer. However, I have found some issues that
> will need to be addressed:
> 1) Koha::Plugins cannot be used without a koha-conf.xml file that is
> configured to enable plugins. That's a problem! The check needs to be done
> at run-time rather than compile time.
> 2) The unit test in t/Plugins.t is data dependent. It would be much better
> to mock the configuration data. Barring that, the test should be moved to
> t/db_dependent.
> 3) The test adds '.' to @INC. Unfortunately, tests are not usually run from
> t/, so it always fails when I run prove t/. The solution is to get the
> directory component from __FILE__
> 4) The test fails spectacularly for me:
> jcamins@kohadev:~/kohaclone/t$ perl Plugins.t                               
> 
> 1..15
> ok 1 - use Koha::Plugins;
> ok 2 - use Koha::Plugins::Handler;
> not ok 3 - use Koha::Plugins::Base;
> #   Failed test 'use Koha::Plugins::Base;'
> #   at Plugins.t line 18.#     Tried to use 'Koha::Plugins::Base'.
> #     Error:  Undefined subroutine &main::/home/jcamins/koha-dev/plugins
> called at (eval 37) line 2.
> #       ...propagated at /usr/share/perl/5.14/base.pm line 93.
> # BEGIN failed--compilation aborted at
> /usr/share/perl5/Authen/CAS/Client/Response.pm line 68.
> # Compilation failed in require at /usr/share/perl5/Authen/CAS/Client.pm
> line 8.
> # BEGIN failed--compilation aborted at /usr/share/perl5/Authen/CAS/Client.pm
> line 8.
> # Compilation failed in require at
> /home/jcamins/kohaclone/C4/Auth_with_cas.pm line 26.
> # BEGIN failed--compilation aborted at
> /home/jcamins/kohaclone/C4/Auth_with_cas.pm line 26.
> # Compilation failed in require at /home/jcamins/kohaclone/C4/Auth.pm line
> 54.
> # BEGIN failed--compilation aborted at /home/jcamins/kohaclone/C4/Auth.pm
> line 63.
> # Compilation failed in require at
> /home/jcamins/kohaclone/Koha/Plugins/Base.pm line 27.
> # BEGIN failed--compilation aborted at
> /home/jcamins/kohaclone/Koha/Plugins/Base.pm line 27.
> # Compilation failed in require at (eval 34) line 2.
> # BEGIN failed--compilation aborted at (eval 34) line 2.
> not ok 4 - use Koha::Plugin::Test;
> #   Failed test 'use Koha::Plugin::Test;'
> #   at Plugins.t line 19.
> #     Tried to use 'Koha::Plugin::Test'.
> #     Error:  Attempt to reload Koha/Plugins/Base.pm aborted.
> # Compilation failed in require at (eval 39) line 2.
> #       ...propagated at /usr/share/perl/5.14/base.pm line 93.
> # BEGIN failed--compilation aborted at Koha/Plugin/Test.pm line 7.
> # Compilation failed in require at (eval 38) line 2.
> # BEGIN failed--compilation aborted at (eval 38) line 2.
> not ok 5 - Test can_load
> #   Failed test 'Test can_load'
> #   at Plugins.t line 22.
> Can't locate object method "new" via package "Koha::Plugin::Test" at
> Plugins.t line 24.
> # Looks like you planned 15 tests but ran 5.
> # Looks like you failed 3 tests of 5 run.
> # Looks like your test exited with 2 just after 5.

I thought I'd also mention I just tested the plugin system on another Koha server where I had never installed it before and both the unit test and the functionality worked as expected.

(In reply to comment #104)
> When this unit test fails for you, are you running any other unit tests? I'm
> wondering if the issue is further up the chain, and perhaps some other unit
> tests may give you the same error.

I just realized what the problem is!
4) Add the lines
      <pluginsdir>/var/lib/koha/plugins</pluginsdir>
      <enable_plugins>1</enable_plugins>"
   to your koha-conf.xml file
... unless you reran the installer, in which case you will already have those lines.

Created attachment 16299 [details] [review]
Bug 7804 - Add Koha Plugin System - QA Followup 3 - Additional Unit Tests

Returning to passed QA. I want to do some additional tests, but I think this is probably just about there.

This patch has been pushed to master.

I, personally, am concerned about security vulnerabilities in this code, but as I haven't noticed any and it is possible to thoroughly disable the plugin code, I am pushing it anyway.

Just noticed that the PLUGINS_DIR isn't saved to the koha-install-log.

Don't know if this is all that important, but worth noting...