(Just changing to grammatically reflect what this task appears to be for :)

From a Dayllan email:

To be more specific, the problem we currently face is that both authentication workflows (create account and login) end up calling the same method on their common parent class. By the time we get to the method that runs the checks there is no context as to what action we are trying to do from the `AuthManager`. [ here is the stack trace: https://phabricator.wikimedia.org/P7902 ]

From the top of my head I can think of two solutions (keep in mind that I'm not familiar at all with MW Authentication and I'm sure I'm oversimplifying this and/or missing things here)

1.- Override `AbstractPasswordPrimaryAuthenticationProvider::checkPasswordValidity`
on both `LocalPasswordPrimaryAuthenticationProvider` and `TemporaryPasswordPrimaryAuthenticationProvider` and depending on the `AuthenticationRequest::$action` we could call `User::checkPasswordValidity` on account creation and a new method that checks only the policies we want on login.

2.- Same override as above but use `UserPasswordPolicy` on the AuthenticatioProviders and move out of the `User` class all the logic about password validation. I think the `User` class should not be concerned about this in the first place.

For both options it might be a good idea to define which policies to run depending on the action in a config so DefaultSettings will need to be changed too.

Probably should be handled the same way as rMWf15ecc60cd94: Add force option to password policy: add some password policy option like skipLogin, handle merging of different policy options / statuses in UserPasswordPolicy and put the result in a status flag, handle the flag in AbstractPasswordPrimaryAuthenticationProvider::setPasswordResetFlag.

In T211621#4878804, @Tgr wrote:

Probably should be handled the same way as rMWf15ecc60cd94: Add force option to password policy: add some password policy option like skipLogin, handle merging of different policy options / statuses in UserPasswordPolicy and put the result in a status flag, handle the flag in AbstractPasswordPrimaryAuthenticationProvider::setPasswordResetFlag.

I'm struggling to see how to properly do this. Following what was done in rMWf15ecc60cd94: Add force option to password policy is not exactly doable for this particular change since forceChange corresponds to the authentication status as a whole and skipLogin would be related to each policy check.

It makes sense to force a password change if one or more policies that fail have the forceChange but not so much for skipLogin. Another thing to consider is that for skipLogin in particular it makes no sense to run a check on a policy when we know we are gonna ignore the result.

Considering how coupled the UserPasswordPolicy is to the User through User::isValidPassword and User::checkPasswordValidity the only thing I can think of right now is to build an instance of UserPasswordPolicy with the policies we want to check based on the AuthenticationRequest action at the AuthenticationProvider level and inject that into the user via a setter, making User::checkPasswordValidity check for the UPP and use it or create one with all the policies and checks.

An alternative would be to move out of the User class the policy checks and handle that as part of the Authentication process since this checks are only needed in an "Auth" context and the User class shouldn't really be concerned about any of this imo. This is the cleaner way I think but it would take more effort and a few deprecations.

The only difference is how policy check results are aggregated: forceChange needs to be set if it is set on any of the failed checks, while skipLogin should only be set if it is set on all the failed checks. Also aggregating policy settings for the same check for different user groups will have to be done differently - if there are multiple user groups that apply configs for the same check to the user, the result should only have the skip flag if all policies have it. (That's a bit annoying because currently UserPasswordPolicy::maxOfPolicies does not have to know the meaning of the settings, but oh well.) Other than that the current structure should be fine.

(That means if there is a failed skipLogin check and a failed normal check, both error messages will show, even though the skipLogin one would not show on its own. That is fine; failing the normal check means that the user will be presented with a password change dialog, and a skipLogin policy will still prevent new passwords which fail that policy, so it's good to warn the user about it.)

In the long term, it would certainly be good to move password checks out of User (in the long term, pretty much everything should be moved out of User, it's an unwieldy god class that does not fit into our current architectural vision of services and dumb value objects) but for this task it is not needed.

That means if there is a failed skipLogin check and a failed normal check, both error messages will show, even though the skipLogin one would not show on its own

This is why I said that the same approach as forceChange can't be used. It was my understanding that the warning shouldn't be shown when other policies fail. If this is an accepted behavior then I agree with you that it should work just fine like you described in your last comment.

@TBolliger are we good with this behavior?

If the user has to be prompted with a password change dialog anyway, showing all problems with their current password is preferable, IMO.
In practice the important point is that the password policy change should not cause password change dialogs to show up for people who have been able to log in without problems so far, and that is true either way. The only way someone can see both errors is if their password was already invalid before the current policy change; which means it has been invalid for years, since we haven't changed password policies in a while, and you can't set an invalid password. Which means they have been shown change dialogs for years, so if any such users exist, their their visual cortex must have evolved by now to completely filter out all mentions of password errors :)

One aspect that's maybe worth considering: do we want to keep a non-skip version? This is not relevant for Wikimedia (currently, anyway) but imagine I want to push people on my site towards longer passwords without causing much disruption, so I disallow passwords shorter than 10 characters with a skip option; but I also want to get rid of really weak passwords, and don't mind some disruption there. So what I would want is

'MinimalPasswordLength' => [ 'value' => 10, 'skipLogin' => true ],
'MinimalPasswordLength' => [ 'value' => 6, 'skipLogin' => false ],

except of course that is not possible. So if we want to support that, then we might want to go for something like [ 'value' => 6, 'skipLoginValue' => 10 ] instead (and that would work neatly out of the box with maxOfPolicies as well). OTOH this is a fringe use case and a site admin can always define two different checks using the same callback... so I guess I wouldn't worry about it.

In T211621#4949111, @Tgr wrote:

If the user has to be prompted with a password change dialog anyway, showing all problems with their current password is preferable, IMO.

I agree.

The goal of this project/endeavor is to increase the security of Wikimedia by encouraging (and in most cases requiring) our users to have stronger passwords. If a user has multiple problems with their password (length, commonality, etc.) it's probably in their best interest to update the password.

Let's shelf the non-skip version for now — would probably be more appropriate for another task and for the Security team to decide.

Change 491686 had a related patch set uploaded (by Dmaza; owner: Dmaza):
[mediawiki/core@master] Add password policy setting skipLogin

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

Change 491686 merged by jenkins-bot:
[mediawiki/core@master] Add password policy setting suggestChangeOnLogin

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

On beta, non-privileged users don't see the "Your password is not valid" dialog. Privileged users do.

When clicking Skip or after changing your password you are properly authenticated, including on other wikis which share central authentication.

If user is in two groups, one with suggestChangeOnLogin=true and one with it set to false, then they see the dialog.

This is true when a user is in different groups on different wikis which share the same central authentication.

This is also true for users in local and global groups with different policies.

Also true if, while logged in as one user, you login as another. Password policy of the user you are attempting to login as is applied.

On beta, Special:ChangeCredentials requires 8 characters for non-privileged groups, 10 for privileged groups. Similarly for Special:PasswordReset.

Briefly tested mobile, which shows/does not show the "Your password is not valid" dialog. Also tested Special:PasswordReset on mobile (Special:ChangeCredentials not suppored on mobile). Appropriate password policies applied.

I notice that users in the "staff" group will have

['MinimumPasswordLengthToLogin'] = [
  'value' => 10,
  'suggestChangeOnLogin' => true,
];

and I believe this will force them to change their passwords.

@dom_walden — Thanks for testing so thoroughly! Based on your comment, it sounds like there are no regressions or oddities found and we're safe to deploy!

Staff passwords are also manually managed by OIT, who require us to have 2FA enabled.

In T211621#5026677, @dom_walden wrote:

I notice that users in the "staff" group will have

['MinimumPasswordLengthToLogin'] = [
  'value' => 10,
  'suggestChangeOnLogin' => true,
];

and I believe this will force them to change their passwords.

@dom_walden FWIW there is a different flag forceChange that will prevent login in unless you change your password. suggestChangeOnLogin will ask you for a password change but you could still "skip" it