Summary:
This is mostly for personal reasons / lols, but they have a perfectly functional OAuth2 API and it takes like 15 minutes to add a provider now and I was in this code anyway...
@chad, we could use JIRA, Twitter and Twitch.tv auth icons if you have a chance.
Test Plan: {F53564}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6706
Summary:
Ref T3373. The submit listener doesn't properly scope the form it listens to right now, so several forms on the page mean that comments post to one of them more or less at random.
Scope it properly by telling it which object PHID it is associated with.
Test Plan: Made Question comments, saw comments Ajax in on the question itself rather than on an arbitrary answer.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3373
Differential Revision: https://secure.phabricator.com/D6611
Summary: Currently, we'll fatal on array typehint issues if this is misconfigured. Instead, we should just reject the configuration. See some discussion in IRC.
Test Plan: Used LDAP to log in.
Reviewers: btrahan, totorico
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6489
Summary: Fixes T3517. Moves the email verification page out of People and into Auth. Makes it look less awful.
Test Plan: {F49636} {F49637}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3517
Differential Revision: https://secure.phabricator.com/D6425
Summary: The once-choppy LDAP waters seem to have calmed down a bit. Use the service profile log to get a pretty good idea of what's going on with LDAP (see D6391) instead of invasive logging to get a slightly better idea.
Test Plan:
$ ~/src/php-src/sapi/cli/php -f ./bin/auth ldap --trace
>>> [2] <connect> phabricator2_auth
<<< [2] <connect> 1,755 us
>>> [3] <query> SELECT * FROM `auth_providerconfig` ORDER BY id DESC
<<< [3] <query> 423 us
Enter LDAP Credentials
LDAP Username: ldapuser
>>> [4] <exec> $ stty -echo
<<< [4] <exec> 10,370 us
LDAP Password: >>> [5] <exec> $ stty echo
<<< [5] <exec> 6,844 us
Connecting to LDAP...
>>> [6] <ldap> connect (127.0.0.1:389)
<<< [6] <ldap> 12,932 us
>>> [7] <ldap> bind (sn=ldapuser,ou=People, dc=aphront, dc=com)
<<< [7] <ldap> 6,860 us
>>> [8] <ldap> search (ou=People, dc=aphront, dc=com, sn=ldapuser)
<<< [8] <ldap> 5,907 us
Found LDAP Account: ldapuser
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6392
Summary: We currently don't read/save this value correctly. Fix the issue. Ref T1536.
Test Plan: Set real name attributes to "x, y".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, colegleason
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6388
Summary:
Ref T1536. Ref T2852. Currently, after refreshing the token we don't actually return it. This means that code relying on token refresh fails once per hour (for Asana) in a sort of subtle way. Derp.
Update `bin/auth refresh` to make this failure more clear.
Test Plan: Set `force refresh` flag and verified a return value.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536, T2852
Differential Revision: https://secure.phabricator.com/D6295
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.
- Allow providers to customize the look of external accounts.
- For username/password auth, don't show the account view (it's confusing and not useful).
- For OAuth accounts, show token status.
Test Plan:
{F47374}
{F47375}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6289
Summary:
Ref T2852. Give OAuth providers a formal method so you can ask them for tokens; they issue a refresh request if necessary.
We could automatically refresh these tokens in daemons as they near expiry to improve performance; refreshes are blocking in-process round trip requests. If we do this for all tokens, it's a lot of requests (say, 20k users * 2 auth mechanisms * 1-hour tokens ~= a million requests a day). We could do it selectively for tokens that are actually in use (i.e., if we refresh a token in response to a user request, we keep refreshing it for 24 hours automatically). For now, I'm not pursuing any of this.
If we fail to refresh a token, we don't have a great way to communicate it to the user right now. The remedy is "log out and log in again", but there's no way for them to figure this out. The major issue is that a lot of OAuth integrations should not throw if they fail, or can't reasonably be rasied to the user (e.g., activity in daemons, loading profile pictures, enriching links, etc). For now, this shouldn't really happen. In future diffs, I plan to make the "External Accounts" settings page provide some information about tokens again, and possibly push some flag to accounts like "you should refresh your X link", but we'll see if issues crop up.
Test Plan: Used `bin/auth refresh` to verify refreshes. I'll wait an hour and reload a page with an Asana link to verify the auto-refresh part.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6280
Summary: Ref T2852. Provide a script for inspecting/debugging OAuth token refresh.
Test Plan: Ran `bin/auth refresh` with various arguments, saw token refreshes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6276
Summary:
- `DoorkeeperObjectRef` is a convenience object to keep track of `<applicationType, applicationDomain, objectType, objectID>` tuples.
- `DoorkeeperBridge` provides pull/push between Phabricator and external systems.
- `DoorkeeperBridgeAsana` is a bridge to Asana.
Test Plan:
Ran this snippet and got a task from Asana:
{P871}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6273
Summary: Ref T1536. This is missing a call.
Test Plan: Viewed a public blog with Facebook comments.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6288
Summary: Ref T1536. After DB-driven auth config, we need to load this differently.
Test Plan: Ran `bin/auth ldap`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6287
Summary: Ref T2852. Asana supports a link directly to this panel, I just wasn't able to find it.
Test Plan:
Clicked the link and got to the apps panel.
{F47346}
Reviewers: isaac_asana, btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6285
Summary: Ref T1536. Like Google, GitHub is actually strict about callback URIs too. Keep them pointed at the old URIs until we can gradually migrate.
Test Plan: Logged in with GitHub.
Reviewers: garoevans, davidreuss, btrahan
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6265
Summary: Changes it to a dialog view, tweaks some layout bugs on full width forms.
Test Plan: Tested loging in and resetting my password. Chrome + Mobile
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, nrp
Differential Revision: https://secure.phabricator.com/D6257
Summary:
Ref T1536.
- When users try to add a one-of provider which already exists, give them a better error (a dialog explaining what's up with reasonable choices).
- Disable such providers and label why they're disabled on the "new provider" screen.
Test Plan:
{F47012}
{F47013}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6256
Summary: Ref T1536. Mostly, this puts "username/password" (which is probably a common selection) first on the list.
Test Plan: {F47010}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6254
Summary: Ref T1536. This "should never happen", but can if you're developing custom providers. Improve the robustness of this interface in the presence of missing provider implementations.
Test Plan: {F47008}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6253
Summary:
Ref T1536.
- Move all the provider-specific help into contextual help in Auth.
- This provides help much more contextually, and we can just tell the user the right values to use to configure things.
- Rewrite account/registration help to reflect the newer state of the word.
- Also clean up a few other loose ends.
Test Plan: {F46937}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6247
Summary: Ref T1536. This is the last major migration. Moves us over to the DB and drops all the config stuff.
Test Plan:
- Ran the migration.
- Saw all my old config brought forward and respected, with accurate settings.
- Ran LDAP import.
- Grepped for all removed config options.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, wez
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6243
Summary:
Ref T1536. This sets us for the "Config -> Database" migration. Basically:
- If stuff is defined in the database, respect the database stuff (no installs have anything defined yet since they can't reach the interfaces/code).
- Otherwise, respect the config stuff (all installs currently do this).
Test Plan: Saw database stuff respected when database stuff was defined; saw config stuff respected otherwise.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6240
Summary:
Ref T1536. Currently, when you install Phabricator you're dumped on the login screen and have to consult the documentation to learn about `bin/accountadmin`.
Instead, detect that an install is running first-time setup:
- It has no configured providers; and
- it has no user accounts.
We can safely deduce that such an install isn't configured yet, and let the user create an admin account from the web UI.
After they login, we raise a setup issue and lead them to configure authentication.
(This could probably use some UI and copy tweaks.)
Test Plan:
{F46738}
{F46739}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6228
Summary: Ref T1536. Make this UI a bit more human-friendly.
Test Plan: {F46873}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6237
Summary:
Ref T1536. This is extremely reachable and changes the login code to the new stuff.
Notes:
- I've hard-disabled password registration since I want installs to explicitly flip it on via config if they want it. New installs will get it by default in the future, but old installs shouldn't have their auth options change.
- Google doesn't let us change the redirect URI, so keep the old one working.
- We need to keep a bit of LDAP around for now for LDAP import.
- **Facebook:** This causes substantive changes in what login code is executed.
Test Plan:
- Logged in / logged out / registered, hit new flows.
- Logged in with google.
- Verified no password registration by default.
Reviewers: btrahan, chad
Reviewed By: chad
CC: wez, nh, aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6222
Summary:
Ref T1536. Because Facebook publishes data from Phabricator to user profiles and that data is sensitive, it wants to require secure browsing to be enabled in order to login.
Respect the existing option, and support it in the UI.
The UI part isn't reachable yet.
Test Plan: {F46723}
Reviewers: chad, btrahan
Reviewed By: chad
CC: arice, wez, aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6224
Summary: Ref T1536. Love me some LDAP.
Test Plan: Viewed and edited form. Looked through transactions.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6227
Summary:
Ref T1536.
- When we render a dialog on a page by itself, put it on a dust background.
- Currently, we render "Logout" in two different places. Stop doing that.
- Make sure the surviving one has workflow so we get a modal ajax dialog if possible.
Test Plan: {F46731}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6226
Summary:
Ref T1536. None of this code is reachable.
For the new web UI for auth edits, give providers more and better customization options for handling the form. Allow them to format transactions.
Also fix the "Auth" application icon.
Test Plan: {F46718}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6221
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.
- Add a "Cancel" button, to get back to login.
- Add a crumb showing the registering provider.
- Add an account card when registering with an external account
- Tailor some language to make it less ambiguous ("Phabricator Username", "Register Phabricator Account").
Test Plan:
{F46618}
{F46619}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6213
Summary: Ref T1536. This gets the single queries out of the View and builds a propery Query class for ExternalAccount.
Test Plan: Linked/unlinked accounts, logged out, logged in.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6212
Summary: Ref T1536. This script basically exists to restore access if/when users shoot themselves in the foot by disabling all auth providers and can no longer log in.
Test Plan: {F46411}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6205
Summary: Ref T1536. When auth providers are edited, show the edit history.
Test Plan: {F46400}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6203
Summary: Ref T1536. Many rough / broken edges, but adds the rough skeleton of the provider edit workflow.
Test Plan: {F46333}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6200
Summary: Ref T1536. Adds an initial "choose a provider type" screen for adding a new provider. This doesn't go anywhere yet.
Test Plan: {F46316}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6199
Summary:
Ref T1536. Currently, we have about 40 auth-related configuration options. This is already roughly 20% of our config, and we want to add more providers. Additionally, we want to turn some of these auth options into multi-auth options (e.g., allow multiple Phabricator OAuth installs, or, theoretically multiple LDAP servers).
I'm going to move this into a separate "Auth" tool with a minimal CLI (`bin/auth`) interface and a more full web interface. Roughly:
- Administrators will use the app to manage authentication providers.
- The `bin/auth` CLI will provide a safety hatch if you lock yourself out by disabling all usable providers somehow.
- We'll migrate existing configuration into the app and remove it.
General goals:
- Make it much easier to configure authentication by providing an interface for it.
- Make it easier to configure everything else by reducing the total number of available options.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6196
Summary: Ref T1536. These can probably use some design tweaking and there's a bit of a bug with profile images for some providers, but generally seems to be in the right ballpark.
Test Plan:
{F46604}
{F46605}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6210
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. Error state is a bit gross but we need to sort that out in general.
Test Plan:
{F46549}
{F46550}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6208
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 T1445. Ref T1536. Although we have separate CSRF protection and have never been vulnerable to OAuth hijacking, properly implementing the "state" parameter provides a little more certainty.
Before OAuth, we set a random value on the client, and pass its hash as the "state" parameter. Upon return, validate that (a) the user has a nonempty "phcid" cookie and (b) the OAuth endpoint passed back the correct state (the hash of that cookie).
Test Plan: Logged in with all OAuth providers, which all apparently support `state`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, arice
Maniphest Tasks: T1445, T1536
Differential Revision: https://secure.phabricator.com/D6179
Summary: Ref T1536. We can safely replace the old login validation controller with this new one, and reduce code dplication while we're at it.
Test Plan: Logged in with LDAP, logged in with OAuth, logged in with username/password, did a password reset.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6178
Summary:
Ref T1536. LDAP is very likely the worst thing in existence.
This has some rough edges (error handling isn't perfect) but is already better than the current LDAP experience! durrr
Test Plan: Registered and logged in using LDAP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6177
Summary: Ref T1536. Support for GitHub on new flows.
Test Plan: Registered and logged in with GitHub.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6166
Summary: Ref T1536. Adds Disqus as a Provider.
Test Plan: Registered and logged in with Disqus.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6165
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. Code is intentionally made unreachable (see PhabricatorAuthProviderOAuthFacebook->isEnabled()).
This adds:
- A provider-driven "start" screen (this has the list of ways you can login/register).
- Registration actually works.
- Facebook OAuth works.
@chad, do you have any design ideas on the start screen? I think we poked at it before, but the big issue was that there were a limitless number of providers. Today, we have:
- Password
- LDAP
- Facebook
- GitHub
- Phabricator
- Disqus
- Google
We plan to add:
- Asana
- An arbitrary number of additional instances of Phabricator
Users want to add:
- OpenID
- Custom providers
And I'd like to have these at some point:
- Stripe
- WePay
- Amazon
- Bitbucket
So basically any UI for this has to accommodate 300 zillion auth options. I don't think we need to solve any UX problems here (realistically, installs enable 1-2 auth options and users don't actually face an overwhelming number of choices) but making the login forms less ugly would be nice. No combination of prebuilt elements seems to look very good for this use case.
Test Plan: Registered a new acount with Facebook.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6161
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
Summary:
Currently, registration and authentication are pretty messy. Two concrete problems:
- The `PhabricatorLDAPRegistrationController` and `PhabricatorOAuthDefaultRegistrationController` controllers are giant copy/pastes of one another. This is really bad.
- We can't practically implement OpenID because we can't reissue the authentication request.
Additionally, the OAuth registration controller can be replaced wholesale by config, which is a huge API surface area and a giant mess.
Broadly, the problem right now is that registration does too much: we hand it some set of indirect credentials (like OAuth tokens) and expect it to take those the entire way to a registered user. Instead, break registration into smaller steps:
- User authenticates with remote service.
- Phabricator pulls information (remote account ID, username, email, real name, profile picture, etc) from the remote service and saves it as `PhabricatorUserCredentials`.
- Phabricator hands the `PhabricatorUserCredentials` to the registration form, which is agnostic about where they originate from: it can process LDAP credentials, OAuth credentials, plain old email credentials, HTTP basic auth credentials, etc.
This doesn't do anything yet -- there is no way to create credentials objects (and no storage patch), but I wanted to get any initial feedback, especially about the event call for T2394. In particular, I think the implementation would look something like this:
$profile = $event->getValue('profile')
$username = $profile->getDefaultUsername();
$is_employee = is_this_a_facebook_employee($username);
if (!$is_employee) {
throw new Exception("You are not employed at Facebook.");
}
$fbid = get_fbid_for_facebook_username($username);
$profile->setDefaultEmail($fbid);
$profile->setCanEditUsername(false);
$profile->setCanEditEmail(false);
$profile->setCanEditRealName(false);
$profile->setShouldVerifyEmail(true);
Seem reasonable?
Test Plan: N/A yet, probably fatals.
Reviewers: vrana, btrahan, codeblock, chad
Reviewed By: btrahan
CC: aran, asherkin, nh, wez
Maniphest Tasks: T1536, T2394
Differential Revision: https://secure.phabricator.com/D4647
Summary: Ref T1536. This is similar to D6172 but much simpler: we don't need to retain external interfaces here and can do a straight migration.
Test Plan: TBA
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6173
Summary: Ref T1536. Migrates the OAuthInfo table to ExternalAccount, and makes `PhabricatorUserOAuthInfo` a wrapper for an ExternalAccount.
Test Plan: Logged in with OAuth, registered with OAuth, linked/unlinked OAuth accounts, checked OAuth status screen, deleted an account with related OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6172
Summary:
Ref T1536. Move all access to the underlying storage to inside the class. My plan is:
- Migrate the table to ExternalAccount.
- Nuke the table.
- Make this class read from and write to ExternalAccount instead.
We can't get rid of OAuthInfo completely because Facebook still depends on it for now, via registration hooks.
Test Plan: Logged in and registered with OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6171
Summary:
Ref T1536. Currently, we store OAuth tokens along with their expiry times and status. However, all we use this for is refreshing profile pictures and showing a silly (and probably somewhat confusing) interface about token status.
I want to move this storage over to `PhabricatorExternalAccount` to make the cutover easier. Drop it for now, including all the profile image stuff (I plan to rebuild that in a more sensible way anyway).
Test Plan: Viewed screen; linked/unlinked accounts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6170
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary: okay title. other apps can get this by implementing shouldAllowPublic and set(ting)RequestURI on TransactionsCommentView. note i put some css inline -- let me know if that belongs someplace else or needs better design.
Test Plan: viewed a mock logged out and saw new button. used new button and ended up on the mock logged in with a clean URI.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2653
Differential Revision: https://secure.phabricator.com/D5266
Summary: Mostly applies a new call spacing rule; also a few things that have slipped through via pull requests and such
Test Plan: `find src/ -type f -name '*.php' | xargs -n16 arc lint --output summary --apply-patches`
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D5002
Summary: Sgrepped for `"=~/</"` and manually changed every HTML.
Test Plan: This doesn't work yet but it is hopefully one of the last diffs before Phabricator will be undoubtedly HTML safe.
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4927
Summary: I'm too lazy to attaching them for diffs where they were introduced.
Test Plan:
/
/D1, wrote comment with code snippet
DarkConsole
commit detail, wrote comment
task detail, wrote comment
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4911
Summary:
This is pretty brutal and it adds some `phutil_safe_html()`.
But it is a big step in the right direction.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4905
Summary:
This resolves lots of double escaping.
We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped.
Also `pht()` auto escapes if it gets `PhutilSafeHTML`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4889
Summary:
Lots of killed `phutil_escape_html()`.
Done by searching for `AphrontTableView` and then `$rows` (usually) backwards.
Test Plan:
Looked at homepage.
echo id(new AphrontTableView(array(array('<'))))->render();
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4884
Summary:
Done by searching for `AphrontDialogView` and then `appendChild()`.
Also added some `pht()`.
Test Plan: None.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4882
Summary: Searched for `AphrontFormView` and then for `appendChild()`.
Test Plan: /login/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4855
Summary: Done by searching for `AphrontErrorView` and then `appendChild()`.
Test Plan:
Looked at Commit Detail.
Looked at Revision Detail.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4843
Summary:
Make `PhabricatorMenuView` more flexible, so callers can add items to the beginning/end/middle.
In particular, this allows event handlers to receive a $menu and call `addMenuItemToLabel('activity', ...)` or similar, for D4708.
Test Plan: Unit tests. Browsed site. Home page, Conpherence, and other pages with menus look correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4792
Summary: Convert most phabricator_render_form callsites. In the case of the "headsup view", it converts it by deleting the element entirely (this is the very old Maniphest/Differential header which we no longer use).
Test Plan: Poked around a bit.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2432
Differential Revision: https://secure.phabricator.com/D4726
Summary: Created Applications application which allows uninstallation & installation of application.
Test Plan: In "Applications" application, clicked on uninstalled the application by cliking Uninstall and chekcing whether they are really uninstalled(Disabling URI & in appearance in the side pane). Then Clicked on the install button of the uninstalled application to check whether they are installed.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4715
Summary: Spent some time going through auth stuff for pht's.
Test Plan: Tested logging in, logging out, reseting password, using Github, creating a new account. I couldn't quite test everything so will double read the diff when I submit it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4671
Summary:
Created with spatch:
lang=diff
- phutil_render_tag
+ phutil_tag
(X, Y, '...')
Then searched for `&` and `<` in the output and replaced them.
Test Plan: Loaded homepage.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4503
Summary: Removes the panel-view on login and adds additonal responsive styles for mobile forms.
Test Plan: View in mobile browser, resize page.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4530
