Summary:
Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things.
Instead:
- Replace the edit pencil with a red lock.
- Provide cursor hints for grabbable / not grabbable.
- Don't let users pick up cards they can't edit.
Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10722
Differential Revision: https://secure.phabricator.com/D20274
Summary: Depends on D20272. Ref T13074. When a task requires MFA to edit, you currently get a fatal. Provide a cancel URI so the prompt works and the edit can go through.
Test Plan:
- Locked a task, dragged it on a workboard.
- Before: fatal trying to build an MFA gate.
- After: got MFA gated, answered prompt, action went through.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20273
Summary:
Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly.
When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing.
Test Plan: {F6264611}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20272
Summary:
Depends on D20270. Ref T10333. If you create a task with a new owner, or edit a task and change the priority/owner, we want to move it (and possibly create a new header) when the response comes back.
Make sure the response includes the appropriate details about the object's header and position.
Test Plan:
- Grouped by Owner.
- Created a new task with a new owner, saw the header appear.
- Edited a task and changed it to give it a new owner, saw the header appear.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20271
Summary: Depends on D20269. Ref T10333. Now that orderings are modularized, this is fairly easy to implement. This isn't super fancy for now (e.g., no profile images) but I'll touch it up in a general polish followup.
Test Plan: {F6264596}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20270
Summary:
Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`.
Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering.
Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header).
Then move the existing "Natural" and "Priority" orders into this new hierarchy.
This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change.
Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20269
Summary:
Depends on D20266. Boards currently have several `whateverMap<cardPHID => stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`.
The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns).
Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20267
Summary:
Depends on D20265. Ref T10333. Now that neither task lists nor workboards use subpriority, we can remove all the readers and writers.
I'm not actually getting rid of the column data yet, but anticipate doing that in a future change.
Note that the subpriority algorithm (removed here) is possibly better than the "natural order" algorithm still in use. It's a bit more clever, and likely performs far fewer writes. I might make the "natural order" code use an algorithm more similar to the "subpriority" algorithm in the future.
Test Plan: Grepped for `subpriority`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20266
Summary:
Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice".
When you drag the bottom-most task under "Alice" to the top, what does that mean?
Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task.
Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute.
We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably.
In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes.
Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by `<priority, natural>`. When you drag stuff around inside a priority group, we update the natural order.
This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to.
Test Plan:
- Viewed and dragged a natural board.
- Viewed and dragged a priority board.
- Dragged within and between groups of 0, 1, and multiple items.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20265
Summary:
Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted.
I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards.
This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards.
As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note".
Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can.
The only real reason to have a global "subpriority" is to support the list-view drag-and-drop.
It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein.
(This just removes UI, the actual subpriority system is still intact and still used on workboards.)
Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13074
Differential Revision: https://secure.phabricator.com/D20263
Summary:
These effects feel like they're possibly overkill, since other CSS rules make the selection reticle behave correctly and the implementation is relatively intuitive.
Or not, either way.
Test Plan: Selected text on either side of a 2-up diff, no more opacity effects.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20264
Summary:
Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards.
As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority.
(Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.)
Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333, T8135
Differential Revision: https://secure.phabricator.com/D20248
Summary:
Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are:
- Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal".
- Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column.
- Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135).
- Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations).
- Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it.
NOTE: This only makes these headers work for display.
They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup.
Test Plan: {F6257686}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10333
Differential Revision: https://secure.phabricator.com/D20247
Summary:
Ref T10334. Partly, this just improves visual feedback for all drag operations. After D20242, we can have cases where you (for example) drag a low-priority node to a very tall column on a priority-ordered workboard. In this case, the actual dashed-border-drop-target may not be on screen.
We might make the column scroll or put some kind of hint in the UI in this case, but an easy starting point is just to make the "yes, you're targeting this column" state a bit more clear.
Test Plan: Dragged tasks between columns, saw the border higlight on the target columns. This is very tricky to take a screenshot of.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20245
Summary:
Ref T10334. When a workboard is ordered by priority, dragging from column "A" to a particular place in column "B" currently means "move this task to column B, and adjust its priority so that it naturally sorts into the location under my mouse cursor".
Users frequently find this confusing / undesirable.
To begin improving this, make "drag from column A to column B" and "drag from somewhere in column A to somewhere else in column A" into different operations. The first operation, a movement between columns, no longer implies an ordering change. The second action still does.
So if you actually want to change the priority of a task, you drag it within its current column. If you just want to move it to a different column, you drag it between columns.
This creates some possible problems:
- Some users may love the current behavior and just not be very vocal about it. I doubt it, but presumably we'll hear from them if we break it.
- If you actualy want to move + reorder, it's a bit more cumbersome now. We could possibly add something like "shift + drag" for this if there's feedback.
- The new behavior is probably less surprising, but may not be much more obvious. Future changes (for example, in T10335) should help make it more clear.
- When you mouse cursor goes over column B, the card dashed-rectangle preview target thing jumps to the correct position in the column -- but that may not be under your mouse cursor. This feels pretty much fine if the whole column fits on screen. It may not be so great if the column does not fit on screen and the dashed-rectangle-thing has vanished. This is just a UI feedback issue and we could refine this later (scroll/highlight the column).
Test Plan:
- Created several tasks at different priority levels, sorted a board by priority, dragged tasks between columns. Dragging from "A" to "B" no longer causes a priority edit.
- Also, dragged within a column. This still performs priority edits.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10334
Differential Revision: https://secure.phabricator.com/D20242
Summary:
Ref T13249. Ref T13258. In some cases, builds are not idempotent and should not be restarted casually.
If the scary part is at the very end (deploy / provision / whatever), it could be okay to restart them if they previously failed.
Also, make the "reasons why you can't restart" and "explanations of why you can't restart" logic a little more cohesive.
Test Plan:
- Tried to restart builds in various states (failed/not failed, restartable always/if failed/never, already restarted), got appropriate errors or restarts.
- (I'm not sure the "Autoplan" error is normally reachable, since you can't edit autoplans to configure things to let you try to restart them.)
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258, T13249
Differential Revision: https://secure.phabricator.com/D20252
Summary:
Depends on D20259. Now that we can index Herald rules to affected objects, show callers on the "Webhooks" UI.
A few other rule types could get indexes too ("Sign Legalpad Documents", "Add Reviewers", "Add Subscribers"), but I think they're less likely to be useful since those triggers are usually more obvious (the transaction timeline makes it clearer what happened/why). We could revisit this in the future now that it's a possibility.
Test Plan: {F6260106}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20260
Summary:
See PHI1123. The key on this table is `<resource, type, code>` but we currently query for only `<type, code>`. This can't use the key.
Constrain the query to the resource we expect (the repository) so it can use the key.
Test Plan: Pushed files using LFS. See PHI1123 for more, likely.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20261
Summary:
Ref T13258. Provide an easy way to find rules which trigger a particular build plan from the build plan page.
The implementation here ends up a little messy: we can't just search for `actionType = 'build' AND targetPHID = '<build plan PHID>'` since the field is a blob of JSON.
Instead, make rules indexable and write a "build plan is affected by rule actions" edge when indexing rules, then search on that edge.
For now, only "Run Build Plan: ..." rules actually write this edge, since I think (?) that it doesn't really have meaningful values for other edge types today. Maybe "Call Webhooks", and you could get a link from a hook to rules that trigger it? Reasonable to do in the future.
Things end up a little bit rough overall, but I think this panel is pretty useful to add to the Build Plan page.
This index needs to be rebuilt with `bin/search index --type HeraldRule`. I'll call this out in the changelog but I'm not planning to explicitly migrate or add an activity, since this is only really important for larger installs and they probably (?) read the changelog. As rules are edited over time, this will converge to the right behavior even if you don't rebuild the index.
Test Plan: {F6260095}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20259
Summary: Depends on D20256. Ref T13249. See PHI1115. This primarily makes `bin/policy unlock --owner epriestley T123` work. This is important for "Edit Locked" tasks, since changing the edit policy doesn't really do anything.
Test Plan: Hard-locked a task as "alice", reassigned it to myself with `bin/policy unlock --owner epriestley`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20257
Summary:
See PHI1115. Ref T13249. Currently, you can `bin/policy unlock` objects which have become inaccessible through some sort of policy mistake.
This script uses a very blunt mechanism to perform unlocks: just manually calling `setXPolicy()` and then trying to `save()` the object. Improve things a bit:
- More surgical: allow selection of which policies you want to adjust with "--view", "--edit", and "--owner" (potentially important for some objects like Herald rules which don't have policies, and "edit-locked" tasks which basically ignore the edit policy).
- More flexible: Instead of unlocking into "All Users" (which could be bad for stuff like Passphrase credentials, since you create a short window where anyone can access them), take a username as a parameter and set the policy to "just that user". Normally, you'd run this as `bin/policy unlock --view myself --edit myself` or similar, now.
- More modular: We can't do "owner" transactions in a generic way, but lay the groundwork for letting applications support providing an owner reassignment mechanism.
- More modern: Use transactions, not raw `set()` + `save()`.
This previously had some hard-coded logic around unlocking applications. I've removed it, and the new generic stuff doesn't actually work. It probably should be made to work at some point, but I believe it's exceptionally difficult to lock yourself out of applications, and you can unlock them with `bin/config set phabricator.application-settings ...` anyway so I'm not too worried about this. It's also hard to figure out the PHID of an application and no one has ever asked about this so I'd guess the reasonable use rate of `bin/policy unlock` to unlock applications in the wild may be zero.
Test Plan:
- Used `bin/policy unlock` to unlock some objects, saw sensible transactions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20256
Summary:
Ref T13249. See PHI1115. I initially wanted to make `bin/policy unlock --owner <user> H123` work to transfer ownership of a Herald rule, although I'm no longer really sure this makes much sense.
In any case, this makes things a little better and more modern.
I removed the storage table for rule comments. Adding comments to Herald rules doesn't work and probably doesn't make much sense.
Test Plan: Created and edited Herald rules, grepped for all the transaction type constants.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20258
Summary:
Ref T13249. Using "--as" to call some Conduit methods as a user can currently fatal when trying to access settings/preferences.
Allow inline regeneration of user caches.
Test Plan: Called `project.edit` to add a member. Before: constructing a policy field tried to access the user's preferences and failed. After: Smooth sailing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20255
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:
Ref T13259. An install provided feedback that it wasn't obvious you could click the buttons in this UI.
Make it more clear that these are clickable buttons.
Test Plan:
{F6251585}
{F6251586}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20238
Summary:
See T13253. After D20200 (which changed the task schema) these migrations no longer run, since the PHP code will expect a column to exist that won't exist until a `20190220.` migration runs.
We don't need these migrations, since anyone upgrading through September 2017 gets a "rebuild search indexes" activity anyway (see T11932). Just no-op them.
Test Plan: Grepped for `queueDocumentForIndexing()` in `autopatches/`, removed all of it.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20243
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
