Summary:
Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:
- Introduces "create" and "create global" permisions for Herald.
- Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
- Add some application policy helper functions.
- Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
- Don't let users who can't use application X create Herald rules for application X.
- Remove Maniphest/Pholio rules when those applications are not installed.
Test Plan:
- Restricted access to Maniphest and uninstalled Pholio.
- Verified Pholio rules no longer appear for anyone.
- Verified Maniphest ruls no longer appear for restricted users.
- Verified users without CREATE_GLOBAL can not create global ruls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7219
Summary: Ref T603. This could be a nicer UX, but limit the amount of foot-shooting that users can possibly do. You can still manage if you're really tricky ("Members of project X", then leave the project) but this should make it hard to make a mistake. It seems very unlikely any user ever intends to lock themselves out of an application.
Test Plan: Set an application's view policy to permissive ("Administrators") and nonpermissive ("No One") values. The former were accepted, the latter rejected.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7218
Summary: Right now emails don't include bookmark info (wasn't added in D2897). Lets include it so it's consistent with the web UI.
Test Plan: Inspected code, made sure it matched web UI code. Verified that web UI with these changes was consistent with rendering before refactoring.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7215
Summary:
Does what it says on the label. We already had 'Any changed file content', now we have 'Any added file content' and 'Any removed file content'.
- There is a bit of copied/pasted code here: I'm open to suggestions on how to refactor it so it's less redundant.
- The wording seems a little awkward, and as @epriestley mentioned in T3829, moved code will be detected less than ideally.
Test Plan: Created Herald Rules, verified via dry run that they were triggered in appropriate situations.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3829
Differential Revision: https://secure.phabricator.com/D7214
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.
This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.
Test Plan:
- Added and executed unit tests.
- Executed most of the tests manually, by using logged in / admin / public / disabled users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7211
Summary:
Fixes T1461.
Adds
- FIELD_ALWAYS - now you could add this to a content type to always get notified
- FIELD_REPOSITORY_AUTOCLOSE_BRANCH - solves T1461
- CONDITION_UNCONDITIONALLY - used by these two fields to not show any value for the user to select
Test Plan: made a herald rule where diffs on autoclose branches would get flagged blue. made a diff on an autoclose branch and committed it. commit was flagged!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1461
Differential Revision: https://secure.phabricator.com/D7210
Summary:
In most cases this just makes the URIs more consistent, but it's funky/breakish for SVN repositories which are only partially tracked.
See also T3915, and IRC.
Test Plan:
- Browsed some repositories, verified URIs generated as expected, with trailing slashes for directories.
- Verified nothing goofy happened in the extremes (like double slashes on the first crumb).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7209
Summary: Ref T603. If you get in trouble, `bin/policy unlock PHID-APPS-PhabricatorApplicationDifferential` and such can get you out now.
Test Plan: Unlocked an application.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7206
Summary:
Ref T603. Enables:
- Application policies can be edited.
- Applications can define custom policies (this will be used for setting defaults, like "what is the default visibiltiy of new tasks", and meta-policies, like "who can create a task?").
Test Plan: Edited application policies. A future diff does more with custom policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7205
Summary:
Ref T603. Broadly, this allows you to implement a policy like "Only users in Engineering can use Differential."
This isn't complete, and there will be a long tail of special cases to deal with. Some examples:
- If you can't use Differential, should you still be able to attach/detach revisions from tasks?
- You currently will be able to.
- This actually seems pretty reasonable.
- But in other cases it might not be: the "send user a message" action should probably require access to Conpherence.
- If you can't use Differential, should you still be able to see feed stories about it?
- You currently will be able to, if you can see the revisions.
- This seems not-so-reasonable and we should probably lock it down.
- If you can't use Differential, can users CC you on revisions?
- Currently, they can, and you can't do anything about it.
- Probably they shouldn't be able to? This seems challenging to explain in the UI.
- If you can't use Differential, can you write a Herald rule against it?
- You currently will be able to.
- Seems like you obviously shouldn't be able to.
- I think this is a general issue right now (you can still write Differential herald rules even if you uninstall the application, I believe).
There are probably a few more things I haven't thought of. However, there are a finite number of these things and I suspect there aren't //too/ many more than this -- I can't come up with like 100 of them, and half of the ones above have easy fixes.
Despite the rough edges, I think this accomplishes 95% of what installs expect from it.
Test Plan: Restricted Differential and saw it vanish from the home page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7203
Summary:
I'm just going to store application policy settings (like view/edit policy, and default policies for content) in config, because:
1) We'll need access to it on every page, and Config is "free" since we already pull it.
2) Building separate storage and transactions seems like overkill, we get less-nice but pretty-reasonable transactions for free with config.
3) We could easily move it later if this is a bad call.
Also fix some formatting.
Test Plan: See future revisions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7202
Summary: Make the application query a little more flexible, and formalize the PHID type.
Test Plan: See next diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7201
Test Plan: Confirm the API returns a single flat result with a unified git diff.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran, charles
Differential Revision: https://secure.phabricator.com/D7199
Test Plan:
Enable inline patches:
```
bin/config set metamta.differential.patch-format 'unified'
bin/config set metamta.differential.inline-patches 100000000
```
Create a new diff and confirm it renders correctly via email.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7198
Summary: Cleans up jump nav so it doesn't hard code a bunch of application behaviors. It still hard-codes a few, but few//er//?
Test Plan: Jumped to stuff like `D12`, `d`, `@dog`, `p admins only`, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7196
Summary: Ref T603. This didn't impact policies anyway, but using PhabricatorObjectQuery is far simpler and more general.
Test Plan: Used "Attach" dialog to find mocks, tasks, and revisions by "Dxx", "Mxx", etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7195
Summary: I removed the only callsite in D7179, but forgot to remove this code.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7194
Summary: Ref T603. Make this rule properly policy-aware, and extend from `PhabricatorRemarkupRuleObject`.
Test Plan:
- Embedded an image, tested all options (name, link, float, layout, size).
- Used lightbox to view several images.
- Embedded a text file, tested all options (name).
- Embedded audio, tested all options (loop, autoplay).
- Attached a file via comment to a task, verified edge was created.
- Attached a file via comment to a conpherence, verified edge was created.
- Viewed old files, verified remarkup version bump rendered them correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7192
Summary: Ref T603. When a user comments on an object with an embedded file, write an "attached" edge.
Test Plan: Made a comment on a task with an embedded file, verified the edge was written in Files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7191
Summary: Ref T603. We might need a fine-grained CLI tool later on, but here's a bat we can bludgeon things with.
Test Plan:
- Ran `bin/policy unlock D12` (adjusted policies).
- Ran `bin/policy unlock rPca85c457ebcb` (got "not mutable" stuff).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7189
Summary: Remove user image background color, fix spacing in titles.
Test Plan: Tested a task and a pholio mock, various states.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7190
Summary:
I use color to convey meaning like "good resource to keep handy for a bit on new way of doing things" or "snipe this task". Now the list can be grouped by these colors.
Note I do this in PHP 'cuz color isn't part of any index AFAIK and pragmatically speaking this dataset should be tiny in the context of "user flags".
Ref T1809
Test Plan: selected group by color and observed the flags were indeed grouped by color
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7188
Summary: Depends on D7163. This adds a "Stop Tracking" link to the right-hand side of ongoing entries in the Phrequent search view. It allows users to stop tracking items without first navigating to the item itself.
Test Plan: Started tracking and item and then clicked the "Stop Tracking" link in Phrequent.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3870
Differential Revision: https://secure.phabricator.com/D7164
Summary:
This updates Phrequent to use new the search infrastructure. Now it looks like:
{F60141}
I've also added the policy infrastructure stubs, but it's probably not even close to being right in terms of enforcing policies (in particular being able to see time tracked against objects the user wouldn't normally be able to see).
At some point I'd like to be able to filter on the objects that the time is tracked against, but I don't believe there's a tokenizer / readahead control that allows you to type any kind of object.
Test Plan: Clicked around the new interface, created some custom queries and saved them.
Reviewers: epriestley
CC: Korvin, aran
Maniphest Tasks: T3870
Differential Revision: https://secure.phabricator.com/D7163
Summary:
Ref T3903. Ref T603. We currently overreact to invalid policies. Instead:
- For non-omnipotent users, just reject the viewer.
- For omnipotent users, we already shortcircuit and permit the viewer.
- Formalize and add test coverage for these behaviors.
Also clean up some strings.
The practical effect of this is that setting an object to an invalid policy (either intentionally or accidentally) doesn't break callers who are querying it.
Test Plan:
- Created a Legalpad document and set view policy to "asldkfnaslkdfna".
- Verified this policy behaved as though it were "no one".
- Added, executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T3903
Differential Revision: https://secure.phabricator.com/D7185
Summary:
Currently, if you attach a revision to a task and the revision has a title with quotes or angle brackets in it, they are over-escaped in the email.
Instead, don't do that.
Test Plan: Attached `"QUOTES" MATH: 1 < 2` to a task, got a reasonable looking email.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7186
Summary:
Ref T603. Principally, I want to implement the rule "when you upload a file to an object, users must be able to see the object in order to see the file", since I think this is strongly in line with user expectation. For example, if you attach a file to a Conpherence, it should only be visible to members of that thread.
This adds storage for policies, but doesn't do anything interesting with it yet.
Test Plan: Ran `bin/storage upgrade`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7175
Summary:
Ref T603. This uses the existing edges (from Conpherence) to record that a file is attached to an object, and uses those edges to create a policy exception: if you can view an attached object, you can view a file.
I'm going to combine this with restrictive defaults to satisfy the other half of the equation (that files you attach to a conpherence usually shouldn't be public by default).
Test Plan:
- Loaded `/files/`.
- Uploaded a file to a Conpherence, looked at it in Files, saw the attachment.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7182
Summary:
- "revision" is misspelled.
- Remove an unused variable.
Test Plan: Used API console to call method.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7184
Summary: See D7162. This was like 99% my fault. Just provide a header; the new ones look pretty reasonable.
Test Plan: Viewed Diffusion change view, no exception.
Reviewers: vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7183
Summary: Ref T603. This has some custom logic which ObjectQuery can now perform more simply and more correctly.
Test Plan: Ran `bin/files purge F1`, `bin/files purge D1`, `bin/files purge --all`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7180
Summary: Ref T603. Clean these up and move them to a single place.
Test Plan:
- Downloaded a raw diff.
- Enabled "attach diffs", created a revision, got an email with a diff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7179
Summary: Ref T603. Swaps out most `PhabricatorFile` loads for `PhabricatorFileQuery`.
Test Plan:
- Viewed Differential changesets.
- Used `file.info`.
- Used `file.download`.
- Viewed a file.
- Deleted a file.
- Used `/Fnnnn` to access a file.
- Uploaded an image, verified a thumbnail generated.
- Created and edited a macro.
- Added a meme.
- Did old-school attach-a-file-to-a-task.
- Viewed a paste.
- Viewed a mock.
- Embedded a mock.
- Profiled a page.
- Parsed a commit with image files linked to a revision with image files.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7178
Summary:
Fixes T3894. The "Log Out" icon has moved away from its rightmost position in the menubar.
In rP2e5ac12, I added a "Policy" application. This was the root cause.
The reordering logic (below) is slightly wrong. The `array_select_keys()` call is actually using the //strings// (like "Admnistration") to select the groups, not the correct constants (like "admin"). Use the constants instead and get the expected group ordering.
Test Plan: Loaded page, "Log Out" is in the rightmost position.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3894
Differential Revision: https://secure.phabricator.com/D7177
Summary:
Three changes here.
- Add `setActionList()`, and use that to set the action list.
- Add `setPropertyList()`, and use that to set the property list.
These will let us add some apropriate CSS so we can fix the border issue, and get rid of a bunch of goofy `.x + .y` selectors.
- Replace `addContent()` with `appendChild()`.
This is just a consistency thing; `AphrontView` already provides `appendChild()`, and `addContent()` did the same thing.
Test Plan:
- Viewed "All Config".
- Viewed a countdown.
- Viewed a revision (add comment, change list, table of contents, comment, local commits, open revisions affecting these files, update history).
- Viewed Diffusion (browse, change, history, repository, lint).
- Viewed Drydock (resource, lease).
- Viewed Files.
- Viewed Herald.
- Viewed Legalpad.
- Viewed macro (edit, edit audio, view).
- Viewed Maniphest.
- Viewed Applications.
- Viewed Paste.
- Viewed People.
- Viewed Phulux.
- Viewed Pholio.
- Viewed Phame (blog, post).
- Viewed Phortune (account, product).
- Viewed Ponder (questions, answers, comments).
- Viewed Releeph.
- Viewed Projects.
- Viewed Slowvote.
NOTE: Images in Files aren't on a black background anymore -- I assume that's on purpose?
NOTE: Some jankiness in Phortune, I'll clean that up when I get back to it. Not related to this diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7174
Summary: Fixes 2x white icons, adds 'user' and 'project' icons.
Test Plan: tested new states in Maniphest
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7176
Summary:
Ref T603. I want to provide at least a basic CLI tool for fixing policy problems, since there are various ways users can lock themselves out of objects right now. Although I imagine we'll solve most of them in the application eventually, having a workaround in the meantime will probably make support a lot easier.
This implements `bin/policy show <object>`, which shows an object's policy settings. In a future diff, I'll implement something like `bin/policy set --capability view --policy users <object>`, although maybe just `bin/policy unlock <object>` (which sets view and edit to "all users") would be better for now. Whichever way we go, it will be some blanket answer to people showing up in IRC having locked themselves out of objects which unblocks them while we work on preventing the issue in the first place.
Test Plan: See screenshot.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7171
Summary:
- Fixes line height when many long tasks are attached to a task.
- Tightens up mobile layout of timeline and object box
- Clean up aphront context bar
Test Plan: Tested all the changes, made tasks, stared at pixels.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3891
Differential Revision: https://secure.phabricator.com/D7169
Summary: Missed this case in my sandbox
Test Plan: Reload a test diff
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7168
Summary: This adds the 'PHUIObjectBox' to nearly every place that should get it. I need to comb through Diffusion a little more. I've left Differential mostly alone, but may decide to do it anyways this weekend. I'm sure I missed something else, but these are easy enough to update.
Test Plan: tested each new layout.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7162
Summary:
Fixes T3887. Two issues:
- Macros were generating entirely before the render cache, so audio macros worked fine in previews and the first time the cache was populated, but not afterward.
- Instead, parse them before the cache but drop them in after the cache. Clean up all the file querying, too. This makes cached remarkup generate the correct audio beahviors.
- Safari sends an HTTP request with a "Range" header, and expects a "206 Partial Content" response. If we don't give it one, it sometimes has trouble figuring out how long a piece of audio is (mostly for longer clips? Or mostly for MP3s?). I'm not exactly sure what triggers it. The net effect is that "loop" does not work when Safari gets confused. While looping a short "quack.wav" worked fine, longer MP3s didn't loop.
- Supporting "Range" and "206 Partial Content", which is straightforward, fixes this problem.
Test Plan:
- Viewed a page with lots of different cached audio macros and lots of different uncached preview audio macros, they all rendered correctly and played audio.
- Viewed a macro with a long MP3 audio loop in Safari. Verified it looped after it completed. Used Charles to check that the server received and responded to the "Range" header correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3887
Differential Revision: https://secure.phabricator.com/D7166
