Bug 21314 - Koha enforces three (3) character password length even if RequireStrongPassword is disabled
Summary: Koha enforces three (3) character password length even if RequireStrongPasswo...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on: 18298
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-04 15:31 UTC by Kyle M Hall
Modified: 2024-03-18 16:44 UTC (History)
11 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 21314: Koha enforces three (3) character password length even if RequireStrongPassword is disabled (1.86 KB, patch)
2018-09-04 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Error message when changing password (80.26 KB, image/png)
2018-09-05 00:19 UTC, Dilan Johnpullé
Details
Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid (1.07 KB, patch)
2018-09-05 10:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid (1.07 KB, patch)
2018-09-05 10:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-09-04 15:31:25 UTC
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.
Comment 1 Jonathan Druart 2018-09-04 17:04:49 UTC
Hi Kyle, another pref drives this behavior, see minPasswordLength
Comment 2 Kyle M Hall 2018-09-04 17:16:44 UTC
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
Comment 3 Kyle M Hall 2018-09-04 17:18:26 UTC
(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.
Comment 4 Katrin Fischer 2018-09-04 17:20:39 UTC
Should we really allow such short passwords?
Comment 5 Dilan Johnpullé 2018-09-05 00:19:27 UTC
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.
Comment 6 Kyle M Hall 2018-09-05 10:37:37 UTC
(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.
Comment 7 Kyle M Hall 2018-09-05 10:47:42 UTC
Created attachment 78432 [details] [review]
Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid
Comment 8 Kyle M Hall 2018-09-05 10:50:23 UTC
Created attachment 78433 [details] [review]
Bug 21314: Fix duplicate code in Koha::AuthUtils::is_password_valid
Comment 9 Kyle M Hall 2018-09-05 10:51:15 UTC
(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!
Comment 10 Marcel de Rooy 2018-09-05 14:25:18 UTC
+    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.
Comment 11 Kyle M Hall 2018-09-05 14:34:56 UTC
(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.
Comment 12 Katrin Fischer 2018-09-05 18:12:22 UTC
(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 :)
Comment 13 Benjamin Daeuber 2018-09-05 18:30:27 UTC
(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).
Comment 14 Jonathan Druart 2018-09-16 18:03:37 UTC
(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.
Comment 15 Mark Tompsett 2018-11-21 04:02:30 UTC
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
Comment 16 Christopher Brannon 2024-01-03 20:56:38 UTC
(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?
Comment 17 Christine Lee 2024-03-18 16:44:20 UTC
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!