Summary: you can now add, edit, and delete status events. also added a "description" to status events and surface it in the big calendar view on mouse hover. some refactoring changes as well to make validation logic centralized within the storage class.
Test Plan: added, edited, deleted. yay.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T407
Differential Revision: https://secure.phabricator.com/D3810
Summary:
Django released a security update recently dealing with malicious "Host" headers:
https://www.djangoproject.com/weblog/2012/oct/17/security/
We're vulnerable to the same attack. Plug the hole.
The risk here is that an attacker does something like this:
# Register "evil.com".
# Point it at secure.phabricator.com in DNS.
# Send a legitimate user a link to "secure.phabricator.com:ignored@evil.com".
# They login and get cookies. Normally Phabricator refuses to set cookies on domains it does not recognize.
# The attacker now points "evil.com" at his own servers and reads the auth cookies on the next request.
Test Plan: Unit tests.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3766
Summary:
Allow skins to serve arbitrary resources without needing to be mapped, so we can have a vibrant community of amateur skinners.
For "basic" skins, just put all the "css/" on the page always.
Includes an image to prove that works.
@vrana, pretty sure this has no impact outside of Phame but it does change Celerity so it might be to blame if there's any weirdness with static resources.
Test Plan:
{F21341}
{F21340}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3719
Summary:
Two high-level things happening here:
- We no longer ever need to put meta-UI (content creation, editing, notices, etc.) on live blog views, since this is all in Phame now. I pulled this out.
- On the other hand, I pushed more routing/control logic into Skins and made the root skin a Controller instead of a View. This simplifies some of the code above skins, and the theory behind this is that it gives us greater flexibility to, e.g., put a glue layer between Phame and Wordpress templates or whatever else, and allows skins to handle routing and thus add pages like "About" or "Bio".
- I added a basic skin below the root skin which is more like the old root skin and has standard rendering hooks.
- "Ten Eleven" is a play on the popular (default?) Wordpress themes called "Twenty Ten", "Twenty Eleven" and "Twenty Twelve".
Test Plan: Viewed live blog and live posts. They aren't pretty, but they don't have extraneous resources.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3714
Summary: I think this is simpler and better than making them conditional. In properly configured installs this should have no impact (they already use a CDN URI). In not-quite-properly configured installs this will add a trivial, highly-compressible number of bytes to the source. In all cases we have less code.
Test Plan: Loaded some pages, everything worked.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3709
Summary:
Cleans up some of the mess I made in D3694. Basically:
- All blogs have an "internal" view with posts that uses mobile-friendly UIs, etc., so we don't have to do as much work with skins -- they just have to look pretty.
- Blogs now have a separate "live" view that we use to handle domains / skins.
- Simplified some views and use IDs in some URLs for consistency.
- Delete a bunch of edge/blogger/multi-blog code that's now obsolete.
Test Plan: Will attach screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3695
Summary:
Adds "can view" and "can edit" policies to blogs. Replaces "bloggers" with "can join".
This doesn't fully remove "bloggers" because I didn't want this to get too crazy/huge.
Test Plan: Created, edited, deleted blogs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3693
Summary:
introduce an abstract "PhameBlogSkin" class and instantiate two versions -- PhabricatorBlogSkin (Default) and PhacilityBlogSkin.
Most notable hack is including the directory /rsrc/images/phacility - this lets things "work" without messing around with the phacility.com CSS and instead just cutting and pasting most of the file.
Test Plan: played around with Phame a bunch. In particular, created a blog with a custom domain and the phacility skin. Verified it looked good and individual posts looked okay.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3687
Summary: ...and use 'em in the phame blog case.
Test Plan: viewed blog.phabricator.dev and it actually looked right!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3666
Summary:
D3542 caused a SEV for us.
Make it better for future.
Test Plan: SEV
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3614
Summary: When I have displayed DarkConsole and write a comment it keeps scrolling because new AJAX requests pop up.
Test Plan: Displayed it, issued couple of AJAX requests.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1316
Differential Revision: https://secure.phabricator.com/D3605
Summary:
I make this error quite often: I forget to declare a property I am writing to or I make a typo in it.
PHP implicitly creates a public property which I don't like.
I would much rather see a linter warning me against this than this runtime check but writing it is very difficult:
- We need to explore all parents of the class we are checking.
- It is even possible that children will declare that property but it's OK to treat this as error anyway.
- We can extend also builtin or external classes.
- It's somewhat doable for `$this` but even more complex for any `$obj` because we don't know the class of it.
This should catch significant part of these errors and I'm fine with that.
I don't plan escalating to exception because this error is not fatal and should not stop the application from working.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3601
Summary:
"blog style" for now is just "true" to make this UI render better for the blog
LATER it will be a string which will choose the larger template. this will also have to do some messing around with links; when viewing on a phabricator instance links need to be a bit dirtier to carry around the blog whereas when viewing offsite we can tell what blog it is based on the host domain. anyhoo, this is future diff work
Test Plan: looked at blog - less ugly. resized blog to smaller sizes - became a "single list" of goodness for quality reading quite quickly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3587
Summary: D3575, D3576, D3577, D3578, D3579, D3580 put all the /apps/ links on /applications/, so we can get rid of /apps/ without loss of functionality.
Test Plan: Clicked "More Stuff" on the homepage, got /applications/ instead of /apps/.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3581
Summary: Basic step toward modernizing Files, makes it appear on /applications/ and in typeahead.
Test Plan: Looked at /applications/.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3575
Summary:
This is mostly to unblock D3547.
- Move "Macros" to a first-class application called "Macros".
- After D3547, this application will also house "Memes" (macros with text on them).
- This will also make them easier to find; the top navigational query I field is "where are image macros?" nowadays, since it's not intuitive they're part of files.
- This makes some of the UI mobile-aware but doesn't set the `device` flag yet, since there are still some missing pieces.
- I'll separate storage out and continue modernizing the UI as we unblock and integrate D3547.
Test Plan: Created, edited and deleted macros. Viewed files.
Reviewers: btrahan, vrana, teisenbe
Reviewed By: vrana
CC: aran
Maniphest Tasks: T175
Differential Revision: https://secure.phabricator.com/D3572
Summary: This currently gives us back "domain.com:port" if there's a port, which messes up the new Phame logic. Make `getHost()` do what one would reasonably expect it to.
Test Plan: Loaded my local, which is on 8080.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3571
Summary:
- Introduce `shouldAllowPublic()`, indicating that logged-out users are OK in a controller if the install is configured to permit public policies.
- Make Paste views and lists allow public users.
- Make UI do sensible things with respect to disabling links, etc.
- Improve behavior of "you need to login" with respect to policy exceptions and Ajax requests.
Test Plan: Looked at "public" paste, saw all unavailable UI disabled, clicked it, got appropraite prompts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D3502
Summary: this then enables people to create blog.theircompany.com. And for us, blog.phacility.com...!
Test Plan:
- created custom URIs of various goodness and verified the error messages were sensical.
- verified if "false" in configuration then custom uri stuff disappears
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3542
Summary:
People have occasionally complained about phabricator being slow. We have
the access log to look at to see when slowness happens, but it doesn't tell
us much about why it happened. Since it's usually a sporadic issue that's
reported, it's hard to reproduce and then profile. This change will allow us
to collect sampled profiles so we can look at them when slowness occurs.
Test Plan:
checking that sampling works correctly:
- set rate to 0; do several page loads; check no new entries in table
- set rate to 1; check that there's a new row in the table for each page load
- set rate to 10; check that some requests write to table and some don't
check new ui for samples:
- load /xhprof/list/all/, see a list with a lot of samples
- load /xhprof/list/sampled/, see only sampled runs
- load /xhprof/list/manual/, see only non-sampled runs
- load /xhprof/list/my-runs/, se only my manual runs
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3458
Summary:
- Get rid of an AphrontSideNavView callsite.
- Modernize and simplify the application implementation.
- Doesn't work perfectly on tablet/phone but that's because not all the UI examples work there yet.
Test Plan: Looked at /applications/ and /uiexample/.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3431
Summary: Rehash of D3411. In cgi/fcgi setups we have no idea if the request is HTTP or HTTPS as far as I can tell, so make this config-triggered again. Also handle @vrana's "off" case.
Test Plan: Set this flag, observed redirect to https when `$_SERVER['HTTPS']` was absent.
Reviewers: nh, vrana
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D3420
Summary: We need to do something here.
Test Plan: None.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3415
Summary:
If the phabricator.base-uri is set up with https and phabricator receives
a request that isn't over ssl, we will issue a redirect response to the user
to force them to use https.
Test Plan:
With this disabled, verified that pages still load correctly over http.
Enabled it; verified I get redirected to the same path but on https when I
make an http request; verified https requests get served without a redirect.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3411
Summary:
- Add a PhabricatorApplication.
- Make most of the views work well on tablets / phones. The actual "Create" form doesn't, but everything else is good -- need to make device-friendly form layouts before I can do the form.
Test Plan: Will attach screenshots.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3293
Summary: ...just in case that stuff happens in the "wild". also cleaned up the logic here since we no longer have the conduit conditionality.
Test Plan: made sure I didn't break JS on the site. reasoned about logic of my function and asking people PHP typing questions in job interviews.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3269
Summary: Make it possible to get to stuff that used to be in tabs.
Test Plan: Clicked links and such.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3264
Summary: Move to application navigation, make it possible to get to /logs/ from the navigation.
Test Plan: Hit all interfaces, verified email.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1569, T631
Differential Revision: https://secure.phabricator.com/D3261
Summary: 'cuz we don't need it and it's lame complexity for API clients of all kinds. Rip the band-aid off now.
Test Plan: used conduit console and verified no more shield. also did some JS stuff around the suite to verify I didn't kill JS
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T891
Differential Revision: https://secure.phabricator.com/D3265
Summary:
- Add an Application.
- Move routes to the application.
- Move nav out of tabs (which no longer exist).
- Fix a couple of random things.
Test Plan: Viewed sent/received mail logs. Performed send/receive tests. Viewed email details.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T631, T1569
Differential Revision: https://secure.phabricator.com/D3255
Summary:
There's currently no way to get here from the UI since nav tabs don't exist anymore. It's also always been hard to find this feature even when we had the tabs, since it's surprising that it's inside "MetaMTA".
- Move mailing lists to a separate application.
- Add `buildApplicationPage()`, since we don't really need `buildStandardPageResponse()` any more -- we can infer all the information from `PhabricatorApplication`. This will let us get rid of a lot of the `PhabricatorXXXController` classes which just define application information.
- Add `getApplicationURI()` to reduce code duplication, and in case we want to let you move applications around some day.
Test Plan: Looked/edited/saved mailing lists.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D3248
Summary:
Ponder is similar in spirit to the Wiki tool, but uses a Q&A
format and up/downvotes to signal user sentiment. Popular
questions are moved to the top of the feed on a 5-minute
cycle based on age (younger is better) and vote count (higher
is better).
Pre-apologies for noob diff.
Test Plan:
- `./bin/phd list` Should include `PonderHeatDaemon`; phd launch it
if necessary.
- Navigate to /ponder/ ; observe sanity when adding questions,
voting on them, and adding answers.
- Confirm that questions and answers are linkable using Q5 / Q5#A5 formatted object links.
- Confirm that searching for Ponder Questions works using built-in
search.
Feedback on code / schema / whatever organization very welcome.
Reviewers: nh, vrana, epriestley
Reviewed By: epriestley
CC: gmarcotte, aran, Korvin, starruler
Differential Revision: https://secure.phabricator.com/D3136
Summary:
In D3144, I made us look in application maps to find routing rules. However, we don't process //all// the maps when we 404 and try to do a "/" redirect. Process all of the maps.
Additionally, in D3146 I made the menu items application-driven. However, some pages (like 404) don't have a controller. Drop the requirement that the controller be nonnull.
Test Plan:
- Visited "/maniphest", got a redirect after this patch.
- Visited "/asldknfalksfn", got a 404 after this patch.
Reviewers: davidreuss, vrana, btrahan
Reviewed By: davidreuss
CC: aran
Maniphest Tasks: T1607
Differential Revision: https://secure.phabricator.com/D3158
Summary:
This makes no changes, it just moves the menu icons to the applications instead of hard-coded on the page.
I'm going to try to address some of the angst in T1593 next...
Test Plan: Loaded logged-in / logged out pages. Clicked menu items. Looked at /applications/.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1593, T1569
Differential Revision: https://secure.phabricator.com/D3146
Summary:
When we match an application route, select it as the current application and store it on the controller.
Move routes for the major applications into their PhabricatorApplication classes so this works properly.
Test Plan: Added a var_dump() and made sure we picked the right app for all these applications.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1569
Differential Revision: https://secure.phabricator.com/D3144
Summary:
Each query takes over 2 seconds in FBCODE.
I didn't find a way how to speed them up.
There's also no easy way how to parallelize them at least.
So AJAX is the last instance.
Test Plan: Loaded commit with one branch and no tag.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3112
Summary: For any count fact, allow a chart to be drawn. INCREDIBLY POWERFUL DATA ANALYSIS PLATFORM.
Test Plan: Drew a chart of object counts. Drew the Maniphest burn chart.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3099
Summary:
Some facts are aggregations of other facts. For example, we may compute how many times each macro is used in each object as a "raw fact":
Dnnn uses macro "psyduck" 6 times.
But we want to present this data in aggregate form, e.g. "order macros by popularity". We can do this at runtime and it probably won't be too awful a query, but we can also aggregate it cheaply:
Macro "psyduck" is used 3920 times across all objects.
...and then do a query like "select macros ordered by usage".
"Aggregate" facts support facts like this. The aggregate facts I've implemented are:
- Count of all objects.
- Count of objects of type X.
- Last time facts were updated.
These clearly fit the "aggregate" facts template well. I'm not 100% sure macros do. We can use this table to answer a question like "What are the most popular macros, ordered by use?" We can also use it to answer a question like "What are the most popular macros in the last 6 months?", if we build a specific fact for that. But we can't use it to answer a question like "What are the most popular macros between times X and Y?". Maybe that's important; maybe not.
This seems like a good fit for at least some types of facts.
I'll de-magic the keys a bit in the next diff.
Test Plan: Ran the engines and got some aggregated facts about other facts.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3089
Summary: I was poking around to see how this class worked and noticed this variable does nothing.
Test Plan: Careful inspection and reasoning that unused private member variable can be deleted.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3068
Summary: We pull "retries" and a doc link from PhabricatorEnv directly. Break these dependencies so the classes can move to libphutil.
Test Plan: Browsed site, triggered a schema exception and verified I still got the useful footer text.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3053
Summary:
I want to move queryfx() and family to libphutil, for @chad and others (see T1283). We need to break a few dependencies to do this.
Since AphrontWriteGuard is independently useful, I broke the dependency between it and AphrontRequest rather than between Connection and WriteGuard. I'll move its implementation to libphutil in a future diff.
Test Plan: Loaded site, submitted CSRF form successfully, monkeyed with CSRF token, submitted CSRF form, got error.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3042
Summary: created a PhabricatorInlineCommentPreviewController so controllers in Diffusion and Differential respectively just have to handle the URI mapping and data loading like good little controllers.
Test Plan:
left inline comments on commits, deleted inline commits, submitted inline comments -- all worked well
did the same on some diffs
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1176
Differential Revision: https://secure.phabricator.com/D3034
Summary:
blogs are collections of posts. a blog also has metadata like a name, description and "bloggers" that can edit the metadata of the blog and contribute posts.
changes include the post edit flow where bloggers can now select which blogs to publish to. also made various small tweaks throughout the UI to make things sensical and clean as the concept of blogs is introduced.
there's edges powering this stuff. bloggers <=> blogs and posts <=> blogs in particular.
Test Plan:
made blogs, deleted blogs, tried to make blogs with no bloggers. all went well.
verified ui to publish only showed up for public posts, published posts to blogs, un-published posts to blogs, re-published posts to blogs, deleted posts and verified they disappeared from blogs.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D3003
Summary:
See D2991 / T1526. Two major changes here:
- PHP just straight-up logs passwords on ldap_bind() failures. Suppress that with "@" and keep them out of DarkConsole by enabling discard mode.
- Use PhutilOpaqueEnvelope whenever we send a password into a call stack.
Test Plan:
- Created a new account.
- Reset password.
- Changed password.
- Logged in with valid password.
- Tried to login with bad password.
- Changed password via accountadmin.
- Hit various LDAP errors and made sure nothing appears in the logs.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2993
Summary:
Currently, it's hard to debug performance issues on POST pages. Add flags to stop redirects and always collect profiles.
Also fix an issue with "all" profiles. This feature is mostly just for profiling DarkConsole itself and is rarely used, I think it's been broken for some time. There's no way to get to it with the UI.
NOTE: Some JS workflows don't stop on redirect because they use JS/AJAX redirects.
Test Plan: Enabled options, browsed, got stopped on redirects and had profiles generated. Disabled options and verified redirects and profiles work normally.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2990
Summary:
accessibility covers not only a given post but also the various "published" views.
to keep the code relative clean, this diff also splits up the post list controller logic quite a bit. this also feels like good preparation for some other work around introducing "blogs" which are collections of published posts from bloggers with some fancy features around that.
Test Plan: clicked around various parts of the Phame application as a logged in user, a logged in user with no personal posts, and without any user logged in at all. various views all seemed reasonable.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1373
Differential Revision: https://secure.phabricator.com/D2898
Summary: Line numbers and file paths may be different in current version.
Test Plan: Disabled editor, issued exception, clicked on the link.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2886
Summary:
- Provide a filter to show just unread notifications.
- Visually show which notifications are unread.
- Style tweaks to make notifications more readable.
Test Plan: {F13494}
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2883
Summary: Add a "View All Notifications" link and page.
Test Plan: Viewed all notifications
Reviewers: jungejason, vrana
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2780
Summary:
- Add a /notification/status/ page which shows server status.
- Remove various test controllers and routes.
- Make the "no notifications" message look better.
- Move port/URI configuration to config file.
Test Plan: Started server, hit /notification/status/, saw server status.
Reviewers: allenjohnashton, ddfisher, keebuhm, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D2756
Summary: Made it possible to link and unlink LDAP accounts with Phabricator accounts.
Test Plan:
I've tested this code locally and in production where I work.
I've tried creating an account from scratch by logging in with LDAP and linking and unlinking an LDAP account with an existing account. I've tried to associate the same LDAP account with different Phabricator accounts and it failed as expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, auduny, svemir
Maniphest Tasks: T742
Differential Revision: https://secure.phabricator.com/D2722
Summary: We allow ".", "_" and "-" in usernames now, but not in the route.
Test Plan: Went to /p/.-_/, got 404'd at the route level before and now make it to the profile controller.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1348
Differential Revision: https://secure.phabricator.com/D2729
Summary:
Adds the node.js Aphlict server, the flash Aphlict client, and some
supporting javascript. Built on top of - and requires - D2703 (which is still
in progress). Will likely work with no modification on top of the final
version, though.
The node server is currently run with
sudo node support/aphlict/server/aphlict_server.js
Test Plan: tested locally
Reviewers: epriestley
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2704
Summary:
Add a dropdown to display notificaitons. Right now
there is nothing real time about it, but we do update the panel
when the user clicks. This panel is only displayed if the
install has notifications enabled and you have them enabled in
your preferences (not using them by default).
Test Plan: Turn off notifications for user1, left them on for user2. Did things from user1 and from user2 on task both were cc'd on. user2 recieved all notifications, user1 recieved nothing. Made new user, made sure everything was switched off by default.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: keebuhm, ddfisher, aran, Korvin
Differential Revision: https://secure.phabricator.com/D2703
Summary: First diff in a series of diffs to add notifications to Phabricator. This is the notification application ONLY. This commit does not include the changes to other applications that makes them add notifications. As such, no notifications will be generated beyond the initial database import.
Test Plan: This is part of the notifications architecture which has been running on http://theoryphabricator.com for the past several months.
Reviewers: epriestley, btrahan, ddfisher
Reviewed By: epriestley
CC: allenjohnashton, keebuhm, aran, Korvin, jungejason, nh
Maniphest Tasks: T974
Differential Revision: https://secure.phabricator.com/D2571
Summary:
"configuration/default" moved to "default". Rename it to "configuration" and put config stuff back there.
Put writeguard into writeguard/.
Test Plan: tools
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1103
Differential Revision: https://secure.phabricator.com/D2639
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
Summary: this section gets updated for each and every request. clicking a given entry updates the larger dark-console area to have the information from that request
Test Plan: clicked around in maniphest and observed request log populating correctly. clicked a few entries in request log and saw it updated properly. clicked a different tab in the dark-console and it worked. clicked a different request log entry and it opened the dark console to the proper request on the proper tab.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1136
Differential Revision: https://secure.phabricator.com/D2574
Summary: Better solution would be to reload the page for user with valid token and all data he inserted but I guess that we don't have enough infrastructure for this.
Test Plan: Mangle token and send form.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2570
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
Summary:
Sorting by last commit date
Branch view limit to 25 branches
All branches table page with pagination on Git
Test Plan:
* Check repository view for expected behavior on branch table view
* Check all branches page & test pagination
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Maniphest Tasks: T1200
Differential Revision: https://secure.phabricator.com/D2442
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
Summary:
- When a user uploads an oversized file, throw an exception.
- When an uncaught exception occurs during a Conduit request, return a Conduit response.
- When an uncaught exception occurs during a non-workflow Ajax request, return an Ajax response.
Test Plan:
- Uploaded overlarge files.
- Hit an exception page with ?__ajax__=1 and ?__conduit__=1
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T875, T788
Differential Revision: https://secure.phabricator.com/D2385
Summary:
I wanted to point someone on a file uploaded to Phabricator and the normal link is just too long.
I guess that this also improves security. Because pointing someone to the file directly reveals the secret key used in /data/ and it can be served without auth?
We already use `{F123}` so there will be no conflicts in future because we wouldn't want to reuse it for something else.
I promote the link on /file/ - it adds one redirect but I think it's worth it. I also considered making the link from the File ID column but there are already too many links (with some duplicity).
Test Plan:
/file/
/F123 (redirect)
/F9999999999 (404)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2380
Summary:
This is mostly intended to simplify D2323.
We currently allow users to edit and customize the links on the homepage, but as far as I know no one actually does this (no one complained when we redid the homepage earlier this year) and it creates a lot of mess in the database patches and quickstart dump. After D2331, this is the only data we load in the patch files. The patch files are also a mess with respect to this data and have various different versions of it.
Also the current UI is just kind of bad, it stretches stuff across too many screens and is generally ungood. Nuking this lets us nuke a lot of code in general.
(In the long term, I think we'll move toward an "application" model anyway, and this stuff will go away sooner or later.)
I'll add a drop-database patch some time later, just in case anyone does actually use this, so they can get their data out of MySQL.
Test Plan: Looked at home page, clicked "More Stuff", got a single list of other apps/things.
Reviewers: btrahan, vrana, edward, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T345
Differential Revision: https://secure.phabricator.com/D2332
Summary:
Also couple of small changes:
- Add method name to title.
- 404 for /conduit/method/x/.
- Remove utilities from side panel.
- Remove side panel from log.
Test Plan:
/conduit/
/conduit/method/x/
/conduit/method/user.whoami/
/conduit/log/
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2326
Summary:
- When viewing a commit, show its tags.
- For commits with many tags, show a list of all tags on the tag list interface.
- Improve some handling of symbolic references.
- When tags contain content, show it on the browse view reached by clicking the tag name.
Test Plan: Looked at commits with and without tags, clicked "More tags...", clicked tag names.
Reviewers: btrahan, vrana, davidreuss, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1130
Differential Revision: https://secure.phabricator.com/D2290
Test Plan:
Click on "passing a null index to idx()" in DarkConsole.
Click on entry in stack trace.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2275
Summary:
Provides a basic start for access policies. Objects expose various capabilities, like CAN_VIEW, CAN_EDIT, etc., and set a policy for each capability. We currently implement three policies, PUBLIC (anyone, including logged-out), USERS (any logged-in) and NOONE (nobody). There's also a way to provide automatic capability grants (e.g., the owner of an object can always see it, even if some capability is set to "NOONE"), but I'm not sure how great the implementation feels and it might change.
Most of the code here is providing a primitive for efficient policy-aware list queries. The problem with doing queries naively is that you have to do crazy amounts of filtering, e.g. to show the user page 6, you need to filter at least 600 objects (and likely more) before you can figure out which ones are 500-600 for them. You can't just do "LIMIT 500, 100" because that might have only 50 results, or no results. Instead, the query looks like "WHERE id > last_visible_id", and then we fetch additional pages as necessary to satisfy the request.
The general idea is that we move all data access to Query classes and have them do object filtering. The ID paging primitive allows efficient paging in most cases, and the executeOne() method provides a concise way to do policy checks for edit/view screens.
We'll probably end up with mostly broader policy UIs or configuration-based policies, but there are at least a few cases for per-object privacy (e.g., marking tasks as "Security", and restricting things to the members of projects) so I figured we'd start with a flexible primitive and the simplify it in the UI where we can.
Test Plan: Unit tests, played around in the UI with various policy settings.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D2210
Summary:
'cuz we need to be phamous!
V1 feature set
- posts
-- standard thing you'd expect - a title and a remarkup-powered body and...
-- "phame" title - a short string that can be used to reference the story. this gets auto-updated when you mess with the title.
-- configuration - for now, do you want Facebook, Disqus or no comments? this is a per-post thing but feeds from an instance-wide configuration
Please do toss out any must have features or changes.
Test Plan: played around with this bad boy like whoa
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, vrana
Maniphest Tasks: T1111
Differential Revision: https://secure.phabricator.com/D2202
Summary:
There have been a couple of requests for this since bookmarks are "out this year like woah" and "totally uncool dude".
Allow users to save named custom queries and make them the /maniphest/ default if they so desire.
A little messy. :/
Test Plan: Saved, edited, deleted custom queries. Made custom query default; made 'no default' default. Verified default behavior. Issued a modified search from a custom query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, 20after4
Maniphest Tasks: T923, T1034
Differential Revision: https://secure.phabricator.com/D1964
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)
A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().
For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.
Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().
Test Plan: Verified most of these by visiting the affected pages.
Reviewers: btrahan, vrana, jungejason, Koolvin
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2169
Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary:
various stripe stuff, including
- external stripe library
- payment form
- test controller to play with payment form, sample business logic
My main questions / discussion topics are...
- is the stripe PHP library too big? (ie should I write something more simple just for phabricator?)
-- if its cool, what is the best way to include the client? (ie should I make it a submodule rather than the flat copy here?)
- is the JS I wrote (too) ridiculous?
-- particularly unhappy with the error message stuff being in JS *but* it seemed the best choice given the most juicy error messages come from the stripe JS such that the overall code complexity is lowest this way.
- how should the stripe JS be included?
-- flat copy like I did here?
-- some sort of external?
-- can we just load it off stripe servers at request time? (I like that from the "if stripe is down, stripe is down" perspective)
- wasn't sure if the date control was too silly and should just be baked into the form?
-- for some reason I feel like its good to be prepared to walk away from Stripe / switch providers here, though I think this is on the wrong side of pragmatic
Test Plan: - played around with sample client form
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2096
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary:
Like the title says, similar to Facebook Tasks.
Not sure how I really feel about this, but I guess it's kind of OK? I never used
this feature in Facebook Tasks but I think some people like it.
The drag-and-drop to repri across priorities feels okayish.
Because subpriority is a double and we just split the difference when
reprioritizing, you lose ~a bit of precision every time you repri two tasks
against each other and so you can break it by swapping the priorities of two
tasks ~50 times. This case is pretty silly and pathological. We can add some
code to deal with this at some point if necessary.
I think this also fixes the whacky task layout widths once and for all.
(There are a couple of minor UI glitches like headers not vanishing and header
counts not updating that I'm not fixing because I am lazy.)
Test Plan: Dragged and dropped tasks around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, mgummelt
Maniphest Tasks: T859
Differential Revision: https://secure.phabricator.com/D1731
Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:
- Personal rules can be deleted only by their owners.
- Global rules can be deleted by any user.
- All deletes are logged.
- Logs are more detailed.
- All logged actions can be viewed in aggregate.
**Minor Cleanup**
- Merged `HomeController` and `AllController`.
- Moved most queries to Query classes.
- Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
- Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
- Reenable some transaction code (this works again now).
- Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
- Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
- Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
- Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.
Test Plan:
- Browsed, created, edited, deleted personal and gules.
- Verified generated logs.
- Did some dry runs.
- Verified transcript list and transcript details.
- Created/edited all/any rules; created/edited once/every time rules.
- Filtered admin views by users.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2040
Summary: 'cuz I miss out on chat room goodness and can't paginate around in the current version
Test Plan: setup a phabot and spammed it in phabot-test. with new test data, set $page_limit = 1 and paged about -- looks good!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T990
Differential Revision: https://secure.phabricator.com/D2032
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
Summary:
We spend a significant amount of time running includes, even with APC. However, we have rigidly structured includes and can safely run them all in workers before requests occur.
Right now, requests go like this:
- Apache spawns a worker.
- Client sends an HTTP request.
- Apache interprets it.
- Apache sees it's ".php", so it hands it off to the PHP SAPI.
- The PHP SAPI starts the PHP interpreter in the worker.
- The request is handled, etc.
Instead, we want to do this:
- Worker spawns and loads the world.
- Client sends an HTTP request.
- Webeserver interprets it.
- Sees it's a ".php", hands it off to the SAPI.
- SAPI executes it on a loaded world.
No SAPIs I know of support this, but I added support to PHP-FPM fairly easily (in the sense that it took me 6 hours and I have a hacky, barely-working mess). Over HTTP (vs HTTPS) the performance improvement is pretty dramatic.
HPHP doesn't significantly defray this cost so we're probably quite a bit faster (to the user) under nginx+PHP-FPM than HPHP after this works for real.
I have the php-fpm half of this patch in a messy state, I'm going to try to port it to be vs php 5.4.
Test Plan: Ran a patched php-fpm, browsed around, site works, appears dramatically faster.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2030
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary:
- Feature request from Airtime that I missed in the feedback notes, came up yesterday.
- Identify git submodules as "FILE_SUBMODULE", not "FILE_NORMAL".
- Link git submodules to an external resolver endpoint, which tries to find commits in tracked repositories.
- Identify git symlinks as "FILE_SYMLINK", not "FILE_NORMAL".
- Add folder, file, symlink and externals icons.
Test Plan:
- externals/javelin is now identified as a submoudule and links to Javelin, not identified as a file and links to error.
- bin/phd is now identified as a symlink.
- Interfaces have pretty icons.
Reviewers: btrahan, cpiro, ddfisher, keebuhm, allenjohnashton
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1975
Test Plan:
Search for some symbol. Click on the result. Verify that there is not // in URL.
Click on the link from generated exception.
View history in Diffusion, click on Browse.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1979
Summary: It's rather confusing now since we'll "seamlessly" redirect you to the right URI, but drop the method and parameters.
Test Plan: Hit a bad URI with POST, got 404.
Reviewers: edward, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1965
Summary:
Diffusion request/uri handling is currently a big, hastily ported mess. In particular, it has:
- Tons and tons of duplicated code.
- Bugs with handling unusual branch and file names.
- An excessively large (and yet insufficiently expressive) API on DiffusionRequest, including a nonsensical concrete base class.
- Other tools were doing hacky things like passing ":" branch names.
This diff attempts to fix these issues.
- Make the base class abstract (it was concrete ONLY for "/diffusion/").
- Move all URI generation to DiffusionRequest. Make the core static. Add unit tests.
- Delete the 300 copies of URI generation code throughout Diffusion.
- Move all URI parsing to DiffusionRequest. Make the core static. Add unit tests.
- Add an appropriate static initializer for other callers.
- Convert all code calling `newFromAphrontRequestDictionary` outside of Diffusion to the new `newFromDictionary` API.
- Refactor static initializers to be sensibly-sized.
- Refactor derived DiffusionRequest classes to remove duplicated code.
- Properly encode branch names (fixes branches with "/", see <https://github.com/facebook/phabricator/issues/100>).
- Properly encode path names (fixes issues in D1742).
- Properly escape delimiter characters ";" and "$" in path names so files like "$100" are not interpreted as "line 100".
- Fix a couple warnings.
- Fix a couple lint issues.
- Fix a bug where we would not parse filenames with spaces in them correctly in the Git browse query.
- Fix a bug where Git change queries would fail unnecessarily.
- Provide or improve some documentation.
This thing is pretty gigantic but also kind of hard to split up. If it's unreasonably difficult to review, let me know and I can take a stab at it though.
This supplants D1742.
Test Plan:
- Used home, repository, branch, browse, change, history, diff (ajax), lastmodified (ajax) views of Diffusion.
- Used Owners typeaheads and search.
- Used diffusion.getrecentcommitsbypath method.
- Pushed a change to an absurdly-named file on an absurdly-named branch, everything worked properly.
{F9185}
Reviewers: nh, vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1921
Summary:
- Add inline comments to Audits, like Differential.
- Creates new storage for the comments in the Audits database.
- Creates a new PhabricatorAuditInlineComment class, similar to DifferentialInlineComment.
- Defines an Interface which Differential and Audit comments conform to.
- Makes consumers of DifferentialInlineComments consume objects which implement that interface instead.
- Adds save
NOTE: Some features are still missing! Wanted to cut this off before it got crazy:
- Inline comments aren't shown in the main comment list.
- Inline comments aren't shown in the emails.
- Inline comments aren't previewed.
I'll followup with those but this was getting pretty big.
@vrana, does the SQL change look correct?
Test Plan:
- Created, edited, deleted, replied to, reloaded and saved inline comments in Diffusion, on the left and right side of diffs.
- Created, edited, deleted, replied to, reloaded and saved inline comments in Differentila, on the left and right side of primary and diff-versus-diff diffs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1898
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
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
Summary: This class is unsused and completely useless.
Test Plan: Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1840
Summary: Provide a reasonable JS API for the Aphlict client. Provide an example behavior to invoke it.
Test Plan:
Ran "aphlict_server.js" with:
$ sudo node aphlict_server.js
Loaded /aphlict/. Opened console. Got "hello" from the server every second.
Got reasonable errors with the server not present ("Security exception", but this is because it can't connect to port 843 to access the policy server).
Reviewers: ddfisher, keebuhm, allenjohnashton, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T944
Differential Revision: https://secure.phabricator.com/D1800
Summary:
- all search boxes are now jump navs (old functionality retained if none
of the jump nav patterns match)
- added global keyboard shortcut '/' to focus the search box in the upper
right
Test Plan:
- pressed '/' and noticed the search box gains keyboard focus
- triggered jump nav functionality from search box and saw it worked
- did a search which did not match a jump nav pattern and saw it worked
(and searched in the selected context)
NOTE: The search box on the /search/ page is also changed to have jump
nav functionality. Old functionality is not impared. Still, this may not
be desirable.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1794
Summary:
- Owners has "by user" commit views, but these are supplanted by the Audit views. Just nuke them.
- Owners has "by package" commit views; consolidate these onto the package detail pages and link into Audit for full details.
Test Plan: Browsed all the Owners interfaces, clicked "View All ... Commits" buttons.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1764
Summary:
I am not sure if it is by purpose but Phabricator now process paths like
https://secure.phabricator.com/D1681-so-freaking-cool.
The reason is that there are bunch of rules with missing '$' at the end.
This mistake is so common and easy to create that I've rather removed all '$'
and changed the way how the key is processed.
I am not absolutelly sure if the '$' was missing in some rules by purpose but if
it is the case then we should rather add explicit '.*'.
This change is backwards compatible with custom maps ending with '$'. It is not
compatible with paths not ending with '$' by purpose.
Test Plan:
Visit /, /differential/, /differential/stats/revisions/, /D1681.
Run before and after:
./aphrontpath.php D123
./aphrontpath.php D123-cool
./aphrontpath.php /
./aphrontpath.php differential
./aphrontpath.php differential/
./aphrontpath.php differential/stats/revisions/
./aphrontpath.php /file/data/x/PHID-FILE-y/z
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1745
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
Summary: Since we embed comments/audits into Diffusion now, we don't need the
old edit interface.
Test Plan: Grepped for links to old interface.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1714
Summary:
We can drive this query better from the Audit tool now; get rid of the Diffusion
version.
Preserve usernames in URIs as per T900.
Test Plan: Clicked "Commits" from profile. Browsed audit commit filters.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1713
Summary:
Allow Maniphest result sets to be exported to Excel.
Spreadsheet_Excel_Writer is awful but comparatively easy to get working. There's
also a "PHPExcel" package but it has some autoload conflicts right now and this
seems good-enough.
Test Plan: Exported a bunch of tasks to Excel.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T923
Differential Revision: https://secure.phabricator.com/D1721
Summary: Add comment previews and saved drafts to audits, like Maniphest /
Differential.
Test Plan: Typed stuff into the box. Got a preview. Reloaded page. Stuff was
still there. Submitted comment. Stuff is gone.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1699
Summary:
Allows you to write a commit rule that triggers an audit by a user (personal
rules) or a project (global rules).
Mostly this is trying to make auditing more lightweight and accessible in
environments where setting up Owners packages doesn't make sense.
For instance, Disqus wants a rule like "trigger an audit for everything that
didn't have a Differential revision". While not necessarily scalable, this is a
perfectly reasonable rule for a small company, but a lot of work to implement
with Owners (and you'll get a lot of collateral damage if you don't make every
committer a project owner).
Instead, they can create a project called 'Unreviewed Commits' and write a rule
like:
- When: Differential revision does not exist
- Action: Trigger an Audit for project: "Unreviewed Commits"
Then whoever cares can join that project and they'll see those audits in their
queue, and when they approve/raise on commits their actions will affect the
project audit.
Similarly, if I want to look at all commits that match some other rule (say,
XSS) but only want to do it like once a month, I can just set up an audit rule
and go through the queue when I feel like it.
NOTE: This abuses the 'packagePHID' field to also store user and project PHIDs.
Through the magic of handles, this (apparently) works fine for now; I'll do a
big schema patch soon but have several other edits I want to make at the same
time.
Also:
- Adds an "active" fiew for /audit/, eventually this will be like the
Differential "active" view (stuff that is relevant to you right now).
- On commits, highlight triggered audits you are responsible for.
Test Plan: Added personal and global audit triggers to Herald, reparsed some
commits with --herald, got audits. Browsed all audit interfaces to make sure
nothing exploded. Viewed a commit where I was responsible for only some audits.
Performed audits and made sure the triggers I am supposed to be responsible for
updated properly.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1690
Summary:
I added multiline highlighting with the syntax:
http://site/path/to/file$from-to
NOTE: you can reverse the from and to
Test Plan: Open a file in diffusion and attempt to highlight multiple lines
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1693
Summary:
This is intended to supplant the existing "audit edit" interface. I've changed
them to both drive down the same write pathway, but the UIs are still different.
I'll fully merge them in a future diff.
Add a comment box (like Maniphest and Differential) to Diffusion. When users
make comments, their comments appear on the commit. Any audits triggers they are
responsible for are updated to reflect actions they take, as well.
Currently, audits can only be triggered by packages, but I intend to allow them
to be triggered by users and projects (via herald rules) in an upcoming diff.
Thus some of the language like "projects, users or packages" when the code is
clearly dealing only with "packagePHID".
Test Plan: Made audit updates via commit interface and via existing edit
interface. Verified both interfaces updated correctly, and that audit
responsibility rules were applied properly.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1688
Summary:
beyond the title, this diff tweaks the test console to have a bit more
functionality. also makes a small change to CSS for AphrontFormControlMarkup,
which IMO fixes a display issue on
https://secure.phabricator.com/settings/page/profile/ where the Profile URI is
all up in the air and whatnot
I think this is missing pagination. I am getting tired of the size though and
will add later. See T905.
Test Plan:
viewed, updated and deleted client authorizations. viewed, created,
updated and deleted clients
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T849, T850, T848
Differential Revision: https://secure.phabricator.com/D1683
Summary:
Currently, audits are only accessible through the Owners tool. Start moving them
to their own first-class tool in preparation for broader audit integration.
- Lay some infrastructure groundwork (e.g. AuditQuery).
- Build a basic /audit/ view.
- Show audits on the commit page in Diffusion.
This has some code duplication with stuff we've already got, but I'll merge
everything together as we move forward on this.
Test Plan: Looked at /audit/ and a commit.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1685
Summary:
First stab at a batch editor for Maniphest. Basically, you can select a group of
tasks and then import them into the "batch" interface, where you can edit all of
them at once.
High level goal is to make it easier for users in PM/filer/support/QA roles to
deal with large numbers of tasks quickly.
This implementation has a few major limitations:
- The only available actions are "add projects" and "remove projects".
- There is no review / undo / log stuff.
- All the changes are applied in-process, which may not scale terribly well.
However, the immediate need is just around projects and this seemed like a
reasonable place to draw the line for a minimal useful version of the tool.
Test Plan: Used batch editor to add and remove projects from groups of tasks.
Reviewers: btrahan, yairlivne
Reviewed By: btrahan
CC: aran, epriestley, sandra
Maniphest Tasks: T441
Differential Revision: https://secure.phabricator.com/D1680
Summary: D1595 split encodeJSONForHTTPResponse() into two methods, but left a
straggling $use_javelin_shield parameter which is no longer used.
Test Plan: Caught errors in error log.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1663
Summary: The /public/ rule needs to come before the more general subfilter rule.
Test Plan: Hit "all", "my projects" and "public" feeds, they all work.
Reviewers: davidreuss
Reviewed By: davidreuss
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1667
Summary:
I haven't actually been using this as much as I thought, and am more interested
in the full view than the per-project view.
Let's try moving it off /home/ and then maybe adding some filtering options at
some point.
Test Plan: Looked at "all" and "my projects" in feed. Looked at home page.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1658
Summary:
Show some statistics, like number of revisions, number of
revisions per week, lines per revision, etc. for phrivolous amusement.
Test Plan:
- Went to /differential/stats/revisions/
Numbers seem right
- Clicked 'Accepted'
Again
- Changed to another user with long history
Load time was not too long though delay noticeable
- Clicked 'Requested changes to'
User was preserved, looks good
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1643
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
Summary:
This is pretty simple and unpolished, but it's getting pretty big and it seems
like a reasonable starting point.
- Log chat in various "channels".
- Conduit record and query methods.
- IRCBot integration for IRC logging
Major TODO:
- Web UI is really unpolished and has no search, paging, anchor-linking, etc.
Basically all presentation stuff, though.
- I think the bot should have a map of channels to log with channel aliases?
- The "channels" should probably be in a separate table.
- The "authors" should probably be correlated to Phabricator accounts somehow,
where possible.
Test Plan: Used phabotlocal to log #phabricator.
Reviewers: kdeggelman, btrahan, Koolvin
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T837
Differential Revision: https://secure.phabricator.com/D1625
Summary:
add support for searching by package owner for Related Commits
and commits that Need Attention.
Test Plan:
verified that
- searching by package still works when there is or there is no commits
found
- searching by package owner works when there is or there is no commits
found
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley, prithvi, dihde14, Girish
Differential Revision: https://secure.phabricator.com/D1631
Summary:
- Restore quick methods for getting to common features (upload file, create
task, etc.)
- Provide a flexible cli-like navigation element similar to stuff used at
Facebook (bunny1 / lolbunny).
Test Plan: Used jump nav and nav buttons.
Reviewers: btrahan, fratrik
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1619
Summary:
Pretty straightforward; see title. Kind of gross but I have a bunch
more iterations in mind here (like filtering). Paging this is a little tricky
since we can't easily use AphrontPagerView, as it relies on OFFSET, and I think
that's sort of sketchy to use here for UX reasons (query performance and view
consistency as feed updates).
Test Plan: Looked at feed, paged through feed.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1616
"Content-Disposition: attachment"
Summary:
We currently serve some files off the primary domain (with "Content-Disposition:
attachment" + a CSRF check) and some files off the alternate domain (without
either).
This is not sufficient, because some UAs (like the iPad) ignore
"Content-Disposition: attachment". So there's an attack that goes like this:
- Alice uploads xss.html
- Alice says to Bob "hey download this file on your iPad"
- Bob clicks "Download" on Phabricator on his iPad, gets XSS'd.
NOTE: This removes the CSRF check for downloading files. The check is nice to
have but only raises the barrier to entry slightly. Between iPad / sniffing /
flash bytecode attacks, single-domain installs are simply insecure. We could
restore the check at some point in conjunction with a derived authentication
cookie (i.e., a mini-session-token which is only useful for downloading files),
but that's a lot of complexity to drop all at once.
(Because files are now authenticated only by knowing the PHID and secret key,
this also fixes the "no profile pictures in public feed while logged out"
issue.)
Test Plan: Viewed, info'd, and downloaded files
Reviewers: btrahan, arice, alok
Reviewed By: arice
CC: aran, epriestley
Maniphest Tasks: T843
Differential Revision: https://secure.phabricator.com/D1608
Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.
See T865.
Also unified some of the code on this pathway.
Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.
Reviewers: cbg, btrahan
Reviewed By: cbg
CC: aran, epriestley
Maniphest Tasks: T139, T865
Differential Revision: https://secure.phabricator.com/D1606
Summary: Rough cut for Quora, we want this too eventually but it's super basic
right now so I'm not linking it anywhere. Once we get a couple more iterations
I'll put it in the UI.
Test Plan: Looked at stats for test data.
Reviewers: btrahan
Reviewed By: btrahan
CC: anjali, aran, epriestley
Differential Revision: https://secure.phabricator.com/D1594
Summary:
Rough cut that still needs a lot of polish, but replace the directory list with
more of a dashboard type thing:
- Show "Unbreak Now", triage-in-your-projects, and other stuff that you're
supposed to deal with, then feed.
- Move tools a click a way behind nav -- this also lets us put more stuff
there and subtools, etc., later.
- Remove tabs.
- Merge the category/item editing views.
- I also added a light blue wash to the side nav, not sure if I like that or
not.
Test Plan:
- Viewed all elements in empty and nonempty states.
- Viewed applications, edited items/categories.
Reviewers: btrahan, aran
Reviewed By: btrahan
CC: aran, epriestley, davidreuss
Maniphest Tasks: T21, T631
Differential Revision: https://secure.phabricator.com/D1574
Summary:
This was a sort of speculative feature added by a contributor some time ago and
just serves as a label; for now, simplify it into "active" and "archived" and
remove "archived" projects from the "active" list.
- Fix a bug where we'd publish a "renamed from X to X" transaction that had no
effect.
- Publish stories about status changes.
- Remove the "edit affiliation" controller, which has no links in the UI
(effectively replaced by join/leave links).
- Add query/conduit support.
Test Plan: Edited the status of several projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1573
Summary:
We save search information and then redirect to a "/search/<query_id>/" URI in
order to make search URIs short and bookmarkable, and save query data for
analysis/improvement of search results.
Currently, there's a vague object enumeration security issue with using
sequential IDs to identify searches, where non-admins can see searches other
users have performed. This isn't really too concerning but we lose nothing by
using random keys from a large ID space instead.
- Drop 'authorPHID', which was unused anyway, so searches can not be
personally identified, even by admins.
- Identify searches by random hash keys, not sequential IDs.
- Map old queries' keys to their IDs so we don't break any existing bookmarked
URIs.
Test Plan: Ran several searches, got redirected to URIs with random hashes from
a large ID space rather than sequential integers.
Reviewers: arice, btrahan
Reviewed By: arice
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1587
Summary:
This addresses a few things:
- Provide a software HTTP response spliting guard as an extra layer of
security, see http://news.php.net/php.internals/57655 and who knows what HPHP/i
does.
- Cleans up webroot/index.php a little bit, I want to get that file under
control eventually.
- Eventually I want to collect bytes in/out metrics and this allows us to do
that easily.
- We may eventually want to write to a socket or do something else like that,
ala Litespawn.
Test Plan:
- Ran unit tests.
- Browsed around, checked headers and HTTP status codes.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1564
Summary:
add a static variable to the method and use it so we don't init more
than once!
Test Plan:
add a "phlog" and noted only init'd one time. verified "show more"
links worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T666
Differential Revision: https://secure.phabricator.com/D1553
Summary:
Add a very basic edit history table to herald rules. This table is updated
whenever saving a herald rule. The contents of the save are not examined, and
the edit history contains no information about the rule itself *yet*. Edit
history can be viewed by anyone through /herald/history/<rule id>/.
Task ID: #
Blame Rev:
Test Plan:
Made a test rule, saved some stuff.
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: zizzy, aran, xela, epriestley
Differential Revision: https://secure.phabricator.com/D1387
Summary:
this has a single side nav now. added a Utilites section below the methods
which houses Logs and Token.
On logs I ended up deleting this whole concept of "view" and the existing side
nav -- I think there were plans to add a way to filter down to subset of the
conduit calls. For logs, I envision that being a separate first class tool when
/ if we think we need additional complexity.
On token I made the form FULL so it was like the rest of the views in this page.
Test Plan:
looks good! clicked on a few methods and it worked! clicked on the
logs and they were there! clicked on the pager within the logs and it worked!
checked out the token page and it looked good too.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D1499
Summary: Make it easy to join or leave (well, slightly less easy) a project.
Publish join/leave to feed. Fix a couple of membership editor bugs.
Test Plan: Joined, left a project.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T681
Differential Revision: https://secure.phabricator.com/D1485
Summary: also fixes a small bug where the page title was always "Create Task".
switch it to the header name which is much more descriptive / correct IMO.
Test Plan:
created a new task and watched the description preview update.
edited an old task and saw the description preview populate with the correct
existing data.
edited an old task and edited the description and saw the description preview
update
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1489
Summary: See T709. I also ran into a case in Drydock where this is useful for
testing/development.
Test Plan: Freed lease of a task; deleted a task.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T709
Differential Revision: https://secure.phabricator.com/D1469
Summary:
Rough cut of Drydock. This is very basic and doesn't do much of use yet (it
//does// allocate EC2 machines as host resources and expose interfaces to them),
but I think the overall structure is more or less reasonable.
== Interfaces
Vision: Applications interact with Drydock resources through DrydockInterfaces,
like **command**, **filesystem** and **httpd** interfaces. Each interface allows
applications to perform some kind of operation on the resource, like executing
commands, reading/writing files, or configuring a web server. Interfaces have a
concrete, specific API:
// Filesystem Interface
$fs = $lease->getInterface('filesystem'); // Constants, some day?
$fs->writeFile('index.html', 'hello world!');
// Command Interface
$cmd = $lease->getInterface('command');
echo $cmd->execx('uptime');
// HTTPD Interface
$httpd = $lease->getInterface('httpd');
$httpd->restart();
Interfaces are mostly just stock, although installs might add new interfaces if
they expose different ways to interact with resources (for instance, a resource
might want to expose a new 'MongoDB' interface or whatever).
Currently: We have like part of a command interface.
== Leases
Vision: Leases keep track of which resources are in use, and what they're being
used for. They allow us to know when we need to allocate more resources (too
many sandcastles on the existing hosts, e.g.) and when we can release resources
(because they are no longer being used). They also give applications something
to hold while resources are being allocated.
// EXAMPLE: How this should work some day.
$allocator = new DrydockAllocator();
$allocator->setResourceType('sandcastle');
$allocator->setAttributes(
array(
'diffID' => $diff->getID(),
));
$lease = $allocator->allocate();
$diff->setSandcastleLeaseID($lease->getID());
// ...
if ($lease->getStatus() == DrydockLeaseStatus::STATUS_ACTIVE) {
$sandcastle_link = $lease->getInterface('httpd')->getURI('/');
} else {
$sandcastle_link = 'Still building your sandcastle...';
}
echo "Sandcastle for this diff: ".$sandcastle_link;
// EXAMPLE: How this actually works now.
$allocator = new DrydockAllocator();
$allocator->setResourceType('host');
// NOTE: Allocation is currently synchronous but will be task-driven soon.
$lease = $allocator->allocate();
Leases are completely stock, installs will not define new lease types.
Currently: Leases exist and work but are very very basic.
== Resources
Vision: Resources represent some actual thing we've put somewhere, whether it's
a host, a block of storage, a webroot, or whatever else. Applications interact
through resources by acquiring leases to them, and then getting interfaces
through these leases. The lease acquisition process has a side effect of
allocating new resources if a lease can't be acquired on existing resources
(e.g., the application wants storage but all storage resources are full) and
things are configured to autoscale.
Resources may themselves acquire leases in order to allocate. For instance, a
storage resource might first acquire a lease to a host resource. A 'test
scaffold' resource might lease a storage resource and a mysql resource.
Not all resources are auto-allocate: the entry-level version of Drydock is that
you manually allocate a couple boxes and configure them through the web console.
Then, e.g., 'storage' / 'webroot' resources allocate on top of them, but the
host pool itself does not autoscale.
Resources are completely stock, they are abstract shells representing any
arbitrary thing.
Currently: Resource exist ('host' only) but are very very basic.
== Blueprints
Vision: Blueprints contain instructions for building interfaces to, (possibly)
allocating, updating, managing, and destroying a specific type of resource in a
specific location. One way to think of them is that they are scripts for
creating and deleting resources. For example, the LocalHost, RemoteHost and
EC2Host blueprints can all manage 'host' resources.
Eventually, we will support more types of resources (storage, webroot,
sandcastle, test scaffold, phacility deployment) and more providers for resource
types, some of which will be in the Phabricator mainline and some of which will
be custom.
Blueprints are very custom and specific to application types, so installs will
define new blueprints if they are making significant use of Drydock.
Currently: They exist but have few capabilities. The stock blueprints do nearly
nothing useful. There is a technically functional blueprint for host allocation
in EC2.
== Allocator
This is just the actual code to execute the lease acquisition process.
Test Plan: Ran "drydock_control.php" script, it allocated a machine in EC2,
acquired a lease on it, interfaced with it, and then released the lease. Ran it
again, got a fresh lease on the existing resource.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1454
Summary:
A personal rule only has actions targeting the owner. Likewise, only they can
edit the rule. OTOH, a global may affect any target and is editable by anyone.
There are no new action types. Instead, type of the rule modifies the available
targets and the messaging in the ui. This is beneficial because herald rule
adapters don't need to be aware of the difference between emailing the owner of
a personal rule and emailing an arbitrary user.
This diff sets up the logic and ui for creating personal/global rules. All
existing rules have been defaulted to global.
TODO: Filter all existing rules into personal/global
TODO: Create a UI for surfacing (relevant?) global rules.
Test Plan:
1. Created a personal rule to email myself. Created a dumby revision satisfying
the conditions of that rule. Verified that I recieved a herald email.
2. Removed my adminship, change the owner of a personal rule. verified that I
couldn't edit the rule.
3.Changed rule type to global. verified that I could edit the rule.
4. Verified that admins can edit both global and personal rules.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, zizzy
Differential Revision: https://secure.phabricator.com/D1449
Summary:
We currently allow you to assign code review to disabled users, but
should not.
Test Plan:
- Created revisions with no reviewers and only disabled reviewers, was
appropriately warned.
- Looked at a disabled user handle link, was clearly informed.
- Tried to create a new revision with a disabled reviewer, was rebuffed.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1429
Summary:
A couple of people mentioned that they've had users accidentally upload
sensitive files. Allow files to be deleted.
(At some point it might be nice to keep the file handle around and log who
deleted it, but this addresses the immediate problem without needing too much
work.)
Test Plan: Deleted some files.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T780
Differential Revision: https://secure.phabricator.com/D1423
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
Summary: I've also moved the response generation for 404 from
##AphrontDefaultApplicationConfiguration## to ##buildResponseString()##
Test Plan:
Visit /
Visit /mail/
Visit /x/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, vrana
Differential Revision: https://secure.phabricator.com/D1406
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
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
Summary:
This provides an easier way to get a quick handle on page costs without
installing XHProf, which can be a bit complicated.
- We currently show an "All" line, but it means "All Services".
- Rename "All" to "All Services".
- Add "Entire Page".
Test Plan: Looked at the services tab, saw "All Services" and "Entire Page".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: https://secure.phabricator.com/D1305
Summary:
- We have a few places where we do some kind of ad-hoc comma list tokenizing,
and I'm adding another one in D1290. Add a helper to the request object.
- Add some unit tests.
Test Plan:
- Ran unit tests.
- Used PHID manager, Maniphest custom view, and Repository project editor.
Reviewers: btrahan, fratrik, jungejason
Reviewed By: btrahan
CC: aran, btrahan, epriestley
Differential Revision: https://secure.phabricator.com/D1302
Summary:
- These never actually did anything.
- I don't even really remember why I built them, maybe the Open Source team
was pushing for more GitHub integration or something? I really have no idea.
- Anyway, repository tailers do everything these could do (and much more).
Test Plan:
- Ran tailers off GitHub for many months without needing post-receive hooks.
- Grepped for relevant strings, couldn't find any references.
- Used "Repository" edit interface for a Git repository.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T706
Differential Revision: https://secure.phabricator.com/D1273
Summary:
add basic auditing functionalities. For the related commits for a
package, we detect the following conditions which might be suspicious to the
owners of the package:
* no revision specified
* revision not found
* author not match
* reviewedby not match
* owners not involved
* commit author not recognized
The owners of the package can change the status of the audit entries by
accepting it or specify concern.
The owner can turn on/off the auditing for a package.
Test Plan:
* verified that non-owner cannot see the details of the audit and cannot modify
it
* verified that all the audit reasons can be detected
* tested dropdown filtering and package search
* verified really normal change not detected
* verified accept/concern a commit
* tested enable/disable a package for auditing
* verified one audit applies to all <commit, packages> to the packages the
auditor owns
* verified that re-parsing a commit won't have effect if there exists a
relationship for <commit, package> already
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, benmathews, btrahan, mpodobnik, prithvi, TomL, epriestley
Differential Revision: 1242
Summary: This was well-intentioned but has not actually proven to be useful.
Test Plan:
- No list tab shows up anymore.
- Looked up a PHID.
Reviewers: btrahan, jungejason, Girish
Reviewed By: Girish
CC: aran, jungejason, edward, emiraga, Girish, nh, tuomaspelkonen, epriestley
Maniphest Tasks: T631
Differential Revision: 1234
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary:
- Allow more than the 100 most recent projects to be viewed.
- Provide some useful filters.
- Default the view to your projects, not all projects.
- Put query logic in a query object.
- Put filter view logic in a view object. We can port more stuff to it later.
Test Plan: Looked at active/owned/all projects. Set page size to 5 and paged
through projects.
Reviewers: btrahan, jungejason, zeeg
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1227
Summary:
kill tabs for Files application. Technique is the "filter list" on the left
hand side, with separation for "Files" versus "Image Macros". UI quirks
include:
- the page title does not change for the 3 files filters while it does change
for each of the two image macro filters.
- standalone "file" pages do not have the filter view
- you can visit /file/upload/ standalone and it doesn't have the pretty filter
list on it
Please do give direction on these quirks if you like. :)
This change also neuters the ?author= functionality for files. The code is
written such that it can easily be brought back.
Test Plan: clicked around on the filters, liked what I saw. uploaded files
fancy-like and basic-like and it worked! made image macros and it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T631
Differential Revision: 1219
Summary:
For each commit, find the affected packages, and provide a way to
search by package.
Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.
Reviewers: epriestley, blair, nh, tuomaspelkonen
Reviewed By: epriestley
CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi
Maniphest Tasks: T83
Differential Revision: 1208
Summary:
merge paste create and paste list into a single controller. Add a "filter list"
to the left hand side and have new "create w/ recent", "my" and "all" views. UI
wrinkle -- "create w/ recent" does not paginate the recent pastes and instead
upsells the user to the new "all" view.
Also includes a business logic clean up or two for simplicity of code.
Test Plan:
- created a paste from the UI
- tried to create a paste with title and no body
- tried to create a paste with no title and no body
- viewed the paste list on "create" view
- viewed the paste list on "author" view
- viewed the paste list on "all" view
- viewed page 2 of the paste list for "author" and "all" views
- "forked" a given paste through completion
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, btrahan
Maniphest Tasks: T631
Differential Revision: 1198
Summary:
use the handy DifferentialChangesetParser to do most of the heavy lifting inside
the pertinent view object. update the controller to be aware of the "show
more" calls coming from the new ui and update the transactionID appropriately.
also snuck in a small change to AprontRequest to all getting all the request
data. I used it to debug building this.
Test Plan: made a task and entered a bunch of test data. had descriptions of
various lengths, as well as really long descriptions that i did not change to
much. verified the diff looked correct and various "show more" links worked as
expected
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1187
Summary:
The conduit access to Differential kind of sucks and we want to break
back-compat in order to fix it (see D1114).
To make it easier to pull this off, I want to build out the Conduit logging a
bit so administrators can identify which users are making deprecated calls.
We should probably build a little more infrastructure around this too (API
versions?), but this is at least a reasonable step forward which gives us more
insight into the use of Conduit and more tools to smooth the deprecation
process.
This initial commit is super basic but the interface currently says "stuff",
I'll build this out a little more in a bit.
Test Plan: Looked at call logs.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1144
Summary:
Move event framework from Phabricator to libphutil so it can be used in other
phutil projects, such as Arcanist.
Test plan:
Use along with path to libphutil, events should work as expected.
Reviewers: epriestley
Differential Revision: 1098
Summary:
enable admin to delete user's herald rules. This is useful for
managing non-active users' rules. For example, ex-employees' rules. The
code change includes:
- Added a 'All' tab which is only accessible to admin.
- Refactor out a HeraldRuleListView which is used by both the home
controller and the all rule controller
Test Plan:
delete an ex-employee rule as an admin; disable myself as
admin and verified that I don't have access to view other user's rules
and I'am not be able to delete them; also verified that as a non-admin,
I can still view, create and delete my own rules.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley, jungejason
Differential Revision: 1064
Summary: Shows events which a page dispatched, plus all the registered
listeners.
Test Plan:
Pretty basic for now, but works OK:
https://secure.phabricator.com/file/view/PHID-FILE-49fcd23081ce55cf9369/
(I also made it dispatch some dummy events to verify they show up.)
Reviewers: aran
Reviewed By: aran
CC: aran
Differential Revision: 973
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary:
we use to only add X-Frame-Options for AphrontWebpageResponse.
There some security concern about it. Example of a drag-drop attack:
http://sites.google.com/site/tentacoloviola/. The fix is to add it to
all AphrontResponse.
Test Plan:
View page which disalble this option still works (like the
xhpast tree page); verify that the AphrontAjaxResponse contains the
X-Frame-Options in the header.
Reviewers: epriestley, benmathews
Reviewed By: epriestley
CC: nh, aran, jungejason, epriestley
Differential Revision: 926
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: This will get fancier, but here's a basic interface for doing symbol
lookups. Still all pretty tentative.
Test Plan: Looked up various things, got some sensible results.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 900
Summary: See T489. Provide slightly more detail so we can figure out if there's
a real issue here.
Test Plan:
Hit URIs like:
/differential/comment/preview/29/
/differential/comment/preview/29/?__ajax__=1
/differential/comment/preview/29/?__csrf__=1
..and got appropriate error messages.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 884
Summary: HPHP has behaviorial differences from PHP which make this logic
problematic and we provide a good error message to users when there's a cookie
issue now, so unsplit the cookie logic and just clear the same cookie we'd
otherwise set, as per ssl / base domain.
Test Plan: Logged in and out of my local install.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 876
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: Some stack frames do not have file/line information, e.g. __autoload
triggers. Render these as "Internal".
Test Plan: Reloaded a trace with an internal __autoload() frame, got
"(Internal)" instead of ": 0" with warnings.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 843
purposes
Summary:
Browsers send port numbers (like ":443" or proxy ports) in the Host header and
we'll currently reject them with a message like:
> Blah is configured on "x.y.com" but you are accessing it on "x.y.com:443".
Instead, examine only the host part.
Test Plan: Had my local listen on port 81 and accessed Phabricator before/after
the change; it now works without throwing.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, abdul, jungejason
Differential Revision: 841
Summary:
In D758, I tightened the scope for which we issue cookies. Instead of setting
them on the whole domain we set them only on the subdomain, and we set them as
HTTPS only if the install is HTTPS.
However, this can leave the user with a stale HTTP cookie which the browser
sends and which never gets cleared. Handle this situation by:
- Clear all four <domain, https> pairs when clearing cookies ("nuke it from
orbit").
- Clear 'phsid' cookies when they're invalid.
Test Plan: Applied a hackier version of this patch to secure.phabricator.com and
was able to login with a stale HTTP cookie.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 838
Summary:
Make the unhandled exception dialogs slightly more useful:
- Make them easier to read.
- Link to files from Phabricator libraries.
- Don't show traces by default.
- Show traces in development mode.
- Rename button from "Cancel" to "Close" and only show it for Ajax.
Test Plan: Rigged DirectoryHomeController to throw, loaded home page. Changed
stack trace setting in config. Clicked some files in the trace.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, codeblock
CC: aran, epriestley
Differential Revision: 823
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
domain
Summary:
See D758, D759.
- Provide a strongly recommended setting which permits configuration of an
alternate domain.
- Lock cookies down better: set them on the exact domain, and use SSL-only if
the configuration is HTTPS.
- Prevent Phabriator from setting cookies on other domains.
This assumes D759 will land, it is not effective without that change.
Test Plan:
- Attempted to login from a different domain and was rejected.
- Logged out, logged back in normally.
- Put install in setup mode and verified it revealed a warning.
- Configured an alterate domain.
- Tried to view an image with an old URI, got a 400.
- Went to /files/ and verified links rendered to the alternate domain.
- Viewed an alternate domain file.
- Tried to view an alternate domain file without the secret key, got a 404.
Reviewers: andrewjcg, erling, aran, tuomaspelkonen, jungejason, codeblock
CC: aran
Differential Revision: 760
Summary:
We don't currently validate CSRF tokens on this workflow. This allows an
attacker to upload arbitrary files on the user's behalf. Although I believe the
tight list of servable mime-types means that's more or less the end of the
attack, this is still a vulnerability.
In the long term, the right solution is probably to pass CSRF tokens on all Ajax
requests in an HTTP header (or just a GET param) or something like that.
However, this endpoint is unique and this is the quickest and most direct way to
close the hole.
Test Plan:
- Drop-uploaded files to Files, Maniphest, Phriction and Differential.
- Modified CSRF vaidator to use __csrf__.'x' and verified uploads and form
submissions don't work.
Reviewers: andrewjcg, aran, jungejason, tuomaspelkonen, erling
Commenters: andrewjcg, pedram
CC: aran, epriestley, andrewjcg, pedram
Differential Revision: 758
Summary:
This is a very small step toward building a Status and possibly an Oncall tool.
Build a calendar view which renders months.
Much of my hesitance to bang these tools out is that dealing with
dates/calendaring is basically horrible, so I'm trying to ease into it.
This calendar is locale-aware and all that jazz.
Test Plan:
- See:
https://secure.phabricator.com/file/view/PHID-FILE-c07a9c663a7d040d2529/
- Verified that months have the right number of days, today is the right day
of the week, months begin on the day after previous months end on, etc.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
Commenters: cwbeck, jungejason
CC: blair, aran, epriestley, cwbeck, jungejason
Differential Revision: 791
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:
I took the wrong route out of the URI map in
rP0de2e03cc245723fd64f410f5fe22ee65f05f568.
The removed route was user account editing/creation.
The intended route was profile editing (now in Settings).
Test Plan:
- Clicked "Create New Account", got account create interface instead of 404.
- Went to /profile/edit/, got 404 instead of class-not-found exception.
Reviewed By: moskov
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, moskov
Differential Revision: 731
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:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.
Show text changes between diffs and allow users to revert to earlier versions.
Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.
I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.
Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
Summary: Preview Phriction documents as they are edited, similar to how
Differential/Maniphest work.
Test Plan: Mashed my keyboard while editing a Phriction document.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb
Differential Revision: 684
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:
Provides a slowvote.poll.info method.
Test Plan:
Web console - seemed to work fine.
Reviewers:
epriestley, phuzion
CC:
Differential Revision: 659
Summary: Pretty much ripped from D636, but somewhat simplified. Lists all the
documents in the system.
Test Plan: Looked at both of the views, seems to work correctly.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb, epriestley
Differential Revision: 645
Summary: Provide a (mostly useless, currently) table of document edits.
Test Plan: Looked at document history for several of my high-quality sandbox
wiki pages.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, hsb
Differential Revision: 644
Summary:
This is another chunk of D636, I just simplified it a bit and added slugs.
When you go to a page like /w/pokemon/, it allows you to create or edit the
page.
Title vs slug stuff is a little funky but I think mostly-reasonable.
Test Plan: Created and edited /w/, /w/pokemon/, etc.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, hsb
Differential Revision: 643
Summary: Depends on D628. Provides a config option so you can set up a public
feed, which you can iframe. This needs some work but sort of works.
Test Plan: Loaded the public feed as a logged-out user.
Reviewed By: codeblock
Reviewers: jungejason, tuomaspelkonen, aran, codeblock
CC: aran, codeblock
Differential Revision: 635
Summary:
- Services: Show summary panel of total service call costs and relative page weight.
- Services: Add "Analyze Query Plans" button, which issues EXPLAIN for each query and flags problems.
- XHPRof: iframe the profile.
Test Plan: Used the new query plan analysis to find missing keys causing table scans, see D627.
Reviewers: jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 628
Summary:
Basically a copy/paste of parts of D636, but with two changes:
- Fully separate the index table ("document") from the content table
("content"). I think this will be a cleaner solution in the long run.
- Build slugs into the document structure.
This doesn't do anything useful, it just normalizes slugs and lays some
groundwork.
Test Plan:
- Visited various /w/ pages and saw them normalize correctly.
- Verified the DAO works by inserting dummy rows.
Reviewed By: codeblock
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, codeblock, epriestley
Differential Revision: 638
Summary:
This is not very useful and not exposed on the web UI. It's also the only caller
for PhabricatorPHIDConstants::getTypes().
I originally wrote this to test PHID allocation when I built the PHID system but
it's no longer really useful in any way.
phid.allocate might be useful to expose over Conduit eventually but the
implementation is trivial.
Test Plan: Grepped for controller and method names, came up empty.
Reviewed By: codeblock
Reviewers: jungejason, tuomaspelkonen, aran, codeblock
CC: aran, codeblock
Differential Revision: 625
Summary: Port slowvote. This has some style/layout roughness but gets us most of
the way there. I'll followup to fix some of the markup issues.
Test Plan: Created and voted in several different kinds of poll.
Reviewed By: codeblock
Reviewers: codeblock, tomo, jungejason, aran, tuomaspelkonen
Commenters: aran, jungejason
CC: aran, codeblock, jungejason, epriestley
Differential Revision: 613
Summary: This is a very small step toward making these good, but a concrete
reduction in clowniness.
Test Plan: Rigged an exception and got a more readable trace.
Reviewed By: aran
Reviewers: aran
CC: aran
Differential Revision: 607
Summary: This defines an extremely basic version of an activity feed, like
Facebook's news feed. It doesn't do much of interest yet.
Test Plan: Published some feed stories:
https://secure.phabricator.com/file/view/PHID-FILE-5061aa72105bbdc05b21/
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
Commenters: codeblock, jungejason
CC: aran, epriestley, codeblock, tuomaspelkonen, jungejason
Differential Revision: 593
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: 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:
The owners of a revision are only really the reviewers when the revision is in
NEEDS_REVIEW.
Also build a raw indexed document viewer so you can look at the index of a
document from the web interface.
Finally, reindex revisions when comments are added, not just when the revision
itself is edited.
Test Plan:
Toggled abandon/reclaim on a revision and verified the relationships indexed
properly.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 470
Summary:
Allow duplicate tasks to be selected and merged in Maniphest.
I didn't create a separate transaction type for this because that implies a
bunch of really complicated rules which I don't want to sort out right now
(e.g., do we need to do cycle detection for merges? If so, what do we do when we
detect a cycle?) since I think it's unnecessary to get right for the initial
implementation (my Tasks merge implementation was similar to this and worked
quite well) and if/when we eventually need the metadata to be available in a
computer-readable form that need should inform the implementation.
Plenty of room for improvement here, of course.
Test Plan:
Merged duplicate tasks, tried to perform invalid merge operations (e.g., merge a
task into itself).
Tested existing attach workflows (task -> revision, revision -> task).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran
Differential Revision: 459
Summary:
Tweaks to the paste app:
- I realized that unlike all the other apps, it makes more sense for the
default view of this one to be "create paste" instead of "list pastes" since
when you access the application directly you are most often wanting to share
something. Swap list out of the default slot and make edit the default.
- Make the textarea bigger (usability).
- Allow you to copy an existing paste.
- Implement 'raw view'.
- Tweak/adjust list view (usability, formatting).
- Tweak page titles.
Test Plan:
Created, copied, and listed pastes. Viewed raw paste. Created an invalid paste.
Tried to create a copy of a nonexistant paste.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, aran, tuomaspelkonen
CC: aran, epriestley, codeblock
Differential Revision: 456
Summary:
Provide a quick workflow for adding a new project. This ended up being sort of
complicated because we don't currently put forms in dialogs. I separated the
actual <form /> tag out of the display/layout of AphrontFormView to enable this
(the dialog is itself a form).
Limitations: if you create a new project and then remove it, it won't appear in
the tokenizer until you reload the page. We need to add the ability for the
datasource to drop its cache to enable this, which is super complicated.
Test Plan:
Used "Create new project" to add a new project when creating a task.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: anjali, aran, epriestley
Differential Revision: 422
Summary:
Addon that allows you to create a live countdown page to
some event.
Here is the ticket that this code is based on
https://secure.phabricator.com/T36
Test Plan:
Tested by manually setting dates in the timer.js file and
checking if they made sense.
I'm not sure if it works across different timezones though.
Reviewers: epriestley
CC:
Differential Revision: 436
Summary:
This is Paste. It needs some work, but epriestley recommended that I just commit something that works, and expand on that later.
Specifically, it lacks the ability to view a raw paste right now, and to turn off line numbers, making it hard to copy/paste from for now. It works for showing other people code, however.
Test Plan:
Pasted stuff, and was able to view it, and see it in the list on /paste/. Put a file extension in the title, and saw that syntax highlighting worked as expected.
Reviewers:
epriestley
CC:
Differential Revision: 424
Summary: Implements a simple infrastructure for keyboard shortcuts, see T184, and a "help" shortcut.
There's a lot of room for refinement here but I think it basically works. Each shortcut can also provide a "tooltip" handler which allows it to show help when the alt/option key is held down.
Test Plan: Pressed "?" and got help. Pressed "?" in various contexts where it should not activate (modifier keys, text input focused) and didn't get help.
Reviewers: aran, tuomaspelkonen, jungejason
CC: moskov
Differential Revision: 362
Summary:
Sendmail is seriously difficult to configure; SendGrid is extremely easy. It's
also pretty expensive ($80/mo) but there are a bunch of startups that already
have plans so it's effectively free for them.
Test Plan:
Configured SendGrid and sent reply email through it.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 376
Summary:
This is still very rough but provides basic support for generating image
thumbnails. I need to separate stuff out a bit but I'm going to integrate into
Maniphest before I hit the profile stuff so this seems like a reasonable
starting point.
Test Plan:
Generated some image thumbnails in various sizes.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 333
Summary:
This needs a bunch of UI polish (critically, it's totally undiscoverable) but it
basically works correctly. I'll clean it up in some followups.
Test Plan:
Uploaded some files via drag-and-drop, made comments, etc.
Reviewed By: aran
Reviewers: tomo, aran, jungejason, tuomaspelkonen
CC: anjali, aran
Differential Revision: 332
Summary:
Use the new API from D322 to highlight text in parallel in Differential.
Test Plan:
Verified that pygemntize calls started within 20ms of one another in DarkConsole
(also: added a feature to let me do this) instead of running serially.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 323
Summary:
When a task description is updated, there's currently no way to see the change.
Build an "expanded summary" mode for transactions that shows description change
details. Also include changes in the email.
Test Plan:
Changed task descriptions, clicked "show details", read email.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, epriestley
Differential Revision: 320
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:
Get rid of the Phabricator-level DarkConsole-specific API and use the more
general Phutil-level one.
Test Plan:
Loaded DarkConsole services plugin, viewed Diffusion, got execs in the trace.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 293
Summary:
You can currently attach tasks to revisions from Differential, but not revisions
to tasks from Maniphest. Allow editing from either side.
This logic is kind of tricky but the alternative was massive code duplication.
Test Plan:
Added and removed revisions from maniphest. Added and removed tasks from
differential.
This should have no impact on the Facebook install since none of this is used
there.
Reviewed By: aran
Reviewers: tomo, tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 288
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:
Sending a response body in a 304 triggers some crazy broken behavior in Safari +
Apache that I never hit during testing. Be spec-compliant.
Test Plan:
Mashed reload a bunch on a .php page with Safari + Apache against localhost.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: aran, tuomaspelkonen
Differential Revision: 261
Summary:
Moves shared code from Differential and Maniphest comment previews into
PhabricatorShapedRequest, and then implements Maniphest previews.
This doesn't implement comment drafts, I'll follow up with that but it requires
this and is completely separable.
This also always shows the preview as "commented" rather than previewing the
actual transaction. I'll follow up with that but I think it will require a
little factoring and this is useful even without transaction details.
I need to tweak the styling a bit too.
Test Plan:
Typed text in Maniphest and Differential. Toggled Differential action. Made
comments.
Reviewed By: rm
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, rm
Differential Revision: 258
Summary:
We always return HTTP 200 right now and don't send a "Last-Modified" header, so
browsers download more data then necessary if you sit on a page mashing reload
(for example).
Test Plan:
Used Charles to verify HTTP response codes from 400, 404 and 304 responses.
Mashed reload a bunch and saw that the server sent back 304s.
Changed the resource hash seed and saw 200s, then 304s on reload.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: bmaurer, aran, tuomaspelkonen
Differential Revision: 253
Summary:
Fixes the issue caused by rPa0af5b66437719dba6136579c051982ab275e6a0. Prior to
that patch, isCommentInNewFile() returned $comment->getIsNewFile(). While this
was often the wrong value, it came from the database and was the integer 1 if
true.
After the patch, the function returns 'true' as a boolean, which is passed to JS
and then back to PHP, interpreted as an integer, and evaluates to 0.
To avoid this issue in general, provide an isBool() method on AphrontRequest
which interprets this correctly.
I will also revert the revert of rPa0af5b66437719dba6136579c051982ab275e6a0 when
I land this.
Test Plan:
Clicked "reply" on the right hand side of a diff, got a right-hand-side inline
comment.
Reviewed By: rm
Reviewers: tuomaspelkonen, jungejason, aran, rm
CC: simpkins, aran, epriestley, rm
Differential Revision: 250
Summary:
Provides support for per-user x per-object unique reply-to email addresses, plus
SMTP integration.
This does not actually make Phabricator use these in outbound email.
Test Plan:
Used test console to validate in-Phabricator routing and handling.
Piped emails into the "mail_handler.php" script to validate mail parsing.
Configured sendmail and sent mail to Phabricator.
Technically I haven't conducted all parts of this test on the same machine since
I lost the will to configure more SMTP servers after configuring phabricator.com
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 226
Summary:
Previously, Remarkup allowed you to paste in an image URI and get an inline
image. However, it did this by hotlinking the image which isn't so hot in an
open source product.
Restore this feature, but use image proxying instead. The existing image macro
code does most of the work.
There is a mild security risk depending on the network setup so I've left this
default-disabled and made a note about it. It should be safe to enable for
Facebook.
Test Plan:
Pasted in image and non-image links, got reasonable behavior. Verified proxying
appears to work. Verified that file:// shenanigans produce 400.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: cpiro
CC: aran, cpiro, tuomaspelkonen
Differential Revision: 214
Summary:
Restores the old "pokedex" feature and allows easy definition of new macros.
Only good can come of this!
Critical feature!!
Test Plan:
nyancat
Reviewed By: tuomaspelkonen
Reviewers: aran, bh, tomo, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 220
Summary:
- Provides an "all daemons" view to look at more than the first 15 daemons.
- Provides a "combined log" view with a large page size, to quickly look at
the log across all the daemons, making it easier to find issues when you have a
bunch of the same daemon and only one is having issues.
- When viewing the web console on the same host as a daemon, show whether it's
running or not.
Test Plan:
Clicked the various daemon log interfaces.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 215
Summary:
The correct name of this key is 'github.application-secret', not
'github.secret'. Make DarkConsole check that all the masked keys exist to
prevent this from happening again. This isn't super important since this
is just intended to protected against casual security lapses (taking a
screenshot with DarkCnosole's "Config" tab open, for instance) but it's easy
to check for so it seems worthwhile to get right.
Test Plan:
Loaded page without the actual config file change, got an exception.
Fixed the config, reloaded the page, good news goats (really trying to get this
to catch on since goats are adorable).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 189
