1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-11 17:32:41 +01:00
Commit graph

4421 commits

Author SHA1 Message Date
epriestley
f010730e49 Migrate all Differential inline comments to ApplicationTransactions
Summary:
Ref T2222. This implements step (1) described there, which is moving over all the inline comments.

The old and new tables are simliar. The only real trick here is that `transactionPHID` and `legacyCommentID` mean roughly the same thing (`null` if the inline is a draft, non-null if it has been submitted) but we don't have real `transactionPHID`s yet. We just make some up -- we'll backfill them later.

Two risks here:

  - I need to take a second look at the keys on this table. I think we need to tweak them a bit, and it will be less disruptive to do that before this migration than after.
  - This will take a while for Facebook, and other large installs with tens of thousands of revisions. I'll communicate this.

I'm otherwise pretty satisfied with this, seems to work well and is pretty low risk / non-disruptive.

Test Plan:
  - Before migrating, then after migrating:
    - Made a bunch of inlines (drafts, submitted).
    - Edited and deleted inlines.
    - Verified inlines showed up in preview.
    - Verified that inlines aren't indexed when they're drafts (`bin/search index D935`).
    - Verified that inlines ARE indexed when they're not drafts.
    - Verified that drafts inlines make revisions appear as "with draft" in the revision list.
  - Made left, right, and draft inlines.
  - Migrated (`bin/storage upgrade`).
  - Verified that my inlines from before the migration still showed up.
  - (Repeated all the stuff above.)
  - Manually inspected the inline comment table.

Reviewers: btrahan

Reviewed By: btrahan

CC: FacebookPOC, aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D7139
2013-10-19 05:03:25 -07:00
epriestley
7180199d13 Fix daemon auth issue
Summary: I touched this code recently but it needs an unusual special case because we call through with the "omnipotent user" from the daemons. As per the TODO below, this will all get cleaned up at some point.

Test Plan: Will make @poop verify.

Reviewers: btrahan, poop

Reviewed By: poop

CC: poop, aran

Differential Revision: https://secure.phabricator.com/D7356
2013-10-18 18:29:36 -07:00
epriestley
7e815a06f8 Fix audio editing!
Summary: This capability was replaced with an application-wide "manage" capability. It's checked for just above.

Test Plan: Edited audio!

Reviewers: btrahan, ljalonen, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7353
2013-10-18 16:23:16 -07:00
epriestley
943080a4de Make Phrequent time accounting aware of the stack
Summary:
Ref T3569. Fixes T3567. When figuring out how much time has been spent on an object, subtract "preemptive" events which interrupted the object.

Also, make the UI look vaguely sane:

{F72773}

Test Plan: Added a bunch of unit tests, mucked around in the UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, skyronic, aran

Maniphest Tasks: T3567, T3569

Differential Revision: https://secure.phabricator.com/D7349
2013-10-18 12:47:36 -07:00
epriestley
0b22777f68 Remove UI warnings about policies being a janky mess
Summary: Ref T603. While policies aren't completely perfect, they are substantially functional to the best of my knowledge -- definitely in good enough shape that we want to hear about issues with them, now.

Test Plan: Edited a task, repository, and project.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7343
2013-10-17 12:59:40 -07:00
epriestley
5171e3684c Require application "Can Use" capability to call Conduit methods
Summary: Ref T603. If you don't have access to an application, prevent execution of its (authenticated) methods.

Test Plan: Restricted Tokens to only admins, then tried to view/call Token methods as a non-admin.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7342
2013-10-17 12:51:19 -07:00
epriestley
32dca4b553 Fix lightbox downloads for embeded images and a warning
Summary:
I refactored this recently and accidentally dropped the download URI.

Also fix a warning with, e.g., files named `README`.

Test Plan: Clicked a thumb, clicked "Download", got a file.

Reviewers: chad, btrahan, dctrwatson

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7341
2013-10-17 11:21:01 -07:00
epriestley
95c2b03fc8 Distinguish between invalid/broken handles and filtered handles
Summary:
Ref T603. Currently, we render handles the user doesn't have permission to see in a manner identical to handles that don't exist. This is confusing, and not required by policies (which restrict content, but permit knowledge that an object exists).

Instead, render them in different styles. Bad/invalid objects look like:

  Unknown Object (Task)

Restricted objects look like:

  [o] Restricted Task

...where `[o]` is the padlock icon.

Test Plan:
{F71100}

{F71101}

It's possible this renders weird somewhere, but I wasn't immediately able to find any issues. Yell if you see something.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7334
2013-10-17 10:49:21 -07:00
epriestley
cf8d7da292 Add some missing non-context translations
Summary: We're missing some strings here.

Test Plan: {F71857}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7340
2013-10-17 10:48:44 -07:00
epriestley
4f05736175 Add an icon+background selector for project images
Summary: Makes it easy to choose distinctive icons for projects.

Test Plan:
{F71018}

{F71020}

{F71019}

{F71021}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7333
2013-10-17 09:32:34 -07:00
Jakub Vrana
a9a3bdf3cc Delete unintentional phlog()
Leaked in D7329.
2013-10-16 13:59:23 -07:00
Chad Little
89d35b98c8 Misc Diffusion/Differential CSS tweaks
Summary: Various tweaks and fixes. Adds a File Contents view in Diffusion, normalizes spaces, colors.

Test Plan: tested differential and diffusion in my sandbox.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3952

Differential Revision: https://secure.phabricator.com/D7325
2013-10-16 13:09:12 -07:00
epriestley
314673f4f6 Fix an issue with rendering PHID lists containing null in Maniphest
Summary: See IRC. Someone got a `null` in CCPHIDs somehow. Moving to subscriptions should prevent this, but paper over it for now.

Test Plan: Will have @dctrwatson check.

Reviewers: btrahan, dctrwatson

Reviewed By: btrahan

CC: dctrwatson, aran

Differential Revision: https://secure.phabricator.com/D7330
2013-10-16 12:46:34 -07:00
epriestley
3410cbd53e Add application and object level policy controls to Countdown
Summary: Ref T603. Give countdowns proper UI-level policy controls, and an application-level default policy. Put policy information in the header.

Test Plan:
  - Adjusted default policy.
  - Created new countdowns.
  - Edited countdowns.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7322
2013-10-16 10:36:08 -07:00
epriestley
e381022bc7 Provide application and object level policy controls in Slowvote
Summary: Ref T603. Gives the create/edit interface a policy control, and adds an application-level default.

Test Plan: Created and edited polls.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7321
2013-10-16 10:36:00 -07:00
epriestley
8c1c6fec5a Modernize policies in Paste and Macro
Summary:
Ref T603. Fixes T2823. This updates Paste and Macro.

  - **Paste**
    - Added default view policy.
    - I didn't add a "create" policy, since I can't come up with any realistic scenario where you'd give users access to pastes but not let them create them.
  - **Macro**
    - Added a "manage" policy, which covers creating and editing macros. This lets an install only allow "People With An Approved Sense of Humor" or whatever to create macros.
    - Removed the "edit" policy, since giving individual users access to specific macros doesn't make much sense to me.
    - Changed the view policy to the "most public" policy the install allows.
    - Added view policy information to the header.

Also fix a couple of minor things in Maniphest.

Test Plan:
  - Set Paste policy, created pastes via web and Conduit, saw they got the right default policies.
  - Set Macro policy, tried to create/edit macros with valid and unauthorized users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2823, T603

Differential Revision: https://secure.phabricator.com/D7317
2013-10-16 10:35:52 -07:00
Jakub Vrana
29391a658e Disallow <! in <script>
Summary:
HTML5 has this crazy script escaping states:

- Script data escaped dash dash state
- Script data double escaped state

https://communities.coverity.com/blogs/security/2012/11/16/did-i-do-that-html-5-js-escapers-3

Perhaps `<!` is too aggressive but I didn't spend much time searching for a more fine grained expression.

Test Plan: Searched for `renderInlineScript()`.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7329
2013-10-16 09:28:37 -07:00
Chad Little
451dfb9afb Differential Changeset header icons
Summary: Adds filetype icons, applying to differential file headers. The main issue is with all the lightening, I wanted something to still anchor 'new file' on the page and adding a sharp icons does that pretty well for me. Feedback is cool too.

Test Plan: Add some new icons, test in previous commits.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7320
2013-10-15 07:34:48 -07:00
epriestley
19968e31f4 Generalize Herald account sources
Summary: The "user" and "user/project" sources exclude system agents and disabled users, but should not.

Test Plan: Added system agents to Herald rules.

Reviewers: btrahan, bigo

Reviewed By: bigo

CC: aran

Differential Revision: https://secure.phabricator.com/D7319
2013-10-14 19:38:35 -07:00
epriestley
76dfeb95ba Allow "Custom" policies to be selected in the policy control
Summary: Ref T603. When a user selects "Custom", we pop open the rules dialog and let them create a new rule or edit the existing rule.

Test Plan: Set some objects to have custom policies.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7300
2013-10-14 16:59:16 -07:00
epriestley
3a4c08d7f1 Simplify custom policies before saving, and reject meaningless policies
Summary:
Ref T603. Do a little more sanity checking on custom policies, so policies like this:

  [ Allow ] [ Users ] [ <no users> ]

...that don't specify anything and thus which aren't meaningful raise errors.

Test Plan: {F69570}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7314
2013-10-14 16:48:41 -07:00
Chad Little
d2895249ee Add Persona login icon
Summary: Adds the new icon 1x and 2x

Test Plan: photoshop

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3958

Differential Revision: https://secure.phabricator.com/D7316
2013-10-14 16:33:49 -07:00
Scott MacVicar
4d1709651e [releeph] Conduit failure with bad IDs
Summary:
Instead of returning a blank result it throws exceptions. Fix this up a
little so we get some consistency with differential

Test Plan:
Loaded a bad phid for releeph, returns empty list.
Try a good phid and get 2 releeph merges.

Reviewers: epriestley, elenaperezrioja, dschleimer, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7302
2013-10-14 16:07:17 -07:00
epriestley
c4abf160cc Fix some file policy issues and add a "Query Workspace"
Summary:
Ref T603. Several issues here:

  1. Currently, `FileQuery` does not actually respect object attachment edges when doing policy checks. Everything else works fine, but this was missing an `array_keys()`.
  2. Once that's fixed, we hit a bunch of recursion issues. For example, when loading a User we load the profile picture, and then that loads the User, and that loads the profile picture, etc.
  3. Introduce a "Query Workspace", which holds objects we know we've loaded and know we can see but haven't finished filtering and/or attaching data to. This allows subqueries to look up objects instead of querying for them.
    - We can probably generalize this a bit to make a few other queries more efficient. Pholio currently has a similar (but less general) "mock cache". However, it's keyed by ID instead of PHID so it's not easy to reuse this right now.

This is a bit complex for the problem being solved, but I think it's the cleanest approach and I believe the primitive will be useful in the future.

Test Plan: Looked at pastes, macros, mocks and projects as a logged-in and logged-out user.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7309
2013-10-14 14:36:06 -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
0ce4f6d176 Add Persona auth provider
Summary: Ref T3958. Adds a provider for Mozilla's Persona auth.

Test Plan:
  - Created a Persona provider.
  - Registered a new account with Persona.
  - Logged in with Persona.
  - Linked an account with Persona.
  - Dissolved an account link with Persona.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3958

Differential Revision: https://secure.phabricator.com/D7313
2013-10-14 14:34:57 -07:00
epriestley
502c6f2d48 Render public content as "Public" in headers, not "Public (No Login Required)"
Summary:
Ref T603. Although I think the parenthetical is valuable when //setting// policies to make sure no one accidentally opens content up, it's super annoying in headers.

This makes headers say "Public". Everything else still says "Public (No Login Required)".

Test Plan: {F69469}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7310
2013-10-14 14:17:25 -07:00
Bob Trahan
cb64bef3c5 Maniphest - make instructions !assign savvy
Summary: forgot this in last diff.  Ref T3937.

Test Plan: looks good

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3937

Differential Revision: https://secure.phabricator.com/D7308
2013-10-14 12:33:38 -07:00
Bob Trahan
d0127f95e5 Maniphest - add support for !assign command
Summary:
also try to centralize some of the command parsing logic. note that differential is still an exception here. it uses a whitelist-style regex. i think long-term we should have this for every app but changing it seemed too big for this diff.

Fixes T3937.

Test Plan:
echo '!assign btrahan' | ./bin/mail receive-test --as xerxes --to T22 ; echo '!claim' | ./bin/mail receive-test --as xerxes --to T22

unit tests passed, though my new one is silly

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3937

Differential Revision: https://secure.phabricator.com/D7307
2013-10-14 12:29:41 -07:00
epriestley
13178ec279 Prepare the policy rule edit endpoint for integration
Summary: Ref T603. Allow the endpoint to take an existing policy PHID to populate the editor and return a useful datastructure.

Test Plan: In the next revision, actually hooked this up.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7299
2013-10-14 12:07:31 -07:00
epriestley
5e5b7576a6 Make PhabricatorPolicyQuery a CursorPagedPolicyAwareQuery
Summary:
Ref T603. Make these actually implement policy interfaces, so shared infrastructure (like handle loading) works as expected. They don't actually have meaningful policies, and we short circuit all the checks.

(I don't plan to let you set policy controls on policies themselves)

Test Plan: Loaded handles for Policy objects via common infrastructure.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7298
2013-10-14 12:06:52 -07:00
epriestley
7364a3bedd Add some missing strings for custom policies
Summary: Ref T603. Fix/provide some rendering stuff related to custom policies.

Test Plan: After setting stuff to custom policies (made easier by future diffs), looked at the various places strings appear in the UI and saw more sensible ones.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7297
2013-10-14 12:05:43 -07:00
epriestley
6c1b00fa40 Rename ACTION_ACCEPT into ACTION_ALLOW
Summary: Ref T603. This is "Allow" in the UI, I just mistyped it when I created the constant.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7296
2013-10-14 12:05:11 -07:00
epriestley
67b17239b8 Allow custom policies to be loaded and exeucuted by the policy filter
Summary: Ref T603. Adds code to actually execute custom policies. (There's still no way to select them in the UI.)

Test Plan:
  - Added and executed unit tests.
  - Edited policies in existing applications.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7292
2013-10-14 11:46:14 -07:00
Chad Little
4733a8ef14 Add border, transparent indicators to images in file property view
Summary: Fixes T3950. This centers the images, adds a thin blue border, and a transparent background.

Test Plan: Tested a file in Files, Diffusion, and Macro.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3950

Differential Revision: https://secure.phabricator.com/D7305
2013-10-14 11:40:19 -07:00
Chad Little
503f413789 Clean up spacing on Diffusion headers
Summary: This adds some controllable space between paths in Diffusion headers. Fixes T3951

Test Plan: Tested new links in diffusion.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3951

Differential Revision: https://secure.phabricator.com/D7304
2013-10-14 09:40:05 -07:00
epriestley
5af031ec9b Make the policy control a JS dropdown with icons
Summary: Ref T603. After thinking about this for a bit I can't really come up with anything better than what Facebook does, so I'm going to implement something similar for choosing custom policies. To start with, swap this over to a JS-driven dropdown.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7285
2013-10-12 17:08:11 -07:00
Chad Little
538d8f27b5 Fix empty property list in Phriction
Summary: Fixes T3944

Test Plan: Will test on secure

Reviewers: epriestley, staticshock

CC: Korvin, epriestley, aran

Maniphest Tasks: T3944

Differential Revision: https://secure.phabricator.com/D7291
2013-10-11 14:20:21 -07:00
Chad Little
97c690fc0f PHUIPropertyListView
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.

Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7283
2013-10-11 07:53:56 -07:00
Asher Baker
f8d963a77e Rename "Upload Image" dialog to "Upload File" to match purpose
Summary: Fixes T3940.

Test Plan: Clicked button, looked at dialog.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: nmalcolm, Korvin, epriestley, aran

Maniphest Tasks: T3940

Differential Revision: https://secure.phabricator.com/D7287
2013-10-11 05:15:45 -07:00
epriestley
5899ae08b3 Add storage for custom policies
Summary: Ref T603. Allows custom policies to be saved. No integration with policy controls yet.

Test Plan:
  mysql> select * from policy where id = 3\G
  *************************** 1. row ***************************
             id: 3
           phid: PHID-PLCY-e4v2fnbyuibi4supl5tn
          rules: [{"action":"allow","rule":"PhabricatorPolicyRuleAdministrators","value":null},{"action":"allow","rule":"PhabricatorPolicyRuleProjects","value":["PHID-PROJ-cwovm5gn2ilubjehcdgd"]},{"action":"allow","rule":"PhabricatorPolicyRuleLunarPhase","value":"new"}]
  defaultAction: deny
    dateCreated: 1381437466
   dateModified: 1381437466
  1 row in set (0.00 sec)

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7282
2013-10-10 16:09:51 -07:00
Bob Trahan
db71bf6128 Fix issue reported from github
Summary:
we filter the $actors above such that its possible to have no $actor anymore (if $actor is not a deliverable email address). ergo, make sure we have actor before we start calling methods.

Fixes github issue 403

Test Plan: logic on this one - not 100% sure how to easily reproduce

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7284
2013-10-10 15:17:37 -07:00
Asher Baker
ff4fa1885b Improve pagination in ChatLog application
Summary:
- Add an extra paginator at the top.
- Add a link to jump to the bottom (where the latest messages are).
- Align paginators with edge of content rather than the page.

Test Plan: Looked at the chatlog.

Reviewers: epriestley, chad, #blessed_reviewers

CC: chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7280
2013-10-10 04:49:04 -07:00
Asher Baker
8cc64a9678 Add a 'create' policy to Project
Summary: UX on this could probably be better 'disabled' crumbs don't appear to have any visible difference, and the policy error has to load the /create page rather than being a modal - not sure on the way to fix these.

Test Plan: Tried to create a project with and without access, saw suitable error.

Reviewers: epriestley, #blessed_reviewers

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7279
2013-10-10 04:33:18 -07:00
epriestley
c39b10aa7a Fix non-public capabilities in Application edit
Summary: Ref T603. I nuked this check by accident and neglected to test the negative case.

Test Plan: Saved a non-public policy (Herald Global) and a public policy (Maniphest View).

Reviewers: asherkin, btrahan

Reviewed By: asherkin

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7278
2013-10-09 16:23:36 -07:00
Asher Baker
e3005fad09 Hide Audit information on Home when the application is uninstalled
Test Plan: Looked at Home with Audit installed and uninstalled.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7277
2013-10-09 15:25:03 -07:00
epriestley
f4582dc49d Allow "Default View" policies to be set to Public
Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.

Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.

Reviewers: btrahan, asherkin

Reviewed By: asherkin

CC: asherkin, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7276
2013-10-09 15:06:18 -07:00
epriestley
11fbd213b1 Custom Policy Editor
Summary:
Ref T603. This isn't remotely usable yet, but I wanted to get any feedback before I build it out anymore.

I think this is a reasonable interface for defining custom policies? It's basically similar to Herald, although it's a bit simpler.

I imagine users will rarely interact with this, but this will service the high end of policy complexity (and allow the definition of things like "is member of LDAP group" or whatever).

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran, asherkin

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7217
2013-10-09 14:05:10 -07:00
epriestley
436a403357 Add a "default view" policy to Differential
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.

I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.

Test Plan: Created new revisions from the CLI and conduit.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7269
2013-10-09 13:58:00 -07:00
epriestley
650dc0cc30 Remove the "create rules" Herald capability
Summary:
Ref T603. In thinking about this, I think I went mad with power in creating this capability. I can't imagine any reason to give users access to Herald but not let them create rules.

We can restore this later if some install comes up with a good reason to have it, but in the interest of keeping policies as simple as possible, I think we're better off without it. In particular, if you don't want a group of users creating rules, just lock them out of the application entirely.

The "Manage Global Rules" capability is still around, I think that one's super good.

Test Plan: Edited Herald policies, created a rule.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7268
2013-10-09 13:55:44 -07:00
epriestley
1ee455c441 Add defualt view and default edit policies for tasks
Summary: Ref T603. Allow global default policies to be configured for tasks.

Test Plan:
  - Created task via web UI.
  - Created task via Conduit.
  - Created task via email.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7267
2013-10-09 13:53:17 -07:00
epriestley
3147a6ca57 Improve messaging of special policy rules in applications
Summary: Ref T603. When the user encounters an action which is controlled by a special policy rule in the application, make it easier for applications to show the user what policy controls the action and what the setting is. I took this about halfway before and left a TODO, but turn it into something more useful.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7265
2013-10-09 13:52:04 -07:00
epriestley
45f38c549b Use header status/policy elements in Applications meta-application
Summary: Ref T603. Use more modern elements.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7264
2013-10-09 13:46:45 -07:00
epriestley
7a97a71e20 Move Herald application capabilities to newer infrastructure
Summary: Ref T603. Use the new hotness.

Test Plan: Edited Herald in Applications, tried to create rules / global rules without capabilities, got reasonable error messages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7263
2013-10-09 13:44:41 -07:00
Chad Little
fe73f9da73 Adds status colors to navbar tabs
Summary: Adds the abilit to set a status color of warning or fail to navbar tab lists (for objectheaders)

Test Plan: uiexamples, photoshop

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7266
2013-10-07 19:29:05 -07:00
epriestley
b1b1ff83f2 Allow applications to define new policy capabilities
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.

Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.

That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.

Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.

Also provide more context, and more useful exception messages.

Test Plan:
  - See screenshots.
  - Also triggered a policy exception and verified it was dramatically more useful than it used to be.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7260
2013-10-07 13:28:58 -07:00
epriestley
68c854b967 Remove dead rejectImpossiblePolicy() method
Summary: Ref T603. Apparently we made all policies possible at some point. Go us! This has no callsites.

Test Plan: `grep`, notice it's a private method

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7259
2013-10-07 12:52:01 -07:00
epriestley
0598600476 Always pass handles to tokenizers, not <phid -> name> maps
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.

Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7248
2013-10-07 12:51:24 -07:00
epriestley
2abbd51868 Don't raise a policy exception if a user can't see the parent revision of a new diff
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.

Test Plan: Created a new revision with `arc diff`.

Reviewers: ljalonen, btrahan, #blessed_reviewers, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7255
2013-10-07 12:51:04 -07:00
epriestley
c6f9316a77 Add missing case for personal ruls which create blocking reviewers
Summary: Ref T1279. I only tested the global case. :O

Test Plan: Created a personal "add me as blocking" rule.

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7261
2013-10-07 11:18:00 -07:00
epriestley
c4ecdfa2a5 Fix missing property on revision adapter. 2013-10-07 03:41:00 -07:00
epriestley
2a69b91000 Fix minor issue with data attachment on project creation 2013-10-06 17:13:51 -07:00
epriestley
953ff197bf Allow Herald rules to be disabled, instead of deleted
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.

  - Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
  - Track disables with transactions.
  - Gate disables with policy controls.
  - Show policy and status information in the headers.
  - Show transaction history on rule detail screens.
  - Remove the delete controller.
  - Support disabled queries in the ApplicationSearch.

Test Plan:
  - Enabled and disabled rules.
  - Searched for enabled/disabled rules.
  - Verified disabled rules don't activate.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279, T603

Differential Revision: https://secure.phabricator.com/D7247
2013-10-06 17:10:29 -07:00
epriestley
c6add6ae73 Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.

  - "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
  - "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".

Practically, the primary net effect of this is just to make blocking reviews actually block.

This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.

Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7245
2013-10-06 17:09:56 -07:00
epriestley
8aa8ef49da Provide an "Add blocking reviewer..." Herald action
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.

Test Plan: Added Herald rules and updated revisions; see screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7244
2013-10-06 17:09:24 -07:00
epriestley
929ad86b57 Allow accepting accepted revisions, and rejecting rejected revisions
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.

This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.

Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7243
2013-10-06 17:09:02 -07:00
epriestley
d518f3c9de When a user accepts a revision, accept for all projects the user has authority over
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.

Test Plan:
  - Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
  - Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.

Also, see screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7242
2013-10-06 17:08:30 -07:00
epriestley
3d3d3b6d80 Move determination of reviewer authority into DifferentialRevisionQuery
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:

  - It puts queries very close to the display layer.
  - We have to query for each revision if we want to figure out authority for several.
  - We need to figure it out in several places, so we'll end up with copies of this logic.
  - The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
  - We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.

Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.

Test Plan:
  - Looked at some revisions, verified the correct lines were highlighted.
    - Looked at a revision I created and verified that projects I was a member of were not highlighted.
      - With self-accept enabled, these //are// highlighted.
    - Looked at a revision I did not create and verified that projects I was a member of were highlighted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7241
2013-10-06 17:08:14 -07:00
epriestley
515f9a36ab When editing objects which use files, attach the files to the objects
Summary: Ref T603. Fixes T3921. Tightens up policy controls for file/object relationships in existing applications.

Test Plan:
  - Uploaded new project image, verified it got an edge to the project.
  - Uploaded new profile image, verified it got an edge to me.
  - Uploaded new macro image, verified it got an edge to the macro.
  - Uploaded new paste via web UI and conduit, verified it got attached.
  - Replaced, added images to a mock, verified they got edges.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3921, T603

Differential Revision: https://secure.phabricator.com/D7254
2013-10-06 17:07:55 -07:00
epriestley
c587b8a9c8 Remove ProjectProfile->loadProfileImageURI()
Summary: Ref T603. Gets rid of a sketchy, non-policy-aware, unbatched file query.

Test Plan: Looked at projects, typeahead, edited project profile images.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7253
2013-10-06 17:07:43 -07:00
epriestley
80f6d00940 Remove PhabricatorProject->loadProfile
Summary: Ref T603. Do modern, sensible queries here.

Test Plan: Viewed project profile, list, member edit, profile edit, used typeahead, changed project image.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7252
2013-10-06 17:07:20 -07:00
epriestley
64e4b3aef4 Remove loadMemberPHIDs from PhabricatorProject
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.

  - Fixes a bug with Asana publishing when the remote task is deleted.
  - Fixes an issue with Herald commit rules.

Test Plan:
  - Viewed projects;
  - edited projects;
  - added and removed members from projects;
  - republished Asana-bridged feed stories about commits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7251
2013-10-06 17:07:08 -07:00
epriestley
b7297a3278 Add an "Author's projects" Herald field to Differential
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:

  When:
    [ ... ] [ ... ]
  Take Action:
    [ Add blockign reviewers ] [ Security ]

...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:

  When:
    [ Author's projects ] [ do not include ] [ Security ]
    [ ... ] [ ... ]
  Take Action:
    [ Add blocking reviewers ] [ Security ]

...which means that people on the Security team don't need a separate signoff from someone else on the team.

I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.

@zeeg, not sure which of these use cases is relevant here, but either one should work after this.

Test Plan: Created rules with this field, verified it populated properly in the transcript.

Reviewers: btrahan

Reviewed By: btrahan

CC: zeeg, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7238
2013-10-05 14:10:53 -07:00
epriestley
c80a4f51c1 Highlight reviews the viewer is responsible for in Differential
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7237
2013-10-05 14:10:52 -07:00
epriestley
4c0ec01ce5 Allow Herald rules to add reviewers
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.

I will probably write some lengthy treatise on how you shouldn't use this rule later.

Implementation is straightforward because Differential previously supported this rule.

This rule can also be used to add project reviewers.

Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7235
2013-10-05 14:10:51 -07:00
epriestley
2d733f88a1 Split users apart from projects/packages in reviewer and audit UIs
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).

Test Plan:
  - Looked at a commit. Saw separation.
  - Looked at a revision. Saw separation.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7233
2013-10-05 14:10:49 -07:00
epriestley
9434df9d7c Accommodate project reviewers in Differential search
Summary:
Ref T1279. Two changes to the search/query for Differential:

  - "Reviewers" now accepts users and projects.
  - "Responsible Users" now includes revisions where a project you are a member of is a reviewer.

Test Plan:
  - Searched for project reviewers.
  - Verified that the dashboard now shows reviews which I'm only part of via project membership.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7231
2013-10-05 14:10:47 -07:00
epriestley
cf4eb3109e Allow projects to review revisions
Summary:
Ref T1279. No actual logical changes, but:

  - You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
  - You can now add projects as reviewers from the revision detail typeahead.
  - You can now add projects as reviewers from the CLI (`#yoloswag`).
  - Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).

I'll separate projects from users in the "Reviewers" tables in the next revision.

Test Plan:
  - Added projects as reviewers using the web UI and CLI.
  - Used `arc amend --show --revision Dnnn` to generate commit messages.
  - Viewed revision with project reviewers in web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7230
2013-10-05 14:10:46 -07:00
epriestley
370c7635a7 Track "accepted" and "commented" in per-reviewer status
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.

Test Plan:
  - Commented on a revision, got a comment icon.
  - Accepted a revision, got an accept icon.
  - Commented again, icon stayed as "accept".
  - Faked the "old diff" states.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7229
2013-10-05 14:10:45 -07:00
epriestley
4d8707df13 Use status list UI to show reviewers in Differential
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.

We currently keep track of only "requested changes".

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7228
2013-10-05 14:10:44 -07:00
epriestley
65ddefad8b Migrate all Differential reviewer data to edges
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.

Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.

I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.

Test Plan:
  - Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
  - Performed the migration. Verified that everything was still unchanged.
  - Dropped the edge table, verified all reviweer data vanished.
  - Migrated again, verified the reviewer stuff was restored.
  - Did various cc/reviewer/subscriber queries, got consistent results.

Reviewers: btrahan

Reviewed By: btrahan

CC: champo, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7227
2013-10-05 13:54:02 -07:00
epriestley
e6d8e1a00a Make Herald rules obey policies during application
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:

  When [Always], [Add me to CC]

...and end up getting email about everything. These rules are now enforced:

  - For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
  - In contrast, //global// rules will //always// trigger.

Also fixes some small bugs:

  - Policy control access to thumbnails was overly restrictive.
  - The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.

Test Plan:
  - Created "always cc me" herald and maniphest rules with a normal user.
  - Created task with "user" visibility, saw CC.
  - Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
  - Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
  - Created "user" and "no one" mocks and saw CC and no CC, respectively.
  - Thumbnail in Pholio worked properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7224
2013-10-05 12:55:34 -07:00
epriestley
7dde01df76 Fix issues with first-time account registration
Summary: This worked originally, but the migration broke slightly after the
config was deprecated, and there was another minor issue during setup.
2013-10-05 08:02:41 -07:00
epriestley
f88a2b735d Remove spurious "+x" from files that shouldn't have it
Summary: We have a bunch of files with +x that aren't actually executable.
Remove +x from PNGs, etc.
2013-10-05 05:18:17 -07:00
Juan Pablo Civile
562da7e98b Stop using loadRelationships in differential related herald adapters
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.

Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D6468

Conflicts:
	src/applications/herald/adapter/HeraldCommitAdapter.php
	src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
	src/applications/herald/controller/HeraldTestConsoleController.php
2013-10-04 16:40:42 -07:00
Bob Trahan
c35b93d9b6 lipsum - tighten up some test data generation
Summary:
maniphest tasks were fataling with priority 0 before making sure to add the return null if new object trick to the maniphest pro editor.

pholio had a problem where if you had no jpegs you were walking off array_rand. tighten the math and then just return a built-in if no uploaded user images could be found. Fixes T3889.

Test Plan: bin/lipsum generate for a few minutes and no errors

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3889

Differential Revision: https://secure.phabricator.com/D7222
2013-10-04 15:29:32 -07:00
epriestley
ee4bdb501b Make Herald transcripts policy-aware
Summary:
Ref T603. Herald transcripts potentially leak a bunch of content (task text, revision/commit content). Don't let users see them if they can't see the actual objects.

This is a little messy but ends up mostly reasonable-ish.

Test Plan:
  - Verified that transcripts for objects I couldn't see no longer appear in the list, and reject access.
  - Verified that transcripts for objects in applications I can't see reject access, albeit less gracefully.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7221
2013-10-04 15:17:18 -07:00
epriestley
a235768d58 Modernize "Test Console" and fix a minor display bug with "Always"
Summary:
  - Use the box view in the test console.
  - Let the test console load tasks and mocks. We should move this to the adapters (`canAdaptObject($object)` or something).
  - Fix a minor issue with "Always": hiding the whole cell could make the table layout weird in Safari, at least. Just hide the select instead.

Test Plan:
  - Used test console on task.
  - Used test console on mock.
  - Created (silly) rule with "Always" and also some other conditions.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7220
2013-10-04 15:17:01 -07:00
Chad Little
cad9e548bc Add Header to Registration
Summary: Adds an ObjectBox to Phabricator Registration

Test Plan: check logged out page for new header.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7223
2013-10-04 15:13:05 -07:00
epriestley
c8127edfe9 Tighten up some policy interactions in Herald
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
2013-10-04 15:15:48 -07:00
epriestley
a600ab7731 Prevent administrators from locking themselves out of applications
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
2013-10-04 15:13:58 -07:00
Neal Poole
1a5de83ad1 Add support for bookmarks in Phabricator emails.
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
2013-10-04 11:19:38 -07:00
epriestley
0883ea6977 Move "unlisted" apps to Query, use Query for app preferences
Summary:
  - Demagic "unlisted" apps.
  - Respect policies in application visibility settings.

Test Plan: Viewed applications, settings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7212
2013-10-04 06:46:47 -07:00
Neal Poole
1edb875978 Adding support for 'adds' and 'removes' in diff content.
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
2013-10-04 06:37:39 -07:00
epriestley
a6c4117ec4 Fix controller-level access rules
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
2013-10-03 19:05:47 -07:00
Bob Trahan
3cf17cc67f Herald - add field + condition for Diffusion Commits for "On autoclose branch"
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
2013-10-03 17:53:12 -07:00
epriestley
5200145b21 Add missing trailing slashes to crumb links in Diffusion
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
2013-10-03 15:51:47 -07:00
epriestley
1296c3d347 Fix two excessively aggressive policy checks
Summary:

  - Some applications need public access regardless of policy configuration.
  - The file data endpoint should ignore policies.
2013-10-03 14:38:08 -07:00
epriestley
6100906273 Support unlocking applications with bin/policy
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
2013-10-03 12:40:20 -07:00
epriestley
c830461b00 Allow application policies to be edited
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
2013-10-03 12:40:08 -07:00