1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-13 10:22:42 +01:00
Commit graph

5260 commits

Author SHA1 Message Date
William R. Otte
29436dfe37 Added 'method' field to the HTTP request build step.
Summary:
This revision adds a 'method' field to the HTTP request harbormaster build step.  This allows the user to specify GET, POST, DELETE, and PUT (limited by the underlying wrapper phabricator uses for HTTP requests).  I'm not sure how much sense PUT makes, but oh well.

Existing plans shouldn't break, as if this field is an empty string, we default to POST, which is the old behavior.

Fixes T4604

Test Plan: 1) Verified that the empty string does, in fact, issue a POST request.  Changed the method to be GET and observed that the problem described in T4604 is resolved.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley

Maniphest Tasks: T4604

Differential Revision: https://secure.phabricator.com/D8520
2014-03-13 15:51:05 -07:00
Bob Trahan
e6118bcbaf Tweak application and maniphest editors to handle policy corner cases better
Summary:
Fixes T4362. If you have a default edit + view policy of "no one" things get weird when you try to create a task - basically its impossible.

Ergo, re-jigger how we do policy checks just a bit.

 - if its a new object, don't bother with the "can the $actor edit this thing by virtue of having can see / can edit priveleges?" That makes no sense on create.
 - add a hook so when doing the "will $actor still be able to edit this thing after all the edits" checks the object can be updated to its ultimate state. This matters for Maniphest as being the owner lets you do all sorts of stuff.

Test Plan:
- made a task with no one policy and assigned to no one - exception
- made a task with no one policy and assigned to me - success
 - made a comment on the task - success
 - reassigned the task to another user - exception
 - reassigned the task to another user and updated policies to "users" - success

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4362

Differential Revision: https://secure.phabricator.com/D8508
2014-03-13 13:50:08 -07:00
epriestley
a9f38e55e5 Modernize Facebook OAuth instructions
Summary: Ref T4593. We have somewhat outdated instructions, and safer setup instructions are now available.

Test Plan: Read instructions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8518
2014-03-13 13:00:47 -07:00
epriestley
ae7324fd5b Fix an anchor redirect issue with OAuth server, plus modernize the application a bit
Summary:
Ref T4593. Via HackerOne. An attacker can use the anchor reattachment, combined with the Facebook token workflow, combined with redirection on OAuth errors to capture access tokens. The attack works roughly like this:

  - Create an OAuth application on Phabricator.
  - Set the domain to `evil.com`.
  - Grab the OAuth URI for it (something like `https://phabricator.com/oauthserver/auth/?redirect_uri=http://evil.com&...`).
  - Add an invalid `scope` parameter (`scope=xyz`).
  - Use //that// URI to build a Facebook OAuth URI (something like `https://facebook.com/oauth/?redirect_uri=http://phabricator.com/...&response_type=token`).
  - After the user authorizes the application on Facebook (or instantly if they've already authorized it), they're redirected to the OAuth server, which processes the request. Since this is the 'token' workflow, it has auth information in the URL anchor/fragment.
  - The OAuth server notices the `scope` error and 302's to the attacker's domain, preserving the anchor in most browsers through anchor reattachment.
  - The attacker reads the anchor in JS and can do client workflow stuff.

To fix this, I've made several general changes/modernizations:

  - Add a new application and make it beta. This is mostly cleanup, but also turns the server off for typical installs (it's not generally useful quite yet).
  - Add a "Console" page to make it easier to navigate.
  - Modernize some of the UI, since I was touching most of it anyways.

Then I've made specific security-focused changes:

  - In the web-based OAuth workflow, send back a human-readable page when errors occur. I //think// this is universally correct. Previously, humans would get a blob of JSON if they entered an invalid URI, etc. This type of response is correct for the companion endpoint ("ServerTokenController") since it's called by programs, but I believe not correct for this endpoint ("AuthController") since it's used by humans. Most of this is general cleanup (give humans human-readable errors instead of JSON blobs).
  - Never 302 off this endpoint automatically. Previously, a small set of errors (notably, bad `scope`) would cause a 302 with 'error'. This exposes us to anchor reattachment, and isn't generally helpful to anyone, since the requesting application did something wrong and even if it's prepared to handle the error, it can't really do anything better than we can.
  - The only time we'll 'error' back now from this workflow is if a user explicitly cancels the workflow. This isn't a 302, but a normal link (the cancel button), so the anchor is lost.
  - Even if the application is already approved, don't blindly 302. Instead, show the user a confirmation dialog with a 'continue' link. This is perhaps slightly less user-friendly than the straight redirect, but I think it's pretty reasonable in general, and it gives us a lot of protection against these classes of attack. This redirect is then through a link, not a 302, so the anchor is again detached.
  -

Test Plan: I attempted to hit everything I touched. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8517
2014-03-13 12:59:10 -07:00
epriestley
969d0c3e8d Use "\z" instead of "$" to anchor validating regular expressions
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.

When we care about this, use `\z` instead, which matches "end of input" only.

This allowed registration of `"username\n"` and similar.

Test Plan:
  - Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
  - Fixed the ones where this seemed like it could have an impact.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8516
2014-03-13 12:42:41 -07:00
epriestley
58877a9b6c Fix some collateral damage from SSH Keypair generation
Summary: There's no `addHiddenInput()` on the layout view. Instead, build it
from the form. See: <https://github.com/facebook/phabricator/issues/538>.

Auditors: btrahan
2014-03-13 07:31:45 -07:00
epriestley
f950985cfd Allow Passphrase to generate SSH keypairs and extact public keys from private keys
Summary:
Ref T4587.

  - Add an option to generate a keypair.
  - Add an option to view the public keys for existing keypairs.

Test Plan:
  - Generated keypairs.
  - Viewed public keys.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4587

Differential Revision: https://secure.phabricator.com/D8515
2014-03-12 18:58:25 -07:00
epriestley
44fc671b3f Add a "Generate Keypair" option on the SSH Keys panel
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.

Test Plan: Generated some keypairs. Hit error conditions, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4587

Differential Revision: https://secure.phabricator.com/D8513
2014-03-12 18:17:11 -07:00
epriestley
d27cd5fb99 Disable Herald more aggressively when it's turned off for a repository
Summary:
Currently, disabling Herald only disables feed, notifications and email. Historically, audits didn't really create external effects so it made sense for Herald to only partially disable itself.

With the advent of Harbormaster/Build Plans, it makes more sense for Herald to just stop doing anything. When this option is disabled, stop all audit/build/publish/feed/email actions for the repository.

Test Plan: Ran `scripts/repository/reparse.php --herald`, etc.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8509
2014-03-12 18:16:50 -07:00
epriestley
8a409aa40f Fix parsing of "Reviewed By" field
Summary: In the Message parser, we read this field and expect to get an array of PHIDs out of it. Currently, we get a string. Instead, get an array of PHIDs.

Test Plan: Wrote a message like "Fixes Tnnn" with "Reviewed by: duck", and saw no more parse error during message parsing.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8510
2014-03-12 18:11:09 -07:00
epriestley
e068b04b5c Silence a warning about unknown fields in 'getcommitmessage'
Summary: Similar to D8491, some of my new exceptions are a bit too aggressive. See IRC. This one's hitting an edit workflow with 'revisionID' onboard somehow.

Test Plan: Not entirely sure how to hit this, but it won't throw anymore.

Reviewers: btrahan, dctrwatson, zeeg

Reviewed By: zeeg

Subscribers: zeeg, aran, epriestley

Differential Revision: https://secure.phabricator.com/D8514
2014-03-12 17:24:58 -07:00
Michael Peters
3f7f5a47ff Fix a small typo when creating a Herald rule
Summary: "Users who an edit" to "Users who can edit"

Test Plan: Verified that typo is gone after the change

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8511
2014-03-12 16:12:43 -07:00
epriestley
4173367f87 Add missing needReviewerAuthority() for differential.createcomment
Summary: For some actions (like accept) we need to load reviewer authority so we can figure out if the actor can act on behalf of project reviewers, etc.

Test Plan: Will make @dctrwatson do it.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: chad, aran, dctrwatson, epriestley

Differential Revision: https://secure.phabricator.com/D8505
2014-03-12 15:24:54 -07:00
epriestley
3b861ab741 Fix two transactional issues around revision status
Summary:
Two issues:

  - Herald is currently overwriting accepts and rejects with "blocking reviewer". Just stop it from doing that.
  - When you update an accepted revision, we put it back in "needs review", then return it to "accepted", generating an extra transaction. Instead, don't.

Test Plan:
  - Updated a revision with an accepting, herald-based blocking project reviewer. Reviewer was still accepting.
  - Updated an accepted revision, didn't get an extra transaction.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8506
2014-03-12 15:24:43 -07:00
epriestley
193e8a54fc Add "pusher is committer" to Herald as a pre-commit rule
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:

  - Committer identities must be authentic.
  - Committer identities must be resolvable.
  - Author identities must be resolvable.

Test Plan: Created some rules using these new rules and ran them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4594

Differential Revision: https://secure.phabricator.com/D8507
2014-03-12 15:24:33 -07:00
epriestley
08d013b391 Minor, fix parameter passed to HarbormasterBuildable::applyBuildPlans()
Summary: This should be a PHID, not an object.
2014-03-12 12:13:54 -07:00
epriestley
c0d433727c Modernize "revision update history" in Differential
Summary:
Ref T4585.

  - Use modern UI kit.
  - Make mobile-ish.
  - Fix a couple minor things.

Test Plan:
{F127155}

{F127156}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley, chad

Maniphest Tasks: T4585

Differential Revision: https://secure.phabricator.com/D8504
2014-03-12 11:39:43 -07:00
epriestley
a4a4322d7a Add a "tags" field to Diffusion commit
Summary:
  - Fixes T4588.
  - See D8501.
  - Adds a "Tags" field for Herald commit emails.
  - Fixes a bug in `tagsquery` when filtering by commit name.
  - Make `tagsquery` just return nothing instead of fataling against Mercurial/Subversion.

Test Plan: Used `bin/repository/reparse.php --herald` to exercise this code.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4588

Differential Revision: https://secure.phabricator.com/D8502
2014-03-12 11:30:52 -07:00
epriestley
611f670720 Add CustomField to Diffusion and add a "branches" field
Summary:
Ref T4588. Request from @zeeg. Adds a "BRANCHES" field to commit emails, so the branches the commit appears on are shown.

I've implemented this with CustomField, but in a very light way.

Test Plan: Used `scripts/repository/reparse.php --herald` to generate mail, got a BRANCHES section where applicable.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley, zeeg

Maniphest Tasks: T4588

Differential Revision: https://secure.phabricator.com/D8501
2014-03-12 11:30:33 -07:00
epriestley
69399dfc2a Remove phpast.* Conduit methods
Summary:
Ref T4592. These were added with the intent of not requiring builds on Windows, but then we got builds on Windows working and they seem to be straightforward. See T4592 for most recent discussion.

Remove these methods because they aren't really practical for anything and increase attack surface area by giving adversaries access to `xhpast`, and generally bloat up the Conduit API. To my knowledge, nothing has ever called them.

(If an install somehow relies on these, they can drop them into `src/extensions/` to expose them again.)

Test Plan: Viewed conduit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4592

Differential Revision: https://secure.phabricator.com/D8500
2014-03-12 11:30:22 -07:00
epriestley
7176240717 Whitelist controllers which can receive a 'code' parameter
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.

We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.

To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.

This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.

Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.

Test Plan:
  - Logged in with OAuth.
  - Hit any other page with `?code=...` in the URL, got an exception.
  - Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8499
2014-03-12 11:30:04 -07:00
epriestley
9e3baacc95 Restore old "author can not be a reviewer" rule to Transactions
Summary:
This is a bit messy, but not tooo bad:

  - In general, stop the author from being added as a reviewer.
  - In the specific case that "self accept" is enabled, allow it. This is easier than trying to special case it.
  - When commandeering, we make the author a reviewer and make the actor the author, but these happen after validation. At validation time, it looks like we're making the author a reviewer. Just special case this.
  - Provide a slightly nicer message when trying to add yourself from `arc`. We hit the Transactions message anyway, but it's not formatted as cleanly.
  - Don't try to add the author via Herald.

Test Plan:
  - Edited a revision with author = reviewer, got stopped.
  - Commandeered revision.
  - Updated from `arc`.
  - Updated in general.
  - Fired a "add author as reviewer" Herald rule without things breaking.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8496
2014-03-12 06:04:30 -07:00
epriestley
95285aee50 When creating a new revision, include the summary in the initial email
Summary: This got dropped in the ApplicationTransactions stuff.

Test Plan: Created a new revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8495
2014-03-11 17:13:00 -07:00
epriestley
0b6c0c135f Default new reviewers to "added", and don't overwrite reviewer states when updating
Summary:
Fixes two issues with Differential:

  - New reviewers on initial diff were being created into a `null` state.
  - The `"="` edge update was overwriting accepted/rejected statuses. This could maybe be more nuanced in the long run, but I've just made it update correctly for now.

Test Plan:
  - Created and updated a revision, paying attention to reviewer statuses.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8494
2014-03-11 17:12:47 -07:00
epriestley
ad88ff28a1 Reject Phame domains which include a port number
Summary: Via HackerOne. This doesn't actually have any security impact as far as we can tell, but a researcher reported it since it seems suspicious. At a minimum, it could be confusing. Also improve some i18n stuff.

Test Plan: Hit all the error cases, then saved a valid custom domain.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8493
2014-03-11 15:53:15 -07:00
Bob Trahan
740757fd9b Diffusion - add ToC for readme files
Summary: see title. Fixes T4549.

Test Plan: made a readme that had some headers and observed a nice ToC

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4549

Differential Revision: https://secure.phabricator.com/D8490
2014-03-11 15:52:16 -07:00
Bob Trahan
a1faac0a21 Phame - create conduit API to create posts
Summary:
nothing too crazy here. try to be smart about some defaults (i.e. phame title is optional and can be derived from title; post as not a draft by default; etc). Fixes T3695.

also do a little re-factoring to centralizing initializing new posts and turning posts into dictionaries. also change blogs => posts in another conduit method so it makes sense and stuff.

Test Plan: made some posts via conduit. testing trying to specify blogger, phame title, and isDraft, all worked nicely

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T3695

Differential Revision: https://secure.phabricator.com/D8485
2014-03-11 15:51:53 -07:00
Bob Trahan
46cf263e9d Fix a bug for updating diffs
Summary: Hit this issue in D8485. I think reviewedByPHID changes should appear in application transactions.

Test Plan: Would like to deploy this and try updating D8485 again.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8491
2014-03-11 15:36:03 -07:00
epriestley
0b15624c37 Fix two minor Differential issues
Summary: The JIRA field is currently always enabled. This isn't correct; it
should be disabled if there's no JIRA provider.

We also use the old set of reviewers to compute mail delivery. Instead, reload
the correct set of reviewers before moving on after finalizing transactions.
2014-03-11 13:50:13 -07:00
Neal Poole
8818252f52 [herald] Add support for Arcanist Project as a field for Differential revisions
Summary: Useful in cases where there is an Arcanist Project but not a repository tracked by Phabricator for a particular revision.

Test Plan: Created a new rule to flag Differential revisions with a particular Arcanist project, verified that it applied as expected via the test console to revisions with the project specified and with a different project specified.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8463
2014-03-11 13:15:14 -07:00
Michael Peters
8b6c86e27d Fix the script that saves lint for a repo into the database and updates diffusion.
Summary:
It appears a change to the way the configuration was loaded into ArcanistRepositoryAPI in rARCa2285b2b broke the save_lint script.
This updates the DiffusionLintSaveRunner to use the configuration correctly, allowing the linter to run

Test Plan: cd /your/project; ../../../path/to/phabricator/scripts/repository/save_lint.php

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8487
2014-03-11 13:07:45 -07:00
epriestley
2dbfb1d5fb Remove DifferentialComment
Summary: Ref T2222. Remove this; no more callsites.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8477
2014-03-11 13:02:33 -07:00
epriestley
592591e715 Clean up various pieces of dead/obsolete Differential code
Summary:
Ref T2222.

  - Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
  - Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
  - Moves Releeph churn field off `DifferentialCommentQuery`.
  - Removes dead code in `DifferentialRevisionViewController`.
  - Removes `DifferentialException` (no references).
  - Removes `DifferentialRevision->loadComments()` (no callsites).
  - Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
  - Removes `DifferentialCommentQuery` (all callsites updated).

Test Plan: Mostly a lot of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8476
2014-03-11 13:02:19 -07:00
epriestley
a49fec39be Move lint/unit test warning code forward to Transactions
Summary: Ref T2222. Makes the "lint/unit errors" warnings work again.

Test Plan: Viewed some revisions with and without these warnings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8475
2014-03-11 13:02:18 -07:00
epriestley
20cc85878e Remove almost all old Differential field code
Summary: Ref T2222. The unit and lint fields still have one piece of functionality that I need to port, but everythign else is obsolete.

Test Plan: Lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8474
2014-03-11 13:02:16 -07:00
epriestley
3f67430f46 Use PhabricatorObjectListQuery in Herald worker
Summary: Ref T2222. Straightforward, just breaks a needless dependency.

Test Plan: Pushed and parsed a commit with "Auditors" in it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8473
2014-03-11 13:02:14 -07:00
epriestley
7cd4e70ef2 Remove DifferentialFieldSelector
Summary: Ref T2222. Gets rid of DifferentialFieldSelector, favoring `differential.fields`.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8472
2014-03-11 13:02:13 -07:00
epriestley
40b471faea Move "close tasks on commit" code out of field specification stuff
Summary: Ref T2222. There's some magic here, just port it forward in a mostly-reasonable way. This could use some refinement eventually.

Test Plan: Pushed commits with "Fixes" and "Ref" language, used `reparse.php` to trigger the new code. Saw expected updates in Maniphest.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8471
2014-03-11 13:02:12 -07:00
epriestley
3910d0d5e1 Remove field selector on Diff view and Revision List View
Summary:
Ref T2222. This has some minor functionality regressions:

  - The plain diff page no longer shows unit/test status. I want to give diffs separate custom fields for this.
  - It was technically possible to shove more data on the list view, although this doensn't affect the default config.

Test Plan: Looked at list view, diff detail view. Grepped for changes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8470
2014-03-11 13:02:10 -07:00
epriestley
48059265f3 Use CustomFields to power Conduit auxiliary dictionaries
Summary: Ref T2222. Moves this Conduit stuff over.

Test Plan: Made Conduit calls, saw data in results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8469
2014-03-11 13:02:09 -07:00
epriestley
77af6be803 Remove host/path and test plan enable/disable options
Summary: Ref T2222. These no longer have an effect, and are obsoleted by `differential.fields`.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8468
2014-03-11 13:02:07 -07:00
epriestley
9e8bbdb3a2 Port Differential mail features forward to transactions
Summary:
Ref T2222. Brings the major mail features (affected files, patches) forward.

This drops some of the minor integrations which just show object state (like "Maniphest Tasks") since I think they're not very important. I'll put them back if users miss them.

Test Plan: Sent mail with inline/attached patches.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8459
2014-03-11 13:02:06 -07:00
epriestley
50331016f7 Modernize commit message tips
Summary: Ref T2222. Fully modernizes these tips. No callsites remain for the old methods.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8457
2014-03-11 13:02:05 -07:00
epriestley
1df84168ef Remove DifferentialRevisionEditor
Summary: Ref T2222. This has no callsites and no functionality not present in the TransactionEditor.

Test Plan: awwyiss

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8456
2014-03-11 13:02:04 -07:00
epriestley
c68703fbcb Perform derived index updates in TransactionEditor
Summary: Ref T2222. We have two tables (one for hashes; one for paths) which were unevenly updated before. Now, update them consistently in the TransactionEditor.

Test Plan: Created a revision, saw it populate this information.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8455
2014-03-11 13:02:03 -07:00
epriestley
a19f49632f Remove willWriteRevision/didWriteRevision hooks
Summary:
Ref T2222. DifferentialRevisionEditor has no remaining callsites, but it has a bit of functionality which still needs to be ported forward. I'm going to rip it apart piece by piece.

This removes the willWriteRevision/didWriteRevision hooks. They are completely encapsulated by transactions now, except for a unique piece of branch/task logic, which I migrated forward.

Test Plan:
  - Lots of `grep`.
  - Created a new revision on branch `T25`, saw it associate with the task.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8454
2014-03-11 13:02:01 -07:00
epriestley
fbaa12440e Use DifferentialRevisionEditor in lipsum
Summary: Ref T2222.

Test Plan: Generated revisions with `bin/lipsum`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8453
2014-03-11 13:02:00 -07:00
epriestley
a5fbe921b7 Use CustomFields in differential.createrevision
Summary:
Ref T2222. Ref T3886. Medium term goal is to remove `DifferentialRevisionEditor`.

This temporarily reduces a couple of pieces of functionality unique to the RevisionEditor, but I'm going to go clean those up in the next couple diffs.

Test Plan: Used `arc diff --create` to create several revisions with different data.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8452
2014-03-11 13:01:59 -07:00
epriestley
d8968755e9 Use CustomField for differential.updaterevision
Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites.

Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3794, T2222

Differential Revision: https://secure.phabricator.com/D8451
2014-03-11 13:01:58 -07:00
epriestley
6dd191a3c1 Allow configuration of Differential custom fields
Summary: Ref T2222. Ref T3886. This is a little early for general use, but the message parse/generate stuff requires CustomFields and FieldSpecifications to be closely aligned, so this provides at least a plausbile approach for any installs that run into trouble.

Test Plan: Viewed config; reordered fields.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222, T3886

Differential Revision: https://secure.phabricator.com/D8450
2014-03-11 13:01:57 -07:00
epriestley
ae3c1f7819 Perform commit message parsing and construction with new CustomFields
Summary: Ref T2222. Ref T3886. Converts parsing and construction of commit messages to be driven by CustomField.

Test Plan:
This is a huge, messy change. I've made an effort to test it exhasutively, but suspect I probably missed a few behaviors. Roughly:

  - Enumerted all current fields (fields implementing `shouldAppearOnCommitMessage()`) and tried to test them one by one.
  - Used `arc diff --edit` repeatedly to manipulate each field (this workflow hits both the parse and construct steps).
  - Used `arc amend --show` to examine construct output (this does not activate the "edit" mode).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3886, T2222

Differential Revision: https://secure.phabricator.com/D8449
2014-03-11 13:01:55 -07:00
epriestley
966eb2ae26 Fail feed story renders individually, instead of in aggregate
Summary: When we fail to render a feed story because something is broken, just break that story, not the entire feed.

Test Plan: {F125898}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8488
2014-03-10 18:22:24 -07:00
epriestley
a0f534b87c Minor improvements to Diviner layout
Summary:
  - Render functions as `func()` for consistency/clarity.
  - Sort articles first.
  - Sort case insensitively.
  - Label the "no group" symbols.

Test Plan: Regenerated and examined docs.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8480
2014-03-10 17:59:13 -07:00
Bob Trahan
8e41315238 Hovercard - add project images
Summary:
adds project images. Also fiddles with HTML + CSS just a bit so we have a "picture" column and a "details" column if a picture exists.

This keeps the details all in a nice column even if there are many details that end up being taller than the picture UI.

Fixes T3991.

Test Plan: looked at a task (no pic), project (pic w/ no details), and user (pic w/ many details) hovercard and all looked good on Chrome and Safari

Reviewers: epriestley, chad

CC: chad, Korvin, epriestley, aran

Maniphest Tasks: T3991

Differential Revision: https://secure.phabricator.com/D8483
2014-03-10 17:10:32 -07:00
epriestley
761b662283 Don't prefill "add email address" from GET
Summary: Via HackerOne. I don't think this is a security vulnerability, but it is inconsistent. There's no reason to prefill this, and I think the code was just lazy.

Test Plan:
  - Hit this page with `?email=xyz` in a GET request, no more prefill.
  - Looped the page with bad addresses, appropriate prefill.
  - Added an address.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8458
2014-03-10 16:21:47 -07:00
epriestley
042ab0ad9d Fix three minor edge case behaviors in Conpherence
Summary:
Couple of tweaks:

  - If a conpherence has no participants, we fail to `attachParticipants()`. This can happen if you leave a Conpherence as the last participant, then visit the URI again explicitly.
  - If you can't load any transactions (usually, because you don't have permission to view a thread's transactions), we try to attach `null` instead of `array()`. This can happen if you attempt to view a thread you don't have permission to see. A more general fix would be to tweak the load/filtering order, but I'm leaving that for another time since it's more involved and only gives us a small performance gain in unusual sitautions.
  - `initializeNewThread()` should be declared `static`.

Test Plan:
  - Viewed a thread with no participants, got proper policy error.
  - Viewed a thread I couldn't see, got proper policy error.
  - Grepped for `initializeNewThread()`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8467
2014-03-10 16:21:28 -07:00
epriestley
5854de8c1c Don't 302 to an external URI, even after CSRF POST
Summary:
Via HackerOne. This defuses an attack which allows users to steal OAuth tokens through a clever sequence of steps:

  - The attacker begins the OAuth workflow and copies the Facebook URL.
  - The attacker mutates the URL to use the JS/anchor workflow, and to redirect to `/phame/live/X/` instead of `/login/facebook:facebook.com/`, where `X` is the ID of some blog they control. Facebook isn't strict about paths, so this is allowed.
  - The blog has an external domain set (`blog.evil.com`), and the attacker controls that domain.
  - The user gets stopped on the "live" controller with credentials in the page anchor (`#access_token=...`) and a message ("This blog has moved...") in a dialog. They click "Continue", which POSTs a CSRF token.
  - When a user POSTs a `<form />` with no `action` attribute, the browser retains the page anchor. So visiting `/phame/live/8/#anchor` and clicking the "Continue" button POSTs you to a page with `#anchor` intact.
  - Some browsers (including Firefox and Chrome) retain the anchor after a 302 redirect.
  - The OAuth credentials are thus preserved when the user reaches `blog.evil.com`, and the attacker's site can read them.

This 302'ing after CSRF post is unusual in Phabricator and unique to Phame. It's not necessary -- instead, just use normal links, which drop anchors.

I'm going to pursue further steps to mitigate this class of attack more thoroughly:

  - Ideally, we should render forms with an explicit `action` attribute, but this might be a lot of work. I might render them with `#` if no action is provided. We never expect anchors to survive POST, and it's surprising to me that they do.
  - I'm going to blacklist OAuth parameters (like `access_token`) from appearing in GET on all pages except whitelisted pages (login pages). Although it's not important here, I think these could be captured from referrers in some cases. See also T4342.

Test Plan: Browsed all the affected Phame interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, arice

Differential Revision: https://secure.phabricator.com/D8481
2014-03-10 16:21:07 -07:00
epriestley
0a779b60a2 Exclude disabled (disapproved) users from count on People application on homepage
Summary:
The People application shows users awaiting approval, but incorrectly counts disabled users (i.e., users who were not approved).

Instead, count only non-disabled, non-approved users.

Test Plan: My homepage count dropped from 4 to 1, corresponding to the actual number of accounts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, spicyj

Differential Revision: https://secure.phabricator.com/D8486
2014-03-10 16:20:49 -07:00
Joshua Spence
e11adc4ad7 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 19:16:21 -08:00
epriestley
06626205fe Minor, fix an issue with StoredCustomFields that serialize, like JIRA 2014-03-08 07:36:04 -08:00
Anirudh Sanjeev
44b41a94ae Add a note suggesting restarting daemons for feed.http-hooks
Summary:
I was trying to set up a http hook, but despite setting the config,
the endpoint wasn't getting a request. I was advised on IRC by balpert to
restart my daemons and it worked great after I did that.

Since this information isn't in the documentation, I am adding it to the
description of the option, so it helps the next person.

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, spicyj

Differential Revision: https://secure.phabricator.com/D8447
2014-03-08 06:37:16 -08:00
epriestley
314edcabbb Fix Herald adapter construction for new revisions. Fixes T4572. 2014-03-08 06:35:41 -08:00
epriestley
76577df506 Extract textual object list parsing from Differential
Summary:
Ref T2222. Currently, Differential has a fairly hairy piece of logic to parse object lists, like `Reviewers: alincoln, htaft`. Extract, generalize, and cover this.

  - Some of the logic can be simplified with modern ObjectQuery stuff.
  - Make `@username` the formal monogram for users.
  - Make `list@domain.com` the formal monogram for mailing lists.
  - Add test coverage.

Test Plan:
  - Ran unit tests.
  - Called `differential.parsecommitmessage` with a bunch of real-world inputs and got sensible results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8445
2014-03-07 17:44:44 -08:00
epriestley
aff34077c5 Move Differential commit message parsing to a separate, tested class
Summary: Ref T2222. We have a hunk of logic that purely does text parsing here; separate it and get coverage on it.

Test Plan:
  - Ran new unit tests.
  - Used `differential.parsecommitmessage`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8444
2014-03-07 17:44:35 -08:00
epriestley
f25cce1e69 Remove DifferentialCommentEditor and DifferentialCommentMail
Summary: Ref T2222. These no longer have any callsites. Also got rid of a little bit of other code which also no longer has callsites.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8443
2014-03-07 17:44:19 -08:00
epriestley
1c51ed5940 Use TransactionEditor in differential.createcomment
Summary: Ref T2222. Update this callsite; pretty straightforward.

Test Plan: Used Conduit to take actions and saw their effects in Differential.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8442
2014-03-07 17:44:10 -08:00
epriestley
b04f706c0a Use TransactionEditor when closing revisions in response to commits
Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.

Test Plan: {F123848}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8441
2014-03-07 17:43:58 -08:00
epriestley
458a28eed3 Truncate logSource in Harbormaster to the database column limit
Summary: This can be a command, which might be arbitrarily long, but the column is VARCHAR(255).

Test Plan: `grep`

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8446
2014-03-07 17:43:46 -08:00
epriestley
0488ce73c4 Add assertions to no-assertions tests in phabricator/
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.

Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.

Reviewers: leebyron, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4570

Differential Revision: https://secure.phabricator.com/D8436
2014-03-07 10:02:46 -08:00
Michael Peters
5e47f2a862 Adding author information to AuditListView
Summary:
Adding the Author to the home page and Audit overview page,
so that at a glance you can see who authored the commits
that need to be audited

Test Plan: View home page and audit overview page and see that author is shown

Reviewers: #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8438
2014-03-07 08:40:35 -08:00
epriestley
49eaa9f8fe Use TransactionEditor in Differential mail handling
Summary: Ref T2222. Moves this instance of CommentEditor to TransactionEditor.

Test Plan: Used `bin/mail receive-test` to test receiving comment mail and action mail.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8427
2014-03-07 08:10:18 -08:00
epriestley
4dfd4944c4 Don't use CommentEditor in lipsum
Summary: Ref T2222. For now, I'm just dropping this rather than updating it since I'll need to come back here later for `DifferentialRevisionEditor` anyway, and no users rely on this functionality.

Test Plan: Static checks; this isn't user-facing.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8426
2014-03-07 08:10:07 -08:00
epriestley
75514cc0f7 Update differential.close to use DifferentialTransactionEditor
Summary: Ref T2222. Straightforward update to new stuff.

Test Plan:
  - Tried to close an uncloseable revision.
  - Closed a closeable revision.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8425
2014-03-07 08:09:59 -08:00
epriestley
3dc9afa28d Remove differential.markcommitted
Summary: Ref T2222. Primary goal is to remove this callsite for `DifferentialCommentEditor`, but rather than updating it I'm just nuking this method since it's been deprecated for more than a year (more than two years?)

Test Plan: Reloaded Conduit method list.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8424
2014-03-07 08:09:33 -08:00
epriestley
5801176edc When creating a new Phame blog post, check that the author has permission to post to the blog
Summary:
Via HackerOne. We're missing this permissions check, so you can sneak around it with URL editing right now.

I checked the other queries in this application and they seem OK.

Test Plan: Tried to post to a blog I had no permission to join.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8423
2014-03-06 14:06:07 -08:00
epriestley
270929dd6b Include Source Sans on-demand with Celerity
Summary:
Unwinds the mess I made in D8422 / D8430:

  - Remove `'fonts'`, since individual fonts can be included via Celerity now.
  - Include Source Sans from the local source when a document uses it as a fontkit.

Test Plan: Browsed Diviner, saw Source Sans.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8431
2014-03-06 11:39:48 -08:00
Peng Li
81d9935efe Allow parenthesis in author name
Summary: We have a dozen users who has `(...)` in their 'real name', like 'Jimmy (He) Zhang', and it's causing the diffusion file browser problems when blame is enabled. The parser does not expect those parenthesis and the lines of code will be empty if they were last touched by a user like that.

Test Plan: Try it

Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8429
2014-03-06 11:28:46 -08:00
epriestley
270916a26e Support WOFF files in Celerity and add Source Sans Pro
Summary:
  - Allow Celerity to map and serve WOFF files.
  - Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
  - Add a `font-source-sans-pro` resource for the font.

Test Plan:
  - Changed body `font-face` to `'Source Sans Pro'`.
  - Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.

Works in Firefox/Chrome/Safari, at least:

{F123296}

{F123297}

{F123298}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D8430
2014-03-06 11:28:24 -08:00
Bob Trahan
d08576ed4e Workboards - add task create + improve task placement wrt priority edits
Summary: Fixes T4553, T4407.

Test Plan: created tasks and they showed up in the proper column. edited task priority and they moved about sensically.

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4553, T4407

Differential Revision: https://secure.phabricator.com/D8420
2014-03-05 18:40:28 -08:00
epriestley
001a9557ec Enable Diviner to use the markup cache. 2014-03-05 17:22:27 -08:00
epriestley
5b8e7d2de3 Minor, allow documentation to be accessed while logged out. 2014-03-05 17:19:40 -08:00
epriestley
f55168b08b Minor, actually show article titles in Diviner. 2014-03-05 16:48:29 -08:00
epriestley
867dae9414 Make Diviner's "advanced search" a little less derp
Summary: Ref T988. This layout got mucked up a while ago, restore it to some semblance of sanity and give it a couple of basic search options.

Test Plan: Searched for stuff. Woooo~~

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8419
2014-03-05 16:45:21 -08:00
epriestley
81d381f69d Give Diviner a non-legacy root controller
Summary: Ref T988. Instead of hard-coding the application landing page, make the Diviner root show books if any have been generated. Otherwise, show a helpful message about how to generate documentation locally.

Test Plan:
{F122723}

{F122724}

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8416
2014-03-05 13:07:50 -08:00
epriestley
d07fc70bbe Make bin/diviner generate with no arguments mean "generate everything"
Summary: Ref T988. This makes it easier to generate documentation.

Test Plan: Ran with and without `--book`. Examined CLI output.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8415
2014-03-05 13:00:50 -08:00
epriestley
fb52eda3d9 Allow Diviner books to have a "preface" section
Summary: Ref T988. This is primarily intended to let us add the "HEY! THIS ISN'T USER DOCUMENTATION" notices to the arcanist and libphutil technical docs.

Test Plan: Added some prefaces, generated docs, looked at them.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8410
2014-03-05 12:08:01 -08:00
epriestley
feac35a0fd Improve 404 behavior for new Diviner
Summary:
Ref T988. When the user clicks a link we haven't explicitly resolved before, we send them to the `/find/` endpoint, but currently just 404 if we can't find the relevant documentation.

Instead, display a more user-friendly error message, since we're probably going to have some of these. Also, make the page title much worse.

Test Plan: Hit a 404 via `/find/`, got a nicer page.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8408
2014-03-05 12:07:39 -08:00
epriestley
03abde0b25 Fix Diviner links to articles by title
Summary:
Ref T988. This fixes the biggest current problem with Diviner, which is dead links to articles.

In the new Diviner, articles can have both a "name" (derived from the file name, and used in the URI) and a "title" (optional, specified explicitly). For example, we have one document with the name "feedback" and the title "Give Feedback! Get Support!".

On disk, we want to use the name for the actual file where the text lives ("feedback.diviner"). We also want to use the name in the URI, to generate a clean URI and to allow us to retitle the document slightly without breaking links to it (for example, we renamed the "Backup" document to "Backups and Migrations").

However, when displaying the article we want to use the title.

Currently, you can //only// link to the name, not the title. This is inconvenient:

  - We have a bunch of existing docs which link to titles.
  - It's natural/intuitive to link to titles.
  - Linking to titles makes it easier/cheaper to generate documentation, because we don't need to be able to resolve things at render time.

To remedy this, allow links to target either names or titles. If we miss on a name query, we'll do a title query. This is implemented with a slug hash to allow approximately correct titles (wrong case/spacing/punctuation, e.g.) and sidestep all the UTF8/column length issues.

(In the long run, atom resolution should theoretically be more sophistiated than it is now, and we should do render-time lookups on at least some documents to catch bad links. However, this is fairly complicated and a relatively advanced feature, and I think allowing links to titles is desirable no matter what.)

Test Plan: The user documentation book now has valid links to articles when the titles and names differ.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T988

Differential Revision: https://secure.phabricator.com/D8407
2014-03-05 12:07:26 -08:00
epriestley
2ceffadee7 Support Herald rules for new Differential edits
Summary:
Ref T2222. Ref T4484. See D8404 for discussion.

When a revision is updated with the new Editor, apply Herald rules. Additionally, apply them in a modern way which generates transactions.

Test Plan: {F122299}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T2222, T4484

Differential Revision: https://secure.phabricator.com/D8405
2014-03-05 12:07:13 -08:00
epriestley
84020a363f Let Herald activation depend on which transactions are being applied, and generate transactions
Summary:
Ref T2222. Ref T4484. This is a stepping stone to getting Herald supported in the new Differental code. Generally:

  - Instead of an Editor either supporting or not supporting Herald, let it choose based on transactions. Specifically, Differential only runs rules on revision creation and diff updates.
  - Optionally, allow an Editor to return some transactions to apply instead of having to apply everything itself. This lets us make it clear why changes happend in the transaction log, and share more code.
  - I updated only one transaction type (owners in Maniphest) since it was the easiest and cleanest to update and test. Everything else still works like it used to, it just won't generate a transaction record yet.
  - The transaction records are a touch rough, but we can clean them up later.

Test Plan: {F122282}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4484, T2222

Differential Revision: https://secure.phabricator.com/D8404
2014-03-05 12:06:59 -08:00
epriestley
4ef87eeac8 Add an explcit "Changes Planned" state for Differential
Summary: Ref T2222. Ref T4481. This fixes the issue where "Plan Changes" could immediately trigger a state change (e.g., back to accepted) because of state-based transitions out of the NEEDS_REVISION state.

Test Plan: Planned changes an "accepted" revision, it didn't immediately return to being accepted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222, T4481

Differential Revision: https://secure.phabricator.com/D8403
2014-03-05 10:47:47 -08:00
epriestley
9da6ec2081 Make updates of rejected revisions behave correctly again
Summary:
Ref T2222. Ref T4481. Specifically:

  - When a revision is updated, change all "Reject" reviewers to "Reject Prior".
  - Change status to "Needs Review".
  - Update the state logic to account for this properly.

Test Plan:
  - Created a revision as user A, with B as a reviewer.
  - Rejected as B.
  - Updated the revision as A.
  - Saw revision in "needs review" state, with B as a "Rejected Prior" reviewer.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4481, T2222

Differential Revision: https://secure.phabricator.com/D8402
2014-03-05 10:47:29 -08:00
epriestley
857e3aee83 Improve ApplicationTransaction behavior for poorly constructed transactions
Summary:
Ref T2222. Five very small improvements:

  - I hit this exception and it took a bit to understand which transaction was causing problems. Add an `Exception` subclass which does a better job of making the message debuggable.
  - The `oldValue` of a transaction may be `null`, legitimately (for example, changing the `repositoryPHID` for a revision from `null` to some valid PHID). Do a check to see if `setOldValue()` has been called, instead of a check for a `null` value.
  - Add an additional check for the other case (shouldn't have a value, but does).
  - When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
  - Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for `CustomField` transactions.

Test Plan: Resolved the issue I was hitting more easily, made updates to a `null`-valued custom field, and applied other normal sorts of transactions successfully.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4557, T2222

Differential Revision: https://secure.phabricator.com/D8401
2014-03-05 10:44:21 -08:00
Chad Little
dd60f25232 Make "My Events" default on Calendar
Summary: Moves Browse to "View All" and makes "My Events" the default on Calendar.

Test Plan: Browse both pages.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8397
2014-03-05 08:24:45 -08:00
Bob Trahan
e78df59ced Maniphest Tasks + Project Boards - some polish
Summary:
Fixes T4550 by changing supportsFeed to shouldPublishFeedStory, so things can be more granular like that are with mail. Attempts to fix things generally too, filtering out xactions that have no business in feed, etc.

Also return an updated Task HTML representation on drag and drop moves, etc. This is important so if the priority changes you can see it reflected in the UI.

Test Plan: dragged tasks around. observed no feed stories on subpriority drags. observed feed stories and updated color bars on stories that changed priority

Reviewers: epriestley, chad

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4550

Differential Revision: https://secure.phabricator.com/D8399
2014-03-04 17:01:33 -08:00
epriestley
cdeea11fd3 Don't actually construct auth providers when checking for their existence
Summary:
A user reported this stack trace:

http://pastebin.com/6auGbZsE

...on this GitHub issue:

https://github.com/facebook/phabricator/issues/389#issuecomment-36612511

The problem is similar to the original report, but not identical. In this case, we're following a sequence of steps like:

  - Run setup checks.
    - Check for enabled providers, in order to raise "no providers configured yet" warning.
      - Try to generate login/redirect URIs.
  - Build the request.
  - Set the default base URI.
  - Run normal code.

Since we try to generate URIs before we provide a default, this fatals. Instead, don't try to build objects.

An alternative fix might be to try to set defaults earlier, but we depend on some config and on building the Request in order to be able to figure out if a request is HTTP or HTTPS right now. We could assume one, or guess, or use protocol-relative URIs (`///host.com`), but I think this fix is a little cleaner overall. If we keep hitting similar stuff, we could look into alternate fixes.

We could also set some kind of "setup mode" flag and make `getURI()` if it's called during setup mode to detect these during testing. I'd like to hit one more of these before doing that, though.

Test Plan: Reproduced the issue, applied the patch, verified this fixes it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8395
2014-03-04 16:11:28 -08:00
epriestley
eec9507ba7 Link board names to board view in "changed column" stories
Summary: Fixes T4552.

Test Plan:
{F122106}

{F122107}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4552

Differential Revision: https://secure.phabricator.com/D8396
2014-03-04 13:40:01 -08:00
Bob Trahan
28f3f8bf7b Board feed story - make the task a link too
Summary: looks better, more useful

Test Plan: looked better, was more useful when I observed my feed

Reviewers: chad, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8394
2014-03-04 12:05:42 -08:00
Bob Trahan
b49b913166 Project work boards - make editing tasks from workboards work
Summary: This wasn't working. Create a little JS handler and server-side support for returning the Task in the "project card" format.

Test Plan: Edited tasks from the board - they worked!

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, chad

Differential Revision: https://secure.phabricator.com/D8392
2014-03-04 11:50:44 -08:00
Alex Wyler
5fabda2a6d Check for null, strictly, in maniphest.update param validation
Summary:
If the first non-null entry in the params array is falsey, the request bombs.

Something like {"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-on3xxsnaljmfn36d4b7a"}

Test Plan:
Before:

  echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
  {"error":"ERR-NO-EFFECT","errorMessage":"ERR-NO-EFFECT: Update has no effect.","response":null}

After:

  echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
  {"error":null,"errorMessage":null,"response":{"id":"279","phid":"PHID-TASK-lbwcq3pmur2c5fuqqhlx"...

Reviewers: garoevans, epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8391
2014-03-04 11:42:18 -08:00