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.
Reviewers: chad
Maniphest Tasks: T9058
Differential Revision: https://secure.phabricator.com/D17152
Summary:
Fixes T9058. Normally, "Query" classes apply an application check and just don't load anything if it fails.
However, in some cases (like email recipient filtering) we run policy checks without having run a Query check first. In that case, one user (the actor) loads the object, then we filter it against other users (the recipeints).
Explicitly apply the application check during normal filtering.
Test Plan: Added a failing test case and made it pass.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9058
Differential Revision: https://secure.phabricator.com/D17127
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.
We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.
Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).
If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.
Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.
Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17029
Summary:
Ref T11853. My CSS change for the more enormous policy dialog was a little too broad, and affected the "You shall not pass!" dialog too.
Narrow the scope of the CSS rules.
Also add a missing "." that I caught.
Test Plan:
- Looked at policy exception dialogs.
- Looked at policy explanation dialogs.
- Looked at the end of that sentence.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11853
Differential Revision: https://secure.phabricator.com/D16841
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
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
Summary:
Ref T11404. Currently, SearchEngineAttachments can bulk-load data but SearchEngineExtensions can not.
This leads to poor performance of custom fields. See T11404 for discussion.
This changes the API to support a bulk load + format pattern like the one Attachments use. The next change will use it to bulk-load custom field data.
Test Plan:
- Ran `differential.query`, `differential.revision.search` as a sanity check.
- No behavioral changes are expected
- See next revision.
Reviewers: yelirekim, chad
Reviewed By: chad
Maniphest Tasks: T11404
Differential Revision: https://secure.phabricator.com/D16350
Summary:
Ref T4103. Convert this into a proper internal setting and use transactions to mutate it.
Also remove some no-longer-used old non-modular settings constants.
Test Plan:
- Used policy dropdown, saw recently-used projects.
- Selected some new projects, saw them appear.
- Grepped for all removed constants.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16027
Summary:
Ref T4103. This doesn't get everything, but takes care of most of the easy stuff.
The tricky-ish bit here is that I need to move timezones, pronouns and translations to proper settings. I expect to pursue that next.
Test Plan:
- Grepped for `loadPreferences` to identify callsites.
- Changed start-of-week setting, loaded Calendar, saw correct start.
- Visited welcome page, read "Adjust Settings" point.
- Loaded Conpherence -- I changed behavior here slightly (switching threads drops the title glyph) but it wasn't consistent to start with and this seems like a good thing to push to the next version of Conpherence.
- Enabled Filetree, toggled in Differential.
- Disabled Filetree, no longer visible in Differential.
- Changed "Unified Diffs" preference to "Small Screens" vs "Always".
- Toggled filetree in Diffusion.
- Edited a task, saw sensible projects in policy dropdown.
- Viewed user profile, uncollapsed/collapsed side nav, reloaded page, sticky'd.
- Toggled "monospaced textareas", used a comment box, got appropriate fonts.
- Toggled durable column.
- Disabled title glyphs.
- Changed monospaced font to 18px/36px impact.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16004
Summary:
Fixes T6741. Ref T10246. Broadly, we want to protect Almanac cluster services:
- Today, against users in the Phacility cluster accidentally breaking their own instances.
- In the future, against attackers compromising administrative accounts and adding a new "cluster database" which points at hardware they control.
The way this works right now is really complicated: there's a global "can create cluster services" setting, and then separate per-service and per-device locks.
Instead, change "Can Create Cluster Services" into "Can Manage Cluster Services". Require this permission (in addition to normal permissions) to edit or create any cluster service.
This permission can be locked to "No One" via config (as we do in the Phacility cluster) so we only need this one simple setting.
There's also zero reason to individually lock //some// of the cluster services.
Also improve extended policy errors.
The UI here is still a little heavy-handed, but should be good enough for the moment.
Test Plan:
- Ran migrations.
- Verified that cluster services and bindings reported that they belonged to the cluster.
- Edited a cluster binding.
- Verified that the bound device was marked as a cluster device
- Moved a cluster binding, verified the old device was unmarked as a cluster device.
- Tried to edit a cluster device as an unprivileged user, got a sensible error.
{F1126552}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6741, T10246
Differential Revision: https://secure.phabricator.com/D15339
Summary:
WMF ran into this after their update. Here's the setup:
- When you enable Spaces, we leave all existing objects set to `null`, which means "these belong to the default space". This is so we don't have to go update a trillion objects.
- New objects get set to the default space explicitly (`PHID-SPCE-...`) but older ones stay with `null`.
- If you edit an older object (like a task) from the time before Spaces, //and// the form doesn't have a Visbility/Spaces control, we would incorrectly poplate the value with `null` when the effective value should be the default space PHID.
- This caused a "You must choose a space." error in the UI.
Instead, populate the control with the effective value instead of the literal database value. This makes the edit go through cleanly.
Also add a note about this for future-me.
Test Plan:
- Disabled "Visibility" control in task edit form.
- Edited an old task which had `null` as a `spacePHID` in the database.
- Before patch: UI error about selecting a Space.
- After patch: edit goes through cleanly.
Reviewers: chad, 20after4
Reviewed By: chad, 20after4
Subscribers: 20after4, aklapper
Differential Revision: https://secure.phabricator.com/D15306
Summary:
When filling in filler projects, only select active ones.
Also use a slightly more modern method signature.
Test Plan: Disabled a project, saw it vanish from the control.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15199
Summary:
Fixes T4136.
When listing projects in the "Visible To" selector control:
- Instead of showing every project you are a member of, show only a few.
- Add an option to choose something else which isn't in the menu.
- If you've used the control before, show the stuff you've selected in the recent past.
- If you haven't used the control before or haven't used it much, show the stuff you've picked and them some filler.
- Don't offer milestones.
- Also don't offer milestones in the custom policy UI.
Test Plan:
{F1091999}
{F1092000}
- Selected a project.
- Used "find" to select a different project.
- Saw reasonable defaults.
- Saw favorites stick.
- Tried to typeahead a milestone (nope).
- Used "Custom Policy", tried to typeahead a milestone (nope).
- Used "Custom Policy" in general.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4136
Differential Revision: https://secure.phabricator.com/D15184
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary:
Via HackerOne. The use of `$key` here should be `$extended_key`.
Exploiting this requires a very unusual group of objects to be subjected to extended policy checks. I believe there is no way to actually get anything bad through the policy filter today, but this could have been an issue in the future.
Test Plan:
- Added a unit test which snuck something through the policy filter.
- Fixed use of `$extended_key`.
- Test now passes.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14993
Summary:
Fixes T9019. Pretty much ripped from D14467. I added the "policy hint" stuff so that you can create a project with this policy immediately.
I really dislike how the "hint" code works, but we //almost// never need to use it and the badness feels fairly well-contained.
Also pick up a quick feedback fix from D14863.
Test Plan:
- Added test coverage, got it to pass.
- Created a project with "Visible To: Project Members".
Reviewers: joshuaspence, chad
Reviewed By: chad
Maniphest Tasks: T9019
Differential Revision: https://secure.phabricator.com/D14869
Summary: See D14467. Just teasing this apart so I can be a little more confident in my commandeering. These are the unambiguous cleanup changes from D14467.
Test Plan: inspection / clicked stuff / no impact
Reviewers: chad, joshuaspence
Reviewed By: joshuaspence
Differential Revision: https://secure.phabricator.com/D14868
Summary:
Ref T10010. This adds infrastructure for querying projects by type, depth, parent and ancestor.
I needed to revise the "extended policy check" cycle detection rules. When, e.g., querying a grandchild, they incorrectly detected a cycle because both the child and grandchild needed to check the policy of the grandparent.
Instead, simplify it to just do a basic runaway calldepth check. There are many other safety mechanisms to make it so this can't ever occur.
(Cycle detection does have existing test coverage, and those tests still pass, it just takes a little longer to detect the cycle internally.)
There is still no way to create subprojects in the UI.
Test Plan: Added and executed unit tests.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14862
Summary:
Ref T10010. This does some cleanups on the schema:
- `viewPolicy`, `editPolicy` and `joinPolicy` were nullable, but should never be `null`. Set them to defaults if they're null, then make the column non-nullable.
- Rename `phrictionSlug` to `primarySlug` and stop adding and removing trailing slashes from it.
- Add new columns to support milestones and non-milestone subprojects.
- Drop very old subprojectPHIDs column. This hasn't done anything in the UI for years and years, and isn't particularly realistic to migrate forward.
The new columns aren't reachable from the UI.
Test Plan:
- Applied patches.
- Grepped for `phrictionSlug`.
- Grepped for `subprojectPHIDs`.
- Created tasks.
- Edited tasks.
- Verified existing tasks still had primary slugs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14825
Summary: Ref T10004. This lost a couple of fields when I rearranged how descriptions work. Restore them.
Test Plan:
- Viewed "Using HTTP Parameters".
- Everything had nice descriptions.
- No more weird phantom/misleading 'comment' transaction in UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14822
Summary:
At least for now, the "Space" field is just a subfield of the "Visible To" field, so:
- it doesn't get any separate settings; and
- it always uses the "Visible To" settings.
Test Plan:
- Created a form with a hidden view policy field.
- Created stuff with no "you must pick a space" errors.
- Created stuff with a normal form.
- Prefilled "Space" on a noraml form.
- Verified that trying to prefill "Space" on a form with "Visible To" hidden does nothing.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14812
Summary:
Ref T9964. Three goals here:
- Make it easier to supply Conduit documentation.
- Make automatic documentation for `*.edit` endpoints more complete, particularly for custom fields.
- Allow type resolution via Conduit types, so you can pass `["alincoln"]` to "subscribers" instead of needing to use PHIDs.
Test Plan:
- Viewed and used all search and edit endpoints, including custom fields.
- Used parameter type resolution to set subscribers to user "dog" instead of "PHID-USER-whatever".
- Viewed HTTP parameter documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14796
Summary: Fixes T9988. This logic got inverted by accident at some point.
Test Plan:
- Edited a task, shifting spaces.
- Created a task in default space.
- Created a task in custom space.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9988
Differential Revision: https://secure.phabricator.com/D14787
Summary: Ref T9964. This just adds more structure to application fields, to make it harder to make typos and easier to validate them later.
Test Plan: Viewed APIs, called some APIs, saw good documentation and correct results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14776
Summary:
Ref T9964. Building tables in Remarkup is kind of neat-ish but ends up feeling kind of hacky, and requires weird workarounds if any of the values have `|` in them.
Switch to normal elements instead.
Also move the magic "ids" and "phids" to be more like real fields. I'll clean this up fully in a diff or two, it's just a little tricky because Maniphest has an "ids" field.
Test Plan: {F1024294}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14768
Summary:
Ref T9964. See that task for some context and discussion.
Ref T7715, which has the bigger picture here.
Basically, I want Conduit read endpoints to be full-power, ApplicationSearch-driven endpoints, so that applications can:
- Write one EditEngine and get web + conduit writes for free.
- Write one SearchEngine and get web + conduit reads for free.
I previously made some steps toward this, but this puts more of the structure in place.
Test Plan:
Viewed API console endpoint and read 20 pages of docs:
{F1021961}
Made various calls: with query keys, constraints, pagination, and limits.
Viewed new {nav Config > Modules} page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7715, T9964
Differential Revision: https://secure.phabricator.com/D14743
Summary:
Ref T9132. Ref T9908. Fixes T5622. This allows you to copy some fields (projects, subscribers, custom fields, some per-application) from another object when creating a new object by passing the `?template=xyz` parameter.
Extend "copy" support to work with all custom fields.
Test Plan:
- Created new pastes, packages, tasks using `?template=...`
- Viewed new template docs page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5622, T9132, T9908
Differential Revision: https://secure.phabricator.com/D14699
Summary:
Ref T9132. Currently, EditEngine had some branchy-`instanceof` code like this:
```
if ($object instanceof Whatever) {
do_magic();
}
if ($object instanceof SomethingElse) {
do_other_magic();
}
```
...where `Whatever` and `SomethingElse` are first-party applications like ProjectsInterface and SubscribersInterface.
This kind of code is generally bad because third-parties can't add new stuff, and it suggest something is kind of hacky in its architecture. Ideally, we would eventually get rid of almost all of this.
T9789 is a similar discussion of this for the next layer down (`TransactionEditor`) and plans to get rid of branchy-instanceofs there too.
Since I'm about to add more stuff here (for Custom Fields), split it out first so I'm not digging us any deeper than I already dug us.
Broadly, this allows third-party extensions to add fields to every EditEngine UI if they want, like we do for Policies, Subscribers, Projects and Comments today (and CustomFields soon).
Test Plan:
{F1007575}
- Observed that all fields still appear on the form and seem to work correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14599
Summary: Move some `PhabricatorPolicyRule` implementations to a subdirectory of the parent application.
Test Plan: N/A
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14478
Summary: Ref T9494. Depends on D14216. Remove 10 copies of this code.
Test Plan: Ran `arc unit --everything`, browsed Config > Modules, clicked around Herald / etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9494
Differential Revision: https://secure.phabricator.com/D14217
Summary:
Ref T7173. Depends on D14049. Now that Phacility can install custom exception handlers, this puts enough information on the exception so that we can figure out what to do with it.
- Generally modernize some of this code.
- Add some more information to PolicyExceptions so the new RequestExceptionHandler can handle them properly.
Test Plan: Failed authorizations, then succeeded authorizations. See next diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7173
Differential Revision: https://secure.phabricator.com/D14050
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.
Test Plan: Poked around a bunch of pages.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13589
Summary: Use `PhutilClassMapQuery` where appropriate.
Test Plan: Browsed around the UI to verify things seemed somewhat working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13429
Summary: Ref T8099, Missed this when testing Spaces. Adds proper list formatting.
Test Plan: Review Policy when object is higher or lower than default.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13478
Summary: Ref T8099, Cleans up UI issues, adds `appendList` and renders lists and paragraphs with Remarkup UI.
Test Plan: Test Policy Dialogs, other various dialogs.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13463
Summary:
Fixes T6787. I'm kind of cheating a little bit here by not unifying default selection with `initializeNew(...)` methods, but I figure we can let this settle for a bit and then go do that later. It's pretty minor.
Since we're not doing templates I kind of want to swap the `'template'` key to `'type'` so maybe I'll do that too at some point.
@chad, freel free to change these, I was just trying to make them pretty obvious. I //do// think it's good for them to stand out, but my approach is probably a bit inconsistent/heavy-handed in the new design.
Test Plan:
{F525024}
{F525025}
{F525026}
{F525027}
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: johnny-bit, joshuaspence, chad, epriestley
Maniphest Tasks: T6787
Differential Revision: https://secure.phabricator.com/D13387
Summary:
Ref T8449. Before we show a policy hint in the header of an object, compare it to the space policy (if one exists).
If the space policy is strictly stronger (more restrictive -- for example, the Space policy is 'members of X', and the object policy is 'public'), show the space policy instead.
See discussion on T8376.
Test Plan: {F509126}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13328
Summary: Ref T8449. When an object is in a space, explain that clearly in the policy description dialog.
Test Plan: {F496126}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8449
Differential Revision: https://secure.phabricator.com/D13264
Summary: All classes should extend from some other class. See D13275 for some explanation.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13283
Summary:
Ref T5681. Getting this to work correctly is a bit tricky, mostly because of the policy checks we do prior to applying an edit.
I think I came up with a mostly-reasonable approach, although it's a little bit gross. It uses `spl_object_hash()` so it shouldn't be able to do anything bad/dangerous (the hints are strictly bound to the hinted object, which is a clone that we destroy moments later).
Test Plan:
- Added + ran a unit test.
- Created a task with a "Subscribers" policy with me as a subscriber (without the hint stuff, this isn't possible: since you aren't a subscriber *yet*, you get a "you won't be able to see it" error).
- Unsubscribed from a task with a "Subscribers" policy, was immediately unable to see it.
- Created a task with a "subscribers" policy and a project subscriber with/without me as a member (error / success, respectively).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681
Differential Revision: https://secure.phabricator.com/D13259
Summary:
Ref T8488. Ref T5681.
Now you can just use `id(new ConpherenceThreadMembersPolicyRule())->getObjectPolicyFullKey()` as a policy.
Added tests for TaskAuthor + ThreadMembers.
Test Plan:
- Ran tests.
- Set a thread policy to "Members of thread'.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13258
Summary:
Ref T5681. Ref T8488. This allows policy rules to provide "Object Policies", which are similar to the global/basic policies:
- They show up directly in the dropdown (you don't have to create a custom rule).
- They don't need to create or load anything in the database.
To implement one, you just add a couple methods on an existing PolicyRule that let Phabricator know it can work as an object policy rule.
{F494764}
These rules only show up where they make sense. For example, the "Task Author" rule is only available in Maniphest, and in "Default View Policy" / "Default Edit Policy" of the Application config.
This should make T8488 easier by letting us set the default policies to "Members of Thread", without having to create a dedicated custom policy for every thread.
Test Plan:
- Set tasks to "Task Author" policy.
- Tried to view them as other users.
- Viewed transaction change strings.
- Viewed policy errors.
- Set them as default policies.
- Verified they don't leak into other policy controls.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681, T8488
Differential Revision: https://secure.phabricator.com/D13257
Summary:
Ref T5681. Policy rules can now select objects they can apply to, so a rule like "task author" only shows up where it makes sense (when defining task policies).
This will let us define rules like "members of thread" in Conpherence, "subscribers", etc., to make custom policies more flexible.
Notes:
- Per D13251, we need to do a little work to get the right options for policies like "Maniphest > Default View Policy". This should allow "task" policies.
- This implements a "task author" policy as a simple example.
- The `willApplyRule()` signature now accepts `$objects` to support bulk-loading things like subscribers.
Test Plan:
- Defined a task to be "visible to: task author", verified author could see it and other users could not.
- `var_dump()`'d willApplyRule() inputs, verified they were correct (exactly the objects which use the rule).
- Set `default view policy` to a task-specific policy.
- Verified that other policies like "Can Use Bulk Editor" don't have these options.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5681
Differential Revision: https://secure.phabricator.com/D13252
Summary:
Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:
- `PolicyFilter` now checks Spaces.
- `PolicyAwareQuery` now automatically adds Spaces constraints.
There's one interesting design decision here: **spaces are stronger than automatic capabilities**. That means that you can't see a task in a space you don't have permission to access, //even if you are the owner//.
I //think// this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.
It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.
For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).
This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.
Test Plan:
- Added and executed unit tests.
- Put objects in spaces and viewed them with multiple users.
- Made the default space visible/invisible, viewed objects.
- Checked the services panel and saw `spacePHID` constraints.
- Verified that this adds only one query to each page.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13156
Summary:
Ref T8424. When users are rejected because they can't see the space an object is in, this isn't really a capability rejection. Don't require a capability when rejecting objects.
This mostly simplifies upcoming changes.
Test Plan:
- Viewed a capability exception dialog, it looked the same as always.
- (After additional changes, viewed a space exception dialog.)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13155
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
