Move the repository policy step into the create workflow

Summary:
Fixes T4242. It's currently possible to set nonsense defaults and create repositories with unintended policies, because policy configuration isn't part of creation. Instead:
  - put a policy page into the creation workflow;
  - require the selection of valid policies (i.e., prevent creating a repository you can't view / edit).

Test Plan:
  - Created imported and hosted repositories, hit policy selection.
  - Edited policies of existing repositories.
  - Tried to set nonsense policies.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4242

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

src/__phutil_library_map__.php

@@ -546,7 +546,6 @@ phutil_register_library_map(array(
     'DiffusionRepositoryEditHostingController' => 'applications/diffusion/controller/DiffusionRepositoryEditHostingController.php',
     'DiffusionRepositoryEditLocalController' => 'applications/diffusion/controller/DiffusionRepositoryEditLocalController.php',
     'DiffusionRepositoryEditMainController' => 'applications/diffusion/controller/DiffusionRepositoryEditMainController.php',
-    'DiffusionRepositoryEditPolicyController' => 'applications/diffusion/controller/DiffusionRepositoryEditPolicyController.php',
     'DiffusionRepositoryEditSubversionController' => 'applications/diffusion/controller/DiffusionRepositoryEditSubversionController.php',
     'DiffusionRepositoryListController' => 'applications/diffusion/controller/DiffusionRepositoryListController.php',
     'DiffusionRepositoryNewController' => 'applications/diffusion/controller/DiffusionRepositoryNewController.php',
@@ -2945,7 +2944,6 @@ phutil_register_library_map(array(
     'DiffusionRepositoryEditHostingController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryEditLocalController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryEditMainController' => 'DiffusionRepositoryEditController',
-    'DiffusionRepositoryEditPolicyController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryEditSubversionController' => 'DiffusionRepositoryEditController',
     'DiffusionRepositoryListController' =>
     array(

src/applications/diffusion/application/PhabricatorApplicationDiffusion.php

@@ -73,11 +73,11 @@ final class PhabricatorApplicationDiffusion extends PhabricatorApplication {
             'encoding/' => 'DiffusionRepositoryEditEncodingController',
             'activate/' => 'DiffusionRepositoryEditActivateController',
             'dangerous/' => 'DiffusionRepositoryEditDangerousController',
-            'policy/' => 'DiffusionRepositoryEditPolicyController',
             'branches/' => 'DiffusionRepositoryEditBranchesController',
             'subversion/' => 'DiffusionRepositoryEditSubversionController',
             'actions/' => 'DiffusionRepositoryEditActionsController',
             '(?P<edit>remote)/' => 'DiffusionRepositoryCreateController',
+            '(?P<edit>policy)/' => 'DiffusionRepositoryCreateController',
             'local/' => 'DiffusionRepositoryEditLocalController',
             'delete/' => 'DiffusionRepositoryEditDeleteController',
             'hosting/' => 'DiffusionRepositoryEditHostingController',

src/applications/diffusion/controller/DiffusionRepositoryCreateController.php

@@ -16,12 +16,13 @@ final class DiffusionRepositoryCreateController
     $viewer = $request->getUser();
 
     // NOTE: We can end up here via either "Create Repository", or via
-    // "Import Repository", or via "Edit Remote". In the latter case, we show
+    // "Import Repository", or via "Edit Remote", or via "Edit Policies". In
-    // only a few of the pages.
+    // the latter two cases, we show only a few of the pages.
 
     $repository = null;
     switch ($this->edit) {
       case 'remote':
+      case 'policy':
         $repository = $this->getDiffusionRequest()->getRepository();
 
         // Make sure we have CAN_EDIT.
@@ -56,11 +57,17 @@ final class DiffusionRepositoryCreateController
           ->addPage('remote-uri', $this->buildRemoteURIPage())
           ->addPage('auth', $this->buildAuthPage());
         break;
+      case 'policy':
+        $title = pht('Edit Policies');
+        $form
+          ->addPage('policy', $this->buildPolicyPage());
+        break;
       case 'create':
         $title = pht('Create Repository');
         $form
           ->addPage('vcs', $this->buildVCSPage())
           ->addPage('name', $this->buildNamePage())
+          ->addPage('policy', $this->buildPolicyPage())
           ->addPage('done', $this->buildDonePage());
         break;
       case 'import':
@@ -70,6 +77,7 @@ final class DiffusionRepositoryCreateController
           ->addPage('name', $this->buildNamePage())
           ->addPage('remote-uri', $this->buildRemoteURIPage())
           ->addPage('auth', $this->buildAuthPage())
+          ->addPage('policy', $this->buildPolicyPage())
           ->addPage('done', $this->buildDonePage());
         break;
     }
@@ -80,6 +88,7 @@ final class DiffusionRepositoryCreateController
 
         $is_create = ($this->edit === 'import' || $this->edit === 'create');
         $is_auth = ($this->edit == 'import' || $this->edit == 'remote');
+        $is_policy = ($this->edit != 'remote');
         $is_init = ($this->edit == 'create');
 
         if ($is_create) {
@@ -96,6 +105,9 @@ final class DiffusionRepositoryCreateController
         $type_remote_uri = PhabricatorRepositoryTransaction::TYPE_REMOTE_URI;
         $type_hosting = PhabricatorRepositoryTransaction::TYPE_HOSTING;
         $type_credential = PhabricatorRepositoryTransaction::TYPE_CREDENTIAL;
+        $type_view = PhabricatorTransactions::TYPE_VIEW_POLICY;
+        $type_edit = PhabricatorTransactions::TYPE_EDIT_POLICY;
+        $type_push = PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY;
 
         $xactions = array();
 
@@ -160,6 +172,25 @@ final class DiffusionRepositoryCreateController
               $form->getPage('auth')->getControl('credential')->getValue());
         }
 
+        if ($is_policy) {
+          $xactions[] = id(clone $template)
+            ->setTransactionType($type_view)
+            ->setNewValue(
+              $form->getPage('policy')->getControl('viewPolicy')->getValue());
+
+          $xactions[] = id(clone $template)
+            ->setTransactionType($type_edit)
+            ->setNewValue(
+              $form->getPage('policy')->getControl('editPolicy')->getValue());
+
+          if ($is_init || $repository->isHosted()) {
+            $xactions[] = id(clone $template)
+              ->setTransactionType($type_push)
+              ->setNewValue(
+                $form->getPage('policy')->getControl('pushPolicy')->getValue());
+          }
+        }
+
         id(new PhabricatorRepositoryEditor())
           ->setContinueOnNoEffect(true)
           ->setContentSourceFromRequest($request)
@@ -175,6 +206,9 @@ final class DiffusionRepositoryCreateController
         $dict = array(
           'remoteURI' => $repository->getRemoteURI(),
           'credential' => $repository->getCredentialPHID(),
+          'viewPolicy' => $repository->getViewPolicy(),
+          'editPolicy' => $repository->getEditPolicy(),
+          'pushPolicy' => $repository->getPushPolicy(),
         );
       }
       $form->readFromObject($dict);
@@ -654,6 +688,122 @@ final class DiffusionRepositoryCreateController
     }
   }
 
+
+/* -(  Page: Policy  )------------------------------------------------------- */
+
+
+  private function buildPolicyPage() {
+    $viewer = $this->getRequest()->getUser();
+    if ($this->getRepository()) {
+      $repository = $this->getRepository();
+    } else {
+      $repository = PhabricatorRepository::initializeNewRepository($viewer);
+    }
+
+    $policies = id(new PhabricatorPolicyQuery())
+      ->setViewer($viewer)
+      ->setObject($repository)
+      ->execute();
+
+    $view_policy = id(new AphrontFormPolicyControl())
+      ->setUser($viewer)
+      ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
+      ->setPolicyObject($repository)
+      ->setPolicies($policies)
+      ->setName('viewPolicy');
+
+    $edit_policy = id(new AphrontFormPolicyControl())
+      ->setUser($viewer)
+      ->setCapability(PhabricatorPolicyCapability::CAN_EDIT)
+      ->setPolicyObject($repository)
+      ->setPolicies($policies)
+      ->setName('editPolicy');
+
+    $push_policy = id(new AphrontFormPolicyControl())
+      ->setUser($viewer)
+      ->setCapability(DiffusionCapabilityPush::CAPABILITY)
+      ->setPolicyObject($repository)
+      ->setPolicies($policies)
+      ->setName('pushPolicy');
+
+    return id(new PHUIFormPageView())
+        ->setPageName(pht('Policies'))
+        ->setValidateFormPageCallback(array($this, 'validatePolicyPage'))
+        ->setAdjustFormPageCallback(array($this, 'adjustPolicyPage'))
+        ->setUser($viewer)
+        ->addRemarkupInstructions(
+          pht(
+            "Select access policies for this repository."))
+        ->addControl($view_policy)
+        ->addControl($edit_policy)
+        ->addControl($push_policy);
+  }
+
+  public function adjustPolicyPage(PHUIFormPageView $page) {
+    if ($this->getRepository()) {
+      $repository = $this->getRepository();
+      $show_push = $repository->isHosted();
+    } else {
+      $show_push = ($this->edit == 'create');
+    }
+
+    if (!$show_push) {
+      $c_push = $page->getControl('pushPolicy');
+      $c_push->setHidden(true);
+    }
+  }
+
+  public function validatePolicyPage(PHUIFormPageView $page) {
+    $form = $page->getForm();
+    $viewer = $this->getRequest()->getUser();
+
+    $c_view = $page->getControl('viewPolicy');
+    $c_edit = $page->getControl('editPolicy');
+    $c_push = $page->getControl('pushPolicy');
+    $v_view = $c_view->getValue();
+    $v_edit = $c_edit->getValue();
+    $v_push = $c_push->getValue();
+
+    if ($this->getRepository()) {
+      $repository = $this->getRepository();
+    } else {
+      $repository = PhabricatorRepository::initializeNewRepository($viewer);
+    }
+
+    $proxy = clone $repository;
+    $proxy->setViewPolicy($v_view);
+    $proxy->setEditPolicy($v_edit);
+
+    $can_view = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $proxy,
+      PhabricatorPolicyCapability::CAN_VIEW);
+
+    $can_edit = PhabricatorPolicyFilter::hasCapability(
+      $viewer,
+      $proxy,
+      PhabricatorPolicyCapability::CAN_EDIT);
+
+    if (!$can_view) {
+      $c_view->setError(pht('Invalid'));
+      $page->addPageError(
+        pht(
+          'You can not use the selected policy, because you would be unable '.
+          'to see the repository.'));
+    }
+
+    if (!$can_edit) {
+      $c_edit->setError(pht('Invalid'));
+      $page->addPageError(
+        pht(
+          'You can not use the selected edit policy, because you would be '.
+          'unable to edit the repository.'));
+    }
+
+    return $c_view->isValid() &&
+           $c_edit->isValid();
+  }
+
 /* -(  Page: Done  )--------------------------------------------------------- */
 
 

src/applications/diffusion/controller/DiffusionRepositoryEditPolicyController.php

@@ -1,133 +0,0 @@
-<?php
-
-final class DiffusionRepositoryEditPolicyController
-  extends DiffusionRepositoryEditController {
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $viewer = $request->getUser();
-    $drequest = $this->diffusionRequest;
-    $repository = $drequest->getRepository();
-
-    $repository = id(new PhabricatorRepositoryQuery())
-      ->setViewer($viewer)
-      ->requireCapabilities(
-        array(
-          PhabricatorPolicyCapability::CAN_VIEW,
-          PhabricatorPolicyCapability::CAN_EDIT,
-        ))
-      ->withIDs(array($repository->getID()))
-      ->executeOne();
-
-    if (!$repository) {
-      return new Aphront404Response();
-    }
-
-    $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/');
-
-    $v_view = $repository->getViewPolicy();
-    $v_edit = $repository->getEditPolicy();
-    $v_push = $repository->getPushPolicy();
-
-    if ($request->isFormPost()) {
-      $v_view = $request->getStr('viewPolicy');
-      $v_edit = $request->getStr('editPolicy');
-      $v_push = $request->getStr('pushPolicy');
-
-      $xactions = array();
-      $template = id(new PhabricatorRepositoryTransaction());
-
-      $type_view = PhabricatorTransactions::TYPE_VIEW_POLICY;
-      $type_edit = PhabricatorTransactions::TYPE_EDIT_POLICY;
-      $type_push = PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY;
-
-      $xactions[] = id(clone $template)
-        ->setTransactionType($type_view)
-        ->setNewValue($v_view);
-
-      $xactions[] = id(clone $template)
-        ->setTransactionType($type_edit)
-        ->setNewValue($v_edit);
-
-      if ($repository->isHosted()) {
-        $xactions[] = id(clone $template)
-          ->setTransactionType($type_push)
-          ->setNewValue($v_push);
-      }
-
-      id(new PhabricatorRepositoryEditor())
-        ->setContinueOnNoEffect(true)
-        ->setContentSourceFromRequest($request)
-        ->setActor($viewer)
-        ->applyTransactions($repository, $xactions);
-
-      return id(new AphrontRedirectResponse())->setURI($edit_uri);
-    }
-
-    $content = array();
-
-    $crumbs = $this->buildApplicationCrumbs();
-    $crumbs->addTextCrumb(pht('Edit Policies'));
-
-    $title = pht('Edit Policies (%s)', $repository->getName());
-
-    $policies = id(new PhabricatorPolicyQuery())
-      ->setViewer($viewer)
-      ->setObject($repository)
-      ->execute();
-
-    $form = id(new AphrontFormView())
-      ->setUser($viewer)
-      ->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
-          ->setPolicyObject($repository)
-          ->setPolicies($policies)
-          ->setName('viewPolicy'))
-      ->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(PhabricatorPolicyCapability::CAN_EDIT)
-          ->setPolicyObject($repository)
-          ->setPolicies($policies)
-          ->setName('editPolicy'));
-
-    if ($repository->isHosted()) {
-      $form->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(DiffusionCapabilityPush::CAPABILITY)
-          ->setPolicyObject($repository)
-          ->setPolicies($policies)
-          ->setName('pushPolicy'));
-    } else {
-      $form->appendChild(
-        id(new AphrontFormMarkupControl())
-          ->setLabel(pht('Can Push'))
-          ->setValue(
-            phutil_tag('em', array(), pht('Not a Hosted Repository'))));
-    }
-
-    $form
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->setValue(pht('Save Policies'))
-          ->addCancelButton($edit_uri));
-
-    $form_box = id(new PHUIObjectBoxView())
-      ->setHeaderText($title)
-      ->setForm($form);
-
-    return $this->buildApplicationPage(
-      array(
-        $crumbs,
-        $form_box,
-      ),
-      array(
-        'title' => $title,
-        'device' => true,
-      ));
-  }
-
-}

src/view/form/control/AphrontFormPolicyControl.php

@@ -124,8 +124,10 @@ final class AphrontFormPolicyControl extends AphrontFormControl {
       // TODO: Make this configurable.
       $policy = PhabricatorPolicies::POLICY_USER;
     }
-    $this->setValue($policy);
 
+    if (!$this->getValue()) {
+      $this->setValue($policy);
+    }
 
     $control_id = celerity_generate_unique_node_id();
     $input_id = celerity_generate_unique_node_id();