1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

16012 commits

Author SHA1 Message Date
epriestley
31ee49b14d Fix Javascript busy loop when trying to delete tokens from an empty tokenizer
Summary:
Fixes T13147. In D19437, I changed this logic to support deleting the `""` (empty string) token, but `[].pop()` returns `undefined`, not `null`, if the list is empty and I didn't think to try deleting an empty input.

Fix the logic so we don't end up in a loop if the input is empty.

Test Plan:
  - In any browser, deleted all tokens in a tokenizer; then pressed delete again.
  - Before: tab hangs in an infinte loop.
  - After: smooth sailing.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13147

Differential Revision: https://secure.phabricator.com/D19456
2018-06-01 14:51:06 -07:00
Austin McKinley
2f6784ee1c Add workflow to create repository identities
Summary:
Depends on D19443. Creates a workflow for populating the new identity table by iterating over commits, either one repo at a time or all at once. Locally caches identities to avoid fetching them `inf` times. An actual migration that invokes this workflow will come in another revision that won't land until at least next week.

Performance is ~2k commits in 4.9s on my local machine.

Test Plan: Ran locally a few times with a few different states of the `repository_identity` table.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: jcox, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19446
2018-05-31 07:29:57 -07:00
Austin McKinley
fe5fde5910 Assign RepositoryIdentity objects to commits
Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.

Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19443
2018-05-31 07:28:23 -07:00
Austin McKinley
f191a66490 Add controllers/search/edit engine functionality to RepositoryIdentity
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.

Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19429
2018-05-31 07:03:25 -07:00
Austin McKinley
cd84e53c44 Begin building out RepositoryIdentity indirection layer
Summary: Ref T12164. Start building initial objects for managing `RepositoryIdentity` objects. This won't land until much more of the infrastructure is in place.

Test Plan: Ran `bin/storage upgrade` and observed expected table.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19423
2018-05-31 07:01:16 -07:00
epriestley
de999af614 Improve some behaviors around memory pressure when pushing many and/or large changes
Summary:
Ref T13142. When commits are pushed, we try to handle them on one of two pathways:

  - Normal changes: we load these into memory and potentially apply Herald content rules to them.
  - "Enormous" changes: we don't load these into memory and skip content rules for them.

The goal is to degrade gracefully when users push huge changes: they should work, just not support all the features.

However, some changes can slip through the cracks right now:

  - If you push a lot of commits at once, we'll try to cache all of the changes smaller than 1GB in memory. This can require an arbitrarily large amount of RAM.
  - We calculate sizes by just looking at the `strlen()` of the diff, but a changeset takes more RAM in PHP than the raw diff does. So even if a diff is "only" 500MB, it can take much more memory than that. On systems with relatively little memory available, this may result in OOM while processing changes that are close to the "enormous" limit.

This change makes two improvements:

  - Instead of caching everything, cache only 64MB of things.
    - For most pushes, this is the same, since they have less than 64MB of diffs.
    - For pushes of single very large changes, this is a bit slower (more CPU) since we have to do some work twice.
    - For pushes of many changes, this is slower (more CPU) since we have to do some work twice, but, critically, doesn't require unlimited memory.
  - Instead of flagging changes as "enormous" at 1GB, flag them as "enormous" at 256MB.
    - This reduces how much memory is required to process the largest "non-enormous" changes.
    - This also gets us under Git's hard-coded 512MB "always binary" cutoff; see T13143.
    - This is still completely gigantic and way larger than any normal change should be.

An additional improvement would be to try to reduce the amount of memory we need to use to hold a change in process memory. I think the other changes here alone will fix the immediate issue in PHI657, but it would be nice if the "largest non-enormous change" required only a couple gigs of RAM.

Test Plan:
- Used `ini_set('memory_limit', '1G')` to artificially limit memory to 1GB.
- Pushed a series of two commits which add two 550MB text files (Temporarily, I added a `--binary` flag to trick Git into showing real diffs for these, see T13143.)
- Got a memory limit error.
- Applied the "cache only 64MB of stuff" and "consider 256MB, not 1GB, to be enormous" changes.
- Pushed again, got properly rejected as enormous.
- Added `memory_get_usage()` calls to measure how actual memory size and reported "size" estimate compare. For these changes, saw a 639MB diff require 31,479MB of memory, i.e. a factor of about 50x. This is, uh, pretty not great.
- Allowed enormous changes, pushed again, push went through.

Reviewers: amckinley

Maniphest Tasks: T13142

Differential Revision: https://secure.phabricator.com/D19455
2018-05-18 17:15:34 -07:00
epriestley
3c5668b4a5 When database connection exceptions occur, raise them to the setup layer
Summary:
Ref T13141. Currently, during first-time setup we don't surface all the details about connection exceptions that we could: the underlying exception is discarded inside cluster connection management.

This isn't a huge issue since the reason for connection problems is usually fairly obvious, but in at least one case (see T13141) we hit a less-than-obvious exception.

Instead, store the original exception and propagate the message up the stack so users have more information about the problem.

Test Plan:
  - Configured an intentionally bad MySQL username.
  - Restarted Apache and loaded Phabricator.
  - Got a more helpful exception with a specific authentication error message.

{F5622361}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13141

Differential Revision: https://secure.phabricator.com/D19454
2018-05-18 09:02:36 -07:00
epriestley
8f9b948447 When showing a diff-of-diffs, hide files which didn't get any more changes and have no inlines
Summary:
Ref T13137. See that task for discussion.

When we show a diff-of-diffs, we often render stubs for files which didn't change between the diffs. These stubs usually aren't a big deal, but for certain types of changes (like refactors) they can create a lot of clutter.

Instead, hide these stubs and show a notice that we hid them.

Test Plan:
  - Created a revision affecting 4 files.
  - Updated it with a diff that changed only 1 of the 4 files.
  - Added an inline comment to a different file.
  - Viewed the diff of diffs.
    - Before: 4 changesets with two "nothing changed" stubs.
    - After: 2 changesets with the stubs hidden.

{F5621083}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

Differential Revision: https://secure.phabricator.com/D19453
2018-05-16 17:18:53 -07:00
epriestley
79fdf5c127 Separate changeset analysis code from DifferentialDiff and provide a standalone rebuild-changesets workflow
Summary:
Ref T13137. The "analyze/cache data about changesets" step is becoming more involved. We recently added detection for generated code to support "Ignore generated changes" in Owners, and I now plan to hash the new file content so we can hide changes which have no effect.

Before adding this new hashing step, pull the "detect copied code" and "detect generated code" stuff out and move them to a separate `ChangesetEngine`. Then support doing a changeset rebuild directly with `bin/differential rebuild-changesets`.

This simplifies things a bit and makes testing easier since you don't need to keep creating new revisions to re-run copy/generated/hash logic.

Test Plan: Ran `bin/differential rebuild-changesets --revision Dxxx`, saw changesets rebuild. See also next change.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

Differential Revision: https://secure.phabricator.com/D19452
2018-05-16 17:17:28 -07:00
epriestley
3544620209 Parse unusual Subversion protocol frames which contain extra whitespace
Summary:
Fixes T13140. See PHI660.

Recent versions of Subversion can send a `(get-file true false  false )` protocol frame with extra space between "false" and "false". This is allowed by the protocol spec, but never normally happens, and we do not parse it correctly.

Instead, parse it correctly.

Test Plan:
  - Added unit tests.
  - Ran `svn proplist svn+ssh://.../diffusion/X/file.c` under SVN 1.10 before and after the change.
    - Before: indefinite hang.
    - After: completed in finite time.

Reviewers: amckinley, asherkin

Reviewed By: amckinley, asherkin

Maniphest Tasks: T13140

Differential Revision: https://secure.phabricator.com/D19451
2018-05-16 17:12:41 -07:00
epriestley
7aa12f192a Add PhabricatorQueryIterator, for buffered iteration over a CursorPagedPolicyAwareQuery
Summary:
See D19446. This should make it easier to process larger, more complex result sets in constant memory.

Today, `LiskMigrationIterator` takes constant memory but can't apply `needX()` reqeusts or `withY(...)` constraints.

Using a raw `Query` can handle this stuff, but requires memory proportional to the size of the result set.

Offer the best of both worlds: constant memory and full access to the power of `Query` classes.

Test Plan:
Used this script to iterate over every commit, saw sensible behavior:

```name=list-commits.php
<?php

require_once 'scripts/init/init-script.php';

$viewer = PhabricatorUser::getOmnipotentUser();

$query = id(new DiffusionCommitQuery())
  ->setViewer($viewer);

$iterator = new PhabricatorQueryIterator($query);
foreach ($iterator as $commit) {
  echo $commit->getID()."\n";
}
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19450
2018-05-14 12:13:51 -07:00
epriestley
29df80b48f Fix a fatal during breadcrumb construction when viewing a dashboard you don't have permission to view
Summary: Ref PHI662. Viewing a dashboard you don't have permission to view (in the Dashboard application) currently fatals while building crumbs, since we fail to build the ` ... > Dashboard 123 > ...` crumb.

Test Plan:
  - Viewed a dashboard I didn't have permission to view in the Dashboards application.
  - Before patch, fatal when calling `getID()` on a non-object.
  - After patch, sensible policy error page.
  - Viewed a dashboard I can view, saw sensible crumbs.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19449
2018-05-14 12:06:56 -07:00
epriestley
28ee6b8080 Consistently require MFA on the actual user creation flow
Summary:
See <https://hackerone.com/reports/351361>. We currently require MFA on the screen leading into the user create flow, but not the actual create flow.

That is, `/people/create/` (which is just a "choose a type of account" page) requires MFA, but `/people/new/<type>/` does not, even though this is the actual creation page.

Requiring MFA to create users isn't especially critical: creating users isn't really a dangerous action. The major threat is probably just that an attacker can extend their access to an install by creating an account which they have credentials for.

It also isn't consistently enforced: you can invite users or approve users without an MFA check.

So there's an argument for just removing the check. However, I think the check is probably reasonable and that we'd likely prefer to add some more checks eventually (e.g., require MFA to approve or invite) since these actions are rare and could represent useful tools for an attacker even if they are not especially dangerous on their own. This is also the only way to create bot or mailing list accounts, so this check does //something// on its own, at least.

Test Plan:
  - Visited `/people/new/standard/` as an admin with MFA configured.
  - Before patch: no MFA prompt.
  - After patch: MFA prompt.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19448
2018-05-14 12:03:07 -07:00
epriestley
26d0862f4f Apply the new patch byte size limit to mail patch generation in Differential
Summary: Ref T13137. See PHI592. Depends on D19444. Apply a limit up front to stop patches which are way too big (e.g., 600MB of videos) from generating in the first place.

Test Plan:
  - Configured inline patches in git format.
  - Created a normal revision, got an inline git patch.
  - Created a revision with a 10MB video file, got no inline patch.
  - (Added a bunch of debugging stuff to make sure the internal pathway was working.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

Differential Revision: https://secure.phabricator.com/D19445
2018-05-14 09:10:47 -07:00
Aviv Eyal
7281300446 Allow number in generated clone uri
Summary:
See https://discourse.phabricator-community.org/t/numerical-characters-are-stripped-from-diffusion-git-repository-name-in-the-uri/

Digits are often considered reasonable characters.

Test Plan: Looked at an ascii table.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, Sam2304, epriestley

Differential Revision: https://secure.phabricator.com/D19447
2018-05-11 16:18:06 +00:00
epriestley
10a4b05ecb Fix "Any Owner" and "No Owners" searches in Maniphest
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-home-page-crash-after-d19417/1445/3>. These special-token-only searches currently end up populating an empty `ownerPHIDs`, which fatals after the stricter check in D19417.

Make the fatal on `withConstraint(array())` explicit and only set the PHID constraint if we have some PHIDs left.

Test Plan: Searched for "No Owner", "Any Owner", an actual owner, "No Owner + actual user".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19440
2018-05-09 13:24:23 -07:00
epriestley
d280b24239 Fix "arc paste" to stop creating pastes with an empty string ("") as the "language"
Summary:
See PHI652. When you `echo x | arc paste` today, you end up with a Paste object that has the empty string as its "language".

This is normally not valid. Pastes where the language should be autodetected should have the value `null`, not the empty string.

This behavior likely changed when `paste.create` got rewritten in terms of `paste.edit`. Adjust the implementation so it only adds the LANGUAGE transaction if there's an actual language.

Also, fix an issue where you can't use the "delete" key to delete tokens with the empty string as their value.

Test Plan:
  - Created a paste with `echo x | arc paste`, got a paste in autodetect mode instead of with a bogus language value.
  - Created a paste with `echo x | arc paste --lang rainbow`, got a rainbow paste.
  - Deleted an empty string token with the keyboard.
  - Deleted normal tokens with the keyboard.
  - Edited subscribers/etc normally with the keyboard and mouse to make sure I didn't ruin anything.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19437
2018-05-09 13:22:58 -07:00
epriestley
26c0db8dd7 Allow navigation breadcrumbs to be marked as "always visible" so they show up on phones
Summary:
See PHI624. Some of the mobile navigation and breadcrumbs in support pacts aren't as good as they could be.

In particular, we generally collapse crumbs on mobile to just the first and last crumbs. The first crumb is the application; the last is the current page.

On `/PHIxxx` pages, the first crumb isn't very useful since the Support landing page is two levels up: you usually want to go back to the pact, not all the way back to the Support landing page.

We also don't need the space since the last crumb (`PHIxxx`) is always small.

Allow Support and other similar applications to tailor the crumb behavior more narrowly if they end up in situations like this.

Test Plan:
  - With an additional change to instances (see next diff), viewed a support issue page (`/PHI123`) on mobile and desktop.
  - Saw a link directly back to the pact on both mobile and desktop.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19438
2018-05-09 13:21:47 -07:00
epriestley
5b640a434c Support an "Ancestors Of: ..." constraint in commit queries
Summary:
Ref T13137. See PHI609. An install would like to filter audit requests on a particular branch, e.g. "master".

This is difficult in the general case because we can not apply this constraint efficiently under every conceivable data shape, but we can do a reasonable job in most practical cases.

See T13137#238822 for more detailed discussion on the approach here.

This is a bit rough, but should do the job for now.

Test Plan:
- Filtered commits by various branches, e.g. "master"; "lfs". Saw correct-seeming results.
- Stubbed out the "just list everything" path to hit the `diffusion.internal.ancestors` path, saw the same correct-seeming results.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

Differential Revision: https://secure.phabricator.com/D19431
2018-05-08 15:51:42 -07:00
epriestley
397645b273 Export task point values as double, not int
Summary:
See <https://discourse.phabricator-community.org/t/maniphest-non-integer-point-values-in-csv-export/1443>.

We currently export the Maniphest "points" field as an integer, but allow it to accept decimal values (e.g. "6.25").

Also fix a bug where we wouldn't roll over from "..., X, Y, Z, AA, AB, ..." correctly for Excel column names if sheet had more than 26 columns.

Test Plan:
  - Set a task point value to 6.25.
  - Exported to text, JSON, XLS.
  - Saw 6.25 represented accurately in exports.
  - Exported an excel sheet with 27+ columns.
  - Manually printed the first 200 column names to check that the algorithm looks correct.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19434
2018-05-08 15:49:40 -07:00
epriestley
304c6a4597 Improve UI and documentation for "Ignore Attributes" in Owners slightly
Summary:
See PHI251. Ref T13137.

  - Replace the perplexing text box with a checkbox that explains what it does.
  - Mention this feature in the documentation.

Test Plan:
  - Clicked/unclicked checkbox.
  - Read documentation.
  - Used an existing checkbox control in Slowvote to make sure I didn't break it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13137

Differential Revision: https://secure.phabricator.com/D19433
2018-05-08 14:03:30 -07:00
epriestley
fddb506e98 Don't render the Maniphest edit form bottom-of-page preview panel if "Description" is locked or hidden
Summary:
See <https://discourse.phabricator-community.org/t/hidden-description-field-in-maniphest-task-breaks-form/1432>.

If you hide the "Description" field in Maniphest, we still try to render a remarkup preview for it. This causes a JS error and a nonfunctional element on the page.

Instead, hide the preview panel if the field has been locked or hidden.

Test Plan:
  - Hid the field, loaded the form, no more preview panel / JS error.
  - Used a normal form with the field visible, saw a normal preview.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19432
2018-05-08 14:01:23 -07:00
epriestley
a4a22dd2f8 Mention the "inline comments" rule in the callout for "Large" diffs
Summary:
See PHI638. When a diff is large (between 100 and 1000 files), we collapse content by default unless a change also has inline comments.

This rule isn't explicitly explained anywhere. Although it's not really a critical rule, it fits easily enough into the UI callout.

Also render the UI callout in a slightly more modern way and avoid `hsprintf()`.

Test Plan:
{F5596496}

  - Also, clicked the "Expand" link and saw everything expand properly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19430
2018-05-07 10:38:58 -07:00
epriestley
4a98e0ff65 Allow Owners packages to be configured to ignore generated paths in Differential
Summary:
Depends on D19427. Ref T13130. See PHI251. Support configuring owners packages so they ignore generated paths.

This is still a little rough. A couple limitations:

  - It's hard to figure out how to use this control if you don't know what it's for, but we don't currently have a "CheckboxesEditField". I may add that soon.
  - The attribute ignore list doesn't apply to Diffusion, only Differential, which isn't obvious. I'll either try to make it work in Diffusion or note this somewhere.
  - No documentation yet (which could mitigate the other two issues a bit).

But the actual behavior seems to work fine.

Test Plan:
  - Set a package to ignore paths with the "generated" attribute. Saw the package stop matching generated paths in Differential.
  - Removed the attribute from the ignore list.
  - Tried to set invalid attributes, got sensible errors.
  - Queried a package with Conduit, got the ignored attribute list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19428
2018-05-05 08:47:29 -07:00
epriestley
dc510354c3 Remove explicit "mailKey" from Owners packages
Summary:
Depends on D19426. Ref T13130. Ref T13065. While I'm making changes to Owners for "Ignore generated paths", clean up the "mailKey" column.

We recently (D19399) added code to automatically generate and manage mail keys so we don't need a ton of `mailKey` properties in the future. Migrate existing mail keys and blow away the explicit column on packages.

Test Plan: Ran migration, manually looked at the database and saw sensible data. Edited a package to send some mail, which looked good.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130, T13065

Differential Revision: https://secure.phabricator.com/D19427
2018-05-05 08:47:08 -07:00
epriestley
5e2af4b9b5 Prepare to support an "Ignore generated files" flag in Owners
Summary:
Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision.

There's no way to actualy configure a package to have this behavior yet.

Test Plan:
  - Created a revision affecting a generated file and a non-generated file.
    - When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it.
    - Normally: lots of packages owning it).
  - Created a revision affecting only generated files.
    - When I faked things, saw no Owners actions trigger.
    - Normally: some packages added reviewers or subscribers.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19426
2018-05-05 08:46:47 -07:00
epriestley
af295341c8 Classify changesets as "generated" at creation time, in addition to display time
Summary:
Ref T13130. See PHI251. Currently, changesets are marked as "generated" (i.e., the file contains generated code and does not normally need to be reviewed) at display time.

An install would like support for having Owners rules ignore generated files. Additionally, future changes anticipate making "generated" and some other similar behaviors more flexible and more general.

To support these, move toward a world where:

  - Changesets have "attributes": today, generated. In the future, perhaps: third-party, highlight-as, encoding, enormous-text-file, etc.
  - Attributes are either "trusted" (usually: the server assigned the attribute) or "untrusted" (usually: the client assigned the attribute). For attributes like "highlight-as", this isn't relevant, but I'd like to provide tools so that you can't make `arc` mark every file as "generated" and sneak past review rules in the future.

Here, the `differential.generated-paths` config can mark a file as "generated" with a trusted attribute. The `@generated`-in-content rule can mark a file as "generated" with an untrusted attribute.

Putting these attributes on changesets at creation time instead of display time will let Owners interact with changesets cheaply: it won't have to render an entire changeset just to figure out if it's generated or not.

Test Plan:
  - Created a revision touching several files, some generated and some not.
  - Saw the generated files get marked properly with attribute metadata in the database, and show/fold as "Generated" in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19425
2018-05-05 08:46:25 -07:00
epriestley
5784e3d3c0 Omit "type" attribute from "<source />" tags in "<video>" to trick Chrome into playing them
Summary:
Fixes T13135. See PHI633. For at least some video files with legitimate MIME type "video/quicktime", Chrome can play them but refuses to if the `<source />` tag has a `type="video/quicktime"` attribute.

To trick Chrome into giving these videos the old college try, omit the "type" attribute. Chrome then tries to play the video, seems to realize it can, and we're back on track.

Since the "type" attribute is theoretically only useful to help browsers select among multiple different alternatives and we're only presenting one alternative, this seems likely safe and reasonable. Omitting "type" also validates. It's hard to be certain that this won't cause any collateral damage, but intuitively it seems like it should be safe and I wasn't able to identify any problems.

Test Plan:
  - Watched a "video/quicktime" MP4 cat video in Chrome/Safari/Firefox.
  - See T13135 for discussion, context, and discussion of the behavior of some smaller reproduction cases.

Reviewers: amckinley, asherkin

Reviewed By: amckinley

Maniphest Tasks: T13135

Differential Revision: https://secure.phabricator.com/D19424
2018-05-04 09:28:47 -07:00
epriestley
332f4ab66d Restore support for using "arc download" to fetch files with no "security.alternate-file-domain"
Summary:
Fixes T13132. I removed this branch in D19156 when tightening the logic for the new CSP header, but there's a legitimate need for it: downloading files via `arc download`, or more generally being an API consumer of files.

This is not completely safe, but attacks I'm aware of (particularly, cookie fixation, where an attacker could potentially force a victim to become logged in to an account they control) are difficult and not very powerful. We already issue clear setup advice about the importance of configuring this option ("Phabricator is currently configured to serve user uploads directly from the same domain as other content. This is a security risk.") and I think there's significant value in letting API clients just GET file data without having to jump through a lot of weird hoops.

Test Plan:
  - With `security.alternate-file-domain` off, tried to `arc download` a file.
  - Before: downloaded an HTML dialog page.
  - After: downloaded the file.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13132

Differential Revision: https://secure.phabricator.com/D19421
2018-05-01 10:08:05 -07:00
epriestley
fb4b9bc2fc Fix an issue where entering the same Owners path for two repositories would incorrectly de-dupe the path
Summary:
Ref T13130. See <https://discourse.phabricator-community.org/t/unable-to-create-owners-package-with-same-path-in-multiple-repositories/1400/1>.

When you edit paths in Owners, we deduplicate similar paths, like `/x/y` and `/x/y/`. However, this logic currently only examines the paths, and incorrectly deduplicates the same path in different repositories.

Instead, consider the repository before deduplicating.

Test Plan:
  - Edited an Owners package and added the path "/" in two different repositories.
  - Before: only one surived the edit.
  - After: both survived.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19420
2018-05-01 09:57:37 -07:00
epriestley
7cfac40a22 Pass full Harbormaster URIs to Buildkite
Summary: See PHI611 for details.

Test Plan:
Ran a Buildkite build, saw Buildkite confirm receipt of these parameters in the HTTP response:

{F5562054}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D19419
2018-04-30 22:32:50 -07:00
epriestley
ee32c186dd Stop computing ownership for changed paths for Very Large revisions
Summary:
Depends on D19416. Ref T13110. Ref T13130. See PHI598. When rendering a "Very Large" revision (affecting more than 1,000 files) we currently compute the package/changeset ownership map normally.

This is basically a big list of which packages own which of the files affected by the change. We use it to:

  # Show which packages own each file in the table of contents.
  # Show an "(Owns No Changed Paths)" hint in the reviewers list to help catch out-of-date packages that are no longer relevant.

However, this is expensive to build. We don't render the table of contents at all, so (1) is pointless. The value of (2) is very small on these types of changes, and certainly not worth spending many many seconds computing ownership.

Instead, just skip building out these relationships for very large changes.

Test Plan: Viewed a very large change with package owners; verified it no longer built package map data and rendered the package owners with no "(Owns No Changed Paths)" hints.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13110

Differential Revision: https://secure.phabricator.com/D19418
2018-04-30 15:44:41 -07:00
epriestley
24305cadb9 Hide the "large" diff warning on "very large" diffs
Summary:
Ref T13110. Ref T13130. When a revision is "large" (100 - 1000 files) we hide the actual textual changes by default. When it is "very large" (more than 1000 files) we hide all the changesets by default.

For "very large" diffs, we currently still show the "large" warning, which doesn't really make sense since there aren't any actual changesets.

When a diff is "very large", don't show the "large" warning.

Test Plan:
  - Viewed a small diff (<100 files), saw no warnings.
  - Viewed a large diff (100-1000 files), saw just the large warning.
  - Viewed a very large diff (>1000 files).
    - Before: both "large" and "very large" help warnings.
    - After: just "very large" warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13110

Differential Revision: https://secure.phabricator.com/D19416
2018-04-30 15:33:20 -07:00
epriestley
afc3099ee7 Add a view option to disable blame in Diffusion and fix some view transition bugs
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.

Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.

Test Plan:
  - Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
  - Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
  - Viewed stuff in Files (no blame UI options).
  - Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13105

Differential Revision: https://secure.phabricator.com/D19414
2018-04-30 15:32:23 -07:00
Austin McKinley
dd6e82698a More-robust search for task assignees
Summary: See discussion in D19415.

Test Plan: Searched for some owners, found tasks as expected.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19417
2018-04-30 12:18:09 -07:00
epriestley
ef48a2b2ee Add a "Rule Detail" link to Herald email
Summary:
See PHI285. Ref T13130. After recent changes Herald sends email about rules, but the mail doesn't currently actually include a link to the rule.

Include a link for consistency and ease-of-use.

Test Plan: Edited a rule, looked at the resulting mail, saw a link to the rule.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19413
2018-04-30 05:20:12 -07:00
Austin McKinley
9a0dd55442 Extend PhabricatorPolicyCodex interface to handle "interesting" policy defaults
Summary:
Fixes T13128. Ref PHI590. This is a rough-and-ready implementation of a new `PhabricatorPolicyCodex->compareToDefaultPolicy()` method that subclasses can override to handle special cases of policy defaults. Also implements a `PolicyCodex` for Phriction documents, because the default policy of a Phriction document is the policy of the root document.

I might break this change into two parts, one of which maintains the current behavior and another which implements `PhrictionDocumentPolicyCodex`.

Test Plan: Created some Phriction docs, fiddled with policies, observed expected colors in the header. Will test more comprehensively after review for basic reasonable-ness.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, swisspol

Maniphest Tasks: T13128

Differential Revision: https://secure.phabricator.com/D19409
2018-04-27 16:56:11 -07:00
epriestley
5f774f7008 Stop build target start times from being overwritten on reentry
Summary:
See PHI615. Ref T13130. An install is reporting that "Lease Working Copy" build steps always report "Built instantly" after completion.

I'm not 100% sure that this is the fix, but I'm like 99% sure: "Lease Working Copy" build steps yield after they ask Drydock for a lease. They will later reenter `doWork()`, see that the lease is filled, and complete.

Right now, we reset the start time every time we enter `doWork()`. Instead, set it only if it hasn't been set yet.

Test Plan: This is low-risk and a bit tricky to reproduce locally, but I'll run some production builds and see what they look like.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19412
2018-04-27 12:25:45 -07:00
epriestley
d40007aa32 Fix an issue where the Herald test console doesn't work with "Content source" rules
Summary:
Ref T13130. See PHI619. Currently, the Herald "Test Console" doesn't pass a "Content Source" to the adapter, so if any rules of the given type execute a "Content source" field rule, they'll fatal.

Provide a content source:

  - If possible, use the content source from the most recent transaction.
  - Otherwise, build a default "web" content source from the current request.

Test Plan:
  - Wrote a "When [content source][is][whatever]" rule for tasks.
  - Ran test console against a task.
  - Before: got a fatal trying to interact with the content source.
  - After: transcript reports sensible content source.
    - Also commented out the "xaction" logic to test the fallback behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19411
2018-04-27 12:25:24 -07:00
epriestley
223d7b84dd Recover more gracefully when favicon configuration points at a corrupt/damaged file
Summary:
Ref T13103. Locally, I managed to break the data for a bunch of files by doing `git clean -df` in a working copy that I'd updated to a commit from many many years ago. Since `conf/local.json` wasn't on the gitignore list many years ago, this removed it, and I lost my encryption keyring.

I've symlinked my local config to a version-controlled file now to avoid this specific type of creative self-sabotage in the future, but this has exposed a few cases where we could handle things more gracefully.

One issue is that if your favicon is customized but the file it points at can't actually be loaded, we fail explosively and you really can't do anything to move forward except somehow guess that you need to fix your favicon. Instead, recover more gracefully.

Test Plan:
  - Configure file encryption.
  - Configure a favicon.
  - Remove the encryption key from your keyring.
  - Purge Phabricator's caches.
  - Before: you pretty much dead-end on a fatal that's hard to understand/fix.
  - After: everything works except your favicon.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13103

Differential Revision: https://secure.phabricator.com/D19406
2018-04-27 12:02:32 -07:00
epriestley
28517110c6 Fix an issue in the new Harbormaster build log view where clicking the "^" icon doesn't work right
Summary:
Ref T13130. See PHI617.

The new build log UI has tags like `<a href="...">Show More Above <span icon>^</span></a>`. If you click the little "^" icon, the event target is the `<span />` instead of the `<a />` so we expand on the wrong node.

Instead, select the `<a />` by sigil explicitly.

Test Plan: Viewed new log UI in Harbormaster, clicked "^" icon and text, got the same (correct) behavior on both.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19410
2018-04-27 11:51:59 -07:00
epriestley
9f8e0ad473 Remove unusual unicode marks in Differential action dropdown
Summary:
See <https://twitter.com/HayleyCAnderson/status/988873585363009536>.

Currently, the action dropdown in Differential shows a heavy "X" after "Request Changes" and a heavy checkmark after "Accept Revision".

Although I'm not convinced that the messaging around "Request Changes" is too strong, I do think these marks are out of place in modern Differential. They came from a simpler time when this dropdown had fewer actions, but feel a little weird and inconsistent to me in the modern UI.

Let's try getting rid of them and see how it goes?

Test Plan:
  - Viewed these actions in the dropdown, no longer saw the mark icons.
  - Grepped for these unicode sequences without getting any other hits.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19405
2018-04-27 11:00:56 -07:00
epriestley
b4796d2837 Add "Content type" and "Rule type" fields to Herald rules for Herald rules
Summary:
Depends on D19400. Ref T13130. Currently, when you write Herald rules about other Herald rules, you can't pick a rule type or content type, so there's no way to get notified about edits to just global rules (which is the primary driving use case).

Add a "Content type" field to let the rule match rules that affect revisions, tasks, commits, etc.

Add a "Rule type" field to let the rule match global, personal, or object rules.

Test Plan:
  - Wrote a global rule for other rules about global Herald rules:

{F5540307}

{F5540308}

  - Ran it against itself which matched:

{F5540309}

  - Ran it against another rule (not a global rule about Herald rules), which did not match:

{F5540311}

  - Also reviewed the fields in those transcripts in more detail to make sure they were extracting matching correctly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19403
2018-04-25 06:54:48 -07:00
epriestley
2319d2ede2 Fix a typo in the Diffusion importing user guide
Summary: This word isn't the right word.

Test Plan: Reading?

Reviewers: avivey, amckinley

Reviewed By: avivey

Differential Revision: https://secure.phabricator.com/D19404
2018-04-25 06:54:15 -07:00
epriestley
cac41d1e48 Support Herald rules for Herald rules
Summary:
Depends on D19399. Ref T13130. This adds basic support for writing Herald rules against Herald rules. See T13130 for a lot more detail.

This needs a bit more work to be useful: for example, there's no way to specify the rule type or subject, so you can't say "notify me when global rules are edited" or "notify me when Maniphest rules are edited". I'll add some fields for that in followup changes to actually solve the original use case.

Test Plan:
  - Wrote Herald rules against Herald rules.
  - Ran them by editing rules and in the test console.
  - Verified they sent some mail with `bin/mail list-outbound`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19400
2018-04-25 06:47:19 -07:00
epriestley
1b24b486f5 Manage object mailKeys automatically in Mail instead of storing them on objects
Summary:
Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef".

  - The `T123` is the object you're actually replying to.
  - The `456` is your user ID.
  - The `abcdef` is a hash of your user account with the `mailKey`.

Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username.

To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.)

Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code.

Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually.

Test Plan:
  - See next change for additional testing and context.
  - Sent mail about Herald rules (next change); saw mail keys generate cleanly.
  - Destroyed a Herald rule with a mail key, saw the mail properties get nuked.
  - Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places.
  - Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13065

Differential Revision: https://secure.phabricator.com/D19399
2018-04-25 06:46:58 -07:00
epriestley
16af0d35e5 In Differential, prevent "Accept" and "Reject" from "Plan Changes + Draft"
Summary:
Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept".

This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency.

Also fix a string that didn't have a natural English version.

Test Plan:
  - Added a failing build plan.
  - Created a revision.
  - Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)".
  - Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft".
  - As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept").
  - Abandoned the revision to test "Abandoned + Draft".
  - As the author and as a reviewer, verified all the actions available to me made sense.
  - Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130

Differential Revision: https://secure.phabricator.com/D19398
2018-04-23 14:39:36 -07:00
Aviv Eyal
15e6e2adea Update install_ubuntu.sh to the new age
Summary:
Ref T4200. Since the last time this script was written, Ubuntu has made lots of changes.
Try to keep up with those.

Test Plan:
Ran this on frash installs of Ubuntu 16.04 LTS and 18.04 LTS (Pre-release).
Got to see Phabricator running.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: amckinley, Korvin

Maniphest Tasks: T4200

Differential Revision: https://secure.phabricator.com/D19394
2018-04-21 09:57:33 -07:00
epriestley
7622f6afcc Fix excessively severe CSP URI error during first-time setup
Summary:
See D19394. Currently, during first-time setup before you configure "phabricator.base-uri", we may attempt to generate a setup page, try to generate a CSP header for it, and fail to access the environmental config. This causes a too-severe error page ("configure phabricator.base-uri") instead of preflight guidance (like "can't connect to MySQL").

Instead, treat this more like "security.alternate-file-domain" and just bail on CSP if we can't fetch it.

Test Plan: On a fresh (non-explodey laptop) install with critical setup errors (no MySQL installed yet), loaded Phabricator. Before: error about phabricator.base-uri. After: more helpful guidance about installing/configuring MySQL.

Reviewers: amckinley, avivey

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19396
2018-04-21 09:43:46 -07:00
epriestley
33da9f833f Fix odd line number line wrapping on embedded pastes ({Pxxx})
Summary: Ref T13126. After SourceView changes, embedded pastes with the `{Pxxx}` syntax are line-wrapping line numbers in Safari, at least. Put a stop to this.

Test Plan: Viewed a `{Pxxx}` with more than 10 lines. Before: weird line wrapping; after: nice consistent display.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13126

Differential Revision: https://secure.phabricator.com/D19393
2018-04-20 14:20:20 -07:00