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
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.
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.)
*** Bug 19321 has been marked as a duplicate of this bug. ***
(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.)
(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)
(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.
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? :)
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
Created attachment 84798 [details] [review] Bug 17746: Make koha-reset-passwd user set_password.pl Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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
*** Bug 21976 has been marked as a duplicate of this bug. ***
Created attachment 84898 [details] [review] Bug 21446: Remove fix comment Fix is not needed. Signed-off-by: helene hickey <hickeyhe@wgc.school.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
I can only sign off the first patch. Since it works, I did it. :)
What about generate a password if none is provided?
(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?
(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!
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>
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>
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>
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>
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>
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>
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>
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.05
backported to 18.05.x for 18.05.12
Isn't this script missing the use Koha::Script from bug 22600?
(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.