Bug 27311 - Don't pass borrowernumber to check_password plugin hook
Summary: Don't pass borrowernumber to check_password plugin hook
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Magnus Enger
QA Contact: Testopia
URL:
Keywords:
Depends on: 22706
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-29 11:25 UTC by Magnus Enger
Modified: 2023-11-03 16:58 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 27311 - Don't pass borrowernumber to check_password (2.56 KB, patch)
2020-12-29 11:44 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-29 11:25:45 UTC
check_password is implemented 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 call to the plugin inside Koha::Patron::store comes before this line:

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

which means there is not yet a borrowernumber to pass to the plugin, and this never worked.
Comment 1 Magnus Enger 2020-12-29 11:44:57 UTC
Created attachment 114721 [details] [review]
Bug 27311 - Don't pass borrowernumber to check_password

Bug 22706 added the check_password plugin hook, taking both a
borrowernumber and the raw password as arguments. The hook is
called in two places in Koha::Patron, but because of how the code
that calls the hook is placed, passing borrowernumber to the plugin
inside "sub store" never worked. This patch removes the passing
of the borrowernumber from both calls.

A tiny plugin that uses the check_password hooks can ve found here:
https://github.com/Libriotech/koha-plugin-pin

To test:
- Enable plugins
- Optionally install the PIN 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

Sign off, if all this works as expected.
Comment 2 Jonathan Druart 2021-01-04 09:57:31 UTC
I understand why you remove it from ->store (because in the creation block), but why from set_password?
Comment 3 David Cook 2021-12-20 04:01:11 UTC
(In reply to Magnus Enger from comment #0)
> which means there is not yet a borrowernumber to pass to the plugin, and
> this never worked.

I was just about to say this when I noticed the bug reference...
Comment 4 Lucas Gass 2023-11-03 16:58:45 UTC
1. Shouldn't this have a test to account for the change? 

2. Processing additional checks

	* Commit title does not start with 'Bug XXXXX: ' - 68f151152e1