Bug 20340 - Ability to use authentication plugin
Summary: Ability to use authentication plugin
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-06 10:38 UTC by Alex Arnaud
Modified: 2024-10-18 06:42 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 20340 - Ability to add and use authentication plugins (10.15 KB, patch)
2018-03-06 13:17 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (6.26 KB, patch)
2018-03-19 10:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.46 KB, patch)
2018-03-20 04:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Nasty Empty Drop Down (221.15 KB, image/png)
2018-03-20 14:47 UTC, Mark Tompsett
Details
Bug 20340: add of an empty method configure (887 bytes, patch)
2019-04-23 13:14 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.94 KB, patch)
2019-04-23 13:23 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 13:24 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of an empty method configure (887 bytes, patch)
2019-04-23 13:24 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of an empty method configure (885 bytes, patch)
2019-04-23 13:28 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of an empty method configure (885 bytes, patch)
2019-04-23 13:31 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (1.36 KB, patch)
2019-04-23 13:31 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of an empty method configure (885 bytes, patch)
2019-04-23 13:31 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.65 KB, patch)
2019-04-23 14:18 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 14:18 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.65 KB, patch)
2019-04-23 14:49 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 14:49 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.65 KB, patch)
2019-04-23 14:56 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 14:56 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.65 KB, patch)
2019-04-23 14:57 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 14:57 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.46 KB, patch)
2019-04-23 15:09 UTC, axel
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.64 KB, patch)
2019-04-23 15:09 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of empty method configure (990 bytes, patch)
2019-04-23 15:12 UTC, axel
Details | Diff | Splinter Review
Bug 20340: add of empty method configure (996 bytes, patch)
2019-04-23 15:15 UTC, axel
Details | Diff | Splinter Review
Bug 20340 - Ability to add and use authentication plugins (7.05 KB, patch)
2019-07-05 02:26 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20340: Followup to allow OPAC login to complete (1.71 KB, patch)
2019-07-05 02:26 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20340: add of empty method configure (1003 bytes, patch)
2019-07-05 02:26 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20340: (follow-up) fix some of Auth.t's tests (1.12 KB, patch)
2019-07-05 02:41 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2018-03-06 10:38:36 UTC
This enhancement intends to allow adding and use authentication plugins. A plugin can be enabled with a system preference (AuthenticationModule) and placed in the 
<pluginsdir>
Comment 1 Alex Arnaud 2018-03-06 13:17:24 UTC
Created attachment 72469 [details] [review]
Bug 20340 - Ability to add and use authentication plugins

Plugins must be placed in <pluginsdir> and enabled with
AuthenticationModule system preference.

Test plan:

  - Apply this patch and update your database,
  - set UseKohaPlugins to enabled,
  - define a pluginsdir in your koha-conf.xml,
  - put a plugin in this directory.
  - I.e, take the one provided in tests directory:
     - t/Koha/Plugin/TestAuth.pm,
     - login/pass is test/test
  - You should have:
      /your/plugins/dir/Koha/Plugin/TestAuth.pm,
  - replace default by your plugin in AuthenticationModule
    system preference,
  - test authentication with and without the plugin.
Comment 2 Katrin Fischer 2018-03-07 07:24:46 UTC
I like the idea of this, but with more and more functionality being moved into plugins, I think we really need to make them translatable - bug 17489.
Comment 3 Kyle M Hall (khall) 2018-03-07 10:59:41 UTC
I think this is an excellent concept, but I don't think it is necessary to have a syspref and limit the auth pluggability to one at a time. In Koha we already essentially loop through all the enabled authentication systems ( ldap, cas, etc ).

I would recommend the following:
* Drop the system preference
* Inside checkpw, loop through all plugins with a checkpw method, and return the first one that 'successfully' authenticates the user/password combo.
Comment 4 Alex Arnaud 2018-03-07 12:53:00 UTC
(In reply to Kyle M Hall from comment #3)
> I think this is an excellent concept, but I don't think it is necessary to
> have a syspref and limit the auth pluggability to one at a time. In Koha we
> already essentially loop through all the enabled authentication systems (
> ldap, cas, etc ).
Not sure Koha works like that. Reading the code, we switch (not loop) on the one that is enabled: Koha *or* LDAP *or* CAS with a fallback on Koha if LDAP or CAS return the code 0.
> 

> I would recommend the following:
> * Drop the system preference
Anyway we need to enable/disable this module(s). So, in a dedicated administration page? In plugins-home.pl (but it is not realy built for that) ?

> * Inside checkpw, loop through all plugins with a checkpw method, and return
> the first one that 'successfully' authenticates the user/password combo.
Comment 5 Kyle M Hall (khall) 2018-03-07 13:05:56 UTC
(In reply to Alex Arnaud from comment #4)
> (In reply to Kyle M Hall from comment #3)
> > I think this is an excellent concept, but I don't think it is necessary to
> > have a syspref and limit the auth pluggability to one at a time. In Koha we
> > already essentially loop through all the enabled authentication systems (
> > ldap, cas, etc ).
> Not sure Koha works like that. Reading the code, we switch (not loop) on the
> one that is enabled: Koha *or* LDAP *or* CAS with a fallback on Koha if LDAP
> or CAS return the code 0.

You are correct! Not sure why I had thought that. Looks like the precedence is LDAP, CAS, Shib. I don't think that should affect my suggestion though.

> > 
> 
> > I would recommend the following:
> > * Drop the system preference
> Anyway we need to enable/disable this module(s). So, in a dedicated
> administration page? In plugins-home.pl (but it is not realy built for that)
> ?
> 
> > * Inside checkpw, loop through all plugins with a checkpw method, and return
> > the first one that 'successfully' authenticates the user/password combo.

I would say the plugin itself should be responsible for having controls to tell Koha if it is enabled or not. There is already precedence for this in Koha ( see bug 19173 ). For simplicity, the plugin could simply return values as if the authentication failed, for the sake of simplicity.

What do you think?
Comment 6 Katrin Fischer 2018-03-07 13:20:20 UTC
Hm, I think it's always possible to use local login and we have a library using both LDAP and Shibboleth... With Shibboleth you have a link to an external login page like with CAS - maybe that's where the confusion is? I think building it to support many plugins would be nice, similar to how we support multiple ILL backends, but maybe not a must.
Comment 7 Kyle M Hall (khall) 2018-03-07 13:26:11 UTC
(In reply to Katrin Fischer from comment #6)
> Hm, I think it's always possible to use local login and we have a library
> using both LDAP and Shibboleth... With Shibboleth you have a link to an
> external login page like with CAS - maybe that's where the confusion is? I
> think building it to support many plugins would be nice, similar to how we
> support multiple ILL backends, but maybe not a must.

You could be right. I based my reassessment on this code: http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Auth.pm;h=c3ad351aa17ae73addb6612e7af2905b003647b4;hb=HEAD#l1778

That being said. We are starting to veer away from the topic of the bug ; )

If implemented the way I suggest, it would support multiple auth plugins, *and* be simpler architecturally. I think that's a win on all fronts : )
Comment 8 Alex Arnaud 2018-03-07 16:33:06 UTC
(In reply to Kyle M Hall from comment #5)
> (In reply to Alex Arnaud from comment #4)
> > (In reply to Kyle M Hall from comment #3)

> I would say the plugin itself should be responsible for having controls to
> tell Koha if it is enabled or not. There is already precedence for this in
> Koha ( see bug 19173 ). For simplicity, the plugin could simply return
> values as if the authentication failed, for the sake of simplicity.
> 
> What do you think?

You mean: plugin exists (in the plugins directory) == plugin enabled ? That also means we need to delete it for make it disabled, right?


If we assume that we want to loop on several plugins, it could be nice to be able to set an ordered stack of authentication modules. This could allow us to chose which one is used first etc... In this case we need a dedicated page in fact.
Comment 9 Alex Arnaud 2018-03-07 16:38:09 UTC
(In reply to Alex Arnaud from comment #8)
> (In reply to Kyle M Hall from comment #5)
> > (In reply to Alex Arnaud from comment #4)
> > > (In reply to Kyle M Hall from comment #3)
> 
> > I would say the plugin itself should be responsible for having controls to
> > tell Koha if it is enabled or not. There is already precedence for this in
> > Koha ( see bug 19173 ). For simplicity, the plugin could simply return
> > values as if the authentication failed, for the sake of simplicity.
> > 
> > What do you think?
> 
> You mean: plugin exists (in the plugins directory) == plugin enabled ? That
> also means we need to delete it for make it disabled, right?
> 
> 
> If we assume that we want to loop on several plugins, it could be nice to be
> able to set an ordered stack of authentication modules. This could allow us
> to chose which one is used first etc... In this case we need a dedicated
> page in fact.

Oh, think i see! We can configure plugins and so use the plugins_data. Also this could allow use to define a "weight" to set the execution order
Comment 10 Katrin Fischer 2018-03-08 06:48:22 UTC
How does one work with plugins in a multi-instance setup? Do you need to store them separately (install multiple times) or can you use a shared plugin folder and turn them on/off with the settings? (sorry, hope not distracting again)
Comment 11 Kyle M Hall (khall) 2018-03-08 11:01:35 UTC
(In reply to Alex Arnaud from comment #9)
> (In reply to Alex Arnaud from comment #8)
> > (In reply to Kyle M Hall from comment #5)
> > > (In reply to Alex Arnaud from comment #4)
> > > > (In reply to Kyle M Hall from comment #3)
> > 
> > > I would say the plugin itself should be responsible for having controls to
> > > tell Koha if it is enabled or not. There is already precedence for this in
> > > Koha ( see bug 19173 ). For simplicity, the plugin could simply return
> > > values as if the authentication failed, for the sake of simplicity.
> > > 
> > > What do you think?
> > 
> > You mean: plugin exists (in the plugins directory) == plugin enabled ? That
> > also means we need to delete it for make it disabled, right?
> > 
> > 
> > If we assume that we want to loop on several plugins, it could be nice to be
> > able to set an ordered stack of authentication modules. This could allow us
> > to chose which one is used first etc... In this case we need a dedicated
> > page in fact.
> 
> Oh, think i see! We can configure plugins and so use the plugins_data. Also
> this could allow use to define a "weight" to set the execution order

No, just the plugin store it's on 'on/off' switch. If it is set to off, have the plugin always return 0 for checkauth.

I personally don't find the ability to set the order to be necessary, but your idea would work. If each plugin stored a param call 'priority' in plugin_data table, it would be easy to do what you propose!
Comment 12 Kyle M Hall (khall) 2018-03-08 13:37:07 UTC
(In reply to Katrin Fischer from comment #10)
> How does one work with plugins in a multi-instance setup? Do you need to
> store them separately (install multiple times) or can you use a shared
> plugin folder and turn them on/off with the settings? (sorry, hope not
> distracting again)

By default, it's per-instance. You *could* also have a 'shared' plugins dir, as the kona-conf plugins dir setting can be set for multiple dirs. See bug 15879 for details.
Comment 13 Alex Arnaud 2018-03-19 10:01:28 UTC
Created attachment 73089 [details] [review]
Bug 20340 - Ability to add and use authentication plugins

Plugins must be placed in <pluginsdir> and enabled with
AuthenticationModule system preference.

Test plan:

  - Apply this patch,
  - set UseKohaPlugins to enabled,
  - define a pluginsdir in your koha-conf.xml,
  - put a plugin in this directory.
  - I.e, take the one provided in tests directory:
     - t/Koha/Plugin/TestAuth.pm,
     - login/pass is test/test
  - You should have:
      /your/plugins/dir/Koha/Plugin/TestAuth.pm,
  - test authentication with and without the plugin.
Comment 14 Mark Tompsett 2018-03-20 04:05:58 UTC
Created attachment 73098 [details] [review]
Bug 20340: Followup to allow OPAC login to complete

The Plugin authenticates correctly, the problem is there
is no patron to match with, and so there is nothing to check
for authorization and user-details once a login is attempted.
This will at least let it fail gracefully.
Comment 15 Mark Tompsett 2018-03-20 04:15:52 UTC
It would be nice to rework the authentication logic to all be plugins, then we could actually prioritize the order of attempts, etc. etc. This is an excellent start, but:

1) the test module should have an uninstall at least.
2) Also, annoying POD coverage failure on the QA Test tools.
Comment 16 Alex Arnaud 2018-03-20 12:59:34 UTC
(In reply to M. Tompsett from comment #15)
> It would be nice to rework the authentication logic to all be plugins, 
If you mean that LDAP, CAS and Shibboleth should be removed from the source code and provided only as plugins, it is also my opinion :)

> then we could actually prioritize the order of attempts, etc. etc.
We are already able to prioritize. Each auth plugin can have a priority value and they are ordered before the call.

> This is an excellent start, but:
> 
> 1) the test module should have an uninstall at least.
Why?
> 2) Also, annoying POD coverage failure on the QA Test tools.

And i don't understand the changes in opac-user.pl

Why did you failed QA?
Comment 17 Mark Tompsett 2018-03-20 14:41:28 UTC
(In reply to Alex Arnaud from comment #16)
> (In reply to M. Tompsett from comment #15)
> > It would be nice to rework the authentication logic to all be plugins, 

> If you mean that LDAP, CAS and Shibboleth should be removed from the source
> code and provided only as plugins, it is also my opinion :)

Exactly.


> > then we could actually prioritize the order of attempts, etc. etc.

> We are already able to prioritize. Each auth plugin can have a priority
> value and they are ordered before the call.

But not the archaic LDAP, CAS, Shibboleth, etc. etc. that's hard coded already.


> > 1) the test module should have an uninstall at least.

> Why?

See the attached empty Actions dropdown. Now bug 20438 will solve this, but still. How hard is an uninstall which calls the inherited parent?


> > 2) Also, annoying POD coverage failure on the QA Test tools.
[SNIP magic]
> Why did you failed QA?

kshell
/home/vagrant/qa-test-tools/koha-qa.pl -v 2 -c 2

Though, I believe bug 20357 will deal with this, but given the feedback on my fixes, it's not getting in quickly.


> And i don't understand the changes in opac-user.pl

try logging in with test/test into OPAC: internal server errors.
Comment 18 Mark Tompsett 2018-03-20 14:47:27 UTC
Created attachment 73106 [details]
Nasty Empty Drop Down

Drop downs should never look like this.
Comment 19 Martin Renvoize (ashimema) 2018-09-28 09:26:44 UTC
Wow, awesome plan guys... keep up the good work :)
Comment 20 Alex Arnaud 2018-12-27 17:35:33 UTC
(In reply to M. Tompsett from comment #18)
> Created attachment 73106 [details]
> Nasty Empty Drop Down
> 
> Drop downs should never look like this.

I see. But this plugin is not intended to go in your plugins directory. It stands only for unit tests.
Comment 21 axel 2019-04-23 13:14:48 UTC Comment hidden (obsolete)
Comment 22 axel 2019-04-23 13:23:59 UTC Comment hidden (obsolete)
Comment 23 axel 2019-04-23 13:24:02 UTC Comment hidden (obsolete)
Comment 24 axel 2019-04-23 13:24:06 UTC Comment hidden (obsolete)
Comment 25 axel 2019-04-23 13:28:40 UTC Comment hidden (obsolete)
Comment 26 axel 2019-04-23 13:31:16 UTC Comment hidden (obsolete)
Comment 27 axel 2019-04-23 13:31:19 UTC Comment hidden (obsolete)
Comment 28 axel 2019-04-23 13:31:22 UTC Comment hidden (obsolete)
Comment 29 axel 2019-04-23 14:18:12 UTC Comment hidden (obsolete)
Comment 30 axel 2019-04-23 14:18:16 UTC Comment hidden (obsolete)
Comment 31 axel 2019-04-23 14:49:06 UTC Comment hidden (obsolete)
Comment 32 axel 2019-04-23 14:49:09 UTC Comment hidden (obsolete)
Comment 33 axel 2019-04-23 14:56:51 UTC Comment hidden (obsolete)
Comment 34 axel 2019-04-23 14:56:54 UTC Comment hidden (obsolete)
Comment 35 axel 2019-04-23 14:57:31 UTC Comment hidden (obsolete)
Comment 36 axel 2019-04-23 14:57:34 UTC Comment hidden (obsolete)
Comment 37 axel 2019-04-23 15:09:33 UTC Comment hidden (obsolete)
Comment 38 axel 2019-04-23 15:09:36 UTC Comment hidden (obsolete)
Comment 39 axel 2019-04-23 15:12:03 UTC Comment hidden (obsolete)
Comment 40 axel 2019-04-23 15:15:07 UTC Comment hidden (obsolete)
Comment 41 David Cook 2019-04-24 00:06:11 UTC
Anyone else think that it's a terrible idea to have authentication plugins that non-technical staff can load into Koha? Sounds like a massive security problem waiting to happen.

That said, I'm in favour of authentication "plugins" that administrators can add to the system via system packages or CPAN.
Comment 42 David Cook 2019-04-24 00:11:49 UTC
I know things like WordPress have authentication plugins, but WordPress is also the biggest security nightmare on the Internet.
Comment 43 David Cook 2019-04-24 00:19:09 UTC
I think we should ask ourselves what we're trying to achieve here. Are we adding authentication plugins via the Staff UI, because it's too difficult to get changes into Koha, especially around authentication?

I would love for there to be more authentication methods for Koha. In fact, I wrote a generic OpenID Connect client for Koha, which I support locally. 

Of course, one of the reasons why I haven't tried to get it into Koha (besides sharing some code at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21586 for other people to use) is that I thought I'd never be able to get changes to C4::Auth pushed. Partially because I didn't have enough time/money to dedicate to working on it for the community, and partially because I thought that I didn't have enough influence/power within the community.

I know that I've heard grumblings behind closed doors from even prominent community folk about it being too hard to get changes into Koha these days. I wonder if the desire for user-managed authentication plugins is indicative of a larger problem within the Koha community.
Comment 44 David Cook 2019-04-24 00:22:18 UTC
Just one final note... if this does make it into Koha, it'll be one of the first things I disable when upgrading to a new release. 

I think plugins are awesome but also dangerous when wielded by people without enough knowledge. 

(For instance, EBSCO have made an authentication plugin, which was installed by a librarian, and I discovered that it had a number of security vulnerabilities. I've sent in pull requests to patch them, but I never would've installed that plugin without reviewing it first. However, the librarian didn't even think to verify the contents of the plugin.)
Comment 45 Katrin Fischer 2019-04-24 05:37:27 UTC
The plugin idea Alex is working on here is not new - see the discussion in the first comments. For added security we could discuss different solutions, like a separate permission for handling authentication plugins maybe?

I don't agree with people saying it's too hard to get things in. But I definitely think we should not have this discussion on bugzilla. The dev meeting today would be a better place.
Comment 46 Chris Cormack 2019-04-24 05:43:27 UTC
I think what David meant to say was thank you Alex, for testing and providing your first patch to Koha to fix a broken test.  Then gone on with the discussion of technical merits 

Rest assured we do value your contribution, and we want Koha to be a welcoming place.
Comment 47 Chris Cormack 2019-04-24 05:45:03 UTC
My apologies Axel, my phone autocorrected your name to Alex
Comment 48 Alex Arnaud 2019-04-24 09:02:14 UTC
(In reply to David Cook from comment #41)
> Anyone else think that it's a terrible idea to have authentication plugins
> that non-technical staff can load into Koha? Sounds like a massive security
> problem waiting to happen.
> 
> That said, I'm in favour of authentication "plugins" that administrators can
> add to the system via system packages or CPAN.

I do understand this argument. I even agree that plugins (and not only authentication ones) could contain security issues. 
For "our" hosted libraries, we disabled writing permission on plugins directory.
Looks like a tricky solution and we probably need a better one but it means that administrators have the final word.

> I think we should ask ourselves what we're trying to achieve here. Are we
> adding authentication plugins via the Staff UI, because it's too difficult to
> get changes into Koha, especially around authentication?
> I would love for there to be more authentication methods for Koha. In fact, I
> wrote a generic OpenID Connect client for Koha, which I support locally.

IMO plugins are useful (even essential) to satisfy specific libraries requests and not to avoid community processes.
i wrote this patch in order to create an authentication plugins that can request many LDAP backends and fallback on an other one.
Seems too specific to be suggested to the community.
To go further, as discussed above, i think we should consider generally LDAP, CAS etc... as specific feature that would become plugins (may be another debate).

To return to security topic:
Today, many free plugable systems provide repositories with a large amount of plugins that have been reviewed, tested and validated by their community as safe. Users can easily download ones from other sources but they know it's at their own risk.
Maybe we should be inspired by that.
Comment 49 Martin Renvoize (ashimema) 2019-04-24 10:41:59 UTC
(In reply to David Cook from comment #41)
> Anyone else think that it's a terrible idea to have authentication plugins
> that non-technical staff can load into Koha? Sounds like a massive security
> problem waiting to happen.

This is a manageable risk. I believe we should end up with a repository of signed and trusted plugins as per Alex's response (comment #48) and have a granular set of permissions on the server as to what types of plugins may be installed via the client.  I've been wanting to work towards this for some time, along with adding translations to plugins and generally enhancing the system as a whole.. but these things all need sponsorship, time and money.

(In reply to David Cook from comment #43)
> I think we should ask ourselves what we're trying to achieve here. Are we 
> adding authentication plugins via the Staff UI, because it's too difficult to 
> get changes into Koha, especially around authentication?

Koha is a well established and highly reliable system these days, relied upon by countless libraries. I am a firm believer in our quality assurance policies and the both the commit logs and release notes serve to prove that the software is still moving forward rapidly with enhancements and new features continually being integrated.  Yes, we could always do with more hands/eyes on the code and people supporting each other by offering SO and QA time. That is something I intend to work on if I am elected to be RM over the next two cycles, but I think it's very unfair to suggest one has to have extensive influence/power to get code into the community. I believe we are a very supportive and friendly community in 99% of cases and I would certainly support anyone's efforts to get code in.. sometimes it is hard to pick which bugs to focus on and I'm always open to suggestions via any means (email, irc, bugzilla priorities)

Finally, I concur with Chris, thankyou very much for taking the time to both look at this and submit followup code Axel. I will take a look at it all at my earliest convenience and attempt to help get things moving again.
Comment 50 Fridolin Somers 2019-04-24 15:03:07 UTC
> have a granular set of permissions on the server
What about adding to koha-conf.xml a boolean to allow or not uploading plugins ?
If not allowed only system admin can add plugins.

My 2c
Comment 51 Martin Renvoize (ashimema) 2019-04-24 18:10:45 UTC
(In reply to Fridolin SOMERS from comment #50)
> > have a granular set of permissions on the server
> What about adding to koha-conf.xml a boolean to allow or not uploading
> plugins ?
> If not allowed only system admin can add plugins.
> 
> My 2c

You can already enable and disable plugins entirely from koha-conf can't you.. I was thinking more having classifactions of plugins so you could allow a whitelist of supported ones for example.. or say.. all cataloguing plugins but not auth plugins.

I like the idea of a whitelist.
Comment 52 David Cook 2019-04-29 00:28:31 UTC
(In reply to Katrin Fischer from comment #45)
> The plugin idea Alex is working on here is not new - see the discussion in
> the first comments. For added security we could discuss different solutions,
> like a separate permission for handling authentication plugins maybe?
> 

I see the logic of adding permissions, but I suppose I'm concerned about small libraries where a solo librarian might have all the permissions, but might not make the best choices with those permissions.

> I don't agree with people saying it's too hard to get things in. But I
> definitely think we should not have this discussion on bugzilla. The dev
> meeting today would be a better place.

Yes, perhaps Bugzilla is not the best place to have that discussion. I wasn't aware of the dev meeting until after it was finished, and it seems that it was at 5am my time, so I was still (barely) asleep. I think they're usually at 5am or 12am my time, which makes them difficult to attend.
Comment 53 David Cook 2019-04-29 00:35:58 UTC
(In reply to Chris Cormack from comment #46)
> I think what David meant to say was thank you Alex, for testing and
> providing your first patch to Koha to fix a broken test.  Then gone on with
> the discussion of technical merits 
> 
> Rest assured we do value your contribution, and we want Koha to be a
> welcoming place.

(In reply to Chris Cormack from comment #46)
> My apologies Axel, my phone autocorrected your name to Alex

Chris is absolutely right. 

I must admit that my eyes actually autocorrected "axel" to "Alex", and I thought the latest patches were follow-up from Alex Arnaud, so I didn't think twice. Looking back, thanks for the contributions, axel! I have no criticism of them.
Comment 54 David Cook 2019-04-29 00:48:25 UTC
(In reply to Alex Arnaud from comment #48)
> I do understand this argument. I even agree that plugins (and not only
> authentication ones) could contain security issues. 
> For "our" hosted libraries, we disabled writing permission on plugins
> directory.
> Looks like a tricky solution and we probably need a better one but it means
> that administrators have the final word.
> 

That's really interesting to know. That's probably the most logical way to do it presently, but I agree that it would be nice to have a more elegant solution. I think that's the key thing I'd like to see come out of this discussion really. 

> IMO plugins are useful (even essential) to satisfy specific libraries
> requests and not to avoid community processes.
> i wrote this patch in order to create an authentication plugins that can
> request many LDAP backends and fallback on an other one.
> Seems too specific to be suggested to the community.
> To go further, as discussed above, i think we should consider generally
> LDAP, CAS etc... as specific feature that would become plugins (may be
> another debate).

I totally agree in theory. I would love to see all the authentication methods structured as plugins that can be added/removed as necessary, although I think it should be done by administrators rather than librarians. 

> To return to security topic:
> Today, many free plugable systems provide repositories with a large amount
> of plugins that have been reviewed, tested and validated by their community
> as safe. Users can easily download ones from other sources but they know
> it's at their own risk.
> Maybe we should be inspired by that.

I'd argue that "they know it's at their own risk" isn't necessarily true. It's like how many people sign contracts without reading them, or tick the "Terms and Conditions" box without reading the Terms and Conditions. People seem to just assume that nothing bad will ever happen to them. 

However, I like the sound of the plugins being reviewed, tested, and validated by their community. I recall there being an unwillingness to provide a repository for fear that it would create an "endorsement" of plugins by being in the repository, but... I think you're right. People read and trust reviews. If they had a centralized place for reviewing plugins, I think that could really build confidence in using them, and provide people without technical knowledge a source to make more informed decisions.
Comment 55 David Cook 2019-04-29 00:59:41 UTC
(In reply to Martin Renvoize from comment #49)
> This is a manageable risk. I believe we should end up with a repository of
> signed and trusted plugins as per Alex's response (comment #48) and have a
> granular set of permissions on the server as to what types of plugins may be
> installed via the client.  I've been wanting to work towards this for some
> time, along with adding translations to plugins and generally enhancing the
> system as a whole.. but these things all need sponsorship, time and money.
> 

I love the sound of all of that, and I really relate in terms of time and money. I know I must sound like a troll by leaving lots of comments and not "doing" anything about them, but it's all I have time for at the moment. 

> Koha is a well established and highly reliable system these days, relied
> upon by countless libraries. I am a firm believer in our quality assurance
> policies and the both the commit logs and release notes serve to prove that
> the software is still moving forward rapidly with enhancements and new
> features continually being integrated.  Yes, we could always do with more
> hands/eyes on the code and people supporting each other by offering SO and
> QA time. That is something I intend to work on if I am elected to be RM over
> the next two cycles, but I think it's very unfair to suggest one has to have
> extensive influence/power to get code into the community. I believe we are a
> very supportive and friendly community in 99% of cases and I would certainly
> support anyone's efforts to get code in.. sometimes it is hard to pick which
> bugs to focus on and I'm always open to suggestions via any means (email,
> irc, bugzilla priorities)
> 

This is very true. I should have chosen my words more carefully. The Koha community is very supportive and very friendly. I don't think there's an open source community that is warmer and receptive than this one. I'm reminded of that every time I look at the photo of my child in a "Future Koha Developer" onesie that Katrin sent us when bub was born.

It's true that you don't need extensive influence/power to get code into the community, and I use my first patch as evidence of that: http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=b49af1df17c06cb41d168fa154cb2104798c33cf. I'd been using Koha for less than 2 months at that point, and I didn't really know anyone in the Koha community.

I think my earlier comment about influence/power relates to particular frustrations experienced by individuals that can be discussed elsewhere. 

Good luck with the elections, Martin! Koha would be very safe in your hands!
Comment 56 David Cook 2019-04-29 01:03:25 UTC
(In reply to Martin Renvoize from comment #51)
> (In reply to Fridolin SOMERS from comment #50)
> > > have a granular set of permissions on the server
> > What about adding to koha-conf.xml a boolean to allow or not uploading
> > plugins ?
> > If not allowed only system admin can add plugins.
> > 
> > My 2c
> 
> You can already enable and disable plugins entirely from koha-conf can't
> you.. I was thinking more having classifactions of plugins so you could
> allow a whitelist of supported ones for example.. or say.. all cataloguing
> plugins but not auth plugins.
> 
> I like the idea of a whitelist.

I think Fridolin was talking about leaving plugins enabled, but disabling the ability to upload via the Web UI. I've thought about doing this myself. 

A whitelist could be interesting. I also liked your mention earlier about signed plugins. One way of whitelisting could be to only allow plugins signed by keys you trust. I mean... that's how a lot of software installers already work, right?
Comment 57 Mark Tompsett 2019-07-05 02:26:27 UTC
Created attachment 91304 [details] [review]
Bug 20340 - Ability to add and use authentication plugins

Plugins must be placed in <pluginsdir> and enabled with
AuthenticationModule system preference.

Test plan:

  - Apply this patch,
  - set UseKohaPlugins to enabled,
  - define a pluginsdir in your koha-conf.xml,
  - put a plugin in this directory.
  - I.e, take the one provided in tests directory:
     - t/Koha/Plugin/TestAuth.pm,
     - login/pass is test/test
  - You should have:
      /your/plugins/dir/Koha/Plugin/TestAuth.pm,
  - test authentication with and without the plugin.
Comment 58 Mark Tompsett 2019-07-05 02:26:32 UTC
Created attachment 91305 [details] [review]
Bug 20340: Followup to allow OPAC login to complete

The Plugin authenticates correctly, the problem is there
is no patron to match with, and so there is nothing to check
for authorization and user-details once a login is attempted.
This will at least let it fail gracefully.
Comment 59 Mark Tompsett 2019-07-05 02:26:37 UTC
Created attachment 91306 [details] [review]
Bug 20340: add of empty method configure
Comment 60 Mark Tompsett 2019-07-05 02:32:37 UTC Comment hidden (obsolete)
Comment 61 Mark Tompsett 2019-07-05 02:41:56 UTC
Created attachment 91307 [details] [review]
Bug 20340: (follow-up) fix some of Auth.t's tests
Comment 62 Mark Tompsett 2019-07-05 02:43:24 UTC
kshell
prove -v t/db_dependent/Auth.t
-- failed.
Was I supposed to put the plugin somewhere? Perhaps that was it?
Comment 63 Mark Tompsett 2019-07-05 05:16:20 UTC
Comment on attachment 91304 [details] [review]
Bug 20340 - Ability to add and use authentication plugins

Review of attachment 91304 [details] [review]:
-----------------------------------------------------------------

::: t/Koha/Plugin/TestAuth.pm
@@ +49,5 @@
> +
> +    if ( $key eq 'priority' ) {
> +        return 1;
> +    }
> +

Please add $self->SUPER::retrieve_data($key);
Otherwise the plugin isn't listed as Enabled which is really scary to an end user.
Comment 64 Mark Tompsett 2019-07-05 05:17:37 UTC
# Subtest: no_set_userenv parameter tests
    1..13
    ok 1 - checkpw returns true
    ok 2 - Userenv should be undef as required
    ok 3 - Userenv gives correct branch
    ok 4 - checkpw returns true
    ok 5 - Userenv branch is preserved if no_set_userenv is true
    ok 6 - checkpw still returns true
    not ok 7 - Userenv branch is overwritten if no_set_userenv is false

    #   Failed test 'Userenv branch is overwritten if no_set_userenv is false'
    #   at t/db_dependent/Auth.t line 143.
    #          got: undef
    #     expected: anything else
    ok 8 - With TestAuth plugin, checkpw returns 0
    ok 9 - With TestAuth plugin, checkpw returns empty cardnumber
    ok 10 - With TestAuth plugin, checkpw returns empty userid
    not ok 11 - With TestAuth plugin, checkpw returns 1
    not ok 12 - With TestAuth plugin, checkpw returns test cardnumber

    #   Failed test 'With TestAuth plugin, checkpw returns 1'
    #   at t/db_dependent/Auth.t line 151.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'With TestAuth plugin, checkpw returns test cardnumber'
    #   at t/db_dependent/Auth.t line 152.
    #          got: undef
    #     expected: 'test'
    not ok 13 - With TestAuth plugin, checkpw returns test userid

    #   Failed test 'With TestAuth plugin, checkpw returns test userid'
    #   at t/db_dependent/Auth.t line 153.
    #          got: undef
    #     expected: 'test'
    # Looks like you failed 4 tests of 13.
not ok 4 - no_set_userenv parameter tests

#   Failed test 'no_set_userenv parameter tests'
#   at t/db_dependent/Auth.t line 154.