Use transactions to apply web UI SSH key edits

Summary:
Ref T10917. Converts web UI edits to transactions.

This is about 95% "the right way", and then I cheated on the last 5% instead of building a real EditEngine. We don't need it for anything else right now and some of the dialog workflows here are a little weird so I'm just planning to skip it for the moment unless it ends up being easier to do after the next phase (mail notifications) or something like that.

Test Plan: {F1652160}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10917

Differential Revision: https://secure.phabricator.com/D15947

resources/sql/autopatches/20160519.ssh.01.xaction.sql

@@ -0,0 +1,19 @@
+CREATE TABLE {$NAMESPACE}_auth.auth_sshkeytransaction (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  phid VARBINARY(64) NOT NULL,
+  authorPHID VARBINARY(64) NOT NULL,
+  objectPHID VARBINARY(64) NOT NULL,
+  viewPolicy VARBINARY(64) NOT NULL,
+  editPolicy VARBINARY(64) NOT NULL,
+  commentPHID VARBINARY(64) DEFAULT NULL,
+  commentVersion INT UNSIGNED NOT NULL,
+  transactionType VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL,
+  oldValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL,
+  newValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL,
+  contentSource LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL,
+  metadata LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL,
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_phid` (`phid`),
+  KEY `key_object` (`objectPHID`)
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};

src/__phutil_library_map__.php

@@ -1878,12 +1878,15 @@ phutil_register_library_map(array(
     'PhabricatorAuthSSHKeyController' => 'applications/auth/controller/PhabricatorAuthSSHKeyController.php',
     'PhabricatorAuthSSHKeyDeactivateController' => 'applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php',
     'PhabricatorAuthSSHKeyEditController' => 'applications/auth/controller/PhabricatorAuthSSHKeyEditController.php',
+    'PhabricatorAuthSSHKeyEditor' => 'applications/auth/editor/PhabricatorAuthSSHKeyEditor.php',
     'PhabricatorAuthSSHKeyGenerateController' => 'applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php',
     'PhabricatorAuthSSHKeyListController' => 'applications/auth/controller/PhabricatorAuthSSHKeyListController.php',
     'PhabricatorAuthSSHKeyPHIDType' => 'applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php',
     'PhabricatorAuthSSHKeyQuery' => 'applications/auth/query/PhabricatorAuthSSHKeyQuery.php',
     'PhabricatorAuthSSHKeySearchEngine' => 'applications/auth/query/PhabricatorAuthSSHKeySearchEngine.php',
     'PhabricatorAuthSSHKeyTableView' => 'applications/auth/view/PhabricatorAuthSSHKeyTableView.php',
+    'PhabricatorAuthSSHKeyTransaction' => 'applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php',
+    'PhabricatorAuthSSHKeyTransactionQuery' => 'applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php',
     'PhabricatorAuthSSHKeyViewController' => 'applications/auth/controller/PhabricatorAuthSSHKeyViewController.php',
     'PhabricatorAuthSSHPublicKey' => 'applications/auth/sshkey/PhabricatorAuthSSHPublicKey.php',
     'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php',
@@ -6305,16 +6308,20 @@ phutil_register_library_map(array(
       'PhabricatorAuthDAO',
       'PhabricatorPolicyInterface',
       'PhabricatorDestructibleInterface',
+      'PhabricatorApplicationTransactionInterface',
     ),
     'PhabricatorAuthSSHKeyController' => 'PhabricatorAuthController',
     'PhabricatorAuthSSHKeyDeactivateController' => 'PhabricatorAuthSSHKeyController',
     'PhabricatorAuthSSHKeyEditController' => 'PhabricatorAuthSSHKeyController',
+    'PhabricatorAuthSSHKeyEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorAuthSSHKeyGenerateController' => 'PhabricatorAuthSSHKeyController',
     'PhabricatorAuthSSHKeyListController' => 'PhabricatorAuthSSHKeyController',
     'PhabricatorAuthSSHKeyPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthSSHKeyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorAuthSSHKeySearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorAuthSSHKeyTableView' => 'AphrontView',
+    'PhabricatorAuthSSHKeyTransaction' => 'PhabricatorApplicationTransaction',
+    'PhabricatorAuthSSHKeyTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorAuthSSHKeyViewController' => 'PhabricatorAuthSSHKeyController',
     'PhabricatorAuthSSHPublicKey' => 'Phobject',
     'PhabricatorAuthSession' => array(

src/applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php

@@ -27,10 +27,18 @@ final class PhabricatorAuthSSHKeyDeactivateController
       $cancel_uri);
 
     if ($request->isFormPost()) {
+      $xactions = array();
 
-      // TODO: Convert to transactions.
+      $xactions[] = id(new PhabricatorAuthSSHKeyTransaction())
-      $key->setIsActive(null);
+        ->setTransactionType(PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE)
-      $key->save();
+        ->setNewValue(true);
+
+      id(new PhabricatorAuthSSHKeyEditor())
+        ->setActor($viewer)
+        ->setContentSourceFromRequest($request)
+        ->setContinueOnNoEffect(true)
+        ->setContinueOnMissingFields(true)
+        ->applyTransactions($key, $xactions);
 
       return id(new AphrontRedirectResponse())->setURI($cancel_uri);
     }

src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php

@@ -59,51 +59,45 @@ final class PhabricatorAuthSSHKeyEditController
     $v_key = $key->getEntireKey();
     $e_key = strlen($v_key) ? null : true;
 
-    $errors = array();
+    $validation_exception = null;
     if ($request->isFormPost()) {
+      $type_create = PhabricatorTransactions::TYPE_CREATE;
+      $type_name = PhabricatorAuthSSHKeyTransaction::TYPE_NAME;
+      $type_key = PhabricatorAuthSSHKeyTransaction::TYPE_KEY;
+
+      $e_name = null;
+      $e_key = null;
+
       $v_name = $request->getStr('name');
       $v_key = $request->getStr('key');
 
-      if (!strlen($v_name)) {
+      $xactions = array();
-        $errors[] = pht('You must provide a name for this public key.');
+
-        $e_name = pht('Required');
+      if (!$key->getID()) {
-      } else {
+        $xactions[] = id(new PhabricatorAuthSSHKeyTransaction())
-        $key->setName($v_name);
+          ->setTransactionType(PhabricatorTransactions::TYPE_CREATE);
       }
 
-      if (!strlen($v_key)) {
+      $xactions[] = id(new PhabricatorAuthSSHKeyTransaction())
-        $errors[] = pht('You must provide a public key.');
+        ->setTransactionType($type_name)
-        $e_key = pht('Required');
+        ->setNewValue($v_name);
-      } else {
-        try {
-          $public_key = PhabricatorAuthSSHPublicKey::newFromRawKey($v_key);
 
-          $type = $public_key->getType();
+      $xactions[] = id(new PhabricatorAuthSSHKeyTransaction())
-          $body = $public_key->getBody();
+        ->setTransactionType($type_key)
-          $comment = $public_key->getComment();
+        ->setNewValue($v_key);
 
-          $key->setKeyType($type);
+      $editor = id(new PhabricatorAuthSSHKeyEditor())
-          $key->setKeyBody($body);
+        ->setActor($viewer)
-          $key->setKeyComment($comment);
+        ->setContentSourceFromRequest($request)
+        ->setContinueOnNoEffect(true);
 
-          $e_key = null;
+      try {
-        } catch (Exception $ex) {
+        $editor->applyTransactions($key, $xactions);
-          $e_key = pht('Invalid');
+        return id(new AphrontRedirectResponse())->setURI($key->getURI());
-          $errors[] = $ex->getMessage();
+      } catch (PhabricatorApplicationTransactionValidationException $ex) {
-        }
+        $validation_exception = $ex;
-      }
+        $e_name = $ex->getShortMessage($type_name);
-
+        $e_key = $ex->getShortMessage($type_key);
-      if (!$errors) {
-        try {
-          $key->save();
-          return id(new AphrontRedirectResponse())->setURI($key->getURI());
-        } catch (Exception $ex) {
-          $e_key = pht('Duplicate');
-          $errors[] = pht(
-            'This public key is already associated with another user or '.
-            'device. Each key must unambiguously identify a single unique '.
-            'owner.');
-        }
       }
     }
 
@@ -134,7 +128,7 @@ final class PhabricatorAuthSSHKeyEditController
     return $this->newDialog()
       ->setTitle($title)
       ->setWidth(AphrontDialogView::WIDTH_FORM)
-      ->setErrors($errors)
+      ->setValidationException($validation_exception)
       ->appendForm($form)
       ->addSubmitButton($save_button)
       ->addCancelButton($cancel_uri);

src/applications/auth/controller/PhabricatorAuthSSHKeyViewController.php

@@ -49,12 +49,10 @@ final class PhabricatorAuthSSHKeyViewController
     $crumbs->addTextCrumb($title);
     $crumbs->setBorder(true);
 
-    // TODO: This doesn't exist yet, build it.
+    $timeline = $this->buildTransactionTimeline(
-    // $timeline = $this->buildTransactionTimeline(
+      $ssh_key,
-    //   $ssh_key,
+      new PhabricatorAuthSSHKeyTransactionQuery());
-    //   new PhabricatorAuthSSHKeyTransactionQuery());
+    $timeline->setShouldTerminate(true);
-    // $timeline->setShouldTerminate(true);
-    $timeline = null;
 
     $view = id(new PHUITwoColumnView())
       ->setHeader($header)
@@ -113,6 +111,9 @@ final class PhabricatorAuthSSHKeyViewController
       ->setUser($viewer);
 
     $properties->addProperty(pht('SSH Key Type'), $ssh_key->getKeyType());
+    $properties->addProperty(
+      pht('Created'),
+      phabricator_datetime($ssh_key->getDateCreated(), $viewer));
 
     return id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Details'))

src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php

@@ -0,0 +1,180 @@
+<?php
+
+final class PhabricatorAuthSSHKeyEditor
+  extends PhabricatorApplicationTransactionEditor {
+
+  public function getEditorApplicationClass() {
+    return 'PhabricatorAuthApplication';
+  }
+
+  public function getEditorObjectsDescription() {
+    return pht('SSH Keys');
+  }
+
+  public function getTransactionTypes() {
+    $types = parent::getTransactionTypes();
+
+    $types[] = PhabricatorAuthSSHKeyTransaction::TYPE_NAME;
+    $types[] = PhabricatorAuthSSHKeyTransaction::TYPE_KEY;
+    $types[] = PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE;
+
+    return $types;
+  }
+
+  protected function getCustomTransactionOldValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuthSSHKeyTransaction::TYPE_NAME:
+        return $object->getName();
+      case PhabricatorAuthSSHKeyTransaction::TYPE_KEY:
+        return $object->getEntireKey();
+      case PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE:
+        return !$object->getIsActive();
+    }
+
+  }
+
+  protected function getCustomTransactionNewValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuthSSHKeyTransaction::TYPE_NAME:
+      case PhabricatorAuthSSHKeyTransaction::TYPE_KEY:
+        return $xaction->getNewValue();
+      case PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE:
+        return (bool)$xaction->getNewValue();
+    }
+  }
+
+  protected function applyCustomInternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    $value = $xaction->getNewValue();
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuthSSHKeyTransaction::TYPE_NAME:
+        $object->setName($value);
+        return;
+      case PhabricatorAuthSSHKeyTransaction::TYPE_KEY:
+        $public_key = PhabricatorAuthSSHPublicKey::newFromRawKey($value);
+
+        $type = $public_key->getType();
+        $body = $public_key->getBody();
+        $comment = $public_key->getComment();
+
+        $object->setKeyType($type);
+        $object->setKeyBody($body);
+        $object->setKeyComment($comment);
+        return;
+      case PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE:
+        if ($value) {
+          $new = null;
+        } else {
+          $new = 1;
+        }
+
+        $object->setIsActive($new);
+        return;
+    }
+  }
+
+  protected function applyCustomExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+    return;
+  }
+
+  protected function validateTransaction(
+    PhabricatorLiskDAO $object,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateTransaction($object, $type, $xactions);
+
+    switch ($type) {
+      case PhabricatorAuthSSHKeyTransaction::TYPE_NAME:
+        $missing = $this->validateIsEmptyTextField(
+          $object->getName(),
+          $xactions);
+
+        if ($missing) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            pht('SSH key name is required.'),
+            nonempty(last($xactions), null));
+
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+        }
+        break;
+
+      case PhabricatorAuthSSHKeyTransaction::TYPE_KEY;
+        $missing = $this->validateIsEmptyTextField(
+          $object->getName(),
+          $xactions);
+
+        if ($missing) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            pht('SSH key material is required.'),
+            nonempty(last($xactions), null));
+
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+        } else {
+          foreach ($xactions as $xaction) {
+            $new = $xaction->getNewValue();
+
+            try {
+              $public_key = PhabricatorAuthSSHPublicKey::newFromRawKey($new);
+            } catch (Exception $ex) {
+              $errors[] = new PhabricatorApplicationTransactionValidationError(
+                $type,
+                pht('Invalid'),
+                $ex->getMessage(),
+                $xaction);
+            }
+          }
+        }
+        break;
+
+      case PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE:
+        foreach ($xactions as $xaction) {
+          if (!$xaction->getNewValue()) {
+            $errors[] = new PhabricatorApplicationTransactionValidationError(
+              $type,
+              pht('Invalid'),
+              pht('SSH keys can not be reactivated.'),
+              $xaction);
+          }
+        }
+        break;
+    }
+
+    return $errors;
+  }
+
+  protected function didCatchDuplicateKeyException(
+    PhabricatorLiskDAO $object,
+    array $xactions,
+    Exception $ex) {
+
+    $errors = array();
+    $errors[] = new PhabricatorApplicationTransactionValidationError(
+      PhabricatorAuthSSHKeyTransaction::TYPE_KEY,
+      pht('Duplicate'),
+      pht(
+        'This public key is already associated with another user or device. '.
+        'Each key must unambiguously identify a single unique owner.'),
+      null);
+
+    throw new PhabricatorApplicationTransactionValidationException($errors);
+  }
+
+
+}

src/applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php

@@ -0,0 +1,10 @@
+<?php
+
+final class PhabricatorAuthSSHKeyTransactionQuery
+  extends PhabricatorApplicationTransactionQuery {
+
+  public function getTemplateApplicationTransaction() {
+    return new PhabricatorAuthSSHKeyTransaction();
+  }
+
+}

src/applications/auth/storage/PhabricatorAuthSSHKey.php

@@ -4,7 +4,8 @@ final class PhabricatorAuthSSHKey
   extends PhabricatorAuthDAO
   implements
     PhabricatorPolicyInterface,
-    PhabricatorDestructibleInterface {
+    PhabricatorDestructibleInterface,
+    PhabricatorApplicationTransactionInterface {
 
   protected $objectPHID;
   protected $name;
@@ -150,4 +151,26 @@ final class PhabricatorAuthSSHKey
     $this->saveTransaction();
   }
 
+
+/* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
+
+
+  public function getApplicationTransactionEditor() {
+    return new PhabricatorAuthSSHKeyEditor();
+  }
+
+  public function getApplicationTransactionObject() {
+    return $this;
+  }
+
+  public function getApplicationTransactionTemplate() {
+    return new PhabricatorAuthProviderConfigTransaction();
+  }
+
+  public function willRenderTimeline(
+    PhabricatorApplicationTransactionView $timeline,
+    AphrontRequest $request) {
+    return $timeline;
+  }
+
 }

src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php

@@ -0,0 +1,55 @@
+<?php
+
+final class PhabricatorAuthSSHKeyTransaction
+  extends PhabricatorApplicationTransaction {
+
+  const TYPE_NAME = 'sshkey.name';
+  const TYPE_KEY = 'sshkey.key';
+  const TYPE_DEACTIVATE = 'sshkey.deactivate';
+
+  public function getApplicationName() {
+    return 'auth';
+  }
+
+  public function getApplicationTransactionType() {
+    return PhabricatorAuthSSHKeyPHIDType::TYPECONST;
+  }
+
+  public function getApplicationTransactionCommentObject() {
+    return null;
+  }
+
+  public function getTitle() {
+    $author_phid = $this->getAuthorPHID();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_NAME:
+        return pht(
+          '%s renamed this key from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $old,
+          $new);
+      case self::TYPE_KEY:
+        return pht(
+          '%s updated the public key material for this SSH key.',
+          $this->renderHandleLink($author_phid));
+      case self::TYPE_DEACTIVATE:
+        if ($new) {
+          return pht(
+            '%s deactivated this key.',
+            $this->renderHandleLink($author_phid));
+        } else {
+          return pht(
+            '%s activated this key.',
+            $this->renderHandleLink($author_phid));
+        }
+
+    }
+
+    return parent::getTitle();
+  }
+
+}