Bug 27309 - Add plugin hooks for capturing raw passwords (NNPDB)
Summary: Add plugin hooks for capturing raw passwords (NNPDB)
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Magnus Enger
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-12-28 13:21 UTC by Magnus Enger
Modified: 2023-11-06 22:45 UTC (History)
3 users (show)

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


Attachments
Bug 27309 - Add plugin hooks for capturing passwords (3.21 KB, patch)
2020-12-29 11:03 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2020-12-28 13:21:42 UTC
Bug 22706 tried to add hooks to make it possible to implement support for the Norwegian National Patron Database (NNPDB) via a plugin. That bug added a new hook called check_password, and this was called in two places: 

1. In Koha::Patron::set_password
https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/Patron.pm#L774

2. In Koha::Patron::store
https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/Patron.pm#L167

The second call to check_password was initially placed after this call:

  $self = $self->SUPER::store;

so we could send both the raw password and the borrowernumber to the hook. This is of course a problem, since checking the password after the borrower has been stored means we can't reject the user completely based on the password. And so the call to the check_password hook was placed bwfore this line. This meant that the check_password hook could not serve the intended purpose of implementing the NNPDB. 

This bug proposes a new plugin hook, to be implemented after the ->store, to finally make it possible to implement NNPDB as a plugin.
Comment 1 Magnus Enger 2020-12-29 11:03:33 UTC
Created attachment 114719 [details] [review]
Bug 27309 - Add plugin hooks for capturing passwords

This might sound like a stupid idea, but it is necessary in order
to implement support for the Norwegian National Patron Database
as a plugin in Koha. The plugin hook captures the borrowernumber
and raw password of a user and sends it to a plugin.

I created a tiny plugin to be able to test the functionality:
https://github.com/Libriotech/koha-plugin-capturepw
This plugin will capture borrowernumbers and raw passwords
when a user is added or updated. The borrowernumber and a
md5-hashed version of the password is stored in a tiny database
table (koha_plugin_no_libriotech_capturepw_capturepw) created by
the plugin.

To test:
- Enable plugins
- Install the CapturePW plugin
- Add and edit users in all possible ways, including
-- Setting a password for a new user
-- Changing a password in the staff client
-- Changing a password in the OPAC
-- Creating a disabled user account

Check the database table for updates after each operation. If a new
line is added to the database for each operation, and the md5 hash
changes when a password is changed, everything should be ok.
Comment 2 Magnus Enger 2020-12-29 11:22:19 UTC
Added capture_raw_password to https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks#Implemented_or_under_development_hooks
Comment 3 David Cook 2021-01-04 01:34:24 UTC
Could you elaborate more on your use case for this? I don't understand why you'd need to capture and store the password.
Comment 4 Magnus Enger 2021-08-16 13:02:35 UTC
(In reply to David Cook from comment #3)
> Could you elaborate more on your use case for this? I don't understand why
> you'd need to capture and store the password.

In Norway we have a national patron database, that among other things holds patron's PIN codes. These are encrypted using a key that only vendors have access to. To be able to store and transmit the encypted PIN in the correct format, we need to be able to capture it, and encrypt it in the way the national patron database expects, so we can sync it to other ILSes. 

This was not my idea, and it might change in the future, but for now it is something we have to implement, in order to be able to answer RFPs etc.
Comment 5 David Cook 2021-08-16 23:33:06 UTC
(In reply to Magnus Enger from comment #4)
> (In reply to David Cook from comment #3)
> > Could you elaborate more on your use case for this? I don't understand why
> > you'd need to capture and store the password.
> 
> In Norway we have a national patron database, that among other things holds
> patron's PIN codes. These are encrypted using a key that only vendors have
> access to. To be able to store and transmit the encypted PIN in the correct
> format, we need to be able to capture it, and encrypt it in the way the
> national patron database expects, so we can sync it to other ILSes. 
> 
> This was not my idea, and it might change in the future, but for now it is
> something we have to implement, in order to be able to answer RFPs etc.

Thanks for explaining. I think that I understand more now. I just took a peek at bug 11401 and https://lanekortet.no/ (thanks Google Translate). 

I wonder why they don't use something like LDAP or use SSO. Inertia maybe. 

I still think how the NNPDB works is bizarre but I understand the rationale.
Comment 6 Lucas Gass 2023-11-06 22:45:28 UTC
Do we need unit tests for this change? 

Also, the commit title needs amended:

Processing additional checks

	* Commit title does not start with 'Bug XXXXX: ' - 82ca5ad8aef