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
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
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
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
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
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
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
Summary:
In password-based auth environments, there is now a user settings
panel to allow them to change their password.
Test Plan:
Click settings, choose password from the left:
* enter current password, new password (twice), log out, and log in with
new password
* enter current password, non-matching passwords, and get error
* enter invalid old password, and get error
* use firebug to change csrf token and verify that it does not save with
and invalid token
Changed config to disable password auth, loaded settings panel and saw
that password was no longer visible. Tried loading the panel anyway and
got redirected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 890
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
Summary: create the page by getting data from the search result.
Test Plan:
load page with url /author/, /author/valid_username, and
/uathor/invalid_username, and verified that it works as expected.
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
Commenters: tuomaspelkonen
CC: hwang, aran, tuomaspelkonen, epriestley, jungejason
Differential Revision: 723
Summary: See T266. Combine these interfaces into one and move it to settings.
Test Plan: Edited my profile and account.
Reviewers: codeblock, tcook, jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 722
Summary:
It makes more sense to just make this a settings panel rather than a standalone
app, particularly since setting panels are relatively well separated now.
Also default-disabled the SSH Keys interface since it won't currently be useful
for most installs.
Test Plan: Edited preferences.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, jungejason
Differential Revision: 716
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
Summary:
I want to do two things here:
- Add SSH Keys
- Move "Preferences" into this panel
But this controller was pretty gigantic and messy. Split it apart and use
delegation instead.
There are no functional changes. I changed some of the conduit certificate text
to simplify it since no one should need to go through that workflow anymore,
given the existence of "arc install-certificate".
Test Plan:
- Edited realname, including attempting to remove it.
- Edited profile picture.
- Edited timezone.
- Edited email, including attempting to remove it.
- Regenerated condiut certificate.
- Linked and unlinked an OAuth account.
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 688
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
Summary:
This thing services every app but it lives inside Differential right now. Pull
it out, and separate the factory interfaces per-application.
This will let us accommodate changes we need to make for Phriction to support
wiki linking.
Test Plan: Tested remarkup in differential, diffusion, maniphest, people,
slowvote.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 646
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
Summary: We currently show a user's signup time in //their// local time, not the
viewer's local time. Oops!
Test Plan: Looked at user list.
Reviewed By: tuomaspelkonen
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 585
images correctly
Summary:
This is sort of doing two things at once:
- Add an "isOwner" flag to Project Affiliation to lay the groundwork for T237.
- Rename the "QuickCreate" workflow to "Create" and funnel all creation
through it.
- Reorganize the image transformation stuff and use it to correctly
crop/resize uploaded images.
Test Plan:
Created and edited projects and affailiations. Uploaded project, user, and
profile photos. Verified existing thumbnailing in Maniphest still works
properly.
Reviewed By: cadamo
Reviewers: cadamo, aran, jungejason, tuomaspelkonen
CC: aran, epriestley, cadamo
Differential Revision: 529
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
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
Summary: Added some change on the project's list view, to show information about
active tasks, population, etc. Also modified the "profile view", and added a class "PhabricatorProfileView" to render the profile, both on projects and users.
Test Plan: play around the project directory :)
Reviewers: epriestley ericfrenkiel
CC:
Differential Revision: 477
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
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
Summary:
Well, since I couldn't regenerate my arcanist cert I figured out that this wass because "workflows" are unavailable there now. I really can not figure out why but it was.
I added in the setup script, the ability to check if is present the protocol of the host and if it has a trailing slash a the end of the line, since both are needed to generate the cert.
Users now only be able to upload valid image files with mimetype of jpg, jpeg,
png and gif.
Test Plan:
FIRST: DO NOT apply those changes! then
1- go to settings->arcanist certificate and the click on regenerate ... humm
2- On your config file, delete the trailing slash at the end and the protocol on "phabricator.base-uri", then go to setting->arcanist certificate. Here you
will see something like this "phabricator.example.comapi\/" instead of
"http:\/\/phabricator.example.com\/api\/".
SECOND: Now apply this changes:
1- Go to settings->arcanist certificate and the click on regenerate.
2- On your config file, delete the trailing slash at the end and the protocol
on "phabricator.base-uri", and setup "phabricator.setup" to true.
3- Then go to setting->arcanist certificate and you could see that this was successfully generated.
THIRD:
Go to settings->account and try to upload an invalid image file, and do the same on "youruserna"->edit profile.
Reviewed By: epriestley
Reviewers: epriestley jungejason
CC: epriestley jugesason cadamo aran
Differential Revision: 391
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
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
Summary: the user can't let the realname and/or e-mail address be empty
Test Plan: enter on 'settings/account' and change your name to '' and the same
for the e-mail 'settings/email'
Reviewers: epriestley
CC: epriestley
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
Summary:
Allow construction of handlers which use Conduit.
Test Plan:
Made a bot that connects to local and runs conduit.ping.
Reviewed By: mroch
Reviewers: mroch, codeblock, aran, jungejason, tuomaspelkonen
CC: aran, mroch
Differential Revision: 299
Summary:
Someone has "defaced" secure.phabricator.com with a helpful suggestion that I
actually do this; fair enough. :P
Test Plan:
Logged in as myself, unable to edit directory information. Logged out, logged in
as admin, was able to edit directory information.
I need to fix some more CSS stuff since some of these tabs render out hideous in
the admin background, but I can followup with that.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 296
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
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
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
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
Summary:
add the conduit URI and the username together with the arc
certificate to the setting page.
Test Plan:
run arc diff to make sure it still works after copying the
generated test into the .arcrc file.
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Differential Revision: 73
Summary:
We have phabricator.conduit-uri in the config setting, but it
is always the phabricator uir appended with '/api'. So we just remove
this setting.
Test Plan:
test arc diff to make sure it still work.
Reviewed By: epriestley
Reviewers: epriestley
CC: jungejason, epriestley
Differential Revision: 71
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
