Summary:
Ref T13249.
- When a line has only increased in indent depth, don't red-fill highlight the left side of the diff. Since reading a diff //mostly// involves focusing on the right side, indent depth changes are generally visible enough without this extra hint. The extra hint can become distracting in cases where there is a large block of indent depth changes.
- Move the markers slightly to the left, to align them with the gutter.
- Make them slightly opaque so they're a little less prominent.
Test Plan: See screenshots.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20251
Summary:
Ref T13258. The general idea here is "if arc land prompted you and you hit 'y', you get a warning about it on the timeline".
This is similar to the existing warning about landing revisions in the wrong state and hitting "y" to get through that. See D18808, previously.
These warnings make it easier to catch process issues at a glance, especially because the overall build status is now more complicated (and may legally include some failures on tests which are marked as unimportant).
The transaction stores which builds had problems, but I'm not doing anything to render that for now. I think you can usually figure it out from the UI already; if not, we could refine this.
Test Plan:
- Used `bin/differential attach-commit` to trigger extraction/attachment.
- Attached a commit to a revision with various build states, and various build plan "Warn When Landing" flags.
- Got sensible warnings and non-warnings based on "Warn When Landing" setting.
{F6251631}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20239
Summary: Ref T13258. Make the "Affects Buildable" option actually work.
Test Plan:
- As in previous change, created a "wait for HTTP request" build plan and had it always run against every revision.
- Created revisions, waited a bit, then sent the build a "Fail" message, with different values of "Affects Buildable":
- "Always": Same behavior as today. Buildable waited for the build, then failed when it failed.
- "While Building": Buildable waited for the build, but passed even though it failed (buildable has green checkmark even though build is red):
{F6250359}
- "Never": Buildable passed immediately (buildable has green checkmark even though build is still running):
{F6250360}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20233
Summary: Ref T13258. Makes the new "Hold Drafts" behavior actually work.
Test Plan:
- Created a build plan which does "Make HTTP Request" somewhere random and then waits for a message.
- Created a Herald rule which "Always" runs this plan.
- Created revisions, loaded them, then sent their build targets a "fail" message a short time later.
- With "Always": Current behavior. Revision was held as a draft while building, and returned to me for changes when the build failed.
- With "If Building": Revision was held as a draft while building, but promoted once the build failed.
- With "Never": Revision promoted immediately, ignoring the build completely.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20232
Summary:
Ref T13258. Implements the "Restartable" behavior, to control whether a build may be restarted or not.
This is fairly straightforward because there are already other existing reasons that a build may not be able to be restarted.
Test Plan: Restarted a build. Marked it as not restartable, saw "Restart" action become disabled. Tried to restart it anyway, got a useful error message.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20230
Summary:
Ref T13258. Fixes T11415. This makes "Runnable" actually do something:
- With "Runnable" set to "If Editable" (default): to manually run, pause, resume, abort, or restart a build, you must normally be able to edit the associated build plan.
- If you toggle "Runnable" to "If Viewable", anyone who can view the build plan may take these actions.
This is pretty straightforward since T9614 already got us pretty close to this ruleset a while ago.
Test Plan:
- Created a Build Plan, set "Can Edit" to just me, toggled "Runnable" to "If Viewable"/"If Editable", tried to take actions as another user.
- With "If Editable", unable to run, pause, resume, abort, or restart as another user.
- With "If Viewable", those actions work.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T11415
Differential Revision: https://secure.phabricator.com/D20229
Summary: Ref T13258. This will support changing behaviors in "arc land".
Test Plan: Called "harbormaster.buildplan.search", saw behavior information in results.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20228
Summary:
Depends on D20219. Ref T13258. Ref T11415. Installs sometimes have long-running builds or unimportant builds which they may not want to hold up drafts, affect buildable status, or warn during `arc land`.
Some builds have side effects (like deployment or merging) and are not idempotent. They can cause problems if restarted.
In other cases, builds are isolated and idempotent and generally safe, and it's okay for marketing interns to restart them.
To address these cases, add "Behaviors" to Build Plans:
- Hold Drafts: Controls how the build affects revision promotion from "Draft".
- Warn on Land: Controls the "arc land" warning.
- Affects Buildable: Controls whether we care about this build when figuring out if a buildable passed or failed overall.
- Restartable: Controls whether this build may restart or not.
- Runnable: Allows you to weaken the requirements to run the build if you're confident it's safe to run it on arbitrary old versions of things.
NOTE: This only implements UI, none of these options actually do anything yet.
Test Plan:
Mostly poked around the UI. I'll actually implement these behaviors next, and vet them more thoroughly.
{F6244828}
{F6244830}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T11415
Differential Revision: https://secure.phabricator.com/D20220
Summary:
Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer.
If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response".
Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do.
Test Plan:
- Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before).
- Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before).
- Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20235
Summary: See PHI1112. See T784. Although some more general/flexible solution is arriving eventually, adding this rule seems reasonable for now, since it's not a big deal if we remove it later to replace this with some fancier system.
Test Plan: Created a diff with the official Go generated marker, saw the changeset marked as generated.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20237
Summary:
Ref T13259. If we miss the separate CSRF step in Duo and proceed directly to prompting, we may fail to build a response which turns into a real control and fatal on `null->setLabel()`.
Instead, let MFA providers customize their "bare prompt dialog" response, then make Duo use the same "you have an outstanding request" response for the CSRF and no-CSRF workflows.
Test Plan: Hit Duo auth on a non-CSRF workflow (e.g., edit an MFA provider with Duo enabled). Previously: `setLabel()` fatal. After patch: smooth sailing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20234
Summary:
See PHI1063. See PHI1114. Ref T13253. Currently, you can't `bin/worker execute` an archived task and can't `bin/worker retry` a successful task.
Although it's good not to do these things by default (particularly, retrying a successful task will double its effects), there are plenty of cases where you want to re-run something for testing/development/debugging and don't care that the effect will repeat (you're in a dev environment, the effect doesn't matter, etc).
Test Plan: Ran `bin/worker execute/retry` against archived/successful tasks. Got prompted to add more flags, then got re-execution.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13253
Differential Revision: https://secure.phabricator.com/D20246
Summary:
Ref T13121. When you connect to a host with SSH, don't already know the host key, and don't have strict host key checking, it prints "Permanently adding host X to known hosts". This is super un-useful.
In a perfect world, we'd probably always have strict host key checking, but this is a significant barrier to configuration/setup and I think not hugely important (MITM attacks against SSH hosts are hard/rare and probably not hugely valuable). I'd imagine a more realistic long term approach is likely optional host key checking.
For now, try using `LogLevel=ERROR` instead of `LogLevel=quiet` to suppress this error. This should be strictly better (since at least some messages we want to see are ERROR or better), although it may not be perfect (there may be other INFO messages we would still like to see).
Test Plan:
- Ran `ssh -o LogLevel=... -o 'StrictHostKeyChecking=no' -o 'UserKnownHostsFile=/dev/null'` with bad credentials, for "ERROR", "quiet", and default ("INFO") log levels.
- With `INFO`, got a warning about adding the key, then an error about bad credentials (bad: don't want the key warning).
- With `quiet`, got nothing (bad: we want the credential error).
- With `ERROR`, got no warning but did get an error (good!).
Not sure this always gives us exactly what we want, but it seems like an improvement over "quiet".
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13121
Differential Revision: https://secure.phabricator.com/D20240
Summary:
Fixes T13260. "QUERY_STRING" and "REQUEST_URI" are similar for our purposes here, but our nginx documentation tells you to pass "QUERY_STRING" and doesn't tell you to pass "REQUEST_URI". We also use "QUERY_STRING" in a couple of other places already, and already have a setup check for it.
Use "QUERY_STRING" instead of "REQUEST_URI".
Test Plan: Visited `/oauth/google/?a=b`, got redirected with parameters preserved.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13260
Differential Revision: https://secure.phabricator.com/D20227
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-muting-completed-bulk-jobs/2449>. Bulk Jobs have an "edge" table but currently do not support edge transactions. Add support.
This stops "Mute Notifications" from fataling.
The action probably doesn't do what the reporting user expects (it stops edits to the job object from sending notifications; it does not stop the edits the job performs from sending notifications) but I think this change puts us in a better place no matter what, even if we eventually clarify or remove this behavior.
Test Plan: Clicked "Mute Notifications" on a bulk job, got an effect instead of a fatal.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20226
Summary:
See <https://discourse.phabricator-community.org/t/traceback-rendering-task-query-in-dashboard/2450/>.
It looks like this blames to D19126, which added some more complex constraint logic but overlooked "range" constraints, which are handled separately.
Test Plan:
- Added a custom "date" field to Maniphest with `"search": true`.
- Executed a range query against the field.
Then:
- Before: Warnings about undefined indexes in the log.
- After: No such warnings.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20225
Summary:
See PHI1059. If you close a task, we apply an "alice closed a subtask: X" transaction to its parents.
This transaction is purely informative, but currently requires `CAN_EDIT` permission after T13186. However, we'd prefer to post this transaction anyway, even if: the parent is locked; or the parent is not editable by the acting user.
Replace the implicit `CAN_EDIT` requirement with no requirement.
(This transaction is only applied internally (by closing a subtask) and can't be applied via the API or any other channel, so this doesn't let attackers spam a bunch of bogus subtask closures all over the place or anything.)
Test Plan:
- Created a parent task A with subtask B.
- Put task A into an "Edits Locked" status.
- As a user other than the owner of A, closed B.
Then:
- Before: Policy exception when trying to apply the "alice closed a subtask: B" transaction to A.
- After: B closed, A got a transaction despite being locked.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20223
Summary:
Depends on D20218. Ref T13258. It's somewhat cumbersome to get from build plans to related builds but this is a reasonable thing to want to do, so make it a little easier.
Also clean up / standardize / hint a few things a little better.
Test Plan: {F6244116}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20219
Summary: Depends on D20217. Ref T13258. Mostly for completeness. You can't edit build steps so this may not be terribly useful, but you can do bulk policy edits or whatever?
Test Plan: Edited a build plan via API.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20218
Summary: Depends on D20216. Ref T13258. Bland infrastructure update to prepare for bigger things.
Test Plan: Created and edited a build plan.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20217
Summary: Ref T13088. Build Plans currently have a "Created" date in the right-hand "Curtain" UI, but this is unusual and the creation date is evident from the timeline. It's also not obvious why anyone would care. Remove it for simplicity/consistency. I think this may have just been a placeholder during initial implementation.
Test Plan: Viewed a build plan, no more "Created" element.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D20216
Summary:
See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.
Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.
Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20214
Summary: Depends on D20209. Ref T13255. It would probably be nice to make this into a "real" `*.search` API method some day, but at least document the features for now.
Test Plan: Read documentation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20211
Summary:
Depends on D20208. Ref T13255. See that task for some long-winded discussion and rationale. Short version:
- This is a list of operations instead of a list of old/new PHIDs because of scalability issues for large lists (T13056).
- This is a fairly verbose list (instead of, for example, the more concise internal map we sometimes use with "+" and "-" as keys) to try to make the structure obvious and extensible in the future.
- The "add" and "remove" echo the `*.edit` operations.
Test Plan: Called `transaction.search` on an object with project tag changes, saw them in the results.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20209
Summary: Ref T13255. The "transaction.search" API method currently does not support author constraints, but this is a reasonable thing to support.
Test Plan: Queried transactions by author, hit the error cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13255
Differential Revision: https://secure.phabricator.com/D20208
Summary: Fixes T13254. See that task for details.
Test Plan: Used iOS Simulator to do a login locally, didn't get blocked. Verified CSP includes "m.facebook.com".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13254
Differential Revision: https://secure.phabricator.com/D20206
Summary: Ref T13251. See <https://phabricator.wikimedia.org/T216849>.
Test Plan:
- With more than 100 projects (or, set `$limit = 3`)...
- Edit a task, then click the "Browse" magnifying glass icon next to the "Tags" typeahead.
- Before change: fatal on 'q' being null.
- After change: no fatal.
Reviewers: amckinley, 20after4
Reviewed By: amckinley
Maniphest Tasks: T13251
Differential Revision: https://secure.phabricator.com/D20204
Summary:
Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running.
Also displays this information in the daemon status console; see screenshot: {F6225726}
Test Plan:
Stopped daemons, ran `bin/search index --all --background` to create lots of tasks, restarted daemons, observed expected values for `dateCreated` and `epochArchived` in the archive worker table.
Also tested the changes to `unarchiveTask` by forcing a search task to permanently fail and then `bin/worker retry`ing it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T5401
Differential Revision: https://secure.phabricator.com/D20200
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 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
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
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
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
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
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
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
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
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
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
Summary:
Depends on D20138. Ref T13250. This improves exception behavior and gives us a standard page with a stack trace instead of a text fatal with no stack trace.
Truly a great day for PHP.
(Eventually we may want to replace all `(string)` with `phutil_string_cast()`, but let's let it have some time in the wild first?)
Test Plan: Triggered the error, got a more useful exception behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20140
Summary: See D20126. I was trying to be a little too cute here with the names and ended up confusing myself, then just tested the method behavior. :/
Test Plan: Persudaded by arguments in D20126.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20135
Summary:
Ref T13250. Currently, datasources have a `setParameters(...)` method. This method accepts a dictionary and adds the key/value pairs to the raw HTTP request to the datasource endpoint.
Since D20049, this no longer works. Since D20116, it fatals explicitly.
In general, the datasource endpoint accepts other values (like `query`, `offset`, and `limit`), and even before these changes, using secret reserved keys in `setParameters(...)` would silently cause program misbehavior.
To deal with this, pass parameters as a JSON string named "parameters". This fixes the HTTP query issue (the more pressing issue affecting users today) and prevents the "shadowing reserved keys" issue (a theoretical issue which might affect users some day).
(I may revisit the `phutil_build_http_querystring()` behavior and possibly let it make this work again, but I think avoiding the duplicate key issue makes this change desirable even if the querystring behavior changes.)
Test Plan:
- Used "Land Revision", selected branches.
- Configured a custom Maniphest "users" field, used the search typeahead, selected users.
- Manually browsed to `/typeahead/class/PhabricatorPeopleDatasource/?query=hi¶meters=xyz` to see the JSON decode exception.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20134
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.
Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.
Also, make the "MFA" and "Silent" emblems more easily visible.
Test Plan:
Edited a locked task, overrode the lock, got marked for it.
{F6195005}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20131
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.
Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):
{F6197394}
After:
{F6197397}
- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20132
Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.
If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.
Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20130
Summary:
Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.
(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)
Test Plan:
- Edited a package to select the various different audit rules.
- Used `bin/repository reparse --force --owners <commit>` to trigger package audits under varied conditions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20126
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.
PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.
Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.
Test Plan:
- Created several packages, some with and some without auditing.
- Inspected database for: package state; and associated transactions.
- Ran the migrations.
- Inspected database to confirm that state and transactions migrated correctly.
- Reviewed transaction logs.
- Created and edited packages and audit state.
- Viewed the "Package List" element in Diffusion.
- Pulled package information with `owners.search`, got sensible results.
- Edited package audit status with `owners.edit`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20124
Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.
It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.
Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.
(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)
Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20129
Summary:
Depends on D20118. Fixes T5351. We possibly raise some warnings about registration (approval queue, email domains), but they aren't relevant if no one can register.
Hide these warnings if no providers actually support registration.
Test Plan: Viewed the Auth provider list with registration providers and with no registration providers, saw more tailored guidance.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5351
Differential Revision: https://secure.phabricator.com/D20119
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.
Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.
Test Plan: {F6193742}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8029
Differential Revision: https://secure.phabricator.com/D20123
Summary: Depends on D20120. Fixes T8907. I thought this needed some Javascript nonsense but Safari, Firefox and Chrome all support an `autofocus` attribute.
Test Plan: Loaded login page with password auth enabled in Safari, Firefox, and Chrome; saw username field automatically gain focus.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8907
Differential Revision: https://secure.phabricator.com/D20122
Summary: Depends on D20108. Ref T6703. Update this outdated callsite to a more modern appraoch.
Test Plan: Destroyed a user with `bin/remove destroy`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20109
Summary:
Depends on D20107. Ref T6703. Legalpad currently inserts "email" records into the external account table, but they're never used for anything and nothing else references them.
They also aren't necessary for anything important to work, and the only effect they have is making the UI say "External Account" instead of "None" under the "Account" column. In particular, the signatures still record the actual email address.
Stop doing this, remove all the references, and destroy all the rows.
(Long ago, Maniphest may also have done this, but no longer does. Nuance/Gatekeeper use a more modern and more suitable "ExternalObject" thing that I initially started adapting here before realizing that Legalpad doesn't actually care about this data.)
Test Plan: Signed documents with an email address, saw signature reflected properly in UI. Grepped for other callsites.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20108
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary: Depends on D20114. This is on the way out, so make that explicitly clear.
Test Plan: Read setup issue and configuration option.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20115
Summary:
See <https://hackerone.com/reports/492525> and <https://hackerone.com/reports/489531>. I previously awarded a bounty for <https://hackerone.com/reports/434116> so Slowvote is getting "researched" a lot.
- Prevent users from undoing their vote by submitting the form with nothing selected.
- Prevent users from racing between the `delete()` and `save()` to vote for multiple options in a plurality poll.
Test Plan:
- Clicked the vote button with nothing selected in plurality and approval polls, got an error now.
- Added a `sleep(5)` between `delete()` and `save()`. Submitted different plurality votes in different windows. Before: votes raced, invalid end state. After: votes waited on the lock, arrived in a valid end state.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20125
Summary: Ref D20122. This is something I wanted in a bunch of places. Looks like at some point the most-annoying one (autofocus for entering TOTOP codes) already got fixed at some point.
Test Plan: Loaded the form, got autofocus as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20128
Summary:
Depends on D20106. Ref T6703. Since I plan to change the `ExternalAccount` table, these migrations (which rely on `save()`) will stop working.
They could be rewritten to use raw queries, but I suspect few or no installs are affected. At least for now, just make them safe: if they would affect data, fatal and tell the user to perform a more gradual upgrade.
Also remove an `ALTER IGNORE TABLE` (this syntax was removed at some point) and fix a `%Q` when adjusting certain types of primary keys.
Test Plan: Ran `bin/storage upgrade --no-quickstart --force --namespace test1234` to get a complete migration since the beginning of time.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20107
Summary:
Depends on D20105. Fixes T7732. T7732 describes a case where a user had their Google credentials swapped and had trouble regaining access to their account.
Since we now allow email login even if password auth is disabled, it's okay to let users unlink their final account, and it's even reasonable for users to unlink their final account if it is mis-linked.
Just give them a warning that what they're doing is a little sketchy, rather than preventing the workflow.
Test Plan: Unlinked my only login account, got a stern warning instead of a dead end.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7732
Differential Revision: https://secure.phabricator.com/D20106
Summary:
Ref T6703. Replaces the small "link" icon with a more obvious "Link External Account" button.
Moves us toward operating against `$config` objects instead of against `$provider` objects, which is more modern and will some day allow us to resolve T6703.
Test Plan: Viewed page, saw a more obvious button. Linked an external account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20105
Summary: This option no longer needs to be configured if you configure inbound mail (and that's the easiest setup approach in a lot of cases), so stop telling users they have to set it up.
Test Plan: Read documentation and configuration help.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20104
Summary:
Depends on D20100. Ref T7732. Ref T13244. This is a bit of an adventure.
Long ago, passwords were digested with usernames as part of the salt. This was a mistake: it meant that your password becomes invalid if your username is changed.
(I think very very long ago, some other hashing may also have used usernames -- perhaps session hashing or CSRF hashing?)
To work around this, the "username change" email included a one-time login link and some language about resetting your password.
This flaw was fixed when passwords were moved to shared infrastructure (they're now salted more cleanly on a per-digest basis), and since D18908 (about a year ago) we've transparently upgraded password digests on use.
Although it's still technically possible that a username change could invalidate your password, it requires:
- You set the password on a version of Phabricator earlier than ~2018 Week 5 (about a year ago).
- You haven't logged into a version of Phabricator newer than that using your password since then.
- Your username is changed.
This probably affects more than zero users, but I suspect not //many// more than zero. These users can always use "Forgot password?" to recover account access.
Since the value of this is almost certainly very near zero now and declining over time, just get rid of it. Also move the actual mail out of `PhabricatorUser`, ala the similar recent change to welcome mail in D19989.
Test Plan: Changed a user's username, reviewed resulting mail with `bin/mail show-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244, T7732
Differential Revision: https://secure.phabricator.com/D20102
Summary:
Depends on D20099. Ref T13244. See PHI774. When password auth is enabled, we support a standard email-based account recovery mechanism with "Forgot password?".
When password auth is not enabled, we disable the self-serve version of this mechanism. You can still get email account login links via "Send Welcome Mail" or "bin/auth recover".
There's no real technical, product, or security reason not to let everyone do email login all the time. On the technical front, these links already work and are used in other contexts. On the product front, we just need to tweak a couple of strings.
On the security front, there's some argument that this mechanism provides more overall surface area for an attacker, but if we find that argument compelling we should probably provide a way to disable the self-serve pathway in all cases, rather than coupling it to which providers are enabled.
Also, inch toward having things iterate over configurations (saved database objects) instead of providers (abstract implementations) so we can some day live in a world where we support multiple configurations of the same provider type (T6703).
Test Plan:
- With password auth enabled, reset password.
- Without password auth enabled, did an email login recovery.
{F6184910}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20100
Summary: Depends on D20096. Reverts D14057. This was added for Phacility use cases in D14057 but never used. It is obsoleted by {nav Auth > Customize Messages} for non-Phacility use cases.
Test Plan: Grepped for removed symbol.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20099
Summary:
Depends on D20095. Ref T13244. Currently, auth providers have a list item view and a single gigantic edit screen complete with a timeline, piles of instructions, supplemental information, etc.
As a step toward making this stuff easier to use and more modern, give them a separate view UI with normal actions, similar to basically every other type of object. Move the timeline and "Disable/Enable" to the view page (from the edit page and the list page, respectively).
Test Plan: Created, edited, and viewed auth providers.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20096
Summary:
Depends on D20094. Ref T13244. Ref T6703. See PHI774. Currently, we use an older-style radio-button UI to choose an auth provider type (Google, Password, LDAP, etc).
Instead, use a more modern click-to-select UI.
Test Plan: {F6184343}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244, T6703
Differential Revision: https://secure.phabricator.com/D20095
Summary:
Ref T13244. See PHI774. If an install does not use password auth, the "one-time login" flow (via "Welcome" email or "bin/auth recover") is pretty rough. Current behavior:
- If an install uses passwords, the user is prompted to set a password.
- If an install does not use passwords, you're dumped to `/settings/external/` to link an external account. This is pretty sketchy and this UI does not make it clear what users are expected to do (link an account) or why (so they can log in).
Instead, improve this flow:
- Password reset flow is fine.
- (Future Change) If there are external linkable accounts (like Google) and the user doesn't have any linked, I want to give users a flow like a password reset flow that says "link to an external account".
- (This Change) If you're an administrator and there are no providers at all, go to "/auth/" so you can set something up.
- (This Change) If we don't hit on any other rules, just go home?
This may be tweaked a bit as we go, but basically I want to refine the "/settings/external/" case into a more useful flow which gives users more of a chance of surviving it.
Test Plan: Logged in with passwords enabled (got password reset), with nothing enabled as an admin (got sent to Auth), and with something other than passwords enabled (got sent home).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20094
Summary:
Ref T13244. This story publishes to the feed (and I think that's reasonable and desirable) but doesn't render as nicely as it could.
Improve the rendering.
(See T9233 for some context on why we render stories like this one in this way.)
Test Plan: {F6184490}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20097
Summary: See PHI1050. Although Diviner hasn't received a ton of new development for a while, it's: not exaclty new; and pretty useful for what we need it for.
Test Plan: Reading.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: leoluk
Differential Revision: https://secure.phabricator.com/D20086
Summary:
Ref T13244. See PHI1047. A while ago, the "Review" field changed from "yes/no" to 20 flavors of "Non-Owner Blocking Under A Full Moon". The sky didn't fall, so we'll probably do this to "Audit" eventually too.
The "owners.search" API method anticipates this and returns "none" or "audit" to describe package audit statuses, so it can begin returning "audit-non-owner-reviewers" or whatever in the future.
However, the "owners.edit" API method doesn't work the same way, and takes strings, and the strings have to be numbers. This is goofy and confusing and generally bad.
Make "owners.edit" take the same strings that "owners.search" emits. For now, continue accepting the old values of "0" and "1".
Test Plan:
- Edited audit status of packages via API using "none", "audit", "0", "1" (worked), and invalid values like "quack" (helpful error).
- Edited audit status of packages via web UI.
- Used `owners.search` to retrieve package information.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20091
Summary:
Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.
For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.
For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.
Test Plan: Ran unit tests.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20087
Summary: The textarea is, in fact, above the description!
Test Plan: Description text changed.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D20092
Summary: This seems generally reasonable, but is also a narrow fix to "Phacility scripts try to move instances into 'up', but the daemons can't MFA".
Test Plan: Launched a new instance locally, no more "daemons can't MFA" error.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20081
Summary: Ref T13242. Currently, the transaction query loads handles by default (this is unusual). We don't need them, so turn them off.
Test Plan: No apparent behavioral change, will compare production profiles.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20068
Summary:
See PHI1015. If you add new repository nodes to a cluster, we may not actually sync some repositories for up to 6 hours (if they've had no commits for 30 days).
Add an explicit check for out-of-sync repositories to trigger background sync.
Test Plan:
- Ran `bin/phd debug pullocal`.
- Fiddled with the `repository_workingcopy` version table to put the local node in and out of sync with the cluster.
- Saw appropriate responses in the daemon (sync; wait if the last sync trigger was too recent).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20078
Summary:
Fixes T13192. See PHI1015. When you deactivate a repository, we currently stop serving it.
This creates a problem for intracluster sync, since new nodes can't sync it. If nothing else, this means that if you "ship of theseus" your cluster and turn nodes over one at a time, you will eventually lose the entire repository. Since that's clearly a bad outcome, support sync.
Test Plan:
Testing this requires a "real" cluster, so I mostly used `secure`.
I deactivated rGITTEST and ran this on `secure002`:
```
./bin/repository thaw --demote secure002.phacility.net --force GITTEST && ./bin/repository update GITTEST
```
Before the patch, this failed:
```
[2019-01-31 19:40:37] EXCEPTION: (CommandException) Command failed with error #128!
COMMAND
git fetch --prune -- 'ssh://172.30.0.64:22/diffusion/GITTEST/' '+refs/*:refs/*'
STDOUT
(empty)
STDERR
Warning: Permanently added '172.30.0.64' (RSA) to the list of known hosts.
phabricator-ssh-exec: This repository ("rGITTEST") is not available over SSH.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
After applying (a similar patch to) this patch to `secure001`, the sync worked.
I'll repeat this test with the actual patch once this deploys to `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13192
Differential Revision: https://secure.phabricator.com/D20077
Summary:
On instances, the "SiteSource" (for site config) pretty much copy-pastes the "read POST data" block because it needs to make some decisions based on POST data when handling inbound mail webhooks.
Move the upstream read a little earlier so we can get rid of this. Now that this step is separated and must happen before the profiler, there's no reason not to do it earlier.
Test Plan: POSTed some data across pages without issue, will remove duplicate code in upcoming change.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20073
Summary: See PHI1048. We have similar transactions elsewhere already (particularly, on tasks) and these edges don't have any special properties (like "Closes Txx As Wontfix" edges do) so this doesn't create any sort of peril.
Test Plan: {F6170556}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20075
Summary:
Ref T12921. I'm moving Instances to modular transactions, and we have an "Alert" transaction type used to send notifications ("Your instance is going to be suspended for nonpayment.").
Currently, there's no way to specifically customize mail rendering under modular transactions. Add crude support for it.
Note that (per comment) this is fairly aspirational right now, since we actually always render everything as text (see T12921). But this API should (?) mostly survive intact when I fix this properly, and allows Instances to move to modular transactions so I can fix some more pressing issues in the meantime.
Test Plan: See next diff for Instances.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12921
Differential Revision: https://secure.phabricator.com/D20057
Summary: Depends on D20069. Ref T13232. This is a very, very weak dependency and we can reasonably polyfill it.
Test Plan: Grepped for `iconv` in libphutil, arcanist, and Phabricator.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13232
Differential Revision: https://secure.phabricator.com/D20070
Summary:
Depends on D20041. See PHI1046. If you do this:
- Create a parent project called "Crab" in Space 1.
- Create a milestone called "Left Claw".
- Shift "Crab" to Space 2.
- Create a milestone called "Right Claw".
...you currently end up with "Left Claw" in the wrong `spacePHID` in the database. At the application level it's in the correct space, but when we `WHERE ... AND spacePHID IN (...)` we can incorrectly filter it out.
Test Plan:
- Did the above setup.
- Saved "Crab", saw the space fix itself.
- Put things back in the broken state.
- Ran the migration script, saw things fix themselves again.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: aeiser, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20063
Summary: This query didn't get updated and could let you through an explicit "Sign with MFA" action if you have only disabled factors on your account.
Test Plan:
- Disabled all factors.
- Used explicit "Sign With MFA".
- Before: Went through.
- After: Sensible error.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20072
Summary:
See PHI1044. When you push a commit, we try to detect if the diff is the same as the last diff in Differential. This is a soft/heuristic check and only used to provide a hint in email ("CHANGED SINCE LAST DIFF").
For some changes, we can end up on this pathway with a binary changeset for a deleted file in the commit, and try to fetch the file data. This will fail since the file has been deleted. I'm not //entirely// sure why this hasn't cropped up before and didn't try to truly build an end-to-end test case, but it's a valid state that we shouldn't fatal in.
When we get here in this state, just detect a change. This is safe, even if it isn't always correct.
(This will be revisited in the future so I'm favoring fixing the broken behavior for now.)
Test Plan: This required some rigging, but I modified `bin/differential extract` to run the `isDiffChangedBeforeCommit()` code, then rigged a commit/diff to hit this case and reproduced the reported error. After the change, the comparison worked cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20056
Summary: Now that we have a nice function for this, use it to simplify some code.
Test Plan: Ran through the Duo enroll workflow to make sure signing still works.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20053
Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.
On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.
I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.
Move the POST rebuild a little earlier to fix this.
Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.
Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242, T12297, T4369
Differential Revision: https://secure.phabricator.com/D20046
Summary: Depends on D20039. Ref T13242. If installs want users to install a specific application, reference particular help, etc., let them customize the MFA enrollment message so they can make it say "if you have issues, see this walkthrough on the corporate wiki" or whatever.
Test Plan:
{F6164340}
{F6164341}
{F6164342}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20043
Summary:
Depends on D20044. Ref T13242. Similar to D20044, add reminder text to edit forms.
It would be nice to "workflow" these so the MFA flow happens inline, but Maniphest's inline edit behavior currently conflicts with this. Set it aside for now since the next workboards iteration (triggers) is probably a good opportunity to revisit it.
Test Plan: {F6164496}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20045
