Dylan Hardison [:dylan] (he/him) Assignee
	Description • 7 years ago

SSIA

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 2 • 7 years ago

All tests should be passing now, after discovering a bug in circle ci.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 3 • 7 years ago

In for a second round of review, with hopefully all issues resolved.

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 4 • 7 years ago

There will be one change that requires discussion from a security perspective.

Currently, if BMO considers a password weak, you cannot login at all. You must initiate a password reset via the "forgot password" mechanism. Typically this process is painful enough that employees ask service desk to intervene.

In the PR currently in for review, I have added a commit that instead sets the "password change required" flag the account.
This also prevents login -- but it has the following differences:

1. The account is effectively disabled -- API calls won't work until the user changes the password. (security++)
2. Also, this event is logged in with [audit] so we know $so_and_so had to change their password for $reason. (security++)
3. The "reason" as reported by passwdqc() is provided as the reason for the password change. (ux++)
4. The user can change their password on Bugzilla, in the same way that currently happens when an admin forces an account to change its password -- this mechanism was implemented in bug 1196134. This might be considered a security--, as the current
system forces the person to prove they own their email address if their password is insecure

What's your take on this? I can provide the audit logging in #2 without any of this,

	Guillaume Destuynder [:kang] [this account is no longer active]
	Comment 5 • 7 years ago

@comment 4
The plan seem ok to me overall - if i try to think of things that may go wrong from the top of my head:

Scenario: someone who already has account access (ie "has the password" - discarding the "has the session" since the session is just lost in this case) gets the message to change password:
- access is lost unless the person also controls the email (++)

Scenario: someone with access to the email but not the account may get a notification:
- this no real difference I can think of really. Likelihood that the person now uses the account is a little higher, but that's still a corner case

Scenario: someone who already has account access also created and saved some API keys for that user:
- do all API keys get disabled until regenerated? (not saying it's the best UX there, but wondering)


As far as logging goes, I/we always like logging specially for identity-related operations regardless of changes, on password change, reset, how it's triggered, why it's triggered, and of course where/when/whom. i.e. if that was easier for you to add audit logging separately first, that would be fine as well.

Finally, do we have an estimate of how many accounts would need to change their passwords when the patch is applied?

	Dylan Hardison [:dylan] (he/him) Assignee
	Comment 6 • 7 years ago

(In reply to Guillaume Destuynder [:kang] (NEEDINFO to ensure replies) from comment #5)

> Finally, do we have an estimate of how many accounts would need to change
> their passwords when the patch is applied?

No idea -- we only store salted hashes. Anyone following the existing rules (in their spirit, at least) should probably be fine.
people that are using "passWord1234!" will be out of luck.