Fill in new URI credential edit web UI interfaces

Summary:
Ref T10748. Ref T10366. Allows users to set credential for new URIs.

  - Ref T7221. Our handling of the "git://" protocol is currently incorrect. This protocol is not authenticated, but is considered an SSH protocol. In the new UI, it is considered an anonymous/unauthenticated protocol instead.
  - Ref T10241. This fixes the `PassphraseCredentialControl` so it doesn't silently edit the value if the current value is not visible to you and/or not valid.

Test Plan:
Performed a whole lot of credential edits, removals, and adjustments. I'll give this additional vetting before cutting over to it.

{F1253207}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7221, T10241, T10366, T10748

Differential Revision: https://secure.phabricator.com/D15829

src/__phutil_library_map__.php

@@ -789,6 +789,7 @@ phutil_register_library_map(array(
     'DiffusionRepositorySymbolsManagementPanel' => 'applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php',
     'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php',
     'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php',
+    'DiffusionRepositoryURICredentialController' => 'applications/diffusion/controller/DiffusionRepositoryURICredentialController.php',
     'DiffusionRepositoryURIDisableController' => 'applications/diffusion/controller/DiffusionRepositoryURIDisableController.php',
     'DiffusionRepositoryURIEditController' => 'applications/diffusion/controller/DiffusionRepositoryURIEditController.php',
     'DiffusionRepositoryURIViewController' => 'applications/diffusion/controller/DiffusionRepositoryURIViewController.php',
@@ -5020,6 +5021,7 @@ phutil_register_library_map(array(
     'DiffusionRepositorySymbolsManagementPanel' => 'DiffusionRepositoryManagementPanel',
     'DiffusionRepositoryTag' => 'Phobject',
     'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController',
+    'DiffusionRepositoryURICredentialController' => 'DiffusionController',
     'DiffusionRepositoryURIDisableController' => 'DiffusionController',
     'DiffusionRepositoryURIEditController' => 'DiffusionController',
     'DiffusionRepositoryURIViewController' => 'DiffusionController',

src/applications/diffusion/application/PhabricatorDiffusionApplication.php

@@ -97,6 +97,8 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication {
               => 'DiffusionRepositoryURIDisableController',
             $this->getEditRoutePattern('edit/')
               => 'DiffusionRepositoryURIEditController',
+            'credential/(?P<id>[0-9]\d*)/(?P<action>edit|remove)/'
+              => 'DiffusionRepositoryURICredentialController',
           ),
           'edit/' => array(
             '' => 'DiffusionRepositoryEditMainController',

src/applications/diffusion/controller/DiffusionMirrorEditController.php

@@ -108,7 +108,7 @@ final class DiffusionMirrorEditController
           ->setName('remoteURI')
           ->setValue($v_remote)
           ->setError($e_remote))
-      ->appendChild(
+      ->appendControl(
         id(new PassphraseCredentialControl())
           ->setLabel(pht('Credentials'))
           ->setName('credential')

src/applications/diffusion/controller/DiffusionRepositoryCreateController.php

@@ -510,6 +510,7 @@ final class DiffusionRepositoryCreateController
       ->setAdjustFormPageCallback(array($this, 'adjustAuthPage'))
       ->addControl(
         id(new PassphraseCredentialControl())
+          ->setViewer($this->getViewer())
           ->setName('credential'));
   }
 

src/applications/diffusion/controller/DiffusionRepositoryURICredentialController.php

@@ -0,0 +1,159 @@
+<?php
+
+final class DiffusionRepositoryURICredentialController
+  extends DiffusionController {
+
+  public function handleRequest(AphrontRequest $request) {
+    $response = $this->loadDiffusionContextForEdit();
+    if ($response) {
+      return $response;
+    }
+
+    $viewer = $this->getViewer();
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+
+    $id = $request->getURIData('id');
+    $uri = id(new PhabricatorRepositoryURIQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($id))
+      ->withRepositories(array($repository))
+      ->requireCapabilities(
+        array(
+          PhabricatorPolicyCapability::CAN_VIEW,
+          PhabricatorPolicyCapability::CAN_EDIT,
+        ))
+      ->executeOne();
+    if (!$uri) {
+      return new Aphront404Response();
+    }
+
+    $is_builtin = $uri->isBuiltin();
+    $has_credential = (bool)$uri->getCredentialPHID();
+    $view_uri = $uri->getViewURI();
+    $is_remove = ($request->getURIData('action') == 'remove');
+
+    if ($is_builtin) {
+      return $this->newDialog()
+        ->setTitle(pht('Builtin URIs Do Not Use Credentials'))
+        ->appendParagraph(
+          pht(
+            'You can not set a credential for builtin URIs which Phabricator '.
+            'hosts and serves. Phabricator does not fetch from these URIs or '.
+            'push to these URIs, and does not need credentials to '.
+            'authenticate any activity against them.'))
+        ->addCancelButton($view_uri);
+    }
+
+    if ($request->isFormPost()) {
+      $xactions = array();
+
+      if ($is_remove) {
+        $new_phid = null;
+      } else {
+        $new_phid = $request->getStr('credentialPHID');
+      }
+
+      $type_credential = PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL;
+
+      $xactions[] = id(new PhabricatorRepositoryURITransaction())
+        ->setTransactionType($type_credential)
+        ->setNewValue($new_phid);
+
+      $editor = id(new DiffusionURIEditor())
+        ->setActor($viewer)
+        ->setContinueOnNoEffect(true)
+        ->setContinueOnMissingFields(true)
+        ->setContentSourceFromRequest($request)
+        ->applyTransactions($uri, $xactions);
+
+      return id(new AphrontRedirectResponse())->setURI($view_uri);
+    }
+
+    $command_engine = $uri->newCommandEngine();
+    $is_supported = $command_engine->isCredentialSupported();
+
+    $body = null;
+    $form = null;
+    $width = AphrontDialogView::WIDTH_DEFAULT;
+    if ($is_remove) {
+      if ($has_credential) {
+        $title = pht('Remove Credential');
+        $body = pht(
+          'This credential will no longer be used to authenticate activity '.
+          'against this URI.');
+        $button = pht('Remove Credential');
+      } else {
+        $title = pht('No Credential');
+        $body = pht(
+          'This URI does not have an associated credential.');
+        $button = null;
+      }
+    } else if (!$is_supported) {
+      $title = pht('Unauthenticated Protocol');
+      $body = pht(
+        'The protocol for this URI ("%s") does not use authentication, so '.
+        'you can not provide a credential.',
+        $command_engine->getDisplayProtocol());
+      $button = null;
+    } else {
+      $effective_uri = $uri->getEffectiveURI();
+
+      $label = $command_engine->getPassphraseCredentialLabel();
+      $credential_type = $command_engine->getPassphraseDefaultCredentialType();
+
+      $provides_type = $command_engine->getPassphraseProvidesCredentialType();
+      $options = id(new PassphraseCredentialQuery())
+        ->setViewer($viewer)
+        ->withIsDestroyed(false)
+        ->withProvidesTypes(array($provides_type))
+        ->execute();
+
+      $control = id(new PassphraseCredentialControl())
+        ->setName('credentialPHID')
+        ->setLabel($label)
+        ->setValue($uri->getCredentialPHID())
+        ->setCredentialType($credential_type)
+        ->setOptions($options);
+
+      $default_user = $effective_uri->getUser();
+      if (strlen($default_user)) {
+        $control->setDefaultUsername($default_user);
+      }
+
+      $form = id(new AphrontFormView())
+        ->setViewer($viewer)
+        ->appendControl($control);
+
+      if ($has_credential) {
+        $title = pht('Update Credential');
+        $button = pht('Update Credential');
+      } else {
+        $title = pht('Set Credential');
+        $button = pht('Set Credential');
+      }
+
+      $width = AphrontDialogView::WIDTH_FORM;
+    }
+
+    $dialog = $this->newDialog()
+      ->setWidth($width)
+      ->setTitle($title)
+      ->addCancelButton($view_uri);
+
+    if ($body) {
+      $dialog->appendParagraph($body);
+    }
+
+    if ($form) {
+      $dialog->appendForm($form);
+    }
+
+    if ($button) {
+      $dialog->addSubmitButton($button);
+    }
+
+    return $dialog;
+  }
+
+}

src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php

@@ -82,6 +82,7 @@ final class DiffusionRepositoryURIViewController
 
   private function buildCurtain(PhabricatorRepositoryURI $uri) {
     $viewer = $this->getViewer();
+    $repository = $uri->getRepository();
     $id = $uri->getID();
 
     $can_edit = PhabricatorPolicyFilter::hasCapability(
@@ -89,7 +90,6 @@ final class DiffusionRepositoryURIViewController
       $uri,
       PhabricatorPolicyCapability::CAN_EDIT);
 
-
     $curtain = $this->newCurtainView($uri);
 
     $edit_uri = $uri->getEditURI();
@@ -102,6 +102,43 @@ final class DiffusionRepositoryURIViewController
         ->setWorkflow(!$can_edit)
         ->setDisabled(!$can_edit));
 
+    $credential_uri = $repository->getPathURI("uri/credential/{$id}/edit/");
+    $remove_uri = $repository->getPathURI("uri/credential/{$id}/remove/");
+    $has_credential = (bool)$uri->getCredentialPHID();
+
+    if ($uri->isBuiltin()) {
+      $can_credential = false;
+    } else if (!$uri->newCommandEngine()->isCredentialSupported()) {
+      $can_credential = false;
+    } else {
+      $can_credential = true;
+    }
+
+    $can_update = ($can_edit && $can_credential);
+    $can_remove = ($can_edit && $has_credential);
+
+    if ($has_credential) {
+      $credential_name = pht('Update Credential');
+    } else {
+      $credential_name = pht('Set Credential');
+    }
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setIcon('fa-key')
+        ->setName($credential_name)
+        ->setHref($credential_uri)
+        ->setWorkflow(true)
+        ->setDisabled(!$can_edit));
+
+    $curtain->addAction(
+      id(new PhabricatorActionView())
+        ->setIcon('fa-times')
+        ->setName(pht('Remove Credential'))
+        ->setHref($remove_uri)
+        ->setWorkflow(true)
+        ->setDisabled(!$can_remove));
+
     if ($uri->getIsDisabled()) {
       $disable_name = pht('Enable URI');
       $disable_icon = 'fa-check';
@@ -110,7 +147,7 @@ final class DiffusionRepositoryURIViewController
       $disable_icon = 'fa-ban';
     }
 
-    $disable_uri = $uri->getRepository()->getPathURI("uri/disable/{$id}/");
+    $disable_uri = $repository->getPathURI("uri/disable/{$id}/");
 
     $curtain->addAction(
       id(new PhabricatorActionView())
@@ -130,7 +167,84 @@ final class DiffusionRepositoryURIViewController
       ->setUser($viewer);
 
     $properties->addProperty(pht('URI'), $uri->getDisplayURI());
-    $properties->addProperty(pht('Credential'), 'TODO');
+
+    $credential_phid = $uri->getCredentialPHID();
+    $command_engine = $uri->newCommandEngine();
+    $is_optional = $command_engine->isCredentialOptional();
+    $is_supported = $command_engine->isCredentialSupported();
+    $is_builtin = $uri->isBuiltin();
+
+    if ($is_builtin) {
+      $credential_icon = 'fa-circle-o';
+      $credential_color = 'grey';
+      $credential_label = pht('Builtin');
+      $credential_note = pht('Builtin URIs do not use credentials.');
+    } else if (!$is_supported) {
+      $credential_icon = 'fa-circle-o';
+      $credential_color = 'grey';
+      $credential_label = pht('Not Supported');
+      $credential_note = pht('This protocol does not support authentication.');
+    } else if (!$credential_phid) {
+      if ($is_optional) {
+        $credential_icon = 'fa-circle-o';
+        $credential_color = 'green';
+        $credential_label = pht('No Credential');
+        $credential_note = pht('Configured for anonymous access.');
+      } else {
+        $credential_icon = 'fa-times';
+        $credential_color = 'red';
+        $credential_label = pht('Required');
+        $credential_note = pht('Credential required but not configured.');
+      }
+    } else {
+      // Don't raise a policy exception if we can't see the credential.
+      $credentials = id(new PassphraseCredentialQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($credential_phid))
+        ->execute();
+      $credential = head($credentials);
+
+      if (!$credential) {
+        $handles = $viewer->loadHandles(array($credential_phid));
+        $handle = $handles[$credential_phid];
+        if ($handle->getPolicyFiltered()) {
+          $credential_icon = 'fa-lock';
+          $credential_color = 'grey';
+          $credential_label = pht('Restricted');
+          $credential_note = pht(
+            'You do not have permission to view the configured '.
+            'credential.');
+        } else {
+          $credential_icon = 'fa-times';
+          $credential_color = 'red';
+          $credential_label = pht('Invalid');
+          $credential_note = pht('Configured credential is invalid.');
+        }
+      } else {
+        $provides = $credential->getProvidesType();
+        $needs = $command_engine->getPassphraseProvidesCredentialType();
+        if ($provides != $needs) {
+          $credential_icon = 'fa-times';
+          $credential_color = 'red';
+          $credential_label = pht('Wrong Type');
+        } else {
+          $credential_icon = 'fa-check';
+          $credential_color = 'green';
+          $credential_label = $command_engine->getPassphraseCredentialLabel();
+        }
+        $credential_note = $viewer->renderHandle($credential_phid);
+      }
+    }
+
+    $credential_item = id(new PHUIStatusItemView())
+      ->setIcon($credential_icon, $credential_color)
+      ->setTarget(phutil_tag('strong', array(), $credential_label))
+      ->setNote($credential_note);
+
+    $credential_view = id(new PHUIStatusListView())
+      ->addItem($credential_item);
+
+    $properties->addProperty(pht('Credential'), $credential_view);
 
 
     $io_type = $uri->getEffectiveIOType();

src/applications/diffusion/editor/DiffusionURIEditEngine.php

@@ -83,6 +83,14 @@ final class DiffusionURIEditEngine
   protected function buildCustomEditFields($object) {
     $viewer = $this->getViewer();
 
+    if ($object->isBuiltin()) {
+      $is_builtin = true;
+      $uri_value = (string)$object->getDisplayURI();
+    } else {
+      $is_builtin = false;
+      $uri_value = $object->getURI();
+    }
+
     return array(
       id(new PhabricatorHandlesEditField())
         ->setKey('repository')
@@ -104,12 +112,13 @@ final class DiffusionURIEditEngine
       id(new PhabricatorTextEditField())
         ->setKey('uri')
         ->setLabel(pht('URI'))
-        ->setIsRequired(true)
         ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_URI)
         ->setDescription(pht('The repository URI.'))
         ->setConduitDescription(pht('Change the repository URI.'))
         ->setConduitTypeDescription(pht('New repository URI.'))
-        ->setValue($object->getURI()),
+        ->setIsRequired(!$is_builtin)
+        ->setIsLocked($is_builtin)
+        ->setValue($uri_value),
       id(new PhabricatorSelectEditField())
         ->setKey('io')
         ->setLabel(pht('I/O Type'))

src/applications/diffusion/editor/DiffusionURIEditor.php

@@ -70,7 +70,42 @@ final class DiffusionURIEditor
 
     switch ($xaction->getTransactionType()) {
       case PhabricatorRepositoryURITransaction::TYPE_URI:
+        if (!$this->getIsNewObject()) {
+          $old_uri = $object->getEffectiveURI();
+        } else {
+          $old_uri = null;
+        }
+
         $object->setURI($xaction->getNewValue());
+
+        // If we've changed the domain or protocol of the URI, remove the
+        // current credential. This improves behavior in several cases:
+
+        // If a user switches between protocols with different credential
+        // types, like HTTP and SSH, the old credential won't be valid anyway.
+        // It's cleaner to remove it than leave a bad credential in place.
+
+        // If a user switches hosts, the old credential is probably not
+        // correct (and potentially confusing/misleading). Removing it forces
+        // users to double check that they have the correct credentials.
+
+        // If an attacker can't see a symmetric credential like a username and
+        // password, they could still potentially capture it by changing the
+        // host for a URI that uses it to `evil.com`, a server they control,
+        // then observing the requests. Removing the credential prevents this
+        // kind of escalation.
+
+        // Since port and path changes are less likely to fall among these
+        // cases, they don't trigger a credential wipe.
+
+        $new_uri = $object->getEffectiveURI();
+        if ($old_uri) {
+          $new_proto = ($old_uri->getProtocol() != $new_uri->getProtocol());
+          $new_domain = ($old_uri->getDomain() != $new_uri->getDomain());
+          if ($new_proto || $new_domain) {
+            $object->setCredentialPHID(null);
+          }
+        }
         break;
       case PhabricatorRepositoryURITransaction::TYPE_IO:
         $object->setIOType($xaction->getNewValue());
@@ -184,6 +219,11 @@ final class DiffusionURIEditor
             continue;
           }
 
+          // Anyone who can edit a URI can remove the credential.
+          if ($credential_phid === null) {
+            continue;
+          }
+
           $credential = id(new PassphraseCredentialQuery())
             ->setViewer($viewer)
             ->withPHIDs(array($credential_phid))

src/applications/diffusion/protocol/DiffusionCommandEngine.php

@@ -57,6 +57,10 @@ abstract class DiffusionCommandEngine extends Phobject {
     return $this->protocol;
   }
 
+  public function getDisplayProtocol() {
+    return $this->getProtocol().'://';
+  }
+
   public function setCredentialPHID($credential_phid) {
     $this->credentialPHID = $credential_phid;
     return $this;
@@ -197,34 +201,82 @@ abstract class DiffusionCommandEngine extends Phobject {
     return $env;
   }
 
-  protected function isSSHProtocol() {
+  public function isSSHProtocol() {
     return ($this->getProtocol() == 'ssh');
   }
 
-  protected function isSVNProtocol() {
+  public function isSVNProtocol() {
     return ($this->getProtocol() == 'svn');
   }
 
-  protected function isSVNSSHProtocol() {
+  public function isSVNSSHProtocol() {
     return ($this->getProtocol() == 'svn+ssh');
   }
 
-  protected function isHTTPProtocol() {
+  public function isHTTPProtocol() {
     return ($this->getProtocol() == 'http');
   }
 
-  protected function isHTTPSProtocol() {
+  public function isHTTPSProtocol() {
     return ($this->getProtocol() == 'https');
   }
 
-  protected function isAnyHTTPProtocol() {
+  public function isAnyHTTPProtocol() {
     return ($this->isHTTPProtocol() || $this->isHTTPSProtocol());
   }
 
-  protected function isAnySSHProtocol() {
+  public function isAnySSHProtocol() {
     return ($this->isSSHProtocol() || $this->isSVNSSHProtocol());
   }
 
+  public function isCredentialSupported() {
+    return ($this->getPassphraseProvidesCredentialType() !== null);
+  }
+
+  public function isCredentialOptional() {
+    if ($this->isAnySSHProtocol()) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function getPassphraseCredentialLabel() {
+    if ($this->isAnySSHProtocol()) {
+      return pht('SSH Key');
+    }
+
+    if ($this->isAnyHTTPProtocol() || $this->isSVNProtocol()) {
+      return pht('Password');
+    }
+
+    return null;
+  }
+
+  public function getPassphraseDefaultCredentialType() {
+    if ($this->isAnySSHProtocol()) {
+      return PassphraseSSHPrivateKeyTextCredentialType::CREDENTIAL_TYPE;
+    }
+
+    if ($this->isAnyHTTPProtocol() || $this->isSVNProtocol()) {
+      return PassphrasePasswordCredentialType::CREDENTIAL_TYPE;
+    }
+
+    return null;
+  }
+
+  public function getPassphraseProvidesCredentialType() {
+    if ($this->isAnySSHProtocol()) {
+      return PassphraseSSHPrivateKeyCredentialType::PROVIDES_TYPE;
+    }
+
+    if ($this->isAnyHTTPProtocol() || $this->isSVNProtocol()) {
+      return PassphrasePasswordCredentialType::PROVIDES_TYPE;
+    }
+
+    return null;
+  }
+
   protected function getSSHWrapper() {
     $root = dirname(phutil_get_library_root('phabricator'));
     return $root.'/bin/ssh-connect';

src/applications/passphrase/view/PassphraseCredentialControl.php

@@ -42,10 +42,50 @@ final class PassphraseCredentialControl extends AphrontFormControl {
     foreach ($this->options as $option) {
       $options_map[$option->getPHID()] = pht(
         '%s %s',
-        'K'.$option->getID(),
+        $option->getMonogram(),
         $option->getName());
     }
 
+    // The user editing the form may not have permission to see the current
+    // credential. Populate it into the menu to allow them to save the form
+    // without making any changes.
+    $current_phid = $this->getValue();
+    if (strlen($current_phid) && empty($options_map[$current_phid])) {
+      $viewer = $this->getViewer();
+
+      $user_credential = id(new PassphraseCredentialQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($current_phid))
+        ->executeOne();
+      if (!$user_credential) {
+        // Pull the credential with the ominipotent viewer so we can look up
+        // the ID and tell if it's restricted or invalid.
+        $omnipotent_credential = id(new PassphraseCredentialQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withPHIDs(array($current_phid))
+          ->executeOne();
+        if ($omnipotent_credential) {
+          $current_name = pht(
+            '%s (Restricted Credential)',
+            $omnipotent_credential->getMonogram());
+        } else {
+          $current_name = pht(
+            'Invalid Credential ("%s")',
+            $current_phid);
+        }
+      } else {
+        $current_name = pht(
+          '%s %s',
+          $user_credential->getMonogram(),
+          $user_credential->getName());
+      }
+
+      $options_map = array(
+        $current_phid => $current_name,
+      ) + $options_map;
+    }
+
+
     $disabled = $this->getDisabled();
     if ($this->allowNull) {
       $options_map = array('' => pht('(No Credentials)')) + $options_map;

src/applications/repository/storage/PhabricatorRepositoryURI.php

@@ -193,13 +193,75 @@ final class PhabricatorRepositoryURI
 
 
   public function getDisplayURI() {
-    $uri = new PhutilURI($this->getURI());
+    return $this->getURIObject(false);
+  }
 
+  public function getEffectiveURI() {
+    return $this->getURIObject(true);
+  }
+
+  private function getURIObject($normalize) {
+    // Users can provide Git/SCP-style URIs in the form "user@host:path".
+    // These are equivalent to "ssh://user@host/path". We use the more standard
+    // form internally, and convert to it if we need to specify a port number,
+    // but try to preserve what the user typed when displaying the URI.
+
+    if ($this->isBuiltin()) {
+      $builtin_protocol = $this->getForcedProtocol();
+      $builtin_domain = $this->getForcedHost();
+      $raw_uri = "{$builtin_protocol}://{$builtin_domain}";
+    } else {
+      $raw_uri = $this->getURI();
+    }
+
+    $port = $this->getForcedPort();
+
+    $default_ports = array(
+      'ssh' => 22,
+      'http' => 80,
+      'https' => 443,
+    );
+
+    $uri = new PhutilURI($raw_uri);
+    if (!$uri->getProtocol()) {
+      $git_uri = new PhutilGitURI($raw_uri);
+
+      // We must normalize this Git-style URI into a normal URI
+      $must_normalize = ($port && ($port != $default_ports['ssh']));
+      if ($must_normalize || $normalize) {
+        $domain = $git_uri->getDomain();
+
+
+        $uri = id(new PhutilURI("ssh://{$domain}"))
+          ->setUser($git_uri->getUser())
+          ->setPath($git_uri->getPath());
+      } else {
+        $uri = $git_uri;
+      }
+    }
+
+    $is_normal = ($uri instanceof PhutilURI);
+
+    if ($is_normal) {
       $protocol = $this->getForcedProtocol();
       if ($protocol) {
         $uri->setProtocol($protocol);
       }
 
+      if ($port) {
+        $uri->setPort($port);
+      }
+
+      // Remove any explicitly set default ports.
+      $uri_port = $uri->getPort();
+      $uri_protocol = $uri->getProtocol();
+
+      $uri_default = idx($default_ports, $uri_protocol);
+      if ($uri_default && ($uri_default == $uri_port)) {
+        $uri->setPort(null);
+      }
+    }
+
     $user = $this->getForcedUser();
     if ($user) {
       $uri->setUser($user);
@@ -210,11 +272,6 @@ final class PhabricatorRepositoryURI
       $uri->setDomain($host);
     }
 
-    $port = $this->getForcedPort();
-    if ($port) {
-      $uri->setPort($port);
-    }
-
     $path = $this->getForcedPath();
     if ($path) {
       $uri->setPath($path);
@@ -223,6 +280,7 @@ final class PhabricatorRepositoryURI
     return $uri;
   }
 
+
   private function getForcedProtocol() {
     switch ($this->getBuiltinProtocol()) {
       case self::BUILTIN_PROTOCOL_SSH:
@@ -446,6 +504,16 @@ final class PhabricatorRepositoryURI
     );
   }
 
+  public function newCommandEngine() {
+    $repository = $this->getRepository();
+    $protocol = $this->getEffectiveURI()->getProtocol();
+
+    return DiffusionCommandEngine::newCommandEngine($repository)
+      ->setCredentialPHID($this->getCredentialPHID())
+      ->setProtocol($protocol);
+  }
+
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
@@ -569,7 +637,8 @@ final class PhabricatorRepositoryURI
       'repositoryPHID' => $this->getRepositoryPHID(),
       'uri' => array(
         'raw' => $this->getURI(),
-        'effective' => (string)$this->getDisplayURI(),
+        'display' => (string)$this->getDisplayURI(),
+        'effective' => (string)$this->getEffectiveURI(),
       ),
       'io' => array(
         'raw' => $this->getIOType(),

src/applications/repository/storage/PhabricatorRepositoryURITransaction.php

@@ -18,6 +18,26 @@ final class PhabricatorRepositoryURITransaction
     return PhabricatorRepositoryURIPHIDType::TYPECONST;
   }
 
+  public function getRequiredHandlePHIDs() {
+    $phids = parent::getRequiredHandlePHIDs();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_CREDENTIAL:
+        if ($old) {
+          $phids[] = $old;
+        }
+        if ($new) {
+          $phids[] = $new;
+        }
+        break;
+    }
+
+    return $phids;
+  }
+
   public function getTitle() {
     $author_phid = $this->getAuthorPHID();
 
@@ -61,6 +81,24 @@ final class PhabricatorRepositoryURITransaction
             '%s enabled this URI.',
             $this->renderHandleLink($author_phid));
         }
+      case self::TYPE_CREDENTIAL:
+        if ($old && $new) {
+          return pht(
+            '%s changed the credential for this URI from %s to %s.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($old),
+            $this->renderHandleLink($new));
+        } else if ($old) {
+          return pht(
+            '%s removed %s as the credential for this URI.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($old));
+        } else if ($new) {
+          return pht(
+            '%s set the credential for this URI to %s.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($new));
+        }
 
     }
 

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldCredential.php

@@ -34,6 +34,7 @@ final class PhabricatorStandardCustomFieldCredential
       ->execute();
 
     return id(new PassphraseCredentialControl())
+      ->setViewer($this->getViewer())
       ->setLabel($this->getFieldName())
       ->setName($this->getFieldKey())
       ->setCaption($this->getCaption())