[WIP] Fix uploads being world readable

[rahuldeve]

Fix for #320. Uploaded files now require an active session to download. Works only in development for now.

[smarx]

Automated message from Dropbox CLA bot

@rahuldeve, it looks like you've already signed the Dropbox CLA. Thanks!

[sharmaeklavya2]

I think you should s/word/world/ in this PR's title.

[rahuldeve]

@sharmaeklavya2 Thanks for that :)

[timabbott]

should be /user_uploads/, right?

[timabbott]

The production test failure is because the python module isn't available in the production environment -- probably not worth worrying about for now

[timabbott]

OK, took a look at this. Generally what you have so far looks pretty reasonable. A few thoughts:
(1) There should be tests
(2) You should be able to get the Travis CI production tests to run hackishly by adding a pip install for the new module in tools/travis/production-helper.
(3) I imagine that you'll need to change code to the Zulip nginx configuration under puppet/zulip/ to support this in addition to adding the settings code to use the nginx backend.

[rahuldeve]

Added a test case and changed the nginx config file.

@timabbott wouldn't the package be installed because of python provision.py --travis

[timabbott]

@rahuldeve python provision.py --travis installs the package in a virtualenv; production doesn't use virtualenvs (it instead uses packages installed via apt). So the package won't be available to the production tests unless you install this there. This is intentional -- the point of those tests is to make sure we're installing the dependencies correctly in production, where provision.py isn't used at all.

We'll package django-sendfile for apt before merging this PR, but it makes sense for you to use pip in production-helper so you can get this working and tested without blocking on someone doing the packaging work.

[rahuldeve]

@timabbott Everything seems to be working fine now. Tested this manually for production in a vagrant box but it would have been better if we could automate it in travis. Also do you have any pointers on how to package django-sendfile for apt ?

[timabbott]

Cool, you should be able to add some extra scripts run at the end of tools/travis/production-helper to test the production sendfile configuration...

[timabbott]

On the front of packaging, either I'll take care of it or we'll have switched to using requirements.txt in production; either way, don't worry about the apt issue.

[timabbott]

@lfaraone do you want to do a quick look at django-sendfile from a security perspective? It's a super simple library.

[rahuldeve]

@timabbott we could just download django-sendfile 0.3.10.tar.gz from pypi and install it directly. I dont think using pip for production is a good idea since it needs a lot of extra dependencies especially in a minimal environment like an lxc container.

[rahuldeve]

As for the tests in production, I'm having trouble automating the login process. For manual testing, I just created a new user and changed its password using manage.py. ./manage.py changepassword prompts for a new password but I'm having trouble automating it through a script.

Also ive added code for an authorization check before an upload is served. So now a user can only get the file if he is either the sender or recipient of that file. If the file is uploaded to a stream, all the members of the stream are considered as recipients.

[rahuldeve]

Also sendfile do require a test to see if it works in production. Now that we have API based endpoints for uploading and downloading, that will be easy. Will probably have to modify the zulip-send api too.

[timabbott]

Yeah, I think marking files that predate this feature as realm-readable is pretty reasonable; that's the policy we had at the time those messages were sent, and I think trying to enforce a stricter policy could break things for users. Then we don't need to loop through messages, just through the files.

Sounds good re: adding a production test.

[rahuldeve]

@timabbott Ive pushed the necessary migration.

[rahuldeve]

@timabbott Ive rebased the whole thing on top of the revert commit. The updated migration should now get all files from either local or s3 and make attachment entries for them.

Also I'm not sure if django-sendfile will work in python3 so I put on in the py2_common file.

[timabbott]

OK, that's probably reasonable for now (but won't last, since we're close to moving Zulip to Python 3). I posted a comment on johnsensible/django-sendfile#25 and also opened johnsensible/django-sendfile#55. If you're excited to move this forward, you could open a pull request for adding python 3 support and running their tests automatically; I don't think it'd be a hard project.

On the front of the migration, I'd like to do something a bit more precise than that using the data we have for attachments in S3; I've been trying to find time to write the migration I want. But my idea was to based it off of the logic we have for iterating through all attachments in the storage mechanism in #673 (with some refactoring).

[rahuldeve]

@timabbott Sorry for the late reply, been really busy the past few weeks. Maybe you could tell me what kind of migration you have in mind? I could try to modify the current migration that I wrote and help you out :)

[timabbott]

I'd like to actually iterate through all uploaded files to record data like the owner (that's available in the S3 metadata currently) as part of the migration process.

[rahuldeve]

@timabbott The only way we can get the owner details for all the local uploads (including the ones uploaded before the Attachment model migration) is to iterate through all the messages and find out the sender of the first message referring the attachment.

Do you plan on using #673 in the zulip upgrade process ?

[timabbott]

@rahuldeve that may be probably true for the local file uploads implementation, but not for the S3 implementation -- we encode the original uploader of the file in the S3 metadata. See the key.metadata['user_profile_id'] and key.metadata['realm_id']` code in #673.

I don't plan to use #673 in the upgrade process; it's intended for moving data from one server to another.

[timabbott]

Closing in favor of the rebased version #4387. Thanks for all your work on this @rahuldeve!