From a0e820ad9a2e6ae66c8522338ec19dd0a5e430d8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Nov 2013 17:35:43 -0700 Subject: [PATCH] Improve repository hinting and feedback Summary: - Warn about "Read/Write" instead of disabling it, to prevent edits which mutate it after changing a hosted repository to an unhosted one. - Warn about authenticated connections with HTTPS auth disabled, and link to the relevant setting. - When "Autoclose" is disabled, show that "Autoclose Branches" won't have an effect. - For hosted repositories, show the HTTP and SSH clone URIs. - Make them easy to copy/paste. - Link to credential management. - Show if they're read-only. - This could be a bit nicer-looking than it is. Test Plan: Looked at repositories in a bunch of states and made various edits to them. Reviewers: btrahan, chad Reviewed By: btrahan CC: chad, aran Maniphest Tasks: T2230 Differential Revision: https://secure.phabricator.com/D7471 --- src/__celerity_resource_map__.php | 22 +++- .../PhabricatorDiffusionConfigOptions.php | 9 +- .../DiffusionRepositoryController.php | 119 ++++++++++++++++-- ...ffusionRepositoryEditHostingController.php | 62 ++++++--- .../DiffusionRepositoryEditMainController.php | 5 + .../storage/PhabricatorRepository.php | 22 +++- .../application/diffusion/diffusion-icons.css | 14 +++ .../rsrc/js/core/behavior-select-on-click.js | 19 +++ 8 files changed, 232 insertions(+), 40 deletions(-) create mode 100644 webroot/rsrc/js/core/behavior-select-on-click.js diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index ae376b8e33..cb97c30541 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -1145,7 +1145,7 @@ celerity_register_resource_map(array( ), 'diffusion-icons-css' => array( - 'uri' => '/res/b93e32c9/rsrc/css/application/diffusion/diffusion-icons.css', + 'uri' => '/res/82e77537/rsrc/css/application/diffusion/diffusion-icons.css', 'type' => 'css', 'requires' => array( @@ -2485,6 +2485,18 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/search/behavior-reorder-queries.js', ), + 'javelin-behavior-select-on-click' => + array( + 'uri' => '/res/f021b754/rsrc/js/core/behavior-select-on-click.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-dom', + ), + 'disk' => '/rsrc/js/core/behavior-select-on-click.js', + ), 'javelin-behavior-slowvote-embed' => array( 'uri' => '/res/8e85e20d/rsrc/js/application/slowvote/behavior-slowvote-embed.js', @@ -4470,7 +4482,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/5e9e5c4e/differential.pkg.js', 'type' => 'js', ), - '270f4eb4' => + '7aa115b4' => array( 'name' => 'diffusion.pkg.css', 'symbols' => @@ -4478,7 +4490,7 @@ celerity_register_resource_map(array( 0 => 'diffusion-commit-view-css', 1 => 'diffusion-icons-css', ), - 'uri' => '/res/pkg/270f4eb4/diffusion.pkg.css', + 'uri' => '/res/pkg/7aa115b4/diffusion.pkg.css', 'type' => 'css', ), 96909266 => @@ -4570,8 +4582,8 @@ celerity_register_resource_map(array( 'differential-revision-history-css' => '1084b12b', 'differential-revision-list-css' => '1084b12b', 'differential-table-of-contents-css' => '1084b12b', - 'diffusion-commit-view-css' => '270f4eb4', - 'diffusion-icons-css' => '270f4eb4', + 'diffusion-commit-view-css' => '7aa115b4', + 'diffusion-icons-css' => '7aa115b4', 'global-drag-and-drop-css' => 'b05e33c6', 'inline-comment-summary-css' => '1084b12b', 'javelin-aphlict' => '2c1dba03', diff --git a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php index de29a507d6..477836fdb6 100644 --- a/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php +++ b/src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php @@ -101,7 +101,14 @@ final class PhabricatorDiffusionConfigOptions "long as Phabricator uses HTTPS, but it presents a much lower ". "barrier to attackers than SSH does.\n\n". "Consider using SSH for authenticated access to repositories ". - "instead of HTTP.")) + "instead of HTTP.")), + $this->newOption('diffusion.ssh-user', 'string', null) + ->setSummary(pht('Login username for SSH connections to repositories.')) + ->setDescription( + pht( + 'When constructing clone URIs to show to users, Diffusion will '. + 'fill in this login username. If you have configured a VCS user '. + 'like `git`, you should provide it here.')), ); } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index 2034e55743..42465ab0bb 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -165,18 +165,64 @@ final class DiffusionRepositoryController extends DiffusionController { ->setUser($user); $view->addProperty(pht('Callsign'), $repository->getCallsign()); - switch ($repository->getVersionControlSystem()) { - case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - $view->addProperty( - pht('Clone URI'), - $repository->getPublicRemoteURI()); - break; - case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $view->addProperty( - pht('Repository Root'), - $repository->getPublicRemoteURI()); - break; + if ($repository->isHosted()) { + $serve_off = PhabricatorRepository::SERVE_OFF; + $callsign = $repository->getCallsign(); + $repo_path = '/diffusion/'.$callsign.'/'; + + $serve_ssh = $repository->getServeOverSSH(); + if ($serve_ssh !== $serve_off) { + $uri = new PhutilURI(PhabricatorEnv::getProductionURI($repo_path)); + $uri->setProtocol('ssh'); + + $ssh_user = PhabricatorEnv::getEnvConfig('diffusion.ssh-user'); + if ($ssh_user) { + $uri->setUser($ssh_user); + } + + // This isn't quite right, but for now it's probably better to drop + // the port than sometimes show ":8080", etc. Using most VCS commands + // against nonstandard ports is a huge pain so few installs are likely + // to configure SSH on a nonstandard port. + $uri->setPort(null); + + $clone_uri = $this->renderCloneURI( + $uri, + $serve_ssh, + '/settings/panel/ssh/'); + + $view->addProperty(pht('Clone URI (SSH)'), $clone_uri); + } + + $serve_http = $repository->getServeOverHTTP(); + if ($serve_http !== $serve_off) { + $http_uri = PhabricatorEnv::getProductionURI($repo_path); + + $clone_uri = $this->renderCloneURI( + $http_uri, + $serve_http, + PhabricatorEnv::getEnvConfig('diffusion.allow-http-auth') + ? '/settings/panel/vcspassword/' + : null); + + $view->addProperty(pht('Clone URI (HTTP)'), $clone_uri); + } + } else { + switch ($repository->getVersionControlSystem()) { + case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: + case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: + $view->addProperty( + pht('Clone URI'), + $this->renderCloneURI( + $repository->getPublicRemoteURI())); + break; + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + $view->addProperty( + pht('Repository Root'), + $this->renderCloneURI( + $repository->getPublicRemoteURI())); + break; + } } $description = $repository->getDetail('description'); @@ -456,4 +502,53 @@ final class DiffusionRepositoryController extends DiffusionController { return $browse_panel; } + private function renderCloneURI( + $uri, + $serve_mode = null, + $manage_uri = null) { + + require_celerity_resource('diffusion-icons-css'); + + Javelin::initBehavior('select-on-click'); + + $input = javelin_tag( + 'input', + array( + 'type' => 'text', + 'value' => (string)$uri, + 'class' => 'diffusion-clone-uri', + 'sigil' => 'select-on-click', + )); + + $extras = array(); + if ($serve_mode) { + if ($serve_mode === PhabricatorRepository::SERVE_READONLY) { + $extras[] = pht('(Read Only)'); + } + } + + if ($manage_uri) { + if ($this->getRequest()->getUser()->isLoggedIn()) { + $extras[] = phutil_tag( + 'a', + array( + 'href' => $manage_uri, + ), + pht('Manage Credentials')); + } + } + + if ($extras) { + $extras = phutil_implode_html(' ', $extras); + $extras = phutil_tag( + 'div', + array( + 'class' => 'diffusion-clone-extras', + ), + $extras); + } + + return array($input, $extras); + } + } diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditHostingController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditHostingController.php index 5a47b08d1e..7b39aa3602 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditHostingController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditHostingController.php @@ -126,8 +126,12 @@ final class DiffusionRepositoryEditHostingController $request = $this->getRequest(); $user = $request->getUser(); - $v_http_mode = $repository->getServeOverHTTP(); - $v_ssh_mode = $repository->getServeOverSSH(); + $v_http_mode = $repository->getDetail( + 'serve-over-http', + PhabricatorRepository::SERVE_OFF); + $v_ssh_mode = $repository->getDetail( + 'serve-over-ssh', + PhabricatorRepository::SERVE_OFF); $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); $prev_uri = $this->getRepositoryControllerURI($repository, 'edit/hosting/'); @@ -167,13 +171,19 @@ final class DiffusionRepositoryEditHostingController $title = pht('Edit Protocols (%s)', $repository->getName()); - if ($repository->isHosted()) { - $rw_message = pht( - 'Phabricator will serve a read-write copy of this repository'); - } else { - $rw_message = pht( - 'This repository is hosted elsewhere, so Phabricator can not perform '. - 'writes.'); + $rw_message = pht( + 'Phabricator will serve a read-write copy of this repository.'); + + if (!$repository->isHosted()) { + $rw_message = array( + $rw_message, + phutil_tag('br'), + phutil_tag('br'), + pht( + '%s: This repository is hosted elsewhere, so Phabricator can not '. + 'perform writes. This mode will act like "Read Only" for '. + 'repositories hosted elsewhere.', + phutil_tag('strong', array(), 'WARNING'))); } $ssh_control = @@ -185,19 +195,19 @@ final class DiffusionRepositoryEditHostingController PhabricatorRepository::SERVE_OFF, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_OFF), - pht('Phabricator will not serve this repository.')) + pht('Phabricator will not serve this repository over SSH.')) ->addButton( PhabricatorRepository::SERVE_READONLY, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_READONLY), - pht('Phabricator will serve a read-only copy of this repository.')) + pht( + 'Phabricator will serve a read-only copy of this repository '. + 'over SSH.')) ->addButton( PhabricatorRepository::SERVE_READWRITE, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_READWRITE), - $rw_message, - $repository->isHosted() ? null : 'disabled', - $repository->isHosted() ? null : true); + $rw_message); $http_control = id(new AphrontFormRadioButtonControl()) @@ -208,19 +218,19 @@ final class DiffusionRepositoryEditHostingController PhabricatorRepository::SERVE_OFF, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_OFF), - pht('Phabricator will not serve this repository.')) + pht('Phabricator will not serve this repository over HTTP.')) ->addButton( PhabricatorRepository::SERVE_READONLY, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_READONLY), - pht('Phabricator will serve a read-only copy of this repository.')) + pht( + 'Phabricator will serve a read-only copy of this repository '. + 'over HTTP.')) ->addButton( PhabricatorRepository::SERVE_READWRITE, PhabricatorRepository::getProtocolAvailabilityName( PhabricatorRepository::SERVE_READWRITE), - $rw_message, - $repository->isHosted() ? null : 'disabled', - $repository->isHosted() ? null : true); + $rw_message); $form = id(new AphrontFormView()) ->setUser($user) @@ -228,7 +238,19 @@ final class DiffusionRepositoryEditHostingController pht( 'Phabricator can serve repositories over various protocols. You can '. 'configure server protocols here.')) - ->appendChild($ssh_control) + ->appendChild($ssh_control); + + if (!PhabricatorEnv::getEnvConfig('diffusion.allow-http-auth')) { + $form->appendRemarkupInstructions( + pht( + 'NOTE: The configuration setting [[ %s | %s ]] is currently '. + 'disabled. You must enable it to activate authenticated access '. + 'to repositories over HTTP.', + '/config/edit/diffusion.allow-http-auth/', + 'diffusion.allow-http-auth')); + } + + $form ->appendChild($http_control) ->appendChild( id(new AphrontFormSubmitControl()) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index 3c6de3e173..4f6e40876d 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -349,6 +349,11 @@ final class DiffusionRepositoryEditMainController $autoclose_only = nonempty( $repository->getHumanReadableDetail('close-commits-filter', array()), phutil_tag('em', array(), pht('Autoclose On All Branches'))); + + if ($repository->getDetail('disable-autoclose')) { + $autoclose_only = phutil_tag('em', array(), pht('Disabled')); + } + $view->addProperty(pht('Autoclose Only'), $autoclose_only); return $view; diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index dcb5a80320..1db2a5cc41 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -737,7 +737,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } public function getServeOverHTTP() { - return $this->getDetail('serve-over-http', self::SERVE_OFF); + $serve = $this->getDetail('serve-over-http', self::SERVE_OFF); + return $this->normalizeServeConfigSetting($serve); } public function setServeOverHTTP($mode) { @@ -745,7 +746,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } public function getServeOverSSH() { - return $this->getDetail('serve-over-ssh', self::SERVE_OFF); + $serve = $this->getDetail('serve-over-ssh', self::SERVE_OFF); + return $this->normalizeServeConfigSetting($serve); } public function setServeOverSSH($mode) { @@ -765,6 +767,22 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } } + private function normalizeServeConfigSetting($value) { + switch ($value) { + case self::SERVE_OFF: + case self::SERVE_READONLY: + return $value; + case self::SERVE_READWRITE: + if ($this->isHosted()) { + return self::SERVE_READWRITE; + } else { + return self::SERVE_READONLY; + } + default: + return self::SERVE_OFF; + } + } + /** * Raise more useful errors when there are basic filesystem problems. diff --git a/webroot/rsrc/css/application/diffusion/diffusion-icons.css b/webroot/rsrc/css/application/diffusion/diffusion-icons.css index f97cf6e5cb..21bb73f0d5 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-icons.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-icons.css @@ -26,3 +26,17 @@ .diffusion-path-icon-link { background-image: url(/rsrc/image/icon/fatcow/page_white_link.png); } + +input.diffusion-clone-uri { + display: block; + width: 100%; + border: 1px solid #efefef; + box-shadow: none; + height: 24px; +} + +.diffusion-clone-extras { + font-size: 11px; + text-align: right; + color: {$lightgreytext}; +} diff --git a/webroot/rsrc/js/core/behavior-select-on-click.js b/webroot/rsrc/js/core/behavior-select-on-click.js new file mode 100644 index 0000000000..0d13dd0d30 --- /dev/null +++ b/webroot/rsrc/js/core/behavior-select-on-click.js @@ -0,0 +1,19 @@ +/** + * @provides javelin-behavior-select-on-click + * @requires javelin-behavior + * javelin-stratcom + * javelin-dom + * @javelin + */ + +JX.behavior('select-on-click', function(config) { + JX.Stratcom.listen( + 'click', + 'select-on-click', + function(e) { + e.kill(); + var node = e.getNode('select-on-click'); + JX.DOM.focus(node); + node.select(); + }); +});