From 291461344404fb3848e45468848dc33e44f164b8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Jan 2018 15:54:47 -0800 Subject: [PATCH] Fix failure to record `pullerPHID` in repository pull logs Summary: See PHI305. Ref T13046. The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer. This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user. This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`. To harden this against future mistakes: - Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code. - Rename `get/setUser()` to `get/setSSHUser()` to make them explicit. Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`. Test Plan: - Pulled and pushed to a repository over SSH. - Grepped all the SSH stuff for the altered symbols. - Saw pulls record a valid `pullerPHID` in the pull log. - Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13046 Differential Revision: https://secure.phabricator.com/D18912 --- scripts/ssh/ssh-exec.php | 2 +- src/__phutil_library_map__.php | 2 +- .../conduit/ssh/ConduitSSHWorkflow.php | 4 ++-- .../ssh/DiffusionGitReceivePackSSHWorkflow.php | 2 +- .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 2 +- .../diffusion/ssh/DiffusionSSHWorkflow.php | 14 +++++++------- .../DiffusionSubversionServeSSHWorkflow.php | 2 +- .../ssh/PhabricatorSSHWorkflow.php | 18 ++++++++++++------ 8 files changed, 26 insertions(+), 20 deletions(-) diff --git a/scripts/ssh/ssh-exec.php b/scripts/ssh/ssh-exec.php index 5748e371cf..2ff1bdc198 100755 --- a/scripts/ssh/ssh-exec.php +++ b/scripts/ssh/ssh-exec.php @@ -245,7 +245,7 @@ try { } $workflow = $parsed_args->parseWorkflows($workflows); - $workflow->setUser($user); + $workflow->setSSHUser($user); $workflow->setOriginalArguments($original_argv); $workflow->setIsClusterRequest($is_cluster_request); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4db524a379..7caf7a6dc2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -9620,7 +9620,7 @@ phutil_register_library_map(array( 'PhabricatorSSHKeysSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorSSHLog' => 'Phobject', 'PhabricatorSSHPassthruCommand' => 'Phobject', - 'PhabricatorSSHWorkflow' => 'PhabricatorManagementWorkflow', + 'PhabricatorSSHWorkflow' => 'PhutilArgumentWorkflow', 'PhabricatorSavedQuery' => array( 'PhabricatorSearchDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/conduit/ssh/ConduitSSHWorkflow.php b/src/applications/conduit/ssh/ConduitSSHWorkflow.php index 6589fac324..603a479ea0 100644 --- a/src/applications/conduit/ssh/ConduitSSHWorkflow.php +++ b/src/applications/conduit/ssh/ConduitSSHWorkflow.php @@ -46,7 +46,7 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow { try { $call = new ConduitCall($method, $params); - $call->setUser($this->getUser()); + $call->setUser($this->getSSHUser()); $result = $call->execute(); } catch (ConduitException $ex) { @@ -77,7 +77,7 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow { $connection_id = idx($metadata, 'connectionID'); $log = id(new PhabricatorConduitMethodCallLog()) - ->setCallerPHID($this->getUser()->getPHID()) + ->setCallerPHID($this->getSSHUser()->getPHID()) ->setConnectionID($connection_id) ->setMethod($method) ->setError((string)$error_code) diff --git a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php index 76f8d3c837..91c8238718 100644 --- a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php @@ -15,7 +15,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { protected function executeRepositoryOperations() { $repository = $this->getRepository(); - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); $device = AlmanacKeys::getLiveDevice(); // This is a write, and must have write access. diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 754bafd850..e0d35c32b9 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -15,7 +15,7 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { protected function executeRepositoryOperations() { $repository = $this->getRepository(); - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); $device = AlmanacKeys::getLiveDevice(); $skip_sync = $this->shouldSkipReadSynchronization(); diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index 0e5ad7bbe1..3882079f69 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -26,7 +26,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { public function getEnvironment() { $env = array( - DiffusionCommitHookEngine::ENV_USER => $this->getUser()->getUsername(), + DiffusionCommitHookEngine::ENV_USER => $this->getSSHUser()->getUsername(), DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh', ); @@ -122,14 +122,14 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { $key_path, $port, $host, - '@'.$this->getUser()->getUsername(), + '@'.$this->getSSHUser()->getUsername(), $this->getOriginalArguments()); } final public function execute(PhutilArgumentParser $args) { $this->args = $args; - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); $have_diffusion = PhabricatorApplication::isClassInstalledForViewer( 'PhabricatorDiffusionApplication', $viewer); @@ -164,7 +164,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { } protected function loadRepositoryWithPath($path, $vcs) { - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); $info = PhabricatorRepository::parseRepositoryServicePath($path, $vcs); if ($info === null) { @@ -214,7 +214,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { } $repository = $this->getRepository(); - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); if ($viewer->isOmnipotent()) { throw new Exception( @@ -252,7 +252,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { } protected function shouldSkipReadSynchronization() { - $viewer = $this->getUser(); + $viewer = $this->getSSHUser(); // Currently, the only case where devices interact over SSH without // assuming user credentials is when synchronizing before a read. These @@ -265,7 +265,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { } protected function newPullEvent() { - $viewer = $this->getViewer(); + $viewer = $this->getSSHUser(); $repository = $this->getRepository(); $remote_address = $this->getSSHRemoteAddress(); diff --git a/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php index f2a932f2bb..0df4aa17fe 100644 --- a/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php @@ -154,7 +154,7 @@ final class DiffusionSubversionServeSSHWorkflow } else { $command = csprintf( 'svnserve -t --tunnel-user=%s', - $this->getUser()->getUsername()); + $this->getSSHUser()->getUsername()); $cwd = PhabricatorEnv::getEmptyCWD(); } diff --git a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php index b6af62d35a..6b6222304c 100644 --- a/src/infrastructure/ssh/PhabricatorSSHWorkflow.php +++ b/src/infrastructure/ssh/PhabricatorSSHWorkflow.php @@ -1,8 +1,14 @@ errorChannel; } - public function setUser(PhabricatorUser $user) { - $this->user = $user; + public function setSSHUser(PhabricatorUser $ssh_user) { + $this->sshUser = $ssh_user; return $this; } - public function getUser() { - return $this->user; + public function getSSHUser() { + return $this->sshUser; } public function setIOChannel(PhutilChannel $channel) {