Page MenuHomePhabricator

Separate "Set/Reset Password" from "Change Password"
ClosedPublic

Authored by epriestley on Dec 22 2017, 8:11 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 19, 7:03 PM
Unknown Object (File)
Thu, Apr 11, 8:28 AM
Unknown Object (File)
Mon, Apr 1, 8:45 AM
Unknown Object (File)
Mar 20 2024, 10:49 PM
Unknown Object (File)
Feb 12 2024, 8:03 AM
Unknown Object (File)
Dec 30 2023, 10:37 PM
Unknown Object (File)
Dec 30 2023, 9:25 PM
Unknown Object (File)
Dec 24 2023, 6:28 PM
Subscribers
None

Details

Summary

See PHI223. Ref T13024. There's a remaining registration/login order issue after the other changes in T13024: we lose track of the current URI when we go through the MFA flow, so we can lose "Set Password" at the end of the flow.

Specifically, the flow goes like this today:

  • User clicks the welcome link in email.
  • They get redirected to the "set password" settings panel.
  • This gets pre-empted by Legalpad (although we'll potentially survive this with the URI intact).
  • This also gets pre-empted by the "Set MFA" workflow. If the user completes this flow, they get redirected to a /auth/multifactor/?id=123 sort of URI to highlight the factor they added. This causes us to lose the /settings/panel/password/blah/blah?key=xyz URI.

The ordering on this is also not ideal; it's preferable to start with a password, then do the other steps, so the user can return to the flow more easily if they are interrupted.

Resolve this by separating the "change your password" and "set/reset your password" flows onto two different pages. This copy/pastes a bit of code, but both flows end up simpler so it feels reasonable to me overall.

We don't require a full session for "set/reset password" (so you can do it if you don't have MFA/legalpad yet) and do it first.

This works better and is broadly simpler for users.

Test Plan
  • Required MFA + legalpad, invited a user via email, registered.
    • Before: password set flow got lost when setting MFA.
    • After: prompted to set password, then sign documents, then set up MFA.
  • Reset password (with MFA confgiured, was required to MFA first).
  • Tried to reset password without a valid reset key, wasn't successful.
  • Changed password using existing flow.
  • Hit various (all?) error cases (short password, common password, mismatch, missing password, etc).

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This is still slightly rough for email invitation to an install that requires MFA and does not support password authentication: you don't end up at the "link an external account" at the end. However, this page isn't very clear anyway (but no one has complained) and a setup wiht "email invites" + "google auth" is probably exceptionally rare. At least, I'm not aware of any installs affected by the remaining unevenness of that flow.

amckinley added inline comments.
src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
44–78

I'm guessing this is the big chunk of copy/pasted code? Not the end of the world, but I can definitely see this code being forgotten when updating the other copy.

src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
38–42

This comment will help at least.

This revision is now accepted and ready to land.Dec 24 2017, 7:16 PM
This revision was automatically updated to reflect the committed changes.

Yeah, it's a bit copy/pastey. In theory we could reduce that a bit by moving Users to EditEngine/Transactions, but that's a substantial change. Some of the code just moved rather than getting copied, so it's not quite as bad as the diff size implies, and most of it is ultimately just display logic.