Bug 22706 - Add plugin hooks to allow custom password strength checking modules
Summary: Add plugin hooks to allow custom password strength checking modules
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Magnus Enger
QA Contact: Martin Renvoize
URL:
Keywords: Manual
Depends on:
Blocks:
 
Reported: 2019-04-15 13:00 UTC by Magnus Enger
Modified: 2019-11-27 10:48 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This new feature allows plugin authors to implement a `check_password` method to enable custom password validation routines. **Warning**: Care should be taken when installing any plugins and only plugins you trust should be used. The hook introduced here allows plugin authors to potentially steel plain text passwords during patron creations and updates.
Version(s) released in:
19.11.00


Attachments
Bug 22706 - Add plugin hooks for Norwegian national patron database (6.46 KB, patch)
2019-10-02 14:22 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22706 - Add plugin hooks for Norwegian national patron database (6.52 KB, patch)
2019-10-03 07:29 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 22706 - Add plugin hooks for Norwegian national patron database (7.81 KB, patch)
2019-10-23 12:23 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22706 - Add plugin hooks for Norwegian national patron database (7.87 KB, patch)
2019-10-23 12:25 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 22706 - Add plugin hooks for Norwegian national patron database (7.94 KB, patch)
2019-10-24 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (QA follow-up) Restore location of logaction (1.31 KB, patch)
2019-10-24 13:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: Add plugin hooks for Norwegian national patron database (7.94 KB, patch)
2019-10-24 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (QA follow-up) Restore location of logaction (1.31 KB, patch)
2019-10-24 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (RM follow-up) Remove use of Koha::Plugins::Handler (3.07 KB, patch)
2019-10-31 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (RM follow-up) Restore ability to create disabled accounts (2.36 KB, patch)
2019-10-31 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (RM follow-up) Add unit tests (985 bytes, patch)
2019-10-31 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22706: (RM follow-up) Add unit tests (3.58 KB, patch)
2019-10-31 12:22 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2019-04-15 13:00:19 UTC
Support for the Norwegian national patron database was added to Koha in bug 11401, then removed again in bug 21068. We need to add some new capabilities to the plugin system to be able to replace the removed code with a plugin.
Comment 1 Magnus Enger 2019-10-02 14:22:52 UTC Comment hidden (obsolete)
Comment 2 Magnus Enger 2019-10-02 14:28:00 UTC
So let me just repeat that I think integration with the Norwegian national patron database is the main point of this patch, and that testing for regressions should be sufficient to sign off. 

The password checking is a nice side effect, but it can and should be extended and improved in some other bug than this.

And yes, this makes it possible to create a plugin that steals passwords. But I think plugins are pretty powerfull already, and we should always inspect the contents of plugins before we install them. :-)
Comment 3 Brendan Gallagher 2019-10-03 07:29:41 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2019-10-23 11:33:34 UTC
Note: I thought maybe there would be overlap between this bug and bug 20340, but there isn't. This bug adds plugin hooks when passwords are added or updated, bug 20340 adds hooks when a user tries to log in.
Comment 5 Martin Renvoize 2019-10-23 12:01:40 UTC
QA comments.. discussed in IRC but adding here for note.

1) The password check should be done before the call to `$self = $self->SUPER::store;` to ensure we throw an error before the patron data is stored.
2) The plugin should respect the 'skip_validation' parameter when passed to set_password.

Otherwise, I'm generally happy with the approach here.

Side note.. I wasn't entirely sure how plugins should interact with the existing system preferences available for password enforcement. (whether the plugin should work along with the existing preferences or outright replace them)  As it stands they will work in combination with system preferences.. this issue can be easily resolved by documenting how they interact... I'm happy with either approach.
Comment 6 Magnus Enger 2019-10-23 12:23:14 UTC Comment hidden (obsolete)
Comment 7 Magnus Enger 2019-10-23 12:25:29 UTC
Created attachment 94614 [details] [review]
Bug 22706 - Add plugin hooks for Norwegian national patron database

The main point of this patch is to make it possible to integrate Koha
with the Norwegian national patron database (NNPDB). Code for this was
earlier introduced in Bug 11401 and removed again in Bug 21068.

To test this is mainly a question of spotting regressions, it should
still be possible to set and change a password in all possible ways:
- Setting a password for a new user
- Changing a password in the staff client
- Changing a password in the OPAC
If these work as expected, everything should be OK.

A nice side effect of this work is that it will allow for plugins that
validate passwords. I have created a tiny plugin that enforces PIN
codes of 4 digits. (Yeah, I know, those are the worst passwords, but
some libraries do require them.) It is published here:
https://github.com/Libriotech/koha-plugin-pin
To test this way, install the plugin and try to change the password
of an exsisting user to something that is not a 4 digit PIN. You
should get an error that says "The password was rejected by a plugin".

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Updated 2019-10-23:
- Moved the plugin checks to before the call to $self->SUPER::store to
  make sure patrons are not saved if the password fails a plugin check
- Made the plugin checks in set_password respect skip_validation while
  retaining the functionality for NNPDB
Comment 8 Martin Renvoize 2019-10-24 13:42:13 UTC
Created attachment 94672 [details] [review]
Bug 22706 - Add plugin hooks for Norwegian national patron database

The main point of this patch is to make it possible to integrate Koha
with the Norwegian national patron database (NNPDB). Code for this was
earlier introduced in Bug 11401 and removed again in Bug 21068.

To test this is mainly a question of spotting regressions, it should
still be possible to set and change a password in all possible ways:
- Setting a password for a new user
- Changing a password in the staff client
- Changing a password in the OPAC
If these work as expected, everything should be OK.

A nice side effect of this work is that it will allow for plugins that
validate passwords. I have created a tiny plugin that enforces PIN
codes of 4 digits. (Yeah, I know, those are the worst passwords, but
some libraries do require them.) It is published here:
https://github.com/Libriotech/koha-plugin-pin
To test this way, install the plugin and try to change the password
of an exsisting user to something that is not a 4 digit PIN. You
should get an error that says "The password was rejected by a plugin".

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Updated 2019-10-23:
- Moved the plugin checks to before the call to $self->SUPER::store to
  make sure patrons are not saved if the password fails a plugin check
- Made the plugin checks in set_password respect skip_validation while
  retaining the functionality for NNPDB

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2019-10-24 13:42:16 UTC
Created attachment 94673 [details] [review]
Bug 22706: (QA follow-up) Restore location of logaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-10-24 13:43:14 UTC
Created attachment 94674 [details] [review]
Bug 22706: Add plugin hooks for Norwegian national patron database

The main point of this patch is to make it possible to integrate Koha
with the Norwegian national patron database (NNPDB). Code for this was
earlier introduced in Bug 11401 and removed again in Bug 21068.

To test this is mainly a question of spotting regressions, it should
still be possible to set and change a password in all possible ways:
- Setting a password for a new user
- Changing a password in the staff client
- Changing a password in the OPAC
If these work as expected, everything should be OK.

A nice side effect of this work is that it will allow for plugins that
validate passwords. I have created a tiny plugin that enforces PIN
codes of 4 digits. (Yeah, I know, those are the worst passwords, but
some libraries do require them.) It is published here:
https://github.com/Libriotech/koha-plugin-pin
To test this way, install the plugin and try to change the password
of an exsisting user to something that is not a 4 digit PIN. You
should get an error that says "The password was rejected by a plugin".

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Updated 2019-10-23:
- Moved the plugin checks to before the call to $self->SUPER::store to
  make sure patrons are not saved if the password fails a plugin check
- Made the plugin checks in set_password respect skip_validation while
  retaining the functionality for NNPDB

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-10-24 13:43:17 UTC
Created attachment 94675 [details] [review]
Bug 22706: (QA follow-up) Restore location of logaction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Jonathan Druart 2019-10-24 14:18:26 UTC
(In reply to Magnus Enger from comment #2)
> And yes, this makes it possible to create a plugin that steals passwords.
> But I think plugins are pretty powerfull already, and we should always
> inspect the contents of plugins before we install them. :-)

Shall not we have a special alert during the install process of the plugin if it has this capability?
Comment 13 Martin Renvoize 2019-10-24 14:29:01 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Magnus Enger from comment #2)
> > And yes, this makes it possible to create a plugin that steals passwords.
> > But I think plugins are pretty powerfull already, and we should always
> > inspect the contents of plugins before we install them. :-)
> 
> Shall not we have a special alert during the install process of the plugin
> if it has this capability?

Hmm, 

I sort of agree but believe that's a little out of scope on this specific bug...

I'm going to open another bug with the in mind as I think the question is broader.

See bug 23890
Comment 14 Martin Renvoize 2019-10-29 12:20:58 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Martin Renvoize 2019-10-31 11:23:59 UTC
Created attachment 94905 [details] [review]
Bug 22706: (RM follow-up) Remove use of Koha::Plugins::Handler

Using both Koha::Plugins and Koha::Plugins::Handler inside the same
class causes issues for runtime module loading which in turn caused all
the plugins tests to fail.

This patch converts this patchset to use the direct call style used
elsewhere in Koha removing the need for Koha::Plugins::Handler.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-10-31 11:24:03 UTC
Created attachment 94906 [details] [review]
Bug 22706: (RM follow-up) Restore ability to create disabled accounts

The patchset inadvertantly disabled the ability to create disabled user
accounts when any check_password plugin was installed.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-10-31 11:24:06 UTC
Created attachment 94907 [details] [review]
Bug 22706: (RM follow-up) Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2019-10-31 11:24:49 UTC
Followups all pushed
Comment 19 Martin Renvoize 2019-10-31 12:21:49 UTC
Nice work!

Pushed to master for 19.11.00
Comment 20 Martin Renvoize 2019-10-31 12:22:11 UTC
Created attachment 94910 [details] [review]
Bug 22706: (RM follow-up) Add unit tests

Helps if one actually adds the new test file to the commit!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Katrin Fischer 2019-11-19 01:09:53 UTC
We have a documentation page for plugin hooks started on the wiki:
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks

Can these be added there please?
Comment 22 Magnus Enger 2019-11-19 07:12:32 UTC
(In reply to Katrin Fischer from comment #21)
> We have a documentation page for plugin hooks started on the wiki:
> https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> 
> Can these be added there please?

Done. Thanks for the reminder.
Comment 23 Katrin Fischer 2019-11-19 09:14:30 UTC
(In reply to Magnus Enger from comment #22)
> (In reply to Katrin Fischer from comment #21)
> > We have a documentation page for plugin hooks started on the wiki:
> > https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
> > 
> > Can these be added there please?
> 
> Done. Thanks for the reminder.

Thank you!