@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Separate "Set/Reset Password" from "Change Password"
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).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18840
+185
-59
+2
src/__phutil_library_map__.php
+2
src/__phutil_library_map__.php
···
2112
2112
'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php',
2113
2113
'PhabricatorAuthSessionInfo' => 'applications/auth/data/PhabricatorAuthSessionInfo.php',
2114
2114
'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
2115
+
'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php',
2115
2116
'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php',
2116
2117
'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
2117
2118
'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php',
···
7377
7378
'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector',
7378
7379
'PhabricatorAuthSessionInfo' => 'Phobject',
7379
7380
'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
7381
+
'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController',
7380
7382
'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck',
7381
7383
'PhabricatorAuthStartController' => 'PhabricatorAuthController',
7382
7384
'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
+1
src/applications/auth/application/PhabricatorAuthApplication.php
+1
src/applications/auth/application/PhabricatorAuthApplication.php
+1
-2
src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+1
-2
src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+155
src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
+155
src/applications/auth/controller/PhabricatorAuthSetPasswordController.php
···
1
+
<?php
2
+
3
+
final class PhabricatorAuthSetPasswordController
4
+
extends PhabricatorAuthController {
5
+
6
+
public function shouldAllowPartialSessions() {
7
+
return true;
8
+
}
9
+
10
+
public function shouldAllowLegallyNonCompliantUsers() {
11
+
return true;
12
+
}
13
+
14
+
public function handleRequest(AphrontRequest $request) {
15
+
$viewer = $this->getViewer();
16
+
17
+
if (!PhabricatorPasswordAuthProvider::getPasswordProvider()) {
18
+
return new Aphront404Response();
19
+
}
20
+
21
+
$token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
22
+
$viewer,
23
+
$request,
24
+
'/');
25
+
26
+
$key = $request->getStr('key');
27
+
$password_type = PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE;
28
+
if (!$key) {
29
+
return new Aphront404Response();
30
+
}
31
+
32
+
$auth_token = id(new PhabricatorAuthTemporaryTokenQuery())
33
+
->setViewer($viewer)
34
+
->withTokenResources(array($viewer->getPHID()))
35
+
->withTokenTypes(array($password_type))
36
+
->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
37
+
->withExpired(false)
38
+
->executeOne();
39
+
if (!$auth_token) {
40
+
return new Aphront404Response();
41
+
}
42
+
43
+
$min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
44
+
$min_len = (int)$min_len;
45
+
46
+
$e_password = true;
47
+
$e_confirm = true;
48
+
$errors = array();
49
+
if ($request->isFormPost()) {
50
+
$password = $request->getStr('password');
51
+
$confirm = $request->getStr('confirm');
52
+
53
+
$e_password = null;
54
+
$e_confirm = null;
55
+
56
+
if (!strlen($password)) {
57
+
$errors[] = pht('You must choose a password or skip this step.');
58
+
$e_password = pht('Required');
59
+
} else if (strlen($password) < $min_len) {
60
+
$errors[] = pht(
61
+
'The selected password is too short. Passwords must be a minimum '.
62
+
'of %s characters.',
63
+
new PhutilNumber($min_len));
64
+
$e_password = pht('Too Short');
65
+
} else if (!strlen($confirm)) {
66
+
$errors[] = pht('You must confirm the selecetd password.');
67
+
$e_confirm = pht('Required');
68
+
} else if ($password !== $confirm) {
69
+
$errors[] = pht('The password and confirmation do not match.');
70
+
$e_password = pht('Invalid');
71
+
$e_confirm = pht('Invalid');
72
+
} else if (PhabricatorCommonPasswords::isCommonPassword($password)) {
73
+
$e_password = pht('Very Weak');
74
+
$errors[] = pht(
75
+
'The selected password is very weak: it is one of the most common '.
76
+
'passwords in use. Choose a stronger password.');
77
+
}
78
+
79
+
if (!$errors) {
80
+
$envelope = new PhutilOpaqueEnvelope($password);
81
+
82
+
// This write is unguarded because the CSRF token has already
83
+
// been checked in the call to $request->isFormPost() and
84
+
// the CSRF token depends on the password hash, so when it
85
+
// is changed here the CSRF token check will fail.
86
+
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
87
+
88
+
id(new PhabricatorUserEditor())
89
+
->setActor($viewer)
90
+
->changePassword($viewer, $envelope);
91
+
92
+
unset($unguarded);
93
+
94
+
// Destroy the token.
95
+
$auth_token->delete();
96
+
97
+
return id(new AphrontRedirectResponse())->setURI('/');
98
+
}
99
+
}
100
+
101
+
$len_caption = null;
102
+
if ($min_len) {
103
+
$len_caption = pht('Minimum password length: %d characters.', $min_len);
104
+
}
105
+
106
+
if ($viewer->hasPassword()) {
107
+
$title = pht('Reset Password');
108
+
$crumb = pht('Reset Password');
109
+
$submit = pht('Reset Password');
110
+
} else {
111
+
$title = pht('Set Password');
112
+
$crumb = pht('Set Password');
113
+
$submit = pht('Set Account Password');
114
+
}
115
+
116
+
$form = id(new AphrontFormView())
117
+
->setViewer($viewer)
118
+
->addHiddenInput('key', $key)
119
+
->appendChild(
120
+
id(new AphrontFormPasswordControl())
121
+
->setDisableAutocomplete(true)
122
+
->setLabel(pht('New Password'))
123
+
->setError($e_password)
124
+
->setName('password'))
125
+
->appendChild(
126
+
id(new AphrontFormPasswordControl())
127
+
->setDisableAutocomplete(true)
128
+
->setLabel(pht('Confirm Password'))
129
+
->setCaption($len_caption)
130
+
->setError($e_confirm)
131
+
->setName('confirm'))
132
+
->appendChild(
133
+
id(new AphrontFormSubmitControl())
134
+
->addCancelButton('/', pht('Skip This Step'))
135
+
->setValue($submit));
136
+
137
+
$form_box = id(new PHUIObjectBoxView())
138
+
->setHeaderText($title)
139
+
->setFormErrors($errors)
140
+
->setBackground(PHUIObjectBoxView::WHITE_CONFIG)
141
+
->setForm($form);
142
+
143
+
$main_view = id(new PHUITwoColumnView())
144
+
->setFooter($form_box);
145
+
146
+
$crumbs = $this->buildApplicationCrumbs()
147
+
->addTextCrumb($crumb)
148
+
->setBorder(true);
149
+
150
+
return $this->newPage()
151
+
->setTitle($title)
152
+
->setCrumbs($crumbs)
153
+
->appendChild($main_view);
154
+
}
155
+
}
+4
src/applications/people/storage/PhabricatorUser.php
+4
src/applications/people/storage/PhabricatorUser.php
···
262
262
PhabricatorPeopleUserPHIDType::TYPECONST);
263
263
}
264
264
265
+
public function hasPassword() {
266
+
return (bool)strlen($this->passwordHash);
267
+
}
268
+
265
269
public function setPassword(PhutilOpaqueEnvelope $envelope) {
266
270
if (!$this->getPHID()) {
267
271
throw new Exception(
+22
-57
src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+22
-57
src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
···
35
35
$min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
36
36
$min_len = (int)$min_len;
37
37
38
-
// NOTE: To change your password, you need to prove you own the account,
39
-
// either by providing the old password or by carrying a token to
40
-
// the workflow from a password reset email.
41
-
42
-
$key = $request->getStr('key');
43
-
$password_type = PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE;
44
-
45
-
$token = null;
46
-
if ($key) {
47
-
$token = id(new PhabricatorAuthTemporaryTokenQuery())
48
-
->setViewer($user)
49
-
->withTokenResources(array($user->getPHID()))
50
-
->withTokenTypes(array($password_type))
51
-
->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
52
-
->withExpired(false)
53
-
->executeOne();
54
-
}
38
+
// NOTE: Users can also change passwords through the separate "set/reset"
39
+
// interface which is reached by logging in with a one-time token after
40
+
// registration or password reset. If this flow changes, that flow may
41
+
// also need to change.
55
42
56
43
$e_old = true;
57
44
$e_new = true;
···
59
46
60
47
$errors = array();
61
48
if ($request->isFormPost()) {
62
-
if (!$token) {
63
-
$envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
64
-
if (!$user->comparePassword($envelope)) {
65
-
$errors[] = pht('The old password you entered is incorrect.');
66
-
$e_old = pht('Invalid');
67
-
}
49
+
$envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
50
+
if (!$user->comparePassword($envelope)) {
51
+
$errors[] = pht('The old password you entered is incorrect.');
52
+
$e_old = pht('Invalid');
68
53
}
69
54
70
55
$pass = $request->getStr('new_pw');
···
98
83
99
84
unset($unguarded);
100
85
101
-
if ($token) {
102
-
// Destroy the token.
103
-
$token->delete();
104
-
105
-
// If this is a password set/reset, kick the user to the home page
106
-
// after we update their account.
107
-
$next = '/';
108
-
} else {
109
-
$next = $this->getPanelURI('?saved=true');
110
-
}
86
+
$next = $this->getPanelURI('?saved=true');
111
87
112
88
id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
113
89
$user,
···
125
101
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
126
102
$can_upgrade = false;
127
103
128
-
// Only show this stuff if we aren't on the reset workflow. We can
129
-
// do resets regardless of the old hasher's availability.
130
-
if (!$token) {
131
-
$errors[] = pht(
132
-
'Your password is currently hashed using an algorithm which is '.
133
-
'no longer available on this install.');
134
-
$errors[] = pht(
135
-
'Because the algorithm implementation is missing, your password '.
136
-
'can not be used or updated.');
137
-
$errors[] = pht(
138
-
'To set a new password, request a password reset link from the '.
139
-
'login screen and then follow the instructions.');
140
-
}
104
+
$errors[] = pht(
105
+
'Your password is currently hashed using an algorithm which is '.
106
+
'no longer available on this install.');
107
+
$errors[] = pht(
108
+
'Because the algorithm implementation is missing, your password '.
109
+
'can not be used or updated.');
110
+
$errors[] = pht(
111
+
'To set a new password, request a password reset link from the '.
112
+
'login screen and then follow the instructions.');
141
113
}
142
114
143
115
if ($can_upgrade) {
···
153
125
$len_caption = pht('Minimum password length: %d characters.', $min_len);
154
126
}
155
127
156
-
$form = new AphrontFormView();
157
-
$form
158
-
->setUser($user)
159
-
->addHiddenInput('key', $key);
160
-
161
-
if (!$token) {
162
-
$form->appendChild(
128
+
$form = id(new AphrontFormView())
129
+
->setViewer($user)
130
+
->appendChild(
163
131
id(new AphrontFormPasswordControl())
164
132
->setLabel(pht('Old Password'))
165
133
->setError($e_old)
166
-
->setName('old_pw'));
167
-
}
168
-
169
-
$form
134
+
->setName('old_pw'))
170
135
->appendChild(
171
136
id(new AphrontFormPasswordControl())
172
137
->setDisableAutocomplete(true)