revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-14 02:42:40 +01:00
Code Issues Releases Wiki Activity

Ask users to sign Legalpad documents before requiring they enroll in MFA

Browse source 
Summary:
Depends on D18789. Ref T13024. See PHI223. Currently, if `security.require-multi-factor-auth` and Legalpad "Signature Required" documents are //both// set, it's not possible to survive account registration, since MFA is requiried to sign and signatures are required to add MFA.

Instead, check for signatures before requiring MFA enrollment. This makes logical sense, since it's silly to add MFA if you don't agree to a Terms of Service or whatever.

(Note that if you already have MFA, we prompt for that first, before either of these steps, which also makes sense.)

Test Plan: Configured `security.require-multi-factor-auth`. Added a signature-required document. Loaded a page as a new user. Went through signature workflow, then through the MFA enrollment workflow.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13024

Differential Revision: https://secure.phabricator.com/D18790
This commit is contained in:
epriestley 2017-11-27 17:02:09 -08:00
parent e850bc6b95
commit c7718d3a21
1 changed files with 13 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
src/applications/base/controller/PhabricatorController.php
View file

@ -160,6 +160,15 @@ abstract class PhabricatorController extends AphrontController {
} }
} }
// Require users sign Legalpad documents before we check if they have
// MFA. If we don't do this, they can get stuck in a state where they
// can't add MFA until they sign, and can't sign until they add MFA.
// See T13024 and PHI223.
$result = $this->requireLegalpadSignatures();
if ($result !== null) {
return $result;
}
// Check if the user needs to configure MFA. // Check if the user needs to configure MFA.
$need_mfa = $this->shouldRequireMultiFactorEnrollment(); $need_mfa = $this->shouldRequireMultiFactorEnrollment();
$have_mfa = $user->getIsEnrolledInMultiFactor(); $have_mfa = $user->getIsEnrolledInMultiFactor();
@ -226,12 +235,6 @@ abstract class PhabricatorController extends AphrontController {
} }
} }
$result = $this->requireLegalpadSignatures();
if ($result !== null) {
return $result;
}
// NOTE: We do this last so that users get a login page instead of a 403 // NOTE: We do this last so that users get a login page instead of a 403
// if they need to login. // if they need to login.
if ($this->shouldRequireAdmin() && !$user->getIsAdmin()) { if ($this->shouldRequireAdmin() && !$user->getIsAdmin()) {
@ -523,6 +526,10 @@ abstract class PhabricatorController extends AphrontController {
} }
private function requireLegalpadSignatures() { private function requireLegalpadSignatures() {
if (!$this->shouldRequireLogin()) {
return null;
}
if ($this->shouldAllowLegallyNonCompliantUsers()) { if ($this->shouldAllowLegallyNonCompliantUsers()) {
return null; return null;
} }