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: 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 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 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:
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: 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: 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
Summary:
Ref T13242. Warn user that they'll need to MFA (so they can go dig their phone out of their bag first or whatever, or don't type a giant comment on mobile if their U2F key is back at the office) on the comment form.
Also, when they'll need MFA and won't be able to provide it (no MFA on account), stop them from typing up a big comment that they can't actually submit: point them at MFA setup first.
Test Plan:
{F6164448}
{F6164449}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20044
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:
Ref T13242. See <https://discourse.phabricator-community.org/t/out-of-range-value-for-column-deviceversion/2218>.
The synchronization log column is `uint32?` and `-1` doesn't go into that column.
Since we're only using `-1` for convenience to cheat through `$max_version > $this_version` checks, use `null` instead and make the checks more explicit.
Test Plan: Reproducing this is a bit tricky and I cheated fairly heavily to force the code down this pathway without actually building a multi-device cluster, but I did reproduce the original exception, apply the patch, and observe that it fixed things.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20047
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 D20036. Ref T13222. Now that we support one-shot MFA, swap this from session MFA to one-shot MFA.
Test Plan: Revealed a credential, was no longer left in high-security mode.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20037
Summary:
Depends on D20035. Ref T13222.
- Allow individual transactions to request one-shot MFA if available.
- Make "change username" request MFA.
Test Plan:
- Renamed a user, got prompted for MFA, provided it.
- Saw that I no longer remain in high-security after performing the edit.
- Grepped for other uses of `PhabricatorUserUsernameTransaction`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20036
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:
Depends on D20033. Ref T13222. Flesh this UI out a bit, and provide bit-strength information for TOTP.
Also, stop users from adding multiple SMS factors since this is pointless (they all always text your primary contact number).
Test Plan:
{F6156245}
{F6156246}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20034
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: See PHI1038. I missed these when pulling the code out.
Test Plan: Sent "Must encrypt" mail, verified it made it through the queue in one piece.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20029
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 D20022. Ref T13222. Since you can easily lock yourself out of your account by swapping to a bad number, prevent contact number edits while "contact number" MFA (today, always SMS) is enabled.
(Another approach would be to bind factors to specific contact numbers, and then prevent that number from being edited or disabled while SMS MFA was attached to it. However, I think that's a bit more complicated and a little more unwieldy, and ends up in about the same place as this. I'd consider it more strongly in the future if we had like 20 users say "I have 9 phones" but I doubt this is a real use case.)
Test Plan:
- With SMS MFA, tried to edit my primary contact number, disable it, and promote another number to become primary. Got a sensible error message in all cases.
- After removing SMS MFA, did all that stuff with no issues.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20023
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 D20020. Ref T13222. This puts another step in the MFA enrollment flow: pick a provider; read text and click "Continue"; actually enroll.
This is primarily to stop CSRF attacks, since otherwise an attacker can put `<img src="phabricator.com/auth/settings/enroll/?providerPHID=xyz" />` on `cute-cat-pix.com` and get you to send yourself some SMS enrollment text messages, which would be mildly annoying.
We could skip this step if we already have a valid CSRF token (and we often will), but I think there's some value in doing it anyway. In particular:
- For SMS/Duo, it seems nice to have an explicit "we're about to hit your phone" button.
- We could let installs customize this text and give users a smoother onboard.
- It allows the relatively wordy enroll form to be a little less wordy.
- For tokens which can expire (SMS, Duo) it might save you from answering too slowly if you have to go dig your phone out of your bag downstairs or something.
Test Plan: Added factors, read text. Tried to CSRF the endpoint, got a dialog instead of a live challenge generation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20021
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 D20017. Ref T13222. Currently, if you:
- type some text at a TOTP gate;
- wait ~60 seconds for the challenge to expire;
- submit the form into a "Wait patiently" message; and
- mash that wait button over and over again very patiently
...you still rack up rate limiting points, because the hidden text from your original request is preserved and triggers the "is the user responding to a challenge" test. Only perform this test if we haven't already decided that we're going to make them wait.
Test Plan:
- Did the above; before patch: rate limited; after patch: not rate limited.
- Intentionally typed a bunch of bad answers which were actually evaluated: rate limited properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20018
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.
SMS will have these guidance messages:
- "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
- "Administrator: SMS is weak."
- "User: you haven't configured a contact number."
Test Plan: {F6151283} {F6151284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20017
Summary:
Depends on D20013. Recently, I renamed the "Account" panel to "Language".
When you land on "Settings" and the first panel is an "EditEngine" panel ("Account/Langauge", "Date and Time", and "Conpherence" are all "EditEngine" panels), the engine shows the controls for the first panel.
However, the "first panel" according to EditEngine and the "first panel" in the menu are currently different: the menu groups panels into topics.
When I renamed "Account" to "Language", it went from conicidentally being the first panel in both lists to being the second panel in the grouped menu list and the, uh, like 12th panel in the ungrouped raw list.
This made landing on "Settings" show you the right chrome, but show you a different panel's controls ("Conpherence", now alphabetically first).
Instead, use the same order in both places.
(This was also a pre-existing bug if you use a language which translates the panel names such that "Account" is not alphabetically first.)
Test Plan: Visited "Settings", saw "Date & Time" form controls instead of "Conpherence" form controls on the default screen with "Date & Time" selected in the menu.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20016
Summary:
Depends on D20012. Ref T920. If you have a test adapter configured, it should swallow messages and prevent them from ever hitting a lower-priority adapter.
Make the test adapter support SMS so this actually happens.
Test Plan: Ran `bin/mail send-test --type sms ...` with a test adapter (first) and a Twilio adapter (second). Got SMS swallowed by test adapter instead of live SMS messages.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20013
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 D20006. Ref T13222. Currently, the "MFA Is Required" gate doesn't let you do anything else, but you'll need to be able to access "Contact Numbers" if an install provides SMS MFA.
Tweak this UI to give users limited access to settings, so they can set up contact numbers and change their language.
(This is a little bit fiddly, and I'm doing it early on partly so it can get more testing as these changes move forward.)
Test Plan: {F6146136}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20008
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
Summary:
Ref T13222. This updates the CLI tools and documentation for the changes in D19975.
The flags `--type` and `--all-types` retain their current meaning. In most cases, `bin/auth strip --type totp` is sufficient and you don't need to bother looking up the relevant provider PHID. The existing `bin/auth list-factors` is also unchanged.
The new `--provider` flag allows you to select configs from a particular provider in a more granular way. The new `bin/auth list-mfa-providers` provides an easy way to get PHIDs.
(In the Phacility cluster, the "Strip MFA" action just reaches into the database and deletes rows manually, so this isn't terribly important. I verified that the code should still work properly.)
Test Plan:
- Ran `bin/auth list-mfa-providers`.
- Stripped by user / type / provider.
- Grepped for `list-factors` and `auth strip`.
- Hit all (?) of the various possible error cases.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19976
Summary:
Ref T13222. Users configure "Factor Configs", which say "I have an entry on my phone for TOTP secret key XYZ".
Currently, these point at raw implementations -- always "TOTP" in practice.
To support configuring available MFA types (like "no MFA") and adding MFA types that need some options set (like "Duo", which needs API keys), bind "Factor Configs" to a "Factor Provider" instead.
In the future, several "Factors" will be available (TOTP, SMS, Duo, Postal Mail, ...). Administrators configure zero or more "MFA Providers" they want to use (e.g., "Duo" + here's my API key). Then users can add configs for these providers (e.g., "here's my Duo account").
Upshot:
- Factor: a PHP subclass, implements the technical details of a type of MFA factor (TOTP, SMS, Duo, etc).
- FactorProvider: a storage object, owned by administrators, configuration of a Factor that says "this should be available on this install", plus provides API keys, a human-readable name, etc.
- FactorConfig: a storage object, owned by a user, says "I have a factor for provider X on my phone/whatever with secret key Q / my duo account is X / my address is Y".
Couple of things not covered here:
- Statuses for providers ("Disabled", "Deprecated") don't do anything yet, but you can't edit them anyway.
- Some `bin/auth` tools need to be updated.
- When no providers are configured, the MFA panel should probably vanish.
- Documentation.
Test Plan:
- Ran migration with providers, saw configs point at the first provider.
- Ran migration without providers, saw a provider created and configs pointed at it.
- Added/removed factors and providers. Passed MFA gates. Spot-checked database for general sanity.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19975
Summary: See https://secure.phabricator.com/D18901#249481. Update the docs and a warning string to reflect the new reality that `bin/auth recover` is now able to recover any account, not just administrators.
Test Plan: Mk 1 eyeball
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20007
Summary:
See PHI1030. When you edit an Almanac object, we attempt to discover all the related objects so we can dirty the repository cluster routing cache: if you modify a Device or Service that's part of a clustered repository, we need to blow away our cached view of the layout.
Currently, we don't correctly find linked Bindings when editing a Device, so we may miss Services which have keys that need to be disabled. Instead, discover these linked objects.
See D17000 for the original implementation and more context.
Test Plan:
- Used `var_dump()` to dump out the discovered objects and dirtied cache keys.
- Before change: editing a Service dirties repository routing keys (this is correct), but editing a Device does not.
- After change: editing a Device now correctly dirties repository routing keys.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20003
Summary:
Fixes T13082. When you create a revision (say, `D111`) with `Ref T222` in the body, we write a `D111 -> T222` edge ("revision 111 references task 222") and an inverse `T222 -> D111` edge ("task 222 is referenced by revision 111").
We also apply a transaction to `D111` ("alice added a task: Txxx.") and an inverse transaction to `T222` ("alice added a revision: Dxxx").
Currently, it appears that the inverse transaction can sometimes generate mail faster than `D111` actually commits its (database) transactions, so the mail says "alice added a revision: Unknown Object (Differential Revision)". See T13082 for evidence that this is true, and a reproduction case.
To fix this, apply the inverse transaction (to `T222`) after we commit the main object (here, `D111`).
This is tricky because when we apply transactions, the transaction editor automatically "fixes" them to be consistent with the database state. For example, if a task already has title "XYZ" and you set the title to "XYZ" (same title), we just no-op the transaction.
It also fixes edge edits. The old sequence was:
- Open (database) transaction.
- Apply our transaction ("alice added a task").
- Apply the inverse transaction ("alice added a revision").
- Write the edges to the database.
- Commit (database) transaction.
Under this sequence, the inverse transaction was "correct" and didn't need to be fixed, so the fixing step didn't touch it.
The new sequence is:
- Open (database) transaction.
- Apply our transaction ("alice added a task").
- Write the edges.
- Commit (database) transaction.
- Apply the inverse transaction ("alice added a revision").
Since the inverse transaction now happens after the database edge write, the fixing step detects that it's a no-op and throws it away if we do this naively.
Instead, add some special cases around inverse edits to skip the correction/fixing logic, and just pass the "right" values in the first place.
Test Plan:
Added and removed related tasks from revisions, saw appropriate transactions render on both objects.
(It's hard to be certain this completely fixes the issue since it only happened occasionally in the first place, but we can see if it happens any more on `secure`.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13082, T222
Differential Revision: https://secure.phabricator.com/D19969
Summary: See PHI1030. When installs hit this error, provide more details about which node we ended up on and what's going on.
Test Plan:
```
$ git pull
phabricator-ssh-exec: This repository request (for repository "spellbook") has been incorrectly routed to a cluster host (with device name "local.phacility.net", and hostname "orbital-3.local") which can not serve the request.
The Almanac device address for the correct device may improperly point at this host, or the "device.id" configuration file on this host may be incorrect.
Requests routed within the cluster by Phabricator are always expected to be sent to a node which can serve the request. To prevent loops, this request will not be proxied again.
(This is a read request.)
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20002
Summary: Fixes T13239. See that task for discussion.
Test Plan: Tried to send welcome mail with no "Welcome" message.
Maniphest Tasks: T13239
Differential Revision: https://secure.phabricator.com/D20001
Summary:
See PHI1026. Allow installs to export Conduit call logs to a flat format.
Also, add date range queries.
Test Plan:
- Exported some call logs.
- Filtered logs by date.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19996
Summary:
Depends on D19994. See PHI1027. If an install has customized the "Welcome Mail" message, include it in welcome mail. A special custom message from the profile screen overrides it, if provided.
(I fiddled with putting the custom message as "placeholder" text in the remarkup area as a hint, but newlines in "placeholder" text appear to have issues in Safari and Firefox. I think this is probably reasonably clear as-is.)
Make both render remarkup-into-text so things like links work properly, as it's reasonably likely that installs will want to link to things.
Test Plan:
- With custom "Welcome Mail" text, sent mail with no custom override (got custom text) and a custom override (got overridden text).
- Linked to some stuff, got sensible links in the mail (`bin/mail show-outbound`).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19995
Summary: Depends on D19992. Ref T13222. If administrators provide a custom login message, show it on the login screen.
Test Plan:
{F6137930}
- Viewed login screen with and without a custom message.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19994
Summary:
Ref T13222. Long ago, we had a Config option (`welcome.html`) to let you dump HTML onto the login screen, but this was relatively hard to use and not good from a security perspective.
In some cases this was obsoleted by Dashboards, but there's at least some remaining set of use cases for actual login instructions on the login screen. For example, WMF has some guidance on //which// SSO mechanism to use based on what types of account you have. On `secure`, users assume they can register by clicking "Log In With GitHub" or whatever, and it might reduce frustration to tell them upfront that registration is closed.
Some other types of auth messaging could also either use customization or defaults (e.g., the invite/welcome/approve mail).
We could do this with a bunch of Config options, but I'd generally like to move to a world where there's less stuff in Config and more configuration is contextual. I think it tends to be easier to use, and we get a lot of fringe benefits (granular permissions, API, normal transaction logs, more abililty to customize workflows and provide contextual help/hints, etc). Here, for example, we can provide a remarkup preview, which would be trickier with Config.
This does not actually do anything yet.
Test Plan: {F6137541}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19992
Summary:
See PHI1027. Currently, we allow you to customize invite email, but not most other types of email (approve, welcome). As a step forward, also allow welcome email to be customized with a message.
I considered separating the custom text from the main text with something heavyhanded ("alice added this custom message:") or a beautiful ASCII art divider like one of these:
https://www.asciiart.eu/art-and-design/dividers
...but nothing truly sung to me.
This only works on the profile flow for now. I'm planning to let you set a default message. I may or may not let you customize from "Create New User", seems like the default message probably covers most of that. Probably won't touch `scripts/user/add_user.php` since that's not really exactly super supported.
Test Plan:
Sent mail with and without custom messages, reviewed it with `bin/mail show-outbound`.
{F6137410}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19991
Summary:
See PHI1027. Currently, the "Welcome" mail always tells users to set a password. This definitely isn't helpful if an install doesn't have password auth enabled.
We can't necessarily guess what they're supposed to do, so just give them generic instructions ("set up your account"). Upcoming changes will give administrators more control over the mail content.
Test Plan: Sent both versions of the mail, used `bin/mail show-outbound` to inspect them for correctness.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19990
Summary:
Ref PHI1027. Currently, `PhabricatorUser` has a couple of mail-related methods which shouldn't really be there in the long term. Immediately, I want to make some adjusments to the welcome email.
Move "Welcome" mail generation to a separate class and consolidate all the error handling. (Eventually, "invite" and "verify address" email should move to similar subclasses, too.) Previously, a bunch of errors/conditions got checked in multiple places.
The only functional change is that we no longer allow you to send welcome mail to disabled users.
Test Plan:
- Used "Send Welcome Mail" from profile pages to send mail.
- Hit "not admin", "disabled user", "bot/mailing list" errors.
- Used `scripts/user/add_user.php` to send welcome mail.
- Used "Create New User" to send welcome mail.
- Verified mail with `bin/mail show-outbound`. (Cleaned up a couple of minor display issues here.)
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19989
Summary: Ref T13238. Warn users about these horrible options and encourage them to defuse them.
Test Plan: Hit both warnings, fixed the issues, issues went away.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13238
Differential Revision: https://secure.phabricator.com/D19999
Summary:
Ref T13222. Currently, if a remote address fails a few login attempts (5) in a short period of time (15 minutes) we require a CAPTCHA for each additional attempt.
This relies on:
- Administrators configuring ReCAPTCHA, which they may just not bother with.
- Administrators being comfortable with Google running arbitrary trusted Javascript, which they may not be comfortable with.
- ReCAPTCHA actually being effective, which seems likely true for unsophisticated attackers but perhaps less true for more sophisticated attackers (see <https://github.com/ecthros/uncaptcha2>, for example).
(For unsophisticated attackers and researchers, "Rumola" has been the standard CAPTCHA bypass tool for some time. This is an extension that pays humans to solve CAPTCHAs for you. This is not practical at "brute force a strong password" scale. Google appears to have removed it from the Chrome store. The "submit the captcha back to Google's APIs" trick probably isn't practical at brute-force-scale either, but it's easier to imagine weaponizing that than weaponizing human solvers.)
Add a hard gate behind the CAPTHCA wall so that we fail into a secure state if there's no CAPTCHA or the attacker can defeat CAPTCHAs at a very low cost.
The big downside to this is that an attacker who controls your remote address (e.g., is behind the same NAT device you're behind on corpnet) can lock you out of your account. However:
- That //should// be a lot of access (although maybe this isn't that high of a barrier in many cases, since compromising a "smart fridge" or "smart water glass" or whatever might be good enough).
- You can still do "Forgot password?" and login via email link, although this may not be obvious.
Test Plan:
- Logged in normally.
- Failed many many login attempts, got hard gated.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19997
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
Ref T13222. If we receive a message and nothing processes it, we normally try to send the user an error message like "hey, nothing handled this, maybe you got the address wrong".
Just skip the "send them an error message" part if any recipient was reserved, so if you "Reply All" to a message that is "From: noreply@phabricator" you don't get a relatively unhelpful error.
This also makes sure that the "void" address doesn't generate bounces if the "From" is a valid user email address (e.g., with `metamta.can-send-as-user`). That is:
- Phabricator needs to send a mail with only "CC" users.
- Phabricator puts the "void" address in "To" as a placeholder.
- The "void" address happens to route back to Phabricator.
We don't want that mail to bounce to anywhere. Normally, it won't:
- From is usually "noreply@phabricator", which isn't a user, so we won't send anything back: we only send mail to verified user email addresses.
- The message will have "X-Phabricator-Sent-This-Message: true" so we won't process it at all.
...but this is another layer of certainty.
Test Plan: Used `bin/mail receive-test` to receive mail to an invalid, unreserved address (bounce/error email) and an invalid, reserved address (no bounce/error email).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19987
Summary:
See PHI1023. Ref T7607. Occasionally, companies need their billing address (or some other custom text) to appear on invoices to satisfy process or compliance requirements.
Allow accounts to have a custom "Billing Name" and a custom "Billing Address" which appear on invoices.
Test Plan: {F6134707}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7607
Differential Revision: https://secure.phabricator.com/D19979
Summary:
See PHI1025. When you "Import Columns", we test if you're trying to import into a board that already has columns. However, this test is too broad (it incorrectly detects "proxy" columns for milestones as columns) and not user-friendly (it returns 400 instead of a readable error).
Correct these issues, and refine some of the logic around proxy columns.
Test Plan:
- Created a project, A.
- Created a milestone under that project.
- Imported another project's columns to A's workboard.
- Before change: Unhelpful 400.
- After change: import worked fine.
- Also, hit the new error dialogs and read through them.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19978
Summary:
Fixes T13234. Don't let application email addresses be configured with user addresses. This might prevent an unlikely bit of mischief where someone does this intentionally, detailed in T13234.
(Possibly, these tables should just be merged some day, similar to how the "Password" table is now a shared resource that's modular enough for multiple applications to use it.)
Test Plan: {F6132259}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13234
Differential Revision: https://secure.phabricator.com/D19974
Summary:
Fixes T11782. When you "Close as Duplicate", generate a "[Merged]" email by making the merge the first transaction.
(There are other, more-deterministic ways to do this with action strength, but this is much simpler and I believe it suffices.)
Test Plan: Used "Close as Duplicate", got a "[Merged]" email out of it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11782
Differential Revision: https://secure.phabricator.com/D19972
Summary:
Fixes T12921. Currently, we call `getTitleForHTMLMail()`, but that calls `getTitleForMail()` which forces us into text rendering mode.
Instead, have `getTitleForHTML/TextMail()` force the rendering mode, then call `getTitleForMail()` with the desired rendering mode.
This causes stories like "epriestely added dependent tasks: x, y." to appear as links in email instead of plain text.
Test Plan: Used `bin/mail show-outbound --id ... --dump-html > out.html` to verify HTML mail.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12921
Differential Revision: https://secure.phabricator.com/D19968
Summary: Ref T13222. Ref T920. This is the last of the upstream adapter updates.
Test Plan:
- Sent mail with SES.
- Sent mail with "sendmail". I don't have sendmail actually configured to an upstream MTA so I'm not 100% sure this worked, but the `sendmail` binary didn't complain and almost all of the code is shared with SES, so I'm reasonably confident this actually works.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T920
Differential Revision: https://secure.phabricator.com/D19965
Summary:
Ref T920. Ref T12404.
- Update to the new "$message" API.
- Remove "encoding". I believe "base64" is always the best value for this since we stopped seeing issues once we changed the default.
- Remove "mailer". This is a legacy option that makes little sense given how configuration now works.
- Rename to "SMTP". This doesn't affect users anymore since this mailer has been configured as `smtp` for about a year.
- This does NOT add a timeout since the SMTP code is inside PHPMailer (see T12404).
Test Plan: Sent messages with many mail features via GMail SMTP and SendGrid SMTP.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12404, T920
Differential Revision: https://secure.phabricator.com/D19961
Summary:
See T1024. When "CAN_EDIT" became default in T13186, this was missed as an exception.
Watching shouldn't require "CAN_EDIT", so exempt it.
Test Plan:
- Before change: tried to watch a project I could not edit, got a policy error.
- After change: watched/unwatched a project I could not edit.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19977
Summary:
Ref T920. Ref T5969.
- Update to the new "$message" API.
- Update to Sendgrid v3.
- Add a timeout.
- This removes the "api-user" option, which Sendgrid no longer seems to use.
Test Plan: Sent Sendgrid messages with `bin/mail send-test ...` using subject/headers/attachments/html/to/cc.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19960
Summary: Ref T920. Ref T5969. Update the Mailgun adapter for the API changes and add a timeout.
Test Plan: Configured Mailgun as a mailer, sent mail with subject/to/cc/headers/html/attachments using `bin/mail send-test`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19959
Summary: Depends on D19956. Ref T920. Move the TestAdapter to the new API and adjust a couple of tests for the changes.
Test Plan: All tests now pass.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19957
Summary:
Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:
- Update the inbound whitelist.
- Add a little support for `media` configuration.
- Add a service call timeout (see T5969).
- Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.
Test Plan: Used `bin/mail send-test` to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T5969, T920
Differential Revision: https://secure.phabricator.com/D19956
Summary:
Depends on D19954. Ref T920. This is a step toward a world where "Mailers" are generic and may send messages over a broader array of channels (email, SMS, postal mail).
There are a few major parts here:
- Instead of calling `$mailer->setSubject()`, `$mailer->setFrom()`, etc., build in intermediate `$message` object first, then pass that to the mailer.
- This breaks every mailer! This change on its own does not fix them. I plan to fix them in a series of "update mailer X", "update mailer Y" followups.
- This generally makes the API easier to change in the far future, and in the near future supports mailers accepting different types of `$message` objects with the same API.
- Pull the "build an email" stuff out into a `PhabricatorMailEmailEngine`. `MetaMTAMail` is already a huge object without also doing this translation step. This is just a separation/simplification change, but also tries to fight against `MetaMTAMail` getting 5K lines of email/sms/whatsapp/postal-mail code.
- Try to rewrite the "build an email" stuff to be a bit more straightforward while making it generate objects. Prior to this change, it had this weird flow:
```lang=php
foreach ($properties as $key => $prop) {
switch ($key) {
case 'xyz':
// ...
}
}
```
This is just inherently somewhat hard to puzzle out, and it means that processing order depends on internal property order, which is quite surprising.
Test Plan: This breaks everything on its own; adapters must be updated to use the new API. See followups.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19955
Summary:
Depends on D19952. Ref T13222. Never process mail targets if they match:
- The "default" address which we send mail "From".
- The "void" address which we use as a placholder "To" when we only have "CC" addresses.
- Any address from a list of reserved/administrative names.
The first two prevent loops. The third one prevents abuse.
There's a reasonably well-annotated list of reservations and reasons here:
https://webmasters.stackexchange.com/questions/104811/is-there-any-list-of-email-addresses-reserved-because-of-security-concerns-for-a
Stuff like `support@` seems fine; stuff like `ssladmin@` might let you get SSL certs issued for a domain you don't control.
Also, forbid users from creating application emails with these reserved addresses.
Finally, build the default and void addresses somewhat more cleverly.
Test Plan: Added unit tests, tried to configured reserved addresses, hit the default/void cases manually with `bin/mail receive-test`.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: olexiy.myronenko
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19953
Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.
**To, CC, and Multiple Receivers**
Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.
Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.
One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:
- If an address specifies a default author, it allows public email.
- If an address does not, it doesn't.
That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.
A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.
**Sender vs Author**
Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).
This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.
Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.
**Other**
This removes the "abundant receivers" error since this is no longer an error.
This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.
Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.
These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.
- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13066, T7477
Differential Revision: https://secure.phabricator.com/D19952
Summary:
Ref T13222. Ref T13231. See PHI912. I'm planning to turn MFA providers into concrete objects, so you can disable and configure them.
Currently, we only support TOTP, which doesn't require any configuration, but other provider types (like Duo or Yubikey OTP) do require some configuration (server URIs, API keys, etc). TOTP //could// also have some configuration, like "bits of entropy" or "allowed window size" or whatever, if we want.
Add concrete objects for this and standard transaction / policy / query support. These objects don't do anything interesting yet and don't actually interact with MFA, this is just skeleton code for now.
Test Plan:
{F6090444}
{F6090445}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13231, T13222
Differential Revision: https://secure.phabricator.com/D19935
Summary:
D19940 removed this file entirely, which has led to at least one user who was unsure how to proceed now that `cluster.mailers` is required for outbound mail: https://discourse.phabricator-community.org/t/invalid-argument-supplied-for-foreach-phabricatormetamtamail-php/2287
This isn't //always// a setup issue for installs that don't care about sending mail, but this at least this gives a sporting chance to users who don't follow the changelogs.
Also, I'm not sure if there's a way to use `pht()` to generate links; right now the phurl is just in plain text.
Test Plan: Removed `cluster.mailers` config; observed expected setup issue.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19964
Summary: See PHI1017. This is a trivial fix even though these burnups are headed toward a grisly fate.
Test Plan: Moused over some January datapoints, saw "1" instead of "0".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19967
Summary:
Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".
Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like `setSubject()`, `addTo()`, etc, that I would like to remove.
I'd like the API to be more like `sendMessage(PhabricatorMailExternalMessage $message)`. This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of `$this->subject = $subject;`) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)
That's a larger change, but move toward a world where we can build a concrete `$message` object for "email" or "sms".
The `PhabricatorMailEmailMessage` object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing `PhabricatorMetaMTAMail` is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.
The new flow will be something like:
- `PhabricatorMetaMTAMail` (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
- It builds a `PhabricatorMailExternalMessage`, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an `EmailMessage`. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
- It passes the `ExternalMessage` (which is a `MailMessage` or `SMSMessage` or whatever) to the adapter.
- The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.
This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling `setX()`, `setY()`, `setZ()` with each individual value.
Test Plan: Grepped for `MetaMTAAttachment`. This doesn't change any behavior yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T9141
Differential Revision: https://secure.phabricator.com/D19954
Summary:
Ref T7477. The various "create a new X via email" applications (Paste, Differential, Maniphest, etc) all have a bunch of duplicate code.
The inheritance stack here is generally a little weird. Extend these from a shared parent to reduce the number of callsites I need to change when this API is adjusted for T7477.
Test Plan: Ran unit tests. This will get more thorough testing once more pieces are in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19950
Summary:
See PHI1014. We may not have Identities if you race the import pipeline, or in some other cases which are more "bug" / "missing migration"-flavored.
Load the commit data so we can fall back to it if we don't have identities.
Test Plan:
- Wiped out all my identities with `UPDATE ... SET authorIdentityPHID = NULL WHERE ...`.
- Before change: blame fataled with `Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached.`.
- After change: blame falls back gracefully.
- Restored identities with `bin/repository rebuild-identities`, checked blame again.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19958
Summary:
Ref T7477. This option was added in D842 in 2011, to support a specific narrow use case at Quora with community moderators using some kind of weird Gmail config.
I don't recall it ever coming up since then, and a survey of a subset of hosted instances (see T11760) reveals that no instances are using this option today. Presumably, even Quora has completed the onboarding discussed in D842, if they still use Phabricator. This option generally does not seem very useful outside of very unusual/narrow cases like the one Quora had.
This would be relatively easy to restore as a local patch if installs //do// need it, but I suspect this has no use cases anywhere.
Test Plan: Grepped for option, blame-delved to figure out why we added it in the first place, surveyed instances for usage.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19949
Summary:
Ref T7477. We have some address normalization code in the reciever stack that is really shared code. I want to introduce some new callsites elsewhere but don't want to put a lot of static calls to other random objects all over the place.
This technically "solves" T7477 (it changes "to" to "to + cc" for finding receivers) but doesn't yet implement proper behavior (with multiple receivers, for example).
Test Plan: Ran unit tests, which cover this pretty well. Additional changes will vet this more thoroughly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19948
Summary:
Ref T920. Ref T7477. We currently drop empty mail only once it reaches the `ReplyHandler` layer.
I think no plausible receiver can ever do anything useful with this kind of mail, so we can safely drop it earlier and simplify some of the logic. After T7477, we'd end up throwing multiple exceptions if you sent empty mail to several valid receivers.
(I also want to move away from APIs oriented around raw addresses in more specialized layers, and this is one of the few callsites for raw mail address information.)
This requires updating some unit tests to actually have message bodies, since they failed with this error before hitting the other errors otherwise.
Test Plan: Used `bin/mail receive-test` to send empty mail, got appropriate "err:empty" out of it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477, T920
Differential Revision: https://secure.phabricator.com/D19947
Summary:
Ref T12509.
- Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
- Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
- Extract the CSRF logic from `PhabricatorUser` to a standalone engine.
I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.
For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.
It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).
This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.
Test Plan:
- As a logged-in user, submitted forms normally (worked).
- As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
- As a logged-out user, hit the success and error cases.
- Visually inspected tokens for correct format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19946
Summary:
Ref T12509.
- Upgrade an old SHA1 to SHA256.
- Replace an old manually configurable HMAC key with an automatically generated one.
This is generally both simpler (less configuration) and more secure (you now get a unique value automatically).
This causes a one-time compatibility break that invalidates old "Reply-To" addresses. I'll note this in the changelog.
If you leaked a bunch of addresses, you could force a change here by mucking around with `phabricator_auth.auth_hmackey`, but AFAIK no one has ever used this value to react to any sort of security issue.
(I'll note the possibility that we might want to provide/document this "manually force HMAC keys to regenerate" stuff some day in T6994.)
Test Plan: Grepped for removed config. I'll vet this pathway more heavily in upcoming changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19945
Summary:
Ref T920. This simplifies mail configuration.
The "metamta.domain" option is only used to generate Thread-ID values, and we just need something that looks like a bit like a domain in order to make GMail happy. Just use the install domain. In most cases, this is almost certainly the configured value anyway. In some cases, this may cause a one-time threading break for existing threads; I'll call this out in the changelog.
The "metamta.placeholder-to-recipient" is used to put some null value in "To:" when a mail only has CCs. This is so that if you write a local client mail rule like "when I'm in CC, burn the message in a fire" it works even if all the "to" addresses have elected not to receive the mail. Instead: just send it to an unlikely address at our own domain.
I'll add some additional handling for the possiblity that we may receive this email ourselves in the next change, but it overlaps with T7477.
Test Plan: Grepped for these configuration values.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19942
Summary:
Ref T12509. This upgrades a `weakDigest()` callsite to SHA256-HMAC and removes three config options:
- `celerity.resource-hash`: Now hard-coded, since the use case for ever adjusting it was very weak.
- `celerity.enable-deflate`: Intended to make cache inspection easier, but we haven't needed to inspect caches in ~forever.
- `celerity.minify`: Intended to make debugging minification easier, but we haven't needed to debug this in ~forever.
In the latter two cases, the options were purely developer-focused, and it's easy to go add an `&& false` somewhere in the code if we need to disable these features to debug something, but the relevant parts of the code basically work properly and never need debugging. These options were excessively paranoid, based on the static resource enviroment at Facebook being far more perilous.
The first case theoretically had end-user utility for fixing stuck content caches. In modern Phabricator, it's not intuitive that you'd go adjust a Config option to fix this. I don't recall any users ever actually running into problems here, though.
(An earlier version of this change did more magic with `celerity.resource-hash`, but this ended up with a more substantial simplification.)
Test Plan: Grepped for removed configuration options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19941
Summary: Ref T920. We currently embed the Twilio PHP API, but can replace it with about 100 lines of code and get a future-oriented interface as a bonus. Add a Future so we can move toward a simpler calling convention for the API.
Test Plan: Used this future to send SMS messages via the Twilio API.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19937
Summary:
Ref T13222. In D19918, I refactored how timelines get "view data". Today, this is always additional data about which images/changesets/diffs are visible on the current revision/commit/mock, so we can tell if inline comments should be linked to a `#anchor` on the same page (if the inline is rendered there somewhere) or to a `/D123?id=1&vs=2` full link on a different page (if it isn't), but in general this could be any sort of state information about the current page that affects how the timeline should render.
Previously, comment previews did not use any specialized object code and always rendered a "generic" timeline story. This was actually a bug, but none of the code we have today cares about this (since it's all inline related, and inlines render separately) so it never impacted anything.
After the `TimelineEngine` change, the preview renders with Differential-specific code. This is more correct, but we were not passing the preview the "view data" so it broke.
This preview doesn't actually need the view data and we could just make it bail out if it isn't present, but pass it through for consistency and so this works like we'd expect if we do something fancier with view data in the future.
Test Plan: Viewed comment and inline comment previews in Differential, saw old behavior restored.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D19943
Summary:
Ref T920. About a year ago (in 2018 Week 6, see D19003) we moved from individually configured mailers to `cluster.mailers`, primarily to support fallback across multiple mail providers.
Since this has been stable for quite a while, drop support for the older options.
Test Plan: Grepped for all removed options.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19940
Summary:
Ref T920. Over time, mail has become much more complex and I think considering "mail", "sms", "postcards", "whatsapp", etc., to be mostly-the-same is now a more promising avenue than building separate stacks for each one.
Throw away all the standalone SMS code, including the Twilio config options. I have a separate diff that adds Twilio as a mail adapter and functions correctly, but it needs some more work to bring upstream.
This permanently destroys the `sms` table, which no real reachable code ever wrote to. I'll call this out in the changelog.
Test Plan:
- Grepped for `SMS` and `Twilio`.
- Ran storage upgrade.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19939
Summary: See T12447 for discussion. It is reasonably intuitive to try and pass Conduit parameters via a JSON-encoded HTTP body, but if you do so, you'll get an unhelpful messsage about how method so-and-so does not accept a parameter named "your_entire_json_body". Instead, detect this mistake and advise developers to use form-encoded parameters.
Test Plan:
Got a better error when attempting to make Conduit calls from React code. Tested the following additional invocations of Conduit and got the expected results without an error:
* From the Conduit UI
* With cURL:
```
~ $ curl http://local.phacility.com:8080/api/conpherence.querythread \
> -d api.token=api-tvv2zb565zrtueab5ddprmpxvrwb \
> -d ids[0]=1
```
* With `arc call-conduit`:
```
~ $ echo '{
> "ids": [
> 1
> ]
> }' | arc call-conduit --conduit-uri http://local.phacility.com:8080/ --conduit-token api-tvv2zb565zrtueab5ddprmpxvrwb conpherence.querythread
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19944
Summary: Ref T920. Bumped into this while looking at SMS support.
Test Plan: Loaded `/mail/`, no more `qsprintf()` warning.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D19936
