Fix excessively strict "Can Use Application" policy filtering
ClosedPublic
Actions

Authored by epriestley on Jan 8 2017, 6:58 PM.

Details

Reviewers

Maniphest Tasks

T9058: PhabricatorPolicyFilter::hasCapability() does not apply application-level "Can Use Application" policy checks

Commits

rP40be2d53743c: (stable) Fix excessively strict "Can Use Application" policy filtering
rPf16778fc18b7: Fix excessively strict "Can Use Application" policy filtering

Summary

Ref T9058. The stricter filtering is over-filtering Handles. For example, in the Phacility cluster, users can not see Almanac services.

So this filtering happens:

The AlmanacServiceQuery filters the service beacuse they can't see the application.
The HandleQuery generates a "you can't see this" handle.
But then the HandleQuery filters that handle! It has a "service" PHID and the user can't see Almanac.

This violates the assumption that all application code makes about handles: it's OK to query handles for objects you can't see, and you'll get something back.

Instead, don't do application filtering on handles.

Test Plan

Added a failing test and made it pass.
As a user who can not see Almanac, viewed an Instances timeline.
- Before patch: fatal on trying to load a handle for a Service.
- After patch: smooth sailing.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jan 8 2017, 6:58 PM

Harbormaster completed remote builds in B15136: Diff 41249.Jan 8 2017, 6:59 PM

I'm going to merge, cherry-pick and deploy this since instance administration is more or less toast without it: normal users can't see Almanac, and every instance gets Almanac transactions during creation as a side-effect of setup.

I missed this in testing because I can see Almanac just fine.

src/applications/policy/filter/PhabricatorPolicyFilter.php
882	Fixed this typo locally.

Closed by commit rPf16778fc18b7: Fix excessively strict "Can Use Application" policy filtering (authored by epriestley, committed by epriestley). · Explain WhyJan 8 2017, 7:01 PM

This revision was automatically updated to reflect the committed changes.

I sent this to production and verified that a non-administrator test account no longer hits this when trying to view instance timelines containing service transactions.

Revision Contents
Changeset List

Path

Size

src/

applications/

policy/

filter/

PhabricatorPolicyFilter.php

13 lines

project/

__tests__/

PhabricatorProjectCoreTestCase.php

20 lines

Diff 41250

View Options

src/applications/policy/filter/PhabricatorPolicyFilter.php

View Options

src/applications/project/tests/PhabricatorProjectCoreTestCase.php

Fix excessively strict "Can Use Application" policy filteringClosedPublicActions