1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-15 19:32:40 +01:00
Commit graph

412 commits

Author SHA1 Message Date
epriestley
99c72a32d0 Allow installs to require multi-factor authentication for all users
Summary: Ref T5089. Adds a `security.require-multi-factor-auth` which forces all users to enroll in MFA before they can use their accounts.

Test Plan:
Config:

{F159750}

Roadblock:

{F159748}

After configuration:

{F159749}

  - Required MFA, got roadblocked, added MFA, got unblocked.
  - Removed MFA, got blocked again.
  - Used `bin/auth strip` to strip MFA, got blocked.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5089

Differential Revision: https://secure.phabricator.com/D9285
2014-06-03 16:50:27 -07:00
epriestley
83112cc2e8 Move email verification into PhabricatorUserEditor
Summary: Both email verify and welcome links now verify email, centralize them and record them in the user activity log.

Test Plan:
  - Followed a "verify email" link and got verified.
  - Followed a "welcome" (verifying) link.
  - Followed a "reset" (non-verifying) link.
  - Looked in the activity log for the verifications.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9284
2014-06-03 16:45:18 -07:00
epriestley
6df1a02413 (Redesign) Clean up older "Tile" code
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:

  - Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
  - Old tile size methods are replaced with `isPinnnedByDefault()`.
  - Shortened some short descriptions.
  - `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
  - Added a marker for third-party / extension applications.

Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9358
2014-06-03 15:47:27 -07:00
epriestley
81d95cf682 Make default view of "Applications" app a full-page launcher
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.

Open to feedback.

Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)

{F160052}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5176

Differential Revision: https://secure.phabricator.com/D9297
2014-05-29 12:17:54 -07:00
epriestley
f1534e6feb Make password reset emails use one-time tokens
Summary:
Ref T4398. This code hadn't been touched in a while and had a few crufty bits.

**One Time Resets**: Currently, password reset (and similar links) are valid for about 48 hours, but we always use one token to generate them (it's bound to the account). This isn't horrible, but it could be better, and it produces a lot of false positives on HackerOne.

Instead, use TemporaryTokens to make each link one-time only and good for no more than 24 hours.

**Coupling of Email Verification and One-Time Login**: Currently, one-time login links ("password reset links") are tightly bound to an email address, and using a link verifies that email address.

This is convenient for "Welcome" emails, so the user doesn't need to go through two rounds of checking email in order to login, then very their email, then actually get access to Phabricator.

However, for other types of these links (like those generated by `bin/auth recover`) there's no need to do any email verification.

Instead, make the email verification part optional, and use it on welcome links but not other types of links.

**Message Customization**: These links can come out of several workflows: welcome, password reset, username change, or `bin/auth recover`. Add a hint to the URI so the text on the page can be customized a bit to help users through the workflow.

**Reset Emails Going to Main Account Email**: Previously, we would send password reset email to the user's primary account email. However, since we verify email coming from reset links this isn't correct and could allow a user to verify an email without actually controlling it.

Since the user needs a real account in the first place this does not seem useful on its own, but might be a component in some other attack. The user might also no longer have access to their primary account, in which case this wouldn't be wrong, but would not be very useful.

Mitigate this in two ways:

  - First, send to the actual email address the user entered, not the primary account email address.
  - Second, don't let these links verify emails: they're just login links. This primarily makes it more difficult for an attacker to add someone else's email to their account, send them a reset link, get them to login and implicitly verify the email by not reading very carefully, and then figure out something interesting to do (there's currently no followup attack here, but allowing this does seem undesirable).

**Password Reset Without Old Password**: After a user logs in via email, we send them to the password settings panel (if passwords are enabled) with a code that lets them set a new password without knowing the old one.

Previously, this code was static and based on the email address. Instead, issue a one-time code.

**Jump Into Hisec**: Normally, when a user who has multi-factor auth on their account logs in, we prompt them for factors but don't put them in high security. You usually don't want to go do high-security stuff immediately after login, and it would be confusing and annoying if normal logins gave you a "YOU ARE IN HIGH SECURITY" alert bubble.

However, if we're taking you to the password reset screen, we //do// want to put the user in high security, since that screen requires high security. If we don't do this, the user gets two factor prompts in a row.

To accomplish this, we set a cookie when we know we're sending the user into a high security workflow. This cookie makes login finalization upgrade all the way from "partial" to "high security", instead of stopping halfway at "normal". This is safe because the user has just passed a factor check; the only reason we don't normally do this is to reduce annoyance.

**Some UI Cleanup**: Some of this was using really old UI. Modernize it a bit.

Test Plan:
  - **One Time Resets**
    - Used a reset link.
    - Tried to reuse a reset link, got denied.
    - Verified each link is different.
  - **Coupling of Email Verification and One-Time Login**
    - Verified that `bin/auth`, password reset, and username change links do not have an email verifying URI component.
    - Tried to tack one on, got denied.
    - Used the welcome email link to login + verify.
    - Tried to mutate the URI to not verify, or verify something else: got denied.
  - **Message Customization**
    - Viewed messages on the different workflows. They seemed OK.
  - **Reset Emails Going to Main Account Email**
    - Sent password reset email to non-primary email.
    - Received email at specified address.
    - Verified it does not verify the address.
  - **Password Reset Without Old Password**
    - Reset password without knowledge of old one after email reset.
    - Tried to do that without a key, got denied.
    - Tried to reuse a key, got denied.
  - **Jump Into Hisec**
    - Logged in with MFA user, got factor'd, jumped directly into hisec.
    - Logged in with non-MFA user, no factors, normal password reset.
  - **Some UI Cleanup**
    - Viewed new UI.
  - **Misc**
    - Created accounts, logged in with welcome link, got verified.
    - Changed a username, used link to log back in.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D9252
2014-05-22 10:41:00 -07:00
epriestley
cac61980f9 Add "temporary tokens" to auth, for SMS codes, TOTP codes, reset codes, etc
Summary:
Ref T4398. We have several auth-related systems which require (or are improved by) the ability to hand out one-time codes which expire after a short period of time.

In particular, these are:

  - SMS multi-factor: we need to be able to hand out one-time codes for this in order to prove the user has the phone.
  - Password reset emails: we use a time-based rotating token right now, but we could improve this with a one-time token, so once you reset your password the link is dead.
  - TOTP auth: we don't need to verify/invalidate keys, but can improve security by doing so.

This adds a generic one-time code storage table, and strengthens the TOTP enrollment process by using it. Specifically, you can no longer edit the enrollment form (the one with a QR code) to force your own key as the TOTP key: only keys Phabricator generated are accepted. This has no practical security impact, but generally helps raise the barrier potential attackers face.

Followup changes will use this for reset emails, then implement SMS multi-factor.

Test Plan:
  - Enrolled in TOTP multi-factor auth.
  - Submitted an error in the form, saw the same key presented.
  - Edited the form with web tools to provide a different key, saw it reject and the server generate an alternate.
  - Change the expiration to 5 seconds instead of 1 hour, submitted the form over and over again, saw it cycle the key after 5 seconds.
  - Looked at the database and saw the tokens I expected.
  - Ran the GC and saw all the 5-second expiry tokens get cleaned up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D9217
2014-05-20 11:43:45 -07:00
Tal Shiri
43d45c4956 can now tell phabricator you trust an auth provider's emails (useful for Google OAuth), which will mark emails as "verified" and will skip email verification.
Summary: This is useful when you're trying to onboard an entire office and you end up using the Google OAuth anyway.

Test Plan: tested locally. Maybe I should write some tests?

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9150
2014-05-16 14:14:06 -07:00
Chad Little
0120388a75 Found some missing icons
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.

Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9088
2014-05-13 07:45:39 -07:00
Chad Little
b2f3001ec4 Replace Sprite-Icons with FontAwesome
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.

Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9052
2014-05-12 10:08:32 -07:00
lkassianik
dfcccd4cb8 Add config to require real name, respect config when creating new users, drop real name from full name if not provided.
Summary: Fixes T4728, first pass, Make real name optional on user accounts

Test Plan: Default real name config should be false (not required). Create new user, real name should not be required. Toggle config, real name should be required. Users with no real name should be always listed by their usernames.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4728

Differential Revision: https://secure.phabricator.com/D9027
2014-05-12 09:51:41 -07:00
Ashish SHUKLA
0da22abdb5 WordPress.com OAuth2 plugin
Summary:
This plugin provides an OAuth authentication provider to authenticate users using WordPress.com Connect.

This diff corresponds to github pull request https://github.com/facebook/phabricator/pull/593/ and had its libphutil counterpart reviewed in D9004.

Test Plan: Configured WordPress.com as an authentication provider, saw it show up on the login screen, registered a new account, got expected defaults for my username/name/email/profile picture.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9019
2014-05-08 14:23:19 -07:00
epriestley
a04e138ae2 Minor cleanup of some session code
Summary: Ref T4398. Add some documentation and use `phutil_units()`.

Test Plan:
  - Established a web session.
  - Established a conduit session.
  - Entered and exited hisec.
  - Used "Sessions" panel to examine results.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8924
2014-05-01 10:23:19 -07:00
epriestley
e146958217 Generate QR codes for TOTP tokens
Summary: Ref T4398. I found a reasonable-ish LGPLv3 library for doing this, which isn't too huge or unwieldy.

Test Plan:
  - Scanned QR code with Authy.
  - Scanned QR code with Google Authenticator.

{F149317}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8923
2014-05-01 10:23:11 -07:00
epriestley
50376aad04 Require multiple auth factors to establish web sessions
Summary:
Ref T4398. This prompts users for multi-factor auth on login.

Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.

Test Plan:
  - Used Conduit.
  - Logged in as multi-factor user.
  - Logged in as no-factor user.
  - Tried to do non-login-things with a partial session.
  - Reviewed account activity logs.

{F149295}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8922
2014-05-01 10:23:02 -07:00
epriestley
e8cebb7da5 Add support for aural-only and visual-only elements
Summary:
Ref T4843. This adds support to `javelin_tag()` for an `aural` attribute. When specified, `true` values mean "this content is aural-only", while `false` values mean "this content is not aural".

  - I've attempted to find the best modern approaches for marking this content, but the `aural` attribute should let us change the mechanism later.
  - Make the "beta" markers on application navigation visual only (see T4843). This information is of very low importance, the application navigation is accessed frequently, and the information is available on the application list.
  - Partially convert the main navigation. This is mostly to test things, since I want to get more concrete feedback about approaches here.
  - Add a `?__aural__=1` attribute, which renders the page with aural-only elements visible and visual-only elements colored.

Test Plan: {F146476}

Reviewers: btrahan, scp, chad

Reviewed By: chad

Subscribers: aklapper, qgil, epriestley

Maniphest Tasks: T4843

Differential Revision: https://secure.phabricator.com/D8830
2014-05-01 07:18:18 -07:00
epriestley
3fde020049 Make many actions require high security
Summary:
Ref T4398. Protects these actions behind a security barrier:

  - Link external account.
  - Retrieve Conduit token.
  - Reveal Passphrase credential.
  - Create user.
  - Admin/de-admin user.
  - Rename user.
  - Show conduit certificate.
  - Make primary email.
  - Change password.
  - Change VCS password.
  - Add SSH key.
  - Generate SSH key.

Test Plan: Tried to take each action and was prompted for two-factor.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8921
2014-04-30 17:44:59 -07:00
epriestley
23e654ec2b Rate limit multi-factor actions
Summary: Ref T4398. Prevent users from brute forcing multi-factor auth by rate limiting attempts. This slightly refines the rate limiting to allow callers to check for a rate limit without adding points, and gives users credit for successfully completing an auth workflow.

Test Plan: Tried to enter hisec with bad credentials 11 times in a row, got rate limited.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8911
2014-04-30 14:30:31 -07:00
epriestley
535cfa3ebe Add bin/auth list-factors and bin/auth strip to remove multi-factor auth
Summary:
Ref T4398. The major goals here is to let administrators strip auth factors in two cases:

  - A user lost their phone and needs access restored to their account; or
  - an install previously used an API-based factor like SMS, but want to stop supporting it (this isn't possible today).

Test Plan:
  - Used `bin/auth list-factors` to show installed factors.
  - Used `bin/auth strip` with various mixtures of flags to selectively choose and strip factors from accounts.
  - Also ran `bin/auth refresh` to verify refreshing OAuth tokens works (small `OAuth` vs `OAuth2` tweak).

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8909
2014-04-30 14:30:00 -07:00
epriestley
a017a8e02b Make two-factor auth actually work
Summary:
Ref T4398. Allows auth factors to render and validate when prompted to take a hi-sec action.

This has a whole lot of rough edges still (see D8875) but does fundamentally work correctly.

Test Plan:
  - Added two different TOTP factors to my account for EXTRA SECURITY.
  - Took hisec actions with no auth factors, and with attached auth factors.
  - Hit all the error/failure states of the hisec entry process.
  - Verified hisec failures appear in activity logs.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8886
2014-04-28 10:20:54 -07:00
epriestley
17709bc167 Add multi-factor auth and TOTP support
Summary:
Ref T4398. This is still pretty rough and isn't exposed in the UI yet, but basically works. Some missing features / areas for improvement:

  - Rate limiting attempts (see TODO).
  - Marking tokens used after they're used once (see TODO), maybe. I can't think of ways an attacker could capture a token without also capturing a session, offhand.
  - Actually turning this on (see TODO).
  - This workflow is pretty wordy. It would be nice to calm it down a bit.
  - But also add more help/context to help users figure out what's going on here, I think it's not very obvious if you don't already know what "TOTP" is.
  - Add admin tool to strip auth factors off an account ("Help, I lost my phone and can't log in!").
  - Add admin tool to show users who don't have multi-factor auth? (so you can pester them)
  - Generate QR codes to make the transfer process easier (they're fairly complicated).
  - Make the "entering hi-sec" workflow actually check for auth factors and use them correctly.
  - Turn this on so users can use it.
  - Adding SMS as an option would be nice eventually.
  - Adding "password" as an option, maybe? TOTP feels fairly good to me.

I'll post a couple of screens...

Test Plan:
  - Added TOTP token with Google Authenticator.
  - Added TOTP token with Authy.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8875
2014-04-28 09:27:11 -07:00
epriestley
3f5a55fa6e Let users review their own account activity logs
Summary:
Ref T4398. This adds a settings panel for account activity so users can review activity on their own account. Some goals are:

  - Make it easier for us to develop and support auth and credential information, see T4398. This is the primary driver.
  - Make it easier for users to understand and review auth and credential information (see T4842 for an example -- this isn't there yet, but builds toward it).
  - Improve user confidence in security by making logging more apparent and accessible.

Minor corresponding changes:

  - Entering and exiting hisec mode is now logged.
  - This, sessions, and OAuth authorizations have moved to a new "Sessions and Logs" area, since "Authentication" was getting huge.

Test Plan:
  - Viewed new panel.
  - Viewed old UI.
  - Entered/exited hisec and got prompted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8871
2014-04-27 17:32:09 -07:00
epriestley
f42ec84d0c Add "High Security" mode to support multi-factor auth
Summary:
Ref T4398. This is roughly a "sudo" mode, like GitHub has for accessing SSH keys, or Facebook has for managing credit cards. GitHub actually calls theirs "sudo" mode, but I think that's too technical for big parts of our audience. I've gone with "high security mode".

This doesn't actually get exposed in the UI yet (and we don't have any meaningful auth factors to prompt the user for) but the workflow works overall. I'll go through it in a comment, since I need to arrange some screenshots.

Test Plan: See guided walkthrough.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8851
2014-04-27 17:31:11 -07:00
Chad Little
11fd6afeb1 Move Timeline icons to Fonts
Summary: Throwing this up for testing, swapped out all icons in timeline for their font equivelants. Used better icons where I could as well. We should feel free to use more / be fun with the icons when possible since there is no penalty anymore.

Test Plan: I browsed many, not all, timelines in my sandbox and in IE8. Some of these were just swagged, but I'm expecting we'll do more SB testing before landing.

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8827
2014-04-22 08:25:54 -07:00
epriestley
499f66963d Minor, fix the scoping of a static variable
Auditors: chad, btrahan
2014-04-11 04:24:58 -07:00
Bob Trahan
2d43cf1296 OAuth - add a little notes section for admins to remember details about external accounts
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.

Test Plan: looked at providers and read pretty instructions.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4755

Differential Revision: https://secure.phabricator.com/D8726
2014-04-09 11:09:50 -07:00
epriestley
582ec54465 Add a checkbox to the LDAP auth configuration UI to "Always Search"
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.

Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??

Reviewers: Firehed

Reviewed By: Firehed

Subscribers: epriestley

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8723
2014-04-08 11:36:23 -07:00
epriestley
750c872839 Update Google auth documentation to discuss "Google+ API" and new console URI
Summary: Fixes T4451. See also D8612.

Test Plan: Viewed panel and read text, saw it matched up with the new console.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4451

Differential Revision: https://secure.phabricator.com/D8613
2014-03-25 13:36:47 -07:00
epriestley
d8713f6f0b Make dialogs a little easier to use
Summary:
  - Dialog pages currently have no titles or crumbs, and look shoddy. Add titles and crumbs.
  - Dialog titles aren't always great for crumbs, add an optional "short title" for crumbs.
  - `AphrontDialogResponse` is pure boilerplate. Allow controllers to just return a `DialogView` instead and get the same effect.
  - Building dialogs requires a bit of boilerplate, and we generally construct them with no explicit `"action"`, which has some issues with T4593. Provide a convenience method to set the viewer and get a reasonable, explict submit URI.

Test Plan:
  - Viewed dialog on its own.
  - Viewed dialog as a dialog.

{F132353}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8577
2014-03-21 14:40:05 -07:00
epriestley
ba8925a531 Support multiple LDAP filters in the Phabricator UI
Summary: Ref T3208. Not ready for prime time yet.

Test Plan: ldap T.T

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, frgtn, aran

Maniphest Tasks: T3208

Differential Revision: https://secure.phabricator.com/D8160
2014-03-17 15:08:30 -07:00
epriestley
aea624118b Allow users to terminate login sessions
Summary:
This is partly a good feature, and partly should reduce false positives on HackerOne reporting things vaguely related to this.

Allow a user to terminate login sessions from the settings panel.

Test Plan:
  - Terminated a session.
  - Terminated all sessions.
  - Tried to terminate all sessions again.
  - Logged in with two browsers, terminated the other browser's session, reloaded, got kicked out.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8556
2014-03-17 15:02:01 -07:00
epriestley
559c0fe886 Tune cookie behaviors for 'phcid', 'phreg', etc
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
2014-03-14 14:33:31 -07:00
epriestley
f7b1ed7221 Fix two registration errors for unusual provider emails
Summary:
See <https://github.com/facebook/phabricator/issues/541>.

  - If a provider returns the email `""` or `"0"`, we currently don't let the user edit it and thus don't let them register.
  - If a provider returns an invalid email like `"!!!"` (permitted by GitHub, e.g.), we show them a nonsense error message.

Instead:

  - Pretend we didn't get an address if we get an invalid address.
  - Test the address strictly against `null`.

Test Plan: Registered on Phabricator with my GitHub email set to `""` (empty string) and `"!!!"` (bang bang bang).

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8528
2014-03-13 19:03:12 -07:00
epriestley
a9f38e55e5 Modernize Facebook OAuth instructions
Summary: Ref T4593. We have somewhat outdated instructions, and safer setup instructions are now available.

Test Plan: Read instructions.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8518
2014-03-13 13:00:47 -07:00
epriestley
969d0c3e8d Use "\z" instead of "$" to anchor validating regular expressions
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.

When we care about this, use `\z` instead, which matches "end of input" only.

This allowed registration of `"username\n"` and similar.

Test Plan:
  - Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
  - Fixed the ones where this seemed like it could have an impact.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8516
2014-03-13 12:42:41 -07:00
epriestley
7176240717 Whitelist controllers which can receive a 'code' parameter
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
2014-03-12 11:30:04 -07:00
epriestley
e62c7321c2 Automatically verify the setup account's email address
Summary: Although the defaults don't require a verified email address, it's easy to lock yourself out by accident by configuring `auth.require-email-verification` or `auth.email-domains` before setting up email. Just force-verify the initial/setup account's address.

Test Plan: Went through setup on a fresh install, saw address verify.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8365
2014-02-27 15:16:04 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
epriestley
bcf255e9c9 Require CSRF submission to verify email addresses
Summary: If an attacker somehow intercepts a verification URL for an email address, they can hypothetically CSRF the account owner into verifying it. What you'd do before (how do you get the link?) and after (why do you care that you tricked them into verifying) performing this attack is unclear, but in theory we should require a CSRF submission here; add one.

Test Plan: {F118691}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8351
2014-02-26 11:17:46 -08:00
epriestley
14627ad65b Fix an incorrectly spelled call on the registration error pathway
Summary: If you copy the registration URL, then register, then load the URL again while logged out (i.e., attempt to reuse the registration URL), we try to show you a tailored error message. However, this call is not correct so we show you a not-so tailored exception instead.

Test Plan:
  - Get to the registration screen.
  - Save URL.
  - Complete registration.
  - Log out.
  - Return to saved URL.

Previously, exception. Now, readable error.

{F117585}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D8322
2014-02-24 11:45:28 -08:00
epriestley
a566ae3730 Require a CSRF code for Twitter and JIRA (OAuth 1) logins
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
2014-02-23 16:39:24 -08:00
epriestley
7cf0358dda Disallow email addresses which will overflow MySQL storage
Summary:
Via HackerOne. An attacker can bypass `auth.email-domains` by registering with an email like:

  aaaaa...aaaaa@evil.com@company.com

We'll validate the full string, then insert it into the database where it will be truncated, removing the `@company.com` part. Then we'll send an email to `@evil.com`.

Instead, reject email addresses which won't fit in the table.

`STRICT_ALL_TABLES` stops this attack, I'm going to add a setup warning encouraging it.

Test Plan:
  - Set `auth.email-domains` to `@company.com`.
  - Registered with `aaa...aaa@evil.com@company.com`. Previously this worked, now it is rejected.
  - Did a valid registration.
  - Tried to add `aaa...aaaa@evil.com@company.com` as an email address. Previously this worked, now it is rejected.
  - Did a valid email add.
  - Added and executed unit tests.

Reviewers: btrahan, arice

Reviewed By: arice

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D8308
2014-02-23 10:19:35 -08:00
epriestley
580bcd0d2b Implement bcrypt hasher, transparent login upgrade, and explicit upgrade for passwords
Summary:
Ref T4443.

  - Add a `password_hash()`-based bcrypt hasher if `password_hash()` is available.
  - When a user logs in using a password, upgrade their password to the strongest available hash format.
  - On the password settings page:
    - Warn the user if their password uses any algorithm other than the strongest one.
    - Show the algorithm the password uses.
    - Show the best available algorithm.

Test Plan: As an md5 user, viewed password settings page and saw a warning. Logged out. Logged in, got upgraded, no more warning. Changed password, verified database rehash. Logged out, logged in.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8270
2014-02-18 14:09:36 -08:00
epriestley
3c9153079f Make password hashing modular
Summary:
Ref T4443. Make hashing algorithms pluggable and extensible so we can deal with the attendant complexities more easily.

This moves "Iterated MD5" to a modular implementation, and adds a tiny bit of hack-glue so we don't need to migrate the DB in this patch. I'll migrate in the next patch, then add bcrypt.

Test Plan:
  - Verified that the same stuff gets stored in the DB (i.e., no functional changes):
    - Logged into an old password account.
    - Changed password.
    - Registered a new account.
    - Changed password.
    - Switched back to master.
    - Logged in / out, changed password.
    - Switched back, logged in.
  - Ran unit tests (they aren't super extensive, but cover some of the basics).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, kofalt

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8268
2014-02-18 14:09:36 -08:00
Chad Little
4743ad9649 Miniturize the nav buttons
Summary: This uses the slightly smaller icons. Not sure about the logout icon, will play with it more in the morning.

Test Plan: tested new nav on desktop and mobile.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8119
2014-01-31 09:10:32 -08:00
epriestley
049fb2018b Add very basic "quick create" menu
Summary:
Ref T3623. This is like a pre-v0, in that it doesn't have a dropdown yet.

Clicking the button takes you to a page which can serve as a right click / mobile / edit target in the long run, but is obviously not great for desktop use. I'll add the dropdown in the next iteration.

Test Plan: {F105631}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3623

Differential Revision: https://secure.phabricator.com/D8088
2014-01-28 20:18:01 -08:00
epriestley
152f05aebe Fix some security issues with email password resets
Summary:
Via HackerOne, there are two related low-severity issues with this workflow:

  - We don't check if you're already logged in, so an attacker can trick a victim (whether they're logged in or not) into clicking a reset link for an account the attacker controls (maybe via an invisible iframe) and log the user in under a different account.
  - We don't check CSRF tokens either, so after fixing the first thing, an attacker can still trick a //logged-out// victim in the same way.

It's not really clear that doing this opens up any significant attacks afterward, but both of these behaviors aren't good.

I'll probably land this for audit in a few hours if @btrahan doesn't have a chance to take a look at it since he's probably on a plane for most of the day, I'm pretty confident it doesn't break anything.

Test Plan:
  - As a logged-in user, clicked another user's password reset link and was not logged in.
  - As a logged-out user, clicked a password reset link and needed to submit a form to complete the workflow.

Reviewers: btrahan

CC: chad, btrahan, aran

Differential Revision: https://secure.phabricator.com/D8079
2014-01-27 16:53:04 -08:00
epriestley
febc494737 Actually check CSRF on Password and LDAP forms
Summary: Ref T4339. We didn't previously check `isFormPost()` on these, but now should.

Test Plan: Changed csrf token on login, got kicked out.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8051
2014-01-23 14:18:26 -08:00
epriestley
5b1d9c935a After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:

  - User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
  - User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).

To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.

The comment in the body text describes this in more detail.

This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.

Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3793

Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 14:16:08 -08:00
epriestley
f9ac534f25 Support CSRF for logged-out users
Summary: Fixes T4339. If you're anonymous, we use a digest of your session key to generate a CSRF token. Otherwise, everything works normally.

Test Plan: Logged out, logged in, tweaked CSRF in forms -- I'll add some inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8046
2014-01-23 14:03:54 -08:00
epriestley
24544b1a2f Straighten out absolute/relative URIs in login providers
Summary:
Ref T4339. Login providers use absolute URIs, but the ones that rely on local form submits should not, because we want to include CSRF tokens where applicable.

Instead, make the default be relative URIs and turn them into absolute ones for the callback proivders.

Test Plan: Clicked, like, every login button.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8045
2014-01-23 14:03:44 -08:00
epriestley
69ddb0ced6 Issue "anonymous" sessions for logged-out users
Summary:
Ref T4339. Ref T4310. Currently, sessions look like `"afad85d675fda87a4fadd54"`, and are only issued for logged-in users. To support logged-out CSRF and (eventually) external user sessions, I made two small changes:

  - First, sessions now have a "kind", which is indicated by a prefix, like `"A/ab987asdcas7dca"`. This mostly allows us to issue session queries more efficiently: we don't have to issue a query at all for anonymous sessions, and can join the correct table for user and external sessions and save a query. Generally, this gives us more debugging information and more opportunity to recover from issues in a user-friendly way, as with the "invalid session" error in this diff.
  - Secondly, if you load a page and don't have a session, we give you an anonymous session. This is just a secret with no special significance.

This does not implement CSRF yet, but gives us a client secret we can use to implement it.

Test Plan:
  - Logged in.
  - Logged out.
  - Browsed around.
  - Logged in again.
  - Went through link/register.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T4339

Differential Revision: https://secure.phabricator.com/D8043
2014-01-23 14:03:22 -08:00
epriestley
0727418023 Consolidate use of magical cookie name strings
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
2014-01-23 14:01:35 -08:00
epriestley
02aa193cb0 Add a common password blacklist
Summary:
Fixes T4143. This mitigates the "use a botnet to slowly try to login to every user account using the passwords '1234', 'password', 'asdfasdf', ..." attack, like the one that hit GitHub.

(I also donated some money to Openwall as a thanks for compiling this wordlist.)

Test Plan:
  - Tried to register with a weak password; registered with a strong password.
  - Tried to set VCS password to a weak password; set VCS password to a strong password.
  - Tried to change password to a weak password; changed password to a strong password.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Maniphest Tasks: T4143

Differential Revision: https://secure.phabricator.com/D8048
2014-01-23 14:01:18 -08:00
epriestley
2ec45d42a6 Remove session limits and sequencing
Summary:
Ref T4310. Fixes T3720. This change:

  - Removes concurrent session limits. Instead, unused sessions are GC'd after a while.
  - Collapses all existing "web-1", "web-2", etc., sessions into "web" sessions.
  - Dramatically simplifies the code for establishing a session (like omg).

Test Plan: Ran migration, checked Sessions panel and database for sanity. Used existing session. Logged out, logged in. Ran Conduit commands.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7978
2014-01-15 17:27:59 -08:00
epriestley
acb141cf52 Expire and garbage collect unused sessions
Summary:
Ref T3720. Ref T4310. Currently, we limit the maximum number of concurrent sessions of each type. This is primarily because sessions predate garbage collection and we had no way to prevent the session table from growing fairly quickly and without bound unless we did this.

Now that we have GC (and it's modular!) we can just expire unused sessions after a while and throw them away:

  - Add a `sessionExpires` column to the table, with a key.
  - Add a GC for old sessions.
  - When we establish a session, set `sessionExpires` to the current time plus the session TTL.
  - When a user uses a session and has used up more than 20% of the time on it, extend the session.

In addition to this, we could also rotate sessions, but I think that provides very little value. If we do want to implement it, we should hold it until after T3720 / T4310.

Test Plan:
  - Ran schema changes.
  - Looked at database.
  - Tested GC:
    - Started GC.
    - Set expires on one row to the past.
    - Restarted GC.
    - Verified GC nuked the session.
  - Logged in.
  - Logged out.
  - Ran Conduit method.
  - Tested refresh:
    - Set threshold to 0.0001% instead of 20%.
    - Loaded page.
    - Saw a session extension ever few page loads.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7976
2014-01-15 13:56:16 -08:00
epriestley
a64228b03f Give the session table a normal id column as a primary key
Summary:
Ref T4310. Ref T3720. Two major things are going on here:

  - I'm making this table work more like a standard table, which, e.g., makes `delete()` simpler to implement.
  - Currently, the primary key is `(userPHID, type)`. I want to get rid of this, issue unlimited sessions, and GC old sessions. This means we can't have a unique key on `(userPHID, type)` anymore. This removes it as the primary key and adds it as a normal key instead. There's no functional change -- the code to generate sessions guarantees that it will never write duplicate rows or write additional rows -- but allows us to drop the `-1`, `-2` qualifiers in the future.
  - Also of note, our task is made far simpler here because MySQL will automatically assign values to new `AUTO_INCREMENT` columns, so we don't have to migrate to get real IDs.

Test Plan: Ran migrations, verified table looked sane. Logged out, logged in.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3720, T4310

Differential Revision: https://secure.phabricator.com/D7975
2014-01-15 13:55:18 -08:00
Bob Trahan
41d2a09536 Legalpad - make it work for not logged in users
Summary: Adds "verified" and "secretKey" to Legalpad document signatures. For logged in users using an email address they own, things are verified right away. Otherwise, the email is sent a verification letter. When the user clicks the link the signature is marked verified.

Test Plan: signed the document with a bogus email address not logged in. verified the email that would be sent looked good from command line. followed link and successfully verified bogus email address

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, asherkin

Maniphest Tasks: T4283

Differential Revision: https://secure.phabricator.com/D7930
2014-01-14 17:17:18 -08:00
Chad Little
31a2bebf63 Move PhabricatorTagView to PHUITagView
Summary: For consistency and great justice.

Test Plan: tested audit, uiexamples, action headers

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7967
2014-01-14 14:09:52 -08:00
epriestley
d392a8f157 Replace "web" and "conduit" magic session strings with constants
Summary: Ref T4310. Ref T3720. We use bare strings to refer to session types in several places right now; use constants instead.

Test Plan: grep; logged out; logged in; ran Conduit commands.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7963
2014-01-14 13:22:34 -08:00
epriestley
eef314b701 Separate session management from PhabricatorUser
Summary: Ref T4310. Ref T3720. Session operations are currently part of PhabricatorUser. This is more tightly coupled than needbe, and makes it difficult to establish login sessions for non-users. Move all the session management code to a `SessionEngine`.

Test Plan:
  - Viewed sessions.
  - Regenerated Conduit certificate.
  - Verified Conduit sessions were destroyed.
  - Logged out.
  - Logged in.
  - Ran conduit commands.
  - Viewed sessions again.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7962
2014-01-14 13:22:27 -08:00
epriestley
3d9e328fb3 Add an "active login sessions" table to Settings
Summary: Ref T4310. Ref T3720. Partly, this makes it easier for users to understand login sessions. Partly, it makes it easier for me to make changes to login sessions for T4310 / T3720 without messing anything up.

Test Plan: {F101512}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3720, T4310

Differential Revision: https://secure.phabricator.com/D7954
2014-01-14 11:05:45 -08:00
epriestley
220d680f37 Allow PhabricatorUserLog to store non-user PHIDs
Summary:
Ref T4310. This is a small step toward separating out the session code so we can establish sessions for `ExternalAccount` and not just `User`.

Also fix an issue with strict MySQL and un-admin / un-disable from web UI.

Test Plan: Logged in, logged out, admined/de-admin'd user, added email address, checked user log for all those events.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310

Differential Revision: https://secure.phabricator.com/D7953
2014-01-14 11:05:26 -08:00
Chad Little
b74c7a3d37 Simplify PHUIObjectBoxViews handling of Save and Error states
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.

Test Plan: Test out the forms, see errors without the text.

Reviewers: epriestley, btrahan

CC: Korvin, epriestley, aran, hach-que

Differential Revision: https://secure.phabricator.com/D7924
2014-01-10 09:17:37 -08:00
epriestley
324ee4e4d5 Fix rendering issue for LDAP configuration provider transactions
Summary: Fixes T4290. At least one of the fields (`realname`) may have a list of items, and `strlen(array('first', 'last'))` produces the warning and stack trace in T4290.

Test Plan:
  - Edited `realname` from an array value to an array value.
  - Hit error.
  - Applied patch.
  - No more error.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4290

Differential Revision: https://secure.phabricator.com/D7905
2014-01-08 13:27:08 -08:00
epriestley
3524ba3fb5 Add warnings to JIRA auth provider that we only support JIRA 6
Summary: Ref T4289. Make it clear that this provider does not currently work with JIRA 5.

Test Plan: Viewed JIRA provider from `/auth/`, saw warnings.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4289

Differential Revision: https://secure.phabricator.com/D7906
2014-01-08 09:43:44 -08:00
epriestley
e397103bf2 Extend all "ManagementWorkflow" classes from a base class
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.

Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.

Test Plan: Lint; ran some of the scripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7838
2013-12-27 13:15:40 -08:00
epriestley
a5dc9067af Provide convenience method addTextCrumb() to PhabricatorCrumbsView
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
2013-12-18 17:47:34 -08:00
epriestley
e4920cdf86 Provide an LDAPS example in LDAP auth
Summary: Fixes T4148. LDAPS works with "ldaps://", it just isn't documented or clear.

Test Plan: {F84893}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4148

Differential Revision: https://secure.phabricator.com/D7652
2013-11-25 14:55:44 -08:00
epriestley
3a035c02e7 Recover more flexibly from an already-verified email
Summary:
Ref T4140. We could hit a redirect loop for a user with a verified primary email address but no "is verified" flag on their account. This shouldn't be possible since the migration should have set the flag, but we can deal with it more gracefully when it does happen (maybe because users forgot to run `storage/upgrade`, or because of ghosts).

In the controller, check the same flag we check before forcing the user to the controller.

When verifying, allow the verification if either the email or user flag isn't set.

Test Plan: Hit `/login/mustverify/`; verified an address.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4140

Differential Revision: https://secure.phabricator.com/D7621
2013-11-21 14:41:32 -08:00
epriestley
a518626a85 Slightly improve behavior for unverified + unapproved users
Summary: Ref T4140. Allow unapproved users to verify their email addresses. Currently, unapproved blocks email verification, but should not.

Test Plan: Clicked email verification link as an unapproved user, got email verified.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4140

Differential Revision: https://secure.phabricator.com/D7618
2013-11-21 12:58:58 -08:00
epriestley
6eb02af314 Allow "bin/auth recover" to succeed before phabricator.base-uri is set
Summary:
Fixes T4132. If you run "bin/auth recover" before setting the base URI, it throws when trying to generate a production URI.

Instead, just show the path. We can't figure out the domain, and I think this is less confusing than showing "your.phabricator.example.com", etc.

Test Plan: Ran `bin/auth recover <user>` for valid and missing base-uri.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4132

Differential Revision: https://secure.phabricator.com/D7615
2013-11-20 10:36:00 -08:00
Aviv Eyal
dcf909ba56 Land to GitHub + support stuff
Summary:
A usable, Land to GitHub flow.

Still to do:
- Refactor all git/hg stratagies to a sane structure.
- Make the dialogs Workflow + explain why it's disabled.
- Show button and request Link Account if GH is enabled, but user is not linked.
- After refreshing token, user ends up in the settings stage.

Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.

Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.

Test Plan:
Went through these flows:
- load revision in hosted, github-backed, non-github backed repos to see button as needed.
- hit land with weak token - sent to refresh it with the extra scope.
- Land to repo I'm not allowed - got proper error message.
- Successfully landed; Failed to apply patch.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T182

Differential Revision: https://secure.phabricator.com/D7555
2013-11-13 17:25:24 -08:00
epriestley
87a655e8c5 Fix new logged-out "Login" button URI and workflowiness
Summary: Whelp apparently I never actually clicked this.

Auditors: btrahan
2013-11-13 11:48:24 -08:00
epriestley
fb6e38548b Respect "can edit username" in registration UI
Summary:
Fixes T3741. The flag is respected in terms of actually creating the account, but the UI is a bit unclear.

This can never occur naturally, but installs can register an event which locks it.

Test Plan:
Artificially locked it, verified I got more reasonable UI;

{F81282}

Reviewers: btrahan, datr

Reviewed By: datr

CC: aran

Maniphest Tasks: T3741

Differential Revision: https://secure.phabricator.com/D7577
2013-11-13 11:25:43 -08:00
epriestley
c0e1a63a63 Implement an approval queue
Summary:
  - Add an option for the queue.
  - By default, enable it.
  - Dump new users into the queue.
  - Send admins an email to approve them.

Test Plan:
  - Registered new accounts with queue on and off.
  - As an admin, approved accounts and disabled the queue from email.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7576
2013-11-13 11:24:56 -08:00
epriestley
0fa411083f Show an "approval queue" item on the home page for admins, and sort out menu item visibility
Summary:
  - If you're an administrator and there are users waiting for approval, show a count on the home page.
  - Sort out the `isUserActivated()` access check.
  - Hide all the menu widgets except "Logout" for disabled and unapproved users.
  - Add a "Log In" item.
  - Add a bunch of unit tests.

Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D7574
2013-11-13 11:24:38 -08:00
epriestley
c8320923c4 Implement most of the administrative UI for approval queues
Summary:
Nothing fancy here, just:

  - UI to show users needing approval.
  - "Approve" and "Disable" actions.
  - Send "Approved" email on approve.
  - "Approve" edit + log operations.
  - "Wait for Approval" state for users who need approval.

There's still no natural way for users to end up not-approved -- you have to write directly to the database.

Test Plan: See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7573
2013-11-13 11:24:18 -08:00
epriestley
7f11e8d740 Improve handling of email verification and "activated" accounts
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:

  - Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
    - Migrate all the existing users.
    - When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
    - Just make the checks look at the `isEmailVerified` field.
  - Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
  - Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
    - When the queue is enabled, registering users are created with `isApproved = false`.
    - Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
    - They go to the web UI and approve the user.
    - Manually-created accounts are auto-approved.
    - The email will have instructions for disabling the queue.

I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.

Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.

Test Plan:
  - Ran migration, verified `isEmailVerified` populated correctly.
  - Created a new user, checked DB for verified (not verified).
  - Verified, checked DB (now verified).
  - Used Conduit, People, Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7572
2013-11-12 14:37:04 -08:00
epriestley
cd73fe78db Roadblock users trying to register with external accounts that have invalid emails
Summary:
Ref T3472. Currently, if an install only allows "@mycompany.com" emails and you try to register with an "@personal.com" account, we let you pick an "@mycompany.com" address instead. This is secure: you still have to verify the email. However, it defies user expectation -- it's somewhat confusing that we let you register. Instead, provide a hard roadblock.

(These accounts can still be linked, just not used for registration.)

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3472

Differential Revision: https://secure.phabricator.com/D7571
2013-11-12 14:36:49 -08:00
epriestley
30a51dac36 Clarify registration rules more aggressively when configuring auth
Summary: See private chatter. Make it explicitly clear when adding a provider that anyone who can browse to Phabricator can register.

Test Plan: See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7570
2013-11-12 10:56:47 -08:00
Jakub Vrana
a29b5b070f Replace some hsprintf() by phutil_tag()
Test Plan: Looked at a diff with inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7549
2013-11-11 09:23:23 -08:00
Aviv Eyal
2250ee6aa6 Allow null for token expiration date
Summary: At least under GitHub, the token value is stored as "null", and not missing. And `null > anything` is false, so Phabricator thinks the token is expired or not there.

Test Plan: http://ph.vm/settings/panel/external/ before shows "No OAuth Access Token," and after it says "Active OAuth Token".

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7466
2013-10-30 17:19:46 -07:00
epriestley
2a5c987c71 Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.

This has several parts:

  - For PolicyAware queries, provide an application class name method.
  - If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
  - For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.

Test Plan:
  - Added a unit test to verify I got all the class names right.
  - Browsed around, logged in/out as a normal user with public policies on and off.
  - Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7367
2013-10-21 17:20:27 -07:00
Chad Little
d2895249ee Add Persona login icon
Summary: Adds the new icon 1x and 2x

Test Plan: photoshop

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3958

Differential Revision: https://secure.phabricator.com/D7316
2013-10-14 16:33:49 -07:00
epriestley
0ce4f6d176 Add Persona auth provider
Summary: Ref T3958. Adds a provider for Mozilla's Persona auth.

Test Plan:
  - Created a Persona provider.
  - Registered a new account with Persona.
  - Logged in with Persona.
  - Linked an account with Persona.
  - Dissolved an account link with Persona.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3958

Differential Revision: https://secure.phabricator.com/D7313
2013-10-14 14:34:57 -07:00
epriestley
7dde01df76 Fix issues with first-time account registration
Summary: This worked originally, but the migration broke slightly after the
config was deprecated, and there was another minor issue during setup.
2013-10-05 08:02:41 -07:00
Chad Little
cad9e548bc Add Header to Registration
Summary: Adds an ObjectBox to Phabricator Registration

Test Plan: check logged out page for new header.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7223
2013-10-04 15:13:05 -07:00
epriestley
2e5ac128b3 Explain policy exception rules to users
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".

This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7150
2013-09-27 08:43:41 -07:00
Chad Little
9be7a948f9 Move PHUIFormBoxView to PHUIObjectBoxView
Summary: I'd like to reuse this for other content areas, renaming for now. This might be weird to keep setForm, but I can fix that later if we need.

Test Plan: reload a few forms in maniphest, projects, differential

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7120
2013-09-25 11:23:29 -07:00
Gareth Evans
e1892e9bfb Add reCaptcha to password registration
Summary: See task

Test Plan:
Attempt to signup with recaptcha disabled.
Attempt to signup with recaptcha enabled with incorrect value.
Attempt to signup with recaptcha enabled with correct value.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Maniphest Tasks: T3832

Differential Revision: https://secure.phabricator.com/D7053
2013-09-20 14:54:57 -07:00
Chad Little
5ba20b8924 Move PhabricatorObjectItem to PHUIObjectItem, add 'plain' setting for lists.
Summary: Adds plain support for object lists that just look like lists

Test Plan: review UIexamples and a number of other applications

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6922
2013-09-09 14:14:34 -07:00
epriestley
825fb9c85a Add JIRA doorkeeper and remarkup support
Summary:
Ref T3687. Adds a Doorkeeper bridge for JIRA issues, plus remarkup support. In particular:

  - The Asana and JIRA remarkup rules shared most of their implementation, so I refactored what I could into a base class.
  - Actual bridge implementation is straightforward and similar to Asana, although probably not similar enough to really justify refactoring.

Test Plan:
  - When logged in as a JIRA-connected user, pasted a JIRA issue link and saw it enriched at rendering time.
  - Logged in and out with JIRA.
  - Tested an Asana link, too (seems I haven't broken anything).

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6878
2013-09-03 17:27:38 -07:00
epriestley
e5b4ce5525 Reduce the amount of OAuth1/OAuth2 code duplication for rendering login buttons
Summary: Ref T3687. These buttons don't work quite the same way, but are similar enough that the code seems worth consolidating.

Test Plan: Viewed and clicked both OAuth1 (Twitter, JIRA) and OAuth2 (Facebook) login buttons. Got logins.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6874
2013-09-03 10:30:53 -07:00
epriestley
25eb401e18 Handle user aborts during auth workflows in Phabricator
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
2013-09-03 10:30:39 -07:00
epriestley
4e12a375f3 Add JIRA as an authentication provider
Summary:
Ref T3687. Depends on D6867. This allows login/registration through JIRA.

The notable difference between this and other providers is that we need to do configuration in two stages, since we need to generate and save a public/private keypair before we can give the user configuration instructions, which takes several seconds and can't change once we've told them to do it.

To this effect, the edit form renders two separate stages, a "setup" stage and a "configure" stage. In the setup stage the user identifies the install and provides the URL. They hit save, we generate a keypair, and take them to the configure stage. In the configure stage, they're walked through setting up all the keys. This ends up feeling a touch rough, but overall pretty reasonable, and we haven't lost much generality.

Test Plan: {F57059}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6868
2013-09-03 05:53:21 -07:00
epriestley
25e43e872b Add Twitter as an authentication provider
Summary: Ref T3687. Depends on D6864. Implements the `OAuth1` provider in Phabricator (which is mostly similar to the OAuth2 provider, but doesn't share quite enough code to actually extend a common base class, I think) and Twitter as a concrete subclass.

Test Plan:
Created a Twitter provider. Registered, logged in, linked, refreshed account link.

{F57054}

{F57056}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3687

Differential Revision: https://secure.phabricator.com/D6865
2013-09-03 05:53:08 -07:00
Chad Little
fe2a96e37f Update Form Layouts
Summary:
This attempts some consistency in form layouts. Notably, they all now contain headers and are 16px off the sides and tops of pages. Also updated dialogs to the same look and feel. I think I got 98% of forms with this pass, but it's likely I missed some buried somewhere.

TODO: will take another pass as consolidating these colors and new gradients in another diff.

Test Plan: Played in my sandbox all week. Please play with it too and let me know how they feel.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6806
2013-08-26 11:53:11 -07:00
epriestley
f034fd80db Remove getApplicationObjectTypeName from ApplicationTransactions
Summary:
We can get this out of PHIDType reasonably in all cases and simplify implementation here.

None of these translate correctly anyway so they're basically debugging/development strings.

Test Plan: `grep`, browsed some transactions

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D6786
2013-08-21 12:32:06 -07:00
epriestley
751cd547c2 Remove dust from page construction
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
2013-08-19 18:09:35 -07:00
Chad Little
fe766ff683 Fix twitch name
Summary: fix spelling

Test Plan: i didn't test this, but seems ok?

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6728
2013-08-12 11:41:03 -07:00
epriestley
07dd5df33b Support Twitch.tv as an OAuth provider
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
2013-08-08 13:34:30 -07:00
epriestley
5cc3bbf721 Use application PHIDs for application transactions
Summary: Ref T2715. Ref T3578. Load application transactions through application PHID infrastructure.

Test Plan: Viewed feed, saw successful loads of application transaction objects and rendered feed stories.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2715, T3578

Differential Revision: https://secure.phabricator.com/D6617
2013-07-29 12:04:15 -07:00
epriestley
cf9dc5d189 Fix bug when multiple comment forms appear on a single page
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
2013-07-29 12:04:10 -07:00
epriestley
db3a0c90bb Use Application PHIDs for XUSR
Summary: Ref T2715. XUSR -> apps

Test Plan: `phid.query`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2715

Differential Revision: https://secure.phabricator.com/D6558
2013-07-24 14:12:39 -07:00
epriestley
ab5ee5d193 Survive mild misconfiguration of Realname Attributes in LDAP
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
2013-07-18 12:40:57 -07:00
epriestley
cff8c50903 Modernize email verification page
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
2013-07-10 18:53:09 -07:00
epriestley
2432a47997 Reduce invasiveness of bin/auth ldap
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
2013-07-09 16:23:12 -07:00
epriestley
1888a84b7e Fix an issue with setting "Real Name Attributes" in LDAP auth
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
2013-07-09 16:22:50 -07:00
epriestley
5ecb77427a Fix OAuth token refresh return value
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
2013-06-25 16:31:01 -07:00
epriestley
fe71b34c68 Add a "refresh" action for external accounts
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
2013-06-24 15:58:27 -07:00
epriestley
e826842179 Show more information about OAuth tokens in the Account Settings -> External Accounts screen
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
2013-06-24 15:57:39 -07:00
epriestley
f8ed6422f8 Provide an auto-refresh mechanism for OAuth providers to deliver fresh tokens
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
2013-06-24 15:56:01 -07:00
epriestley
c94ef134e4 Add bin/auth refresh for debugging OAuth token refresh issues
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
2013-06-24 15:55:41 -07:00
epriestley
e723b7e119 Add DoorkeeperObjectRef, DoorkeeperBridge, DoorkeeperBridgeAsana
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
2013-06-24 15:54:54 -07:00
epriestley
71e4870a8e Fix OAuth Facebook Phame property access
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
2013-06-24 12:02:53 -07:00
epriestley
705dfa25e6 Load LDAP provider correctly in bin/auth LDAP debug script
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
2013-06-24 10:37:40 -07:00
epriestley
edee95e355 Tailor the Asana OAuth help URI
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
2013-06-24 10:00:09 -07:00
epriestley
0a044ef275 Make old GitHub OAuth URIs work for now
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
2013-06-21 06:11:57 -07:00
Chad Little
e275f94fd8 Update styles on Login Reset
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
2013-06-20 17:11:57 -07:00
epriestley
46a7c61c80 Improve errors associated with adding new login providers
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
2013-06-20 14:13:53 -07:00
epriestley
619069e234 Show providers in login order, not alphabetical order
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
2013-06-20 14:04:36 -07:00
epriestley
052193ce2d Improve /auth/ behavior when a provider implementation is missing
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
2013-06-20 14:04:20 -07:00
Chad Little
de9cf72a64 Update Reset Password Page 2013-06-20 13:38:19 -07:00
epriestley
f7e27e36da Minor, clean up an obsolete hardcode (no password registration). 2013-06-20 11:25:57 -07:00
epriestley
7eb579788e Minor, fix an issue where creating a provider without changing anything
fails to save it because there are no effective transactions.
2013-06-20 11:23:58 -07:00
epriestley
e117b320e2 Add Amazon and Asana providers
Summary: Ref T1536. Ref T2852. Adds providers for Asana and Amazon. See D6248.

Test Plan:
{F46960}

{F46961}

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536, T2852

Differential Revision: https://secure.phabricator.com/D6249
2013-06-20 11:19:11 -07:00
epriestley
1834584e98 Provide contextual help on auth provider configuration
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
2013-06-20 11:18:48 -07:00
epriestley
3b9ccf11f2 Drive auth config with the database
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
2013-06-20 11:18:11 -07:00
epriestley
d8394b2ee0 Prepare for db-driven auth configuration by making proviers operate in dual modes
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
2013-06-20 11:17:53 -07:00
epriestley
32f6c88896 Add first-time-setup registration flow
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
2013-06-19 16:28:48 -07:00
epriestley
6b1f15ac54 Build out Auth UI a little bit
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
2013-06-19 15:00:37 -07:00
epriestley
73c2c1d2e6 Send old login code to the bottom of the sea
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
2013-06-19 01:33:27 -07:00
epriestley
8c8ab25fa1 Restore/respect "require secure browsing" for Facebook (phabricator)
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
2013-06-18 15:52:01 -07:00
epriestley
26944e2876 Expose all LDAP configuration options in the web UI
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
2013-06-18 13:58:40 -07:00
epriestley
fe29b27582 Fix several issues with "Logout"
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
2013-06-18 11:05:28 -07:00
epriestley
e58f383d91 Allow authentication providers to store and customize additional configuration
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
2013-06-18 10:02:34 -07:00
epriestley
c0cc7bbfdf Provide bin/auth ldap for LDAP diagnostics
Summary: Ref T1536.

Test Plan: Ran `bin/auth ldap`.

Reviewers: mbishopim3, chad

Reviewed By: mbishopim3

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6218
2013-06-17 13:26:25 -07:00
epriestley
fded36cc21 Improve more crumbs and cancel buttons for auth
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
2013-06-17 12:14:51 -07:00
epriestley
433c6550b2 Add a cancel button, provider crumb, and account card to registration
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
2013-06-17 12:14:25 -07:00
epriestley
30237aaa47 Clean up image loading for ExternalAccounts
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
2013-06-17 12:14:00 -07:00
epriestley
278905543e Add very basic bin/auth tool
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
2013-06-17 10:55:05 -07:00
epriestley
fc2973c5d3 Allow AuthenticationProviderConfig to be enabled and disabled
Summary: Ref T1536. Nothing too exciting here, one TODO about tailoring error messages.

Test Plan:
{F46403}

{F46404}

{F46405}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6204
2013-06-17 10:54:08 -07:00
epriestley
07423211e9 Show edit transactions for AuthProviders
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
2013-06-17 10:53:29 -07:00
epriestley
c6374e25d5 Very rough edit workflow for AuthProvider configuration
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
2013-06-17 10:52:38 -07:00
epriestley
abb367dd5b Add initial create screen for auth providers
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
2013-06-17 10:51:35 -07:00
epriestley
b927dc057d Add List/ApplicationSearch to AuthProviderConfig
Summary: Ref T1536. Adds a list controller and ApplicationSearch integration for listing providers.

Test Plan: {F46308}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6198
2013-06-17 10:50:43 -07:00
epriestley
f0ddfe6565 Add PhabricatorAuthProviderConfigQuery
Summary: Ref T1536. See D6196. Code not called yet.

Test Plan: Static checks only.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6197
2013-06-17 10:49:18 -07:00
epriestley
5f29ccaaca Add storage for Auth configuration in preparation for moving it into a web interface
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
2013-06-17 10:48:41 -07:00
epriestley
86ff112eeb Fix some account profile image derps
Summary: Ref T1536. Several derps here.

Test Plan:
Oh hey this works now:

{F46610}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6211
2013-06-17 07:22:20 -07:00
epriestley
7271547132 Show account cards for external accounts on "linked accounts" screen and "link new account"
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
2013-06-17 07:08:50 -07:00