Skip to content

Commit 87207b2

Browse files
author
epriestley
committedMay 7, 2012
Allow users to have multiple email addresses, and verify emails
Summary: - Move email to a separate table. - Migrate existing email to new storage. - Allow users to add and remove email addresses. - Allow users to verify email addresses. - Allow users to change their primary email address. - Convert all the registration/reset/login code to understand these changes. - There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific. - This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up. Not included here (next steps): - Allow configuration to restrict email to certain domains. - Allow configuration to require validated email. Test Plan: This is a fairly extensive, difficult-to-test change. - From "Email Addresses" interface: - Added new email (verified email verifications sent). - Changed primary email (verified old/new notificactions sent). - Resent verification emails (verified they sent). - Removed email. - Tried to add already-owned email. - Created new users with "accountadmin". Edited existing users with "accountadmin". - Created new users with "add_user.php". - Created new users with web interface. - Clicked welcome email link, verified it verified email. - Reset password. - Linked/unlinked oauth accounts. - Logged in with oauth account. - Logged in with email. - Registered with Oauth account. - Tried to register with OAuth account with duplicate email. - Verified errors for email verification with bad tokens, etc. Reviewers: btrahan, vrana, jungejason Reviewed By: btrahan CC: aran Maniphest Tasks: T1184 Differential Revision: https://secure.phabricator.com/D2393
1 parent 803dea1 commit 87207b2

38 files changed

+900
-140
lines changed
 

‎resources/sql/patches/emailtable.sql

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
CREATE TABLE {$NAMESPACE}_user.user_email (
2+
`id` int unsigned NOT NULL AUTO_INCREMENT PRIMARY KEY,
3+
userPHID varchar(64) collate utf8_bin NOT NULL,
4+
address varchar(128) collate utf8_general_ci NOT NULL,
5+
isVerified bool not null default 0,
6+
isPrimary bool not null default 0,
7+
verificationCode varchar(64) collate utf8_bin,
8+
dateCreated int unsigned not null,
9+
dateModified int unsigned not null,
10+
KEY (userPHID, isPrimary),
11+
UNIQUE KEY (address)
12+
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

‎resources/sql/patches/emailtableport.php

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<?php
2+
3+
/*
4+
* Copyright 2012 Facebook, Inc.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
echo "Migrating user emails...\n";
20+
21+
$table = new PhabricatorUser();
22+
$conn = $table->establishConnection('r');
23+
24+
$emails = queryfx_all(
25+
$conn,
26+
'SELECT phid, email FROM %T',
27+
$table->getTableName());
28+
$emails = ipull($emails, 'email', 'phid');
29+
30+
$etable = new PhabricatorUserEmail();
31+
$econn = $etable->establishConnection('w');
32+
33+
foreach ($emails as $phid => $email) {
34+
35+
// NOTE: Grandfather all existing email in as primary / verified. We generate
36+
// verification codes because they are used for password resets, etc.
37+
38+
echo "Migrating '{$phid}'...\n";
39+
queryfx(
40+
$econn,
41+
'INSERT INTO %T (userPHID, address, verificationCode, isVerified, isPrimary)
42+
VALUES (%s, %s, %s, 1, 1)',
43+
$etable->getTableName(),
44+
$phid,
45+
$email,
46+
PhabricatorFile::readRandomCharacters(24));
47+
}
48+
49+
echo "Done.\n";

‎resources/sql/patches/emailtableremove.sql

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
ALTER TABLE {$NAMESPACE}_user.user DROP email;

‎scripts/user/account_admin.php

+37-24
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@
5555
}
5656
$user = new PhabricatorUser();
5757
$user->setUsername($username);
58+
59+
$is_new = true;
5860
} else {
5961
$original = clone $user;
6062

@@ -66,6 +68,8 @@
6668
echo "Cancelled.\n";
6769
exit(1);
6870
}
71+
72+
$is_new = false;
6973
}
7074

7175
$user_realname = $user->getRealName();
@@ -79,31 +83,29 @@
7983
$user_realname);
8084
$user->setRealName($realname);
8185

82-
$user_email = $user->getEmail();
83-
if (strlen($user_email)) {
84-
$email_prompt = ' ['.$user_email.']';
85-
} else {
86-
$email_prompt = '';
86+
// When creating a new user we prompt for an email address; when editing an
87+
// existing user we just skip this because it would be quite involved to provide
88+
// a reasonable CLI interface for editing multiple addresses and managing email
89+
// verification and primary addresses.
90+
91+
$new_email = null;
92+
if ($is_new) {
93+
do {
94+
$email = phutil_console_prompt("Enter user email address:");
95+
$duplicate = id(new PhabricatorUserEmail())->loadOneWhere(
96+
'address = %s',
97+
$email);
98+
if ($duplicate) {
99+
echo "ERROR: There is already a user with that email address. ".
100+
"Each user must have a unique email address.\n";
101+
} else {
102+
break;
103+
}
104+
} while (true);
105+
106+
$new_email = $email;
87107
}
88108

89-
do {
90-
$email = nonempty(
91-
phutil_console_prompt("Enter user email address{$email_prompt}:"),
92-
$user_email);
93-
$duplicate = id(new PhabricatorUser())->loadOneWhere(
94-
'email = %s',
95-
$email);
96-
if ($duplicate && $duplicate->getID() != $user->getID()) {
97-
$duplicate_username = $duplicate->getUsername();
98-
echo "ERROR: There is already a user with that email address ".
99-
"({$duplicate_username}). Each user must have a unique email ".
100-
"address.\n";
101-
} else {
102-
break;
103-
}
104-
} while (true);
105-
$user->setEmail($email);
106-
107109
$changed_pass = false;
108110
// This disables local echo, so the user's password is not shown as they type
109111
// it.
@@ -126,7 +128,9 @@
126128
printf($tpl, null, 'OLD VALUE', 'NEW VALUE');
127129
printf($tpl, 'Username', $original->getUsername(), $user->getUsername());
128130
printf($tpl, 'Real Name', $original->getRealName(), $user->getRealName());
129-
printf($tpl, 'Email', $original->getEmail(), $user->getEmail());
131+
if ($new_email) {
132+
printf($tpl, 'Email', '', $new_email);
133+
}
130134
printf($tpl, 'Password', null,
131135
($changed_pass !== false)
132136
? 'Updated'
@@ -153,4 +157,13 @@
153157
$user->save();
154158
}
155159

160+
if ($new_email) {
161+
id(new PhabricatorUserEmail())
162+
->setUserPHID($user->getPHID())
163+
->setAddress($new_email)
164+
->setIsVerified(1)
165+
->setIsPrimary(1)
166+
->save();
167+
}
168+
156169
echo "Saved changes.\n";

‎scripts/user/add_user.php

+10-4
Original file line numberDiff line numberDiff line change
@@ -50,20 +50,26 @@
5050
"There is already a user with the username '{$username}'!");
5151
}
5252

53-
$existing_user = id(new PhabricatorUser())->loadOneWhere(
54-
'email = %s',
53+
$existing_email = id(new PhabricatorUserEmail())->loadOneWhere(
54+
'address = %s',
5555
$email);
56-
if ($existing_user) {
56+
if ($existing_email) {
5757
throw new Exception(
5858
"There is already a user with the email '{$email}'!");
5959
}
6060

6161
$user = new PhabricatorUser();
6262
$user->setUsername($username);
63-
$user->setEmail($email);
6463
$user->setRealname($realname);
6564
$user->save();
6665

66+
$email_object = id(new PhabricatorUserEmail())
67+
->setUserPHID($user->getPHID())
68+
->setAddress($email)
69+
->setIsVerified(1)
70+
->setIsPrimary(1)
71+
->save();
72+
6773
$user->sendWelcomeEmail($admin);
6874

6975
echo "Created user '{$username}' (realname='{$realname}', email='{$email}').\n";

‎src/__phutil_library_map__.php

+4
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,7 @@
595595
'PhabricatorEdgeQuery' => 'infrastructure/edges/query/edge',
596596
'PhabricatorEmailLoginController' => 'applications/auth/controller/email',
597597
'PhabricatorEmailTokenController' => 'applications/auth/controller/emailtoken',
598+
'PhabricatorEmailVerificationController' => 'applications/people/controller/emailverification',
598599
'PhabricatorEnv' => 'infrastructure/env',
599600
'PhabricatorEnvTestCase' => 'infrastructure/env/__tests__',
600601
'PhabricatorErrorExample' => 'applications/uiexample/examples/error',
@@ -946,6 +947,7 @@
946947
'PhabricatorUserAccountSettingsPanelController' => 'applications/people/controller/settings/panels/account',
947948
'PhabricatorUserConduitSettingsPanelController' => 'applications/people/controller/settings/panels/conduit',
948949
'PhabricatorUserDAO' => 'applications/people/storage/base',
950+
'PhabricatorUserEmail' => 'applications/people/storage/email',
949951
'PhabricatorUserEmailPreferenceSettingsPanelController' => 'applications/people/controller/settings/panels/emailpref',
950952
'PhabricatorUserEmailSettingsPanelController' => 'applications/people/controller/settings/panels/email',
951953
'PhabricatorUserLog' => 'applications/people/storage/log',
@@ -1534,6 +1536,7 @@
15341536
'PhabricatorEdgeQuery' => 'PhabricatorQuery',
15351537
'PhabricatorEmailLoginController' => 'PhabricatorAuthController',
15361538
'PhabricatorEmailTokenController' => 'PhabricatorAuthController',
1539+
'PhabricatorEmailVerificationController' => 'PhabricatorPeopleController',
15371540
'PhabricatorEnvTestCase' => 'PhabricatorTestCase',
15381541
'PhabricatorErrorExample' => 'PhabricatorUIExample',
15391542
'PhabricatorEvent' => 'PhutilEvent',
@@ -1819,6 +1822,7 @@
18191822
'PhabricatorUserAccountSettingsPanelController' => 'PhabricatorUserSettingsPanelController',
18201823
'PhabricatorUserConduitSettingsPanelController' => 'PhabricatorUserSettingsPanelController',
18211824
'PhabricatorUserDAO' => 'PhabricatorLiskDAO',
1825+
'PhabricatorUserEmail' => 'PhabricatorUserDAO',
18221826
'PhabricatorUserEmailPreferenceSettingsPanelController' => 'PhabricatorUserSettingsPanelController',
18231827
'PhabricatorUserEmailSettingsPanelController' => 'PhabricatorUserSettingsPanelController',
18241828
'PhabricatorUserLog' => 'PhabricatorUserDAO',

‎src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

+3
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,9 @@ public function getURIMap() {
433433
'testpaymentform/' => 'PhortuneStripeTestPaymentFormController',
434434
),
435435
),
436+
437+
'/emailverify/(?P<code>[^/]+)/' =>
438+
'PhabricatorEmailVerificationController',
436439
);
437440
}
438441

‎src/aphront/response/redirect/AphrontRedirectResponse.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public function setURI($uri) {
3131
}
3232

3333
public function getURI() {
34-
return $this->uri;
34+
return (string)$this->uri;
3535
}
3636

3737
public function getHeaders() {

‎src/applications/auth/controller/email/PhabricatorEmailLoginController.php

+10-3
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,24 @@ public function processRequest() {
5757
// it expensive to fish for valid email addresses while giving the user
5858
// a better error if they goof their email.
5959

60-
$target_user = id(new PhabricatorUser())->loadOneWhere(
61-
'email = %s',
60+
$target_email = id(new PhabricatorUserEmail())->loadOneWhere(
61+
'address = %s',
6262
$email);
6363

64+
$target_user = null;
65+
if ($target_email) {
66+
$target_user = id(new PhabricatorUser())->loadOneWhere(
67+
'phid = %s',
68+
$target_email->getUserPHID());
69+
}
70+
6471
if (!$target_user) {
6572
$errors[] = "There is no account associated with that email address.";
6673
$e_email = "Invalid";
6774
}
6875

6976
if (!$errors) {
70-
$uri = $target_user->getEmailLoginURI();
77+
$uri = $target_user->getEmailLoginURI($target_email);
7178
if ($is_serious) {
7279
$body = <<<EOBODY
7380
You can use this link to reset your Phabricator password:

‎src/applications/auth/controller/email/__init__.php

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
phutil_require_module('phabricator', 'aphront/response/400');
1010
phutil_require_module('phabricator', 'applications/auth/controller/base');
1111
phutil_require_module('phabricator', 'applications/metamta/storage/mail');
12+
phutil_require_module('phabricator', 'applications/people/storage/email');
1213
phutil_require_module('phabricator', 'applications/people/storage/user');
1314
phutil_require_module('phabricator', 'infrastructure/env');
1415
phutil_require_module('phabricator', 'view/form/base');

‎src/applications/auth/controller/emailtoken/PhabricatorEmailTokenController.php

+38-5
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,31 @@ public function processRequest() {
5555
$token = $this->token;
5656
$email = $request->getStr('email');
5757

58-
$target_user = id(new PhabricatorUser())->loadOneWhere(
59-
'email = %s',
58+
// NOTE: We need to bind verification to **addresses**, not **users**,
59+
// because we verify addresses when they're used to login this way, and if
60+
// we have a user-based verification you can:
61+
//
62+
// - Add some address you do not own;
63+
// - request a password reset;
64+
// - change the URI in the email to the address you don't own;
65+
// - login via the email link; and
66+
// - get a "verified" address you don't control.
67+
68+
$target_email = id(new PhabricatorUserEmail())->loadOneWhere(
69+
'address = %s',
6070
$email);
6171

62-
if (!$target_user || !$target_user->validateEmailToken($token)) {
72+
$target_user = null;
73+
if ($target_email) {
74+
$target_user = id(new PhabricatorUser())->loadOneWhere(
75+
'phid = %s',
76+
$target_email->getUserPHID());
77+
}
78+
79+
if (!$target_email ||
80+
!$target_user ||
81+
!$target_user->validateEmailToken($target_email, $token)) {
82+
6383
$view = new AphrontRequestFailureView();
6484
$view->setHeader('Unable to Login');
6585
$view->appendChild(
@@ -71,19 +91,32 @@ public function processRequest() {
7191
'<div class="aphront-failure-continue">'.
7292
'<a class="button" href="/login/email/">Send Another Email</a>'.
7393
'</div>');
94+
7495
return $this->buildStandardPageResponse(
7596
$view,
7697
array(
77-
'title' => 'Email Sent',
98+
'title' => 'Login Failure',
7899
));
79100
}
80101

102+
// Verify email so that clicking the link in the "Welcome" email is good
103+
// enough, without requiring users to go through a second round of email
104+
// verification.
105+
106+
$target_email->setIsVerified(1);
107+
$target_email->save();
108+
81109
$session_key = $target_user->establishSession('web');
82110
$request->setCookie('phusr', $target_user->getUsername());
83111
$request->setCookie('phsid', $session_key);
84112

85113
if (PhabricatorEnv::getEnvConfig('account.editable')) {
86-
$next = '/settings/page/password/?token='.$token;
114+
$next = (string)id(new PhutilURI('/settings/page/password/'))
115+
->setQueryParams(
116+
array(
117+
'token' => $token,
118+
'email' => $email,
119+
));
87120
} else {
88121
$next = '/';
89122
}

‎src/applications/auth/controller/emailtoken/__init__.php

+1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
phutil_require_module('phabricator', 'aphront/response/400');
1010
phutil_require_module('phabricator', 'aphront/response/redirect');
1111
phutil_require_module('phabricator', 'applications/auth/controller/base');
12+
phutil_require_module('phabricator', 'applications/people/storage/email');
1213
phutil_require_module('phabricator', 'applications/people/storage/user');
1314
phutil_require_module('phabricator', 'infrastructure/env');
1415
phutil_require_module('phabricator', 'view/page/failure');

‎src/applications/auth/controller/login/PhabricatorLoginController.php

+1-3
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,7 @@ public function processRequest() {
113113
$username_or_email);
114114

115115
if (!$user) {
116-
$user = id(new PhabricatorUser())->loadOneWhere(
117-
'email = %s',
118-
$username_or_email);
116+
$user = PhabricatorUser::loadOneWithEmailAddress($username_or_email);
119117
}
120118

121119
if (!$errors) {

‎src/applications/auth/controller/oauth/PhabricatorOAuthLoginController.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,8 @@ public function processRequest() {
176176

177177
$oauth_email = $provider->retrieveUserEmail();
178178
if ($oauth_email) {
179-
$known_email = id(new PhabricatorUser())
180-
->loadOneWhere('email = %s', $oauth_email);
179+
$known_email = id(new PhabricatorUserEmail())
180+
->loadOneWhere('address = %s', $oauth_email);
181181
if ($known_email) {
182182
$dialog = new AphrontDialogView();
183183
$dialog->setUser($current_user);

‎src/applications/auth/controller/oauth/__init__.php

+1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
phutil_require_module('phabricator', 'applications/auth/controller/base');
1414
phutil_require_module('phabricator', 'applications/auth/oauth/provider/base');
1515
phutil_require_module('phabricator', 'applications/auth/view/oauthfailure');
16+
phutil_require_module('phabricator', 'applications/people/storage/email');
1617
phutil_require_module('phabricator', 'applications/people/storage/user');
1718
phutil_require_module('phabricator', 'applications/people/storage/useroauthinfo');
1819
phutil_require_module('phabricator', 'infrastructure/env');

0 commit comments

Comments
 (0)
Failed to load comments.