@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
Make two-factor auth actually work
Summary:
Ref T4398. Allows auth factors to render and validate when prompted to take a hi-sec action.
This has a whole lot of rough edges still (see D8875) but does fundamentally work correctly.
Test Plan:
- Added two different TOTP factors to my account for EXTRA SECURITY.
- Took hisec actions with no auth factors, and with attached auth factors.
- Hit all the error/failure states of the hisec entry process.
- Verified hisec failures appear in activity logs.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D8886
+171
-36
+2
src/aphront/configuration/AphrontDefaultApplicationConfiguration.php
+2
src/aphront/configuration/AphrontDefaultApplicationConfiguration.php
+66
-23
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+66
-23
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
···
227
227
228
228
$session = $viewer->getSession();
229
229
230
+
// Check if the session is already in high security mode.
230
231
$token = $this->issueHighSecurityToken($session);
231
232
if ($token) {
232
233
return $token;
233
234
}
234
235
236
+
// Load the multi-factor auth sources attached to this account.
237
+
$factors = id(new PhabricatorAuthFactorConfig())->loadAllWhere(
238
+
'userPHID = %s',
239
+
$viewer->getPHID());
240
+
241
+
// If the account has no associated multi-factor auth, just issue a token
242
+
// without putting the session into high security mode. This is generally
243
+
// easier for users. A minor but desirable side effect is that when a user
244
+
// adds an auth factor, existing sessions won't get a free pass into hisec,
245
+
// since they never actually got marked as hisec.
246
+
if (!$factors) {
247
+
return $this->issueHighSecurityToken($session, true);
248
+
}
249
+
250
+
$validation_results = array();
235
251
if ($request->isHTTPPost()) {
236
252
$request->validateCSRF();
237
253
if ($request->getExists(AphrontRequest::TYPE_HISEC)) {
238
254
239
-
// TODO: Actually verify that the user provided some multi-factor
240
-
// auth credentials here. For now, we just let you enter high
241
-
// security.
255
+
$ok = true;
256
+
foreach ($factors as $factor) {
257
+
$id = $factor->getID();
258
+
$impl = $factor->requireImplementation();
259
+
260
+
$validation_results[$id] = $impl->processValidateFactorForm(
261
+
$factor,
262
+
$viewer,
263
+
$request);
242
264
243
-
$until = time() + phutil_units('15 minutes in seconds');
244
-
$session->setHighSecurityUntil($until);
265
+
if (!$impl->isFactorValid($factor, $validation_results[$id])) {
266
+
$ok = false;
267
+
}
268
+
}
245
269
246
-
queryfx(
247
-
$session->establishConnection('w'),
248
-
'UPDATE %T SET highSecurityUntil = %d WHERE id = %d',
249
-
$session->getTableName(),
250
-
$until,
251
-
$session->getID());
270
+
if ($ok) {
271
+
$until = time() + phutil_units('15 minutes in seconds');
272
+
$session->setHighSecurityUntil($until);
273
+
274
+
queryfx(
275
+
$session->establishConnection('w'),
276
+
'UPDATE %T SET highSecurityUntil = %d WHERE id = %d',
277
+
$session->getTableName(),
278
+
$until,
279
+
$session->getID());
252
280
253
-
$log = PhabricatorUserLog::initializeNewLog(
254
-
$viewer,
255
-
$viewer->getPHID(),
256
-
PhabricatorUserLog::ACTION_ENTER_HISEC);
257
-
$log->save();
281
+
$log = PhabricatorUserLog::initializeNewLog(
282
+
$viewer,
283
+
$viewer->getPHID(),
284
+
PhabricatorUserLog::ACTION_ENTER_HISEC);
285
+
$log->save();
286
+
} else {
287
+
$log = PhabricatorUserLog::initializeNewLog(
288
+
$viewer,
289
+
$viewer->getPHID(),
290
+
PhabricatorUserLog::ACTION_FAIL_HISEC);
291
+
$log->save();
292
+
}
258
293
}
259
294
}
260
295
···
264
299
}
265
300
266
301
throw id(new PhabricatorAuthHighSecurityRequiredException())
267
-
->setCancelURI($cancel_uri);
302
+
->setCancelURI($cancel_uri)
303
+
->setFactors($factors)
304
+
->setFactorValidationResults($validation_results);
268
305
}
269
306
270
307
···
291
328
* @return AphrontFormView Renderable form.
292
329
*/
293
330
public function renderHighSecurityForm(
331
+
array $factors,
332
+
array $validation_results,
294
333
PhabricatorUser $viewer,
295
334
AphrontRequest $request) {
296
335
297
-
// TODO: This is stubbed.
298
-
299
336
$form = id(new AphrontFormView())
300
337
->setUser($viewer)
301
-
->appendRemarkupInstructions('')
302
-
->appendChild(
303
-
id(new AphrontFormTextControl())
304
-
->setLabel(pht('Secret Stuff')))
305
338
->appendRemarkupInstructions('');
339
+
340
+
foreach ($factors as $factor) {
341
+
$factor->requireImplementation()->renderValidateFactorForm(
342
+
$factor,
343
+
$form,
344
+
$viewer,
345
+
idx($validation_results, $factor->getID()));
346
+
}
347
+
348
+
$form->appendRemarkupInstructions('');
306
349
307
350
return $form;
308
351
}
+21
src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php
+21
src/applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php
···
3
3
final class PhabricatorAuthHighSecurityRequiredException extends Exception {
4
4
5
5
private $cancelURI;
6
+
private $factors;
7
+
private $factorValidationResults;
8
+
9
+
public function setFactorValidationResults(array $results) {
10
+
$this->factorValidationResults = $results;
11
+
return $this;
12
+
}
13
+
14
+
public function getFactorValidationResults() {
15
+
return $this->factorValidationResults;
16
+
}
17
+
18
+
public function setFactors(array $factors) {
19
+
assert_instances_of($factors, 'PhabricatorAuthFactorConfig');
20
+
$this->factors = $factors;
21
+
return $this;
22
+
}
23
+
24
+
public function getFactors() {
25
+
return $this->factors;
26
+
}
6
27
7
28
public function setCancelURI($cancel_uri) {
8
29
$this->cancelURI = $cancel_uri;
+23
src/applications/auth/factor/PhabricatorAuthFactor.php
+23
src/applications/auth/factor/PhabricatorAuthFactor.php
···
10
10
AphrontRequest $request,
11
11
PhabricatorUser $user);
12
12
13
+
abstract public function renderValidateFactorForm(
14
+
PhabricatorAuthFactorConfig $config,
15
+
AphrontFormView $form,
16
+
PhabricatorUser $viewer,
17
+
$validation_result);
18
+
19
+
abstract public function processValidateFactorForm(
20
+
PhabricatorAuthFactorConfig $config,
21
+
PhabricatorUser $viewer,
22
+
AphrontRequest $request);
23
+
24
+
public function isFactorValid(
25
+
PhabricatorAuthFactorConfig $config,
26
+
$validation_result) {
27
+
return (idx($validation_result, 'valid') === true);
28
+
}
29
+
30
+
public function getParameterName(
31
+
PhabricatorAuthFactorConfig $config,
32
+
$name) {
33
+
return 'authfactor.'.$config->getID().'.'.$name;
34
+
}
35
+
13
36
public static function getAllFactors() {
14
37
static $factors;
15
38
+43
src/applications/auth/factor/PhabricatorAuthFactorTOTP.php
+43
src/applications/auth/factor/PhabricatorAuthFactorTOTP.php
···
97
97
98
98
}
99
99
100
+
public function renderValidateFactorForm(
101
+
PhabricatorAuthFactorConfig $config,
102
+
AphrontFormView $form,
103
+
PhabricatorUser $viewer,
104
+
$validation_result) {
105
+
106
+
if (!$validation_result) {
107
+
$validation_result = array();
108
+
}
109
+
110
+
$form->appendChild(
111
+
id(new AphrontFormTextControl())
112
+
->setName($this->getParameterName($config, 'totpcode'))
113
+
->setLabel(pht('App Code'))
114
+
->setCaption(pht('Factor Name: %s', $config->getFactorName()))
115
+
->setValue(idx($validation_result, 'value'))
116
+
->setError(idx($validation_result, 'error', true)));
117
+
}
118
+
119
+
public function processValidateFactorForm(
120
+
PhabricatorAuthFactorConfig $config,
121
+
PhabricatorUser $viewer,
122
+
AphrontRequest $request) {
123
+
124
+
$code = $request->getStr($this->getParameterName($config, 'totpcode'));
125
+
$key = new PhutilOpaqueEnvelope($config->getFactorSecret());
126
+
127
+
if (self::verifyTOTPCode($viewer, $key, $code)) {
128
+
return array(
129
+
'error' => null,
130
+
'value' => $code,
131
+
'valid' => true,
132
+
);
133
+
} else {
134
+
return array(
135
+
'error' => strlen($code) ? pht('Invalid') : pht('Required'),
136
+
'value' => $code,
137
+
'valid' => false,
138
+
);
139
+
}
140
+
}
141
+
142
+
100
143
public static function generateNewTOTPKey() {
101
144
return strtoupper(Filesystem::readRandomCharacters(16));
102
145
}
+13
src/applications/auth/storage/PhabricatorAuthFactorConfig.php
+13
src/applications/auth/storage/PhabricatorAuthFactorConfig.php
···
26
26
return idx(PhabricatorAuthFactor::getAllFactors(), $this->getFactorKey());
27
27
}
28
28
29
+
public function requireImplementation() {
30
+
$impl = $this->getImplementation();
31
+
if (!$impl) {
32
+
throw new Exception(
33
+
pht(
34
+
'Attempting to operate on multi-factor auth which has no '.
35
+
'corresponding implementation (factor key is "%s").',
36
+
$this->getFactorKey()));
37
+
}
38
+
39
+
return $impl;
40
+
}
41
+
29
42
}
+2
src/applications/people/storage/PhabricatorUserLog.php
+2
src/applications/people/storage/PhabricatorUserLog.php
···
29
29
30
30
const ACTION_ENTER_HISEC = 'hisec-enter';
31
31
const ACTION_EXIT_HISEC = 'hisec-exit';
32
+
const ACTION_FAIL_HISEC = 'hisec-fail';
32
33
33
34
const ACTION_MULTI_ADD = 'multi-add';
34
35
const ACTION_MULTI_REMOVE = 'multi-remove';
···
66
67
self::ACTION_CHANGE_USERNAME => pht('Change Username'),
67
68
self::ACTION_ENTER_HISEC => pht('Hisec: Enter'),
68
69
self::ACTION_EXIT_HISEC => pht('Hisec: Exit'),
70
+
self::ACTION_FAIL_HISEC => pht('Hisec: Failed Attempt'),
69
71
self::ACTION_MULTI_ADD => pht('Multi-Factor: Add Factor'),
70
72
self::ACTION_MULTI_REMOVE => pht('Multi-Factor: Remove Factor'),
71
73
);
-5
src/applications/settings/panel/PhabricatorSettingsPanelMultiFactor.php
-5
src/applications/settings/panel/PhabricatorSettingsPanelMultiFactor.php
···
15
15
return pht('Authentication');
16
16
}
17
17
18
-
public function isEnabled() {
19
-
// TODO: Enable this panel once more pieces work correctly.
20
-
return false;
21
-
}
22
-
23
18
public function processRequest(AphrontRequest $request) {
24
19
if ($request->getExists('new')) {
25
20
return $this->processNew($request);
+1
-8
src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php
+1
-8
src/applications/settings/panel/PhabricatorSettingsPanelSSHKeys.php
···
38
38
return $this->renderKeyListView($request);
39
39
}
40
40
41
-
/*
42
-
43
-
NOTE: Uncomment this to test hisec.
44
-
TOOD: Implement this fully once hisec does something useful.
45
-
46
41
$token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
47
42
$viewer,
48
43
$request,
49
-
'/settings/panel/ssh/');
50
-
51
-
*/
44
+
$this->getPanelURI());
52
45
53
46
$id = nonempty($edit, $delete);
54
47