@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
Simplify policy filtering for projects and ObjectQuery
Summary:
Ref T603. Moves to detangle and optimize how we apply policies to filtering objects. Notably:
- Add a short circuit for omnipotent users.
- When performing project filtering, do a stricter check for user membership. We don't actually care if the user can see the project or not according to other policy constraints, and checking if they can may be complicated.
- When performing project filtering, do a local check to see if we're filtering the project itself. This is a common case (a project editable by members of itself, for example) and we can skip queries when it is satisfied.
- Don't perform policy filtering in ObjectQuery. All the data it aggregates is already filtered correctly.
- Clean up a little bit of stuff in Feed.
Test Plan: Pages like the Maniphest task list and Project profile pages now issue dramatically fewer queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6931
+74
-36
+6
-4
src/applications/feed/story/PhabricatorFeedStory.php
+6
-4
src/applications/feed/story/PhabricatorFeedStory.php
···
71
71
$object_phids += $phids;
72
72
}
73
73
74
-
$objects = id(new PhabricatorObjectHandleData(array_keys($object_phids)))
74
+
$objects = id(new PhabricatorObjectQuery())
75
75
->setViewer($viewer)
76
-
->loadObjects();
76
+
->withPHIDs(array_keys($object_phids))
77
+
->execute();
77
78
78
79
foreach ($key_phids as $key => $phids) {
79
80
if (!$phids) {
···
102
103
$handle_phids += $key_phids[$key];
103
104
}
104
105
105
-
$handles = id(new PhabricatorObjectHandleData(array_keys($handle_phids)))
106
+
$handles = id(new PhabricatorHandleQuery())
106
107
->setViewer($viewer)
107
-
->loadHandles();
108
+
->withPHIDs(array_keys($handle_phids))
109
+
->execute();
108
110
109
111
foreach ($key_phids as $key => $phids) {
110
112
if (!$phids) {
+9
src/applications/phid/query/PhabricatorObjectQuery.php
+9
src/applications/phid/query/PhabricatorObjectQuery.php
···
118
118
}
119
119
}
120
120
121
+
/**
122
+
* This query disables policy filtering because it is performed in the
123
+
* subqueries which actually load objects. We don't need to re-filter
124
+
* results, since policies have already been applied.
125
+
*/
126
+
protected function shouldDisablePolicyFiltering() {
127
+
return true;
128
+
}
129
+
121
130
}
+40
-31
src/applications/policy/filter/PhabricatorPolicyFilter.php
+40
-31
src/applications/policy/filter/PhabricatorPolicyFilter.php
···
70
70
'Call setViewer() and requireCapabilities() before apply()!');
71
71
}
72
72
73
+
// If the viewer is omnipotent, short circuit all the checks and just
74
+
// return the input unmodified. This is an optimization; we know the
75
+
// result already.
76
+
if ($viewer->isOmnipotent()) {
77
+
return $objects;
78
+
}
79
+
73
80
$filtered = array();
81
+
$viewer_phid = $viewer->getPHID();
82
+
83
+
if (empty($this->userProjects[$viewer_phid])) {
84
+
$this->userProjects[$viewer_phid] = array();
85
+
}
74
86
75
87
$need_projects = array();
76
88
foreach ($objects as $key => $object) {
···
85
97
$policy = $object->getPolicy($capability);
86
98
$type = phid_get_type($policy);
87
99
if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) {
88
-
$need_projects[] = $policy;
100
+
$need_projects[$policy] = $policy;
89
101
}
90
102
}
91
103
}
92
104
105
+
// If we need projects, check if any of the projects we need are also the
106
+
// objects we're filtering. Because of how project rules work, this is a
107
+
// common case.
93
108
if ($need_projects) {
94
-
$need_projects = array_unique($need_projects);
95
-
96
-
// If projects have recursive policies, automatically fail them rather
97
-
// than looping. This will fall back to automatic capabilities and
98
-
// resolve the policies in a sensible way.
99
-
static $querying_projects = array();
100
-
foreach ($need_projects as $key => $project) {
101
-
if (empty($querying_projects[$project])) {
102
-
$querying_projects[$project] = true;
103
-
continue;
109
+
foreach ($objects as $object) {
110
+
if ($object instanceof PhabricatorProject) {
111
+
$project_phid = $object->getPHID();
112
+
if (isset($need_projects[$project_phid])) {
113
+
$is_member = $object->isUserMember($viewer_phid);
114
+
$this->userProjects[$viewer_phid][$project_phid] = $is_member;
115
+
unset($need_projects[$project_phid]);
116
+
}
104
117
}
105
-
unset($need_projects[$key]);
106
118
}
119
+
}
107
120
108
-
if ($need_projects) {
109
-
$caught = null;
110
-
try {
111
-
$projects = id(new PhabricatorProjectQuery())
112
-
->setViewer($viewer)
113
-
->withMemberPHIDs(array($viewer->getPHID()))
114
-
->withPHIDs($need_projects)
115
-
->execute();
116
-
} catch (Exception $ex) {
117
-
$caught = $ex;
118
-
}
121
+
if ($need_projects) {
122
+
$need_projects = array_unique($need_projects);
119
123
120
-
foreach ($need_projects as $key => $project) {
121
-
unset($querying_projects[$project]);
122
-
}
124
+
// NOTE: We're using the omnipotent user here to avoid a recursive
125
+
// descent into madness. We don't actually need to know if the user can
126
+
// see these projects or not, since: the check is "user is member of
127
+
// project", not "user can see project"; and membership implies
128
+
// visibility anyway. Without this, we may load other projects and
129
+
// re-enter the policy filter and generally create a huge mess.
123
130
124
-
if ($caught) {
125
-
throw $caught;
126
-
}
131
+
$projects = id(new PhabricatorProjectQuery())
132
+
->setViewer(PhabricatorUser::getOmnipotentUser())
133
+
->withMemberPHIDs(array($viewer->getPHID()))
134
+
->withPHIDs($need_projects)
135
+
->execute();
127
136
128
-
$projects = mpull($projects, null, 'getPHID');
129
-
$this->userProjects[$viewer->getPHID()] = $projects;
137
+
foreach ($projects as $project) {
138
+
$this->userProjects[$viewer_phid][$project->getPHID()] = true;
130
139
}
131
140
}
132
141
+19
-1
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+19
-1
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
···
182
182
$maybe_visible = array();
183
183
}
184
184
185
-
$visible = $filter->apply($maybe_visible);
185
+
if ($this->shouldDisablePolicyFiltering()) {
186
+
$visible = $maybe_visible;
187
+
} else {
188
+
$visible = $filter->apply($maybe_visible);
189
+
}
186
190
187
191
$removed = array();
188
192
foreach ($maybe_visible as $key => $object) {
···
326
330
*/
327
331
protected function didLoadResults(array $results) {
328
332
return $results;
333
+
}
334
+
335
+
336
+
/**
337
+
* Allows a subclass to disable policy filtering. This method is dangerous.
338
+
* It should be used only if the query loads data which has already been
339
+
* filtered (for example, because it wraps some other query which uses
340
+
* normal policy filtering).
341
+
*
342
+
* @return bool True to disable all policy filtering.
343
+
* @task policyimpl
344
+
*/
345
+
protected function shouldDisablePolicyFiltering() {
346
+
return false;
329
347
}
330
348
331
349
}