recaptime.dev / phorge
@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
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Allow applications to require a High Security token without doing a session upgrade

Summary:
Ref T13222. See PHI873. Currently, when applications prompt users to enter MFA, their session upgrades as a side effect.

In some cases (like managing your email addresses) it makes sense to upgrade your session for a little while since it's common to make multiple edits in sequence (add a new address, make it primary, remove an old address). We generally want MFA to stay out of the way and not feel annoying.

In other cases, we don't expect multiple high-security actions in a row. Notably, PHI873 looks at more "one-shot" use cases where a prompt is answering a specific workflow. We already have at least one of these in the upstream: answering an MFA prompt when signing a Legalpad document.

Introduce a "token" workflow (in contrast to the existing "session") workflow that just does a one-shot prompt without upgrading your session statefully. Then, make Legalpad use this new workflow.

Note that this workflow has a significant problem: if the form submission is invalid for some other reason, we re-prompt you on resubmit. In Legalpad, this workflow looks like:

- Forget to check the "I agree" checkbox.
- Submit the form.
- Get prompted for MFA.
- Answer MFA prompt.
- Get dumped back to the form with an error.
- When you fix the error and submit again, you have to do another MFA check.

This isn't a fatal flaw in Legalpad, but would become a problem with wider adoption. I'll work on fixing this (so the MFA token sticks to the form) in the next set of changes.

Roughly, this is headed toward "MFA sticks to the form/workflow" instead of "MFA sticks to the user/session".

Test Plan:
- Signed a legalpad document with MFA enabled.
- Was prompted for MFA.
- Session no longer upgraded (no purple "session in high security" badge).
- Submitted form with error, answered MFA, fixed error, submitted form again.
- Bad behavior: got re-prompted for MFA. In the future, MFA should stick to the form.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D19843

epriestley 1d0b99e1 bb369c7b

+63 -6
+56
src/applications/auth/engine/PhabricatorAuthSessionEngine.php
··· 346 346 347 347 348 348 /** 349 + * Require the user respond to a high security (MFA) check. 350 + * 351 + * This method differs from @{method:requireHighSecuritySession} in that it 352 + * does not upgrade the user's session as a side effect. This method is 353 + * appropriate for one-time checks. 354 + * 355 + * @param PhabricatorUser User whose session needs to be in high security. 356 + * @param AphrontReqeust Current request. 357 + * @param string URI to return the user to if they cancel. 358 + * @return PhabricatorAuthHighSecurityToken Security token. 359 + * @task hisec 360 + */ 361 + public function requireHighSecurityToken( 362 + PhabricatorUser $viewer, 363 + AphrontRequest $request, 364 + $cancel_uri) { 365 + 366 + return $this->newHighSecurityToken( 367 + $viewer, 368 + $request, 369 + $cancel_uri, 370 + false, 371 + false); 372 + } 373 + 374 + 375 + /** 349 376 * Require high security, or prompt the user to enter high security. 350 377 * 351 378 * If the user's session is in high security, this method will return a 352 379 * token. Otherwise, it will throw an exception which will eventually 353 380 * be converted into a multi-factor authentication workflow. 354 381 * 382 + * This method upgrades the user's session to high security for a short 383 + * period of time, and is appropriate if you anticipate they may need to 384 + * take multiple high security actions. To perform a one-time check instead, 385 + * use @{method:requireHighSecurityToken}. 386 + * 355 387 * @param PhabricatorUser User whose session needs to be in high security. 356 388 * @param AphrontReqeust Current request. 357 389 * @param string URI to return the user to if they cancel. ··· 367 399 $cancel_uri, 368 400 $jump_into_hisec = false) { 369 401 402 + return $this->newHighSecurityToken( 403 + $viewer, 404 + $request, 405 + $cancel_uri, 406 + false, 407 + true); 408 + } 409 + 410 + private function newHighSecurityToken( 411 + PhabricatorUser $viewer, 412 + AphrontRequest $request, 413 + $cancel_uri, 414 + $jump_into_hisec, 415 + $upgrade_session) { 416 + 370 417 if (!$viewer->hasSession()) { 371 418 throw new Exception( 372 419 pht('Requiring a high-security session from a user with no session!')); 373 420 } 421 + 422 + // TODO: If a user answers a "requireHighSecurityToken()" prompt and hits 423 + // a "requireHighSecuritySession()" prompt a short time later, the one-shot 424 + // token should be good enough to upgrade the session. 374 425 375 426 $session = $viewer->getSession(); 376 427 ··· 438 489 // If we have a partial session and are not jumping directly into 439 490 // hisec, just issue a token without putting it in high security 440 491 // mode. 492 + return $this->issueHighSecurityToken($session, true); 493 + } 494 + 495 + // If we aren't upgrading the session itself, just issue a token. 496 + if (!$upgrade_session) { 441 497 return $this->issueHighSecurityToken($session, true); 442 498 } 443 499
+6 -5
src/applications/legalpad/controller/LegalpadDocumentSignController.php
··· 149 149 } 150 150 151 151 $errors = array(); 152 + $hisec_token = null; 152 153 if ($request->isFormOrHisecPost() && !$has_signed) { 153 154 154 155 // Require two-factor auth to sign legal documents. 155 156 if ($viewer->isLoggedIn()) { 156 - $engine = new PhabricatorAuthSessionEngine(); 157 - $engine->requireHighSecuritySession( 158 - $viewer, 159 - $request, 160 - '/'.$document->getMonogram()); 157 + $hisec_token = id(new PhabricatorAuthSessionEngine()) 158 + ->requireHighSecurityToken( 159 + $viewer, 160 + $request, 161 + $document->getURI()); 161 162 } 162 163 163 164 list($form_data, $errors, $field_errors) = $this->readSignatureForm(
+1 -1
src/applications/legalpad/storage/LegalpadDocument.php
··· 120 120 return 'L'.$this->getID(); 121 121 } 122 122 123 - public function getViewURI() { 123 + public function getURI() { 124 124 return '/'.$this->getMonogram(); 125 125 } 126 126