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

17025 commits

Author SHA1 Message Date
epriestley
d22495a820 Make external link/refresh use provider IDs, switch external account MFA to one-shot
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
2019-02-12 15:18:08 -08:00
epriestley
e5ee656fff Make external account unlinking use account IDs, not "providerType + providerDomain" nonsense
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
2019-02-12 15:16:24 -08:00
epriestley
541d794c13 Give ExternalAccount a providerConfigPHID, tying it to a particular provider
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
2019-02-12 14:48:14 -08:00
epriestley
55c18bc900 During first-time setup, create an administrator account with no authentication instead of weird, detached authentication
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
2019-02-12 14:47:47 -08:00
epriestley
378a43d09c Remove the highly suspect "Import from LDAP" workflow
Summary: Depends on D20109. Ref T6703. This flow was contributed in 2012 and I'm not sure it ever worked, or at least ever worked nondestructively. For now, get rid of it. We'll do importing and external sync properly at some point (T3980, T13190).

Test Plan: Grepped for `ldap/`, grepped for controller.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20110
2019-02-12 14:45:58 -08:00
epriestley
fcd85b6d7b Replace "getRequestURI()->setQueryParams(array())" with "getPath()"
Summary:
Ref T13250. A handful of callsites are doing `getRequestURI()` + `setQueryParams(array())` to get a bare request path.

They can just use `getPath()` instead.

Test Plan: See inlines.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20150
2019-02-12 14:43:33 -08:00
epriestley
00ffb190cc In Webhooks, label HTTP response codes as "HTTP Status Code", not "HTTP Error"
Summary: See PHI1068. We currently show "HTTP Error - 200", which is misleading. Instead, label these results as "HTTP Status Code".

Test Plan: {F6206016}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20148
2019-02-12 14:41:10 -08:00
epriestley
308c4f2407 Fix "AphrontRequest->getRequestURI()" for requests with "x[]=1" parameters in the URI
Summary:
Ref T13250. See PHI1069. This is a small fix for `getRequestURI()` currently not working if the request includes "x[]=..." PHP-flavored array parameters, beacause they're parsed into arrays by `$_GET` and `setQueryParams(...)` no longer accepts nonscalars.

Instead, just parse the raw request URI.

Test Plan: Visited `/search/hovercard/?phids[]=X`, no more fatal. Dumped the resulting URI, saw it had the right value. Tried `?phids[]=x&x=1&x=1&x=1`, saw the parameters correctly preserved.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20147
2019-02-12 11:03:43 -08:00
epriestley
1fd69f788c Replace "getQueryParams()" callsites in Phabricator
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
2019-02-12 06:37:03 -08:00
epriestley
187356fea5 Let the top-level exception handler dump a stack trace if we reach debug mode before things go sideways
Summary:
Depends on D20140. Ref T13250. Currently, the top-level exception handler doesn't dump stacks because we might not be in debug mode, and we might double-extra-super fatal if we call `PhabricatorEnv:...` to try to figure out if we're in debug mode or not.

We can get around this by setting a flag on the Sink once we're able to confirm that we're in debug mode. Then it's okay for the top-level error handler to show traces.

There's still some small possibility that showing a trace could make us double-super-fatal since we have to call a little more code, but AphrontStackTraceView is pretty conservative about what it does and 99% of the time this is a huge improvement.

Test Plan: {F6205122}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20142
2019-02-11 15:36:19 -08:00
epriestley
51cca22d07 Support EU domains for Mailgun API
Summary:
See <https://discourse.phabricator-community.org/t/mailgun-eu-zone-not-working/2332/4>.

Mailgun has a couple of API domains depending on where your account is based.

Test Plan:
  - Sent normal mail successfully with my non-EU account.
  - Waiting on user confirmation that this works in the EU.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20055
2019-02-11 15:06:04 -08:00
epriestley
b9a1260ef5 Improve top-level fatal exception handling in PHP 7+
Summary:
Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to `Throwables`, a superclass of `Exception`.

This is generally a good change, but code written against PHP 5.x before `Throwable` was introduced may not catch these errors, even when the code is intended to be a top-level exception handler.

(The double-catch pattern here and elsewhere is because `Throwable` does not exist in older PHP, so `catch (Throwable $ex)` catches nothing. The `Exception $ex` clause catches everything in old PHP, the `Throwable $ex` clause catches everything in newer PHP.)

Generalize some `Exception` into `Throwable`.

Test Plan:
  - Added a bogus function call to the rendering stack.
  - Before change: got a blank page.
  - After change: nice exception page.

{F6205012}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250, T12101

Differential Revision: https://secure.phabricator.com/D20138
2019-02-11 15:05:15 -08:00
epriestley
e03079aaaa Try harder to present display/rendering exceptions to the user using standard exception handling
Summary:
Ref T13250. When exceptions occur in display/rendering/writing, they currently go straight to the fallback handler. This is a minimal handler which doesn't show a stack trace or include any debugging details.

In some cases, we have to do this: some of these exceptions prevent us from building a normal page. For example, if the menu bar has a hard fatal in it, we aren't going to be able to build a nice exception page with a menu bar no matter how hard we try.

However, in many cases the error is mundane: something detected something invalid and raised an exception during rendering. In these cases there's no problem with the page chrome or the rendering pathway itself, just with rendering the page data.

When we get a rendering/response exception, try a second time to build a nice normal exception page. This will often work. If it doesn't work, fall back as before.

Test Plan:
  - Forced the error from T13250 by applying D20136 but not D20134.
  - Before:

{F6205001}

  - After:

{F6205002}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20137
2019-02-11 14:40:52 -08:00
epriestley
1275326ea6 Use "phutil_string_cast()" in TypeaheadDatasource
Summary:
Depends on D20138. Ref T13250. This improves exception behavior and gives us a standard page with a stack trace instead of a text fatal with no stack trace.

Truly a great day for PHP.

(Eventually we may want to replace all `(string)` with `phutil_string_cast()`, but let's let it have some time in the wild first?)

Test Plan: Triggered the error, got a more useful exception behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20140
2019-02-11 14:39:12 -08:00
epriestley
77247084bd Fix inverted check in audit triggers for "uninvolved owner"
Summary: See D20126. I was trying to be a little too cute here with the names and ended up confusing myself, then just tested the method behavior. :/

Test Plan: Persudaded by arguments in D20126.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20135
2019-02-11 14:08:04 -08:00
epriestley
711871f6bc Allow typeaheads to pass nonscalar data to datasources
Summary:
Ref T13250. Currently, datasources have a `setParameters(...)` method. This method accepts a dictionary and adds the key/value pairs to the raw HTTP request to the datasource endpoint.

Since D20049, this no longer works. Since D20116, it fatals explicitly.

In general, the datasource endpoint accepts other values (like `query`, `offset`, and `limit`), and even before these changes, using secret reserved keys in `setParameters(...)` would silently cause program misbehavior.

To deal with this, pass parameters as a JSON string named "parameters". This fixes the HTTP query issue (the more pressing issue affecting users today) and prevents the "shadowing reserved keys" issue (a theoretical issue which might affect users some day).

(I may revisit the `phutil_build_http_querystring()` behavior and possibly let it make this work again, but I think avoiding the duplicate key issue makes this change desirable even if the querystring behavior changes.)

Test Plan:
  - Used "Land Revision", selected branches.
  - Configured a custom Maniphest "users" field, used the search typeahead, selected users.
  - Manually browsed to `/typeahead/class/PhabricatorPeopleDatasource/?query=hi&parameters=xyz` to see the JSON decode exception.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20134
2019-02-11 10:53:39 -08:00
epriestley
a20f108034 When an edit overrides an object lock, note it in the transaction record
Summary:
Ref T13244. See PHI1059. When you lock a task, users who can edit the task can currently override the lock by using "Edit Task" if they confirm that they want to do this.

Mark these edits with an emblem, similar to the "MFA" and "Silent" emblems, so it's clear that they may have bent the rules.

Also, make the "MFA" and "Silent" emblems more easily visible.

Test Plan:
Edited a locked task, overrode the lock, got marked for it.

{F6195005}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aeiser

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20131
2019-02-09 06:10:07 -08:00
epriestley
2b718d78bb Improve UI/UX when users try to add an invalid card with Stripe
Summary: Ref T13244. See PHI1052. Our error handling for Stripe errors isn't great right now. We can give users a bit more information, and a less jarring UI.

Test Plan:
Before (this is in developer mode, production doesn't get a stack trace):

{F6197394}

After:

{F6197397}

- Tried all the invalid test codes listed here: https://stripe.com/docs/testing#cards

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20132
2019-02-09 05:54:42 -08:00
epriestley
ae54af32c1 When an Owners package accepts a revision, count that as an "involved owner" for the purposes of audit
Summary:
Depends on D20129. Ref T13244. See PHI1058. When a revision has an "Accept" from a package, count the owners as "involved" in the change whether or not any actual human owners are actually accepting reviewers.

If a user owns "/" and uses "force accept" to cause "/src/javascript" to accept, or a user who legitimately owns "/src/javascript" accepts on behalf of the package but not on behalf of themselves (for whatever reason), it generally makes practical sense that these changes have owners involved in them (i.e., that's what a normal user would expect in both cases) and don't need to trigger audits under "no involvement" rules.

Test Plan: Used `bin/repository reparse --force --owners <commit>` to trigger audit logic. Saw a commit owned by `O1` with a revision counted as "involved" when `O1` had accepted the revision, even though no actual human owner had accepted it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20130
2019-02-07 15:41:56 -08:00
epriestley
31a0ed92d0 Support a wider range of "Audit" rules for Owners packages
Summary:
Depends on D20124. Ref T13244. See PHI1055. Add a few more builtin audit behaviors to make Owners more flexible.

(At the upper end of flexibility you can trigger audits in a very granular way with Herald, but you tend to need to write one rule per Owners package, and providing a middle ground here has worked reasonably well for "review" rules so far.)

Test Plan:
  - Edited a package to select the various different audit rules.
  - Used `bin/repository reparse --force --owners <commit>` to trigger package audits under varied conditions.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20126
2019-02-07 15:39:39 -08:00
epriestley
8fab8d8a18 Prepare owners package audit rules to become more flexible
Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.

PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.

Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.

Test Plan:
  - Created several packages, some with and some without auditing.
  - Inspected database for: package state; and associated transactions.
  - Ran the migrations.
  - Inspected database to confirm that state and transactions migrated correctly.
  - Reviewed transaction logs.
  - Created and edited packages and audit state.
  - Viewed the "Package List" element in Diffusion.
  - Pulled package information with `owners.search`, got sensible results.
  - Edited package audit status with `owners.edit`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20124
2019-02-07 15:38:12 -08:00
epriestley
509fbb6c20 When building audit queries, prefilter possible "authorPHID" values
Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.

It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.

Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.

(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)

Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20129
2019-02-07 15:36:56 -08:00
epriestley
a4bab60ad0 Don't show "registration might be too open" warnings unless an auth provider actually allows registration
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
2019-02-07 15:32:42 -08:00
epriestley
7469075a83 Allow users to be approved from the profile "Manage" page, alongside other similar actions
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.

Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.

Test Plan: {F6193742}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T8029

Differential Revision: https://secure.phabricator.com/D20123
2019-02-07 15:04:23 -08:00
epriestley
949afb02fd On login forms, autofocus the "username" field
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
2019-02-07 15:03:43 -08:00
epriestley
8054f7d191 Convert a manual query against external accounts into a modern Query
Summary: Depends on D20108. Ref T6703. Update this outdated callsite to a more modern appraoch.

Test Plan: Destroyed a user with `bin/remove destroy`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20109
2019-02-07 15:02:00 -08:00
epriestley
f0364eef8a Remove weird integration between Legalpad and the ExternalAccount table
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
2019-02-07 15:00:00 -08:00
epriestley
9f5e6bee90 Make the default behavior of getApplicationTransactionCommentObject() "return null" instead of "throw"
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
2019-02-07 14:56:38 -08:00
epriestley
4fa5a2421e Add formal setup guidance warning that "feed.http-hooks" will be removed in a future version of Phabricator
Summary: Depends on D20114. This is on the way out, so make that explicitly clear.

Test Plan: Read setup issue and configuration option.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20115
2019-02-07 14:54:09 -08:00
epriestley
e25dc2dfe2 Revert "feed.http-hooks" HTTP request construction to use "http_build_query()" so nested "storyData" is handled correctly
Summary:
See <https://discourse.phabricator-community.org/t/storydata-is-blank-in-outgoing-requests-to-the-configured-feed-http-hooks/2366/>.

This behavior was changed by D20049. I think it's generally good that we not accept/encode nested values in a PHP-specific way, but retain `feed.http-hooks` compatibility for now.

Test Plan: {F6190681}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20114
2019-02-07 14:53:03 -08:00
epriestley
26081594e2 Fix two very, very minor correctness issues in Slowvote
Summary:
See <https://hackerone.com/reports/492525> and <https://hackerone.com/reports/489531>. I previously awarded a bounty for <https://hackerone.com/reports/434116> so Slowvote is getting "researched" a lot.

  - Prevent users from undoing their vote by submitting the form with nothing selected.
  - Prevent users from racing between the `delete()` and `save()` to vote for multiple options in a plurality poll.

Test Plan:
  - Clicked the vote button with nothing selected in plurality and approval polls, got an error now.
  - Added a `sleep(5)` between `delete()` and `save()`. Submitted different plurality votes in different windows. Before: votes raced, invalid end state. After: votes waited on the lock, arrived in a valid end state.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20125
2019-02-07 12:45:11 -08:00
Austin McKinley
f2236eb061 Autofocus form control for adding TOTP codes
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
2019-02-07 11:56:49 -08:00
epriestley
a46c25d2ba Make two ancient migrations fatal if they affect data
Summary:
Depends on D20106. Ref T6703. Since I plan to change the `ExternalAccount` table, these migrations (which rely on `save()`) will stop working.

They could be rewritten to use raw queries, but I suspect few or no installs are affected. At least for now, just make them safe: if they would affect data, fatal and tell the user to perform a more gradual upgrade.

Also remove an `ALTER IGNORE TABLE` (this syntax was removed at some point) and fix a `%Q` when adjusting certain types of primary keys.

Test Plan: Ran `bin/storage upgrade --no-quickstart --force --namespace test1234` to get a complete migration since the beginning of time.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T6703

Differential Revision: https://secure.phabricator.com/D20107
2019-02-06 17:08:34 -08:00
epriestley
fc3b90e1d1 Allow users to unlink their last external account with a warning, instead of preventing the action
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
2019-02-06 17:07:41 -08:00
epriestley
d6f691cf5d In "External Accounts", replace hard-to-find tiny "link" icon with a nice button with text on it
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
2019-02-06 16:07:16 -08:00
epriestley
55286d49e8 Clarify "metamta.default-address" instructions and lock the option
Summary: This option no longer needs to be configured if you configure inbound mail (and that's the easiest setup approach in a lot of cases), so stop telling users they have to set it up.

Test Plan: Read documentation and configuration help.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20104
2019-02-06 16:03:49 -08:00
epriestley
113a2773dd Remove one-time login from username change email
Summary:
Depends on D20100. Ref T7732. Ref T13244. This is a bit of an adventure.

Long ago, passwords were digested with usernames as part of the salt. This was a mistake: it meant that your password becomes invalid if your username is changed.

(I think very very long ago, some other hashing may also have used usernames -- perhaps session hashing or CSRF hashing?)

To work around this, the "username change" email included a one-time login link and some language about resetting your password.

This flaw was fixed when passwords were moved to shared infrastructure (they're now salted more cleanly on a per-digest basis), and since D18908 (about a year ago) we've transparently upgraded password digests on use.

Although it's still technically possible that a username change could invalidate your password, it requires:

  - You set the password on a version of Phabricator earlier than ~2018 Week 5 (about a year ago).
  - You haven't logged into a version of Phabricator newer than that using your password since then.
  - Your username is changed.

This probably affects more than zero users, but I suspect not //many// more than zero. These users can always use "Forgot password?" to recover account access.

Since the value of this is almost certainly very near zero now and declining over time, just get rid of it. Also move the actual mail out of `PhabricatorUser`, ala the similar recent change to welcome mail in D19989.

Test Plan: Changed a user's username, reviewed resulting mail with `bin/mail show-outbound`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244, T7732

Differential Revision: https://secure.phabricator.com/D20102
2019-02-05 16:01:53 -08:00
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