Ping T211621 and CC author of the patch and reviewers

We had a similar email sent to Trust-and-Safety the other day related to this as well.

In T233119#5499894, @jrbs wrote:

We had a similar email sent to Trust-and-Safety the other day related to this as well.

Same person as the security one? Any more specific than "the other day"? Just wondering if it was related to bumping to 10 characters, or the enforcing of the blacklist

In T233119#5499903, @Reedy wrote:

In T233119#5499894, @jrbs wrote:

We had a similar email sent to Trust-and-Safety the other day related to this as well.

Same person as the security one? Any more specific than "the other day"? Just wondering if it was related to bumping to 10 characters, or the enforcing of the blacklist

Different person - just someone forgetting their new password and not having an email assigned to the account. I don't think there's a technical solution to that, just wanted to mention it here.

Looking into this

Been looking into this with @dmaza - seems to be a difference in the type of status returned by the checks for the different policies:

• PasswordPolicyChecks::checkMinimalPasswordLength calls Status::error
• PasswordPolicyChecks::checkMinimumPasswordLengthToLogin calls Status::fatal (sets Status::ok to false).

In the latter case, TemporaryPasswordPrimaryAuthenticationProvider::beginPrimaryAuthentication then returns an AuthenticationResponse failure.

Ideally we'd want to show the password reset form, but without the option to skip.

Change 537512 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/core@master] Change PasswordPolicy failed status from fatal to error

https://gerrit.wikimedia.org/r/537512

As the name implies, MinimumPasswordLengthToLogin means that you are not allowed to log in with a password shorter than that. The assumption is that the password is so short that in all likelihood it has already been cracked, and the user must find another login method that we still trust (e.g. triggering a password reset). Of course is feasible for staff, and maybe advanced permission holders like stewards, where we know such method exists, and not really feasible for admins in general. So MinimumPasswordLength should be used instead (note the different prefix).

• PasswordPolicyChecks::checkMinimumPasswordLengthToLogin calls Status::fatal (sets Status::ok to false).

Was the idea behind doing this was to prevent people from logging in if their password is too short?

Possible ways to do that after https://gerrit.wikimedia.org/r/537512:

1. Always remember to add 'forceChange' => true for MinimumPasswordLengthToLogin, and not for MinimalPasswordLength - messy, and would need to be backed up by documentation
2. Discard MinimumPasswordLengthToLogin and instead use MinimalPasswordLength with and without forceChange - seems the most sensible to me, though you lose the ability to have the two policies set to different password lengths (but is that really needed?)
3. Automatically add forceChange to MinimumPasswordLengthToLogin somewhere in the code - this doesn't seem to be a good idea from a usability perspective; setting the policies should lead to predictable behaviour

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Change 537518 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537518

Change 537512 abandoned by Dmaza:
Change PasswordPolicy failed status from fatal to error

https://gerrit.wikimedia.org/r/537512

In T233119#5500584, @Tchanders wrote:

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Yeah, we do want to keep that for staff and other highly sensitive accounts as a paranoia measure. Forcing password change on login does not protect the account if the owner never logs in.

Change 537518 merged by jenkins-bot:
[mediawiki/core@master] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537518

In T233119#5500759, @Tgr wrote:

In T233119#5500584, @Tchanders wrote:

@Tgr OK, it sounds like we should be doing #2 then, but leaving MinimumPasswordLengthToLogin alone instead of discarding?

Yeah, we do want to keep that for staff and other highly sensitive accounts as a paranoia measure. Forcing password change on login does not protect the account if the owner never logs in.

I wonder if we should be giving the user some sort of hint in this case... ie use Special:PasswordReset...

Otherwise it's basically a dead end from usability...

Change 537545 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_33] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537545

In T233119#5500929, @Reedy wrote:

I wonder if we should be giving the user some sort of hint in this case... ie use Special:PasswordReset...

Otherwise it's basically a dead end from usability...

Turns out I did that! T214215: MinimumPasswordLengthToLogin error message is unhelpful / https://gerrit.wikimedia.org/r/c/mediawiki/core/+/485480
It was a long time ago so I'm not quite sure, but I think the patch was functional and just got lost in the code review limbo.

Change 537545 merged by jenkins-bot:
[mediawiki/core@REL1_33] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537545

Change 537550 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_32] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537550

Change 537551 had a related patch set uploaded (by Reedy; owner: Tchanders):
[mediawiki/core@REL1_31] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537551

In T233119#5501256, @Reedy wrote:

I think this task is done, and we don't actually need to re-instate https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ as the behaviour is now as expected?

Unless there was an intention to force the password change?

Thanks for backporting.

Change 537551 merged by jenkins-bot:
[mediawiki/core@REL1_31] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537551

Change 537550 merged by jenkins-bot:
[mediawiki/core@REL1_32] Improve documentation for the MinimumPasswordLengthToLogin policy

https://gerrit.wikimedia.org/r/537550

In T233119#5501262, @Tchanders wrote:

In T233119#5501256, @Reedy wrote:

I think this task is done, and we don't actually need to re-instate https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/534707/ as the behaviour is now as expected?

Unless there was an intention to force the password change?

I should probably have a look at the policy and what we were planning

Maybe we do it more staged, forcing reset first, and then eventually bumping it to completely prevent login of those old passwords. However, if we are going to do that, I definitely think we should get T214215 fixed (or more specifically, get tgr's patch merged. help reviewing welcome :)) before we do that

Change 537555 had a related patch set uploaded (by Jforrester; owner: Reedy):
[operations/mediawiki-config@master] Add comment about MinimumPasswordLengthToLogin

https://gerrit.wikimedia.org/r/537555

Change 537555 merged by jenkins-bot:
[operations/mediawiki-config@master] Add comment about MinimumPasswordLengthToLogin

https://gerrit.wikimedia.org/r/537555

@Tchanders: Maybe this summary will help:

1. 'MinimalPasswordLength' => [ 'value' => 10 ]
   • Prevents setting a password shorter than 10.
   • Existing passwords shorter than 10 can be used for login, and give no notification during the login process.
2. 'MinimalPasswordLength' => [ 'value' => 10, 'suggestChangeOnLogin' => true ]
   • Prevents setting a password shorter than 10.
   • Existing passwords shorter than 10 can be used for login. The user will be prompted to change their password during login, but may choose to keep their existing password.
3. 'MinimalPasswordLength' => [ 'value' => 10, 'forceChange' => true ]
   • Prevents setting a password shorter than 10.
   • Existing passwords shorter than 10 can be used for login, but the user will be required to change their password before they can complete the login process.
4. 'MinimalPasswordLengthToLogin' => [ 'value' => 10 ]
   • Prevents setting a password shorter than 10.
   • Existing passwords shorter than 10 cannot be used. The user will have to log in via some other method in order to change their password, or have a sysadmin reset their password or something like that.

It sounds like 92dbafe was intended to do #3, but actually did #4.

@Anomie That's correct - thank you for summarising so clearly for future readers of this task.

If you think the documentation of this behaviour needs updating beyond what I've already added, please feel free to add to it.