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

13997 commits

Author SHA1 Message Date
epriestley
af1d494d66 Fix an issue where rejecting reviewers weren't powerful enough
Summary:
Previously, "reject" and "reject older" were separate statuses. Now, they're both shades of "reject".

Set the "older reject" flag properly when we find a non-current reject.

Test Plan:
  - User A accepts a revision.
  - User B rejects it.
  - Author updates it.
  - Before patch: incorrectly transitions to "accepted" ("older" reject is ignored).
  - After patch: correctly transitions to "needs review".

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17653
2017-04-11 09:54:34 -07:00
Chad Little
28941b3105 Update PhortuneMerchant to Modular Transactions
Summary: Modernize PhortuneMerchant for Modular Transactions. Also changed the language of "Members" to "Managers", which I think fits better given the power/capability.

Test Plan:
- Create a new Merchant
- Test not filling in a name, see error
- Test removing myself, see error
- Edit an existing Merchant
- Add new managers
- Test removing myself, see error
- Replace Picture
- Update various fields, contact info, email, footer
- Verify transactions are now nice and pretty

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17584
2017-04-11 09:32:12 -07:00
epriestley
26d6096e0a When reviewing, always show "Accept" checkboxes for packages/projects, even if there's only one checkbox
Summary: Fixes T12533.

Test Plan: {F4853371}

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12533

Differential Revision: https://secure.phabricator.com/D17652
2017-04-10 17:28:02 -07:00
epriestley
5cf53f7b3b Fix some minor curtain overflow/wrap issues
Summary:
Fixes T12503.

  - Users with creative usernames like `MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM` could overflow "Subscribers" in the curtain UI.
  - Other content like packages could also overflow.
  - Users with interesting and unique names like `WWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW` who were also away or disabled could get a linebreak between their availability dot and their username.

Test Plan:
See T12503 for "before" screenshots. Also tested mobile, which looked fine, but didn't screenshot it.

{F4849900}

{F4849912}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12503

Differential Revision: https://secure.phabricator.com/D17650
2017-04-10 16:02:00 -07:00
epriestley
a7a068f84c Correct two parameter strictness issues with file uploads
Summary:
Fixes T12531. Strictness fallout from adding typechecking in D17616.

  - `chunkedHash` is not a real parameter, so the new typechecking was unhappy about it.
  - `mime-type` no longer allows `null`.

Test Plan:
  - Ran `arc upload --conduit-uri ... 12MB.zero` on a 12MB file full of zeroes.
  - Before patch: badness, failure, fallback to one-shot uploads.
  - After patch: success and glory.

Reviewers: chad

Subscribers: joshuaspence

Maniphest Tasks: T12531

Differential Revision: https://secure.phabricator.com/D17651
2017-04-10 16:01:15 -07:00
epriestley
49132b884b Sell Yellow! Buy Indigo!
Summary: Fixes T12504. Replaces all tags with indigo.

Test Plan: {F4849487}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12504

Differential Revision: https://secure.phabricator.com/D17649
2017-04-10 15:01:10 -07:00
Chad Little
4a84954957 Prevent Send on Enter in Fullscreen Remarkup Mode
Summary: Fixes T12138. Test for the presence of being in fullscreen mode, and disable send on enter if present. Side note, I'd love a first class "hasClass" type Javelin function.

Test Plan:
- Go to Conpherence
- Type some smack, see it send on enter
- Go fullscreen like a boss
- Let the words flow
- Close fullscreen, then send on enter.
- (might be nice someday to add a "submit" button to fullscreen editor)

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12138

Differential Revision: https://secure.phabricator.com/D17590
2017-04-10 14:39:50 -07:00
epriestley
dee9c33be2 Suggest use of "usermod" rather than manually editing critical files in /etc
Summary: Fixes T12529.

Test Plan: O_O

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12529

Differential Revision: https://secure.phabricator.com/D17648
2017-04-10 14:15:38 -07:00
epriestley
00a1dec7a6 Render timezones in event reminder mail, and render them more nicely
Summary:
Fixes T12356.

  - In this mail, we currently render "6:00 AM". Instead, render "6:00 AM (PDT)" or similar. This is consistent with times in other modern Transaction mail.
  - Previously, we would render "UTC-7". Render "PDT" instead. For obscure zones with no known timezone abbreviation, fall back to "UTC-7".

Test Plan:
  - Used `bin/calendar notify --minutes X` to trigger notifications, read email bodies.
  - Used this script to list all `T` values and checked them for sanity:

```lang=php
<?php

$now = new DateTime();

$locales = DateTimeZone::listIdentifiers();
foreach ($locales as $locale) {
  $zone = new DateTimeZone($locale);
  $now->setTimeZone($zone);

  printf(
    "%s (%s)\n",
    $locale,
    $now->format('T'));
}
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12356

Differential Revision: https://secure.phabricator.com/D17646
2017-04-10 08:48:37 -07:00
epriestley
50e809e06f Fix an issue where recurring ghost events could go missing if queried with a limit
Summary:
Ref T11816. Depends on D17644. When you executed a query like "upcoming, limit 5 events" you might match some recurring events starting from, say, a year ago and repeating every month.

We'd then generate the first 5 ghosts for these events (say, last January, February, ... May) and later throw them out, so the correct events in the query window (say, this April) would never get generated.

Instead, generate ghosts beginning with the start of the window. The fix in D17644 to number results correctly allows us to do this.

Test Plan:
  - Made a query panel showing 5 events, scheduled an event long in the past, did not visit any of the instances of it so they didn't generate concrete objects.
  - Before the patch, near-future instances failed to show; after the patch, they show.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D17645
2017-04-10 08:48:21 -07:00
epriestley
ab06a9681c Fix two issues with user Calendar event availability cache display
Summary:
Ref T11816. Two minor issues:

  - We used `$event`, not `$next_event`, as the event providing the PHID for "Busy at <event name>". This rendered "Busy at <most future event>" on the profile instead of "Busy at <next upcoming event".
  - The TTL computation used the event start, not the event end, so we could end up rebuilding the cache too often for users busy at an event.

Test Plan:
  - Attended an event in the near future and one later on.
  - Saw profile now say "busy at <near future event>" correctly.
  - In DarkConsole "Services" tab, no longer saw unnecessary cache refills while attending an event.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11816

Differential Revision: https://secure.phabricator.com/D17643
2017-04-10 08:47:27 -07:00
epriestley
d4d46501ed Add a repair migration to fix legacy Calendar events without "utcInstanceEpoch"
Summary: Fixes T12488. Some events appear to have survived earlier migrations without getting completely fixed. Fix them.

Test Plan:
  - Ran migration locally with `bin/storage upgrade` (but: I could not reproduce this problem locally).
  - Ran migration in production and saw ICS import stop fataling.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12488

Differential Revision: https://secure.phabricator.com/D17642
2017-04-10 08:47:07 -07:00
epriestley
c9f51fd405 Write a "Developer Setup" guide for onboarding
Summary: Fixes T11561. Collect guidance about local configuration which hasn't been obvious in the past.

Test Plan:
  - Read document carefully.
  - Used `./bin/diviner generate` to generate documentation.
  - Previewed in Diviner locally:

{F4795021}

Reviewers: amckinley, chad

Reviewed By: chad

Subscribers: cspeckmim

Maniphest Tasks: T11561

Differential Revision: https://secure.phabricator.com/D17641
2017-04-10 08:36:49 -07:00
epriestley
d1421bc3a1 Add "bin/storage optimize" to run OPTIMIZE TABLE on everything
Summary:
Even with `innodb_file_per_table` enabled, individual table files on disk don't normally shrink.

For most tables, like `maniphest_task`, this is fine, since the data in the table normally never shrinks, or only shinks a tiny amount.

However, some tables (like the "worker" and "daemon" tables) grow very large during a huge import but most of the data is later deleted by garbage collection. In these cases, this lost space can be reclaimed by running `OPTIMIZE TABLE` on the tables.

Add a script to `OPTIMIZE TABLE` every table.

My primary goal here is just to reduce storage pressure on `db001` since there are a couple of "import the linux kernel" installs on that host wasting a bunch of space. We're not in any trouble, but this should buy us a good chunk of headroom.

Test Plan: Ran `bin/storage optimize` locally and manually ran `OPTIMIZE TABLE` in production, saw tables get optimized.

Reviewers: chad

Reviewed By: chad

Subscribers: cspeckmim

Differential Revision: https://secure.phabricator.com/D17640
2017-04-08 15:15:49 -07:00
epriestley
7707685733 Fix two strings with missing pht()
Summary: Fixes T12517.

Test Plan: Viewed Config application; viewed repository list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12517

Differential Revision: https://secure.phabricator.com/D17639
2017-04-07 10:07:01 -07:00
Rabah Meradi
0bf106eeea Fix 4 typos in code
Summary: Fixes T12516

Test Plan: grep for those typos

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12516

Differential Revision: https://secure.phabricator.com/D17638
2017-04-07 04:09:56 -07:00
epriestley
9856802ba2 Disallow /source/ in robots.txt
Summary: Ref T4245. We disallow `/diffusion/` in robots.txt already because indexers tend to get lost blaming every line of every file throughout history, but didn't update the list for the `/source/` alias. Update it.

Test Plan: Visited `/robots.txt` locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D17637
2017-04-06 16:28:09 -07:00
epriestley
3d816e94df Rename "PhabricatorHash::digest()" to "weakDigest()"
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17632
2017-04-06 15:43:33 -07:00
epriestley
3a3626834e Replace Remarkup calls to PhabricatorHash::digest() with SHA256
Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.

Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.

I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.

Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17631
2017-04-06 15:43:18 -07:00
epriestley
d450a08890 Support HMAC+SHA256 with automatic key generation and management
Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.

The new mechanism also automatically generates and stores HMAC keys.

Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.

We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.

Beyond that, this isn't something users should really have to think about or bother configuring.

Test Plan:
  - Added unit tests.
  - Used `bin/files integrity` to verify, strip, and recompute hashes.
  - Tampered with a generated HMAC key, verified it invalidated hashes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17630
2017-04-06 15:42:59 -07:00
epriestley
08a4225437 Provide "bin/files integrity" for debugging, maintaining and backfilling integrity hashes
Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:

  - Verify: check that hashes match.
  - Compute: backfill missing hashes.
  - Strip: remove hashes. Useful for upgrading across a hash change.
  - Corrupt: intentionally corrupt hashes. Useful for debugging.
  - Overwrite: force hash recomputation.

Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.

I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.

Test Plan:
  - Ran the script in many modes against various files, saw expected operation, including:
  - Verified a file, corrupted it, saw it fail.
  - Verified a file, stripped it, saw it have no hash.
  - Stripped a file, computed it, got a clean verify.
  - Stripped a file, overwrote it, got a clean verify.
  - Corrupted a file, overwrote it, got a clean verify.
  - Overwrote a file, overwrote again, got a no-op.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17629
2017-04-06 15:42:43 -07:00
epriestley
845a7d8716 Allow the PullLocal daemon to actually hibernate
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.

Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.

Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.

Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17635
2017-04-06 15:41:19 -07:00
epriestley
f1eeaaf59f Fix scope of "Accept" when you don't check all the "Force Accept" boxes
Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".

This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.

Test Plan:
  - Did an accept with a force-accept available but unchecked.
  - Before patch: incorrectly accepted all possible reviewers.
  - After patch: accepted only checked reviewers.
  - Also checked the force-accept box, accepted, got a proper force-accept.

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17634
2017-04-06 15:03:32 -07:00
epriestley
cefbdbcffe Provide a "Reviewers" attachment to "differential.revision.search"
Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan: {F4675784}

Reviewers: chad, lvital

Reviewed By: lvital

Subscribers: lvital

Differential Revision: https://secure.phabricator.com/D17633
2017-04-06 14:46:39 -07:00
epriestley
2f4ff6a850 Fix bad "editPolicy" key in Paste
Summary: Fixes T12508. Files don't have an `editPolicy`, and we started actually checking that the keys are real things in D17616.

Test Plan:
  - Before patch: created a paste, got an "editPolicy" exception.
  - After patch: created a paste that worked properly.

Reviewers: avivey, chad

Reviewed By: avivey

Maniphest Tasks: T12508

Differential Revision: https://secure.phabricator.com/D17628
2017-04-05 13:09:51 -07:00
epriestley
f2a26b2601 When requesting file data, make "Range: bytes=0-" work correctly
Summary: Ref T12219. Chrome can send requests with a "Range: bytes=0-" header, which just means "the whole file", but we don't respond correctly because of a `null` vs `0` issue.

Test Plan: Sent a raw `bytes=0-` request, saw a proper resonse.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12219

Differential Revision: https://secure.phabricator.com/D17627
2017-04-05 11:48:33 -07:00
epriestley
d1a971e221 Support "Range: bytes=123-" requests
Summary:
Ref T12219. We currently only support Range requests like "bytes=123-456", but "bytes=123-", meaning "until end of file", is valid, and Chrome can send these requests.

I suspect this is the issue with T12219.

Test Plan: Used `nc local.phacility.com 80` to pipe raw requests, saw both "bytes=123-456" and "bytes=123-" requests satisfied correctly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12219

Differential Revision: https://secure.phabricator.com/D17626
2017-04-05 11:25:44 -07:00
epriestley
63828f5806 Store and verify content integrity checksums for files
Summary:
Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access.

Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files.

By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering.

This also helps detect mundane corruption and integrity issues.

Test Plan:
  - Added unit tests.
  - Uploaded new files, saw them get integrity hashes.
  - Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway.
  - Tampered with IVs, saw integrity failures.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17625
2017-04-05 11:12:31 -07:00
epriestley
c4c3de7bfa Correct an issue where the wrong "Content-Length" was set for partial content responses
Summary: Ref T8266. Although we compute this correctly above, we ignored it when actually setting the header. Use the computed value to set the "Content-Length" header. This is consistent with the spec/documentation.

Test Plan: Before, some audio (like `rain.mp3`) was pretty spotty about loading in Safari. It now loads consistently for me locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8266

Differential Revision: https://secure.phabricator.com/D17624
2017-04-05 10:03:48 -07:00
epriestley
45fc4f6f64 Iterate over ranges correctly for encryped files
Summary:
Fixes T12079. Currently, when a file is encrypted and a request has "Content-Range", we apply the range first, //then// decrypt the result. This doesn't work since you can't start decrypting something from somewhere in the middle (at least, not with our cipher selection).

Instead: decrypt the result, //then// apply the range.

Test Plan: Added failing unit tests, made them pass

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12079

Differential Revision: https://secure.phabricator.com/D17623
2017-04-05 09:56:30 -07:00
epriestley
f70ff34d97 Fix a copy/paste typo with sticky accept
The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.

Auditors: chad
2017-04-04 18:33:59 -07:00
epriestley
58011a4e8e Upgrade File content hashing to SHA256
Summary:
Ref T12464. This defuses any possible SHA1-collision attacks by using SHA256, for which there is no known collision.

(SHA256 hashes are larger -- 256 bits -- so expand the storage column to 64 bytes to hold them.)

Test Plan:
  - Uploaded the same file twice, saw the two files generate the same SHA256 content hash and use the same underlying data.
  - Tried with a fake hash algorihtm ("quackxyz") to make sure the failure mode worked/degraded correctly if we don't have SHA256 for some reason. Got two valid files with two copies of the same data, as expected.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17620
2017-04-04 16:23:08 -07:00
epriestley
440ef5b7a7 Remove SHA1 file content hashing and make Files work without any hashing
Summary:
Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data.

Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack.

Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates.

(This mechanism is also less important now than it once was, before we added temporary files.)

Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17619
2017-04-04 16:22:10 -07:00
epriestley
1e181f0781 Deprecate "file.uploadhash"
Summary:
Ref T12464. This is a very old method which let you create a file on the server by referring to data which already existed in another file.

Basically, long ago, `arc` could say "Do you already have a file with hash X?" and just skip some work if the server did.

`arc` has not called this method since D13017, in May 2015.

Since it's easy to do so, just make this method pretend that it never has the file. Very old clients will continue to work, since they would expect this response in the common case and continue by uploading data.

Test Plan:
  - Grepped for `uploadhash` in Phabricator and Arcanist.
  - Called the method with the console, verified it returned `null`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17618
2017-04-04 16:18:26 -07:00
epriestley
873b39be82 Remove PhabricatorFile::buildFromFileDataOrHash()
Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.

In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.

Specifically, this is the case where we get into trouble:

  - I upload a private file with content "X".
  - You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
  - If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.

Just get rid of this.

Test Plan:
  - Generated an SSH key.
  - Viewed a raw diff in Differential.
  - (Did not test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17617
2017-04-04 16:18:00 -07:00
epriestley
45b386596e Make the Files "TTL" API more structured
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.

For example, to mean "this file expires in 60 minutes", you might pass either of these:

  - `time() + phutil_units('60 minutes in seconds')`
  - `phutil_units('60 minutes in seconds')`

The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.

Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.

Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17616
2017-04-04 16:16:28 -07:00
epriestley
2896da384c Only require POST to fetch file data if the viewer is logged in
Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.

However, in some cases you can't GET this URI because of a security measure:

  - You have not configured `security.alternate-file-domain`.
  - The file isn't web-viewable.
  - (The request isn't an LFS request.)

The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.

Test Plan:
  - From the browser (with a session), tried to GET a binary data file. Got redirected.
  - Got a download with POST.
  - From the CLI (without a session), tried to GET a binary data file. Go a download.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17613
2017-04-04 16:16:01 -07:00
epriestley
2369fa38e1 Provide a modern ("v3") API for querying files ("file.search")
Summary: Ref T11357. Implements a modern `file.search` for files, and freezes `file.info`.

Test Plan: Ran `file.search` from the Conduit console.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17612
2017-04-04 16:15:36 -07:00
epriestley
260a08a128 Move Files editing and commenting to EditEngine
Summary:
Ref T11357. This moves editing and commenting (but not creation) to EditEngine.

Since only the name is really editable, this is pretty straightforward.

Test Plan: Renamed files; commented on files.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17611
2017-04-04 16:15:11 -07:00
epriestley
8500f78e45 Move Files to ModularTransactions
Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name.

Test Plan:
Created and edited files.

{F4559287}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17610
2017-04-04 10:25:05 -07:00
epriestley
5e44711218 Provide a missing feed transaction string for space creation
Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.

This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.

Test Plan:
  - Created a task in a space.
  - Viewed feed.
  - Saw the story render with readable text.

{F4555747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12502

Differential Revision: https://secure.phabricator.com/D17609
2017-04-04 10:24:11 -07:00
epriestley
9ebb5f8cda Don't downgrade accepts on update (fix "sticky accept")
Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.

Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.

Test Plan:
  - With sticky accept off, updated an accepted revision: new state is "needs review".
  - With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
  - Did "reject" + "request review" to make sure that still works, worked fine.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12496

Differential Revision: https://secure.phabricator.com/D17605
2017-04-03 09:55:22 -07:00
epriestley
163e1ec442 Expose the commit/task/revision relationship edges to "edge.search"
Summary: Fixes T12480.

Test Plan: {F4465908}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12480

Differential Revision: https://secure.phabricator.com/D17604
2017-04-02 19:49:55 -07:00
epriestley
009aff1a23 Return task descriptions from "maniphest.search"
Summary:
Fixes T12461. This returns the field as a dictionary with a `"raw"` value, so we could eventually do this if we want without breaking the API:

```
{
  "type": "remarkup",
  "raw": "**raw**",
  "html": "<strong>raw</strong>",
  "text": "raw"
}
```

Test Plan: Called `maniphest.search`, reviewed output.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12461

Differential Revision: https://secure.phabricator.com/D17603
2017-04-02 17:44:22 -07:00
epriestley
7e6f37fffb Rename "ElasticSearch" filenames to "Elasticsearch" (2/2)
Sometimes git does some odd magic on case-insensitive filesystems, try to
trick it.

Auditors: chad
2017-04-02 14:59:36 -07:00
epriestley
a9e2732a5c Spell "Elasticsearch" correctly, not "ElasticSearch"
Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding.

Test Plan: `grep ElasticSearch`

Reviewers: chad, 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17601
2017-04-02 14:58:59 -07:00
epriestley
304d19f92a After a fulltext write to a particular service fails, keep trying writes to other services
Summary:
Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues.

Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update).

Test Plan:
  - Configured a bad ElasticSearch engine and a good MySQL engine.
  - Ran `bin/search index ... --force`.
  - Saw MySQL get updated even though ElasticSearch failed.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17599
2017-04-02 13:47:52 -07:00
epriestley
0f144d29e9 When "cluster.search" changes, don't trust the old index versions
Summary:
Ref T12450. We track a "document version" for updating search indexes, so that if a document is rapidly updated many times in a row we can skip most of the work.

However, this version doesn't consider "cluster.search" configuration, so if you add a new service (like a new ElasticSearch host) we still think that every document is up-to-date. When you run `bin/search index` to populate the index (without `--force`), we just do nothing.

This isn't necessarily very obvious. D17597 makes it more clear, by printing "everything was skipped and nothing happened" at the end.

Here, fix the issue by considering the content of "cluster.search" when computing fulltext document versions: if you change `cluster.search`, we throw away the version index and reindex everything.

This is slightly more work than we need to do, but changes to "cluster.search" are rare and this is much easier than trying to individually track which versions of which documents are in which services, which probably isn't very useful anyway.

Test Plan:
  - Ran `bin/search index --type project`, saw everything get skipped.
  - Changed `cluster.search`.
  - Ran `search index` again, saw everything get updated.
  - Ran a third time without changing `cluster.search`, everything was properly skipped.

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17598
2017-04-02 13:45:48 -07:00
epriestley
bd93978200 Count and report skipped documents from "bin/search index"
Summary:
Ref T12450. There's currently a bad behavior where inserting a document into one search service marks it as up to date everywhere.

This isn't nearly as obvious as it should be because `bin/search index` doesn't make it terribly clear when a document was skipped because the index version was already up to date.

When running `bin/seach index` without `--force` or `--background`, keep track of updated vs not-updated documents and print out some guidance. In other configurations, try to provide more help too.

Test Plan: {F4452134}

Reviewers: chad, 20after4

Reviewed By: 20after4

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17597
2017-04-02 13:45:30 -07:00
epriestley
6d81675032 Remove "url" from Elasticsearch index
Summary:
Ref T12450. This was added a very very long time ago (D2298).

I don't want to put this in the upstream index anymore because I don't want to encourage third parties to develop software which reads the index directly. Reading the index directly is a big skeleton key which bypasses policy checks.

This was added before much of the policy model existed, when that wasn't as much of a concern. On a tecnhnical note, this also doesn't update when `phabricator.base-uri` changes.

This can be written as a search index extension if an install relies on it for some bizarre reason, although none should and I'm unaware of any actual use cases in the wild for it, even at Facebook.

Test Plan: Indexed some random stuff into ElasticSearch.

Reviewers: chad, 20after4

Reviewed By: chad

Maniphest Tasks: T12450

Differential Revision: https://secure.phabricator.com/D17600
2017-04-02 13:26:45 -07:00