From 32d4ae8cb2cc5ef3c728288b4bada0a8f3b7b20e Mon Sep 17 00:00:00 2001 From: Christopher Speck Date: Sat, 10 Oct 2015 07:14:48 -0700 Subject: [PATCH 01/28] Added an intercept to Mercurial's `capabilities` command to remove bundle2. Summary: If Mercurial 3.4+ is used to host repositories in Phabricator, any clients using 3.5+ will receive an exception after the bundle is pushed up. Clients will also fail to update phases for changesets pushed up. Before directly responding to mercurial clients with all capabilities, this change filters out the 'bundle2' capability so the client negotiates using a legacy bundle wire format instead. Test Plan: Server: Mercurial 3.5 Client: Mercurial 3.4 Test with both HTTP and SSH protocols: 1. Create a local commit on client 2. Push commit to server 3. Verify the client emits something like: ``` searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 1 changes to 1 files ``` Closes T9450 Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T9450 Differential Revision: https://secure.phabricator.com/D14241 --- src/__phutil_library_map__.php | 2 + .../controller/DiffusionServeController.php | 4 +- .../DiffusionMercurialWireProtocol.php | 30 ++++++++++++ .../DiffusionMercurialWireProtocolTests.php | 48 +++++++++++++++++++ .../DiffusionMercurialServeSSHWorkflow.php | 8 +++- 5 files changed, 90 insertions(+), 2 deletions(-) create mode 100644 src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 184a484929..aaa4a1c5ac 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -618,6 +618,7 @@ phutil_register_library_map(array( 'DiffusionMercurialServeSSHWorkflow' => 'applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php', 'DiffusionMercurialWireClientSSHProtocolChannel' => 'applications/diffusion/ssh/DiffusionMercurialWireClientSSHProtocolChannel.php', 'DiffusionMercurialWireProtocol' => 'applications/diffusion/protocol/DiffusionMercurialWireProtocol.php', + 'DiffusionMercurialWireProtocolTests' => 'applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php', 'DiffusionMercurialWireSSHTestCase' => 'applications/diffusion/ssh/__tests__/DiffusionMercurialWireSSHTestCase.php', 'DiffusionMergedCommitsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionMergedCommitsQueryConduitAPIMethod.php', 'DiffusionMirrorDeleteController' => 'applications/diffusion/controller/DiffusionMirrorDeleteController.php', @@ -4341,6 +4342,7 @@ phutil_register_library_map(array( 'DiffusionMercurialServeSSHWorkflow' => 'DiffusionMercurialSSHWorkflow', 'DiffusionMercurialWireClientSSHProtocolChannel' => 'PhutilProtocolChannel', 'DiffusionMercurialWireProtocol' => 'Phobject', + 'DiffusionMercurialWireProtocolTests' => 'PhabricatorTestCase', 'DiffusionMercurialWireSSHTestCase' => 'PhabricatorTestCase', 'DiffusionMergedCommitsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionMirrorDeleteController' => 'DiffusionController', diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index a76acd704c..271ab36e91 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -536,12 +536,14 @@ final class DiffusionServeController extends DiffusionController { $body = strlen($stderr)."\n".$stderr; } else { list($length, $body) = explode("\n", $stdout, 2); + if ($cmd == 'capabilities') { + $body = DiffusionMercurialWireProtocol::filterBundle2Capability($body); + } } return id(new DiffusionMercurialResponse())->setContent($body); } - private function getMercurialArguments() { // Mercurial sends arguments in HTTP headers. "Why?", you might wonder, // "Why would you do this?". diff --git a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php index 4f7fa6e29c..11e864e74e 100644 --- a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php +++ b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php @@ -99,4 +99,34 @@ final class DiffusionMercurialWireProtocol extends Phobject { return true; } + /** If the server version is running 3.4+ it will respond + * with 'bundle2' capability in the format of "bundle2=(url-encoding)". + * Until we maange to properly package up bundles to send back we + * disallow the client from knowing we speak bundle2 by removing it + * from the capabilities listing. + * + * The format of the capabilties string is: "a space separated list + * of strings representing what commands the server supports" + * @link https://www.mercurial-scm.org/wiki/CommandServer#Protocol + * + * @param string $capabilities - The string of capabilities to + * strip the bundle2 capability from. This is expected to be + * the space-separated list of strings resulting from the + * querying the 'capabilties' command. + * + * @return string The resulting space-separated list of capabilities + * which no longer contains the 'bundle2' capability. This is meant + * to replace the original $body to send back to client. + */ + public static function filterBundle2Capability($capabilities) { + $parts = explode(' ', $capabilities); + foreach ($parts as $key => $part) { + if (preg_match('/^bundle2=/', $part)) { + unset($parts[$key]); + break; + } + } + return implode(' ', $parts); + } + } diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php b/src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php new file mode 100644 index 0000000000..5878f5a9e2 --- /dev/null +++ b/src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php @@ -0,0 +1,48 @@ + pht('Filter bundle2 from Mercurial 3.5.1'), + 'input' => $capabilities_with_bundle2_hg_351, + 'expect' => $capabilities_without_bundle2_hg_351, + ), + + array( + 'name' => pht('Filter bundle does not affect Mercurial 2.6.2'), + 'input' => $capabilities_hg_262, + 'expect' => $capabilities_hg_262, + ), + ); + + foreach ($cases as $case) { + $actual = DiffusionMercurialWireProtocol::filterBundle2Capability( + $case['input']); + $this->assertEqual($case['expect'], $actual, $case['name']); + } + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php index 51ffdeb3bd..3f66110abc 100644 --- a/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php @@ -107,8 +107,14 @@ final class DiffusionMercurialServeSSHWorkflow $this->didSeeWrite = true; } + $raw_message = $message['raw']; + if ($command == 'capabilities') { + $raw_message = DiffusionMercurialWireProtocol::filterBundle2Capability( + $raw_message); + } + // If we're good, return the raw message data. - return $message['raw']; + return $raw_message; } } From 2f6d3119f5f6f022bae2a432c6691ec4f14f750d Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 10 Oct 2015 07:15:25 -0700 Subject: [PATCH 02/28] Rough cut of "Blueprint Authorizations" Summary: Ref T9519. This is like 80% of the way there and doesn't fully work yet, but roughly shows the shape of things to come. Here's how it works: First, there's a new custom field type for blueprints which works like a normal typeahead but has some extra logic. It's implemented this way to make it easy to add to Blueprints in Drydock and Build Plans in Harbormaster. Here, I've added a "Use Blueprints" field to the "WorkingCopy" blueprint, so you can control which hosts the working copies are permitted to allocate on: {F869865} This control has a bit of custom rendering logic. Instead of rendering a normal list of PHIDs, it renders an annotated list with icons: {F869866} These icons show whether the blueprint on the other size of the authorization has approved this object. Once you have a green checkmark, you're good to go. On the blueprint side, things look like this: {F869867} This table shows all the objects which have asked for access to this blueprint. In this case it's showing that one object is approved to use the blueprint since I already approved it, but by default new requests come in here as "Authorization Requested" and someone has to go approve them. You approve them from within the authorization detail screen: {F869868} You can use the "Approve" or "Decline" buttons to allow or prevent use of the blueprint. This doesn't actually do anything yet -- objects don't need to be authorized in order to use blueprints quite yet. That will come in the next diff, I just wanted to get the UI in reasonable shape first. The authorization also has a second piece of state, which is whether the request from the object is active or inactive. We use this to keep track of the authorization if the blueprint is (maybe temporarily) deleted. For example, you might have a Build Plan that uses Blueprints A and B. For a couple days, you only want to use A, so you remove B from the "Use Blueprints: ..." field. Later, you can add B back and it will connect to its old authorization again, so you don't need to go re-approve things (and if you're declined, you stay declined instead of being able to request authorization over and over again). This should make working with authorizations a little easier and less labor intensive. Stuff not in this diff: - Actually preventing any allocations (next diff). - Probably should have transactions for approve/decline, at least, at some point, so there's a log of who did approvals and when. - Maybe should have a more clear/loud error state when no blueprints are approved? - Should probably restrict the typeahead to specific blueprint types. Test Plan: - Added the field. - Typed some stuff into it. - Saw the UI update properly. - Approved an authorization. - Declined an authorization. - Saw active authorizations on a blueprint page. - Didn't see any inactive authroizations there. - Clicked "View All Authorizations", saw all authorizations. Reviewers: chad, hach-que Reviewed By: chad Maniphest Tasks: T9519 Differential Revision: https://secure.phabricator.com/D14251 --- .../autopatches/20151009.drydock.auth.1.sql | 14 ++ src/__phutil_library_map__.php | 21 +++ .../PhabricatorDrydockApplication.php | 9 ++ ...dockWorkingCopyBlueprintImplementation.php | 10 ++ ...rydockAuthorizationAuthorizeController.php | 95 +++++++++++ .../DrydockAuthorizationListController.php | 87 +++++++++++ .../DrydockAuthorizationViewController.php | 141 +++++++++++++++++ .../DrydockBlueprintViewController.php | 71 ++++++++- .../drydock/controller/DrydockController.php | 2 +- .../DrydockResourceViewController.php | 2 +- .../DrydockBlueprintCoreCustomField.php | 5 - .../phid/DrydockAuthorizationPHIDType.php | 37 +++++ .../drydock/phid/DrydockBlueprintPHIDType.php | 7 +- .../query/DrydockAuthorizationQuery.php | 146 +++++++++++++++++ .../DrydockAuthorizationSearchEngine.php | 87 +++++++++++ .../drydock/storage/DrydockAuthorization.php | 122 +++++++++++++++ .../view/DrydockAuthorizationListView.php | 65 ++++++++ ...abricatorStandardCustomFieldBlueprints.php | 147 ++++++++++++++++++ .../PhabricatorStandardCustomFieldPHIDs.php | 2 +- 19 files changed, 1059 insertions(+), 11 deletions(-) create mode 100644 resources/sql/autopatches/20151009.drydock.auth.1.sql create mode 100644 src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php create mode 100644 src/applications/drydock/controller/DrydockAuthorizationListController.php create mode 100644 src/applications/drydock/controller/DrydockAuthorizationViewController.php create mode 100644 src/applications/drydock/phid/DrydockAuthorizationPHIDType.php create mode 100644 src/applications/drydock/query/DrydockAuthorizationQuery.php create mode 100644 src/applications/drydock/query/DrydockAuthorizationSearchEngine.php create mode 100644 src/applications/drydock/storage/DrydockAuthorization.php create mode 100644 src/applications/drydock/view/DrydockAuthorizationListView.php create mode 100644 src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php diff --git a/resources/sql/autopatches/20151009.drydock.auth.1.sql b/resources/sql/autopatches/20151009.drydock.auth.1.sql new file mode 100644 index 0000000000..8e68977492 --- /dev/null +++ b/resources/sql/autopatches/20151009.drydock.auth.1.sql @@ -0,0 +1,14 @@ +CREATE TABLE {$NAMESPACE}_drydock.drydock_authorization ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + blueprintPHID VARBINARY(64) NOT NULL, + blueprintAuthorizationState VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}, + objectPHID VARBINARY(64) NOT NULL, + objectAuthorizationState VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (phid), + UNIQUE KEY `key_unique` (objectPHID, blueprintPHID), + KEY `key_blueprint` (blueprintPHID, blueprintAuthorizationState), + KEY `key_object` (objectPHID, objectAuthorizationState) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index aaa4a1c5ac..ae2187265f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -799,6 +799,14 @@ phutil_register_library_map(array( 'DoorkeeperTagsController' => 'applications/doorkeeper/controller/DoorkeeperTagsController.php', 'DrydockAlmanacServiceHostBlueprintImplementation' => 'applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php', 'DrydockApacheWebrootInterface' => 'applications/drydock/interface/webroot/DrydockApacheWebrootInterface.php', + 'DrydockAuthorization' => 'applications/drydock/storage/DrydockAuthorization.php', + 'DrydockAuthorizationAuthorizeController' => 'applications/drydock/controller/DrydockAuthorizationAuthorizeController.php', + 'DrydockAuthorizationListController' => 'applications/drydock/controller/DrydockAuthorizationListController.php', + 'DrydockAuthorizationListView' => 'applications/drydock/view/DrydockAuthorizationListView.php', + 'DrydockAuthorizationPHIDType' => 'applications/drydock/phid/DrydockAuthorizationPHIDType.php', + 'DrydockAuthorizationQuery' => 'applications/drydock/query/DrydockAuthorizationQuery.php', + 'DrydockAuthorizationSearchEngine' => 'applications/drydock/query/DrydockAuthorizationSearchEngine.php', + 'DrydockAuthorizationViewController' => 'applications/drydock/controller/DrydockAuthorizationViewController.php', 'DrydockBlueprint' => 'applications/drydock/storage/DrydockBlueprint.php', 'DrydockBlueprintController' => 'applications/drydock/controller/DrydockBlueprintController.php', 'DrydockBlueprintCoreCustomField' => 'applications/drydock/customfield/DrydockBlueprintCoreCustomField.php', @@ -2946,6 +2954,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesTestCase' => 'applications/spaces/__tests__/PhabricatorSpacesTestCase.php', 'PhabricatorSpacesViewController' => 'applications/spaces/controller/PhabricatorSpacesViewController.php', 'PhabricatorStandardCustomField' => 'infrastructure/customfield/standard/PhabricatorStandardCustomField.php', + 'PhabricatorStandardCustomFieldBlueprints' => 'infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php', 'PhabricatorStandardCustomFieldBool' => 'infrastructure/customfield/standard/PhabricatorStandardCustomFieldBool.php', 'PhabricatorStandardCustomFieldCredential' => 'infrastructure/customfield/standard/PhabricatorStandardCustomFieldCredential.php', 'PhabricatorStandardCustomFieldDatasource' => 'infrastructure/customfield/standard/PhabricatorStandardCustomFieldDatasource.php', @@ -4537,6 +4546,17 @@ phutil_register_library_map(array( 'DoorkeeperTagsController' => 'PhabricatorController', 'DrydockAlmanacServiceHostBlueprintImplementation' => 'DrydockBlueprintImplementation', 'DrydockApacheWebrootInterface' => 'DrydockWebrootInterface', + 'DrydockAuthorization' => array( + 'DrydockDAO', + 'PhabricatorPolicyInterface', + ), + 'DrydockAuthorizationAuthorizeController' => 'DrydockController', + 'DrydockAuthorizationListController' => 'DrydockController', + 'DrydockAuthorizationListView' => 'AphrontView', + 'DrydockAuthorizationPHIDType' => 'PhabricatorPHIDType', + 'DrydockAuthorizationQuery' => 'DrydockQuery', + 'DrydockAuthorizationSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'DrydockAuthorizationViewController' => 'DrydockController', 'DrydockBlueprint' => array( 'DrydockDAO', 'PhabricatorApplicationTransactionInterface', @@ -7091,6 +7111,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesTestCase' => 'PhabricatorTestCase', 'PhabricatorSpacesViewController' => 'PhabricatorSpacesController', 'PhabricatorStandardCustomField' => 'PhabricatorCustomField', + 'PhabricatorStandardCustomFieldBlueprints' => 'PhabricatorStandardCustomFieldTokenizer', 'PhabricatorStandardCustomFieldBool' => 'PhabricatorStandardCustomField', 'PhabricatorStandardCustomFieldCredential' => 'PhabricatorStandardCustomField', 'PhabricatorStandardCustomFieldDatasource' => 'PhabricatorStandardCustomFieldTokenizer', diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php index e662fea9e6..929b4658ed 100644 --- a/src/applications/drydock/application/PhabricatorDrydockApplication.php +++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php @@ -57,6 +57,8 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { 'DrydockResourceListController', 'logs/(?:query/(?P[^/]+)/)?' => 'DrydockLogListController', + 'authorizations/(?:query/(?P[^/]+)/)?' => + 'DrydockAuthorizationListController', ), 'create/' => 'DrydockBlueprintCreateController', 'edit/(?:(?P[1-9]\d*)/)?' => 'DrydockBlueprintEditController', @@ -81,6 +83,13 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { 'DrydockLogListController', ), ), + '(?Pauthorization)/' => array( + '(?P[1-9]\d*)/' => array( + '' => 'DrydockAuthorizationViewController', + '(?Pauthorize|decline)/' => + 'DrydockAuthorizationAuthorizeController', + ), + ), ), ); } diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index 30dd6fe4c5..939d6d2e9b 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -390,5 +390,15 @@ final class DrydockWorkingCopyBlueprintImplementation return $lease; } + public function getFieldSpecifications() { + return array( + 'blueprintPHIDs' => array( + 'name' => pht('Use Blueprints'), + 'type' => 'blueprints', + 'required' => true, + ), + ) + parent::getFieldSpecifications(); + } + } diff --git a/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php b/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php new file mode 100644 index 0000000000..41010ff882 --- /dev/null +++ b/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php @@ -0,0 +1,95 @@ +getViewer(); + $id = $request->getURIData('id'); + + $authorization = id(new DrydockAuthorizationQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$authorization) { + return new Aphront404Response(); + } + + $authorization_uri = $this->getApplicationURI("authorization/{$id}/"); + $is_authorize = ($request->getURIData('action') == 'authorize'); + + $state_authorized = DrydockAuthorization::BLUEPRINTAUTH_AUTHORIZED; + $state_declined = DrydockAuthorization::BLUEPRINTAUTH_DECLINED; + + $state = $authorization->getBlueprintAuthorizationState(); + $can_authorize = ($state != $state_authorized); + $can_decline = ($state != $state_declined); + + if ($is_authorize && !$can_authorize) { + return $this->newDialog() + ->setTitle(pht('Already Authorized')) + ->appendParagraph( + pht( + 'This authorization has already been approved.')) + ->addCancelButton($authorization_uri); + } + + if (!$is_authorize && !$can_decline) { + return $this->newDialog() + ->setTitle(pht('Already Declined')) + ->appendParagraph( + pht('This authorization has already been declined.')) + ->addCancelButton($authorization_uri); + } + + if ($request->isFormPost()) { + if ($is_authorize) { + $new_state = $state_authorized; + } else { + $new_state = $state_declined; + } + + $authorization + ->setBlueprintAuthorizationState($new_state) + ->save(); + + return id(new AphrontRedirectResponse())->setURI($authorization_uri); + } + + if ($is_authorize) { + $title = pht('Approve Authorization'); + $body = pht( + 'Approve this authorization? The object will be able to lease and '. + 'allocate resources created by this blueprint.'); + $button = pht('Approve Authorization'); + } else { + $title = pht('Decline Authorization'); + $body = pht( + 'Decline this authorization? The object will not be able to lease '. + 'or allocate resources created by this blueprint.'); + $button = pht('Decline Authorization'); + } + + return $this->newDialog() + ->setTitle($title) + ->appendParagraph($body) + ->addSubmitButton($button) + ->addCancelButton($authorization_uri); + } + + public function buildSideNavView() { + // TODO: Get rid of this, but it's currently required by DrydockController. + return null; + } + + public function buildApplicationMenu() { + // TODO: As above. + return null; + } + +} diff --git a/src/applications/drydock/controller/DrydockAuthorizationListController.php b/src/applications/drydock/controller/DrydockAuthorizationListController.php new file mode 100644 index 0000000000..164ca8e5cc --- /dev/null +++ b/src/applications/drydock/controller/DrydockAuthorizationListController.php @@ -0,0 +1,87 @@ +blueprint = $blueprint; + return $this; + } + + public function getBlueprint() { + return $this->blueprint; + } + + public function shouldAllowPublic() { + return true; + } + + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + + $engine = new DrydockAuthorizationSearchEngine(); + + $id = $request->getURIData('id'); + + $blueprint = id(new DrydockBlueprintQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$blueprint) { + return new Aphront404Response(); + } + + $this->setBlueprint($blueprint); + $engine->setBlueprint($blueprint); + + $querykey = $request->getURIData('queryKey'); + + $controller = id(new PhabricatorApplicationSearchController()) + ->setQueryKey($querykey) + ->setSearchEngine($engine) + ->setNavigation($this->buildSideNavView()); + + return $this->delegateToController($controller); + } + + public function buildSideNavView() { + $nav = new AphrontSideNavFilterView(); + $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); + + $engine = id(new DrydockAuthorizationSearchEngine()) + ->setViewer($this->getViewer()); + + $engine->setBlueprint($this->getBlueprint()); + $engine->addNavigationItems($nav->getMenu()); + + $nav->selectFilter(null); + + return $nav; + } + + protected function buildApplicationCrumbs() { + $crumbs = parent::buildApplicationCrumbs(); + + $blueprint = $this->getBlueprint(); + if ($blueprint) { + $id = $blueprint->getID(); + + $crumbs->addTextCrumb( + pht('Blueprints'), + $this->getApplicationURI('blueprint/')); + + $crumbs->addTextCrumb( + $blueprint->getBlueprintName(), + $this->getApplicationURI("blueprint/{$id}/")); + + $crumbs->addTextCrumb( + pht('Authorizations'), + $this->getApplicationURI("blueprint/{$id}/authorizations/")); + } + + return $crumbs; + } + +} diff --git a/src/applications/drydock/controller/DrydockAuthorizationViewController.php b/src/applications/drydock/controller/DrydockAuthorizationViewController.php new file mode 100644 index 0000000000..95270c4b51 --- /dev/null +++ b/src/applications/drydock/controller/DrydockAuthorizationViewController.php @@ -0,0 +1,141 @@ +getViewer(); + $id = $request->getURIData('id'); + + $authorization = id(new DrydockAuthorizationQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$authorization) { + return new Aphront404Response(); + } + + $id = $authorization->getID(); + $title = pht('Authorization %d', $id); + + $blueprint = $authorization->getBlueprint(); + $blueprint_id = $blueprint->getID(); + + $header = id(new PHUIHeaderView()) + ->setHeader($title) + ->setUser($viewer) + ->setPolicyObject($authorization); + + + $state = $authorization->getBlueprintAuthorizationState(); + $icon = DrydockAuthorization::getBlueprintStateIcon($state); + $name = DrydockAuthorization::getBlueprintStateName($state); + + $header->setStatus($icon, null, $name); + + $actions = $this->buildActionListView($authorization); + $properties = $this->buildPropertyListView($authorization); + $properties->setActionList($actions); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb( + pht('Blueprints'), + $this->getApplicationURI('blueprint/')); + $crumbs->addTextCrumb( + $blueprint->getBlueprintName(), + $this->getApplicationURI("blueprint/{$blueprint_id}/")); + $crumbs->addTextCrumb($title); + + $object_box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->addPropertyList($properties); + + return $this->buildApplicationPage( + array( + $crumbs, + $object_box, + ), + array( + 'title' => $title, + )); + + } + + private function buildActionListView(DrydockAuthorization $authorization) { + $viewer = $this->getViewer(); + $id = $authorization->getID(); + + $view = id(new PhabricatorActionListView()) + ->setUser($viewer) + ->setObjectURI($this->getRequest()->getRequestURI()) + ->setObject($authorization); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $authorization, + PhabricatorPolicyCapability::CAN_EDIT); + + $authorize_uri = $this->getApplicationURI("authorization/{$id}/authorize/"); + $decline_uri = $this->getApplicationURI("authorization/{$id}/decline/"); + + $state_authorized = DrydockAuthorization::BLUEPRINTAUTH_AUTHORIZED; + $state_declined = DrydockAuthorization::BLUEPRINTAUTH_DECLINED; + + $state = $authorization->getBlueprintAuthorizationState(); + $can_authorize = $can_edit && ($state != $state_authorized); + $can_decline = $can_edit && ($state != $state_declined); + + $view->addAction( + id(new PhabricatorActionView()) + ->setHref($authorize_uri) + ->setName(pht('Approve Authorization')) + ->setIcon('fa-check') + ->setWorkflow(true) + ->setDisabled(!$can_authorize)); + + $view->addAction( + id(new PhabricatorActionView()) + ->setHref($decline_uri) + ->setName(pht('Decline Authorization')) + ->setIcon('fa-times') + ->setWorkflow(true) + ->setDisabled(!$can_decline)); + + return $view; + } + + private function buildPropertyListView(DrydockAuthorization $authorization) { + $viewer = $this->getViewer(); + + $object_phid = $authorization->getObjectPHID(); + $handles = $viewer->loadHandles(array($object_phid)); + $handle = $handles[$object_phid]; + + $view = new PHUIPropertyListView(); + + $view->addProperty( + pht('Authorized Object'), + $handle->renderLink($handle->getFullName())); + + $view->addProperty(pht('Object Type'), $handle->getTypeName()); + + $object_state = $authorization->getObjectAuthorizationState(); + + $view->addProperty( + pht('Authorization State'), + DrydockAuthorization::getObjectStateName($object_state)); + + return $view; + } + + public function buildSideNavView() { + // TODO: Get rid of this, but it's currently required by DrydockController. + return null; + } + + public function buildApplicationMenu() { + // TODO: As above. + return null; + } + +} diff --git a/src/applications/drydock/controller/DrydockBlueprintViewController.php b/src/applications/drydock/controller/DrydockBlueprintViewController.php index 7102962600..f90dcb9d82 100644 --- a/src/applications/drydock/controller/DrydockBlueprintViewController.php +++ b/src/applications/drydock/controller/DrydockBlueprintViewController.php @@ -51,6 +51,8 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController { $resource_box = $this->buildResourceBox($blueprint); + $authorizations_box = $this->buildAuthorizationsBox($blueprint); + $timeline = $this->buildTransactionTimeline( $blueprint, new DrydockBlueprintTransactionQuery()); @@ -68,6 +70,7 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController { $crumbs, $object_box, $resource_box, + $authorizations_box, $log_box, $timeline, ), @@ -167,12 +170,78 @@ final class DrydockBlueprintViewController extends DrydockBlueprintController { ->setTag('a') ->setHref($resources_uri) ->setIconFont('fa-search') - ->setText(pht('View All Resources'))); + ->setText(pht('View All'))); return id(new PHUIObjectBoxView()) ->setHeader($resource_header) ->setObjectList($resource_list); } + private function buildAuthorizationsBox(DrydockBlueprint $blueprint) { + $viewer = $this->getViewer(); + + $limit = 25; + + // If there are pending authorizations against this blueprint, make sure + // we show them first. + + $pending_authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer($viewer) + ->withBlueprintPHIDs(array($blueprint->getPHID())) + ->withObjectStates( + array( + DrydockAuthorization::OBJECTAUTH_ACTIVE, + )) + ->withBlueprintStates( + array( + DrydockAuthorization::BLUEPRINTAUTH_REQUESTED, + )) + ->setLimit($limit) + ->execute(); + + $all_authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer($viewer) + ->withBlueprintPHIDs(array($blueprint->getPHID())) + ->withObjectStates( + array( + DrydockAuthorization::OBJECTAUTH_ACTIVE, + )) + ->withBlueprintStates( + array( + DrydockAuthorization::BLUEPRINTAUTH_REQUESTED, + DrydockAuthorization::BLUEPRINTAUTH_AUTHORIZED, + )) + ->setLimit($limit) + ->execute(); + + $authorizations = + mpull($pending_authorizations, null, 'getPHID') + + mpull($all_authorizations, null, 'getPHID'); + + $authorization_list = id(new DrydockAuthorizationListView()) + ->setUser($viewer) + ->setAuthorizations($authorizations) + ->setNoDataString( + pht('No objects have active authorizations to use this blueprint.')); + + $id = $blueprint->getID(); + $authorizations_uri = "blueprint/{$id}/authorizations/query/all/"; + $authorizations_uri = $this->getApplicationURI($authorizations_uri); + + $authorizations_header = id(new PHUIHeaderView()) + ->setHeader(pht('Active Authorizations')) + ->addActionLink( + id(new PHUIButtonView()) + ->setTag('a') + ->setHref($authorizations_uri) + ->setIconFont('fa-search') + ->setText(pht('View All'))); + + return id(new PHUIObjectBoxView()) + ->setHeader($authorizations_header) + ->setObjectList($authorization_list); + + } + } diff --git a/src/applications/drydock/controller/DrydockController.php b/src/applications/drydock/controller/DrydockController.php index 760334cbdf..67e4278ae3 100644 --- a/src/applications/drydock/controller/DrydockController.php +++ b/src/applications/drydock/controller/DrydockController.php @@ -105,7 +105,7 @@ abstract class DrydockController extends PhabricatorController { ->setTag('a') ->setHref($all_uri) ->setIconFont('fa-search') - ->setText(pht('View All Logs'))); + ->setText(pht('View All'))); return id(new PHUIObjectBoxView()) ->setHeader($log_header) diff --git a/src/applications/drydock/controller/DrydockResourceViewController.php b/src/applications/drydock/controller/DrydockResourceViewController.php index 4809cf970c..71e4a09db1 100644 --- a/src/applications/drydock/controller/DrydockResourceViewController.php +++ b/src/applications/drydock/controller/DrydockResourceViewController.php @@ -170,7 +170,7 @@ final class DrydockResourceViewController extends DrydockResourceController { ->setTag('a') ->setHref($leases_uri) ->setIconFont('fa-search') - ->setText(pht('View All Leases'))); + ->setText(pht('View All'))); $lease_list = id(new DrydockLeaseListView()) ->setUser($viewer) diff --git a/src/applications/drydock/customfield/DrydockBlueprintCoreCustomField.php b/src/applications/drydock/customfield/DrydockBlueprintCoreCustomField.php index f4e6ba3a27..9af418b4d1 100644 --- a/src/applications/drydock/customfield/DrydockBlueprintCoreCustomField.php +++ b/src/applications/drydock/customfield/DrydockBlueprintCoreCustomField.php @@ -41,11 +41,6 @@ final class DrydockBlueprintCoreCustomField $object->setDetail($key, $value); } - public function applyApplicationTransactionExternalEffects( - PhabricatorApplicationTransaction $xaction) { - return; - } - public function getBlueprintFieldValue() { return $this->getProxy()->getFieldValue(); } diff --git a/src/applications/drydock/phid/DrydockAuthorizationPHIDType.php b/src/applications/drydock/phid/DrydockAuthorizationPHIDType.php new file mode 100644 index 0000000000..e518149945 --- /dev/null +++ b/src/applications/drydock/phid/DrydockAuthorizationPHIDType.php @@ -0,0 +1,37 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $authorization = $objects[$phid]; + $id = $authorization->getID(); + + $handle->setName(pht('Drydock Authorization %d', $id)); + $handle->setURI("/drydock/authorization/{$id}/"); + } + } + +} diff --git a/src/applications/drydock/phid/DrydockBlueprintPHIDType.php b/src/applications/drydock/phid/DrydockBlueprintPHIDType.php index 86eeb7f3c5..3d19198192 100644 --- a/src/applications/drydock/phid/DrydockBlueprintPHIDType.php +++ b/src/applications/drydock/phid/DrydockBlueprintPHIDType.php @@ -28,9 +28,12 @@ final class DrydockBlueprintPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $blueprint = $objects[$phid]; $id = $blueprint->getID(); + $name = $blueprint->getBlueprintName(); - $handle->setName($blueprint->getBlueprintName()); - $handle->setURI("/drydock/blueprint/{$id}/"); + $handle + ->setName($name) + ->setFullName(pht('Blueprint %d: %s', $id, $name)) + ->setURI("/drydock/blueprint/{$id}/"); } } diff --git a/src/applications/drydock/query/DrydockAuthorizationQuery.php b/src/applications/drydock/query/DrydockAuthorizationQuery.php new file mode 100644 index 0000000000..6d2cddcf8a --- /dev/null +++ b/src/applications/drydock/query/DrydockAuthorizationQuery.php @@ -0,0 +1,146 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withBlueprintPHIDs(array $phids) { + $this->blueprintPHIDs = $phids; + return $this; + } + + public function withObjectPHIDs(array $phids) { + $this->objectPHIDs = $phids; + return $this; + } + + public function withBlueprintStates(array $states) { + $this->blueprintStates = $states; + return $this; + } + + public function withObjectStates(array $states) { + $this->objectStates = $states; + return $this; + } + + public function newResultObject() { + return new DrydockAuthorization(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function willFilterPage(array $authorizations) { + $blueprint_phids = mpull($authorizations, 'getBlueprintPHID'); + if ($blueprint_phids) { + $blueprints = id(new DrydockBlueprintQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($blueprint_phids) + ->execute(); + $blueprints = mpull($blueprints, null, 'getPHID'); + } else { + $blueprints = array(); + } + + foreach ($authorizations as $key => $authorization) { + $blueprint = idx($blueprints, $authorization->getBlueprintPHID()); + if (!$blueprint) { + $this->didRejectResult($authorization); + unset($authorizations[$key]); + continue; + } + $authorization->attachBlueprint($blueprint); + } + + $object_phids = mpull($authorizations, 'getObjectPHID'); + if ($object_phids) { + $objects = id(new PhabricatorObjectQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($object_phids) + ->execute(); + $objects = mpull($objects, null, 'getPHID'); + } else { + $objects = array(); + } + + foreach ($authorizations as $key => $authorization) { + $object = idx($objects, $authorization->getObjectPHID()); + if (!$object) { + $this->didRejectResult($authorization); + unset($authorizations[$key]); + continue; + } + $authorization->attachObject($object); + } + + return $authorizations; + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->blueprintPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'blueprintPHID IN (%Ls)', + $this->blueprintPHIDs); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + if ($this->blueprintStates !== null) { + $where[] = qsprintf( + $conn, + 'blueprintAuthorizationState IN (%Ls)', + $this->blueprintStates); + } + + if ($this->objectStates !== null) { + $where[] = qsprintf( + $conn, + 'objectAuthorizationState IN (%Ls)', + $this->objectStates); + } + + return $where; + } + +} diff --git a/src/applications/drydock/query/DrydockAuthorizationSearchEngine.php b/src/applications/drydock/query/DrydockAuthorizationSearchEngine.php new file mode 100644 index 0000000000..7aaef65650 --- /dev/null +++ b/src/applications/drydock/query/DrydockAuthorizationSearchEngine.php @@ -0,0 +1,87 @@ +blueprint = $blueprint; + return $this; + } + + public function getBlueprint() { + return $this->blueprint; + } + + public function getResultTypeDescription() { + return pht('Drydock Authorizations'); + } + + public function getApplicationClassName() { + return 'PhabricatorDrydockApplication'; + } + + public function canUseInPanelContext() { + return false; + } + + public function newQuery() { + $query = new DrydockAuthorizationQuery(); + + $blueprint = $this->getBlueprint(); + $query->withBlueprintPHIDs(array($blueprint->getPHID())); + + return $query; + } + + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + + return $query; + } + + protected function buildCustomSearchFields() { + return array(); + } + + protected function getURI($path) { + $blueprint = $this->getBlueprint(); + $id = $blueprint->getID(); + return "/drydock/blueprint/{$id}/authorizations/".$path; + } + + protected function getBuiltinQueryNames() { + return array( + 'all' => pht('All Authorizations'), + ); + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $authorizations, + PhabricatorSavedQuery $query, + array $handles) { + + $list = id(new DrydockAuthorizationListView()) + ->setUser($this->requireViewer()) + ->setAuthorizations($authorizations); + + $result = new PhabricatorApplicationSearchResultView(); + $result->setTable($list); + + return $result; + } + +} diff --git a/src/applications/drydock/storage/DrydockAuthorization.php b/src/applications/drydock/storage/DrydockAuthorization.php new file mode 100644 index 0000000000..2bb5decb14 --- /dev/null +++ b/src/applications/drydock/storage/DrydockAuthorization.php @@ -0,0 +1,122 @@ + true, + self::CONFIG_COLUMN_SCHEMA => array( + 'blueprintAuthorizationState' => 'text32', + 'objectAuthorizationState' => 'text32', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_unique' => array( + 'columns' => array('objectPHID', 'blueprintPHID'), + 'unique' => true, + ), + 'key_blueprint' => array( + 'columns' => array('blueprintPHID', 'blueprintAuthorizationState'), + ), + 'key_object' => array( + 'columns' => array('objectPHID', 'objectAuthorizationState'), + ), + ), + ) + parent::getConfiguration(); + } + + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + DrydockAuthorizationPHIDType::TYPECONST); + } + + public function attachBlueprint(DrydockBlueprint $blueprint) { + $this->blueprint = $blueprint; + return $this; + } + + public function getBlueprint() { + return $this->assertAttached($this->blueprint); + } + + public function attachObject($object) { + $this->object = $object; + return $this; + } + + public function getObject() { + return $this->assertAttached($this->object); + } + + public static function getBlueprintStateIcon($state) { + $map = array( + self::BLUEPRINTAUTH_REQUESTED => 'fa-exclamation-circle indigo', + self::BLUEPRINTAUTH_AUTHORIZED => 'fa-check-circle green', + self::BLUEPRINTAUTH_DECLINED => 'fa-times red', + ); + + return idx($map, $state, null); + } + + public static function getBlueprintStateName($state) { + $map = array( + self::BLUEPRINTAUTH_REQUESTED => pht('Requested'), + self::BLUEPRINTAUTH_AUTHORIZED => pht('Authorized'), + self::BLUEPRINTAUTH_DECLINED => pht('Declined'), + ); + + return idx($map, $state, pht('', $state)); + } + + public static function getObjectStateName($state) { + $map = array( + self::OBJECTAUTH_ACTIVE => pht('Active'), + self::OBJECTAUTH_INACTIVE => pht('Inactive'), + ); + + return idx($map, $state, pht('', $state)); + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + public function getPolicy($capability) { + return $this->getBlueprint()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getBlueprint()->hasAutomaticCapability($capability, $viewer); + } + + public function describeAutomaticCapability($capability) { + return pht( + 'An authorization inherits the policies of the blueprint it '. + 'authorizes access to.'); + } + + +} diff --git a/src/applications/drydock/view/DrydockAuthorizationListView.php b/src/applications/drydock/view/DrydockAuthorizationListView.php new file mode 100644 index 0000000000..28296b6a3a --- /dev/null +++ b/src/applications/drydock/view/DrydockAuthorizationListView.php @@ -0,0 +1,65 @@ +authorizations = $authorizations; + return $this; + } + + public function setNoDataString($string) { + $this->noDataString = $string; + return $this; + } + + public function getNoDataString() { + return $this->noDataString; + } + + public function render() { + $viewer = $this->getUser(); + + $authorizations = $this->authorizations; + + $view = new PHUIObjectItemListView(); + + $nodata = $this->getNoDataString(); + if ($nodata) { + $view->setNoDataString($nodata); + } + + $handles = $viewer->loadHandles(mpull($authorizations, 'getObjectPHID')); + + foreach ($authorizations as $authorization) { + $id = $authorization->getID(); + $object_phid = $authorization->getObjectPHID(); + $handle = $handles[$object_phid]; + + $item = id(new PHUIObjectItemView()) + ->setHref("/drydock/authorization/{$id}/") + ->setObjectName(pht('Authorization %d', $id)) + ->setHeader($handle->getFullName()); + + $item->addAttribute($handle->getTypeName()); + + $object_state = $authorization->getObjectAuthorizationState(); + $item->addAttribute( + DrydockAuthorization::getObjectStateName($object_state)); + + $state = $authorization->getBlueprintAuthorizationState(); + $icon = DrydockAuthorization::getBlueprintStateIcon($state); + $name = DrydockAuthorization::getBlueprintStateName($state); + + $item->setStatusIcon($icon, $name); + + $view->addItem($item); + } + + return $view; + } + +} diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php new file mode 100644 index 0000000000..29edd6d472 --- /dev/null +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php @@ -0,0 +1,147 @@ +getObjectPHID(); + + $old = $this->decodeValue($xaction->getOldValue()); + $new = $this->decodeValue($xaction->getNewValue()); + + $old_phids = array_fuse($old); + $new_phids = array_fuse($new); + + $rem_phids = array_diff_key($old_phids, $new_phids); + $add_phids = array_diff_key($new_phids, $old_phids); + + $altered_phids = $rem_phids + $add_phids; + + if (!$altered_phids) { + return; + } + + $authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withObjectPHIDs(array($object_phid)) + ->withBlueprintPHIDs($altered_phids) + ->execute(); + $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); + + $state_active = DrydockAuthorization::OBJECTAUTH_ACTIVE; + $state_inactive = DrydockAuthorization::OBJECTAUTH_INACTIVE; + + $state_requested = DrydockAuthorization::BLUEPRINTAUTH_REQUESTED; + + // Disable the object side of the authorization for any existing + // authorizations. + foreach ($rem_phids as $rem_phid) { + $authorization = idx($authorizations, $rem_phid); + if (!$authorization) { + continue; + } + + $authorization + ->setObjectAuthorizationState($state_inactive) + ->save(); + } + + // For new authorizations, either add them or reactivate them depending + // on the current state. + foreach ($add_phids as $add_phid) { + $needs_update = false; + + $authorization = idx($authorizations, $add_phid); + if (!$authorization) { + $authorization = id(new DrydockAuthorization()) + ->setObjectPHID($object_phid) + ->setObjectAuthorizationState($state_active) + ->setBlueprintPHID($add_phid) + ->setBlueprintAuthorizationState($state_requested); + + $needs_update = true; + } else { + $current_state = $authorization->getObjectAuthorizationState(); + if ($current_state != $state_active) { + $authorization->setObjectAuthorizationState($state_active); + $needs_update = true; + } + } + + if ($needs_update) { + $authorization->save(); + } + } + + } + + public function renderPropertyViewValue(array $handles) { + $value = $this->getFieldValue(); + if (!$value) { + return phutil_tag('em', array(), pht('No authorized blueprints.')); + } + + $object = $this->getObject(); + $object_phid = $object->getPHID(); + + // NOTE: We're intentionally letting you see the authorization state on + // blueprints you can't see because this has a tremendous potential to + // be extremely confusing otherwise. You still can't see the blueprints + // themselves, but you can know if the object is authorized on something. + + if ($value) { + $handles = $this->getViewer()->loadHandles($value); + + $authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withObjectPHIDs(array($object_phid)) + ->withBlueprintPHIDs($value) + ->execute(); + $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); + } else { + $handles = array(); + $authorizations = array(); + } + + $items = array(); + foreach ($value as $phid) { + $authorization = idx($authorizations, $phid); + if (!$authorization) { + continue; + } + + $handle = $handles[$phid]; + + $item = id(new PHUIStatusItemView()) + ->setTarget($handle->renderLink()); + + $state = $authorization->getBlueprintAuthorizationState(); + $item->setIcon( + DrydockAuthorization::getBlueprintStateIcon($state), + null, + DrydockAuthorization::getBlueprintStateName($state)); + + $items[] = $item; + } + + $status = new PHUIStatusListView(); + foreach ($items as $item) { + $status->addItem($item); + } + + return $status; + } + + + +} diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php index ecaf67caa9..c7be0f7acb 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php @@ -217,7 +217,7 @@ abstract class PhabricatorStandardCustomFieldPHIDs return array(); } - private function decodeValue($value) { + protected function decodeValue($value) { $value = json_decode($value); if (!is_array($value)) { $value = array(); From bb37ad65a2b757891cc47efb32e9a2b9d63699e0 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Sun, 11 Oct 2015 08:18:42 -0700 Subject: [PATCH 03/28] Update Differential for handleRequest Summary: Moves from processRequest to handleRequest. Test Plan: New diff, edit diff, leave comment, view list, browse revisions, etc. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14252 --- .../DifferentialCommentPreviewController.php | 16 +++---- .../DifferentialCommentSaveController.php | 14 ++---- .../DifferentialDiffViewController.php | 14 ++---- ...erentialRevisionCloseDetailsController.php | 15 ++----- .../DifferentialRevisionEditController.php | 20 +++------ .../DifferentialRevisionLandController.php | 5 +-- .../DifferentialRevisionListController.php | 10 +---- .../DifferentialRevisionViewController.php | 44 +++++++++---------- 8 files changed, 47 insertions(+), 91 deletions(-) diff --git a/src/applications/differential/controller/DifferentialCommentPreviewController.php b/src/applications/differential/controller/DifferentialCommentPreviewController.php index 66d1cc9338..706c2dcaf2 100644 --- a/src/applications/differential/controller/DifferentialCommentPreviewController.php +++ b/src/applications/differential/controller/DifferentialCommentPreviewController.php @@ -3,19 +3,13 @@ final class DifferentialCommentPreviewController extends DifferentialController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); $revision = id(new DifferentialRevisionQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->executeOne(); if (!$revision) { return new Aphront404Response(); @@ -119,7 +113,7 @@ final class DifferentialCommentPreviewController $metadata['action'] = $action; } - $draft_key = 'differential-comment-'.$this->id; + $draft_key = 'differential-comment-'.$id; $draft = id(new PhabricatorDraft()) ->setAuthorPHID($viewer->getPHID()) ->setDraftKey($draft_key) diff --git a/src/applications/differential/controller/DifferentialCommentSaveController.php b/src/applications/differential/controller/DifferentialCommentSaveController.php index 7d918a792c..831d7c90c8 100644 --- a/src/applications/differential/controller/DifferentialCommentSaveController.php +++ b/src/applications/differential/controller/DifferentialCommentSaveController.php @@ -3,15 +3,9 @@ final class DifferentialCommentSaveController extends DifferentialController { - private $id; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); if (!$request->isFormPost()) { return new Aphront400Response(); @@ -19,7 +13,7 @@ final class DifferentialCommentSaveController $revision = id(new DifferentialRevisionQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->needReviewerStatus(true) ->needReviewerAuthority(true) ->executeOne(); diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index 6ffd57396c..716a183b5b 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -2,23 +2,17 @@ final class DifferentialDiffViewController extends DifferentialController { - private $id; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); $diff = id(new DifferentialDiffQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->executeOne(); if (!$diff) { return new Aphront404Response(); diff --git a/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php index 25051bcbb5..d5a7d897a8 100644 --- a/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php +++ b/src/applications/differential/controller/DifferentialRevisionCloseDetailsController.php @@ -3,20 +3,11 @@ final class DifferentialRevisionCloseDetailsController extends DifferentialController { - private $phid; - - public function willProcessRequest(array $data) { - $this->phid = idx($data, 'phid'); - } - - public function processRequest() { - $request = $this->getRequest(); - - $viewer = $request->getUser(); - $xaction_phid = $this->phid; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $xaction = id(new PhabricatorObjectQuery()) - ->withPHIDs(array($xaction_phid)) + ->withPHIDs(array($request->getURIData('phid'))) ->setViewer($viewer) ->executeOne(); if (!$xaction) { diff --git a/src/applications/differential/controller/DifferentialRevisionEditController.php b/src/applications/differential/controller/DifferentialRevisionEditController.php index 1c664f07d2..a52538ca55 100644 --- a/src/applications/differential/controller/DifferentialRevisionEditController.php +++ b/src/applications/differential/controller/DifferentialRevisionEditController.php @@ -3,24 +3,18 @@ final class DifferentialRevisionEditController extends DifferentialController { - private $id; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); - public function willProcessRequest(array $data) { - $this->id = idx($data, 'id'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - if (!$this->id) { - $this->id = $request->getInt('revisionID'); + if (!$id) { + $id = $request->getInt('revisionID'); } - if ($this->id) { + if ($id) { $revision = id(new DifferentialRevisionQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->needRelationships(true) ->needReviewerStatus(true) ->needActiveDiffs(true) diff --git a/src/applications/differential/controller/DifferentialRevisionLandController.php b/src/applications/differential/controller/DifferentialRevisionLandController.php index e19762b44d..4f8787956f 100644 --- a/src/applications/differential/controller/DifferentialRevisionLandController.php +++ b/src/applications/differential/controller/DifferentialRevisionLandController.php @@ -11,9 +11,8 @@ final class DifferentialRevisionLandController extends DifferentialController { $this->strategyClass = $data['strategy']; } - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $revision_id = $this->revisionID; diff --git a/src/applications/differential/controller/DifferentialRevisionListController.php b/src/applications/differential/controller/DifferentialRevisionListController.php index e48b088e6b..29567a53bf 100644 --- a/src/applications/differential/controller/DifferentialRevisionListController.php +++ b/src/applications/differential/controller/DifferentialRevisionListController.php @@ -2,19 +2,13 @@ final class DifferentialRevisionListController extends DifferentialController { - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($request->getURIData('queryKey')) ->setSearchEngine(new DifferentialRevisionSearchEngine()) ->setNavigation($this->buildSideNavView()); diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 4af487252c..f748b8d9d5 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -8,15 +8,11 @@ final class DifferentialRevisionViewController extends DifferentialController { return true; } - public function willProcessRequest(array $data) { - $this->revisionID = $data['id']; - } + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $this->revisionID = $request->getURIData('id'); - public function processRequest() { - - $request = $this->getRequest(); - $user = $request->getUser(); - $viewer_is_anonymous = !$user->isLoggedIn(); + $viewer_is_anonymous = !$viewer->isLoggedIn(); $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($this->revisionID)) @@ -68,7 +64,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $repository = $revision->getRepository(); } else { $repository = id(new PhabricatorRepositoryQuery()) - ->setViewer($user) + ->setViewer($viewer) ->withPHIDs(array($repository_phid)) ->executeOne(); } @@ -117,7 +113,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision->loadCommitPHIDs(), array( $revision->getAuthorPHID(), - $user->getPHID(), + $viewer->getPHID(), )); foreach ($revision->getAttached() as $type => $phids) { @@ -130,7 +126,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision, PhabricatorCustomField::ROLE_VIEW); - $field_list->setViewer($user); + $field_list->setViewer($viewer); $field_list->readFieldsFromStorage($revision); $warning_handle_map = array(); @@ -174,7 +170,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $new = array_select_keys($changesets, $new_ids); $query = id(new DifferentialInlineCommentQuery()) - ->setViewer($user) + ->setViewer($viewer) ->needHidden(true) ->withRevisionPHIDs(array($revision->getPHID())); $inlines = $query->execute(); @@ -205,7 +201,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $commit_hashes = array_unique(array_filter($commit_hashes)); if ($commit_hashes) { $commits_for_links = id(new DiffusionCommitQuery()) - ->setViewer($user) + ->setViewer($viewer) ->withIdentifiers($commit_hashes) ->execute(); $commits_for_links = mpull( @@ -217,7 +213,7 @@ final class DifferentialRevisionViewController extends DifferentialController { } $revision_detail = id(new DifferentialRevisionDetailView()) - ->setUser($user) + ->setUser($viewer) ->setRevision($revision) ->setDiff(end($diffs)) ->setCustomFields($field_list) @@ -239,7 +235,7 @@ final class DifferentialRevisionViewController extends DifferentialController { } $revision_detail->setActions($actions); - $revision_detail->setUser($user); + $revision_detail->setUser($viewer); $revision_detail_box = $revision_detail->render(); @@ -261,7 +257,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $detail_diffs = mpull($detail_diffs, null, 'getPHID'); $buildables = id(new HarbormasterBuildableQuery()) - ->setViewer($user) + ->setViewer($viewer) ->withBuildablePHIDs(array_keys($detail_diffs)) ->withManualBuildables(false) ->needBuilds(true) @@ -311,7 +307,7 @@ final class DifferentialRevisionViewController extends DifferentialController { '/differential/changeset/?view=old', '/differential/changeset/?view=new'); - $changeset_view->setUser($user); + $changeset_view->setUser($viewer); $changeset_view->setDiff($target); $changeset_view->setRenderingReferences($rendering_references); $changeset_view->setVsMap($vs_map); @@ -323,7 +319,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $changeset_view->setTitle(pht('Diff %s', $target->getID())); $diff_history = id(new DifferentialRevisionUpdateHistoryView()) - ->setUser($user) + ->setUser($viewer) ->setDiffs($diffs) ->setSelectedVersusDiffID($diff_vs) ->setSelectedDiffID($target->getID()) @@ -331,7 +327,7 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setCommitsForLinks($commits_for_links); $local_view = id(new DifferentialLocalCommitsView()) - ->setUser($user) + ->setUser($viewer) ->setLocalCommits(idx($props, 'local:commits')) ->setCommitsForLinks($commits_for_links); @@ -352,13 +348,13 @@ final class DifferentialRevisionViewController extends DifferentialController { $toc_view = $this->buildTableOfContents( $changesets, $visible_changesets, - $target->loadCoverageMap($user)); + $target->loadCoverageMap($viewer)); $comment_form = null; if (!$viewer_is_anonymous) { $draft = id(new PhabricatorDraft())->loadOneWhere( 'authorPHID = %s AND draftKey = %s', - $user->getPHID(), + $viewer->getPHID(), 'differential-comment-'.$revision->getID()); $reviewers = array(); @@ -394,7 +390,7 @@ final class DifferentialRevisionViewController extends DifferentialController { 'comment/save/'.$revision->getID().'/'); $comment_form->setActionURI($action_uri); - $comment_form->setUser($user); + $comment_form->setUser($viewer); $comment_form->setDraft($draft); $comment_form->setReviewers(mpull($reviewers, 'getFullName', 'getPHID')); $comment_form->setCCs(mpull($ccs, 'getFullName', 'getPHID')); @@ -461,7 +457,7 @@ final class DifferentialRevisionViewController extends DifferentialController { // TODO: For now, just use this to get "Login to Comment". $page_pane->appendChild( id(new PhabricatorApplicationTransactionCommentView()) - ->setUser($user) + ->setUser($viewer) ->setRequestURI($request->getRequestURI())); } @@ -476,7 +472,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb($object_id, '/'.$object_id); - $prefs = $user->loadPreferences(); + $prefs = $viewer->loadPreferences(); $pref_filetree = PhabricatorUserPreferences::PREFERENCE_DIFF_FILETREE; if ($prefs->getPreference($pref_filetree)) { From 02f42628c3bafd3fb8d893724dc8096f9b73c450 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 12 Oct 2015 11:39:01 -0700 Subject: [PATCH 04/28] Update Harbormaster for handleRequest Summary: Updates Harbormaster for handleRequest over processRequest Test Plan: Went through various Harbormaster areas, buildables, actions. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14255 --- .../HarbormasterBuildActionController.php | 29 +++++++------------ .../HarbormasterBuildableActionController.php | 26 ++++++----------- .../HarbormasterBuildableListController.php | 10 ++----- .../HarbormasterPlanViewController.php | 1 - .../HarbormasterStepEditController.php | 2 +- 5 files changed, 22 insertions(+), 46 deletions(-) diff --git a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php index 932a498a37..d729d868b0 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php @@ -3,24 +3,15 @@ final class HarbormasterBuildActionController extends HarbormasterController { - private $id; - private $action; - private $via; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - $this->action = $data['action']; - $this->via = idx($data, 'via'); - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - $command = $this->action; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); + $action = $request->getURIData('action'); + $via = $request->getURIData('via'); $build = id(new HarbormasterBuildQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->requireCapabilities( array( PhabricatorPolicyCapability::CAN_VIEW, @@ -31,7 +22,7 @@ final class HarbormasterBuildActionController return new Aphront404Response(); } - switch ($command) { + switch ($action) { case HarbormasterBuildCommand::COMMAND_RESTART: $can_issue = $build->canRestartBuild(); break; @@ -48,7 +39,7 @@ final class HarbormasterBuildActionController return new Aphront400Response(); } - switch ($this->via) { + switch ($via) { case 'buildable': $return_uri = '/'.$build->getBuildable()->getMonogram(); break; @@ -66,14 +57,14 @@ final class HarbormasterBuildActionController $xaction = id(new HarbormasterBuildTransaction()) ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) - ->setNewValue($command); + ->setNewValue($action); $editor->applyTransactions($build, array($xaction)); return id(new AphrontRedirectResponse())->setURI($return_uri); } - switch ($command) { + switch ($action) { case HarbormasterBuildCommand::COMMAND_RESTART: if ($can_issue) { $title = pht('Really restart build?'); diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildableActionController.php index 2716befd00..9fa7d6f006 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableActionController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableActionController.php @@ -3,22 +3,14 @@ final class HarbormasterBuildableActionController extends HarbormasterController { - private $id; - private $action; - - public function willProcessRequest(array $data) { - $this->id = $data['id']; - $this->action = $data['action']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - $command = $this->action; + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); + $id = $request->getURIData('id'); + $action = $request->getURIData('action'); $buildable = id(new HarbormasterBuildableQuery()) ->setViewer($viewer) - ->withIDs(array($this->id)) + ->withIDs(array($id)) ->needBuilds(true) ->requireCapabilities( array( @@ -33,7 +25,7 @@ final class HarbormasterBuildableActionController $issuable = array(); foreach ($buildable->getBuilds() as $build) { - switch ($command) { + switch ($action) { case HarbormasterBuildCommand::COMMAND_RESTART: if ($build->canRestartBuild()) { $issuable[] = $build; @@ -69,7 +61,7 @@ final class HarbormasterBuildableActionController $xaction = id(new HarbormasterBuildableTransaction()) ->setTransactionType(HarbormasterBuildableTransaction::TYPE_COMMAND) - ->setNewValue($command); + ->setNewValue($action); $editor->applyTransactions($buildable, array($xaction)); @@ -82,14 +74,14 @@ final class HarbormasterBuildableActionController foreach ($issuable as $build) { $xaction = id(new HarbormasterBuildTransaction()) ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) - ->setNewValue($command); + ->setNewValue($action); $build_editor->applyTransactions($build, array($xaction)); } return id(new AphrontRedirectResponse())->setURI($return_uri); } - switch ($command) { + switch ($action) { case HarbormasterBuildCommand::COMMAND_RESTART: if ($issuable) { $title = pht('Really restart all builds?'); diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableListController.php b/src/applications/harbormaster/controller/HarbormasterBuildableListController.php index aac41dc57b..b7e6dcd978 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableListController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableListController.php @@ -2,19 +2,13 @@ final class HarbormasterBuildableListController extends HarbormasterController { - private $queryKey; - public function shouldAllowPublic() { return true; } - public function willProcessRequest(array $data) { - $this->queryKey = idx($data, 'queryKey'); - } - - public function processRequest() { + public function handleRequest(AphrontRequest $request) { $controller = id(new PhabricatorApplicationSearchController()) - ->setQueryKey($this->queryKey) + ->setQueryKey($request->getURIData('queryKey')) ->setSearchEngine(new HarbormasterBuildableSearchEngine()) ->setNavigation($this->buildSideNavView()); diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 95db4c52c0..680594277d 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -4,7 +4,6 @@ final class HarbormasterPlanViewController extends HarbormasterPlanController { public function handleRequest(AphrontRequest $request) { $viewer = $this->getviewer(); - $id = $request->getURIData('id'); $plan = id(new HarbormasterBuildPlanQuery()) diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index 089a801220..9e99cc6c7f 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -4,11 +4,11 @@ final class HarbormasterStepEditController extends HarbormasterController { public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); + $id = $request->getURIData('id'); $this->requireApplicationCapability( HarbormasterManagePlansCapability::CAPABILITY); - $id = $request->getURIData('id'); if ($id) { $step = id(new HarbormasterBuildStepQuery()) ->setViewer($viewer) From 44e61a239754307fab33f9c9acca6f9a81227ac7 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 12 Oct 2015 12:01:02 -0700 Subject: [PATCH 05/28] Update home for handleRequest Summary: Updates /home/ for handleRequest Test Plan: Visit /home/creat/ Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14257 --- .../home/controller/PhabricatorHomeQuickCreateController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/home/controller/PhabricatorHomeQuickCreateController.php b/src/applications/home/controller/PhabricatorHomeQuickCreateController.php index deb54072a0..40f9afeb8c 100644 --- a/src/applications/home/controller/PhabricatorHomeQuickCreateController.php +++ b/src/applications/home/controller/PhabricatorHomeQuickCreateController.php @@ -3,8 +3,8 @@ final class PhabricatorHomeQuickCreateController extends PhabricatorHomeController { - public function processRequest() { - $viewer = $this->getRequest()->getUser(); + public function handleRequest(AphrontRequest $request) { + $viewer = $this->getViewer(); $items = $this->getCurrentApplication()->loadAllQuickCreateItems($viewer); From dac16264e401247477a328230abe63a142f7e201 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 12 Oct 2015 12:02:11 -0700 Subject: [PATCH 06/28] Update metamta for handleRequest Summary: Updates metamta for handleRequest Test Plan: Unable to test this, but looks safe? Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14256 --- .../controller/PhabricatorMetaMTAMailgunReceiveController.php | 3 +-- .../PhabricatorMetaMTASendGridReceiveController.php | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php index 0c353bf543..af5ca34712 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php @@ -18,7 +18,7 @@ final class PhabricatorMetaMTAMailgunReceiveController return phutil_hashes_are_identical($sig, $hash); } - public function processRequest() { + public function handleRequest(AphrontRequest $request) { // No CSRF for Mailgun. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); @@ -28,7 +28,6 @@ final class PhabricatorMetaMTAMailgunReceiveController pht('Mail signature is not valid. Check your Mailgun API key.')); } - $request = $this->getRequest(); $user = $request->getUser(); $raw_headers = $request->getStr('headers'); diff --git a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php index 646d6ef2ae..0a5e28fcee 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php @@ -7,12 +7,10 @@ final class PhabricatorMetaMTASendGridReceiveController return false; } - public function processRequest() { + public function handleRequest(AphrontRequest $request) { // No CSRF for SendGrid. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $request = $this->getRequest(); $user = $request->getUser(); $raw_headers = $request->getStr('headers'); From 1bdf22535414e5473aea98dc7da3a25d6d4afe4e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Oct 2015 17:02:35 -0700 Subject: [PATCH 07/28] Use Drydock authorizations when acquiring leases Summary: Ref T9519. When acquiring leases on resources: - Only consider resources created by authorized blueprints. - Only consider authorized blueprints when creating new resources. - Fail with a tailored error if no blueprints are allowed. - Fail with a tailored error if missing authorizations are causing acquisition failure. One somewhat-substantial issue with this is that it's pretty hard to figure out from the Harbormaster side. Specifically, the Build step UI does not show field value anywhere, so the presence of unapproved blueprints is not communicated. This is much more clear in Drydock. I'll plan to address this in future changes to Harbormaster, since there are other related/similar issues anyway. Test Plan: {F872527} Reviewers: hach-que, chad Reviewed By: chad Maniphest Tasks: T9519 Differential Revision: https://secure.phabricator.com/D14254 --- .../autopatches/20151010.drydock.auth.2.sql | 2 + src/__phutil_library_map__.php | 4 ++ .../DrydockBlueprintImplementation.php | 3 +- ...dockWorkingCopyBlueprintImplementation.php | 5 +- .../controller/DrydockLeaseViewController.php | 8 ++++ .../DrydockLeaseNoAuthorizationsLogType.php | 26 ++++++++++ .../DrydockLeaseNoBlueprintsLogType.php | 19 ++++++++ .../DrydockManagementLeaseWorkflow.php | 19 +++++++- .../drydock/phid/DrydockBlueprintPHIDType.php | 8 ++++ .../drydock/phid/DrydockLeasePHIDType.php | 8 ++++ .../drydock/phid/DrydockResourcePHIDType.php | 8 ++++ .../drydock/query/DrydockBlueprintQuery.php | 47 +++++++++++++++++-- .../drydock/storage/DrydockLease.php | 29 ++++++++++++ .../worker/DrydockLeaseUpdateWorker.php | 41 ++++++++++++++-- .../HarbormasterBuildStepCoreCustomField.php | 5 -- .../phid/HarbormasterBuildStepPHIDType.php | 6 ++- ...easeWorkingCopyBuildStepImplementation.php | 12 ++++- 17 files changed, 232 insertions(+), 18 deletions(-) create mode 100644 resources/sql/autopatches/20151010.drydock.auth.2.sql create mode 100644 src/applications/drydock/logtype/DrydockLeaseNoAuthorizationsLogType.php create mode 100644 src/applications/drydock/logtype/DrydockLeaseNoBlueprintsLogType.php diff --git a/resources/sql/autopatches/20151010.drydock.auth.2.sql b/resources/sql/autopatches/20151010.drydock.auth.2.sql new file mode 100644 index 0000000000..14c98b8b61 --- /dev/null +++ b/resources/sql/autopatches/20151010.drydock.auth.2.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_lease + ADD authorizingPHID VARBINARY(64) NOT NULL; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ae2187265f..ddf92ee46c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -847,6 +847,8 @@ phutil_register_library_map(array( 'DrydockLeaseDestroyedLogType' => 'applications/drydock/logtype/DrydockLeaseDestroyedLogType.php', 'DrydockLeaseListController' => 'applications/drydock/controller/DrydockLeaseListController.php', 'DrydockLeaseListView' => 'applications/drydock/view/DrydockLeaseListView.php', + 'DrydockLeaseNoAuthorizationsLogType' => 'applications/drydock/logtype/DrydockLeaseNoAuthorizationsLogType.php', + 'DrydockLeaseNoBlueprintsLogType' => 'applications/drydock/logtype/DrydockLeaseNoBlueprintsLogType.php', 'DrydockLeasePHIDType' => 'applications/drydock/phid/DrydockLeasePHIDType.php', 'DrydockLeaseQuery' => 'applications/drydock/query/DrydockLeaseQuery.php', 'DrydockLeaseQueuedLogType' => 'applications/drydock/logtype/DrydockLeaseQueuedLogType.php', @@ -4611,6 +4613,8 @@ phutil_register_library_map(array( 'DrydockLeaseDestroyedLogType' => 'DrydockLogType', 'DrydockLeaseListController' => 'DrydockLeaseController', 'DrydockLeaseListView' => 'AphrontView', + 'DrydockLeaseNoAuthorizationsLogType' => 'DrydockLogType', + 'DrydockLeaseNoBlueprintsLogType' => 'DrydockLogType', 'DrydockLeasePHIDType' => 'PhabricatorPHIDType', 'DrydockLeaseQuery' => 'DrydockQuery', 'DrydockLeaseQueuedLogType' => 'DrydockLogType', diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index 0f2e0aad44..e66b1616e5 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -292,7 +292,8 @@ abstract class DrydockBlueprintImplementation extends Phobject { } protected function newLease(DrydockBlueprint $blueprint) { - return DrydockLease::initializeNewLease(); + return DrydockLease::initializeNewLease() + ->setAuthorizingPHID($blueprint->getPHID()); } protected function requireActiveLease(DrydockLease $lease) { diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index 939d6d2e9b..1d7776af14 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -118,10 +118,13 @@ final class DrydockWorkingCopyBlueprintImplementation $resource_phid = $resource->getPHID(); + $blueprint_phids = $blueprint->getFieldValue('blueprintPHIDs'); + $host_lease = $this->newLease($blueprint) ->setResourceType('host') ->setOwnerPHID($resource_phid) - ->setAttribute('workingcopy.resourcePHID', $resource_phid); + ->setAttribute('workingcopy.resourcePHID', $resource_phid) + ->setAllowedBlueprintPHIDs($blueprint_phids); $resource ->setAttribute('host.leasePHID', $host_lease->getPHID()) diff --git a/src/applications/drydock/controller/DrydockLeaseViewController.php b/src/applications/drydock/controller/DrydockLeaseViewController.php index b9cf592313..0b5eae3e12 100644 --- a/src/applications/drydock/controller/DrydockLeaseViewController.php +++ b/src/applications/drydock/controller/DrydockLeaseViewController.php @@ -116,6 +116,14 @@ final class DrydockLeaseViewController extends DrydockLeaseController { } $view->addProperty(pht('Owner'), $owner_display); + $authorizing_phid = $lease->getAuthorizingPHID(); + if ($authorizing_phid) { + $authorizing_display = $viewer->renderHandle($authorizing_phid); + } else { + $authorizing_display = phutil_tag('em', array(), pht('None')); + } + $view->addProperty(pht('Authorized By'), $authorizing_display); + $resource_phid = $lease->getResourcePHID(); if ($resource_phid) { $resource_display = $viewer->renderHandle($resource_phid); diff --git a/src/applications/drydock/logtype/DrydockLeaseNoAuthorizationsLogType.php b/src/applications/drydock/logtype/DrydockLeaseNoAuthorizationsLogType.php new file mode 100644 index 0000000000..b5a7ca1b13 --- /dev/null +++ b/src/applications/drydock/logtype/DrydockLeaseNoAuthorizationsLogType.php @@ -0,0 +1,26 @@ +getViewer(); + $authorizing_phid = idx($data, 'authorizingPHID'); + + return pht( + 'The object which authorized this lease (%s) is not authorized to use '. + 'any of the blueprints the lease lists. Approve the authorizations '. + 'before using the lease.', + $viewer->renderHandle($authorizing_phid)->render()); + } + +} diff --git a/src/applications/drydock/logtype/DrydockLeaseNoBlueprintsLogType.php b/src/applications/drydock/logtype/DrydockLeaseNoBlueprintsLogType.php new file mode 100644 index 0000000000..b835ba32ef --- /dev/null +++ b/src/applications/drydock/logtype/DrydockLeaseNoBlueprintsLogType.php @@ -0,0 +1,19 @@ +getViewer(); $resource_type = $args->getArg('type'); if (!$resource_type) { @@ -59,6 +59,23 @@ final class DrydockManagementLeaseWorkflow $lease = id(new DrydockLease()) ->setResourceType($resource_type); + $drydock_phid = id(new PhabricatorDrydockApplication())->getPHID(); + $lease->setAuthorizingPHID($drydock_phid); + + // TODO: This is not hugely scalable, although this is a debugging workflow + // so maybe it's fine. Do we even need `bin/drydock lease` in the long run? + $all_blueprints = id(new DrydockBlueprintQuery()) + ->setViewer($viewer) + ->execute(); + $allowed_phids = mpull($all_blueprints, 'getPHID'); + if (!$allowed_phids) { + throw new Exception( + pht( + 'No blueprints exist which can plausibly allocate resources to '. + 'satisfy the requested lease.')); + } + $lease->setAllowedBlueprintPHIDs($allowed_phids); + if ($attributes) { $lease->setAttributes($attributes); } diff --git a/src/applications/drydock/phid/DrydockBlueprintPHIDType.php b/src/applications/drydock/phid/DrydockBlueprintPHIDType.php index 3d19198192..e63f1294a7 100644 --- a/src/applications/drydock/phid/DrydockBlueprintPHIDType.php +++ b/src/applications/drydock/phid/DrydockBlueprintPHIDType.php @@ -8,6 +8,14 @@ final class DrydockBlueprintPHIDType extends PhabricatorPHIDType { return pht('Blueprint'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorDrydockApplication'; + } + + public function getTypeIcon() { + return 'fa-map-o'; + } + public function newObject() { return new DrydockBlueprint(); } diff --git a/src/applications/drydock/phid/DrydockLeasePHIDType.php b/src/applications/drydock/phid/DrydockLeasePHIDType.php index c3006164e5..fc921cee3a 100644 --- a/src/applications/drydock/phid/DrydockLeasePHIDType.php +++ b/src/applications/drydock/phid/DrydockLeasePHIDType.php @@ -8,6 +8,14 @@ final class DrydockLeasePHIDType extends PhabricatorPHIDType { return pht('Drydock Lease'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorDrydockApplication'; + } + + public function getTypeIcon() { + return 'fa-link'; + } + public function newObject() { return new DrydockLease(); } diff --git a/src/applications/drydock/phid/DrydockResourcePHIDType.php b/src/applications/drydock/phid/DrydockResourcePHIDType.php index 9eb85e7561..966cf35abe 100644 --- a/src/applications/drydock/phid/DrydockResourcePHIDType.php +++ b/src/applications/drydock/phid/DrydockResourcePHIDType.php @@ -8,6 +8,14 @@ final class DrydockResourcePHIDType extends PhabricatorPHIDType { return pht('Drydock Resource'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorDrydockApplication'; + } + + public function getTypeIcon() { + return 'fa-map'; + } + public function newObject() { return new DrydockResource(); } diff --git a/src/applications/drydock/query/DrydockBlueprintQuery.php b/src/applications/drydock/query/DrydockBlueprintQuery.php index 7ce5dcbe5b..169e47b4f7 100644 --- a/src/applications/drydock/query/DrydockBlueprintQuery.php +++ b/src/applications/drydock/query/DrydockBlueprintQuery.php @@ -7,6 +7,7 @@ final class DrydockBlueprintQuery extends DrydockQuery { private $blueprintClasses; private $datasourceQuery; private $disabled; + private $authorizedPHIDs; public function withIDs(array $ids) { $this->ids = $ids; @@ -33,10 +34,19 @@ final class DrydockBlueprintQuery extends DrydockQuery { return $this; } + public function withAuthorizedPHIDs(array $phids) { + $this->authorizedPHIDs = $phids; + return $this; + } + public function newResultObject() { return new DrydockBlueprint(); } + protected function getPrimaryTableAlias() { + return 'blueprint'; + } + protected function loadPage() { return $this->loadStandardPage($this->newResultObject()); } @@ -63,39 +73,66 @@ final class DrydockBlueprintQuery extends DrydockQuery { if ($this->ids !== null) { $where[] = qsprintf( $conn, - 'id IN (%Ld)', + 'blueprint.id IN (%Ld)', $this->ids); } if ($this->phids !== null) { $where[] = qsprintf( $conn, - 'phid IN (%Ls)', + 'blueprint.phid IN (%Ls)', $this->phids); } if ($this->datasourceQuery !== null) { $where[] = qsprintf( $conn, - 'blueprintName LIKE %>', + 'blueprint.blueprintName LIKE %>', $this->datasourceQuery); } if ($this->blueprintClasses !== null) { $where[] = qsprintf( $conn, - 'className IN (%Ls)', + 'blueprint.className IN (%Ls)', $this->blueprintClasses); } if ($this->disabled !== null) { $where[] = qsprintf( $conn, - 'isDisabled = %d', + 'blueprint.isDisabled = %d', (int)$this->disabled); } return $where; } + protected function shouldGroupQueryResultRows() { + if ($this->authorizedPHIDs !== null) { + return true; + } + return parent::shouldGroupQueryResultRows(); + } + + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { + $joins = parent::buildJoinClauseParts($conn); + + if ($this->authorizedPHIDs !== null) { + $joins[] = qsprintf( + $conn, + 'JOIN %T authorization + ON authorization.blueprintPHID = blueprint.phid + AND authorization.objectPHID IN (%Ls) + AND authorization.objectAuthorizationState = %s + AND authorization.blueprintAuthorizationState = %s', + id(new DrydockAuthorization())->getTableName(), + $this->authorizedPHIDs, + DrydockAuthorization::OBJECTAUTH_ACTIVE, + DrydockAuthorization::BLUEPRINTAUTH_AUTHORIZED); + } + + return $joins; + } + } diff --git a/src/applications/drydock/storage/DrydockLease.php b/src/applications/drydock/storage/DrydockLease.php index 01ccb0a5c4..af475c5d61 100644 --- a/src/applications/drydock/storage/DrydockLease.php +++ b/src/applications/drydock/storage/DrydockLease.php @@ -7,6 +7,7 @@ final class DrydockLease extends DrydockDAO protected $resourceType; protected $until; protected $ownerPHID; + protected $authorizingPHID; protected $attributes = array(); protected $status = DrydockLeaseStatus::STATUS_PENDING; @@ -141,6 +142,25 @@ final class DrydockLease extends DrydockDAO pht('Only new leases may be queued for activation!')); } + if (!$this->getAuthorizingPHID()) { + throw new Exception( + pht( + 'Trying to queue a lease for activation without an authorizing '. + 'object. Use "%s" to specify the PHID of the authorizing object. '. + 'The authorizing object must be approved to use the allowed '. + 'blueprints.', + 'setAuthorizingPHID()')); + } + + if (!$this->getAllowedBlueprintPHIDs()) { + throw new Exception( + pht( + 'Trying to queue a lease for activation without any allowed '. + 'Blueprints. Use "%s" to specify allowed blueprints. The '. + 'authorizing object must be approved to use the allowed blueprints.', + 'setAllowedBlueprintPHIDs()')); + } + $this ->setStatus(DrydockLeaseStatus::STATUS_PENDING) ->save(); @@ -376,6 +396,15 @@ final class DrydockLease extends DrydockDAO return $this; } + public function setAllowedBlueprintPHIDs(array $phids) { + $this->setAttribute('internal.blueprintPHIDs', $phids); + return $this; + } + + public function getAllowedBlueprintPHIDs() { + return $this->getAttribute('internal.blueprintPHIDs', array()); + } + private function didActivate() { $viewer = PhabricatorUser::getOmnipotentUser(); $need_update = false; diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index a93997259d..71cc13038a 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -300,11 +300,46 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { return array(); } - $blueprints = id(new DrydockBlueprintQuery()) + $query = id(new DrydockBlueprintQuery()) ->setViewer($viewer) ->withBlueprintClasses(array_keys($impls)) - ->withDisabled(false) - ->execute(); + ->withDisabled(false); + + $blueprint_phids = $lease->getAllowedBlueprintPHIDs(); + if (!$blueprint_phids) { + $lease->logEvent(DrydockLeaseNoBlueprintsLogType::LOGCONST); + return array(); + } + + // The Drydock application itself is allowed to authorize anything. This + // is primarily used for leases generated by CLI administrative tools. + $drydock_phid = id(new PhabricatorDrydockApplication())->getPHID(); + + $authorizing_phid = $lease->getAuthorizingPHID(); + if ($authorizing_phid != $drydock_phid) { + $blueprints = id(clone $query) + ->withAuthorizedPHIDs(array($authorizing_phid)) + ->execute(); + if (!$blueprints) { + // If we didn't hit any blueprints, check if this is an authorization + // problem: re-execute the query without the authorization constraint. + // If the second query hits blueprints, the overall configuration is + // fine but this is an authorization problem. If the second query also + // comes up blank, this is some other kind of configuration issue so + // we fall through to the default pathway. + $all_blueprints = $query->execute(); + if ($all_blueprints) { + $lease->logEvent( + DrydockLeaseNoAuthorizationsLogType::LOGCONST, + array( + 'authorizingPHID' => $authorizing_phid, + )); + return array(); + } + } + } else { + $blueprints = $query->execute(); + } $keep = array(); foreach ($blueprints as $key => $blueprint) { diff --git a/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php index 0ad8f960cf..296669d2b5 100644 --- a/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php +++ b/src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php @@ -67,11 +67,6 @@ final class HarbormasterBuildStepCoreCustomField $object->setDetail($key, $value); } - public function applyApplicationTransactionExternalEffects( - PhabricatorApplicationTransaction $xaction) { - return; - } - public function getBuildTargetFieldValue() { return $this->getProxy()->getFieldValue(); } diff --git a/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php index b728f0e3a7..92e1980d8c 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildStepPHIDType.php @@ -28,9 +28,13 @@ final class HarbormasterBuildStepPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $build_step = $objects[$phid]; + $id = $build_step->getID(); $name = $build_step->getName(); - $handle->setName($name); + $handle + ->setName($name) + ->setFullName(pht('Build Step %d: %s', $id, $name)) + ->setURI("/harbormaster/step/{$id}/edit/"); } } diff --git a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php index 73ea19bd7a..1bf5b33a0c 100644 --- a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php @@ -41,9 +41,14 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation()) ->getType(); + $allowed_phids = $build_target->getFieldValue('repositoryPHIDs'); + $authorizing_phid = $build_target->getBuildStep()->getPHID(); + $lease = DrydockLease::initializeNewLease() ->setResourceType($working_copy_type) - ->setOwnerPHID($build_target->getPHID()); + ->setOwnerPHID($build_target->getPHID()) + ->setAuthorizingPHID($authorizing_phid) + ->setAllowedBlueprintPHIDs($allowed_phids); $map = $this->buildRepositoryMap($build_target); @@ -104,6 +109,11 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation 'type' => 'text', 'required' => true, ), + 'blueprintPHIDs' => array( + 'name' => pht('Use Blueprints'), + 'type' => 'blueprints', + 'required' => true, + ), 'repositoryPHIDs' => array( 'name' => pht('Also Clone'), 'type' => 'datasource', From 3ff5ca789a6027cda0f0a03f71fff5e5fd905369 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Oct 2015 17:02:42 -0700 Subject: [PATCH 08/28] Fix `/tag/aa%20bb` project URIs Summary: Ref T9551. To set things up: - Name a project `aa bb`. This will have the tag `aa_bb`. - Try to visit `/tag/aa%20bb`. Here's what happens now: - You get an Aphront redirect error as it tries to add the trailing `/`. Add `phutil_escape_uri()` so that works again. - Then, you 404, even though this tag is reasonably equivalent to the real project tag and could be redirected. Add a fallback to lookup, resolve, and redirect if we can find a hit for the tag. This also fixes stuff like `/tag/AA_BB/`. Test Plan: Visited URIs like `/tag/aa%20bb`, `/tag/aa%20bb/`, `/tag/Aa_bB/`, etc. None of them worked before and now they all do. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9551 Differential Revision: https://secure.phabricator.com/D14260 --- .../AphrontApplicationConfiguration.php | 7 ++++ .../PhabricatorProjectViewController.php | 36 ++++++++++++++++++- .../PhabricatorProjectTransactionEditor.php | 4 +-- src/infrastructure/util/PhabricatorSlug.php | 6 ++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 50b8c123fd..b48ea3be13 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -372,6 +372,13 @@ abstract class AphrontApplicationConfiguration extends Phobject { $result = $this->routePath($maps, $path.'/'); if ($result) { $slash_uri = $request->getRequestURI()->setPath($path.'/'); + + // We need to restore URI encoding because the webserver has + // interpreted it. For example, this allows us to redirect a path + // like `/tag/aa%20bb` to `/tag/aa%20bb/`, which may eventually be + // resolved meaningfully by an application. + $slash_uri = phutil_escape_uri($slash_uri); + $external = strlen($request->getRequestURI()->getDomain()); return $this->buildRedirectController($slash_uri, $external); } diff --git a/src/applications/project/controller/PhabricatorProjectViewController.php b/src/applications/project/controller/PhabricatorProjectViewController.php index 45329285df..bfc9827ab5 100644 --- a/src/applications/project/controller/PhabricatorProjectViewController.php +++ b/src/applications/project/controller/PhabricatorProjectViewController.php @@ -26,10 +26,17 @@ final class PhabricatorProjectViewController } $project = $query->executeOne(); if (!$project) { + + // If this request corresponds to a project but just doesn't have the + // slug quite right, redirect to the proper URI. + $uri = $this->getNormalizedURI($slug); + if ($uri !== null) { + return id(new AphrontRedirectResponse())->setURI($uri); + } + return new Aphront404Response(); } - $columns = id(new PhabricatorProjectColumnQuery()) ->setViewer($viewer) ->withProjectPHIDs(array($project->getPHID())) @@ -53,4 +60,31 @@ final class PhabricatorProjectViewController return $this->delegateToController($controller_object); } + private function getNormalizedURI($slug) { + if (!strlen($slug)) { + return null; + } + + $normal = PhabricatorSlug::normalizeProjectSlug($slug); + if ($normal === $slug) { + return null; + } + + $viewer = $this->getViewer(); + + // Do execute() instead of executeOne() here so we canonicalize before + // raising a policy exception. This is a little more polished than letting + // the user hit the error on any variant of the slug. + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withSlugs(array($normal)) + ->execute(); + if (!$projects) { + return null; + } + + return "/tag/{$normal}/"; + } + } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index c5253da956..ecd2d5119f 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -498,9 +498,7 @@ final class PhabricatorProjectTransactionEditor PhabricatorLiskDAO $object, $name) { - $object = (clone $object); - $object->setPhrictionSlug($name); - $slug = $object->getPrimarySlug(); + $slug = PhabricatorSlug::normalizeProjectSlug($name); $slug_object = id(new PhabricatorProjectSlug())->loadOneWhere( 'slug = %s', diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php index 53330391b1..fdac30a094 100644 --- a/src/infrastructure/util/PhabricatorSlug.php +++ b/src/infrastructure/util/PhabricatorSlug.php @@ -2,6 +2,12 @@ final class PhabricatorSlug extends Phobject { + public static function normalizeProjectSlug($slug) { + $slug = str_replace('/', ' ', $slug); + $slug = self::normalize($slug); + return rtrim($slug, '/'); + } + public static function normalize($slug) { $slug = preg_replace('@/+@', '/', $slug); $slug = trim($slug, '/'); From cd8be8106be8a5713a80e7d0e7ee3642d05c6f3b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Oct 2015 17:02:58 -0700 Subject: [PATCH 09/28] Improve ruleset for generating project hashtags Summary: Ref T9551. We currently use the same logic for generating project hashtags and Phriction slugs, but should be a little more conservative with project hashtags. Stop them from generating with stuff that won't parse in a "Reviewers:" field or generally in commments (commas, colons, etc). Test Plan: Created a bunch of projects with nonsense in them and saw them generate pretty reasonable hashtags. {F873456} Reviewers: chad Reviewed By: chad Maniphest Tasks: T9551 Differential Revision: https://secure.phabricator.com/D14261 --- .../20141107.phriction.policy.2.php | 6 ++- .../patches/090.forceuniqueprojectnames.php | 15 +++---- .../conduit/ProjectQueryConduitAPIMethod.php | 2 +- .../PhabricatorProjectTransactionEditor.php | 11 +++-- .../project/query/PhabricatorProjectQuery.php | 13 ------ .../project/storage/PhabricatorProject.php | 17 +------- src/infrastructure/util/PhabricatorSlug.php | 41 +++++++++++++++++-- .../__tests__/PhabricatorSlugTestCase.php | 21 ++++++++++ 8 files changed, 78 insertions(+), 48 deletions(-) diff --git a/resources/sql/autopatches/20141107.phriction.policy.2.php b/resources/sql/autopatches/20141107.phriction.policy.2.php index a7cc6ca3e4..5e00bd7a40 100644 --- a/resources/sql/autopatches/20141107.phriction.policy.2.php +++ b/resources/sql/autopatches/20141107.phriction.policy.2.php @@ -24,12 +24,14 @@ foreach (new LiskMigrationIterator($table) as $doc) { $prefix = 'projects/'; if (($slug != $prefix) && (strncmp($slug, $prefix, strlen($prefix)) === 0)) { $parts = explode('/', $slug); - $project_slug = $parts[1].'/'; + + $project_slug = $parts[1]; + $project_slug = PhabricatorSlug::normalizeProjectSlug($project_slug); $project_slugs = array($project_slug); $project = id(new PhabricatorProjectQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPhrictionSlugs($project_slugs) + ->withSlugs($project_slugs) ->executeOne(); if ($project) { diff --git a/resources/sql/patches/090.forceuniqueprojectnames.php b/resources/sql/patches/090.forceuniqueprojectnames.php index a12de8ecfd..a3e029d50a 100644 --- a/resources/sql/patches/090.forceuniqueprojectnames.php +++ b/resources/sql/patches/090.forceuniqueprojectnames.php @@ -10,13 +10,14 @@ $projects = $table->loadAll(); $slug_map = array(); foreach ($projects as $project) { - $project->setPhrictionSlug($project->getName()); - $slug = $project->getPhrictionSlug(); - if ($slug == '/') { + $slug = PhabricatorSlug::normalizeProjectSlug($project->getName()); + + if (!strlen($slug)) { $project_id = $project->getID(); echo pht("Project #%d doesn't have a meaningful name...", $project_id)."\n"; $project->setName(trim(pht('Unnamed Project %s', $project->getName()))); } + $slug_map[$slug][] = $project->getID(); } @@ -47,8 +48,8 @@ while ($update) { foreach ($update as $key => $project) { $id = $project->getID(); $name = $project->getName(); - $project->setPhrictionSlug($name); - $slug = $project->getPhrictionSlug(); + + $slug = PhabricatorSlug::normalizeProjectSlug($name).'/'; echo pht("Updating project #%d '%s' (%s)... ", $id, $name, $slug); try { @@ -87,8 +88,8 @@ function rename_project($project, $projects) { $suffix = 2; while (true) { $new_name = $project->getName().' ('.$suffix.')'; - $project->setPhrictionSlug($new_name); - $new_slug = $project->getPhrictionSlug(); + + $new_slug = PhabricatorSlug::normalizeProjectSlug($new_name).'/'; $okay = true; foreach ($projects as $other) { diff --git a/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php b/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php index 3d059d064d..2512a9a019 100644 --- a/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php +++ b/src/applications/project/conduit/ProjectQueryConduitAPIMethod.php @@ -112,7 +112,7 @@ final class ProjectQueryConduitAPIMethod extends ProjectConduitAPIMethod { $slug_map = array(); if ($slugs) { foreach ($slugs as $slug) { - $normal = rtrim(PhabricatorSlug::normalize($slug), '/'); + $normal = PhabricatorSlug::normalizeProjectSlug($slug); foreach ($projects as $project) { if (in_array($normal, $project['slugs'])) { $slug_map[$slug] = $project['phid']; diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index ecd2d5119f..7813195b02 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -81,9 +81,9 @@ final class PhabricatorProjectTransactionEditor switch ($xaction->getTransactionType()) { case PhabricatorProjectTransaction::TYPE_NAME: - $object->setName($xaction->getNewValue()); - // TODO - this is really "setPrimarySlug" - $object->setPhrictionSlug($xaction->getNewValue()); + $name = $xaction->getNewValue(); + $object->setName($name); + $object->setPrimarySlug(PhabricatorSlug::normalizeProjectSlug($name)); return; case PhabricatorProjectTransaction::TYPE_SLUGS: return; @@ -265,9 +265,8 @@ final class PhabricatorProjectTransactionEditor $errors[] = $error; } - $slug_builder = clone $object; - $slug_builder->setPhrictionSlug($name); - $slug = $slug_builder->getPrimarySlug(); + $slug = PhabricatorSlug::normalizeProjectSlug($name); + $slug_used_already = id(new PhabricatorProjectSlug()) ->loadOneWhere('slug = %s', $slug); if ($slug_used_already && diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index e607ff2dc5..e534e1c4bf 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -7,7 +7,6 @@ final class PhabricatorProjectQuery private $phids; private $memberPHIDs; private $slugs; - private $phrictionSlugs; private $names; private $nameTokens; private $icons; @@ -50,11 +49,6 @@ final class PhabricatorProjectQuery return $this; } - public function withPhrictionSlugs(array $slugs) { - $this->phrictionSlugs = $slugs; - return $this; - } - public function withNames(array $names) { $this->names = $names; return $this; @@ -308,13 +302,6 @@ final class PhabricatorProjectQuery $this->slugs); } - if ($this->phrictionSlugs !== null) { - $where[] = qsprintf( - $conn, - 'phrictionSlug IN (%Ls)', - $this->phrictionSlugs); - } - if ($this->names !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index e5e6e19554..8bbc6e14ad 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -189,24 +189,11 @@ final class PhabricatorProject extends PhabricatorProjectDAO return $this->assertAttached($this->memberPHIDs); } - public function setPhrictionSlug($slug) { - - // NOTE: We're doing a little magic here and stripping out '/' so that - // project pages always appear at top level under projects/ even if the - // display name is "Hack / Slash" or similar (it will become - // 'hack_slash' instead of 'hack/slash'). - - $slug = str_replace('/', ' ', $slug); - $slug = PhabricatorSlug::normalize($slug); - $this->phrictionSlug = $slug; + public function setPrimarySlug($slug) { + $this->phrictionSlug = $slug.'/'; return $this; } - public function getFullPhrictionSlug() { - $slug = $this->getPhrictionSlug(); - return 'projects/'.$slug; - } - // TODO - once we sever project => phriction automagicalness, // migrate getPhrictionSlug to have no trailing slash and be called // getPrimarySlug diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php index fdac30a094..a6cf0bc70c 100644 --- a/src/infrastructure/util/PhabricatorSlug.php +++ b/src/infrastructure/util/PhabricatorSlug.php @@ -4,21 +4,54 @@ final class PhabricatorSlug extends Phobject { public static function normalizeProjectSlug($slug) { $slug = str_replace('/', ' ', $slug); - $slug = self::normalize($slug); + $slug = self::normalize($slug, $hashtag = true); return rtrim($slug, '/'); } - public static function normalize($slug) { + public static function normalize($slug, $hashtag = false) { $slug = preg_replace('@/+@', '/', $slug); $slug = trim($slug, '/'); $slug = phutil_utf8_strtolower($slug); - $slug = preg_replace("@[\\x00-\\x19#%&+=\\\\?<> ]+@", '_', $slug); + + $ban = + // Ban control characters since users can't type them and they create + // various other problems with parsing and rendering. + "\\x00-\\x19". + + // Ban characters with special meanings in URIs (and spaces), since we + // want slugs to produce nice URIs. + "#%&+=?". + " ". + + // Ban backslashes and various brackets for parsing and URI quality. + "\\\\". + "<>{}\\[\\]". + + // Ban single and double quotes since they can mess up URIs. + "'". + '"'; + + // In hashtag mode (used for Project hashtags), ban additional characters + // which cause parsing problems. + if ($hashtag) { + $ban .= '`~!@$^*,:;(|)'; + } + + $slug = preg_replace('(['.$ban.']+)', '_', $slug); $slug = preg_replace('@_+@', '_', $slug); + $parts = explode('/', $slug); + + // Convert "_-_" into "-". This is a little nicer for inputs with + // hyphens used as internal separators, and turns an input like "A - B" + // into "a-b" instead of "a_-_b"; + foreach ($parts as $key => $part) { + $parts[$key] = str_replace('_-_', '-', $part); + } + // Remove leading and trailing underscores from each component, if the // component has not been reduced to a single underscore. For example, "a?" // converts to "a", but "??" converts to "_". - $parts = explode('/', $slug); foreach ($parts as $key => $part) { if ($part != '_') { $parts[$key] = trim($part, '_'); diff --git a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php index 0f96a735e6..e493ab7363 100644 --- a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php +++ b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php @@ -34,6 +34,9 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { 'a/??/c' => 'a/_/c/', 'a/?b/c' => 'a/b/c/', 'a/b?/c' => 'a/b/c/', + 'a - b' => 'a-b/', + 'a[b]' => 'a_b/', + 'ab!' => 'ab!/', ); foreach ($slugs as $slug => $normal) { @@ -44,6 +47,24 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { } } + public function testProjectSlugs() { + $slugs = array( + 'a:b' => 'a_b', + 'a!b' => 'a_b', + 'a - b' => 'a-b', + '' => '', + 'Demonology: HSA (Hexes, Signs, Alchemy)' => + 'demonology_hsa_hexes_signs_alchemy', + ); + + foreach ($slugs as $slug => $normal) { + $this->assertEqual( + $normal, + PhabricatorSlug::normalizeProjectSlug($slug), + pht('Hashtag normalization of "%s"', $slug)); + } + } + public function testSlugAncestry() { $slugs = array( '/' => array(), From 812c41a18abdd94be85f6902977ee3cf44b7eaf7 Mon Sep 17 00:00:00 2001 From: Christopher Speck Date: Mon, 12 Oct 2015 17:50:26 -0700 Subject: [PATCH 10/28] Conditionally use `hg files` vs. `hg locate` depending on version of Mercurial Summary: In Mercurial 3.2 the `locate` command was deprecated in favor of `files` command. This change updates the DiffusionLowLevelMercurialPathsQuery command to conditionally use `locate` or `files` based on the version of Mercurial used. Closes T7375 Test Plan: My test/develop Phabricator instance is setup to run Mercurial 3.5.1. The test procedure to verify valid file listings are being returned: 1. I navigated to `http://192.168.0.133/conduit/method/diffusion.querypaths/` 2. I populated the following fields: - path: `"/"` - commit: `"d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"` - callsign: `"HGTEST"` 3. I submitted request and verified that result contained all files in the repository: ``` { "0": "README", "1": "alpha/beta/trifle", "2": "test/Chupacabra.cow", "3": "test/socket.ks" } ``` I repeated the above steps after setting up Mercurial 2.6.2, which I installed in the following manner: 1. I downloaded Mercurial 2.6.2 source and run `make local` which will only compile it to work from its own directory (`/opt/mercurial-2.6.2`) 2. I linked `/usr/local/bin/hg -> /opt/mercurial-2.6.2/hg` (there's also a `/usr/bin/hg` which is a link to `/usr/local/bin/hg`) 3. I navigated to my home directory and verify that `hg --version` returns 2.6.2. 4. I restarted phabricator services (probably unnecessary). With the Multimeter application active 1. I verified that `/usr/local/bin/hg` referred to version 2.6 2. I ran the same conduit call from the conduit application 3. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg locate`. 4. I swapped out mercurial versions for 3.5.1 5. I ran the same conduit call from the conduit application 6. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg files` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T7375 Differential Revision: https://secure.phabricator.com/D14253 --- src/__phutil_library_map__.php | 2 ++ .../DiffusionLowLevelMercurialPathsQuery.php | 9 +++++- ...fusionLowLevelMercurialPathsQueryTests.php | 31 +++++++++++++++++++ .../multimeter/data/MultimeterControl.php | 1 + .../PhabricatorRepositoryVersion.php | 18 +++++++++++ 5 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ddf92ee46c..bc8b6a17d0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -607,6 +607,7 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php', 'DiffusionLowLevelMercurialBranchesQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php', 'DiffusionLowLevelMercurialPathsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php', + 'DiffusionLowLevelMercurialPathsQueryTests' => 'applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php', 'DiffusionLowLevelParentsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php', 'DiffusionLowLevelQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelQuery.php', 'DiffusionLowLevelResolveRefsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php', @@ -4342,6 +4343,7 @@ phutil_register_library_map(array( 'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelMercurialPathsQuery' => 'DiffusionLowLevelQuery', + 'DiffusionLowLevelMercurialPathsQueryTests' => 'PhabricatorTestCase', 'DiffusionLowLevelParentsQuery' => 'DiffusionLowLevelQuery', 'DiffusionLowLevelQuery' => 'Phobject', 'DiffusionLowLevelResolveRefsQuery' => 'DiffusionLowLevelQuery', diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php index aca82df79f..12b9e661d7 100644 --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php @@ -24,10 +24,17 @@ final class DiffusionLowLevelMercurialPathsQuery $path = $this->path; $commit = $this->commit; + $hg_paths_command = 'locate --print0 --rev %s -I %s'; + $hg_version = PhabricatorRepositoryVersion::getMercurialVersion(); + if (PhabricatorRepositoryVersion::isMercurialFilesCommandAvailable( + $hg_version)) { + $hg_paths_command = 'files --print0 --rev %s -I %s'; + } + $match_against = trim($path, '/'); $prefix = trim('./'.$match_against, '/'); list($entire_manifest) = $repository->execxLocalCommand( - 'locate --print0 --rev %s -I %s', + $hg_paths_command, hgsprintf('%s', $commit), $prefix); return explode("\0", $entire_manifest); diff --git a/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php b/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php new file mode 100644 index 0000000000..075ca2f1a5 --- /dev/null +++ b/src/applications/diffusion/query/lowlevel/__tests__/DiffusionLowLevelMercurialPathsQueryTests.php @@ -0,0 +1,31 @@ + pht('Versions which should not use `files`'), + 'versions' => array('2.6.2', '2.9', '3.1'), + 'match' => false, + ), + + array( + 'name' => pht('Versions which should use `files`'), + 'versions' => array('3.2', '3.3', '3.5.2'), + 'match' => true, + ), + ); + + foreach ($cases as $case) { + foreach ($case['versions'] as $version) { + $actual = PhabricatorRepositoryVersion + ::isMercurialFilesCommandAvailable($version); + $expect = $case['match']; + $this->assertEqual($expect, $actual, $case['name']); + } + } + } + +} diff --git a/src/applications/multimeter/data/MultimeterControl.php b/src/applications/multimeter/data/MultimeterControl.php index 1b01ce2e35..6658319469 100644 --- a/src/applications/multimeter/data/MultimeterControl.php +++ b/src/applications/multimeter/data/MultimeterControl.php @@ -265,6 +265,7 @@ final class MultimeterControl extends Phobject { 'init' => true, 'diff' => true, 'cat' => true, + 'files' => true, ), 'svnadmin' => array( 'create' => true, diff --git a/src/applications/repository/constants/PhabricatorRepositoryVersion.php b/src/applications/repository/constants/PhabricatorRepositoryVersion.php index a68e07d56a..5f722fa40a 100644 --- a/src/applications/repository/constants/PhabricatorRepositoryVersion.php +++ b/src/applications/repository/constants/PhabricatorRepositoryVersion.php @@ -19,4 +19,22 @@ final class PhabricatorRepositoryVersion extends Phobject { return null; } + /** + * The `locate` command is deprecated as of Mercurial 3.2, to be + * replaced with `files` command, which supports most of the same + * arguments. This determines whether the new `files` command should + * be used instead of the `locate` command. + * + * @param string $mercurial_version - The current version of mercurial + * which can be retrieved by calling: + * PhabricatorRepositoryVersion::getMercurialVersion() + * + * @return boolean True if the version of Mercurial is new enough to support + * the `files` command, or false if otherwise. + */ + public static function isMercurialFilesCommandAvailable($mercurial_version) { + $min_version_for_files = '3.2'; + return version_compare($mercurial_version, $min_version_for_files, '>='); + } + } From 3f3626c11a04736181dac8e9b0962b34338cb2de Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Oct 2015 17:54:11 -0700 Subject: [PATCH 11/28] Write some documentation about Drydock security and repository automation Summary: Ref T182. Ref T9519. Some of what this describes doesn't exist yet, but should soon. Test Plan: Read documentation. Reviewers: chad Reviewed By: chad Maniphest Tasks: T182, T9519 Differential Revision: https://secure.phabricator.com/D14258 --- src/docs/user/userguide/drydock.diviner | 9 + .../drydock_repository_automation.diviner | 39 ++++ .../user/userguide/drydock_security.diviner | 209 ++++++++++++++++++ 3 files changed, 257 insertions(+) create mode 100644 src/docs/user/userguide/drydock_repository_automation.diviner create mode 100644 src/docs/user/userguide/drydock_security.diviner diff --git a/src/docs/user/userguide/drydock.diviner b/src/docs/user/userguide/drydock.diviner index a03346f1cb..49b1f1df9e 100644 --- a/src/docs/user/userguide/drydock.diviner +++ b/src/docs/user/userguide/drydock.diviner @@ -58,3 +58,12 @@ a corresponding resource by either finding a suitable unused resource or creating a new resource. When work completes, the resource is returned to the resource pool or destroyed. +Next Steps +========== + +Continue by: + + - understanding Drydock security concerns with + @{article:Drydock User Guide: Security}; or + - allowing Phabricator to write to repositories with + @{article:Drydock User Guide: Repository Automation}. diff --git a/src/docs/user/userguide/drydock_repository_automation.diviner b/src/docs/user/userguide/drydock_repository_automation.diviner new file mode 100644 index 0000000000..a16e8d33e9 --- /dev/null +++ b/src/docs/user/userguide/drydock_repository_automation.diviner @@ -0,0 +1,39 @@ +@title Drydock User Guide: Repository Automation +@group userguide + +Configuring repository automation so Phabricator can push commits. + + +Overview +======== + +IMPORTANT: This feature is very new and most of the capabilities described +in this document are not yet available. This feature as a whole is a prototype. + +By configuring Drydock and Diffusion appropriately, you can enable **Repository +Automation** for a repository. Once automation is set up, Phabricator will be +able to make changes to the repository. + + +Security +======== + +Configuring repository automation amounts to telling Phabricator where it +should perform working copy operations (like merges, cherry-picks and pushes) +when doing writes. + +Depending on how stringent you are about change control, you may want to +make sure these processes are isolated and can not be tampered with. If you +run tests and automation on the same hardware, tests may be able to interfere +with automation. You can read more about this in +@{article:Drydock User Guide: Security}. + + +Next Steps +========== + +Continue by: + + - understanding Drydock security concerns with + @{article:Drydock User Guide: Security}; or + - returning to the @{article:Drydock User Guide}. diff --git a/src/docs/user/userguide/drydock_security.diviner b/src/docs/user/userguide/drydock_security.diviner new file mode 100644 index 0000000000..f12586ab35 --- /dev/null +++ b/src/docs/user/userguide/drydock_security.diviner @@ -0,0 +1,209 @@ +@title Drydock User Guide: Security +@group userguide + +Understanding security concerns in Drydock. + +Overview +======== + +Different applications use Drydock for different things, and some of the things +they do with Drydock require different levels of trust and access. It is +important to configure Drydock properly so that less trusted code can't do +anything you aren't comfortable with. + +For example, running unit tests on Drydock normally involves running relatively +untrusted code (it often has a single author and has not yet been reviewed) +that needs very few capabilities (generally, it only needs to be able to report +results back to Phabricator). In contrast, automating merge requests on Drydock +involves running trusted code that needs more access (it must be able to write +to repositories). + +Drydock allows resources to be shared and reused, so it's possible to configure +Drydock in a way that gives untrusted code a lot of access by accident. + +One way Drydock makes allocations faster is by sharing, reusing, and recycling +resources. When an application asks Drydock for a working copy, it will try to +satisfy the request by cleaning up a suitable existing working copy if it can, +instead of building a new one. This is faster, but it means that tasks have +some ability to interact or interfere with each other. + +Similarly, Drydock may allocate multiple leases on the same host at the same +time, running as the same user. This is generally simpler to configure and less +wasteful than fully isolating leases, but means that they can interact. + +Depending on your organization, environment and use cases, you might not want +this, and it may be important that different use cases are unable to interfere +with each other. For example, you might want to prevent unit tests from writing +to repositories. + +**Drydock does not guarantee that resources are isolated by default**. When +resources are more isolated, they are usually also harder to configure and +slower to allocate. Because most installs will want to find a balance between +isolation and complexity/performance, Drydock does not make assumptions about +either isolation or performance having absolute priority. + +You'll usually want to isolate things just enough that nothing bad can happen. +Fortunately, this is straightforward. This document describes how to make sure +you have enough isolation so that nothing you're uncomfortable with can occur. + + +Choosing an Isolation Policy +============================ + +This section provides some reasonable examples of ways you might approach +configuring Drydock. + +| Isolation | Suitable For | Description +|-----------|-----|------- +| Zero | Development | Everything on one host. +| Low | Small Installs | Use a dedicated Drydock host. +| High | Most Installs | **Recommended**. Use low-trust and high-trust pools. +| Custom | Special Requirements | Use multiple pools. +| Absolute | Special Requirements | Completely isolate all resources. + +**Zero Isolation**: Run Drydock operations on the same host that Phabricator +runs on. This is only suitable for developing or testing Phabricator. Any +Drydock operation can potentially compromise Phabricator. It is intentionally +difficult to configure Drydock to operate in this mode. Running Drydock +operations on the Phabricator host is strongly discouraged. + +**Low Isolation**: Designate a separate Drydock host and run Drydock +operations on it. This is suitable for small installs and provides a reasonable +level of isolation. However, it will allow unit tests (which often run +lower-trust code) to interfere with repository automation operations. + +**High Isolation**: Designate two Drydock host pools and run low-trust +operations (like builds) on one pool and high-trust operations (like repository +automation) on a separate pool. This provides a good balance between isolation +and performance, although tests can still potentially interfere with the +execution of unrelated tests. + +**Custom Isolation**: You can continue adding pools to refine the resource +isolation model. For example, you may have higher-trust and lower-trust +repositories or do builds on a mid-trust tier which runs only reviewed code. + +**Absolute Isolation**: Configure blueprints to completely initialize and +destroy hosts or containers on every request, and limit all resources to one +simultaneous lease. This will completely isolate every operation, but come at +a high performance and complexity cost. + +NOTE: It is not currently possible to configure Drydock in an absolute +isolation mode. + +It is usually reasonable to choose one of these approaches as a starting point +and then adjust it to fit your requirements. You can also evolve your use of +Drydock over time as your needs change. + + +Threat Scenarios +================ + +This section will help you understand the threats to a Drydock environment. +Not all threats will be concerning to all installs, and you can choose an +approach which defuses only the threats you care about. + +Attackers have three primary targets: + + - capturing hosts; + - compromising Phabricator; and + - compromising the integrity of other Drydock processes. + +**Attacks against hosts** are the least sophisticated. In this scenario, an +attacker wants to run a program like a Bitcoin miner or botnet client on +hardware that they aren't paying for or which can't be traced to them. They +write a "unit test" or which launches this software, then send a revision +containing this "unit test" for review. If Phabricator is configured to +automatically run tests on new revisions, it may execute automatically and give +the attacker access to computing resources they did not previously control and +which can not easily be traced back to them. + +This is usually only a meaningful threat for open source installs, because +there is a high probability of eventual detection and the value of these +resources is small, so employees will generally not have an incentive to +attempt this sort of attack. The easiest way to prevent this attack is to +prevent untrusted, anonymous contributors from running tests. For example, +create a "Trusted Contributors" project and only run tests if a revision author +is a member of the project. + +**Attacks against Phabricator** are more sophisticated. In this scenario, an +attacker tries to compromise Phabricator itself (for example, to make themselves +an administrator or gain access to an administrator account). + +This is made possible if Drydock is running on the same host as Phabricator or +runs on a privileged subnet with access to resources like Phabricator database +hosts. Most installs should be concerned about this attack. + +The best way to defuse this attack is to run Drydock processes on a separate +host which is not on a privileged subnet. For example, use a +`build.mycompany.com` host or pool for Drydock processes, separate from your +`phabricator.mycompany.com` host or pool. + +Even if the host is not privileged, many Drydock processes have some level of +privilege (enabling them to clone repositories, or report test results back to +Phabricator). Be aware that tests can hijack credentials they are run with, +and potentialy hijack credentials given to other processes on the same hosts. +You should use credentials with a minimum set of privileges and assume all +processes on a host have the highest level of access that any process on the +host has. + +**Attacks against Drydock** are the most sophisticated. In this scenario, an +attacker uses one Drydock process to compromise a different process: for +example, a unit test which tampers with a merge or injects code into a build. +This might allow an attacker to make changes to a repository or binary without +going through review or triggering other rules which would normally detect the +change. + +These attackers could also make failing tests appear to pass, or break tests or +builds, but these attacks are generally less interesting than tampering with +a repository or binary. + +This is a complex attack which you may not have to worry about unless you have +a high degree of process and control in your change pipeline. If users can push +changes directly to repositories, this often represents a faster and easier way +to achieve the same tampering. + +The best way to defuse this attack is to prevent high-trust (repository +automation) processes from running on the same hosts as low-trust (unit test) +processes. For example, use an `automation.mycompany.com` host or pool for +repository automation, and a `build.mycompany.com` host or pool for tests. + + +Applying an Isolation Policy +============================ + +Designing a security and isolation policy for Drydock can take some thought, +but applying it is straightforward. Applications which want to use Drydock must +explicitly list which blueprints they are allowed to use, and they must be +approved to use them in Drydock. By default, nothing can do anything, which is +very safe and secure. + +To get builds or automation running on a host, specify the host blueprint as a +usable blueprint in the build step or repository configuration. This creates a +new authorization request in Drydock which must be approved before things can +move forward. + +Until the authorization is approved, the process can not use the blueprint to +create any resources, nor can it use resources previously created by the +blueprint. + +You can review and approve requests from the blueprint detail view in Drydock: +find the request and click {nav Approve Authorization}. You can also revoke +approval at any time from this screen which will prevent the object from +continuing to use the blueprint (but note that this does not release any +existing leases). + +Once the authorization request is approved, the build or automation process +should be able to run if everything else is configured properly. + +Note that authorizations are transitive: if a build step is authorized to use +blueprint A, and blueprint A is authorized to use blueprint B, the build step +may indirectly operate on resources created by blueprint B. This should +normally be consistent with expectations. + + +Next Steps +========== + +Continue by: + + - returning to the @{article:Drydock User Guide}. From 0b6c0310424b15b9dfc0a2feab314537f5693454 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Oct 2015 08:41:49 -0700 Subject: [PATCH 12/28] Work around an issue with custom "users" fields in Maniphest Summary: Fixes T9558. The recent changes to validate PHID fields don't work cleanly with this gross hack. This can probably be unwound now but it will definitely get fixed in T9132 so I may just wait for that. Test Plan: Edited a custom "users" field in Maniphest. This should only affect Maniphest because it has a weird hack. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9558 Differential Revision: https://secure.phabricator.com/D14264 --- .../maniphest/controller/ManiphestTaskEditController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 703bb61768..5c4a3b4862 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -198,7 +198,8 @@ final class ManiphestTaskEditController extends ManiphestController { // in a meaningful way. For now, build User objects. Once the Maniphest // objects exist, this will switch over automatically. This is a big // hack but shouldn't be long for this world. - $placeholder_editor = new PhabricatorUserProfileEditor(); + $placeholder_editor = id(new PhabricatorUserProfileEditor()) + ->setActor($viewer); $field_errors = $aux_field->validateApplicationTransactions( $placeholder_editor, From 6ff1354ac1ff053532e85660f74216a9b57be5cd Mon Sep 17 00:00:00 2001 From: Chad Little Date: Tue, 13 Oct 2015 09:09:07 -0700 Subject: [PATCH 13/28] Fix errors when mentioning others in Ponder Summary: Fixes T9552. We need to set a questionID and the question object (for policy) when initializing a new Answer. Test Plan: Write an answer that mentions another user. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9552 Differential Revision: https://secure.phabricator.com/D14263 --- .../ponder/controller/PonderAnswerSaveController.php | 2 +- src/applications/ponder/storage/PonderAnswer.php | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/applications/ponder/controller/PonderAnswerSaveController.php b/src/applications/ponder/controller/PonderAnswerSaveController.php index 3953fed23f..cac17acbed 100644 --- a/src/applications/ponder/controller/PonderAnswerSaveController.php +++ b/src/applications/ponder/controller/PonderAnswerSaveController.php @@ -32,7 +32,7 @@ final class PonderAnswerSaveController extends PonderController { return id(new AphrontDialogResponse())->setDialog($dialog); } - $answer = PonderAnswer::initializeNewAnswer($viewer); + $answer = PonderAnswer::initializeNewAnswer($viewer, $question); // Question Editor diff --git a/src/applications/ponder/storage/PonderAnswer.php b/src/applications/ponder/storage/PonderAnswer.php index 2c8aff23da..722fee8eec 100644 --- a/src/applications/ponder/storage/PonderAnswer.php +++ b/src/applications/ponder/storage/PonderAnswer.php @@ -26,15 +26,18 @@ final class PonderAnswer extends PonderDAO private $userVotes = array(); - public static function initializeNewAnswer(PhabricatorUser $actor) { + public static function initializeNewAnswer( + PhabricatorUser $actor, + PonderQuestion $question) { $app = id(new PhabricatorApplicationQuery()) ->setViewer($actor) ->withClasses(array('PhabricatorPonderApplication')) ->executeOne(); return id(new PonderAnswer()) - ->setQuestionID(0) + ->setQuestionID($question->getID()) ->setContent('') + ->attachQuestion($question) ->setAuthorPHID($actor->getPHID()) ->setVoteCount(0) ->setStatus(PonderAnswerStatus::ANSWER_STATUS_VISIBLE); From df5a031b54a5691f5665e70e58e7ef1d75f0aee0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Oct 2015 15:45:59 -0700 Subject: [PATCH 14/28] Allow "Repository Automation" to be configured for repositories Summary: Ref T182. This allows you to assign blueprints that a repository can use to perform working copy operations. Eventually, this will support "merge this" in Differential, etc. This is just UI for now, with no material effects. Most of this diff is just taking logic that was in the existing "Blueprints" CustomField and putting it in more general places so Diffusion (which does not use CustomFields) can also access it. Test Plan: - Configured repository automation for a repository. - Removed repository automation for a repository. Reviewers: chad Reviewed By: chad Subscribers: avivey Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14259 --- src/__phutil_library_map__.php | 4 + .../PhabricatorDiffusionApplication.php | 1 + ...sionRepositoryEditAutomationController.php | 94 +++++++++++++ .../DiffusionRepositoryEditMainController.php | 56 +++++++- .../drydock/storage/DrydockAuthorization.php | 80 +++++++++++ .../view/DrydockObjectAuthorizationView.php | 79 +++++++++++ .../phid/query/PhabricatorObjectQuery.php | 36 +++++ .../editor/PhabricatorRepositoryEditor.php | 63 +++++---- .../storage/PhabricatorRepository.php | 18 ++- .../PhabricatorRepositoryTransaction.php | 30 +++++ ...abricatorStandardCustomFieldBlueprints.php | 124 ++---------------- .../PhabricatorStandardCustomFieldPHIDs.php | 19 +-- .../PhabricatorUSEnglishTranslation.php | 17 +++ 13 files changed, 465 insertions(+), 156 deletions(-) create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php create mode 100644 src/applications/drydock/view/DrydockObjectAuthorizationView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bc8b6a17d0..d1a4e587af 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -688,6 +688,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryDefaultController' => 'applications/diffusion/controller/DiffusionRepositoryDefaultController.php', 'DiffusionRepositoryEditActionsController' => 'applications/diffusion/controller/DiffusionRepositoryEditActionsController.php', 'DiffusionRepositoryEditActivateController' => 'applications/diffusion/controller/DiffusionRepositoryEditActivateController.php', + 'DiffusionRepositoryEditAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php', 'DiffusionRepositoryEditBasicController' => 'applications/diffusion/controller/DiffusionRepositoryEditBasicController.php', 'DiffusionRepositoryEditBranchesController' => 'applications/diffusion/controller/DiffusionRepositoryEditBranchesController.php', 'DiffusionRepositoryEditController' => 'applications/diffusion/controller/DiffusionRepositoryEditController.php', @@ -875,6 +876,7 @@ phutil_register_library_map(array( 'DrydockManagementUpdateLeaseWorkflow' => 'applications/drydock/management/DrydockManagementUpdateLeaseWorkflow.php', 'DrydockManagementUpdateResourceWorkflow' => 'applications/drydock/management/DrydockManagementUpdateResourceWorkflow.php', 'DrydockManagementWorkflow' => 'applications/drydock/management/DrydockManagementWorkflow.php', + 'DrydockObjectAuthorizationView' => 'applications/drydock/view/DrydockObjectAuthorizationView.php', 'DrydockQuery' => 'applications/drydock/query/DrydockQuery.php', 'DrydockResource' => 'applications/drydock/storage/DrydockResource.php', 'DrydockResourceActivationFailureLogType' => 'applications/drydock/logtype/DrydockResourceActivationFailureLogType.php', @@ -4424,6 +4426,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryDefaultController' => 'DiffusionController', 'DiffusionRepositoryEditActionsController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryEditActivateController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositoryEditAutomationController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryEditBasicController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryEditBranchesController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryEditController' => 'DiffusionController', @@ -4645,6 +4648,7 @@ phutil_register_library_map(array( 'DrydockManagementUpdateLeaseWorkflow' => 'DrydockManagementWorkflow', 'DrydockManagementUpdateResourceWorkflow' => 'DrydockManagementWorkflow', 'DrydockManagementWorkflow' => 'PhabricatorManagementWorkflow', + 'DrydockObjectAuthorizationView' => 'AphrontView', 'DrydockQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DrydockResource' => array( 'DrydockDAO', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 0314318643..09a5b35f19 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -102,6 +102,7 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { 'update/' => 'DiffusionRepositoryEditUpdateController', 'symbol/' => 'DiffusionRepositorySymbolsController', 'staging/' => 'DiffusionRepositoryEditStagingController', + 'automation/' => 'DiffusionRepositoryEditAutomationController', ), 'pathtree/(?P.*)' => 'DiffusionPathTreeController', 'mirror/' => array( diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php new file mode 100644 index 0000000000..1c94e8bbc5 --- /dev/null +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditAutomationController.php @@ -0,0 +1,94 @@ +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(); + } + + if (!$repository->supportsAutomation()) { + return new Aphront404Response(); + } + + $edit_uri = $this->getRepositoryControllerURI($repository, 'edit/'); + + $v_blueprints = $repository->getHumanReadableDetail( + 'automation.blueprintPHIDs'); + + if ($request->isFormPost()) { + $v_blueprints = $request->getArr('blueprintPHIDs'); + + $xactions = array(); + $template = id(new PhabricatorRepositoryTransaction()); + + $type_blueprints = + PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS; + + $xactions[] = id(clone $template) + ->setTransactionType($type_blueprints) + ->setNewValue($v_blueprints); + + id(new PhabricatorRepositoryEditor()) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request) + ->setActor($viewer) + ->applyTransactions($repository, $xactions); + + return id(new AphrontRedirectResponse())->setURI($edit_uri); + } + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb(pht('Edit Automation')); + + $title = pht('Edit %s', $repository->getName()); + + $form = id(new AphrontFormView()) + ->setUser($viewer) + ->appendRemarkupInstructions( + pht( + "Configure **Repository Automation** to allow Phabricator to ". + "write to this repository.". + "\n\n". + "IMPORTANT: This feature is new, experimental, and not supported. ". + "Use it at your own risk.")) + ->appendControl( + id(new AphrontFormTokenizerControl()) + ->setLabel(pht('Use Blueprints')) + ->setName('blueprintPHIDs') + ->setValue($v_blueprints) + ->setDatasource(new DrydockBlueprintDatasource())) + ->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue(pht('Save')) + ->addCancelButton($edit_uri)); + + $object_box = id(new PHUIObjectBoxView()) + ->setHeaderText($title) + ->setForm($form); + + return $this->buildApplicationPage( + array( + $crumbs, + $object_box, + ), + array( + 'title' => $title, + )); + } + +} diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index 6519a4380e..5def1c754d 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -31,6 +31,7 @@ final class DiffusionRepositoryEditMainController $has_branches = ($is_git || $is_hg); $has_local = $repository->usesLocalWorkingCopy(); $supports_staging = $repository->supportsStaging(); + $supports_automation = $repository->supportsAutomation(); $crumbs = $this->buildApplicationCrumbs($is_main = true); @@ -100,6 +101,13 @@ final class DiffusionRepositoryEditMainController $this->buildStagingActions($repository)); } + $automation_properties = null; + if ($supports_automation) { + $automation_properties = $this->buildAutomationProperties( + $repository, + $this->buildAutomationActions($repository)); + } + $actions_properties = $this->buildActionsProperties( $repository, $this->buildActionsActions($repository)); @@ -171,6 +179,12 @@ final class DiffusionRepositoryEditMainController ->addPropertyList($staging_properties); } + if ($automation_properties) { + $boxes[] = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Automation')) + ->addPropertyList($automation_properties); + } + $boxes[] = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Text Encoding')) ->addPropertyList($encoding_properties); @@ -622,7 +636,6 @@ final class DiffusionRepositoryEditMainController return $view; } - private function buildStagingActions(PhabricatorRepository $repository) { $viewer = $this->getViewer(); @@ -661,6 +674,47 @@ final class DiffusionRepositoryEditMainController return $view; } + private function buildAutomationActions(PhabricatorRepository $repository) { + $viewer = $this->getViewer(); + + $view = id(new PhabricatorActionListView()) + ->setObjectURI($this->getRequest()->getRequestURI()) + ->setUser($viewer); + + $edit = id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Automation')) + ->setHref( + $this->getRepositoryControllerURI($repository, 'edit/automation/')); + $view->addAction($edit); + + return $view; + } + + private function buildAutomationProperties( + PhabricatorRepository $repository, + PhabricatorActionListView $actions) { + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setUser($viewer) + ->setActionList($actions); + + $blueprint_phids = $repository->getAutomationBlueprintPHIDs(); + if (!$blueprint_phids) { + $blueprint_view = phutil_tag('em', array(), pht('Not Configured')); + } else { + $blueprint_view = id(new DrydockObjectAuthorizationView()) + ->setUser($viewer) + ->setObjectPHID($repository->getPHID()) + ->setBlueprintPHIDs($blueprint_phids); + } + + $view->addProperty(pht('Automation'), $blueprint_view); + + return $view; + } + private function buildHostingActions(PhabricatorRepository $repository) { $user = $this->getRequest()->getUser(); diff --git a/src/applications/drydock/storage/DrydockAuthorization.php b/src/applications/drydock/storage/DrydockAuthorization.php index 2bb5decb14..8872ef7f6f 100644 --- a/src/applications/drydock/storage/DrydockAuthorization.php +++ b/src/applications/drydock/storage/DrydockAuthorization.php @@ -93,6 +93,86 @@ final class DrydockAuthorization extends DrydockDAO return idx($map, $state, pht('', $state)); } + /** + * Apply external authorization effects after a user chagnes the value of a + * blueprint selector control an object. + * + * @param PhabricatorUser User applying the change. + * @param phid Object PHID change is being applied to. + * @param list Old blueprint PHIDs. + * @param list New blueprint PHIDs. + * @return void + */ + public static function applyAuthorizationChanges( + PhabricatorUser $viewer, + $object_phid, + array $old, + array $new) { + + $old_phids = array_fuse($old); + $new_phids = array_fuse($new); + + $rem_phids = array_diff_key($old_phids, $new_phids); + $add_phids = array_diff_key($new_phids, $old_phids); + + $altered_phids = $rem_phids + $add_phids; + + if (!$altered_phids) { + return; + } + + $authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withObjectPHIDs(array($object_phid)) + ->withBlueprintPHIDs($altered_phids) + ->execute(); + $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); + + $state_active = self::OBJECTAUTH_ACTIVE; + $state_inactive = self::OBJECTAUTH_INACTIVE; + + $state_requested = self::BLUEPRINTAUTH_REQUESTED; + + // Disable the object side of the authorization for any existing + // authorizations. + foreach ($rem_phids as $rem_phid) { + $authorization = idx($authorizations, $rem_phid); + if (!$authorization) { + continue; + } + + $authorization + ->setObjectAuthorizationState($state_inactive) + ->save(); + } + + // For new authorizations, either add them or reactivate them depending + // on the current state. + foreach ($add_phids as $add_phid) { + $needs_update = false; + + $authorization = idx($authorizations, $add_phid); + if (!$authorization) { + $authorization = id(new DrydockAuthorization()) + ->setObjectPHID($object_phid) + ->setObjectAuthorizationState($state_active) + ->setBlueprintPHID($add_phid) + ->setBlueprintAuthorizationState($state_requested); + + $needs_update = true; + } else { + $current_state = $authorization->getObjectAuthorizationState(); + if ($current_state != $state_active) { + $authorization->setObjectAuthorizationState($state_active); + $needs_update = true; + } + } + + if ($needs_update) { + $authorization->save(); + } + } + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/view/DrydockObjectAuthorizationView.php b/src/applications/drydock/view/DrydockObjectAuthorizationView.php new file mode 100644 index 0000000000..261829bfe5 --- /dev/null +++ b/src/applications/drydock/view/DrydockObjectAuthorizationView.php @@ -0,0 +1,79 @@ +objectPHID = $object_phid; + return $this; + } + + public function getObjectPHID() { + return $this->objectPHID; + } + + public function setBlueprintPHIDs(array $blueprint_phids) { + $this->blueprintPHIDs = $blueprint_phids; + return $this; + } + + public function getBlueprintPHIDs() { + return $this->blueprintPHIDs; + } + + public function render() { + $viewer = $this->getUser(); + $blueprint_phids = $this->getBlueprintPHIDs(); + $object_phid = $this->getObjectPHID(); + + // NOTE: We're intentionally letting you see the authorization state on + // blueprints you can't see because this has a tremendous potential to + // be extremely confusing otherwise. You still can't see the blueprints + // themselves, but you can know if the object is authorized on something. + + if ($blueprint_phids) { + $handles = $viewer->loadHandles($blueprint_phids); + + $authorizations = id(new DrydockAuthorizationQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withObjectPHIDs(array($object_phid)) + ->withBlueprintPHIDs($blueprint_phids) + ->execute(); + $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); + } else { + $handles = array(); + $authorizations = array(); + } + + $items = array(); + foreach ($blueprint_phids as $phid) { + $authorization = idx($authorizations, $phid); + if (!$authorization) { + continue; + } + + $handle = $handles[$phid]; + + $item = id(new PHUIStatusItemView()) + ->setTarget($handle->renderLink()); + + $state = $authorization->getBlueprintAuthorizationState(); + $item->setIcon( + DrydockAuthorization::getBlueprintStateIcon($state), + null, + DrydockAuthorization::getBlueprintStateName($state)); + + $items[] = $item; + } + + $status = new PHUIStatusListView(); + foreach ($items as $item) { + $status->addItem($item); + } + + return $status; + } + +} diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php index d4bcc9edf3..65e9938311 100644 --- a/src/applications/phid/query/PhabricatorObjectQuery.php +++ b/src/applications/phid/query/PhabricatorObjectQuery.php @@ -178,4 +178,40 @@ final class PhabricatorObjectQuery return null; } + + /** + * Select invalid or restricted PHIDs from a list. + * + * PHIDs are invalid if their objects do not exist or can not be seen by the + * viewer. This method is generally used to validate that PHIDs affected by + * a transaction are valid. + * + * @param PhabricatorUser Viewer. + * @param list List of ostensibly valid PHIDs. + * @return list List of invalid or restricted PHIDs. + */ + public static function loadInvalidPHIDsForViewer( + PhabricatorUser $viewer, + array $phids) { + + if (!$phids) { + return array(); + } + + $objects = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs($phids) + ->execute(); + $objects = mpull($objects, null, 'getPHID'); + + $invalid = array(); + foreach ($phids as $phid) { + if (empty($objects[$phid])) { + $invalid[] = $phid; + } + } + + return $invalid; + } + } diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 2e769975ab..af4226a4f6 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -44,6 +44,7 @@ final class PhabricatorRepositoryEditor $types[] = PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE; $types[] = PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES; $types[] = PhabricatorRepositoryTransaction::TYPE_STAGING_URI; + $types[] = PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS; $types[] = PhabricatorTransactions::TYPE_EDGE; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; @@ -107,6 +108,8 @@ final class PhabricatorRepositoryEditor return $object->getSymbolSources(); case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: return $object->getDetail('staging-uri'); + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: + return $object->getDetail('automation.blueprintPHIDs', array()); } } @@ -143,6 +146,7 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: return $xaction->getNewValue(); case PhabricatorRepositoryTransaction::TYPE_NOTIFY: case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: @@ -226,6 +230,11 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: $object->setDetail('staging-uri', $xaction->getNewValue()); return; + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: + $object->setDetail( + 'automation.blueprintPHIDs', + $xaction->getNewValue()); + return; case PhabricatorRepositoryTransaction::TYPE_ENCODING: // Make sure the encoding is valid by converting to UTF-8. This tests // that the user has mbstring installed, and also that they didn't type @@ -276,33 +285,17 @@ final class PhabricatorRepositoryEditor $editor->save(); break; + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: + DrydockAuthorization::applyAuthorizationChanges( + $this->getActor(), + $object->getPHID(), + $xaction->getOldValue(), + $xaction->getNewValue()); + break; } } - protected function mergeTransactions( - PhabricatorApplicationTransaction $u, - PhabricatorApplicationTransaction $v) { - - $type = $u->getTransactionType(); - switch ($type) {} - - return parent::mergeTransactions($u, $v); - } - - protected function transactionHasEffect( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - $old = $xaction->getOldValue(); - $new = $xaction->getNewValue(); - - $type = $xaction->getTransactionType(); - switch ($type) {} - - return parent::transactionHasEffect($object, $xaction); - } - protected function requireCapabilities( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -338,6 +331,7 @@ final class PhabricatorRepositoryEditor case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: PhabricatorPolicyFilter::requireCapability( $this->requireActor(), $object, @@ -431,6 +425,29 @@ final class PhabricatorRepositoryEditor } } break; + + case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: + foreach ($xactions as $xaction) { + $old = nonempty($xaction->getOldValue(), array()); + $new = nonempty($xaction->getNewValue(), array()); + + $add = array_diff($new, $old); + + $invalid = PhabricatorObjectQuery::loadInvalidPHIDsForViewer( + $this->getActor(), + $add); + if ($invalid) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Some of the selected automation blueprints are invalid '. + 'or restricted: %s.', + implode(', ', $invalid)), + $xaction); + } + } + break; } return $errors; diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 084f531a10..c15b9d9dc2 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1799,7 +1799,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } -/* -( Staging )-------------------------------------------------------------*/ +/* -( Staging )------------------------------------------------------------ */ public function supportsStaging() { @@ -1815,6 +1815,22 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } +/* -( Automation )--------------------------------------------------------- */ + + + public function supportsAutomation() { + return $this->isGit(); + } + + + public function getAutomationBlueprintPHIDs() { + if (!$this->supportsAutomation()) { + return array(); + } + return $this->getDetail('automation.blueprintPHIDs', array()); + } + + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php index de1310f320..c9077e0236 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryTransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryTransaction.php @@ -28,6 +28,7 @@ final class PhabricatorRepositoryTransaction const TYPE_SYMBOLS_SOURCES = 'repo:symbol-source'; const TYPE_SYMBOLS_LANGUAGE = 'repo:symbol-language'; const TYPE_STAGING_URI = 'repo:staging-uri'; + const TYPE_AUTOMATION_BLUEPRINTS = 'repo:automation-blueprints'; // TODO: Clean up these legacy transaction types. const TYPE_SSH_LOGIN = 'repo:ssh-login'; @@ -65,6 +66,7 @@ final class PhabricatorRepositoryTransaction } break; case self::TYPE_SYMBOLS_SOURCES: + case self::TYPE_AUTOMATION_BLUEPRINTS: if ($old) { $phids = array_merge($phids, $old); } @@ -436,6 +438,34 @@ final class PhabricatorRepositoryTransaction $old, $new); } + + case self::TYPE_AUTOMATION_BLUEPRINTS: + $add = array_diff($new, $old); + $rem = array_diff($old, $new); + + if ($add && $rem) { + return pht( + '%s changed %s automation blueprint(s), '. + 'added %s: %s; removed %s: %s.', + $this->renderHandleLink($author_phid), + new PhutilNumber(count($add) + count($rem)), + new PhutilNumber(count($add)), + $this->renderHandleList($add), + new PhutilNumber(count($rem)), + $this->renderHandleList($rem)); + } else if ($add) { + return pht( + '%s added %s automation blueprint(s): %s.', + $this->renderHandleLink($author_phid), + new PhutilNumber(count($add)), + $this->renderHandleList($add)); + } else { + return pht( + '%s removed %s automation blueprint(s): %s.', + $this->renderHandleLink($author_phid), + new PhutilNumber(count($rem)), + $this->renderHandleList($rem)); + } } return parent::getTitle(); diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php index 29edd6d472..ad2bb62d81 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldBlueprints.php @@ -14,75 +14,14 @@ final class PhabricatorStandardCustomFieldBlueprints public function applyApplicationTransactionExternalEffects( PhabricatorApplicationTransaction $xaction) { - $object_phid = $xaction->getObjectPHID(); - $old = $this->decodeValue($xaction->getOldValue()); $new = $this->decodeValue($xaction->getNewValue()); - $old_phids = array_fuse($old); - $new_phids = array_fuse($new); - - $rem_phids = array_diff_key($old_phids, $new_phids); - $add_phids = array_diff_key($new_phids, $old_phids); - - $altered_phids = $rem_phids + $add_phids; - - if (!$altered_phids) { - return; - } - - $authorizations = id(new DrydockAuthorizationQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withObjectPHIDs(array($object_phid)) - ->withBlueprintPHIDs($altered_phids) - ->execute(); - $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); - - $state_active = DrydockAuthorization::OBJECTAUTH_ACTIVE; - $state_inactive = DrydockAuthorization::OBJECTAUTH_INACTIVE; - - $state_requested = DrydockAuthorization::BLUEPRINTAUTH_REQUESTED; - - // Disable the object side of the authorization for any existing - // authorizations. - foreach ($rem_phids as $rem_phid) { - $authorization = idx($authorizations, $rem_phid); - if (!$authorization) { - continue; - } - - $authorization - ->setObjectAuthorizationState($state_inactive) - ->save(); - } - - // For new authorizations, either add them or reactivate them depending - // on the current state. - foreach ($add_phids as $add_phid) { - $needs_update = false; - - $authorization = idx($authorizations, $add_phid); - if (!$authorization) { - $authorization = id(new DrydockAuthorization()) - ->setObjectPHID($object_phid) - ->setObjectAuthorizationState($state_active) - ->setBlueprintPHID($add_phid) - ->setBlueprintAuthorizationState($state_requested); - - $needs_update = true; - } else { - $current_state = $authorization->getObjectAuthorizationState(); - if ($current_state != $state_active) { - $authorization->setObjectAuthorizationState($state_active); - $needs_update = true; - } - } - - if ($needs_update) { - $authorization->save(); - } - } - + DrydockAuthorization::applyAuthorizationChanges( + $this->getViewer(), + $xaction->getObjectPHID(), + $old, + $new); } public function renderPropertyViewValue(array $handles) { @@ -91,55 +30,10 @@ final class PhabricatorStandardCustomFieldBlueprints return phutil_tag('em', array(), pht('No authorized blueprints.')); } - $object = $this->getObject(); - $object_phid = $object->getPHID(); - - // NOTE: We're intentionally letting you see the authorization state on - // blueprints you can't see because this has a tremendous potential to - // be extremely confusing otherwise. You still can't see the blueprints - // themselves, but you can know if the object is authorized on something. - - if ($value) { - $handles = $this->getViewer()->loadHandles($value); - - $authorizations = id(new DrydockAuthorizationQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withObjectPHIDs(array($object_phid)) - ->withBlueprintPHIDs($value) - ->execute(); - $authorizations = mpull($authorizations, null, 'getBlueprintPHID'); - } else { - $handles = array(); - $authorizations = array(); - } - - $items = array(); - foreach ($value as $phid) { - $authorization = idx($authorizations, $phid); - if (!$authorization) { - continue; - } - - $handle = $handles[$phid]; - - $item = id(new PHUIStatusItemView()) - ->setTarget($handle->renderLink()); - - $state = $authorization->getBlueprintAuthorizationState(); - $item->setIcon( - DrydockAuthorization::getBlueprintStateIcon($state), - null, - DrydockAuthorization::getBlueprintStateName($state)); - - $items[] = $item; - } - - $status = new PHUIStatusListView(); - foreach ($items as $item) { - $status->addItem($item); - } - - return $status; + return id(new DrydockObjectAuthorizationView()) + ->setUser($this->getViewer()) + ->setObjectPHID($this->getObject()->getPHID()) + ->setBlueprintPHIDs($value); } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php index c7be0f7acb..81b94aff31 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php @@ -158,22 +158,9 @@ abstract class PhabricatorStandardCustomFieldPHIDs $add = array_diff($new, $old); - if (!$add) { - continue; - } - - $objects = id(new PhabricatorObjectQuery()) - ->setViewer($editor->getActor()) - ->withPHIDs($add) - ->execute(); - $objects = mpull($objects, null, 'getPHID'); - - $invalid = array(); - foreach ($add as $phid) { - if (empty($objects[$phid])) { - $invalid[] = $phid; - } - } + $invalid = PhabricatorObjectQuery::loadInvalidPHIDsForViewer( + $editor->getActor(), + $add); if ($invalid) { $error = new PhabricatorApplicationTransactionValidationError( diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index beda39c21c..a3c1833468 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -1403,6 +1403,23 @@ final class PhabricatorUSEnglishTranslation 'Waiting %s seconds for lease to activate.', ), + '%s changed %s automation blueprint(s), added %s: %s; removed %s: %s.' => + '%s changed automation blueprints, added: %4$s; removed: %6$s.', + + '%s added %s automation blueprint(s): %s.' => array( + array( + '%s added an automation blueprint: %3$s.', + '%s added automation blueprints: %3$s.', + ), + ), + + '%s removed %s automation blueprint(s): %s.' => array( + array( + '%s removed an automation blueprint: %3$s.', + '%s removed automation blueprints: %3$s.', + ), + ), + ); } From b4af57ec51e1c449e43b4da5113b44fbab49adde Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Oct 2015 15:46:12 -0700 Subject: [PATCH 15/28] Rough cut of DrydockRepositoryOperation Summary: Ref T182. This doesn't do anything interesting yet and is mostly scaffolding, but here's roughly the workflow. From previous revision, you can configure "Repository Automation" for a repository: {F875741} If it's configured, a new "Land Revision" button shows up: {F875743} Once you click it you get a big warning dialog that it won't work, and then this shows up at the top of the revision (completely temporary/placeholder UI, some day a nice progress bar or whatever): {F875747} If you're lucky, the operation eventually sort of works: {F875750} It only runs `git show` right now, doesn't actually do any writes or anything. Test Plan: - Clicked "Land Revision". - Watched `phd debug task`. - Saw it log `git show` to output. - Verified operation success in UI (by fiddling URL, no way to get there normally yet). Reviewers: chad Reviewed By: chad Subscribers: revi Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14266 --- .../sql/autopatches/20151013.drydock.op.1.sql | 16 ++ src/__phutil_library_map__.php | 19 ++ .../PhabricatorDifferentialApplication.php | 2 + ...ifferentialRevisionOperationController.php | 105 +++++++++++ .../DifferentialRevisionViewController.php | 54 ++++++ ...erentialLandingActionMenuEventListener.php | 12 ++ .../PhabricatorDrydockApplication.php | 5 + ...ydockRepositoryOperationViewController.php | 92 ++++++++++ .../DrydockLandRepositoryOperation.php | 8 + .../DrydockRepositoryOperationType.php | 16 ++ .../DrydockRepositoryOperationPHIDType.php | 37 ++++ .../query/DrydockRepositoryOperationQuery.php | 132 +++++++++++++ .../storage/DrydockRepositoryOperation.php | 148 +++++++++++++++ ...DrydockRepositoryOperationUpdateWorker.php | 173 ++++++++++++++++++ .../drydock/worker/DrydockWorker.php | 15 ++ .../storage/PhabricatorRepository.php | 11 ++ 16 files changed, 845 insertions(+) create mode 100644 resources/sql/autopatches/20151013.drydock.op.1.sql create mode 100644 src/applications/differential/controller/DifferentialRevisionOperationController.php create mode 100644 src/applications/drydock/controller/DrydockRepositoryOperationViewController.php create mode 100644 src/applications/drydock/operation/DrydockLandRepositoryOperation.php create mode 100644 src/applications/drydock/operation/DrydockRepositoryOperationType.php create mode 100644 src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php create mode 100644 src/applications/drydock/query/DrydockRepositoryOperationQuery.php create mode 100644 src/applications/drydock/storage/DrydockRepositoryOperation.php create mode 100644 src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php diff --git a/resources/sql/autopatches/20151013.drydock.op.1.sql b/resources/sql/autopatches/20151013.drydock.op.1.sql new file mode 100644 index 0000000000..e5fed58afd --- /dev/null +++ b/resources/sql/autopatches/20151013.drydock.op.1.sql @@ -0,0 +1,16 @@ +CREATE TABLE {$NAMESPACE}_drydock.drydock_repositoryoperation ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + repositoryPHID VARBINARY(64) NOT NULL, + repositoryTarget LONGBLOB NOT NULL, + operationType VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}, + operationState VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}, + properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (phid), + KEY `key_object` (objectPHID), + KEY `key_repository` (repositoryPHID, operationState) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d1a4e587af..0c3995d41d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -467,6 +467,7 @@ phutil_register_library_map(array( 'DifferentialRevisionListController' => 'applications/differential/controller/DifferentialRevisionListController.php', 'DifferentialRevisionListView' => 'applications/differential/view/DifferentialRevisionListView.php', 'DifferentialRevisionMailReceiver' => 'applications/differential/mail/DifferentialRevisionMailReceiver.php', + 'DifferentialRevisionOperationController' => 'applications/differential/controller/DifferentialRevisionOperationController.php', 'DifferentialRevisionPHIDType' => 'applications/differential/phid/DifferentialRevisionPHIDType.php', 'DifferentialRevisionPackageHeraldField' => 'applications/differential/herald/DifferentialRevisionPackageHeraldField.php', 'DifferentialRevisionPackageOwnerHeraldField' => 'applications/differential/herald/DifferentialRevisionPackageOwnerHeraldField.php', @@ -839,6 +840,7 @@ phutil_register_library_map(array( 'DrydockDefaultViewCapability' => 'applications/drydock/capability/DrydockDefaultViewCapability.php', 'DrydockFilesystemInterface' => 'applications/drydock/interface/filesystem/DrydockFilesystemInterface.php', 'DrydockInterface' => 'applications/drydock/interface/DrydockInterface.php', + 'DrydockLandRepositoryOperation' => 'applications/drydock/operation/DrydockLandRepositoryOperation.php', 'DrydockLease' => 'applications/drydock/storage/DrydockLease.php', 'DrydockLeaseAcquiredLogType' => 'applications/drydock/logtype/DrydockLeaseAcquiredLogType.php', 'DrydockLeaseActivatedLogType' => 'applications/drydock/logtype/DrydockLeaseActivatedLogType.php', @@ -878,6 +880,12 @@ phutil_register_library_map(array( 'DrydockManagementWorkflow' => 'applications/drydock/management/DrydockManagementWorkflow.php', 'DrydockObjectAuthorizationView' => 'applications/drydock/view/DrydockObjectAuthorizationView.php', 'DrydockQuery' => 'applications/drydock/query/DrydockQuery.php', + 'DrydockRepositoryOperation' => 'applications/drydock/storage/DrydockRepositoryOperation.php', + 'DrydockRepositoryOperationPHIDType' => 'applications/drydock/phid/DrydockRepositoryOperationPHIDType.php', + 'DrydockRepositoryOperationQuery' => 'applications/drydock/query/DrydockRepositoryOperationQuery.php', + 'DrydockRepositoryOperationType' => 'applications/drydock/operation/DrydockRepositoryOperationType.php', + 'DrydockRepositoryOperationUpdateWorker' => 'applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php', + 'DrydockRepositoryOperationViewController' => 'applications/drydock/controller/DrydockRepositoryOperationViewController.php', 'DrydockResource' => 'applications/drydock/storage/DrydockResource.php', 'DrydockResourceActivationFailureLogType' => 'applications/drydock/logtype/DrydockResourceActivationFailureLogType.php', 'DrydockResourceActivationYieldLogType' => 'applications/drydock/logtype/DrydockResourceActivationYieldLogType.php', @@ -4205,6 +4213,7 @@ phutil_register_library_map(array( 'DifferentialRevisionListController' => 'DifferentialController', 'DifferentialRevisionListView' => 'AphrontView', 'DifferentialRevisionMailReceiver' => 'PhabricatorObjectMailReceiver', + 'DifferentialRevisionOperationController' => 'DifferentialController', 'DifferentialRevisionPHIDType' => 'PhabricatorPHIDType', 'DifferentialRevisionPackageHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionPackageOwnerHeraldField' => 'DifferentialRevisionHeraldField', @@ -4605,6 +4614,7 @@ phutil_register_library_map(array( 'DrydockDefaultViewCapability' => 'PhabricatorPolicyCapability', 'DrydockFilesystemInterface' => 'DrydockInterface', 'DrydockInterface' => 'Phobject', + 'DrydockLandRepositoryOperation' => 'DrydockRepositoryOperationType', 'DrydockLease' => array( 'DrydockDAO', 'PhabricatorPolicyInterface', @@ -4650,6 +4660,15 @@ phutil_register_library_map(array( 'DrydockManagementWorkflow' => 'PhabricatorManagementWorkflow', 'DrydockObjectAuthorizationView' => 'AphrontView', 'DrydockQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'DrydockRepositoryOperation' => array( + 'DrydockDAO', + 'PhabricatorPolicyInterface', + ), + 'DrydockRepositoryOperationPHIDType' => 'PhabricatorPHIDType', + 'DrydockRepositoryOperationQuery' => 'DrydockQuery', + 'DrydockRepositoryOperationType' => 'Phobject', + 'DrydockRepositoryOperationUpdateWorker' => 'DrydockWorker', + 'DrydockRepositoryOperationViewController' => 'DrydockController', 'DrydockResource' => array( 'DrydockDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/differential/application/PhabricatorDifferentialApplication.php b/src/applications/differential/application/PhabricatorDifferentialApplication.php index 2cc611027f..9203d0c522 100644 --- a/src/applications/differential/application/PhabricatorDifferentialApplication.php +++ b/src/applications/differential/application/PhabricatorDifferentialApplication.php @@ -75,6 +75,8 @@ final class PhabricatorDifferentialApplication extends PhabricatorApplication { => 'DifferentialRevisionCloseDetailsController', 'update/(?P[1-9]\d*)/' => 'DifferentialDiffCreateController', + 'operation/(?P[1-9]\d*)/' + => 'DifferentialRevisionOperationController', ), 'comment/' => array( 'preview/(?P[1-9]\d*)/' => 'DifferentialCommentPreviewController', diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php new file mode 100644 index 0000000000..49d28787f3 --- /dev/null +++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php @@ -0,0 +1,105 @@ +getViewer(); + $id = $request->getURIData('id'); + + $revision = id(new DifferentialRevisionQuery()) + ->withIDs(array($id)) + ->setViewer($viewer) + ->executeOne(); + if (!$revision) { + return new Aphront404Response(); + } + + $detail_uri = "/D{$id}"; + + $repository = $revision->getRepository(); + if (!$repository) { + return $this->rejectOperation( + $revision, + pht('No Repository'), + pht( + 'This revision is not associated with a known repository. Only '. + 'revisions associated with a tracked repository can be landed '. + 'automatically.')); + } + + if (!$repository->canPerformAutomation()) { + return $this->rejectOperation( + $revision, + pht('No Repository Automation'), + pht( + 'The repository this revision is associated with ("%s") is not '. + 'configured to support automation. Configure automation for the '. + 'repository to enable revisions to be landed automatically.', + $repository->getMonogram())); + } + + // TODO: At some point we should allow installs to give "land reviewed + // code" permission to more users than "push any commit", because it is + // a much less powerful operation. For now, just require push so this + // doesn't do anything users can't do on their own. + $can_push = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + DiffusionPushCapability::CAPABILITY); + if (!$can_push) { + return $this->rejectOperation( + $revision, + pht('Unable to Push'), + pht( + 'You do not have permission to push to the repository this '. + 'revision is associated with ("%s"), so you can not land it.', + $repository->getMonogram())); + } + + if ($request->isFormPost()) { + $op = new DrydockLandRepositoryOperation(); + + $operation = DrydockRepositoryOperation::initializeNewOperation($op) + ->setAuthorPHID($viewer->getPHID()) + ->setObjectPHID($revision->getPHID()) + ->setRepositoryPHID($repository->getPHID()) + ->setRepositoryTarget('branch:master'); + + $operation->save(); + $operation->scheduleUpdate(); + + return id(new AphrontRedirectResponse()) + ->setURI($detail_uri); + } + + return $this->newDialog() + ->setTitle(pht('Land Revision')) + ->appendParagraph( + pht( + 'In theory, this will do approximately what `arc land` would do. '. + 'In practice, that is almost certainly not what it will actually '. + 'do.')) + ->appendParagraph( + pht( + 'THIS FEATURE IS EXPERIMENTAL AND DANGEROUS! USE IT AT YOUR '. + 'OWN RISK!')) + ->addCancelButton($detail_uri) + ->addSubmitButton(pht('Mutate Repository Unpredictably')); + } + + private function rejectOperation( + DifferentialRevision $revision, + $title, + $body) { + + $id = $revision->getID(); + $detail_uri = "/D{$id}"; + + return $this->newDialog() + ->setTitle($title) + ->appendParagraph($body) + ->addCancelButton($detail_uri); + } + +} diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index f748b8d9d5..2e1cb4c931 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -463,7 +463,10 @@ final class DifferentialRevisionViewController extends DifferentialController { $object_id = 'D'.$revision->getID(); + $operations_box = $this->buildOperationsBox($revision); + $content = array( + $operations_box, $revision_detail_box, $diff_detail_box, $page_pane, @@ -1032,4 +1035,55 @@ final class DifferentialRevisionViewController extends DifferentialController { return $view; } + private function buildOperationsBox(DifferentialRevision $revision) { + $viewer = $this->getViewer(); + + // Save a query if we can't possibly have pending operations. + $repository = $revision->getRepository(); + if (!$repository || !$repository->canPerformAutomation()) { + return null; + } + + $operations = id(new DrydockRepositoryOperationQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($revision->getPHID())) + ->withOperationStates( + array( + DrydockRepositoryOperation::STATE_WAIT, + DrydockRepositoryOperation::STATE_WORK, + DrydockRepositoryOperation::STATE_FAIL, + )) + ->execute(); + if (!$operations) { + return null; + } + + $operation = head(msort($operations, 'getID')); + + // TODO: This is completely made up for now, give it useful information and + // a sweet progress bar. + + switch ($operation->getOperationState()) { + case DrydockRepositoryOperation::STATE_WAIT: + case DrydockRepositoryOperation::STATE_WORK: + $severity = PHUIInfoView::SEVERITY_NOTICE; + $text = pht( + 'Some sort of repository operation is currently running.'); + break; + default: + $severity = PHUIInfoView::SEVERITY_ERROR; + $text = pht( + 'Some sort of repository operation failed.'); + break; + } + + $info_view = id(new PHUIInfoView()) + ->setSeverity($severity) + ->appendChild($text); + + return id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Active Operations (EXPERIMENTAL!)')) + ->setInfoView($info_view); + } + } diff --git a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php index ad713db943..7dcf3f462d 100644 --- a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php +++ b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php @@ -37,6 +37,18 @@ final class DifferentialLandingActionMenuEventListener return null; } + if ($repository->canPerformAutomation()) { + $revision_id = $revision->getID(); + + $action = id(new PhabricatorActionView()) + ->setWorkflow(true) + ->setName(pht('Land Revision')) + ->setIcon('fa-fighter-jet') + ->setHref("/differential/revision/operation/{$revision_id}/"); + + $this->addActionMenuItems($event, $action); + } + $strategies = id(new PhutilClassMapQuery()) ->setAncestorClass('DifferentialLandingStrategy') ->execute(); diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php index 929b4658ed..cd8cfcad5b 100644 --- a/src/applications/drydock/application/PhabricatorDrydockApplication.php +++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php @@ -90,6 +90,11 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { 'DrydockAuthorizationAuthorizeController', ), ), + '(?Poperation)/' => array( + '(?P[1-9]\d*)/' => array( + '' => 'DrydockRepositoryOperationViewController', + ), + ), ), ); } diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php new file mode 100644 index 0000000000..882a9ab57f --- /dev/null +++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php @@ -0,0 +1,92 @@ +getViewer(); + $id = $request->getURIData('id'); + + $operation = id(new DrydockRepositoryOperationQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$operation) { + return new Aphront404Response(); + } + + $id = $operation->getID(); + $title = pht('Repository Operation %d', $id); + + $header = id(new PHUIHeaderView()) + ->setHeader($title) + ->setUser($viewer) + ->setPolicyObject($operation); + + $state = $operation->getOperationState(); + $icon = DrydockRepositoryOperation::getOperationStateIcon($state); + $name = DrydockRepositoryOperation::getOperationStateName($state); + $header->setStatus($icon, null, $name); + + $actions = $this->buildActionListView($operation); + $properties = $this->buildPropertyListView($operation); + $properties->setActionList($actions); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb($title); + + $object_box = id(new PHUIObjectBoxView()) + ->setHeader($header) + ->addPropertyList($properties); + + return $this->buildApplicationPage( + array( + $crumbs, + $object_box, + ), + array( + 'title' => $title, + )); + + } + + private function buildActionListView(DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + $id = $operation->getID(); + + $view = id(new PhabricatorActionListView()) + ->setUser($viewer) + ->setObjectURI($this->getRequest()->getRequestURI()) + ->setObject($operation); + + return $view; + } + + private function buildPropertyListView( + DrydockRepositoryOperation $operation) { + + $viewer = $this->getViewer(); + + $view = new PHUIPropertyListView(); + $view->addProperty( + pht('Repository'), + $viewer->renderHandle($operation->getRepositoryPHID())); + + $view->addProperty( + pht('Object'), + $viewer->renderHandle($operation->getObjectPHID())); + + return $view; + } + + public function buildSideNavView() { + // TODO: Get rid of this, but it's currently required by DrydockController. + return null; + } + + public function buildApplicationMenu() { + // TODO: As above. + return null; + } + +} diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php new file mode 100644 index 0000000000..d61b8e7be2 --- /dev/null +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -0,0 +1,8 @@ +getPhobjectClassConstant('OPCONST', 32); + } + + final public static function getAllOperationTypes() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getOperationConstant') + ->execute(); + } + +} diff --git a/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php b/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php new file mode 100644 index 0000000000..d21efd8a86 --- /dev/null +++ b/src/applications/drydock/phid/DrydockRepositoryOperationPHIDType.php @@ -0,0 +1,37 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $operation = $objects[$phid]; + $id = $operation->getID(); + + $handle->setName(pht('Drydock Repository Operation %d', $id)); + $handle->setURI("/drydock/operation/{$id}/"); + } + } + +} diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php new file mode 100644 index 0000000000..409a01a07e --- /dev/null +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -0,0 +1,132 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withObjectPHIDs(array $object_phids) { + $this->objectPHIDs = $object_phids; + return $this; + } + + public function withRepositoryPHIDs(array $repository_phids) { + $this->repositoryPHIDs = $repository_phids; + return $this; + } + + public function withOperationStates(array $states) { + $this->operationStates = $states; + return $this; + } + + public function newResultObject() { + return new DrydockRepositoryOperation(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function willFilterPage(array $operations) { + $repository_phids = mpull($operations, 'getRepositoryPHID'); + if ($repository_phids) { + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($repository_phids) + ->execute(); + $repositories = mpull($repositories, null, 'getPHID'); + } else { + $repositories = array(); + } + + foreach ($operations as $key => $operation) { + $repository = idx($repositories, $operation->getRepositoryPHID()); + if (!$repository) { + $this->didRejectResult($operation); + unset($operations[$key]); + continue; + } + $operation->attachRepository($repository); + } + + return $operations; + } + + protected function didFilterPage(array $operations) { + $object_phids = mpull($operations, 'getObjectPHID'); + if ($object_phids) { + $objects = id(new PhabricatorObjectQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($object_phids) + ->execute(); + $objects = mpull($objects, 'getPHID'); + } else { + $objects = array(); + } + + foreach ($operations as $key => $operation) { + $object = idx($objects, $operation->getObjectPHID()); + $operation->attachObject($object); + } + + return $operations; + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + if ($this->repositoryPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'repositoryPHID IN (%Ls)', + $this->repositoryPHIDs); + } + + if ($this->operationStates !== null) { + $where[] = qsprintf( + $conn, + 'operationState IN (%Ls)', + $this->operationStates); + } + + return $where; + } + +} diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php new file mode 100644 index 0000000000..d5befa37b2 --- /dev/null +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -0,0 +1,148 @@ +setOperationState(self::STATE_WAIT) + ->setOperationType($op->getOperationConstant()); + } + + protected function getConfiguration() { + return array( + self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'repositoryTarget' => 'bytes', + 'operationType' => 'text32', + 'operationState' => 'text32', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_object' => array( + 'columns' => array('objectPHID'), + ), + 'key_repository' => array( + 'columns' => array('repositoryPHID', 'operationState'), + ), + ), + ) + parent::getConfiguration(); + } + + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + DrydockRepositoryOperationPHIDType::TYPECONST); + } + + public function attachRepository(PhabricatorRepository $repository) { + $this->repository = $repository; + return $this; + } + + public function getRepository() { + return $this->assertAttached($this->repository); + } + + public function attachObject($object) { + $this->object = $object; + return $this; + } + + public function getObject() { + return $this->assertAttached($this->object); + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public static function getOperationStateIcon($state) { + $map = array( + self::STATE_WAIT => 'fa-clock-o', + self::STATE_WORK => 'fa-refresh blue', + self::STATE_DONE => 'fa-check green', + self::STATE_FAIL => 'fa-times red', + ); + + return idx($map, $state, null); + } + + public static function getOperationStateName($state) { + $map = array( + self::STATE_WAIT => pht('Waiting'), + self::STATE_WORK => pht('Working'), + self::STATE_DONE => pht('Done'), + self::STATE_FAIL => pht('Failed'), + ); + + return idx($map, $state, pht('', $state)); + } + + public function scheduleUpdate() { + PhabricatorWorker::scheduleTask( + 'DrydockRepositoryOperationUpdateWorker', + array( + 'operationPHID' => $this->getPHID(), + ), + array( + 'objectPHID' => $this->getPHID(), + 'priority' => PhabricatorWorker::PRIORITY_ALERTS, + )); + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + public function getPolicy($capability) { + return $this->getRepository()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getRepository()->hasAutomaticCapability($capability, $viewer); + } + + public function describeAutomaticCapability($capability) { + return pht( + 'A repository operation inherits the policies of the repository it '. + 'affects.'); + } + +} diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php new file mode 100644 index 0000000000..d1d1faccd8 --- /dev/null +++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php @@ -0,0 +1,173 @@ +getTaskDataValue('operationPHID'); + + $hash = PhabricatorHash::digestForIndex($operation_phid); + $lock_key = 'drydock.operation:'.$hash; + + $lock = PhabricatorGlobalLock::newLock($lock_key) + ->lock(1); + + try { + $operation = $this->loadOperation($operation_phid); + $this->handleUpdate($operation); + } catch (Exception $ex) { + $lock->unlock(); + throw $ex; + } + + $lock->unlock(); + } + + + private function handleUpdate(DrydockRepositoryOperation $operation) { + $operation_state = $operation->getOperationState(); + + switch ($operation_state) { + case DrydockRepositoryOperation::STATE_WAIT: + $operation + ->setOperationState(DrydockRepositoryOperation::STATE_WORK) + ->save(); + break; + case DrydockRepositoryOperation::STATE_WORK: + break; + case DrydockRepositoryOperation::STATE_DONE: + case DrydockRepositoryOperation::STATE_FAIL: + // No more processing for these requests. + return; + } + + // TODO: We should probably check for other running operations with lower + // IDs and the same repository target and yield to them here? That is, + // enforce sequential evaluation of operations against the same target so + // that if you land "A" and then land "B", we always finish "A" first. + // For now, just let stuff happen in any order. We can't lease until + // we know we're good to move forward because we might deadlock if we do: + // we're waiting for another operation to complete, and that operation is + // waiting for a lease we're holding. + + try { + $lease = $this->loadWorkingCopyLease($operation); + + $interface = $lease->getInterface( + DrydockCommandInterface::INTERFACE_TYPE); + + // No matter what happens here, destroy the lease away once we're done. + $lease->releaseOnDestruction(true); + + // TODO: Some day, do useful things instead of running `git show`. + list($stdout) = $interface->execx('git show'); + phlog($stdout); + } catch (PhabricatorWorkerYieldException $ex) { + throw $ex; + } catch (Exception $ex) { + $operation + ->setOperationState(DrydockRepositoryOperation::STATE_FAIL) + ->save(); + throw $ex; + } + + $operation + ->setOperationState(DrydockRepositoryOperation::STATE_DONE) + ->save(); + + // TODO: Once we have sequencing, we could awaken the next operation + // against this target after finishing or failing. + } + + private function loadWorkingCopyLease( + DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + + // TODO: This is very similar to leasing in Harbormaster, maybe we can + // share some of the logic? + + $lease_phid = $operation->getProperty('exec.leasePHID'); + if ($lease_phid) { + $lease = id(new DrydockLeaseQuery()) + ->setViewer($viewer) + ->withPHIDs(array($lease_phid)) + ->executeOne(); + if (!$lease) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Lease "%s" could not be loaded.', + $lease_phid)); + } + } else { + $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation()) + ->getType(); + + $repository = $operation->getRepository(); + + $allowed_phids = $repository->getAutomationBlueprintPHIDs(); + $authorizing_phid = $repository->getPHID(); + + $lease = DrydockLease::initializeNewLease() + ->setResourceType($working_copy_type) + ->setOwnerPHID($operation->getPHID()) + ->setAuthorizingPHID($authorizing_phid) + ->setAllowedBlueprintPHIDs($allowed_phids); + + $map = $this->buildRepositoryMap($operation); + + $lease->setAttribute('repositories.map', $map); + + $task_id = $this->getCurrentWorkerTaskID(); + if ($task_id) { + $lease->setAwakenTaskIDs(array($task_id)); + } + + $operation + ->setProperty('exec.leasePHID', $lease->getPHID()) + ->save(); + + $lease->queueForActivation(); + } + + if ($lease->isActivating()) { + throw new PhabricatorWorkerYieldException(15); + } + + if (!$lease->isActive()) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Lease "%s" never activated.', + $lease->getPHID())); + } + + return $lease; + } + + private function buildRepositoryMap(DrydockRepositoryOperation $operation) { + $repository = $operation->getRepository(); + + $target = $operation->getRepositoryTarget(); + list($type, $name) = explode(':', $target, 2); + switch ($type) { + case 'branch': + $spec = array( + 'branch' => $name, + ); + break; + default: + throw new Exception( + pht( + 'Unknown repository operation target type "%s" (in target "%s").', + $type, + $target)); + } + + $map = array(); + $map[$repository->getCloneName()] = array( + 'phid' => $repository->getPHID(), + 'default' => true, + ) + $spec; + + return $map; + } +} diff --git a/src/applications/drydock/worker/DrydockWorker.php b/src/applications/drydock/worker/DrydockWorker.php index d2dc1ca399..029cbf1c84 100644 --- a/src/applications/drydock/worker/DrydockWorker.php +++ b/src/applications/drydock/worker/DrydockWorker.php @@ -36,6 +36,21 @@ abstract class DrydockWorker extends PhabricatorWorker { return $resource; } + protected function loadOperation($operation_phid) { + $viewer = $this->getViewer(); + + $operation = id(new DrydockRepositoryOperationQuery()) + ->setViewer($viewer) + ->withPHIDs(array($operation_phid)) + ->executeOne(); + if (!$operation) { + throw new PhabricatorWorkerPermanentFailureException( + pht('No such operation "%s"!', $operation_phid)); + } + + return $operation; + } + protected function loadCommands($target_phid) { $viewer = $this->getViewer(); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index c15b9d9dc2..c7ef71e9b3 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1822,6 +1822,17 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $this->isGit(); } + public function canPerformAutomation() { + if (!$this->supportsAutomation()) { + return false; + } + + if (!$this->getAutomationBlueprintPHIDs()) { + return false; + } + + return true; + } public function getAutomationBlueprintPHIDs() { if (!$this->supportsAutomation()) { From 43bee4562c72a59fcbea054427a9d239c4c96a13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 13 Oct 2015 15:46:30 -0700 Subject: [PATCH 16/28] If the stars align, make "Land Revision" kind of work Summary: Ref T182. If 35 other things are configured completely correctly, make it remotely possible that this button may do something approximating the thing that the user wanted. This primarily fleshes out the idea that "operations" (like landing, merging or cherry-picking) can have some beahavior, and when we run an operation we do whatever that behavior is instead of just running `git show`. Broadly, this isn't too terrible because Drydock seems like it actually works properly for the most part (???!?!). Test Plan: {F876431} Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14270 --- ...ifferentialRevisionOperationController.php | 10 ++- .../differential/storage/DifferentialDiff.php | 13 +-- .../DrydockLandRepositoryOperation.php | 90 +++++++++++++++++++ .../DrydockRepositoryOperationType.php | 15 ++++ .../query/DrydockRepositoryOperationQuery.php | 15 +++- .../storage/DrydockRepositoryOperation.php | 16 ++++ ...DrydockRepositoryOperationUpdateWorker.php | 10 ++- 7 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionOperationController.php b/src/applications/differential/controller/DifferentialRevisionOperationController.php index 49d28787f3..6ec5b8edec 100644 --- a/src/applications/differential/controller/DifferentialRevisionOperationController.php +++ b/src/applications/differential/controller/DifferentialRevisionOperationController.php @@ -10,6 +10,7 @@ final class DifferentialRevisionOperationController $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($id)) ->setViewer($viewer) + ->needActiveDiffs(true) ->executeOne(); if (!$revision) { return new Aphront404Response(); @@ -58,13 +59,20 @@ final class DifferentialRevisionOperationController } if ($request->isFormPost()) { + // NOTE: The operation is locked to the current active diff, so if the + // revision is updated before the operation applies nothing sneaky + // occurs. + + $diff = $revision->getActiveDiff(); + $op = new DrydockLandRepositoryOperation(); $operation = DrydockRepositoryOperation::initializeNewOperation($op) ->setAuthorPHID($viewer->getPHID()) ->setObjectPHID($revision->getPHID()) ->setRepositoryPHID($repository->getPHID()) - ->setRepositoryTarget('branch:master'); + ->setRepositoryTarget('branch:master') + ->setProperty('differential.diffPHID', $diff->getPHID()); $operation->save(); $operation->scheduleUpdate(); diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 7b3a31635a..959d7f24c1 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -447,12 +447,8 @@ final class DifferentialDiff $results['repository.vcs'] = $repo->getVersionControlSystem(); $results['repository.uri'] = $repo->getPublicCloneURI(); - // TODO: We're just hoping to get lucky. Instead, `arc` should store - // where it sent changes and we should only provide staging details - // if we reasonably believe they are accurate. - $staging_ref = 'refs/tags/phabricator/diff/'.$this->getID(); $results['repository.staging.uri'] = $repo->getStagingURI(); - $results['repository.staging.ref'] = $staging_ref; + $results['repository.staging.ref'] = $this->getStagingRef(); } } @@ -480,6 +476,13 @@ final class DifferentialDiff ); } + public function getStagingRef() { + // TODO: We're just hoping to get lucky. Instead, `arc` should store + // where it sent changes and we should only provide staging details + // if we reasonably believe they are accurate. + return 'refs/tags/phabricator/diff/'.$this->getID(); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index d61b8e7be2..a4549e072c 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -5,4 +5,94 @@ final class DrydockLandRepositoryOperation const OPCONST = 'land'; + public function applyOperation( + DrydockRepositoryOperation $operation, + DrydockInterface $interface) { + $viewer = $this->getViewer(); + $repository = $operation->getRepository(); + + $cmd = array(); + $arg = array(); + + $object = $operation->getObject(); + if ($object instanceof DifferentialRevision) { + $revision = $object; + + $diff_phid = $operation->getProperty('differential.diffPHID'); + + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withPHIDs(array($diff_phid)) + ->executeOne(); + if (!$diff) { + throw new Exception( + pht( + 'Unable to load diff "%s".', + $diff_phid)); + } + + $diff_revid = $diff->getRevisionID(); + $revision_id = $revision->getID(); + if ($diff_revid != $revision_id) { + throw new Exception( + pht( + 'Diff ("%s") has wrong revision ID ("%s", expected "%s").', + $diff_phid, + $diff_revid, + $revision_id)); + } + + $cmd[] = 'git fetch --no-tags -- %s +%s:%s'; + $arg[] = $repository->getStagingURI(); + $arg[] = $diff->getStagingRef(); + $arg[] = $diff->getStagingRef(); + + $merge_src = $diff->getStagingRef(); + } else { + throw new Exception( + pht( + 'Invalid or unknown object ("%s") for land operation, expected '. + 'Differential Revision.', + $operation->getObjectPHID())); + } + + $target = $operation->getRepositoryTarget(); + list($type, $name) = explode(':', $target, 2); + switch ($type) { + case 'branch': + $push_dst = 'refs/heads/'.$name; + $merge_dst = 'refs/remotes/origin/'.$name; + break; + default: + throw new Exception( + pht( + 'Unknown repository operation target type "%s" (in target "%s").', + $type, + $target)); + } + + $cmd[] = 'git checkout %s'; + $arg[] = $merge_dst; + + $cmd[] = 'git merge --no-stat --squash --ff-only -- %s'; + $arg[] = $merge_src; + + $cmd[] = 'git -c user.name=%s -c user.email=%s commit --author %s -m %s'; + $arg[] = 'autocommitter'; + $arg[] = 'autocommitter@example.com'; + $arg[] = 'autoauthor '; + $arg[] = pht('(Automerge!)'); + + $cmd[] = 'git push origin -- %s:%s'; + $arg[] = 'HEAD'; + $arg[] = $push_dst; + + $cmd = implode(' && ', $cmd); + $argv = array_merge(array($cmd), $arg); + + $result = call_user_func_array( + array($interface, 'execx'), + $argv); + } + } diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index bbccc0a9bf..3ec8f8ba9e 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -2,6 +2,21 @@ abstract class DrydockRepositoryOperationType extends Phobject { + private $viewer; + + abstract public function applyOperation( + DrydockRepositoryOperation $operation, + DrydockInterface $interface); + + final public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + final public function getOperationConstant() { return $this->getPhobjectClassConstant('OPCONST', 32); } diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php index 409a01a07e..b72e654603 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -42,6 +42,19 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { } protected function willFilterPage(array $operations) { + $implementations = DrydockRepositoryOperationType::getAllOperationTypes(); + + foreach ($operations as $key => $operation) { + $impl = idx($implementations, $operation->getOperationType()); + if (!$impl) { + $this->didRejectResult($operation); + unset($operations[$key]); + continue; + } + $impl = clone $impl; + $operation->attachImplementation($impl); + } + $repository_phids = mpull($operations, 'getRepositoryPHID'); if ($repository_phids) { $repositories = id(new PhabricatorRepositoryQuery()) @@ -75,7 +88,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { ->setParentQuery($this) ->withPHIDs($object_phids) ->execute(); - $objects = mpull($objects, 'getPHID'); + $objects = mpull($objects, null, 'getPHID'); } else { $objects = array(); } diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index d5befa37b2..aed6b147fa 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -23,6 +23,7 @@ final class DrydockRepositoryOperation extends DrydockDAO private $repository = self::ATTACHABLE; private $object = self::ATTACHABLE; + private $implementation = self::ATTACHABLE; public static function initializeNewOperation( DrydockRepositoryOperationType $op) { @@ -77,6 +78,15 @@ final class DrydockRepositoryOperation extends DrydockDAO return $this->assertAttached($this->object); } + public function attachImplementation(DrydockRepositoryOperationType $impl) { + $this->implementation = $impl; + return $this; + } + + public function getImplementation() { + return $this->implementation; + } + public function getProperty($key, $default = null) { return idx($this->properties, $key, $default); } @@ -120,6 +130,12 @@ final class DrydockRepositoryOperation extends DrydockDAO )); } + public function applyOperation(DrydockInterface $interface) { + return $this->getImplementation()->applyOperation( + $this, + $interface); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php index d1d1faccd8..556119f6b3 100644 --- a/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php @@ -25,6 +25,8 @@ final class DrydockRepositoryOperationUpdateWorker private function handleUpdate(DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + $operation_state = $operation->getOperationState(); switch ($operation_state) { @@ -59,9 +61,11 @@ final class DrydockRepositoryOperationUpdateWorker // No matter what happens here, destroy the lease away once we're done. $lease->releaseOnDestruction(true); - // TODO: Some day, do useful things instead of running `git show`. - list($stdout) = $interface->execx('git show'); - phlog($stdout); + $operation->getImplementation() + ->setViewer($viewer); + + $operation->applyOperation($interface); + } catch (PhabricatorWorkerYieldException $ex) { throw $ex; } catch (Exception $ex) { From 4169d7bfd5b1c636e8c9672d6f10fa51084a6272 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Oct 2015 02:56:39 -0700 Subject: [PATCH 17/28] Fix an issue where Harbormaster might cycle while saving The way custom field interact with storage is a little odd, and can send us down a bad path when applying external effect while saving changes. --- .../customfield/field/PhabricatorCustomField.php | 4 +--- .../standard/PhabricatorStandardCustomField.php | 7 ++++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index f9f5979029..64639ed34f 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -540,9 +540,7 @@ abstract class PhabricatorCustomField extends Phobject { * @task storage */ public function newStorageObject() { - if ($this->proxy) { - return $this->proxy->newStorageObject(); - } + // NOTE: This intentionally isn't proxied, to avoid call cycles. throw new PhabricatorCustomFieldImplementationIncompleteException($this); } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index 76a1de9989..ac3e163f6a 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -186,7 +186,12 @@ abstract class PhabricatorStandardCustomField } public function shouldUseStorage() { - return true; + try { + $object = $this->newStorageObject(); + return true; + } catch (PhabricatorCustomFieldImplementationIncompleteException $ex) { + return false; + } } public function getValueForStorage() { From 083a321dad1b58a8440882944d83cec15a72409d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Oct 2015 06:16:21 -0700 Subject: [PATCH 18/28] Fix an issue where newly created Drydock resources could be improperly acquired Summary: Ref T9252. This is mostly a fix for an edge case from D14236. Here's the setup: - There are no resources. - A request for a new resource arrives. - We build a new resource. Now, if we were leasing an existing resource, we'd call `canAcquireLeaseOnResource()` before acquiring a lease on the new resource. However, for new resources we don't do that: we just acquire a lease immediately. This is wrong, because we now allow and expect some resources to be unleasable when created. In a more complex workflow, this can also produce the wrong result and leave the lease acquired sub-optimally (and, today, deadlocked). Make the "can we acquire?" pathway consistent for new and existing resources, so we always do the same set of checks. Test Plan: - Started daemons. - Deleted all working copy resources. - Ran two working-copy-using build plans at the same time. - Before this change, one would often [1] acquire a lease on a pending resource which never allocated, then deadlock. - After this change, the same thing happens except that the lease remains pending and the work completes. [1] Although the race this implies is allowed (resource pool limits are soft/advisory, and it is expected that we may occasionally run over them), it's MUCH easier to hit right now than I would expect it to be, so I think there's probably at least one more small bug here somewhere. I'll see if I can root it out after this change. Reviewers: chad, hach-que Reviewed By: hach-que Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14272 --- .../worker/DrydockLeaseUpdateWorker.php | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 71cc13038a..7d8cc98219 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -211,10 +211,19 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { $exceptions); } + $resources = $this->removeUnacquirableResources($resources, $lease); + if (!$resources) { + // If we make it here, we just built a resource but aren't allowed + // to acquire it. We expect this during routine operation if the + // resource prevents acquisition until it activates. Yield and wait + // for activation. + throw new PhabricatorWorkerYieldException(15); + } + // NOTE: We have not acquired the lease yet, so it is possible that the // resource we just built will be snatched up by some other lease before - // we can. This is not problematic: we'll retry a little later and should - // suceed eventually. + // we can acquire it. This is not problematic: we'll retry a little later + // and should suceed eventually. } $resources = $this->rankResources($resources, $lease); @@ -382,6 +391,22 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { )) ->execute(); + return $this->removeUnacquirableResources($resources, $lease); + } + + + /** + * Remove resources which can not be acquired by a given lease from a list. + * + * @param list Candidate resources. + * @param DrydockLease Acquiring lease. + * @return list Resources which the lease may be able to + * acquire. + * @task allocator + */ + private function removeUnacquirableResources( + array $resources, + DrydockLease $lease) { $keep = array(); foreach ($resources as $key => $resource) { $blueprint = $resource->getBlueprint(); From ac7edf54afe4d9ba8ac077758d8d11de670c2a66 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Oct 2015 06:18:10 -0700 Subject: [PATCH 19/28] Fix bad counting in SQL when enforcing Drydock allocator soft limits Summary: Ref T9252. This fixes a bug from D14236. D14272 discusses the observable effects of the bug, primarily that the window for racing is widened from ~a few milliseconds to several minutes under our configuration. This SQL query is missing a `GROUP BY` clause, so all of the resources get counted as having the same status (specifically, the alphabetically earliest status any resource had, I think). For test cases this often gets the right result since the number of resources may be small and they may all have the same status, but in production this isn't true. In particular, the allocator would sometimes see "35 destroyed resources" (or whatever), when the real counts were "32 destroyed resources + 3 pending resources". Since this allocator behavior is soft/advisory this didn't cause any actual problems, per se (we do expect races here occasionally), it just made the race very very easy to hit. For example, Drydock in production currently has three pending working copy resources. Although we do expect this to be //possible//, getting 4 resources when the configured limit is 1 should be hard (not lightning strike / cosmic radiaion hard, but "happens once a year" hard). Also exclude destroyed resources since we never care about them. Test Plan: Followed the plan from D14272 and restarted two Harbormaster workers at the same time. After this patch was applied, they no longer created two different resources (we expect it to be possible for this to happen, just very hard). We should still be able to force this race by putting something like `sleep(10)` right before the query, then `sleep(10)` right after it. That would prevent the allocators from seeing one another (so they would both think there were no other resources) and push us down the pathway where we exceed the soft limit. Reviewers: chad, hach-que Reviewed By: hach-que Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14274 --- .../drydock/blueprint/DrydockBlueprintImplementation.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index e66b1616e5..0c53b19fcf 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -343,9 +343,12 @@ abstract class DrydockBlueprintImplementation extends Phobject { $counts = queryfx_all( $conn_r, - 'SELECT status, COUNT(*) N FROM %T WHERE blueprintPHID = %s', + 'SELECT status, COUNT(*) N FROM %T + WHERE blueprintPHID = %s AND status != %s + GROUP BY status', $resource->getTableName(), - $blueprint->getPHID()); + $blueprint->getPHID(), + DrydockResourceStatus::STATUS_DESTROYED); $counts = ipull($counts, 'N', 'status'); $n_alloc = idx($counts, DrydockResourceStatus::STATUS_PENDING, 0); From 3a91e648975cb69761d92c1c72ca6ca06c393daf Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Oct 2015 08:15:14 -0700 Subject: [PATCH 20/28] Preserve "Space" UI control value when editing Passphrase credentials Summary: Fixes T9568. We just weren't setting this properly so it would default away from the proper value. Test Plan: - Edited a credential in a non-default space, edit form populated properly. - Changed "Space", introduced an error, saved form, got error with sticky value for "Space" properly. - Saved form with new space value. - Created a new credential. Reviewers: chad Reviewed By: chad Subscribers: revi Maniphest Tasks: T9568 Differential Revision: https://secure.phabricator.com/D14278 --- .../passphrase/controller/PassphraseCredentialEditController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/passphrase/controller/PassphraseCredentialEditController.php b/src/applications/passphrase/controller/PassphraseCredentialEditController.php index 89fe783fac..a749e82af3 100644 --- a/src/applications/passphrase/controller/PassphraseCredentialEditController.php +++ b/src/applications/passphrase/controller/PassphraseCredentialEditController.php @@ -249,6 +249,7 @@ final class PassphraseCredentialEditController extends PassphraseController { id(new AphrontFormPolicyControl()) ->setName('viewPolicy') ->setPolicyObject($credential) + ->setSpacePHID($v_space) ->setCapability(PhabricatorPolicyCapability::CAN_VIEW) ->setPolicies($policies)) ->appendControl( From f3f3d957021b4ab6da2256dc099e152b95720c3d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Oct 2015 10:50:53 -0700 Subject: [PATCH 21/28] When landing revisions via repository automation, use better metadata Summary: Ref T182. Make a reasonable attempt to get the commit message, author, and committer data correct. Test Plan: BEHOLD: rGITTEST810b7f17cd0c909256a45d29a5062fcf417d0489 Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14280 --- .../differential/storage/DifferentialDiff.php | 6 ++ .../DrydockLandRepositoryOperation.php | 55 +++++++++++++++++-- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 959d7f24c1..42bee674ba 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -251,6 +251,12 @@ final class DifferentialDiff $dict['changes'] = $this->buildChangesList(); + return $dict + $this->getDiffAuthorshipDict(); + } + + public function getDiffAuthorshipDict() { + $dict = array(); + $properties = id(new DifferentialDiffProperty())->loadAllWhere( 'diffID = %d', $this->getID()); diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index a4549e072c..a71de3fff9 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -48,6 +48,19 @@ final class DrydockLandRepositoryOperation $arg[] = $diff->getStagingRef(); $merge_src = $diff->getStagingRef(); + + $dict = $diff->getDiffAuthorshipDict(); + $author_name = idx($dict, 'authorName'); + $author_email = idx($dict, 'authorEmail'); + + $api_method = 'differential.getcommitmessage'; + $api_params = array( + 'revision_id' => $revision->getID(), + ); + + $commit_message = id(new ConduitCall($api_method, $api_params)) + ->setUser($viewer) + ->execute(); } else { throw new Exception( pht( @@ -71,6 +84,8 @@ final class DrydockLandRepositoryOperation $target)); } + $committer_info = $this->getCommitterInfo($operation); + $cmd[] = 'git checkout %s'; $arg[] = $merge_dst; @@ -78,10 +93,12 @@ final class DrydockLandRepositoryOperation $arg[] = $merge_src; $cmd[] = 'git -c user.name=%s -c user.email=%s commit --author %s -m %s'; - $arg[] = 'autocommitter'; - $arg[] = 'autocommitter@example.com'; - $arg[] = 'autoauthor '; - $arg[] = pht('(Automerge!)'); + + $arg[] = $committer_info['name']; + $arg[] = $committer_info['email']; + + $arg[] = "{$author_name} <{$author_email}>"; + $arg[] = $commit_message; $cmd[] = 'git push origin -- %s:%s'; $arg[] = 'HEAD'; @@ -95,4 +112,34 @@ final class DrydockLandRepositoryOperation $argv); } + private function getCommitterInfo(DrydockRepositoryOperation $operation) { + $viewer = $this->getViewer(); + + $committer_name = null; + + $author_phid = $operation->getAuthorPHID(); + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($author_phid)) + ->executeOne(); + + if ($object) { + if ($object instanceof PhabricatorUser) { + $committer_name = $object->getUsername(); + } + } + + if (!strlen($committer_name)) { + $committer_name = pht('autocommitter'); + } + + // TODO: Probably let users choose a VCS email address in settings. For + // now just make something up so we don't leak anyone's stuff. + + return array( + 'name' => $committer_name, + 'email' => 'autocommitter@example.com', + ); + } + } From f1552f54a05623550820123b0499e9d216546417 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 14 Oct 2015 16:28:10 -0700 Subject: [PATCH 22/28] Link Timeline image to profile Summary: Ref T9336. Links the timeline photo to user profile. Presume this always exists? Test Plan: Review a few timelines, click on heads. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T9336 Differential Revision: https://secure.phabricator.com/D14283 --- resources/celerity/map.php | 6 +++--- src/view/phui/PHUITimelineEventView.php | 3 ++- webroot/rsrc/css/phui/phui-timeline-view.css | 1 + 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 12b63a615d..bed21d4b4e 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'a11c3643', + 'core.pkg.css' => 'c65b251d', 'core.pkg.js' => '47dc9ebb', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '2de124c9', @@ -147,7 +147,7 @@ return array( 'rsrc/css/phui/phui-status.css' => '888cedb8', 'rsrc/css/phui/phui-tag-view.css' => '402691cc', 'rsrc/css/phui/phui-text.css' => 'cf019f54', - 'rsrc/css/phui/phui-timeline-view.css' => 'f1bccf73', + 'rsrc/css/phui/phui-timeline-view.css' => '2efceff8', 'rsrc/css/phui/phui-two-column-view.css' => '39ecafb1', 'rsrc/css/phui/phui-workboard-view.css' => '6704d68d', 'rsrc/css/phui/phui-workpanel-view.css' => 'adec7699', @@ -797,7 +797,7 @@ return array( 'phui-tag-view-css' => '402691cc', 'phui-text-css' => 'cf019f54', 'phui-theme-css' => '6b451f24', - 'phui-timeline-view-css' => 'f1bccf73', + 'phui-timeline-view-css' => '2efceff8', 'phui-two-column-view-css' => '39ecafb1', 'phui-workboard-view-css' => '6704d68d', 'phui-workpanel-view-css' => 'adec7699', diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index 519cbdf768..f9f1d4d3b4 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -374,10 +374,11 @@ final class PHUITimelineEventView extends AphrontView { $badges = null; if ($image_uri) { $image = phutil_tag( - 'div', + ($this->userHandle->getURI()) ? 'a' : 'div', array( 'style' => 'background-image: url('.$image_uri.')', 'class' => 'phui-timeline-image', + 'href' => $this->userHandle->getURI(), ), ''); if ($this->badges) { diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css index de6658f7dc..d69df93953 100644 --- a/webroot/rsrc/css/phui/phui-timeline-view.css +++ b/webroot/rsrc/css/phui/phui-timeline-view.css @@ -102,6 +102,7 @@ border-radius: 3px; box-shadow: {$borderinset}; background-size: 100%; + display: block; } .device-desktop .phui-timeline-major-event .phui-timeline-image { From 034ff3c8708f4e7fc7eb5727f4a7ddfe107dd7e0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Oct 2015 07:04:14 -0700 Subject: [PATCH 23/28] Remove "_-_" -> "-" slug behavior Summary: Fixes T9573. This incorrectly affected Phriction. I could restore it for only projects, but you didn't like the rule very much anyway and I don't feel strongly about it. Test Plan: Unit tests. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9573 Differential Revision: https://secure.phabricator.com/D14287 --- src/infrastructure/util/PhabricatorSlug.php | 7 ------- .../util/__tests__/PhabricatorSlugTestCase.php | 4 ++-- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/infrastructure/util/PhabricatorSlug.php b/src/infrastructure/util/PhabricatorSlug.php index a6cf0bc70c..fd169914fe 100644 --- a/src/infrastructure/util/PhabricatorSlug.php +++ b/src/infrastructure/util/PhabricatorSlug.php @@ -42,13 +42,6 @@ final class PhabricatorSlug extends Phobject { $parts = explode('/', $slug); - // Convert "_-_" into "-". This is a little nicer for inputs with - // hyphens used as internal separators, and turns an input like "A - B" - // into "a-b" instead of "a_-_b"; - foreach ($parts as $key => $part) { - $parts[$key] = str_replace('_-_', '-', $part); - } - // Remove leading and trailing underscores from each component, if the // component has not been reduced to a single underscore. For example, "a?" // converts to "a", but "??" converts to "_". diff --git a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php index e493ab7363..6a801bb54c 100644 --- a/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php +++ b/src/infrastructure/util/__tests__/PhabricatorSlugTestCase.php @@ -34,7 +34,7 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { 'a/??/c' => 'a/_/c/', 'a/?b/c' => 'a/b/c/', 'a/b?/c' => 'a/b/c/', - 'a - b' => 'a-b/', + 'a - b' => 'a_-_b/', 'a[b]' => 'a_b/', 'ab!' => 'ab!/', ); @@ -51,7 +51,7 @@ final class PhabricatorSlugTestCase extends PhabricatorTestCase { $slugs = array( 'a:b' => 'a_b', 'a!b' => 'a_b', - 'a - b' => 'a-b', + 'a - b' => 'a_-_b', '' => '', 'Demonology: HSA (Hexes, Signs, Alchemy)' => 'demonology_hsa_hexes_signs_alchemy', From 4b43667086ae4faecb56bf54e0f743e53e0687a1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 15 Oct 2015 10:20:19 -0700 Subject: [PATCH 24/28] Introduce PHUIRemarkupView, a sane way to work with Remarkup Summary: Fixes T9273. Remarkup has reasonably good fundamentals but the API is a giant pain to work with. Provide a `PHUIRemarkupView` to make it easier. This object is way simpler to use by default. It's not currently as powerful, but we can expand the power level later by adding more setters. Eventually I'd expect to replace `PhabricatorRemarkupInterface` and `PhabricatorMarkupOneOff` with this, but no rush on those. I converted a few callsites as a sanity check that it works OK. Test Plan: - Viewed remarkup in Passphrase. - Viewed remarkup in Badges. - Viewed a Conduit method. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9273 Differential Revision: https://secure.phabricator.com/D14289 --- src/__phutil_library_map__.php | 2 ++ .../PhabricatorBadgesViewController.php | 8 ++--- .../PhabricatorConduitConsoleController.php | 9 ++---- .../PassphraseCredentialViewController.php | 6 +--- .../markup/PhabricatorMarkupOneOff.php | 11 +------ .../markup/view/PHUIRemarkupView.php | 32 +++++++++++++++++++ 6 files changed, 40 insertions(+), 28 deletions(-) create mode 100644 src/infrastructure/markup/view/PHUIRemarkupView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0c3995d41d..749876acc9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1459,6 +1459,7 @@ phutil_register_library_map(array( 'PHUIPropertyListExample' => 'applications/uiexample/examples/PHUIPropertyListExample.php', 'PHUIPropertyListView' => 'view/phui/PHUIPropertyListView.php', 'PHUIRemarkupPreviewPanel' => 'view/phui/PHUIRemarkupPreviewPanel.php', + 'PHUIRemarkupView' => 'infrastructure/markup/view/PHUIRemarkupView.php', 'PHUISpacesNamespaceContextView' => 'applications/spaces/view/PHUISpacesNamespaceContextView.php', 'PHUIStatusItemView' => 'view/phui/PHUIStatusItemView.php', 'PHUIStatusListView' => 'view/phui/PHUIStatusListView.php', @@ -5352,6 +5353,7 @@ phutil_register_library_map(array( 'PHUIPropertyListExample' => 'PhabricatorUIExample', 'PHUIPropertyListView' => 'AphrontView', 'PHUIRemarkupPreviewPanel' => 'AphrontTagView', + 'PHUIRemarkupView' => 'AphrontView', 'PHUISpacesNamespaceContextView' => 'AphrontView', 'PHUIStatusItemView' => 'AphrontTagView', 'PHUIStatusListView' => 'AphrontTagView', diff --git a/src/applications/badges/controller/PhabricatorBadgesViewController.php b/src/applications/badges/controller/PhabricatorBadgesViewController.php index abaafe6d0f..411970f0bd 100644 --- a/src/applications/badges/controller/PhabricatorBadgesViewController.php +++ b/src/applications/badges/controller/PhabricatorBadgesViewController.php @@ -104,14 +104,10 @@ final class PhabricatorBadgesViewController $description = $badge->getDescription(); if (strlen($description)) { - $description = PhabricatorMarkupEngine::renderOneObject( - id(new PhabricatorMarkupOneOff())->setContent($description), - 'default', - $viewer); - $view->addSectionHeader( pht('Description'), PHUIPropertyListView::ICON_SUMMARY); - $view->addTextContent($description); + $view->addTextContent( + new PHUIRemarkupView($viewer, $description)); } $badge = id(new PHUIBadgeView()) diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php index 79312b5480..e570861f29 100644 --- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php +++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php @@ -195,15 +195,10 @@ final class PhabricatorConduitConsoleController pht('Errors'), $error_description); - - $description = $method->getMethodDescription(); - $description = PhabricatorMarkupEngine::renderOneObject( - id(new PhabricatorMarkupOneOff())->setContent($description), - 'default', - $viewer); $view->addSectionHeader( pht('Description'), PHUIPropertyListView::ICON_SUMMARY); - $view->addTextContent($description); + $view->addTextContent( + new PHUIRemarkupView($viewer, $method->getMethodDescription())); return $view; } diff --git a/src/applications/passphrase/controller/PassphraseCredentialViewController.php b/src/applications/passphrase/controller/PassphraseCredentialViewController.php index 46fd0c4c7a..46ae06cdc7 100644 --- a/src/applications/passphrase/controller/PassphraseCredentialViewController.php +++ b/src/applications/passphrase/controller/PassphraseCredentialViewController.php @@ -201,11 +201,7 @@ final class PassphraseCredentialViewController extends PassphraseController { pht('Description'), PHUIPropertyListView::ICON_SUMMARY); $properties->addTextContent( - PhabricatorMarkupEngine::renderOneObject( - id(new PhabricatorMarkupOneOff()) - ->setContent($description), - 'default', - $viewer)); + new PHUIRemarkupView($viewer, $description)); } return $properties; diff --git a/src/infrastructure/markup/PhabricatorMarkupOneOff.php b/src/infrastructure/markup/PhabricatorMarkupOneOff.php index 7f1708745c..0bfba1722f 100644 --- a/src/infrastructure/markup/PhabricatorMarkupOneOff.php +++ b/src/infrastructure/markup/PhabricatorMarkupOneOff.php @@ -1,16 +1,7 @@ setContent($some_content), - * 'default', - * $viewer); - * - * This is less efficient than batching rendering, but appropriate for small - * amounts of one-off text in form instructions. + * DEPRECATED. Use @{class:PHUIRemarkupView}. */ final class PhabricatorMarkupOneOff extends Phobject diff --git a/src/infrastructure/markup/view/PHUIRemarkupView.php b/src/infrastructure/markup/view/PHUIRemarkupView.php new file mode 100644 index 0000000000..0272c9c3a9 --- /dev/null +++ b/src/infrastructure/markup/view/PHUIRemarkupView.php @@ -0,0 +1,32 @@ +appendChild($fancy_text); + * + */ +final class PHUIRemarkupView extends AphrontView { + + private $corpus; + + public function __construct(PhabricatorUser $viewer, $corpus) { + $this->setUser($viewer); + $this->corpus = $corpus; + } + + public function render() { + $viewer = $this->getUser(); + $corpus = $this->corpus; + + return PhabricatorMarkupEngine::renderOneObject( + id(new PhabricatorMarkupOneOff()) + ->setContent($corpus), + 'default', + $viewer); + } + +} From cdd5e3f7ddacadf37c382b73ec85e0d899405482 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 16 Oct 2015 06:39:31 -0700 Subject: [PATCH 25/28] Initialize `$assign_phid` properly in the "!assign" email action Summary: If you `!assign cahd` when you meant to `!assign chad`, we'll hit an "Undefined variable: assign_phid" a little further down. Test Plan: Eyeballed it. See IRC. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D14291 --- .../maniphest/command/ManiphestAssignEmailCommand.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/maniphest/command/ManiphestAssignEmailCommand.php b/src/applications/maniphest/command/ManiphestAssignEmailCommand.php index 9d2a51b107..98e8a913a8 100644 --- a/src/applications/maniphest/command/ManiphestAssignEmailCommand.php +++ b/src/applications/maniphest/command/ManiphestAssignEmailCommand.php @@ -34,6 +34,7 @@ final class ManiphestAssignEmailCommand array $argv) { $xactions = array(); + $assign_phid = null; $assign_to = head($argv); if ($assign_to) { From 34d6612f07037dab834ecc6f8a89b4c72c1a4fdc Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 16 Oct 2015 07:14:26 -0700 Subject: [PATCH 26/28] Fix font size, highlight color in Diffusion Summary: Minor CSS modernization. Test Plan: Highlight a file in Diffusion. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14290 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/application/diffusion/diffusion-source.css | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index bed21d4b4e..778c419638 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -67,7 +67,7 @@ return array( 'rsrc/css/application/differential/table-of-contents.css' => 'ae4b7a55', 'rsrc/css/application/diffusion/diffusion-icons.css' => '2941baf1', 'rsrc/css/application/diffusion/diffusion-readme.css' => '2106ea08', - 'rsrc/css/application/diffusion/diffusion-source.css' => '66fdf661', + 'rsrc/css/application/diffusion/diffusion-source.css' => '075ba788', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => '697324ad', 'rsrc/css/application/flag/flag.css' => '5337623f', @@ -521,7 +521,7 @@ return array( 'differential-table-of-contents-css' => 'ae4b7a55', 'diffusion-icons-css' => '2941baf1', 'diffusion-readme-css' => '2106ea08', - 'diffusion-source-css' => '66fdf661', + 'diffusion-source-css' => '075ba788', 'diviner-shared-css' => '5a337049', 'font-fontawesome' => 'd2fc4e8d', 'font-lato' => '5ab1a46a', diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index abf68e3aeb..ca52d53a79 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -4,13 +4,12 @@ .diffusion-source { width: 100%; - font-family: "Monaco", Consolas, monospace; - font-size: 10px; + font: 10px/13px "Menlo", "Consolas", "Monaco", monospace; background: #fff; } .diffusion-source tr.phabricator-source-highlight { - background: #ffff00; + background: {$sh-yellowbackground}; } .diffusion-source th { @@ -19,7 +18,6 @@ background: {$lightgreybackground}; color: {$bluetext}; border-right: 1px solid {$thinblueborder}; - font-size: {$smallestfontsize}; } .diffusion-source td { From c9e3dd98d1bbc388eb6d7d85b954da2c9f4af68f Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Fri, 16 Oct 2015 09:51:39 -0700 Subject: [PATCH 27/28] Fix message about pygments being in $PATH Test Plan: read it Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D14269 --- src/applications/config/check/PhabricatorPygmentSetupCheck.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/config/check/PhabricatorPygmentSetupCheck.php b/src/applications/config/check/PhabricatorPygmentSetupCheck.php index 228eb7a33b..0ffb4ff7ad 100644 --- a/src/applications/config/check/PhabricatorPygmentSetupCheck.php +++ b/src/applications/config/check/PhabricatorPygmentSetupCheck.php @@ -45,8 +45,8 @@ final class PhabricatorPygmentSetupCheck extends PhabricatorSetupCheck { 'Phabricator has %s available in %s, but the binary '. 'exited with an error code when run as %s. Check that it is '. 'installed correctly.', - phutil_tag('tt', array(), '$PATH'), phutil_tag('tt', array(), 'pygmentize'), + phutil_tag('tt', array(), '$PATH'), phutil_tag('tt', array(), 'pygmentize -h')); $this From 92a626fc1ca3bae14f65cc05225387b332f03fdd Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 16 Oct 2015 18:47:05 -0700 Subject: [PATCH 28/28] Add a basic list view for repository operations Summary: Ref T182. Nothing fancy, just make these slightly easier to work with. Test Plan: {F884754} Reviewers: chad Reviewed By: chad Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D14295 --- src/__phutil_library_map__.php | 4 + .../PhabricatorDrydockApplication.php | 2 + ...rydockAuthorizationAuthorizeController.php | 10 -- .../DrydockAuthorizationViewController.php | 10 -- .../controller/DrydockConsoleController.php | 8 ++ .../drydock/controller/DrydockController.php | 6 -- ...ydockRepositoryOperationListController.php | 37 +++++++ ...ydockRepositoryOperationViewController.php | 17 ++-- .../DrydockLandRepositoryOperation.php | 6 ++ .../DrydockRepositoryOperationType.php | 4 + ...DrydockRepositoryOperationSearchEngine.php | 99 +++++++++++++++++++ .../storage/DrydockRepositoryOperation.php | 6 ++ 12 files changed, 173 insertions(+), 36 deletions(-) create mode 100644 src/applications/drydock/controller/DrydockRepositoryOperationListController.php create mode 100644 src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 749876acc9..89f65a8db0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -881,8 +881,10 @@ phutil_register_library_map(array( 'DrydockObjectAuthorizationView' => 'applications/drydock/view/DrydockObjectAuthorizationView.php', 'DrydockQuery' => 'applications/drydock/query/DrydockQuery.php', 'DrydockRepositoryOperation' => 'applications/drydock/storage/DrydockRepositoryOperation.php', + 'DrydockRepositoryOperationListController' => 'applications/drydock/controller/DrydockRepositoryOperationListController.php', 'DrydockRepositoryOperationPHIDType' => 'applications/drydock/phid/DrydockRepositoryOperationPHIDType.php', 'DrydockRepositoryOperationQuery' => 'applications/drydock/query/DrydockRepositoryOperationQuery.php', + 'DrydockRepositoryOperationSearchEngine' => 'applications/drydock/query/DrydockRepositoryOperationSearchEngine.php', 'DrydockRepositoryOperationType' => 'applications/drydock/operation/DrydockRepositoryOperationType.php', 'DrydockRepositoryOperationUpdateWorker' => 'applications/drydock/worker/DrydockRepositoryOperationUpdateWorker.php', 'DrydockRepositoryOperationViewController' => 'applications/drydock/controller/DrydockRepositoryOperationViewController.php', @@ -4665,8 +4667,10 @@ phutil_register_library_map(array( 'DrydockDAO', 'PhabricatorPolicyInterface', ), + 'DrydockRepositoryOperationListController' => 'DrydockController', 'DrydockRepositoryOperationPHIDType' => 'PhabricatorPHIDType', 'DrydockRepositoryOperationQuery' => 'DrydockQuery', + 'DrydockRepositoryOperationSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DrydockRepositoryOperationType' => 'Phobject', 'DrydockRepositoryOperationUpdateWorker' => 'DrydockWorker', 'DrydockRepositoryOperationViewController' => 'DrydockController', diff --git a/src/applications/drydock/application/PhabricatorDrydockApplication.php b/src/applications/drydock/application/PhabricatorDrydockApplication.php index cd8cfcad5b..237e4afd9a 100644 --- a/src/applications/drydock/application/PhabricatorDrydockApplication.php +++ b/src/applications/drydock/application/PhabricatorDrydockApplication.php @@ -91,6 +91,8 @@ final class PhabricatorDrydockApplication extends PhabricatorApplication { ), ), '(?Poperation)/' => array( + '(?:query/(?P[^/]+)/)?' + => 'DrydockRepositoryOperationListController', '(?P[1-9]\d*)/' => array( '' => 'DrydockRepositoryOperationViewController', ), diff --git a/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php b/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php index 41010ff882..6422512df5 100644 --- a/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php +++ b/src/applications/drydock/controller/DrydockAuthorizationAuthorizeController.php @@ -82,14 +82,4 @@ final class DrydockAuthorizationAuthorizeController ->addCancelButton($authorization_uri); } - public function buildSideNavView() { - // TODO: Get rid of this, but it's currently required by DrydockController. - return null; - } - - public function buildApplicationMenu() { - // TODO: As above. - return null; - } - } diff --git a/src/applications/drydock/controller/DrydockAuthorizationViewController.php b/src/applications/drydock/controller/DrydockAuthorizationViewController.php index 95270c4b51..3609b95f9f 100644 --- a/src/applications/drydock/controller/DrydockAuthorizationViewController.php +++ b/src/applications/drydock/controller/DrydockAuthorizationViewController.php @@ -128,14 +128,4 @@ final class DrydockAuthorizationViewController return $view; } - public function buildSideNavView() { - // TODO: Get rid of this, but it's currently required by DrydockController. - return null; - } - - public function buildApplicationMenu() { - // TODO: As above. - return null; - } - } diff --git a/src/applications/drydock/controller/DrydockConsoleController.php b/src/applications/drydock/controller/DrydockConsoleController.php index 1964f508d0..77346cea94 100644 --- a/src/applications/drydock/controller/DrydockConsoleController.php +++ b/src/applications/drydock/controller/DrydockConsoleController.php @@ -15,6 +15,7 @@ final class DrydockConsoleController extends DrydockController { $nav->addFilter('blueprint', pht('Blueprints')); $nav->addFilter('resource', pht('Resources')); $nav->addFilter('lease', pht('Leases')); + $nav->addFilter('operation', pht('Repository Operations')); $nav->selectFilter(null); @@ -52,6 +53,13 @@ final class DrydockConsoleController extends DrydockController { ->setHref($this->getApplicationURI('lease/')) ->addAttribute(pht('Manage leases on resources.'))); + $menu->addItem( + id(new PHUIObjectItemView()) + ->setHeader(pht('Repository Operations')) + ->setFontIcon('fa-fighter-jet') + ->setHref($this->getApplicationURI('operation/')) + ->addAttribute(pht('Review the repository operation queue.'))); + $crumbs = $this->buildApplicationCrumbs(); $crumbs->addTextCrumb(pht('Console')); diff --git a/src/applications/drydock/controller/DrydockController.php b/src/applications/drydock/controller/DrydockController.php index 67e4278ae3..ddb6788fb6 100644 --- a/src/applications/drydock/controller/DrydockController.php +++ b/src/applications/drydock/controller/DrydockController.php @@ -2,12 +2,6 @@ abstract class DrydockController extends PhabricatorController { - abstract public function buildSideNavView(); - - public function buildApplicationMenu() { - return $this->buildSideNavView()->getMenu(); - } - protected function buildLocksTab($owner_phid) { $locks = DrydockSlotLock::loadLocks($owner_phid); diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationListController.php b/src/applications/drydock/controller/DrydockRepositoryOperationListController.php new file mode 100644 index 0000000000..581302817b --- /dev/null +++ b/src/applications/drydock/controller/DrydockRepositoryOperationListController.php @@ -0,0 +1,37 @@ +getURIData('queryKey'); + + $engine = new DrydockRepositoryOperationSearchEngine(); + + $controller = id(new PhabricatorApplicationSearchController()) + ->setQueryKey($query_key) + ->setSearchEngine($engine) + ->setNavigation($this->buildSideNavView()); + + return $this->delegateToController($controller); + } + + public function buildSideNavView() { + $nav = new AphrontSideNavFilterView(); + $nav->setBaseURI(new PhutilURI($this->getApplicationURI())); + + $engine = id(new DrydockRepositoryOperationSearchEngine()) + ->setViewer($this->getViewer()); + + $engine->addNavigationItems($nav->getMenu()); + + $nav->selectFilter(null); + + return $nav; + } + +} diff --git a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php index 882a9ab57f..e740073dd9 100644 --- a/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php +++ b/src/applications/drydock/controller/DrydockRepositoryOperationViewController.php @@ -3,6 +3,10 @@ final class DrydockRepositoryOperationViewController extends DrydockController { + public function shouldAllowPublic() { + return true; + } + public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); $id = $request->getURIData('id'); @@ -33,6 +37,9 @@ final class DrydockRepositoryOperationViewController $properties->setActionList($actions); $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb( + pht('Operations'), + $this->getApplicationURI('operation/')); $crumbs->addTextCrumb($title); $object_box = id(new PHUIObjectBoxView()) @@ -79,14 +86,4 @@ final class DrydockRepositoryOperationViewController return $view; } - public function buildSideNavView() { - // TODO: Get rid of this, but it's currently required by DrydockController. - return null; - } - - public function buildApplicationMenu() { - // TODO: As above. - return null; - } - } diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index a71de3fff9..050aaeeb36 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -5,6 +5,12 @@ final class DrydockLandRepositoryOperation const OPCONST = 'land'; + public function getOperationDescription( + DrydockRepositoryOperation $operation, + PhabricatorUser $viewer) { + return pht('Land Revision'); + } + public function applyOperation( DrydockRepositoryOperation $operation, DrydockInterface $interface) { diff --git a/src/applications/drydock/operation/DrydockRepositoryOperationType.php b/src/applications/drydock/operation/DrydockRepositoryOperationType.php index 3ec8f8ba9e..a78ed3173e 100644 --- a/src/applications/drydock/operation/DrydockRepositoryOperationType.php +++ b/src/applications/drydock/operation/DrydockRepositoryOperationType.php @@ -8,6 +8,10 @@ abstract class DrydockRepositoryOperationType extends Phobject { DrydockRepositoryOperation $operation, DrydockInterface $interface); + abstract public function getOperationDescription( + DrydockRepositoryOperation $operation, + PhabricatorUser $viewer); + final public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; return $this; diff --git a/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php new file mode 100644 index 0000000000..c4befe8201 --- /dev/null +++ b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php @@ -0,0 +1,99 @@ +newQuery(); + + return $query; + } + + protected function buildCustomSearchFields() { + return array( + ); + } + + protected function getURI($path) { + return '/drydock/operation/'.$path; + } + + protected function getBuiltinQueryNames() { + return array( + 'all' => pht('All Operations'), + ); + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $operations, + PhabricatorSavedQuery $query, + array $handles) { + assert_instances_of($operations, 'DrydockRepositoryOperation'); + + $viewer = $this->requireViewer(); + + $view = new PHUIObjectItemListView(); + foreach ($operations as $operation) { + $id = $operation->getID(); + + $item = id(new PHUIObjectItemView()) + ->setHeader($operation->getOperationDescription($viewer)) + ->setHref($this->getApplicationURI("operation/{$id}/")) + ->setObjectName(pht('Repository Operation %d', $id)); + + $state = $operation->getOperationState(); + + $icon = DrydockRepositoryOperation::getOperationStateIcon($state); + $name = DrydockRepositoryOperation::getOperationStateName($state); + + $item->addIcon($icon, $name); + $item->addByline( + array( + pht('Via:'), + ' ', + $viewer->renderHandle($operation->getAuthorPHID()), + )); + + $item->addAttribute( + $viewer->renderHandle( + $operation->getObjectPHID())); + + $item->addAttribute( + $viewer->renderHandle( + $operation->getRepositoryPHID())); + + $view->addItem($item); + } + + $result = id(new PhabricatorApplicationSearchResultView()) + ->setObjectList($view) + ->setNoDataString(pht('No matching operations.')); + + return $result; + } + +} diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index aed6b147fa..7a8e35ea68 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -136,6 +136,12 @@ final class DrydockRepositoryOperation extends DrydockDAO $interface); } + public function getOperationDescription(PhabricatorUser $viewer) { + return $this->getImplementation()->getOperationDescription( + $this, + $viewer); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */