Refine core webhook implementation somewhat

Summary: Depends on D19045. Ref T11330. - View/regenerate HMAC keys. - Pretty JSON. - Readable status transactions. - test, silent, secure flags. - Dates on request view. - More icons. - Can test any object. - GC for requests. Test Plan: Went through each feature poking at it in the web UI and with `bin/webhook call ...` / `bin/garbage collect ...`. Subscribers: ftdysa Maniphest Tasks: T11330 Differential Revision: https://secure.phabricator.com/D19046
2025-01-22 20:51:10 +01:00 · 2018-02-09 10:45:18 -08:00 · 2018-02-09 10:45:18 -08:00 · dc2995c4ca
commit dc2995c4ca
parent 0470125d9e
13 changed files with 355 additions and 31 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -1447,12 +1447,14 @@ phutil_register_library_map(array(
    'HeraldWebhookEditController' => 'applications/herald/controller/HeraldWebhookEditController.php',
    'HeraldWebhookEditEngine' => 'applications/herald/editor/HeraldWebhookEditEngine.php',
    'HeraldWebhookEditor' => 'applications/herald/editor/HeraldWebhookEditor.php',
+    'HeraldWebhookKeyController' => 'applications/herald/controller/HeraldWebhookKeyController.php',
    'HeraldWebhookListController' => 'applications/herald/controller/HeraldWebhookListController.php',
    'HeraldWebhookManagementWorkflow' => 'applications/herald/management/HeraldWebhookManagementWorkflow.php',
    'HeraldWebhookNameTransaction' => 'applications/herald/xaction/HeraldWebhookNameTransaction.php',
    'HeraldWebhookPHIDType' => 'applications/herald/phid/HeraldWebhookPHIDType.php',
    'HeraldWebhookQuery' => 'applications/herald/query/HeraldWebhookQuery.php',
    'HeraldWebhookRequest' => 'applications/herald/storage/HeraldWebhookRequest.php',
+    'HeraldWebhookRequestGarbageCollector' => 'applications/herald/garbagecollector/HeraldWebhookRequestGarbageCollector.php',
    'HeraldWebhookRequestListView' => 'applications/herald/view/HeraldWebhookRequestListView.php',
    'HeraldWebhookRequestPHIDType' => 'applications/herald/phid/HeraldWebhookRequestPHIDType.php',
    'HeraldWebhookRequestQuery' => 'applications/herald/query/HeraldWebhookRequestQuery.php',
@ -6735,12 +6737,14 @@ phutil_register_library_map(array(
      'PhabricatorPolicyInterface',
      'PhabricatorApplicationTransactionInterface',
      'PhabricatorDestructibleInterface',
+      'PhabricatorProjectInterface',
    ),
    'HeraldWebhookCallManagementWorkflow' => 'HeraldWebhookManagementWorkflow',
    'HeraldWebhookController' => 'HeraldController',
    'HeraldWebhookEditController' => 'HeraldWebhookController',
    'HeraldWebhookEditEngine' => 'PhabricatorEditEngine',
    'HeraldWebhookEditor' => 'PhabricatorApplicationTransactionEditor',
+    'HeraldWebhookKeyController' => 'HeraldWebhookController',
    'HeraldWebhookListController' => 'HeraldWebhookController',
    'HeraldWebhookManagementWorkflow' => 'PhabricatorManagementWorkflow',
    'HeraldWebhookNameTransaction' => 'HeraldWebhookTransactionType',
@ -6751,6 +6755,7 @@ phutil_register_library_map(array(
      'PhabricatorPolicyInterface',
      'PhabricatorExtendedPolicyInterface',
    ),
+    'HeraldWebhookRequestGarbageCollector' => 'PhabricatorGarbageCollector',
    'HeraldWebhookRequestListView' => 'AphrontView',
    'HeraldWebhookRequestPHIDType' => 'PhabricatorPHIDType',
    'HeraldWebhookRequestQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
--- a/src/applications/herald/application/PhabricatorHeraldApplication.php
+++ b/src/applications/herald/application/PhabricatorHeraldApplication.php
@ -68,10 +68,8 @@ final class PhabricatorHeraldApplication extends PhabricatorApplication {
            'HeraldWebhookViewController',
          $this->getEditRoutePattern('edit/') => 'HeraldWebhookEditController',
          'test/(?P<id>\d+)/' => 'HeraldWebhookTestController',
-          'key/' => array(
-            'view/(?P<id>\d+)/' => 'HeraldWebhookViewKeyController',
-            'cycle/(?P<id>\d+)/' => 'HeraldWebhookCycleKeyController',
-          ),
+          'key/(?P<action>view|cycle)/(?P<id>\d+)/' =>
+            'HeraldWebhookKeyController',
        ),
      ),
    );
--- a/src/applications/herald/controller/HeraldWebhookKeyController.php
+++ b/src/applications/herald/controller/HeraldWebhookKeyController.php
@ -0,0 +1,56 @@
+<?php
+
+final class HeraldWebhookKeyController
+  extends HeraldWebhookController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $viewer = $this->getViewer();
+
+    $hook = id(new HeraldWebhookQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($request->getURIData('id')))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+    if (!$hook) {
+      return new Aphront404Response();
+    }
+
+    $action = $request->getURIData('action');
+    if ($action === 'cycle') {
+      if (!$request->isFormPost()) {
+        return $this->newDialog()
+          ->setTitle(pht('Regenerate HMAC Key'))
+          ->appendParagraph(
+            pht(
+              'Regenerate the HMAC key used to sign requests made by this '.
+              'webhook?'))
+          ->appendParagraph(
+            pht(
+              'Requests which are currently authenticated with the old key '.
+              'may fail.'))
+          ->addCancelButton($hook->getURI())
+          ->addSubmitButton(pht('Regnerate Key'));
+      } else {
+        $hook->regenerateHMACKey()->save();
+      }
+    }
+
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer)
+      ->appendControl(
+        id(new AphrontFormTextControl())
+          ->setLabel(pht('HMAC Key'))
+          ->setValue($hook->getHMACKey()));
+
+    return $this->newDialog()
+      ->setTitle(pht('Webhook HMAC Key'))
+      ->appendForm($form)
+      ->addCancelButton($hook->getURI(), pht('Done'));
+  }
+
+
+}
--- a/src/applications/herald/controller/HeraldWebhookTestController.php
+++ b/src/applications/herald/controller/HeraldWebhookTestController.php
@ -19,26 +19,74 @@ final class HeraldWebhookTestController
      return new Aphront404Response();
    }

+    $v_object = null;
+    $e_object = null;
+    $errors = array();
    if ($request->isFormPost()) {
-      $object = $hook;

-      $request = HeraldWebhookRequest::initializeNewWebhookRequest($hook)
-        ->setObjectPHID($object->getPHID())
-        ->save();
+      $v_object = $request->getStr('object');
+      if (!strlen($v_object)) {
+        $object = $hook;
+      } else {
+        $objects = id(new PhabricatorObjectQuery())
+          ->setViewer($viewer)
+          ->withNames(array($v_object))
+          ->execute();
+        if ($objects) {
+          $object = head($objects);
+        } else {
+          $e_object = pht('Invalid');
+          $errors[] = pht('Specified object could not be loaded.');
+        }
+      }

-      $request->queueCall();
+      if (!$errors) {
+        $xaction_query =
+          PhabricatorApplicationTransactionQuery::newQueryForObject($object);

-      $next_uri = $hook->getURI().'request/'.$request->getID().'/';
+        $xactions = $xaction_query
+          ->withObjectPHIDs(array($object->getPHID()))
+          ->setViewer($viewer)
+          ->setLimit(10)
+          ->execute();

-      return id(new AphrontRedirectResponse())->setURI($next_uri);
+        $request = HeraldWebhookRequest::initializeNewWebhookRequest($hook)
+          ->setObjectPHID($object->getPHID())
+          ->setIsTestAction(true)
+          ->setTransactionPHIDs(mpull($xactions, 'getPHID'))
+          ->save();
+
+        $request->queueCall();
+
+        $next_uri = $hook->getURI().'request/'.$request->getID().'/';
+
+        return id(new AphrontRedirectResponse())->setURI($next_uri);
+      }
    }

+    $instructions = <<<EOREMARKUP
+Optionally, choose an object to generate test data for (like `D123` or `T234`).
+
+The 10 most recent transactions for the object will be submitted to the webhook.
+EOREMARKUP;
+
+    $form = id(new AphrontFormView())
+      ->setViewer($viewer)
+      ->appendControl(
+        id(new AphrontFormTextControl())
+          ->setLabel(pht('Object'))
+          ->setName('object')
+          ->setError($e_object)
+          ->setValue($v_object));
+
    return $this->newDialog()
+      ->setErrors($errors)
+      ->setWidth(AphrontDialogView::WIDTH_FORM)
      ->setTitle(pht('New Test Request'))
-      ->appendParagraph(
-        pht('This will make a new test request to the configured URI.'))
+      ->appendParagraph(new PHUIRemarkupView($viewer, $instructions))
+      ->appendForm($form)
      ->addCancelButton($hook->getURI())
-      ->addSubmitButton(pht('Make Request'));
+      ->addSubmitButton(pht('Test Webhook'));
  }


--- a/src/applications/herald/controller/HeraldWebhookViewController.php
+++ b/src/applications/herald/controller/HeraldWebhookViewController.php
@ -94,10 +94,15 @@ final class HeraldWebhookViewController

    $title = $hook->getName();

+    $status_icon = $hook->getStatusIcon();
+    $status_color = $hook->getStatusColor();
+    $status_name = $hook->getStatusDisplayName();
+
    $header = id(new PHUIHeaderView())
      ->setHeader($title)
      ->setViewer($viewer)
      ->setPolicyObject($hook)
+      ->setStatus($status_icon, $status_color, $status_name)
      ->setHeaderIcon('fa-cloud-upload');

    return $header;
@ -117,6 +122,9 @@ final class HeraldWebhookViewController
    $edit_uri = $this->getApplicationURI("webhook/edit/{$id}/");
    $test_uri = $this->getApplicationURI("webhook/test/{$id}/");

+    $key_view_uri = $this->getApplicationURI("webhook/key/view/{$id}/");
+    $key_cycle_uri = $this->getApplicationURI("webhook/key/cycle/{$id}/");
+
    $curtain->addAction(
      id(new PhabricatorActionView())
        ->setName(pht('Edit Webhook'))
@ -133,6 +141,22 @@ final class HeraldWebhookViewController
        ->setWorkflow(true)
        ->setHref($test_uri));

+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('View HMAC Key'))
+        ->setIcon('fa-key')
+        ->setDisabled(!$can_edit)
+        ->setWorkflow(true)
+        ->setHref($key_view_uri));
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setName(pht('Regenerate HMAC Key'))
+        ->setIcon('fa-refresh')
+        ->setDisabled(!$can_edit)
+        ->setWorkflow(true)
+        ->setHref($key_cycle_uri));
+
    return $curtain;
  }

--- a/src/applications/herald/garbagecollector/HeraldWebhookRequestGarbageCollector.php
+++ b/src/applications/herald/garbagecollector/HeraldWebhookRequestGarbageCollector.php
@ -0,0 +1,29 @@
+<?php
+
+final class HeraldWebhookRequestGarbageCollector
+  extends PhabricatorGarbageCollector {
+
+  const COLLECTORCONST = 'herald.webhooks';
+
+  public function getCollectorName() {
+    return pht('Herald Webhooks');
+  }
+
+  public function getDefaultRetentionPolicy() {
+    return phutil_units('7 days in seconds');
+  }
+
+  protected function collectGarbage() {
+    $table = new HeraldWebhookRequest();
+    $conn_w = $table->establishConnection('w');
+
+    queryfx(
+      $conn_w,
+      'DELETE FROM %T WHERE dateCreated < %d LIMIT 100',
+      $table->getTableName(),
+      $this->getGarbageEpoch());
+
+    return ($conn_w->getAffectedRows() == 100);
+  }
+
+}
--- a/src/applications/herald/management/HeraldWebhookCallManagementWorkflow.php
+++ b/src/applications/herald/management/HeraldWebhookCallManagementWorkflow.php
@ -6,7 +6,7 @@ final class HeraldWebhookCallManagementWorkflow
  protected function didConstruct() {
    $this
      ->setName('call')
-      ->setExamples('**call** --id __id__')
+      ->setExamples('**call** --id __id__ [--object __object__]')
      ->setSynopsis(pht('Call a webhook.'))
      ->setArguments(
        array(
@ -15,6 +15,19 @@ final class HeraldWebhookCallManagementWorkflow
            'param' => 'id',
            'help' => pht('Webhook ID to call'),
          ),
+          array(
+            'name' => 'object',
+            'param' => 'object',
+            'help' => pht('Submit transactions for a particular object.'),
+          ),
+          array(
+            'name' => 'silent',
+            'help' => pht('Set the "silent" flag on the request.'),
+          ),
+          array(
+            'name' => 'secure',
+            'help' => pht('Set the "secure" flag on the request.'),
+          ),
        ));
  }

@ -39,12 +52,38 @@ final class HeraldWebhookCallManagementWorkflow
          $id));
    }

-    $object = $hook;
+    $object_name = $args->getArg('object');
+    if ($object_name === null) {
+      $object = $hook;
+    } else {
+      $objects = id(new PhabricatorObjectQuery())
+        ->setViewer($viewer)
+        ->withNames(array($object_name))
+        ->execute();
+      if (!$objects) {
+        throw new PhutilArgumentUsageException(
+          pht(
+            'Unable to load specified object ("%s").',
+            $object_name));
+      }
+      $object = head($objects);
+    }

-    $application_phid = id(new PhabricatorHeraldApplication())->getPHID();
+    $xaction_query =
+      PhabricatorApplicationTransactionQuery::newQueryForObject($object);
+
+    $xactions = $xaction_query
+      ->withObjectPHIDs(array($object->getPHID()))
+      ->setViewer($viewer)
+      ->setLimit(10)
+      ->execute();

    $request = HeraldWebhookRequest::initializeNewWebhookRequest($hook)
      ->setObjectPHID($object->getPHID())
+      ->setIsTestAction(true)
+      ->setIsSilentAction((bool)$args->getArg('silent'))
+      ->setIsSecureAction((bool)$args->getArg('secure'))
+      ->setTransactionPHIDs(mpull($xactions, 'getPHID'))
      ->save();

    PhabricatorWorker::setRunAllTasksInProcess(true);
--- a/src/applications/herald/query/HeraldWebhookSearchEngine.php
+++ b/src/applications/herald/query/HeraldWebhookSearchEngine.php
@ -80,9 +80,16 @@ final class HeraldWebhookSearchEngine
      ->setViewer($viewer);
    foreach ($hooks as $hook) {
      $item = id(new PHUIObjectItemView())
-        ->setObjectName(pht('Hook %d', $hook->getID()))
+        ->setObjectName(pht('Webhook %d', $hook->getID()))
        ->setHeader($hook->getName())
-        ->setHref($hook->getURI());
+        ->setHref($hook->getURI())
+        ->addAttribute($hook->getWebhookURI());
+
+      $item->addIcon($hook->getStatusIcon(), $hook->getStatusDisplayName());
+
+      if ($hook->isDisabled()) {
+        $item->setDisabled(true);
+      }

      $list->addItem($item);
    }
--- a/src/applications/herald/storage/HeraldWebhook.php
+++ b/src/applications/herald/storage/HeraldWebhook.php
@ -5,7 +5,8 @@ final class HeraldWebhook
  implements
    PhabricatorPolicyInterface,
    PhabricatorApplicationTransactionInterface,
-    PhabricatorDestructibleInterface {
+    PhabricatorDestructibleInterface,
+    PhabricatorProjectInterface {

  protected $name;
  protected $webhookURI;
@ -44,7 +45,7 @@ final class HeraldWebhook
      ->setStatus(self::HOOKSTATUS_ENABLED)
      ->setViewPolicy(PhabricatorPolicies::getMostOpenPolicy())
      ->setEditPolicy($viewer->getPHID())
-      ->setHmacKey(Filesystem::readRandomCharacters(32));
+      ->regenerateHMACKey();
  }

  public function getURI() {
@ -56,16 +57,79 @@ final class HeraldWebhook
  }

  public static function getStatusDisplayNameMap() {
-    return array(
-      self::HOOKSTATUS_FIREHOSE => pht('Firehose'),
-      self::HOOKSTATUS_ENABLED => pht('Enabled'),
-      self::HOOKSTATUS_DISABLED => pht('Disabled'),
+    $specs = self::getStatusSpecifications();
+    return ipull($specs, 'name', 'key');
+  }
+
+  private static function getStatusSpecifications() {
+    $specs = array(
+      array(
+        'key' => self::HOOKSTATUS_FIREHOSE,
+        'name' => pht('Firehose'),
+        'color' => 'orange',
+        'icon' => 'fa-star-o',
+      ),
+      array(
+        'key' => self::HOOKSTATUS_ENABLED,
+        'name' => pht('Enabled'),
+        'color' => 'bluegrey',
+        'icon' => 'fa-check',
+      ),
+      array(
+        'key' => self::HOOKSTATUS_DISABLED,
+        'name' => pht('Disabled'),
+        'color' => 'dark',
+        'icon' => 'fa-ban',
+      ),
    );
+
+    return ipull($specs, null, 'key');
+  }
+
+
+  private static function getSpecificationForStatus($status) {
+    $specs = self::getStatusSpecifications();
+
+    if (isset($specs[$status])) {
+      return $specs[$status];
+    }
+
+    return array(
+      'key' => $status,
+      'name' => pht('Unknown ("%s")', $status),
+      'icon' => 'fa-question',
+      'color' => 'indigo',
+    );
+  }
+
+  public static function getDisplayNameForStatus($status) {
+    $spec = self::getSpecificationForStatus($status);
+    return $spec['name'];
+  }
+
+  public static function getIconForStatus($status) {
+    $spec = self::getSpecificationForStatus($status);
+    return $spec['icon'];
+  }
+
+  public static function getColorForStatus($status) {
+    $spec = self::getSpecificationForStatus($status);
+    return $spec['color'];
  }

  public function getStatusDisplayName() {
    $status = $this->getStatus();
-    return idx($this->getStatusDisplayNameMap(), $status);
+    return self::getDisplayNameForStatus($status);
+  }
+
+  public function getStatusIcon() {
+    $status = $this->getStatus();
+    return self::getIconForStatus($status);
+  }
+
+  public function getStatusColor() {
+    $status = $this->getStatus();
+    return self::getColorForStatus($status);
  }

  public function getErrorBackoffWindow() {
@ -101,6 +165,9 @@ final class HeraldWebhook
    return false;
  }

+  public function regenerateHMACKey() {
+    return $this->setHMACKey(Filesystem::readRandomCharacters(32));
+  }

 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */

--- a/src/applications/herald/storage/HeraldWebhookRequest.php
+++ b/src/applications/herald/storage/HeraldWebhookRequest.php
@ -116,6 +116,30 @@ final class HeraldWebhookRequest
    return $this->getProperty('transactionPHIDs', array());
  }

+  public function setIsSilentAction($bool) {
+    return $this->setProperty('silent', $bool);
+  }
+
+  public function getIsSilentAction() {
+    return $this->getProperty('silent', false);
+  }
+
+  public function setIsTestAction($bool) {
+    return $this->setProperty('test', $bool);
+  }
+
+  public function getIsTestAction() {
+    return $this->getProperty('test', false);
+  }
+
+  public function setIsSecureAction($bool) {
+    return $this->setProperty('secure', $bool);
+  }
+
+  public function getIsSecureAction() {
+    return $this->getProperty('secure', false);
+  }
+
  public function queueCall() {
    PhabricatorWorker::scheduleTask(
      'HeraldWebhookWorker',
--- a/src/applications/herald/view/HeraldWebhookRequestListView.php
+++ b/src/applications/herald/view/HeraldWebhookRequestListView.php
@ -44,12 +44,20 @@ final class HeraldWebhookRequestListView
        $rowc[] = null;
      }

+      $last_epoch = $request->getLastRequestEpoch();
+      if ($request->getLastRequestEpoch()) {
+        $last_request = phabricator_datetime($last_epoch, $viewer);
+      } else {
+        $last_request = null;
+      }
+
      $rows[] = array(
        $request->getID(),
        $icon,
        $handles[$request->getObjectPHID()]->renderLink(),
        $request->getErrorType(),
        $request->getErrorCode(),
+        $last_request,
      );
    }

@ -62,6 +70,7 @@ final class HeraldWebhookRequestListView
          pht('Object'),
          pht('Type'),
          pht('Code'),
+          pht('Requested At'),
        ))
      ->setColumnClasses(
        array(
@ -70,6 +79,7 @@ final class HeraldWebhookRequestListView
          'wide',
          '',
          '',
+          '',
        ));

    return $table;
--- a/src/applications/herald/worker/HeraldWebhookWorker.php
+++ b/src/applications/herald/worker/HeraldWebhookWorker.php
@ -143,10 +143,15 @@ final class HeraldWebhookWorker
      'object' => array(
        'phid' => $object->getPHID(),
      ),
+      'action' => array(
+        'test' => $request->getIsTestAction(),
+        'silent' => $request->getIsSilentAction(),
+        'secure' => $request->getIsSecureAction(),
+      ),
      'transactions' => $xaction_data,
    );

-    $payload = phutil_json_encode($payload);
+    $payload = id(new PhutilJSON())->encodeFormatted($payload);
    $key = $hook->getHmacKey();
    $signature = PhabricatorHash::digestHMACSHA256($payload, $key);
    $uri = $hook->getWebhookURI();
--- a/src/applications/herald/xaction/HeraldWebhookStatusTransaction.php
+++ b/src/applications/herald/xaction/HeraldWebhookStatusTransaction.php
@ -14,20 +14,32 @@ final class HeraldWebhookStatusTransaction
  }

  public function getTitle() {
+    $old_value = $this->getOldValue();
+    $new_value = $this->getNewValue();
+
+    $old_status = HeraldWebhook::getDisplayNameForStatus($old_value);
+    $new_status = HeraldWebhook::getDisplayNameForStatus($new_value);
+
    return pht(
      '%s changed hook status from %s to %s.',
      $this->renderAuthor(),
-      $this->renderOldValue(),
-      $this->renderNewValue());
+      $this->renderValue($old_status),
+      $this->renderValue($new_status));
  }

  public function getTitleForFeed() {
+    $old_value = $this->getOldValue();
+    $new_value = $this->getNewValue();
+
+    $old_status = HeraldWebhook::getDisplayNameForStatus($old_value);
+    $new_status = HeraldWebhook::getDisplayNameForStatus($new_value);
+
    return pht(
      '%s changed %s from %s to %s.',
      $this->renderAuthor(),
      $this->renderObject(),
-      $this->renderOldValue(),
-      $this->renderNewValue());
+      $this->renderValue($old_status),
+      $this->renderValue($new_status));
  }

  public function validateTransactions($object, array $xactions) {