From 6f6ca0102daa2ac39c19a2c9b6867c921e1021ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 May 2016 10:34:38 -0700 Subject: [PATCH] Send forced mail on SSH key edits Summary: Ref T10917. This cheats fairly heavily to generate SSH key mail: - Generate normal transaction mail. - Force it to go to the user. - Use `setForceDelivery()` to force it to actually be delivered. - Add some warning language to the mail body. This doesn't move us much closer to Glorious Infrastructure for this whole class of events, but should do what it needs to for now and doesn't really require anything sketchy. Test Plan: Created and edited SSH keys, got security notice mail. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10917 Differential Revision: https://secure.phabricator.com/D15948 --- src/__phutil_library_map__.php | 2 + .../almanac/storage/AlmanacDevice.php | 8 +++ ...habricatorAuthSSHKeyGenerateController.php | 30 +++++++-- .../editor/PhabricatorAuthSSHKeyEditor.php | 64 +++++++++++++++++++ .../PhabricatorAuthSSHKeyReplyHandler.php | 17 +++++ .../PhabricatorSSHPublicKeyInterface.php | 2 + .../auth/storage/PhabricatorAuthSSHKey.php | 8 ++- .../PhabricatorAuthSSHKeyTransaction.php | 4 ++ .../people/storage/PhabricatorUser.php | 6 ++ 9 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 src/applications/auth/mail/PhabricatorAuthSSHKeyReplyHandler.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7e5d1e098a..c827314248 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1883,6 +1883,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSSHKeyListController' => 'applications/auth/controller/PhabricatorAuthSSHKeyListController.php', 'PhabricatorAuthSSHKeyPHIDType' => 'applications/auth/phid/PhabricatorAuthSSHKeyPHIDType.php', 'PhabricatorAuthSSHKeyQuery' => 'applications/auth/query/PhabricatorAuthSSHKeyQuery.php', + 'PhabricatorAuthSSHKeyReplyHandler' => 'applications/auth/mail/PhabricatorAuthSSHKeyReplyHandler.php', 'PhabricatorAuthSSHKeySearchEngine' => 'applications/auth/query/PhabricatorAuthSSHKeySearchEngine.php', 'PhabricatorAuthSSHKeyTableView' => 'applications/auth/view/PhabricatorAuthSSHKeyTableView.php', 'PhabricatorAuthSSHKeyTransaction' => 'applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php', @@ -6318,6 +6319,7 @@ phutil_register_library_map(array( 'PhabricatorAuthSSHKeyListController' => 'PhabricatorAuthSSHKeyController', 'PhabricatorAuthSSHKeyPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthSSHKeyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorAuthSSHKeyReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'PhabricatorAuthSSHKeySearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorAuthSSHKeyTableView' => 'AphrontView', 'PhabricatorAuthSSHKeyTransaction' => 'PhabricatorApplicationTransaction', diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 6c7f3cb57f..d2a3e4a763 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -227,6 +227,14 @@ final class AlmanacDevice return $this->getName(); } + public function getSSHKeyNotifyPHIDs() { + // Devices don't currently have anyone useful to notify about SSH key + // edits, and they're usually a difficult vector to attack since you need + // access to a cluster host. However, it would be nice to make them + // subscribable at some point. + return array(); + } + /* -( PhabricatorDestructibleInterface )----------------------------------- */ diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php index 2cb6dc81ea..69d93548e5 100644 --- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php +++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php @@ -36,13 +36,31 @@ final class PhabricatorAuthSSHKeyGenerateController $type = $public_key->getType(); $body = $public_key->getBody(); + $comment = pht('Generated'); - $key - ->setName($default_name) - ->setKeyType($type) - ->setKeyBody($body) - ->setKeyComment(pht('Generated')) - ->save(); + $entire_key = "{$type} {$body} {$comment}"; + + $type_create = PhabricatorTransactions::TYPE_CREATE; + $type_name = PhabricatorAuthSSHKeyTransaction::TYPE_NAME; + $type_key = PhabricatorAuthSSHKeyTransaction::TYPE_KEY; + + $xactions = array(); + + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_CREATE); + + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType($type_name) + ->setNewValue($default_name); + + $xactions[] = id(new PhabricatorAuthSSHKeyTransaction()) + ->setTransactionType($type_key) + ->setNewValue($entire_key); + + $editor = id(new PhabricatorAuthSSHKeyEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->applyTransactions($key, $xactions); // NOTE: We're disabling workflow on submit so the download works. We're // disabling workflow on cancel so the page reloads, showing the new diff --git a/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php index 7d19bee498..aaaa90e80f 100644 --- a/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php +++ b/src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php @@ -177,4 +177,68 @@ final class PhabricatorAuthSSHKeyEditor } + protected function shouldSendMail( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + + protected function getMailSubjectPrefix() { + return pht('[SSH Key]'); + } + + protected function getMailThreadID(PhabricatorLiskDAO $object) { + return 'ssh-key-'.$object->getPHID(); + } + + protected function getMailTo(PhabricatorLiskDAO $object) { + return $object->getObject()->getSSHKeyNotifyPHIDs(); + } + + protected function getMailCC(PhabricatorLiskDAO $object) { + return array(); + } + + protected function buildReplyHandler(PhabricatorLiskDAO $object) { + return id(new PhabricatorAuthSSHKeyReplyHandler()) + ->setMailReceiver($object); + } + + protected function buildMailTemplate(PhabricatorLiskDAO $object) { + $id = $object->getID(); + $name = $object->getName(); + $phid = $object->getPHID(); + + $mail = id(new PhabricatorMetaMTAMail()) + ->setSubject(pht('SSH Key %d: %s', $id, $name)) + ->addHeader('Thread-Topic', $phid); + + // The primary value of this mail is alerting users to account compromises, + // so force delivery. In particular, this mail should still be delievered + // even if "self mail" is disabled. + $mail->setForceDelivery(true); + + return $mail; + } + + protected function buildMailBody( + PhabricatorLiskDAO $object, + array $xactions) { + + $body = parent::buildMailBody($object, $xactions); + + $body->addLinkSection( + pht('SECURITY WARNING'), + pht( + 'If you do not recognize this change, it may indicate your account '. + 'has been compromised.')); + + $detail_uri = $object->getURI(); + $detail_uri = PhabricatorEnv::getProductionURI($detail_uri); + + $body->addLinkSection(pht('SSH KEY DETAIL'), $detail_uri); + + return $body; + } + } diff --git a/src/applications/auth/mail/PhabricatorAuthSSHKeyReplyHandler.php b/src/applications/auth/mail/PhabricatorAuthSSHKeyReplyHandler.php new file mode 100644 index 0000000000..e84d7a4efa --- /dev/null +++ b/src/applications/auth/mail/PhabricatorAuthSSHKeyReplyHandler.php @@ -0,0 +1,17 @@ +getPHID()); + } + public function toPublicKey() { return PhabricatorAuthSSHPublicKey::newFromStoredKey($this); } @@ -164,7 +170,7 @@ final class PhabricatorAuthSSHKey } public function getApplicationTransactionTemplate() { - return new PhabricatorAuthProviderConfigTransaction(); + return new PhabricatorAuthSSHKeyTransaction(); } public function willRenderTimeline( diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php index 3cf0eac799..37edb7384d 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKeyTransaction.php @@ -26,6 +26,10 @@ final class PhabricatorAuthSSHKeyTransaction $new = $this->getNewValue(); switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_CREATE: + return pht( + '%s created this key.', + $this->renderHandleLink($author_phid)); case self::TYPE_NAME: return pht( '%s renamed this key from "%s" to "%s".', diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index b504936825..bdb2415733 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -1342,6 +1342,12 @@ final class PhabricatorUser return 'id_rsa_phabricator'; } + public function getSSHKeyNotifyPHIDs() { + return array( + $this->getPHID(), + ); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */