Bug 20415 - Remove UseKohaPlugins system preference
Summary: Remove UseKohaPlugins system preference
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Fridolin SOMERS
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 7804
Blocks: 20396 25131
  Show dependency treegraph
 
Reported: 2018-03-14 16:43 UTC by Gaetan Boisson
Modified: 2020-05-16 16:33 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
`UseKohaPlugins` system preferences is removed. Koha plugins now only depends on config key `enable_plugins`.
Version(s) released in:
20.05.00


Attachments
Bug 20415: add Koha.Config Template Toolkit method (6.08 KB, patch)
2020-03-19 14:38 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20415: Remove UseKohaPlugins system preference (20.06 KB, patch)
2020-03-19 14:39 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20415: add Koha.Config Template Toolkit method (6.17 KB, patch)
2020-03-19 23:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20415: Remove UseKohaPlugins system preference (20.33 KB, patch)
2020-03-19 23:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method (6.93 KB, patch)
2020-03-23 12:27 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20415: Remove UseKohaPlugins system preference (20.03 KB, patch)
2020-03-23 12:27 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method (7.02 KB, patch)
2020-03-24 20:35 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 20415: Remove UseKohaPlugins system preference (20.09 KB, patch)
2020-03-24 20:35 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 20415: add Koha.ArePluginsEnabled Template Toolkit method (7.09 KB, patch)
2020-03-25 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20415: Remove UseKohaPlugins system preference (20.17 KB, patch)
2020-03-25 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20415: Add POD (1.27 KB, patch)
2020-03-25 14:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Follow-up] Bug 20415: Fix typo causing SQL error in updatedatabase.pl (938 bytes, patch)
2020-03-27 00:14 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gaetan Boisson 2018-03-14 16:43:07 UTC
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.
Comment 1 Tomás Cohen Arazi 2018-03-14 16:46:40 UTC
I'd vote to remove the koha-conf.xml entry instead!
Comment 2 Liz Rea 2018-03-15 00:34:52 UTC
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.
Comment 3 Marcel de Rooy 2018-03-15 07:51:34 UTC
(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.
Comment 4 Gaetan Boisson 2018-03-15 09:34:00 UTC
But how will they upload plugins without having talked to a sysadmin?
Comment 5 Liz Rea 2018-03-16 00:29:34 UTC
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
Comment 6 Tomás Cohen Arazi 2018-03-16 02:47:38 UTC
(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.
Comment 7 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.
Comment 8 Paul Poulain 2018-04-05 14:42:45 UTC
(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
Comment 9 Fridolin SOMERS 2018-04-05 14:48:58 UTC
(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.
Comment 10 Blou 2018-04-06 15:54:16 UTC
+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 :-) )
Comment 11 Elaine Bradtke 2018-04-23 17:55:36 UTC
+1
This preference does not add anything.
Comment 12 Fridolin SOMERS 2019-06-28 10:07:45 UTC
I will try to work on this
Comment 13 Fridolin SOMERS 2020-03-19 14:38:55 UTC
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)
Comment 14 Fridolin SOMERS 2020-03-19 14:39:23 UTC
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
Comment 15 Owen Leonard 2020-03-19 23:23:27 UTC
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>
Comment 16 Owen Leonard 2020-03-19 23:23:32 UTC
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>
Comment 17 Owen Leonard 2020-03-19 23:27:02 UTC
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
Comment 18 Julian Maurice 2020-03-20 06:38:43 UTC
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.
Comment 19 David Cook 2020-03-20 07:06:09 UTC
(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?
Comment 20 Julian Maurice 2020-03-20 07:07:44 UTC
(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.
Comment 21 Jonathan Druart 2020-03-20 09:31:17 UTC
(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.
Comment 22 Julian Maurice 2020-03-20 13:41:59 UTC
(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 ?
Comment 23 David Cook 2020-03-23 03:01:40 UTC
(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.
Comment 24 David Cook 2020-03-23 03:03:31 UTC
(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.)
Comment 25 Fridolin SOMERS 2020-03-23 12:00:25 UTC
(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.
Comment 26 Fridolin SOMERS 2020-03-23 12:27:18 UTC
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
Comment 27 Fridolin SOMERS 2020-03-23 12:27:39 UTC
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
Comment 28 Fridolin SOMERS 2020-03-23 12:29:17 UTC
TT plugin changed + Unit test added
Main patch rebased on master with conflic on _after_item_action_hooks move.
Comment 29 Bernardo Gonzalez Kriegel 2020-03-24 20:35:27 UTC
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.
Comment 30 Bernardo Gonzalez Kriegel 2020-03-24 20:35:32 UTC
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>
Comment 31 Kyle M Hall 2020-03-25 14:29:42 UTC
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>
Comment 32 Kyle M Hall 2020-03-25 14:29:54 UTC
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>
Comment 33 Kyle M Hall 2020-03-25 14:29:58 UTC
Created attachment 101719 [details] [review]
Bug 20415: Add POD

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Martin Renvoize 2020-03-26 12:04:08 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 35 David Cook 2020-03-27 00:10:37 UTC
(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/';
Comment 36 David Cook 2020-03-27 00:12:21 UTC
Oh wait. Maybe not broke. I forget we don't throw fatal errors for SQL errors.
Comment 37 David Cook 2020-03-27 00:14:36 UTC
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.
Comment 38 Kyle M Hall 2020-04-02 13:35:20 UTC
(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 :\
Comment 39 Jonathan Druart 2020-04-02 13:50:06 UTC
(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
Comment 40 Joy Nelson 2020-04-03 22:30:48 UTC
enhancement not backported to 19.11.x