1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-02-01 01:18:22 +01:00
Commit graph

10 commits

Author SHA1 Message Date
epriestley
4811e6e7c1 Require several advanced postgraduate degrees to understand object policies
Summary:
Fixes T11836. See some prior discussion in T8376#120613.

The policy hint in headers in the UI is not exhaustive, and can not reasonably be exhaustive. For example, on a revision, it may say "All Users", but really mean "All users who can see the space this object is in and the repository it belongs to, plus the revision author and reviewers".

These rules are explained if you click (and, often, in the documentation), but "All Users" is still at least somewhat misleading.

I don't think there's any perfect solution here that balances the needs of both new and experienced users perfectly, but this change tries to do a bit better about avoiding cases where we say something very open (like "All Users") when the real policy is not very open.

Specifically, I've made these changes to the header:

  - Spaces are now listed in the tag, so it will say `(S3 > All Users)` instead of `(All Users)`. They're already listed in the header, this just makes it more explicit that Spaces are a policy container and part of the view policy.
  - Extended policy objects are now listed in the tag, so it will say `(S3 > rARC > All Users)` for a revision in the Arcanist repository which is also in Space 3.
  - Objects can now provide a "Policy Codex", which is an object that represents a rulebook of more sophisticated policy descriptions. This codex can replace the tag with something else.
    - Imported calendar events now say "Uses Import Policy" instead of, e.g., "All Users".

I've made these changes to the policy dialog:

  - Split it into more visually separate sections.
  - Added an explicit section for extended policies ("You must also have access to these other objects: ...").
  - Broken the object policy rules into a "Special Rules" section (for rules like "you can only see a revision if you can see the repository it is part of") and an "Object Policy" section (for the actual object policy).
  - Tried to make it a little more readable?
  - The new policy dialogs are great to curl up with in front of a fire with a nice cup of cocoa.

I've made these changes to infrastructure:

  - Implementing `PhabricatorPolicyInterface` no longer requires you to implement `describeAutomaticCapability()`.
  - Instead, implement `PhabricatorPolicyCodexInterface` and return a `PhabricatorPolicyCodex` object.
  - This "codex" is a policy rulebook which can set all the policy icons, labels, colors, rules, etc., to properly explain complex policies.
  - Broadly, the old method was usually either not useful (most objects have no special rules) or not powerful enough (objects with special rules often need to do more in order to explain them).

Test Plan:
{F1912860}

{F1912861}

{F1912862}

{F1912863}

Reviewers: chad

Reviewed By: chad

Subscribers: avivey

Maniphest Tasks: T11836

Differential Revision: https://secure.phabricator.com/D16830
2016-11-09 15:05:38 -08:00
epriestley
d6247ffca5 Add support for "Extended Policies"
Summary:
Ref T7703. See that task and inline for a bunch of discussion.

Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.

We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.

Test Plan:
  - Added and executed unit tests.
  - Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
  - Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7703

Differential Revision: https://secure.phabricator.com/D13104
2015-06-03 18:59:27 -07:00
epriestley
1975bdfb36 Work around a bug in PHP 5.3-ish with abstract methods in interfaces
Summary:
@chad is hitting an issue described in P961, which I think is this bug in PHP: https://bugs.php.net/bug.php?id=43200

Work around it by defining a "PHIDInterface" and having both "Flaggable" and "Policy" extend it, so that there is only one `getPHID()` declaration.

Test Plan: shrug~

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7408
2013-10-25 15:58:17 -07:00
epriestley
073cb0e78c Make PhabricatorPolicyInterface require a getPHID() method
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.

Some policy objects don't have real PHIDs:

  PhabricatorTokenGiven
  PhabricatorSavedQuery
  PhabricatorNamedQuery
  PhrequentUserTime
  PhabricatorFlag
  PhabricatorDaemonLog
  PhabricatorConduitMethodCallLog
  ConduitAPIMethod
  PhabricatorChatLogEvent
  PhabricatorChatLogChannel

Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.

Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.

Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7306
2013-10-14 14:35:47 -07:00
epriestley
f75c13b987 Use ApplicationSearch in Applications application
Summary: Ref T603. OMG SO META

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7197
2013-10-02 13:13:07 -07:00
epriestley
2e5ac128b3 Explain policy exception rules to users
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".

This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7150
2013-09-27 08:43:41 -07:00
vrana
ef85f49adc Delete license headers from files
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).

We are removing the headers for these reasons:

- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.

This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).

Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.

Reviewers: epriestley, davidrecordon

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2035

Differential Revision: https://secure.phabricator.com/D3886
2012-11-05 11:16:51 -08:00
vrana
6cc196a2e5 Move files in Phabricator one level up
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.

NOTE: `arc diff` timed out so I'm pushing it without review.

Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.

Auditors: epriestley

Maniphest Tasks: T1103
2012-06-01 12:32:44 -07:00
epriestley
09c8af4de0 Upgrade phabricator to libphutil v2
Summary: Mechanical changes from D2588. No "Class.php" moves yet.

Test Plan: See D2588.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2591
2012-05-30 14:26:29 -07:00
epriestley
ded641ae32 Add basic per-object privacy policies
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.

Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.

The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.

We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.

Test Plan: Unit tests, played around in the UI with various policy settings.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D2210
2012-04-14 10:13:29 -07:00