Bug 17746 - koha-reset-passwd should use Koha::Patron->set_password
Summary: koha-reset-passwd should use Koha::Patron->set_password
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
: 19321 (view as bug list)
Depends on:
Blocks: 21976 23193
  Show dependency treegraph
 
Reported: 2016-12-08 10:15 UTC by Magnus Enger
Modified: 2020-06-04 20:32 UTC (History)
13 users (show)

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


Attachments
Bug 17746: Add misc/admin/set_password.pl script (4.29 KB, patch)
2019-02-05 17:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17746: Make koha-reset-passwd user set_password.pl (2.51 KB, patch)
2019-02-05 17:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21446: Remove fix comment (1.36 KB, patch)
2019-02-08 20:23 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 17746: Add misc/admin/set_password.pl script (4.36 KB, patch)
2019-02-08 20:24 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required (3.77 KB, patch)
2019-02-20 14:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17746: Make koha-reset-passwd user set_password.pl (2.56 KB, patch)
2019-03-23 01:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17746: Add misc/admin/set_password.pl script (4.41 KB, patch)
2019-03-23 01:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required (3.82 KB, patch)
2019-03-23 01:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 17746: Make koha-reset-passwd user set_password.pl (2.61 KB, patch)
2019-03-23 08:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17746: Add misc/admin/set_password.pl script (4.46 KB, patch)
2019-03-23 08:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required (3.87 KB, patch)
2019-03-23 08:56 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2016-12-08 10:15:15 UTC
debian/scripts/koha-reset-passwd uses MD5 to hash passwords, when it is used to reset a password: 

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=debian/scripts/koha-reset-passwd;h=317e2bce66bdea0183dec28136d2e17f02ddea1e;hb=HEAD

It would probably be better if it used Bcrypt, the way it is done in Koha proper? 

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=C4/Auth.pm;h=6b7dc6602a11e3ff07ba9e4242fec58af300f580;hb=HEAD#l1862
Comment 1 Chris Cormack 2017-01-24 01:58:47 UTC
To make this possible we really need to make sure Koha::AuthUtils is in the perl path.

Are we still doing the bad idea that the scripts should work even without the packages? (I think this is a really bad idea, and if so they shouldn't be in debian/)

If we can use say these scripts only work with the packages, then we know what the path will be and this can be easily fixed.
Comment 2 Marcel de Rooy 2017-01-24 14:51:05 UTC
I think it is a valid assumption that the debian scripts belong to the debian package installs.
But I do also think that we should prevent putting hardcoded paths in scripts, if we can do it nicer with only a bit more effort.

And I do not think that this construction in the script should win a price:
echo "UPDATE borrowers SET password = '$digest' WHERE userid = '$userid';"
Why not create a small script to do so and call that if needed. (That script on itself should just call a module again.)
Comment 3 Jonathan Druart 2017-09-15 13:08:49 UTC
*** Bug 19321 has been marked as a duplicate of this bug. ***
Comment 4 David Cook 2017-09-20 00:05:35 UTC
(In reply to Chris Cormack from comment #1)
> Are we still doing the bad idea that the scripts should work even without
> the packages? (I think this is a really bad idea, and if so they shouldn't
> be in debian/)
> 
> If we can use say these scripts only work with the packages, then we know
> what the path will be and this can be easily fixed.

I've been thinking for years that the scripts should be moved out of debian/ so that they could work with any Koha install, but after looking through the scripts in more depth I think that would actually take a lot of work and might actually not even be possible, since it makes a lot of assumptions that aren't enforced anywhere in Koha presently. So it might not be worth the effort. So probably safe to hard-code the package conventions into it.

(That said, I'd love to make RPMs for Koha, but having the scripts in debian/scripts would require duplicating the scripts just for the RPMs or not having the package scripts for a RPM package, which seems suboptimal, since the same layout, conventions, etc could be used by RPMs. So I think maybe there is some merit in putting the scripts somewhere else... but still have them be package-specific... alas I don't have the time to really work on it. I've been thinking of putting together a RPM specfile and getting that pushed to Koha so that other people might be inspired to look at how the package scripts might be changed to work for both those kinds of packages.)
Comment 5 Tomás Cohen Arazi 2017-09-20 00:37:19 UTC
(In reply to David Cook from comment #4)
> (In reply to Chris Cormack from comment #1)
> > Are we still doing the bad idea that the scripts should work even without
> > the packages? (I think this is a really bad idea, and if so they shouldn't
> > be in debian/)
> > 
> > If we can use say these scripts only work with the packages, then we know
> > what the path will be and this can be easily fixed.
> 
> I've been thinking for years that the scripts should be moved out of debian/
> so that they could work with any Koha install, but after looking through the
> scripts in more depth I think that would actually take a lot of work and
> might actually not even be possible, since it makes a lot of assumptions
> that aren't enforced anywhere in Koha presently. So it might not be worth
> the effort. So probably safe to hard-code the package conventions into it.
> 
> (That said, I'd love to make RPMs for Koha, but having the scripts in
> debian/scripts would require duplicating the scripts just for the RPMs or
> not having the package scripts for a RPM package, which seems suboptimal,
> since the same layout, conventions, etc could be used by RPMs. So I think
> maybe there is some merit in putting the scripts somewhere else... but still
> have them be package-specific... alas I don't have the time to really work
> on it. I've been thinking of putting together a RPM specfile and getting
> that pushed to Koha so that other people might be inspired to look at how
> the package scripts might be changed to work for both those kinds of
> packages.)

That was the idea behind the cluster-mode install bug. I wish I could spend time on it! Fedora and others would be easily possible (modulo dependencies)
Comment 6 David Cook 2017-09-20 01:09:59 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> That was the idea behind the cluster-mode install bug. I wish I could spend
> time on it! Fedora and others would be easily possible (modulo dependencies)

A cluster-mode install bug sounds interesting. Personally, I've been thinking it would be useful if we could install multiple different versions as well. I've seen multiple versions of PostgreSQL installed like so: /var/lib/pgsql/<version>/<appdata> (e.g. /var/lib/pgsql/9.1/<appdata> alongside /var/lib/pgsql/9.3/<appdata>).

I don't know how that would work with the scripts currently in debian/scripts though for Koha since they operate at a global system level. Maybe the code for selecting instances could be shared across versions and that could be used to determine things like what version of the script needs to be used for a particular instance, but I don't know.
Comment 7 Katrin Fischer 2019-02-03 12:17:54 UTC
Bringing this discussion back to the bug... can we fix the password encryption and worry about where and how the scripts should work later/separately? :)
Comment 8 Tomás Cohen Arazi 2019-02-05 17:05:33 UTC
Created attachment 84797 [details] [review]
Bug 17746: Add misc/admin/set_password.pl script

This patch introduces a script that allows changing a patron's password.
The change overrides the defined password enforcement policy.

If multiple conditions are passed to match on the patron, they all need
to match. Otherwise an error message is printed.

Attributes to search the patron on:
- cardnumber
- patron_id (a.k.a. borrowernumber)
- userid

we usually know some of them, but if we specify more than one, they need
to match a patron, together.

To test:
1) Apply this patch
2) Have a known patron (i.e. you know the cardnumber, the borrowernumber
   and the userid).
3) Run:
  $ kshell
 k$ perl misc/admin/set_password.pl --cardnumber <the_card_number> \
                                    --password a_password
4) Verify you can login with the new password
5) Repeat 3) through 5) with --patron_id and --userid
=> SUCCESS: You can login in all cases
6) Try combining some or all the parameters
=> SUCCESS: It fails when it should, it succeeds when it should
7) Sign off :-D
Comment 9 Tomás Cohen Arazi 2019-02-05 17:05:37 UTC
Created attachment 84798 [details] [review]
Bug 17746: Make koha-reset-passwd user set_password.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2019-02-05 17:08:05 UTC
Ok, this is my approach. I've:

- Added a set_password.pl script that uses Koha's API internally to do the right thing, and has the flexibility to specify multiple patron matching criterias.
- Rewrote koha-reset-passwd so it just calls set_password.pl instead of the current templ file + onliner + SQL generation implementation.

The expected behaviour (for koha-reset-passwd) should remain. Check the first patch in order to test set_password.pl
Comment 11 Tomás Cohen Arazi 2019-02-05 17:09:24 UTC
*** Bug 21976 has been marked as a duplicate of this bug. ***
Comment 12 Pierre-Marc Thibault 2019-02-08 20:23:54 UTC Comment hidden (obsolete)
Comment 13 Pierre-Marc Thibault 2019-02-08 20:24:14 UTC
Created attachment 84899 [details] [review]
Bug 17746: Add misc/admin/set_password.pl script

This patch introduces a script that allows changing a patron's password.
The change overrides the defined password enforcement policy.

If multiple conditions are passed to match on the patron, they all need
to match. Otherwise an error message is printed.

Attributes to search the patron on:
- cardnumber
- patron_id (a.k.a. borrowernumber)
- userid

we usually know some of them, but if we specify more than one, they need
to match a patron, together.

To test:
1) Apply this patch
2) Have a known patron (i.e. you know the cardnumber, the borrowernumber
   and the userid).
3) Run:
  $ kshell
 k$ perl misc/admin/set_password.pl --cardnumber <the_card_number> \
                                    --password a_password
4) Verify you can login with the new password
5) Repeat 3) through 5) with --patron_id and --userid
=> SUCCESS: You can login in all cases
6) Try combining some or all the parameters
=> SUCCESS: It fails when it should, it succeeds when it should
7) Sign off :-D

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>
Comment 14 Pierre-Marc Thibault 2019-02-08 20:27:30 UTC
I can only sign off the first patch. Since it works, I did it. :)
Comment 15 Jonathan Druart 2019-02-16 16:43:01 UTC
What about generate a password if none is provided?
Comment 16 Tomás Cohen Arazi 2019-02-16 18:00:04 UTC
(In reply to Jonathan Druart from comment #15)
> What about generate a password if none is provided?

You talking about the set_password.pl script?
Comment 17 Jonathan Druart 2019-02-17 14:27:39 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> (In reply to Jonathan Druart from comment #15)
> > What about generate a password if none is provided?
> 
> You talking about the set_password.pl script?

Yep!
Comment 18 Tomás Cohen Arazi 2019-02-20 14:31:15 UTC
Created attachment 85363 [details] [review]
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required

This patch makes the set_password.pl script generate the password if it
is not passed as an argument.

It also changes the behaviour of set_password.pl: it will now print the
userid and password (generated or not) to mimick the behaviour from
koha-reset-passwd.

The koha-reset-passwd gets simplified as it doesn't generate the random
password anymore, it passes the responsability to set_password.pl

To test:
- Follow the steps from the original platch
- Verify all behaves as expected

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Liz Rea 2019-03-23 01:52:30 UTC
Created attachment 86932 [details] [review]
Bug 17746: Make koha-reset-passwd user set_password.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 20 Liz Rea 2019-03-23 01:52:33 UTC
Created attachment 86933 [details] [review]
Bug 17746: Add misc/admin/set_password.pl script

This patch introduces a script that allows changing a patron's password.
The change overrides the defined password enforcement policy.

If multiple conditions are passed to match on the patron, they all need
to match. Otherwise an error message is printed.

Attributes to search the patron on:
- cardnumber
- patron_id (a.k.a. borrowernumber)
- userid

we usually know some of them, but if we specify more than one, they need
to match a patron, together.

To test:
1) Apply this patch
2) Have a known patron (i.e. you know the cardnumber, the borrowernumber
   and the userid).
3) Run:
  $ kshell
 k$ perl misc/admin/set_password.pl --cardnumber <the_card_number> \
                                    --password a_password
4) Verify you can login with the new password
5) Repeat 3) through 5) with --patron_id and --userid
=> SUCCESS: You can login in all cases
6) Try combining some or all the parameters
=> SUCCESS: It fails when it should, it succeeds when it should
7) Sign off :-D

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 21 Liz Rea 2019-03-23 01:52:37 UTC
Created attachment 86934 [details] [review]
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required

This patch makes the set_password.pl script generate the password if it
is not passed as an argument.

It also changes the behaviour of set_password.pl: it will now print the
userid and password (generated or not) to mimick the behaviour from
koha-reset-passwd.

The koha-reset-passwd gets simplified as it doesn't generate the random
password anymore, it passes the responsability to set_password.pl

To test:
- Follow the steps from the original platch
- Verify all behaves as expected

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 22 Josef Moravec 2019-03-23 08:55:52 UTC
Created attachment 86937 [details] [review]
Bug 17746: Make koha-reset-passwd user set_password.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Josef Moravec 2019-03-23 08:55:56 UTC
Created attachment 86938 [details] [review]
Bug 17746: Add misc/admin/set_password.pl script

This patch introduces a script that allows changing a patron's password.
The change overrides the defined password enforcement policy.

If multiple conditions are passed to match on the patron, they all need
to match. Otherwise an error message is printed.

Attributes to search the patron on:
- cardnumber
- patron_id (a.k.a. borrowernumber)
- userid

we usually know some of them, but if we specify more than one, they need
to match a patron, together.

To test:
1) Apply this patch
2) Have a known patron (i.e. you know the cardnumber, the borrowernumber
   and the userid).
3) Run:
  $ kshell
 k$ perl misc/admin/set_password.pl --cardnumber <the_card_number> \
                                    --password a_password
4) Verify you can login with the new password
5) Repeat 3) through 5) with --patron_id and --userid
=> SUCCESS: You can login in all cases
6) Try combining some or all the parameters
=> SUCCESS: It fails when it should, it succeeds when it should
7) Sign off :-D

Signed-off-by: Pierre-Marc Thibault <pierre-marc.thibault@inLibro.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 24 Josef Moravec 2019-03-23 08:56:00 UTC
Created attachment 86939 [details] [review]
Bug 17746: (QA follow-up) Make set_password.pl generate a password if required

This patch makes the set_password.pl script generate the password if it
is not passed as an argument.

It also changes the behaviour of set_password.pl: it will now print the
userid and password (generated or not) to mimick the behaviour from
koha-reset-passwd.

The koha-reset-passwd gets simplified as it doesn't generate the random
password anymore, it passes the responsability to set_password.pl

To test:
- Follow the steps from the original platch
- Verify all behaves as expected

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Nick Clemens 2019-03-28 13:28:32 UTC
Awesome work all!

Pushed to master for 19.05
Comment 26 Martin Renvoize 2019-04-01 10:51:36 UTC
Pushed to 18.11.x for 18.11.05
Comment 27 Lucas Gass 2019-04-01 16:24:19 UTC
backported to 18.05.x for 18.05.12
Comment 28 Jonathan Druart 2019-06-23 15:51:18 UTC
Isn't this script missing the use Koha::Script from bug 22600?
Comment 29 Tomás Cohen Arazi 2019-06-24 13:29:08 UTC
(In reply to Jonathan Druart from comment #28)
> Isn't this script missing the use Koha::Script from bug 22600?

True! I filed bug 23193.