From f48a8337048d617d840aba1a08fe9f845bc8751a Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 30 Oct 2015 16:02:35 +0000 Subject: [PATCH] Fix an issue with incorrect authorization handling in Working Copy build steps Summary: Fixes T9669. Two issues: - We were using `repositoryPHIDs` instead of `blueprintPHIDs` for the list of allowed blueprints. Use the correct value. - We weren't enforcing `allowedBlueprintPHIDs` fully correctly. We //did// require an authorization, so the net effect was correct in nearly all cases, but we could have selected from too large a pool in the case where the application itself was doing the authorization (e.g., from the command line). Test Plan: Ran a build through Drydock/Harbormaster locally. Reviewers: chad, tycho.tatitscheff Reviewed By: chad, tycho.tatitscheff Subscribers: tycho.tatitscheff Maniphest Tasks: T9669 Differential Revision: https://secure.phabricator.com/D14368 --- .../drydock/worker/DrydockLeaseUpdateWorker.php | 11 ++++++----- ...rmasterLeaseWorkingCopyBuildStepImplementation.php | 5 ++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index 0c93647b89..8f34a88342 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -309,17 +309,18 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { return array(); } - $query = id(new DrydockBlueprintQuery()) - ->setViewer($viewer) - ->withBlueprintClasses(array_keys($impls)) - ->withDisabled(false); - $blueprint_phids = $lease->getAllowedBlueprintPHIDs(); if (!$blueprint_phids) { $lease->logEvent(DrydockLeaseNoBlueprintsLogType::LOGCONST); return array(); } + $query = id(new DrydockBlueprintQuery()) + ->setViewer($viewer) + ->withPHIDs($blueprint_phids) + ->withBlueprintClasses(array_keys($impls)) + ->withDisabled(false); + // 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(); diff --git a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php index 3c4d18f4b4..82c041f423 100644 --- a/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php @@ -41,7 +41,10 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation $working_copy_type = id(new DrydockWorkingCopyBlueprintImplementation()) ->getType(); - $allowed_phids = $build_target->getFieldValue('repositoryPHIDs'); + $allowed_phids = $build_target->getFieldValue('blueprintPHIDs'); + if (!is_array($allowed_phids)) { + $allowed_phids = array(); + } $authorizing_phid = $build_target->getBuildStep()->getPHID(); $lease = DrydockLease::initializeNewLease()