Summary:
Depends on D20197. Ref T13161. We currently try to build a "ScopeEngine" even for diffs with no context (e.g., `git diff` instead of `git diff -U9999`).
Since we don't have any context, we won't really be able to figure out anything useful about scopes. Also, since ScopeEngine is pretty strict about what it accepts, we crash.
In these cases, just don't build a ScopeEngine.
Test Plan: Viewed a diff I copy/pasted with `git diff` instead of an `arc diff` / `git diff -U99999`, got a sensible diff with no context instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20198
Summary:
Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool:
{F6225179}
If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd:
{F6225181}
Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff:
{F6225183}
Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20197
Summary:
Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191.
I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case.
Test Plan: Grepped for behavior name, selected stuff on both sides of a diff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20196
Summary:
Ref T13253. Long ago, daemon logs were visible in the web UI. They were removed because access to logs generally does not conform to policy rules, and may leak the existence (and sometimes contents) of hidden objects, occasionally leak credentials in certain error messages, etc.
These bits and pieces were missed.
Test Plan: Grepped for removed symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253
Differential Revision: https://secure.phabricator.com/D20199
Summary:
Ref T13161.
- Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed".
- The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway.
Test Plan: Got slightly better rendering for some diffs locally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161
Differential Revision: https://secure.phabricator.com/D20195
Summary:
Ref T12822. Ref T2495. This is the good version of D20193.
Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible.
In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint.
That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep.
At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want.
In particular, we can render any character `X` as `<span data-copy-text="Y">X</span>`, and then copy "Y" instead of "X" when the user copies the node. Limitations:
- If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it?
- If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine.
- We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right.
With those tools in hand:
- Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display.
- Replace ZWS and NBSP (raw text) with a special marker for display.
- Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose.
Test Plan:
- Generated and viewed a file like this one:
{F6220422}
- Copied text out of it, got authentic raw original source text instead of displayed text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822, T2495
Differential Revision: https://secure.phabricator.com/D20194
Summary:
Ref T12822. Fixes a few things:
- Firefox selection of weird ranges with an inline between the start and end of the range now works correctly.
- "Show More Context" rows now render, highlight, and select properly.
- Prepares for nodes to have copy-text which is different from display-text.
- Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive.
Test Plan:
- Selected and copied weird ranges in Firefox.
- Kept an eye on "Show More Context" rows across select and copy operations.
- Generally poked around in Safari/Firefox/Chrome.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20192
Summary:
Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy.
As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers.
We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want.
In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway.
In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇".
Finally, we have to figure out which side of the diff we're copying from. The rule here is:
- If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying.
- Otherwise, use normal document copy rules.
So the overall flow here is:
- Listen for clicks.
- When the user clicks the left or right side of the diff, store what they clicked.
- When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy.
- (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.)
- When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode.
- When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today.
Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20191
Summary:
Ref T12822. The next change hits these warnings but I think neither is a net positive.
The "function called before it is defined" error alerts on this kind of thing:
```
function a() {
b();
}
function b() {
}
a();
```
Here, `b()` is called before it is defined. This code, as written, is completely safe. Although it's possible that this kind of construct may be unsafe, I think the number of programs where there's unsafe behavior here AND the whole thing doesn't immediately break when you run it at all is very very small.
Complying with this warning is sometimes impossible -- at least without cheating/restructuring/abuse -- for example, if you have two functions which are mutually recursive.
Although compliance is usually possible, it forces you to define all your small utility functions at the top of a behavior. This isn't always the most logical or comprehensible order.
I think we also have some older code which did `var a = function() { ... }` to escape this, which I think is just silly/confusing.
Bascially, this is almost always a false positive and I think it makes the code worse more often than it makes it better.
---
The "unused function parameter" error warns about this:
```
function onevent(e) {
do_something();
```
We aren't using `e`, so this warning is correct. However, when the function is a callback (as here), I think it's generally good hygiene to include the callback parameters in the signature (`onresponse(response)`, `onevent(event)`, etc), even if you aren't using any/all of them. This is a useful hint to future editors that the function is a callback.
Although this //can// catch mistakes, I think this is also a situation where the number of cases where it catches a mistake and even the most cursory execution of the code doesn't catch the mistake is vanishingly small.
Test Plan: Egregiously violated both rules in the next diff. Before change: complaints. After change: no complaints.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12822
Differential Revision: https://secure.phabricator.com/D20190
Summary:
Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential.
After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely.
This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet.
Test Plan:
- Grepped for `zws`, `grep -i zero | grep -i width`.
- Copied text out of Differential: got both sides of the diff (not ideal).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13161, T12822
Differential Revision: https://secure.phabricator.com/D20189
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
