1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-09 16:32:39 +01:00

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
This commit is contained in:
epriestley 2018-01-22 15:54:47 -08:00
parent bf2868c070
commit 2914613444
8 changed files with 26 additions and 20 deletions

View file

@ -245,7 +245,7 @@ try {
} }
$workflow = $parsed_args->parseWorkflows($workflows); $workflow = $parsed_args->parseWorkflows($workflows);
$workflow->setUser($user); $workflow->setSSHUser($user);
$workflow->setOriginalArguments($original_argv); $workflow->setOriginalArguments($original_argv);
$workflow->setIsClusterRequest($is_cluster_request); $workflow->setIsClusterRequest($is_cluster_request);

View file

@ -9620,7 +9620,7 @@ phutil_register_library_map(array(
'PhabricatorSSHKeysSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorSSHKeysSettingsPanel' => 'PhabricatorSettingsPanel',
'PhabricatorSSHLog' => 'Phobject', 'PhabricatorSSHLog' => 'Phobject',
'PhabricatorSSHPassthruCommand' => 'Phobject', 'PhabricatorSSHPassthruCommand' => 'Phobject',
'PhabricatorSSHWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorSSHWorkflow' => 'PhutilArgumentWorkflow',
'PhabricatorSavedQuery' => array( 'PhabricatorSavedQuery' => array(
'PhabricatorSearchDAO', 'PhabricatorSearchDAO',
'PhabricatorPolicyInterface', 'PhabricatorPolicyInterface',

View file

@ -46,7 +46,7 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow {
try { try {
$call = new ConduitCall($method, $params); $call = new ConduitCall($method, $params);
$call->setUser($this->getUser()); $call->setUser($this->getSSHUser());
$result = $call->execute(); $result = $call->execute();
} catch (ConduitException $ex) { } catch (ConduitException $ex) {
@ -77,7 +77,7 @@ final class ConduitSSHWorkflow extends PhabricatorSSHWorkflow {
$connection_id = idx($metadata, 'connectionID'); $connection_id = idx($metadata, 'connectionID');
$log = id(new PhabricatorConduitMethodCallLog()) $log = id(new PhabricatorConduitMethodCallLog())
->setCallerPHID($this->getUser()->getPHID()) ->setCallerPHID($this->getSSHUser()->getPHID())
->setConnectionID($connection_id) ->setConnectionID($connection_id)
->setMethod($method) ->setMethod($method)
->setError((string)$error_code) ->setError((string)$error_code)

View file

@ -15,7 +15,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow {
protected function executeRepositoryOperations() { protected function executeRepositoryOperations() {
$repository = $this->getRepository(); $repository = $this->getRepository();
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
$device = AlmanacKeys::getLiveDevice(); $device = AlmanacKeys::getLiveDevice();
// This is a write, and must have write access. // This is a write, and must have write access.

View file

@ -15,7 +15,7 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow {
protected function executeRepositoryOperations() { protected function executeRepositoryOperations() {
$repository = $this->getRepository(); $repository = $this->getRepository();
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
$device = AlmanacKeys::getLiveDevice(); $device = AlmanacKeys::getLiveDevice();
$skip_sync = $this->shouldSkipReadSynchronization(); $skip_sync = $this->shouldSkipReadSynchronization();

View file

@ -26,7 +26,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
public function getEnvironment() { public function getEnvironment() {
$env = array( $env = array(
DiffusionCommitHookEngine::ENV_USER => $this->getUser()->getUsername(), DiffusionCommitHookEngine::ENV_USER => $this->getSSHUser()->getUsername(),
DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh', DiffusionCommitHookEngine::ENV_REMOTE_PROTOCOL => 'ssh',
); );
@ -122,14 +122,14 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
$key_path, $key_path,
$port, $port,
$host, $host,
'@'.$this->getUser()->getUsername(), '@'.$this->getSSHUser()->getUsername(),
$this->getOriginalArguments()); $this->getOriginalArguments());
} }
final public function execute(PhutilArgumentParser $args) { final public function execute(PhutilArgumentParser $args) {
$this->args = $args; $this->args = $args;
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
$have_diffusion = PhabricatorApplication::isClassInstalledForViewer( $have_diffusion = PhabricatorApplication::isClassInstalledForViewer(
'PhabricatorDiffusionApplication', 'PhabricatorDiffusionApplication',
$viewer); $viewer);
@ -164,7 +164,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
} }
protected function loadRepositoryWithPath($path, $vcs) { protected function loadRepositoryWithPath($path, $vcs) {
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
$info = PhabricatorRepository::parseRepositoryServicePath($path, $vcs); $info = PhabricatorRepository::parseRepositoryServicePath($path, $vcs);
if ($info === null) { if ($info === null) {
@ -214,7 +214,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
} }
$repository = $this->getRepository(); $repository = $this->getRepository();
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
if ($viewer->isOmnipotent()) { if ($viewer->isOmnipotent()) {
throw new Exception( throw new Exception(
@ -252,7 +252,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
} }
protected function shouldSkipReadSynchronization() { protected function shouldSkipReadSynchronization() {
$viewer = $this->getUser(); $viewer = $this->getSSHUser();
// Currently, the only case where devices interact over SSH without // Currently, the only case where devices interact over SSH without
// assuming user credentials is when synchronizing before a read. These // assuming user credentials is when synchronizing before a read. These
@ -265,7 +265,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow {
} }
protected function newPullEvent() { protected function newPullEvent() {
$viewer = $this->getViewer(); $viewer = $this->getSSHUser();
$repository = $this->getRepository(); $repository = $this->getRepository();
$remote_address = $this->getSSHRemoteAddress(); $remote_address = $this->getSSHRemoteAddress();

View file

@ -154,7 +154,7 @@ final class DiffusionSubversionServeSSHWorkflow
} else { } else {
$command = csprintf( $command = csprintf(
'svnserve -t --tunnel-user=%s', 'svnserve -t --tunnel-user=%s',
$this->getUser()->getUsername()); $this->getSSHUser()->getUsername());
$cwd = PhabricatorEnv::getEmptyCWD(); $cwd = PhabricatorEnv::getEmptyCWD();
} }

View file

@ -1,8 +1,14 @@
<?php <?php
abstract class PhabricatorSSHWorkflow extends PhabricatorManagementWorkflow { abstract class PhabricatorSSHWorkflow
extends PhutilArgumentWorkflow {
private $user; // NOTE: We are explicitly extending "PhutilArgumentWorkflow", not
// "PhabricatorManagementWorkflow". We want to avoid inheriting "getViewer()"
// and other methods which assume workflows are administrative commands
// like `bin/storage`.
private $sshUser;
private $iochannel; private $iochannel;
private $errorChannel; private $errorChannel;
private $isClusterRequest; private $isClusterRequest;
@ -21,13 +27,13 @@ abstract class PhabricatorSSHWorkflow extends PhabricatorManagementWorkflow {
return $this->errorChannel; return $this->errorChannel;
} }
public function setUser(PhabricatorUser $user) { public function setSSHUser(PhabricatorUser $ssh_user) {
$this->user = $user; $this->sshUser = $ssh_user;
return $this; return $this;
} }
public function getUser() { public function getSSHUser() {
return $this->user; return $this->sshUser;
} }
public function setIOChannel(PhutilChannel $channel) { public function setIOChannel(PhutilChannel $channel) {