From 778dfff277dc0e180c568a10204f86ad617b6113 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Jan 2018 08:17:33 -0800 Subject: [PATCH] Make minor correctness and display improvements to pull logs Summary: Depends on D18915. Ref T13046. - Distinguish between HTTP and HTTPS. - Use more constants and fewer magical strings. - For HTTP responses, give them better type information and more helpful UI behaviors. Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13046 Differential Revision: https://secure.phabricator.com/D18917 --- .../controller/DiffusionServeController.php | 24 +++++++-- .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 4 +- .../diffusion/ssh/DiffusionSSHWorkflow.php | 2 +- .../view/DiffusionPullLogListView.php | 2 +- .../PhabricatorRepositoryPullEvent.php | 51 ++++++++++++++++--- 5 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 0496b93bb6..c0f72ae404 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -104,15 +104,29 @@ final class DiffusionServeController extends DiffusionController { try { $remote_addr = $request->getRemoteAddress(); + if ($request->isHTTPS()) { + $remote_protocol = PhabricatorRepositoryPullEvent::PROTOCOL_HTTPS; + } else { + $remote_protocol = PhabricatorRepositoryPullEvent::PROTOCOL_HTTP; + } + $pull_event = id(new PhabricatorRepositoryPullEvent()) ->setEpoch(PhabricatorTime::getNow()) ->setRemoteAddress($remote_addr) - ->setRemoteProtocol('http'); + ->setRemoteProtocol($remote_protocol); if ($response) { - $pull_event - ->setResultType('wild') - ->setResultCode($response->getHTTPResponseCode()); + $response_code = $response->getHTTPResponseCode(); + + if ($response_code == 200) { + $pull_event + ->setResultType(PhabricatorRepositoryPullEvent::RESULT_PULL) + ->setResultCode($response_code); + } else { + $pull_event + ->setResultType(PhabricatorRepositoryPullEvent::RESULT_ERROR) + ->setResultCode($response_code); + } if ($response instanceof PhabricatorVCSResponse) { $pull_event->setProperties( @@ -122,7 +136,7 @@ final class DiffusionServeController extends DiffusionController { } } else { $pull_event - ->setResultType('exception') + ->setResultType(PhabricatorRepositoryPullEvent::RESULT_EXCEPTION) ->setResultCode(500) ->setProperties( array( diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index e0d35c32b9..5ed42ba79d 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -61,11 +61,11 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { if ($err) { $pull_event - ->setResultType('error') + ->setResultType(PhabricatorRepositoryPullEvent::RESULT_ERROR) ->setResultCode($err); } else { $pull_event - ->setResultType('pull') + ->setResultType(PhabricatorRepositoryPullEvent::RESULT_PULL) ->setResultCode(0); } diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index 3882079f69..e40d8e1f51 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -272,7 +272,7 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { return id(new PhabricatorRepositoryPullEvent()) ->setEpoch(PhabricatorTime::getNow()) ->setRemoteAddress($remote_address) - ->setRemoteProtocol('ssh') + ->setRemoteProtocol(PhabricatorRepositoryPullEvent::PROTOCOL_SSH) ->setPullerPHID($viewer->getPHID()) ->setRepositoryPHID($repository->getPHID()); } diff --git a/src/applications/diffusion/view/DiffusionPullLogListView.php b/src/applications/diffusion/view/DiffusionPullLogListView.php index 7babb9c584..f2e3280eba 100644 --- a/src/applications/diffusion/view/DiffusionPullLogListView.php +++ b/src/applications/diffusion/view/DiffusionPullLogListView.php @@ -94,7 +94,7 @@ final class DiffusionPullLogListView extends AphrontView { pht('From'), pht('Via'), null, - pht('Error'), + pht('Code'), pht('Date'), )) ->setColumnClasses( diff --git a/src/applications/repository/storage/PhabricatorRepositoryPullEvent.php b/src/applications/repository/storage/PhabricatorRepositoryPullEvent.php index 1b095ce8fd..85ea27062b 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPullEvent.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPullEvent.php @@ -15,6 +15,14 @@ final class PhabricatorRepositoryPullEvent private $repository = self::ATTACHABLE; + const RESULT_PULL = 'pull'; + const RESULT_ERROR = 'error'; + const RESULT_EXCEPTION = 'exception'; + + const PROTOCOL_HTTP = 'http'; + const PROTOCOL_HTTPS = 'https'; + const PROTOCOL_SSH = 'ssh'; + public static function initializeNewEvent(PhabricatorUser $viewer) { return id(new PhabricatorRepositoryPushEvent()) ->setPusherPHID($viewer->getPHID()); @@ -62,8 +70,9 @@ final class PhabricatorRepositoryPullEvent public function getRemoteProtocolDisplayName() { $map = array( - 'ssh' => pht('SSH'), - 'http' => pht('HTTP'), + self::PROTOCOL_SSH => pht('SSH'), + self::PROTOCOL_HTTP => pht('HTTP'), + self::PROTOCOL_HTTPS => pht('HTTPS'), ); $protocol = $this->getRemoteProtocol(); @@ -74,25 +83,53 @@ final class PhabricatorRepositoryPullEvent public function newResultIcon() { $icon = new PHUIIconView(); $type = $this->getResultType(); + $code = $this->getResultCode(); + + $protocol = $this->getRemoteProtocol(); + + $is_any_http = + ($protocol === self::PROTOCOL_HTTP) || + ($protocol === self::PROTOCOL_HTTPS); + + // If this was an HTTP request and we responded with a 401, that means + // the user didn't provide credentials. This is technically an error, but + // it's routine and just causes the client to prompt them. Show a more + // comforting icon and description in the UI. + if ($is_any_http) { + if ($code == 401) { + return $icon + ->setIcon('fa-key blue') + ->setTooltip(pht('Authentication Required')); + } + } switch ($type) { - case 'wild': + case self::RESULT_ERROR: $icon - ->setIcon('fa-question indigo') - ->setTooltip(pht('Unknown ("%s")', $type)); + ->setIcon('fa-times red') + ->setTooltip(pht('Error')); break; - case 'pull': + case self::RESULT_EXCEPTION: + $icon + ->setIcon('fa-exclamation-triangle red') + ->setTooltip(pht('Exception')); + break; + case self::RESULT_PULL: $icon ->setIcon('fa-download green') ->setTooltip(pht('Pull')); break; + default: + $icon + ->setIcon('fa-question indigo') + ->setTooltip(pht('Unknown ("%s")', $type)); + break; } return $icon; } - /* -( PhabricatorPolicyInterface )----------------------------------------- */