Summary: Ref T10262. Instead of dumping an unhelpful 403 "ACCESS DENIED" page on users, explain the most likely cause of the issue and give them a link to return to the file detail page to learn more or get an up-to-date link.
Test Plan: Hit both errors, had a lovely experience with the helpful dialog text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15650
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).
The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.
Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.
Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.
Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.
Test Plan:
- Configured alternate file domain.
- Viewed site: stuff worked.
- Accessed a file on primary domain, got redirected to alternate domain.
- Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
- Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
- Reloaded task, new URI generated transparently.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10262
Differential Revision: https://secure.phabricator.com/D15642
Summary: Modernize Files a bit, use newPage
Test Plan: New file, drag and drop file, view file, edit file
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15631
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: I moved history to manage and missed this callsite.
Test Plan: Use present icon/color quick select.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15099
Summary:
Ref T10054. This shuffles some stuff around to move us closer to mocks in M1450 in terms of what information is on which pages.
Home now has feed, members, watchers, link to "edit project / project edit history".
History now has edit history, edit details, edit picture, archive/unarchive.
Test Plan:
New home page:
{F1064889}
New edit/history page:
{F1064890}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15060
Summary: These are old project image choices, remove and only go with FontAwesome related images.
Test Plan: Project -> Edit Picture -> Save
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15051
Summary:
Fixes T6856. Fixes T10164.
- Make the profile image composer code use the underlying icon name instead of the top-level icon key, so it works instead of 404'ing.
- Change the button to show a preview of the profile icon instead of the text "Use Icon and Color".
- When creating a new non-milestone project, automatically set the profile image to the icon + color image.
Test Plan:
- Created several new projects, saw appropriate default icons.
- Edited projects, saw icon previews.
- Clicked icon buttons to set icons.
- Poked around other applications which use builtins (Pholio, user profiles) to look for anything I broke, but everything seemed fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6856, T10164
Differential Revision: https://secure.phabricator.com/D15050
Summary: Ref T10054. Just simplifying this a bit before I start laying in the new profile menus.
Test Plan:
- Viewed Diviner on desktop and checked the mobile menu.
- Viewed Files on desktop and checked the mobile menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10054
Differential Revision: https://secure.phabricator.com/D15015
Summary:
Ref T10010. Ref T5819. General alignment of the stars:
- There were some hacks in Conduit around stripping `fa-...` off icons when reading and writing that I wanted to get rid of.
- We probably have room for a subtitle in the new heavy nav, and using the icon name is a good starting point (and maybe good enough on its own?)
- The project list was real bad looking with redundant tag/names, now it is very slightly less bad looking with non-redundant types?
- Some installs will want to call Milestones something else, and this gets us a big part of the way there.
- This may slightly help to reinforce "tag" vs "policy" vs "group" stuff?
---
I'm letting installs have enough rope to shoot themselves in the foot (e.g., define 100 icons). It isn't the end of the world if they reuse icons, and is clearly their fault.
I think the cases where 100 icons will break down are:
- Icon selector dialog may get very unwieldy.
- Query UI will be pretty iffy/huge with 100 icons.
We could improve these fairly easily if an install comes up with a reasonable use case for having 100 icons.
---
The UI on the icon itself in the list views is a little iffy -- mostly, it's too saturated/bold.
I'd ideally like to try either:
- rendering a "shade" version (i.e. lighter, less-saturated color); or
- rendering a "shade" tag with just the icon in it.
However, there didn't seem to be a way to do the first one right now (`fa-example sh-blue` doesn't work) and the second one had weird margins/padding, so I left it like this for now. I figure we can clean it up once we build the thick nav, since that will probably also want an identical element.
(I don't want to render a full tag with the icon + name since I think that's confusing -- it looks like a project/object tag, but is not.)
Test Plan:
{F1049905}
{F1049906}
Reviewers: chad
Reviewed By: chad
Subscribers: 20after4, Luke081515.2
Maniphest Tasks: T5819, T10010
Differential Revision: https://secure.phabricator.com/D14918
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.
(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)
Test Plan:
- As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
- Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14805
Summary:
Ref T9992. This is a step on the path to getting EditEngine working in Badges, Projects and Calendar.
This doesn't add a new `EditField` for icons yet, just standardizes the old stuff. New stuff is more general and I saved 150 lines of code.
I put the endpoint in Files because the similar "choose a profile picture" endpoint will definitely go there, and this endpoint might eventually feature, like, "draw your own icon~~" or something.
Test Plan:
- Created events, projects and badges with custom icons.
- Edited events, projects and badges, changing their icons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9992
Differential Revision: https://secure.phabricator.com/D14799
Summary:
Fixes T8984. Because of how drag-and-drop upload works, the text file with content `code` is interpreted as a forbidden variable. Disable this check for the drop upload controller.
(The risk here is a general one where the controller redirects and bundles paramters; this controller does not do that, so it's safe to make this change.)
Test Plan: Uploaded a text file containing only the string "code" (no quotes) by using drag-and-drop.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8984
Differential Revision: https://secure.phabricator.com/D13744
Summary: Not sure if we want this, but it seems to work fine.
Test Plan: {F516736}
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence, chad
Subscribers: joshuaspence, epriestley
Differential Revision: https://secure.phabricator.com/D13363
Summary: Fixes T8277. Transforming files can race; resolve the race after we lose.
Test Plan:
- Added `sleep(10)` near the bottom of the transform controller.
- Transformed a file in two browser windows at the same time; got something like this (exception corresponds to the loser of the race):
{F412526}
- Applied patch.
- Repeated process, got this:
{F412527}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8277
Differential Revision: https://secure.phabricator.com/D12965
Summary:
Fixes T6956. Before this change, we called PhabricatorUser::getOmnipotentUser in the various delete methods to query the data. Now, we use $engine->getViewer(), since its always a good thing to have less calls to PhabricatorUser::getOmnipotentUser thrown around the codebase.
I used the "codemod" tool to audit the existing calls to PhabricatorDestructorEngine (all of them) so ostensibly this gets all the spots. If I missed something though, its still going to work, so this change is very low risk.
Test Plan: ./bin/remove destroy P1; visit P1 and get a 404
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6956
Differential Revision: https://secure.phabricator.com/D12866
Summary: Use `__CLASS__` instead of hard-coding class names. Depends on D12605.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12806
Summary:
Ref T7707. Fixes T7879. Fixes T4406. When creating profile images:
- Use the new transforms;
- mark them as "profile" images so they're forced to the most-open policies.
Test Plan:
- Set restrictive default file policies.
- Changed profile picture, project pictures, etc. Verified they were visible to logged-out users.
- Registered via OAuth.
- Updated a Conpherence thread image.
- Browsed around looking for profile images, fixed sizing on everything I could find.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7879, T7707, T4406
Differential Revision: https://secure.phabricator.com/D12821
Summary:
Ref T7707. This ends up being sort of complicated: to support 100x100 images in T4406, we need to scale small images //up// so they look OK when we scale them back down with `background-size` in CSS.
The rest of it is mostly straightforward.
Test Plan:
- Did an OAuth handshake and saw a scaled-up, scaled-down profile picture that looked correct.
- Used Pholio, edited pholio, embedded pholio.
- Uploaded a bunch of small/weird/big images and regenerated all their transforms.
- Uploaded some text files into Pholio.
- Grepped for removed methods, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12818
Summary: Ref T7707. Move the 220px (file uploads) and 100px (Pholio thumbgrid) previews over to the new stuff.
Test Plan: Uploaded a bunch of images to remarkup and Pholio; they generated reasonable results in the web UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12814
Summary:
Ref T7707. Ref T4406. Ref T2479. This implements the profile-style (fixed width and height) transforms in a modern way.
- Added a "regnerate" feature to the support UI to make testing easier and surface errors.
- Laboriously check errors from everything.
- Fix the profile thumbnailing so it crops properly instead of leaving margins.
- Also defuses the "gigantic white PNG" attack.
This doesn't handle the imagemagick case (for animated GIFs) yet.
Test Plan:
- Uploaded a variety of wide/narrow/small/large files and converted them into sensible profile pictures.
- Tried to thumbnail some text files.
- Set the pixel-size and file-size limits artificially small and hit them.
- Used "regenerate" a bunch while testing the rest of this stuff.
- Verified that non-regenerate flows still produce a default/placeholder image.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4406, T2479, T7707
Differential Revision: https://secure.phabricator.com/D12811
Summary:
Ref T7707. Ref T2479. Ref T5258.
The thumbnailing code is some of the only code in the codebase which doesn't use exceptions to handle errors. I'm going to convert it to use exceptions; make sure they do something reasonable at top level.
Strategy here is:
- By default, we just fall back to a placeholder image if anything goes wrong.
- Later, I'll likely add a "debug" workflow from the new "Transforms" UI which will surface the specific exception instead (the code can't really raise any interesting exceptions right now).
Test Plan: Faked an exception and saw some reasonable default images.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5258, T2479, T7707
Differential Revision: https://secure.phabricator.com/D12809
Summary:
Ref T7707. Available transforms are currently relatively hard-coded and don't really have any support UI.
Modularize them so we can build some support UI.
This doesn't actually //use// any of the new stuff yet: I want to make a clean cutover once I fix the aspect ratio stuff so I can pick up a cachekey/URI change as a side effect.
Test Plan: {F400524}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T7707
Differential Revision: https://secure.phabricator.com/D12808
Summary: Fixes T7480, File names should be editable and the event should show up in feed.
Test Plan: Upload a file, view file details, edit file, change file name by adding a space and a word to the name, save changes, file name should retain space and not normalize the name, file details should show the edit event, install feed should correctly show an event for the action.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7480
Differential Revision: https://secure.phabricator.com/D12561
Summary: Fixes T7685. This required making the global drag and drop behavior able to "uninstall" itself so to speak, and then it re-installs it self as necessary.
Test Plan:
Did the following all successfully
- uploaded a file to homepage
- homepage -> differential -- no way to upload via drag and drop
- homepage -> differential -> homepage -- uploaded a file
- homepage -> differential -> browser back button to homepage -- uploaded a file
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7685
Differential Revision: https://secure.phabricator.com/D12534
Summary:
Ref T7149. We can simplify configuration somewhat by removing the upload limit setting, now that we support arbitrarily large files.
- Merge configuration documentation.
- Tell users to set things to at least 32MB. This is 8MB maximum one-shot file + 4x headroom. Chunk sizes are 4MB.
Test Plan:
- Faked all the setup warnings.
- Read documentation.
- Uploaded some files.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12083
Summary: Ref T7149. This works now, so enable it.
Test Plan:
- Uploaded large and small files in Firefox, Safari and Chrome.
- Uploaded large files with `arc upload`.
- Stopped/resumed large files with all clients.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12079
Summary: Ref T7149. This was just to make testing easier, but chunking substantially works now.
Test Plan: `grep`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12076
Summary:
Ref T7149. This still buffers the whole file, but is reaaaaal close to not doing that.
Allow Responses to be streamed, and rewrite the range stuff in the FileResponse so it does not rely on having the entire content available.
Test Plan:
- Artificially slowed down downloads, suspended/resumed them (works in chrome, not so much in Safari/Firefox?)
- Played sounds in Safari/Chrome.
- Viewed a bunch of pages and files in every browser.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12072
Summary:
Ref T7149. This adds chunking support to drag-and-drop uploads. It never activates right now unless you hack things up, since the chunk engine is still hard-coded as disabled.
The overall approach is the same as `arc upload` in D12061, with some slight changes to the API return values to avoid a few extra HTTP calls.
Test Plan:
- Enabled chunk engine.
- Uploaded some READMEs in a bunch of tiny 32 byte chunks.
- Worked out of the box in Safari, Chrome, Firefox.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12066
Summary:
Ref T7149. This flags allocated but incomplete files and doesn't explode when trying to download them.
Files are marked complete when the last chunk is uploaded.
I added a key on `<authorPHID, isPartial>` so we can show you a list of partially uploaded files and prompt you to resume them at some point down the road.
Test Plan: Massaged debugging settings and uploaded README.md very slowly in 32b chunks. Saw the file lose its "Partial" flag when the last chunk finished.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12063
Summary:
Ref T7149. This isn't complete and isn't active yet, but does basically work. I'll shore it up in the next few diffs.
The new workflow goes like this:
> Client, file.allocate(): I'd like to upload a file with length L, metadata M, and hash H.
Then the server returns `upload` (a boolean) and `filePHID` (a PHID). These mean:
| upload | filePHID | means |
|---|---|---|
| false | false | Server can't accept file.
| false | true | File data already known, file created from hash.
| true | false | Just upload normally.
| true | true | Query chunks to start or resume a chunked upload.
All but the last case are uninteresting and work like exising uploads with `file.uploadhash` (which we can eventually deprecate).
In the last case:
> Client, file.querychunks(): Give me a list of chunks that I should upload.
This returns all the chunks for the file. Chunks have a start byte, an end byte, and a "complete" flag to indicate that the server already has the data.
Then, the client fills in chunks by sending them:
> Client, file.uploadchunk(): Here is the data for one chunk.
This stuff doesn't work yet or has some caveats:
- I haven't tested resume much.
- Files need an "isPartial()" flag for partial uploads, and the UI needs to respect it.
- The JS client needs to become chunk-aware.
- Chunk size is set crazy low to make testing easier.
- Some debugging flags that I'll remove soon-ish.
- Downloading works, but still streams the whole file into memory.
- This storage engine is disabled by default (hardcoded as a unit test engine) because it's still sketchy.
- Need some code to remove the "isParital" flag when the last chunk is uploaded.
- Maybe do checksumming on chunks.
Test Plan:
- Hacked up `arc upload` (see next diff) to be chunk-aware and uploaded a readme in 18 32-byte chunks. Then downloaded it. Got the same file back that I uploaded.
- File UI now shows some basic chunk info for chunked files:
{F336434}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: joshuaspence, epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D12060
Summary: Fixes T7398. Previously, we would redirect to get a token and then redirect back to make use of it, but lose "download" in the process, and thus not get the correct "Content-Disposition" header.
Test Plan: Clicked "Download" on a lightboxed file.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7398
Differential Revision: https://secure.phabricator.com/D11915
Summary:
Fixes T7379. Currently, builtin files generate with a "users" view policy even if an install is public.
Because these files TTL after 7 days, there's no migration here. Installs won't see the fix actually happen for up to 7 days after updating, though.
Test Plan:
- Deleted a builtin.
- Loaded projects page to regenerate it.
- Saw new builtin had most open policy and was marked as a builtin.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7379
Differential Revision: https://secure.phabricator.com/D11917
Summary:
Taking a pass at revamping the edit pages in Projects. Specifically:
- Remove EditMainController
- Move actions from EditMain to Profile
- Move properties from EditMain to Profile
- Move timeline from EditMain to Profile
- Move Open Tasks from Profile to sidenavicon
- Add custom icons and colors to timeline
Feel free to bang on this a bit and give feedback, feels generally correct to me.
Test Plan: Edit everything I could on various projects. Check links, timelines, actions.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11421
Summary: Ref T6822. This method needs to be `public` because it is called from `PhabricatorApplicationSearchController::buildApplicationMenu()`.
Test Plan: I wouldn't expect //increasing// method visibility to break anything.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11416
Summary: Ref T5752, moves mobile action menus to the object box instead of crumbs.
Test Plan: View action menus at tablet, desktop, and mobile break points. Verify clicking buttons works as expected opening menu.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5752
Differential Revision: https://secure.phabricator.com/D11340
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary: **WIP** This adds default capability to the Files application
Test Plan: Set default to public, go to Files page, see public preset. Upload File. Doesn't work.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6564
Differential Revision: https://secure.phabricator.com/D10888
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
