From 7d309a8e46caca66425cac75501f729791983611 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 29 Jan 2015 14:41:09 -0800 Subject: [PATCH] Application Emails - make various user email editing paths respect application emails Summary: Ref T3404. The only mildly sketchy bit is these codepaths all load the application email directly, by-passing privacy. I think this is necessary because not getting to see an application doesn't mean you should be able to break the application by registering a colliding email address. Test Plan: Tried to add a registered application email to a user account via the web ui and got a pretty error. Ran unit tests. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T3404 Differential Revision: https://secure.phabricator.com/D11565 --- .../PhabricatorAuthRegisterController.php | 13 +++++++++++- .../PhabricatorMetaMTAApplicationEmail.php | 21 +++++++++++++++++++ .../people/editor/PhabricatorUserEditor.php | 8 +++++++ .../PhabricatorUserEditorTestCase.php | 20 ++++++++++++++++++ ...PhabricatorEmailAddressesSettingsPanel.php | 10 +++++++++ 5 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php index 72a97b1523..53adf6be0d 100644 --- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php +++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php @@ -62,6 +62,18 @@ final class PhabricatorAuthRegisterController if (!PhabricatorUserEmail::isValidAddress($default_email)) { $default_email = null; } + if ($default_email !== null) { + // We should bypass policy here becase e.g. limiting an application use + // to a subset of users should not allow the others to overwrite + // configured application emails + $application_email = id(new PhabricatorMetaMTAApplicationEmailQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withAddresses(array($default_email)) + ->executeOne(); + if ($application_email) { + $default_email = null; + } + } if ($default_email !== null) { // If the account source provided an email, but it's not allowed by @@ -86,7 +98,6 @@ final class PhabricatorAuthRegisterController // 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. // TODO: See T3472. if ($default_email !== null) { diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php index 0cec7f1950..561301f7a8 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php @@ -61,6 +61,27 @@ final class PhabricatorMetaMTAApplicationEmail return idx($this->configData, $key, $default); } + + public function getInUseMessage() { + $applications = PhabricatorApplication::getAllApplications(); + $applications = mpull($applications, null, 'getPHID'); + $application = idx( + $applications, + $this->getApplicationPHID()); + if ($application) { + $message = pht( + 'The address %s is configured to be used by the %s Application.', + $this->getAddress(), + $application->getName()); + } else { + $message = pht( + 'The address %s is configured to be used by an application.', + $this->getAddress()); + } + + return $message; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/people/editor/PhabricatorUserEditor.php b/src/applications/people/editor/PhabricatorUserEditor.php index ed038ee59c..5c3ff67a6c 100644 --- a/src/applications/people/editor/PhabricatorUserEditor.php +++ b/src/applications/people/editor/PhabricatorUserEditor.php @@ -578,6 +578,14 @@ final class PhabricatorUserEditor extends PhabricatorEditor { if (!PhabricatorUserEmail::isAllowedAddress($email->getAddress())) { throw new Exception(PhabricatorUserEmail::describeAllowedAddresses()); } + + $application_email = id(new PhabricatorMetaMTAApplicationEmailQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withAddresses(array($email->getAddress())) + ->executeOne(); + if ($application_email) { + throw new Exception($application_email->getInUseMessage()); + } } private function revokePasswordResetLinks(PhabricatorUser $user) { diff --git a/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php b/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php index cbd2a938ed..f5c3c73a9a 100644 --- a/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php +++ b/src/applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php @@ -53,6 +53,26 @@ final class PhabricatorUserEditorTestCase extends PhabricatorTestCase { $this->assertTrue($caught instanceof Exception); } + public function testRegistrationEmailApplicationEmailCollide() { + $app_email = 'bugs@whitehouse.gov'; + $app_email_object = + PhabricatorMetaMTAApplicationEmail::initializeNewAppEmail( + $this->generateNewTestUser()); + $app_email_object->setAddress($app_email); + $app_email_object->setApplicationPHID('test'); + $app_email_object->save(); + + $caught = null; + try { + $this->registerUser( + 'PhabricatorUserEditorTestCaseDomain', + $app_email); + } catch (Exception $ex) { + $caught = $ex; + } + $this->assertTrue($caught instanceof Exception); + } + private function registerUser($username, $email) { $user = id(new PhabricatorUser()) ->setUsername($username) diff --git a/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php b/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php index c4f5d15877..4794814592 100644 --- a/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php @@ -185,6 +185,16 @@ final class PhabricatorEmailAddressesSettingsPanel $e_email = pht('Disallowed'); $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); } + if ($e_email === true) { + $application_email = id(new PhabricatorMetaMTAApplicationEmailQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withAddresses(array($email)) + ->executeOne(); + if ($application_email) { + $e_email = pht('In Use'); + $errors[] = $application_email->getInUseMessage(); + } + } if (!$errors) { $object = id(new PhabricatorUserEmail())