Summary:
Fixes T3471. Specific issues:
- Add the ability to set a temporary cookie (expires when the browser closes).
- We overwrote 'phcid' on every page load. This creates some issues with browser extensions. Instead, only write it if isn't set. To counterbalance this, make it temporary.
- Make the 'next_uri' cookie temporary.
- Make the 'phreg' cookie temporary.
- Fix an issue where deleted cookies would persist after 302 (?) in some cases (this is/was 100% for me locally).
Test Plan:
- Closed my browser, reopned it, verified temporary cookies were gone.
- Logged in, authed, linked, logged out.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3471
Differential Revision: https://secure.phabricator.com/D8537
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.
We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.
To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.
This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.
Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.
Test Plan:
- Logged in with OAuth.
- Hit any other page with `?code=...` in the URL, got an exception.
- Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4593
Differential Revision: https://secure.phabricator.com/D8499
Summary:
OAuth1 doesn't have anything like the `state` parameter, and I overlooked that we need to shove one in there somewhere. Append it to the callback URI. This functions like `state` in OAuth2.
Without this, an attacker can trick a user into logging into Phabricator with an account the attacker controls.
Test Plan:
- Logged in with JIRA.
- Logged in with Twitter.
- Logged in with Facebook (an OAuth2 provider).
- Linked a Twitter account.
- Linked a Facebook account.
- Jiggered codes in URIs and verified that I got the exceptions I expected.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Differential Revision: https://secure.phabricator.com/D8318
Summary: Ref T4339. We have more magical cookie names than we should, move them all to a central location.
Test Plan: Registered, logged in, linked account, logged out. See inlines.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4339
Differential Revision: https://secure.phabricator.com/D8041
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary: Depends on D6872. Ref T3687. Give the user a nice dialog instead of a bare exception.
Test Plan: Cancelled out of Twitter and JIRA workflows. We should probably do this for the OAuth2 workflows too, but they're a bit of a pain to de-auth and I am lazy.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D6873
Summary:
^\s+(['"])dust\1\s*=>\s*true,?\s*$\n
Test Plan: Looked through the diff.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6769
Summary:
Ref T1536. This is equivalent to logging out and logging back in again, but a bit less disruptive for users. For some providers (like Google), this may eventually do something different (Google has a "force" parameter which forces re-auth and is ostensibly required to refresh long-lived tokens).
Broadly, this process fixes OAuth accounts with busted access tokens so we can do API stuff. For other accounts, it mostly just syncs profile pictures.
Test Plan:
Refreshed LDAP and Oauth accounts, linked OAuth accounts, hit error conditions.
{F47390}
{F47391}
{F47392}
{F47393}
{F47394}
{F47395}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6290
Summary:
Ref T1536.
- When linking accounts after initially failing, make the crumb say "Link Account" instead of "Login".
- When on the LDAP failure form, show a "Cancel" button returning to start (if logging in) or settings (if linking accounts).
- Allow providers to distinguish between "start", "login" and "link" rendering.
Test Plan: Linked and logged in with LDAP and other registration mechainsms.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6214
Summary:
Ref T1536. Currently, we have separate panels for each link/unlink and separate controllers for OAuth vs LDAP.
Instead, provide a single "External Accounts" panel which shows all linked accounts and allows you to link/unlink more easily.
Move link/unlink over to a full externalaccount-based workflow.
Test Plan:
- Linked and unlinked OAuth accounts.
- Linked and unlinked LDAP accounts.
- Registered new accounts.
- Exercised most/all of the error cases.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6189
Summary:
Ref T1536. Facebook currently does a check which should be on-login in registration hooks, and this is generally a reasonable hook to provide.
The "will login" event allows listeners to reject or modify a login, or just log it or whatever.
NOTE: This doesn't cover non-web logins right now -- notably Conduit. That's presumably fine.
(This can't land for a while, it depends on about 10 uncommitted revisions.)
Test Plan: Logged out and in again.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6202
Summary: Ref T1536. Like D6080, we don't need to store the registration key itself. This prevents a theoretical attacker who can read the database but not write to it from hijacking registrations.
Test Plan: Registered a new account.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6188
Summary:
Ref T1536. Ref T1930. Code is not reachable.
This provides password authentication and registration on the new provider/adapter framework.
I sort of cheated a little bit and don't really route any password logic through the adapter (instead, this provider uses an empty adapter and just sets the type/domain on it). I think the right way to do this //conceptually// is to treat username/passwords as an external black box which the adapter communicates with. However, this creates a lot of practical implementation and UX problems:
- There would basically be two steps -- in the first one, you interact with the "password black box", which behaves like an OAuth provider. This produces some ExternalAccount associated with the username/password pair, then we go into normal registration.
- In normal registration, we'd proceed normally.
This means:
- The registration flow would be split into two parts, one where you select a username/password (interacting with the black box) and one where you actually register (interacting with the generic flow). This is unusual and probably confusing for users.
- We would need to do a lot of re-hashing of passwords, since passwords currently depend on the username and user PHID, which won't exist yet during registration or the "black box" phase. This is a big mess I don't want to deal with.
- We hit a weird condition where two users complete step 1 with the same username but don't complete step 2 yet. The box knows about two different copies of the username, with two different passwords. When we arrive at step 2 the second time we have a lot of bad choices about how to reoslve it, most of which create security problems. The most stragihtforward and "pure" way to resolve the issues is to put password-auth usernames in a separate space, but this would be incredibly confusuing to users (your login name might not be the same as your username, which is bizarre).
- If we change this, we need to update all the other password-related code, which I don't want to bother with (at least for now).
Instead, let registration know about a "default" registration controller (which is always password, if enabled), and let it require a password. This gives us a much simpler (albeit slightly less pure) implementation:
- All the fields are on one form.
- Password adapter is just a shell.
- Password provider does the heavy lifting.
We might make this more pure at some point, but I'm generally pretty satisfied with this.
This doesn't implement the brute-force CAPTCHA protection, that will be coming soon.
Test Plan: Registered with password only and logged in with a password. Hit various error conditions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536, T1930
Differential Revision: https://secure.phabricator.com/D6164
Summary:
Ref T1536. None of this code is reachable.
Implements new-auth login (so you can actually login) and login validation (which checks that cookies were set correctly).
Test Plan: Manually enabled FB auth, went through the auth flow to login/logout. Manually hit most of the validation errors.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6162
Summary:
Ref T1536. None of this code is reachable.
`PhabricatorAuthLoginController` provides a completely generic login/link flow, similar to how D6155 provides a generic registration flow.
`PhabricatorAuthProvider` wraps a `PhutilAuthAdapter` and glues the generic top-level flow to a concrete authentication provider.
Test Plan: Static only, code isn't meaningfully reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6159
