From 1bdf22535414e5473aea98dc7da3a25d6d4afe4e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Oct 2015 17:02:35 -0700 Subject: [PATCH] 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',