Page MenuHomePhabricator

Fix an issue with incorrect authorization handling in Working Copy build steps
ClosedPublic

Authored by epriestley on Oct 30 2015, 3:01 PM.

Details

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.

Diff Detail

Repository
rP Phabricator
Branch
alloclease
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 8497
Build 9794: Run Core Tests
Build 9793: arc lint + arc unit

Event Timeline

epriestley retitled this revision from to Fix an issue with incorrect authorization handling in Working Copy build steps.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
320

This line is the important one we were missing -- we didn't actually do anything with $blueprint_phids before.

331

...although it didn't really matter much, because we almost always enforced an effectively identical check here anyway.

351

...but could have gone through this branch without enforcing the check, causing a CLI allocation to end up on a surprising blueprint.

Does this also add the buildable repository asdefault repository cloned ?

Does this also add the buildable repository as default repository cloned ?

The repository for the buildable is already always cloned.

src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
173–201

Here's where we explicitly configure the lease to clone the buildable's repository, no matter what Also Clone is set to.

chad edited edge metadata.
This revision is now accepted and ready to land.Oct 30 2015, 3:53 PM
src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php
173–201

That was what I though. I wasn't sure about it though.
Thanks for confirming.

This revision was automatically updated to reflect the committed changes.