Skip to content

Commit 50376aa

Browse files
author
epriestley
committedMay 1, 2014
Require multiple auth factors to establish web sessions
Summary: Ref T4398. This prompts users for multi-factor auth on login. Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one. Test Plan: - Used Conduit. - Logged in as multi-factor user. - Logged in as no-factor user. - Tried to do non-login-things with a partial session. - Reviewed account activity logs. {F149295} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4398 Differential Revision: https://secure.phabricator.com/D8922
1 parent 1e6b2f2 commit 50376aa

15 files changed

+190
-27
lines changed
 
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
ALTER TABLE {$NAMESPACE}_user.phabricator_session
2+
ADD isPartial BOOL NOT NULL DEFAULT 0;

‎src/__phutil_library_map__.php

+2
Original file line numberDiff line numberDiff line change
@@ -1213,6 +1213,7 @@
12131213
'PhabricatorAuthFactorConfig' => 'applications/auth/storage/PhabricatorAuthFactorConfig.php',
12141214
'PhabricatorAuthFactorTOTP' => 'applications/auth/factor/PhabricatorAuthFactorTOTP.php',
12151215
'PhabricatorAuthFactorTOTPTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTOTPTestCase.php',
1216+
'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php',
12161217
'PhabricatorAuthHighSecurityRequiredException' => 'applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php',
12171218
'PhabricatorAuthHighSecurityToken' => 'applications/auth/data/PhabricatorAuthHighSecurityToken.php',
12181219
'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php',
@@ -3980,6 +3981,7 @@
39803981
'PhabricatorAuthFactorConfig' => 'PhabricatorAuthDAO',
39813982
'PhabricatorAuthFactorTOTP' => 'PhabricatorAuthFactor',
39823983
'PhabricatorAuthFactorTOTPTestCase' => 'PhabricatorTestCase',
3984+
'PhabricatorAuthFinishController' => 'PhabricatorAuthController',
39833985
'PhabricatorAuthHighSecurityRequiredException' => 'Exception',
39843986
'PhabricatorAuthLinkController' => 'PhabricatorAuthController',
39853987
'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController',

‎src/aphront/console/DarkConsoleController.php

+4
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ public function shouldRequireEnabledUser() {
1616
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
1717
}
1818

19+
public function shouldAllowPartialSessions() {
20+
return true;
21+
}
22+
1923
public function processRequest() {
2024
$request = $this->getRequest();
2125
$user = $request->getUser();

‎src/aphront/console/DarkConsoleDataController.php

+4
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ public function shouldRequireEnabledUser() {
1515
return !PhabricatorEnv::getEnvConfig('darkconsole.always-on');
1616
}
1717

18+
public function shouldAllowPartialSessions() {
19+
return true;
20+
}
21+
1822
public function willProcessRequest(array $data) {
1923
$this->key = $data['key'];
2024
}

‎src/applications/auth/application/PhabricatorApplicationAuth.php

+1
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ public function getRoutes() {
8383
'register/(?:(?P<akey>[^/]+)/)?' => 'PhabricatorAuthRegisterController',
8484
'start/' => 'PhabricatorAuthStartController',
8585
'validate/' => 'PhabricatorAuthValidateController',
86+
'finish/' => 'PhabricatorAuthFinishController',
8687
'unlink/(?P<pkey>[^/]+)/' => 'PhabricatorAuthUnlinkController',
8788
'(?P<action>link|refresh)/(?P<pkey>[^/]+)/'
8889
=> 'PhabricatorAuthLinkController',

‎src/applications/auth/controller/PhabricatorAuthController.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ protected function loginUser(PhabricatorUser $user) {
8282
$should_login = $event->getValue('shouldLogin');
8383
if ($should_login) {
8484
$session_key = id(new PhabricatorAuthSessionEngine())
85-
->establishSession($session_type, $user->getPHID());
85+
->establishSession($session_type, $user->getPHID(), $partial = true);
8686

8787
// NOTE: We allow disabled users to login and roadblock them later, so
8888
// there's no check for users being disabled here.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
<?php
2+
3+
final class PhabricatorAuthFinishController
4+
extends PhabricatorAuthController {
5+
6+
public function shouldRequireLogin() {
7+
return false;
8+
}
9+
10+
public function shouldAllowPartialSessions() {
11+
return true;
12+
}
13+
14+
public function processRequest() {
15+
$request = $this->getRequest();
16+
$viewer = $request->getUser();
17+
18+
// If the user already has a full session, just kick them out of here.
19+
$has_partial_session = $viewer->hasSession() &&
20+
$viewer->getSession()->getIsPartial();
21+
if (!$has_partial_session) {
22+
return id(new AphrontRedirectResponse())->setURI('/');
23+
}
24+
25+
$engine = new PhabricatorAuthSessionEngine();
26+
27+
try {
28+
$token = $engine->requireHighSecuritySession(
29+
$viewer,
30+
$request,
31+
'/logout/');
32+
} catch (PhabricatorAuthHighSecurityRequiredException $ex) {
33+
$form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm(
34+
$ex->getFactors(),
35+
$ex->getFactorValidationResults(),
36+
$viewer,
37+
$request);
38+
39+
return $this->newDialog()
40+
->setTitle(pht('Provide Multi-Factor Credentials'))
41+
->setShortTitle(pht('Multi-Factor Login'))
42+
->setWidth(AphrontDialogView::WIDTH_FORM)
43+
->addHiddenInput(AphrontRequest::TYPE_HISEC, true)
44+
->appendParagraph(
45+
pht(
46+
'Welcome, %s. To complete the login process, provide your '.
47+
'multi-factor credentials.',
48+
phutil_tag('strong', array(), $viewer->getUsername())))
49+
->appendChild($form->buildLayoutView())
50+
->setSubmitURI($request->getPath())
51+
->addCancelButton($ex->getCancelURI())
52+
->addSubmitButton(pht('Continue'));
53+
}
54+
55+
// Upgrade the partial session to a full session.
56+
$engine->upgradePartialSession($viewer);
57+
58+
// TODO: It might be nice to add options like "bind this session to my IP"
59+
// here, even for accounts without multi-factor auth attached to them.
60+
61+
$next = PhabricatorCookies::getNextURICookie($request);
62+
$request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
63+
64+
if (!PhabricatorEnv::isValidLocalWebResource($next)) {
65+
$next = '/';
66+
}
67+
68+
return id(new AphrontRedirectResponse())->setURI($next);
69+
}
70+
71+
}

‎src/applications/auth/controller/PhabricatorAuthValidateController.php

+6-8
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ public function shouldRequireLogin() {
77
return false;
88
}
99

10+
public function shouldAllowPartialSessions() {
11+
return true;
12+
}
13+
1014
public function processRequest() {
1115
$request = $this->getRequest();
1216
$viewer = $request->getUser();
@@ -54,14 +58,8 @@ public function processRequest() {
5458
return $this->renderErrors($failures);
5559
}
5660

57-
$next = PhabricatorCookies::getNextURICookie($request);
58-
$request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
59-
60-
if (!PhabricatorEnv::isValidLocalWebResource($next)) {
61-
$next = '/';
62-
}
63-
64-
return id(new AphrontRedirectResponse())->setURI($next);
61+
$finish_uri = $this->getApplicationURI('finish/');
62+
return id(new AphrontRedirectResponse())->setURI($finish_uri);
6563
}
6664

6765
private function renderErrors(array $messages) {

‎src/applications/auth/controller/PhabricatorLogoutController.php

+4
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@ public function shouldRequireEnabledUser() {
1717
return false;
1818
}
1919

20+
public function shouldAllowPartialSessions() {
21+
return true;
22+
}
23+
2024
public function processRequest() {
2125
$request = $this->getRequest();
2226
$user = $request->getUser();

‎src/applications/auth/engine/PhabricatorAuthSessionEngine.php

+62-9
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ public function loadUserForSession($session_type, $session_token) {
9494
s.sessionExpires AS s_sessionExpires,
9595
s.sessionStart AS s_sessionStart,
9696
s.highSecurityUntil AS s_highSecurityUntil,
97+
s.isPartial AS s_isPartial,
9798
u.*
9899
FROM %T u JOIN %T s ON u.phid = s.userPHID
99100
AND s.type = %s AND s.sessionKey = %s',
@@ -159,9 +160,10 @@ public function loadUserForSession($session_type, $session_token) {
159160
* @{class:PhabricatorAuthSession}).
160161
* @param phid|null Identity to establish a session for, usually a user
161162
* PHID. With `null`, generates an anonymous session.
163+
* @param bool True to issue a partial session.
162164
* @return string Newly generated session key.
163165
*/
164-
public function establishSession($session_type, $identity_phid) {
166+
public function establishSession($session_type, $identity_phid, $partial) {
165167
// Consume entropy to generate a new session key, forestalling the eventual
166168
// heat death of the universe.
167169
$session_key = Filesystem::readRandomCharacters(40);
@@ -176,26 +178,32 @@ public function establishSession($session_type, $identity_phid) {
176178
// This has a side effect of validating the session type.
177179
$session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
178180

181+
$digest_key = PhabricatorHash::digest($session_key);
182+
179183
// Logging-in users don't have CSRF stuff yet, so we have to unguard this
180184
// write.
181185
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
182186
id(new PhabricatorAuthSession())
183187
->setUserPHID($identity_phid)
184188
->setType($session_type)
185-
->setSessionKey(PhabricatorHash::digest($session_key))
189+
->setSessionKey($digest_key)
186190
->setSessionStart(time())
187191
->setSessionExpires(time() + $session_ttl)
192+
->setIsPartial($partial ? 1 : 0)
188193
->save();
189194

190195
$log = PhabricatorUserLog::initializeNewLog(
191196
null,
192197
$identity_phid,
193-
PhabricatorUserLog::ACTION_LOGIN);
198+
($partial
199+
? PhabricatorUserLog::ACTION_LOGIN_PARTIAL
200+
: PhabricatorUserLog::ACTION_LOGIN));
201+
194202
$log->setDetails(
195203
array(
196204
'session_type' => $session_type,
197205
));
198-
$log->setSession($session_key);
206+
$log->setSession($digest_key);
199207
$log->save();
200208
unset($unguarded);
201209

@@ -287,6 +295,12 @@ public function requireHighSecuritySession(
287295
new PhabricatorAuthTryFactorAction(),
288296
-1);
289297

298+
if ($session->getIsPartial()) {
299+
// If we have a partial session, just issue a token without
300+
// putting it in high security mode.
301+
return $this->issueHighSecurityToken($session, true);
302+
}
303+
290304
$until = time() + phutil_units('15 minutes in seconds');
291305
$session->setHighSecurityUntil($until);
292306

@@ -303,9 +317,6 @@ public function requireHighSecuritySession(
303317
PhabricatorUserLog::ACTION_ENTER_HISEC);
304318
$log->save();
305319
} else {
306-
307-
308-
309320
$log = PhabricatorUserLog::initializeNewLog(
310321
$viewer,
311322
$viewer->getPHID(),
@@ -331,11 +342,15 @@ public function requireHighSecuritySession(
331342
* Issue a high security token for a session, if authorized.
332343
*
333344
* @param PhabricatorAuthSession Session to issue a token for.
345+
* @param bool Force token issue.
334346
* @return PhabricatorAuthHighSecurityToken|null Token, if authorized.
335347
*/
336-
private function issueHighSecurityToken(PhabricatorAuthSession $session) {
348+
private function issueHighSecurityToken(
349+
PhabricatorAuthSession $session,
350+
$force = false) {
351+
337352
$until = $session->getHighSecurityUntil();
338-
if ($until > time()) {
353+
if ($until > time() || $force) {
339354
return new PhabricatorAuthHighSecurityToken();
340355
}
341356
return null;
@@ -390,4 +405,42 @@ public function exitHighSecurity(
390405
$log->save();
391406
}
392407

408+
409+
/**
410+
* Upgrade a partial session to a full session.
411+
*
412+
* @param PhabricatorAuthSession Session to upgrade.
413+
* @return void
414+
*/
415+
public function upgradePartialSession(PhabricatorUser $viewer) {
416+
if (!$viewer->hasSession()) {
417+
throw new Exception(
418+
pht('Upgrading partial session of user with no session!'));
419+
}
420+
421+
$session = $viewer->getSession();
422+
423+
if (!$session->getIsPartial()) {
424+
throw new Exception(pht('Session is not partial!'));
425+
}
426+
427+
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
428+
$session->setIsPartial(0);
429+
430+
queryfx(
431+
$session->establishConnection('w'),
432+
'UPDATE %T SET isPartial = %d WHERE id = %d',
433+
$session->getTableName(),
434+
0,
435+
$session->getID());
436+
437+
$log = PhabricatorUserLog::initializeNewLog(
438+
$viewer,
439+
$viewer->getPHID(),
440+
PhabricatorUserLog::ACTION_LOGIN_FULL);
441+
$log->save();
442+
unset($unguarded);
443+
444+
}
445+
393446
}

‎src/applications/auth/storage/PhabricatorAuthSession.php

+1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
1212
protected $sessionStart;
1313
protected $sessionExpires;
1414
protected $highSecurityUntil;
15+
protected $isPartial;
1516

1617
private $identityObject = self::ATTACHABLE;
1718

‎src/applications/base/controller/PhabricatorController.php

+19-4
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ public function shouldAllowPublic() {
2020
return false;
2121
}
2222

23+
public function shouldAllowPartialSessions() {
24+
return false;
25+
}
26+
2327
public function shouldRequireEmailVerification() {
2428
return PhabricatorUserEmail::isEmailVerificationRequired();
2529
}
@@ -53,7 +57,8 @@ public function willBeginExecution() {
5357
// session. This is used to provide CSRF protection to logged-out users.
5458
$phsid = $session_engine->establishSession(
5559
PhabricatorAuthSession::TYPE_WEB,
56-
null);
60+
null,
61+
$partial = false);
5762

5863
// This may be a resource request, in which case we just don't set
5964
// the cookie.
@@ -133,14 +138,24 @@ public function willBeginExecution() {
133138
return $this->delegateToController($checker_controller);
134139
}
135140

141+
$auth_class = 'PhabricatorApplicationAuth';
142+
$auth_application = PhabricatorApplication::getByClass($auth_class);
143+
144+
// Require partial sessions to finish login before doing anything.
145+
if (!$this->shouldAllowPartialSessions()) {
146+
if ($user->hasSession() &&
147+
$user->getSession()->getIsPartial()) {
148+
$login_controller = new PhabricatorAuthFinishController($request);
149+
$this->setCurrentApplication($auth_application);
150+
return $this->delegateToController($login_controller);
151+
}
152+
}
153+
136154
if ($this->shouldRequireLogin()) {
137155
// This actually means we need either:
138156
// - a valid user, or a public controller; and
139157
// - permission to see the application.
140158

141-
$auth_class = 'PhabricatorApplicationAuth';
142-
$auth_application = PhabricatorApplication::getByClass($auth_class);
143-
144159
$allow_public = $this->shouldAllowPublic() &&
145160
PhabricatorEnv::getEnvConfig('policy.allow-public');
146161

‎src/applications/conduit/method/ConduitAPI_conduit_connect_Method.php

+4-4
Original file line numberDiff line numberDiff line change
@@ -145,10 +145,10 @@ protected function execute(ConduitAPIRequest $request) {
145145
if ($valid != $signature) {
146146
throw new ConduitException('ERR-INVALID-CERTIFICATE');
147147
}
148-
$session_key = id(new PhabricatorAuthSessionEngine())
149-
->establishSession(
150-
PhabricatorAuthSession::TYPE_CONDUIT,
151-
$user->getPHID());
148+
$session_key = id(new PhabricatorAuthSessionEngine())->establishSession(
149+
PhabricatorAuthSession::TYPE_CONDUIT,
150+
$user->getPHID(),
151+
$partial = false);
152152
} else {
153153
throw new ConduitException('ERR-NO-CERTIFICATE');
154154
}

0 commit comments

Comments
 (0)
Failed to load comments.