From da6b3de65c142bda62b5bc425c6cc8f08c5094ea Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 May 2016 09:54:36 -0700 Subject: [PATCH] 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 --- .../autopatches/20160519.ssh.01.xaction.sql | 19 ++ src/__phutil_library_map__.php | 7 + ...bricatorAuthSSHKeyDeactivateController.php | 14 +- .../PhabricatorAuthSSHKeyEditController.php | 68 +++---- .../PhabricatorAuthSSHKeyViewController.php | 13 +- .../editor/PhabricatorAuthSSHKeyEditor.php | 180 ++++++++++++++++++ .../PhabricatorAuthSSHKeyTransactionQuery.php | 10 + .../auth/storage/PhabricatorAuthSSHKey.php | 25 ++- .../PhabricatorAuthSSHKeyTransaction.php | 55 ++++++ 9 files changed, 344 insertions(+), 47 deletions(-) create mode 100644 resources/sql/autopatches/20160519.ssh.01.xaction.sql create mode 100644 src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php create mode 100644 src/applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php create mode 100644 src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php diff --git a/resources/sql/autopatches/20160519.ssh.01.xaction.sql b/resources/sql/autopatches/20160519.ssh.01.xaction.sql new file mode 100644 index 0000000000..8b6ddc62cd --- /dev/null +++ b/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}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f216b32c54..7e5d1e098a 100644 --- a/src/__phutil_library_map__.php +++ b/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( diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php index 50c1f89a3d..8eca02340d 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyDeactivateController.php @@ -27,10 +27,18 @@ final class PhabricatorAuthSSHKeyDeactivateController $cancel_uri); if ($request->isFormPost()) { + $xactions = array(); - // TODO: Convert to transactions. - $key->setIsActive(null); - $key->save(); + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType(PhabricatorAuthSSHKeyTransaction::TYPE_DEACTIVATE) + ->setNewValue(true); + + id(new PhabricatorAuthSSHKeyEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($key, $xactions); return id(new AphrontRedirectResponse())->setURI($cancel_uri); } diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php index 541ed28531..1023b0cd75 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyEditController.php +++ b/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)) { - $errors[] = pht('You must provide a name for this public key.'); - $e_name = pht('Required'); - } else { - $key->setName($v_name); + $xactions = array(); + + if (!$key->getID()) { + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_CREATE); } - if (!strlen($v_key)) { - $errors[] = pht('You must provide a public key.'); - $e_key = pht('Required'); - } else { - try { - $public_key = PhabricatorAuthSSHPublicKey::newFromRawKey($v_key); + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType($type_name) + ->setNewValue($v_name); - $type = $public_key->getType(); - $body = $public_key->getBody(); - $comment = $public_key->getComment(); + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType($type_key) + ->setNewValue($v_key); - $key->setKeyType($type); - $key->setKeyBody($body); - $key->setKeyComment($comment); + $editor = id(new PhabricatorAuthSSHKeyEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true); - $e_key = null; - } catch (Exception $ex) { - $e_key = pht('Invalid'); - $errors[] = $ex->getMessage(); - } - } - - 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.'); - } + try { + $editor->applyTransactions($key, $xactions); + return id(new AphrontRedirectResponse())->setURI($key->getURI()); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + $e_name = $ex->getShortMessage($type_name); + $e_key = $ex->getShortMessage($type_key); } } @@ -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); diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyViewController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyViewController.php index cdca15cd22..66c3a5f09b 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyViewController.php +++ b/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( - // $ssh_key, - // new PhabricatorAuthSSHKeyTransactionQuery()); - // $timeline->setShouldTerminate(true); - $timeline = null; + $timeline = $this->buildTransactionTimeline( + $ssh_key, + new PhabricatorAuthSSHKeyTransactionQuery()); + $timeline->setShouldTerminate(true); $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')) diff --git a/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php new file mode 100644 index 0000000000..7d19bee498 --- /dev/null +++ b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php @@ -0,0 +1,180 @@ +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); + } + + +} diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php new file mode 100644 index 0000000000..397a03f2b0 --- /dev/null +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyTransactionQuery.php @@ -0,0 +1,10 @@ +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; + } + } diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php new file mode 100644 index 0000000000..3cf0eac799 --- /dev/null +++ b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php @@ -0,0 +1,55 @@ +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(); + } + +}