recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Add PhabricatorOwnersPackageQuery

Summary:
This is a step toward unearthing Project queries enough that I can make them policy-aware. Right now, some ProjectQuery callsites do not have reasonable access to the viewer. In particular, Owners packages need to issue Project queries because we allow projects to own packages and resolve project members inside of some package queries.

Currently, we have a very unmodern approach to querying packages, with a large number of one-off static load methods:

PhabricatorOwnersPackage::loadAffectedPackages()
PhabricatorOwnersPackage::loadOwningPackages()
PhabricatorOwnersPackage::loadPackagesForPaths()
PhabricatorOwnersOwner::loadAllForPackages()
PhabricatorOwnersOwner::loadAffiliatedUserPHIDs()
PhabricatorOwnersOwner::loadAffiliatedPackages()
ConduitAPI_owners_query_Method::queryAll()
ConduitAPI_owners_query_Method::queryByOwner()
ConduitAPI_owners_query_Method::queryByAffiliatedUser()
ConduitAPI_owners_query_Method::queryByPath()

We should replace `PhabricatorOwnersOwner` with an Edge and move all of these calls to a Query class. I'm going to try to do as little of this work as I can for now since I'm much more interested in getting a functional policy implementation into other applications, but ProjectQuery needs to be policy-aware before I can do that and I need to dig some at least some of the callsites out enough that I can get a viewer in there without making the code worse than it is.

This adds a PhabricatorOwnersPackageQuery class and removes one callsite of one of those static methods.

I also intend to dissolve the two separate concepts of an "owner" (direct owner) and an "affiliated user" (indirect owner via project membership) since I think we're always fine with "affiliated users" owners.

Test Plan: Loaded home page / audit tool, which use the modified path. Ran queries manually via script. Made sure results included directly owned packages and packages owned through project membership.

Reviewers: vrana, btrahan, meitros

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

epriestley 8cdd8015 ab92242e

+119 -14
+8 -2
src/__phutil_library_map__.php
··· 865 865 'PhabricatorOwnersListController' => 'applications/owners/controller/PhabricatorOwnersListController.php', 866 866 'PhabricatorOwnersOwner' => 'applications/owners/storage/PhabricatorOwnersOwner.php', 867 867 'PhabricatorOwnersPackage' => 'applications/owners/storage/PhabricatorOwnersPackage.php', 868 + 'PhabricatorOwnersPackageQuery' => 'applications/owners/query/PhabricatorOwnersPackageQuery.php', 868 869 'PhabricatorOwnersPath' => 'applications/owners/storage/PhabricatorOwnersPath.php', 869 870 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', 870 871 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', ··· 1904 1905 'PhabricatorOwnersEditController' => 'PhabricatorOwnersController', 1905 1906 'PhabricatorOwnersListController' => 'PhabricatorOwnersController', 1906 1907 'PhabricatorOwnersOwner' => 'PhabricatorOwnersDAO', 1907 - 'PhabricatorOwnersPackage' => 'PhabricatorOwnersDAO', 1908 + 'PhabricatorOwnersPackage' => 1909 + array( 1910 + 0 => 'PhabricatorOwnersDAO', 1911 + 1 => 'PhabricatorPolicyInterface', 1912 + ), 1913 + 'PhabricatorOwnersPackageQuery' => 'PhabricatorCursorPagedPolicyQuery', 1908 1914 'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO', 1909 1915 'PhabricatorPHIDController' => 'PhabricatorController', 1910 1916 'PhabricatorPHIDLookupController' => 'PhabricatorPHIDController', ··· 1928 1934 'PhabricatorPolicies' => 'PhabricatorPolicyConstants', 1929 1935 'PhabricatorPolicyCapability' => 'PhabricatorPolicyConstants', 1930 1936 'PhabricatorPolicyException' => 'Exception', 1931 - 'PhabricatorPolicyQuery' => 'PhabricatorQuery', 1937 + 'PhabricatorPolicyQuery' => 'PhabricatorOffsetPagedQuery', 1932 1938 'PhabricatorPolicyTestCase' => 'PhabricatorTestCase', 1933 1939 'PhabricatorPolicyTestObject' => 'PhabricatorPolicyInterface', 1934 1940 'PhabricatorPolicyTestQuery' => 'PhabricatorPolicyQuery',
+11 -11
src/applications/audit/editor/PhabricatorAuditCommentEditor.php
··· 305 305 public static function loadAuditPHIDsForUser(PhabricatorUser $user) { 306 306 $phids = array(); 307 307 308 + // TODO: This method doesn't really use the right viewer, but in practice we 309 + // never issue this query of this type on behalf of another user and are 310 + // unlikely to do so in the future. This entire method should be refactored 311 + // into a Query class, however, and then we should use a proper viewer. 312 + 308 313 // The user can audit on their own behalf. 309 314 $phids[$user->getPHID()] = true; 310 315 311 - // The user can audit on behalf of all packages they own. 312 - $owned_packages = PhabricatorOwnersOwner::loadAffiliatedPackages( 313 - $user->getPHID()); 314 - 315 - if ($owned_packages) { 316 - $packages = id(new PhabricatorOwnersPackage())->loadAllWhere( 317 - 'id IN (%Ld)', 318 - mpull($owned_packages, 'getPackageID')); 319 - foreach (mpull($packages, 'getPHID') as $phid) { 320 - $phids[$phid] = true; 321 - } 316 + $owned_packages = id(new PhabricatorOwnersPackageQuery()) 317 + ->setViewer($user) 318 + ->withOwnerPHIDs(array($user->getPHID())) 319 + ->execute(); 320 + foreach ($owned_packages as $package) { 321 + $phids[$package->getPHID()] = true; 322 322 } 323 323 324 324 // The user can audit on behalf of all projects they are a member of.
+84
src/applications/owners/query/PhabricatorOwnersPackageQuery.php
··· 1 + <?php 2 + 3 + /* 4 + * Copyright 2012 Facebook, Inc. 5 + * 6 + * Licensed under the Apache License, Version 2.0 (the "License"); 7 + * you may not use this file except in compliance with the License. 8 + * You may obtain a copy of the License at 9 + * 10 + * http://www.apache.org/licenses/LICENSE-2.0 11 + * 12 + * Unless required by applicable law or agreed to in writing, software 13 + * distributed under the License is distributed on an "AS IS" BASIS, 14 + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. 15 + * See the License for the specific language governing permissions and 16 + * limitations under the License. 17 + */ 18 + 19 + final class PhabricatorOwnersPackageQuery 20 + extends PhabricatorCursorPagedPolicyQuery { 21 + 22 + private $ownerPHIDs; 23 + 24 + /** 25 + * Owners are direct owners, and members of owning projects. 26 + */ 27 + public function withOwnerPHIDs(array $phids) { 28 + $this->ownerPHIDs = $phids; 29 + return $this; 30 + } 31 + 32 + protected function loadPage() { 33 + $table = new PhabricatorOwnersPackage(); 34 + $conn_r = $table->establishConnection('r'); 35 + 36 + $data = queryfx_all( 37 + $conn_r, 38 + 'SELECT p.* FROM %T p %Q %Q %Q %Q', 39 + $table->getTableName(), 40 + $this->buildJoinClause($conn_r), 41 + $this->buildWhereClause($conn_r), 42 + $this->buildOrderClause($conn_r), 43 + $this->buildLimitClause($conn_r)); 44 + 45 + return $table->loadAllFromArray($data); 46 + } 47 + 48 + private function buildJoinClause(AphrontDatabaseConnection $conn_r) { 49 + $joins = array(); 50 + 51 + if ($this->ownerPHIDs) { 52 + $joins[] = qsprintf( 53 + $conn_r, 54 + 'JOIN %T o ON o.packageID = p.id', 55 + id(new PhabricatorOwnersOwner())->getTableName()); 56 + } 57 + 58 + return implode('', $joins); 59 + } 60 + 61 + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { 62 + $where = array(); 63 + 64 + if ($this->ownerPHIDs) { 65 + $base_phids = $this->ownerPHIDs; 66 + 67 + $query = new PhabricatorProjectQuery(); 68 + $query->withMemberPHIDs($base_phids); 69 + $projects = $query->execute(); 70 + $project_phids = mpull($projects, 'getPHID'); 71 + 72 + $all_phids = array_merge($base_phids, $project_phids); 73 + 74 + $where[] = qsprintf( 75 + $conn_r, 76 + 'o.userPHID IN (%Ls)', 77 + $all_phids); 78 + } 79 + 80 + $where[] = $this->buildPagingClause($conn_r); 81 + return $this->formatWhereClause($where); 82 + } 83 + 84 + }
+16 -1
src/applications/owners/storage/PhabricatorOwnersPackage.php
··· 16 16 * limitations under the License. 17 17 */ 18 18 19 - final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO { 19 + final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO 20 + implements PhabricatorPolicyInterface { 20 21 21 22 protected $phid; 22 23 protected $name; ··· 30 31 private $actorPHID; 31 32 private $oldPrimaryOwnerPHID; 32 33 private $oldAuditingEnabled; 34 + 35 + public function getCapabilities() { 36 + return array( 37 + PhabricatorPolicyCapability::CAN_VIEW, 38 + ); 39 + } 40 + 41 + public function getPolicy($capability) { 42 + return PhabricatorPolicies::POLICY_USER; 43 + } 44 + 45 + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { 46 + return false; 47 + } 33 48 34 49 public function getConfiguration() { 35 50 return array(