Skip to content

Commit c0e1a63

Browse files
author
epriestley
committed
Implement an approval queue
Summary: - Add an option for the queue. - By default, enable it. - Dump new users into the queue. - Send admins an email to approve them. Test Plan: - Registered new accounts with queue on and off. - As an admin, approved accounts and disabled the queue from email. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D7576
1 parent 0fa4110 commit c0e1a63

File tree

8 files changed

+101
-16
lines changed

8 files changed

+101
-16
lines changed

scripts/user/account_admin.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@
181181
$editor->createNewUser($user, $email);
182182
} else {
183183
if ($verify_email) {
184+
$user->setIsEmailVerified(1);
184185
$verify_email->setIsVerified($set_verified ? 1 : 0);
185186
}
186187
$editor->updateUser($user, $verify_email);

scripts/user/add_user.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
$user = new PhabricatorUser();
4343
$user->setUsername($username);
4444
$user->setRealname($realname);
45+
$user->setIsApproved(1);
4546

4647
$email_object = id(new PhabricatorUserEmail())
4748
->setAddress($email)

src/applications/auth/controller/PhabricatorAuthRegisterController.php

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,19 @@ public function processRequest() {
232232
$user->setUsername($value_username);
233233
$user->setRealname($value_realname);
234234

235+
if ($is_setup) {
236+
$must_approve = false;
237+
} else {
238+
$must_approve = PhabricatorEnv::getEnvConfig(
239+
'auth.require-approval');
240+
}
241+
242+
if ($must_approve) {
243+
$user->setIsApproved(0);
244+
} else {
245+
$user->setIsApproved(1);
246+
}
247+
235248
$user->openTransaction();
236249

237250
$editor = id(new PhabricatorUserEditor())
@@ -257,6 +270,10 @@ public function processRequest() {
257270
$email_obj->sendVerificationEmail($user);
258271
}
259272

273+
if ($must_approve) {
274+
$this->sendWaitingForApprovalEmail($user);
275+
}
276+
260277
return $this->loginUser($user);
261278
} catch (AphrontQueryDuplicateKeyException $exception) {
262279
$same_username = id(new PhabricatorUser())->loadOneWhere(
@@ -506,4 +523,43 @@ protected function renderError($message) {
506523
array($message));
507524
}
508525

526+
private function sendWaitingForApprovalEmail(PhabricatorUser $user) {
527+
$title = '[Phabricator] '.pht(
528+
'New User "%s" Awaiting Approval',
529+
$user->getUsername());
530+
531+
$body = new PhabricatorMetaMTAMailBody();
532+
533+
$body->addRawSection(
534+
pht(
535+
'Newly registered user "%s" is awaiting account approval by an '.
536+
'administrator.',
537+
$user->getUsername()));
538+
539+
$body->addTextSection(
540+
pht('APPROVAL QUEUE'),
541+
PhabricatorEnv::getProductionURI(
542+
'/people/query/approval/'));
543+
544+
$body->addTextSection(
545+
pht('DISABLE APPROVAL QUEUE'),
546+
PhabricatorEnv::getProductionURI(
547+
'/config/edit/auth.require-approval/'));
548+
549+
$admins = id(new PhabricatorPeopleQuery())
550+
->setViewer(PhabricatorUser::getOmnipotentUser())
551+
->withIsAdmin(true)
552+
->execute();
553+
554+
if (!$admins) {
555+
return;
556+
}
557+
558+
$mail = id(new PhabricatorMetaMTAMail())
559+
->addTos(mpull($admins, 'getPHID'))
560+
->setSubject($title)
561+
->setBody($body->render())
562+
->saveAndSend();
563+
}
564+
509565
}

src/applications/config/controller/PhabricatorConfigEditController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -490,7 +490,7 @@ private function renderExamples(PhabricatorConfigOption $option) {
490490

491491
$table[] = phutil_tag('tr', array(), array(
492492
phutil_tag('th', array(), $description),
493-
phutil_tag('th', array(), $value),
493+
phutil_tag('td', array(), $value),
494494
));
495495
}
496496

src/applications/config/option/PhabricatorAuthenticationConfigOptions.php

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@ public function getOptions() {
2121
"Maximum number of simultaneous web sessions each user is ".
2222
"permitted to have. Setting this to '1' will prevent a user from ".
2323
"logging in on more than one browser at the same time.")),
24-
$this->newOption('auth.sessions.conduit', 'int', 5)
24+
$this->newOption('auth.sessions.conduit', 'int', 5)
2525
->setSummary(
2626
pht(
2727
"Number of simultaneous Conduit sessions each user is permitted."))
2828
->setDescription(
2929
pht(
3030
"Maximum number of simultaneous Conduit sessions each user is ".
3131
"permitted to have.")),
32-
$this->newOption('auth.require-email-verification', 'bool', false)
32+
$this->newOption('auth.require-email-verification', 'bool', false)
3333
->setBoolOptions(
3434
array(
3535
pht("Require email verification"),
@@ -41,32 +41,55 @@ public function getOptions() {
4141
pht(
4242
"If true, email addresses must be verified (by clicking a link ".
4343
"in an email) before a user can login. By default, verification ".
44-
"is optional unless 'auth.email-domains' is nonempty.")),
45-
$this->newOption('auth.email-domains', 'list<string>', array())
44+
"is optional unless {{auth.email-domains}} is nonempty.")),
45+
$this->newOption('auth.require-approval', 'bool', true)
46+
->setBoolOptions(
47+
array(
48+
pht("Require Administrators to Approve Accounts"),
49+
pht("Don't Require Manual Approval"),
50+
))
51+
->setSummary(
52+
pht("Require administrators to approve new accounts."))
53+
->setDescription(
54+
pht(
55+
"Newly registered Phabricator accounts can either be placed ".
56+
"into a manual approval queue for administrative review, or ".
57+
"automatically activated immediately. The approval queue is ".
58+
"enabled by default because it gives you greater control over ".
59+
"who can register an account and access Phabricator.\n\n".
60+
"If your install is completely public, or on a VPN, or users can ".
61+
"only register with a trusted provider like LDAP, or you've ".
62+
"otherwise configured Phabricator to prevent unauthorized ".
63+
"registration, you can disable the queue to reduce administrative ".
64+
"overhead.\n\n".
65+
"NOTE: Before you disable the queue, make sure ".
66+
"{{auth.email-domains}} is configured correctly for your ".
67+
"install!")),
68+
$this->newOption('auth.email-domains', 'list<string>', array())
4669
->setSummary(pht("Only allow registration from particular domains."))
4770
->setDescription(
4871
pht(
4972
"You can restrict allowed email addresses to certain domains ".
50-
"(like 'yourcompany.com') by setting a list of allowed domains ".
51-
"here. Users will only be allowed to register using email ".
73+
"(like `yourcompany.com`) by setting a list of allowed domains ".
74+
"here.\n\nUsers will only be allowed to register using email ".
5275
"addresses at one of the domains, and will only be able to add ".
5376
"new email addresses for these domains. If you configure this, ".
54-
"it implies 'auth.require-email-verification'.\n\n".
55-
"You should omit the '@' from domains. Note that the domain must ".
56-
"match exactly. If you allow 'yourcompany.com', that permits ".
57-
"'joe@yourcompany.com' but rejects 'joe@mail.yourcompany.com'."))
77+
"it implies {{auth.require-email-verification}}.\n\n".
78+
"You should omit the `@` from domains. Note that the domain must ".
79+
"match exactly. If you allow `yourcompany.com`, that permits ".
80+
"`joe@yourcompany.com` but rejects `joe@mail.yourcompany.com`."))
5881
->addExample(
5982
"yourcompany.com\nmail.yourcompany.com",
6083
pht('Valid Setting')),
61-
$this->newOption('auth.login-message', 'string', null)
84+
$this->newOption('auth.login-message', 'string', null)
6285
->setLocked(true)
6386
->setSummary(pht("A block of HTML displayed on the login screen."))
6487
->setDescription(
6588
pht(
6689
"You can provide an arbitrary block of HTML here, which will ".
6790
"appear on the login screen. Normally, you'd use this to provide ".
6891
"login or registration instructions to users.")),
69-
$this->newOption('account.editable', 'bool', true)
92+
$this->newOption('account.editable', 'bool', true)
7093
->setBoolOptions(
7194
array(
7295
pht("Allow editing"),
@@ -83,7 +106,7 @@ public function getOptions() {
83106
"synchronize account information from some other authoritative ".
84107
"system, you can disable this to ensure information remains ".
85108
"consistent across both systems.")),
86-
$this->newOption('account.minimum-password-length', 'int', 8)
109+
$this->newOption('account.minimum-password-length', 'int', 8)
87110
->setSummary(pht("Minimum password length."))
88111
->setDescription(
89112
pht(

src/applications/people/controller/PhabricatorPeopleEditController.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,9 @@ private function processBasicRequest(PhabricatorUser $user) {
182182
->setAddress($new_email)
183183
->setIsVerified(0);
184184

185+
// Automatically approve the user, since an admin is creating them.
186+
$user->setIsApproved(1);
187+
185188
id(new PhabricatorUserEditor())
186189
->setActor($admin)
187190
->createNewUser($user, $email);

src/applications/people/storage/PhabricatorUser.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ final class PhabricatorUser
3030
protected $isAdmin = 0;
3131
protected $isDisabled = 0;
3232
protected $isEmailVerified = 0;
33-
protected $isApproved = 1;
33+
protected $isApproved = 0;
3434

3535
private $profileImage = null;
3636
private $profile = null;

src/infrastructure/testing/PhabricatorTestCase.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,8 @@ protected function generateNewTestUser() {
181181

182182
$user = id(new PhabricatorUser())
183183
->setRealName("Test User {$seed}}")
184-
->setUserName("test{$seed}");
184+
->setUserName("test{$seed}")
185+
->setIsApproved(1);
185186

186187
$email = id(new PhabricatorUserEmail())
187188
->setAddress("testuser{$seed}@example.com")

0 commit comments

Comments
 (0)