When a patron registers for a new account, their loginattempt value is NULL and not zero. This is a change since 18.11. Since this value is NULL, a patron cannot reset their password due to this NULL value.
Created attachment 97058 [details] [review] Bug 24379: Make login_attempts not nullable While the column defaults to 0 in Koha::Object->store we set to NULL if NULLABLE When trying to reset a patrons password we check that the account is not administratively locked: login_attempts != -1 This query does not return rows where login_attempts IS NULL. It will return accounts where login_attempts = 0 Let's default to 0 like we intend To test: 1 - Create a new patron 2 - Note their login_attempts is NULL SELECT login_attempts FROM borrowers ORDER BY borrowernumber DESC LIMIT 1 3 - Enable OpacResetPassword 4 - Attempt to reset password before logging in, you cannot 5 - Apply patch, updatedatabase, restart_all, update schema 6 - Create another patron 7 - Their login attempts should be 0 8 - Attempt to reset password, it works!
Tests don't pass, something is missing
Created attachment 97073 [details] [review] Bug 24379: Fix the test First we create a patron using TestBuilder to get a hashref of valid info. Then we delete it and create a new patron using Koha::Patron->new Once stored, we should call discard_changes to make the calculated values available in the currenct object.
Is this ready for testing?
I cannot recreate this in master. When I create a new patron (either from the staff client or selfreg via OPAC) login_attempts are initially set to 0 and I am able to reset my password via OpacResetPassword.
I think it might have been fixed already: kohastructure.sql: `login_attempts` int(4) default 0, -- number of failed login attemps updatedatabase: 14478 $DBversion = '16.12.00.037'; - with default 0 14666 $DBversion = '17.06.00.009'; - is a little weird: 14668 $dbh->do(q{ 14669 ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) AFTER lang; 14670 }); 14671 $dbh->do(q{ 14672 ALTER TABLE deletedborrowers MODIFY COLUMN login_attempts int(4) AFTER lang; 14673 }); Is it possible the 17.06 change broke the default 0 for updated installations? That could explain, why you don't see the issue on master.
Forget the first line... I think it's possibly not fixed, started the comment before looking at updatedatabase.
This is currently still the case on 19.11. When creating a new patron, the login attempts are set to NULL making the password recovery not work on the OPAC.
I have tried on both master and 19.11.x and login_attempts contains "0"
Created attachment 105522 [details] [review] Bug 24379: Don't drop default of 0 for login attempts When moving the column we drop the default, this means that DBs upgraded form earlier versions get the wrong values set To test: 1 - Checkout 16.11.x 2 - Reset all 3 - Checkout master 4 - updatedatabase 5 - SHOW CREATE TABLE borrowers; 6 - Note the column login_attempts defaults to NULL 7 - Apply patch(es) 8 - Repeat 9 - Now it defaults ot 0 (and has NOT NULL if applied all)
(In reply to Jonathan Druart from comment #9) > I have tried on both master and 19.11.x and login_attempts contains "0" I have tried on 19.11.05 and login_attempts is null.
No change to login_attempts between 19.11.05 and 19.11.07... No change to login_attempts between 19.11.05 and origin/19.11.x... Ah, but this is an upgraded database. I see in kohastructure.sql that login_attempts has "default 0" but in my actual upgraded database it has "default null". In 16.12.00.037, there is the following (for Bug 18314 - Add FailedLoginAttempts and borrowers.login_attempts): ALTER TABLE borrowers ADD COLUMN login_attempts INT(4) DEFAULT 0 AFTER lastseen In 17.06.00.009, the DEFAULT 0 was removed (for Bug 19344 - Reorder lang and login_attempts in the [deleted]borrowers tables): ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) AFTER lang; So that explains that. New databases will be fine, but upgraded databases will not be.
At a glance, these patches look good. They're a bit of a pain to test though.
Sorry, Nick, but can't apply the patch: error: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql). error: could not build fake ancestor Patch failed at 0001 Bug 24379: Make login_attempts not nullable The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-24379-Make-loginattempts-not-nullable-I8ewqG.patch
Created attachment 106846 [details] [review] Bug 24379: Make login_attempts not nullable While the column defaults to 0 in Koha::Object->store we set to NULL if NULLABLE When trying to reset a patrons password we check that the account is not administratively locked: login_attempts != -1 This query does not return rows where login_attempts IS NULL. It will return accounts where login_attempts = 0 Let's default to 0 like we intend To test: 1 - Create a new patron 2 - Note their login_attempts is NULL SELECT login_attempts FROM borrowers ORDER BY borrowernumber DESC LIMIT 1 3 - Enable OpacResetPassword 4 - Attempt to reset password before logging in, you cannot 5 - Apply patch, updatedatabase, restart_all, update schema 6 - Create another patron 7 - Their login attempts should be 0 8 - Attempt to reset password, it works! Bug 24379: Fix the test First we create a patron using TestBuilder to get a hashref of valid info. Then we delete it and create a new patron using Koha::Patron->new Once stored, we should call discard_changes to make the calculated values available in the currenct object. Bug 24379: Don't drop default of 0 for login attempts When moving the column we drop the default, this means that DBs upgraded form earlier versions get the wrong values set To test: 1 - Checkout 16.11.x 2 - Reset all 3 - Checkout master 4 - updatedatabase 5 - SHOW CREATE TABLE borrowers; 6 - Note the column login_attempts defaults to NULL 7 - Apply patch(es) 8 - Repeat 9 - Now it defaults ot 0 (and has NOT NULL if applied all)
Created attachment 106847 [details] [review] Bug 24379: Make login_attempts not nullable While the column defaults to 0 in Koha::Object->store we set to NULL if NULLABLE When trying to reset a patrons password we check that the account is not administratively locked: login_attempts != -1 This query does not return rows where login_attempts IS NULL. It will return accounts where login_attempts = 0 Let's default to 0 like we intend To test: 1 - Create a new patron 2 - Note their login_attempts is NULL SELECT login_attempts FROM borrowers ORDER BY borrowernumber DESC LIMIT 1 3 - Enable OpacResetPassword 4 - Attempt to reset password before logging in, you cannot 5 - Apply patch, updatedatabase, restart_all, update schema 6 - Create another patron 7 - Their login attempts should be 0 8 - Attempt to reset password, it works! Bug 24379: Fix the test First we create a patron using TestBuilder to get a hashref of valid info. Then we delete it and create a new patron using Koha::Patron->new Once stored, we should call discard_changes to make the calculated values available in the currenct object. Bug 24379: Don't drop default of 0 for login attempts When moving the column we drop the default, this means that DBs upgraded form earlier versions get the wrong values set To test: 1 - Checkout 16.11.x 2 - Reset all 3 - Checkout master 4 - updatedatabase 5 - SHOW CREATE TABLE borrowers; 6 - Note the column login_attempts defaults to NULL 7 - Apply patch(es) 8 - Repeat 9 - Now it defaults ot 0 (and has NOT NULL if applied all) Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
I wondered if we should have a statement for changing the NULL to 0 on existing patrons, but it seems to automatically happen in my testing.
Created attachment 106890 [details] [review] Bug 24379: Make login_attempts not nullable While the column defaults to 0 in Koha::Object->store we set to NULL if NULLABLE When trying to reset a patrons password we check that the account is not administratively locked: login_attempts != -1 This query does not return rows where login_attempts IS NULL. It will return accounts where login_attempts = 0 Let's default to 0 like we intend To test: 1 - Create a new patron 2 - Note their login_attempts is NULL SELECT login_attempts FROM borrowers ORDER BY borrowernumber DESC LIMIT 1 3 - Enable OpacResetPassword 4 - Attempt to reset password before logging in, you cannot 5 - Apply patch, updatedatabase, restart_all, update schema 6 - Create another patron 7 - Their login attempts should be 0 8 - Attempt to reset password, it works! Bug 24379: Fix the test First we create a patron using TestBuilder to get a hashref of valid info. Then we delete it and create a new patron using Koha::Patron->new Once stored, we should call discard_changes to make the calculated values available in the currenct object. Bug 24379: Don't drop default of 0 for login attempts When moving the column we drop the default, this means that DBs upgraded form earlier versions get the wrong values set To test: 1 - Checkout 16.11.x 2 - Reset all 3 - Checkout master 4 - updatedatabase 5 - SHOW CREATE TABLE borrowers; 6 - Note the column login_attempts defaults to NULL 7 - Apply patch(es) 8 - Repeat 9 - Now it defaults ot 0 (and has NOT NULL if applied all) Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 107099 [details] [review] Bug 24379: Fix failing tests kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Patrons/Import.t t/db_dependent/Koha/Patrons/Import.t .. 2/159 # Failed test 'No warning raised by import_patrons' # at t/db_dependent/Koha/Patrons/Import.t line 171. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2. # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2. # didn't expect to find a warning Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_4> line 2. Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_4> line 2. t/db_dependent/Koha/Patrons/Import.t .. 152/159 # Looks like you failed 1 test of 159. t/db_dependent/Koha/Patrons/Import.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/159 subtests Test Summary Report ------------------- t/db_dependent/Koha/Patrons/Import.t (Wstat: 256 Tests: 159 Failed: 1) Failed test: 39 Non-zero exit status: 1 Files=1, Tests=159, 3 wallclock secs ( 0.04 usr 0.01 sys + 2.11 cusr 0.24 csys = 2.40 CPU) Result: FAIL
Follow-up pushed to master for 20.11
not backporting to 19.11.x because of the db updates.
(In reply to Aleisha Amohia from comment #22) > not backporting to 19.11.x because of the db updates. Hi Aleisha, The patches here fix a problem caused by an upgrade in 17.06.00.009 We need the DB updates to restore the intended settings and prevent password reset problems for upgraded databases. Would you reconsider this one? -Nick
backported to 19.11.x for 19.11.09
(In reply to Nick Clemens from comment #23) > The patches here fix a problem caused by an upgrade in 17.06.00.009 > > We need the DB updates to restore the intended settings and prevent password > reset problems for upgraded databases. Didn't go through the details but it feels safe to raise the severity from normal to (at least) major.
Backported to 19.05.x branch for 19.05.15