From 8eec9e2c0ef22cccc21f1d94940c88fbc3fb50b3 Mon Sep 17 00:00:00 2001 From: Nick Zheng Date: Sat, 19 Dec 2015 11:48:24 -0800 Subject: [PATCH] Provide a more straightforward way to revoke SSH keys by finding and destroying the objects Summary: Ref T9967 Test Plan: Ran migrations. Verified database populated properly with PHIDs (SELECT * FROM auth_sshkey;). Ran auth.querypublickeys conduit method to see phids show up Ran bin/remove destroy . Viewed the test key was gone. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Maniphest Tasks: T9967 Differential Revision: https://secure.phabricator.com/D14823 --- .../autopatches/20151218.key.1.keyphid.sql | 2 + .../autopatches/20151218.key.2.keyphid.php | 17 +++++++ src/__phutil_library_map__.php | 3 ++ ...torAuthQueryPublicKeysConduitAPIMethod.php | 7 +++ .../phid/PhabricatorAuthSSHKeyPHIDType.php | 38 ++++++++++++++++ .../auth/query/PhabricatorAuthSSHKeyQuery.php | 44 +++++++++++-------- .../auth/storage/PhabricatorAuthSSHKey.php | 22 ++++++++-- 7 files changed, 111 insertions(+), 22 deletions(-) create mode 100644 resources/sql/autopatches/20151218.key.1.keyphid.sql create mode 100644 resources/sql/autopatches/20151218.key.2.keyphid.php create mode 100644 src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php diff --git a/resources/sql/autopatches/20151218.key.1.keyphid.sql b/resources/sql/autopatches/20151218.key.1.keyphid.sql new file mode 100644 index 0000000000..568572c1ba --- /dev/null +++ b/resources/sql/autopatches/20151218.key.1.keyphid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_sshkey + ADD phid VARBINARY(64) NOT NULL AFTER id; diff --git a/resources/sql/autopatches/20151218.key.2.keyphid.php b/resources/sql/autopatches/20151218.key.2.keyphid.php new file mode 100644 index 0000000000..eb732742ca --- /dev/null +++ b/resources/sql/autopatches/20151218.key.2.keyphid.php @@ -0,0 +1,17 @@ +establishConnection('w'); + +foreach (new LiskMigrationIterator($table) as $cursor) { + if (strlen($cursor->getPHID())) { + continue; + } + + queryfx( + $conn_w, + 'UPDATE %T SET phid = %s WHERE id = %d', + $table->getTableName(), + $table->generatePHID(), + $cursor->getID()); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index dadc3b74ac..40f83cb066 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1684,6 +1684,7 @@ phutil_register_library_map(array( 'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php', 'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php', 'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php', + 'PhabricatorAuthSSHKeyPHIDType' => 'applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php', 'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php', 'PhabricatorAuthConfirmLinkController' => 'applications/auth/controller/PhabricatorAuthConfirmLinkController.php', 'PhabricatorAuthController' => 'applications/auth/controller/PhabricatorAuthController.php', @@ -5833,6 +5834,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSSHKey' => array( 'PhabricatorAuthDAO', 'PhabricatorPolicyInterface', + 'PhabricatorDestructibleInterface', ), 'PhabricatorAuthSSHKeyController' => 'PhabricatorAuthController', 'PhabricatorAuthSSHKeyDeleteController' => 'PhabricatorAuthSSHKeyController', @@ -5840,6 +5842,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSSHKeyGenerateController' => 'PhabricatorAuthSSHKeyController', 'PhabricatorAuthSSHKeyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthSSHKeyTableView' => 'AphrontView', + 'PhabricatorAuthSSHKeyPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthSSHPublicKey' => 'Phobject', 'PhabricatorAuthSession' => array( 'PhabricatorAuthDAO', diff --git a/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php b/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php index e7d036aee4..be91af7863 100644 --- a/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php +++ b/src/applications/auth/conduit/PhabricatorAuthQueryPublicKeysConduitAPIMethod.php @@ -14,6 +14,7 @@ final class PhabricatorAuthQueryPublicKeysConduitAPIMethod protected function defineParamTypes() { return array( 'ids' => 'optional list', + 'phids' => 'optional list', 'objectPHIDs' => 'optional list', 'keys' => 'optional list', ) + self::getPagerParamTypes(); @@ -34,6 +35,11 @@ final class PhabricatorAuthQueryPublicKeysConduitAPIMethod $query->withIDs($ids); } + $phids = $request->getValue('phids'); + if ($phids !== null) { + $query->withPHIDs($phids); + } + $object_phids = $request->getValue('objectPHIDs'); if ($object_phids !== null) { $query->withObjectPHIDs($object_phids); @@ -57,6 +63,7 @@ final class PhabricatorAuthQueryPublicKeysConduitAPIMethod $data[] = array( 'id' => $public_key->getID(), 'name' => $public_key->getName(), + 'phid' => $public_key->getPHID(), 'objectPHID' => $public_key->getObjectPHID(), 'isTrusted' => (bool)$public_key->getIsTrusted(), 'key' => $public_key->getEntireKey(), diff --git a/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php b/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php new file mode 100644 index 0000000000..10ab2fdfb6 --- /dev/null +++ b/src/applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php @@ -0,0 +1,38 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + foreach ($handles as $phid => $handle) { + $key = $objects[$phid]; + $handle->setName(pht('SSH Key %d', $key->getID())); + } + } + +} diff --git a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php index 4a14456f35..f68969d0e8 100644 --- a/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php +++ b/src/applications/auth/query/PhabricatorAuthSSHKeyQuery.php @@ -4,6 +4,7 @@ final class PhabricatorAuthSSHKeyQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $ids; + private $phids; private $objectPHIDs; private $keys; @@ -12,6 +13,11 @@ final class PhabricatorAuthSSHKeyQuery return $this; } + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + public function withObjectPHIDs(array $object_phids) { $this->objectPHIDs = $object_phids; return $this; @@ -23,19 +29,12 @@ final class PhabricatorAuthSSHKeyQuery return $this; } + public function newResultObject() { + return new PhabricatorAuthSSHKey(); + } + protected function loadPage() { - $table = new PhabricatorAuthSSHKey(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $keys) { @@ -54,6 +53,7 @@ final class PhabricatorAuthSSHKeyQuery // We must have an object, and that object must be a valid object for // SSH keys. if (!$object || !($object instanceof PhabricatorSSHPublicKeyInterface)) { + $this->didRejectResult($ssh_key); unset($keys[$key]); continue; } @@ -64,19 +64,26 @@ final class PhabricatorAuthSSHKeyQuery return $keys; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + if ($this->objectPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'objectPHID IN (%Ls)', $this->objectPHIDs); } @@ -85,7 +92,7 @@ final class PhabricatorAuthSSHKeyQuery $sql = array(); foreach ($this->keys as $key) { $sql[] = qsprintf( - $conn_r, + $conn, '(keyType = %s AND keyIndex = %s)', $key->getType(), $key->getHash()); @@ -93,9 +100,8 @@ final class PhabricatorAuthSSHKeyQuery $where[] = implode(' OR ', $sql); } - $where[] = $this->buildPagingClause($conn_r); + return $where; - return $this->formatWhereClause($where); } public function getQueryApplicationClass() { diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index c87c9b50b6..3e77c1bca1 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKey.php @@ -2,7 +2,9 @@ final class PhabricatorAuthSSHKey extends PhabricatorAuthDAO - implements PhabricatorPolicyInterface { + implements + PhabricatorPolicyInterface, + PhabricatorDestructibleInterface { protected $objectPHID; protected $name; @@ -16,6 +18,7 @@ final class PhabricatorAuthSSHKey protected function getConfiguration() { return array( + self::CONFIG_AUX_PHID => true, self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'text255', 'keyType' => 'text255', @@ -63,8 +66,10 @@ final class PhabricatorAuthSSHKey return $this; } - - + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + PhabricatorAuthSSHKeyPHIDType::TYPECONST); + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -89,4 +94,15 @@ final class PhabricatorAuthSSHKey 'SSH keys inherit the policies of the user or object they authenticate.'); } +/* -( PhabricatorDestructibleInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + + $this->openTransaction(); + $this->delete(); + $this->saveTransaction(); + } + }