Bug 18298 added the option to enforce password complexity *if* the new system preference RequireStrongPassword is enabled. However, it appears that the code defining the minimum number of characters was not put under control of this preference and thus Koha enforces a minimum password length of 3 even if RequireStrongPassword is disabled.
Hi Kyle, another pref drives this behavior, see minPasswordLength
Created attachment 78414 [details] [review] Bug 21314: Koha enforces three (3) character password length even if RequireStrongPassword is disabled Bug 18298 added the option to enforce password complexity *if* the new system preference RequireStrongPassword is enabled. However, it appears that the code defining the minimum number of characters was not put under control of this preference and thus Koha enforces a minimum password length of 3 even if RequireStrongPassword is disabled. Test Plan: 1) Set minPasswordLength to 2 2) Disable RequireStrongPassword 3) Attempt to change a password to a two character password 4) Note Koha tells you the minimum length is 3 characters 5) Apply this patch 6) Restart memached/plack, Reload the page 7) You should now be able to set the password to a 2 character string
(In reply to Jonathan Druart from comment #1) > Hi Kyle, another pref drives this behavior, see minPasswordLength That is not correct, that bug hard coded the 3 limit in such that if minPassword is less than 3 it is changed to 3. Please see the patch file for where this happens.
Should we really allow such short passwords?
Created attachment 78427 [details] Error message when changing password Applied the patch, restarted plack, apache and memcached and attempted to set a patron password of 2 characters after changing the sysprefs as per the test plan. I'm getting a very confusing error message saying that I'm not entering two characters even though I am. I tried setting the minPasswordLength syspref to one but it then complains that I'm not entering at least 1 character. This is also true if I type in a password with two characters. It looks like it's still searching for three characters when changing patron passwords.
(In reply to Katrin Fischer from comment #4) > Should we really allow such short passwords? I don't see why we as a community should decide arbitrarily how libraries should set their policies. This bug was revealed by a specific partner who has always set the patron password to his or her last name by default. Bug 18298 broke this workflow. As a community we've always been adverse to changing default behavior without necessity, and I see no reason why this situation should not be included.
Created attachment 78432 [details] [review] Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid
Created attachment 78433 [details] [review] Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid
(In reply to Dilan Johnpullé from comment #5) > Created attachment 78427 [details] > Error message when changing password > > Applied the patch, restarted plack, apache and memcached and attempted to > set a patron password of 2 characters after changing the sysprefs as per the > test plan. > > I'm getting a very confusing error message saying that I'm not entering two > characters even though I am. I tried setting the minPasswordLength syspref > to one but it then complains that I'm not entering at least 1 character. > This is also true if I type in a password with two characters. > > It looks like it's still searching for three characters when changing patron > passwords. This followup patch should take care of the error you are getting Dilan!
+ if ( C4::Context->preference('RequireStrongPassword') ) { + $minPasswordLength = 3 if $minPasswordLength < 3; + } How strong is that? LOL Imo we should enforce at least 8 chars or so, and educate users that security has a price.
(In reply to Marcel de Rooy from comment #10) > + if ( C4::Context->preference('RequireStrongPassword') ) { > + $minPasswordLength = 3 if $minPasswordLength < 3; > + } > > How strong is that? LOL > Imo we should enforce at least 8 chars or so, and educate users that > security has a price. Agreed, but that's another bug report ;) IMO we should have a syspref where we can add a custom or canned regex to define password difficulty. But again, that's quite out of scope here.
(In reply to Kyle M Hall from comment #6) > (In reply to Katrin Fischer from comment #4) > > Should we really allow such short passwords? > > I don't see why we as a community should decide arbitrarily how libraries > should set their policies. This bug was revealed by a specific partner who > has always set the patron password to his or her last name by default. Bug > 18298 broke this workflow. As a community we've always been adverse to > changing default behavior without necessity, and I see no reason why this > situation should not be included. I see your point about changing behaviour, but secretly hoping that this is not the usual library next door :)
(In reply to Marcel de Rooy from comment #10) > + if ( C4::Context->preference('RequireStrongPassword') ) { > + $minPasswordLength = 3 if $minPasswordLength < 3; > + } > > How strong is that? LOL > Imo we should enforce at least 8 chars or so, and educate users that > security has a price. As the library in question, I have to say that I agree with you personally, but professionally I believe this would be a barrier to service. We serve a broad range from the very young to very old to customers with severe learning disabilities. Education is simply not always an option. Users have to use their password to access public computers (as well as a variety of other services) and the true outcome here is that we would spend our time resetting passwords all day long. Last names are easy to remember and secure enough to prevent abuse when cards are lost (which a default password would not be).
(In reply to Marcel de Rooy from comment #10) > + if ( C4::Context->preference('RequireStrongPassword') ) { > + $minPasswordLength = 3 if $minPasswordLength < 3; > + } > > How strong is that? LOL > Imo we should enforce at least 8 chars or so, and educate users that > security has a price. See bug 18308.
git grep -l minPasswordLength -- there are other places: Koha/Patron.pm, C4/InstallAuth.pm --- BEGIN FURTHER THOUGHTS --- Perhaps a C4::Context function to return this value, so you don't end up putting the same code in 4 spots? Also, perhaps cutting the minPasswordLength from the template variables set, and update Koha/Template/Plugin/Koha.pm with a call to the new C4::Context function? because I also noticed the password_check.inc files. just reading members/member-password.tt -- Hmmm... logic there might be broken, because you could set minPasswordLength=2, RequireStrongPassword = true, and then you need 3, but the message will say 2. Another good place for such a tweak to the plugin. members/memberentrygen.tt bypasses this issue, by using the template value. similarly, opac-passwd.tt. similarly, opac-password-recovery.tt opac-memberentry.tt uses the template value, but also has a direct preference call too. DOH! DOH! AuthUtils.t doesn't test for the 2 but strong case. That's beyond scope. Koha/Patrons.t doesn't maybe? Beyond scope, but I'm still looking at all minPasswordLength files not po files. :) Hmmm... api patron test file mocks the system preference. *shrug* --- END FURTHER THOUGHTS --- Okay... to summarize: Failed QA because git grep -l minPasswordLength -- there are other places: Koha/Patron.pm, C4/InstallAuth.pm
(In reply to Marcel de Rooy from comment #10) > + if ( C4::Context->preference('RequireStrongPassword') ) { > + $minPasswordLength = 3 if $minPasswordLength < 3; > + } > > How strong is that? LOL > Imo we should enforce at least 8 chars or so, and educate users that > security has a price. Our library practices similar methods, and we've run into this issue as well. I've never liked software that tells users how to enforce their security. The options should be there, and how secure or insecure they choose to run it should be up to them. However, there are bugs, like https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33462 that focus on improving things in another way, which would give us more flexibility and options. It just feels like libraries are being forced to bend to the will of programmers based on personal opinions. There are a lot of minimum values that could be forced on libraries and their patrons, but that should be the choice of the library, not the program. Imo, our default passwords SHOULD be stronger. But I don't get to make that decision, so why should a developer?
I completely agree with Christopher - we're in the same boat! We've received many preferences regarding the security concern since the initial creation of the thread (like expiring passwords, password expiration by patron category). It would be nice to allow us the flexibility to adjust security to our individual system needs. Also it is misleading to allow us to set minPasswordLength preference to 2 and not have clarification it will not function. It will only clarify the hard coded limit when you attempt to create an account with a 2 character PIN. Thank you!