Summary:
Depends on D21022. Ref T13493. The JIRA API has changed from using "key" to identify users to using "accountId".
By reading both identifiers, this linkage "just works" if you run against an old version of JIRA, a new version of JIRA, or an intermediate version of JIRA.
It also "just works" if you run old JIRA, upgrade to intermediate JIRA, everyone refreshes their link at least once, then you upgrade to new JIRA.
This is a subset of cases and does not include "sudden upgrade to new JIRA", but it's strictly better than the old behavior for all cases it covers.
Test Plan: Linked, unlinked, and logged in with JIRA. Looked at the "ExternalAccountIdentifier" table and saw a sensible value.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21023
Summary: Depends on D21019. Ref T13493. There are no more barriers to removing readers and writers of "accountID"; the new "ExternalAccountIdentity" table can replace it completely.
Test Plan: Linked and unlinked OAuth accounts, logged in with OAuth accounts, tried to double-link OAuth accounts, grepped for affected symbols.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21022
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.
Remove this key.
Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.
This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".
Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493, T6703
Differential Revision: https://secure.phabricator.com/D21019
Summary:
Depends on D21015. When we sync an external account and get a list of account identifiers, write them to the database.
Nothing reads them yet and we still write "accountId", this just prepares us for reads.
Test Plan: Linked, refreshed, unlinked, and re-linked an external account. Peeked at the database and saw a sensible-looking row.
Differential Revision: https://secure.phabricator.com/D21016
Summary: Depends on D21014. Ref T13493. Make these objects all use destructible interfaces and destroy sub-objects appropriately.
Test Plan:
- Used `bin/remove destroy --trace ...` to destroy a provider, a user, and an external account.
- Observed destruction of sub-objects, including external account identifiers.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21015
Summary:
Depends on D21013. Ref T13493. When users log in with most providers, the provider returns an "ExternalAccount" identifier (like an Asana account GUID) and the workflow figures out where to go from there, usually a decision to try to send the user to registration (if the external account isn't linked to anything yet) or login (if it is).
In the case of password providers, the password is really a property of an existing account, so sending the user to registration never makes sense. We can bypass the "external identifier" indirection layer and just say "username -> internal account" instead of "external GUID -> internal mapping -> internal account".
Formalize this so that "AuthProvider" can generate either a "map this external account" value or a "use this internal account" value.
This stops populating "accountID" on "password" "ExternalAccount" objects, but this was only an artifact of convenience. (These records don't really need to exist at all, but there's little harm in going down the same workflow as everything else for consistency.)
Test Plan: Logged in with a username/password. Wiped the external account table and repeated the process.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21014
Summary:
Depends on D21012. Ref T13493. Currently, auth adapters return a single identifier for each external account.
Allow them to return more than one identifier, to better handle cases where an API changes from providing a lower-quality identifier to a higher-quality identifier.
On its own, this change doesn't change any user-facing behavior.
Test Plan: Linked and unlinked external accounts.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21013
Summary:
Ref T13493. This check was introduced in D4647, but the condition can never be reached in modern Phabricator because the table has a unique key on `<accountType, accountDomain, accountID>` -- so no row can ever exist with the same value for that tuple but a different ID.
(I'm not entirely sure if it was reachable in D4647 either.)
Test Plan: Used `SHOW CREATE TABLE` to look at keys on the table and reasoned that this block can never have any effect.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21012
Summary:
Depends on D21010. Ref T13493. External accounts may have multiple different unique identifiers, most often when v1 of the API makes a questionable choice (and provies a mutable, non-unique, or PII identifier) and v2 of the API uses an immutable, unique, random identifier.
Allow Phabricator to store multiple identifiers per external account.
Test Plan: Storage only, see followup changes.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21011
Summary:
Ref T13493. The "AuthAccountView" UI element currently exposes raw account ID values, but I'm trying to make these many-to-one.
This isn't terribly useful as-is, so get rid of it. This element could use a design refresh in general.
Test Plan: Viewed the UI element in "External Accounts".
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21010
Summary: Fixes T13485. GitHub has deprecated the "access_token" URI parameter for API authentication. Update to "Authorization: token ...".
Test Plan: Linked and unlinked a GitHub account locally.
Maniphest Tasks: T13485
Differential Revision: https://secure.phabricator.com/D20964
Summary:
Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.
An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.
(Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)
When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.
This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.
Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.
Test Plan:
- As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
- As a user with MFA, renamed a user. Got badged stories in both cases.
Maniphest Tasks: T13475
Differential Revision: https://secure.phabricator.com/D20958
Summary:
Ref T13454. Fixes T13006. When a user provide us with an SSH private key and (possibly) a passphrase:
# Try to verify that they're correct by extracting the public key.
# If that fails, try to figure out why it didn't work.
Our success in step (2) will vary depending on what the problem is, and we may end up falling through to a very generic error, but the outcome should generally be better than the old approach.
Previously, we had a very unsophisticated test for the text "ENCRYPTED" in the key body and questionable handling of the results: for example, providing a passphrase when a key did not require one did not raise an error.
Test Plan:
Created and edited credentials with:
- Valid, passphrase-free keys.
- Valid, passphrased keys with the right passphrase.
- Valid, passphrase-free keys with a passphrase ("surplus passphrase" error).
- Valid, passphrased keys with no passphrase ("missing passphrase" error).
- Valid, passphrased keys with an invalid passphrase ("invalid passphrase" error).
- Invalid keys ("format" error).
The precision of these errors will vary depending on how helpful "ssh-keygen" is.
Maniphest Tasks: T13454, T13006
Differential Revision: https://secure.phabricator.com/D20905
Summary: Ref T13453. The Asana API has changed, replacing all "id" fields with "gid", including the "users/me" API call result.
Test Plan: Linked an Asana account. Before: error about missing 'id'. After: clean link.
Maniphest Tasks: T13453
Differential Revision: https://secure.phabricator.com/D20899
Summary:
Fixes T13433. Currently, "Login Screen Instructions" in "Auth" are shown only on the main login screen. If you enter a bad password or bad LDAP credential set and move to the flow-specific login failure screen (for example, "invalid password"), the instructions vanish.
Instead, persist them. There are reasonable cases where this is highly useful and the cases which spring to mind where this is possibly misleading are fairly easy to fix by making the instructions more specific.
Test Plan:
- Configured login instructions in "Auth".
- Viewed main login screen, saw instructions.
- Entered a bad username/password and a bad LDAP credential set, got kicked to workflow sub-pages and still saw instructions (previously: no instructions).
- Grepped for other callers to `buildProviderPageResponse()` to look for anything weird, came up empty.
Maniphest Tasks: T13433
Differential Revision: https://secure.phabricator.com/D20863
Summary: Fixes T13420. Allow installs to provide username change instructions if there's someone you should contact to get this done.
Test Plan: {F6885027}
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20828
Summary: Fixes T13406. On the logout screen, test for no configured providers and warn users they may be getting into more trouble than they expect.
Test Plan:
- Logged out of a normal install and a fresh (unconfigured) install.
{F6847659}
Maniphest Tasks: T13406
Differential Revision: https://secure.phabricator.com/D20789
Summary: See downstream <https://phabricator.wikimedia.org/T229757>. The "autofocus" attribute mostly just works, so add it to this input.
Test Plan: As a user with TOTP enabled, established a new session. Saw browser automatically focus the "App Code" input on the TOTP prompt screen.
Differential Revision: https://secure.phabricator.com/D20703
Summary:
Depends on D20672. Ref T13343. When a user requests an account access link via email:
- log it in the activity log; and
- reference the log in the mail.
This makes it easier to ban users misusing the feature, provided they're coming from a single remote address, and takes a few steps down the pathway toward a button in the mail that users can click to report the action, suspend account recovery for their account, etc.
Test Plan:
- Requested an email recovery link.
- Saw request appear in the user activity log.
- Saw a reference to the log entry in the mail footer.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20673
Summary:
Depends on D20670. Ref T13343. The user activity message log types are currently hard-coded, so only upstream code can really use the log construct.
Under the theory that we're going to keep this log around going forward (just focus it a little bit), modularize things so the log is extensible.
Test Plan:
Grepped for `UserLog::`, viewed activity logs in People and Settings.
(If I missed something here -- say, misspelled a constant -- the effect should just be that older logs don't get a human-readable label, so stakes are very low.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20671
Summary: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.
Test Plan: `grep` for `getActionConstant()`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20669
Summary:
Depends on D20667. Ref T13343. Password auth currently uses an older rate limiting mechanism, upgrade it to the modern "SystemAction" mechanism.
This mostly just improves consistency, although there are some tangential/theoretical benefits:
- it's not obvious that making the user log GC very quickly could disable rate limiting;
- if we let you configure action limits in the future, which we might, this would become configurable for free.
Test Plan:
- With CAPTCHAs off, made a bunch of invalid login attempts. Got rate limited.
- With CAPTCHAs on, made a bunch of invalid login attempts. Got downgraded to CAPTCHAs after a few.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20668
Summary:
Depends on D20666. Ref T13343. In D20666, I limited the rate at which a given user account can be sent account recovery links.
Here, add a companion limit to the rate at which a given remote address may request recovery of any account. This limit is a little more forgiving since reasonable users may plausibly try multiple variations of several email addresses, make typos, etc. The goal is just to hinder attackers from fishing for every address under the sun on installs with no CAPTCHA configured and no broad-spectrum VPN-style access controls.
Test Plan: {F6607846}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20667
Summary:
Depends on D20665. Ref T13343. We support CAPTCHAs on the "Forgot password?" flow, but not everyone configures them (or necessarily should, since ReCAPTCHA is a huge external dependency run by Google that requires you allow Google to execute JS on your domain) and the rate at which any reasonable user needs to take this action is very low.
Put a limit on the rate at which account recovery links may be generated for a particular account, so the worst case is a trickle of annoyance rather than a flood of nonsense.
Test Plan: {F6607794}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20666
Summary:
Depends on D20664. Ref T13343. There's a reasonable value for the default "Email Login" auth message (generic "you reset your password" text) that installs may reasonably want to replace. Add support for a default value.
Also, since it isn't completely obvious where this message shows up, add support for an extended description and explain what's going on in more detail.
Test Plan:
- Viewed message detail page, saw more detailed information.
- Sent mail (got default), overrode message and sent mail (got custom message), deleted message (got default again).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20665
Summary:
Depends on D20663. Ref T13343. Currently, if an Auth message hasn't been customized yet, clicking the message type takes you straight to an edit screen to create a message.
If an auth message has already been customized, you go to a detail screen instead.
Since there's no detail screen on the "create for the first time" flow, we don't have anywhere to put a more detailed description or a preview of a default value.
Add a view screen that works if a message is "empty" so we can add this stuff.
(The only reason we don't already have this is that it took a little work to build; this also generally improves the consistency and predictability of this interface.)
Test Plan: {F6607665}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20664
Summary:
Depends on D20662. Ref T13343. Installs may reasonably want to change the guidance users receive in "Email Login"/"Forgot Password" email.
(In an upcoming change I plan to supply a piece of default guidance, but Auth Messages need a few tweaks for this.)
There's probably little reason to provide guidance on the "Set Password" flow, but any guidance one might issue on the "Email Login" flow probably doesn't make sense on the "Set Password" flow, so I've included it mostly to make it clear that this is a different flow from a user perspective.
Test Plan:
- Set custom "Email Login" and "Set Password" messages.
- Generated "Email Login" mail by using the "Login via email" link on the login screen.
- Generated "Set Password" email by trying to set a password on an account with no password yet.
- Saw my custom messages in the resulting mail bodies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20663
Summary:
Ref T13343. This makes "Password Reset" email a little more consistent with other modern types of email. My expectation is that this patch has no functional changes, just organizes code a little more consistently.
The new `setRecipientAddress()` mechanism deals with the case where the user types a secondary (but still verified) address.
Test Plan:
- Sent a normal "login with email" email.
- Sent a "login with email to set password" email by trying to set a password on an account with no password yet.
- Tried to email reset a bot account (no dice: they can't do web logins so this operation isn't valid).
- Tested existing "PeopleMailEngine" subclasses:
- Created a new user and sent a "welcome" email.
- Renamed a user and sent a "username changed" email.
- Reviewed all generated mail with `bin/mail list-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20662
Summary: I was poking around in `PhabricatorAuthProviderViewController` and noticed that none of the subclass-specific rendering was working. Figured out that no one ever calls `PhabricatorAuthProviderConfigTransaction->setProvider()`, so instead of adding all those calls, just pull the provider out of the config object.
Test Plan:
Before: {F6598145}
After: {F6598147}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20655
Summary: Forgot to post this after D20394. Fixes T7667.
Test Plan:
* Edited some providers with the config locked and unlocked.
* Opened the edit form with the config unlocked, locked the config, then saved, and got a sensible error: {F6576023}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20645
Summary:
Ref D20645. Start making this view a little more useful:
{F6573605}
Test Plan: Mk. 1 eyeball
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20646
Summary:
Ref T13291. See PHI1312. Currently, if you link to a JIRA or Asana issue with an anchor (`#asdf`) or query parameters (`?a=b`), we:
- treat the link as an external object reference and attempt a lookup on it;
- if the lookup succeeds, we discard the fragment or parameters when re-rendering the rich link (with the issue/task title).
Particularly, the re-rendering part uses the canonical URI of the object, and can discard these parameters/fragments, which is broken/bad.
As a first pass at improving this, just don't apply special behavior for links with anchors or parameters -- simply treat them as links.
In some future change, we could specialize this behavior and permit certain known parameters or anchors or something, but these use cases are likely fairly marginal.
Test Plan:
Before:
{F6516392}
After:
{F6516393}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20592
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes.
These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`.
Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20587
Summary:
Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support.
Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out.
For users who are already logged out, this just redirects home with no effect.
Test Plan:
- As a user with access to no Spaces, logged out. (Before: error; after: worked).
- As a logged-out user, logged out (was redirected).
- As a normal user, logged out (normal logout).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13310
Differential Revision: https://secure.phabricator.com/D20578
Summary:
Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT".
Since the actions like "Accept Revision" already work like this, the fix is one line.
Test Plan: {F6488135}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13307
Differential Revision: https://secure.phabricator.com/D20574
Summary:
Depends on D20527. Ref T13291. Now that we have more flexible support for URI rewriting, use it for Doorkeeper URIs.
These are used when you set up Asana or JIRA and include the URI to an Asana task or a JIRA issue in a comment.
Test Plan:
- Linked up to Asana and JIRA.
- Put Asana and JIRA URIs in comments.
- Saw the UI update to pull task titles from Asana / JIRA using my OAuth credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20528
Summary:
See PHI1229. An install has a somewhat duct-taped registration flow which can dump users on the "Wait for Approval" screen without clear guidance. The desired guidance is something like "this is totally normal, just wait a bit for a bot to approve you".
Adding guidance here is generally reasonable and consistent with the intent of this feature.
Test Plan: {F6426583}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: kylec
Differential Revision: https://secure.phabricator.com/D20492
Summary:
If you edit an auth message in Auth > Customize Messages, then click "Show Details" in the transaction record, the resulting dialog uses the object's handle's URI to generate a "cancel" button.
Since these handles currently have no URI, the dialog currently has no cancel/done button to close it.
Test Plan: Edited an auth message, clicked "Show Details", was now able to click "Done" to close the dialog.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20471
Summary:
Ref T7667. On the road to locking the auth config, also clean up some minor UI issues:
* Only show the warning about not Phacility instance auth if the user isn't a manager (see next diff).
* When rendering more than one warning in the guidance, add bullets.
* I didn't like the text in the `auth.config-lock` config setting.
Test Plan: Loaded the page, saw more reasonable-looking guidance: {F6369405}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20400
Summary:
Depends on D20407. Ref T13272. This updates the "add panel" (which has two flavors: "add existing" and "create new") and "remove panel" flows to work with the new duplicate-friendly storage format.
- We now modify panels by "panelKey", not by panel PHID, so one dashboard may have multiple copies of the same panel and we can still figure out what's going on.
- We now work with "contextPHID", not "dashboardID", to make some flows with tab panels (or other nested panels in the future) easier.
The only major remaining flow is the Javascript "move panels around with drag-and-drop" flow.
Test Plan:
- Added panels to a dashboard with "Create New Panel".
- Added panels to a dashboard with "Add Existing Panel".
- Removed panels from a dashboard.
- Added and removed duplicate panels, got a correctly-functioning dashboard that didn't care about duplicates.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20408
Summary:
Ref T7667. Adds new flows `bin/auth lock` and `bin/auth unlock` to prevent compromised administrator accounts from doing additional damage by altering the authentication provider configuration.
Note that this currently doesn't actually do anything because we aren't checking this config key in any of the edit controllers yet.
Test Plan: Ran `lock` and `unlock`, checked for correct DB state, observed expected setup warning.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20394
Summary:
See <https://discourse.phabricator-community.org/t/duo-broken-in-2019-week-12/2580/>.
The "live update Duo status" endpoint currently requires full sessions, and doesn't work from the session upgrade gate on login.
Don't require a full session to check the status of an MFA challenge.
Test Plan: Went through Duo gate in a new session, got a live update.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20347
Summary:
Ref T13259. If we miss the separate CSRF step in Duo and proceed directly to prompting, we may fail to build a response which turns into a real control and fatal on `null->setLabel()`.
Instead, let MFA providers customize their "bare prompt dialog" response, then make Duo use the same "you have an outstanding request" response for the CSRF and no-CSRF workflows.
Test Plan: Hit Duo auth on a non-CSRF workflow (e.g., edit an MFA provider with Duo enabled). Previously: `setLabel()` fatal. After patch: smooth sailing.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20234
Summary: Fixes T13254. See that task for details.
Test Plan: Used iOS Simulator to do a login locally, didn't get blocked. Verified CSP includes "m.facebook.com".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13254
Differential Revision: https://secure.phabricator.com/D20206
Summary:
Ref T13249. See PHI774. When users follow an email login link ("Forgot password?", "Send Welcome Email", "Send a login link to your email address.", `bin/auth recover`), we send them to a password reset flow if an install uses passwords.
If an install does not use passwords, we previously dumped them unceremoniously into the {nav Settings > External Accounts} UI with no real guidance about what they were supposed to do. Since D20094 we do a slightly better job here in some cases. Continue improving this workflow.
This adds a page like "Reset Password" for "Hey, You Should Probably Link An Account, Here's Some Options".
Overall, this stuff is still pretty rough in a couple of areas that I imagine addressing in the future:
- When you finish linking, we still dump you back in Settings. At least we got you to link things. But better would be to return you here and say "great job, you're a pro".
- This UI can become a weird pile of buttons in certain configs and generally looks a little unintentional. This problem is shared among all the "linkable" providers, and the non-login link flow is also weird.
So: step forward, but more work to be done.
Test Plan: {F6211115}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20170
Summary: Ref T13249. Poll for Duo updates in the background so we can automatically update the UI when the user clicks the mobile phone app button.
Test Plan: Hit a Duo gate, clicked "Approve" in the mobile app, saw the UI update immediately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20169
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20151
Summary:
Depends on D20119. Fixes T9512. When you don't have a password on your account, the "Password" panel in Settings is non-obviously useless: you can't provide an old password, so you can't change your password.
The correct remedy is to "Forgot password?" and go through the password reset flow. However, we don't guide you to this and it isn't really self-evident.
Instead:
- Guide users to the password reset flow.
- Make it work when you're already logged in.
- Skin it as a "set password" flow.
We're still requiring you to prove you own the email associated with your account. This is a pretty weak requirement, but maybe stops attackers who use the computer at the library after you do in some bizarre emergency and forget to log out? It would probably be fine to just let users "set password", this mostly just keeps us from having two different pieces of code responsible for setting passwords.
Test Plan:
- Set password as a logged-in user.
- Reset password on the normal flow as a logged-out user.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: revi
Maniphest Tasks: T9512
Differential Revision: https://secure.phabricator.com/D20120
Summary:
Depends on D20117. Fixes T10071. When you're sent an email invitation, it's intended to allow you to register an account even if you otherwise could not (see D11737).
Some time between D11737 and today, this stopped working (or perhaps it never worked and I got things wrong in D11737). I think this actually ended up not mattering for us, given the way Phacility auth was ultimately built.
This feature generally seems reasonable, though, and probably //should// work. Make it work in the "password" and "oauth" cases, at least. This may //still// not work for LDAP, but testing that is nontrivial.
Test Plan:
- Enabled only passwords, turned off registration, sent an invite, registered with a password.
- Enabled only Google OAuth, turned off registration, sent an invite, registered with Google OAuth.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T10071
Differential Revision: https://secure.phabricator.com/D20118
Summary:
Depends on D20113. Ref T6703. Continue moving toward a future where multiple copies of a given type of provider may exist.
Switch MFA from session-MFA at the start to one-shot MFA at the actual link action.
Add one-shot MFA to the unlink action. This theoretically prevents an attacker from unlinking an account while you're getting coffee, registering `alIce` which they control, adding a copy of your profile picture, and then trying to trick you into writing a private note with your personal secrets or something.
Test Plan: Linked and unlinked accounts. Refreshed account. Unlinked, then registered a new account. Unlinked, then relinked to my old account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20117
Summary: Depends on D20112. Ref T6703. When you go to unlink an account, unlink it by ID. Crazy!
Test Plan: Unlinked and relinked Google accounts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20113
Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.
Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.
(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)
Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20112
Summary:
Ref T6703. Currently, when you create an account on a new install, we prompt you to select a password.
You can't actually use that password unless you set up a password provider, and that password can't be associated with a provider since a password provider won't exist yet.
Instead, just don't ask for a password: create an account with a username and an email address only. Setup guidance points you toward Auth.
If you lose the session, you can send yourself an email link (if email works yet) or `bin/auth recover` it. This isn't really much different than the pre-change behavior, since you can't use the password you set anyway until you configure password auth.
This also makes fixing T9512 more important, which I'll do in a followup. I also plan to add slightly better guideposts toward Auth.
Test Plan: Hit first-time setup, created an account.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: revi
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20111
Summary: 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 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 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 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: 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:
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: 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: 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 D20038. Ref T13231. Although I planned to keep this out of the upstream (see T13229) it ended up having enough pieces that I imagine it may need more fixes/updates than we can reasonably manage by copy/pasting stuff around. Until T5055, we don't really have good tools for managing this. Make my life easier by just upstreaming this.
Test Plan: See T13231 for a bunch of workflow discussion.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13231
Differential Revision: https://secure.phabricator.com/D20039
Summary: Depends on D20037. Ref T13222. Ref T7667. Although administrators can now disable MFA from the web UI, at least require that they survive MFA gates to do so. T7667 (`bin/auth lock`) should provide a sturdier approach here in the long term.
Test Plan: Created and edited MFA providers, was prompted for MFA.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222, T7667
Differential Revision: https://secure.phabricator.com/D20038
Summary: Depends on D20034. Ref T13222. This is just cleanup -- I thought we'd have like two of these, but we ended up having a whole lot in Duo and a decent number in SMS. Just let factors return a result explicitly if they can make a decision early. I think using `instanceof` for control flow is a lesser evil than using `catch`, on the balance.
Test Plan: `grep`, went through enroll/gate flows on SMS and Duo.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20035
Summary:
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: 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 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:
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:
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
