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

42 commits

Author SHA1 Message Date
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
5f8711ebf8 Minor, fix CSRF error caused by D1329. 2012-01-09 13:47:33 -08:00
epriestley
58f2cb2509 Provide a script for batch creating user accounts
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.

Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.

Reviewers: btrahan, jungejason, rguerin

Reviewed By: btrahan

CC: aran, btrahan, rguerin

Differential Revision: https://secure.phabricator.com/D1336
2012-01-06 11:50:51 -08:00
epriestley
d16454d45d Improve a race condition in session establishment code
Summary:
If you try to establish several sessions quickly (e.g., by running several
copies of "arc" at once, as in "arc x | arc y"), the current logic has a high
chance of making them all pick the same conduit session to refresh (since it's
the oldest one when each process selects the current sessions). This means they
all issue updates against "conduit-3" (or whatever) and one ends up with a bogus
session.

Instead, do an update against the table with the session key we read, so only
one process wins the race. If we don't win the race, try again until we do or
have tried every session slot.

Test Plan:
  - Wiped conduit sessions, ran arc commands to verify the fresh session case.
  - Ran a bunch of arc piped to itself, e.g. "arc list | arc list | arc list |
...". It succeeds up to the session limit, and above that gets failures as
expected.
  - Manually checked the session table to make sure things seemed reasonable
there.
  - Generally ran a bunch of arc commands.
  - Logged out and logged in on the web interface.

Reviewers: btrahan, jungejason

Reviewed By: btrahan

CC: aran, btrahan

Maniphest Tasks: T687

Differential Revision: https://secure.phabricator.com/D1329
2012-01-06 11:33:03 -08:00
epriestley
e45ffda55a Move most remaining sha1() calls to HMAC
Summary:
  - For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
  - This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
  - We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
  - The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.

Test Plan:
  - Verified CSRF tokens generate properly.
  - Manually changed CSRF to an incorrect value and got an error.
  - Verified mail generates with a new mail hash.
  - Verified Phabricator accepts both old and new mail hashes.
  - Verified Phabricator rejects bad mail hashes.
  - Checked user log, things look OK.

Reviewers: btrahan, jungejason, benmathews

Reviewed By: btrahan

CC: aran, epriestley, btrahan

Maniphest Tasks: T547

Differential Revision: 1237
2011-12-19 08:56:53 -08:00
epriestley
98c8e150b0 Prevent delivery of email to disabled objects
Summary: See T625. Facebook's REST-based MTA layer had a check for this so I
overlooked it in porting it out. We should not attempt to deliver email to
disabled users.

Test Plan:
Used MetaMTA console to send email to:

  - No users: received "no To" exception.
  - A disabled user: received "all To disabled" exception.
  - A valid user: received email.
  - A valid user and a disabled user: received email to valid user only.

(Note that you can't easily send to disabled users directly since they don't
appear in the typeahead, but you can prefill it and then disable the user by
hitting "Send".)

Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran

Reviewed By: aran

CC: skrul, aran, epriestley

Differential Revision: 1120
2011-11-16 11:07:50 -08:00
Marek Sapota
789dc6cb5e Allow anonymus access to Differential.
Summary:
Add possibility for not logged in users to browse and see Differential
revisions.

Test Plan:
Set 'differential.anonymous-access' config option to true, log out, you should
be able to browse Differential without logging back in.

Reviewers: epriestley, jungejason

Reviewed By: epriestley

CC: aran, epriestley, mareksapota

Differential Revision: 1044
2011-10-25 10:23:08 -07:00
epriestley
ddce177d81 Add a name token table so on-demand typeaheads can match last names
Summary: See T585. We currently don't match middle/last/nth names in on-demand
tokenizers. Build a table so we can match them.

Test Plan:
Ran upgrade script, verified table looks sensible. Searched for "priestley" in a
tokenizer, got a bunch of test account hits.

  mysql> select * from user_nametoken;
  +-------------------+--------+
  | token             | userID |
  +-------------------+--------+
  | evan              |      1 |
  | priestley         |      1 |
  | epriestley        |      1 |
  | epriestley2       |      2 |
  | ducks             |      4 |
  | epriestley3       |      4 |
  | asdf              |      6 |
  | epriestley99      |      6 |
  ...

Reviewers: bh, nh, jungejason, tuomaspelkonen, aran

Reviewed By: aran

CC: aran

Differential Revision: 1034
2011-10-23 14:25:26 -07:00
epriestley
661f077bf7 Replace callsites to sha1() that use it to asciify entropy with
Filesystem::readRandomCharacters()

Summary: See T547. To improve auditability of use of crypto-sensitive hash
functions, use Filesystem::readRandomCharacters() in place of
sha1(Filesystem::readRandomBytes()) when we're just generating random ASCII
strings.

Test Plan:
  - Generated a new PHID.
  - Logged out and logged back in (to test sessions).
  - Regenerated Conduit certificate.
  - Created a new task, verified mail key generated sensibly.
  - Created a new revision, verified mail key generated sensibly.
  - Ran "arc list", got blocked, installed new certificate, ran "arc list"
again.

Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Differential Revision: 1000
2011-10-21 11:55:28 -07:00
Nicholas Harper
872ac17dbc Selectively load columns for differential typeahead
Summary:
Change the differential typeahead to only load columns that it needs. To do
this, I also enabled partial objects for PhabricatorUser (and made necessary
changes to support this). I also changed the functionality of Lisk's loadColumns
to either accept columns as multiple string arguments or a single array of
strings.

Test Plan:
With tokenizer.ondemand set to false, checked that the typeahead loaded and I
can type multiple people's names. Set tokenizer.ondemand to true and tried
again. In both cases, the typeahead worked.

Reviewers: epriestley

Reviewed By: epriestley

CC: jungejason, aran, epriestley, nh

Differential Revision: 990
2011-10-07 15:47:35 -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
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
8df62d5352 Allow users to associate SSH Public Keys with their accounts
Summary:
With the sshd-vcs thing I hacked together, this will enable Phabricator to host
repositories without requiring users to have SSH accounts.

I also fixed "subporjects" and added an explicit ENGINE to it.

Test Plan: Created, edited and deleted public keys. Attempted to add the same
public key twice. Attempted to add invalid and unnamed public keys.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran, cadamo, codeblock
CC: aran, epriestley
Differential Revision: 711
2011-07-23 09:15:20 -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
epriestley
a632b220a8 Index users in search results.
Summary:
Add users to the search results. I need to follow this up with a patch to make
the search results stop being terrible. I'll do that.

Test Plan:
Searched for users, ran "reindex_all_users.php"

Reviewed By: jungejason
Reviewers: tomo, jungejason, aran
CC: aran, jungejason
Differential Revision: 508
2011-06-23 16:56:10 -07:00
epriestley
25bbe741f4 Provide a default non-NULL timezone in the PhabricatorUser class
Summary:
Without this, user creation throws an exception when trying to insert NULL into
a non-NULL field.

Test Plan:
Created a new user.

Reviewed By: fratrik
Reviewers: fratrik, toulouse, aran, jungejason, tuomaspelkonen
CC: aran, fratrik
Differential Revision: 480
2011-06-20 13:13:51 -07:00
epriestley
1cd1e1ed18 "arc install-certificate", server-side components
Summary:
Provides a new workflow for making it non-horrible to install certificates.
Basically you run "arc install-certificate" and then copy/paste a short token
off a webpage and it does the ~/.arcrc edits for you.

Test Plan:
Installed certificates, used bad tokens, hit rate limiting.

Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 460
2011-06-20 05:18:02 -07:00
Andrew Toulouse
9b522982fa Add timezone support
Summary:
Allows user-configurable timezones. Adds a preference panel, and migrates to the
new date rendering in easily-modified areas of the code. ***In progress***.

Test Plan:
Check database to make sure the field is being changed when the settings are
changed; check affected views to see how they render times.

Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, toulouse
Differential Revision: 475
2011-06-18 13:07:43 -07: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
bc71888249 Mask typed passwords as they are entered into 'accountadmin'
Summary:
Currently, we echo the password as the user types it. This turns out to be a bit
of an issue in over-the-shoulder installs. Instead, disable tty echo while the
user is typing their password so nothing is shown (like how 'sudo' works).

Also show a better error message if the user chooses a duplicate email; without
testing for this we just throw a duplicate key exception when saving, which
isn't easy to understand. The other duplicate key exception is duplicate
username, which is impossible (the script updates rather than creating in this
case).

There's currently a bug where creating a user and setting their password at the
same time doesn't work. This is because we hash the PHID into the password hash,
but it's empty if the user hasn't been persisted yet. Make sure the user is
persisted before setting their password.

Finally, fix an issue where $original would have the new username set, creating
a somewhat confusing summary at the end.

I'm also going to improve the password behavior/explanation here once I add
welcome emails ("Hi Joe, epriestley created an account for you on Phabricator,
click here to login...").

Test Plan:
- Typed a password and didn't have it echoed. I also tested this on Ubuntu
without encountering problems.
  - Chose a duplicate email, got a useful error message instead of the exception
I'd encountered earlier.
  - Created a new user with a password in one pass and logged in as that user,
this worked properly.
  - Verified summary table does not contain username for new users.

Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: moskov, jr, aran, jungejason
Differential Revision: 358
2011-05-28 11:52:59 -07:00
epriestley
deb80b7652 Provide an activity log for login and administrative actions
Summary: This isn't complete, but I figured I'd ship it for review while it's still smallish.

Provide an activity log for high-level system actions (logins, admin actions). This basically allows two things to happen:

  - The log itself is useful if there are shenanigans.
  - Password login can check it and start CAPTCHA'ing users after a few failed attempts.

I'm going to change how the admin stuff works a little bit too, since right now you can make someone an agent, grab their certificate, revert them back to a normal user, and then act on their behalf over Conduit. This is a little silly, I'm going to move "agent" to the create workflow instead. I'll also add a confirm/email step to the administrative password reset flow.

Test Plan: Took various administrative and non-administrative actions, they appeared in the logs. Filtered the logs in a bunch of different ways.

Reviewers: jungejason, tuomaspelkonen, aran

CC:

Differential Revision: 302
2011-05-20 19:08:26 -07:00
epriestley
3c30ea41f1 Enable multiple web sessions
Summary:
Conduit already has multiple-session code, just move it to the main
establishSession() method and set a web session limit larger than 1.

NOTE: This will log everyone out since we no longer look for the "web" session,
only for "web-1", "web-2", ..., etc. Presumably this doesn't matter.

Test Plan:
Applied patch, was logged out. Logged in in Safari. Verified I was issued
"web-1". Logged in in Firefox. Verified I was issued "web-2".

Kept logging in and out until I got issued "web-5", then did it again and was
issued "web-1" with a new key.

Ran conduit methods and verified they work and correctly cycled session keys.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: jungejason
CC: rm, fzamore, ola, aran, epriestley, jungejason, tuomaspelkonen
Differential Revision: 264
2011-05-12 18:45:19 -07:00
epriestley
f9f8ef0e6e Admin and disabled flags for users
Summary:
Provide an "isAdmin" flag for users, to designate administrative users.

Restore the account editing interface and allow it to set role flags and reset
passwords.

Provide an "isDisabled" flag for users and shut down all system access for them.

Test Plan:
Created "admin" and "disabled" users. Did administrative things with the admin
user. Tried to do stuff with the disabled user and was rebuffed. Tried to access
administrative interfaces with a normal non-admin user and was denied.

Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: ccheever, aran
Differential Revision: 278
2011-05-12 11:17:50 -07:00
epriestley
bed2120b86 Fix systemAgent stuff. 2011-04-12 19:00:54 -07:00
epriestley
43449e49c0 Very basic system agent support. 2011-04-12 18:19:25 -07:00
epriestley
3369147cab Paginate Diffusion history views, respect 'pagesize' parameter. 2011-03-31 18:46:53 -07:00
epriestley
d3766cad98 Minor tweaks to Preferences
Summary:
Get rid of HPHP-only syntax, add a header and width restriction.

Test Plan:
Looked at /preferences/, saved preferences.

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: tuomaspelkonen
Differential Revision: 92
2011-03-31 16:07:14 -07:00
tuomaspelkonen
28fe9f4eca User preferences ported from tools
Summary:

Internal tools, e.g., differential and diffusion  have user defined
preferences for monospaced font and the option for showing either the
name of the tool or the glyph of the tool in the title.

These preferences were ported to phabricator. These preferences can be
modified in /preferences/ and they both affect diffusion and differential
at the moment.

Test Plan:

* Created an empty database
* Loaded /preferences/ and modified the monospaced font and clicked save
* Confirmed that the same page was loaded with the message that preferences
  have been saved and that the example text used the user defined font

* in /preferences/ changed the option to show tool names as plain text and
  clicked save
* Confirmed that the same page was loaded with '[Preferences]' in the title
  instead of a glyph

* These same tests were also executed for differential and diffusion

Reviewers: epriestley
CC: jungejason

Differential Revision: 91
2011-03-31 13:44:20 -07:00
jungejason
9bc04fe03d Change hard-coded PHID types to constants.
Summary:
add a constants module
src/applications/phid/constants/PhabricatorPHIDConstants.

Test Plan:
Execute applications which were using the hard-coded string.

Differential Revision: 44
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
2011-03-03 12:00:53 -08:00
epriestley
063269a00a Store OAuth tokens and more OAuth account info.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-22 10:27:27 -08:00
epriestley
c3c16d0ac0 Github OAuth
Summary:

Test Plan:

Reviewers:

CC:
2011-02-21 00:23:24 -08:00
epriestley
c457032645 Slightly more sophisticated profiles.
Summary:

Test Plan:

Reviewers:

CC:
2011-02-19 18:28:41 -08:00
epriestley
bacf622178 Use Filesystem::readRandomBytes() instead of copy/pasting reads of /dev/urandom
everywhere.

Summary:

Test Plan: Logged out and in a few times, verified I got reasonable session
keys assigned.

Reviewers:

CC:
2011-02-11 16:12:34 -08:00
epriestley
605268f9aa Some acutal conduit authentication. 2011-02-05 22:36:21 -08:00
epriestley
c93dd9c090 Flesh out some DarkConsole stuff. 2011-02-02 22:38:42 -08:00
epriestley
9fbb0cc40a Full lint pass. 2011-02-02 13:59:52 -08:00
epriestley
e28c2e8899 Profile image stuff 2011-01-31 16:00:42 -08:00
epriestley
03fec6e911 PhabricatorEnv
'infratructure' -> 'infrastructure' (rofl)
Recaptcha
Email Login / Forgot Password
Password Reset
2011-01-31 11:55:26 -08:00
epriestley
25aae76c8a Basic Facebook OAuth implementation. 2011-01-30 21:28:45 -08:00
epriestley
29f7219a49 CSRF / Logout 2011-01-30 18:52:29 -08:00
epriestley
ccf7df6093 Authentication 2011-01-26 15:34:20 -08:00
epriestley
89dae65c35 Basic user/account tool. 2011-01-23 18:09:16 -08:00