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

73 commits

Author SHA1 Message Date
epriestley
0e1bbbd489 Allow administrators to change usernames
Summary:
Give them a big essay about how it's dangerous, but allow them to do it formally.

Because the username is part of the password salt, users must change their passwords after a username change.

Make password reset links work for already-logged-in-users since there's no reason not to (if you have a reset link, you can log out and use it) and it's much less confusing if you get this email and are already logged in.

Depends on: D2651

Test Plan: Changed a user's username to all kinds of crazy things. Clicked reset links in email. Tried to make invalid/nonsense name changes.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1303

Differential Revision: https://secure.phabricator.com/D2657
2012-06-06 07:09:56 -07:00
epriestley
0a7b4591ef Allow usernames to include ".", "-" and "_"
Summary:
See T1303, which presents a reasonable case for inclusion of these characters in valid usernames.

Also, unify username validity handling.

Test Plan: Created a new user with a valid name. Tried to create a new user with an invalid name. Ran unit tests.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1303

Differential Revision: https://secure.phabricator.com/D2651
2012-06-06 07:09:05 -07:00
vrana
6cc196a2e5 Move files in Phabricator one level up
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.

NOTE: `arc diff` timed out so I'm pushing it without review.

Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.

Auditors: epriestley

Maniphest Tasks: T1103
2012-06-01 12:32:44 -07:00
vrana
1ebf9186b4 Depend on class autoloading
Test Plan:
Run setup.
/differential/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2612
2012-05-30 16:57:21 -07:00
epriestley
09c8af4de0 Upgrade phabricator to libphutil v2
Summary: Mechanical changes from D2588. No "Class.php" moves yet.

Test Plan: See D2588.

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1103

Differential Revision: https://secure.phabricator.com/D2591
2012-05-30 14:26:29 -07:00
epriestley
557e508656 Allow restriction of permitted email domains
Summary:
Allow allowed email addresses to be restricted to certain domains. This implies email must be verified.

This probably isn't QUITE ready for prime-time without a few other tweaks (better administrative tools, notably) but we're nearly there.

Test Plan:
  - With no restrictions:
    - Registered with OAuth
    - Created an account with accountadmin
    - Added an email
  - With restrictions:
    - Tried to OAuth register with a restricted address, was prompted to provide a valid one.
    - Tried to OAuth register with a valid address, worked fine.
    - Tried to accountadmin a restricted address, got blocked.
    - Tried to accountadmin a valid address, worked fine.
    - Tried to add a restricted address, blocked.
    - Tried to add a valid address, worked fine.
    - Created a user with People with an invalid address, got blocked.
    - Created a user with People with a valid address, worked fine.

Reviewers: btrahan, csilvers

Reviewed By: csilvers

CC: aran, joe, csilvers

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2581
2012-05-26 06:04:35 -07:00
epriestley
70fd96037b Consolidate user editing code
Summary:
  - We currently have some bugs in account creation due to nontransactional user/email editing.
    - We save $user, then try to save $email. This may fail for various reasons, commonly because the email isn't unique.
    - This leaves us with a $user with no email.
  - Also, logging of edits is somewhat inconsistent across various edit mechanisms.
  - Move all editing to a `PhabricatorUserEditor` class.
  - Handle some broken-data cases more gracefully.

Test Plan:
  - Created and edited a user with `accountadmin`.
  - Created a user with `add_user.php`
  - Created and edited a user with People editor.
  - Created a user with OAuth.
  - Edited user information via Settings.
  - Tried to create an OAuth user with a duplicate email address, got a proper error.
  - Tried to create a user via People with a duplicate email address, got a proper error.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: tberman, aran

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2569
2012-05-25 07:30:44 -07:00
epriestley
77f546c572 Allow installs to require email verification
Summary:
Allow installs to require users to verify email addresses before they can use Phabricator. If a user logs in without a verified email address, they're given instructions to verify their address.

This isn't too useful on its own since we don't actually have arbitrary email registration, but the next step is to allow installs to restrict email to only some domains (e.g., @mycompany.com).

Test Plan:
  - Verification
    - Set verification requirement to `true`.
    - Tried to use Phabricator with an unverified account, was told to verify.
    - Tried to use Conduit, was given a verification error.
    - Verified account, used Phabricator.
    - Unverified account, reset password, verified implicit verification, used Phabricator.
  - People Admin Interface
    - Viewed as admin. Clicked "Administrate User".
    - Viewed as non-admin
  - Sanity Checks
    - Used Conduit normally from web/CLI with a verified account.
    - Logged in/out.
    - Sent password reset email.
    - Created a new user.
    - Logged in with an unverified user but with the configuration set to off.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran, csilvers

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2520
2012-05-21 12:47:38 -07:00
epriestley
905cc5ff32 Minor, fix OAuth registration, see D2431.
Auditors: btrahan
2012-05-09 11:19:15 -07:00
Bob Trahan
679f778235 OAuth -- add support for Disqus
Summary:
also fix some bugs where we weren't properly capturing the expiry value or scope of access tokens.

This code isn't the cleanest as some providers don't confirm what scope you've been granted. In that case, assume the access token is of the minimum scope Phabricator requires. This seems more useful to me as only Phabricator at the moment really easily / consistently lets the user increase / decrease the granted scope so its basically always the correct assumption at the time we make it.

Test Plan: linked and unlinked Phabricator, Github, Disqus and Facebook accounts from Phabricator instaneces

Reviewers: epriestley

Reviewed By: epriestley

CC: zeeg, aran, Koolvin

Maniphest Tasks: T1110

Differential Revision: https://secure.phabricator.com/D2431
2012-05-08 12:08:05 -07:00
epriestley
87207b2f4e Allow users to have multiple email addresses, and verify emails
Summary:
  - Move email to a separate table.
  - Migrate existing email to new storage.
  - Allow users to add and remove email addresses.
  - Allow users to verify email addresses.
  - Allow users to change their primary email address.
  - Convert all the registration/reset/login code to understand these changes.
  - There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
  - This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.

Not included here (next steps):

  - Allow configuration to restrict email to certain domains.
  - Allow configuration to require validated email.

Test Plan:
This is a fairly extensive, difficult-to-test change.

  - From "Email Addresses" interface:
    - Added new email (verified email verifications sent).
    - Changed primary email (verified old/new notificactions sent).
    - Resent verification emails (verified they sent).
    - Removed email.
    - Tried to add already-owned email.
  - Created new users with "accountadmin". Edited existing users with "accountadmin".
  - Created new users with "add_user.php".
  - Created new users with web interface.
  - Clicked welcome email link, verified it verified email.
  - Reset password.
  - Linked/unlinked oauth accounts.
  - Logged in with oauth account.
  - Logged in with email.
  - Registered with Oauth account.
  - Tried to register with OAuth account with duplicate email.
  - Verified errors for email verification with bad tokens, etc.

Reviewers: btrahan, vrana, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1184

Differential Revision: https://secure.phabricator.com/D2393
2012-05-07 10:29:33 -07:00
epriestley
a122336b3e Try to diagnose App Login only for OAuth providers which support it
Summary: We currently try to do "app login" for all OAuth providers, but not all of them support it in a meaningful way. Particularly, it always fails for Google.

Test Plan: Ran google diagnostics on a working config, no longer got a diagnostic failure.

Reviewers: btrahan, vrana, csilvers

Reviewed By: csilvers

CC: aran

Differential Revision: https://secure.phabricator.com/D2377
2012-05-04 16:16:29 -07:00
epriestley
6a04328430 Tighten scope requests with Google OAuth
Summary: We currently make a ludicrously gigantic permission request to do Google auth (read/write access to the entire address book), since I couldn't figure out how to do a more narrowly tailored request when I implemented it. @csilvers pointed me at some much more sensible APIs; we can now just ask for user ID, name, and email address.

Test Plan: Created a new account via Google Oauth. Linked/unlinked an existing account. Verified diagnostics page still works correctly. Logged in with a pre-existing Google account created with the old API (to verify user IDs are the same through both methods).

Reviewers: btrahan, vrana, csilvers, Makinde

Reviewed By: csilvers

CC: aran

Differential Revision: https://secure.phabricator.com/D2378
2012-05-03 13:25:06 -07:00
Bob Trahan
8988667dcc Make Oauth-registration flows a bit more resilient to failures from the providers
Summary: basically by validating we have good user data when we set the user data.

Test Plan: simulated a failure from a phabricator on phabricator oauth scenario. viewed ui that correctly told me there was an error with the provider and to try again.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1077

Differential Revision: https://secure.phabricator.com/D2337
2012-05-01 11:51:40 +02:00
Bob Trahan
8069694640 Fix next_uri
Summary: we were using the "path" as the next_uri and that drops some delicious get parameters

Test Plan: see T1140; basically re-ran the steps listed there and they passed!

Reviewers: epriestley, njhartwell

Reviewed By: epriestley

CC: aran, Koolvin

Maniphest Tasks: T1140, T1009

Differential Revision: https://secure.phabricator.com/D2299
2012-04-22 20:39:54 +02:00
vrana
d9ce80aa17 Explicitly set URL parameter separator for external URLs
Summary:
PHP has this crazy [[ http://php.net/arg_separator.output | arg_separator.output ]] INI setting which allows setting different string for URL parameters separator instead of `&` (e.g. in `?a=1&b=2`).

Don't use it for external URLs.

Test Plan: Log in through OAuth.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2284
2012-04-19 14:54:47 -07:00
vrana
9be054443f Handle refreshing profile image with expired OAuth token
Summary:
If OAuth token is expired then refreshing profile image doesn't work.
This diffs solves it this way:

- Hide Refresh Profile Image button with expired token.
- Display Refresh Token with expired token.
- Update token after logging-in.

Test Plan:
Wait until token expires.
/settings/page/facebook/ - no Refresh Profile Image button.
Refresh Token.
Refresh Profile Image.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: michalburger1, aran, Koolvin

Differential Revision: https://secure.phabricator.com/D2281
2012-04-19 09:30:25 -07:00
vrana
ced84470a9 Move invalid session error to login page
Summary:
With invalid session (which happens for me when I change production and dev db but can of course happen in other cases), Phabricator displays an ugly unhandled exception dialog suggesting to logging in again.
But there's no login dialog on that page.

This also changes how users with invalid session are treated on pages not requiring logging.
Previously, an exception was thrown on them. Now they are treated as unlogged users.

Test Plan: Corrupt session, go to /, login.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran

Differential Revision: https://secure.phabricator.com/D2236
2012-04-14 22:19:04 -07:00
epriestley
2e45839e42 Minor, see rPc92e37c1b1b17b8c937ef618a6e856499c23cc80
Auditors: vrana
2012-03-26 10:03:32 -07:00
epriestley
c92e37c1b1 Minor, pass $request to OAuthLogin controller. See https://secure.phabricator.com/rP4fba549a99f7e9d318012856d0826b10e2bc0845#3c49d03b
Auditors: vrana
2012-03-22 13:43:50 -07:00
vrana
4fba549a99 Use PhabricatorEnv::newObjectFromConfig() wherever possible
Test Plan:
/mail/send/
scripts/aphront/aphrontpath.php /

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1983
2012-03-21 14:57:52 -07:00
epriestley
11cccb98c2 Add "final" to more classes
Summary: No big surprises here, delted the unused "DarkConsole" class.

Test Plan: Ran 'testEverythingImplemented' to verify I wasn't finalizing anything we extend.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

Differential Revision: https://secure.phabricator.com/D1876
2012-03-13 11:18:11 -07:00
epriestley
b2890eeb0e Add "final" to all Phabricator "Controller" classes
Summary:
These are all unambiguously unextensible. Issues I hit:

  - Maniphest Change/Diff controllers, just consolidated them.
  - Some search controllers incorrectly extend from "Search" but should extend from "SearchBase". This has no runtime effects.
  - D1836 introduced a closure, which we don't handle correctly (somewhat on purpose; we target PHP 5.2). See T962.

Test Plan: Ran "testEverythingImplemented" unit test to identify classes extending from `final` classes. Resolved issues.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T795

Differential Revision: https://secure.phabricator.com/D1843
2012-03-09 15:46:25 -08:00
Bob Trahan
0327a5fc69 OAuthServer polish and random sauce
Summary:
This diff makes the OAuthServer more compliant with the spec by
- making it return well-formatted error codes with error types from the spec.
- making it respect the "state" variable, which is a transparent variable the
client passes and the server passes back
- making it be super, duper compliant with respect to redirect uris
-- if specified in authorization step, check if its valid relative to the client
registered URI and if so save it
-- if specified in authorization step, check if its been specified in the access
step and error if it doesn't match or doesn't exist
-- note we don't make any use of it in the access step which seems strange but
hey, that's what the spec says!
This diff makes the OAuthServer suck less by
- making the "cancel" button do something in the user authorization flow
- making the client list view and client edit view be a bit more usable around
client secrets
- fixing a few bugs I managed to introduce along the way

Test Plan:
- create a test phabricator client, updated my conf, and then linked and
unlinked phabricator to itself
- wrote some tests for PhabricatorOAuthServer -- they pass!
-- these validate the various validate URI checks
- tried a few important authorization calls
--
http://phabricator.dev/oauthserver/auth/?client_id=X&state=test&redirect_uri=http://www.evil.com
--- verified error'd from mismatching redirect uri's
--- verified state parameter in response
--- verified did not redirect to client redirect uri
-- http://phabricator.dev/oauthserver/auth/?client_id=X w/ existing
authorization
--- got redirected to proper client url with error that response_type not
specified
-- http://phabricator.dev/oauthserver/auth/?client_id=X&response_type=code w/
existing authorization
--- got redirected to proper client url with pertinent code!
- tried a few important access calls
-- verified appropriate errors if missing any required parameters
-- verified good access code with appropriate other variables resulted in an
access token
- verified that if redirect_uri set correctly in authorization required for
access and errors if differs at all / only succeeds if exactly the same

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley, ajtrichards

Maniphest Tasks: T889, T906, T897

Differential Revision: https://secure.phabricator.com/D1727
2012-03-01 14:46:18 -08:00
Bob Trahan
be66a52050 Make conduit read access_token and login the pertinent $user
Summary: This makes the oauth server a bunch more useful.

Test Plan:
- used /oauth/phabricator/diagnose/ and it actually passed!
- played around with conduit via hacking URL to include access_token on a logged
out browser
- linked my account to itself by going to /settings/page/phabricator/, clicking
"link" account, then cutting and pasting the pertinent ?code=X into
/oauth/phabricator/login/.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Maniphest Tasks: T852

Differential Revision: https://secure.phabricator.com/D1644
2012-02-20 10:21:23 -08:00
Bob Trahan
7a3f33b5c2 OAuth - Phabricator OAuth server and Phabricator client for new Phabricator OAuth Server
Summary:
adds a Phabricator OAuth server, which has three big commands:
 - auth - allows $user to authorize a given client or application.  if $user has already authorized, it hands an authoization code back to $redirect_uri
 - token - given a valid authorization code, this command returns an authorization token
 - whoami - Conduit.whoami, all nice and purdy relative to the oauth server.
Also has a "test" handler, which I used to create some test data.  T850 will
delete this as it adds the ability to create this data in the Phabricator
product.

This diff also adds the corresponding client in Phabricator for the Phabricator
OAuth Server.  (Note that clients are known as "providers" in the Phabricator
codebase but client makes more sense relative to the server nomenclature)

Also, related to make this work well
 - clean up the diagnostics page by variabilizing the provider-specific
information and extending the provider classes as appropriate.
 - augment Conduit.whoami for more full-featured OAuth support, at least where
the Phabricator client is concerned

What's missing here...   See T844, T848, T849, T850, and T852.

Test Plan:
- created a dummy client via the test handler.   setup development.conf to have
have proper variables for this dummy client.  went through authorization and
de-authorization flows
- viewed the diagnostics page for all known oauth providers and saw
provider-specific debugging information

Reviewers: epriestley

CC: aran, epriestley

Maniphest Tasks: T44, T797

Differential Revision: https://secure.phabricator.com/D1595
2012-02-19 14:00:13 -08:00
Nick Harper
89128a70d5 Remove references to nonexistent PhabricatorOAuthProviderPhabricator
Summary:
This gets added in D1595 (which hasn't landed yet), but was referred to in
D1632 (already committed). This unbreaks master for me.

Test Plan: I no longer get an error trying to load
PhabricatorOAuthProviderPhabricator

Reviewers: btrahan, epriestley

Reviewed By: epriestley

CC: jungejason, aran, epriestley

Differential Revision: https://secure.phabricator.com/D1633
2012-02-17 12:28:17 -08:00
Bob Trahan
5ba9edff51 OAuth -- generalize / refactor providers and diagnostics page
Summary: split out from D1595

Test Plan: oauth/facebook/diagnose still looks good!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1632
2012-02-17 11:13:51 -08:00
epriestley
ecd4b03a4e Unbreak OAuth Registration
Summary:
@vrana patched an important external-CSRF-leaking hole recently (D1558), but
since we are sloppy in building this form it got caught in the crossfire.

We set action to something like "http://this.server.com/oauth/derp/", but that
triggers CSRF protection by removing CSRF tokens from the form. This makes OAuth
login not work.

Instead, use the local path only so we generate a CSRF token.

Test Plan: Registered locally via oauth.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran, epriestley, demo

Maniphest Tasks: T853

Differential Revision: https://secure.phabricator.com/D1597
2012-02-08 13:42:48 -08:00
vrana
be424bf381 Utilize hsprintf() in OAuth
Test Plan: /oauth/facebook/login/?error=<a>

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1577
2012-02-04 21:14:06 -08:00
vrana
fe4d717cc7 Escape result of PhabricatorOAuthProvider::getProviderName()
Test Plan: /settings/page/facebook/

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1556
2012-02-02 18:35:45 -08:00
vrana
339369dc36 Github is actually GitHub
Test Plan: none

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1557
2012-02-02 17:47:04 -08:00
epriestley
ad36865e50 Add optional "Re:" prefix to all threaded mail and allow disabling mail about
your own actions

Summary:
  - Mail.app on Lion has cumbersome threading rules, see T782. Add an option to
stick "Re: " in front of all threaded mail so it behaves. This is horrible, but
apparently the least-horrible option.
  - While I was in there, I added an option for T228.

Test Plan:
  - Sent a bunch of threaded and unthreaded mail with varous "Re:" settings,
seemed to get "Re:" in the right places.
  - Disabled email about my stuff, created a task with just me, got voided mail,
added a CC, got mail to just the CC.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, mkjones

Maniphest Tasks: T228, T782

Differential Revision: https://secure.phabricator.com/D1448
2012-01-18 15:20:50 -08:00
epriestley
82c0795e54 Unify logic for username validation
Summary: Revisit of D1254. Don't require lowercase, just standardize the logic.
The current implementation has nonuniform logic -- PeopleEditController forbids
uppercase.

Test Plan: Ran unit tests, see also D1254.

Reviewers: btrahan, jungejason, aran

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D1415
2012-01-16 11:52:59 -08:00
epriestley
f81021fa7f Improve error message for Conduit path problems
Summary:
A few people in IRC have been having issues here recently. If you misconfigure
the IRC bot, e.g., you get a 200 response back with a bunch of login HTML in it.
This is unhelpful.

Try to detect that a conduit request is going to the wrong path and raise a
concise, explicit error which is comprehensible from the CLI.

Also created a "PlainText" response and moved the IE nosniff header to the base
response object.

Test Plan: As a logged-out user, hit various nonsense with "?__conduit__=true"
in the URI. Got good error messages. Hit nonsense without it, got login screens.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T775

Differential Revision: https://secure.phabricator.com/D1407
2012-01-16 11:48:21 -08:00
epriestley
cedb0c045a Lock down accepted next URI values for redirect after login
Summary:
I locked this down a little bit recently, but make
double-extra-super-sure that we aren't sending the user anywhere suspicious or
open-redirecty. This also locks down protocol-relative URIs (//evil.com/path)
although I don't think any browsers do bad stuff with them in this context, and
header injection URIs (although I don't think any of the modern PHP runtimes are
vulnerable).

Test Plan:
  - Ran tests.
  - Hit redirect page with valid and invalid next URIs; was punted to / for
invalid ones and to the right place for valid ones.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: arice, aran, epriestley, btrahan

Differential Revision: https://secure.phabricator.com/D1369
2012-01-13 11:58:45 -08:00
epriestley
bfbe6ec594 Prevent login brute forcing with captchas
Summary: If a remote address has too many recent login failures, require they
fill out a captcha before they can attempt to login.

Test Plan: Tried to login a bunch of times, then submitted the CAPTHCA form with
various combinations of valid/invalid passwords and valid/invalid captchas.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Maniphest Tasks: T765

Differential Revision: https://secure.phabricator.com/D1379
2012-01-12 15:22:05 -08:00
epriestley
02fb5fea89 Allow configuration of a minimum password length, unify password reset
interfaces

Summary:
  - We have a hard-coded minimum length of 3 right now (and 1 in the other
interface), which is sort of silly.
  - Provide a more reasonable default, and allow it to be configured.
  - We have two password reset interfaces, one of which no longer actually
requires you to verify you own the account. This is more than a bit derp.
  - Merge the interfaces into one, using either an email token or the account's
current password to let you change the password.

Test Plan:
  - Reset password on an account.
  - Changed password on an account.
  - Created a new account, logged in, set the password.
  - Tried to set a too-short password, got an error.

Reviewers: btrahan, jungejason, nh

Reviewed By: jungejason

CC: aran, jungejason

Maniphest Tasks: T766

Differential Revision: https://secure.phabricator.com/D1374
2012-01-12 07:39:13 -08:00
epriestley
d75007cf42 Validate logins, and simplify email password resets
Summary:
  - There are some recent reports of login issues, see T755 and T754. I'm not
really sure what's going on, but this is an attempt at getting some more
information.
  - When we login a user by setting 'phusr' and 'phsid', send them to
/login/validate/ to validate that the cookies actually got set.
  - Do email password resets in two steps: first, log the user in. Redirect them
through validate, then give them the option to reset their password.
  - Don't CSRF logged-out users. It technically sort of works most of the time
right now, but is silly. If we need logged-out CSRF we should generate it in
some more reliable way.

Test Plan:
  - Logged in with username/password.
  - Logged in with OAuth.
  - Logged in with email password reset.
  - Sent bad values to /login/validate/, got appropriate errors.
  - Reset password.
  - Verified next_uri still works.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan, j3kuntz

Maniphest Tasks: T754, T755

Differential Revision: https://secure.phabricator.com/D1353
2012-01-11 08:25:55 -08:00
epriestley
9c5a6601d6 Remove some Mercurial and Google caveats
Summary: These seem to work relatively reasonably and don't have any known
deal-breaking failures.

Test Plan: shrug~

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Differential Revision: https://secure.phabricator.com/D1324
2012-01-05 14:14:06 -08:00
epriestley
fbfb263cd9 Provide a configuration flag to disable silliness in the UI
Summary: See comments. A few installs have remarked that their organizations
would prefer buttons labled "Submit" to buttons labeled "Clowncopterize".

Test Plan:
  - In "serious" mode, verified Differential and Maniphest have serious strings,
tasks can not be closed out of spite, and reset/welcome emails are extremely
serious.
  - In unserious mode, verified Differential and Maniphest have normal strings,
tasks can be closed out of spite, and reset/welcome emails are silly.
  - This does not disable the "fax these changes" message in Arcanist (no
reasonable way for it to read the config value) or the rainbow syntax
highlighter (already removable though configuration).

Reviewers: moskov, jungejason, nh, tuomaspelkonen, aran

Reviewed By: moskov

CC: aran, moskov

Differential Revision: 1081
2011-11-04 15:24:54 -07:00
epriestley
1620bce842 Add Google as an OAuth2 provider (BETA)
Summary:
This is pretty straightforward, except:

  - We need to request read/write access to the address book to get the account
ID (which we MUST have) and real name, email and account name (which we'd like
to have). This is way more access than we should need, but there's apparently no
"get_loggedin_user_basic_information" type of call in the Google API suite (or,
at least, I couldn't find one).
  - We can't get the profile picture or profile URI since there's no Plus API
access and Google users don't have meaningful public pages otherwise.
  - Google doesn't save the fact that you've authorized the app, so every time
you want to login you need to reaffirm that you want to give us silly amounts of
access. Phabricator sessions are pretty long-duration though so this shouldn't
be a major issue.

Test Plan:
  - Registered, logged out, and logged in with Google.
  - Registered, logged out, and logged in with Facebook / Github to make sure I
didn't break anything.
  - Linked / unlinked Google accounts.

Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran

Reviewed By: aran

CC: aran, epriestley, Makinde

Differential Revision: 916
2011-09-14 07:32:04 -07:00
epriestley
87309734cc Nuke sessions from the database when users logout
Summary:
@tomo ran into an issue where he had some non-SSL-only cookie or whatever, so
"Logout" had no apparent effect. Make sure "Logout" really works by destroying
the session.

I originally kept the sessions around to be able to debug session stuff, but we
have a fairly good session log now and no reprorted session bugs except for all
the cookie stuff. It's also slightly more secure to actually destroy sessions,
since it means "logout" breaks any cookies that attackers somehow stole (e.g.,
by reading your requests off a public wifi network).

Test Plan: Commented out the cookie clear and logged out. I was logged out and
given a useful error message about clearing my cookies.

Reviewers: jungejason, nh, tuomaspelkonen, aran

Reviewed By: aran

CC: tomo, aran, epriestley

Differential Revision: 911
2011-09-08 14:30:16 -07:00
Jason Ge
c04805cde4 Open AphrontWriteGuard for user login
Summary: Open AphrontWriteGuard for user login.

Test Plan: verified that the user can log in.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, epriestley

Differential Revision: 840
2011-08-19 21:30:10 -07:00
epriestley
39b4d20ce5 Create AphrontWriteGuard, a backup mechanism for CSRF validation
Summary:
Provide a catchall mechanism to find unprotected writes.

  - Depends on D758.
  - Similar to WriteOnHTTPGet stuff from Facebook's stack.
  - Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
  - Never allow writes without CSRF checks.
  - This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
  - **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**

Test Plan:
  - Ran some scripts that perform writes (scripts/search indexers), no issues.
  - Performed normal CSRF submits.
  - Added writes to an un-CSRF'd page, got an exception.
  - Executed conduit methods.
  - Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
  - Did OAuth login.
  - Did OAuth registration.

Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
2011-08-16 13:29:57 -07:00
epriestley
15ef2fced0 Fix conservative CSRF token cycling limit
Summary:
We currently cycle CSRF tokens every hour and check for the last two valid ones.
This means that a form could go stale in as little as an hour, and is certainly
stale after two.

When a stale form is submitted, you basically get a terrible heisen-state where
some of your data might persist if you're lucky but more likely it all just
vanishes. The .js file below outlines some more details.

This is a pretty terrible UX and we don't need to be as conservative about CSRF
validation as we're being. Remedy this problem by:

  - Accepting the last 6 CSRF tokens instead of the last 1 (i.e., pages are
valid for at least 6 hours, and for as long as 7).
  - Using JS to refresh the CSRF token every 55 minutes (i.e., pages connected
to the internet are valid indefinitely).
  - Showing the user an explicit message about what went wrong when CSRF
validation fails so the experience is less bewildering.

They should now only be able to submit with a bad CSRF token if:

  - They load a page, disconnect from the internet for 7 hours, reconnect, and
submit the form within 55 minutes; or
  - They are actually the victim of a CSRF attack.

We could eventually fix the first one by tracking reconnects, which might be
"free" once the notification server gets built. It will probably never be an
issue in practice.

Test Plan:
  - Reduced CSRF cycle frequency to 2 seconds, submitted a form after 15
seconds, got the CSRF exception.
  - Reduced csrf-refresh cycle frequency to 3 seconds, submitted a form after 15
seconds, got a clean form post.
  - Added debugging code the the csrf refresh to make sure it was doing sensible
things (pulling different tokens, finding all the inputs).

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 660
2011-07-14 08:09:40 -07:00
Ricky Elrod
420235f9c4 Drag-drop file upload.
Summary:
- have files be uploaded by drag+drop instead of browse.
- Files are named by their uploaded filename, the user isn't given a chance to enter a file name. Is this bad?
- Store author PHID now with files
- Allow an ?author=<username> to limit the /files/ list by author.
- If one file is uploaded, the user is taken to its info page.
- If several are uploaded, they are taken to a list of their files.

Test Plan:
- Quickly tested everything and it still worked, I'd recommend some people try this out before it gets committed though. It's a rather huge revision.

Reviewers:
epriestley, Ttech

CC:

Differential Revision: 612
2011-07-08 15:20:57 -04:00
Ricky Elrod
42fba24523 Fix github and local login redirects.
Summary:
Send the user where they were intending to go after github and localized logins.
Before, because Github didn't send oauthState, we would force / upon them.

Test Plan:
Tried all three methods of login successfully.

Reviewers:
epriestley

CC:

Differential Revision: 602
2011-07-07 01:05:30 -04:00
epriestley
301fed1b43 Revise administrative workflow for user creation
Summary:
- When an administrator creates a user, provide an option to send a welcome
email. Right now this workflow kind of dead-ends.
  - Prevent administrators from changing the "System Agent" flag. If they can
change it, they can grab another user's certificate and then act as them. This
is a vaguely weaker security policy than is exhibited elsewhere in the
application. Instead, make user accounts immutably normal users or system agents
at creation time.
  - Prevent administrators from changing email addresses after account creation.
Same deal as conduit certs. The 'bin/accountadmin' script can still do this if a
user has a real problem.
  - Prevent administrators from resetting passwords. There's no need for this
anymore with welcome emails plus email login and it raises the same issues.

Test Plan:
- Created a new account, selected "send welcome email", got a welcome email,
logged in with the link inside it.
  - Created a new system agent.
  - Reset an account's password.

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, epriestley
Differential Revision: 379
2011-05-31 13:06:32 -07:00
epriestley
d96d515cc2 Add comment linking to Maniphest and Differential
Summary:
Allows you to link to comments with "D123#3" or "T123#3", then adds a pile of JS
to try to make it not terrible. :/

The thing I'm trying to avoid here is when someone says "look at this!
http://blog.com/#comment-239291" and you click and your browser jumps somewhere
random and you have no idea which comment they meant. Since I really hate this,
I've tried to avoid it by making sure the comment is always highlighted.

Test Plan:
Put T1#1 and D1#1 in remarkup and verified they linked properly.

Clicked anchors on individual comments.

Faked all comments hidden in Differential and verified they expanded on anchor
or anchor change.

Reviewed By: aran
Reviewers: aran, tomo, mroch, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 383
2011-05-31 11:11:19 -07:00