From 711871f6bc743a7e91c953695a429c37b6692ca0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 06:48:13 -0800
Subject: [PATCH 01/36] Allow typeaheads to pass nonscalar data to datasources

Summary:
Ref T13250. Currently, datasources have a `setParameters(...)` method. This method accepts a dictionary and adds the key/value pairs to the raw HTTP request to the datasource endpoint.

Since D20049, this no longer works. Since D20116, it fatals explicitly.

In general, the datasource endpoint accepts other values (like `query`, `offset`, and `limit`), and even before these changes, using secret reserved keys in `setParameters(...)` would silently cause program misbehavior.

To deal with this, pass parameters as a JSON string named "parameters". This fixes the HTTP query issue (the more pressing issue affecting users today) and prevents the "shadowing reserved keys" issue (a theoretical issue which might affect users some day).

(I may revisit the `phutil_build_http_querystring()` behavior and possibly let it make this work again, but I think avoiding the duplicate key issue makes this change desirable even if the querystring behavior changes.)

Test Plan:
  - Used "Land Revision", selected branches.
  - Configured a custom Maniphest "users" field, used the search typeahead, selected users.
  - Manually browsed to `/typeahead/class/PhabricatorPeopleDatasource/?query=hi&parameters=xyz` to see the JSON decode exception.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20134
---
 ...orTypeaheadModularDatasourceController.php | 21 ++++++++++++++++++-
 .../PhabricatorTypeaheadDatasource.php        | 16 ++++++++++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
index 9e905f8cce..7c2205df46 100644
--- a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
+++ b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
@@ -35,7 +35,26 @@ final class PhabricatorTypeaheadModularDatasourceController
 
     if (isset($sources[$class])) {
       $source = $sources[$class];
-      $source->setParameters($request->getRequestData());
+
+      $parameters = array();
+
+      $raw_parameters = $request->getStr('parameters');
+      if (strlen($raw_parameters)) {
+        try {
+          $parameters = phutil_json_decode($raw_parameters);
+        } catch (PhutilJSONParserException $ex) {
+          return $this->newDialog()
+            ->setTitle(pht('Invalid Parameters'))
+            ->appendParagraph(
+              pht(
+                'The HTTP parameter named "parameters" for this request is '.
+                'not a valid JSON parameter. JSON is required. Exception: %s',
+                $ex->getMessage()))
+            ->addCancelButton('/');
+        }
+      }
+
+      $source->setParameters($parameters);
       $source->setViewer($viewer);
 
       // NOTE: Wrapping the source in a Composite datasource ensures we perform
diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
index 2e369a3f67..196ad1b98b 100644
--- a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
+++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
@@ -100,7 +100,7 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
 
   public function getDatasourceURI() {
     $uri = new PhutilURI('/typeahead/class/'.get_class($this).'/');
-    $uri->setQueryParams($this->parameters);
+    $uri->setQueryParams($this->newURIParameters());
     return (string)$uri;
   }
 
@@ -110,10 +110,22 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
     }
 
     $uri = new PhutilURI('/typeahead/browse/'.get_class($this).'/');
-    $uri->setQueryParams($this->parameters);
+    $uri->setQueryParams($this->newURIParameters());
     return (string)$uri;
   }
 
+  private function newURIParameters() {
+    if (!$this->parameters) {
+      return array();
+    }
+
+    $map = array(
+      'parameters' => phutil_json_encode($this->parameters),
+    );
+
+    return $map;
+  }
+
   abstract public function getPlaceholderText();
 
   public function getBrowseTitle() {

From 77247084bd365e9020b942a6ac37b367dad1f4a7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 06:55:23 -0800
Subject: [PATCH 02/36] Fix inverted check in audit triggers for "uninvolved
 owner"

Summary: See D20126. I was trying to be a little too cute here with the names and ended up confusing myself, then just tested the method behavior. :/

Test Plan: Persudaded by arguments in D20126.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20135
---
 .../worker/PhabricatorRepositoryCommitOwnersWorker.php        | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index b0c60667a4..d5054a7f18 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -174,12 +174,12 @@ final class PhabricatorRepositoryCommitOwnersWorker
     // 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(
+      $owner_involved = $this->isOwnerInvolved(
         $commit,
         $package,
         $author_phid,
         $revision);
-      if ($commit_uninvolved) {
+      if (!$owner_involved) {
         return true;
       }
     }

From 1275326ea6b6ce16ed2ebda895cc8cd0849f242d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 09:58:38 -0800
Subject: [PATCH 03/36] Use "phutil_string_cast()" in TypeaheadDatasource

Summary:
Depends on D20138. Ref T13250. This improves exception behavior and gives us a standard page with a stack trace instead of a text fatal with no stack trace.

Truly a great day for PHP.

(Eventually we may want to replace all `(string)` with `phutil_string_cast()`, but let's let it have some time in the wild first?)

Test Plan: Triggered the error, got a more useful exception behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20140
---
 .../typeahead/datasource/PhabricatorTypeaheadDatasource.php   | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
index 196ad1b98b..43cc72eaaa 100644
--- a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
+++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
@@ -101,7 +101,7 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
   public function getDatasourceURI() {
     $uri = new PhutilURI('/typeahead/class/'.get_class($this).'/');
     $uri->setQueryParams($this->newURIParameters());
-    return (string)$uri;
+    return phutil_string_cast($uri);
   }
 
   public function getBrowseURI() {
@@ -111,7 +111,7 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
 
     $uri = new PhutilURI('/typeahead/browse/'.get_class($this).'/');
     $uri->setQueryParams($this->newURIParameters());
-    return (string)$uri;
+    return phutil_string_cast($uri);
   }
 
   private function newURIParameters() {

From e03079aaaad2ee43637caacb9d6916dce57650af Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 09:32:33 -0800
Subject: [PATCH 04/36] Try harder to present display/rendering exceptions to
 the user using standard exception handling

Summary:
Ref T13250. When exceptions occur in display/rendering/writing, they currently go straight to the fallback handler. This is a minimal handler which doesn't show a stack trace or include any debugging details.

In some cases, we have to do this: some of these exceptions prevent us from building a normal page. For example, if the menu bar has a hard fatal in it, we aren't going to be able to build a nice exception page with a menu bar no matter how hard we try.

However, in many cases the error is mundane: something detected something invalid and raised an exception during rendering. In these cases there's no problem with the page chrome or the rendering pathway itself, just with rendering the page data.

When we get a rendering/response exception, try a second time to build a nice normal exception page. This will often work. If it doesn't work, fall back as before.

Test Plan:
  - Forced the error from T13250 by applying D20136 but not D20134.
  - Before:

{F6205001}

  - After:

{F6205002}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20137
---
 .../AphrontApplicationConfiguration.php       | 47 +++++++++++++++++--
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 8d36bbc880..350688d4fb 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -282,23 +282,62 @@ final class AphrontApplicationConfiguration
       }
     } catch (Exception $ex) {
       $original_exception = $ex;
-      $response = $this->handleThrowable($ex);
     } catch (Throwable $ex) {
       $original_exception = $ex;
-      $response = $this->handleThrowable($ex);
     }
 
     try {
+      if ($original_exception) {
+        $response = $this->handleThrowable($original_exception);
+      }
+
       $response = $this->produceResponse($request, $response);
       $response = $controller->willSendResponse($response);
       $response->setRequest($request);
 
       self::writeResponse($sink, $response);
-    } catch (Exception $ex) {
+    } catch (Exception $response_exception) {
+      // If we encountered an exception while building a normal response, then
+      // encountered another exception while building a response for the first
+      // exception, just throw the original exception. It is more likely to be
+      // useful and point at a root cause than the second exception we ran into
+      // while telling the user about it.
       if ($original_exception) {
         throw $original_exception;
       }
-      throw $ex;
+
+      // If we built a response successfully and then ran into an exception
+      // trying to render it, try to handle and present that exception to the
+      // user using the standard handler.
+
+      // The problem here might be in rendering (more common) or in the actual
+      // response mechanism (less common). If it's in rendering, we can likely
+      // still render a nice exception page: the majority of rendering issues
+      // are in main page content, not content shared with the exception page.
+
+      $handling_exception = null;
+      try {
+        $response = $this->handleThrowable($response_exception);
+
+        $response = $this->produceResponse($request, $response);
+        $response = $controller->willSendResponse($response);
+        $response->setRequest($request);
+
+        self::writeResponse($sink, $response);
+      } catch (Exception $ex) {
+        $handling_exception = $ex;
+      } catch (Throwable $ex) {
+        $handling_exception = $ex;
+      }
+
+      // If we didn't have any luck with that, raise the original response
+      // exception. As above, this is the root cause exception and more likely
+      // to be useful. This will go to the fallback error handler at top
+      // level.
+
+      if ($handling_exception) {
+        throw $response_exception;
+      }
     }
 
     return $response;

From b9a1260ef5c01bbc912be9a2ff591b29e7d6b13c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 09:58:45 -0800
Subject: [PATCH 05/36] Improve top-level fatal exception handling in PHP 7+

Summary:
Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to `Throwables`, a superclass of `Exception`.

This is generally a good change, but code written against PHP 5.x before `Throwable` was introduced may not catch these errors, even when the code is intended to be a top-level exception handler.

(The double-catch pattern here and elsewhere is because `Throwable` does not exist in older PHP, so `catch (Throwable $ex)` catches nothing. The `Exception $ex` clause catches everything in old PHP, the `Throwable $ex` clause catches everything in newer PHP.)

Generalize some `Exception` into `Throwable`.

Test Plan:
  - Added a bogus function call to the rendering stack.
  - Before change: got a blank page.
  - After change: nice exception page.

{F6205012}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250, T12101

Differential Revision: https://secure.phabricator.com/D20138
---
 .../AphrontApplicationConfiguration.php       |  9 ++-
 support/startup/PhabricatorStartup.php        |  4 +-
 webroot/index.php                             | 55 ++++++++++++++++---
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 350688d4fb..8c1b4b710f 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -286,6 +286,7 @@ final class AphrontApplicationConfiguration
       $original_exception = $ex;
     }
 
+    $response_exception = null;
     try {
       if ($original_exception) {
         $response = $this->handleThrowable($original_exception);
@@ -296,7 +297,13 @@ final class AphrontApplicationConfiguration
       $response->setRequest($request);
 
       self::writeResponse($sink, $response);
-    } catch (Exception $response_exception) {
+    } catch (Exception $ex) {
+      $response_exception = $ex;
+    } catch (Throwable $ex) {
+      $response_exception = $ex;
+    }
+
+    if ($response_exception) {
       // If we encountered an exception while building a normal response, then
       // encountered another exception while building a response for the first
       // exception, just throw the original exception. It is more likely to be
diff --git a/support/startup/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php
index 1bfb74d886..4c577ca20c 100644
--- a/support/startup/PhabricatorStartup.php
+++ b/support/startup/PhabricatorStartup.php
@@ -315,7 +315,7 @@ final class PhabricatorStartup {
    *
    * @param   string    Brief description of the exception context, like
    *                    `"Rendering Exception"`.
-   * @param   Exception The exception itself.
+   * @param   Throwable The exception itself.
    * @param   bool      True if it's okay to show the exception's stack trace
    *                    to the user. The trace will always be logged.
    * @return  exit      This method **does not return**.
@@ -324,7 +324,7 @@ final class PhabricatorStartup {
    */
   public static function didEncounterFatalException(
     $note,
-    Exception $ex,
+    $ex,
     $show_trace) {
 
     $message = '['.$note.'/'.get_class($ex).'] '.$ex->getMessage();
diff --git a/webroot/index.php b/webroot/index.php
index 5c7d79bfa1..6c3d66305e 100644
--- a/webroot/index.php
+++ b/webroot/index.php
@@ -2,6 +2,7 @@
 
 phabricator_startup();
 
+$fatal_exception = null;
 try {
   PhabricatorStartup::beginStartupPhase('libraries');
   PhabricatorStartup::loadCoreLibraries();
@@ -12,25 +13,65 @@ try {
   PhabricatorStartup::beginStartupPhase('sink');
   $sink = new AphrontPHPHTTPSink();
 
+  // PHP introduced a "Throwable" interface in PHP 7 and began making more
+  // runtime errors throw as "Throwable" errors. This is generally good, but
+  // makes top-level exception handling that is compatible with both PHP 5
+  // and PHP 7 a bit tricky.
+
+  // In PHP 5, "Throwable" does not exist, so "catch (Throwable $ex)" catches
+  // nothing.
+
+  // In PHP 7, various runtime conditions raise an Error which is a Throwable
+  // but NOT an Exception, so "catch (Exception $ex)" will not catch them.
+
+  // To cover both cases, we "catch (Exception $ex)" to catch everything in
+  // PHP 5, and most things in PHP 7. Then, we "catch (Throwable $ex)" to catch
+  // everything else in PHP 7. For the most part, we only need to do this at
+  // the top level.
+
+  $main_exception = null;
   try {
     PhabricatorStartup::beginStartupPhase('run');
     AphrontApplicationConfiguration::runHTTPRequest($sink);
   } catch (Exception $ex) {
+    $main_exception = $ex;
+  } catch (Throwable $ex) {
+    $main_exception = $ex;
+  }
+
+  if ($main_exception) {
+    $response_exception = null;
     try {
       $response = new AphrontUnhandledExceptionResponse();
-      $response->setException($ex);
+      $response->setException($main_exception);
 
       PhabricatorStartup::endOutputCapture();
       $sink->writeResponse($response);
-    } catch (Exception $response_exception) {
-      // If we hit a rendering exception, ignore it and throw the original
-      // exception. It is generally more interesting and more likely to be
-      // the root cause.
-      throw $ex;
+    } catch (Exception $ex) {
+      $response_exception = $ex;
+    } catch (Throwable $ex) {
+      $response_exception = $ex;
+    }
+
+    // If we hit a rendering exception, ignore it and throw the original
+    // exception. It is generally more interesting and more likely to be
+    // the root cause.
+
+    if ($response_exception) {
+      throw $main_exception;
     }
   }
 } catch (Exception $ex) {
-  PhabricatorStartup::didEncounterFatalException('Core Exception', $ex, false);
+  $fatal_exception = $ex;
+} catch (Throwable $ex) {
+  $fatal_exception = $ex;
+}
+
+if ($fatal_exception) {
+  PhabricatorStartup::didEncounterFatalException(
+    'Core Exception',
+    $fatal_exception,
+    false);
 }
 
 function phabricator_startup() {

From 51cca22d07028ec40da61d515095442ee8822d45 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 29 Jan 2019 06:07:43 -0800
Subject: [PATCH 06/36] Support EU domains for Mailgun API

Summary:
See <https://discourse.phabricator-community.org/t/mailgun-eu-zone-not-working/2332/4>.

Mailgun has a couple of API domains depending on where your account is based.

Test Plan:
  - Sent normal mail successfully with my non-EU account.
  - Waiting on user confirmation that this works in the EU.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20055
---
 .../metamta/adapter/PhabricatorMailMailgunAdapter.php       | 6 +++++-
 .../user/configuration/configuring_outbound_email.diviner   | 3 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/applications/metamta/adapter/PhabricatorMailMailgunAdapter.php b/src/applications/metamta/adapter/PhabricatorMailMailgunAdapter.php
index 9eb478efc5..8223ee8102 100644
--- a/src/applications/metamta/adapter/PhabricatorMailMailgunAdapter.php
+++ b/src/applications/metamta/adapter/PhabricatorMailMailgunAdapter.php
@@ -24,6 +24,7 @@ final class PhabricatorMailMailgunAdapter
       array(
         'api-key' => 'string',
         'domain' => 'string',
+        'api-hostname' => 'string',
       ));
   }
 
@@ -31,12 +32,14 @@ final class PhabricatorMailMailgunAdapter
     return array(
       'api-key' => null,
       'domain' => null,
+      'api-hostname' => 'api.mailgun.net',
     );
   }
 
   public function sendMessage(PhabricatorMailExternalMessage $message) {
     $api_key = $this->getOption('api-key');
     $domain = $this->getOption('domain');
+    $api_hostname = $this->getOption('api-hostname');
     $params = array();
 
     $subject = $message->getSubject();
@@ -92,7 +95,8 @@ final class PhabricatorMailMailgunAdapter
     }
 
     $mailgun_uri = urisprintf(
-      'https://api.mailgun.net/v2/%s/messages',
+      'https://%s/v2/%s/messages',
+      $api_hostname,
       $domain);
 
     $future = id(new HTTPSFuture($mailgun_uri, $params))
diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner
index 6f5212680e..b77d761f80 100644
--- a/src/docs/user/configuration/configuring_outbound_email.diviner
+++ b/src/docs/user/configuration/configuring_outbound_email.diviner
@@ -227,6 +227,9 @@ To use this mailer, set `type` to `mailgun`, then configure these `options`:
 
   - `api-key`: Required string. Your Mailgun API key.
   - `domain`: Required string. Your Mailgun domain.
+  - `api-hostname`: Optional string. Defaults to "api.mailgun.net". If your
+    account is in another region (like the EU), you may need to specify a
+    different hostname. Consult the Mailgun documentation.
 
 
 Mailer: Amazon SES

From 187356fea5155b5895b9e4c07344476c09196dc1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 13:00:53 -0800
Subject: [PATCH 07/36] Let the top-level exception handler dump a stack trace
 if we reach debug mode before things go sideways

Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.

We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.

There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.

Test Plan: {F6205122}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20142
---
 resources/celerity/map.php                    |  4 +-
 .../AphrontApplicationConfiguration.php       |  6 +++
 .../AphrontUnhandledExceptionResponse.php     | 43 ++++++++++++++++++-
 src/aphront/sink/AphrontHTTPSink.php          | 14 ++++--
 src/view/widget/AphrontStackTraceView.php     |  1 -
 webroot/index.php                             |  1 +
 .../config/unhandled-exception.css            | 32 +++++++++++++-
 7 files changed, 91 insertions(+), 10 deletions(-)

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index cb44dd5585..99f3333106 100644
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -46,7 +46,7 @@ return array(
     'rsrc/css/application/config/config-options.css' => '16c920ae',
     'rsrc/css/application/config/config-template.css' => '20babf50',
     'rsrc/css/application/config/setup-issue.css' => '5eed85b2',
-    'rsrc/css/application/config/unhandled-exception.css' => '9da8fdab',
+    'rsrc/css/application/config/unhandled-exception.css' => '9ecfc00d',
     'rsrc/css/application/conpherence/color.css' => 'b17746b0',
     'rsrc/css/application/conpherence/durable-column.css' => '2d57072b',
     'rsrc/css/application/conpherence/header-pane.css' => 'c9a3db8e',
@@ -877,7 +877,7 @@ return array(
     'syntax-highlighting-css' => '8a16f91b',
     'tokens-css' => 'ce5a50bd',
     'typeahead-browse-css' => 'b7ed02d2',
-    'unhandled-exception-css' => '9da8fdab',
+    'unhandled-exception-css' => '9ecfc00d',
   ),
   'requires' => array(
     '01384686' => array(
diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 8c1b4b710f..8cd27fa62b 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -118,6 +118,12 @@ final class AphrontApplicationConfiguration
       $database_exception = $ex;
     }
 
+    // If we're in developer mode, set a flag so that top-level exception
+    // handlers can add more information.
+    if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) {
+      $sink->setShowStackTraces(true);
+    }
+
     if ($database_exception) {
       $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
         $database_exception,
diff --git a/src/aphront/response/AphrontUnhandledExceptionResponse.php b/src/aphront/response/AphrontUnhandledExceptionResponse.php
index efd9d70ead..32d612ca50 100644
--- a/src/aphront/response/AphrontUnhandledExceptionResponse.php
+++ b/src/aphront/response/AphrontUnhandledExceptionResponse.php
@@ -4,8 +4,20 @@ final class AphrontUnhandledExceptionResponse
   extends AphrontStandaloneHTMLResponse {
 
   private $exception;
+  private $showStackTraces;
+
+  public function setShowStackTraces($show_stack_traces) {
+    $this->showStackTraces = $show_stack_traces;
+    return $this;
+  }
+
+  public function getShowStackTraces() {
+    return $this->showStackTraces;
+  }
+
+  public function setException($exception) {
+    // NOTE: We accept an Exception or a Throwable.
 
-  public function setException(Exception $exception) {
     // Log the exception unless it's specifically a silent malformed request
     // exception.
 
@@ -61,10 +73,36 @@ final class AphrontUnhandledExceptionResponse
     $body = $ex->getMessage();
     $body = phutil_escape_html_newlines($body);
 
+    $classes = array();
+    $classes[] = 'unhandled-exception-detail';
+
+    $stack = null;
+    if ($this->getShowStackTraces()) {
+      try {
+        $stack = id(new AphrontStackTraceView())
+          ->setTrace($ex->getTrace());
+
+        $stack = hsprintf('%s', $stack);
+
+        $stack = phutil_tag(
+          'div',
+          array(
+            'class' => 'unhandled-exception-stack',
+          ),
+          $stack);
+
+        $classes[] = 'unhandled-exception-with-stack';
+      } catch (Exception $trace_exception) {
+        $stack = null;
+      } catch (Throwable $trace_exception) {
+        $stack = null;
+      }
+    }
+
     return phutil_tag(
       'div',
       array(
-        'class' => 'unhandled-exception-detail',
+        'class' => implode(' ', $classes),
       ),
       array(
         phutil_tag(
@@ -79,6 +117,7 @@ final class AphrontUnhandledExceptionResponse
             'class' => 'unhandled-exception-body',
           ),
           $body),
+        $stack,
       ));
   }
 
diff --git a/src/aphront/sink/AphrontHTTPSink.php b/src/aphront/sink/AphrontHTTPSink.php
index 51c54df520..60deba78ca 100644
--- a/src/aphront/sink/AphrontHTTPSink.php
+++ b/src/aphront/sink/AphrontHTTPSink.php
@@ -5,14 +5,22 @@
  * Normally this is just @{class:AphrontPHPHTTPSink}, which uses "echo" and
  * "header()" to emit responses.
  *
- * Mostly, this class allows us to do install security or metrics hooks in the
- * output pipeline.
- *
  * @task write  Writing Response Components
  * @task emit   Emitting the Response
  */
 abstract class AphrontHTTPSink extends Phobject {
 
+  private $showStackTraces = false;
+
+  final public function setShowStackTraces($show_stack_traces) {
+    $this->showStackTraces = $show_stack_traces;
+    return $this;
+  }
+
+  final public function getShowStackTraces() {
+    return $this->showStackTraces;
+  }
+
 
 /* -(  Writing Response Components  )---------------------------------------- */
 
diff --git a/src/view/widget/AphrontStackTraceView.php b/src/view/widget/AphrontStackTraceView.php
index 1d0616df3d..edb805af8f 100644
--- a/src/view/widget/AphrontStackTraceView.php
+++ b/src/view/widget/AphrontStackTraceView.php
@@ -10,7 +10,6 @@ final class AphrontStackTraceView extends AphrontView {
   }
 
   public function render() {
-    $user = $this->getUser();
     $trace = $this->trace;
 
     $libraries = PhutilBootloader::getInstance()->getAllLibraries();
diff --git a/webroot/index.php b/webroot/index.php
index 6c3d66305e..0014edfa2c 100644
--- a/webroot/index.php
+++ b/webroot/index.php
@@ -44,6 +44,7 @@ try {
     try {
       $response = new AphrontUnhandledExceptionResponse();
       $response->setException($main_exception);
+      $response->setShowStackTraces($sink->getShowStackTraces());
 
       PhabricatorStartup::endOutputCapture();
       $sink->writeResponse($response);
diff --git a/webroot/rsrc/css/application/config/unhandled-exception.css b/webroot/rsrc/css/application/config/unhandled-exception.css
index 831148cdad..cd8ad313dc 100644
--- a/webroot/rsrc/css/application/config/unhandled-exception.css
+++ b/webroot/rsrc/css/application/config/unhandled-exception.css
@@ -8,12 +8,12 @@
   background: #fff;
   border: 1px solid #c0392b;
   border-radius: 3px;
-  padding: 0 8px;
+  padding: 8px;
 }
 
 .unhandled-exception-detail .unhandled-exception-title {
   color: #c0392b;
-  padding: 12px 8px;
+  padding: 4px 8px 12px;
   border-bottom: 1px solid #f4dddb;
   font-size: 16px;
   font-weight: 500;
@@ -23,3 +23,31 @@
 .unhandled-exception-detail .unhandled-exception-body {
   padding: 16px 12px;
 }
+
+.unhandled-exception-with-stack {
+  max-width: 95%;
+}
+
+.unhandled-exception-stack {
+  background: #fcfcfc;
+  overflow-x: auto;
+  overflow-y: hidden;
+}
+
+.unhandled-exception-stack table {
+  border-spacing: 0;
+  border-collapse: collapse;
+  width: 100%;
+  border: 1px solid #d7d7d7;
+}
+
+.unhandled-exception-stack th {
+  background: #e7e7e7;
+  border-bottom: 1px solid #d7d7d7;
+  padding: 8px;
+}
+
+.unhandled-exception-stack td {
+  padding: 4px 8px;
+  white-space: nowrap;
+}

From 1fd69f788cee6f6a9a5d7c31e54655e176e018c2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Feb 2019 11:14:57 -0800
Subject: [PATCH 08/36] Replace "getQueryParams()" callsites in Phabricator

Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.

Test Plan: Bit of an adventure, see inlines in a minute.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20141
---
 .../auth/provider/PhabricatorAuthProvider.php |  5 ++--
 .../DifferentialRevisionViewController.php    |  3 ++-
 .../view/DifferentialChangesetListView.php    | 25 ++++++++++++++++---
 .../controller/DiffusionBrowseController.php  |  2 --
 .../oauthserver/PhabricatorOAuthServer.php    |  4 +--
 .../rule/PhabricatorYoutubeRemarkupRule.php   | 16 +++++++++---
 6 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index bcccca5121..04548d1f0c 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -438,12 +438,13 @@ abstract class PhabricatorAuthProvider extends Phobject {
 
     $uri = $attributes['uri'];
     $uri = new PhutilURI($uri);
-    $params = $uri->getQueryParams();
+    $params = $uri->getQueryParamsAsPairList();
     $uri->setQueryParams(array());
 
     $content = array($button);
 
-    foreach ($params as $key => $value) {
+    foreach ($params as $pair) {
+      list($key, $value) = $pair;
       $content[] = phutil_tag(
         'input',
         array(
diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index 8216c11557..9bc6345576 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -1098,7 +1098,8 @@ final class DifferentialRevisionViewController
     //   D123.vs123.id123.whitespaceignore-all.diff
     // lame but nice to include these options
     $file_name = ltrim($request_uri->getPath(), '/').'.';
-    foreach ($request_uri->getQueryParams() as $key => $value) {
+    foreach ($request_uri->getQueryParamsAsPairList() as $pair) {
+      list($key, $value) = $pair;
       if ($key == 'download') {
         continue;
       }
diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php
index 14de553e59..367991497c 100644
--- a/src/applications/differential/view/DifferentialChangesetListView.php
+++ b/src/applications/differential/view/DifferentialChangesetListView.php
@@ -358,7 +358,7 @@ final class DifferentialChangesetListView extends AphrontView {
 
     if ($this->standaloneURI) {
       $uri = new PhutilURI($this->standaloneURI);
-      $uri->setQueryParams($uri->getQueryParams() + $qparams);
+      $uri = $this->appendDefaultQueryParams($uri, $qparams);
       $meta['standaloneURI'] = (string)$uri;
     }
 
@@ -381,7 +381,7 @@ final class DifferentialChangesetListView extends AphrontView {
     if ($this->leftRawFileURI) {
       if ($change != DifferentialChangeType::TYPE_ADD) {
         $uri = new PhutilURI($this->leftRawFileURI);
-        $uri->setQueryParams($uri->getQueryParams() + $qparams);
+        $uri = $this->appendDefaultQueryParams($uri, $qparams);
         $meta['leftURI'] = (string)$uri;
       }
     }
@@ -390,7 +390,7 @@ final class DifferentialChangesetListView extends AphrontView {
       if ($change != DifferentialChangeType::TYPE_DELETE &&
           $change != DifferentialChangeType::TYPE_MULTICOPY) {
         $uri = new PhutilURI($this->rightRawFileURI);
-        $uri->setQueryParams($uri->getQueryParams() + $qparams);
+        $uri = $this->appendDefaultQueryParams($uri, $qparams);
         $meta['rightURI'] = (string)$uri;
       }
     }
@@ -421,4 +421,23 @@ final class DifferentialChangesetListView extends AphrontView {
 
   }
 
+  private function appendDefaultQueryParams(PhutilURI $uri, array $params) {
+    // Add these default query parameters to the query string if they do not
+    // already exist.
+
+    $have = array();
+    foreach ($uri->getQueryParamsAsPairList() as $pair) {
+      list($key, $value) = $pair;
+      $have[$key] = true;
+    }
+
+    foreach ($params as $key => $value) {
+      if (!isset($have[$key])) {
+        $uri->appendQueryParam($key, $value);
+      }
+    }
+
+    return $uri;
+  }
+
 }
diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 6a863a4a92..fcef87b7ef 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -709,8 +709,6 @@ final class DiffusionBrowseController extends DiffusionController {
         'path'      => $path,
       ));
 
-    $before_uri->setQueryParams($request->getRequestURI()->getQueryParams());
-    $before_uri = $before_uri->alter('before', null);
     $before_uri = $before_uri->alter('renamed', $renamed);
     $before_uri = $before_uri->alter('follow', $follow);
 
diff --git a/src/applications/oauthserver/PhabricatorOAuthServer.php b/src/applications/oauthserver/PhabricatorOAuthServer.php
index f5c074f4eb..889e960213 100644
--- a/src/applications/oauthserver/PhabricatorOAuthServer.php
+++ b/src/applications/oauthserver/PhabricatorOAuthServer.php
@@ -256,8 +256,8 @@ final class PhabricatorOAuthServer extends Phobject {
 
     // Any query parameters present in the first URI must be exactly present
     // in the second URI.
-    $need_params = $primary_uri->getQueryParams();
-    $have_params = $secondary_uri->getQueryParams();
+    $need_params = $primary_uri->getQueryParamsAsMap();
+    $have_params = $secondary_uri->getQueryParamsAsMap();
 
     foreach ($need_params as $key => $value) {
       if (!array_key_exists($key, $have_params)) {
diff --git a/src/infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php
index cbf322b2d9..9d79d223e0 100644
--- a/src/infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php
+++ b/src/infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php
@@ -18,12 +18,22 @@ final class PhabricatorYoutubeRemarkupRule extends PhutilRemarkupRule {
       return $text;
     }
 
-    $params = $uri->getQueryParams();
-    $v_param = idx($params, 'v');
-    if (!strlen($v_param)) {
+    $v_params = array();
+
+    $params = $uri->getQueryParamsAsPairList();
+    foreach ($params as $pair) {
+      list($k, $v) = $pair;
+      if ($k === 'v') {
+        $v_params[] = $v;
+      }
+    }
+
+    if (count($v_params) !== 1) {
       return $text;
     }
 
+    $v_param = head($v_params);
+
     $text_mode = $this->getEngine()->isTextMode();
     $mail_mode = $this->getEngine()->isHTMLMailMode();
 

From 308c4f2407543ae37abdf78653a1c7ea70d02c2a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 05:31:04 -0800
Subject: [PATCH 09/36] Fix "AphrontRequest->getRequestURI()" for requests with
 "x[]=1" parameters in the URI

Summary:
Ref T13250. See PHI1069. This is a small fix for `getRequestURI()` currently not working if the request includes "x[]=..." PHP-flavored array parameters, beacause they're parsed into arrays by `$_GET` and `setQueryParams(...)` no longer accepts nonscalars.

Instead, just parse the raw request URI.

Test Plan: Visited `/search/hovercard/?phids[]=X`, no more fatal. Dumped the resulting URI, saw it had the right value. Tried `?phids[]=x&x=1&x=1&x=1`, saw the parameters correctly preserved.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20147
---
 src/aphront/AphrontRequest.php | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 78e6dac9d3..a65566e867 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -591,10 +591,15 @@ final class AphrontRequest extends Phobject {
   }
 
   public function getRequestURI() {
-    $get = $_GET;
-    unset($get['__path__']);
+    $request_uri = idx($_SERVER, 'REQUEST_URI', '/');
+
+    $uri = new PhutilURI($request_uri);
+    $uri->setQueryParam('__path__', null);
+
     $path = phutil_escape_uri($this->getPath());
-    return id(new PhutilURI($path))->setQueryParams($get);
+    $uri->setPath($path);
+
+    return $uri;
   }
 
   public function getAbsoluteRequestURI() {

From 00ffb190cc71b173b99ae4ab155b44a0ea9dc95d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 05:49:59 -0800
Subject: [PATCH 10/36] In Webhooks, label HTTP response codes as "HTTP Status
 Code", not "HTTP Error"

Summary: See PHI1068. We currently show "HTTP Error - 200", which is misleading. Instead, label these results as "HTTP Status Code".

Test Plan: {F6206016}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20148
---
 src/applications/herald/storage/HeraldWebhookRequest.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/herald/storage/HeraldWebhookRequest.php b/src/applications/herald/storage/HeraldWebhookRequest.php
index 3381f6a99c..bc916fd60b 100644
--- a/src/applications/herald/storage/HeraldWebhookRequest.php
+++ b/src/applications/herald/storage/HeraldWebhookRequest.php
@@ -120,7 +120,7 @@ final class HeraldWebhookRequest
   public function getErrorTypeForDisplay() {
     $map = array(
       self::ERRORTYPE_HOOK => pht('Hook Error'),
-      self::ERRORTYPE_HTTP => pht('HTTP Error'),
+      self::ERRORTYPE_HTTP => pht('HTTP Status Code'),
       self::ERRORTYPE_TIMEOUT => pht('Request Timeout'),
     );
 

From fcd85b6d7bedf5c1fc76f5ac309c99b9a73fc584 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 08:07:10 -0800
Subject: [PATCH 11/36] Replace "getRequestURI()->setQueryParams(array())" with
 "getPath()"

Summary:
Ref T13250. A handful of callsites are doing `getRequestURI()` + `setQueryParams(array())` to get a bare request path.

They can just use `getPath()` instead.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20150
---
 src/aphront/response/AphrontAjaxResponse.php         | 12 +++++-------
 .../PhabricatorChatLogChannelLogController.php       |  3 +--
 .../PhabricatorMetaMTAApplicationEmailPanel.php      |  3 +--
 .../panel/PhabricatorEmailAddressesSettingsPanel.php |  3 +--
 .../editengine/PhabricatorEditEngine.php             |  3 +--
 5 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/aphront/response/AphrontAjaxResponse.php b/src/aphront/response/AphrontAjaxResponse.php
index 1ccb3fe97e..2187defc8f 100644
--- a/src/aphront/response/AphrontAjaxResponse.php
+++ b/src/aphront/response/AphrontAjaxResponse.php
@@ -32,22 +32,21 @@ final class AphrontAjaxResponse extends AphrontResponse {
   }
 
   public function buildResponseString() {
+    $request = $this->getRequest();
     $console = $this->getConsole();
     if ($console) {
       // NOTE: We're stripping query parameters here both for readability and
       // to mitigate BREACH and similar attacks. The parameters are available
       // in the "Request" tab, so this should not impact usability. See T3684.
-      $uri = $this->getRequest()->getRequestURI();
-      $uri = new PhutilURI($uri);
-      $uri->setQueryParams(array());
+      $path = $request->getPath();
 
       Javelin::initBehavior(
         'dark-console',
         array(
-          'uri'       => (string)$uri,
-          'key'       => $console->getKey($this->getRequest()),
+          'uri'       => $path,
+          'key'       => $console->getKey($request),
           'color'     => $console->getColor(),
-          'quicksand' => $this->getRequest()->isQuicksand(),
+          'quicksand' => $request->isQuicksand(),
         ));
     }
 
@@ -60,7 +59,6 @@ final class AphrontAjaxResponse extends AphrontResponse {
 
     $response = CelerityAPI::getStaticResourceResponse();
 
-    $request = $this->getRequest();
     if ($request) {
       $viewer = $request->getViewer();
       if ($viewer) {
diff --git a/src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php b/src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php
index 2c6e58da50..b9893f6924 100644
--- a/src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php
+++ b/src/applications/chatlog/controller/PhabricatorChatLogChannelLogController.php
@@ -11,8 +11,7 @@ final class PhabricatorChatLogChannelLogController
     $viewer = $request->getViewer();
     $id = $request->getURIData('channelID');
 
-    $uri = clone $request->getRequestURI();
-    $uri->setQueryParams(array());
+    $uri = new PhutilURI($request->getPath());
 
     $pager = new AphrontCursorPagerView();
     $pager->setURI($uri);
diff --git a/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php b/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php
index c13835e6f4..2f9ddcf22b 100644
--- a/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php
+++ b/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php
@@ -54,8 +54,7 @@ final class PhabricatorMetaMTAApplicationEmailPanel
       return new Aphront404Response();
     }
 
-    $uri = $request->getRequestURI();
-    $uri->setQueryParams(array());
+    $uri = new PhutilURI($request->getPath());
 
     $new = $request->getStr('new');
     $edit = $request->getInt('edit');
diff --git a/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php b/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php
index 1b69adcd62..8f7f633e7e 100644
--- a/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php
@@ -31,8 +31,7 @@ final class PhabricatorEmailAddressesSettingsPanel
     $user = $this->getUser();
     $editable = PhabricatorEnv::getEnvConfig('account.editable');
 
-    $uri = $request->getRequestURI();
-    $uri->setQueryParams(array());
+    $uri = new PhutilURI($request->getPath());
 
     if ($editable) {
       $new = $request->getStr('new');
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
index feb783e724..d1a2fb72b9 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -1279,8 +1279,7 @@ abstract class PhabricatorEditEngine
 
     $fields = $this->willBuildEditForm($object, $fields);
 
-    $request_path = $request->getRequestURI()
-      ->setQueryParams(array());
+    $request_path = $request->getPath();
 
     $form = id(new AphrontFormView())
       ->setUser($viewer)

From 378a43d09c1fdbe7fd88d5bac6609391161aa49e Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 12:46:42 -0800
Subject: [PATCH 12/36] Remove the highly suspect "Import from LDAP" workflow

Summary: Depends on D20109. Ref T6703. This flow was contributed in 2012 and I'm not sure it ever worked, or at least ever worked nondestructively. For now, get rid of it. We'll do importing and external sync properly at some point (T3980, T13190).

Test Plan: Grepped for `ldap/`, grepped for controller.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20110
---
 src/__phutil_library_map__.php                |   2 -
 .../PhabricatorPeopleApplication.php          |   1 -
 .../PhabricatorPeopleController.php           |   4 -
 .../PhabricatorPeopleLdapController.php       | 214 ------------------
 4 files changed, 221 deletions(-)
 delete mode 100644 src/applications/people/controller/PhabricatorPeopleLdapController.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index b704a7cc2e..6c21edc0da 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3867,7 +3867,6 @@ phutil_register_library_map(array(
     'PhabricatorPeopleInviteController' => 'applications/people/controller/PhabricatorPeopleInviteController.php',
     'PhabricatorPeopleInviteListController' => 'applications/people/controller/PhabricatorPeopleInviteListController.php',
     'PhabricatorPeopleInviteSendController' => 'applications/people/controller/PhabricatorPeopleInviteSendController.php',
-    'PhabricatorPeopleLdapController' => 'applications/people/controller/PhabricatorPeopleLdapController.php',
     'PhabricatorPeopleListController' => 'applications/people/controller/PhabricatorPeopleListController.php',
     'PhabricatorPeopleLogQuery' => 'applications/people/query/PhabricatorPeopleLogQuery.php',
     'PhabricatorPeopleLogSearchEngine' => 'applications/people/query/PhabricatorPeopleLogSearchEngine.php',
@@ -9866,7 +9865,6 @@ phutil_register_library_map(array(
     'PhabricatorPeopleInviteController' => 'PhabricatorPeopleController',
     'PhabricatorPeopleInviteListController' => 'PhabricatorPeopleInviteController',
     'PhabricatorPeopleInviteSendController' => 'PhabricatorPeopleInviteController',
-    'PhabricatorPeopleLdapController' => 'PhabricatorPeopleController',
     'PhabricatorPeopleListController' => 'PhabricatorPeopleController',
     'PhabricatorPeopleLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorPeopleLogSearchEngine' => 'PhabricatorApplicationSearchEngine',
diff --git a/src/applications/people/application/PhabricatorPeopleApplication.php b/src/applications/people/application/PhabricatorPeopleApplication.php
index 06740be26e..9cc3607930 100644
--- a/src/applications/people/application/PhabricatorPeopleApplication.php
+++ b/src/applications/people/application/PhabricatorPeopleApplication.php
@@ -63,7 +63,6 @@ final class PhabricatorPeopleApplication extends PhabricatorApplication {
         'welcome/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleWelcomeController',
         'create/' => 'PhabricatorPeopleCreateController',
         'new/(?P<type>[^/]+)/' => 'PhabricatorPeopleNewController',
-        'ldap/' => 'PhabricatorPeopleLdapController',
         'editprofile/(?P<id>[1-9]\d*)/' =>
           'PhabricatorPeopleProfileEditController',
         'badges/(?P<id>[1-9]\d*)/' =>
diff --git a/src/applications/people/controller/PhabricatorPeopleController.php b/src/applications/people/controller/PhabricatorPeopleController.php
index e3b60eff2b..c2c262f9f4 100644
--- a/src/applications/people/controller/PhabricatorPeopleController.php
+++ b/src/applications/people/controller/PhabricatorPeopleController.php
@@ -28,10 +28,6 @@ abstract class PhabricatorPeopleController extends PhabricatorController {
 
       if ($viewer->getIsAdmin()) {
         $nav->addLabel(pht('User Administration'));
-        if (PhabricatorLDAPAuthProvider::getLDAPProvider()) {
-          $nav->addFilter('ldap', pht('Import from LDAP'));
-        }
-
         $nav->addFilter('logs', pht('Activity Logs'));
         $nav->addFilter('invite', pht('Email Invitations'));
       }
diff --git a/src/applications/people/controller/PhabricatorPeopleLdapController.php b/src/applications/people/controller/PhabricatorPeopleLdapController.php
deleted file mode 100644
index 876bf986ad..0000000000
--- a/src/applications/people/controller/PhabricatorPeopleLdapController.php
+++ /dev/null
@@ -1,214 +0,0 @@
-<?php
-
-final class PhabricatorPeopleLdapController
-  extends PhabricatorPeopleController {
-
-  public function handleRequest(AphrontRequest $request) {
-    $this->requireApplicationCapability(
-      PeopleCreateUsersCapability::CAPABILITY);
-    $admin = $request->getUser();
-
-    $content = array();
-
-    $form = id(new AphrontFormView())
-      ->setAction($request->getRequestURI()
-        ->alter('search', 'true')->alter('import', null))
-      ->setUser($admin)
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('LDAP username'))
-          ->setName('username'))
-      ->appendChild(
-        id(new AphrontFormPasswordControl())
-          ->setDisableAutocomplete(true)
-          ->setLabel(pht('Password'))
-          ->setName('password'))
-      ->appendChild(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('LDAP query'))
-          ->setCaption(pht('A filter such as %s.', '(objectClass=*)'))
-          ->setName('query'))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->setValue(pht('Search')));
-
-    $panel = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Import LDAP Users'))
-      ->setForm($form);
-
-    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(
-      pht('Import LDAP Users'),
-      $this->getApplicationURI('/ldap/'));
-
-    $nav = $this->buildSideNavView();
-    $nav->selectFilter('ldap');
-    $nav->appendChild($content);
-
-    if ($request->getStr('import')) {
-      $nav->appendChild($this->processImportRequest($request));
-    }
-
-    $nav->appendChild($panel);
-
-    if ($request->getStr('search')) {
-      $nav->appendChild($this->processSearchRequest($request));
-    }
-
-    return $this->newPage()
-      ->setTitle(pht('Import LDAP Users'))
-      ->setCrumbs($crumbs)
-      ->setNavigation($nav);
-  }
-
-  private function processImportRequest($request) {
-    $admin = $request->getUser();
-    $usernames = $request->getArr('usernames');
-    $emails = $request->getArr('email');
-    $names = $request->getArr('name');
-
-    $notice_view = new PHUIInfoView();
-    $notice_view->setSeverity(PHUIInfoView::SEVERITY_NOTICE);
-    $notice_view->setTitle(pht('Import Successful'));
-    $notice_view->setErrors(array(
-      pht('Successfully imported users from LDAP'),
-    ));
-
-    $list = new PHUIObjectItemListView();
-    $list->setNoDataString(pht('No users imported?'));
-
-    foreach ($usernames as $username) {
-      $user = new PhabricatorUser();
-      $user->setUsername($username);
-      $user->setRealname($names[$username]);
-
-      $email_obj = id(new PhabricatorUserEmail())
-        ->setAddress($emails[$username])
-        ->setIsVerified(1);
-      try {
-        id(new PhabricatorUserEditor())
-          ->setActor($admin)
-          ->createNewUser($user, $email_obj);
-
-        id(new PhabricatorExternalAccount())
-          ->setUserPHID($user->getPHID())
-          ->setAccountType('ldap')
-          ->setAccountDomain('self')
-          ->setAccountID($username)
-          ->save();
-
-        $header = pht('Successfully added %s', $username);
-        $attribute = null;
-        $color = 'fa-check green';
-      } catch (Exception $ex) {
-        $header = pht('Failed to add %s', $username);
-        $attribute = $ex->getMessage();
-        $color = 'fa-times red';
-      }
-
-      $item = id(new PHUIObjectItemView())
-        ->setHeader($header)
-        ->addAttribute($attribute)
-        ->setStatusIcon($color);
-
-      $list->addItem($item);
-    }
-
-    return array(
-      $notice_view,
-      $list,
-    );
-
-  }
-
-  private function processSearchRequest($request) {
-    $panel = new PHUIBoxView();
-    $admin = $request->getUser();
-
-    $search = $request->getStr('query');
-
-    $ldap_provider = PhabricatorLDAPAuthProvider::getLDAPProvider();
-    if (!$ldap_provider) {
-      throw new Exception(pht('No LDAP provider enabled!'));
-    }
-
-    $ldap_adapter = $ldap_provider->getAdapter();
-    $ldap_adapter->setLoginUsername($request->getStr('username'));
-    $ldap_adapter->setLoginPassword(
-      new PhutilOpaqueEnvelope($request->getStr('password')));
-
-    // This causes us to connect and bind.
-    // TODO: Clean up this discard mode stuff.
-    DarkConsoleErrorLogPluginAPI::enableDiscardMode();
-      $ldap_adapter->getAccountID();
-    DarkConsoleErrorLogPluginAPI::disableDiscardMode();
-
-    $results = $ldap_adapter->searchLDAP('%Q', $search);
-
-    foreach ($results as $key => $record) {
-      $account_id = $ldap_adapter->readLDAPRecordAccountID($record);
-      if (!$account_id) {
-        unset($results[$key]);
-        continue;
-      }
-
-      $info = array(
-        $account_id,
-        $ldap_adapter->readLDAPRecordEmail($record),
-        $ldap_adapter->readLDAPRecordRealName($record),
-      );
-      $results[$key] = $info;
-      $results[$key][] = $this->renderUserInputs($info);
-    }
-
-    $form = id(new AphrontFormView())
-      ->setUser($admin);
-
-    $table = new AphrontTableView($results);
-    $table->setHeaders(
-      array(
-        pht('Username'),
-        pht('Email'),
-        pht('Real Name'),
-        pht('Import?'),
-      ));
-    $form->appendChild($table);
-    $form->setAction($request->getRequestURI()
-      ->alter('import', 'true')->alter('search', null))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-        ->setValue(pht('Import')));
-
-    $panel->appendChild($form);
-
-    return $panel;
-  }
-
-  private function renderUserInputs($user) {
-    $username = $user[0];
-    return hsprintf(
-      '%s%s%s',
-      phutil_tag(
-        'input',
-        array(
-          'type' => 'checkbox',
-          'name' => 'usernames[]',
-          'value' => $username,
-        )),
-      phutil_tag(
-        'input',
-        array(
-          'type' => 'hidden',
-          'name' => "email[$username]",
-          'value' => $user[1],
-        )),
-      phutil_tag(
-        'input',
-        array(
-          'type' => 'hidden',
-          'name' => "name[$username]",
-          'value' => $user[2],
-        )));
-  }
-
-}

From 55c18bc90041f15759f62eaa44e31e5f61359be1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 12:59:55 -0800
Subject: [PATCH 13/36] During first-time setup, create an administrator
 account with no authentication instead of weird, detached authentication

Summary:
Ref T6703. Currently, when you create an account on a new install, we prompt you to select a password.

You can't actually use that password unless you set up a password provider, and that password can't be associated with a provider since a password provider won't exist yet.

Instead, just don't ask for a password: create an account with a username and an email address only. Setup guidance points you toward Auth.

If you lose the session, you can send yourself an email link (if email works yet) or `bin/auth recover` it. This isn't really much different than the pre-change behavior, since you can't use the password you set anyway until you configure password auth.

This also makes fixing T9512 more important, which I'll do in a followup. I also plan to add slightly better guideposts toward Auth.

Test Plan: Hit first-time setup, created an account.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: revi

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20111
---
 .../PhabricatorAuthRegisterController.php     | 118 +++++++++---------
 .../people/storage/PhabricatorUser.php        |   2 +-
 .../people/storage/PhabricatorUserEmail.php   |   5 +-
 ...figuring_accounts_and_registration.diviner |  32 +++--
 4 files changed, 89 insertions(+), 68 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
index 9e1aef592c..0562a4242c 100644
--- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
@@ -21,7 +21,9 @@ final class PhabricatorAuthRegisterController
       list($account, $provider, $response) = $result;
       $is_default = false;
     } else if ($this->isFirstTimeSetup()) {
-      list($account, $provider, $response) = $this->loadSetupAccount();
+      $account = null;
+      $provider = null;
+      $response = null;
       $is_default = true;
       $is_setup = true;
     } else {
@@ -35,22 +37,24 @@ final class PhabricatorAuthRegisterController
 
     $invite = $this->loadInvite();
 
-    if (!$provider->shouldAllowRegistration()) {
-      if ($invite) {
-        // If the user has an invite, we allow them to register with any
-        // provider, even a login-only provider.
-      } else {
-        // TODO: This is a routine error if you click "Login" on an external
-        // auth source which doesn't allow registration. The error should be
-        // more tailored.
+    if (!$is_setup) {
+      if (!$provider->shouldAllowRegistration()) {
+        if ($invite) {
+          // If the user has an invite, we allow them to register with any
+          // provider, even a login-only provider.
+        } else {
+          // TODO: This is a routine error if you click "Login" on an external
+          // auth source which doesn't allow registration. The error should be
+          // more tailored.
 
-        return $this->renderError(
-          pht(
-            'The account you are attempting to register with uses an '.
-            'authentication provider ("%s") which does not allow '.
-            'registration. An administrator may have recently disabled '.
-            'registration with this provider.',
-            $provider->getProviderName()));
+          return $this->renderError(
+            pht(
+              'The account you are attempting to register with uses an '.
+              'authentication provider ("%s") which does not allow '.
+              'registration. An administrator may have recently disabled '.
+              'registration with this provider.',
+              $provider->getProviderName()));
+        }
       }
     }
 
@@ -58,14 +62,19 @@ final class PhabricatorAuthRegisterController
 
     $user = new PhabricatorUser();
 
-    $default_username = $account->getUsername();
-    $default_realname = $account->getRealName();
+    if ($is_setup) {
+      $default_username = null;
+      $default_realname = null;
+      $default_email = null;
+    } else {
+      $default_username = $account->getUsername();
+      $default_realname = $account->getRealName();
+      $default_email = $account->getEmail();
+    }
 
     $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
     $content_source = PhabricatorContentSource::newFromRequest($request);
 
-    $default_email = $account->getEmail();
-
     if ($invite) {
       $default_email = $invite->getEmailAddress();
     }
@@ -212,7 +221,11 @@ final class PhabricatorAuthRegisterController
     $can_edit_email = $profile->getCanEditEmail();
     $can_edit_realname = $profile->getCanEditRealName();
 
-    $must_set_password = $provider->shouldRequireRegistrationPassword();
+    if ($is_setup) {
+      $must_set_password = false;
+    } else {
+      $must_set_password = $provider->shouldRequireRegistrationPassword();
+    }
 
     $can_edit_anything = $profile->getCanEditAnything() || $must_set_password;
     $force_verify = $profile->getShouldVerifyEmail();
@@ -334,9 +347,11 @@ final class PhabricatorAuthRegisterController
       }
 
       if (!$errors) {
-        $image = $this->loadProfilePicture($account);
-        if ($image) {
-          $user->setProfileImagePHID($image->getPHID());
+        if (!$is_setup) {
+          $image = $this->loadProfilePicture($account);
+          if ($image) {
+            $user->setProfileImagePHID($image->getPHID());
+          }
         }
 
         try {
@@ -346,17 +361,19 @@ final class PhabricatorAuthRegisterController
             $verify_email = true;
           }
 
-          if ($value_email === $default_email) {
-            if ($account->getEmailVerified()) {
-              $verify_email = true;
-            }
+          if (!$is_setup) {
+            if ($value_email === $default_email) {
+              if ($account->getEmailVerified()) {
+                $verify_email = true;
+              }
 
-            if ($provider->shouldTrustEmails()) {
-              $verify_email = true;
-            }
+              if ($provider->shouldTrustEmails()) {
+                $verify_email = true;
+              }
 
-            if ($invite) {
-              $verify_email = true;
+              if ($invite) {
+                $verify_email = true;
+              }
             }
           }
 
@@ -438,9 +455,11 @@ final class PhabricatorAuthRegisterController
               $transaction_editor->applyTransactions($user, $xactions);
             }
 
-            $account->setUserPHID($user->getPHID());
-            $provider->willRegisterAccount($account);
-            $account->save();
+            if (!$is_setup) {
+              $account->setUserPHID($user->getPHID());
+              $provider->willRegisterAccount($account);
+              $account->save();
+            }
 
           $user->saveTransaction();
 
@@ -501,7 +520,6 @@ final class PhabricatorAuthRegisterController
               ->setAuthProvider($provider)));
     }
 
-
     if ($can_edit_username) {
       $form->appendChild(
         id(new AphrontFormTextControl())
@@ -595,7 +613,7 @@ final class PhabricatorAuthRegisterController
           pht(
             'Installation is complete. Register your administrator account '.
             'below to log in. You will be able to configure options and add '.
-            'other authentication mechanisms (like LDAP or OAuth) later on.'));
+            'authentication mechanisms later on.'));
     }
 
     $object_box = id(new PHUIObjectBoxView())
@@ -612,11 +630,12 @@ final class PhabricatorAuthRegisterController
 
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)
-      ->setFooter(array(
-      $welcome_view,
-      $invite_header,
-      $object_box,
-    ));
+      ->setFooter(
+        array(
+          $welcome_view,
+          $invite_header,
+          $object_box,
+        ));
 
     return $this->newPage()
       ->setTitle($title)
@@ -657,19 +676,6 @@ final class PhabricatorAuthRegisterController
     return array($account, $provider, $response);
   }
 
-  private function loadSetupAccount() {
-    $provider = new PhabricatorPasswordAuthProvider();
-    $provider->attachProviderConfig(
-      id(new PhabricatorAuthProviderConfig())
-        ->setShouldAllowRegistration(1)
-        ->setShouldAllowLogin(1)
-        ->setIsEnabled(true));
-
-    $account = $provider->getDefaultExternalAccount();
-    $response = null;
-    return array($account, $provider, $response);
-  }
-
   private function loadProfilePicture(PhabricatorExternalAccount $account) {
     $phid = $account->getProfileImagePHID();
     if (!$phid) {
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 055df8b79e..c675878747 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -557,7 +557,7 @@ final class PhabricatorUser
 
   public static function describeValidUsername() {
     return pht(
-      'Usernames must contain only numbers, letters, period, underscore and '.
+      'Usernames must contain only numbers, letters, period, underscore, and '.
       'hyphen, and can not end with a period. They must have no more than %d '.
       'characters.',
       new PhutilNumber(self::MAXIMUM_USERNAME_LENGTH));
diff --git a/src/applications/people/storage/PhabricatorUserEmail.php b/src/applications/people/storage/PhabricatorUserEmail.php
index 42946015de..572c7d6e8b 100644
--- a/src/applications/people/storage/PhabricatorUserEmail.php
+++ b/src/applications/people/storage/PhabricatorUserEmail.php
@@ -83,9 +83,8 @@ final class PhabricatorUserEmail extends PhabricatorUserDAO {
    */
   public static function describeValidAddresses() {
     return pht(
-      "Email addresses should be in the form '%s'. The maximum ".
-      "length of an email address is %s character(s).",
-      'user@domain.com',
+      'Email addresses should be in the form "user@domain.com". The maximum '.
+      'length of an email address is %s characters.',
       new PhutilNumber(self::MAX_ADDRESS_LENGTH));
   }
 
diff --git a/src/docs/user/configuration/configuring_accounts_and_registration.diviner b/src/docs/user/configuration/configuring_accounts_and_registration.diviner
index 05d11b11f3..a56d7377cb 100644
--- a/src/docs/user/configuration/configuring_accounts_and_registration.diviner
+++ b/src/docs/user/configuration/configuring_accounts_and_registration.diviner
@@ -3,7 +3,8 @@
 
 Describes how to configure user access to Phabricator.
 
-= Overview =
+Overview
+========
 
 Phabricator supports a number of login systems. You can enable or disable these
 systems to configure who can register for and access your install, and how users
@@ -28,24 +29,37 @@ After you add a provider, you can link it to existing accounts (for example,
 associate an existing Phabricator account with a GitHub OAuth account) or users
 can use it to register new accounts (assuming you enable these options).
 
-= Recovering Inaccessible Accounts =
+
+Recovering Inaccessible Accounts
+================================
 
 If you accidentally lock yourself out of Phabricator (for example, by disabling
-all authentication providers), you can use the `bin/auth`
-script to recover access to an account. To recover access, run:
+all authentication providers), you can normally use the "send a login link"
+action from the login screen to email yourself a login link and regain access
+to your account.
 
-  phabricator/ $ ./bin/auth recover <username>
+If that isn't working (perhaps because you haven't configured email yet), you
+can use the `bin/auth` script to recover access to an account. To recover
+access, run:
+
+```
+phabricator/ $ ./bin/auth recover <username>
+```
 
 ...where `<username>` is the account username you want to recover access
 to. This will generate a link which will log you in as the specified user.
 
-= Managing Accounts with the Web Console =
+
+Managing Accounts with the Web Console
+======================================
 
 To manage accounts from the web, login as an administrator account and go to
 `/people/` or click "People" on the homepage. Provided you're an admin,
 you'll see options to create or edit accounts.
 
-= Manually Creating New Accounts =
+
+Manually Creating New Accounts
+==============================
 
 There are two ways to manually create new accounts: via the web UI using
 the "People" application (this is easiest), or via the CLI using the
@@ -60,7 +74,9 @@ the CLI. You can also use this script to make a user
 an administrator (if you accidentally remove your admin flag) or to create an
 administrative account.
 
-= Next Steps =
+
+Next Steps
+==========
 
 Continue by:
 

From 541d794c13df806f916e0829fefa89230c2720cd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 13:11:34 -0800
Subject: [PATCH 14/36] Give ExternalAccount a providerConfigPHID, tying it to
 a particular provider

Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.

Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.

(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)

Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20112
---
 .../20190206.external.03.providerphid.sql     |  2 ++
 .../20190206.external.04.providerlink.php     | 36 +++++++++++++++++++
 .../PhabricatorAuthRegisterController.php     |  2 +-
 .../auth/provider/PhabricatorAuthProvider.php | 18 +++++++---
 .../PhabricatorPasswordAuthProvider.php       |  8 -----
 .../query/PhabricatorExternalAccountQuery.php | 20 +++++++++++
 .../storage/PhabricatorExternalAccount.php    | 24 +++++++------
 7 files changed, 85 insertions(+), 25 deletions(-)
 create mode 100644 resources/sql/autopatches/20190206.external.03.providerphid.sql
 create mode 100644 resources/sql/autopatches/20190206.external.04.providerlink.php

diff --git a/resources/sql/autopatches/20190206.external.03.providerphid.sql b/resources/sql/autopatches/20190206.external.03.providerphid.sql
new file mode 100644
index 0000000000..0b2f498e02
--- /dev/null
+++ b/resources/sql/autopatches/20190206.external.03.providerphid.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.user_externalaccount
+  ADD providerConfigPHID VARBINARY(64) NOT NULL;
diff --git a/resources/sql/autopatches/20190206.external.04.providerlink.php b/resources/sql/autopatches/20190206.external.04.providerlink.php
new file mode 100644
index 0000000000..e4a2e2d4bf
--- /dev/null
+++ b/resources/sql/autopatches/20190206.external.04.providerlink.php
@@ -0,0 +1,36 @@
+<?php
+
+$account_table = new PhabricatorExternalAccount();
+$account_conn = $account_table->establishConnection('w');
+$table_name = $account_table->getTableName();
+
+$config_table = new PhabricatorAuthProviderConfig();
+$config_conn = $config_table->establishConnection('w');
+
+foreach (new LiskRawMigrationIterator($account_conn, $table_name) as $row) {
+  if (strlen($row['providerConfigPHID'])) {
+    continue;
+  }
+
+  $config_row = queryfx_one(
+    $config_conn,
+    'SELECT phid
+      FROM %R
+      WHERE providerType = %s AND providerDomain = %s
+      LIMIT 1',
+    $config_table,
+    $row['accountType'],
+    $row['accountDomain']);
+  if (!$config_row) {
+    continue;
+  }
+
+  queryfx(
+    $account_conn,
+    'UPDATE %R
+      SET providerConfigPHID = %s
+      WHERE id = %d',
+    $account_table,
+    $config_row['phid'],
+    $row['id']);
+}
diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
index 0562a4242c..b1b2c86dc4 100644
--- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
@@ -671,7 +671,7 @@ final class PhabricatorAuthRegisterController
     }
 
     $provider = head($providers);
-    $account = $provider->getDefaultExternalAccount();
+    $account = $provider->newDefaultExternalAccount();
 
     return array($account, $provider, $response);
   }
diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index 04548d1f0c..78aeebd810 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -220,9 +220,7 @@ abstract class PhabricatorAuthProvider extends Phobject {
       $adapter->getAdapterDomain(),
       $account_id);
     if (!$account) {
-      $account = id(new PhabricatorExternalAccount())
-        ->setAccountType($adapter->getAdapterType())
-        ->setAccountDomain($adapter->getAdapterDomain())
+      $account = $this->newExternalAccount()
         ->setAccountID($account_id);
     }
 
@@ -299,8 +297,18 @@ abstract class PhabricatorAuthProvider extends Phobject {
     return false;
   }
 
-  public function getDefaultExternalAccount() {
-    throw new PhutilMethodNotImplementedException();
+  public function newDefaultExternalAccount() {
+    return $this->newExternalAccount();
+  }
+
+  protected function newExternalAccount() {
+    $config = $this->getProviderConfig();
+    $adapter = $this->getAdapter();
+
+    return id(new PhabricatorExternalAccount())
+      ->setAccountType($adapter->getAdapterType())
+      ->setAccountDomain($adapter->getAdapterDomain())
+      ->setProviderConfigPHID($config->getPHID());
   }
 
   public function getLoginOrder() {
diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
index ec5720e078..146e41706a 100644
--- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
@@ -359,14 +359,6 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
     return true;
   }
 
-  public function getDefaultExternalAccount() {
-    $adapter = $this->getAdapter();
-
-    return id(new PhabricatorExternalAccount())
-      ->setAccountType($adapter->getAdapterType())
-      ->setAccountDomain($adapter->getAdapterDomain());
-  }
-
   protected function willSaveAccount(PhabricatorExternalAccount $account) {
     parent::willSaveAccount($account);
     $account->setUserPHID($account->getAccountID());
diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
index c4a53c12f8..f67fb4b581 100644
--- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php
+++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
@@ -71,6 +71,26 @@ final class PhabricatorExternalAccountQuery
   }
 
   protected function willFilterPage(array $accounts) {
+    $viewer = $this->getViewer();
+
+    $configs = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(mpull($accounts, 'getProviderConfigPHID'))
+      ->execute();
+    $configs = mpull($configs, null, 'getPHID');
+
+    foreach ($accounts as $key => $account) {
+      $config_phid = $account->getProviderConfigPHID();
+      $config = idx($configs, $config_phid);
+
+      if (!$config) {
+        unset($accounts[$key]);
+        continue;
+      }
+
+      $account->attachProviderConfig($config);
+    }
+
     if ($this->needImages) {
       $file_phids = mpull($accounts, 'getProfileImagePHID');
       $file_phids = array_filter($file_phids);
diff --git a/src/applications/people/storage/PhabricatorExternalAccount.php b/src/applications/people/storage/PhabricatorExternalAccount.php
index 4bc0fcae98..bde9588339 100644
--- a/src/applications/people/storage/PhabricatorExternalAccount.php
+++ b/src/applications/people/storage/PhabricatorExternalAccount.php
@@ -16,8 +16,10 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO
   protected $accountURI;
   protected $profileImagePHID;
   protected $properties = array();
+  protected $providerConfigPHID;
 
   private $profileImageFile = self::ATTACHABLE;
+  private $providerConfig = self::ATTACHABLE;
 
   public function getProfileImageFile() {
     return $this->assertAttached($this->profileImageFile);
@@ -65,13 +67,6 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO
     ) + parent::getConfiguration();
   }
 
-  public function getPhabricatorUser() {
-    $tmp_usr = id(new PhabricatorUser())
-      ->makeEphemeral()
-      ->setPHID($this->getPHID());
-    return $tmp_usr;
-  }
-
   public function getProviderKey() {
     return $this->getAccountType().':'.$this->getAccountDomain();
   }
@@ -93,13 +88,12 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO
   }
 
   public function isUsableForLogin() {
-    $key = $this->getProviderKey();
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey($key);
-
-    if (!$provider) {
+    $config = $this->getProviderConfig();
+    if (!$config->getIsEnabled()) {
       return false;
     }
 
+    $provider = $config->getProvider();
     if (!$provider->shouldAllowLogin()) {
       return false;
     }
@@ -125,6 +119,14 @@ final class PhabricatorExternalAccount extends PhabricatorUserDAO
     return idx($map, $type, pht('"%s" User', $type));
   }
 
+  public function attachProviderConfig(PhabricatorAuthProviderConfig $config) {
+    $this->providerConfig = $config;
+    return $this;
+  }
+
+  public function getProviderConfig() {
+    return $this->assertAttached($this->providerConfig);
+  }
 
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */

From e5ee656fff0cc0a548534912e74bfed00e472b80 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 16:00:40 -0800
Subject: [PATCH 15/36] Make external account unlinking use account IDs, not
 "providerType + providerDomain" nonsense

Summary: Depends on D20112. Ref T6703. When you go to unlink an account, unlink it by ID. Crazy!

Test Plan: Unlinked and relinked Google accounts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20113
---
 .../PhabricatorAuthApplication.php            |   2 +-
 .../PhabricatorAuthUnlinkController.php       | 117 +++++++-----------
 ...abricatorExternalAccountsSettingsPanel.php |   2 +-
 3 files changed, 50 insertions(+), 71 deletions(-)

diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index 307cab61c8..0b3487b71b 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -61,7 +61,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
         'start/' => 'PhabricatorAuthStartController',
         'validate/' => 'PhabricatorAuthValidateController',
         'finish/' => 'PhabricatorAuthFinishController',
-        'unlink/(?P<pkey>[^/]+)/' => 'PhabricatorAuthUnlinkController',
+        'unlink/(?P<id>\d+)/' => 'PhabricatorAuthUnlinkController',
         '(?P<action>link|refresh)/(?P<pkey>[^/]+)/'
           => 'PhabricatorAuthLinkController',
         'confirmlink/(?P<akey>[^/]+)/'
diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
index 1e3023e5d2..004ddf4f9a 100644
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -3,48 +3,45 @@
 final class PhabricatorAuthUnlinkController
   extends PhabricatorAuthController {
 
-  private $providerKey;
-
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
-    $this->providerKey = $request->getURIData('pkey');
+    $id = $request->getURIData('id');
 
-    list($type, $domain) = explode(':', $this->providerKey, 2);
-
-    // Check that this account link actually exists. We don't require the
-    // provider to exist because we want users to be able to delete links to
-    // dead accounts if they want.
-    $account = id(new PhabricatorExternalAccount())->loadOneWhere(
-      'accountType = %s AND accountDomain = %s AND userPHID = %s',
-      $type,
-      $domain,
-      $viewer->getPHID());
+    $account = id(new PhabricatorExternalAccountQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
     if (!$account) {
-      return $this->renderNoAccountErrorDialog();
+      return new Aphront404Response();
     }
 
-    // Check that the provider (if it exists) allows accounts to be unlinked.
-    $provider_key = $this->providerKey;
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key);
-    if ($provider) {
-      if (!$provider->shouldAllowAccountUnlink()) {
-        return $this->renderNotUnlinkableErrorDialog($provider);
-      }
+    $done_uri = '/settings/panel/external/';
+
+    $config = $account->getProviderConfig();
+    $provider = $config->getProvider();
+    if (!$provider->shouldAllowAccountUnlink()) {
+      return $this->renderNotUnlinkableErrorDialog($provider, $done_uri);
     }
 
     $confirmations = $request->getStrList('confirmations');
     $confirmations = array_fuse($confirmations);
 
     if (!$request->isFormPost() || !isset($confirmations['unlink'])) {
-      return $this->renderConfirmDialog($confirmations);
+      return $this->renderConfirmDialog($confirmations, $config, $done_uri);
     }
 
     // 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',
-        $viewer->getPHID());
+      $other_accounts = id(new PhabricatorExternalAccountQuery())
+        ->setViewer($viewer)
+        ->withUserPHIDs(array($viewer->getPHID()))
+        ->execute();
 
       $valid_accounts = 0;
       foreach ($other_accounts as $other_account) {
@@ -55,7 +52,9 @@ final class PhabricatorAuthUnlinkController
 
       if ($valid_accounts < 2) {
         if (!isset($confirmations['only'])) {
-          return $this->renderOnlyUsableAccountConfirmDialog($confirmations);
+          return $this->renderOnlyUsableAccountConfirmDialog(
+            $confirmations,
+            $done_uri);
         }
       }
     }
@@ -67,42 +66,27 @@ final class PhabricatorAuthUnlinkController
       new PhutilOpaqueEnvelope(
         $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
-    return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
-  }
-
-  private function getDoneURI() {
-    return '/settings/panel/external/';
-  }
-
-  private function renderNoAccountErrorDialog() {
-    $dialog = id(new AphrontDialogView())
-      ->setUser($this->getRequest()->getUser())
-      ->setTitle(pht('No Such Account'))
-      ->appendChild(
-        pht(
-          'You can not unlink this account because it is not linked.'))
-      ->addCancelButton($this->getDoneURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
+    return id(new AphrontRedirectResponse())->setURI($done_uri);
   }
 
   private function renderNotUnlinkableErrorDialog(
-    PhabricatorAuthProvider $provider) {
+    PhabricatorAuthProvider $provider,
+    $done_uri) {
 
-    $dialog = id(new AphrontDialogView())
-      ->setUser($this->getRequest()->getUser())
+    return $this->newDialog()
       ->setTitle(pht('Permanent Account Link'))
       ->appendChild(
         pht(
           'You can not unlink this account because the administrator has '.
-          'configured Phabricator to make links to %s accounts permanent.',
+          'configured Phabricator to make links to "%s" accounts permanent.',
           $provider->getProviderName()))
-      ->addCancelButton($this->getDoneURI());
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
+      ->addCancelButton($done_uri);
   }
 
-  private function renderOnlyUsableAccountConfirmDialog(array $confirmations) {
+  private function renderOnlyUsableAccountConfirmDialog(
+    array $confirmations,
+    $done_uri) {
+
     $confirmations[] = 'only';
 
     return $this->newDialog()
@@ -116,28 +100,23 @@ final class PhabricatorAuthUnlinkController
         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())
+      ->addCancelButton($done_uri)
       ->addSubmitButton(pht('Unlink External Account'));
   }
 
-  private function renderConfirmDialog(array $confirmations) {
+  private function renderConfirmDialog(
+    array $confirmations,
+    PhabricatorAuthProviderConfig $config,
+    $done_uri) {
+
     $confirmations[] = 'unlink';
+    $provider = $config->getProvider();
 
-    $provider_key = $this->providerKey;
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey($provider_key);
-
-    if ($provider) {
-      $title = pht('Unlink "%s" Account?', $provider->getProviderName());
-      $body = pht(
-        'You will no longer be able to use your %s account to '.
-        'log in to Phabricator.',
-        $provider->getProviderName());
-    } else {
-      $title = pht('Unlink Account?');
-      $body = pht(
-        'You will no longer be able to use this account to log in '.
-        'to Phabricator.');
-    }
+    $title = pht('Unlink "%s" Account?', $provider->getProviderName());
+    $body = pht(
+      'You will no longer be able to use your %s account to '.
+      'log in to Phabricator.',
+      $provider->getProviderName());
 
     return $this->newDialog()
       ->setTitle($title)
@@ -148,7 +127,7 @@ final class PhabricatorAuthUnlinkController
           'Note: Unlinking an authentication provider will terminate any '.
           'other active login sessions.'))
       ->addSubmitButton(pht('Unlink Account'))
-      ->addCancelButton($this->getDoneURI());
+      ->addCancelButton($done_uri);
   }
 
 }
diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
index 29ef9fa2c7..9904c7369f 100644
--- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
@@ -78,7 +78,7 @@ final class PhabricatorExternalAccountsSettingsPanel
           ->setIcon('fa-times')
           ->setWorkflow(true)
           ->setDisabled(!$can_unlink)
-          ->setHref('/auth/unlink/'.$account->getProviderKey().'/'));
+          ->setHref('/auth/unlink/'.$account->getID().'/'));
 
       if ($provider) {
         $provider->willRenderLinkedAccount($viewer, $item, $account);

From d22495a820c67a7e5e156f2a8792620bea44bbdc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 17:12:12 -0800
Subject: [PATCH 16/36] Make external link/refresh use provider IDs, switch
 external account MFA to one-shot

Summary:
Depends on D20113. Ref T6703. Continue moving toward a future where multiple copies of a given type of provider may exist.

Switch MFA from session-MFA at the start to one-shot MFA at the actual link action.

Add one-shot MFA to the unlink action. This theoretically prevents an attacker from unlinking an account while you're getting coffee, registering `alIce` which they control, adding a copy of your profile picture, and then trying to trick you into writing a private note with your personal secrets or something.

Test Plan: Linked and unlinked accounts. Refreshed account. Unlinked, then registered a new account. Unlinked, then relinked to my old account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20117
---
 .../PhabricatorAuthApplication.php            |  2 +-
 .../PhabricatorAuthConfirmLinkController.php  | 19 ++++++-----
 .../controller/PhabricatorAuthController.php  | 16 ++++-----
 .../PhabricatorAuthLinkController.php         | 33 ++++++++++---------
 .../PhabricatorAuthUnlinkController.php       | 10 +++++-
 .../query/PhabricatorExternalAccountQuery.php | 13 ++++++++
 ...bricatorPeopleProfilePictureController.php | 11 +++----
 ...abricatorExternalAccountsSettingsPanel.php | 32 ++++++++----------
 8 files changed, 76 insertions(+), 60 deletions(-)

diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index 0b3487b71b..52cf01b2aa 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -62,7 +62,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
         'validate/' => 'PhabricatorAuthValidateController',
         'finish/' => 'PhabricatorAuthFinishController',
         'unlink/(?P<id>\d+)/' => 'PhabricatorAuthUnlinkController',
-        '(?P<action>link|refresh)/(?P<pkey>[^/]+)/'
+        '(?P<action>link|refresh)/(?P<id>\d+)/'
           => 'PhabricatorAuthLinkController',
         'confirmlink/(?P<akey>[^/]+)/'
           => 'PhabricatorAuthConfirmLinkController',
diff --git a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php
index 664a97885f..9ceb10df8b 100644
--- a/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthConfirmLinkController.php
@@ -20,7 +20,15 @@ final class PhabricatorAuthConfirmLinkController
 
     $panel_uri = '/settings/panel/external/';
 
-    if ($request->isFormPost()) {
+    if ($request->isFormOrHisecPost()) {
+      $workflow_key = sprintf(
+        'account.link(%s)',
+        $account->getPHID());
+
+      $hisec_token = id(new PhabricatorAuthSessionEngine())
+        ->setWorkflowKey($workflow_key)
+        ->requireHighSecurityToken($viewer, $request, $panel_uri);
+
       $account->setUserPHID($viewer->getPHID());
       $account->save();
 
@@ -31,14 +39,7 @@ final class PhabricatorAuthConfirmLinkController
       return id(new AphrontRedirectResponse())->setURI($panel_uri);
     }
 
-    // TODO: Provide more information about the external account. Clicking
-    // through this form blindly is dangerous.
-
-    // TODO: If the user has password authentication, require them to retype
-    // their password here.
-
-    $dialog = id(new AphrontDialogView())
-      ->setUser($viewer)
+    $dialog = $this->newDialog()
       ->setTitle(pht('Confirm %s Account Link', $provider->getProviderName()))
       ->addCancelButton($panel_uri)
       ->addSubmitButton(pht('Confirm Account Link'));
diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php
index 9b7267ec96..e81301218c 100644
--- a/src/applications/auth/controller/PhabricatorAuthController.php
+++ b/src/applications/auth/controller/PhabricatorAuthController.php
@@ -213,19 +213,19 @@ abstract class PhabricatorAuthController extends PhabricatorController {
       return array($account, $provider, $response);
     }
 
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-      $account->getProviderKey());
-
-    if (!$provider) {
+    $config = $account->getProviderConfig();
+    if (!$config->getIsEnabled()) {
       $response = $this->renderError(
         pht(
-          'The account you are attempting to register with uses a nonexistent '.
-          'or disabled authentication provider (with key "%s"). An '.
-          'administrator may have recently disabled this provider.',
-          $account->getProviderKey()));
+          'The account you are attempting to register with uses a disabled '.
+          'authentication provider ("%s"). An administrator may have '.
+          'recently disabled this provider.',
+          $config->getDisplayName()));
       return array($account, $provider, $response);
     }
 
+    $provider = $config->getProvider();
+
     return array($account, $provider, null);
   }
 
diff --git a/src/applications/auth/controller/PhabricatorAuthLinkController.php b/src/applications/auth/controller/PhabricatorAuthLinkController.php
index 44176a278e..4b127b9ad1 100644
--- a/src/applications/auth/controller/PhabricatorAuthLinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLinkController.php
@@ -6,14 +6,20 @@ final class PhabricatorAuthLinkController
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
     $action = $request->getURIData('action');
-    $provider_key = $request->getURIData('pkey');
 
-    $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-      $provider_key);
-    if (!$provider) {
+    $id = $request->getURIData('id');
+
+    $config = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->withIsEnabled(true)
+      ->executeOne();
+    if (!$config) {
       return new Aphront404Response();
     }
 
+    $provider = $config->getProvider();
+
     switch ($action) {
       case 'link':
         if (!$provider->shouldAllowAccountLink()) {
@@ -37,15 +43,15 @@ final class PhabricatorAuthLinkController
         return new Aphront400Response();
     }
 
-    $account = id(new PhabricatorExternalAccount())->loadOneWhere(
-      'accountType = %s AND accountDomain = %s AND userPHID = %s',
-      $provider->getProviderType(),
-      $provider->getProviderDomain(),
-      $viewer->getPHID());
+    $accounts = id(new PhabricatorExternalAccountQuery())
+      ->setViewer($viewer)
+      ->withUserPHIDs(array($viewer->getPHID()))
+      ->withProviderConfigPHIDs(array($config->getPHID()))
+      ->execute();
 
     switch ($action) {
       case 'link':
-        if ($account) {
+        if ($accounts) {
           return $this->renderErrorPage(
             pht('Account Already Linked'),
             array(
@@ -56,7 +62,7 @@ final class PhabricatorAuthLinkController
         }
         break;
       case 'refresh':
-        if (!$account) {
+        if (!$accounts) {
           return $this->renderErrorPage(
             pht('No Account Linked'),
             array(
@@ -76,11 +82,6 @@ final class PhabricatorAuthLinkController
 
     switch ($action) {
       case 'link':
-        id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
-          $viewer,
-          $request,
-          $panel_uri);
-
         $form = $provider->buildLinkForm($this);
         break;
       case 'refresh':
diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
index 004ddf4f9a..43e7b1b362 100644
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -31,7 +31,7 @@ final class PhabricatorAuthUnlinkController
     $confirmations = $request->getStrList('confirmations');
     $confirmations = array_fuse($confirmations);
 
-    if (!$request->isFormPost() || !isset($confirmations['unlink'])) {
+    if (!$request->isFormOrHisecPost() || !isset($confirmations['unlink'])) {
       return $this->renderConfirmDialog($confirmations, $config, $done_uri);
     }
 
@@ -59,6 +59,14 @@ final class PhabricatorAuthUnlinkController
       }
     }
 
+    $workflow_key = sprintf(
+      'account.unlink(%s)',
+      $account->getPHID());
+
+    $hisec_token = id(new PhabricatorAuthSessionEngine())
+      ->setWorkflowKey($workflow_key)
+      ->requireHighSecurityToken($viewer, $request, $done_uri);
+
     $account->delete();
 
     id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
diff --git a/src/applications/auth/query/PhabricatorExternalAccountQuery.php b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
index f67fb4b581..1c5b3fe14f 100644
--- a/src/applications/auth/query/PhabricatorExternalAccountQuery.php
+++ b/src/applications/auth/query/PhabricatorExternalAccountQuery.php
@@ -21,6 +21,7 @@ final class PhabricatorExternalAccountQuery
   private $userPHIDs;
   private $needImages;
   private $accountSecrets;
+  private $providerConfigPHIDs;
 
   public function withUserPHIDs(array $user_phids) {
     $this->userPHIDs = $user_phids;
@@ -62,6 +63,11 @@ final class PhabricatorExternalAccountQuery
     return $this;
   }
 
+  public function withProviderConfigPHIDs(array $phids) {
+    $this->providerConfigPHIDs = $phids;
+    return $this;
+  }
+
   public function newResultObject() {
     return new PhabricatorExternalAccount();
   }
@@ -181,6 +187,13 @@ final class PhabricatorExternalAccountQuery
         $this->accountSecrets);
     }
 
+    if ($this->providerConfigPHIDs !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'providerConfigPHID IN (%Ls)',
+        $this->providerConfigPHIDs);
+    }
+
     return $where;
   }
 
diff --git a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php
index 5cab924255..92bb2e0b86 100644
--- a/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php
+++ b/src/applications/people/controller/PhabricatorPeopleProfilePictureController.php
@@ -157,13 +157,10 @@ final class PhabricatorPeopleProfilePictureController
         continue;
       }
 
-      $provider = PhabricatorAuthProvider::getEnabledProviderByKey(
-        $account->getProviderKey());
-      if ($provider) {
-        $tip = pht('Picture From %s', $provider->getProviderName());
-      } else {
-        $tip = pht('Picture From External Account');
-      }
+      $config = $account->getProviderConfig();
+      $provider = $config->getProvider();
+
+      $tip = pht('Picture From %s', $provider->getProviderName());
 
       if ($file->isTransformableImage()) {
         $images[$file->getPHID()] = array(
diff --git a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
index 9904c7369f..60389e1590 100644
--- a/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorExternalAccountsSettingsPanel.php
@@ -44,17 +44,13 @@ final class PhabricatorExternalAccountsSettingsPanel
     foreach ($accounts as $account) {
       $item = new PHUIObjectItemView();
 
-      $provider = idx($providers, $account->getProviderKey());
-      if ($provider) {
-        $item->setHeader($provider->getProviderName());
-        $can_unlink = $provider->shouldAllowAccountUnlink();
-        if (!$can_unlink) {
-          $item->addAttribute(pht('Permanently Linked'));
-        }
-      } else {
-        $item->setHeader(
-          pht('Unknown Account ("%s")', $account->getProviderKey()));
-        $can_unlink = true;
+      $config = $account->getProviderConfig();
+      $provider = $config->getProvider();
+
+      $item->setHeader($provider->getProviderName());
+      $can_unlink = $provider->shouldAllowAccountUnlink();
+      if (!$can_unlink) {
+        $item->addAttribute(pht('Permanently Linked'));
       }
 
       $can_login = $account->isUsableForLogin();
@@ -65,12 +61,12 @@ final class PhabricatorExternalAccountsSettingsPanel
             'account provider).'));
       }
 
-      $can_refresh = $provider && $provider->shouldAllowAccountRefresh();
+      $can_refresh = $provider->shouldAllowAccountRefresh();
       if ($can_refresh) {
         $item->addAction(
           id(new PHUIListItemView())
             ->setIcon('fa-refresh')
-            ->setHref('/auth/refresh/'.$account->getProviderKey().'/'));
+            ->setHref('/auth/refresh/'.$config->getID().'/'));
       }
 
       $item->addAction(
@@ -94,14 +90,15 @@ final class PhabricatorExternalAccountsSettingsPanel
       ->setNoDataString(
         pht('Your account is linked with all available providers.'));
 
-    $accounts = mpull($accounts, null, 'getProviderKey');
-
     $configs = id(new PhabricatorAuthProviderConfigQuery())
       ->setViewer($viewer)
       ->withIsEnabled(true)
       ->execute();
     $configs = msort($configs, 'getSortVector');
 
+    $account_map = mgroup($accounts, 'getProviderConfigPHID');
+
+
     foreach ($configs as $config) {
       $provider = $config->getProvider();
 
@@ -110,12 +107,11 @@ final class PhabricatorExternalAccountsSettingsPanel
       }
 
       // Don't show the user providers they already have linked.
-      $provider_key = $config->getProvider()->getProviderKey();
-      if (isset($accounts[$provider_key])) {
+      if (isset($account_map[$config->getPHID()])) {
         continue;
       }
 
-      $link_uri = '/auth/link/'.$provider->getProviderKey().'/';
+      $link_uri = '/auth/link/'.$config->getID().'/';
 
       $link_button = id(new PHUIButtonView())
         ->setTag('a')

From 3f35c0068ad1e458565fb32e20f23d9a9feef6b4 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 17:55:31 -0800
Subject: [PATCH 17/36] Allow users to register with non-registration providers
 if they are invited to an instance

Summary:
Depends on D20117. Fixes T10071. When you're sent an email invitation, it's intended to allow you to register an account even if you otherwise could not (see D11737).

Some time between D11737 and today, this stopped working (or perhaps it never worked and I got things wrong in D11737). I think this actually ended up not mattering for us, given the way Phacility auth was ultimately built.

This feature generally seems reasonable, though, and probably //should// work. Make it work in the "password" and "oauth" cases, at least. This may //still// not work for LDAP, but testing that is nontrivial.

Test Plan:
  - Enabled only passwords, turned off registration, sent an invite, registered with a password.
  - Enabled only Google OAuth, turned off registration, sent an invite, registered with Google OAuth.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10071

Differential Revision: https://secure.phabricator.com/D20118
---
 .../PhabricatorAuthLoginController.php        |  3 ++-
 .../PhabricatorAuthRegisterController.php     | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php
index 54649a6a69..e7dabd9340 100644
--- a/src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -35,6 +35,7 @@ final class PhabricatorAuthLoginController
       return $response;
     }
 
+    $invite = $this->loadInvite();
     $provider = $this->provider;
 
     try {
@@ -103,7 +104,7 @@ final class PhabricatorAuthLoginController
       // The account is not yet attached to a Phabricator user, so this is
       // either a registration or an account link request.
       if (!$viewer->isLoggedIn()) {
-        if ($provider->shouldAllowRegistration()) {
+        if ($provider->shouldAllowRegistration() || $invite) {
           return $this->processRegisterUser($account);
         } else {
           return $this->renderError(
diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
index b1b2c86dc4..5a46d0e604 100644
--- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
@@ -11,10 +11,12 @@ final class PhabricatorAuthRegisterController
     $viewer = $this->getViewer();
     $account_key = $request->getURIData('akey');
 
-    if ($request->getUser()->isLoggedIn()) {
+    if ($viewer->isLoggedIn()) {
       return id(new AphrontRedirectResponse())->setURI('/');
     }
 
+    $invite = $this->loadInvite();
+
     $is_setup = false;
     if (strlen($account_key)) {
       $result = $this->loadAccountForRegistrationOrLinking($account_key);
@@ -27,7 +29,7 @@ final class PhabricatorAuthRegisterController
       $is_default = true;
       $is_setup = true;
     } else {
-      list($account, $provider, $response) = $this->loadDefaultAccount();
+      list($account, $provider, $response) = $this->loadDefaultAccount($invite);
       $is_default = true;
     }
 
@@ -35,8 +37,6 @@ final class PhabricatorAuthRegisterController
       return $response;
     }
 
-    $invite = $this->loadInvite();
-
     if (!$is_setup) {
       if (!$provider->shouldAllowRegistration()) {
         if ($invite) {
@@ -643,17 +643,20 @@ final class PhabricatorAuthRegisterController
       ->appendChild($view);
   }
 
-  private function loadDefaultAccount() {
+  private function loadDefaultAccount($invite) {
     $providers = PhabricatorAuthProvider::getAllEnabledProviders();
     $account = null;
     $provider = null;
     $response = null;
 
     foreach ($providers as $key => $candidate_provider) {
-      if (!$candidate_provider->shouldAllowRegistration()) {
-        unset($providers[$key]);
-        continue;
+      if (!$invite) {
+        if (!$candidate_provider->shouldAllowRegistration()) {
+          unset($providers[$key]);
+          continue;
+        }
       }
+
       if (!$candidate_provider->isDefaultRegistrationProvider()) {
         unset($providers[$key]);
       }

From 5a89da12e2931bc8dd2a787cd2d458a56d25cffe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 6 Feb 2019 19:08:03 -0800
Subject: [PATCH 18/36] When users have no password on their account, guide
 them through the "reset password" flow in the guise of "set password"

Summary:
Depends on D20119. Fixes T9512. When you don't have a password on your account, the "Password" panel in Settings is non-obviously useless: you can't provide an old password, so you can't change your password.

The correct remedy is to "Forgot password?" and go through the password reset flow. However, we don't guide you to this and it isn't really self-evident.

Instead:

  - Guide users to the password reset flow.
  - Make it work when you're already logged in.
  - Skin it as a "set password" flow.

We're still requiring you to prove you own the email associated with your account. This is a pretty weak requirement, but maybe stops attackers who use the computer at the library after you do in some bizarre emergency and forget to log out? It would probably be fine to just let users "set password", this mostly just keeps us from having two different pieces of code responsible for setting passwords.

Test Plan:
  - Set password as a logged-in user.
  - Reset password on the normal flow as a logged-out user.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: revi

Maniphest Tasks: T9512

Differential Revision: https://secure.phabricator.com/D20120
---
 .../PhabricatorAuthOneTimeLoginController.php |  26 ++++-
 .../PhabricatorEmailLoginController.php       | 107 +++++++++++++-----
 .../PhabricatorPasswordSettingsPanel.php      |  46 ++++++--
 3 files changed, 134 insertions(+), 45 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index f51f379d2b..26f8785c0a 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -14,11 +14,6 @@ final class PhabricatorAuthOneTimeLoginController
     $key = $request->getURIData('key');
     $email_id = $request->getURIData('emailID');
 
-    if ($request->getUser()->isLoggedIn()) {
-      return $this->renderError(
-        pht('You are already logged in.'));
-    }
-
     $target_user = id(new PhabricatorPeopleQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withIDs(array($id))
@@ -27,6 +22,19 @@ final class PhabricatorAuthOneTimeLoginController
       return new Aphront404Response();
     }
 
+    // NOTE: We allow you to use a one-time login link for your own current
+    // login account. This supports the "Set Password" flow.
+
+    $is_logged_in = false;
+    if ($viewer->isLoggedIn()) {
+      if ($viewer->getPHID() !== $target_user->getPHID()) {
+        return $this->renderError(
+          pht('You are already logged in.'));
+      } else {
+        $is_logged_in = true;
+      }
+    }
+
     // NOTE: As a convenience to users, these one-time login URIs may also
     // be associated with an email address which will be verified when the
     // URI is used.
@@ -100,7 +108,7 @@ final class PhabricatorAuthOneTimeLoginController
         ->addCancelButton('/');
     }
 
-    if ($request->isFormPost()) {
+    if ($request->isFormPost() || $is_logged_in) {
       // If we have an email bound into this URI, verify email so that clicking
       // the link in the "Welcome" email is good enough, without requiring users
       // to go through a second round of email verification.
@@ -121,6 +129,12 @@ final class PhabricatorAuthOneTimeLoginController
 
       $next_uri = $this->getNextStepURI($target_user);
 
+      // If the user is already logged in, we're just doing a "password set"
+      // flow. Skip directly to the next step.
+      if ($is_logged_in) {
+        return id(new AphrontRedirectResponse())->setURI($next_uri);
+      }
+
       PhabricatorCookies::setNextURICookie($request, $next_uri, $force = true);
 
       $force_full_session = false;
diff --git a/src/applications/auth/controller/PhabricatorEmailLoginController.php b/src/applications/auth/controller/PhabricatorEmailLoginController.php
index eef30e6989..76b288f059 100644
--- a/src/applications/auth/controller/PhabricatorEmailLoginController.php
+++ b/src/applications/auth/controller/PhabricatorEmailLoginController.php
@@ -9,20 +9,38 @@ final class PhabricatorEmailLoginController
 
   public function handleRequest(AphrontRequest $request) {
     $viewer = $this->getViewer();
+    $is_logged_in = $viewer->isLoggedIn();
 
     $e_email = true;
     $e_captcha = true;
     $errors = array();
 
-    $v_email = $request->getStr('email');
+    if ($is_logged_in) {
+      if (!$this->isPasswordAuthEnabled()) {
+        return $this->newDialog()
+          ->setTitle(pht('No Password Auth'))
+          ->appendParagraph(
+            pht(
+              'Password authentication is not enabled and you are already '.
+              'logged in. There is nothing for you here.'))
+          ->addCancelButton('/', pht('Continue'));
+      }
+
+      $v_email = $viewer->loadPrimaryEmailAddress();
+    } else {
+      $v_email = $request->getStr('email');
+    }
+
     if ($request->isFormPost()) {
       $e_email = null;
       $e_captcha = pht('Again');
 
-      $captcha_ok = AphrontFormRecaptchaControl::processCaptcha($request);
-      if (!$captcha_ok) {
-        $errors[] = pht('Captcha response is incorrect, try again.');
-        $e_captcha = pht('Invalid');
+      if (!$is_logged_in) {
+        $captcha_ok = AphrontFormRecaptchaControl::processCaptcha($request);
+        if (!$captcha_ok) {
+          $errors[] = pht('Captcha response is incorrect, try again.');
+          $e_captcha = pht('Invalid');
+        }
       }
 
       if (!strlen($v_email)) {
@@ -76,10 +94,24 @@ final class PhabricatorEmailLoginController
         }
 
         if (!$errors) {
-          $body = $this->newAccountLoginMailBody($target_user);
+          $body = $this->newAccountLoginMailBody(
+            $target_user,
+            $is_logged_in);
+
+          if ($is_logged_in) {
+            $subject = pht('[Phabricator] Account Password Link');
+            $instructions = pht(
+              'An email has been sent containing a link you can use to set '.
+              'a password for your account.');
+          } else {
+            $subject = pht('[Phabricator] Account Login Link');
+            $instructions = pht(
+              'An email has been sent containing a link you can use to log '.
+              'in to your account.');
+          }
 
           $mail = id(new PhabricatorMetaMTAMail())
-            ->setSubject(pht('[Phabricator] Account Login Link'))
+            ->setSubject($subject)
             ->setForceDelivery(true)
             ->addRawTos(array($target_email->getAddress()))
             ->setBody($body)
@@ -88,8 +120,7 @@ final class PhabricatorEmailLoginController
           return $this->newDialog()
             ->setTitle(pht('Check Your Email'))
             ->setShortTitle(pht('Email Sent'))
-            ->appendParagraph(
-              pht('An email has been sent with a link you can use to log in.'))
+            ->appendParagraph($instructions)
             ->addCancelButton('/', pht('Done'));
         }
       }
@@ -99,33 +130,47 @@ final class PhabricatorEmailLoginController
       ->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.'));
+      if ($is_logged_in) {
+        $title = pht('Set Password');
+        $form->appendRemarkupInstructions(
+          pht(
+            'A password reset link will be sent to your primary email '.
+            'address. Follow the link to set an account password.'));
+      } else {
+        $title = pht('Password Reset');
+        $form->appendRemarkupInstructions(
+          pht(
+            'To reset your password, provide your email address. An email '.
+            'with a login link will be sent to you.'));
+      }
     } else {
+      $title = pht('Email Login');
       $form->appendRemarkupInstructions(
         pht(
           'To access your account, provide your email address. An email '.
           'with a login link will be sent to you.'));
     }
 
+    if ($is_logged_in) {
+      $address_control = new AphrontFormStaticControl();
+    } else {
+      $address_control = id(new AphrontFormTextControl())
+        ->setName('email')
+        ->setError($e_email);
+    }
+
+    $address_control
+      ->setLabel(pht('Email Address'))
+      ->setValue($v_email);
+
     $form
-      ->appendControl(
-        id(new AphrontFormTextControl())
-          ->setLabel(pht('Email Address'))
-          ->setName('email')
-          ->setValue($v_email)
-          ->setError($e_email))
-      ->appendControl(
+      ->appendControl($address_control);
+
+    if (!$is_logged_in) {
+      $form->appendControl(
         id(new AphrontFormRecaptchaControl())
           ->setLabel(pht('Captcha'))
           ->setError($e_captcha));
-
-    if ($this->isPasswordAuthEnabled()) {
-      $title = pht('Password Reset');
-    } else {
-      $title = pht('Email Login');
     }
 
     return $this->newDialog()
@@ -137,7 +182,10 @@ final class PhabricatorEmailLoginController
       ->addSubmitButton(pht('Send Email'));
   }
 
-  private function newAccountLoginMailBody(PhabricatorUser $user) {
+  private function newAccountLoginMailBody(
+    PhabricatorUser $user,
+    $is_logged_in) {
+
     $engine = new PhabricatorAuthSessionEngine();
     $uri = $engine->getOneTimeLoginURI(
       $user,
@@ -148,7 +196,12 @@ final class PhabricatorEmailLoginController
     $have_passwords = $this->isPasswordAuthEnabled();
 
     if ($have_passwords) {
-      if ($is_serious) {
+      if ($is_logged_in) {
+        $body = pht(
+          'You can use this link to set a password on your account:'.
+          "\n\n  %s\n",
+          $uri);
+      } else if ($is_serious) {
         $body = pht(
           "You can use this link to reset your Phabricator password:".
           "\n\n  %s\n",
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
index 37393d5d4f..77f32f977d 100644
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -34,11 +34,6 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
 
     $content_source = PhabricatorContentSource::newFromRequest($request);
 
-    $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
-      $viewer,
-      $request,
-      '/settings/');
-
     $min_len = PhabricatorEnv::getEnvConfig('account.minimum-password-length');
     $min_len = (int)$min_len;
 
@@ -55,20 +50,25 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
       ->withPasswordTypes(array($account_type))
       ->withIsRevoked(false)
       ->execute();
-    if ($password_objects) {
-      $password_object = head($password_objects);
-    } else {
-      $password_object = PhabricatorAuthPassword::initializeNewPassword(
-        $user,
-        $account_type);
+    if (!$password_objects) {
+      return $this->newSetPasswordView($request);
     }
+    $password_object = head($password_objects);
 
     $e_old = true;
     $e_new = true;
     $e_conf = true;
 
     $errors = array();
-    if ($request->isFormPost()) {
+    if ($request->isFormOrHisecPost()) {
+      $workflow_key = sprintf(
+        'password.change(%s)',
+        $user->getPHID());
+
+      $hisec_token = id(new PhabricatorAuthSessionEngine())
+        ->setWorkflowKey($workflow_key)
+        ->requireHighSecurityToken($viewer, $request, '/settings/');
+
       // Rate limit guesses about the old password. This page requires MFA and
       // session compromise already, so this is mostly just to stop researchers
       // from reporting this as a vulnerability.
@@ -218,5 +218,27 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
     );
   }
 
+  private function newSetPasswordView(AphrontRequest $request) {
+    $viewer = $request->getUser();
+    $user = $this->getUser();
+
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer)
+      ->appendRemarkupInstructions(
+        pht(
+          'Your account does not currently have a password set. You can '.
+          'choose a password by performing a password reset.'))
+      ->appendControl(
+        id(new AphrontFormSubmitControl())
+          ->addCancelButton('/login/email/', pht('Reset Password')));
+
+    $form_box = id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Set Password'))
+      ->setBackground(PHUIObjectBoxView::WHITE_CONFIG)
+      ->setForm($form);
+
+    return $form_box;
+  }
+
 
 }

From 7d6d2c128a9a7dbf788bb5d9b541e7e31480f118 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 10:27:17 -0800
Subject: [PATCH 19/36] Make "bin/audit delete" synchronize commit audit
 status, and improve "bin/audit synchronize" documentation

Summary:
Depends on D20126. See PHI1056. Ref T13244.

  - `bin/audit delete` destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required".
  - `bin/audit synchronize` does this synchronize step, but is poorly documented.

Make `bin/audit delete` synchronize affected commits.

Document `bin/audit synchronize` better.

There's some reasonable argument that `bin/audit synchronize` perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live.

Test Plan:
  - Ran `bin/audit delete`, saw audits destroyed and affected commits synchornized.
  - Ran `bin/audit synchronize`, saw behavior unchanged.
  - Ran `bin/audit help`, got better help.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20127
---
 ...abricatorAuditManagementDeleteWorkflow.php | 106 ++++++++++++------
 .../PhabricatorAuditManagementWorkflow.php    |  35 ++++++
 ...atorAuditSynchronizeManagementWorkflow.php |  43 ++-----
 src/docs/user/userguide/audit.diviner         |   9 +-
 4 files changed, 120 insertions(+), 73 deletions(-)

diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
index 23583422fb..7d14df7239 100644
--- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
+++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
@@ -105,10 +105,10 @@ final class PhabricatorAuditManagementDeleteWorkflow
       $query->withPHIDs(mpull($commits, 'getPHID'));
     }
 
-    $commits = $query->execute();
-    $commits = mpull($commits, null, 'getPHID');
+    $commit_iterator = new PhabricatorQueryIterator($query);
+
     $audits = array();
-    foreach ($commits as $commit) {
+    foreach ($commit_iterator as $commit) {
       $commit_audits = $commit->getAudits();
       foreach ($commit_audits as $key => $audit) {
         if ($id_map && empty($id_map[$audit->getID()])) {
@@ -131,51 +131,87 @@ final class PhabricatorAuditManagementDeleteWorkflow
           continue;
         }
       }
-      $audits[] = $commit_audits;
-    }
-    $audits = array_mergev($audits);
 
-    $console = PhutilConsole::getConsole();
+      if (!$commit_audits) {
+        continue;
+      }
 
-    if (!$audits) {
-      $console->writeErr("%s\n", pht('No audits match the query.'));
-      return 0;
-    }
+      $handles = id(new PhabricatorHandleQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(mpull($commit_audits, 'getAuditorPHID'))
+        ->execute();
 
-    $handles = id(new PhabricatorHandleQuery())
-      ->setViewer($this->getViewer())
-      ->withPHIDs(mpull($audits, 'getAuditorPHID'))
-      ->execute();
+      foreach ($commit_audits as $audit) {
+        $audit_id = $audit->getID();
 
-
-    foreach ($audits as $audit) {
-      $commit = $commits[$audit->getCommitPHID()];
-
-      $console->writeOut(
-        "%s\n",
-        sprintf(
+        $description = sprintf(
           '%10d %-16s %-16s %s: %s',
-          $audit->getID(),
+          $audit_id,
           $handles[$audit->getAuditorPHID()]->getName(),
           PhabricatorAuditStatusConstants::getStatusName(
             $audit->getAuditStatus()),
           $commit->getRepository()->formatCommitName(
             $commit->getCommitIdentifier()),
-          trim($commit->getSummary())));
+          trim($commit->getSummary()));
+
+        $audits[] = array(
+          'auditID' => $audit_id,
+          'commitPHID' => $commit->getPHID(),
+          'description' => $description,
+        );
+      }
     }
 
-    if (!$is_dry_run) {
-      $message = pht(
-        'Really delete these %d audit(s)? They will be permanently deleted '.
-        'and can not be recovered.',
-        count($audits));
-      if ($console->confirm($message)) {
-        foreach ($audits as $audit) {
-          $id = $audit->getID();
-          $console->writeOut("%s\n", pht('Deleting audit %d...', $id));
-          $audit->delete();
-        }
+    if (!$audits) {
+      echo tsprintf(
+        "%s\n",
+        pht('No audits match the query.'));
+      return 0;
+    }
+
+    foreach ($audits as $audit_spec) {
+      echo tsprintf(
+        "%s\n",
+        $audit_spec['description']);
+    }
+
+    if ($is_dry_run) {
+      echo tsprintf(
+        "%s\n",
+        pht('This is a dry run, so no changes will be made.'));
+      return 0;
+    }
+
+    $message = pht(
+      'Really delete these %s audit(s)? They will be permanently deleted '.
+      'and can not be recovered.',
+      phutil_count($audits));
+    if (!phutil_console_confirm($message)) {
+      echo tsprintf(
+        "%s\n",
+        pht('User aborted the workflow.'));
+      return 1;
+    }
+
+    $audits_by_commit = igroup($audits, 'commitPHID');
+    foreach ($audits_by_commit as $commit_phid => $audit_specs) {
+      $audit_ids = ipull($audit_specs, 'auditID');
+
+      $audits = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere(
+        'id IN (%Ld)',
+        $audit_ids);
+
+      foreach ($audits as $audit) {
+        $id = $audit->getID();
+
+        echo tsprintf(
+          "%s\n",
+          pht('Deleting audit %d...', $id));
+
+        $audit->delete();
       }
+
+      $this->synchronizeCommitAuditState($commit_phid);
     }
 
     return 0;
diff --git a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php
index 6112a38e1d..b9d90bddc8 100644
--- a/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php
+++ b/src/applications/audit/management/PhabricatorAuditManagementWorkflow.php
@@ -87,4 +87,39 @@ abstract class PhabricatorAuditManagementWorkflow
     return $commits;
   }
 
+  protected function synchronizeCommitAuditState($commit_phid) {
+    $viewer = $this->getViewer();
+
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($commit_phid))
+      ->needAuditRequests(true)
+      ->executeOne();
+    if (!$commit) {
+      return;
+    }
+
+    $old_status = $commit->getAuditStatusObject();
+    $commit->updateAuditStatus($commit->getAudits());
+    $new_status = $commit->getAuditStatusObject();
+
+    if ($old_status->getKey() == $new_status->getKey()) {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'No synchronization changes for "%s".',
+          $commit->getDisplayName()));
+    } else {
+      echo tsprintf(
+        "%s\n",
+        pht(
+          'Synchronizing "%s": "%s" -> "%s".',
+          $commit->getDisplayName(),
+          $old_status->getName(),
+          $new_status->getName()));
+
+      $commit->save();
+    }
+  }
+
 }
diff --git a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php
index 96d06e65c2..abd0a3c637 100644
--- a/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php
+++ b/src/applications/audit/management/PhabricatorAuditSynchronizeManagementWorkflow.php
@@ -6,8 +6,16 @@ final class PhabricatorAuditSynchronizeManagementWorkflow
   protected function didConstruct() {
     $this
       ->setName('synchronize')
-      ->setExamples('**synchronize** ...')
-      ->setSynopsis(pht('Update audit status for commits.'))
+      ->setExamples(
+        "**synchronize** __repository__ ...\n".
+        "**synchronize** __commit__ ...\n".
+        "**synchronize** --all")
+      ->setSynopsis(
+        pht(
+          'Update commits to make their summary audit state reflect the '.
+          'state of their actual audit requests. This can fix inconsistencies '.
+          'in database state if audit requests have been mangled '.
+          'accidentally (or on purpose).'))
       ->setArguments(
         array_merge(
           $this->getCommitConstraintArguments(),
@@ -21,36 +29,7 @@ final class PhabricatorAuditSynchronizeManagementWorkflow
     foreach ($objects as $object) {
       $commits = $this->loadCommitsForConstraintObject($object);
       foreach ($commits as $commit) {
-        $commit = id(new DiffusionCommitQuery())
-          ->setViewer($viewer)
-          ->withPHIDs(array($commit->getPHID()))
-          ->needAuditRequests(true)
-          ->executeOne();
-        if (!$commit) {
-          continue;
-        }
-
-        $old_status = $commit->getAuditStatusObject();
-        $commit->updateAuditStatus($commit->getAudits());
-        $new_status = $commit->getAuditStatusObject();
-
-        if ($old_status->getKey() == $new_status->getKey()) {
-          echo tsprintf(
-            "%s\n",
-            pht(
-              'No changes for "%s".',
-              $commit->getDisplayName()));
-        } else {
-          echo tsprintf(
-            "%s\n",
-            pht(
-              'Updating "%s": "%s" -> "%s".',
-              $commit->getDisplayName(),
-              $old_status->getName(),
-              $new_status->getName()));
-
-          $commit->save();
-        }
+        $this->synchronizeCommitAuditState($commit->getPHID());
       }
     }
   }
diff --git a/src/docs/user/userguide/audit.diviner b/src/docs/user/userguide/audit.diviner
index 0d10867906..5223f6c969 100644
--- a/src/docs/user/userguide/audit.diviner
+++ b/src/docs/user/userguide/audit.diviner
@@ -175,16 +175,13 @@ You can use this command to forcibly delete requests which may have triggered
 incorrectly (for example, because a package or Herald rule was configured in an
 overbroad way).
 
-After deleting audits, you may want to run `bin/audit synchronize` to
-synchronize audit state.
-
 **Synchronize Audit State**: Synchronize the audit state of commits to the
 current open audit requests with `bin/audit synchronize`.
 
 Normally, overall audit state is automatically kept up to date as changes are
-made to an audit. However, if you delete audits or manually update the database
-to make changes to audit request state, the state of corresponding commits may
-no longer be correct.
+made to an audit. However, if you manually update the database to make changes
+to audit request state, the state of corresponding commits may no longer be
+consistent.
 
 This command will update commits so their overall audit state reflects the
 cumulative state of their actual audit requests.

From a7bf279fd517bbc914b1116563119c7a7c1a7be2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 15:24:45 -0800
Subject: [PATCH 20/36] Don't try to publish build results to bare diffs

Summary:
See <https://discourse.phabricator-community.org/t/conduit-call-is-generating-phd-log-error-message/2380/>. If you run builds against a diff which is not attached to a revision (this is unusual) we still try to publish to the associated revision. This won't work since there is no associated revision.

Since bare diffs don't really have a timeline, just publish nowhere for now.

Test Plan:
  - Created a diff.
  - Did not attach it to a revision.
  - Created a build plan with "make http request + wait for response".
  - Manually ran the build plan against the bare diff.
  - Used `bin/phd debug task` to run the build and hit a "revision not attached" exception during publishing.
  - Applied patch.
  - Ran `bin/phd debug task`, got clean (no-op) publish.
  - Sent build a failure message with "harbormaster.sendmessage", got a failed build.

This isn't a real workflow, but shouldn't fail.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20156
---
 .../harbormaster/DifferentialBuildableEngine.php            | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/applications/differential/harbormaster/DifferentialBuildableEngine.php b/src/applications/differential/harbormaster/DifferentialBuildableEngine.php
index 8554f7be25..8565c2dcad 100644
--- a/src/applications/differential/harbormaster/DifferentialBuildableEngine.php
+++ b/src/applications/differential/harbormaster/DifferentialBuildableEngine.php
@@ -7,7 +7,11 @@ final class DifferentialBuildableEngine
     $object = $this->getObject();
 
     if ($object instanceof DifferentialDiff) {
-      return $object->getRevision();
+      if ($object->getRevisionID()) {
+        return $object->getRevision();
+      } else {
+        return null;
+      }
     }
 
     return $object;

From 991368128e4dc7819f0772f25f7cca4a6a428c41 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 08:56:54 -0800
Subject: [PATCH 21/36] Bump the markup cache version for URI changes

Summary:
Ref T13250. Ref T13249. Some remarkup rules, including `{image ...}` and `{meme ...}`, may cache URIs as objects because the remarkup cache is `serialize()`-based.

URI objects with `query` cached as a key-value map are no longer valid and can raise `__toString()` fatals.

Bump the cache version to purge them out of the cache.

Test Plan: See PHI1074.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250, T13249

Differential Revision: https://secure.phabricator.com/D20160
---
 src/infrastructure/markup/PhabricatorMarkupEngine.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php
index 868bbc5676..3a63cb97a6 100644
--- a/src/infrastructure/markup/PhabricatorMarkupEngine.php
+++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php
@@ -42,7 +42,7 @@ final class PhabricatorMarkupEngine extends Phobject {
   private $objects = array();
   private $viewer;
   private $contextObject;
-  private $version = 17;
+  private $version = 18;
   private $engineCaches = array();
   private $auxiliaryConfig = array();
 

From 9a9fa8bed283043815bf427562922275f93ab5fe Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 05:14:43 -0800
Subject: [PATCH 22/36] Rate limit attempts to add payment methods in Phortune

Summary: Ref T13249. See D20132. Although we're probably a poor way to validate a big list of stolen cards in practice in production today (it's very hard to quickly generate a large number of small charges), putting rate limiting on "Add Payment Method" is generally reasonable, can't really hurt anything (no legitimate user will ever hit this limit), and might frustrate attackers in the future if it becomes easier to generate ad-hoc charges (for example, if we run a deal on support pacts and reduce their cost from $1,000 to $1).

Test Plan: Reduced limit to 4 / hour, tried to add a card several times, got rate limited.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20158
---
 src/__phutil_library_map__.php                |  2 ++
 .../action/PhortuneAddPaymentMethodAction.php | 22 +++++++++++++++++++
 .../PhortunePaymentMethodCreateController.php |  9 ++++++++
 3 files changed, 33 insertions(+)
 create mode 100644 src/applications/phortune/action/PhortuneAddPaymentMethodAction.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 6c21edc0da..8f06586a68 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -4986,6 +4986,7 @@ phutil_register_library_map(array(
     'PhortuneAccountViewController' => 'applications/phortune/controller/account/PhortuneAccountViewController.php',
     'PhortuneAdHocCart' => 'applications/phortune/cart/PhortuneAdHocCart.php',
     'PhortuneAdHocProduct' => 'applications/phortune/product/PhortuneAdHocProduct.php',
+    'PhortuneAddPaymentMethodAction' => 'applications/phortune/action/PhortuneAddPaymentMethodAction.php',
     'PhortuneCart' => 'applications/phortune/storage/PhortuneCart.php',
     'PhortuneCartAcceptController' => 'applications/phortune/controller/cart/PhortuneCartAcceptController.php',
     'PhortuneCartCancelController' => 'applications/phortune/controller/cart/PhortuneCartCancelController.php',
@@ -11227,6 +11228,7 @@ phutil_register_library_map(array(
     'PhortuneAccountViewController' => 'PhortuneAccountProfileController',
     'PhortuneAdHocCart' => 'PhortuneCartImplementation',
     'PhortuneAdHocProduct' => 'PhortuneProductImplementation',
+    'PhortuneAddPaymentMethodAction' => 'PhabricatorSystemAction',
     'PhortuneCart' => array(
       'PhortuneDAO',
       'PhabricatorApplicationTransactionInterface',
diff --git a/src/applications/phortune/action/PhortuneAddPaymentMethodAction.php b/src/applications/phortune/action/PhortuneAddPaymentMethodAction.php
new file mode 100644
index 0000000000..09a8cd2f5d
--- /dev/null
+++ b/src/applications/phortune/action/PhortuneAddPaymentMethodAction.php
@@ -0,0 +1,22 @@
+<?php
+
+final class PhortuneAddPaymentMethodAction
+  extends PhabricatorSystemAction {
+
+  const TYPECONST = 'phortune.payment-method.add';
+
+  public function getActionConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getScoreThreshold() {
+    return 60 / phutil_units('1 hour in seconds');
+  }
+
+  public function getLimitExplanation() {
+    return pht(
+      'You are making too many attempts to add payment methods in a short '.
+      'period of time.');
+  }
+
+}
diff --git a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
index 521508e0e8..847fbf5716 100644
--- a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
+++ b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
@@ -82,6 +82,15 @@ final class PhortunePaymentMethodCreateController
         ->setProviderPHID($provider->getProviderConfig()->getPHID())
         ->setStatus(PhortunePaymentMethod::STATUS_ACTIVE);
 
+      // Limit the rate at which you can attempt to add payment methods. This
+      // is intended as a line of defense against using Phortune to validate a
+      // large list of stolen credit card numbers.
+
+      PhabricatorSystemActionEngine::willTakeAction(
+        array($viewer->getPHID()),
+        new PhortuneAddPaymentMethodAction(),
+        1);
+
       if (!$errors) {
         $errors = $this->processClientErrors(
           $provider,

From 88d5233b7776cae0fcedf85fc9ce22615c036d92 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 04:38:41 -0800
Subject: [PATCH 23/36] Fix specifications of some "Visual Only" elements

Summary: See PHI823. These got "visual-only" but should acutally get "aural => false" to pick up "aria-hidden".

Test Plan: Viewed page source, saw both "visual-only" and "aria-hidden".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20157
---
 .../view/PhabricatorApplicationTransactionCommentView.php    | 5 +++--
 src/view/phui/PHUIHeadThingView.php                          | 5 +++--
 src/view/phui/PHUITimelineEventView.php                      | 5 +++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
index f6a27d4bcd..1fb850887f 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
@@ -254,11 +254,12 @@ final class PhabricatorApplicationTransactionCommentView
 
     require_celerity_resource('phui-comment-form-css');
     $image_uri = $viewer->getProfileImageURI();
-    $image = phutil_tag(
+    $image = javelin_tag(
       'div',
       array(
         'style' => 'background-image: url('.$image_uri.')',
-        'class' => 'phui-comment-image visual-only',
+        'class' => 'phui-comment-image',
+        'aural' => false,
       ));
     $wedge = phutil_tag(
       'div',
diff --git a/src/view/phui/PHUIHeadThingView.php b/src/view/phui/PHUIHeadThingView.php
index ab2feee984..219ed28be0 100644
--- a/src/view/phui/PHUIHeadThingView.php
+++ b/src/view/phui/PHUIHeadThingView.php
@@ -52,12 +52,13 @@ final class PHUIHeadThingView extends AphrontTagView {
 
   protected function getTagContent() {
 
-    $image = phutil_tag(
+    $image = javelin_tag(
       'a',
       array(
-        'class' => 'phui-head-thing-image visual-only',
+        'class' => 'phui-head-thing-image',
         'style' => 'background-image: url('.$this->image.');',
         'href' => $this->imageHref,
+        'aural' => false,
       ));
 
     if ($this->image) {
diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php
index 78a75a2063..5013611084 100644
--- a/src/view/phui/PHUITimelineEventView.php
+++ b/src/view/phui/PHUITimelineEventView.php
@@ -420,12 +420,13 @@ final class PHUITimelineEventView extends AphrontView {
     $image = null;
     $badges = null;
     if ($image_uri) {
-      $image = phutil_tag(
+      $image = javelin_tag(
         ($this->userHandle->getURI()) ? 'a' : 'div',
         array(
           'style' => 'background-image: url('.$image_uri.')',
-          'class' => 'phui-timeline-image visual-only',
+          'class' => 'phui-timeline-image',
           'href' => $this->userHandle->getURI(),
+          'aural' => false,
         ),
         '');
       if ($this->badges && $show_badges) {

From eb73cb68ff5b6ae131c9e11bf56d0cfeb6bbd7b7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 06:37:49 -0800
Subject: [PATCH 24/36] Raise a setup warning when locked configuration has a
 configuration value stored in the database

Summary:
Ref T13249. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>.

Today, when a configuration value is "locked", we prevent //writes// to the database. However, we still perform reads. When you upgrade, we generally don't want a bunch of your configuration to change by surprise.

Some day, I'd like to stop reading locked configuration from the database. This would defuse an escalation where an attacker finds a way to write to locked configuration despite safeguards, e.g. through SQL injection or policy bypass. Today, they could write to `cluster.mailers` or similar and substantially escalate access. A better behavior would be to ignore database values for `cluster.mailers` and other locked config, so that these impermissible writes have no effect.

Doing this today would break a lot of installs, but we can warn them about it now and then make the change at a later date.

Test Plan:
  - Forced a `phd.taskmasters` config value into the database.
  - Saw setup warning.
  - Used `bin/config delete --database phd.taskmasters` to clear the warning.
  - Reviewed documentation changes.
  - Reviewed `phd.taskmasters` documentation adjustment.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20159
---
 .../PhabricatorExtraConfigSetupCheck.php      | 101 +++++++++++++++++-
 .../option/PhabricatorPHDConfigOptions.php    |   7 +-
 .../configuration_locked.diviner              |  49 +++++++++
 3 files changed, 153 insertions(+), 4 deletions(-)

diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
index 932e04db4b..a742e3b82b 100644
--- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
+++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
@@ -15,6 +15,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
 
     $defined_keys = PhabricatorApplicationConfigOptions::loadAllOptions();
 
+    $stack = PhabricatorEnv::getConfigSourceStack();
+    $stack = $stack->getStack();
+
     foreach ($all_keys as $key) {
       if (isset($defined_keys[$key])) {
         continue;
@@ -48,9 +51,6 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
         ->setName($name)
         ->setSummary($summary);
 
-      $stack = PhabricatorEnv::getConfigSourceStack();
-      $stack = $stack->getStack();
-
       $found = array();
       $found_local = false;
       $found_database = false;
@@ -85,6 +85,101 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
       }
     }
 
+    $options = PhabricatorApplicationConfigOptions::loadAllOptions();
+    foreach ($defined_keys as $key => $value) {
+      $option = idx($options, $key);
+      if (!$option) {
+        continue;
+      }
+
+      if (!$option->getLocked()) {
+        continue;
+      }
+
+      $found_database = false;
+      foreach ($stack as $source_key => $source) {
+        $value = $source->getKeys(array($key));
+        if ($value) {
+          if ($source instanceof PhabricatorConfigDatabaseSource) {
+            $found_database = true;
+            break;
+          }
+        }
+      }
+
+      if (!$found_database) {
+        continue;
+      }
+
+      // NOTE: These are values which we don't let you edit directly, but edit
+      // via other UI workflows. For now, don't raise this warning about them.
+      // In the future, before we stop reading database configuration for
+      // locked values, we either need to add a flag which lets these values
+      // continue reading from the database or move them to some other storage
+      // mechanism.
+      $soft_locks = array(
+        'phabricator.uninstalled-applications',
+        'phabricator.application-settings',
+        'config.ignore-issues',
+      );
+      $soft_locks = array_fuse($soft_locks);
+      if (isset($soft_locks[$key])) {
+        continue;
+      }
+
+      $doc_name = 'Configuration Guide: Locked and Hidden Configuration';
+      $doc_href = PhabricatorEnv::getDoclink($doc_name);
+
+      $set_command = phutil_tag(
+        'tt',
+        array(),
+        csprintf(
+          'bin/config set %R <value>',
+          $key));
+
+      $summary = pht(
+        'Configuration value "%s" is locked, but has a value in the database.',
+        $key);
+      $message = pht(
+        'The configuration value "%s" is locked (so it can not be edited '.
+        'from the web UI), but has a database value. Usually, this means '.
+        'that it was previously not locked, you set it using the web UI, '.
+        'and it later became locked.'.
+        "\n\n".
+        'You should copy this configuration value in a local configuration '.
+        'source (usually by using %s) and then remove it from the database '.
+        'with the command below.'.
+        "\n\n".
+        'For more information on locked and hidden configuration, including '.
+        'details about this setup issue, see %s.'.
+        "\n\n".
+        'This database value is currently respected, but a future version '.
+        'of Phabricator will stop respecting database values for locked '.
+        'configuration options.',
+        $key,
+        $set_command,
+        phutil_tag(
+          'a',
+          array(
+            'href' => $doc_href,
+            'target' => '_blank',
+          ),
+          $doc_name));
+      $command = csprintf(
+        'phabricator/ $ ./bin/config delete --database %R',
+        $key);
+
+      $this->newIssue('config.locked.'.$key)
+        ->setShortName(pht('Deprecated Config Source'))
+        ->setName(
+          pht(
+            'Locked Configuration Option "%s" Has Database Value',
+            $key))
+        ->setSummary($summary)
+        ->setMessage($message)
+        ->addCommand($command)
+        ->addPhabricatorConfig($key);
+    }
 
     if (PhabricatorEnv::getEnvConfig('feed.http-hooks')) {
       $this->newIssue('config.deprecated.feed.http-hooks')
diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php
index 37fae45dfb..e04353876a 100644
--- a/src/applications/config/option/PhabricatorPHDConfigOptions.php
+++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php
@@ -41,7 +41,12 @@ final class PhabricatorPHDConfigOptions
             "If you are running a cluster, this limit applies separately ".
             "to each instance of `phd`. For example, if this limit is set ".
             "to `4` and you have three hosts running daemons, the effective ".
-            "global limit will be 12.")),
+            "global limit will be 12.".
+            "\n\n".
+            "After changing this value, you must restart the daemons. Most ".
+            "configuration changes are picked up by the daemons ".
+            "automatically, but pool sizes can not be changed without a ".
+            "restart.")),
       $this->newOption('phd.verbose', 'bool', false)
         ->setLocked(true)
         ->setBoolOptions(
diff --git a/src/docs/user/configuration/configuration_locked.diviner b/src/docs/user/configuration/configuration_locked.diviner
index 958124c381..f96adc2d82 100644
--- a/src/docs/user/configuration/configuration_locked.diviner
+++ b/src/docs/user/configuration/configuration_locked.diviner
@@ -111,6 +111,55 @@ phabricator/ $ ./bin/config set <key> <value>
 ```
 
 
+Locked Configuration With Database Values
+=========================================
+
+You may receive a setup issue warning you that a locked configuration key has a
+value set in the database. Most commonly, this is because:
+
+  - In some earlier version of Phabricator, this configuration was not locked.
+  - In the past, you or some other administrator used the web UI to set a
+    value. This value was written to the database.
+  - In a later version of the software, the value became locked.
+
+When Phabricator was originally released, locked configuration did not yet
+exist. Locked configuration was introduced later, and then configuration options
+were gradually locked for a long time after that.
+
+In some cases the meaning of a value changed and it became possible to use it
+to break an install or the configuration became a security risk. In other
+cases, we identified an existing security risk or arrived at some other reason
+to lock the value.
+
+Locking values was more common in the past, and it is now relatively rare for
+an unlocked value to become locked: when new values are introduced, they are
+generally locked or hidden appropriately. In most cases, this setup issue only
+affects installs that have used Phabricator for a long time.
+
+At time of writing (February 2019), Phabricator currently respects these old
+database values. However, some future version of Phabricator will refuse to
+read locked configuration from the database, because this improves security if
+an attacker manages to find a way to bypass restrictions on editing locked
+configuration from the web UI.
+
+To clear this setup warning and avoid surprise behavioral changes in the future,
+you should move these configuration values from the database to a local config
+file. Usually, you'll do this by first copying the value from the database:
+
+```
+phabricator/ $ ./bin/config set <key> <value>
+```
+
+...and then removing the database value:
+
+```
+phabricator/ $ ./bin/config delete --database <key>
+```
+
+See @{Configuration User Guide: Advanced Configuration} for some more detailed
+discussion of different configuration sources.
+
+
 Next Steps
 ==========
 

From 4c124201623acfe27487faf715a4045cd72c5d6b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 10:45:33 -0800
Subject: [PATCH 25/36] Replace "URI->setQueryParams()" after initialization
 with a constructor argument

Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20151
---
 .../almanac/controller/AlmanacController.php  | 18 +++------
 .../PhabricatorAuthOneTimeLoginController.php | 10 ++---
 .../auth/future/PhabricatorDuoFuture.php      | 19 +++++----
 .../diffusion/view/DiffusionView.php          | 12 +++---
 .../markup/DivinerSymbolRemarkupRule.php      | 18 ++++-----
 .../herald/controller/HeraldNewController.php | 40 +++++++++----------
 .../PhabricatorOwnersDetailController.php     | 10 ++---
 .../cart/PhortuneCartCheckoutController.php   | 12 +++---
 .../PhortunePayPalPaymentProvider.php         | 14 ++++---
 .../provider/PhortunePaymentProvider.php      |  3 +-
 .../storage/PhabricatorRepository.php         |  8 +---
 .../editengine/PhabricatorEditEngine.php      |  3 +-
 .../PhabricatorTypeaheadDatasource.php        |  8 ++--
 13 files changed, 83 insertions(+), 92 deletions(-)

diff --git a/src/applications/almanac/controller/AlmanacController.php b/src/applications/almanac/controller/AlmanacController.php
index 24a8986766..7fad62a509 100644
--- a/src/applications/almanac/controller/AlmanacController.php
+++ b/src/applications/almanac/controller/AlmanacController.php
@@ -67,19 +67,13 @@ abstract class AlmanacController
       $is_builtin = isset($builtins[$key]);
       $is_persistent = (bool)$property->getID();
 
-      $delete_uri = id(new PhutilURI($delete_base))
-        ->setQueryParams(
-          array(
-            'key' => $key,
-            'objectPHID' => $object->getPHID(),
-          ));
+      $params = array(
+        'key' => $key,
+        'objectPHID' => $object->getPHID(),
+      );
 
-      $edit_uri = id(new PhutilURI($edit_base))
-        ->setQueryParams(
-          array(
-            'key' => $key,
-            'objectPHID' => $object->getPHID(),
-          ));
+      $delete_uri = new PhutilURI($delete_base, $params);
+      $edit_uri = new PhutilURI($edit_base, $params);
 
       $delete = javelin_tag(
         'a',
diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index 26f8785c0a..353f31562c 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -218,11 +218,11 @@ final class PhabricatorAuthOneTimeLoginController
 
       $request->setTemporaryCookie(PhabricatorCookies::COOKIE_HISEC, 'yes');
 
-      return (string)id(new PhutilURI($panel_uri))
-        ->setQueryParams(
-          array(
-            'key' => $key,
-          ));
+      $params = array(
+        'key' => $key,
+      );
+
+      return (string)new PhutilURI($panel_uri, $params);
     }
 
     $providers = id(new PhabricatorAuthProviderConfigQuery())
diff --git a/src/applications/auth/future/PhabricatorDuoFuture.php b/src/applications/auth/future/PhabricatorDuoFuture.php
index 81a5a2a2b8..1e70ec2a57 100644
--- a/src/applications/auth/future/PhabricatorDuoFuture.php
+++ b/src/applications/auth/future/PhabricatorDuoFuture.php
@@ -80,11 +80,6 @@ final class PhabricatorDuoFuture
       $host = $this->apiHostname;
       $host = phutil_utf8_strtolower($host);
 
-      $uri = id(new PhutilURI(''))
-        ->setProtocol('https')
-        ->setDomain($host)
-        ->setPath($path);
-
       $data = $this->parameters;
       $date = date('r');
 
@@ -109,11 +104,19 @@ final class PhabricatorDuoFuture
       $signature = new PhutilOpaqueEnvelope($signature);
 
       if ($http_method === 'GET') {
-        $uri->setQueryParams($data);
-        $data = array();
+        $uri_data = $data;
+        $body_data = array();
+      } else {
+        $uri_data = array();
+        $body_data = $data;
       }
 
-      $future = id(new HTTPSFuture($uri, $data))
+      $uri = id(new PhutilURI('', $uri_data))
+        ->setProtocol('https')
+        ->setDomain($host)
+        ->setPath($path);
+
+      $future = id(new HTTPSFuture($uri, $body_data))
         ->setHTTPBasicAuthCredentials($this->integrationKey, $signature)
         ->setMethod($http_method)
         ->addHeader('Accept', 'application/json')
diff --git a/src/applications/diffusion/view/DiffusionView.php b/src/applications/diffusion/view/DiffusionView.php
index eb7f3eb72f..123d22af5e 100644
--- a/src/applications/diffusion/view/DiffusionView.php
+++ b/src/applications/diffusion/view/DiffusionView.php
@@ -81,12 +81,12 @@ abstract class DiffusionView extends AphrontView {
     }
 
     if (isset($details['external'])) {
-      $href = id(new PhutilURI('/diffusion/external/'))
-        ->setQueryParams(
-          array(
-            'uri' => idx($details, 'external'),
-            'id'  => idx($details, 'hash'),
-          ));
+      $params = array(
+        'uri' => idx($details, 'external'),
+        'id'  => idx($details, 'hash'),
+      );
+
+      $href = new PhutilURI('/diffusion/external/', $params);
       $tip = pht('Browse External');
     } else {
       $href = $this->getDiffusionRequest()->generateURI(
diff --git a/src/applications/diviner/markup/DivinerSymbolRemarkupRule.php b/src/applications/diviner/markup/DivinerSymbolRemarkupRule.php
index 7fdb4cb37e..db68927625 100644
--- a/src/applications/diviner/markup/DivinerSymbolRemarkupRule.php
+++ b/src/applications/diviner/markup/DivinerSymbolRemarkupRule.php
@@ -111,15 +111,15 @@ final class DivinerSymbolRemarkupRule extends PhutilRemarkupRule {
         // Here, we're generating comment text or something like that. Just
         // link to Diviner and let it sort things out.
 
-        $href = id(new PhutilURI('/diviner/find/'))
-          ->setQueryParams(
-            array(
-              'book' => $ref->getBook(),
-              'name' => $ref->getName(),
-              'type' => $ref->getType(),
-              'context' => $ref->getContext(),
-              'jump' => true,
-            ));
+        $params = array(
+          'book' => $ref->getBook(),
+          'name' => $ref->getName(),
+          'type' => $ref->getType(),
+          'context' => $ref->getContext(),
+          'jump' => true,
+        );
+
+        $href = new PhutilURI('/diviner/find/', $params);
       }
 
       // TODO: This probably is not the best place to do this. Move it somewhere
diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php
index fbaf1aeb9e..f571aeb395 100644
--- a/src/applications/herald/controller/HeraldNewController.php
+++ b/src/applications/herald/controller/HeraldNewController.php
@@ -81,13 +81,13 @@ final class HeraldNewController extends HeraldController {
         }
 
         if (!$errors && $done) {
-          $uri = id(new PhutilURI('edit/'))
-            ->setQueryParams(
-              array(
-                'content_type' => $content_type,
-                'rule_type' => $rule_type,
-                'targetPHID' => $target_phid,
-              ));
+          $params = array(
+            'content_type' => $content_type,
+            'rule_type' => $rule_type,
+            'targetPHID' => $target_phid,
+          );
+
+          $uri = new PhutilURI('edit/', $params);
           $uri = $this->getApplicationURI($uri);
           return id(new AphrontRedirectResponse())->setURI($uri);
         }
@@ -126,13 +126,13 @@ final class HeraldNewController extends HeraldController {
           ->addHiddenInput('step', 2)
           ->appendChild($rule_types);
 
+        $params = array(
+          'content_type' => $content_type,
+          'step' => '0',
+        );
+
         $cancel_text = pht('Back');
-        $cancel_uri = id(new PhutilURI('new/'))
-          ->setQueryParams(
-            array(
-              'content_type' => $content_type,
-              'step' => 0,
-            ));
+        $cancel_uri = new PhutilURI('new/', $params);
         $cancel_uri = $this->getApplicationURI($cancel_uri);
         $title = pht('Create Herald Rule: %s',
           idx($content_type_map, $content_type));
@@ -173,14 +173,14 @@ final class HeraldNewController extends HeraldController {
               ->setValue($request->getStr('objectName'))
               ->setLabel(pht('Object')));
 
+        $params = array(
+          'content_type' => $content_type,
+          'rule_type' => $rule_type,
+          'step' => 1,
+        );
+
         $cancel_text = pht('Back');
-        $cancel_uri = id(new PhutilURI('new/'))
-          ->setQueryParams(
-            array(
-              'content_type' => $content_type,
-              'rule_type' => $rule_type,
-              'step' => 1,
-            ));
+        $cancel_uri = new PhutilURI('new/', $params);
         $cancel_uri = $this->getApplicationURI($cancel_uri);
         $title = pht('Create Herald Rule: %s',
           idx($content_type_map, $content_type));
diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
index e28ae2b3bb..c458e4dbd1 100644
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@@ -65,11 +65,11 @@ final class PhabricatorOwnersDetailController
 
     $commit_views = array();
 
-    $commit_uri = id(new PhutilURI('/diffusion/commit/'))
-      ->setQueryParams(
-        array(
-          'package' => $package->getPHID(),
-        ));
+    $params = array(
+      'package' => $package->getPHID(),
+    );
+
+    $commit_uri = new PhutilURI('/diffusion/commit/', $params);
 
     $status_concern = DiffusionCommitAuditStatus::CONCERN_RAISED;
 
diff --git a/src/applications/phortune/controller/cart/PhortuneCartCheckoutController.php b/src/applications/phortune/controller/cart/PhortuneCartCheckoutController.php
index dd611ecb7b..874ecf63aa 100644
--- a/src/applications/phortune/controller/cart/PhortuneCartCheckoutController.php
+++ b/src/applications/phortune/controller/cart/PhortuneCartCheckoutController.php
@@ -134,13 +134,13 @@ final class PhortuneCartCheckoutController
 
     $account_id = $account->getID();
 
+    $params = array(
+      'merchantID' => $merchant->getID(),
+      'cartID' => $cart->getID(),
+    );
+
     $payment_method_uri = $this->getApplicationURI("{$account_id}/card/new/");
-    $payment_method_uri = new PhutilURI($payment_method_uri);
-    $payment_method_uri->setQueryParams(
-      array(
-        'merchantID' => $merchant->getID(),
-        'cartID' => $cart->getID(),
-      ));
+    $payment_method_uri = new PhutilURI($payment_method_uri, $params);
 
     $form = id(new AphrontFormView())
       ->setUser($viewer)
diff --git a/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php b/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php
index 078141f8a1..262606ca62 100644
--- a/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php
+++ b/src/applications/phortune/provider/PhortunePayPalPaymentProvider.php
@@ -348,12 +348,14 @@ final class PhortunePayPalPaymentProvider extends PhortunePaymentProvider {
           ->setRawPayPalQuery('SetExpressCheckout', $params)
           ->resolve();
 
-        $uri = new PhutilURI('https://www.sandbox.paypal.com/cgi-bin/webscr');
-        $uri->setQueryParams(
-          array(
-            'cmd'   => '_express-checkout',
-            'token' => $result['TOKEN'],
-          ));
+        $params = array(
+          'cmd'   => '_express-checkout',
+          'token' => $result['TOKEN'],
+        );
+
+        $uri = new PhutilURI(
+          'https://www.sandbox.paypal.com/cgi-bin/webscr',
+          $params);
 
         $cart->setMetadataValue('provider.checkoutURI', (string)$uri);
         $cart->save();
diff --git a/src/applications/phortune/provider/PhortunePaymentProvider.php b/src/applications/phortune/provider/PhortunePaymentProvider.php
index 90e354c5dc..57b2956ecb 100644
--- a/src/applications/phortune/provider/PhortunePaymentProvider.php
+++ b/src/applications/phortune/provider/PhortunePaymentProvider.php
@@ -273,8 +273,7 @@ abstract class PhortunePaymentProvider extends Phobject {
     $app = PhabricatorApplication::getByClass('PhabricatorPhortuneApplication');
     $path = $app->getBaseURI().'provider/'.$id.'/'.$action.'/';
 
-    $uri = new PhutilURI($path);
-    $uri->setQueryParams($params);
+    $uri = new PhutilURI($path, $params);
 
     if ($local) {
       return $uri;
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index e66fb78afa..d5ad83b6d6 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -820,8 +820,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       return $uri;
     }
 
-    $uri = new PhutilURI($uri);
-
     if (isset($params['lint'])) {
       $params['params'] = idx($params, 'params', array()) + array(
         'lint' => $params['lint'],
@@ -830,11 +828,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
     $query = idx($params, 'params', array()) + $query;
 
-    if ($query) {
-      $uri->setQueryParams($query);
-    }
-
-    return $uri;
+    return new PhutilURI($uri, $query);
   }
 
   public function updateURIIndex() {
diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php
index d1a2fb72b9..353d7ee385 100644
--- a/src/applications/transactions/editengine/PhabricatorEditEngine.php
+++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php
@@ -1541,8 +1541,7 @@ abstract class PhabricatorEditEngine
         $config_uri = $config->getCreateURI();
 
         if ($parameters) {
-          $config_uri = (string)id(new PhutilURI($config_uri))
-            ->setQueryParams($parameters);
+          $config_uri = (string)new PhutilURI($config_uri, $parameters);
         }
 
         $specs[] = array(
diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
index 43cc72eaaa..e077d7a7ec 100644
--- a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
+++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
@@ -99,8 +99,8 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
   }
 
   public function getDatasourceURI() {
-    $uri = new PhutilURI('/typeahead/class/'.get_class($this).'/');
-    $uri->setQueryParams($this->newURIParameters());
+    $params = $this->newURIParameters();
+    $uri = new PhutilURI('/typeahead/class/'.get_class($this).'/', $params);
     return phutil_string_cast($uri);
   }
 
@@ -109,8 +109,8 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject {
       return null;
     }
 
-    $uri = new PhutilURI('/typeahead/browse/'.get_class($this).'/');
-    $uri->setQueryParams($this->newURIParameters());
+    $params = $this->newURIParameters();
+    $uri = new PhutilURI('/typeahead/browse/'.get_class($this).'/', $params);
     return phutil_string_cast($uri);
   }
 

From 241f06c9ffc63909ec04e29f67f53cb310fb1953 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 13:18:32 -0800
Subject: [PATCH 26/36] Clean up final `setQueryParams()` callsites

Summary: Ref T13250. See D20149.

Test Plan: All trivial?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20153
---
 src/aphront/AphrontRequest.php                               | 5 ++++-
 src/aphront/response/AphrontResponse.php                     | 2 +-
 src/applications/auth/provider/PhabricatorAuthProvider.php   | 2 +-
 .../files/controller/PhabricatorFileDataController.php       | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index a65566e867..38eb5a6ffc 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -829,7 +829,10 @@ final class AphrontRequest extends Phobject {
     }
 
     $uri->setPath($this->getPath());
-    $uri->setQueryParams(self::flattenData($_GET));
+    $uri->removeAllQueryParams();
+    foreach (self::flattenData($_GET) as $query_key => $query_value) {
+      $uri->appendQueryParam($query_key, $query_value);
+    }
 
     $input = PhabricatorStartup::getRawInput();
 
diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php
index 0eab91b2af..9cfda0f5c1 100644
--- a/src/aphront/response/AphrontResponse.php
+++ b/src/aphront/response/AphrontResponse.php
@@ -218,7 +218,7 @@ abstract class AphrontResponse extends Phobject {
     $uri = id(new PhutilURI($uri))
       ->setPath(null)
       ->setFragment(null)
-      ->setQueryParams(array());
+      ->removeAllQueryParams();
 
     $uri = (string)$uri;
     if (preg_match('/[ ;\']/', $uri)) {
diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index 78aeebd810..dfdc7c6a5b 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -447,7 +447,7 @@ abstract class PhabricatorAuthProvider extends Phobject {
     $uri = $attributes['uri'];
     $uri = new PhutilURI($uri);
     $params = $uri->getQueryParamsAsPairList();
-    $uri->setQueryParams(array());
+    $uri->removeAllQueryParams();
 
     $content = array($button);
 
diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index dd5e0adb7d..8ceb1e7907 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -135,7 +135,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
       $request_uri = id(clone $request->getAbsoluteRequestURI())
         ->setPath(null)
         ->setFragment(null)
-        ->setQueryParams(array());
+        ->removeAllQueryParams();
 
       $response->addContentSecurityPolicyURI(
         'object-src',

From 5892c78986037c3314f4f820decef38902f18166 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 12 Feb 2019 13:35:59 -0800
Subject: [PATCH 27/36] Replace all "setQueryParam()" calls with
 "remove/replaceQueryParam()"

Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.

Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20154
---
 src/aphront/AphrontRequest.php                |  2 +-
 .../almanac/controller/AlmanacController.php  |  2 +-
 .../controller/PhabricatorAuthController.php  |  2 +-
 .../PhabricatorAuthStartController.php        |  4 +--
 .../config/PhabricatorAuthNewController.php   |  2 +-
 ...icatorAuthFactorProviderEditController.php |  2 +-
 .../PhabricatorAuthMainMenuBarExtension.php   |  2 +-
 ...habricatorCalendarImportViewController.php |  6 ++--
 .../check/PhabricatorWebServerSetupCheck.php  |  2 +-
 .../controller/ConpherenceViewController.php  |  2 +-
 ...abricatorDashboardPanelRenderingEngine.php |  4 +--
 .../PhabricatorDashboardRenderingEngine.php   |  6 ++--
 .../DifferentialDiffCreateController.php      |  2 +-
 .../PhabricatorFactHomeController.php         |  2 +-
 .../PhabricatorFileLightboxController.php     |  2 +-
 ...PhabricatorFileTransformListController.php |  2 +-
 .../markup/PhabricatorImageRemarkupRule.php   |  2 +-
 .../ManiphestTaskDetailController.php         |  6 ++--
 .../ManiphestTaskSubtaskController.php        |  6 ++--
 .../maniphest/view/ManiphestTaskListView.php  |  2 +-
 .../controller/MultimeterSampleController.php |  6 ++--
 .../PhabricatorNotificationServerRef.php      |  2 +-
 ...PhabricatorNotificationPanelController.php |  2 +-
 .../PhabricatorNotificationSearchEngine.php   |  2 +-
 .../oauthserver/PhabricatorOAuthResponse.php  |  2 +-
 .../PhabricatorOAuthServerAuthController.php  |  2 +-
 .../pholio/view/PholioMockImagesView.php      |  2 +-
 .../PhortunePaymentMethodCreateController.php |  2 +-
 .../PhortuneSubscriptionEditController.php    |  4 +--
 .../ponder/view/PonderAddAnswerView.php       |  2 +-
 .../PhabricatorProjectBoardViewController.php | 30 +++++++++++--------
 ...PhabricatorProjectColumnHideController.php |  2 +-
 .../PhabricatorProjectDefaultController.php   |  2 +-
 ...ephRequestDifferentialCreateController.php |  2 +-
 ...PhabricatorApplicationSearchController.php |  4 +--
 .../PhabricatorMultiFactorSettingsPanel.php   |  2 +-
 ...catorApplicationTransactionCommentView.php |  2 +-
 ...orTypeaheadModularDatasourceController.php | 12 ++++----
 src/infrastructure/env/PhabricatorEnv.php     | 16 ++++++----
 src/view/phui/PHUITimelineView.php            |  4 +--
 40 files changed, 87 insertions(+), 75 deletions(-)

diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php
index 38eb5a6ffc..46d1266b08 100644
--- a/src/aphront/AphrontRequest.php
+++ b/src/aphront/AphrontRequest.php
@@ -594,7 +594,7 @@ final class AphrontRequest extends Phobject {
     $request_uri = idx($_SERVER, 'REQUEST_URI', '/');
 
     $uri = new PhutilURI($request_uri);
-    $uri->setQueryParam('__path__', null);
+    $uri->removeQueryParam('__path__');
 
     $path = phutil_escape_uri($this->getPath());
     $uri->setPath($path);
diff --git a/src/applications/almanac/controller/AlmanacController.php b/src/applications/almanac/controller/AlmanacController.php
index 7fad62a509..918b3a4ad9 100644
--- a/src/applications/almanac/controller/AlmanacController.php
+++ b/src/applications/almanac/controller/AlmanacController.php
@@ -137,7 +137,7 @@ abstract class AlmanacController
 
     $phid = $object->getPHID();
     $add_uri = id(new PhutilURI($edit_base))
-      ->setQueryParam('objectPHID', $object->getPHID());
+      ->replaceQueryParam('objectPHID', $object->getPHID());
 
     $can_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php
index e81301218c..cda56d34b1 100644
--- a/src/applications/auth/controller/PhabricatorAuthController.php
+++ b/src/applications/auth/controller/PhabricatorAuthController.php
@@ -95,7 +95,7 @@ abstract class PhabricatorAuthController extends PhabricatorController {
 
   private function buildLoginValidateResponse(PhabricatorUser $user) {
     $validate_uri = new PhutilURI($this->getApplicationURI('validate/'));
-    $validate_uri->setQueryParam('expect', $user->getUsername());
+    $validate_uri->replaceQueryParam('expect', $user->getUsername());
 
     return id(new AphrontRedirectResponse())->setURI((string)$validate_uri);
   }
diff --git a/src/applications/auth/controller/PhabricatorAuthStartController.php b/src/applications/auth/controller/PhabricatorAuthStartController.php
index 0b823098d7..72cbbea5a8 100644
--- a/src/applications/auth/controller/PhabricatorAuthStartController.php
+++ b/src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -54,7 +54,7 @@ final class PhabricatorAuthStartController
           }
 
           $redirect_uri = $request->getRequestURI();
-          $redirect_uri->setQueryParam('cleared', 1);
+          $redirect_uri->replaceQueryParam('cleared', 1);
           return id(new AphrontRedirectResponse())->setURI($redirect_uri);
       }
     }
@@ -64,7 +64,7 @@ final class PhabricatorAuthStartController
     // the workflow will continue normally.
     if ($did_clear) {
       $redirect_uri = $request->getRequestURI();
-      $redirect_uri->setQueryParam('cleared', null);
+      $redirect_uri->removeQueryParam('cleared');
       return id(new AphrontRedirectResponse())->setURI($redirect_uri);
     }
 
diff --git a/src/applications/auth/controller/config/PhabricatorAuthNewController.php b/src/applications/auth/controller/config/PhabricatorAuthNewController.php
index c8fd0ad8a5..770c43208d 100644
--- a/src/applications/auth/controller/config/PhabricatorAuthNewController.php
+++ b/src/applications/auth/controller/config/PhabricatorAuthNewController.php
@@ -32,7 +32,7 @@ final class PhabricatorAuthNewController
       $provider_class = get_class($provider);
 
       $provider_uri = id(new PhutilURI('/config/edit/'))
-        ->setQueryParam('provider', $provider_class);
+        ->replaceQueryParam('provider', $provider_class);
       $provider_uri = $this->getApplicationURI($provider_uri);
 
       $already_exists = isset($configured_classes[get_class($provider)]);
diff --git a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderEditController.php b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderEditController.php
index a8d87e2ead..a1636396ac 100644
--- a/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderEditController.php
+++ b/src/applications/auth/controller/mfa/PhabricatorAuthFactorProviderEditController.php
@@ -45,7 +45,7 @@ final class PhabricatorAuthFactorProviderEditController
 
     foreach ($factors as $factor_key => $factor) {
       $factor_uri = id(new PhutilURI('/mfa/edit/'))
-        ->setQueryParam('providerFactorKey', $factor_key);
+        ->replaceQueryParam('providerFactorKey', $factor_key);
       $factor_uri = $this->getApplicationURI($factor_uri);
 
       $is_enabled = $factor->canCreateNewProvider();
diff --git a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php
index d9fb5d013b..d49a447df6 100644
--- a/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php
+++ b/src/applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php
@@ -42,7 +42,7 @@ final class PhabricatorAuthMainMenuBarExtension
     $uri = new PhutilURI('/auth/start/');
     if ($controller) {
       $path = $controller->getRequest()->getPath();
-      $uri->setQueryParam('next', $path);
+      $uri->replaceQueryParam('next', $path);
     }
 
     return id(new PHUIButtonView())
diff --git a/src/applications/calendar/controller/PhabricatorCalendarImportViewController.php b/src/applications/calendar/controller/PhabricatorCalendarImportViewController.php
index 9c07d371d0..6d92f00a97 100644
--- a/src/applications/calendar/controller/PhabricatorCalendarImportViewController.php
+++ b/src/applications/calendar/controller/PhabricatorCalendarImportViewController.php
@@ -234,7 +234,7 @@ final class PhabricatorCalendarImportViewController
 
     $all_uri = $this->getApplicationURI('import/log/');
     $all_uri = (string)id(new PhutilURI($all_uri))
-      ->setQueryParam('importSourcePHID', $import->getPHID());
+      ->replaceQueryParam('importSourcePHID', $import->getPHID());
 
     $all_button = id(new PHUIButtonView())
       ->setTag('a')
@@ -273,8 +273,8 @@ final class PhabricatorCalendarImportViewController
 
     $all_uri = $this->getApplicationURI();
     $all_uri = (string)id(new PhutilURI($all_uri))
-      ->setQueryParam('importSourcePHID', $import->getPHID())
-      ->setQueryParam('display', 'list');
+      ->replaceQueryParam('importSourcePHID', $import->getPHID())
+      ->replaceQueryParam('display', 'list');
 
     $all_button = id(new PHUIButtonView())
       ->setTag('a')
diff --git a/src/applications/config/check/PhabricatorWebServerSetupCheck.php b/src/applications/config/check/PhabricatorWebServerSetupCheck.php
index 398ebd6376..8f6885e8e8 100644
--- a/src/applications/config/check/PhabricatorWebServerSetupCheck.php
+++ b/src/applications/config/check/PhabricatorWebServerSetupCheck.php
@@ -40,7 +40,7 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck {
 
     $base_uri = id(new PhutilURI($base_uri))
       ->setPath($send_path)
-      ->setQueryParam($expect_key, $expect_value);
+      ->replaceQueryParam($expect_key, $expect_value);
 
     $self_future = id(new HTTPSFuture($base_uri))
       ->addHeader('X-Phabricator-SelfCheck', 1)
diff --git a/src/applications/conpherence/controller/ConpherenceViewController.php b/src/applications/conpherence/controller/ConpherenceViewController.php
index 996417a307..357d07631a 100644
--- a/src/applications/conpherence/controller/ConpherenceViewController.php
+++ b/src/applications/conpherence/controller/ConpherenceViewController.php
@@ -188,7 +188,7 @@ final class ConpherenceViewController extends
     } else {
       // user not logged in so give them a login button.
       $login_href = id(new PhutilURI('/auth/start/'))
-        ->setQueryParam('next', '/'.$conpherence->getMonogram());
+        ->replaceQueryParam('next', '/'.$conpherence->getMonogram());
       return id(new PHUIFormLayoutView())
         ->addClass('login-to-participate')
         ->appendInstructions(pht('Log in to join this room and participate.'))
diff --git a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php b/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
index dfe328933a..fc62c4d5cb 100644
--- a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
+++ b/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
@@ -287,7 +287,7 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject {
       $edit_uri = "/dashboard/panel/edit/{$panel_id}/";
       $edit_uri = new PhutilURI($edit_uri);
       if ($dashboard_id) {
-        $edit_uri->setQueryParam('dashboardID', $dashboard_id);
+        $edit_uri->replaceQueryParam('dashboardID', $dashboard_id);
       }
 
       $action_edit = id(new PHUIIconView())
@@ -303,7 +303,7 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject {
 
       $remove_uri = "/dashboard/removepanel/{$dashboard_id}/";
       $remove_uri = id(new PhutilURI($remove_uri))
-        ->setQueryParam('panelPHID', $panel_phid);
+        ->replaceQueryParam('panelPHID', $panel_phid);
 
       $action_remove = id(new PHUIIconView())
         ->setIcon('fa-trash-o')
diff --git a/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php b/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
index ba6aace971..9f6481c05b 100644
--- a/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
+++ b/src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
@@ -113,11 +113,11 @@ final class PhabricatorDashboardRenderingEngine extends Phobject {
     $dashboard_id = $this->dashboard->getID();
 
     $create_uri = id(new PhutilURI('/dashboard/panel/create/'))
-      ->setQueryParam('dashboardID', $dashboard_id)
-      ->setQueryParam('column', $column);
+      ->replaceQueryParam('dashboardID', $dashboard_id)
+      ->replaceQueryParam('column', $column);
 
     $add_uri = id(new PhutilURI('/dashboard/addpanel/'.$dashboard_id.'/'))
-      ->setQueryParam('column', $column);
+      ->replaceQueryParam('column', $column);
 
     $create_button = id(new PHUIButtonView())
       ->setTag('a')
diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php
index 284edf49d3..9aaf407e28 100644
--- a/src/applications/differential/controller/DifferentialDiffCreateController.php
+++ b/src/applications/differential/controller/DifferentialDiffCreateController.php
@@ -71,7 +71,7 @@ final class DifferentialDiffCreateController extends DifferentialController {
           $uri = $this->getApplicationURI("diff/{$diff_id}/");
           $uri = new PhutilURI($uri);
           if ($revision) {
-            $uri->setQueryParam('revisionID', $revision->getID());
+            $uri->replaceQueryParam('revisionID', $revision->getID());
           }
 
           return id(new AphrontRedirectResponse())->setURI($uri);
diff --git a/src/applications/fact/controller/PhabricatorFactHomeController.php b/src/applications/fact/controller/PhabricatorFactHomeController.php
index 82f6a0905b..56ffe3930b 100644
--- a/src/applications/fact/controller/PhabricatorFactHomeController.php
+++ b/src/applications/fact/controller/PhabricatorFactHomeController.php
@@ -11,7 +11,7 @@ final class PhabricatorFactHomeController extends PhabricatorFactController {
 
     if ($request->isFormPost()) {
       $uri = new PhutilURI('/fact/chart/');
-      $uri->setQueryParam('y1', $request->getStr('y1'));
+      $uri->replaceQueryParam('y1', $request->getStr('y1'));
       return id(new AphrontRedirectResponse())->setURI($uri);
     }
 
diff --git a/src/applications/files/controller/PhabricatorFileLightboxController.php b/src/applications/files/controller/PhabricatorFileLightboxController.php
index 1f679d621b..59a826dd42 100644
--- a/src/applications/files/controller/PhabricatorFileLightboxController.php
+++ b/src/applications/files/controller/PhabricatorFileLightboxController.php
@@ -70,7 +70,7 @@ final class PhabricatorFileLightboxController
 
     if (!$viewer->isLoggedIn()) {
       $login_href = id(new PhutilURI('/auth/start/'))
-        ->setQueryParam('next', '/'.$file->getMonogram());
+        ->replaceQueryParam('next', '/'.$file->getMonogram());
       return id(new PHUIFormLayoutView())
         ->addClass('phui-comment-panel-empty')
         ->appendChild(
diff --git a/src/applications/files/controller/PhabricatorFileTransformListController.php b/src/applications/files/controller/PhabricatorFileTransformListController.php
index ab5322fc1a..7b5bc9299d 100644
--- a/src/applications/files/controller/PhabricatorFileTransformListController.php
+++ b/src/applications/files/controller/PhabricatorFileTransformListController.php
@@ -61,7 +61,7 @@ final class PhabricatorFileTransformListController
 
         $view_href = $file->getURIForTransform($xform);
         $view_href = new PhutilURI($view_href);
-        $view_href->setQueryParam('regenerate', 'true');
+        $view_href->replaceQueryParam('regenerate', 'true');
 
         $view_text = pht('Regenerate');
 
diff --git a/src/applications/files/markup/PhabricatorImageRemarkupRule.php b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
index 5d1979ed3c..57ad75bbc5 100644
--- a/src/applications/files/markup/PhabricatorImageRemarkupRule.php
+++ b/src/applications/files/markup/PhabricatorImageRemarkupRule.php
@@ -149,7 +149,7 @@ final class PhabricatorImageRemarkupRule extends PhutilRemarkupRule {
           ));
       } else {
         $src_uri = id(new PhutilURI('/file/imageproxy/'))
-          ->setQueryParam('uri', $uri);
+          ->replaceQueryParam('uri', $uri);
 
         $img = id(new PHUIRemarkupImageView())
           ->setURI($src_uri)
diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
index 0f96d76b91..ba826e16e8 100644
--- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
@@ -300,9 +300,9 @@ final class ManiphestTaskDetailController extends ManiphestController {
       $subtask_form = head($subtask_options);
       $form_key = $subtask_form->getIdentifier();
       $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
-        ->setQueryParam('parent', $id)
-        ->setQueryParam('template', $id)
-        ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
+        ->replaceQueryParam('parent', $id)
+        ->replaceQueryParam('template', $id)
+        ->replaceQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
       $subtask_workflow = false;
     }
 
diff --git a/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
index 5256c1bd26..3105cf661d 100644
--- a/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskSubtaskController.php
@@ -47,9 +47,9 @@ final class ManiphestTaskSubtaskController
       $subtype = $subtype_map->getSubtype($subtype_key);
 
       $subtask_uri = id(new PhutilURI("/task/edit/form/{$form_key}/"))
-        ->setQueryParam('parent', $id)
-        ->setQueryParam('template', $id)
-        ->setQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
+        ->replaceQueryParam('parent', $id)
+        ->replaceQueryParam('template', $id)
+        ->replaceQueryParam('status', ManiphestTaskStatus::getDefaultStatus());
       $subtask_uri = $this->getApplicationURI($subtask_uri);
 
       $item = id(new PHUIObjectItemView())
diff --git a/src/applications/maniphest/view/ManiphestTaskListView.php b/src/applications/maniphest/view/ManiphestTaskListView.php
index e7128fb850..6bf5daf29b 100644
--- a/src/applications/maniphest/view/ManiphestTaskListView.php
+++ b/src/applications/maniphest/view/ManiphestTaskListView.php
@@ -135,7 +135,7 @@ final class ManiphestTaskListView extends ManiphestView {
       if ($this->showBatchControls) {
         $href = new PhutilURI('/maniphest/task/edit/'.$task->getID().'/');
         if (!$this->showSubpriorityControls) {
-          $href->setQueryParam('ungrippable', 'true');
+          $href->replaceQueryParam('ungrippable', 'true');
         }
         $item->addAction(
           id(new PHUIListItemView())
diff --git a/src/applications/multimeter/controller/MultimeterSampleController.php b/src/applications/multimeter/controller/MultimeterSampleController.php
index da09641d22..73c9ba530e 100644
--- a/src/applications/multimeter/controller/MultimeterSampleController.php
+++ b/src/applications/multimeter/controller/MultimeterSampleController.php
@@ -302,11 +302,11 @@ final class MultimeterSampleController extends MultimeterController {
     if (!strlen($group)) {
       $group = null;
     }
-    $uri->setQueryParam('group', $group);
+    $uri->replaceQueryParam('group', $group);
 
     if ($wipe) {
       foreach ($this->getColumnMap() as $key => $column) {
-        $uri->setQueryParam($key, null);
+        $uri->removeQueryParam($key);
       }
     }
 
@@ -317,7 +317,7 @@ final class MultimeterSampleController extends MultimeterController {
     $value = (array)$value;
 
     $uri = clone $this->getRequest()->getRequestURI();
-    $uri->setQueryParam($key, implode(',', $value));
+    $uri->replaceQueryParam($key, implode(',', $value));
 
     return phutil_tag(
       'a',
diff --git a/src/applications/notification/client/PhabricatorNotificationServerRef.php b/src/applications/notification/client/PhabricatorNotificationServerRef.php
index b183221eee..46d03a5c3a 100644
--- a/src/applications/notification/client/PhabricatorNotificationServerRef.php
+++ b/src/applications/notification/client/PhabricatorNotificationServerRef.php
@@ -153,7 +153,7 @@ final class PhabricatorNotificationServerRef
 
     $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
     if (strlen($instance)) {
-      $uri->setQueryParam('instance', $instance);
+      $uri->replaceQueryParam('instance', $instance);
     }
 
     return $uri;
diff --git a/src/applications/notification/controller/PhabricatorNotificationPanelController.php b/src/applications/notification/controller/PhabricatorNotificationPanelController.php
index 1e956a60ea..5991e8db7b 100644
--- a/src/applications/notification/controller/PhabricatorNotificationPanelController.php
+++ b/src/applications/notification/controller/PhabricatorNotificationPanelController.php
@@ -25,7 +25,7 @@ final class PhabricatorNotificationPanelController
 
       $notifications_view = $builder->buildView();
       $content = $notifications_view->render();
-      $clear_uri->setQueryParam(
+      $clear_uri->replaceQueryParam(
         'chronoKey',
         head($stories)->getChronologicalKey());
     } else {
diff --git a/src/applications/notification/query/PhabricatorNotificationSearchEngine.php b/src/applications/notification/query/PhabricatorNotificationSearchEngine.php
index 0ee7327bfc..c7e1998333 100644
--- a/src/applications/notification/query/PhabricatorNotificationSearchEngine.php
+++ b/src/applications/notification/query/PhabricatorNotificationSearchEngine.php
@@ -111,7 +111,7 @@ final class PhabricatorNotificationSearchEngine
         ->setUser($viewer);
 
       $view = $builder->buildView();
-      $clear_uri->setQueryParam(
+      $clear_uri->replaceQueryParam(
         'chronoKey',
         head($notifications)->getChronologicalKey());
     } else {
diff --git a/src/applications/oauthserver/PhabricatorOAuthResponse.php b/src/applications/oauthserver/PhabricatorOAuthResponse.php
index 62c0fc9821..e0fca827b4 100644
--- a/src/applications/oauthserver/PhabricatorOAuthResponse.php
+++ b/src/applications/oauthserver/PhabricatorOAuthResponse.php
@@ -36,7 +36,7 @@ final class PhabricatorOAuthResponse extends AphrontResponse {
     $base_uri     = $this->getClientURI();
     $query_params = $this->buildResponseDict();
     foreach ($query_params as $key => $value) {
-      $base_uri->setQueryParam($key, $value);
+      $base_uri->replaceQueryParam($key, $value);
     }
     return $base_uri;
   }
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
index 745be3e820..2b454e00ef 100644
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerAuthController.php
@@ -306,7 +306,7 @@ final class PhabricatorOAuthServerAuthController
 
     foreach ($params as $key => $value) {
       if (strlen($value)) {
-        $full_uri->setQueryParam($key, $value);
+        $full_uri->replaceQueryParam($key, $value);
       }
     }
 
diff --git a/src/applications/pholio/view/PholioMockImagesView.php b/src/applications/pholio/view/PholioMockImagesView.php
index 99645c4a91..786de07cfd 100644
--- a/src/applications/pholio/view/PholioMockImagesView.php
+++ b/src/applications/pholio/view/PholioMockImagesView.php
@@ -133,7 +133,7 @@ final class PholioMockImagesView extends AphrontView {
     );
 
     $login_uri = id(new PhutilURI('/login/'))
-      ->setQueryParam('next', (string)$this->getRequestURI());
+      ->replaceQueryParam('next', (string)$this->getRequestURI());
 
     $config = array(
       'mockID' => $mock->getID(),
diff --git a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
index 847fbf5716..c068862631 100644
--- a/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
+++ b/src/applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php
@@ -143,7 +143,7 @@ final class PhortunePaymentMethodCreateController
             "cart/{$cart_id}/checkout/?paymentMethodID=".$method->getID());
         } else if ($subscription_id) {
           $next_uri = new PhutilURI($cancel_uri);
-          $next_uri->setQueryParam('added', true);
+          $next_uri->replaceQueryParam('added', true);
         } else {
           $account_uri = $this->getApplicationURI($account->getID().'/');
           $next_uri = new PhutilURI($account_uri);
diff --git a/src/applications/phortune/controller/subscription/PhortuneSubscriptionEditController.php b/src/applications/phortune/controller/subscription/PhortuneSubscriptionEditController.php
index e7287f3d29..04367a88a0 100644
--- a/src/applications/phortune/controller/subscription/PhortuneSubscriptionEditController.php
+++ b/src/applications/phortune/controller/subscription/PhortuneSubscriptionEditController.php
@@ -118,8 +118,8 @@ final class PhortuneSubscriptionEditController extends PhortuneController {
 
     $uri = $this->getApplicationURI($account->getID().'/card/new/');
     $uri = new PhutilURI($uri);
-    $uri->setQueryParam('merchantID', $merchant->getID());
-    $uri->setQueryParam('subscriptionID', $subscription->getID());
+    $uri->replaceQueryParam('merchantID', $merchant->getID());
+    $uri->replaceQueryParam('subscriptionID', $subscription->getID());
 
     $add_method_button = phutil_tag(
       'a',
diff --git a/src/applications/ponder/view/PonderAddAnswerView.php b/src/applications/ponder/view/PonderAddAnswerView.php
index 43bfd0d6ba..20c52dac8e 100644
--- a/src/applications/ponder/view/PonderAddAnswerView.php
+++ b/src/applications/ponder/view/PonderAddAnswerView.php
@@ -66,7 +66,7 @@ final class PonderAddAnswerView extends AphrontView {
 
     if (!$viewer->isLoggedIn()) {
       $login_href = id(new PhutilURI('/auth/start/'))
-          ->setQueryParam('next', '/Q'.$question->getID());
+          ->replaceQueryParam('next', '/Q'.$question->getID());
       $form = id(new PHUIFormLayoutView())
         ->addClass('login-to-participate')
         ->appendChild(
diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
index 15e0c5d075..ee239035da 100644
--- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php
+++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php
@@ -284,7 +284,7 @@ final class PhabricatorProjectBoardViewController
       $query_key = $saved_query->getQueryKey();
 
       $bulk_uri = new PhutilURI("/maniphest/bulk/query/{$query_key}/");
-      $bulk_uri->setQueryParam('board', $this->id);
+      $bulk_uri->replaceQueryParam('board', $this->id);
 
       return id(new AphrontRedirectResponse())
         ->setURI($bulk_uri);
@@ -878,7 +878,7 @@ final class PhabricatorProjectBoardViewController
       }
 
       $uri = $this->getURIWithState($uri)
-        ->setQueryParam('filter', null);
+        ->removeQueryParam('filter');
       $item->setHref($uri);
 
       $items[] = $item;
@@ -966,12 +966,12 @@ final class PhabricatorProjectBoardViewController
 
     if ($show_hidden) {
       $hidden_uri = $this->getURIWithState()
-        ->setQueryParam('hidden', null);
+        ->removeQueryParam('hidden');
       $hidden_icon = 'fa-eye-slash';
       $hidden_text = pht('Hide Hidden Columns');
     } else {
       $hidden_uri = $this->getURIWithState()
-        ->setQueryParam('hidden', 'true');
+        ->replaceQueryParam('hidden', 'true');
       $hidden_icon = 'fa-eye';
       $hidden_text = pht('Show Hidden Columns');
     }
@@ -999,7 +999,7 @@ final class PhabricatorProjectBoardViewController
       ->setHref($manage_uri);
 
     $batch_edit_uri = $request->getRequestURI();
-    $batch_edit_uri->setQueryParam('batch', self::BATCH_EDIT_ALL);
+    $batch_edit_uri->replaceQueryParam('batch', self::BATCH_EDIT_ALL);
     $can_batch_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       PhabricatorApplication::getByClass('PhabricatorManiphestApplication'),
@@ -1090,7 +1090,7 @@ final class PhabricatorProjectBoardViewController
     }
 
     $batch_edit_uri = $request->getRequestURI();
-    $batch_edit_uri->setQueryParam('batch', $column->getID());
+    $batch_edit_uri->replaceQueryParam('batch', $column->getID());
     $can_batch_edit = PhabricatorPolicyFilter::hasCapability(
       $viewer,
       PhabricatorApplication::getByClass('PhabricatorManiphestApplication'),
@@ -1103,7 +1103,7 @@ final class PhabricatorProjectBoardViewController
       ->setDisabled(!$can_batch_edit);
 
     $batch_move_uri = $request->getRequestURI();
-    $batch_move_uri->setQueryParam('move', $column->getID());
+    $batch_move_uri->replaceQueryParam('move', $column->getID());
     $column_items[] = id(new PhabricatorActionView())
       ->setIcon('fa-arrow-right')
       ->setName(pht('Move Tasks to Column...'))
@@ -1111,7 +1111,7 @@ final class PhabricatorProjectBoardViewController
       ->setWorkflow(true);
 
     $query_uri = $request->getRequestURI();
-    $query_uri->setQueryParam('queryColumnID', $column->getID());
+    $query_uri->replaceQueryParam('queryColumnID', $column->getID());
 
     $column_items[] = id(new PhabricatorActionView())
       ->setName(pht('View as Query'))
@@ -1188,18 +1188,22 @@ final class PhabricatorProjectBoardViewController
     $base = new PhutilURI($base);
 
     if ($force || ($this->sortKey != $this->getDefaultSort($project))) {
-      $base->setQueryParam('order', $this->sortKey);
+      $base->replaceQueryParam('order', $this->sortKey);
     } else {
-      $base->setQueryParam('order', null);
+      $base->removeQueryParam('order');
     }
 
     if ($force || ($this->queryKey != $this->getDefaultFilter($project))) {
-      $base->setQueryParam('filter', $this->queryKey);
+      $base->replaceQueryParam('filter', $this->queryKey);
     } else {
-      $base->setQueryParam('filter', null);
+      $base->removeQueryParam('filter');
     }
 
-    $base->setQueryParam('hidden', $this->showHidden ? 'true' : null);
+    if ($this->showHidden) {
+      $base->replaceQueryParam('hidden', 'true');
+    } else {
+      $base->removeQueryParam('hidden');
+    }
 
     return $base;
   }
diff --git a/src/applications/project/controller/PhabricatorProjectColumnHideController.php b/src/applications/project/controller/PhabricatorProjectColumnHideController.php
index 1dd5e47ecb..fbda2feb1e 100644
--- a/src/applications/project/controller/PhabricatorProjectColumnHideController.php
+++ b/src/applications/project/controller/PhabricatorProjectColumnHideController.php
@@ -41,7 +41,7 @@ final class PhabricatorProjectColumnHideController
     $view_uri = $this->getApplicationURI('/board/'.$project_id.'/');
     $view_uri = new PhutilURI($view_uri);
     foreach ($request->getPassthroughRequestData() as $key => $value) {
-      $view_uri->setQueryParam($key, $value);
+      $view_uri->replaceQueryParam($key, $value);
     }
 
     if ($column->isDefaultColumn()) {
diff --git a/src/applications/project/controller/PhabricatorProjectDefaultController.php b/src/applications/project/controller/PhabricatorProjectDefaultController.php
index 8f42ff9736..2c7a47b2df 100644
--- a/src/applications/project/controller/PhabricatorProjectDefaultController.php
+++ b/src/applications/project/controller/PhabricatorProjectDefaultController.php
@@ -54,7 +54,7 @@ final class PhabricatorProjectDefaultController
     $view_uri = $this->getApplicationURI("board/{$id}/");
     $view_uri = new PhutilURI($view_uri);
     foreach ($request->getPassthroughRequestData() as $key => $value) {
-      $view_uri->setQueryParam($key, $value);
+      $view_uri->replaceQueryParam($key, $value);
     }
 
     if ($request->isFormPost()) {
diff --git a/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php b/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php
index ffd6388284..1835e6f7f9 100644
--- a/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php
+++ b/src/applications/releeph/controller/request/ReleephRequestDifferentialCreateController.php
@@ -96,7 +96,7 @@ final class ReleephRequestDifferentialCreateController
   private function buildReleephRequestURI(ReleephBranch $branch) {
     $uri = $branch->getURI('request/');
     return id(new PhutilURI($uri))
-      ->setQueryParam('D', $this->revision->getID());
+      ->replaceQueryParam('D', $this->revision->getID());
   }
 
 }
diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php
index cf4f95c16d..6b43883c78 100644
--- a/src/applications/search/controller/PhabricatorApplicationSearchController.php
+++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php
@@ -905,7 +905,7 @@ final class PhabricatorApplicationSearchController
       $engine = $this->getSearchEngine();
       $nux_uri = $engine->getQueryBaseURI();
       $nux_uri = id(new PhutilURI($nux_uri))
-        ->setQueryParam('nux', true);
+        ->replaceQueryParam('nux', true);
 
       $actions[] = id(new PhabricatorActionView())
         ->setIcon('fa-user-plus')
@@ -915,7 +915,7 @@ final class PhabricatorApplicationSearchController
 
     if ($is_dev) {
       $overheated_uri = $this->getRequest()->getRequestURI()
-        ->setQueryParam('overheated', true);
+        ->replaceQueryParam('overheated', true);
 
       $actions[] = id(new PhabricatorActionView())
         ->setIcon('fa-fire')
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
index 6809b51334..09193f3c96 100644
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -219,7 +219,7 @@ final class PhabricatorMultiFactorSettingsPanel
 
       foreach ($providers as $provider_phid => $provider) {
         $provider_uri = id(new PhutilURI($this->getPanelURI()))
-          ->setQueryParam('providerPHID', $provider_phid);
+          ->replaceQueryParam('providerPHID', $provider_phid);
 
         $is_enabled = $provider->canCreateNewConfiguration($viewer);
 
diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
index 1fb850887f..115c7b950e 100644
--- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
+++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php
@@ -198,7 +198,7 @@ final class PhabricatorApplicationTransactionCommentView
     $viewer = $this->getViewer();
     if (!$viewer->isLoggedIn()) {
       $uri = id(new PhutilURI('/login/'))
-        ->setQueryParam('next', (string)$this->getRequestURI());
+        ->replaceQueryParam('next', (string)$this->getRequestURI());
       return id(new PHUIObjectBoxView())
         ->setFlush(true)
         ->appendChild(
diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
index 7c2205df46..efc9ea5f65 100644
--- a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
+++ b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php
@@ -126,10 +126,10 @@ final class PhabricatorTypeaheadModularDatasourceController
           $results = array_slice($results, 0, $limit, $preserve_keys = true);
           if (($offset + (2 * $limit)) < $hard_limit) {
             $next_uri = id(new PhutilURI($request->getRequestURI()))
-              ->setQueryParam('offset', $offset + $limit)
-              ->setQueryParam('q', $query)
-              ->setQueryParam('raw', $raw_query)
-              ->setQueryParam('format', 'html');
+              ->replaceQueryParam('offset', $offset + $limit)
+              ->replaceQueryParam('q', $query)
+              ->replaceQueryParam('raw', $raw_query)
+              ->replaceQueryParam('format', 'html');
 
             $next_link = javelin_tag(
               'a',
@@ -248,7 +248,9 @@ final class PhabricatorTypeaheadModularDatasourceController
           $parameters = $source->getParameters();
           if ($parameters) {
             $reference_uri = (string)id(new PhutilURI($reference_uri))
-              ->setQueryParam('parameters', phutil_json_encode($parameters));
+              ->replaceQueryParam(
+                'parameters',
+                phutil_json_encode($parameters));
           }
 
           $reference_link = phutil_tag(
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
index 8b2af38d3d..24fb940c9a 100644
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -475,11 +475,17 @@ final class PhabricatorEnv extends Phobject {
    * @task read
    */
   public static function getDoclink($resource, $type = 'article') {
-    $uri = new PhutilURI('https://secure.phabricator.com/diviner/find/');
-    $uri->setQueryParam('name', $resource);
-    $uri->setQueryParam('type', $type);
-    $uri->setQueryParam('jump', true);
-    return (string)$uri;
+    $params = array(
+      'name' => $resource,
+      'type' => $type,
+      'jump' => true,
+    );
+
+    $uri = new PhutilURI(
+      'https://secure.phabricator.com/diviner/find/',
+      $params);
+
+    return phutil_string_cast($uri);
   }
 
 
diff --git a/src/view/phui/PHUITimelineView.php b/src/view/phui/PHUITimelineView.php
index d0e942f461..d8c06d6a9f 100644
--- a/src/view/phui/PHUITimelineView.php
+++ b/src/view/phui/PHUITimelineView.php
@@ -154,8 +154,8 @@ final class PHUITimelineView extends AphrontView {
       }
 
       $uri = $this->getPager()->getNextPageURI();
-      $uri->setQueryParam('quoteTargetID', $this->getQuoteTargetID());
-      $uri->setQueryParam('quoteRef', $this->getQuoteRef());
+      $uri->replaceQueryParam('quoteTargetID', $this->getQuoteTargetID());
+      $uri->replaceQueryParam('quoteRef', $this->getQuoteRef());
       $events[] = javelin_tag(
         'div',
         array(

From be21dd3b52b8b83839d225e39ed9bd37e0a2658a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 09:13:38 -0800
Subject: [PATCH 28/36] Fix some "URI->alter(X, null)" callsites

Summary:
Ref T13250. This internally calls `replaceQueryParam(X, null)` now, which fatals if the second parameter is `null`. I hit these legitimately, but I'll look for more callsites and follow up by either allowing this, removing `alter()`, fixing the callsites, or some combination.

(I'm not much of a fan of `alter()`.)

Test Plan: Browsing a paginated list no longer complains about URI construction.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20162
---
 src/view/control/AphrontCursorPagerView.php | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/view/control/AphrontCursorPagerView.php b/src/view/control/AphrontCursorPagerView.php
index 4608158481..cdb9562624 100644
--- a/src/view/control/AphrontCursorPagerView.php
+++ b/src/view/control/AphrontCursorPagerView.php
@@ -99,9 +99,9 @@ final class AphrontCursorPagerView extends AphrontView {
       return null;
     }
 
-    return $this->uri
-      ->alter('before', null)
-      ->alter('after', null);
+    return id(clone $this->uri)
+      ->removeQueryParam('after')
+      ->removeQueryParam('before');
   }
 
   public function getPrevPageURI() {
@@ -113,9 +113,9 @@ final class AphrontCursorPagerView extends AphrontView {
       return null;
     }
 
-    return $this->uri
-      ->alter('after', null)
-      ->alter('before', $this->prevPageID);
+    return id(clone $this->uri)
+      ->removeQueryParam('after')
+      ->replaceQueryParam('before', $this->prevPageID);
   }
 
   public function getNextPageURI() {
@@ -127,9 +127,9 @@ final class AphrontCursorPagerView extends AphrontView {
       return null;
     }
 
-    return $this->uri
-      ->alter('after', $this->nextPageID)
-      ->alter('before', null);
+    return id(clone $this->uri)
+      ->replaceQueryParam('after', $this->nextPageID)
+      ->removeQueryParam('before');
   }
 
   public function render() {

From 889eca1af94e3375c7f2b8b0ea673ebd0325700a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 14 Feb 2019 04:18:46 -0800
Subject: [PATCH 29/36] Allow a DAO object storage namespace to be forced to a
 particular value

Summary:
Ref T6703. When we import external data from a third-party install to a Phacility instance, we must link instance accounts to central accounts: either existing central accounts, or newly created central accounts that we send invites for.

During this import, or when users register and claim those new accounts, we do a write from `admin.phacility.com` directly into the instance database to link the accounts.

This is pretty sketchy, and should almost certainly just be an internal API instead, particularly now that it's relatively stable.

However, it's what we use for now. The process has had some issues since the introduction of `%R` (combined database name and table refrence in queries), and now needs to be updated for the new `providerConfigPHID` column in `ExternalAccount`.

The problem is that `%R` isn't doing the right thing. We have code like this:

```
$conn = new_connection_to_instance('turtle');
queryf($conn, 'INSERT INTO %R ...', $table);
```

However, the `$table` resolves `%R` using the currently-executing-process information, not anything specific to `$conn`, so it prints `admin_user.user_externalaccount` (the name of the table on `admin.phacility.com`, where the code is running).

We want it to print `turtle_user.user_externalaccount` instead: the name of the table on `turtle.phacility.com`, where we're actually writing.

To force this to happen, let callers override the namespace part of the database name.

Long term: I'd plan to rip this out and replace it with an API call. This "connect directly to the database" stuff is nice for iterating on (only `admin` needs hotfixes) but very very sketchy for maintaining.

Test Plan: See next diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20167
---
 .../storage/lisk/PhabricatorLiskDAO.php            | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
index e47d701df9..99603da567 100644
--- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
+++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
@@ -6,6 +6,7 @@
 abstract class PhabricatorLiskDAO extends LiskDAO {
 
   private static $namespaceStack = array();
+  private $forcedNamespace;
 
   const ATTACHABLE = '<attachable>';
   const CONFIG_APPLICATION_SERIALIZERS = 'phabricator/serializers';
@@ -47,6 +48,11 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
     return $namespace;
   }
 
+  public function setForcedStorageNamespace($namespace) {
+    $this->forcedNamespace = $namespace;
+    return $this;
+  }
+
   /**
    * @task config
    */
@@ -188,7 +194,13 @@ abstract class PhabricatorLiskDAO extends LiskDAO {
   abstract public function getApplicationName();
 
   protected function getDatabaseName() {
-    return self::getStorageNamespace().'_'.$this->getApplicationName();
+    if ($this->forcedNamespace) {
+      $namespace = $this->forcedNamespace;
+    } else {
+      $namespace = self::getStorageNamespace();
+    }
+
+    return $namespace.'_'.$this->getApplicationName();
   }
 
   /**

From c5772f51dea721134c129fdcd151a21850d03f72 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 15:25:20 -0800
Subject: [PATCH 30/36] Fix Content-Security-Policy headers on "Email Login"
 page

Summary:
In D20100, I changed this page from returning a `newPage()` with a dialog as its content to returning a more modern `newDialog()`.

However, the magic to add stuff to the CSP header is actually only on the `newPage()` pathway today, so this accidentally dropped the extra "Content-Security-Policy" rule for Google.

Lift the magic up one level so both Dialog and Page responses hit it.

Test Plan:
  - Configured Recaptcha.
  - Between D20100 and this patch: got a CSP error on the Email Login page.
  - After this patch: clicked all the pictures of cars / store fronts.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20163
---
 src/aphront/sink/AphrontHTTPSink.php          | 11 +++++++++++
 src/view/page/PhabricatorStandardPageView.php |  7 -------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/aphront/sink/AphrontHTTPSink.php b/src/aphront/sink/AphrontHTTPSink.php
index 60deba78ca..9e43e4a687 100644
--- a/src/aphront/sink/AphrontHTTPSink.php
+++ b/src/aphront/sink/AphrontHTTPSink.php
@@ -111,6 +111,17 @@ abstract class AphrontHTTPSink extends Phobject {
     // HTTP headers.
     $data = $response->getContentIterator();
 
+    // This isn't an exceptionally clean separation of concerns, but we need
+    // to add CSP headers for all response types (including both web pages
+    // and dialogs) and can't determine the correct CSP until after we render
+    // the page (because page elements like Recaptcha may add CSP rules).
+    $static = CelerityAPI::getStaticResourceResponse();
+    foreach ($static->getContentSecurityPolicyURIMap() as $kind => $uris) {
+      foreach ($uris as $uri) {
+        $response->addContentSecurityPolicyURI($kind, $uri);
+      }
+    }
+
     $all_headers = array_merge(
       $response->getHeaders(),
       $response->getCacheHeaders());
diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php
index 99143add5f..cfb1b4abbe 100644
--- a/src/view/page/PhabricatorStandardPageView.php
+++ b/src/view/page/PhabricatorStandardPageView.php
@@ -892,13 +892,6 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView
       $response = id(new AphrontWebpageResponse())
         ->setContent($content)
         ->setFrameable($this->getFrameable());
-
-      $static = CelerityAPI::getStaticResourceResponse();
-      foreach ($static->getContentSecurityPolicyURIMap() as $kind => $uris) {
-        foreach ($uris as $uri) {
-          $response->addContentSecurityPolicyURI($kind, $uri);
-        }
-      }
     }
 
     return $response;

From b09cf166a8bde2283b2de2fc3803b605430d8aa3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 15 Feb 2019 13:57:25 -0800
Subject: [PATCH 31/36] Clean up a couple more URI alter() calls

Summary:
See <https://discourse.phabricator-community.org/t/create-new-phriction-document-fails-with-unhandled-exception-invalidargumentexception/2406>.

These weren't obviously nullable from a cursory `grep`, but are sometimes nullable in practice.

Test Plan: Created, then saved a new Phriction document.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20184
---
 .../editor/PhrictionTransactionEditor.php       | 11 ++++++++---
 src/view/phui/PHUITimelineView.php              | 17 +++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php
index d09ff5d556..1476b24c46 100644
--- a/src/applications/phriction/editor/PhrictionTransactionEditor.php
+++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php
@@ -229,9 +229,14 @@ final class PhrictionTransactionEditor
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
         case PhrictionDocumentContentTransaction::TRANSACTIONTYPE:
-          $uri = id(new PhutilURI('/phriction/diff/'.$object->getID().'/'))
-            ->alter('l', $this->getOldContent()->getVersion())
-            ->alter('r', $this->getNewContent()->getVersion());
+          $params = array(
+            'l' => $this->getOldContent()->getVersion(),
+            'r' => $this->getNewContent()->getVersion(),
+          );
+
+          $path = '/phriction/diff/'.$object->getID().'/';
+          $uri = new PhutilURI($path, $params);
+
           $this->contentDiffURI = (string)$uri;
           break 2;
         default:
diff --git a/src/view/phui/PHUITimelineView.php b/src/view/phui/PHUITimelineView.php
index d8c06d6a9f..2e6d8298c8 100644
--- a/src/view/phui/PHUITimelineView.php
+++ b/src/view/phui/PHUITimelineView.php
@@ -154,8 +154,21 @@ final class PHUITimelineView extends AphrontView {
       }
 
       $uri = $this->getPager()->getNextPageURI();
-      $uri->replaceQueryParam('quoteTargetID', $this->getQuoteTargetID());
-      $uri->replaceQueryParam('quoteRef', $this->getQuoteRef());
+
+      $target_id = $this->getQuoteTargetID();
+      if ($target_id === null) {
+        $uri->removeQueryParam('quoteTargetID');
+      } else {
+        $uri->replaceQueryParam('quoteTargetID', $target_id);
+      }
+
+      $quote_ref = $this->getQuoteRef();
+      if ($quote_ref === null) {
+        $uri->removeQueryParam('quoteRef');
+      } else {
+        $uri->replaceQueryParam('quoteRef', $quote_ref);
+      }
+
       $events[] = javelin_tag(
         'div',
         array(

From 66060b294bce163037afa2e9352be9336c28cee6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 15 Feb 2019 04:53:42 -0800
Subject: [PATCH 32/36] Fix a URI construction in remarkup macro/meme rules

Summary: Ref T13250. Some of these parameters may be NULL, and `alter()` is no longer happy about that.

Test Plan: Ran daemon tasks that happened to render some memes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20176
---
 .../macro/engine/PhabricatorMemeEngine.php            | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/applications/macro/engine/PhabricatorMemeEngine.php b/src/applications/macro/engine/PhabricatorMemeEngine.php
index 7433a4e8bc..afee0f9b18 100644
--- a/src/applications/macro/engine/PhabricatorMemeEngine.php
+++ b/src/applications/macro/engine/PhabricatorMemeEngine.php
@@ -47,10 +47,13 @@ final class PhabricatorMemeEngine extends Phobject {
   }
 
   public function getGenerateURI() {
-    return id(new PhutilURI('/macro/meme/'))
-      ->alter('macro', $this->getTemplate())
-      ->alter('above', $this->getAboveText())
-      ->alter('below', $this->getBelowText());
+    $params = array(
+      'macro' => $this->getTemplate(),
+      'above' => $this->getAboveText(),
+      'below' => $this->getBelowText(),
+    );
+
+    return new PhutilURI('/macro/meme/', $params);
   }
 
   public function newAsset() {

From 454a762562289f421b02ed7caf80b9ff399c11bf Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 15 Feb 2019 05:04:30 -0800
Subject: [PATCH 33/36] Queue search indexing tasks at a new PRIORITY_INDEX,
 not PRIORITY_IMPORT

Summary:
Depends on D20175. Ref T12425. Ref T13253. Currently, importing commits can stall search index rebuilds, since index rebuilds use an older priority from before T11677 and weren't really updated for D16585.

In general, we'd like to complete all indexing tasks before continuing repository imports. A possible exception is if you rebuild an entire index with `bin/search index --rebuild-the-world`, but we could queue those at a separate lower priority if issues arise.

Test Plan: Ran some search indexing through the queue.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13253, T12425

Differential Revision: https://secure.phabricator.com/D20177
---
 src/applications/search/worker/PhabricatorSearchWorker.php | 2 +-
 src/infrastructure/daemon/workers/PhabricatorWorker.php    | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/applications/search/worker/PhabricatorSearchWorker.php b/src/applications/search/worker/PhabricatorSearchWorker.php
index f93df63981..361d9c9b6f 100644
--- a/src/applications/search/worker/PhabricatorSearchWorker.php
+++ b/src/applications/search/worker/PhabricatorSearchWorker.php
@@ -14,7 +14,7 @@ final class PhabricatorSearchWorker extends PhabricatorWorker {
         'parameters' => $parameters,
       ),
       array(
-        'priority' => parent::PRIORITY_IMPORT,
+        'priority' => parent::PRIORITY_INDEX,
         'objectPHID' => $phid,
       ));
   }
diff --git a/src/infrastructure/daemon/workers/PhabricatorWorker.php b/src/infrastructure/daemon/workers/PhabricatorWorker.php
index 1b9821b68d..f055544b7b 100644
--- a/src/infrastructure/daemon/workers/PhabricatorWorker.php
+++ b/src/infrastructure/daemon/workers/PhabricatorWorker.php
@@ -18,6 +18,7 @@ abstract class PhabricatorWorker extends Phobject {
   const PRIORITY_DEFAULT = 2000;
   const PRIORITY_COMMIT  = 2500;
   const PRIORITY_BULK    = 3000;
+  const PRIORITY_INDEX   = 3500;
   const PRIORITY_IMPORT  = 4000;
 
   /**

From 2ca316d652d84500c44a7412083714fd313ff932 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 14 Feb 2019 05:06:06 -0800
Subject: [PATCH 34/36] When users confirm Duo MFA in the mobile app,
 live-update the UI

Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.

Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20169
---
 resources/celerity/map.php                    | 13 ++-
 src/__phutil_library_map__.php                |  4 +
 .../PhabricatorAuthApplication.php            |  2 +
 ...abricatorAuthChallengeStatusController.php | 40 +++++++++
 .../auth/factor/PhabricatorAuthFactor.php     | 23 +++--
 .../factor/PhabricatorAuthFactorResult.php    | 20 ++---
 .../auth/factor/PhabricatorDuoAuthFactor.php  | 83 ++++++++++++++++---
 .../view/PhabricatorAuthChallengeUpdate.php   | 44 ++++++++++
 .../form/control/PHUIFormTimerControl.php     | 30 ++++++-
 webroot/rsrc/css/phui/phui-form-view.css      | 14 ++++
 .../js/phui/behavior-phui-timer-control.js    | 41 +++++++++
 11 files changed, 284 insertions(+), 30 deletions(-)
 create mode 100644 src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php
 create mode 100644 src/applications/auth/view/PhabricatorAuthChallengeUpdate.php
 create mode 100644 webroot/rsrc/js/phui/behavior-phui-timer-control.js

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 99f3333106..2dd5cf0966 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' => '7a73ffc5',
+    'core.pkg.css' => 'e0f5d66f',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -151,7 +151,7 @@ return array(
     'rsrc/css/phui/phui-document.css' => '52b748a5',
     'rsrc/css/phui/phui-feed-story.css' => 'a0c05029',
     'rsrc/css/phui/phui-fontkit.css' => '9b714a5e',
-    'rsrc/css/phui/phui-form-view.css' => '0807e7ac',
+    'rsrc/css/phui/phui-form-view.css' => '01b796c0',
     'rsrc/css/phui/phui-form.css' => '159e2d9c',
     'rsrc/css/phui/phui-head-thing.css' => 'd7f293df',
     'rsrc/css/phui/phui-header-view.css' => '93cea4ec',
@@ -502,6 +502,7 @@ return array(
     'rsrc/js/phui/behavior-phui-selectable-list.js' => 'b26a41e4',
     'rsrc/js/phui/behavior-phui-submenu.js' => 'b5e9bff9',
     'rsrc/js/phui/behavior-phui-tab-group.js' => '242aa08b',
+    'rsrc/js/phui/behavior-phui-timer-control.js' => 'f84bcbf4',
     'rsrc/js/phuix/PHUIXActionListView.js' => 'c68f183f',
     'rsrc/js/phuix/PHUIXActionView.js' => 'aaa08f3b',
     'rsrc/js/phuix/PHUIXAutocomplete.js' => '58cc4ab8',
@@ -650,6 +651,7 @@ return array(
     'javelin-behavior-phui-selectable-list' => 'b26a41e4',
     'javelin-behavior-phui-submenu' => 'b5e9bff9',
     'javelin-behavior-phui-tab-group' => '242aa08b',
+    'javelin-behavior-phui-timer-control' => 'f84bcbf4',
     'javelin-behavior-phuix-example' => 'c2c500a7',
     'javelin-behavior-policy-control' => '0eaa33a9',
     'javelin-behavior-policy-rule-editor' => '9347f172',
@@ -817,7 +819,7 @@ return array(
     'phui-font-icon-base-css' => 'd7994e06',
     'phui-fontkit-css' => '9b714a5e',
     'phui-form-css' => '159e2d9c',
-    'phui-form-view-css' => '0807e7ac',
+    'phui-form-view-css' => '01b796c0',
     'phui-head-thing-view-css' => 'd7f293df',
     'phui-header-view-css' => '93cea4ec',
     'phui-hovercard' => '074f0783',
@@ -2111,6 +2113,11 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
+    'f84bcbf4' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+    ),
     'f8c4e135' => array(
       'javelin-install',
       'javelin-dom',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 8f06586a68..e3287df74c 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2195,6 +2195,8 @@ phutil_register_library_map(array(
     'PhabricatorAuthChallengeGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthChallengeGarbageCollector.php',
     'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php',
     'PhabricatorAuthChallengeQuery' => 'applications/auth/query/PhabricatorAuthChallengeQuery.php',
+    'PhabricatorAuthChallengeStatusController' => 'applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php',
+    'PhabricatorAuthChallengeUpdate' => 'applications/auth/view/PhabricatorAuthChallengeUpdate.php',
     'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php',
     'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php',
     'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php',
@@ -7925,6 +7927,8 @@ phutil_register_library_map(array(
     'PhabricatorAuthChallengeGarbageCollector' => 'PhabricatorGarbageCollector',
     'PhabricatorAuthChallengePHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthChallengeQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
+    'PhabricatorAuthChallengeStatusController' => 'PhabricatorAuthController',
+    'PhabricatorAuthChallengeUpdate' => 'Phobject',
     'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction',
     'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod',
     'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker',
diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index 52cf01b2aa..4e0baff229 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -97,6 +97,8 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
             'PhabricatorAuthFactorProviderViewController',
           'message/(?P<id>[1-9]\d*)/' =>
             'PhabricatorAuthFactorProviderMessageController',
+          'challenge/status/(?P<id>[1-9]\d*)/' =>
+            'PhabricatorAuthChallengeStatusController',
         ),
 
         'message/' => array(
diff --git a/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php b/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php
new file mode 100644
index 0000000000..884bbaad6d
--- /dev/null
+++ b/src/applications/auth/controller/mfa/PhabricatorAuthChallengeStatusController.php
@@ -0,0 +1,40 @@
+<?php
+
+final class PhabricatorAuthChallengeStatusController
+  extends PhabricatorAuthController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');
+    $now = PhabricatorTime::getNow();
+
+    $result = new PhabricatorAuthChallengeUpdate();
+
+    $challenge = id(new PhabricatorAuthChallengeQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->withUserPHIDs(array($viewer->getPHID()))
+      ->withChallengeTTLBetween($now, null)
+      ->executeOne();
+    if ($challenge) {
+      $config = id(new PhabricatorAuthFactorConfigQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($challenge->getFactorPHID()))
+        ->executeOne();
+      if ($config) {
+        $provider = $config->getFactorProvider();
+        $factor = $provider->getFactor();
+
+        $result = $factor->newChallengeStatusView(
+          $config,
+          $provider,
+          $viewer,
+          $challenge);
+      }
+    }
+
+    return id(new AphrontAjaxResponse())
+      ->setContent($result->newContent());
+  }
+
+}
diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php
index 912a2c31c9..d7e6e60ecc 100644
--- a/src/applications/auth/factor/PhabricatorAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactor.php
@@ -80,6 +80,14 @@ abstract class PhabricatorAuthFactor extends Phobject {
     return array();
   }
 
+  public function newChallengeStatusView(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorAuthFactorProvider $provider,
+    PhabricatorUser $viewer,
+    PhabricatorAuthChallenge $challenge) {
+    return null;
+  }
+
   /**
    * Is this a factor which depends on the user's contact number?
    *
@@ -210,8 +218,6 @@ abstract class PhabricatorAuthFactor extends Phobject {
           get_class($this)));
     }
 
-    $result->setIssuedChallenges($challenges);
-
     return $result;
   }
 
@@ -242,8 +248,6 @@ abstract class PhabricatorAuthFactor extends Phobject {
           get_class($this)));
     }
 
-    $result->setIssuedChallenges($challenges);
-
     return $result;
   }
 
@@ -339,9 +343,18 @@ abstract class PhabricatorAuthFactor extends Phobject {
         ->setIcon('fa-commenting', 'green');
     }
 
-    return id(new PHUIFormTimerControl())
+    $control = id(new PHUIFormTimerControl())
       ->setIcon($icon)
       ->appendChild($error);
+
+    $status_challenge = $result->getStatusChallenge();
+    if ($status_challenge) {
+      $id = $status_challenge->getID();
+      $uri = "/auth/mfa/challenge/status/{$id}/";
+      $control->setUpdateURI($uri);
+    }
+
+    return $control;
   }
 
 
diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php
index f03c3674da..b5da379545 100644
--- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php
+++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php
@@ -11,6 +11,7 @@ final class PhabricatorAuthFactorResult
   private $value;
   private $issuedChallenges = array();
   private $icon;
+  private $statusChallenge;
 
   public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) {
     if (!$challenge->getIsAnsweredChallenge()) {
@@ -34,6 +35,15 @@ final class PhabricatorAuthFactorResult
     return $this->answeredChallenge;
   }
 
+  public function setStatusChallenge(PhabricatorAuthChallenge $challenge) {
+    $this->statusChallenge = $challenge;
+    return $this;
+  }
+
+  public function getStatusChallenge() {
+    return $this->statusChallenge;
+  }
+
   public function getIsValid() {
     return (bool)$this->getAnsweredChallenge();
   }
@@ -83,16 +93,6 @@ final class PhabricatorAuthFactorResult
     return $this->value;
   }
 
-  public function setIssuedChallenges(array $issued_challenges) {
-    assert_instances_of($issued_challenges, 'PhabricatorAuthChallenge');
-    $this->issuedChallenges = $issued_challenges;
-    return $this;
-  }
-
-  public function getIssuedChallenges() {
-    return $this->issuedChallenges;
-  }
-
   public function setIcon(PHUIIconView $icon) {
     $this->icon = $icon;
     return $this;
diff --git a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
index 4be4c15ea8..66bd7c9ebd 100644
--- a/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorDuoAuthFactor.php
@@ -585,7 +585,7 @@ final class PhabricatorDuoAuthFactor
       $result = $this->newDuoFuture($provider)
         ->setHTTPMethod('GET')
         ->setMethod('auth_status', $parameters)
-        ->setTimeout(5)
+        ->setTimeout(3)
         ->resolve();
 
       $state = $result['response']['result'];
@@ -661,15 +661,6 @@ final class PhabricatorDuoAuthFactor
     PhabricatorAuthFactorResult $result) {
 
     $control = $this->newAutomaticControl($result);
-    if (!$control) {
-      $result = $this->newResult()
-        ->setIsContinue(true)
-        ->setErrorMessage(
-          pht(
-            'A challenge has been sent to your phone. Open the Duo '.
-            'application and confirm the challenge, then continue.'));
-      $control = $this->newAutomaticControl($result);
-    }
 
     $control
       ->setLabel(pht('Duo'))
@@ -689,7 +680,27 @@ final class PhabricatorDuoAuthFactor
     PhabricatorUser $viewer,
     AphrontRequest $request,
     array $challenges) {
-    return $this->newResult();
+
+    $result = $this->newResult()
+      ->setIsContinue(true)
+      ->setErrorMessage(
+        pht(
+          'A challenge has been sent to your phone. Open the Duo '.
+          'application and confirm the challenge, then continue.'));
+
+    $challenge = $this->getChallengeForCurrentContext(
+      $config,
+      $viewer,
+      $challenges);
+    if ($challenge) {
+      $result
+        ->setStatusChallenge($challenge)
+        ->setIcon(
+          id(new PHUIIconView())
+            ->setIcon('fa-refresh', 'green ph-spin'));
+    }
+
+    return $result;
   }
 
   private function newDuoFuture(PhabricatorAuthFactorProvider $provider) {
@@ -790,4 +801,54 @@ final class PhabricatorDuoAuthFactor
         $hostname));
   }
 
+  public function newChallengeStatusView(
+    PhabricatorAuthFactorConfig $config,
+    PhabricatorAuthFactorProvider $provider,
+    PhabricatorUser $viewer,
+    PhabricatorAuthChallenge $challenge) {
+
+    $duo_xaction = $challenge->getChallengeKey();
+
+    $parameters = array(
+      'txid' => $duo_xaction,
+    );
+
+    $default_result = id(new PhabricatorAuthChallengeUpdate())
+      ->setRetry(true);
+
+    try {
+      $result = $this->newDuoFuture($provider)
+        ->setHTTPMethod('GET')
+        ->setMethod('auth_status', $parameters)
+        ->setTimeout(5)
+        ->resolve();
+
+      $state = $result['response']['result'];
+    } catch (HTTPFutureCURLResponseStatus $exception) {
+      // If we failed or timed out, retry. Usually, this is a timeout.
+      return id(new PhabricatorAuthChallengeUpdate())
+        ->setRetry(true);
+    }
+
+    // For now, don't update the view for anything but an "Allow". Updates
+    // here are just about providing more visual feedback for user convenience.
+    if ($state !== 'allow') {
+      return id(new PhabricatorAuthChallengeUpdate())
+        ->setRetry(false);
+    }
+
+    $icon = id(new PHUIIconView())
+      ->setIcon('fa-check-circle-o', 'green');
+
+    $view = id(new PHUIFormTimerControl())
+      ->setIcon($icon)
+      ->appendChild(pht('You responded to this challenge correctly.'))
+      ->newTimerView();
+
+    return id(new PhabricatorAuthChallengeUpdate())
+      ->setState('allow')
+      ->setRetry(false)
+      ->setMarkup($view);
+  }
+
 }
diff --git a/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php b/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php
new file mode 100644
index 0000000000..a8ae5b8825
--- /dev/null
+++ b/src/applications/auth/view/PhabricatorAuthChallengeUpdate.php
@@ -0,0 +1,44 @@
+<?php
+
+final class PhabricatorAuthChallengeUpdate
+  extends Phobject {
+
+  private $retry = false;
+  private $state;
+  private $markup;
+
+  public function setRetry($retry) {
+    $this->retry = $retry;
+    return $this;
+  }
+
+  public function getRetry() {
+    return $this->retry;
+  }
+
+  public function setState($state) {
+    $this->state = $state;
+    return $this;
+  }
+
+  public function getState() {
+    return $this->state;
+  }
+
+  public function setMarkup($markup) {
+    $this->markup = $markup;
+    return $this;
+  }
+
+  public function getMarkup() {
+    return $this->markup;
+  }
+
+  public function newContent() {
+    return array(
+      'retry' => $this->getRetry(),
+      'state' => $this->getState(),
+      'markup' => $this->getMarkup(),
+    );
+  }
+}
diff --git a/src/view/form/control/PHUIFormTimerControl.php b/src/view/form/control/PHUIFormTimerControl.php
index 7229d649e9..090de2c8e4 100644
--- a/src/view/form/control/PHUIFormTimerControl.php
+++ b/src/view/form/control/PHUIFormTimerControl.php
@@ -3,6 +3,7 @@
 final class PHUIFormTimerControl extends AphrontFormControl {
 
   private $icon;
+  private $updateURI;
 
   public function setIcon(PHUIIconView $icon) {
     $this->icon = $icon;
@@ -13,11 +14,24 @@ final class PHUIFormTimerControl extends AphrontFormControl {
     return $this->icon;
   }
 
+  public function setUpdateURI($update_uri) {
+    $this->updateURI = $update_uri;
+    return $this;
+  }
+
+  public function getUpdateURI() {
+    return $this->updateURI;
+  }
+
   protected function getCustomControlClass() {
     return 'phui-form-timer';
   }
 
   protected function renderInput() {
+    return $this->newTimerView();
+  }
+
+  public function newTimerView() {
     $icon_cell = phutil_tag(
       'td',
       array(
@@ -34,7 +48,21 @@ final class PHUIFormTimerControl extends AphrontFormControl {
 
     $row = phutil_tag('tr', array(), array($icon_cell, $content_cell));
 
-    return phutil_tag('table', array(), $row);
+    $node_id = null;
+
+    $update_uri = $this->getUpdateURI();
+    if ($update_uri) {
+      $node_id = celerity_generate_unique_node_id();
+
+      Javelin::initBehavior(
+        'phui-timer-control',
+        array(
+          'nodeID' => $node_id,
+          'uri' => $update_uri,
+        ));
+    }
+
+    return phutil_tag('table', array('id' => $node_id), $row);
   }
 
 }
diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css
index 3368bcaafb..accce86819 100644
--- a/webroot/rsrc/css/phui/phui-form-view.css
+++ b/webroot/rsrc/css/phui/phui-form-view.css
@@ -578,3 +578,17 @@ properly, and submit values. */
 .mfa-form-enroll-button {
   text-align: center;
 }
+
+.phui-form-timer-updated {
+  animation: phui-form-timer-fade-in 2s linear;
+}
+
+
+@keyframes phui-form-timer-fade-in {
+  0% {
+    background-color: {$lightyellow};
+  }
+  100% {
+    background-color: transparent;
+  }
+}
diff --git a/webroot/rsrc/js/phui/behavior-phui-timer-control.js b/webroot/rsrc/js/phui/behavior-phui-timer-control.js
new file mode 100644
index 0000000000..d5b73a5ee2
--- /dev/null
+++ b/webroot/rsrc/js/phui/behavior-phui-timer-control.js
@@ -0,0 +1,41 @@
+/**
+ * @provides javelin-behavior-phui-timer-control
+ * @requires javelin-behavior
+ *           javelin-stratcom
+ *           javelin-dom
+ */
+
+JX.behavior('phui-timer-control', function(config) {
+  var node = JX.$(config.nodeID);
+  var uri = config.uri;
+  var state = null;
+
+  function onupdate(result) {
+    var markup = result.markup;
+    if (markup) {
+      var new_node = JX.$H(markup).getFragment().firstChild;
+      JX.DOM.replace(node, new_node);
+      node = new_node;
+
+      // If the overall state has changed from the previous display state,
+      // animate the control to draw the user's attention to the state change.
+      if (result.state !== state) {
+        state = result.state;
+        JX.DOM.alterClass(node, 'phui-form-timer-updated', true);
+      }
+    }
+
+    var retry = result.retry;
+    if (retry) {
+      setTimeout(update, 1000);
+    }
+  }
+
+  function update() {
+    new JX.Request(uri, onupdate)
+      .setTimeout(10000)
+      .send();
+  }
+
+  update();
+});

From 8f8e863613c0935a0272fdbe7825480f9a0781b0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Feb 2019 05:22:39 -0800
Subject: [PATCH 35/36] When users follow an email login link but an install
 does not use passwords, try to get them to link an account

Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.

If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.

This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".

Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:

  - When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
  - This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.

So: step forward, but more work to be done.

Test Plan: {F6211115}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20170
---
 resources/celerity/map.php                    |   6 +-
 src/__phutil_library_map__.php                |   4 +
 .../PhabricatorAuthApplication.php            |   2 +
 .../PhabricatorAuthOneTimeLoginController.php |  32 ++++-
 .../PhabricatorAuthSetExternalController.php  | 110 ++++++++++++++++++
 .../PhabricatorAuthLinkMessageType.php        |  18 +++
 .../auth/provider/PhabricatorAuthProvider.php |   2 +-
 .../PhabricatorPasswordAuthProvider.php       |   3 +-
 webroot/rsrc/css/phui/phui-object-box.css     |   5 +
 9 files changed, 174 insertions(+), 8 deletions(-)
 create mode 100644 src/applications/auth/controller/PhabricatorAuthSetExternalController.php
 create mode 100644 src/applications/auth/message/PhabricatorAuthLinkMessageType.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index 2dd5cf0966..b1a97639c2 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' => 'e0f5d66f',
+    'core.pkg.css' => '4ed8ce1f',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -164,7 +164,7 @@ return array(
     'rsrc/css/phui/phui-left-right.css' => '68513c34',
     'rsrc/css/phui/phui-lightbox.css' => '4ebf22da',
     'rsrc/css/phui/phui-list.css' => '470b1adb',
-    'rsrc/css/phui/phui-object-box.css' => '9b58483d',
+    'rsrc/css/phui/phui-object-box.css' => 'f434b6be',
     'rsrc/css/phui/phui-pager.css' => 'd022c7ad',
     'rsrc/css/phui/phui-pinboard-view.css' => '1f08f5d8',
     'rsrc/css/phui/phui-property-list-view.css' => 'cad62236',
@@ -833,7 +833,7 @@ return array(
     'phui-left-right-css' => '68513c34',
     'phui-lightbox-css' => '4ebf22da',
     'phui-list-view-css' => '470b1adb',
-    'phui-object-box-css' => '9b58483d',
+    'phui-object-box-css' => 'f434b6be',
     'phui-oi-big-ui-css' => '9e037c7a',
     'phui-oi-color-css' => 'b517bfa0',
     'phui-oi-drag-ui-css' => 'da15d3dc',
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index e3287df74c..b10c0e109f 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2272,6 +2272,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthInviteVerifyException' => 'applications/auth/exception/PhabricatorAuthInviteVerifyException.php',
     'PhabricatorAuthInviteWorker' => 'applications/auth/worker/PhabricatorAuthInviteWorker.php',
     'PhabricatorAuthLinkController' => 'applications/auth/controller/PhabricatorAuthLinkController.php',
+    'PhabricatorAuthLinkMessageType' => 'applications/auth/message/PhabricatorAuthLinkMessageType.php',
     'PhabricatorAuthListController' => 'applications/auth/controller/config/PhabricatorAuthListController.php',
     'PhabricatorAuthLoginController' => 'applications/auth/controller/PhabricatorAuthLoginController.php',
     'PhabricatorAuthLoginMessageType' => 'applications/auth/message/PhabricatorAuthLoginMessageType.php',
@@ -2370,6 +2371,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionPHIDType' => 'applications/auth/phid/PhabricatorAuthSessionPHIDType.php',
     'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php',
     'PhabricatorAuthSessionRevoker' => 'applications/auth/revoker/PhabricatorAuthSessionRevoker.php',
+    'PhabricatorAuthSetExternalController' => 'applications/auth/controller/PhabricatorAuthSetExternalController.php',
     'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php',
     'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php',
     'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php',
@@ -8023,6 +8025,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthInviteVerifyException' => 'PhabricatorAuthInviteDialogException',
     'PhabricatorAuthInviteWorker' => 'PhabricatorWorker',
     'PhabricatorAuthLinkController' => 'PhabricatorAuthController',
+    'PhabricatorAuthLinkMessageType' => 'PhabricatorAuthMessageType',
     'PhabricatorAuthListController' => 'PhabricatorAuthProviderConfigController',
     'PhabricatorAuthLoginController' => 'PhabricatorAuthController',
     'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType',
@@ -8142,6 +8145,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthSessionPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthSessionRevoker' => 'PhabricatorAuthRevoker',
+    'PhabricatorAuthSetExternalController' => 'PhabricatorAuthController',
     'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController',
     'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorAuthStartController' => 'PhabricatorAuthController',
diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php
index 4e0baff229..3446ad597d 100644
--- a/src/applications/auth/application/PhabricatorAuthApplication.php
+++ b/src/applications/auth/application/PhabricatorAuthApplication.php
@@ -86,7 +86,9 @@ final class PhabricatorAuthApplication extends PhabricatorApplication {
             => 'PhabricatorAuthSSHKeyRevokeController',
           'view/(?P<id>\d+)/' => 'PhabricatorAuthSSHKeyViewController',
         ),
+
         'password/' => 'PhabricatorAuthSetPasswordController',
+        'external/' => 'PhabricatorAuthSetExternalController',
 
         'mfa/' => array(
           $this->getQueryRoutePattern() =>
diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index 353f31562c..d176a67119 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -225,17 +225,45 @@ final class PhabricatorAuthOneTimeLoginController
       return (string)new PhutilURI($panel_uri, $params);
     }
 
-    $providers = id(new PhabricatorAuthProviderConfigQuery())
+    // Check if the user already has external accounts linked. If they do,
+    // it's not obvious why they aren't using them to log in, but assume they
+    // know what they're doing. We won't send them to the link workflow.
+    $accounts = id(new PhabricatorExternalAccountQuery())
+      ->setViewer($user)
+      ->withUserPHIDs(array($user->getPHID()))
+      ->execute();
+
+    $configs = id(new PhabricatorAuthProviderConfigQuery())
       ->setViewer($user)
       ->withIsEnabled(true)
       ->execute();
 
+    $linkable = array();
+    foreach ($configs as $config) {
+      if (!$config->getShouldAllowLink()) {
+        continue;
+      }
+
+      $provider = $config->getProvider();
+      if (!$provider->isLoginFormAButton()) {
+        continue;
+      }
+
+      $linkable[] = $provider;
+    }
+
+    // If there's at least one linkable provider, and the user doesn't already
+    // have accounts, send the user to the link workflow.
+    if (!$accounts && $linkable) {
+      return '/auth/external/';
+    }
+
     // 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()) {
+    if (!$configs && $user->getIsAdmin()) {
       return '/auth/';
     }
 
diff --git a/src/applications/auth/controller/PhabricatorAuthSetExternalController.php b/src/applications/auth/controller/PhabricatorAuthSetExternalController.php
new file mode 100644
index 0000000000..51dfcab53f
--- /dev/null
+++ b/src/applications/auth/controller/PhabricatorAuthSetExternalController.php
@@ -0,0 +1,110 @@
+<?php
+
+final class PhabricatorAuthSetExternalController
+  extends PhabricatorAuthController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $configs = id(new PhabricatorAuthProviderConfigQuery())
+      ->setViewer($viewer)
+      ->withIsEnabled(true)
+      ->execute();
+
+    $linkable = array();
+    foreach ($configs as $config) {
+      if (!$config->getShouldAllowLink()) {
+        continue;
+      }
+
+      // For now, only buttons get to appear here: for example, we can't
+      // reasonably embed an entire LDAP form into this UI.
+      $provider = $config->getProvider();
+      if (!$provider->isLoginFormAButton()) {
+        continue;
+      }
+
+      $linkable[] = $config;
+    }
+
+    if (!$linkable) {
+      return $this->newDialog()
+        ->setTitle(pht('No Linkable External Providers'))
+        ->appendParagraph(
+          pht(
+            'Currently, there are no configured external auth providers '.
+            'which you can link your account to.'))
+        ->addCancelButton('/');
+    }
+
+    $text = PhabricatorAuthMessage::loadMessageText(
+      $viewer,
+      PhabricatorAuthLinkMessageType::MESSAGEKEY);
+    if (!strlen($text)) {
+      $text = pht(
+        'You can link your Phabricator account to an external account to '.
+        'allow you to log in more easily in the future. To continue, choose '.
+        'an account to link below. If you prefer not to link your account, '.
+        'you can skip this step.');
+    }
+
+    $remarkup_view = new PHUIRemarkupView($viewer, $text);
+    $remarkup_view = phutil_tag(
+      'div',
+      array(
+        'class' => 'phui-object-box-instructions',
+      ),
+      $remarkup_view);
+
+    PhabricatorCookies::setClientIDCookie($request);
+
+    $view = array();
+    foreach ($configs as $config) {
+      $provider = $config->getProvider();
+
+      $form = $provider->buildLinkForm($this);
+
+      if ($provider->isLoginFormAButton()) {
+        require_celerity_resource('auth-css');
+        $form = phutil_tag(
+          'div',
+          array(
+            'class' => 'phabricator-link-button pl',
+          ),
+          $form);
+      }
+
+      $view[] = $form;
+    }
+
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer)
+      ->appendControl(
+        id(new AphrontFormSubmitControl())
+          ->addCancelButton('/', pht('Skip This Step')));
+
+    $header = id(new PHUIHeaderView())
+      ->setHeader(pht('Link External Account'));
+
+    $box = id(new PHUIObjectBoxView())
+      ->setViewer($viewer)
+      ->setHeader($header)
+      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
+      ->appendChild($remarkup_view)
+      ->appendChild($view)
+      ->appendChild($form);
+
+    $main_view = id(new PHUITwoColumnView())
+      ->setFooter($box);
+
+    $crumbs = $this->buildApplicationCrumbs()
+      ->addTextCrumb(pht('Link External Account'))
+      ->setBorder(true);
+
+    return $this->newPage()
+      ->setTitle(pht('Link External Account'))
+      ->setCrumbs($crumbs)
+      ->appendChild($main_view);
+  }
+
+}
diff --git a/src/applications/auth/message/PhabricatorAuthLinkMessageType.php b/src/applications/auth/message/PhabricatorAuthLinkMessageType.php
new file mode 100644
index 0000000000..17991231c1
--- /dev/null
+++ b/src/applications/auth/message/PhabricatorAuthLinkMessageType.php
@@ -0,0 +1,18 @@
+<?php
+
+final class PhabricatorAuthLinkMessageType
+  extends PhabricatorAuthMessageType {
+
+  const MESSAGEKEY = 'auth.link';
+
+  public function getDisplayName() {
+    return pht('Unlinked Account Instructions');
+  }
+
+  public function getShortDescription() {
+    return pht(
+      'Guidance shown after a user logs in with an email link and is '.
+      'prompted to link an external account.');
+  }
+
+}
diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index dfdc7c6a5b..eaf52552ef 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -161,7 +161,7 @@ abstract class PhabricatorAuthProvider extends Phobject {
   abstract public function processLoginRequest(
     PhabricatorAuthLoginController $controller);
 
-  public function buildLinkForm(PhabricatorAuthLinkController $controller) {
+  public function buildLinkForm($controller) {
     return $this->renderLoginForm($controller->getRequest(), $mode = 'link');
   }
 
diff --git a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
index 146e41706a..d39e378798 100644
--- a/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorPasswordAuthProvider.php
@@ -159,8 +159,7 @@ final class PhabricatorPasswordAuthProvider extends PhabricatorAuthProvider {
     return $dialog;
   }
 
-  public function buildLinkForm(
-    PhabricatorAuthLinkController $controller) {
+  public function buildLinkForm($controller) {
     throw new Exception(pht("Password providers can't be linked."));
   }
 
diff --git a/webroot/rsrc/css/phui/phui-object-box.css b/webroot/rsrc/css/phui/phui-object-box.css
index 4999a4c2c2..f95e36eedc 100644
--- a/webroot/rsrc/css/phui/phui-object-box.css
+++ b/webroot/rsrc/css/phui/phui-object-box.css
@@ -158,3 +158,8 @@ div.phui-object-box.phui-object-box-flush {
   margin-top: 8px;
   margin-bottom: 8px;
 }
+
+.phui-object-box-instructions {
+  padding: 16px;
+  border-bottom: 1px solid {$thinblueborder};
+}

From 8810cd2f4d1572e536f7aab7e76969bea80fda65 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 13 Feb 2019 17:50:10 -0800
Subject: [PATCH 36/36] Add a standalone view for the Maniphest task graph

Summary:
See PHI1073. Improve the UX here:

  - When there are a small number of connected tasks, no changes.
  - When there are too many total connected tasks, but not too many directly connected tasks, show hint text with a "View Standalone Graph" button to view more of the graph.
  - When there are too many directly connected tasks, show better hint text with a "View Standalone Graph" button.
  - Always show a "View Standalone Graph" option in the dropdown menu.
  - Add a standalone view which works the same way but has a limit of 2,000.
    - This view doesn't have "View Standalone Graph" links, since they'd just link back to the same page, but is basically the same otherwise.
  - Increase the main page task limit from 100 to 200.

Test Plan:
Mobile View:

{F6210326}

Way too much stuff:

{F6210327}

New persistent link to the standalone page:

{F6210328}

Kind of too much stuff:

{F6210329}

Standalone view:

{F6210330}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: 20after4

Differential Revision: https://secure.phabricator.com/D20164
---
 resources/celerity/map.php                    |   6 +-
 src/__phutil_library_map__.php                |   2 +
 .../PhabricatorManiphestApplication.php       |   1 +
 .../controller/ManiphestController.php        |  98 ++++++++++++++
 .../ManiphestTaskDetailController.php         |  73 +++++-----
 .../ManiphestTaskGraphController.php          | 125 ++++++++++++++++++
 webroot/rsrc/css/aphront/table-view.css       |  37 ++++++
 7 files changed, 300 insertions(+), 42 deletions(-)
 create mode 100644 src/applications/maniphest/controller/ManiphestTaskGraphController.php

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
index b1a97639c2..2d402dbaa3 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' => '4ed8ce1f',
+    'core.pkg.css' => '85a1da99',
     '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' => 'daa1f9df',
+    'rsrc/css/aphront/table-view.css' => '205053cd',
     'rsrc/css/aphront/tokenizer.css' => 'b52d0668',
     'rsrc/css/aphront/tooltip.css' => 'e3f2412f',
     'rsrc/css/aphront/typeahead-browse.css' => 'b7ed02d2',
@@ -520,7 +520,7 @@ return array(
     'aphront-list-filter-view-css' => 'feb64255',
     'aphront-multi-column-view-css' => 'fbc00ba3',
     'aphront-panel-view-css' => '46923d46',
-    'aphront-table-view-css' => 'daa1f9df',
+    'aphront-table-view-css' => '205053cd',
     '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 b10c0e109f..3c9a329d99 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1714,6 +1714,7 @@ phutil_register_library_map(array(
     'ManiphestTaskFerretEngine' => 'applications/maniphest/search/ManiphestTaskFerretEngine.php',
     'ManiphestTaskFulltextEngine' => 'applications/maniphest/search/ManiphestTaskFulltextEngine.php',
     'ManiphestTaskGraph' => 'infrastructure/graph/ManiphestTaskGraph.php',
+    'ManiphestTaskGraphController' => 'applications/maniphest/controller/ManiphestTaskGraphController.php',
     'ManiphestTaskHasCommitEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php',
     'ManiphestTaskHasCommitRelationship' => 'applications/maniphest/relationship/ManiphestTaskHasCommitRelationship.php',
     'ManiphestTaskHasDuplicateTaskEdgeType' => 'applications/maniphest/edge/ManiphestTaskHasDuplicateTaskEdgeType.php',
@@ -7401,6 +7402,7 @@ phutil_register_library_map(array(
     'ManiphestTaskFerretEngine' => 'PhabricatorFerretEngine',
     'ManiphestTaskFulltextEngine' => 'PhabricatorFulltextEngine',
     'ManiphestTaskGraph' => 'PhabricatorObjectGraph',
+    'ManiphestTaskGraphController' => 'ManiphestController',
     'ManiphestTaskHasCommitEdgeType' => 'PhabricatorEdgeType',
     'ManiphestTaskHasCommitRelationship' => 'ManiphestTaskRelationship',
     'ManiphestTaskHasDuplicateTaskEdgeType' => 'PhabricatorEdgeType',
diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php
index 35c1efb6e8..ec732791fa 100644
--- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php
+++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php
@@ -55,6 +55,7 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication {
           'subtask/(?P<id>[1-9]\d*)/' => 'ManiphestTaskSubtaskController',
         ),
         'subpriority/' => 'ManiphestSubpriorityController',
+        'graph/(?P<id>[1-9]\d*)/' => 'ManiphestTaskGraphController',
       ),
     );
   }
diff --git a/src/applications/maniphest/controller/ManiphestController.php b/src/applications/maniphest/controller/ManiphestController.php
index c80a1a462a..872d3f7b38 100644
--- a/src/applications/maniphest/controller/ManiphestController.php
+++ b/src/applications/maniphest/controller/ManiphestController.php
@@ -61,4 +61,102 @@ abstract class ManiphestController extends PhabricatorController {
     return $view;
   }
 
+  final protected function newTaskGraphDropdownMenu(
+    ManiphestTask $task,
+    $has_parents,
+    $has_subtasks,
+    $include_standalone) {
+    $viewer = $this->getViewer();
+
+    $parents_uri = urisprintf(
+      '/?subtaskIDs=%d#R',
+      $task->getID());
+    $parents_uri = $this->getApplicationURI($parents_uri);
+
+    $subtasks_uri = urisprintf(
+      '/?parentIDs=%d#R',
+      $task->getID());
+    $subtasks_uri = $this->getApplicationURI($subtasks_uri);
+
+    $dropdown_menu = id(new PhabricatorActionListView())
+      ->setViewer($viewer)
+      ->addAction(
+        id(new PhabricatorActionView())
+          ->setHref($parents_uri)
+          ->setName(pht('Search Parent Tasks'))
+          ->setDisabled(!$has_parents)
+          ->setIcon('fa-chevron-circle-up'))
+      ->addAction(
+        id(new PhabricatorActionView())
+          ->setHref($subtasks_uri)
+          ->setName(pht('Search Subtasks'))
+          ->setDisabled(!$has_subtasks)
+          ->setIcon('fa-chevron-circle-down'));
+
+    if ($include_standalone) {
+      $standalone_uri = urisprintf('/graph/%d/', $task->getID());
+      $standalone_uri = $this->getApplicationURI($standalone_uri);
+
+      $dropdown_menu->addAction(
+        id(new PhabricatorActionView())
+          ->setHref($standalone_uri)
+          ->setName(pht('View Standalone Graph'))
+          ->setIcon('fa-code-fork'));
+    }
+
+    $graph_menu = id(new PHUIButtonView())
+      ->setTag('a')
+      ->setIcon('fa-search')
+      ->setText(pht('Search...'))
+      ->setDropdownMenu($dropdown_menu);
+
+    return $graph_menu;
+  }
+
+  final protected function newTaskGraphOverflowView(
+    ManiphestTask $task,
+    $overflow_message,
+    $include_standalone) {
+
+    $id = $task->getID();
+
+    if ($include_standalone) {
+      $standalone_uri = $this->getApplicationURI("graph/{$id}/");
+
+      $standalone_link = id(new PHUIButtonView())
+        ->setTag('a')
+        ->setHref($standalone_uri)
+        ->setColor(PHUIButtonView::GREY)
+        ->setIcon('fa-code-fork')
+        ->setText(pht('View Standalone Graph'));
+    } else {
+      $standalone_link = null;
+    }
+
+    $standalone_icon = id(new PHUIIconView())
+      ->setIcon('fa-exclamation-triangle', 'yellow')
+      ->addClass('object-graph-header-icon');
+
+    $standalone_view = phutil_tag(
+      'div',
+      array(
+        'class' => 'object-graph-header',
+      ),
+      array(
+        $standalone_link,
+        $standalone_icon,
+        phutil_tag(
+          'div',
+          array(
+            'class' => 'object-graph-header-message',
+          ),
+          array(
+            $overflow_message,
+          )),
+      ));
+
+    return $standalone_view;
+  }
+
+
 }
diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
index ba826e16e8..c5dba7d3b5 100644
--- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php
@@ -80,7 +80,8 @@ final class ManiphestTaskDetailController extends ManiphestController {
     $related_tabs = array();
     $graph_menu = null;
 
-    $graph_limit = 100;
+    $graph_limit = 200;
+    $overflow_message = null;
     $task_graph = id(new ManiphestTaskGraph())
       ->setViewer($viewer)
       ->setSeedPHID($task->getPHID())
@@ -96,61 +97,55 @@ final class ManiphestTaskDetailController extends ManiphestController {
       $has_parents = (bool)$parent_list;
       $has_subtasks = (bool)$subtask_list;
 
-      $search_text = pht('Search...');
-
       // First, get a count of direct parent tasks and subtasks. If there
       // are too many of these, we just don't draw anything. You can use
       // the search button to browse tasks with the search UI instead.
       $direct_count = count($parent_list) + count($subtask_list);
 
       if ($direct_count > $graph_limit) {
-        $message = pht(
-          'Task graph too large to display (this task is directly connected '.
-          'to more than %s other tasks). Use %s to explore connected tasks.',
-          $graph_limit,
-          phutil_tag('strong', array(), $search_text));
-        $message = phutil_tag('em', array(), $message);
-        $graph_table = id(new PHUIPropertyListView())
-          ->addTextContent($message);
+        $overflow_message = pht(
+          'This task is directly connected to more than %s other tasks. '.
+          'Use %s to browse parents or subtasks, or %s to show more of the '.
+          'graph.',
+          new PhutilNumber($graph_limit),
+          phutil_tag('strong', array(), pht('Search...')),
+          phutil_tag('strong', array(), pht('View Standalone Graph')));
+
+        $graph_table = null;
       } else {
         // If there aren't too many direct tasks, but there are too many total
         // tasks, we'll only render directly connected tasks.
         if ($task_graph->isOverLimit()) {
           $task_graph->setRenderOnlyAdjacentNodes(true);
+
+          $overflow_message = pht(
+            'This task is connected to more than %s other tasks. '.
+            'Only direct parents and subtasks are shown here. Use '.
+            '%s to show more of the graph.',
+            new PhutilNumber($graph_limit),
+            phutil_tag('strong', array(), pht('View Standalone Graph')));
         }
+
         $graph_table = $task_graph->newGraphTable();
       }
 
-      $parents_uri = urisprintf(
-        '/?subtaskIDs=%d#R',
-        $task->getID());
-      $parents_uri = $this->getApplicationURI($parents_uri);
+      if ($overflow_message) {
+        $overflow_view = $this->newTaskGraphOverflowView(
+          $task,
+          $overflow_message,
+          true);
 
-      $subtasks_uri = urisprintf(
-        '/?parentIDs=%d#R',
-        $task->getID());
-      $subtasks_uri = $this->getApplicationURI($subtasks_uri);
+        $graph_table = array(
+          $overflow_view,
+          $graph_table,
+        );
+      }
 
-      $dropdown_menu = id(new PhabricatorActionListView())
-        ->setViewer($viewer)
-        ->addAction(
-          id(new PhabricatorActionView())
-            ->setHref($parents_uri)
-            ->setName(pht('Search Parent Tasks'))
-            ->setDisabled(!$has_parents)
-            ->setIcon('fa-chevron-circle-up'))
-        ->addAction(
-          id(new PhabricatorActionView())
-            ->setHref($subtasks_uri)
-            ->setName(pht('Search Subtasks'))
-            ->setDisabled(!$has_subtasks)
-            ->setIcon('fa-chevron-circle-down'));
-
-      $graph_menu = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setIcon('fa-search')
-        ->setText($search_text)
-        ->setDropdownMenu($dropdown_menu);
+      $graph_menu = $this->newTaskGraphDropdownMenu(
+        $task,
+        $has_parents,
+        $has_subtasks,
+        true);
 
       $related_tabs[] = id(new PHUITabView())
         ->setName(pht('Task Graph'))
diff --git a/src/applications/maniphest/controller/ManiphestTaskGraphController.php b/src/applications/maniphest/controller/ManiphestTaskGraphController.php
new file mode 100644
index 0000000000..2f342a2d0f
--- /dev/null
+++ b/src/applications/maniphest/controller/ManiphestTaskGraphController.php
@@ -0,0 +1,125 @@
+<?php
+
+final class ManiphestTaskGraphController
+  extends ManiphestController {
+
+  public function shouldAllowPublic() {
+    return true;
+  }
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+    $id = $request->getURIData('id');
+
+    $task = id(new ManiphestTaskQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->executeOne();
+    if (!$task) {
+      return new Aphront404Response();
+    }
+
+    $crumbs = $this->buildApplicationCrumbs()
+      ->addTextCrumb($task->getMonogram(), $task->getURI())
+      ->addTextCrumb(pht('Graph'))
+      ->setBorder(true);
+
+    $graph_limit = 2000;
+    $overflow_message = null;
+    $task_graph = id(new ManiphestTaskGraph())
+      ->setViewer($viewer)
+      ->setSeedPHID($task->getPHID())
+      ->setLimit($graph_limit)
+      ->loadGraph();
+    if (!$task_graph->isEmpty()) {
+      $parent_type = ManiphestTaskDependedOnByTaskEdgeType::EDGECONST;
+      $subtask_type = ManiphestTaskDependsOnTaskEdgeType::EDGECONST;
+      $parent_map = $task_graph->getEdges($parent_type);
+      $subtask_map = $task_graph->getEdges($subtask_type);
+      $parent_list = idx($parent_map, $task->getPHID(), array());
+      $subtask_list = idx($subtask_map, $task->getPHID(), array());
+      $has_parents = (bool)$parent_list;
+      $has_subtasks = (bool)$subtask_list;
+
+      // First, get a count of direct parent tasks and subtasks. If there
+      // are too many of these, we just don't draw anything. You can use
+      // the search button to browse tasks with the search UI instead.
+      $direct_count = count($parent_list) + count($subtask_list);
+
+      if ($direct_count > $graph_limit) {
+        $overflow_message = pht(
+          'This task is directly connected to more than %s other tasks, '.
+          'which is too many tasks to display. Use %s to browse parents '.
+          'or subtasks.',
+          new PhutilNumber($graph_limit),
+          phutil_tag('strong', array(), pht('Search...')));
+
+        $graph_table = null;
+      } else {
+        // If there aren't too many direct tasks, but there are too many total
+        // tasks, we'll only render directly connected tasks.
+        if ($task_graph->isOverLimit()) {
+          $task_graph->setRenderOnlyAdjacentNodes(true);
+
+          $overflow_message = pht(
+            'This task is connected to more than %s other tasks. '.
+            'Only direct parents and subtasks are shown here.',
+            new PhutilNumber($graph_limit));
+        }
+
+        $graph_table = $task_graph->newGraphTable();
+      }
+
+      $graph_menu = $this->newTaskGraphDropdownMenu(
+        $task,
+        $has_parents,
+        $has_subtasks,
+        false);
+    } else {
+      $graph_menu = null;
+      $graph_table = null;
+
+      $overflow_message = pht(
+        'This task has no parent tasks and no subtasks, so there is no '.
+        'graph to draw.');
+    }
+
+    if ($overflow_message) {
+      $overflow_view = $this->newTaskGraphOverflowView(
+        $task,
+        $overflow_message,
+        false);
+
+      $graph_table = array(
+        $overflow_view,
+        $graph_table,
+      );
+    }
+
+    $header = id(new PHUIHeaderView())
+      ->setHeader(pht('Task Graph'));
+
+    if ($graph_menu) {
+      $header->addActionLink($graph_menu);
+    }
+
+    $tab_view = id(new PHUIObjectBoxView())
+      ->setHeader($header)
+      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
+      ->appendChild($graph_table);
+
+    $view = id(new PHUITwoColumnView())
+      ->setFooter($tab_view);
+
+    return $this->newPage()
+      ->setTitle(
+        array(
+          $task->getMonogram(),
+          pht('Graph'),
+        ))
+      ->setCrumbs($crumbs)
+      ->appendChild($view);
+  }
+
+
+}
diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css
index 9bc8536054..a08674cf14 100644
--- a/webroot/rsrc/css/aphront/table-view.css
+++ b/webroot/rsrc/css/aphront/table-view.css
@@ -327,3 +327,40 @@ span.single-display-line-content {
 .phui-object-box .aphront-table-view {
   border: none;
 }
+
+.object-graph-header {
+  padding: 8px 12px;
+  overflow: hidden;
+  background: {$lightyellow};
+  border-bottom: 1px solid {$lightblueborder};
+  vertical-align: middle;
+}
+
+.object-graph-header .object-graph-header-icon {
+  float: left;
+  margin-top: 10px;
+}
+
+.object-graph-header a.button {
+  float: right;
+}
+
+.object-graph-header-message {
+  margin: 8px 200px 8px 20px;
+}
+
+.device .object-graph-header .object-graph-header-icon {
+  display: none;
+}
+
+.device .object-graph-header-message {
+  clear: both;
+  margin: 0;
+}
+
+.device .object-graph-header a.button {
+  margin: 0 auto 12px;
+  display: block;
+  width: 180px;
+  float: none;
+}