Description
Magnus Enger
2019-04-15 13:00:19 UTC
Created attachment 93488 [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. 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". 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. :-) Created attachment 93526 [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. 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> 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. 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. Created attachment 94613 [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". 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 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 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> Created attachment 94673 [details] [review] Bug 22706: (QA follow-up) Restore location of logaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Created attachment 94675 [details] [review] Bug 22706: (QA follow-up) Restore location of logaction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (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? (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 Nice work! Pushed to master for 19.11.00 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> 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> Created attachment 94907 [details] [review] Bug 22706: (RM follow-up) Add unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Followups all pushed Nice work! Pushed to master for 19.11.00 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> 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? (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. (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! |