diff --git a/conf/default.conf.php b/conf/default.conf.php index 4d949fd363..c13a78e94f 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -454,9 +454,28 @@ return array( 'auth.sshkeys.enabled' => false, // If true, email addresses must be verified (by clicking a link in an - // email) before a user can login. By default, verification is optional. + // email) before a user can login. By default, verification is optional + // unless 'auth.email-domains' is nonempty (see below). 'auth.require-email-verification' => false, + // You can restrict allowed email addresses to certain domains (like + // "yourcompany.com") by setting a list of allowed domains here. Users will + // only be allowed to register using email addresses at one of the domains, + // and will only be able to add new email addresses for these domains. If + // you configure this, it implies 'auth.require-email-verification'. + // + // To configure email domains, set a list of domains like this: + // + // array( + // 'yourcompany.com', + // 'yourcompany.co.uk', + // ) + // + // You should omit the "@" from domains. Note that the domain must match + // exactly. If you allow "yourcompany.com", that permits "joe@yourcompany.com" + // but rejects "joe@mail.yourcompany.com". + 'auth.email-domains' => array(), + // -- Accounts -------------------------------------------------------------- // diff --git a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php index 5a52d8fdaf..4d6a89f4eb 100644 --- a/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php +++ b/src/applications/auth/controller/oauthregistration/default/PhabricatorOAuthDefaultRegistrationController.php @@ -36,6 +36,26 @@ final class PhabricatorOAuthDefaultRegistrationController $new_email = $provider->retrieveUserEmail(); + if ($new_email) { + // If the user's OAuth provider account has an email address but the + // email address domain is not allowed by the Phabricator configuration, + // we just pretend the provider did not supply an address. + // + // For instance, if the user uses Google OAuth and their Google address + // is "joe@personal.com" but Phabricator is configured to require users + // use "@company.com" addresses, we show a prompt below and tell the user + // to provide their "@company.com" address. They can still use the OAuth + // account to login, they just need to associate their account with an + // allowed address. + // + // If the OAuth address is fine, we just use it and don't prompt the user. + if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { + $new_email = null; + } + } + + $show_email_input = ($new_email === null); + if ($request->isFormPost()) { $user->setUsername($request->getStr('username')); @@ -60,6 +80,14 @@ final class PhabricatorOAuthDefaultRegistrationController } } + if ($new_email) { + $email_ok = PhabricatorUserEmail::isAllowedAddress($new_email); + if (!$email_ok) { + $e_email = 'Invalid'; + $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); + } + } + if (!strlen($user->getRealName())) { $user->setRealName($request->getStr('realname')); if (!strlen($user->getRealName())) { @@ -160,16 +188,17 @@ final class PhabricatorOAuthDefaultRegistrationController ->setValue($user->getUsername()) ->setError($e_username)); - if ($provider->retrieveUserEmail() === null) { + if ($show_email_input) { $form->appendChild( id(new AphrontFormTextControl()) ->setLabel('Email') ->setName('email') ->setValue($request->getStr('email')) + ->setCaption(PhabricatorUserEmail::describeAllowedAddresses()) ->setError($e_email)); } - if ($provider->retrieveUserRealName () === null) { + if ($provider->retrieveUserRealName() === null) { $form->appendChild( id(new AphrontFormTextControl()) ->setLabel('Real Name') diff --git a/src/applications/base/controller/base/PhabricatorController.php b/src/applications/base/controller/base/PhabricatorController.php index 9edf431525..94a8ed6734 100644 --- a/src/applications/base/controller/base/PhabricatorController.php +++ b/src/applications/base/controller/base/PhabricatorController.php @@ -31,9 +31,7 @@ abstract class PhabricatorController extends AphrontController { } public function shouldRequireEmailVerification() { - $config_key = 'auth.require-email-verification'; - - $need_verify = PhabricatorEnv::getEnvConfig($config_key); + $need_verify = PhabricatorUserEmail::isEmailVerificationRequired(); $need_login = $this->shouldRequireLogin(); return ($need_login && $need_verify); @@ -79,6 +77,11 @@ abstract class PhabricatorController extends AphrontController { } } + if ($this->shouldRequireLogin() && !$user->getPHID()) { + $login_controller = newv('PhabricatorLoginController', array($request)); + return $this->delegateToController($login_controller); + } + if ($this->shouldRequireEmailVerification()) { $email = $user->loadPrimaryEmail(); if (!$email) { @@ -93,11 +96,6 @@ abstract class PhabricatorController extends AphrontController { } } - if ($this->shouldRequireLogin() && !$user->getPHID()) { - $login_controller = newv('PhabricatorLoginController', array($request)); - return $this->delegateToController($login_controller); - } - if ($this->shouldRequireAdmin() && !$user->getIsAdmin()) { return new Aphront403Response(); } diff --git a/src/applications/base/controller/base/__init__.php b/src/applications/base/controller/base/__init__.php index 71078ccc60..84710a9f1e 100644 --- a/src/applications/base/controller/base/__init__.php +++ b/src/applications/base/controller/base/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'aphront/console/core'); phutil_require_module('phabricator', 'aphront/controller'); phutil_require_module('phabricator', 'aphront/response/403'); phutil_require_module('phabricator', 'aphront/response/webpage'); +phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/queryfx'); diff --git a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php index b69456f469..451ec78bb4 100644 --- a/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/api/PhabricatorConduitAPIController.php @@ -387,7 +387,7 @@ final class PhabricatorConduitAPIController 'User is disabled.'); } - if (PhabricatorEnv::getEnvConfig('auth.require-email-verification')) { + if (PhabricatorUserEmail::isEmailVerificationRequired()) { $email = $user->loadPrimaryEmail(); if (!$email) { return array( diff --git a/src/applications/conduit/controller/api/__init__.php b/src/applications/conduit/controller/api/__init__.php index b2bd93c152..c48c2fad65 100644 --- a/src/applications/conduit/controller/api/__init__.php +++ b/src/applications/conduit/controller/api/__init__.php @@ -16,9 +16,9 @@ phutil_require_module('phabricator', 'applications/conduit/storage/methodcalllog phutil_require_module('phabricator', 'applications/oauthserver/scope'); phutil_require_module('phabricator', 'applications/oauthserver/server'); phutil_require_module('phabricator', 'applications/oauthserver/storage/accesstoken'); +phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'applications/people/storage/user'); phutil_require_module('phabricator', 'infrastructure/accesslog'); -phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'storage/queryfx'); phutil_require_module('phabricator', 'view/control/table'); phutil_require_module('phabricator', 'view/layout/panel'); diff --git a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php index 944021e290..16ac401b3d 100644 --- a/src/applications/people/controller/edit/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/edit/PhabricatorPeopleEditController.php @@ -136,6 +136,9 @@ final class PhabricatorPeopleEditController if (!strlen($new_email)) { $errors[] = 'Email is required.'; $e_email = 'Required'; + } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { + $e_email = 'Invalid'; + $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); } if ($request->getStr('role') == 'agent') { @@ -249,6 +252,7 @@ final class PhabricatorPeopleEditController ->setName('email') ->setDisabled($is_immutable) ->setValue($new_email) + ->setCaption(PhabricatorUserEmail::describeAllowedAddresses()) ->setError($e_email)); } else { $email = $user->loadPrimaryEmail(); diff --git a/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php b/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php index 362363b1d6..e3af109929 100644 --- a/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/email/PhabricatorUserEmailSettingsPanelController.php @@ -171,6 +171,9 @@ final class PhabricatorUserEmailSettingsPanelController if (!strlen($email)) { $e_email = 'Required'; $errors[] = 'Email is required.'; + } else if (!PhabricatorUserEmail::isAllowedAddress($email)) { + $e_email = 'Invalid'; + $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); } if (!$errors) { @@ -216,6 +219,7 @@ final class PhabricatorUserEmailSettingsPanelController ->setLabel('Email') ->setName('email') ->setValue($request->getStr('email')) + ->setCaption(PhabricatorUserEmail::describeAllowedAddresses()) ->setError($e_email)); $dialog = id(new AphrontDialogView()) diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index 67db0eaa18..6fa19424d3 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -65,6 +65,8 @@ final class PhabricatorUserEditor { // Always set a new user's email address to primary. $email->setIsPrimary(1); + $this->willAddEmail($email); + $user->openTransaction(); $user->save(); @@ -241,6 +243,8 @@ final class PhabricatorUserEditor { $email->setIsPrimary(0); $email->setUserPHID($user->getPHID()); + $this->willAddEmail($email); + $user->openTransaction(); $user->beginWriteLocking(); @@ -390,4 +394,20 @@ final class PhabricatorUserEditor { return $this->actor; } + + /** + * @task internal + */ + private function willAddEmail(PhabricatorUserEmail $email) { + + // Hard check before write to prevent creation of disallowed email + // addresses. Normally, the application does checks and raises more + // user friendly errors for us, but we omit the courtesy checks on some + // pathways like administrative scripts for simplicity. + + if (!PhabricatorUserEmail::isAllowedAddress($email->getAddress())) { + throw new Exception(PhabricatorUserEmail::describeAllowedAddresses()); + } + } + } diff --git a/src/applications/people/editor/__init__.php b/src/applications/people/editor/__init__.php index 3896d621ca..6b81022f2a 100644 --- a/src/applications/people/editor/__init__.php +++ b/src/applications/people/editor/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/people/storage/email'); phutil_require_module('phabricator', 'applications/people/storage/log'); diff --git a/src/applications/people/storage/email/PhabricatorUserEmail.php b/src/applications/people/storage/email/PhabricatorUserEmail.php index 954409598b..17b6423b2a 100644 --- a/src/applications/people/storage/email/PhabricatorUserEmail.php +++ b/src/applications/people/storage/email/PhabricatorUserEmail.php @@ -17,7 +17,8 @@ */ /** - * @task email Email About Email + * @task restrictions Domain Restrictions + * @task email Email About Email */ final class PhabricatorUserEmail extends PhabricatorUserDAO { @@ -39,6 +40,61 @@ final class PhabricatorUserEmail extends PhabricatorUserDAO { } +/* -( Domain Restrictions )------------------------------------------------ */ + + + /** + * @task restrictions + */ + public static function isAllowedAddress($address) { + $allowed_domains = PhabricatorEnv::getEnvConfig('auth.email-domains'); + if (!$allowed_domains) { + return true; + } + + $addr_obj = new PhutilEmailAddress($address); + + $domain = $addr_obj->getDomainName(); + if (!$domain) { + return false; + } + + return in_array($domain, $allowed_domains); + } + + + /** + * @task restrictions + */ + public static function describeAllowedAddresses() { + $domains = PhabricatorEnv::getEnvConfig('auth.email-domains'); + if (!$domains) { + return null; + } + + if (count($domains) == 1) { + return 'Email address must be @'.head($domains); + } else { + return 'Email address must be at one of: '. + implode(', ', $domains); + } + } + + + /** + * Check if this install requires email verification. + * + * @return bool True if email addresses must be verified. + * + * @task restrictions + */ + public static function isEmailVerificationRequired() { + // NOTE: Configuring required email domains implies required verification. + return PhabricatorEnv::getEnvConfig('auth.require-email-verification') || + PhabricatorEnv::getEnvConfig('auth.email-domains'); + } + + /* -( Email About Email )-------------------------------------------------- */ diff --git a/src/applications/people/storage/email/__init__.php b/src/applications/people/storage/email/__init__.php index 77b722bb2d..69baa1c1c4 100644 --- a/src/applications/people/storage/email/__init__.php +++ b/src/applications/people/storage/email/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'applications/people/storage/base'); phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'filesystem'); +phutil_require_module('phutil', 'parser/emailaddress'); phutil_require_module('phutil', 'utils');