From db1e12370629a18ec9ee13a5af3edd311f8a7f65 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 3 Feb 2019 06:36:49 -0800
Subject: [PATCH 01/34] Fix an issue where Duo validation could incorrectly
 apply to other factor types

See <https://discourse.phabricator-community.org/t/configuring-mfa-provider-totp-fails-for-missing-duo-only-options/2355>.

Test Plan: Created a TOTP provider; created a Duo provider (with missing and supplied values).
---
 ...catorAuthFactorProviderDuoCredentialTransaction.php |  4 ++++
 ...ricatorAuthFactorProviderDuoHostnameTransaction.php |  4 ++++
 .../PhabricatorAuthFactorProviderTransactionType.php   | 10 +++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoCredentialTransaction.php b/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoCredentialTransaction.php
index 532fc271f4..f5a52cb90f 100644
--- a/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoCredentialTransaction.php
+++ b/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoCredentialTransaction.php
@@ -27,6 +27,10 @@ final class PhabricatorAuthFactorProviderDuoCredentialTransaction
     $actor = $this->getActor();
     $errors = array();
 
+    if (!$this->isDuoProvider($object)) {
+      return $errors;
+    }
+
     $old_value = $this->generateOldValue($object);
     if ($this->isEmptyTextTransaction($old_value, $xactions)) {
       $errors[] = $this->newRequiredError(
diff --git a/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoHostnameTransaction.php b/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoHostnameTransaction.php
index ce1838594e..27ae271137 100644
--- a/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoHostnameTransaction.php
+++ b/src/applications/auth/xaction/PhabricatorAuthFactorProviderDuoHostnameTransaction.php
@@ -26,6 +26,10 @@ final class PhabricatorAuthFactorProviderDuoHostnameTransaction
   public function validateTransactions($object, array $xactions) {
     $errors = array();
 
+    if (!$this->isDuoProvider($object)) {
+      return $errors;
+    }
+
     $old_value = $this->generateOldValue($object);
     if ($this->isEmptyTextTransaction($old_value, $xactions)) {
       $errors[] = $this->newRequiredError(
diff --git a/src/applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php b/src/applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php
index fe17eee545..3f16249e0c 100644
--- a/src/applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php
+++ b/src/applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php
@@ -1,4 +1,12 @@
 <?php
 
 abstract class PhabricatorAuthFactorProviderTransactionType
-  extends PhabricatorModularTransactionType {}
+  extends PhabricatorModularTransactionType {
+
+  final protected function isDuoProvider(
+    PhabricatorAuthFactorProvider $provider) {
+    $duo_key = id(new PhabricatorDuoAuthFactor())->getFactorKey();
+    return ($provider->getProviderFactorKey() === $duo_key);
+  }
+
+}

From b8fe991ba28eb883a64b1309e3e02c6158fcef32 Mon Sep 17 00:00:00 2001
From: Leopold Schabel <mail@leoschabel.de>
Date: Tue, 5 Feb 2019 13:47:24 +0000
Subject: [PATCH 02/34] Improve description text in the "Create Diff" form

Summary: The textarea is, in fact, above the description!

Test Plan: Description text changed.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D20092
---
 .../controller/DifferentialDiffCreateController.php             | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php
index 36f0b86202..284edf49d3 100644
--- a/src/applications/differential/controller/DifferentialDiffCreateController.php
+++ b/src/applications/differential/controller/DifferentialDiffCreateController.php
@@ -112,7 +112,7 @@ final class DifferentialDiffCreateController extends DifferentialController {
               $arcanist_link,
             ),
             pht(
-              'You can also paste a diff below, or upload a file '.
+              'You can also paste a diff above, or upload a file '.
               'containing a diff (for example, from %s, %s or %s).',
               phutil_tag('tt', array(), 'svn diff'),
               phutil_tag('tt', array(), 'git diff'),

From 4675306615da8848e0f196820af7bcc6f9fa6118 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Feb 2019 16:52:58 -0800
Subject: [PATCH 03/34] Add a "metronome" for spreading service call load

Summary:
Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.

For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.

For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20087
---
 src/__phutil_library_map__.php                |  4 +
 .../util/PhabricatorMetronome.php             | 92 +++++++++++++++++++
 .../PhabricatorMetronomeTestCase.php          | 61 ++++++++++++
 3 files changed, 157 insertions(+)
 create mode 100644 src/infrastructure/util/PhabricatorMetronome.php
 create mode 100644 src/infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 324fc3c5de..fe77880417 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3552,6 +3552,8 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTASchemaSpec' => 'applications/metamta/storage/PhabricatorMetaMTASchemaSpec.php',
     'PhabricatorMetaMTASendGridReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php',
     'PhabricatorMetaMTAWorker' => 'applications/metamta/PhabricatorMetaMTAWorker.php',
+    'PhabricatorMetronome' => 'infrastructure/util/PhabricatorMetronome.php',
+    'PhabricatorMetronomeTestCase' => 'infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php',
     'PhabricatorMetronomicTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorMetronomicTriggerClock.php',
     'PhabricatorModularTransaction' => 'applications/transactions/storage/PhabricatorModularTransaction.php',
     'PhabricatorModularTransactionType' => 'applications/transactions/storage/PhabricatorModularTransactionType.php',
@@ -9477,6 +9479,8 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTASchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'PhabricatorMetaMTASendGridReceiveController' => 'PhabricatorMetaMTAController',
     'PhabricatorMetaMTAWorker' => 'PhabricatorWorker',
+    'PhabricatorMetronome' => 'Phobject',
+    'PhabricatorMetronomeTestCase' => 'PhabricatorTestCase',
     'PhabricatorMetronomicTriggerClock' => 'PhabricatorTriggerClock',
     'PhabricatorModularTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorModularTransactionType' => 'Phobject',
diff --git a/src/infrastructure/util/PhabricatorMetronome.php b/src/infrastructure/util/PhabricatorMetronome.php
new file mode 100644
index 0000000000..24f58127f6
--- /dev/null
+++ b/src/infrastructure/util/PhabricatorMetronome.php
@@ -0,0 +1,92 @@
+<?php
+
+/**
+ * Tick at a given frequency with a specifiable offset.
+ *
+ * One use case for this is to flatten out load spikes caused by periodic
+ * service calls. Give each host a metronome that ticks at the same frequency,
+ * but with different offsets. Then, have hosts make service calls only after
+ * their metronome ticks. This spreads service calls out evenly more quickly
+ * and more predictably than adding random jitter.
+ */
+final class PhabricatorMetronome
+  extends Phobject {
+
+  private $offset = 0;
+  private $frequency;
+
+  public function setOffset($offset) {
+    if (!is_int($offset)) {
+      throw new Exception(pht('Metronome offset must be an integer.'));
+    }
+
+    if ($offset < 0) {
+      throw new Exception(pht('Metronome offset must be 0 or more.'));
+    }
+
+    // We're not requiring that the offset be smaller than the frequency. If
+    // the offset is larger, we'll just clamp it to the frequency before we
+    // use it. This allows the offset to be configured before the frequency
+    // is configured, which is useful for using a hostname as an offset seed.
+
+    $this->offset = $offset;
+
+    return $this;
+  }
+
+  public function setFrequency($frequency) {
+    if (!is_int($frequency)) {
+      throw new Exception(pht('Metronome frequency must be an integer.'));
+    }
+
+    if ($frequency < 1) {
+      throw new Exception(pht('Metronome frequency must be 1 or more.'));
+    }
+
+    $this->frequency = $frequency;
+
+    return $this;
+  }
+
+  public function setOffsetFromSeed($seed) {
+    $offset = PhabricatorHash::digestToRange($seed, 0, PHP_INT_MAX);
+    return $this->setOffset($offset);
+  }
+
+  public function getFrequency() {
+    if ($this->frequency === null) {
+      throw new PhutilInvalidStateException('setFrequency');
+    }
+    return $this->frequency;
+  }
+
+  public function getOffset() {
+    $frequency = $this->getFrequency();
+    return ($this->offset % $frequency);
+  }
+
+  public function getNextTickAfter($epoch) {
+    $frequency = $this->getFrequency();
+    $offset = $this->getOffset();
+
+    $remainder = ($epoch % $frequency);
+
+    if ($remainder < $offset) {
+      return ($epoch - $remainder) + $offset;
+    } else {
+      return ($epoch - $remainder) + $frequency + $offset;
+    }
+  }
+
+  public function didTickBetween($min, $max) {
+    if ($max < $min) {
+      throw new Exception(
+        pht(
+          'Maximum tick window must not be smaller than minimum tick window.'));
+    }
+
+    $next = $this->getNextTickAfter($min);
+    return ($next <= $max);
+  }
+
+}
diff --git a/src/infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php b/src/infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php
new file mode 100644
index 0000000000..9ad74e2b90
--- /dev/null
+++ b/src/infrastructure/util/__tests__/PhabricatorMetronomeTestCase.php
@@ -0,0 +1,61 @@
+<?php
+
+final class PhabricatorMetronomeTestCase
+  extends PhabricatorTestCase {
+
+  public function testMetronomeOffsets() {
+    $cases = array(
+      'web001.example.net' => 44,
+      'web002.example.net' => 36,
+      'web003.example.net' => 25,
+      'web004.example.net' => 25,
+      'web005.example.net' => 16,
+      'web006.example.net' => 26,
+      'web007.example.net' => 35,
+      'web008.example.net' => 14,
+    );
+
+    $metronome = id(new PhabricatorMetronome())
+      ->setFrequency(60);
+
+    foreach ($cases as $input => $expect) {
+      $metronome->setOffsetFromSeed($input);
+
+      $this->assertEqual(
+        $expect,
+        $metronome->getOffset(),
+        pht('Offset for: %s', $input));
+    }
+  }
+
+  public function testMetronomeTicks() {
+    $metronome = id(new PhabricatorMetronome())
+      ->setFrequency(60)
+      ->setOffset(13);
+
+    $tick_epoch = strtotime('2000-01-01 11:11:13 AM UTC');
+
+    // Since the epoch is at "0:13" on the clock, the metronome should tick
+    // then.
+    $this->assertEqual(
+      $tick_epoch,
+      $metronome->getNextTickAfter($tick_epoch - 1),
+      pht('Tick at 11:11:13 AM.'));
+
+    // The next tick should be a minute later.
+    $this->assertEqual(
+      $tick_epoch + 60,
+      $metronome->getNextTickAfter($tick_epoch),
+      pht('Tick at 11:12:13 AM.'));
+
+
+    // There's no tick in the next 59 seconds.
+    $this->assertFalse(
+      $metronome->didTickBetween($tick_epoch, $tick_epoch + 59));
+
+    $this->assertTrue(
+      $metronome->didTickBetween($tick_epoch, $tick_epoch + 60));
+  }
+
+
+}

From ee24eb60b765c1cf831a97aabff69afc9ca921be Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Feb 2019 19:33:30 -0800
Subject: [PATCH 04/34] In Owners Packages, make the API representation of the
 "Auditing" field more consistent

Summary:
Ref T13244. See PHI1047. A while ago, the "Review" field changed from "yes/no" to 20 flavors of "Non-Owner Blocking Under A Full Moon". The sky didn't fall, so we'll probably do this to "Audit" eventually too.

The "owners.search" API method anticipates this and returns "none" or "audit" to describe package audit statuses, so it can begin returning "audit-non-owner-reviewers" or whatever in the future.

However, the "owners.edit" API method doesn't work the same way, and takes strings, and the strings have to be numbers. This is goofy and confusing and generally bad.

Make "owners.edit" take the same strings that "owners.search" emits. For now, continue accepting the old values of "0" and "1".

Test Plan:
  - Edited audit status of packages via API using "none", "audit", "0", "1" (worked), and invalid values like "quack" (helpful error).
  - Edited audit status of packages via web UI.
  - Used `owners.search` to retrieve package information.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20091
---
 .../PhabricatorOwnersPackageEditEngine.php    |  6 +--
 .../storage/PhabricatorOwnersPackage.php      | 14 ++++-
 ...icatorOwnersPackageAuditingTransaction.php | 53 ++++++++++++++++++-
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
index 044cb8beda..c4ee026374 100644
--- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
@@ -140,11 +140,11 @@ EOTEXT
         ->setTransactionType(
           PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
-        ->setValue($object->getAuditingEnabled())
+        ->setValue($object->getAuditingState())
         ->setOptions(
           array(
-            '' => pht('Disabled'),
-            '1' => pht('Enabled'),
+            PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'),
+            PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'),
           )),
       id(new PhabricatorRemarkupEditField())
         ->setKey('description')
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 564fc8a28b..207e0cb809 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -38,6 +38,9 @@ final class PhabricatorOwnersPackage
   const AUTOREVIEW_BLOCK = 'block';
   const AUTOREVIEW_BLOCK_ALWAYS = 'block-always';
 
+  const AUDITING_NONE = 'none';
+  const AUDITING_AUDIT = 'audit';
+
   const DOMINION_STRONG = 'strong';
   const DOMINION_WEAK = 'weak';
 
@@ -564,6 +567,14 @@ final class PhabricatorOwnersPackage
     return '/owners/package/'.$this->getID().'/';
   }
 
+  public function getAuditingState() {
+    if ($this->getAuditingEnabled()) {
+      return self::AUDITING_AUDIT;
+    } else {
+      return self::AUDITING_NONE;
+    }
+  }
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 
@@ -720,11 +731,10 @@ final class PhabricatorOwnersPackage
       'label' => $review_label,
     );
 
+    $audit_value = $this->getAuditingState();
     if ($this->getAuditingEnabled()) {
-      $audit_value = 'audit';
       $audit_label = pht('Auditing Enabled');
     } else {
-      $audit_value = 'none';
       $audit_label = pht('No Auditing');
     }
 
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
index df4f0feb01..d7ea7093f9 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
@@ -10,7 +10,15 @@ final class PhabricatorOwnersPackageAuditingTransaction
   }
 
   public function generateNewValue($object, $value) {
-    return (int)$value;
+    switch ($value) {
+      case PhabricatorOwnersPackage::AUDITING_AUDIT:
+        return 1;
+      case '1':
+        // TODO: Remove, deprecated.
+        return 1;
+      default:
+        return 0;
+    }
   }
 
   public function applyInternalEffects($object, $value) {
@@ -29,4 +37,47 @@ final class PhabricatorOwnersPackageAuditingTransaction
     }
   }
 
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    // See PHI1047. This transaction type accepted some weird stuff. Continue
+    // supporting it for now, but move toward sensible consistency.
+
+    $modern_options = array(
+      PhabricatorOwnersPackage::AUDITING_NONE =>
+        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_NONE),
+      PhabricatorOwnersPackage::AUDITING_AUDIT =>
+        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_AUDIT),
+    );
+
+    $deprecated_options = array(
+      '0' => '"0"',
+      '1' => '"1"',
+      '' => pht('"" (empty string)'),
+    );
+
+    foreach ($xactions as $xaction) {
+      $new_value = $xaction->getNewValue();
+
+      if (isset($modern_options[$new_value])) {
+        continue;
+      }
+
+      if (isset($deprecated_options[$new_value])) {
+        continue;
+      }
+
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Package auditing value "%s" is not supported. Supported options '.
+          'are: %s. Deprecated options are: %s.',
+          $new_value,
+          implode(', ', $modern_options),
+          implode(', ', $deprecated_options)),
+        $xaction);
+    }
+
+    return $errors;
+  }
+
 }

From a58c9d50b2e1f9cbeabb979cbc4372d0c1981e68 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Feb 2019 16:13:04 -0800
Subject: [PATCH 05/34] Slightly update the Diviner documentation

Summary: See PHI1050. Although Diviner hasn't received a ton of new development for a while, it's: not exaclty new; and pretty useful for what we need it for.

Test Plan: Reading.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: leoluk

Differential Revision: https://secure.phabricator.com/D20086
---
 src/docs/user/userguide/diviner.diviner | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/docs/user/userguide/diviner.diviner b/src/docs/user/userguide/diviner.diviner
index e94c33d275..01484be14c 100644
--- a/src/docs/user/userguide/diviner.diviner
+++ b/src/docs/user/userguide/diviner.diviner
@@ -3,17 +3,28 @@
 
 Using Diviner, a documentation generator.
 
-= Overview =
+Overview
+========
 
-NOTE: Diviner is new and not yet generally useful.
+Diviner is an application for creating technical documentation.
 
-= Generating Documentation =
+This article is maintained in a text file in the Phabricator repository and
+generated into the display document you are currently reading using Diviner.
+
+Beyond generating articles, Diviner can also analyze source code and generate
+documentation about classes, methods, and other primitives.
+
+
+Generating Documentation
+========================
 
 To generate documentation, run:
 
   phabricator/ $ ./bin/diviner generate --book <book>
 
-= .book Files =
+
+Diviner ".book" Files
+=====================
 
 Diviner documentation books are configured using JSON `.book` files, which
 look like this:

From 5dc650229350ed5c6df44d351e80bb24fc547f84 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 05:11:01 -0800
Subject: [PATCH 06/34] Make the mobile menu available in "/mail/"

Summary: Ref T13244. See <https://discourse.phabricator-community.org/t/left-hand-menu-not-responsive-on-mobile/2358>.

Test Plan: {F6184160}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20093
---
 .../controller/PhabricatorMetaMTAMailListController.php       | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailListController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailListController.php
index 0651068550..01d6f1e218 100644
--- a/src/applications/metamta/controller/PhabricatorMetaMTAMailListController.php
+++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailListController.php
@@ -27,4 +27,8 @@ final class PhabricatorMetaMTAMailListController
     return $nav;
   }
 
+  public function buildApplicationMenu() {
+    return $this->buildSideNav()->getMenu();
+  }
+
 }

From ab467d52f4dd7be9b137cab533cec7b155547fe9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 07:16:56 -0800
Subject: [PATCH 07/34] Improve feed rendering of user rename story

Summary:
Ref T13244. This story publishes to the feed (and I think that's reasonable and desirable) but doesn't render as nicely as it could.

Improve the rendering.

(See T9233 for some context on why we render stories like this one in this way.)

Test Plan: {F6184490}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20097
---
 .../xaction/PhabricatorUserUsernameTransaction.php       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
index b6d23b3511..f2226d0010 100644
--- a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
+++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
@@ -40,6 +40,15 @@ final class PhabricatorUserUsernameTransaction
       $this->renderNewValue());
   }
 
+  public function getTitleForFeed() {
+    return pht(
+      '%s renamed %s from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderObject(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
   public function validateTransactions($object, array $xactions) {
     $actor = $this->getActor();
     $errors = array();

From 03eb989fd875215d5ac8c47ed8c171f3bdb86163 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Feb 2019 16:52:58 -0800
Subject: [PATCH 08/34] Give Duo MFA a stronger hint if users continue without
 answering the challenge

Summary: See PHI912. Also, clean up some leftover copy/pastey code here.

Test Plan: {F6182333}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20088
---
 .../auth/factor/PhabricatorAuthFactor.php     | 29 +++++++---
 .../factor/PhabricatorAuthFactorResult.php    | 10 ++++
 .../auth/factor/PhabricatorDuoAuthFactor.php  | 56 ++++++-------------
 .../auth/storage/PhabricatorAuthChallenge.php | 10 ++++
 4 files changed, 59 insertions(+), 46 deletions(-)

diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php
index ec49f7f748..912a2c31c9 100644
--- a/src/applications/auth/factor/PhabricatorAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactor.php
@@ -123,6 +123,7 @@ abstract class PhabricatorAuthFactor extends Phobject {
       ->setUserPHID($viewer->getPHID())
       ->setSessionPHID($viewer->getSession()->getPHID())
       ->setFactorPHID($config->getPHID())
+      ->setIsNewChallenge(true)
       ->setWorkflowKey($engine->getWorkflowKey());
   }
 
@@ -283,8 +284,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-clock-o', 'red');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-clock-o', 'red');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -295,8 +299,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
   private function newAnsweredControl(
     PhabricatorAuthFactorResult $result) {
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-check-circle-o', 'green');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-check-circle-o', 'green');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -309,8 +316,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-times', 'red');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-times', 'red');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
@@ -323,8 +333,11 @@ abstract class PhabricatorAuthFactor extends Phobject {
 
     $error = $result->getErrorMessage();
 
-    $icon = id(new PHUIIconView())
-      ->setIcon('fa-commenting', 'green');
+    $icon = $result->getIcon();
+    if (!$icon) {
+      $icon = id(new PHUIIconView())
+        ->setIcon('fa-commenting', 'green');
+    }
 
     return id(new PHUIFormTimerControl())
       ->setIcon($icon)
diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php
index 2282f162a9..f03c3674da 100644
--- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php
@@ -10,6 +10,7 @@ final class PhabricatorAuthFactorResult
   private $errorMessage;
   private $value;
   private $issuedChallenges = array();
+  private $icon;
 
   public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) {
     if (!$challenge->getIsAnsweredChallenge()) {
@@ -92,4 +93,13 @@ final class PhabricatorAuthFactorResult
     return $this->issuedChallenges;
   }
 
+  public function setIcon(PHUIIconView $icon) {
+    $this->icon = $icon;
+    return $this;
+  }
+
+  public function getIcon() {
+    return $this->icon;
+  }
+
 }
diff --git a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
index 187e011953..4be4c15ea8 100644
--- a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
@@ -612,7 +612,22 @@ final class PhabricatorDuoAuthFactor
         return $this->newResult()
           ->setAnsweredChallenge($challenge);
       case 'waiting':
-        // No result yet, we'll render a default state later on.
+        // If we didn't just issue this challenge, give the user a stronger
+        // hint that they need to follow the instructions.
+        if (!$challenge->getIsNewChallenge()) {
+          return $this->newResult()
+            ->setIsContinue(true)
+            ->setIcon(
+              id(new PHUIIconView())
+              ->setIcon('fa-exclamation-triangle', 'yellow'))
+            ->setErrorMessage(
+              pht(
+                'You must approve the challenge which was sent to your '.
+                'phone. Open the Duo application and confirm the challenge, '.
+                'then continue.'));
+        }
+
+        // Otherwise, we'll construct a default message later on.
         break;
       default:
       case 'deny':
@@ -666,8 +681,7 @@ final class PhabricatorDuoAuthFactor
   public function getRequestHasChallengeResponse(
     PhabricatorAuthFactorConfig $config,
     AphrontRequest $request) {
-    $value = $this->getChallengeResponseFromRequest($config, $request);
-    return (bool)strlen($value);
+    return false;
   }
 
   protected function newResultFromChallengeResponse(
@@ -675,41 +689,7 @@ final class PhabricatorDuoAuthFactor
     PhabricatorUser $viewer,
     AphrontRequest $request,
     array $challenges) {
-
-    $challenge = $this->getChallengeForCurrentContext(
-      $config,
-      $viewer,
-      $challenges);
-
-    $code = $this->getChallengeResponseFromRequest(
-      $config,
-      $request);
-
-    $result = $this->newResult()
-      ->setValue($code);
-
-    if ($challenge->getIsAnsweredChallenge()) {
-      return $result->setAnsweredChallenge($challenge);
-    }
-
-    if (phutil_hashes_are_identical($code, $challenge->getChallengeKey())) {
-      $ttl = PhabricatorTime::getNow() + phutil_units('15 minutes in seconds');
-
-      $challenge
-        ->markChallengeAsAnswered($ttl);
-
-      return $result->setAnsweredChallenge($challenge);
-    }
-
-    if (strlen($code)) {
-      $error_message = pht('Invalid');
-    } else {
-      $error_message = pht('Required');
-    }
-
-    $result->setErrorMessage($error_message);
-
-    return $result;
+    return $this->newResult();
   }
 
   private function newDuoFuture(PhabricatorAuthFactorProvider $provider) {
diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php
index 8fa07d712f..0b740e5fa7 100644
--- a/src/applications/auth/storage/PhabricatorAuthChallenge.php
+++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php
@@ -16,6 +16,7 @@ final class PhabricatorAuthChallenge
   protected $properties = array();
 
   private $responseToken;
+  private $isNewChallenge;
 
   const HTTPKEY = '__hisec.challenges__';
   const TOKEN_DIGEST_KEY = 'auth.challenge.token';
@@ -241,6 +242,15 @@ final class PhabricatorAuthChallenge
     return $this->properties[$key];
   }
 
+  public function setIsNewChallenge($is_new_challenge) {
+    $this->isNewChallenge = $is_new_challenge;
+    return $this;
+  }
+
+  public function getIsNewChallenge() {
+    return $this->isNewChallenge;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

From 6f3bd13cf5da0f62b9b7df8304105c82bb9bd2ee Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 05:22:39 -0800
Subject: [PATCH 09/34] Begin adding more guidance to the "One-Time Login" flow

Summary:
Ref T13244. See PHI774. If an install does not use password auth, the "one-time login" flow (via "Welcome" email or "bin/auth recover") is pretty rough. Current behavior:

  - If an install uses passwords, the user is prompted to set a password.
  - If an install does not use passwords, you're dumped to `/settings/external/` to link an external account. This is pretty sketchy and this UI does not make it clear what users are expected to do (link an account) or why (so they can log in).

Instead, improve this flow:

  - Password reset flow is fine.
  - (Future Change) If there are external linkable accounts (like Google) and the user doesn't have any linked, I want to give users a flow like a password reset flow that says "link to an external account".
  - (This Change) If you're an administrator and there are no providers at all, go to "/auth/" so you can set something up.
  - (This Change) If we don't hit on any other rules, just go home?

This may be tweaked a bit as we go, but basically I want to refine the "/settings/external/" case into a more useful flow which gives users more of a chance of surviving it.

Test Plan: Logged in with passwords enabled (got password reset), with nothing enabled as an admin (got sent to Auth), and with something other than passwords enabled (got sent home).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20094
---
 .../PhabricatorAuthOneTimeLoginController.php | 86 ++++++++++++-------
 .../PhabricatorAuthProviderConfigQuery.php    | 60 ++++---------
 2 files changed, 72 insertions(+), 74 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index 0cac95f53d..f51f379d2b 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -119,38 +119,9 @@ final class PhabricatorAuthOneTimeLoginController
         }
       unset($unguarded);
 
-      $next = '/';
-      if (!PhabricatorPasswordAuthProvider::getPasswordProvider()) {
-        $next = '/settings/panel/external/';
-      } else {
+      $next_uri = $this->getNextStepURI($target_user);
 
-        // We're going to let the user reset their password without knowing
-        // the old one. Generate a one-time token for that.
-        $key = Filesystem::readRandomCharacters(16);
-        $password_type =
-          PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE;
-
-        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-          id(new PhabricatorAuthTemporaryToken())
-            ->setTokenResource($target_user->getPHID())
-            ->setTokenType($password_type)
-            ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-            ->setTokenCode(PhabricatorHash::weakDigest($key))
-            ->save();
-        unset($unguarded);
-
-        $panel_uri = '/auth/password/';
-
-        $next = (string)id(new PhutilURI($panel_uri))
-          ->setQueryParams(
-            array(
-              'key' => $key,
-            ));
-
-        $request->setTemporaryCookie(PhabricatorCookies::COOKIE_HISEC, 'yes');
-      }
-
-      PhabricatorCookies::setNextURICookie($request, $next, $force = true);
+      PhabricatorCookies::setNextURICookie($request, $next_uri, $force = true);
 
       $force_full_session = false;
       if ($link_type === PhabricatorAuthSessionEngine::ONETIME_RECOVER) {
@@ -206,4 +177,57 @@ final class PhabricatorAuthOneTimeLoginController
 
     return id(new AphrontDialogResponse())->setDialog($dialog);
   }
+
+  private function getNextStepURI(PhabricatorUser $user) {
+    $request = $this->getRequest();
+
+    // If we have password auth, let the user set or reset their password after
+    // login.
+    $have_passwords = PhabricatorPasswordAuthProvider::getPasswordProvider();
+    if ($have_passwords) {
+      // We're going to let the user reset their password without knowing
+      // the old one. Generate a one-time token for that.
+      $key = Filesystem::readRandomCharacters(16);
+      $password_type =
+        PhabricatorAuthPasswordResetTemporaryTokenType::TOKENTYPE;
+
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        id(new PhabricatorAuthTemporaryToken())
+          ->setTokenResource($user->getPHID())
+          ->setTokenType($password_type)
+          ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
+          ->setTokenCode(PhabricatorHash::weakDigest($key))
+          ->save();
+      unset($unguarded);
+
+      $panel_uri = '/auth/password/';
+
+      $request->setTemporaryCookie(PhabricatorCookies::COOKIE_HISEC, 'yes');
+
+      return (string)id(new PhutilURI($panel_uri))
+        ->setQueryParams(
+          array(
+            'key' => $key,
+          ));
+    }
+
+    $providers = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($user)
+      ->withIsEnabled(true)
+      ->execute();
+
+    // If there are no configured providers and the user is an administrator,
+    // send them to Auth to configure a provider. This is probably where they
+    // want to go. You can end up in this state by accidentally losing your
+    // first session during initial setup, or after restoring exported data
+    // from a hosted instance.
+    if (!$providers && $user->getIsAdmin()) {
+      return '/auth/';
+    }
+
+    // If we didn't find anywhere better to send them, give up and just send
+    // them to the home page.
+    return '/';
+  }
+
 }
diff --git a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
index 626c80348f..1d21342e4e 100644
--- a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
@@ -6,11 +6,7 @@ final class PhabricatorAuthProviderConfigQuery
   private $ids;
   private $phids;
   private $providerClasses;
-
-  const STATUS_ALL = 'status:all';
-  const STATUS_ENABLED = 'status:enabled';
-
-  private $status = self::STATUS_ALL;
+  private $isEnabled;
 
   public function withPHIDs(array $phids) {
     $this->phids = $phids;
@@ -22,40 +18,26 @@ final class PhabricatorAuthProviderConfigQuery
     return $this;
   }
 
-  public function withStatus($status) {
-    $this->status = $status;
-    return $this;
-  }
-
   public function withProviderClasses(array $classes) {
     $this->providerClasses = $classes;
     return $this;
   }
 
-  public static function getStatusOptions() {
-    return array(
-      self::STATUS_ALL      => pht('All Providers'),
-      self::STATUS_ENABLED  => pht('Enabled Providers'),
-    );
+  public function withIsEnabled($is_enabled) {
+    $this->isEnabled = $is_enabled;
+    return $this;
+  }
+
+  public function newResultObject() {
+    return new PhabricatorAuthProviderConfig();
   }
 
   protected function loadPage() {
-    $table = new PhabricatorAuthProviderConfig();
-    $conn_r = $table->establishConnection('r');
-
-    $data = queryfx_all(
-      $conn_r,
-      'SELECT * FROM %T %Q %Q %Q',
-      $table->getTableName(),
-      $this->buildWhereClause($conn_r),
-      $this->buildOrderClause($conn_r),
-      $this->buildLimitClause($conn_r));
-
-    return $table->loadAllFromArray($data);
+    return $this->loadStandardPage($this->newResultObject());
   }
 
-  protected function buildWhereClause(AphrontDatabaseConnection $conn) {
-    $where = array();
+  protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
+    $where = parent::buildWhereClauseParts($conn);
 
     if ($this->ids !== null) {
       $where[] = qsprintf(
@@ -78,22 +60,14 @@ final class PhabricatorAuthProviderConfigQuery
         $this->providerClasses);
     }
 
-    $status = $this->status;
-    switch ($status) {
-      case self::STATUS_ALL:
-        break;
-      case self::STATUS_ENABLED:
-        $where[] = qsprintf(
-          $conn,
-          'isEnabled = 1');
-        break;
-      default:
-        throw new Exception(pht("Unknown status '%s'!", $status));
+    if ($this->isEnabled !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'isEnabled = %d',
+        (int)$this->isEnabled);
     }
 
-    $where[] = $this->buildPagingClause($conn);
-
-    return $this->formatWhereClause($conn, $where);
+    return $where;
   }
 
   public function getQueryApplicationClass() {

From 8c8d56dc5636df949e7bacf77abfeb9e51be7b65 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 05:50:58 -0800
Subject: [PATCH 10/34] Replace "Add Auth Provider" radio buttons with a more
 modern "click to select" UI

Summary:
Depends on D20094. Ref T13244. Ref T6703. See PHI774. Currently, we use an older-style radio-button UI to choose an auth provider type (Google, Password, LDAP, etc).

Instead, use a more modern click-to-select UI.

Test Plan: {F6184343}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244, T6703

Differential Revision: https://secure.phabricator.com/D20095
---
 .../PhabricatorAuthApplication.php            |   3 +-
 .../config/PhabricatorAuthEditController.php  |   6 +-
 .../config/PhabricatorAuthNewController.php   | 127 +++++++-----------
 .../auth/provider/PhabricatorAuthProvider.php |   6 +
 4 files changed, 56 insertions(+), 86 deletions(-)

diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index df86595b46..15d753a286 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -48,8 +48,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
         '' => 'PhabricatorAuthListController',
         'config/' => array(
           'new/' => 'PhabricatorAuthNewController',
-          'new/(?P<className>[^/]+)/' => 'PhabricatorAuthEditController',
-          'edit/(?P<id>\d+)/' => 'PhabricatorAuthEditController',
+          'edit/(?:(?P<id>\d+)/)?' => 'PhabricatorAuthEditController',
           '(?P<action>enable|disable)/(?P<id>\d+)/'
             => 'PhabricatorAuthDisableController',
         ),
diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php
index 6ff0be4383..016fe51b1a 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php
@@ -6,8 +6,9 @@ final class PhabricatorAuthEditController
   public function handleRequest(AphrontRequest $request) {
     $this->requireApplicationCapability(
       AuthManageProvidersCapability::CAPABILITY);
-    $viewer = $request->getUser();
-    $provider_class = $request->getURIData('className');
+
+    $viewer = $this->getViewer();
+    $provider_class = $request->getStr('provider');
     $config_id = $request->getURIData('id');
 
     if ($config_id) {
@@ -275,6 +276,7 @@ final class PhabricatorAuthEditController
 
     $form = id(new AphrontFormView())
       ->setUser($viewer)
+      ->addHiddenInput('provider', $provider_class)
       ->appendChild(
         id(new AphrontFormCheckboxControl())
           ->setLabel(pht('Allow'))
diff --git a/src/applications/auth/controller/config/PhabricatorAuthNewController.php b/src/applications/auth/controller/config/PhabricatorAuthNewController.php
index dbd43f9ea8..c8fd0ad8a5 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthNewController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthNewController.php
@@ -6,44 +6,12 @@ final class PhabricatorAuthNewController
   public function handleRequest(AphrontRequest $request) {
     $this->requireApplicationCapability(
       AuthManageProvidersCapability::CAPABILITY);
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
+
+    $viewer = $this->getViewer();
+    $cancel_uri = $this->getApplicationURI();
 
     $providers = PhabricatorAuthProvider::getAllBaseProviders();
 
-    $e_provider = null;
-    $errors = array();
-
-    if ($request->isFormPost()) {
-      $provider_string = $request->getStr('provider');
-      if (!strlen($provider_string)) {
-        $e_provider = pht('Required');
-        $errors[] = pht('You must select an authentication provider.');
-      } else {
-        $found = false;
-        foreach ($providers as $provider) {
-          if (get_class($provider) === $provider_string) {
-            $found = true;
-            break;
-          }
-        }
-        if (!$found) {
-          $e_provider = pht('Invalid');
-          $errors[] = pht('You must select a valid provider.');
-        }
-      }
-
-      if (!$errors) {
-        return id(new AphrontRedirectResponse())->setURI(
-          $this->getApplicationURI('/config/new/'.$provider_string.'/'));
-      }
-    }
-
-    $options = id(new AphrontFormRadioButtonControl())
-      ->setLabel(pht('Provider'))
-      ->setName('provider')
-      ->setError($e_provider);
-
     $configured = PhabricatorAuthProvider::getAllProviders();
     $configured_classes = array();
     foreach ($configured as $configured_provider) {
@@ -55,57 +23,52 @@ final class PhabricatorAuthNewController
     $providers = msort($providers, 'getLoginOrder');
     $providers = array_diff_key($providers, $configured_classes) + $providers;
 
-    foreach ($providers as $provider) {
-      if (isset($configured_classes[get_class($provider)])) {
-        $disabled = true;
-        $description = pht('This provider is already configured.');
+    $menu = id(new PHUIObjectItemListView())
+      ->setViewer($viewer)
+      ->setBig(true)
+      ->setFlush(true);
+
+    foreach ($providers as $provider_key => $provider) {
+      $provider_class = get_class($provider);
+
+      $provider_uri = id(new PhutilURI('/config/edit/'))
+        ->setQueryParam('provider', $provider_class);
+      $provider_uri = $this->getApplicationURI($provider_uri);
+
+      $already_exists = isset($configured_classes[get_class($provider)]);
+
+      $item = id(new PHUIObjectItemView())
+        ->setHeader($provider->getNameForCreate())
+        ->setImageIcon($provider->newIconView())
+        ->addAttribute($provider->getDescriptionForCreate());
+
+      if (!$already_exists) {
+        $item
+          ->setHref($provider_uri)
+          ->setClickable(true);
       } else {
-        $disabled = false;
-        $description = $provider->getDescriptionForCreate();
+        $item->setDisabled(true);
       }
-      $options->addButton(
-        get_class($provider),
-        $provider->getNameForCreate(),
-        $description,
-        $disabled ? 'disabled' : null,
-        $disabled);
+
+      if ($already_exists) {
+        $messages = array();
+        $messages[] = pht('You already have a provider of this type.');
+
+        $info = id(new PHUIInfoView())
+          ->setSeverity(PHUIInfoView::SEVERITY_WARNING)
+          ->setErrors($messages);
+
+        $item->appendChild($info);
+      }
+
+      $menu->addItem($item);
     }
 
-    $form = id(new AphrontFormView())
-      ->setUser($viewer)
-      ->appendChild($options)
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->addCancelButton($this->getApplicationURI())
-          ->setValue(pht('Continue')));
-
-    $form_box = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Provider'))
-      ->setFormErrors($errors)
-      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-      ->setForm($form);
-
-    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(pht('Add Provider'));
-    $crumbs->setBorder(true);
-
-    $title = pht('Add Auth Provider');
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title)
-      ->setHeaderIcon('fa-plus-square');
-
-    $view = id(new PHUITwoColumnView())
-      ->setHeader($header)
-      ->setFooter(array(
-        $form_box,
-      ));
-
-    return $this->newPage()
-      ->setTitle($title)
-      ->setCrumbs($crumbs)
-      ->appendChild($view);
-
+    return $this->newDialog()
+      ->setTitle(pht('Add Auth Provider'))
+      ->setWidth(AphrontDialogView::WIDTH_FORM)
+      ->appendChild($menu)
+      ->addCancelButton($cancel_uri);
   }
 
 }
diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index 0525edad54..bcccca5121 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -311,6 +311,12 @@ abstract class PhabricatorAuthProvider extends Phobject {
     return 'Generic';
   }
 
+  public function newIconView() {
+    return id(new PHUIIconView())
+      ->setSpriteSheet(PHUIIconView::SPRITE_LOGIN)
+      ->setSpriteIcon($this->getLoginIcon());
+  }
+
   public function isLoginFormAButton() {
     return false;
   }

From 4fcb38a2a94cdf9a0abb1723c81ea88211ea2595 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 07:04:47 -0800
Subject: [PATCH 11/34] Move the Auth Provider edit flow toward a more modern
 layout

Summary:
Depends on D20095. Ref T13244. Currently, auth providers have a list item view and a single gigantic edit screen complete with a timeline, piles of instructions, supplemental information, etc.

As a step toward making this stuff easier to use and more modern, give them a separate view UI with normal actions, similar to basically every other type of object. Move the timeline and "Disable/Enable" to the view page (from the edit page and the list page, respectively).

Test Plan: Created, edited, and viewed auth providers.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20096
---
 src/__phutil_library_map__.php                |   2 +
 .../PhabricatorAuthApplication.php            |   1 +
 .../PhabricatorAuthDisableController.php      |  19 ++-
 .../config/PhabricatorAuthEditController.php  |  22 +---
 .../config/PhabricatorAuthListController.php  |  50 ++------
 .../PhabricatorAuthProviderViewController.php | 119 ++++++++++++++++++
 .../PhabricatorAuthProviderConfigQuery.php    |  13 ++
 .../storage/PhabricatorAuthProviderConfig.php |  12 ++
 8 files changed, 171 insertions(+), 67 deletions(-)
 create mode 100644 src/applications/auth/controller/config/PhabricatorAuthProviderViewController.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index fe77880417..56af6a4374 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2335,6 +2335,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthProviderConfigTransaction' => 'applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php',
     'PhabricatorAuthProviderConfigTransactionQuery' => 'applications/auth/query/PhabricatorAuthProviderConfigTransactionQuery.php',
     'PhabricatorAuthProviderController' => 'applications/auth/controller/config/PhabricatorAuthProviderController.php',
+    'PhabricatorAuthProviderViewController' => 'applications/auth/controller/config/PhabricatorAuthProviderViewController.php',
     'PhabricatorAuthProvidersGuidanceContext' => 'applications/auth/guidance/PhabricatorAuthProvidersGuidanceContext.php',
     'PhabricatorAuthProvidersGuidanceEngineExtension' => 'applications/auth/guidance/PhabricatorAuthProvidersGuidanceEngineExtension.php',
     'PhabricatorAuthQueryPublicKeysConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php',
@@ -8094,6 +8095,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthProviderConfigTransaction' => 'PhabricatorApplicationTransaction',
     'PhabricatorAuthProviderConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorAuthProviderController' => 'PhabricatorAuthController',
+    'PhabricatorAuthProviderViewController' => 'PhabricatorAuthProviderConfigController',
     'PhabricatorAuthProvidersGuidanceContext' => 'PhabricatorGuidanceContext',
     'PhabricatorAuthProvidersGuidanceEngineExtension' => 'PhabricatorGuidanceEngineExtension',
     'PhabricatorAuthQueryPublicKeysConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod',
diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index 15d753a286..307cab61c8 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -51,6 +51,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
           'edit/(?:(?P<id>\d+)/)?' => 'PhabricatorAuthEditController',
           '(?P<action>enable|disable)/(?P<id>\d+)/'
             => 'PhabricatorAuthDisableController',
+          'view/(?P<id>\d+)/' => 'PhabricatorAuthProviderViewController',
         ),
         'login/(?P<pkey>[^/]+)/(?:(?P<extra>[^/]+)/)?'
           => 'PhabricatorAuthLoginController',
diff --git a/src/applications/auth/controller/config/PhabricatorAuthDisableController.php b/src/applications/auth/controller/config/PhabricatorAuthDisableController.php
index 5863aceca9..252f159ec4 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthDisableController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthDisableController.php
@@ -6,7 +6,8 @@ final class PhabricatorAuthDisableController
   public function handleRequest(AphrontRequest $request) {
     $this->requireApplicationCapability(
       AuthManageProvidersCapability::CAPABILITY);
-    $viewer = $request->getUser();
+
+    $viewer = $this->getViewer();
     $config_id = $request->getURIData('id');
     $action = $request->getURIData('action');
 
@@ -24,6 +25,7 @@ final class PhabricatorAuthDisableController
     }
 
     $is_enable = ($action === 'enable');
+    $done_uri = $config->getURI();
 
     if ($request->isDialogFormPost()) {
       $xactions = array();
@@ -39,8 +41,7 @@ final class PhabricatorAuthDisableController
         ->setContinueOnNoEffect(true)
         ->applyTransactions($config, $xactions);
 
-      return id(new AphrontRedirectResponse())->setURI(
-        $this->getApplicationURI());
+      return id(new AphrontRedirectResponse())->setURI($done_uri);
     }
 
     if ($is_enable) {
@@ -64,8 +65,9 @@ final class PhabricatorAuthDisableController
       // account and pop a warning like "YOU WILL NO LONGER BE ABLE TO LOGIN
       // YOU GOOF, YOU PROBABLY DO NOT MEAN TO DO THIS". None of this is
       // critical and we can wait to see how users manage to shoot themselves
-      // in the feet. Shortly, `bin/auth` will be able to recover from these
-      // types of mistakes.
+      // in the feet.
+
+      // `bin/auth` can recover from these types of mistakes.
 
       $title = pht('Disable Provider?');
       $body = pht(
@@ -77,14 +79,11 @@ final class PhabricatorAuthDisableController
       $button = pht('Disable Provider');
     }
 
-    $dialog = id(new AphrontDialogView())
-      ->setUser($viewer)
+    return $this->newDialog()
       ->setTitle($title)
       ->appendChild($body)
-      ->addCancelButton($this->getApplicationURI())
+      ->addCancelButton($done_uri)
       ->addSubmitButton($button);
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }
diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php
index 016fe51b1a..d3cd2fef98 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php
@@ -156,12 +156,7 @@ final class PhabricatorAuthEditController
           ->setContinueOnNoEffect(true)
           ->applyTransactions($config, $xactions);
 
-        if ($provider->hasSetupStep() && $is_new) {
-          $id = $config->getID();
-          $next_uri = $this->getApplicationURI('config/edit/'.$id.'/');
-        } else {
-          $next_uri = $this->getApplicationURI();
-        }
+        $next_uri = $config->getURI();
 
         return id(new AphrontRedirectResponse())->setURI($next_uri);
       }
@@ -185,7 +180,7 @@ final class PhabricatorAuthEditController
       $crumb = pht('Edit Provider');
       $title = pht('Edit Auth Provider');
       $header_icon = 'fa-pencil';
-      $cancel_uri = $this->getApplicationURI();
+      $cancel_uri = $config->getURI();
     }
 
     $header = id(new PHUIHeaderView())
@@ -348,18 +343,6 @@ final class PhabricatorAuthEditController
     $crumbs->addTextCrumb($crumb);
     $crumbs->setBorder(true);
 
-    $timeline = null;
-    if (!$is_new) {
-      $timeline = $this->buildTransactionTimeline(
-        $config,
-        new PhabricatorAuthProviderConfigTransactionQuery());
-      $xactions = $timeline->getTransactions();
-      foreach ($xactions as $xaction) {
-        $xaction->setProvider($provider);
-      }
-      $timeline->setShouldTerminate(true);
-    }
-
     $form_box = id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Provider'))
       ->setFormErrors($errors)
@@ -371,7 +354,6 @@ final class PhabricatorAuthEditController
       ->setFooter(array(
         $form_box,
         $footer,
-        $timeline,
       ));
 
     return $this->newPage()
diff --git a/src/applications/auth/controller/config/PhabricatorAuthListController.php b/src/applications/auth/controller/config/PhabricatorAuthListController.php
index bb118d798e..f4b05e8adf 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthListController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthListController.php
@@ -19,31 +19,18 @@ final class PhabricatorAuthListController
 
       $id = $config->getID();
 
-      $edit_uri = $this->getApplicationURI('config/edit/'.$id.'/');
-      $enable_uri = $this->getApplicationURI('config/enable/'.$id.'/');
-      $disable_uri = $this->getApplicationURI('config/disable/'.$id.'/');
+      $view_uri = $config->getURI();
 
       $provider = $config->getProvider();
-      if ($provider) {
-        $name = $provider->getProviderName();
-      } else {
-        $name = $config->getProviderType().' ('.$config->getProviderClass().')';
-      }
+      $name = $provider->getProviderName();
 
-      $item->setHeader($name);
+      $item
+        ->setHeader($name)
+        ->setHref($view_uri);
 
-      if ($provider) {
-        $item->setHref($edit_uri);
-      } else {
-        $item->addAttribute(pht('Provider Implementation Missing!'));
-      }
-
-      $domain = null;
-      if ($provider) {
-        $domain = $provider->getProviderDomain();
-        if ($domain !== 'self') {
-          $item->addAttribute($domain);
-        }
+      $domain = $provider->getProviderDomain();
+      if ($domain !== 'self') {
+        $item->addAttribute($domain);
       }
 
       if ($config->getShouldAllowRegistration()) {
@@ -54,21 +41,9 @@ final class PhabricatorAuthListController
 
       if ($config->getIsEnabled()) {
         $item->setStatusIcon('fa-check-circle green');
-        $item->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('fa-times')
-            ->setHref($disable_uri)
-            ->setDisabled(!$can_manage)
-            ->addSigil('workflow'));
       } else {
         $item->setStatusIcon('fa-ban red');
         $item->addIcon('fa-ban grey', pht('Disabled'));
-        $item->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('fa-plus')
-            ->setHref($enable_uri)
-            ->setDisabled(!$can_manage)
-            ->addSigil('workflow'));
       }
 
       $list->addItem($item);
@@ -123,10 +98,11 @@ final class PhabricatorAuthListController
 
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)
-      ->setFooter(array(
-        $guidance,
-        $list,
-      ));
+      ->setFooter(
+        array(
+          $guidance,
+          $list,
+        ));
 
     $nav = $this->newNavigation()
       ->setCrumbs($crumbs)
diff --git a/src/applications/auth/controller/config/PhabricatorAuthProviderViewController.php b/src/applications/auth/controller/config/PhabricatorAuthProviderViewController.php
new file mode 100644
index 0000000000..532744001c
--- /dev/null
+++ b/src/applications/auth/controller/config/PhabricatorAuthProviderViewController.php
@@ -0,0 +1,119 @@
+<?php
+
+final class PhabricatorAuthProviderViewController
+  extends PhabricatorAuthProviderConfigController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $this->requireApplicationCapability(
+      AuthManageProvidersCapability::CAPABILITY);
+
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');
+
+    $config = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->withIDs(array($id))
+      ->executeOne();
+    if (!$config) {
+      return new Aphront404Response();
+    }
+
+    $header = $this->buildHeaderView($config);
+    $properties = $this->buildPropertiesView($config);
+    $curtain = $this->buildCurtain($config);
+
+    $timeline = $this->buildTransactionTimeline(
+      $config,
+      new PhabricatorAuthProviderConfigTransactionQuery());
+    $timeline->setShouldTerminate(true);
+
+    $view = id(new PHUITwoColumnView())
+      ->setHeader($header)
+      ->setCurtain($curtain)
+      ->addPropertySection(pht('Details'), $properties)
+      ->setMainColumn($timeline);
+
+    $crumbs = $this->buildApplicationCrumbs()
+      ->addTextCrumb($config->getObjectName())
+      ->setBorder(true);
+
+    return $this->newPage()
+      ->setTitle(pht('Auth Provider: %s', $config->getDisplayName()))
+      ->setCrumbs($crumbs)
+      ->appendChild($view);
+  }
+
+  private function buildHeaderView(PhabricatorAuthProviderConfig $config) {
+    $viewer = $this->getViewer();
+
+    $view = id(new PHUIHeaderView())
+      ->setViewer($viewer)
+      ->setHeader($config->getDisplayName());
+
+    if ($config->getIsEnabled()) {
+      $view->setStatus('fa-check', 'bluegrey', pht('Enabled'));
+    } else {
+      $view->setStatus('fa-ban', 'red', pht('Disabled'));
+    }
+
+    return $view;
+  }
+
+  private function buildCurtain(PhabricatorAuthProviderConfig $config) {
+    $viewer = $this->getViewer();
+    $id = $config->getID();
+
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $config,
+      PhabricatorPolicyCapability::CAN_EDIT);
+
+    $curtain = $this->newCurtainView($config);
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Edit Auth Provider'))
+        ->setIcon('fa-pencil')
+        ->setHref($this->getApplicationURI("config/edit/{$id}/"))
+        ->setDisabled(!$can_edit)
+        ->setWorkflow(!$can_edit));
+
+    if ($config->getIsEnabled()) {
+      $disable_uri = $this->getApplicationURI('config/disable/'.$id.'/');
+      $disable_icon = 'fa-ban';
+      $disable_text = pht('Disable Provider');
+    } else {
+      $disable_uri = $this->getApplicationURI('config/enable/'.$id.'/');
+      $disable_icon = 'fa-check';
+      $disable_text = pht('Enable Provider');
+    }
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setName($disable_text)
+        ->setIcon($disable_icon)
+        ->setHref($disable_uri)
+        ->setDisabled(!$can_edit)
+        ->setWorkflow(true));
+
+    return $curtain;
+  }
+
+  private function buildPropertiesView(PhabricatorAuthProviderConfig $config) {
+    $viewer = $this->getViewer();
+
+    $view = id(new PHUIPropertyListView())
+      ->setViewer($viewer);
+
+    $view->addProperty(
+      pht('Provider Type'),
+      $config->getProvider()->getProviderName());
+
+    return $view;
+  }
+}
diff --git a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
index 1d21342e4e..ee073e3ac1 100644
--- a/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthProviderConfigQuery.php
@@ -70,6 +70,19 @@ final class PhabricatorAuthProviderConfigQuery
     return $where;
   }
 
+  protected function willFilterPage(array $configs) {
+
+    foreach ($configs as $key => $config) {
+      $provider = $config->getProvider();
+      if (!$provider) {
+        unset($configs[$key]);
+        continue;
+      }
+    }
+
+    return $configs;
+  }
+
   public function getQueryApplicationClass() {
     return 'PhabricatorAuthApplication';
   }
diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
index 1de34c4077..6a8bbe1a0d 100644
--- a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
+++ b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
@@ -83,6 +83,18 @@ final class PhabricatorAuthProviderConfig
     return $this->provider;
   }
 
+  public function getURI() {
+    return '/auth/config/view/'.$this->getID().'/';
+  }
+
+  public function getObjectName() {
+    return pht('Auth Provider %d', $this->getID());
+  }
+
+  public function getDisplayName() {
+    return $this->getProvider()->getProviderName();
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

From 99e5ef84fc252eec7c3eeca82b2d1d3766ee670d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 08:52:13 -0800
Subject: [PATCH 12/34] Remove obsolete "PhabricatorAuthLoginHandler"

Summary: Depends on D20096. Reverts D14057. This was added for Phacility use cases in D14057 but never used. It is obsoleted by {nav Auth > Customize Messages} for non-Phacility use cases.

Test Plan: Grepped for removed symbol.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20099
---
 src/__phutil_library_map__.php                |  2 --
 .../PhabricatorAuthStartController.php        | 18 ----------
 .../handler/PhabricatorAuthLoginHandler.php   | 36 -------------------
 3 files changed, 56 deletions(-)
 delete mode 100644 src/applications/auth/handler/PhabricatorAuthLoginHandler.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 56af6a4374..7bc8670661 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2272,7 +2272,6 @@ phutil_register_library_map(array(
     'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php',
     'PhabricatorAuthListController' => 'applications/auth/controller/config/PhabricatorAuthListController.php',
     'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php',
-    'PhabricatorAuthLoginHandler' => 'applications/auth/handler/PhabricatorAuthLoginHandler.php',
     'PhabricatorAuthLoginMessageType' => 'applications/auth/message/PhabricatorAuthLoginMessageType.php',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthLogoutConduitAPIMethod.php',
     'PhabricatorAuthMFAEditEngineExtension' => 'applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php',
@@ -8019,7 +8018,6 @@ phutil_register_library_map(array(
     'PhabricatorAuthLinkController' => 'PhabricatorAuthController',
     'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController',
     'PhabricatorAuthLoginController' => 'PhabricatorAuthController',
-    'PhabricatorAuthLoginHandler' => 'Phobject',
     'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType',
     'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod',
     'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension',
diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php
index 29fa7e0b9f..848a5bda27 100644
--- a/src/applications/auth/controller/PhabricatorAuthStartController.php
+++ b/src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -172,23 +172,6 @@ final class PhabricatorAuthStartController
         $button_columns);
     }
 
-    $handlers = PhabricatorAuthLoginHandler::getAllHandlers();
-
-    $delegating_controller = $this->getDelegatingController();
-
-    $header = array();
-    foreach ($handlers as $handler) {
-      $handler = clone $handler;
-
-      $handler->setRequest($request);
-
-      if ($delegating_controller) {
-        $handler->setDelegatingController($delegating_controller);
-      }
-
-      $header[] = $handler->getAuthLoginHeaderContent();
-    }
-
     $invite_message = null;
     if ($invite) {
       $invite_message = $this->renderInviteHeader($invite);
@@ -202,7 +185,6 @@ final class PhabricatorAuthStartController
 
     $title = pht('Login');
     $view = array(
-      $header,
       $invite_message,
       $custom_message,
       $out,
diff --git a/src/applications/auth/handler/PhabricatorAuthLoginHandler.php b/src/applications/auth/handler/PhabricatorAuthLoginHandler.php
deleted file mode 100644
index eabbf91843..0000000000
--- a/src/applications/auth/handler/PhabricatorAuthLoginHandler.php
+++ /dev/null
@@ -1,36 +0,0 @@
-<?php
-
-abstract class PhabricatorAuthLoginHandler
-  extends Phobject {
-
-  private $request;
-  private $delegatingController;
-
-  public function getAuthLoginHeaderContent() {
-    return array();
-  }
-
-  final public function setDelegatingController(AphrontController $controller) {
-    $this->delegatingController = $controller;
-    return $this;
-  }
-
-  final public function getDelegatingController() {
-    return $this->delegatingController;
-  }
-
-  final public function setRequest(AphrontRequest $request) {
-    $this->request = $request;
-    return $this;
-  }
-
-  final public function getRequest() {
-    return $this->request;
-  }
-
-  final public static function getAllHandlers() {
-    return id(new PhutilClassMapQuery())
-      ->setAncestorClass(__CLASS__)
-      ->execute();
-  }
-}

From 9632c704c69d29134ed18d13282f1ce84f8a092e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 09:10:26 -0800
Subject: [PATCH 13/34] Always allow users to login via email link, even if an
 install does not use passwords

Summary:
Depends on D20099. Ref T13244. See PHI774. When password auth is enabled, we support a standard email-based account recovery mechanism with "Forgot password?".

When password auth is not enabled, we disable the self-serve version of this mechanism. You can still get email account login links via "Send Welcome Mail" or "bin/auth recover".

There's no real technical, product, or security reason not to let everyone do email login all the time. On the technical front, these links already work and are used in other contexts. On the product front, we just need to tweak a couple of strings.

On the security front, there's some argument that this mechanism provides more overall surface area for an attacker, but if we find that argument compelling we should probably provide a way to disable the self-serve pathway in all cases, rather than coupling it to which providers are enabled.

Also, inch toward having things iterate over configurations (saved database objects) instead of providers (abstract implementations) so we can some day live in a world where we support multiple configurations of the same provider type (T6703).

Test Plan:
  - With password auth enabled, reset password.
  - Without password auth enabled, did an email login recovery.

{F6184910}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20100
---
 .../PhabricatorAuthStartController.php        |  47 ++++++
 .../PhabricatorEmailLoginController.php       | 142 ++++++++++--------
 2 files changed, 127 insertions(+), 62 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php
index 848a5bda27..0b823098d7 100644
--- a/src/applications/auth/controller/PhabricatorAuthStartController.php
+++ b/src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -75,6 +75,11 @@ final class PhabricatorAuthStartController
       }
     }
 
+    $configs = array();
+    foreach ($providers as $provider) {
+      $configs[] = $provider->getProviderConfig();
+    }
+
     if (!$providers) {
       if ($this->isFirstTimeSetup()) {
         // If this is a fresh install, let the user register their admin
@@ -179,6 +184,8 @@ final class PhabricatorAuthStartController
 
     $custom_message = $this->newCustomStartMessage();
 
+    $email_login = $this->newEmailLoginView($configs);
+
     $crumbs = $this->buildApplicationCrumbs();
     $crumbs->addTextCrumb(pht('Login'));
     $crumbs->setBorder(true);
@@ -188,6 +195,7 @@ final class PhabricatorAuthStartController
       $invite_message,
       $custom_message,
       $out,
+      $email_login,
     );
 
     return $this->newPage()
@@ -311,4 +319,43 @@ final class PhabricatorAuthStartController
       $remarkup_view);
   }
 
+  private function newEmailLoginView(array $configs) {
+    assert_instances_of($configs, 'PhabricatorAuthProviderConfig');
+
+    // Check if password auth is enabled. If it is, the password login form
+    // renders a "Forgot password?" link, so we don't need to provide a
+    // supplemental link.
+
+    $has_password = false;
+    foreach ($configs as $config) {
+      $provider = $config->getProvider();
+      if ($provider instanceof PhabricatorPasswordAuthProvider) {
+        $has_password = true;
+      }
+    }
+
+    if ($has_password) {
+      return null;
+    }
+
+    $view = array(
+      pht('Trouble logging in?'),
+      ' ',
+      phutil_tag(
+        'a',
+        array(
+          'href' => '/login/email/',
+        ),
+        pht('Send a login link to your email address.')),
+    );
+
+    return phutil_tag(
+      'div',
+      array(
+        'class' => 'auth-custom-message',
+      ),
+      $view);
+  }
+
+
 }
diff --git a/src/applications/auth/controller/PhabricatorEmailLoginController.php b/src/applications/auth/controller/PhabricatorEmailLoginController.php
index f57a29b11a..eef30e6989 100644
--- a/src/applications/auth/controller/PhabricatorEmailLoginController.php
+++ b/src/applications/auth/controller/PhabricatorEmailLoginController.php
@@ -8,17 +8,13 @@ final class PhabricatorEmailLoginController
   }
 
   public function handleRequest(AphrontRequest $request) {
-
-    if (!PhabricatorPasswordAuthProvider::getPasswordProvider()) {
-      return new Aphront400Response();
-    }
+    $viewer = $this->getViewer();
 
     $e_email = true;
     $e_captcha = true;
     $errors = array();
 
-    $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business');
-
+    $v_email = $request->getStr('email');
     if ($request->isFormPost()) {
       $e_email = null;
       $e_captcha = pht('Again');
@@ -29,8 +25,7 @@ final class PhabricatorEmailLoginController
         $e_captcha = pht('Invalid');
       }
 
-      $email = $request->getStr('email');
-      if (!strlen($email)) {
+      if (!strlen($v_email)) {
        $errors[] = pht('You must provide an email address.');
        $e_email = pht('Required');
       }
@@ -42,7 +37,7 @@ final class PhabricatorEmailLoginController
 
         $target_email = id(new PhabricatorUserEmail())->loadOneWhere(
           'address = %s',
-          $email);
+          $v_email);
 
         $target_user = null;
         if ($target_email) {
@@ -81,33 +76,10 @@ final class PhabricatorEmailLoginController
         }
 
         if (!$errors) {
-          $engine = new PhabricatorAuthSessionEngine();
-          $uri = $engine->getOneTimeLoginURI(
-            $target_user,
-            null,
-            PhabricatorAuthSessionEngine::ONETIME_RESET);
-
-          if ($is_serious) {
-            $body = pht(
-              "You can use this link to reset your Phabricator password:".
-              "\n\n  %s\n",
-              $uri);
-          } else {
-            $body = pht(
-              "Condolences on forgetting your password. You can use this ".
-              "link to reset it:\n\n".
-              "  %s\n\n".
-              "After you set a new password, consider writing it down on a ".
-              "sticky note and attaching it to your monitor so you don't ".
-              "forget again! Choosing a very short, easy-to-remember password ".
-              "like \"cat\" or \"1234\" might also help.\n\n".
-              "Best Wishes,\nPhabricator\n",
-              $uri);
-
-          }
+          $body = $this->newAccountLoginMailBody($target_user);
 
           $mail = id(new PhabricatorMetaMTAMail())
-            ->setSubject(pht('[Phabricator] Password Reset'))
+            ->setSubject(pht('[Phabricator] Account Login Link'))
             ->setForceDelivery(true)
             ->addRawTos(array($target_email->getAddress()))
             ->setBody($body)
@@ -123,44 +95,90 @@ final class PhabricatorEmailLoginController
       }
     }
 
-    $error_view = null;
-    if ($errors) {
-      $error_view = new PHUIInfoView();
-      $error_view->setErrors($errors);
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer);
+
+    if ($this->isPasswordAuthEnabled()) {
+      $form->appendRemarkupInstructions(
+        pht(
+          'To reset your password, provide your email address. An email '.
+          'with a login link will be sent to you.'));
+    } else {
+      $form->appendRemarkupInstructions(
+        pht(
+          'To access your account, provide your email address. An email '.
+          'with a login link will be sent to you.'));
     }
 
-    $email_auth = new PHUIFormLayoutView();
-    $email_auth->appendChild($error_view);
-    $email_auth
-      ->setUser($request->getUser())
-      ->setFullWidth(true)
-      ->appendChild(
+    $form
+      ->appendControl(
         id(new AphrontFormTextControl())
-          ->setLabel(pht('Email'))
+          ->setLabel(pht('Email Address'))
           ->setName('email')
-          ->setValue($request->getStr('email'))
+          ->setValue($v_email)
           ->setError($e_email))
-      ->appendChild(
+      ->appendControl(
         id(new AphrontFormRecaptchaControl())
           ->setLabel(pht('Captcha'))
           ->setError($e_captcha));
 
-    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(pht('Reset Password'));
-    $crumbs->setBorder(true);
-
-    $dialog = new AphrontDialogView();
-    $dialog->setUser($request->getUser());
-    $dialog->setTitle(pht('Forgot Password / Email Login'));
-    $dialog->appendChild($email_auth);
-    $dialog->addSubmitButton(pht('Send Email'));
-    $dialog->setSubmitURI('/login/email/');
-
-    return $this->newPage()
-      ->setTitle(pht('Forgot Password'))
-      ->setCrumbs($crumbs)
-      ->appendChild($dialog);
+    if ($this->isPasswordAuthEnabled()) {
+      $title = pht('Password Reset');
+    } else {
+      $title = pht('Email Login');
+    }
 
+    return $this->newDialog()
+      ->setTitle($title)
+      ->setErrors($errors)
+      ->setWidth(AphrontDialogView::WIDTH_FORM)
+      ->appendForm($form)
+      ->addCancelButton('/auth/start/')
+      ->addSubmitButton(pht('Send Email'));
   }
 
+  private function newAccountLoginMailBody(PhabricatorUser $user) {
+    $engine = new PhabricatorAuthSessionEngine();
+    $uri = $engine->getOneTimeLoginURI(
+      $user,
+      null,
+      PhabricatorAuthSessionEngine::ONETIME_RESET);
+
+    $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business');
+    $have_passwords = $this->isPasswordAuthEnabled();
+
+    if ($have_passwords) {
+      if ($is_serious) {
+        $body = pht(
+          "You can use this link to reset your Phabricator password:".
+          "\n\n  %s\n",
+          $uri);
+      } else {
+        $body = pht(
+          "Condolences on forgetting your password. You can use this ".
+          "link to reset it:\n\n".
+          "  %s\n\n".
+          "After you set a new password, consider writing it down on a ".
+          "sticky note and attaching it to your monitor so you don't ".
+          "forget again! Choosing a very short, easy-to-remember password ".
+          "like \"cat\" or \"1234\" might also help.\n\n".
+          "Best Wishes,\nPhabricator\n",
+          $uri);
+
+      }
+    } else {
+      $body = pht(
+        "You can use this login link to regain access to your Phabricator ".
+        "account:".
+        "\n\n".
+        "  %s\n",
+        $uri);
+    }
+
+    return $body;
+  }
+
+  private function isPasswordAuthEnabled() {
+    return (bool)PhabricatorPasswordAuthProvider::getPasswordProvider();
+  }
 }

From 113a2773dd5bb41921dedc2205820d528ca42db6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 10:38:20 -0800
Subject: [PATCH 14/34] Remove one-time login from username change email

Summary:
Depends on D20100. Ref T7732. Ref T13244. This is a bit of an adventure.

Long ago, passwords were digested with usernames as part of the salt. This was a mistake: it meant that your password becomes invalid if your username is changed.

(I think very very long ago, some other hashing may also have used usernames -- perhaps session hashing or CSRF hashing?)

To work around this, the "username change" email included a one-time login link and some language about resetting your password.

This flaw was fixed when passwords were moved to shared infrastructure (they're now salted more cleanly on a per-digest basis), and since D18908 (about a year ago) we've transparently upgraded password digests on use.

Although it's still technically possible that a username change could invalidate your password, it requires:

  - You set the password on a version of Phabricator earlier than ~2018 Week 5 (about a year ago).
  - You haven't logged into a version of Phabricator newer than that using your password since then.
  - Your username is changed.

This probably affects more than zero users, but I suspect not //many// more than zero. These users can always use "Forgot password?" to recover account access.

Since the value of this is almost certainly very near zero now and declining over time, just get rid of it. Also move the actual mail out of `PhabricatorUser`, ala the similar recent change to welcome mail in D19989.

Test Plan: Changed a user's username, reviewed resulting mail with `bin/mail show-outbound`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244, T7732

Differential Revision: https://secure.phabricator.com/D20102
---
 src/__phutil_library_map__.php                |  2 +
 .../PhabricatorPeopleUsernameMailEngine.php   | 60 +++++++++++++++++++
 .../people/storage/PhabricatorUser.php        | 49 ---------------
 .../PhabricatorUserUsernameTransaction.php    | 15 ++++-
 4 files changed, 74 insertions(+), 52 deletions(-)
 create mode 100644 src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 7bc8670661..b9d468ea3c 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3899,6 +3899,7 @@ phutil_register_library_map(array(
     'PhabricatorPeopleTransactionQuery' => 'applications/people/query/PhabricatorPeopleTransactionQuery.php',
     'PhabricatorPeopleUserFunctionDatasource' => 'applications/people/typeahead/PhabricatorPeopleUserFunctionDatasource.php',
     'PhabricatorPeopleUserPHIDType' => 'applications/people/phid/PhabricatorPeopleUserPHIDType.php',
+    'PhabricatorPeopleUsernameMailEngine' => 'applications/people/mail/PhabricatorPeopleUsernameMailEngine.php',
     'PhabricatorPeopleWelcomeController' => 'applications/people/controller/PhabricatorPeopleWelcomeController.php',
     'PhabricatorPeopleWelcomeMailEngine' => 'applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php',
     'PhabricatorPhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorPhabricatorAuthProvider.php',
@@ -9895,6 +9896,7 @@ phutil_register_library_map(array(
     'PhabricatorPeopleTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorPeopleUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
     'PhabricatorPeopleUserPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorPeopleUsernameMailEngine' => 'PhabricatorPeopleMailEngine',
     'PhabricatorPeopleWelcomeController' => 'PhabricatorPeopleController',
     'PhabricatorPeopleWelcomeMailEngine' => 'PhabricatorPeopleMailEngine',
     'PhabricatorPhabricatorAuthProvider' => 'PhabricatorOAuth2AuthProvider',
diff --git a/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php b/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php
new file mode 100644
index 0000000000..c954b7c38e
--- /dev/null
+++ b/src/applications/people/mail/PhabricatorPeopleUsernameMailEngine.php
@@ -0,0 +1,60 @@
+<?php
+
+final class PhabricatorPeopleUsernameMailEngine
+  extends PhabricatorPeopleMailEngine {
+
+  private $oldUsername;
+  private $newUsername;
+
+  public function setNewUsername($new_username) {
+    $this->newUsername = $new_username;
+    return $this;
+  }
+
+  public function getNewUsername() {
+    return $this->newUsername;
+  }
+
+  public function setOldUsername($old_username) {
+    $this->oldUsername = $old_username;
+    return $this;
+  }
+
+  public function getOldUsername() {
+    return $this->oldUsername;
+  }
+
+  public function validateMail() {
+    return;
+  }
+
+  protected function newMail() {
+    $sender = $this->getSender();
+    $recipient = $this->getRecipient();
+
+    $sender_username = $sender->getUsername();
+    $sender_realname = $sender->getRealName();
+
+    $old_username = $this->getOldUsername();
+    $new_username = $this->getNewUsername();
+
+    $body = sprintf(
+      "%s\n\n  %s\n  %s\n",
+      pht(
+        '%s (%s) has changed your Phabricator username.',
+        $sender_username,
+        $sender_realname),
+      pht(
+        'Old Username: %s',
+        $old_username),
+      pht(
+        'New Username: %s',
+        $new_username));
+
+    return id(new PhabricatorMetaMTAMail())
+      ->addTos(array($recipient->getPHID()))
+      ->setSubject(pht('[Phabricator] Username Changed'))
+      ->setBody($body);
+  }
+
+}
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 0b18c292c2..70d2d9fb4e 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -555,55 +555,6 @@ final class PhabricatorUser
     }
   }
 
-  public function sendUsernameChangeEmail(
-    PhabricatorUser $admin,
-    $old_username) {
-
-    $admin_username = $admin->getUserName();
-    $admin_realname = $admin->getRealName();
-    $new_username = $this->getUserName();
-
-    $password_instructions = null;
-    if (PhabricatorPasswordAuthProvider::getPasswordProvider()) {
-      $engine = new PhabricatorAuthSessionEngine();
-      $uri = $engine->getOneTimeLoginURI(
-        $this,
-        null,
-        PhabricatorAuthSessionEngine::ONETIME_USERNAME);
-      $password_instructions = sprintf(
-        "%s\n\n  %s\n\n%s\n",
-        pht(
-          "If you use a password to login, you'll need to reset it ".
-          "before you can login again. You can reset your password by ".
-          "following this link:"),
-        $uri,
-        pht(
-          "And, of course, you'll need to use your new username to login ".
-          "from now on. If you use OAuth to login, nothing should change."));
-    }
-
-    $body = sprintf(
-      "%s\n\n  %s\n  %s\n\n%s",
-      pht(
-        '%s (%s) has changed your Phabricator username.',
-        $admin_username,
-        $admin_realname),
-      pht(
-        'Old Username: %s',
-        $old_username),
-      pht(
-        'New Username: %s',
-        $new_username),
-      $password_instructions);
-
-    $mail = id(new PhabricatorMetaMTAMail())
-      ->addTos(array($this->getPHID()))
-      ->setForceDelivery(true)
-      ->setSubject(pht('[Phabricator] Username Changed'))
-      ->setBody($body)
-      ->saveAndSend();
-  }
-
   public static function describeValidUsername() {
     return pht(
       'Usernames must contain only numbers, letters, period, underscore and '.
diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
index f2226d0010..b436b76716 100644
--- a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
+++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php
@@ -18,18 +18,27 @@ final class PhabricatorUserUsernameTransaction
   }
 
   public function applyExternalEffects($object, $value) {
+    $actor = $this->getActor();
     $user = $object;
 
+    $old_username = $this->getOldValue();
+    $new_username = $this->getNewValue();
+
     $this->newUserLog(PhabricatorUserLog::ACTION_CHANGE_USERNAME)
-      ->setOldValue($this->getOldValue())
-      ->setNewValue($value)
+      ->setOldValue($old_username)
+      ->setNewValue($new_username)
       ->save();
 
     // The SSH key cache currently includes usernames, so dirty it. See T12554
     // for discussion.
     PhabricatorAuthSSHKeyQuery::deleteSSHKeyCache();
 
-    $user->sendUsernameChangeEmail($this->getActor(), $this->getOldValue());
+    id(new PhabricatorPeopleUsernameMailEngine())
+      ->setSender($actor)
+      ->setRecipient($object)
+      ->setOldUsername($old_username)
+      ->setNewUsername($new_username)
+      ->sendMail();
   }
 
   public function getTitle() {

From 55286d49e8ec74e03b3bdbd5374e50415b2fab48 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 06:19:12 -0800
Subject: [PATCH 15/34] Clarify "metamta.default-address" instructions and lock
 the option

Summary: This option no longer needs to be configured if you configure inbound mail (and that's the easiest setup approach in a lot of cases), so stop telling users they have to set it up.

Test Plan: Read documentation and configuration help.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20104
---
 .../PhabricatorMetaMTAConfigOptions.php       | 23 +++++++++++++-
 .../configuring_outbound_email.diviner        | 31 +++++++++++++------
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
index ce24d48ead..7e6978dfd8 100644
--- a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
+++ b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
@@ -182,6 +182,25 @@ EODOC
     $mailers_description = $this->deformat(pht(<<<EODOC
 Define one or more mail transmission services. For help with configuring
 mailers, see **[[ %s | %s ]]** in the documentation.
+EODOC
+      ,
+      PhabricatorEnv::getDoclink('Configuring Outbound Email'),
+      pht('Configuring Outbound Email')));
+
+    $default_description = $this->deformat(pht(<<<EODOC
+Default address used as a "From" or "To" email address when an address is
+required but no meaningful address is available.
+
+If you configure inbound mail, you generally do not need to set this:
+Phabricator will automatically generate and use a suitable mailbox on the
+inbound mail domain.
+
+Otherwise, this option should be configured to point at a valid mailbox which
+discards all mail sent to it. If you point it at an invalid mailbox, mail sent
+by Phabricator and some mail sent by users will bounce. If you point it at a
+real user mailbox, that user will get a lot of mail they don't want.
+
+For further guidance, see **[[ %s | %s ]]** in the documentation.
 EODOC
       ,
       PhabricatorEnv::getDoclink('Configuring Outbound Email'),
@@ -192,7 +211,9 @@ EODOC
         ->setHidden(true)
         ->setDescription($mailers_description),
       $this->newOption('metamta.default-address', 'string', null)
-        ->setDescription(pht('Default "From" address.')),
+        ->setLocked(true)
+        ->setSummary(pht('Default address used when generating mail.'))
+        ->setDescription($default_description),
       $this->newOption(
         'metamta.one-mail-per-recipient',
         'bool',
diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner
index 4d18ba0eb2..6f5212680e 100644
--- a/src/docs/user/configuration/configuring_outbound_email.diviner
+++ b/src/docs/user/configuration/configuring_outbound_email.diviner
@@ -47,18 +47,31 @@ not. For more information on using daemons, see
 @{article:Managing Daemons with phd}.
 
 
-Basics
-======
+Outbound "From" and "To" Addresses
+==================================
 
-Before configuring outbound mail, you should first set up
-`metamta.default-address` in Configuration. This determines where mail is sent
-"From" by default.
+When Phabricator sends outbound mail, it must select some "From" address to
+send mail from, since mailers require this.
 
-If your domain is `example.org`, set this to something
-like `noreply@example.org`.
+When mail only has "CC" recipients, Phabricator generates a dummy "To" address,
+since some mailers require this and some users write mail rules that depend
+on whether they appear in the "To" or "CC" line.
 
-Ideally, this should be a valid, deliverable address that doesn't bounce if
-users accidentally send mail to it.
+In both cases, the address should ideally correspond to a valid, deliverable
+mailbox that accepts the mail and then simply discards it. If the address is
+not valid, some outbound mail will bounce, and users will receive bounces when
+they "Reply All" even if the other recipients for the message are valid. In
+contrast, if the address is a real user address, that user will receive a lot
+of mail they probably don't want.
+
+If you plan to configure //inbound// mail later, you usually don't need to do
+anything. Phabricator will automatically create a `noreply@` mailbox which
+works the right way (accepts and discards all mail it receives) and
+automatically use it when generating addresses.
+
+If you don't plan to configure inbound mail, you may need to configure an
+address for Phabricator to use. You can do this by setting
+`metamta.default-address`.
 
 
 Configuring Mailers

From d6f691cf5d5d839e470db0ba1778ef59220937f7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 07:23:59 -0800
Subject: [PATCH 16/34] In "External Accounts", replace hard-to-find tiny
 "link" icon with a nice button with text on it

Summary:
Ref T6703. Replaces the small "link" icon with a more obvious "Link External Account" button.

Moves us toward operating against `$config` objects instead of against `$provider` objects, which is more modern and will some day allow us to resolve T6703.

Test Plan: Viewed page, saw a more obvious button. Linked an external account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20105
---
 .../storage/PhabricatorAuthProviderConfig.php |  9 +++++
 ...abricatorExternalAccountsSettingsPanel.php | 37 +++++++++++++------
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
index 6a8bbe1a0d..876a70c2d0 100644
--- a/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
+++ b/src/applications/auth/storage/PhabricatorAuthProviderConfig.php
@@ -95,6 +95,15 @@ final class PhabricatorAuthProviderConfig
     return $this->getProvider()->getProviderName();
   }
 
+  public function getSortVector() {
+    return id(new PhutilSortVector())
+      ->addString($this->getDisplayName());
+  }
+
+  public function newIconView() {
+    return $this->getProvider()->newIconView();
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
index 1215487208..9401cddbef 100644
--- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
@@ -105,26 +105,39 @@ final class PhabricatorExternalAccountsSettingsPanel
 
     $accounts = mpull($accounts, null, 'getProviderKey');
 
-    $providers = PhabricatorAuthProvider::getAllEnabledProviders();
-    $providers = msort($providers, 'getProviderName');
-    foreach ($providers as $key => $provider) {
-      if (isset($accounts[$key])) {
-        continue;
-      }
+    $configs = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->withIsEnabled(true)
+      ->execute();
+    $configs = msort($configs, 'getSortVector');
+
+    foreach ($configs as $config) {
+      $provider = $config->getProvider();
 
       if (!$provider->shouldAllowAccountLink()) {
         continue;
       }
 
+      // Don't show the user providers they already have linked.
+      $provider_key = $config->getProvider()->getProviderKey();
+      if (isset($accounts[$provider_key])) {
+        continue;
+      }
+
       $link_uri = '/auth/link/'.$provider->getProviderKey().'/';
 
-      $item = id(new PHUIObjectItemView())
-        ->setHeader($provider->getProviderName())
+      $link_button = id(new PHUIButtonView())
+        ->setTag('a')
+        ->setIcon('fa-link')
         ->setHref($link_uri)
-        ->addAction(
-          id(new PHUIListItemView())
-            ->setIcon('fa-link')
-            ->setHref($link_uri));
+        ->setColor(PHUIButtonView::GREY)
+        ->setText(pht('Link External Account'));
+
+      $item = id(new PHUIObjectItemView())
+        ->setHeader($config->getDisplayName())
+        ->setHref($link_uri)
+        ->setImageIcon($config->newIconView())
+        ->setSideColumn($link_button);
 
       $linkable->addItem($item);
     }

From fc3b90e1d1b1cd64866ad9274594072e6c4c2486 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 11:43:01 -0800
Subject: [PATCH 17/34] Allow users to unlink their last external account with
 a warning, instead of preventing the action

Summary:
Depends on D20105. Fixes T7732. T7732 describes a case where a user had their Google credentials swapped and had trouble regaining access to their account.

Since we now allow email login even if password auth is disabled, it's okay to let users unlink their final account, and it's even reasonable for users to unlink their final account if it is mis-linked.

Just give them a warning that what they're doing is a little sketchy, rather than preventing the workflow.

Test Plan: Unlinked my only login account, got a stern warning instead of a dead end.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7732

Differential Revision: https://secure.phabricator.com/D20106
---
 .../PhabricatorAuthUnlinkController.php       | 68 +++++++++++--------
 ...abricatorExternalAccountsSettingsPanel.php |  9 ---
 2 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
index e6e1493e5a..1e3023e5d2 100644
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -32,8 +32,15 @@ final class PhabricatorAuthUnlinkController
       }
     }
 
-    // Check that this account isn't the last account which can be used to
-    // login. We prevent you from removing the last account.
+    $confirmations = $request->getStrList('confirmations');
+    $confirmations = array_fuse($confirmations);
+
+    if (!$request->isFormPost() || !isset($confirmations['unlink'])) {
+      return $this->renderConfirmDialog($confirmations);
+    }
+
+    // Check that this account isn't the only account which can be used to
+    // login. We warn you when you remove your only login account.
     if ($account->isUsableForLogin()) {
       $other_accounts = id(new PhabricatorExternalAccount())->loadAllWhere(
         'userPHID = %s',
@@ -47,22 +54,20 @@ final class PhabricatorAuthUnlinkController
       }
 
       if ($valid_accounts < 2) {
-        return $this->renderLastUsableAccountErrorDialog();
+        if (!isset($confirmations['only'])) {
+          return $this->renderOnlyUsableAccountConfirmDialog($confirmations);
+        }
       }
     }
 
-    if ($request->isDialogFormPost()) {
-      $account->delete();
+    $account->delete();
 
-      id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
-        $viewer,
-        new PhutilOpaqueEnvelope(
-          $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
+    id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
+      $viewer,
+      new PhutilOpaqueEnvelope(
+        $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
-      return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
-    }
-
-    return $this->renderConfirmDialog();
+    return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
   }
 
   private function getDoneURI() {
@@ -97,22 +102,27 @@ final class PhabricatorAuthUnlinkController
     return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
-  private function renderLastUsableAccountErrorDialog() {
-    $dialog = id(new AphrontDialogView())
-      ->setUser($this->getRequest()->getUser())
-      ->setTitle(pht('Last Valid Account'))
-      ->appendChild(
-        pht(
-          'You can not unlink this account because you have no other '.
-          'valid login accounts. If you removed it, you would be unable '.
-          'to log in. Add another authentication method before removing '.
-          'this one.'))
-      ->addCancelButton($this->getDoneURI());
+  private function renderOnlyUsableAccountConfirmDialog(array $confirmations) {
+    $confirmations[] = 'only';
 
-    return id(new AphrontDialogResponse())->setDialog($dialog);
+    return $this->newDialog()
+      ->setTitle(pht('Unlink Your Only Login Account?'))
+      ->addHiddenInput('confirmations', implode(',', $confirmations))
+      ->appendParagraph(
+        pht(
+          'This is the only external login account linked to your Phabicator '.
+          'account. If you remove it, you may no longer be able to log in.'))
+      ->appendParagraph(
+        pht(
+          'If you lose access to your account, you can recover access by '.
+          'sending yourself an email login link from the login screen.'))
+      ->addCancelButton($this->getDoneURI())
+      ->addSubmitButton(pht('Unlink External Account'));
   }
 
-  private function renderConfirmDialog() {
+  private function renderConfirmDialog(array $confirmations) {
+    $confirmations[] = 'unlink';
+
     $provider_key = $this->providerKey;
     $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key);
 
@@ -129,9 +139,9 @@ final class PhabricatorAuthUnlinkController
         'to Phabricator.');
     }
 
-    $dialog = id(new AphrontDialogView())
-      ->setUser($this->getRequest()->getUser())
+    return $this->newDialog()
       ->setTitle($title)
+      ->addHiddenInput('confirmations', implode(',', $confirmations))
       ->appendParagraph($body)
       ->appendParagraph(
         pht(
@@ -139,8 +149,6 @@ final class PhabricatorAuthUnlinkController
           'other active login sessions.'))
       ->addSubmitButton(pht('Unlink Account'))
       ->addCancelButton($this->getDoneURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
   }
 
 }
diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
index 9401cddbef..29ef9fa2c7 100644
--- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
@@ -41,13 +41,6 @@ final class PhabricatorExternalAccountsSettingsPanel
       ->setUser($viewer)
       ->setNoDataString(pht('You have no linked accounts.'));
 
-    $login_accounts = 0;
-    foreach ($accounts as $account) {
-      if ($account->isUsableForLogin()) {
-        $login_accounts++;
-      }
-    }
-
     foreach ($accounts as $account) {
       $item = new PHUIObjectItemView();
 
@@ -72,8 +65,6 @@ final class PhabricatorExternalAccountsSettingsPanel
             'account provider).'));
       }
 
-      $can_unlink = $can_unlink && (!$can_login || ($login_accounts > 1));
-
       $can_refresh = $provider && $provider->shouldAllowAccountRefresh();
       if ($can_refresh) {
         $item->addAction(

From a46c25d2baf307f52a5a456f0445ea21dbac3a28 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 08:51:42 -0800
Subject: [PATCH 18/34] Make two ancient migrations fatal if they affect data

Summary:
Depends on D20106. Ref T6703. Since I plan to change the `ExternalAccount` table, these migrations (which rely on `save()`) will stop working.

They could be rewritten to use raw queries, but I suspect few or no installs are affected. At least for now, just make them safe: if they would affect data, fatal and tell the user to perform a more gradual upgrade.

Also remove an `ALTER IGNORE TABLE` (this syntax was removed at some point) and fix a `%Q` when adjusting certain types of primary keys.

Test Plan: Ran `bin/storage upgrade --no-quickstart --force --namespace test1234` to get a complete migration since the beginning of time.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20107
---
 resources/sql/patches/133.imagemacro.sql      |  4 +-
 .../sql/patches/20130611.migrateoauth.php     | 68 +++----------------
 resources/sql/patches/20130611.nukeldap.php   | 43 +++---------
 .../PhabricatorStorageManagementWorkflow.php  |  2 +-
 4 files changed, 19 insertions(+), 98 deletions(-)

diff --git a/resources/sql/patches/133.imagemacro.sql b/resources/sql/patches/133.imagemacro.sql
index 01852c6b48..1477fd879f 100644
--- a/resources/sql/patches/133.imagemacro.sql
+++ b/resources/sql/patches/133.imagemacro.sql
@@ -1,2 +1,2 @@
-ALTER IGNORE TABLE `{$NAMESPACE}_file`.`file_imagemacro`
-  ADD UNIQUE `name` (`name`);
+ALTER TABLE `{$NAMESPACE}_file`.`file_imagemacro`
+  ADD UNIQUE KEY `name` (`name`);
diff --git a/resources/sql/patches/20130611.migrateoauth.php b/resources/sql/patches/20130611.migrateoauth.php
index 3622b2772e..92fe854cfd 100644
--- a/resources/sql/patches/20130611.migrateoauth.php
+++ b/resources/sql/patches/20130611.migrateoauth.php
@@ -1,66 +1,14 @@
 <?php
 
-// NOTE: We aren't using PhabricatorUserOAuthInfo anywhere here because it is
-// getting nuked in a future diff.
-
 $table = new PhabricatorUser();
+$conn = $table->establishConnection('w');
 $table_name = 'user_oauthinfo';
-$conn_w = $table->establishConnection('w');
 
-$xaccount = new PhabricatorExternalAccount();
-
-echo pht('Migrating OAuth to %s...', 'ExternalAccount')."\n";
-
-$domain_map = array(
-  'disqus'      => 'disqus.com',
-  'facebook'    => 'facebook.com',
-  'github'      => 'github.com',
-  'google'      => 'google.com',
-);
-
-try {
-  $phabricator_oauth_uri = new PhutilURI(
-    PhabricatorEnv::getEnvConfig('phabricator.oauth-uri'));
-  $domain_map['phabricator'] = $phabricator_oauth_uri->getDomain();
-} catch (Exception $ex) {
-  // Ignore; this likely indicates that we have removed `phabricator.oauth-uri`
-  // in some future diff.
+foreach (new LiskRawMigrationIterator($conn, $table_name) as $row) {
+  throw new Exception(
+    pht(
+      'Your Phabricator install has ancient OAuth account data and is '.
+      'too old to upgrade directly to a modern version of Phabricator. '.
+      'Upgrade to a version released between June 2013 and February 2019 '.
+      'first, then upgrade to a modern version.'));
 }
-
-$rows = queryfx_all(
-  $conn_w,
-  'SELECT * FROM user_oauthinfo');
-foreach ($rows as $row) {
-  echo pht('Migrating row ID #%d.', $row['id'])."\n";
-  $user = id(new PhabricatorUser())->loadOneWhere(
-    'id = %d',
-    $row['userID']);
-  if (!$user) {
-    echo pht('Bad user ID!')."\n";
-    continue;
-  }
-
-  $domain = idx($domain_map, $row['oauthProvider']);
-  if (empty($domain)) {
-    echo pht('Unknown OAuth provider!')."\n";
-    continue;
-  }
-
-
-  $xaccount = id(new PhabricatorExternalAccount())
-    ->setUserPHID($user->getPHID())
-    ->setAccountType($row['oauthProvider'])
-    ->setAccountDomain($domain)
-    ->setAccountID($row['oauthUID'])
-    ->setAccountURI($row['accountURI'])
-    ->setUsername($row['accountName'])
-    ->setDateCreated($row['dateCreated']);
-
-  try {
-    $xaccount->save();
-  } catch (Exception $ex) {
-    phlog($ex);
-  }
-}
-
-echo pht('Done.')."\n";
diff --git a/resources/sql/patches/20130611.nukeldap.php b/resources/sql/patches/20130611.nukeldap.php
index 3f225cfa84..0f0b976a58 100644
--- a/resources/sql/patches/20130611.nukeldap.php
+++ b/resources/sql/patches/20130611.nukeldap.php
@@ -1,41 +1,14 @@
 <?php
 
-// NOTE: We aren't using PhabricatorUserLDAPInfo anywhere here because it is
-// being nuked by this change
-
 $table = new PhabricatorUser();
+$conn = $table->establishConnection('w');
 $table_name = 'user_ldapinfo';
-$conn_w = $table->establishConnection('w');
 
-$xaccount = new PhabricatorExternalAccount();
-
-echo pht('Migrating LDAP to %s...', 'ExternalAccount')."\n";
-
-$rows = queryfx_all($conn_w, 'SELECT * FROM %T', $table_name);
-foreach ($rows as $row) {
-  echo pht('Migrating row ID #%d.', $row['id'])."\n";
-  $user = id(new PhabricatorUser())->loadOneWhere(
-    'id = %d',
-    $row['userID']);
-  if (!$user) {
-    echo pht('Bad user ID!')."\n";
-    continue;
-  }
-
-
-  $xaccount = id(new PhabricatorExternalAccount())
-    ->setUserPHID($user->getPHID())
-    ->setAccountType('ldap')
-    ->setAccountDomain('self')
-    ->setAccountID($row['ldapUsername'])
-    ->setUsername($row['ldapUsername'])
-    ->setDateCreated($row['dateCreated']);
-
-  try {
-    $xaccount->save();
-  } catch (Exception $ex) {
-    phlog($ex);
-  }
+foreach (new LiskRawMigrationIterator($conn, $table_name) as $row) {
+  throw new Exception(
+    pht(
+      'Your Phabricator install has ancient LDAP account data and is '.
+      'too old to upgrade directly to a modern version of Phabricator. '.
+      'Upgrade to a version released between June 2013 and February 2019 '.
+      'first, then upgrade to a modern version.'));
 }
-
-echo pht('Done.')."\n";
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
index 5bc83972dd..acbbb4fbda 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
@@ -432,7 +432,7 @@ abstract class PhabricatorStorageManagementWorkflow
             case 'key':
               if (($phase == 'drop_keys') && $adjust['exists']) {
                 if ($adjust['name'] == 'PRIMARY') {
-                  $key_name = 'PRIMARY KEY';
+                  $key_name = qsprintf($conn, 'PRIMARY KEY');
                 } else {
                   $key_name = qsprintf($conn, 'KEY %T', $adjust['name']);
                 }

From f2236eb061a6fb7cc8fb645fe21d8194966cbc86 Mon Sep 17 00:00:00 2001
From: Austin McKinley <austin@phacility.com>
Date: Thu, 7 Feb 2019 11:32:48 -0800
Subject: [PATCH 19/34] Autofocus form control for adding TOTP codes

Summary: Ref D20122. This is something I wanted in a bunch of places. Looks like at some point the most-annoying one (autofocus for entering TOTOP codes) already got fixed at some point.

Test Plan: Loaded the form, got autofocus as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D20128
---
 .../auth/factor/PhabricatorTOTPAuthFactor.php         |  1 +
 src/view/form/control/PHUIFormNumberControl.php       | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
index ba6613c014..7e77dfc11a 100644
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -128,6 +128,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
         ->setLabel(pht('TOTP Code'))
         ->setName('totpcode')
         ->setValue($code)
+        ->setAutofocus(true)
         ->setError($e_code));
 
   }
diff --git a/src/view/form/control/PHUIFormNumberControl.php b/src/view/form/control/PHUIFormNumberControl.php
index 26e7e03955..c577bebbd0 100644
--- a/src/view/form/control/PHUIFormNumberControl.php
+++ b/src/view/form/control/PHUIFormNumberControl.php
@@ -3,6 +3,7 @@
 final class PHUIFormNumberControl extends AphrontFormControl {
 
   private $disableAutocomplete;
+  private $autofocus;
 
   public function setDisableAutocomplete($disable_autocomplete) {
     $this->disableAutocomplete = $disable_autocomplete;
@@ -13,6 +14,15 @@ final class PHUIFormNumberControl extends AphrontFormControl {
     return $this->disableAutocomplete;
   }
 
+  public function setAutofocus($autofocus) {
+    $this->autofocus = $autofocus;
+    return $this;
+  }
+
+  public function getAutofocus() {
+    return $this->autofocus;
+  }
+
   protected function getCustomControlClass() {
     return 'phui-form-number';
   }
@@ -34,6 +44,7 @@ final class PHUIFormNumberControl extends AphrontFormControl {
         'disabled' => $this->getDisabled() ? 'disabled' : null,
         'autocomplete' => $autocomplete,
         'id' => $this->getID(),
+        'autofocus' => ($this->getAutofocus() ? 'autofocus' : null),
       ));
   }
 

From 26081594e2f3c1bd01ce785089b5b5a23625e4ef Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 09:26:07 -0800
Subject: [PATCH 20/34] Fix two very, very minor correctness issues in Slowvote

Summary:
See <https://hackerone.com/reports/492525> and <https://hackerone.com/reports/489531>. I previously awarded a bounty for <https://hackerone.com/reports/434116> so Slowvote is getting "researched" a lot.

  - Prevent users from undoing their vote by submitting the form with nothing selected.
  - Prevent users from racing between the `delete()` and `save()` to vote for multiple options in a plurality poll.

Test Plan:
  - Clicked the vote button with nothing selected in plurality and approval polls, got an error now.
  - Added a `sleep(5)` between `delete()` and `save()`. Submitted different plurality votes in different windows. Before: votes raced, invalid end state. After: votes waited on the lock, arrived in a valid end state.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20125
---
 .../PhabricatorSlowvoteVoteController.php     | 53 ++++++++++++++-----
 1 file changed, 41 insertions(+), 12 deletions(-)

diff --git a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
index 62913b09e3..e1a1b9df34 100644
--- a/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
+++ b/src/applications/slowvote/controller/PhabricatorSlowvoteVoteController.php
@@ -37,6 +37,19 @@ final class PhabricatorSlowvoteVoteController
     $method = $poll->getMethod();
     $is_plurality = ($method == PhabricatorSlowvotePoll::METHOD_PLURALITY);
 
+    if (!$votes) {
+      if ($is_plurality) {
+        $message = pht('You must vote for something.');
+      } else {
+        $message = pht('You must vote for at least one option.');
+      }
+
+      return $this->newDialog()
+        ->setTitle(pht('Stand For Something'))
+        ->appendParagraph($message)
+        ->addCancelButton($poll->getURI());
+    }
+
     if ($is_plurality && count($votes) > 1) {
       throw new Exception(
         pht('In this poll, you may only vote for one option.'));
@@ -52,23 +65,39 @@ final class PhabricatorSlowvoteVoteController
       }
     }
 
-    foreach ($old_votes as $old_vote) {
-      if (!idx($votes, $old_vote->getOptionID(), false)) {
+    $poll->openTransaction();
+      $poll->beginReadLocking();
+
+      $poll->reload();
+
+      $old_votes = id(new PhabricatorSlowvoteChoice())->loadAllWhere(
+        'pollID = %d AND authorPHID = %s',
+        $poll->getID(),
+        $viewer->getPHID());
+      $old_votes = mpull($old_votes, null, 'getOptionID');
+
+      foreach ($old_votes as $old_vote) {
+        if (idx($votes, $old_vote->getOptionID())) {
+          continue;
+        }
+
         $old_vote->delete();
       }
-    }
 
-    foreach ($votes as $vote) {
-      if (idx($old_votes, $vote, false)) {
-        continue;
+      foreach ($votes as $vote) {
+        if (idx($old_votes, $vote)) {
+          continue;
+        }
+
+        id(new PhabricatorSlowvoteChoice())
+          ->setAuthorPHID($viewer->getPHID())
+          ->setPollID($poll->getID())
+          ->setOptionID($vote)
+          ->save();
       }
 
-      id(new PhabricatorSlowvoteChoice())
-        ->setAuthorPHID($viewer->getPHID())
-        ->setPollID($poll->getID())
-        ->setOptionID($vote)
-        ->save();
-    }
+      $poll->endReadLocking();
+    $poll->saveTransaction();
 
     return id(new AphrontRedirectResponse())
       ->setURI($poll->getURI());

From e25dc2dfe283794222f284bce5c1247f3d3d1100 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 16:17:09 -0800
Subject: [PATCH 21/34] Revert "feed.http-hooks" HTTP request construction to
 use "http_build_query()" so nested "storyData" is handled correctly

Summary:
See <https://discourse.phabricator-community.org/t/storydata-is-blank-in-outgoing-requests-to-the-configured-feed-http-hooks/2366/>.

This behavior was changed by D20049. I think it's generally good that we not accept/encode nested values in a PHP-specific way, but retain `feed.http-hooks` compatibility for now.

Test Plan: {F6190681}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20114
---
 src/applications/feed/worker/FeedPublisherHTTPWorker.php | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/applications/feed/worker/FeedPublisherHTTPWorker.php b/src/applications/feed/worker/FeedPublisherHTTPWorker.php
index 4742e52a29..27a869ddef 100644
--- a/src/applications/feed/worker/FeedPublisherHTTPWorker.php
+++ b/src/applications/feed/worker/FeedPublisherHTTPWorker.php
@@ -26,6 +26,11 @@ final class FeedPublisherHTTPWorker extends FeedPushWorker {
       'epoch'           => $data->getEpoch(),
     );
 
+    // NOTE: We're explicitly using "http_build_query()" here because the
+    // "storyData" parameter may be a nested object with arbitrary nested
+    // sub-objects.
+    $post_data = http_build_query($post_data, '', '&');
+
     id(new HTTPSFuture($uri, $post_data))
       ->setMethod('POST')
       ->setTimeout(30)

From 4fa5a2421ea5e0296c40840ec785f5f9b9252beb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 16:28:50 -0800
Subject: [PATCH 22/34] Add formal setup guidance warning that
 "feed.http-hooks" will be removed in a future version of Phabricator

Summary: Depends on D20114. This is on the way out, so make that explicitly clear.

Test Plan: Read setup issue and configuration option.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20115
---
 .../PhabricatorExtraConfigSetupCheck.php      | 18 +++++++++++++
 .../config/PhabricatorFeedConfigOptions.php   | 26 +++++++++----------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
index 1c8a593a78..932e04db4b 100644
--- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
+++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
@@ -84,6 +84,24 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
         $issue->addPhabricatorConfig($key);
       }
     }
+
+
+    if (PhabricatorEnv::getEnvConfig('feed.http-hooks')) {
+      $this->newIssue('config.deprecated.feed.http-hooks')
+        ->setShortName(pht('Feed Hooks Deprecated'))
+        ->setName(pht('Migrate From "feed.http-hooks" to Webhooks'))
+        ->addPhabricatorConfig('feed.http-hooks')
+        ->setMessage(
+          pht(
+            'The "feed.http-hooks" option is deprecated in favor of '.
+            'Webhooks. This option will be removed in a future version '.
+            'of Phabricator.'.
+            "\n\n".
+            'You can configure Webhooks in Herald.'.
+            "\n\n".
+            'To resolve this issue, remove all URIs from "feed.http-hooks".'));
+    }
+
   }
 
   /**
diff --git a/src/applications/feed/config/PhabricatorFeedConfigOptions.php b/src/applications/feed/config/PhabricatorFeedConfigOptions.php
index 4b6612f931..29c5a9549b 100644
--- a/src/applications/feed/config/PhabricatorFeedConfigOptions.php
+++ b/src/applications/feed/config/PhabricatorFeedConfigOptions.php
@@ -20,22 +20,22 @@ final class PhabricatorFeedConfigOptions
   }
 
   public function getOptions() {
+    $hooks_help = $this->deformat(pht(<<<EODOC
+IMPORTANT: Feed hooks are deprecated and have been replaced by Webhooks.
+
+You can configure Webhooks in Herald. This configuration option will be removed
+in a future version of Phabricator.
+
+(This legacy option may be configured with a list of URIs; feed stories will
+send to these URIs.)
+EODOC
+      ));
+
     return array(
       $this->newOption('feed.http-hooks', 'list<string>', array())
         ->setLocked(true)
-        ->setSummary(pht('POST notifications of feed events.'))
-        ->setDescription(
-          pht(
-            "If you set this to a list of HTTP URIs, when a feed story is ".
-            "published a task will be created for each URI that posts the ".
-            "story data to the URI. Daemons automagically retry failures 100 ".
-            "times, waiting `\$fail_count * 60s` between each subsequent ".
-            "failure. Be sure to keep the daemon console (`%s`) open ".
-            "while developing and testing your end points. You may need to".
-            "restart your daemons to start sending HTTP requests.\n\n".
-            "NOTE: URIs are not validated, the URI must return HTTP status ".
-            "200 within 30 seconds, and no permission checks are performed.",
-            '/daemon/')),
+        ->setSummary(pht('Deprecated.'))
+        ->setDescription($hooks_help),
     );
   }
 

From 9f5e6bee903740eaf9423f6fd8915aa4f6bff66e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 19:36:41 -0800
Subject: [PATCH 23/34] Make the default behavior of
 getApplicationTransactionCommentObject() "return null" instead of "throw"

Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.

Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.

This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.

Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.

Test Plan:
  - Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
  - Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jbrownEP

Differential Revision: https://secure.phabricator.com/D20121
---
 .../almanac/storage/AlmanacModularTransaction.php        | 4 ----
 .../auth/storage/PhabricatorAuthPasswordTransaction.php  | 4 ----
 .../storage/PhabricatorAuthProviderConfigTransaction.php | 4 ----
 .../auth/storage/PhabricatorAuthSSHKeyTransaction.php    | 4 ----
 .../config/storage/PhabricatorConfigTransaction.php      | 4 ----
 .../diviner/storage/DivinerLiveBookTransaction.php       | 4 ----
 src/applications/fund/storage/FundBackerTransaction.php  | 4 ----
 src/applications/herald/action/HeraldCommentAction.php   | 9 +++------
 .../herald/storage/HeraldWebhookTransaction.php          | 4 ----
 .../PhabricatorMetaMTAApplicationEmailTransaction.php    | 4 ----
 .../storage/PhabricatorOAuthServerTransaction.php        | 4 ----
 .../storage/PhabricatorOwnersPackageTransaction.php      | 4 ----
 .../storage/PassphraseCredentialTransaction.php          | 4 ----
 .../people/storage/PhabricatorUserTransaction.php        | 4 ----
 src/applications/phame/storage/PhameBlogTransaction.php  | 4 ----
 src/applications/phlux/storage/PhluxTransaction.php      | 4 ----
 .../phortune/storage/PhortuneAccountTransaction.php      | 4 ----
 .../phortune/storage/PhortuneCartTransaction.php         | 4 ----
 .../phortune/storage/PhortuneMerchantTransaction.php     | 4 ----
 .../storage/PhortunePaymentProviderConfigTransaction.php | 4 ----
 .../project/storage/PhabricatorProjectTransaction.php    | 4 ----
 .../storage/PhabricatorRepositoryTransaction.php         | 4 ----
 .../PhabricatorFulltextIndexEngineExtension.php          | 8 ++------
 ...habricatorProfileMenuItemConfigurationTransaction.php | 4 ----
 .../storage/PhabricatorUserPreferencesTransaction.php    | 4 ----
 .../storage/PhabricatorSpacesNamespaceTransaction.php    | 4 ----
 .../editor/PhabricatorApplicationTransactionEditor.php   | 7 +------
 .../PhabricatorCommentEditEngineExtension.php            | 7 +------
 .../storage/PhabricatorApplicationTransaction.php        | 9 ++-------
 .../PhabricatorEditEngineConfigurationTransaction.php    | 4 ----
 30 files changed, 9 insertions(+), 131 deletions(-)

diff --git a/src/applications/almanac/storage/AlmanacModularTransaction.php b/src/applications/almanac/storage/AlmanacModularTransaction.php
index 6497069241..3e2eb0a3ec 100644
--- a/src/applications/almanac/storage/AlmanacModularTransaction.php
+++ b/src/applications/almanac/storage/AlmanacModularTransaction.php
@@ -7,8 +7,4 @@ abstract class AlmanacModularTransaction
     return 'almanac';
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
 }
diff --git a/src/applications/auth/storage/PhabricatorAuthPasswordTransaction.php b/src/applications/auth/storage/PhabricatorAuthPasswordTransaction.php
index 9d02112dff..a8cb6d10a3 100644
--- a/src/applications/auth/storage/PhabricatorAuthPasswordTransaction.php
+++ b/src/applications/auth/storage/PhabricatorAuthPasswordTransaction.php
@@ -11,10 +11,6 @@ final class PhabricatorAuthPasswordTransaction
     return PhabricatorAuthPasswordPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhabricatorAuthPasswordTransactionType';
   }
diff --git a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
index e1453b4383..d5a3588d59 100644
--- a/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
+++ b/src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
@@ -33,10 +33,6 @@ final class PhabricatorAuthProviderConfigTransaction
     return PhabricatorAuthAuthProviderPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getIcon() {
     $old = $this->getOldValue();
     $new = $this->getNewValue();
diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php
index bb08310cf3..028be1746d 100644
--- a/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php
+++ b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php
@@ -15,10 +15,6 @@ final class PhabricatorAuthSSHKeyTransaction
     return PhabricatorAuthSSHKeyPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
diff --git a/src/applications/config/storage/PhabricatorConfigTransaction.php b/src/applications/config/storage/PhabricatorConfigTransaction.php
index b7cfb6f495..94272bfb1a 100644
--- a/src/applications/config/storage/PhabricatorConfigTransaction.php
+++ b/src/applications/config/storage/PhabricatorConfigTransaction.php
@@ -13,10 +13,6 @@ final class PhabricatorConfigTransaction
     return PhabricatorConfigConfigPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
diff --git a/src/applications/diviner/storage/DivinerLiveBookTransaction.php b/src/applications/diviner/storage/DivinerLiveBookTransaction.php
index ae461e751a..f8eb81d1f3 100644
--- a/src/applications/diviner/storage/DivinerLiveBookTransaction.php
+++ b/src/applications/diviner/storage/DivinerLiveBookTransaction.php
@@ -11,8 +11,4 @@ final class DivinerLiveBookTransaction
     return DivinerBookPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
 }
diff --git a/src/applications/fund/storage/FundBackerTransaction.php b/src/applications/fund/storage/FundBackerTransaction.php
index c24e769eb6..c08958a29a 100644
--- a/src/applications/fund/storage/FundBackerTransaction.php
+++ b/src/applications/fund/storage/FundBackerTransaction.php
@@ -11,10 +11,6 @@ final class FundBackerTransaction
     return FundBackerPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'FundBackerTransactionType';
   }
diff --git a/src/applications/herald/action/HeraldCommentAction.php b/src/applications/herald/action/HeraldCommentAction.php
index fa52ba1f5f..f8b8fbe813 100644
--- a/src/applications/herald/action/HeraldCommentAction.php
+++ b/src/applications/herald/action/HeraldCommentAction.php
@@ -19,12 +19,9 @@ final class HeraldCommentAction extends HeraldAction {
     }
 
     $xaction = $object->getApplicationTransactionTemplate();
-    try {
-      $comment = $xaction->getApplicationTransactionCommentObject();
-      if (!$comment) {
-        return false;
-      }
-    } catch (PhutilMethodNotImplementedException $ex) {
+
+    $comment = $xaction->getApplicationTransactionCommentObject();
+    if (!$comment) {
       return false;
     }
 
diff --git a/src/applications/herald/storage/HeraldWebhookTransaction.php b/src/applications/herald/storage/HeraldWebhookTransaction.php
index 03c8cbb776..4f924cd4bb 100644
--- a/src/applications/herald/storage/HeraldWebhookTransaction.php
+++ b/src/applications/herald/storage/HeraldWebhookTransaction.php
@@ -11,10 +11,6 @@ final class HeraldWebhookTransaction
     return HeraldWebhookPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'HeraldWebhookTransactionType';
   }
diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmailTransaction.php b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmailTransaction.php
index 019adb338d..af6a6fbb88 100644
--- a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmailTransaction.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmailTransaction.php
@@ -16,8 +16,4 @@ final class PhabricatorMetaMTAApplicationEmailTransaction
     return PhabricatorMetaMTAApplicationEmailPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
 }
diff --git a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php
index b2624dd9a4..acfb88ef48 100644
--- a/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php
+++ b/src/applications/oauthserver/storage/PhabricatorOAuthServerTransaction.php
@@ -19,10 +19,6 @@ final class PhabricatorOAuthServerTransaction
     return PhabricatorOAuthServerClientPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
     $old = $this->getOldValue();
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php
index 1dfc944f63..66e15b634a 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php
@@ -15,8 +15,4 @@ final class PhabricatorOwnersPackageTransaction
     return 'PhabricatorOwnersPackageTransactionType';
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
 }
diff --git a/src/applications/passphrase/storage/PassphraseCredentialTransaction.php b/src/applications/passphrase/storage/PassphraseCredentialTransaction.php
index b7e4f904ef..bbc3b09668 100644
--- a/src/applications/passphrase/storage/PassphraseCredentialTransaction.php
+++ b/src/applications/passphrase/storage/PassphraseCredentialTransaction.php
@@ -11,10 +11,6 @@ final class PassphraseCredentialTransaction
     return PassphraseCredentialPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PassphraseCredentialTransactionType';
   }
diff --git a/src/applications/people/storage/PhabricatorUserTransaction.php b/src/applications/people/storage/PhabricatorUserTransaction.php
index 24edb2f5b5..81ca52a132 100644
--- a/src/applications/people/storage/PhabricatorUserTransaction.php
+++ b/src/applications/people/storage/PhabricatorUserTransaction.php
@@ -11,10 +11,6 @@ final class PhabricatorUserTransaction
     return PhabricatorPeopleUserPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhabricatorUserTransactionType';
   }
diff --git a/src/applications/phame/storage/PhameBlogTransaction.php b/src/applications/phame/storage/PhameBlogTransaction.php
index d3d6a79d0a..c605510d7d 100644
--- a/src/applications/phame/storage/PhameBlogTransaction.php
+++ b/src/applications/phame/storage/PhameBlogTransaction.php
@@ -15,10 +15,6 @@ final class PhameBlogTransaction
     return PhabricatorPhameBlogPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhameBlogTransactionType';
   }
diff --git a/src/applications/phlux/storage/PhluxTransaction.php b/src/applications/phlux/storage/PhluxTransaction.php
index 1224caf201..b1624d581a 100644
--- a/src/applications/phlux/storage/PhluxTransaction.php
+++ b/src/applications/phlux/storage/PhluxTransaction.php
@@ -13,10 +13,6 @@ final class PhluxTransaction extends PhabricatorApplicationTransaction {
     return PhluxVariablePHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
diff --git a/src/applications/phortune/storage/PhortuneAccountTransaction.php b/src/applications/phortune/storage/PhortuneAccountTransaction.php
index 6733cbe879..e333ef4a26 100644
--- a/src/applications/phortune/storage/PhortuneAccountTransaction.php
+++ b/src/applications/phortune/storage/PhortuneAccountTransaction.php
@@ -11,10 +11,6 @@ final class PhortuneAccountTransaction
     return PhortuneAccountPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhortuneAccountTransactionType';
   }
diff --git a/src/applications/phortune/storage/PhortuneCartTransaction.php b/src/applications/phortune/storage/PhortuneCartTransaction.php
index 41790011a2..c7a1e36e73 100644
--- a/src/applications/phortune/storage/PhortuneCartTransaction.php
+++ b/src/applications/phortune/storage/PhortuneCartTransaction.php
@@ -19,10 +19,6 @@ final class PhortuneCartTransaction
     return PhortuneCartPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function shouldHideForMail(array $xactions) {
     switch ($this->getTransactionType()) {
       case self::TYPE_CREATED:
diff --git a/src/applications/phortune/storage/PhortuneMerchantTransaction.php b/src/applications/phortune/storage/PhortuneMerchantTransaction.php
index 3befb12212..976259c534 100644
--- a/src/applications/phortune/storage/PhortuneMerchantTransaction.php
+++ b/src/applications/phortune/storage/PhortuneMerchantTransaction.php
@@ -11,10 +11,6 @@ final class PhortuneMerchantTransaction
     return PhortuneMerchantPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhortuneMerchantTransactionType';
   }
diff --git a/src/applications/phortune/storage/PhortunePaymentProviderConfigTransaction.php b/src/applications/phortune/storage/PhortunePaymentProviderConfigTransaction.php
index 08872d48fd..9241c7ae04 100644
--- a/src/applications/phortune/storage/PhortunePaymentProviderConfigTransaction.php
+++ b/src/applications/phortune/storage/PhortunePaymentProviderConfigTransaction.php
@@ -17,10 +17,6 @@ final class PhortunePaymentProviderConfigTransaction
     return PhortunePaymentProviderPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
diff --git a/src/applications/project/storage/PhabricatorProjectTransaction.php b/src/applications/project/storage/PhabricatorProjectTransaction.php
index a8b2bb0d4a..158c2480c0 100644
--- a/src/applications/project/storage/PhabricatorProjectTransaction.php
+++ b/src/applications/project/storage/PhabricatorProjectTransaction.php
@@ -19,10 +19,6 @@ final class PhabricatorProjectTransaction
     return PhabricatorProjectProjectPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhabricatorProjectTransactionType';
   }
diff --git a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php
index 85c354ba67..8729e462a3 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php
@@ -11,10 +11,6 @@ final class PhabricatorRepositoryTransaction
     return PhabricatorRepositoryRepositoryPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhabricatorRepositoryTransactionType';
   }
diff --git a/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php b/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php
index ab4da88420..126f298f1f 100644
--- a/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php
+++ b/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php
@@ -70,12 +70,8 @@ final class PhabricatorFulltextIndexEngineExtension
   private function getCommentVersion($object) {
     $xaction = $object->getApplicationTransactionTemplate();
 
-    try {
-      $comment = $xaction->getApplicationTransactionCommentObject();
-      if (!$comment) {
-        return 'none';
-      }
-    } catch (Exception $ex) {
+    $comment = $xaction->getApplicationTransactionCommentObject();
+    if (!$comment) {
       return 'none';
     }
 
diff --git a/src/applications/search/storage/PhabricatorProfileMenuItemConfigurationTransaction.php b/src/applications/search/storage/PhabricatorProfileMenuItemConfigurationTransaction.php
index b1d30a5b9d..4624d6f9af 100644
--- a/src/applications/search/storage/PhabricatorProfileMenuItemConfigurationTransaction.php
+++ b/src/applications/search/storage/PhabricatorProfileMenuItemConfigurationTransaction.php
@@ -20,8 +20,4 @@ final class PhabricatorProfileMenuItemConfigurationTransaction
     return PhabricatorProfileMenuItemPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
 }
diff --git a/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php
index 6378ee29d3..3ef48c01e0 100644
--- a/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php
+++ b/src/applications/settings/storage/PhabricatorUserPreferencesTransaction.php
@@ -11,10 +11,6 @@ final class PhabricatorUserPreferencesTransaction
     return 'user';
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getApplicationTransactionType() {
     return PhabricatorUserPreferencesPHIDType::TYPECONST;
   }
diff --git a/src/applications/spaces/storage/PhabricatorSpacesNamespaceTransaction.php b/src/applications/spaces/storage/PhabricatorSpacesNamespaceTransaction.php
index 0f50a870f6..bac0ea636f 100644
--- a/src/applications/spaces/storage/PhabricatorSpacesNamespaceTransaction.php
+++ b/src/applications/spaces/storage/PhabricatorSpacesNamespaceTransaction.php
@@ -11,10 +11,6 @@ final class PhabricatorSpacesNamespaceTransaction
     return PhabricatorSpacesNamespacePHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getBaseTransactionClass() {
     return 'PhabricatorSpacesNamespaceTransactionType';
   }
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 91825eb73d..216c1e00a2 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -360,12 +360,7 @@ abstract class PhabricatorApplicationTransactionEditor
     }
 
     if ($template) {
-      try {
-        $comment = $template->getApplicationTransactionCommentObject();
-      } catch (PhutilMethodNotImplementedException $ex) {
-        $comment = null;
-      }
-
+      $comment = $template->getApplicationTransactionCommentObject();
       if ($comment) {
         $types[] = PhabricatorTransactions::TYPE_COMMENT;
       }
diff --git a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php
index 0d20533798..7d80082cb2 100644
--- a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php
+++ b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php
@@ -23,12 +23,7 @@ final class PhabricatorCommentEditEngineExtension
     PhabricatorApplicationTransactionInterface $object) {
 
     $xaction = $object->getApplicationTransactionTemplate();
-
-    try {
-      $comment = $xaction->getApplicationTransactionCommentObject();
-    } catch (PhutilMethodNotImplementedException $ex) {
-      $comment = null;
-    }
+    $comment = $xaction->getApplicationTransactionCommentObject();
 
     return (bool)$comment;
   }
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 6d047fc823..acfc4d0cfa 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -76,7 +76,7 @@ abstract class PhabricatorApplicationTransaction
   }
 
   public function getApplicationTransactionCommentObject() {
-    throw new PhutilMethodNotImplementedException();
+    return null;
   }
 
   public function getMetadataValue($key, $default = null) {
@@ -1731,12 +1731,7 @@ abstract class PhabricatorApplicationTransaction
     PhabricatorDestructionEngine $engine) {
 
     $this->openTransaction();
-      $comment_template = null;
-      try {
-        $comment_template = $this->getApplicationTransactionCommentObject();
-      } catch (Exception $ex) {
-        // Continue; no comments for these transactions.
-      }
+      $comment_template = $this->getApplicationTransactionCommentObject();
 
       if ($comment_template) {
         $comments = $comment_template->loadAllWhere(
diff --git a/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php b/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
index a1c44cc003..8cf7fe5b48 100644
--- a/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorEditEngineConfigurationTransaction.php
@@ -23,10 +23,6 @@ final class PhabricatorEditEngineConfigurationTransaction
     return PhabricatorEditEngineConfigurationPHIDType::TYPECONST;
   }
 
-  public function getApplicationTransactionCommentObject() {
-    return null;
-  }
-
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 

From f0364eef8af2c3ce8a527bba677d343ed9e704bf Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 10:28:56 -0800
Subject: [PATCH 24/34] Remove weird integration between Legalpad and the
 ExternalAccount table

Summary:
Depends on D20107. Ref T6703. Legalpad currently inserts "email" records into the external account table, but they're never used for anything and nothing else references them.

They also aren't necessary for anything important to work, and the only effect they have is making the UI say "External Account" instead of "None" under the "Account" column. In particular, the signatures still record the actual email address.

Stop doing this, remove all the references, and destroy all the rows.

(Long ago, Maniphest may also have done this, but no longer does. Nuance/Gatekeeper use a more modern and more suitable "ExternalObject" thing that I initially started adapting here before realizing that Legalpad doesn't actually care about this data.)

Test Plan: Signed documents with an email address, saw signature reflected properly in UI. Grepped for other callsites.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20108
---
 .../20190206.external.01.legalpad.sql         |  2 ++
 .../20190206.external.02.email.sql            |  2 ++
 .../query/PhabricatorExternalAccountQuery.php | 31 -------------------
 .../LegalpadDocumentSignController.php        | 10 ------
 .../LegalpadDocumentSignatureSearchEngine.php |  2 +-
 5 files changed, 5 insertions(+), 42 deletions(-)
 create mode 100644 resources/sql/autopatches/20190206.external.01.legalpad.sql
 create mode 100644 resources/sql/autopatches/20190206.external.02.email.sql

diff --git a/resources/sql/autopatches/20190206.external.01.legalpad.sql b/resources/sql/autopatches/20190206.external.01.legalpad.sql
new file mode 100644
index 0000000000..8afa9dd9ff
--- /dev/null
+++ b/resources/sql/autopatches/20190206.external.01.legalpad.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_legalpad.legalpad_documentsignature
+  SET signerPHID = NULL WHERE signerPHID LIKE 'PHID-XUSR-%';
diff --git a/resources/sql/autopatches/20190206.external.02.email.sql b/resources/sql/autopatches/20190206.external.02.email.sql
new file mode 100644
index 0000000000..14f5f4791f
--- /dev/null
+++ b/resources/sql/autopatches/20190206.external.02.email.sql
@@ -0,0 +1,2 @@
+DELETE FROM {$NAMESPACE}_user.user_externalaccount
+  WHERE accountType = 'email';
diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
index b34199ce60..c4a53c12f8 100644
--- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php
+++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
@@ -168,35 +168,4 @@ final class PhabricatorExternalAccountQuery
     return 'PhabricatorPeopleApplication';
   }
 
-  /**
-   * Attempts to find an external account and if none exists creates a new
-   * external account with a shiny new ID and PHID.
-   *
-   * NOTE: This function assumes the first item in various query parameters is
-   * the correct value to use in creating a new external account.
-   */
-  public function loadOneOrCreate() {
-    $account = $this->executeOne();
-    if (!$account) {
-      $account = new PhabricatorExternalAccount();
-      if ($this->accountIDs) {
-        $account->setAccountID(reset($this->accountIDs));
-      }
-      if ($this->accountTypes) {
-        $account->setAccountType(reset($this->accountTypes));
-      }
-      if ($this->accountDomains) {
-        $account->setAccountDomain(reset($this->accountDomains));
-      }
-      if ($this->accountSecrets) {
-        $account->setAccountSecret(reset($this->accountSecrets));
-      }
-      if ($this->userPHIDs) {
-        $account->setUserPHID(reset($this->userPHIDs));
-      }
-      $account->save();
-    }
-    return $account;
-  }
-
 }
diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php
index ab98c0bb78..f09d95af29 100644
--- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php
+++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php
@@ -364,16 +364,6 @@ final class LegalpadDocumentSignController extends LegalpadController {
             if ($email_obj) {
               return $this->signInResponse();
             }
-            $external_account = id(new PhabricatorExternalAccountQuery())
-              ->setViewer($viewer)
-              ->withAccountTypes(array('email'))
-              ->withAccountDomains(array($email->getDomainName()))
-              ->withAccountIDs(array($email->getAddress()))
-              ->loadOneOrCreate();
-            if ($external_account->getUserPHID()) {
-              return $this->signInResponse();
-            }
-            $signer_phid = $external_account->getPHID();
           }
         }
         break;
diff --git a/src/applications/legalpad/query/LegalpadDocumentSignatureSearchEngine.php b/src/applications/legalpad/query/LegalpadDocumentSignatureSearchEngine.php
index 9df8d2478d..ea14fd4a2f 100644
--- a/src/applications/legalpad/query/LegalpadDocumentSignatureSearchEngine.php
+++ b/src/applications/legalpad/query/LegalpadDocumentSignatureSearchEngine.php
@@ -226,7 +226,7 @@ final class LegalpadDocumentSignatureSearchEngine
         $handles[$document->getPHID()]->renderLink(),
         $signer_phid
           ? $handles[$signer_phid]->renderLink()
-          : null,
+          : phutil_tag('em', array(), pht('None')),
         $name,
         phutil_tag(
           'a',

From 8054f7d19156d6a9c336a1f99a5f0e289bc4f3b1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 12:45:02 -0800
Subject: [PATCH 25/34] Convert a manual query against external accounts into a
 modern Query

Summary: Depends on D20108. Ref T6703. Update this outdated callsite to a more modern appraoch.

Test Plan: Destroyed a user with `bin/remove destroy`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20109
---
 src/applications/people/storage/PhabricatorUser.php | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 70d2d9fb4e..055df8b79e 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -1131,9 +1131,10 @@ final class PhabricatorUser
     $this->openTransaction();
       $this->delete();
 
-      $externals = id(new PhabricatorExternalAccount())->loadAllWhere(
-        'userPHID = %s',
-        $this->getPHID());
+      $externals = id(new PhabricatorExternalAccountQuery())
+        ->setViewer($engine->getViewer())
+        ->withUserPHIDs(array($this->getPHID()))
+        ->execute();
       foreach ($externals as $external) {
         $external->delete();
       }

From 949afb02fd19b376d219bb6be8539ec0f9329e99 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 20:53:25 -0800
Subject: [PATCH 26/34] On login forms, autofocus the "username" field

Summary: Depends on D20120. Fixes T8907. I thought this needed some Javascript nonsense but Safari, Firefox and Chrome all support an `autofocus` attribute.

Test Plan: Loaded login page with password auth enabled in Safari, Firefox, and Chrome; saw username field automatically gain focus.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8907

Differential Revision: https://secure.phabricator.com/D20122
---
 .../auth/provider/PhabricatorLDAPAuthProvider.php     |  1 +
 .../auth/provider/PhabricatorPasswordAuthProvider.php |  1 +
 src/view/form/control/AphrontFormTextControl.php      | 11 +++++++++++
 3 files changed, 13 insertions(+)

diff --git a/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php b/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
index 44b58b85ff..4a4babcc12 100644
--- a/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorLDAPAuthProvider.php
@@ -112,6 +112,7 @@ final class PhabricatorLDAPAuthProvider extends PhabricatorAuthProvider {
         id(new AphrontFormTextControl())
           ->setLabel(pht('LDAP Username'))
           ->setName('ldap_username')
+          ->setAutofocus(true)
           ->setValue($v_user)
           ->setError($e_user))
       ->appendChild(
diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
index d841f091aa..ec5720e078 100644
--- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
@@ -229,6 +229,7 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
         id(new AphrontFormTextControl())
           ->setLabel(pht('Username or Email'))
           ->setName('username')
+          ->setAutofocus(true)
           ->setValue($v_user)
           ->setError($e_user))
       ->appendChild(
diff --git a/src/view/form/control/AphrontFormTextControl.php b/src/view/form/control/AphrontFormTextControl.php
index 581f22682d..f7fd117cfd 100644
--- a/src/view/form/control/AphrontFormTextControl.php
+++ b/src/view/form/control/AphrontFormTextControl.php
@@ -5,6 +5,7 @@ final class AphrontFormTextControl extends AphrontFormControl {
   private $disableAutocomplete;
   private $sigil;
   private $placeholder;
+  private $autofocus;
 
   public function setDisableAutocomplete($disable) {
     $this->disableAutocomplete = $disable;
@@ -24,6 +25,15 @@ final class AphrontFormTextControl extends AphrontFormControl {
     return $this;
   }
 
+  public function setAutofocus($autofocus) {
+    $this->autofocus = $autofocus;
+    return $this;
+  }
+
+  public function getAutofocus() {
+    return $this->autofocus;
+  }
+
   public function getSigil() {
     return $this->sigil;
   }
@@ -49,6 +59,7 @@ final class AphrontFormTextControl extends AphrontFormControl {
         'id'           => $this->getID(),
         'sigil'        => $this->getSigil(),
         'placeholder'  => $this->getPlaceholder(),
+        'autofocus' => ($this->getAutofocus() ? 'autofocus' : null),
       ));
   }
 

From 7469075a8315c7709aad87ccec7cb284d4f71c6e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 07:06:16 -0800
Subject: [PATCH 27/34] Allow users to be approved from the profile "Manage"
 page, alongside other similar actions

Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.

Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.

Test Plan: {F6193742}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8029

Differential Revision: https://secure.phabricator.com/D20123
---
 resources/celerity/map.php                    |  6 +-
 .../PhabricatorPeopleApplication.php          |  3 +-
 .../PhabricatorPeopleApproveController.php    | 10 ++-
 .../PhabricatorPeopleProfileController.php    | 61 +++++++++++--------
 ...abricatorPeopleProfileManageController.php | 31 +++++++---
 src/view/layout/PhabricatorActionView.php     |  1 +
 webroot/rsrc/css/phui/phui-action-list.css    | 17 ++++++
 7 files changed, 93 insertions(+), 36 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 51bc9338d2..537d1f46dc 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
-    'core.pkg.css' => 'e0cb8094',
+    'core.pkg.css' => 'eab5ccaf',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -133,7 +133,7 @@ return array(
     'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '490e2e2e',
     'rsrc/css/phui/object-item/phui-oi-list-view.css' => '909f3844',
     'rsrc/css/phui/object-item/phui-oi-simple-ui.css' => '6a30fa46',
-    'rsrc/css/phui/phui-action-list.css' => 'c1a7631d',
+    'rsrc/css/phui/phui-action-list.css' => 'c4972757',
     'rsrc/css/phui/phui-action-panel.css' => '6c386cbf',
     'rsrc/css/phui/phui-badge.css' => '666e25ad',
     'rsrc/css/phui/phui-basic-nav-view.css' => '56ebd66d',
@@ -740,7 +740,7 @@ return array(
     'path-typeahead' => 'ad486db3',
     'people-picture-menu-item-css' => 'fe8e07cf',
     'people-profile-css' => '2ea2daa1',
-    'phabricator-action-list-view-css' => 'c1a7631d',
+    'phabricator-action-list-view-css' => 'c4972757',
     'phabricator-busy' => '5202e831',
     'phabricator-chatlog-css' => 'abdc76ee',
     'phabricator-content-source-view-css' => 'cdf0d579',
diff --git a/src/applications/people/application/PhabricatorPeopleApplication.php b/src/applications/people/application/PhabricatorPeopleApplication.php
index 9238d8da3b..06740be26e 100644
--- a/src/applications/people/application/PhabricatorPeopleApplication.php
+++ b/src/applications/people/application/PhabricatorPeopleApplication.php
@@ -51,7 +51,8 @@ final class PhabricatorPeopleApplication extends PhabricatorApplication {
           'send/'
             => 'PhabricatorPeopleInviteSendController',
         ),
-        'approve/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleApproveController',
+        'approve/(?P<id>[1-9]\d*)/(?:via/(?P<via>[^/]+)/)?'
+          => 'PhabricatorPeopleApproveController',
         '(?P<via>disapprove)/(?P<id>[1-9]\d*)/'
           => 'PhabricatorPeopleDisableController',
         '(?P<via>disable)/(?P<id>[1-9]\d*)/'
diff --git a/src/applications/people/controller/PhabricatorPeopleApproveController.php b/src/applications/people/controller/PhabricatorPeopleApproveController.php
index 013f4371f6..af08a6fbdc 100644
--- a/src/applications/people/controller/PhabricatorPeopleApproveController.php
+++ b/src/applications/people/controller/PhabricatorPeopleApproveController.php
@@ -14,7 +14,15 @@ final class PhabricatorPeopleApproveController
       return new Aphront404Response();
     }
 
-    $done_uri = $this->getApplicationURI('query/approval/');
+    $via = $request->getURIData('via');
+    switch ($via) {
+      case 'profile':
+        $done_uri = urisprintf('/people/manage/%d/', $user->getID());
+        break;
+      default:
+        $done_uri = $this->getApplicationURI('query/approval/');
+        break;
+    }
 
     if ($user->getIsApproved()) {
       return $this->newDialog()
diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php
index 902b21efcc..91afda123b 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php
@@ -70,40 +70,53 @@ abstract class PhabricatorPeopleProfileController
     $profile_icon = PhabricatorPeopleIconSet::getIconIcon($profile->getIcon());
     $profile_title = $profile->getDisplayTitle();
 
-    $roles = array();
+
+    $tag = id(new PHUITagView())
+      ->setType(PHUITagView::TYPE_SHADE);
+
+    $tags = array();
     if ($user->getIsAdmin()) {
-      $roles[] = pht('Administrator');
-    }
-    if ($user->getIsDisabled()) {
-      $roles[] = pht('Disabled');
-    }
-    if (!$user->getIsApproved()) {
-      $roles[] = pht('Not Approved');
-    }
-    if ($user->getIsSystemAgent()) {
-      $roles[] = pht('Bot');
-    }
-    if ($user->getIsMailingList()) {
-      $roles[] = pht('Mailing List');
-    }
-    if (!$user->getIsEmailVerified()) {
-      $roles[] = pht('Email Not Verified');
+      $tags[] = id(clone $tag)
+        ->setName(pht('Administrator'))
+        ->setColor('blue');
     }
 
-    $tag = null;
-    if ($roles) {
-      $tag = id(new PHUITagView())
-        ->setName(implode(', ', $roles))
-        ->addClass('project-view-header-tag')
-        ->setType(PHUITagView::TYPE_SHADE);
+    // "Disabled" gets a stronger status tag below.
+
+    if (!$user->getIsApproved()) {
+      $tags[] = id(clone $tag)
+        ->setName('Not Approved')
+        ->setColor('yellow');
+    }
+
+    if ($user->getIsSystemAgent()) {
+      $tags[] = id(clone $tag)
+        ->setName(pht('Bot'))
+        ->setColor('orange');
+    }
+
+    if ($user->getIsMailingList()) {
+      $tags[] = id(clone $tag)
+        ->setName(pht('Mailing List'))
+        ->setColor('orange');
+    }
+
+    if (!$user->getIsEmailVerified()) {
+      $tags[] = id(clone $tag)
+        ->setName(pht('Email Not Verified'))
+        ->setColor('violet');
     }
 
     $header = id(new PHUIHeaderView())
-      ->setHeader(array($user->getFullName(), $tag))
+      ->setHeader($user->getFullName())
       ->setImage($picture)
       ->setProfileHeader(true)
       ->addClass('people-profile-header');
 
+    foreach ($tags as $tag) {
+      $header->addTag($tag);
+    }
+
     require_celerity_resource('project-view-css');
 
     if ($user->getIsDisabled()) {
diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
index e9faae3d62..835935f775 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php
@@ -92,6 +92,8 @@ final class PhabricatorPeopleProfileManageController
       PeopleDisableUsersCapability::CAPABILITY);
     $can_disable = ($has_disable && !$is_self);
 
+    $id = $user->getID();
+
     $welcome_engine = id(new PhabricatorPeopleWelcomeMailEngine())
       ->setSender($viewer)
       ->setRecipient($user);
@@ -103,7 +105,7 @@ final class PhabricatorPeopleProfileManageController
       id(new PhabricatorActionView())
         ->setIcon('fa-pencil')
         ->setName(pht('Edit Profile'))
-        ->setHref($this->getApplicationURI('editprofile/'.$user->getID().'/'))
+        ->setHref($this->getApplicationURI('editprofile/'.$id.'/'))
         ->setDisabled(!$can_edit)
         ->setWorkflow(!$can_edit));
 
@@ -111,7 +113,7 @@ final class PhabricatorPeopleProfileManageController
       id(new PhabricatorActionView())
         ->setIcon('fa-picture-o')
         ->setName(pht('Edit Profile Picture'))
-        ->setHref($this->getApplicationURI('picture/'.$user->getID().'/'))
+        ->setHref($this->getApplicationURI('picture/'.$id.'/'))
         ->setDisabled(!$can_edit)
         ->setWorkflow(!$can_edit));
 
@@ -137,7 +139,7 @@ final class PhabricatorPeopleProfileManageController
         ->setName($empower_name)
         ->setDisabled(!$can_admin)
         ->setWorkflow(true)
-        ->setHref($this->getApplicationURI('empower/'.$user->getID().'/')));
+        ->setHref($this->getApplicationURI('empower/'.$id.'/')));
 
     $curtain->addAction(
       id(new PhabricatorActionView())
@@ -145,7 +147,7 @@ final class PhabricatorPeopleProfileManageController
         ->setName(pht('Change Username'))
         ->setDisabled(!$is_admin)
         ->setWorkflow(true)
-        ->setHref($this->getApplicationURI('rename/'.$user->getID().'/')));
+        ->setHref($this->getApplicationURI('rename/'.$id.'/')));
 
     if ($user->getIsDisabled()) {
       $disable_icon = 'fa-check-circle-o';
@@ -161,19 +163,34 @@ final class PhabricatorPeopleProfileManageController
         ->setName(pht('Send Welcome Email'))
         ->setWorkflow(true)
         ->setDisabled(!$can_welcome)
-        ->setHref($this->getApplicationURI('welcome/'.$user->getID().'/')));
+        ->setHref($this->getApplicationURI('welcome/'.$id.'/')));
 
     $curtain->addAction(
       id(new PhabricatorActionView())
         ->setType(PhabricatorActionView::TYPE_DIVIDER));
 
+    if (!$user->getIsApproved()) {
+      $approve_action = id(new PhabricatorActionView())
+        ->setIcon('fa-thumbs-up')
+        ->setName(pht('Approve User'))
+        ->setWorkflow(true)
+        ->setDisabled(!$is_admin)
+        ->setHref("/people/approve/{$id}/via/profile/");
+
+      if ($is_admin) {
+        $approve_action->setColor(PhabricatorActionView::GREEN);
+      }
+
+      $curtain->addAction($approve_action);
+    }
+
     $curtain->addAction(
       id(new PhabricatorActionView())
         ->setIcon($disable_icon)
         ->setName($disable_name)
         ->setDisabled(!$can_disable)
         ->setWorkflow(true)
-        ->setHref($this->getApplicationURI('disable/'.$user->getID().'/')));
+        ->setHref($this->getApplicationURI('disable/'.$id.'/')));
 
     $curtain->addAction(
       id(new PhabricatorActionView())
@@ -181,7 +198,7 @@ final class PhabricatorPeopleProfileManageController
         ->setName(pht('Delete User'))
         ->setDisabled(!$can_admin)
         ->setWorkflow(true)
-        ->setHref($this->getApplicationURI('delete/'.$user->getID().'/')));
+        ->setHref($this->getApplicationURI('delete/'.$id.'/')));
 
     $curtain->addAction(
       id(new PhabricatorActionView())
diff --git a/src/view/layout/PhabricatorActionView.php b/src/view/layout/PhabricatorActionView.php
index a1d8fe2664..3de60a2374 100644
--- a/src/view/layout/PhabricatorActionView.php
+++ b/src/view/layout/PhabricatorActionView.php
@@ -25,6 +25,7 @@ final class PhabricatorActionView extends AphrontView {
   const TYPE_DIVIDER  = 'type-divider';
   const TYPE_LABEL  = 'label';
   const RED = 'action-item-red';
+  const GREEN = 'action-item-green';
 
   public function setSelected($selected) {
     $this->selected = $selected;
diff --git a/webroot/rsrc/css/phui/phui-action-list.css b/webroot/rsrc/css/phui/phui-action-list.css
index e7ee38a8bf..3df4ff1b78 100644
--- a/webroot/rsrc/css/phui/phui-action-list.css
+++ b/webroot/rsrc/css/phui/phui-action-list.css
@@ -99,11 +99,20 @@
   background-color: {$sh-redbackground};
 }
 
+.phabricator-action-view.action-item-green {
+  background-color: {$sh-greenbackground};
+}
+
 .phabricator-action-view.action-item-red .phabricator-action-view-item,
 .phabricator-action-view.action-item-red .phabricator-action-view-icon {
   color: {$sh-redtext};
 }
 
+.phabricator-action-view.action-item-green .phabricator-action-view-item,
+.phabricator-action-view.action-item-green .phabricator-action-view-icon {
+  color: {$sh-greentext};
+}
+
 .device-desktop .phabricator-action-view.action-item-red:hover
   .phabricator-action-view-item,
 .device-desktop .phabricator-action-view.action-item-red:hover
@@ -111,6 +120,14 @@
   color: {$red};
 }
 
+.device-desktop .phabricator-action-view.action-item-green:hover
+  .phabricator-action-view-item,
+.device-desktop .phabricator-action-view.action-item-green:hover
+  .phabricator-action-view-icon {
+  color: {$green};
+}
+
+
 .phabricator-action-view-label .phabricator-action-view-item,
 .phabricator-action-view-type-label .phabricator-action-view-item {
   font-size: {$smallerfontsize};

From a4bab60ad0aeb002277b52dcd5b67c286819e3e1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 18:11:36 -0800
Subject: [PATCH 28/34] Don't show "registration might be too open" warnings
 unless an auth provider actually allows registration

Summary:
Depends on D20118. Fixes T5351. We possibly raise some warnings about registration (approval queue, email domains), but they aren't relevant if no one can register.

Hide these warnings if no providers actually support registration.

Test Plan: Viewed the Auth provider list with registration providers and with no registration providers, saw more tailored guidance.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5351

Differential Revision: https://secure.phabricator.com/D20119
---
 ...orAuthProvidersGuidanceEngineExtension.php | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/applications/auth/guidance/PhabricatorAuthProvidersGuidanceEngineExtension.php b/src/applications/auth/guidance/PhabricatorAuthProvidersGuidanceEngineExtension.php
index ac3fe4d309..d1f67393ca 100644
--- a/src/applications/auth/guidance/PhabricatorAuthProvidersGuidanceEngineExtension.php
+++ b/src/applications/auth/guidance/PhabricatorAuthProvidersGuidanceEngineExtension.php
@@ -10,6 +10,26 @@ final class PhabricatorAuthProvidersGuidanceEngineExtension
   }
 
   public function generateGuidance(PhabricatorGuidanceContext $context) {
+    $configs = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withIsEnabled(true)
+      ->execute();
+
+    $allows_registration = false;
+    foreach ($configs as $config) {
+      $provider = $config->getProvider();
+      if ($provider->shouldAllowRegistration()) {
+        $allows_registration = true;
+        break;
+      }
+    }
+
+    // If no provider allows registration, we don't need provide any warnings
+    // about registration being too open.
+    if (!$allows_registration) {
+      return array();
+    }
+
     $domains_key = 'auth.email-domains';
     $domains_link = $this->renderConfigLink($domains_key);
     $domains_value = PhabricatorEnv::getEnvConfig($domains_key);

From 509fbb6c20e24374506196ffa14bed143f0bc88f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 11:44:48 -0800
Subject: [PATCH 29/34] When building audit queries, prefilter possible
 "authorPHID" values

Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.

It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.

Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.

(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)

Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20129
---
 .../diffusion/query/DiffusionCommitQuery.php  | 51 ++++++++++++++++---
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 05072e07c7..03eb0b9edf 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -202,6 +202,7 @@ final class DiffusionCommitQuery
     $table = $this->newResultObject();
     $conn = $table->establishConnection('r');
 
+    $empty_exception = null;
     $subqueries = array();
     if ($this->responsiblePHIDs) {
       $base_authors = $this->authorPHIDs;
@@ -222,21 +223,33 @@ final class DiffusionCommitQuery
 
       $this->authorPHIDs = $all_authors;
       $this->auditorPHIDs = $base_auditors;
-      $subqueries[] = $this->buildStandardPageQuery(
-        $conn,
-        $table->getTableName());
+      try {
+        $subqueries[] = $this->buildStandardPageQuery(
+          $conn,
+          $table->getTableName());
+      } catch (PhabricatorEmptyQueryException $ex) {
+        $empty_exception = $ex;
+      }
 
       $this->authorPHIDs = $base_authors;
       $this->auditorPHIDs = $all_auditors;
-      $subqueries[] = $this->buildStandardPageQuery(
-        $conn,
-        $table->getTableName());
+      try {
+        $subqueries[] = $this->buildStandardPageQuery(
+          $conn,
+          $table->getTableName());
+      } catch (PhabricatorEmptyQueryException $ex) {
+        $empty_exception = $ex;
+      }
     } else {
       $subqueries[] = $this->buildStandardPageQuery(
         $conn,
         $table->getTableName());
     }
 
+    if (!$subqueries) {
+      throw $empty_exception;
+    }
+
     if (count($subqueries) > 1) {
       $unions = null;
       foreach ($subqueries as $subquery) {
@@ -642,10 +655,19 @@ final class DiffusionCommitQuery
     }
 
     if ($this->authorPHIDs !== null) {
+      $author_phids = $this->authorPHIDs;
+      if ($author_phids) {
+        $author_phids = $this->selectPossibleAuthors($author_phids);
+        if (!$author_phids) {
+          throw new PhabricatorEmptyQueryException(
+            pht('Author PHIDs contain no possible authors.'));
+        }
+      }
+
       $where[] = qsprintf(
         $conn,
         'commit.authorPHID IN (%Ls)',
-        $this->authorPHIDs);
+        $author_phids);
     }
 
     if ($this->epochMin !== null) {
@@ -934,5 +956,20 @@ final class DiffusionCommitQuery
     ) + $parent;
   }
 
+  private function selectPossibleAuthors(array $phids) {
+    // See PHI1057. Select PHIDs which might possibly be commit authors from
+    // a larger list of PHIDs. This primarily filters out packages and projects
+    // from "Responsible Users: ..." queries. Our goal in performing this
+    // filtering is to improve the performance of the final query.
+
+    foreach ($phids as $key => $phid) {
+      if (phid_get_type($phid) !== PhabricatorPeopleUserPHIDType::TYPECONST) {
+        unset($phids[$key]);
+      }
+    }
+
+    return $phids;
+  }
+
 
 }

From 8fab8d8a18ef301d780af44dc920cbcfc50ff083 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 08:00:43 -0800
Subject: [PATCH 30/34] Prepare owners package audit rules to become more
 flexible

Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.

PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.

Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.

Test Plan:
  - Created several packages, some with and some without auditing.
  - Inspected database for: package state; and associated transactions.
  - Ran the migrations.
  - Inspected database to confirm that state and transactions migrated correctly.
  - Reviewed transaction logs.
  - Created and edited packages and audit state.
  - Viewed the "Package List" element in Diffusion.
  - Pulled package information with `owners.search`, got sensible results.
  - Edited package audit status with `owners.edit`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20124
---
 .../20190207.packages.01.state.sql            |   2 +
 .../20190207.packages.02.migrate.sql          |   2 +
 .../autopatches/20190207.packages.03.drop.sql |   2 +
 .../20190207.packages.04.xactions.php         |  41 +++++++
 src/__phutil_library_map__.php                |   2 +
 .../controller/DiffusionBrowseController.php  |   7 +-
 .../constants/PhabricatorOwnersAuditRule.php  | 101 ++++++++++++++++++
 .../PhabricatorOwnersDetailController.php     |   8 +-
 .../PhabricatorOwnersPackageEditEngine.php    |   6 +-
 .../storage/PhabricatorOwnersPackage.php      |  28 ++---
 ...icatorOwnersPackageAuditingTransaction.php |  48 +++------
 ...habricatorRepositoryCommitOwnersWorker.php |   3 +-
 12 files changed, 181 insertions(+), 69 deletions(-)
 create mode 100644 resources/sql/autopatches/20190207.packages.01.state.sql
 create mode 100644 resources/sql/autopatches/20190207.packages.02.migrate.sql
 create mode 100644 resources/sql/autopatches/20190207.packages.03.drop.sql
 create mode 100644 resources/sql/autopatches/20190207.packages.04.xactions.php
 create mode 100644 src/applications/owners/constants/PhabricatorOwnersAuditRule.php

diff --git a/resources/sql/autopatches/20190207.packages.01.state.sql b/resources/sql/autopatches/20190207.packages.01.state.sql
new file mode 100644
index 0000000000..0e74f269ba
--- /dev/null
+++ b/resources/sql/autopatches/20190207.packages.01.state.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  ADD auditingState VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT};
diff --git a/resources/sql/autopatches/20190207.packages.02.migrate.sql b/resources/sql/autopatches/20190207.packages.02.migrate.sql
new file mode 100644
index 0000000000..60bf364ac1
--- /dev/null
+++ b/resources/sql/autopatches/20190207.packages.02.migrate.sql
@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_owners.owners_package
+  SET auditingState = IF(auditingEnabled = 0, 'none', 'audit');
diff --git a/resources/sql/autopatches/20190207.packages.03.drop.sql b/resources/sql/autopatches/20190207.packages.03.drop.sql
new file mode 100644
index 0000000000..24d0ce1a4f
--- /dev/null
+++ b/resources/sql/autopatches/20190207.packages.03.drop.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  DROP auditingEnabled;
diff --git a/resources/sql/autopatches/20190207.packages.04.xactions.php b/resources/sql/autopatches/20190207.packages.04.xactions.php
new file mode 100644
index 0000000000..5a8609166e
--- /dev/null
+++ b/resources/sql/autopatches/20190207.packages.04.xactions.php
@@ -0,0 +1,41 @@
+<?php
+
+$table = new PhabricatorOwnersPackageTransaction();
+$conn = $table->establishConnection('w');
+$iterator = new LiskRawMigrationIterator($conn, $table->getTableName());
+
+// Migrate "Auditing State" transactions for Owners Packages from old values
+// (which were "0" or "1", as JSON integer literals, without quotes) to new
+// values (which are JSON strings, with quotes).
+
+foreach ($iterator as $row) {
+  if ($row['transactionType'] !== 'owners.auditing') {
+    continue;
+  }
+
+  $old_value = (int)$row['oldValue'];
+  $new_value = (int)$row['newValue'];
+
+  if (!$old_value) {
+    $old_value = 'none';
+  } else {
+    $old_value = 'audit';
+  }
+
+  if (!$new_value) {
+    $new_value = 'none';
+  } else {
+    $new_value = 'audit';
+  }
+
+  $old_value = phutil_json_encode($old_value);
+  $new_value = phutil_json_encode($new_value);
+
+  queryfx(
+    $conn,
+    'UPDATE %R SET oldValue = %s, newValue = %s WHERE id = %d',
+    $table,
+    $old_value,
+    $new_value,
+    $row['id']);
+}
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index b9d468ea3c..4f9d7123c1 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3668,6 +3668,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php',
     'PhabricatorOwnersApplication' => 'applications/owners/application/PhabricatorOwnersApplication.php',
     'PhabricatorOwnersArchiveController' => 'applications/owners/controller/PhabricatorOwnersArchiveController.php',
+    'PhabricatorOwnersAuditRule' => 'applications/owners/constants/PhabricatorOwnersAuditRule.php',
     'PhabricatorOwnersConfigOptions' => 'applications/owners/config/PhabricatorOwnersConfigOptions.php',
     'PhabricatorOwnersConfiguredCustomField' => 'applications/owners/customfield/PhabricatorOwnersConfiguredCustomField.php',
     'PhabricatorOwnersController' => 'applications/owners/controller/PhabricatorOwnersController.php',
@@ -9613,6 +9614,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnerPathQuery' => 'Phobject',
     'PhabricatorOwnersApplication' => 'PhabricatorApplication',
     'PhabricatorOwnersArchiveController' => 'PhabricatorOwnersController',
+    'PhabricatorOwnersAuditRule' => 'Phobject',
     'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorOwnersConfiguredCustomField' => array(
       'PhabricatorOwnersCustomField',
diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 54be7dd7f1..6a863a4a92 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -566,11 +566,8 @@ final class DiffusionBrowseController extends DiffusionController {
         $name = idx($spec, 'name', $auto);
         $item->addIcon('fa-code', $name);
 
-        if ($package->getAuditingEnabled()) {
-          $item->addIcon('fa-check', pht('Auditing Enabled'));
-        } else {
-          $item->addIcon('fa-ban', pht('No Auditing'));
-        }
+        $rule = $package->newAuditingRule();
+        $item->addIcon($rule->getIconIcon(), $rule->getDisplayName());
 
         if ($package->isArchived()) {
           $item->setDisabled(true);
diff --git a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
new file mode 100644
index 0000000000..1f8fd0b1bd
--- /dev/null
+++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
@@ -0,0 +1,101 @@
+<?php
+
+final class PhabricatorOwnersAuditRule
+  extends Phobject {
+
+  const AUDITING_NONE = 'none';
+  const AUDITING_AUDIT = 'audit';
+
+  private $key;
+  private $spec;
+
+  public static function newFromState($key) {
+    $specs = self::newSpecifications();
+    $spec = idx($specs, $key, array());
+
+    $rule = new self();
+    $rule->key = $key;
+    $rule->spec = $spec;
+
+    return $rule;
+  }
+
+  public function getKey() {
+    return $this->key;
+  }
+
+  public function getDisplayName() {
+    return idx($this->spec, 'name', $this->key);
+  }
+
+  public function getIconIcon() {
+    return idx($this->spec, 'icon.icon');
+  }
+
+  public static function newSelectControlMap() {
+    $specs = self::newSpecifications();
+    return ipull($specs, 'name');
+  }
+
+  public static function getStorageValueFromAPIValue($value) {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $deprecated = idx($spec, 'deprecated', array());
+      if (isset($deprecated[$value])) {
+        return $key;
+      }
+    }
+
+    return $value;
+  }
+
+  public static function getModernValueMap() {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $map[$key] = pht('"%s"', $key);
+    }
+
+    return $map;
+  }
+
+  public static function getDeprecatedValueMap() {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $deprecated_map = idx($spec, 'deprecated', array());
+      foreach ($deprecated_map as $deprecated_key => $label) {
+        $map[$deprecated_key] = $label;
+      }
+    }
+
+    return $map;
+  }
+
+  private static function newSpecifications() {
+    return array(
+      self::AUDITING_NONE => array(
+        'name' => pht('No Auditing'),
+        'icon.icon' => 'fa-ban',
+        'deprecated' => array(
+          '' => pht('"" (empty string)'),
+          '0' => '"0"',
+        ),
+      ),
+      self::AUDITING_AUDIT => array(
+        'name' => pht('Audit Commits'),
+        'icon.icon' => 'fa-check',
+        'deprecated' => array(
+          '1' => '"1"',
+        ),
+      ),
+    );
+  }
+
+
+
+}
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index f71009cf19..e28ae2b3bb 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -194,12 +194,8 @@ final class PhabricatorOwnersDetailController
     $name = idx($spec, 'name', $auto);
     $view->addProperty(pht('Auto Review'), $name);
 
-    if ($package->getAuditingEnabled()) {
-      $auditing = pht('Enabled');
-    } else {
-      $auditing = pht('Disabled');
-    }
-    $view->addProperty(pht('Auditing'), $auditing);
+    $rule = $package->newAuditingRule();
+    $view->addProperty(pht('Auditing'), $rule->getDisplayName());
 
     $ignored = $package->getIgnoredPathAttributes();
     $ignored = array_keys($ignored);
diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
index c4ee026374..13f896d3f0 100644
--- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
@@ -141,11 +141,7 @@ EOTEXT
           PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
         ->setValue($object->getAuditingState())
-        ->setOptions(
-          array(
-            PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'),
-            PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'),
-          )),
+        ->setOptions(PhabricatorOwnersAuditRule::newSelectControlMap()),
       id(new PhabricatorRemarkupEditField())
         ->setKey('description')
         ->setLabel(pht('Description'))
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 207e0cb809..b9e91ef958 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -13,7 +13,6 @@ final class PhabricatorOwnersPackage
     PhabricatorNgramsInterface {
 
   protected $name;
-  protected $auditingEnabled;
   protected $autoReview;
   protected $description;
   protected $status;
@@ -21,6 +20,7 @@ final class PhabricatorOwnersPackage
   protected $editPolicy;
   protected $dominion;
   protected $properties = array();
+  protected $auditingState;
 
   private $paths = self::ATTACHABLE;
   private $owners = self::ATTACHABLE;
@@ -38,9 +38,6 @@ final class PhabricatorOwnersPackage
   const AUTOREVIEW_BLOCK = 'block';
   const AUTOREVIEW_BLOCK_ALWAYS = 'block-always';
 
-  const AUDITING_NONE = 'none';
-  const AUDITING_AUDIT = 'audit';
-
   const DOMINION_STRONG = 'strong';
   const DOMINION_WEAK = 'weak';
 
@@ -58,7 +55,7 @@ final class PhabricatorOwnersPackage
       PhabricatorOwnersDefaultEditCapability::CAPABILITY);
 
     return id(new PhabricatorOwnersPackage())
-      ->setAuditingEnabled(0)
+      ->setAuditingState(PhabricatorOwnersAuditRule::AUDITING_NONE)
       ->setAutoReview(self::AUTOREVIEW_NONE)
       ->setDominion(self::DOMINION_STRONG)
       ->setViewPolicy($view_policy)
@@ -129,7 +126,7 @@ final class PhabricatorOwnersPackage
       self::CONFIG_COLUMN_SCHEMA => array(
         'name' => 'sort',
         'description' => 'text',
-        'auditingEnabled' => 'bool',
+        'auditingState' => 'text32',
         'status' => 'text32',
         'autoReview' => 'text32',
         'dominion' => 'text32',
@@ -567,12 +564,8 @@ final class PhabricatorOwnersPackage
     return '/owners/package/'.$this->getID().'/';
   }
 
-  public function getAuditingState() {
-    if ($this->getAuditingEnabled()) {
-      return self::AUDITING_AUDIT;
-    } else {
-      return self::AUDITING_NONE;
-    }
+  public function newAuditingRule() {
+    return PhabricatorOwnersAuditRule::newFromState($this->getAuditingState());
   }
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
@@ -731,16 +724,11 @@ final class PhabricatorOwnersPackage
       'label' => $review_label,
     );
 
-    $audit_value = $this->getAuditingState();
-    if ($this->getAuditingEnabled()) {
-      $audit_label = pht('Auditing Enabled');
-    } else {
-      $audit_label = pht('No Auditing');
-    }
+    $audit_rule = $this->newAuditingRule();
 
     $audit = array(
-      'value' => $audit_value,
-      'label' => $audit_label,
+      'value' => $audit_rule->getKey(),
+      'label' => $audit_rule->getDisplayName(),
     );
 
     $dominion_value = $this->getDominion();
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
index d7ea7093f9..7c16c850fd 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
@@ -6,35 +6,29 @@ final class PhabricatorOwnersPackageAuditingTransaction
   const TRANSACTIONTYPE = 'owners.auditing';
 
   public function generateOldValue($object) {
-    return (int)$object->getAuditingEnabled();
+    return $object->getAuditingState();
   }
 
   public function generateNewValue($object, $value) {
-    switch ($value) {
-      case PhabricatorOwnersPackage::AUDITING_AUDIT:
-        return 1;
-      case '1':
-        // TODO: Remove, deprecated.
-        return 1;
-      default:
-        return 0;
-    }
+    return PhabricatorOwnersAuditRule::getStorageValueFromAPIValue($value);
   }
 
   public function applyInternalEffects($object, $value) {
-    $object->setAuditingEnabled($value);
+    $object->setAuditingState($value);
   }
 
   public function getTitle() {
-    if ($this->getNewValue()) {
-      return pht(
-        '%s enabled auditing for this package.',
-        $this->renderAuthor());
-    } else {
-      return pht(
-        '%s disabled auditing for this package.',
-        $this->renderAuthor());
-    }
+    $old_value = $this->getOldValue();
+    $new_value = $this->getNewValue();
+
+    $old_rule = PhabricatorOwnersAuditRule::newFromState($old_value);
+    $new_rule = PhabricatorOwnersAuditRule::newFromState($new_value);
+
+    return pht(
+      '%s changed the audit rule for this package from %s to %s.',
+      $this->renderAuthor(),
+      $this->renderValue($old_rule->getDisplayName()),
+      $this->renderValue($new_rule->getDisplayName()));
   }
 
   public function validateTransactions($object, array $xactions) {
@@ -43,18 +37,8 @@ final class PhabricatorOwnersPackageAuditingTransaction
     // See PHI1047. This transaction type accepted some weird stuff. Continue
     // supporting it for now, but move toward sensible consistency.
 
-    $modern_options = array(
-      PhabricatorOwnersPackage::AUDITING_NONE =>
-        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_NONE),
-      PhabricatorOwnersPackage::AUDITING_AUDIT =>
-        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_AUDIT),
-    );
-
-    $deprecated_options = array(
-      '0' => '"0"',
-      '1' => '"1"',
-      '' => pht('"" (empty string)'),
-    );
+    $modern_options = PhabricatorOwnersAuditRule::getModernValueMap();
+    $deprecated_options = PhabricatorOwnersAuditRule::getDeprecatedValueMap();
 
     foreach ($xactions as $xaction) {
       $new_value = $xaction->getNewValue();
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 75ae0c9c14..219314c9d5 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -133,7 +133,8 @@ final class PhabricatorRepositoryCommitOwnersWorker
     $revision) {
 
     // Don't trigger an audit if auditing isn't enabled for the package.
-    if (!$package->getAuditingEnabled()) {
+    $rule = $package->newAuditingRule();
+    if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) {
       return false;
     }
 

From 31a0ed92d08d52adf88791e3f01da3b65121294f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 09:16:22 -0800
Subject: [PATCH 31/34] Support a wider range of "Audit" rules for Owners
 packages

Summary:
Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.

(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)

Test Plan:
  - Edited a package to select the various different audit rules.
  - Used `bin/repository reparse --force --owners <commit>` to trigger package audits under varied conditions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20126
---
 .../constants/PhabricatorOwnersAuditRule.php  | 22 ++++-
 ...habricatorRepositoryCommitOwnersWorker.php | 86 +++++++++++++++----
 src/docs/user/userguide/owners.diviner        | 16 ++--
 3 files changed, 100 insertions(+), 24 deletions(-)

diff --git a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
index 1f8fd0b1bd..32a9bb804b 100644
--- a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
+++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php
@@ -4,7 +4,10 @@ final class PhabricatorOwnersAuditRule
   extends Phobject {
 
   const AUDITING_NONE = 'none';
-  const AUDITING_AUDIT = 'audit';
+  const AUDITING_NO_OWNER = 'audit';
+  const AUDITING_UNREVIEWED = 'unreviewed';
+  const AUDITING_NO_OWNER_AND_UNREVIEWED = 'uninvolved-unreviewed';
+  const AUDITING_ALL = 'all';
 
   private $key;
   private $spec;
@@ -86,13 +89,26 @@ final class PhabricatorOwnersAuditRule
           '0' => '"0"',
         ),
       ),
-      self::AUDITING_AUDIT => array(
-        'name' => pht('Audit Commits'),
+      self::AUDITING_UNREVIEWED => array(
+        'name' => pht('Audit Unreviewed Commits'),
+        'icon.icon' => 'fa-check',
+      ),
+      self::AUDITING_NO_OWNER => array(
+        'name' => pht('Audit Commits With No Owner Involvement'),
         'icon.icon' => 'fa-check',
         'deprecated' => array(
           '1' => '"1"',
         ),
       ),
+      self::AUDITING_NO_OWNER_AND_UNREVIEWED => array(
+        'name' => pht(
+          'Audit Unreviewed Commits and Commits With No Owner Involvement'),
+        'icon.icon' => 'fa-check',
+      ),
+      self::AUDITING_ALL => array(
+        'name' => pht('Audit All Commits'),
+        'icon.icon' => 'fa-check',
+      ),
     );
   }
 
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 219314c9d5..65434658ab 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -132,29 +132,85 @@ final class PhabricatorRepositoryCommitOwnersWorker
     $author_phid,
     $revision) {
 
-    // Don't trigger an audit if auditing isn't enabled for the package.
+    $audit_uninvolved = false;
+    $audit_unreviewed = false;
+
     $rule = $package->newAuditingRule();
-    if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) {
-      return false;
+    switch ($rule->getKey()) {
+      case PhabricatorOwnersAuditRule::AUDITING_NONE:
+        return false;
+      case PhabricatorOwnersAuditRule::AUDITING_ALL:
+        return true;
+      case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER:
+        $audit_uninvolved = true;
+        break;
+      case PhabricatorOwnersAuditRule::AUDITING_UNREVIEWED:
+        $audit_unreviewed = true;
+        break;
+      case PhabricatorOwnersAuditRule::AUDITING_NO_OWNER_AND_UNREVIEWED:
+        $audit_uninvolved = true;
+        $audit_unreviewed = true;
+        break;
     }
 
-    // Trigger an audit if we don't recognize the commit's author.
-    if (!$author_phid) {
-      return true;
+    // If auditing is configured to trigger on unreviewed changes, check if
+    // the revision was "Accepted" when it landed. If not, trigger an audit.
+    if ($audit_unreviewed) {
+      $commit_unreviewed = true;
+      if ($revision) {
+        $was_accepted = DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED;
+        if ($revision->isPublished()) {
+          if ($revision->getProperty($was_accepted)) {
+            $commit_unreviewed = false;
+          }
+        }
+      }
+
+      if ($commit_unreviewed) {
+        return true;
+      }
     }
 
+    // If auditing is configured to trigger on changes with no involved owner,
+    // check for an owner. If we don't find one, trigger an audit.
+    if ($audit_uninvolved) {
+      $commit_uninvolved = $this->isOwnerInvolved(
+        $commit,
+        $package,
+        $author_phid,
+        $revision);
+      if ($commit_uninvolved) {
+        return true;
+      }
+    }
+
+    // We can't find any reason to trigger an audit for this commit.
+    return false;
+  }
+
+  private function isOwnerInvolved(
+    PhabricatorRepositoryCommit $commit,
+    PhabricatorOwnersPackage $package,
+    $author_phid,
+    $revision) {
+
     $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
       array(
         $package->getID(),
       ));
     $owner_phids = array_fuse($owner_phids);
 
-    // Don't trigger an audit if the author is a package owner.
-    if (isset($owner_phids[$author_phid])) {
-      return false;
+    // If the commit author is identifiable and a package owner, they're
+    // involved.
+    if ($author_phid) {
+      if (isset($owner_phids[$author_phid])) {
+        return true;
+      }
     }
 
-    // Trigger an audit of there is no corresponding revision.
+    // Otherwise, we need to find an owner as a reviewer.
+
+    // If we don't have a revision, this is hopeless: no owners are involved.
     if (!$revision) {
       return true;
     }
@@ -174,21 +230,19 @@ final class PhabricatorRepositoryCommitOwnersWorker
         continue;
       }
 
-      // If this reviewer accepted the revision and owns the package, we're
-      // all clear and do not need to trigger an audit.
+      // If this reviewer accepted the revision and owns the package, we've
+      // found an involved owner.
       if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) {
         $found_accept = true;
         break;
       }
     }
 
-    // Don't trigger an audit if a package owner already reviewed the
-    // revision.
     if ($found_accept) {
-      return false;
+      return true;
     }
 
-    return true;
+    return false;
   }
 
 }
diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner
index 95a3882552..df74fc6e83 100644
--- a/src/docs/user/userguide/owners.diviner
+++ b/src/docs/user/userguide/owners.diviner
@@ -114,16 +114,22 @@ Auditing
 ========
 
 You can automatically trigger audits on unreviewed code by configuring
-**Auditing**. The available settings are:
+**Auditing**. The available settings allow you to select behavior based on
+these conditions:
 
-  - **Disabled**: Do not trigger audits.
-  - **Enabled**: Trigger audits.
+  - **No Owner Involvement**: Triggers an audit when the commit author is not
+    a package owner, and no package owner reviewed an associated revision in
+    Differential.
+  - **Unreviewed Commits**: Triggers an audit when a commit has no associated
+    revision in Differential, or the associated revision in Differential landed
+    without being "Accepted".
 
-When enabled, audits are triggered for commits which:
+For example, the **Audit Commits With No Owner Involvement** option triggers
+audits for commits which:
 
   - affect code owned by the package;
   - were not authored by a package owner; and
-  - were not accepted by a package owner.
+  - were not accepted (in Differential) by a package owner.
 
 Audits do not trigger if the package has been archived.
 

From ae54af32c145fec2c4628f360f169d2f6395939f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 13:41:34 -0800
Subject: [PATCH 32/34] When an Owners package accepts a revision, count that
 as an "involved owner" for the purposes of audit

Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.

If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.

Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20130
---
 .../PhabricatorRepositoryCommitOwnersWorker.php     | 13 ++++++++++---
 src/docs/user/userguide/owners.diviner              |  3 ++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 65434658ab..b0c60667a4 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -200,6 +200,12 @@ final class PhabricatorRepositoryCommitOwnersWorker
       ));
     $owner_phids = array_fuse($owner_phids);
 
+    // For the purposes of deciding whether the owners were involved in the
+    // revision or not, consider a review by the package itself to count as
+    // involvement. This can happen when human reviewers force-accept on
+    // behalf of packages they don't own but have authority over.
+    $owner_phids[$package->getPHID()] = $package->getPHID();
+
     // If the commit author is identifiable and a package owner, they're
     // involved.
     if ($author_phid) {
@@ -225,13 +231,14 @@ final class PhabricatorRepositoryCommitOwnersWorker
     foreach ($revision->getReviewers() as $reviewer) {
       $reviewer_phid = $reviewer->getReviewerPHID();
 
-      // If this reviewer isn't a package owner, just ignore them.
+      // If this reviewer isn't a package owner or the package itself,
+      // just ignore them.
       if (empty($owner_phids[$reviewer_phid])) {
         continue;
       }
 
-      // If this reviewer accepted the revision and owns the package, we've
-      // found an involved owner.
+      // If this reviewer accepted the revision and owns the package (or is
+      // the package), we've found an involved owner.
       if (isset($accepted_statuses[$reviewer->getReviewerStatus()])) {
         $found_accept = true;
         break;
diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner
index df74fc6e83..11dee8941a 100644
--- a/src/docs/user/userguide/owners.diviner
+++ b/src/docs/user/userguide/owners.diviner
@@ -129,7 +129,8 @@ audits for commits which:
 
   - affect code owned by the package;
   - were not authored by a package owner; and
-  - were not accepted (in Differential) by a package owner.
+  - were not accepted (in Differential) by a package owner or the package
+    itself.
 
 Audits do not trigger if the package has been archived.
 

From 2b718d78bba24f0636842a9281d32fe1d09cc84d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 8 Feb 2019 07:12:39 -0800
Subject: [PATCH 33/34] Improve UI/UX when users try to add an invalid card
 with Stripe

Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.

Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):

{F6197394}

After:

{F6197397}

- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20132
---
 resources/celerity/map.php                    |  6 +-
 src/__phutil_library_map__.php                |  2 +
 src/applications/fund/storage/FundBacker.php  |  3 +
 .../PhortunePaymentMethodCreateController.php | 36 ++++---
 .../exception/PhortuneDisplayException.php    | 15 +++
 .../PhortuneStripePaymentProvider.php         | 94 ++++++++++++++++++-
 .../policy/filter/PhabricatorPolicyFilter.php |  7 +-
 webroot/rsrc/css/aphront/table-view.css       | 16 ++--
 8 files changed, 149 insertions(+), 30 deletions(-)
 create mode 100644 src/applications/phortune/exception/PhortuneDisplayException.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 537d1f46dc..8312828c6e 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
-    'core.pkg.css' => 'eab5ccaf',
+    'core.pkg.css' => '08baca0c',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -30,7 +30,7 @@ return array(
     'rsrc/css/aphront/notification.css' => '30240bd2',
     'rsrc/css/aphront/panel-view.css' => '46923d46',
     'rsrc/css/aphront/phabricator-nav-view.css' => 'f8a0c1bf',
-    'rsrc/css/aphront/table-view.css' => '76eda3f8',
+    'rsrc/css/aphront/table-view.css' => 'daa1f9df',
     'rsrc/css/aphront/tokenizer.css' => 'b52d0668',
     'rsrc/css/aphront/tooltip.css' => 'e3f2412f',
     'rsrc/css/aphront/typeahead-browse.css' => 'b7ed02d2',
@@ -519,7 +519,7 @@ return array(
     'aphront-list-filter-view-css' => 'feb64255',
     'aphront-multi-column-view-css' => 'fbc00ba3',
     'aphront-panel-view-css' => '46923d46',
-    'aphront-table-view-css' => '76eda3f8',
+    'aphront-table-view-css' => 'daa1f9df',
     'aphront-tokenizer-control-css' => 'b52d0668',
     'aphront-tooltip-css' => 'e3f2412f',
     'aphront-typeahead-control-css' => '8779483d',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 4f9d7123c1..b704a7cc2e 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -5015,6 +5015,7 @@ phutil_register_library_map(array(
     'PhortuneCurrencySerializer' => 'applications/phortune/currency/PhortuneCurrencySerializer.php',
     'PhortuneCurrencyTestCase' => 'applications/phortune/currency/__tests__/PhortuneCurrencyTestCase.php',
     'PhortuneDAO' => 'applications/phortune/storage/PhortuneDAO.php',
+    'PhortuneDisplayException' => 'applications/phortune/exception/PhortuneDisplayException.php',
     'PhortuneErrCode' => 'applications/phortune/constants/PhortuneErrCode.php',
     'PhortuneInvoiceView' => 'applications/phortune/view/PhortuneInvoiceView.php',
     'PhortuneLandingController' => 'applications/phortune/controller/PhortuneLandingController.php',
@@ -11263,6 +11264,7 @@ phutil_register_library_map(array(
     'PhortuneCurrencySerializer' => 'PhabricatorLiskSerializer',
     'PhortuneCurrencyTestCase' => 'PhabricatorTestCase',
     'PhortuneDAO' => 'PhabricatorLiskDAO',
+    'PhortuneDisplayException' => 'Exception',
     'PhortuneErrCode' => 'PhortuneConstants',
     'PhortuneInvoiceView' => 'AphrontTagView',
     'PhortuneLandingController' => 'PhortuneController',
diff --git a/src/applications/fund/storage/FundBacker.php b/src/applications/fund/storage/FundBacker.php
index 87ab342e2a..ebdf39ae17 100644
--- a/src/applications/fund/storage/FundBacker.php
+++ b/src/applications/fund/storage/FundBacker.php
@@ -76,6 +76,7 @@ final class FundBacker extends FundDAO
   public function getCapabilities() {
     return array(
       PhabricatorPolicyCapability::CAN_VIEW,
+      PhabricatorPolicyCapability::CAN_EDIT,
     );
   }
 
@@ -91,6 +92,8 @@ final class FundBacker extends FundDAO
           return $initiative->getPolicy($capability);
         }
         return PhabricatorPolicies::POLICY_NOONE;
+      case PhabricatorPolicyCapability::CAN_EDIT:
+        return PhabricatorPolicies::POLICY_NOONE;
     }
   }
 
diff --git a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
index 87bddd4d33..521508e0e8 100644
--- a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
+++ b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
@@ -73,6 +73,7 @@ final class PhortunePaymentMethodCreateController
     $provider = $providers[$provider_id];
 
     $errors = array();
+    $display_exception = null;
     if ($request->isFormPost() && $request->getBool('isProviderForm')) {
       $method = id(new PhortunePaymentMethod())
         ->setAccountPHID($account->getPHID())
@@ -107,14 +108,23 @@ final class PhortunePaymentMethodCreateController
         }
 
         if (!$errors) {
-          $errors = $provider->createPaymentMethodFromRequest(
-            $request,
-            $method,
-            $client_token);
+          try {
+            $provider->createPaymentMethodFromRequest(
+              $request,
+              $method,
+              $client_token);
+          } catch (PhortuneDisplayException $exception) {
+            $display_exception = $exception;
+          } catch (Exception $ex) {
+            $errors = array(
+              pht('There was an error adding this payment method:'),
+              $ex->getMessage(),
+            );
+          }
         }
       }
 
-      if (!$errors) {
+      if (!$errors && !$display_exception) {
         $method->save();
 
         // If we added this method on a cart flow, return to the cart to
@@ -133,13 +143,17 @@ final class PhortunePaymentMethodCreateController
 
         return id(new AphrontRedirectResponse())->setURI($next_uri);
       } else {
-        $dialog = id(new AphrontDialogView())
-          ->setUser($viewer)
-          ->setTitle(pht('Error Adding Payment Method'))
-          ->appendChild(id(new PHUIInfoView())->setErrors($errors))
-          ->addCancelButton($request->getRequestURI());
+        if ($display_exception) {
+          $dialog_body = $display_exception->getView();
+        } else {
+          $dialog_body = id(new PHUIInfoView())
+            ->setErrors($errors);
+        }
 
-        return id(new AphrontDialogResponse())->setDialog($dialog);
+        return $this->newDialog()
+          ->setTitle(pht('Error Adding Payment Method'))
+          ->appendChild($dialog_body)
+          ->addCancelButton($request->getRequestURI());
       }
     }
 
diff --git a/src/applications/phortune/exception/PhortuneDisplayException.php b/src/applications/phortune/exception/PhortuneDisplayException.php
new file mode 100644
index 0000000000..7b2bbf6875
--- /dev/null
+++ b/src/applications/phortune/exception/PhortuneDisplayException.php
@@ -0,0 +1,15 @@
+<?php
+
+final class PhortuneDisplayException
+  extends Exception {
+
+  public function setView($view) {
+    $this->view = $view;
+    return $this;
+  }
+
+  public function getView() {
+    return $this->view;
+  }
+
+}
diff --git a/src/applications/phortune/provider/PhortuneStripePaymentProvider.php b/src/applications/phortune/provider/PhortuneStripePaymentProvider.php
index bdaa4294b2..0463881016 100644
--- a/src/applications/phortune/provider/PhortuneStripePaymentProvider.php
+++ b/src/applications/phortune/provider/PhortuneStripePaymentProvider.php
@@ -233,8 +233,6 @@ final class PhortuneStripePaymentProvider extends PhortunePaymentProvider {
     array $token) {
     $this->loadStripeAPILibraries();
 
-    $errors = array();
-
     $secret_key = $this->getSecretKey();
     $stripe_token = $token['stripeCardToken'];
 
@@ -253,7 +251,15 @@ final class PhortuneStripePaymentProvider extends PhortunePaymentProvider {
     // the card more than once. We create one Customer for each card;
     // they do not map to PhortuneAccounts because we allow an account to
     // have more than one active card.
-    $customer = Stripe_Customer::create($params, $secret_key);
+    try {
+      $customer = Stripe_Customer::create($params, $secret_key);
+    } catch (Stripe_CardError $ex) {
+      $display_exception = $this->newDisplayExceptionFromCardError($ex);
+      if ($display_exception) {
+        throw $display_exception;
+      }
+      throw $ex;
+    }
 
     $card = $info->card;
 
@@ -267,8 +273,6 @@ final class PhortuneStripePaymentProvider extends PhortunePaymentProvider {
           'stripe.customerID' => $customer->id,
           'stripe.cardToken' => $stripe_token,
         ));
-
-    return $errors;
   }
 
   public function renderCreatePaymentMethodForm(
@@ -383,4 +387,84 @@ final class PhortuneStripePaymentProvider extends PhortunePaymentProvider {
     require_once $root.'/externals/stripe-php/lib/Stripe.php';
   }
 
+
+  private function newDisplayExceptionFromCardError(Stripe_CardError $ex) {
+    $body = $ex->getJSONBody();
+    if (!$body) {
+      return null;
+    }
+
+    $map = idx($body, 'error');
+    if (!$map) {
+      return null;
+    }
+
+    $view = array();
+
+    $message = idx($map, 'message');
+
+    $view[] = id(new PHUIInfoView())
+      ->setErrors(array($message));
+
+    $view[] = phutil_tag(
+      'div',
+      array(
+        'class' => 'mlt mlb',
+      ),
+      pht('Additional details about this error:'));
+
+    $rows = array();
+
+    $rows[] = array(
+      pht('Error Code'),
+      idx($map, 'code'),
+    );
+
+    $rows[] = array(
+      pht('Error Type'),
+      idx($map, 'type'),
+    );
+
+    $param = idx($map, 'param');
+    if (strlen($param)) {
+      $rows[] = array(
+        pht('Error Param'),
+        $param,
+      );
+    }
+
+    $decline_code = idx($map, 'decline_code');
+    if (strlen($decline_code)) {
+      $rows[] = array(
+        pht('Decline Code'),
+        $decline_code,
+      );
+    }
+
+    $doc_url = idx($map, 'doc_url');
+    if ($doc_url) {
+      $rows[] = array(
+        pht('Learn More'),
+        phutil_tag(
+          'a',
+          array(
+            'href' => $doc_url,
+            'target' => '_blank',
+          ),
+          $doc_url),
+      );
+    }
+
+    $view[] = id(new AphrontTableView($rows))
+      ->setColumnClasses(
+        array(
+          'header',
+          'wide',
+        ));
+
+    return id(new PhortuneDisplayException(get_class($ex)))
+      ->setView($view);
+  }
+
+
 }
diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php
index fb03936ec2..a5c9f356f4 100644
--- a/src/applications/policy/filter/PhabricatorPolicyFilter.php
+++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -175,9 +175,10 @@ final class PhabricatorPolicyFilter extends Phobject {
         if (!in_array($capability, $object_capabilities)) {
           throw new Exception(
             pht(
-              "Testing for capability '%s' on an object which does ".
-              "not have that capability!",
-              $capability));
+              'Testing for capability "%s" on an object ("%s") which does '.
+              'not support that capability.',
+              $capability,
+              get_class($object)));
         }
 
         $policy = $this->getObjectPolicy($object, $capability);
diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css
index 1a7d2eb215..9bc8536054 100644
--- a/webroot/rsrc/css/aphront/table-view.css
+++ b/webroot/rsrc/css/aphront/table-view.css
@@ -45,16 +45,20 @@
   background: inherit;
 }
 
-.aphront-table-view th {
+.aphront-table-view th,
+.aphront-table-view td.header {
   font-weight: bold;
   white-space: nowrap;
   color: {$bluetext};
-  text-shadow: 0 1px 0 white;
   font-weight: bold;
-  border-bottom: 1px solid {$thinblueborder};
+  text-shadow: 0 1px 0 white;
   background-color: {$lightbluebackground};
 }
 
+.aphront-table-view th {
+  border-bottom: 1px solid {$thinblueborder};
+}
+
 th.aphront-table-view-sortable-selected {
   background-color: {$greybackground};
 }
@@ -74,12 +78,8 @@ th.aphront-table-view-sortable-selected {
 }
 
 .aphront-table-view td.header {
-  padding: 4px 8px;
-  white-space: nowrap;
   text-align: right;
-  color: {$bluetext};
-  font-weight: bold;
-  vertical-align: top;
+  border-right: 1px solid {$thinblueborder};
 }
 
 .aphront-table-view td {

From a20f108034126ff7dc45604dec80319e1ec76172 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 14:09:53 -0800
Subject: [PATCH 34/34] When an edit overrides an object lock, note it in the
 transaction record

Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.

Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.

Also, make the "MFA" and "Silent" emblems more easily visible.

Test Plan:
Edited a locked task, overrode the lock, got marked for it.

{F6195005}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aeiser

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20131
---
 resources/celerity/map.php                    |  6 ++---
 ...habricatorApplicationTransactionEditor.php | 14 +++++++++++
 .../PhabricatorApplicationTransaction.php     | 14 +++++++++++
 .../PhabricatorApplicationTransactionView.php |  3 ++-
 src/view/phui/PHUIIconView.php                | 14 +++++++++++
 src/view/phui/PHUITimelineEventView.php       | 23 +++++++++++++++++--
 webroot/rsrc/css/phui/phui-icon.css           | 21 +++++++++++++++++
 7 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 8312828c6e..cb44dd5585 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
-    'core.pkg.css' => '08baca0c',
+    'core.pkg.css' => '7a73ffc5',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -157,7 +157,7 @@ return array(
     'rsrc/css/phui/phui-header-view.css' => '93cea4ec',
     'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0',
     'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec',
-    'rsrc/css/phui/phui-icon.css' => '281f964d',
+    'rsrc/css/phui/phui-icon.css' => '4cbc684a',
     'rsrc/css/phui/phui-image-mask.css' => '62c7f4d2',
     'rsrc/css/phui/phui-info-view.css' => '37b8d9ce',
     'rsrc/css/phui/phui-invisible-character-view.css' => 'c694c4a4',
@@ -823,7 +823,7 @@ return array(
     'phui-hovercard' => '074f0783',
     'phui-hovercard-view-css' => '6ca90fa0',
     'phui-icon-set-selector-css' => '7aa5f3ec',
-    'phui-icon-view-css' => '281f964d',
+    'phui-icon-view-css' => '4cbc684a',
     'phui-image-mask-css' => '62c7f4d2',
     'phui-info-view-css' => '37b8d9ce',
     'phui-inline-comment-view-css' => '48acce5b',
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
index 216c1e00a2..bd066e633b 100644
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -1115,6 +1115,16 @@ abstract class PhabricatorApplicationTransactionEditor
         $transaction_open = true;
       }
 
+      // We can technically test any object for CAN_INTERACT, but we can
+      // run into some issues in doing so (for example, in project unit tests).
+      // For now, only test for CAN_INTERACT if the object is explicitly a
+      // lockable object.
+
+      $was_locked = false;
+      if ($object instanceof PhabricatorEditEngineLockableInterface) {
+        $was_locked = !PhabricatorPolicyFilter::canInteract($actor, $object);
+      }
+
       foreach ($xactions as $xaction) {
         $this->applyInternalEffects($object, $xaction);
       }
@@ -1132,6 +1142,10 @@ abstract class PhabricatorApplicationTransactionEditor
       }
 
       foreach ($xactions as $xaction) {
+        if ($was_locked) {
+          $xaction->setIsLockOverrideTransaction(true);
+        }
+
         $xaction->setObjectPHID($object->getPHID());
         if ($xaction->getComment()) {
           $xaction->setPHID($xaction->generatePHID());
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index acfc4d0cfa..d71728a01f 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -169,6 +169,14 @@ abstract class PhabricatorApplicationTransaction
     return (bool)$this->getMetadataValue('core.mfa', false);
   }
 
+  public function setIsLockOverrideTransaction($override) {
+    return $this->setMetadataValue('core.lock-override', $override);
+  }
+
+  public function getIsLockOverrideTransaction() {
+    return (bool)$this->getMetadataValue('core.lock-override', false);
+  }
+
   public function attachComment(
     PhabricatorApplicationTransactionComment $comment) {
     $this->comment = $comment;
@@ -1529,6 +1537,12 @@ abstract class PhabricatorApplicationTransaction
           return false;
         }
       }
+
+      // Don't group lock override and non-override transactions together.
+      $is_override = $this->getIsLockOverrideTransaction();
+      if ($is_override != $xaction->getIsLockOverrideTransaction()) {
+        return false;
+      }
     }
 
     return true;
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
index c2b32aa190..4d738877b8 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php
@@ -416,7 +416,8 @@ class PhabricatorApplicationTransactionView extends AphrontView {
       ->setColor($xaction->getColor())
       ->setHideCommentOptions($this->getHideCommentOptions())
       ->setIsSilent($xaction->getIsSilentTransaction())
-      ->setIsMFA($xaction->getIsMFATransaction());
+      ->setIsMFA($xaction->getIsMFATransaction())
+      ->setIsLockOverride($xaction->getIsLockOverrideTransaction());
 
     list($token, $token_removed) = $xaction->getToken();
     if ($token) {
diff --git a/src/view/phui/PHUIIconView.php b/src/view/phui/PHUIIconView.php
index 8cc61ba2eb..d907cb3343 100644
--- a/src/view/phui/PHUIIconView.php
+++ b/src/view/phui/PHUIIconView.php
@@ -19,6 +19,7 @@ final class PHUIIconView extends AphrontTagView {
   private $iconColor;
   private $iconBackground;
   private $tooltip;
+  private $emblemColor;
 
   public function setHref($href) {
     $this->href = $href;
@@ -66,6 +67,15 @@ final class PHUIIconView extends AphrontTagView {
     return $this;
   }
 
+  public function setEmblemColor($emblem_color) {
+    $this->emblemColor = $emblem_color;
+    return $this;
+  }
+
+  public function getEmblemColor() {
+    return $this->emblemColor;
+  }
+
   protected function getTagName() {
     $tag = 'span';
     if ($this->href) {
@@ -106,6 +116,10 @@ final class PHUIIconView extends AphrontTagView {
       $this->appendChild($this->text);
     }
 
+    if ($this->emblemColor) {
+      $classes[] = 'phui-icon-emblem phui-icon-emblem-'.$this->emblemColor;
+    }
+
     $sigil = null;
     $meta = array();
     if ($this->tooltip) {
diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php
index 86628058fe..78a75a2063 100644
--- a/src/view/phui/PHUITimelineEventView.php
+++ b/src/view/phui/PHUITimelineEventView.php
@@ -31,6 +31,7 @@ final class PHUITimelineEventView extends AphrontView {
   private $pinboardItems = array();
   private $isSilent;
   private $isMFA;
+  private $isLockOverride;
 
   public function setAuthorPHID($author_phid) {
     $this->authorPHID = $author_phid;
@@ -197,6 +198,15 @@ final class PHUITimelineEventView extends AphrontView {
     return $this->isMFA;
   }
 
+  public function setIsLockOverride($is_override) {
+    $this->isLockOverride = $is_override;
+    return $this;
+  }
+
+  public function getIsLockOverride() {
+    return $this->isLockOverride;
+  }
+
   public function setReallyMajorEvent($me) {
     $this->reallyMajorEvent = $me;
     return $this;
@@ -597,7 +607,8 @@ final class PHUITimelineEventView extends AphrontView {
       // not expect to have received any mail or notifications.
       if ($this->getIsSilent()) {
         $extra[] = id(new PHUIIconView())
-          ->setIcon('fa-bell-slash', 'red')
+          ->setIcon('fa-bell-slash', 'white')
+          ->setEmblemColor('red')
           ->setTooltip(pht('Silent Edit'));
       }
 
@@ -605,9 +616,17 @@ final class PHUITimelineEventView extends AphrontView {
       // provide a hint that it was extra authentic.
       if ($this->getIsMFA()) {
         $extra[] = id(new PHUIIconView())
-          ->setIcon('fa-vcard', 'pink')
+          ->setIcon('fa-vcard', 'white')
+          ->setEmblemColor('pink')
           ->setTooltip(pht('MFA Authenticated'));
       }
+
+      if ($this->getIsLockOverride()) {
+        $extra[] = id(new PHUIIconView())
+          ->setIcon('fa-chain-broken', 'white')
+          ->setEmblemColor('violet')
+          ->setTooltip(pht('Lock Overridden'));
+      }
     }
 
     $extra = javelin_tag(
diff --git a/webroot/rsrc/css/phui/phui-icon.css b/webroot/rsrc/css/phui/phui-icon.css
index 4108074b08..5436bb04b1 100644
--- a/webroot/rsrc/css/phui/phui-icon.css
+++ b/webroot/rsrc/css/phui/phui-icon.css
@@ -183,3 +183,24 @@ a.phui-icon-view.phui-icon-square:hover {
   text-decoration: none;
   color: #fff;
 }
+
+
+.phui-icon-emblem {
+  border-radius: 4px;
+}
+
+.phui-timeline-extra .phui-icon-emblem {
+  padding: 4px 6px;
+}
+
+.phui-icon-emblem-violet {
+  background-color: {$violet};
+}
+
+.phui-icon-emblem-red {
+  background-color: {$red};
+}
+
+.phui-icon-emblem-pink {
+  background-color: {$pink};
+}