In order to use plugins you need: - to have uploaded some plugins - to have activated the relevant part of your koha conf file - to have the right permissions - to have activated this system preference I think that's a little too much and the system preference could be removed and be always on.
I'd vote to remove the koha-conf.xml entry instead!
I don't think it's too much. Plugins enable unverified code to be run, I'm for belt and bracers all the way here. I *want* people to have to talk to a sysadmin before enabling plugins.
(In reply to Liz Rea from comment #2) > I don't think it's too much. Plugins enable unverified code to be run, I'm > for belt and bracers all the way here. > > I *want* people to have to talk to a sysadmin before enabling plugins. Sounds good to me.
But how will they upload plugins without having talked to a sysadmin?
They don't, that's the point. The reason for the many hoops here was that we didn't want front end staff uploading un-verified code (none of the plugins are independently audited, nor do they go through a community QA process) to the public server filled with personally identifying information without the IT departments of libraries being aware that it was happening. I can imagine a (however unlikely) scenario where someone makes a Really Awesome Plugin(tm) that provided a function that lots of libraries want, that does the feature but also sends the entire database to an unscrupulous 3rd party. With the hoops, we can at least be sure that someone with access to the server has spoken to the person in the library about their intentions. Without them, either the sysadmin or the librarian could do this independently and without speaking to each other. Experience tells me that librarians will do almost anything to get out of talking to the IT department (yes it's a generalisation). This isn't a good thing in this scenario, we have to think about the potential for theft of data via the plugin system and do what we can to make sure that the people tasked with protecting the data (the IT departments, usually) know exactly what code is running on their publicly facing web servers. The multi factor turn on for this feature is at the very least, due diligence on our part. We could warn more, to be honest. I hope this helps explain my perspective a bit. Cheers, Liz
(In reply to Liz Rea from comment #5) > They don't, that's the point. The reason for the many hoops here was that we > didn't want front end staff uploading un-verified code (none of the plugins > are independently audited, nor do they go through a community QA process) to > the public server filled with personally identifying information without the > IT departments of libraries being aware that it was happening. > > I can imagine a (however unlikely) scenario where someone makes a Really > Awesome Plugin(tm) that provided a function that lots of libraries want, > that does the feature but also sends the entire database to an unscrupulous > 3rd party. With the hoops, we can at least be sure that someone with access > to the server has spoken to the person in the library about their > intentions. Without them, either the sysadmin or the librarian could do this > independently and without speaking to each other. > > Experience tells me that librarians will do almost anything to get out of > talking to the IT department (yes it's a generalisation). This isn't a good > thing in this scenario, we have to think about the potential for theft of > data via the plugin system and do what we can to make sure that the people > tasked with protecting the data (the IT departments, usually) know exactly > what code is running on their publicly facing web servers. The multi factor > turn on for this feature is at the very least, due diligence on our part. We > could warn more, to be honest. > > I hope this helps explain my perspective a bit. > > Cheers, > Liz I agree with Liz.
I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins.
(In reply to Owen Leonard from comment #7) > I don't think the system preference adds any security. There are already > multiple permissions required for working with plugins: > > - Configure plugins > - Manage plugins ( install / uninstall ) > - Use report plugins > - Use tool plugins > > And even with those permissions your server must be configured to allow the > use of plugins. +1
(In reply to Owen Leonard from comment #7) > I don't think the system preference adds any security. There are already > multiple permissions required for working with plugins: > > - Configure plugins > - Manage plugins ( install / uninstall ) > - Use report plugins > - Use tool plugins > > And even with those permissions your server must be configured to allow the > use of plugins. +1 to reduce complexity. Also I think that a database dump should not carry a parameter related to the system. Think about cloning db to a test server.
+1 The koha-conf.xml supercede the syspref. So in term of security, if you can modify the koha-conf.xml, the syspref is not a blocker. If your goal is to prevent people (including superlibrarians) from damaging Koha using a plugin, the koha-conf.xml is the only real way. If you only mean to customize the presentation, then the user permissions do that. The syspref is one too-many step. I see value, just not enough. (this is a nice bz to keep in my pocket for training my next interns :-) )
+1 This preference does not add anything.
I will try to work on this
Created attachment 101021 [details] [review] Bug 20415: add Koha.Config Template Toolkit method In order to remove UseKohaPlugins syspref, adds to Koha Template Toolkit plugin a method Config() that calls C4::Context->config(). Test plan: 1) Set syspref UseKohaPlugins enabled 2) Set config enable_plugins=1 3) Check you see "Tool plugins" in "Tools" home page and menu 4) Check you see "Manage plugins" in "Koha administration" home page 5) Check you see "Report plugins" in "Reports" home page and menu 6) Set config enable_plugins=0 7) Check you don't see in 3) 4) 5)
Created attachment 101022 [details] [review] Bug 20415: Remove UseKohaPlugins system preference Owen Leonard 2018-03-16 10:47:47 UTC : << I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins. >> Test plan : 1) Install kitchen sink plugin https://github.com/bywatersolutions/koha-plugin-kitchen-sink 2) Run misc/devel/install_plugins.pl 3) Set config enable_plugins=1 4) Check all parts of the plugin are working 5) Set config enable_plugins=0 6) Check all parts of the plugin are disabled
Created attachment 101060 [details] [review] Bug 20415: add Koha.Config Template Toolkit method In order to remove UseKohaPlugins syspref, adds to Koha Template Toolkit plugin a method Config() that calls C4::Context->config(). Test plan: 1) Set syspref UseKohaPlugins enabled 2) Set config enable_plugins=1 3) Check you see "Tool plugins" in "Tools" home page and menu 4) Check you see "Manage plugins" in "Koha administration" home page 5) Check you see "Report plugins" in "Reports" home page and menu 6) Set config enable_plugins=0 7) Check you don't see in 3) 4) 5) Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 101061 [details] [review] Bug 20415: Remove UseKohaPlugins system preference Owen Leonard 2018-03-16 10:47:47 UTC : << I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins. >> Test plan : 1) Install kitchen sink plugin https://github.com/bywatersolutions/koha-plugin-kitchen-sink 2) Run misc/devel/install_plugins.pl 3) Set config enable_plugins=1 4) Check all parts of the plugin are working 5) Set config enable_plugins=0 6) Check all parts of the plugin are disabled Signed-off-by: Owen Leonard <oleonard@myacpl.org>
I wanted to add my signoff to these because they're working well following the test plan. However there is one problem highlighted by the QA tool: FAIL Koha/Template/Plugin/Koha.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for Config
Isn't it a little dangerous to give access to all of koha-conf.xml inside templates ? This could be used to retrieve sensitive information like mysql password, or the REST API's secret passphrase.
(In reply to Julian Maurice from comment #18) > Isn't it a little dangerous to give access to all of koha-conf.xml inside > templates ? This could be used to retrieve sensitive information like mysql > password, or the REST API's secret passphrase. You can already retrieve that information using Perl though. What scenario do you see someone using it maliciously?
(In reply to David Cook from comment #19) > You can already retrieve that information using Perl though. What scenario > do you see someone using it maliciously? Templates can be used in "Tools » Notices & Slips", they can be written by users.
(In reply to Julian Maurice from comment #20) > (In reply to David Cook from comment #19) > > You can already retrieve that information using Perl though. What scenario > > do you see someone using it maliciously? > > Templates can be used in "Tools » Notices & Slips", they can be written by > users. I agree, that's definitely wrong. Fridolin, you should pass the variable the dirty way, from C4::Auth.
(In reply to Jonathan Druart from comment #21) > (In reply to Julian Maurice from comment #20) > > (In reply to David Cook from comment #19) > > > You can already retrieve that information using Perl though. What scenario > > > do you see someone using it maliciously? > > > > Templates can be used in "Tools » Notices & Slips", they can be written by > > users. > > I agree, that's definitely wrong. > > Fridolin, you should pass the variable the dirty way, from C4::Auth. Maybe we can find something better than the dirty way and the security-hole way, like a method ArePluginsEnabled in Koha::Template::Plugin::Koha ?
(In reply to Julian Maurice from comment #20) > Templates can be used in "Tools » Notices & Slips", they can be written by > users. I admit that I hadn't inspected that code. I had expected that we weren't letting users use plugins. However, looking at C4::Letters::_process_tt, it does look like we're using "Koha::Template::Plugin" for a PLUGIN_BASE.
(In reply to Julian Maurice from comment #22) > Maybe we can find something better than the dirty way and the security-hole > way, like a method ArePluginsEnabled in Koha::Template::Plugin::Koha ? That sounds reasonable to me. (Alternatively, we can use multiple PLUGIN_BASE values for system templates (ie templates that can't be manipulated by a user), but that could be easy to screw up too.)
(In reply to Julian Maurice from comment #22) > (In reply to Jonathan Druart from comment #21) > > (In reply to Julian Maurice from comment #20) > > > (In reply to David Cook from comment #19) > > > > You can already retrieve that information using Perl though. What scenario > > > > do you see someone using it maliciously? > > > > > > Templates can be used in "Tools » Notices & Slips", they can be written by > > > users. > > > > I agree, that's definitely wrong. > > > > Fridolin, you should pass the variable the dirty way, from C4::Auth. > > Maybe we can find something better than the dirty way and the security-hole > way, like a method ArePluginsEnabled in Koha::Template::Plugin::Koha ? Exactly what I was going to propose. I'm on a patch.
Created attachment 101433 [details] [review] Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method In order to remove UseKohaPlugins syspref, adds to Koha Template Toolkit plugin a method ArePluginsEnabled(). Test plan: 1) Set syspref UseKohaPlugins enabled 2) Set config enable_plugins=1 3) Check you see "Tool plugins" in "Tools" home page and menu 4) Check you see "Manage plugins" in "Koha administration" home page 5) Check you see "Report plugins" in "Reports" home page and menu 6) Set config enable_plugins=0 7) Check you don't see in 3) 4) 5) 8) Run prove t/Koha_Template_Plugin_Koha.t
Created attachment 101434 [details] [review] Bug 20415: Remove UseKohaPlugins system preference Owen Leonard 2018-03-16 10:47:47 UTC : << I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins. >> Test plan : 1) Install kitchen sink plugin https://github.com/bywatersolutions/koha-plugin-kitchen-sink 2) Run misc/devel/install_plugins.pl 3) Set config enable_plugins=1 4) Check all parts of the plugin are working 5) Set config enable_plugins=0 6) Check all parts of the plugin are disabled
TT plugin changed + Unit test added Main patch rebased on master with conflic on _after_item_action_hooks move.
Created attachment 101663 [details] [review] Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method In order to remove UseKohaPlugins syspref, adds to Koha Template Toolkit plugin a method ArePluginsEnabled(). Test plan: 1) Set syspref UseKohaPlugins enabled 2) Set config enable_plugins=1 3) Check you see "Tool plugins" in "Tools" home page and menu 4) Check you see "Manage plugins" in "Koha administration" home page 5) Check you see "Report plugins" in "Reports" home page and menu 6) Set config enable_plugins=0 7) Check you don't see in 3) 4) 5) 8) Run prove t/Koha_Template_Plugin_Koha.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no errors.
Created attachment 101664 [details] [review] Bug 20415: Remove UseKohaPlugins system preference Owen Leonard 2018-03-16 10:47:47 UTC : << I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins. >> Test plan : 1) Install kitchen sink plugin https://github.com/bywatersolutions/koha-plugin-kitchen-sink 2) Run misc/devel/install_plugins.pl 3) Set config enable_plugins=1 4) Check all parts of the plugin are working 5) Set config enable_plugins=0 6) Check all parts of the plugin are disabled Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 101717 [details] [review] Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method In order to remove UseKohaPlugins syspref, adds to Koha Template Toolkit plugin a method ArePluginsEnabled(). Test plan: 1) Set syspref UseKohaPlugins enabled 2) Set config enable_plugins=1 3) Check you see "Tool plugins" in "Tools" home page and menu 4) Check you see "Manage plugins" in "Koha administration" home page 5) Check you see "Report plugins" in "Reports" home page and menu 6) Set config enable_plugins=0 7) Check you don't see in 3) 4) 5) 8) Run prove t/Koha_Template_Plugin_Koha.t Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no errors. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 101718 [details] [review] Bug 20415: Remove UseKohaPlugins system preference Owen Leonard 2018-03-16 10:47:47 UTC : << I don't think the system preference adds any security. There are already multiple permissions required for working with plugins: - Configure plugins - Manage plugins ( install / uninstall ) - Use report plugins - Use tool plugins And even with those permissions your server must be configured to allow the use of plugins. >> Test plan : 1) Install kitchen sink plugin https://github.com/bywatersolutions/koha-plugin-kitchen-sink 2) Run misc/devel/install_plugins.pl 3) Set config enable_plugins=1 4) Check all parts of the plugin are working 5) Set config enable_plugins=0 6) Check all parts of the plugin are disabled Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 101719 [details] [review] Bug 20415: Add POD Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
(In reply to Martin Renvoize from comment #34) > Nice work everyone! > > Pushed to master for 20.05 Ooops, I think you may have accidentally broken updatedatabase.pl: diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 65ecbb7169..3c62761737 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -21296,6 +21296,7 @@ if( CheckVersion( $DBversion ) ) { $dbh->do(q| ALTER TABLE suggestions ADD CONSTRAINT `suggestions_ibfk_lastmodificationby` FOREIGN KEY (`lastmodificationby`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE + A |); } @@ -21335,6 +21336,15 @@ if( CheckVersion( $DBversion ) ) { NewVersion( $DBversion, 23590, "Add lastmodificationby and lastmodificationdate to the suggestions table"); } +$DBversion = '19.12.00.056'; +if( CheckVersion( $DBversion ) ) { + + $dbh->do( "DELETE FROM systempreferences WHERE variable='UseKohaPlugins'" ); + + NewVersion( $DBversion, 20415, "Remove UseKohaPlugins preference"); +} + + # SEE bug 13068 # if there is anything in the atomicupdate, read and execute it. my $update_dir = C4::Context->config('intranetdir') . '/installer/data/mysql/atomicupdate/';
Oh wait. Maybe not broke. I forget we don't throw fatal errors for SQL errors.
Created attachment 101928 [details] [review] [Follow-up] Bug 20415: Fix typo causing SQL error in updatedatabase.pl This patch just removes a typo that is causing a database update to fail.
(In reply to David Cook from comment #37) > Created attachment 101928 [details] [review] [review] > [Follow-up] Bug 20415: Fix typo causing SQL error in updatedatabase.pl > > This patch just removes a typo that is causing a database update to fail. My bad. I must've only tested with a fresh db :\
(In reply to David Cook from comment #37) > Created attachment 101928 [details] [review] [review] > [Follow-up] Bug 20415: Fix typo causing SQL error in updatedatabase.pl > > This patch just removes a typo that is causing a database update to fail. I pushed a follow-up: commit 77f481a37b9e97aab7f8db1b7e9215fd0ca5d904 Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org> CommitDate: Fri Mar 27 08:20:47 2020 +0100 Bug 20415: (follow-up) DBRev 19.12.00.056
enhancement not backported to 19.11.x