archive/phabricator
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

New Registration Workflow

Browse Source 
Summary:
Currently, registration and authentication are pretty messy. Two concrete problems:

  - The `PhabricatorLDAPRegistrationController` and `PhabricatorOAuthDefaultRegistrationController` controllers are giant copy/pastes of one another. This is really bad.
  - We can't practically implement OpenID because we can't reissue the authentication request.

Additionally, the OAuth registration controller can be replaced wholesale by config, which is a huge API surface area and a giant mess.

Broadly, the problem right now is that registration does too much: we hand it some set of indirect credentials (like OAuth tokens) and expect it to take those the entire way to a registered user. Instead, break registration into smaller steps:

  - User authenticates with remote service.
  - Phabricator pulls information (remote account ID, username, email, real name, profile picture, etc) from the remote service and saves it as `PhabricatorUserCredentials`.
  - Phabricator hands the `PhabricatorUserCredentials` to the registration form, which is agnostic about where they originate from: it can process LDAP credentials, OAuth credentials, plain old email credentials, HTTP basic auth credentials, etc.

This doesn't do anything yet -- there is no way to create credentials objects (and no storage patch), but I wanted to get any initial feedback, especially about the event call for T2394. In particular, I think the implementation would look something like this:

  $profile = $event->getValue('profile')

  $username = $profile->getDefaultUsername();
  $is_employee = is_this_a_facebook_employee($username);
  if (!$is_employee) {
    throw new Exception("You are not employed at Facebook.");
  }

  $fbid = get_fbid_for_facebook_username($username);
  $profile->setDefaultEmail($fbid);

  $profile->setCanEditUsername(false);
  $profile->setCanEditEmail(false);
  $profile->setCanEditRealName(false);
  $profile->setShouldVerifyEmail(true);

Seem reasonable?

Test Plan: N/A yet, probably fatals.

Reviewers: vrana, btrahan, codeblock, chad

Reviewed By: btrahan

CC: aran, asherkin, nh, wez

Maniphest Tasks: T1536, T2394

Differential Revision: https://secure.phabricator.com/D4647
This commit is contained in:
epriestley
2013-06-16 10:13:49 -07:00
parent 8744cdb699
commit db1cf41ec4
6 changed files with 616 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
src/__phutil_library_map__.php
View File

@@ -815,6 +815,7 @@ phutil_register_library_map(array(
'PhabricatorAuditReplyHandler' => 'applications/audit/mail/PhabricatorAuditReplyHandler.php',
'PhabricatorAuditStatusConstants' => 'applications/audit/constants/PhabricatorAuditStatusConstants.php',
'PhabricatorAuthController' => 'applications/auth/controller/PhabricatorAuthController.php',
'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php',
'PhabricatorAuthenticationConfigOptions' => 'applications/config/option/PhabricatorAuthenticationConfigOptions.php',
'PhabricatorBarePageExample' => 'applications/uiexample/examples/PhabricatorBarePageExample.php',
'PhabricatorBarePageView' => 'view/page/PhabricatorBarePageView.php',
@@ -1335,6 +1336,7 @@ phutil_register_library_map(array(
'PhabricatorRecaptchaConfigOptions' => 'applications/config/option/PhabricatorRecaptchaConfigOptions.php',
'PhabricatorRedirectController' => 'applications/base/controller/PhabricatorRedirectController.php',
'PhabricatorRefreshCSRFController' => 'applications/auth/controller/PhabricatorRefreshCSRFController.php',
'PhabricatorRegistrationProfile' => 'applications/people/storage/PhabricatorRegistrationProfile.php',
'PhabricatorRemarkupControl' => 'view/form/control/PhabricatorRemarkupControl.php',
'PhabricatorRemarkupRuleEmbedFile' => 'applications/files/remarkup/PhabricatorRemarkupRuleEmbedFile.php',
'PhabricatorRemarkupRuleImageMacro' => 'applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php',
@@ -2667,6 +2669,7 @@ phutil_register_library_map(array(
'PhabricatorAuditPreviewController' => 'PhabricatorAuditController',
'PhabricatorAuditReplyHandler' => 'PhabricatorMailReplyHandler',
'PhabricatorAuthController' => 'PhabricatorController',
'PhabricatorAuthRegisterController' => 'PhabricatorAuthController',
'PhabricatorAuthenticationConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorBarePageExample' => 'PhabricatorUIExample',
'PhabricatorBarePageView' => 'AphrontPageView',
@@ -3182,6 +3185,7 @@ phutil_register_library_map(array(
'PhabricatorRecaptchaConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorRedirectController' => 'PhabricatorController',
'PhabricatorRefreshCSRFController' => 'PhabricatorAuthController',
'PhabricatorRegistrationProfile' => 'Phobject',
'PhabricatorRemarkupControl' => 'AphrontFormTextAreaControl',
'PhabricatorRemarkupRuleEmbedFile' => 'PhutilRemarkupRule',
'PhabricatorRemarkupRuleImageMacro' => 'PhutilRemarkupRule',

8
src/applications/auth/application/PhabricatorApplicationAuth.php
View File

@@ -29,4 +29,12 @@ final class PhabricatorApplicationAuth extends PhabricatorApplication {
return $items;
}
public function getRoutes() {
return array(
'/auth/' => array(
'register/(?P<akey>[^/]+)/' => 'PhabricatorAuthRegisterController',
),
);
}
}

35
src/applications/auth/controller/PhabricatorAuthController.php
View File

@@ -14,4 +14,39 @@ abstract class PhabricatorAuthController extends PhabricatorController {
return $response->setContent($page->render());
}
protected function renderErrorPage($title, array $messages) {
$view = new AphrontErrorView();
$view->setTitle($title);
$view->setErrors($messages);
return $this->buildApplicationPage(
$view,
array(
'title' => $title,
'device' => true,
'dust' => true,
));
}
protected function establishWebSession(PhabricatorUser $user) {
$session_key = $user->establishSession('web');
$request = $this->getRequest();
// NOTE: We allow disabled users to login and roadblock them later, so
// there's no check for users being disabled here.
$request->setCookie('phusr', $user->getUsername());
$request->setCookie('phsid', $session_key);
$request->clearCookie('phreg');
}
protected function buildLoginValidateResponse(PhabricatorUser $user) {
$validate_uri = new PhutilURI($this->getApplicationURI('validate/'));
$validate_uri->setQueryParam('phusr', $user->getUsername());
return id(new AphrontRedirectResponse())->setURI((string)$validate_uri);
}
}

483
src/applications/auth/controller/PhabricatorAuthRegisterController.php Normal file
View File

@@ -0,0 +1,483 @@
<?php
final class PhabricatorAuthRegisterController
extends PhabricatorAuthController {
private $accountKey;
private $account;
private $provider;
public function shouldRequireLogin() {
return false;
}
public function willProcessRequest(array $data) {
$this->accountKey = idx($data, 'akey');
}
public function processRequest() {
$request = $this->getRequest();
if ($request->getUser()->isLoggedIn()) {
return $this->renderError(pht('You are already logged in.'));
}
if (strlen($this->accountKey)) {
$response = $this->loadAccount();
} else {
$response = $this->loadDefaultAccount();
}
if ($response) {
return $response;
}
$account = $this->account;
$user = new PhabricatorUser();
$default_username = $account->getUsername();
$default_realname = $account->getRealName();
$default_email = $account->getEmail();
if ($default_email) {
// If the account source provided an email but it's not allowed by
// the configuration, just pretend we didn't get an email at all.
if (!PhabricatorUserEmail::isAllowedAddress($default_email)) {
$default_email = null;
}
// If the account source provided an email, but another account already
// has that email, just pretend we didn't get an email.
// TODO: See T3340.
if ($default_email) {
$same_email = id(new PhabricatorUserEmail())->loadOneWhere(
'address = %s',
$default_email);
if ($same_email) {
$default_email = null;
}
}
}
$profile = id(new PhabricatorRegistrationProfile())
->setDefaultUsername($default_username)
->setDefaultEmail($default_email)
->setDefaultRealName($default_realname)
->setCanEditUsername(true)
->setCanEditEmail(($default_email === null))
->setCanEditRealName(true)
->setShouldVerifyEmail(false);
$event_type = PhabricatorEventType::TYPE_AUTH_WILLREGISTERUSER;
$event_data = array(
'account' => $account,
'profile' => $profile,
);
$event = id(new PhabricatorEvent($event_type, $event_data))
->setUser($user);
PhutilEventEngine::dispatchEvent($event);
$default_username = $profile->getDefaultUsername();
$default_email = $profile->getDefaultEmail();
$default_realname = $profile->getDefaultRealName();
$can_edit_username = $profile->getCanEditUsername();
$can_edit_email = $profile->getCanEditEmail();
$can_edit_realname = $profile->getCanEditRealName();
$must_set_password = $this->provider->shouldRequireRegistrationPassword();
$can_edit_anything = $profile->getCanEditAnything() || $must_set_password;
$force_verify = $profile->getShouldVerifyEmail();
$value_username = $default_username;
$value_realname = $default_realname;
$value_email = $default_email;
$value_password = null;
$errors = array();
$e_username = strlen($value_username) ? null : true;
$e_realname = strlen($value_realname) ? null : true;
$e_email = strlen($value_email) ? null : true;
$e_password = true;
$min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
$min_len = (int)$min_len;
if ($request->isFormPost() || !$can_edit_anything) {
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
if ($can_edit_username) {
$value_username = $request->getStr('username');
if (!strlen($value_username)) {
$e_username = pht('Required');
$errors[] = pht('Username is required.');
} else if (!PhabricatorUser::validateUsername($value_username)) {
$e_username = pht('Invalid');
$errors[] = PhabricatorUser::describeValidUsername();
} else {
$e_username = null;
}
}
if ($must_set_password) {
$value_password = $request->getStr('password');
$value_confirm = $request->getStr('confirm');
if (!strlen($value_password)) {
$e_password = pht('Required');
$errors[] = pht('You must choose a password.');
} else if ($value_password !== $value_confirm) {
$e_password = pht('No Match');
$errors[] = pht('Password and confirmation must match.');
} else if (strlen($value_password) < $min_len) {
$e_password = pht('Too Short');
$errors[] = pht(
'Password is too short (must be at least %d characters long).',
$min_len);
} else {
$e_password = null;
}
}
if ($can_edit_email) {
$value_email = $request->getStr('email');
if (!strlen($value_email)) {
$e_email = pht('Required');
$errors[] = pht('Email is required.');
} else if (!PhabricatorUserEmail::isAllowedAddress($value_email)) {
$e_email = pht('Invalid');
$errors[] = PhabricatorUserEmail::describeAllowedAddresses();
} else {
$e_email = null;
}
}
if ($can_edit_realname) {
$value_realname = $request->getStr('realName');
if (!strlen($value_realname)) {
$e_realname = pht('Required');
$errors[] = pht('Real name is required.');
} else {
$e_realname = null;
}
}
if (!$errors) {
$image = $this->loadProfilePicture($account);
if ($image) {
$user->setProfileImagePHID($image->getPHID());
}
try {
if ($force_verify) {
$verify_email = true;
} else {
$verify_email =
($account->getEmailVerified()) &&
($value_email === $default_email);
}
$email_obj = id(new PhabricatorUserEmail())
->setAddress($value_email)
->setIsVerified((int)$verify_email);
$user->setUsername($value_username);
$user->setRealname($value_realname);
$user->openTransaction();
$editor = id(new PhabricatorUserEditor())
->setActor($user);
$editor->createNewUser($user, $email_obj);
if ($must_set_password) {
$envelope = new PhutilOpaqueEnvelope($value_password);
$editor->changePassword($user, $envelope);
}
$account->setUserPHID($user->getPHID());
$this->provider->willRegisterAccount($account);
$account->save();
$user->saveTransaction();
$this->establishWebSession($user);
if (!$email_obj->getIsVerified()) {
$email_obj->sendVerificationEmail($user);
}
return $this->buildLoginValidateResponse($user);
} catch (AphrontQueryDuplicateKeyException $exception) {
$same_username = id(new PhabricatorUser())->loadOneWhere(
'userName = %s',
$user->getUserName());
$same_email = id(new PhabricatorUserEmail())->loadOneWhere(
'address = %s',
$value_email);
if ($same_username) {
$e_username = pht('Duplicate');
$errors[] = pht('Another user already has that username.');
}
if ($same_email) {
// TODO: See T3340.
$e_email = pht('Duplicate');
$errors[] = pht('Another user already has that email.');
}
if (!$same_username && !$same_email) {
throw $exception;
}
}
}
unset($unguarded);
}
$error_view = null;
if ($errors) {
$error_view = new AphrontErrorView();
$error_view->setTitle(pht('Registration Failed'));
$error_view->setErrors($errors);
}
$form = id(new AphrontFormView())
->setUser($request->getUser())
->appendChild(
id(new AphrontFormTextControl())
->setLabel(pht('Username'))
->setName('username')
->setValue($value_username)
->setError($e_username));
if ($must_set_password) {
$form->appendChild(
id(new AphrontFormPasswordControl())
->setLabel(pht('Password'))
->setName('password')
->setError($e_password)
->setCaption(
$min_len
? pht('Minimum length of %d characters.', $min_len)
: null));
$form->appendChild(
id(new AphrontFormPasswordControl())
->setLabel(pht('Confirm Password'))
->setName('confirm')
->setError($e_password));
}
if ($can_edit_email) {
$form->appendChild(
id(new AphrontFormTextControl())
->setLabel(pht('Email'))
->setName('email')
->setValue($value_email)
->setCaption(PhabricatorUserEmail::describeAllowedAddresses())
->setError($e_email));
}
if ($can_edit_realname) {
$form->appendChild(
id(new AphrontFormTextControl())
->setLabel(pht('Real Name'))
->setName('realName')
->setValue($value_realname)
->setError($e_realname));
}
$form->appendChild(
id(new AphrontFormSubmitControl())
->setValue(pht('Create Account')));
$title = pht('Phabricator Registration');
$crumbs = $this->buildApplicationCrumbs();
$crumbs->addCrumb(
id(new PhabricatorCrumbView())
->setName(pht('Register')));
return $this->buildApplicationPage(
array(
$crumbs,
$error_view,
$form,
),
array(
'title' => $title,
'device' => true,
'dust' => true,
));
}
private function loadAccount() {
$request = $this->getRequest();
if (!$this->accountKey) {
return $this->renderError(pht('Request did not include account key.'));
}
$account = id(new PhabricatorExternalAccount())->loadOneWhere(
'accountSecret = %s',
$this->accountKey);
if (!$account) {
return $this->renderError(pht('No registration account.'));
}
if ($account->getUserPHID()) {
return $this->renderError(
pht(
'The account you are attempting to register with is already '.
'registered to another user.'));
}
$registration_key = $request->getCookie('phreg');
// NOTE: This registration key check is not strictly necessary, because
// we're only creating new accounts, not linking existing accounts. It
// might be more hassle than it is worth, especially for email.
//
// The attack this prevents is getting to the registration screen, then
// copy/pasting the URL and getting someone else to click it and complete
// the process. They end up with an account bound to credentials you
// control. This doesn't really let you do anything meaningful, though,
// since you could have simply completed the process yourself.
if (!$registration_key) {
return $this->renderError(
pht(
'Your browser did not submit a registration key with the request. '.
'You must use the same browser to begin and complete registration. '.
'Check that cookies are enabled and try again.'));
}
if ($registration_key != $account->getProperty('registrationKey')) {
return $this->renderError(
pht(
'Your browser submitted a different registration key than the one '.
'associated with this account. You may need to clear your cookies.'));
}
$other_account = id(new PhabricatorExternalAccount())->loadAllWhere(
'accountType = %s AND accountDomain = %s AND accountID = %s
AND id != %d',
$account->getAccountType(),
$account->getAccountDomain(),
$account->getAccountID(),
$account->getID());
if ($other_account) {
return $this->renderError(
pht(
'The account you are attempting to register with already belongs '.
'to another user.'));
}
$provider = PhabricatorAuthProvider::getEnabledProviderByKey(
$account->getProviderKey());
if (!$provider) {
return $this->renderError(
pht(
'The account you are attempting to register with uses a nonexistent '.
'or disabled authentication provider (with key "%s"). An '.
'administrator may have recently disabled this provider.',
$account->getProviderKey()));
}
if (!$provider->shouldAllowRegistration()) {
// TODO: This is a routine error if you click "Login" on an external
// auth source which doesn't allow registration. The error should be
// more tailored.
return $this->renderError(
pht(
'The account you are attempting to register with uses an '.
'authentication provider ("%s") which does not allow registration. '.
'An administrator may have recently disabled registration with this '.
'provider.',
$provider->getProviderName()));
}
$this->account = $account;
$this->provider = $provider;
return null;
}
private function loadDefaultAccount() {
$providers = PhabricatorAuthProvider::getAllEnabledProviders();
foreach ($providers as $key => $provider) {
if (!$provider->shouldAllowRegistration()) {
unset($providers[$key]);
continue;
}
if (!$provider->isDefaultRegistrationProvider()) {
unset($providers[$key]);
}
}
if (!$providers) {
return $this->renderError(
pht(
"There are no configured default registration providers."));
} else if (count($providers) > 1) {
return $this->renderError(
pht(
"There are too many configured default registration providers."));
}
$this->account = $provider->getDefaultExternalAccount();
$this->provider = $provider;
return null;
}
private function loadProfilePicture(PhabricatorExternalAccount $account) {
$phid = $account->getProfileImagePHID();
if (!$phid) {
return null;
}
// NOTE: Use of omnipotent user is okay here because the registering user
// can not control the field value, and we can't use their user object to
// do meaningful policy checks anyway since they have not registered yet.
// Reaching this means the user holds the account secret key and the
// registration secret key, and thus has permission to view the image.
$file = id(new PhabricatorFileQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
->withPHIDs(array($phid))
->executeOne();
if (!$file) {
return null;
}
try {
$xformer = new PhabricatorImageTransformer();
return $xformer->executeProfileTransform(
$file,
$width = 50,
$min_height = 50,
$max_height = 50);
} catch (Exception $ex) {
phlog($ex);
return null;
}
}
private function renderError($message) {
return $this->renderErrorPage(
pht('Registration Failed'),
array($message));
}
}

84
src/applications/people/storage/PhabricatorRegistrationProfile.php Normal file
View File

@@ -0,0 +1,84 @@
<?php
final class PhabricatorRegistrationProfile extends Phobject {
private $defaultUsername;
private $defaultEmail;
private $defaultRealName;
private $canEditUsername;
private $canEditEmail;
private $canEditRealName;
private $shouldVerifyEmail;
public function setShouldVerifyEmail($should_verify_email) {
$this->shouldVerifyEmail = $should_verify_email;
return $this;
}
public function getShouldVerifyEmail() {
return $this->shouldVerifyEmail;
}
public function setCanEditEmail($can_edit_email) {
$this->canEditEmail = $can_edit_email;
return $this;
}
public function getCanEditEmail() {
return $this->canEditEmail;
}
public function setCanEditRealName($can_edit_real_name) {
$this->canEditRealName = $can_edit_real_name;
return $this;
}
public function getCanEditRealName() {
return $this->canEditRealName;
}
public function setCanEditUsername($can_edit_username) {
$this->canEditUsername = $can_edit_username;
return $this;
}
public function getCanEditUsername() {
return $this->canEditUsername;
}
public function setDefaultEmail($default_email) {
$this->defaultEmail = $default_email;
return $this;
}
public function getDefaultEmail() {
return $this->defaultEmail;
}
public function setDefaultRealName($default_real_name) {
$this->defaultRealName = $default_real_name;
return $this;
}
public function getDefaultRealName() {
return $this->defaultRealName;
}
public function setDefaultUsername($default_username) {
$this->defaultUsername = $default_username;
return $this;
}
public function getDefaultUsername() {
return $this->defaultUsername;
}
public function getCanEditAnything() {
return $this->getCanEditUsername() ||
$this->getCanEditEmail() ||
$this->getCanEditRealName();
}
}

2
src/infrastructure/events/constant/PhabricatorEventType.php
View File

@@ -36,4 +36,6 @@ final class PhabricatorEventType extends PhutilEventType {
const TYPE_UI_DIDRENDERHOVERCARD = 'ui.didRenderHovercard';
const TYPE_PEOPLE_DIDRENDERMENU = 'people.didRenderMenu';
const TYPE_AUTH_WILLREGISTERUSER = 'auth.willRegisterUser';
}