1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

16438 commits

Author SHA1 Message Date
epriestley
9632c704c6 Always allow users to login via email link, even if an install does not use passwords
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
2019-02-05 16:00:55 -08:00
epriestley
99e5ef84fc Remove obsolete "PhabricatorAuthLoginHandler"
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
2019-02-05 14:20:14 -08:00
epriestley
4fcb38a2a9 Move the Auth Provider edit flow toward a more modern layout
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
2019-02-05 14:19:26 -08:00
epriestley
8c8d56dc56 Replace "Add Auth Provider" radio buttons with a more modern "click to select" UI
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
2019-02-05 14:18:16 -08:00
epriestley
6f3bd13cf5 Begin adding more guidance to the "One-Time Login" flow
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
2019-02-05 14:17:25 -08:00
epriestley
03eb989fd8 Give Duo MFA a stronger hint if users continue without answering the challenge
Summary: See PHI912. Also, clean up some leftover copy/pastey code here.

Test Plan: {F6182333}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20088
2019-02-05 14:14:41 -08:00
epriestley
ab467d52f4 Improve feed rendering of user rename story
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
2019-02-05 14:13:44 -08:00
epriestley
5dc6502293 Make the mobile menu available in "/mail/"
Summary: Ref T13244. See <https://discourse.phabricator-community.org/t/left-hand-menu-not-responsive-on-mobile/2358>.

Test Plan: {F6184160}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20093
2019-02-05 14:10:57 -08:00
epriestley
a58c9d50b2 Slightly update the Diviner documentation
Summary: See PHI1050. Although Diviner hasn't received a ton of new development for a while, it's: not exaclty new; and pretty useful for what we need it for.

Test Plan: Reading.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: leoluk

Differential Revision: https://secure.phabricator.com/D20086
2019-02-05 14:09:43 -08:00
epriestley
ee24eb60b7 In Owners Packages, make the API representation of the "Auditing" field more consistent
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
2019-02-05 14:07:23 -08:00
epriestley
4675306615 Add a "metronome" for spreading service call load
Summary:
Ref T13244. See D20080. Rather than randomly jittering service calls, we can give each host a "metronome" that ticks every 60 seconds to get load to spread out after one cycle.

For example, web001 ticks (and makes a service call) when the second hand points at 0:17, web002 at 0:43, web003 at 0:04, etc.

For now I'm just planning to seed the metronomes randomly based on hostname, but we could conceivably give each host an assigned offset some day if we want perfectly smooth service call rates.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20087
2019-02-05 14:06:22 -08:00
Leopold Schabel
b8fe991ba2 Improve description text in the "Create Diff" form
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
2019-02-05 13:47:53 +00:00
epriestley
db1e123706 Fix an issue where Duo validation could incorrectly apply to other factor types
See <https://discourse.phabricator-community.org/t/configuring-mfa-provider-totp-fails-for-missing-duo-only-options/2355>.

Test Plan: Created a TOTP provider; created a Duo provider (with missing and supplied values).
2019-02-03 06:36:49 -08:00
epriestley
7c795ab757 Let omnipotent actors skip MFA transactions
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
2019-02-01 13:32:09 -08:00
epriestley
942d6d60d5 Don't load unnecessary handle data on "transaction.search"
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
2019-02-01 08:14:26 -08:00
epriestley
7dd55a728f Make repository daemons periodically check for out-of-sync repositories
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
2019-01-31 22:12:39 -08:00
epriestley
f3e154eb02 Allow "inactive" repositories to be read over SSH for cluster sync
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
2019-01-31 22:12:13 -08:00
epriestley
b3e4743ad7 Read POST data sightly earlier in request startup
Summary:
On instances, the "SiteSource" (for site config) pretty much copy-pastes the "read POST data" block because it needs to make some decisions based on POST data when handling inbound mail webhooks.

Move the upstream read a little earlier so we can get rid of this. Now that this step is separated and must happen before the profiler, there's no reason not to do it earlier.

Test Plan: POSTed some data across pages without issue, will remove duplicate code in upcoming change.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20073
2019-01-31 22:11:42 -08:00
epriestley
bc61d09f55 Allow parent and child revisions to be modified via Conduit
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
2019-01-31 22:11:17 -08:00
epriestley
b33333ab8e Fix method name typo in new modular transaction text/html mail methods
See PHI1039. See D20057.
2019-01-31 08:52:27 -08:00
epriestley
138c07f32c Allow modular transactions to override transaction title and body text in mail
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
2019-01-30 19:48:30 -08:00
epriestley
87b0ef8839 Remove "iconv" PHP extension dependency
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
2019-01-30 19:46:58 -08:00
epriestley
48a3760814 Correct a bug where milestone "spacePHID" columns could become desynchronized
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
2019-01-30 19:41:49 -08:00
epriestley
e9b2d667ee Improve handling of "Deny" responses from Duo
Summary:
Ref T13231. See <https://discourse.phabricator-community.org/t/duo-integration-crashes-if-user-is-not-enrolled-and-enrollment-is-disabled/2340/5>

(There's an actual bug here, although I'm not sure exactly what's going on on the Duo side in the report.)

Test Plan:
To reproduce this, I was only able to actually "Deny" my account explicitly in Duo.

  - With "Deny", tried to add a factor. Got a nice helpful error message.
  - Undenied, added a factor, re-denied, tried to pass an MFA gate. Got another nice helpful error message.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231

Differential Revision: https://secure.phabricator.com/D20065
2019-01-30 19:33:15 -08:00
epriestley
93b512b63c Update a factor query in TransactionEditor for providers
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
2019-01-30 19:27:37 -08:00
epriestley
612e9c6e09 Hide "Signed with MFA" stories from feed
Summary:
See <https://discourse.phabricator-community.org/t/sign-with-mfa-action-on-differential-revisions-creates-strange-feed-entries/2346>.

If you "Sign with MFA" and only add a comment, we can produce a weird notification and feed story. Hide these stories from feed since they're broadly not interesting.

Test Plan:
  - Used "sign with MFA" and only posted a comment.
  - Observed feed.
    - Before: Janky "untitled story".
    - After: Sensible "added a comment" story.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20071
2019-01-30 19:26:33 -08:00
epriestley
6bcfc212eb Allow diff change detection to complete for Mercurial changes which remove a binary file
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
2019-01-30 19:17:03 -08:00
epriestley
1767b80654 Replace manual query string construction with "phutil_build_http_querystring()"
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
2019-01-30 19:14:57 -08:00
epriestley
a24e6deb57 Read "$_POST" before hooking the profiler, and remove "aphront.default-application-configuration-class"
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
2019-01-30 06:22:41 -08:00
epriestley
70b474e550 Allow MFA enrollment guidance to be customized
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
2019-01-30 06:21:58 -08:00
epriestley
2374c92544 Add a warning about MFA requirements to edit forms
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
2019-01-30 06:21:25 -08:00
epriestley
13c5b427d6 Warn users about MFA requirements when interacting with "MFA Required" objects via the comment form
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
2019-01-30 06:20:52 -08:00
epriestley
f7e8fa0764 Provide an Editor extension point for transaction validation
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
2019-01-30 06:18:41 -08:00
epriestley
c9760e8d64 Support subtypes in Projects
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
2019-01-30 06:17:55 -08:00
epriestley
13e4aeb590 Give MFA gates a more consistent UI
Summary: Depends on D20057. Currently, we show an "MFA" message on one of these and an "Error" message on the other, with different icons and colors. Use "MFA" for both, with the MFA icon / color.

Test Plan: Hit both varations, saw more consistency.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20059
2019-01-30 06:16:32 -08:00
epriestley
d254c1f8b1 Use "null", not "-1", as a local "no version" marker when performing intracluster repository sync
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
2019-01-28 18:50:01 -08:00
epriestley
9fd8343704 Bring Duo MFA upstream
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
2019-01-28 18:26:45 -08:00
epriestley
d8d4efe89e Require MFA to edit MFA providers
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
2019-01-28 09:44:39 -08:00
epriestley
44a0b3e83d Replace "Show Secret" in Passphrase with one-shot MFA
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
2019-01-28 09:44:08 -08:00
epriestley
d24e66724d Convert "Rename User" from session MFA to one-shot MFA
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
2019-01-28 09:41:10 -08:00
epriestley
29b4fad941 Get rid of "throwResult()" for control flow in MFA factors
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
2019-01-28 09:40:28 -08:00
epriestley
bce44385e1 Add more factor details to the Settings factor list
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
2019-01-28 09:40:00 -08:00
epriestley
2dd8a0fc69 Update documentation for MFA, including administrator guidance
Summary: Depends on D20032. Ref T13222.

Test Plan: Read documentation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20033
2019-01-28 09:35:02 -08:00
epriestley
50abc87363 Expand outbound mailer documentation to mention SMS and include Twilio
Summary: Depends on D20031. Ref T13222.

Test Plan: Read "carefully".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13222

Differential Revision: https://secure.phabricator.com/D20032
2019-01-28 09:32:00 -08:00
epriestley
8e5d9c6f0e Allow MFA providers to be deprecated or disabled
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
2019-01-28 09:29:27 -08:00
epriestley
03ac59a877 Don't put "spacePHID IN (...)" constraints in queries which will raise policy exceptions
Summary:
See T13240. Ref T13242. When we're issuing a query that will raise policy exceptions (i.e., give the user a "You Shall Not Pass" dialog if they can not see objects it loads), don't do space filtering in MySQL: when objects are filtered out in MySQL, we can't distinguish between "bad/invalid ID/object" and "policy filter", so we can't raise a policy exception.

This leads to cases where viewing an object shows "You Shall Not Pass" if you can't see it for any non-Spaces reason, but "404" if the reason is Spaces.

There's no product reason for this, it's just that `spacePHID IN (...)` is important for non-policy-raising queries (like a list of tasks) to reduce how much application filtering we need to do.

Test Plan:
Before:

```
$ git pull
phabricator-ssh-exec: No repository "spellbook" exists!
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
```

After:

```
$ git pull
phabricator-ssh-exec: [You Shall Not Pass: Unknown Object (Repository)] This object is in a space you do not have permission to access.
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

Maniphest Tasks: T13242

Differential Revision: https://secure.phabricator.com/D20042
2019-01-28 09:03:32 -08:00
epriestley
c9ff6ce390 Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)
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
2019-01-24 15:10:57 -08:00
epriestley
069160404f Add a Duo API future
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
2019-01-24 15:10:17 -08:00
epriestley
98c4cdc5be Make the "PHP 7" setup warning more explicit about what it means
Summary: See <https://discourse.phabricator-community.org/t/minimum-php-versions-supported-by-latest-stable-phabricator/2314/3>.

Test Plan: o~o

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20027
2019-01-24 15:09:00 -08:00
epriestley
5cfcef7f53 Fix bad "$this" references in "Must Encrypt" mail after MailEngine changes
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
2019-01-24 15:07:42 -08:00