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

15862 commits

Author SHA1 Message Date
epriestley
98fe8fae4a Use <td class="n" data-n="3"> instead of <th>3</th> for line numbers
Summary:
Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `<th>3</th>` was a perfectly fine way to render a line number cell containing the number "3".

But times have changed!

  - In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to `<td>`.
  - In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own.

The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately.

Test Plan:
  - Created, edited, deleted inlines in 1-up and 2-up views.
  - Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T12822

Differential Revision: https://secure.phabricator.com/D20188
2019-02-19 15:07:02 -08:00
epriestley
3f8eccdaec Put some whitespace behaviors back, but only for "diff alignment", not display
Summary:
Depends on D20185. Ref T13161. Fixes T6791.

See some discusison in T13161. I want to move to a world where:

  - whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
  - the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.

D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.

However, a second aspect to this is how the diff is "aligned". If this file:

```
A
```

..is changed to this file:

```
X
A
Y
Z
```

...diff tools will generally produce this diff:

```
+ X
  A
+ Y
+ Z
```

This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:

```
       1 X
1 A    2 A
       3 Y
       4 Z
```

However, if the new file looks like this instead:

```
X
A'
Y
Z
```

...we get a diff like this:

```
- A
+ X
+ A'
+ Y
+ Z
```

This one aligns like this:

```
1 A
        1 X
        2 A'
        3 Y
        4 Z
```

This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment:

```
        1 X
1 A     2 A'
        3 Y
        4 Z
```

Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.

Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.

This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:

  - Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc.
  - Normalize the files (for example, by removing whitespace from each line).
  - Diff the normalized files to produce a second diff.
  - Parse that diff.
  - Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.

Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).

Test Plan:
{F6217133}

(Also, fix a unit test.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T6791

Differential Revision: https://secure.phabricator.com/D20187
2019-02-19 13:11:50 -08:00
epriestley
5310f1cdd9 Remove all whitespace options/configuration everywhere
Summary:
Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely.

I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction.

Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161, T3498

Differential Revision: https://secure.phabricator.com/D20185
2019-02-19 13:09:29 -08:00
epriestley
661c758ff9 Render indent depth changes more clearly
Summary:
Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`.

However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach.

We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration.

I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc.

Move toward detecting and rendering indent depth changes. Followup work:

  - These should get colorblind colors and the design can probably use a little more tweaking.
  - The OneUp mode is okay, but could be improved.
  - Whitespace mode can now be removed completely.
  - I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right.

Test Plan: {F6214084}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13161

Differential Revision: https://secure.phabricator.com/D20181
2019-02-19 12:40:05 -08:00
epriestley
deea2f01f5 Allow unit tests to have arbitrarily long names (>255 characters)
Summary:
Depends on D20179. Ref T13088. See PHI351. See PHI1018. In various cases, unit tests names are 19 paths mashed together.

This is probably not an ideal name, and the test harness should probably pick a better name, but if users are fine with it and don't want to do the work to summarize on their own, accept them. We may summarize with "..." in some cases depending on how this fares in the UI.

The actual implementation is a separate "strings" table which is just `<hash-of-string, full-string>`. The unit message table can end up being mostly strings, so this should reduce storage requirements a bit.

For now, I'm not forcing a migration: new writes use the new table, existing rows retain the data. I plan to provide a migration tool, recommend migration, then force migration eventually.

Prior to that, I'm likely to move at least some other columns to use this table (e.g., lint names), since we have a lot of similar data (arbitrarily long user string constants that we are unlikely to need to search or filter).

Test Plan: {F6213819}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13088

Differential Revision: https://secure.phabricator.com/D20180
2019-02-19 11:21:42 -08:00
epriestley
312ba30714 Don't report search indexing errors to the daemon log except from "bin/search index"
Summary:
Depends on D20177. Fixes T12425. See <https://discourse.phabricator-community.org/t/importing-libphutil-repository-on-fresh-phabricator-triggers-an-error/2391/>.

Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state).

I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs.

Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error.

Test Plan:
  - Faked errors in both cases (initial load, re-load inside the locked section).
  - Ran indexes in strict/non-strict mode.
  - Got exception reports from both branches in strict mode.
  - Got task success without errors in both cases in non-strict mode.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12425

Differential Revision: https://secure.phabricator.com/D20178
2019-02-19 11:17:11 -08:00
epriestley
aa470d2154 Show user availability dots (red = away, orange = busy) in typeaheads, tokenizer tokens, and autocompletes
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).

They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.

(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)

Test Plan:
{F6212044}

{F6212045}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20173
2019-02-19 10:57:20 -08:00
epriestley
92abe3c8fb Extract scope line selection logic from the diff rendering engine so it can reasonably be iterated on
Summary:
Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context.

We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time.

Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics.

Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249, T11738

Differential Revision: https://secure.phabricator.com/D20171
2019-02-19 10:55:10 -08:00
epriestley
8d348e2eeb Clean up a couple of %Q issues in "Has Parents" task queries
Summary: Stragglers from the great "%Q" migration.

Test Plan: Ran a query for tasks with parent tasks.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20183
2019-02-19 10:54:23 -08:00
epriestley
e44b40ca4d Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT"
Summary:
Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.

Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.

Increase the requirements for the actual transaction, which mostly applies to API changes:

  - To remove subscribers other than yourself, require CAN_EDIT.
  - To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.

This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.

Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20174
2019-02-19 10:52:34 -08:00
epriestley
8cf6c68c95 Fix a PhutilURI issue in workboards
Ref PHI1082.
2019-02-19 09:01:10 -08:00
epriestley
adab702403 Fix a PhutilURI issue in Multimeter
Fished this out of the `secure` error logs.
2019-02-17 17:39:34 -08:00
epriestley
dbcf41dbea Fix a couple more "URI->alter()" callsites in paging code
Summary: `grep` had a hard time finding these.

Test Plan: Will just hotfix this since I'm still reasonably in the deploy window, this currently fatals: <https://secure.phabricator.com/search/query/_dgatshiRBSy/#R>

Reviewers: amckinley

Differential Revision: https://secure.phabricator.com/D20186
2019-02-16 07:28:35 -08:00
epriestley
3058cae4b8 Allow task statuses to specify that either "comments" or "edits" are "locked"
Summary:
Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things).

Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.)

When "edits" are locked:

  - The edit policy looks like "no one" to normal callers.
  - In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One".
  - We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable.

For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene?

Test Plan:
  - Defined "Soft Locked" and "Hard Locked" statues.
  - "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task).
  - Saw nice new policy guidance icon in header.

{F6210362}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20165
2019-02-15 19:18:40 -08:00
epriestley
0b2d25778d Add basic, rough support for changing field behavior based on object subtype
Summary:
Ref T13248. This will probably need quite a bit of refinement, but we can reasonably allow subtype definitions to adjust custom field behavior.

Some places where we use fields are global, and always need to show all the fields. For example, on `/maniphest/`, where you can search across all tasks, you need to be able to search across all fields that are present on any task.

Likewise, if you "export" a bunch of tasks into a spreadsheet, we need to have columns for every field.

However, when you're clearly in the scope of a particular task (like viewing or editing `T123`), there's no reason we can't hide fields based on the task subtype.

To start with, allow subtypes to override "disabled" and "name" for custom fields.

Test Plan:
  - Defined several custom fields and several subtypes.
  - Disabled/renamed some fields for some subtypes.
  - Viewed/edited tasks of different subtypes, got desired field behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13248

Differential Revision: https://secure.phabricator.com/D20161
2019-02-15 19:17:57 -08:00
epriestley
4b10bc2b64 Correct schema irregularities (including weird keys) with worker task tables
Summary:
Ref T13253. Fixes T6615. See that task for discussion.

  - Remove three keys which serve no real purpose: `dataID` doesn't do anything for us, and the two `leaseOwner` keys are unused.
  - Rename `leaseOwner_2` to `key_owner`.
  - Fix an issue where `dataID` was nullable in the active table and non-nullable in the archive table.

In practice, //all// workers have data, so all workers have a `dataID`: if they didn't, we'd already fatal when trying to move tasks to the archive table. Just clean this up for consistency, and remove the ancient codepath which imagined tasks with no data.

Test Plan:
  - Ran `bin/storage upgrade`, inspected tables.
  - Ran `bin/phd debug taskmaster`, worked through a bunch of tasks with no problems.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13253, T6615

Differential Revision: https://secure.phabricator.com/D20175
2019-02-15 19:17:33 -08:00
epriestley
c5e16f9bd9 Give HarbormasterBuildUnitMessage a real Query class
Summary: Ref T13088. Prepares for putting test names in a separate table to release the 255-character limit.

Test Plan: Viewed revisions, buildables, builds, test lists, specific tests.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13088

Differential Revision: https://secure.phabricator.com/D20179
2019-02-15 19:16:47 -08:00
epriestley
8810cd2f4d Add a standalone view for the Maniphest task graph
Summary:
See PHI1073. Improve the UX here:

  - When there are a small number of connected tasks, no changes.
  - When there are too many total connected tasks, but not too many directly connected tasks, show hint text with a "View Standalone Graph" button to view more of the graph.
  - When there are too many directly connected tasks, show better hint text with a "View Standalone Graph" button.
  - Always show a "View Standalone Graph" option in the dropdown menu.
  - Add a standalone view which works the same way but has a limit of 2,000.
    - This view doesn't have "View Standalone Graph" links, since they'd just link back to the same page, but is basically the same otherwise.
  - Increase the main page task limit from 100 to 200.

Test Plan:
Mobile View:

{F6210326}

Way too much stuff:

{F6210327}

New persistent link to the standalone page:

{F6210328}

Kind of too much stuff:

{F6210329}

Standalone view:

{F6210330}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: 20after4

Differential Revision: https://secure.phabricator.com/D20164
2019-02-15 14:43:38 -08:00
epriestley
8f8e863613 When users follow an email login link but an install does not use passwords, try to get them to link an account
Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.

If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.

This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".

Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:

  - When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
  - This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.

So: step forward, but more work to be done.

Test Plan: {F6211115}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20170
2019-02-15 14:41:31 -08:00
epriestley
2ca316d652 When users confirm Duo MFA in the mobile app, live-update the UI
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.

Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20169
2019-02-15 14:38:15 -08:00
epriestley
454a762562 Queue search indexing tasks at a new PRIORITY_INDEX, not PRIORITY_IMPORT
Summary:
Depends on D20175. Ref T12425. Ref T13253. Currently, importing commits can stall search index rebuilds, since index rebuilds use an older priority from before T11677 and weren't really updated for D16585.

In general, we'd like to complete all indexing tasks before continuing repository imports. A possible exception is if you rebuild an entire index with `bin/search index --rebuild-the-world`, but we could queue those at a separate lower priority if issues arise.

Test Plan: Ran some search indexing through the queue.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13253, T12425

Differential Revision: https://secure.phabricator.com/D20177
2019-02-15 14:16:28 -08:00
epriestley
66060b294b Fix a URI construction in remarkup macro/meme rules
Summary: Ref T13250. Some of these parameters may be NULL, and `alter()` is no longer happy about that.

Test Plan: Ran daemon tasks that happened to render some memes.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20176
2019-02-15 14:08:46 -08:00
epriestley
b09cf166a8 Clean up a couple more URI alter() calls
Summary:
See <https://discourse.phabricator-community.org/t/create-new-phriction-document-fails-with-unhandled-exception-invalidargumentexception/2406>.

These weren't obviously nullable from a cursory `grep`, but are sometimes nullable in practice.

Test Plan: Created, then saved a new Phriction document.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20184
2019-02-15 14:07:17 -08:00
epriestley
c5772f51de Fix Content-Security-Policy headers on "Email Login" page
Summary:
In D20100, I changed this page from returning a `newPage()` with a dialog as its content to returning a more modern `newDialog()`.

However, the magic to add stuff to the CSP header is actually only on the `newPage()` pathway today, so this accidentally dropped the extra "Content-Security-Policy" rule for Google.

Lift the magic up one level so both Dialog and Page responses hit it.

Test Plan:
  - Configured Recaptcha.
  - Between D20100 and this patch: got a CSP error on the Email Login page.
  - After this patch: clicked all the pictures of cars / store fronts.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20163
2019-02-14 12:53:33 -08:00
epriestley
889eca1af9 Allow a DAO object storage namespace to be forced to a particular value
Summary:
Ref T6703. When we import external data from a third-party install to a Phacility instance, we must link instance accounts to central accounts: either existing central accounts, or newly created central accounts that we send invites for.

During this import, or when users register and claim those new accounts, we do a write from `admin.phacility.com` directly into the instance database to link the accounts.

This is pretty sketchy, and should almost certainly just be an internal API instead, particularly now that it's relatively stable.

However, it's what we use for now. The process has had some issues since the introduction of `%R` (combined database name and table refrence in queries), and now needs to be updated for the new `providerConfigPHID` column in `ExternalAccount`.

The problem is that `%R` isn't doing the right thing. We have code like this:

```
$conn = new_connection_to_instance('turtle');
queryf($conn, 'INSERT INTO %R ...', $table);
```

However, the `$table` resolves `%R` using the currently-executing-process information, not anything specific to `$conn`, so it prints `admin_user.user_externalaccount` (the name of the table on `admin.phacility.com`, where the code is running).

We want it to print `turtle_user.user_externalaccount` instead: the name of the table on `turtle.phacility.com`, where we're actually writing.

To force this to happen, let callers override the namespace part of the database name.

Long term: I'd plan to rip this out and replace it with an API call. This "connect directly to the database" stuff is nice for iterating on (only `admin` needs hotfixes) but very very sketchy for maintaining.

Test Plan: See next diff.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20167
2019-02-14 12:02:08 -08:00
epriestley
be21dd3b52 Fix some "URI->alter(X, null)" callsites
Summary:
Ref T13250. This internally calls `replaceQueryParam(X, null)` now, which fatals if the second parameter is `null`. I hit these legitimately, but I'll look for more callsites and follow up by either allowing this, removing `alter()`, fixing the callsites, or some combination.

(I'm not much of a fan of `alter()`.)

Test Plan: Browsing a paginated list no longer complains about URI construction.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20162
2019-02-14 11:59:07 -08:00
epriestley
5892c78986 Replace all "setQueryParam()" calls with "remove/replaceQueryParam()"
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.

Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20154
2019-02-14 11:56:39 -08:00
epriestley
241f06c9ff Clean up final setQueryParams() callsites
Summary: Ref T13250. See D20149.

Test Plan: All trivial?

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20153
2019-02-14 11:54:56 -08:00
epriestley
4c12420162 Replace "URI->setQueryParams()" after initialization with a constructor argument
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20151
2019-02-14 11:46:37 -08:00
epriestley
eb73cb68ff Raise a setup warning when locked configuration has a configuration value stored in the database
Summary:
Ref T13249. See <https://discourse.phabricator-community.org/t/configuring-the-number-of-taskmaster-daemons/2394/>.

Today, when a configuration value is "locked", we prevent //writes// to the database. However, we still perform reads. When you upgrade, we generally don't want a bunch of your configuration to change by surprise.

Some day, I'd like to stop reading locked configuration from the database. This would defuse an escalation where an attacker finds a way to write to locked configuration despite safeguards, e.g. through SQL injection or policy bypass. Today, they could write to `cluster.mailers` or similar and substantially escalate access. A better behavior would be to ignore database values for `cluster.mailers` and other locked config, so that these impermissible writes have no effect.

Doing this today would break a lot of installs, but we can warn them about it now and then make the change at a later date.

Test Plan:
  - Forced a `phd.taskmasters` config value into the database.
  - Saw setup warning.
  - Used `bin/config delete --database phd.taskmasters` to clear the warning.
  - Reviewed documentation changes.
  - Reviewed `phd.taskmasters` documentation adjustment.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20159
2019-02-13 12:27:48 -08:00
epriestley
88d5233b77 Fix specifications of some "Visual Only" elements
Summary: See PHI823. These got "visual-only" but should acutally get "aural => false" to pick up "aria-hidden".

Test Plan: Viewed page source, saw both "visual-only" and "aria-hidden".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20157
2019-02-13 12:26:28 -08:00
epriestley
9a9fa8bed2 Rate limit attempts to add payment methods in Phortune
Summary: Ref T13249. See D20132. Although we're probably a poor way to validate a big list of stolen cards in practice in production today (it's very hard to quickly generate a large number of small charges), putting rate limiting on "Add Payment Method" is generally reasonable, can't really hurt anything (no legitimate user will ever hit this limit), and might frustrate attackers in the future if it becomes easier to generate ad-hoc charges (for example, if we run a deal on support pacts and reduce their cost from $1,000 to $1).

Test Plan: Reduced limit to 4 / hour, tried to add a card several times, got rate limited.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13249

Differential Revision: https://secure.phabricator.com/D20158
2019-02-13 12:25:56 -08:00
epriestley
991368128e Bump the markup cache version for URI changes
Summary:
Ref T13250. Ref T13249. Some remarkup rules, including `{image ...}` and `{meme ...}`, may cache URIs as objects because the remarkup cache is `serialize()`-based.

URI objects with `query` cached as a key-value map are no longer valid and can raise `__toString()` fatals.

Bump the cache version to purge them out of the cache.

Test Plan: See PHI1074.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250, T13249

Differential Revision: https://secure.phabricator.com/D20160
2019-02-13 12:25:18 -08:00
epriestley
a7bf279fd5 Don't try to publish build results to bare diffs
Summary:
See <https://discourse.phabricator-community.org/t/conduit-call-is-generating-phd-log-error-message/2380/>. If you run builds against a diff which is not attached to a revision (this is unusual) we still try to publish to the associated revision. This won't work since there is no associated revision.

Since bare diffs don't really have a timeline, just publish nowhere for now.

Test Plan:
  - Created a diff.
  - Did not attach it to a revision.
  - Created a build plan with "make http request + wait for response".
  - Manually ran the build plan against the bare diff.
  - Used `bin/phd debug task` to run the build and hit a "revision not attached" exception during publishing.
  - Applied patch.
  - Ran `bin/phd debug task`, got clean (no-op) publish.
  - Sent build a failure message with "harbormaster.sendmessage", got a failed build.

This isn't a real workflow, but shouldn't fail.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20156
2019-02-13 12:19:29 -08:00
epriestley
7d6d2c128a Make "bin/audit delete" synchronize commit audit status, and improve "bin/audit synchronize" documentation
Summary:
Depends on D20126. See PHI1056. Ref T13244.

  - `bin/audit delete` destroys audit requests, but does not update the overall audit state for associated commits. For example, if you destroy all audit requests for a commit, it does not move to "No Audit Required".
  - `bin/audit synchronize` does this synchronize step, but is poorly documented.

Make `bin/audit delete` synchronize affected commits.

Document `bin/audit synchronize` better.

There's some reasonable argument that `bin/audit synchronize` perhaps shouldn't exist, but it does let you recover from an accidentally (or intentionally) mangled database state. For now, let it live.

Test Plan:
  - Ran `bin/audit delete`, saw audits destroyed and affected commits synchornized.
  - Ran `bin/audit synchronize`, saw behavior unchanged.
  - Ran `bin/audit help`, got better help.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20127
2019-02-13 05:50:14 -08:00
epriestley
5a89da12e2 When users have no password on their account, guide them through the "reset password" flow in the guise of "set password"
Summary:
Depends on D20119. Fixes T9512. When you don't have a password on your account, the "Password" panel in Settings is non-obviously useless: you can't provide an old password, so you can't change your password.

The correct remedy is to "Forgot password?" and go through the password reset flow. However, we don't guide you to this and it isn't really self-evident.

Instead:

  - Guide users to the password reset flow.
  - Make it work when you're already logged in.
  - Skin it as a "set password" flow.

We're still requiring you to prove you own the email associated with your account. This is a pretty weak requirement, but maybe stops attackers who use the computer at the library after you do in some bizarre emergency and forget to log out? It would probably be fine to just let users "set password", this mostly just keeps us from having two different pieces of code responsible for setting passwords.

Test Plan:
  - Set password as a logged-in user.
  - Reset password on the normal flow as a logged-out user.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: revi

Maniphest Tasks: T9512

Differential Revision: https://secure.phabricator.com/D20120
2019-02-12 15:19:46 -08:00
epriestley
3f35c0068a Allow users to register with non-registration providers if they are invited to an instance
Summary:
Depends on D20117. Fixes T10071. When you're sent an email invitation, it's intended to allow you to register an account even if you otherwise could not (see D11737).

Some time between D11737 and today, this stopped working (or perhaps it never worked and I got things wrong in D11737). I think this actually ended up not mattering for us, given the way Phacility auth was ultimately built.

This feature generally seems reasonable, though, and probably //should// work. Make it work in the "password" and "oauth" cases, at least. This may //still// not work for LDAP, but testing that is nontrivial.

Test Plan:
  - Enabled only passwords, turned off registration, sent an invite, registered with a password.
  - Enabled only Google OAuth, turned off registration, sent an invite, registered with Google OAuth.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10071

Differential Revision: https://secure.phabricator.com/D20118
2019-02-12 15:19:03 -08:00
epriestley
d22495a820 Make external link/refresh use provider IDs, switch external account MFA to one-shot
Summary:
Depends on D20113. Ref T6703. Continue moving toward a future where multiple copies of a given type of provider may exist.

Switch MFA from session-MFA at the start to one-shot MFA at the actual link action.

Add one-shot MFA to the unlink action. This theoretically prevents an attacker from unlinking an account while you're getting coffee, registering `alIce` which they control, adding a copy of your profile picture, and then trying to trick you into writing a private note with your personal secrets or something.

Test Plan: Linked and unlinked accounts. Refreshed account. Unlinked, then registered a new account. Unlinked, then relinked to my old account.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20117
2019-02-12 15:18:08 -08:00
epriestley
e5ee656fff Make external account unlinking use account IDs, not "providerType + providerDomain" nonsense
Summary: Depends on D20112. Ref T6703. When you go to unlink an account, unlink it by ID. Crazy!

Test Plan: Unlinked and relinked Google accounts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20113
2019-02-12 15:16:24 -08:00
epriestley
541d794c13 Give ExternalAccount a providerConfigPHID, tying it to a particular provider
Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.

Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.

(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)

Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20112
2019-02-12 14:48:14 -08:00
epriestley
55c18bc900 During first-time setup, create an administrator account with no authentication instead of weird, detached authentication
Summary:
Ref T6703. Currently, when you create an account on a new install, we prompt you to select a password.

You can't actually use that password unless you set up a password provider, and that password can't be associated with a provider since a password provider won't exist yet.

Instead, just don't ask for a password: create an account with a username and an email address only. Setup guidance points you toward Auth.

If you lose the session, you can send yourself an email link (if email works yet) or `bin/auth recover` it. This isn't really much different than the pre-change behavior, since you can't use the password you set anyway until you configure password auth.

This also makes fixing T9512 more important, which I'll do in a followup. I also plan to add slightly better guideposts toward Auth.

Test Plan: Hit first-time setup, created an account.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: revi

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20111
2019-02-12 14:47:47 -08:00
epriestley
378a43d09c Remove the highly suspect "Import from LDAP" workflow
Summary: Depends on D20109. Ref T6703. This flow was contributed in 2012 and I'm not sure it ever worked, or at least ever worked nondestructively. For now, get rid of it. We'll do importing and external sync properly at some point (T3980, T13190).

Test Plan: Grepped for `ldap/`, grepped for controller.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20110
2019-02-12 14:45:58 -08:00
epriestley
fcd85b6d7b Replace "getRequestURI()->setQueryParams(array())" with "getPath()"
Summary:
Ref T13250. A handful of callsites are doing `getRequestURI()` + `setQueryParams(array())` to get a bare request path.

They can just use `getPath()` instead.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20150
2019-02-12 14:43:33 -08:00
epriestley
00ffb190cc In Webhooks, label HTTP response codes as "HTTP Status Code", not "HTTP Error"
Summary: See PHI1068. We currently show "HTTP Error - 200", which is misleading. Instead, label these results as "HTTP Status Code".

Test Plan: {F6206016}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20148
2019-02-12 14:41:10 -08:00
epriestley
308c4f2407 Fix "AphrontRequest->getRequestURI()" for requests with "x[]=1" parameters in the URI
Summary:
Ref T13250. See PHI1069. This is a small fix for `getRequestURI()` currently not working if the request includes "x[]=..." PHP-flavored array parameters, beacause they're parsed into arrays by `$_GET` and `setQueryParams(...)` no longer accepts nonscalars.

Instead, just parse the raw request URI.

Test Plan: Visited `/search/hovercard/?phids[]=X`, no more fatal. Dumped the resulting URI, saw it had the right value. Tried `?phids[]=x&x=1&x=1&x=1`, saw the parameters correctly preserved.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20147
2019-02-12 11:03:43 -08:00
epriestley
1fd69f788c Replace "getQueryParams()" callsites in Phabricator
Summary: See D20136. This method is sort of inherently bad because it is destructive for some inputs (`x=1&x=2`) and had "PHP-flavored" behavior for other inputs (`x[]=1&x[]=2`). Move to explicit `...AsMap` and `...AsPairList` methods.

Test Plan: Bit of an adventure, see inlines in a minute.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20141
2019-02-12 06:37:03 -08:00
epriestley
187356fea5 Let the top-level exception handler dump a stack trace if we reach debug mode before things go sideways
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.

We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.

There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.

Test Plan: {F6205122}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20142
2019-02-11 15:36:19 -08:00
epriestley
51cca22d07 Support EU domains for Mailgun API
Summary:
See <https://discourse.phabricator-community.org/t/mailgun-eu-zone-not-working/2332/4>.

Mailgun has a couple of API domains depending on where your account is based.

Test Plan:
  - Sent normal mail successfully with my non-EU account.
  - Waiting on user confirmation that this works in the EU.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20055
2019-02-11 15:06:04 -08:00
epriestley
b9a1260ef5 Improve top-level fatal exception handling in PHP 7+
Summary:
Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to `Throwables`, a superclass of `Exception`.

This is generally a good change, but code written against PHP 5.x before `Throwable` was introduced may not catch these errors, even when the code is intended to be a top-level exception handler.

(The double-catch pattern here and elsewhere is because `Throwable` does not exist in older PHP, so `catch (Throwable $ex)` catches nothing. The `Exception $ex` clause catches everything in old PHP, the `Throwable $ex` clause catches everything in newer PHP.)

Generalize some `Exception` into `Throwable`.

Test Plan:
  - Added a bogus function call to the rendering stack.
  - Before change: got a blank page.
  - After change: nice exception page.

{F6205012}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250, T12101

Differential Revision: https://secure.phabricator.com/D20138
2019-02-11 15:05:15 -08:00
epriestley
e03079aaaa Try harder to present display/rendering exceptions to the user using standard exception handling
Summary:
Ref T13250. When exceptions occur in display/rendering/writing, they currently go straight to the fallback handler. This is a minimal handler which doesn't show a stack trace or include any debugging details.

In some cases, we have to do this: some of these exceptions prevent us from building a normal page. For example, if the menu bar has a hard fatal in it, we aren't going to be able to build a nice exception page with a menu bar no matter how hard we try.

However, in many cases the error is mundane: something detected something invalid and raised an exception during rendering. In these cases there's no problem with the page chrome or the rendering pathway itself, just with rendering the page data.

When we get a rendering/response exception, try a second time to build a nice normal exception page. This will often work. If it doesn't work, fall back as before.

Test Plan:
  - Forced the error from T13250 by applying D20136 but not D20134.
  - Before:

{F6205001}

  - After:

{F6205002}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20137
2019-02-11 14:40:52 -08:00