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:
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:
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:
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 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. 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:
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:
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 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:
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:
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:
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 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: 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: 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 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:
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:
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 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:
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:
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 D20040. Ref T13242. See PHI1039. See PHI873. Two reasonable cases have arisen recently where extending validation rules would help solve problems.
We can do this in a pretty straightforward way with a standard extension pattern.
Test Plan:
Used this extension to keep ducks away from projects:
```lang=php
<?php
final class NoDucksEditorExtension
extends PhabricatorEditorExtension {
const EXTENSIONKEY = 'no.ducks';
public function getExtensionName() {
return pht('No Ducks!');
}
public function supportsObject(
PhabricatorApplicationTransactionEditor $editor,
PhabricatorApplicationTransactionInterface $object) {
return ($object instanceof PhabricatorProject);
}
public function validateTransactions($object, array $xactions) {
$errors = array();
$name_type = PhabricatorProjectNameTransaction::TRANSACTIONTYPE;
$old_value = $object->getName();
foreach ($xactions as $xaction) {
if ($xaction->getTransactionType() !== $name_type) {
continue;
}
$new_value = $xaction->getNewValue();
if ($old_value === $new_value) {
continue;
}
if (preg_match('/duck/i', $new_value)) {
$errors[] = $this->newInvalidTransactionError(
$xaction,
pht('Project names must not contain the substring "duck".'));
}
}
return $errors;
}
}
```
{F6162585}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20041
Summary:
Ref T13242. See PHI1039. Maniphest subtypes generally seem to be working well. I designed them as a general capability that might be extended to other `EditEngine` objects later, and PHI1039 describes a situation where extending subtypes to projects would give us some reasonable tools.
(Some installs also already use icons/colors as a sort of lightweight version of subtypes, so I believe this is generally useful capability.)
Some of this is a little bit copy-pasted and could probably be shared, but I'd like to wait a bit longer before merging it. For example, both configs have exactly the same structure right now, but Projects should possibly have some different flags (for example: to disable creating subprojects / milestones).
This implementation is pretty basic for now: notably, subprojects/milestones don't get the nice "choose from among subtype forms" treatment that tasks do. If this ends up being part of a solution to PHI1039, I'd plan to fill that in later on.
Test Plan: Defined multiple subtypes, created subtype forms, created projects with appropriate subtypes. Filtered them by subtype. Saw subtype information on list/detail views.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20040
Summary: Depends on D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.
Test Plan: See T13231 for a bunch of workflow discussion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20039
Summary: Depends on D20037. Ref T13222. Ref T7667. Although administrators can now disable MFA from the web UI, at least require that they survive MFA gates to do so. T7667 (`bin/auth lock`) should provide a sturdier approach here in the long term.
Test Plan: Created and edited MFA providers, was prompted for MFA.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T7667
Differential Revision: https://secure.phabricator.com/D20038
Summary: Depends on D20034. Ref T13222. This is just cleanup -- I thought we'd have like two of these, but we ended up having a whole lot in Duo and a decent number in SMS. Just let factors return a result explicitly if they can make a decision early. I think using `instanceof` for control flow is a lesser evil than using `catch`, on the balance.
Test Plan: `grep`, went through enroll/gate flows on SMS and Duo.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20035
Summary: Ref T13222. Providers can now be deprecated (existing factors still work, but users can't add new factors for the provider) or disabled (factors stop working, also can't add new ones).
Test Plan:
- Enabled, deprecated, and disabled some providers.
- Viewed provider detail, provider list.
- Viewed MFA settings list.
- Verified that I'm prompted for enabled + deprecated only at gates.
- Tried to disable final provider, got an error.
- Hit the MFA setup gate by enabling "Require MFA" with no providers, got a more useful message.
- Immediately forced a user to the "MFA Setup Gate" by disabling their only active provider with another provider enabled ("We no longer support TOTP, you HAVE to finish Duo enrollment to continue starting Monday.").
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20031
Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.
Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.
Test Plan: Implemented Duo, elsewhere.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D20028
Summary: Depends on D20025. Ref T13231. Although I'm not currently planning to actually upstream a Duo MFA provider, it's probably easiest to put most of the support pieces in the upstream until T5055.
Test Plan: Used a test script to make some (mostly trivial) API calls and got valid results back, so I think the parameter signing is correct.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20026
Summary:
Ref T920. Ref T13235. This adds a `Future`, similar to `TwilioFuture`, for interacting with Amazon's SNS service.
Also updates the documentation.
Also makes the code consistent with the documentation by accepting a `media` argument.
Test Plan: Clicked the "send test message" button from the Settings UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13235, T920
Differential Revision: https://secure.phabricator.com/D19982
Summary: Depends on D20024. See D20022. Put something in place temporarily until we build out validation at some point.
Test Plan: Sent myself a test message.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20025
Summary:
Depends on D20023. Ref T13222. Although I think this isn't strictly necessary from a pure security perspective (since you can't modify the primary number while you have MFA SMS), it seems like a generally good idea.
This adds a slightly new MFA mode, where we want MFA if it's available but don't strictly require it.
Test Plan: Disabled, enabled, primaried, unprimaried, and edited contact numbers. With MFA enabled, got prompted for MFA. With no MFA, no prompts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20024
Summary:
Depends on D20021. Ref T13222. This has a few rough edges, including:
- The challenges theselves are CSRF-able.
- You can go disable/edit your contact number after setting up SMS MFA and lock yourself out of your account.
- SMS doesn't require MFA so an attacker can just swap your number to their number.
...but mostly works.
Test Plan:
- Added SMS MFA to my account.
- Typed in the number I was texted.
- Typed in some other different numbers (didn't work).
- Cancelled/resumed the workflow, used SMS in conjunction with other factors, tried old codes, etc.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20022
Summary:
Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).
However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.
Also, slightly simplify some of the HTTP form gymnastics.
Test Plan:
- Hit the TOTP enroll screen.
- Reloaded it, got new secrets.
- Reloaded it more than 10 times, got told to stop generating new challenges.
- Answered a challenge properly, got a new TOTP factor.
- Grepped for removed class name.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20020
Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.
This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.
Test Plan:
- Added TOTP, guessed wrong a ton of times, got rate limited.
- Added TOTP, guessed right, got a TOTP factor configuration added to my account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20019
Summary:
Depends on D20011. Ref T920. This change lets a "MetaMTAMail" storage object represent various different types of messages, and makes "all" the `bin/mail` stuff "totally work" with messages of non-email types.
In practice, a lot of the related tooling needs some polish/refinement, but the basics work.
Test Plan: Used `echo beep boop | bin/mail send-test --to epriestley --type sms` to send myself SMS.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20012
Summary:
Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.
Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.
Test Plan:
- Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20011
Summary: Depends on D20008. Ref T920. Continue fleshing out contact number behaviors.
Test Plan:
- Enabled and disabled a contact number.
- Saw list, detail views reflect change.
- Added number X, disabled it, added it again (allowed), enabled the disabled one ("already in use" exception).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20010
Summary:
Depends on D19988. See D19826 for the last UI expansion. I don't have an especially strong product rationale for un-fixed-width'ing Settings since it doesn't suffer from the "mystery meat actions" issues that other fixed-width UIs do, but I like the full-width UI better and the other other fixed-width UIs all (?) have some actual rationale (e.g., large tables, multiple actions on subpanels), so "consistency" is an argument here.
Also rename "account" to "language" since both settings are language-related.
This moves away from the direction in D18436.
Test Plan:
Clicked each Settings panel, saw sensible rendering at full-width.
{F6145944}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20005
Summary:
Ref T920. To send you SMS messages, we need to know your phone number.
This adds bare-bone basics (transactions, storage, editor, etc).
From here:
**Disabling Numbers**: I'll let you disable numbers in an upcoming diff.
**Primary Number**: I think I'm just going to let you pick a number as "primary", similar to how email works. We could imagine a world where you have one "MFA" number and one "notifications" number, but this seems unlikely-ish?
**Publishing Numbers (Profile / API)**: At some point, we could let you say that a number is public / "show on my profile" and provide API access / directory features. Not planning to touch this for now.
**Non-Phone Numbers**: Eventually this could be a list of other similar contact mechanisms (APNS/GCM devices, Whatsapp numbers, ICQ number, twitter handle so MFA can slide into your DM's?). Not planning to touch this for now, but the path should be straightforward when we get there. This is why it's called "Contact Number", not "Phone Number".
**MFA-Required + SMS**: Right now, if the only MFA provider is SMS and MFA is required on the install, you can't actually get into Settings to add a contact number to configure SMS. I'll look at the best way to deal with this in an upcoming diff -- likely, giving you partial access to more of Setings before you get thorugh the MFA gate. Conceptually, it seems reasonable to let you adjust some other settings, like "Language" and "Accessibility", before you set up MFA, so if the "you need to add MFA" portal was more like a partial Settings screen, maybe that's pretty reasonable.
**Verifying Numbers**: We'll probably need to tackle this eventually, but I'm not planning to worry about it for now.
Test Plan: {F6137174}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: avivey, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19988
