@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
Provide better strings in policy errors and exceptions
Summary:
Ref T603. This could probably use a little more polish, but improve the quality of policy error messages.
- Provide as much detail as possible.
- Fix all the strings for i18n.
- Explain special rules to the user.
- Allow indirect policy filters to raise policy exceptions instead of 404s.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7151
+146
-30
+17
-3
src/aphront/configuration/AphrontDefaultApplicationConfiguration.php
+17
-3
src/aphront/configuration/AphrontDefaultApplicationConfiguration.php
···
164
164
return $login_controller->processRequest();
165
165
}
166
166
167
-
$content = hsprintf(
168
-
'<div class="aphront-policy-exception">%s</div>',
169
-
$ex->getMessage());
167
+
$list = $ex->getMoreInfo();
168
+
foreach ($list as $key => $item) {
169
+
$list[$key] = phutil_tag('li', array(), $item);
170
+
}
171
+
if ($list) {
172
+
$list = phutil_tag('ul', array(), $list);
173
+
}
174
+
175
+
$content = phutil_tag(
176
+
'div',
177
+
array(
178
+
'class' => 'aphront-policy-exception',
179
+
),
180
+
array(
181
+
$ex->getMessage(),
182
+
$list,
183
+
));
170
184
171
185
$dialog = new AphrontDialogView();
172
186
$dialog
+4
src/applications/differential/query/DifferentialRevisionQuery.php
+4
src/applications/differential/query/DifferentialRevisionQuery.php
···
416
416
// The revision has an associated repository, and the viewer can't see
417
417
// it, and the viewer has no special capabilities. Filter out this
418
418
// revision.
419
+
$this->didRejectResult($revision);
419
420
unset($revisions[$key]);
420
421
}
421
422
423
+
if (!$revisions) {
424
+
return array();
425
+
}
422
426
423
427
$table = new DifferentialRevision();
424
428
$conn_r = $table->establishConnection('r');
+1
-1
src/applications/differential/storage/DifferentialRevision.php
+1
-1
src/applications/differential/storage/DifferentialRevision.php
···
338
338
case PhabricatorPolicyCapability::CAN_VIEW:
339
339
$description[] = pht(
340
340
"A revision's reviewers can always view it.");
341
-
if ($this->getRepository()) {
341
+
if ($this->getRepositoryPHID()) {
342
342
$description[] = pht(
343
343
'This revision belongs to a repository. Other users must be able '.
344
344
'to view the repository in order to view this revision.');
+11
src/applications/policy/exception/PhabricatorPolicyException.php
+11
src/applications/policy/exception/PhabricatorPolicyException.php
···
2
2
3
3
final class PhabricatorPolicyException extends Exception {
4
4
5
+
private $moreInfo = array();
6
+
7
+
public function setMoreInfo(array $more_info) {
8
+
$this->moreInfo = $more_info;
9
+
return $this;
10
+
}
11
+
12
+
public function getMoreInfo() {
13
+
return $this->moreInfo;
14
+
}
15
+
5
16
}
+89
-14
src/applications/policy/filter/PhabricatorPolicyFilter.php
+89
-14
src/applications/policy/filter/PhabricatorPolicyFilter.php
···
248
248
"This object has an impossible {$verb} policy.");
249
249
}
250
250
251
-
private function rejectObject($object, $policy, $capability) {
251
+
public function rejectObject(
252
+
PhabricatorPolicyInterface $object,
253
+
$policy,
254
+
$capability) {
255
+
252
256
if (!$this->raisePolicyExceptions) {
253
257
return;
254
258
}
255
259
256
-
// TODO: clean this up
257
-
$verb = $capability;
258
-
259
-
$message = "You do not have permission to {$verb} this object.";
260
+
$more = array();
261
+
switch ($capability) {
262
+
case PhabricatorPolicyCapability::CAN_VIEW:
263
+
$message = pht(
264
+
'This object exists, but you do not have permission to view it.');
265
+
break;
266
+
case PhabricatorPolicyCapability::CAN_EDIT:
267
+
$message = pht('You do not have permission to edit this object.');
268
+
break;
269
+
case PhabricatorPolicyCapability::CAN_JOIN:
270
+
$message = pht('You do not have permission to join this object.');
271
+
break;
272
+
}
260
273
261
274
switch ($policy) {
262
275
case PhabricatorPolicies::POLICY_PUBLIC:
263
-
$who = "This is curious, since anyone can {$verb} the object.";
276
+
// Presumably, this is a bug, so we don't bother specializing the
277
+
// strings.
278
+
$more = pht('This object is public.');
264
279
break;
265
280
case PhabricatorPolicies::POLICY_USER:
266
-
$who = "To {$verb} this object, you must be logged in.";
281
+
// We always raise this as "log in", so we don't need to specialize.
282
+
$more = pht('This object is available to logged in users.');
267
283
break;
268
284
case PhabricatorPolicies::POLICY_ADMIN:
269
-
$who = "To {$verb} this object, you must be an administrator.";
285
+
switch ($capability) {
286
+
case PhabricatorPolicyCapability::CAN_VIEW:
287
+
$more = pht('Administrators can view this object.');
288
+
break;
289
+
case PhabricatorPolicyCapability::CAN_EDIT:
290
+
$more = pht('Administrators can edit this object.');
291
+
break;
292
+
case PhabricatorPolicyCapability::CAN_JOIN:
293
+
$more = pht('Administrators can join this object.');
294
+
break;
295
+
}
270
296
break;
271
297
case PhabricatorPolicies::POLICY_NOONE:
272
-
$who = "No one can {$verb} this object.";
298
+
switch ($capability) {
299
+
case PhabricatorPolicyCapability::CAN_VIEW:
300
+
$more = pht('By default, no one can view this object.');
301
+
break;
302
+
case PhabricatorPolicyCapability::CAN_EDIT:
303
+
$more = pht('By default, no one can edit this object.');
304
+
break;
305
+
case PhabricatorPolicyCapability::CAN_JOIN:
306
+
$more = pht('By default, no one can join this object.');
307
+
break;
308
+
}
273
309
break;
274
310
default:
275
311
$handle = id(new PhabricatorHandleQuery())
···
279
315
280
316
$type = phid_get_type($policy);
281
317
if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) {
282
-
$who = "To {$verb} this object, you must be a member of project ".
283
-
"'".$handle->getFullName()."'.";
318
+
switch ($capability) {
319
+
case PhabricatorPolicyCapability::CAN_VIEW:
320
+
$more = pht(
321
+
'This object is visible to members of the project "%s".',
322
+
$handle->getFullName());
323
+
break;
324
+
case PhabricatorPolicyCapability::CAN_EDIT:
325
+
$more = pht(
326
+
'This object can be edited by members of the project "%s".',
327
+
$handle->getFullName());
328
+
break;
329
+
case PhabricatorPolicyCapability::CAN_JOIN:
330
+
$more = pht(
331
+
'This object can be joined by members of the project "%s".',
332
+
$handle->getFullName());
333
+
break;
334
+
}
284
335
} else if ($type == PhabricatorPeoplePHIDTypeUser::TYPECONST) {
285
-
$who = "Only '".$handle->getFullName()."' can {$verb} this object.";
336
+
switch ($capability) {
337
+
case PhabricatorPolicyCapability::CAN_VIEW:
338
+
$more = pht(
339
+
'%s can view this object.',
340
+
$handle->getFullName());
341
+
break;
342
+
case PhabricatorPolicyCapability::CAN_EDIT:
343
+
$more = pht(
344
+
'%s can edit this object.',
345
+
$handle->getFullName());
346
+
break;
347
+
case PhabricatorPolicyCapability::CAN_JOIN:
348
+
$more = pht(
349
+
'%s can join this object.',
350
+
$handle->getFullName());
351
+
break;
352
+
}
353
+
286
354
} else {
287
-
$who = "It is unclear who can {$verb} this object.";
355
+
$who = pht("This object has an unknown policy setting.");
288
356
}
289
357
break;
290
358
}
291
359
292
-
throw new PhabricatorPolicyException("{$message} {$who}");
360
+
$more = array_merge(
361
+
array($more),
362
+
array_filter((array)$object->describeAutomaticCapability($capability)));
363
+
364
+
$exception = new PhabricatorPolicyException($message);
365
+
$exception->setMoreInfo($more);
366
+
367
+
throw $exception;
293
368
}
294
369
}
+24
-12
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+24
-12
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
···
190
190
191
191
$results = array();
192
192
193
-
$filter = new PhabricatorPolicyFilter();
194
-
$filter->setViewer($this->viewer);
195
-
196
-
if (!$this->capabilities) {
197
-
$capabilities = array(
198
-
PhabricatorPolicyCapability::CAN_VIEW,
199
-
);
200
-
} else {
201
-
$capabilities = $this->capabilities;
202
-
}
203
-
$filter->requireCapabilities($capabilities);
204
-
$filter->raisePolicyExceptions($this->shouldRaisePolicyExceptions());
193
+
$filter = $this->getPolicyFilter();
205
194
206
195
$offset = (int)$this->getOffset();
207
196
$limit = (int)$this->getLimit();
···
285
274
$results = $this->didLoadResults($results);
286
275
287
276
return $results;
277
+
}
278
+
279
+
private function getPolicyFilter() {
280
+
$filter = new PhabricatorPolicyFilter();
281
+
$filter->setViewer($this->viewer);
282
+
if (!$this->capabilities) {
283
+
$capabilities = array(
284
+
PhabricatorPolicyCapability::CAN_VIEW,
285
+
);
286
+
} else {
287
+
$capabilities = $this->capabilities;
288
+
}
289
+
$filter->requireCapabilities($capabilities);
290
+
$filter->raisePolicyExceptions($this->shouldRaisePolicyExceptions());
291
+
292
+
return $filter;
293
+
}
294
+
295
+
protected function didRejectResult(PhabricatorPolicyInterface $object) {
296
+
$this->getPolicyFilter()->rejectObject(
297
+
$object,
298
+
$object->getPolicy(PhabricatorPolicyCapability::CAN_VIEW),
299
+
PhabricatorPolicyCapability::CAN_VIEW);
288
300
}
289
301
290
302