FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

implement filepicker-django with less custom code #308

Closed btbonval closed 10 years ago

btbonval commented 10 years ago

"The filepicker django library defines the FPFileField model field so you can get all the benefits of using Filepicker.io as a drop-in replacement for the standard django FileField. No need to change any of your view logic." https://github.com/Ink/django-filepicker/wiki#models

And yet here's part of our view logic that we've changed: https://github.com/FinalsClub/karmaworld/blob/6688df3805a24b7996d04649b4443413dce83552/karmaworld/apps/document_upload/views.py#L12

which is called from this AJAX url: https://github.com/FinalsClub/karmaworld/blob/675f7b061f2a96f3ce87ac92bda5c2f8554a0578/karmaworld/urls.py#L76-L77

and that url is called by a submit for the form in this template... https://github.com/FinalsClub/karmaworld/blob/497aab5d45513b0d8d39b928a28a6f69d3d9a8b6/karmaworld/templates/partial/filepicker.html#L53

... the form of which contains a pretty major change to view logic: https://github.com/FinalsClub/karmaworld/blob/497aab5d45513b0d8d39b928a28a6f69d3d9a8b6/karmaworld/templates/partial/filepicker.html#L7-L22

The above highlighted code is in stark contrast to step 5 here: https://github.com/Ink/django-filepicker#installation

illustrated in the demo's template and view code as such:

More to the point, because of our custom code in the template, all of the following code is unused (the latter two should indeed be used, while former two might not be necessary):

btbonval commented 10 years ago

Many improvements to the template made by @charlesconnell regarding a number of tickets have been encoded into the template file noted above. If we were to drop all our custom code and implement django-filepicker exactly as advertised, we'd lose a number of features he has built.

We need to find a way to transition his changes on top of the proper django-filepicker stuff, which is a non-trivial problem.

btbonval commented 10 years ago

Since it looks like our Filepicker API key is going to be exposed to the public no matter what we do here, we should ensure that Filepicker rejects API key usage outside of accepted IP addresses. Prod's API key will need to be tied to www.karmanotes.org, and Beta's key to the beta system.

Developers working on the software would need one of the following:

btbonval commented 10 years ago

oh, maybe Filepicker supports CORS? I'll pretend I know something about CORS besides what I read in the first two sentences of this then glazed over due to lack of pictures: http://www.w3.org/TR/cors/#security

btbonval commented 10 years ago

Beta's filepicker is sort of broken as far as I can tell. I couldn't hack Karmanote's Filepicker account with my limited knowledge of assets/resources to check on that, so I made my own user FP account for testing this transition.

Sad day. Signing up for services is just the worst thing ever.

btbonval commented 10 years ago

OH! I bet Beta has an App Secret, but since nothing is ever signed, nothing works on Beta! Woooo! That'd be a nice step in the right direction, but it doesn't help me.

Created a Filepicker API Application. Went to Security / App Secret and checked Use Security. Adding the secret to filepicker.py as SECRET = 0. This step will need to be documented in the README.

It'll be best to generate a signed permission at system start by pulling in the secret from filepicker.py, so it doesn't hard code anything anywhere or make lame assumptions about which system is running.

filepicker.py is a Python file. On import (at system load) it'll do whatever Python can do. Probably best to put the JSON permission in there and sign it, with the signature stored as a variable in the secret filepicker.py file.

btbonval commented 10 years ago

HMAC boyyyyy! https://developers.inkfilepicker.com/docs/security/

JSON permission to Pick a file, push a file to S3, read a file, and stat a file's attributes. The good news is that this will exclude remove and write* options, which are destructive and bad.

{
  "expiry": "PYTHONCODE(int(time.time() + 365*3600))",
  "call": ["pick","store","read","stat"]
}

The expiry is an absolute date. Above it is set for a year after the server starts, server should restart before that interval is over, so the signature won't suddenly go stale. However, it might be more appropriate to generate this signature and cache it using some function somewheres in the Django system. This way the expiration could be an hour or so for any particular page load.

We take the JSON permission string and encode it safely to Base64 for whatever reason, then HMAC-SHA256 it with the secret.

import hmac
import base64
from hashlib import sha256
from filepickersecretfile import SECRET

policy = 'JSON JSON JSON'
policy64 = base64.urlsafe_b64encode(policy)
ng = hmac.new(SECRET, digestmod=sha256)
ng.update(policy64)
signature = ng.hexdigest()
btbonval commented 10 years ago

Need to append to the Filepicker URL signature=HEX and policy=HEX. So policy needs to be encoded to safe Base64 separately from the signature digest, because the raw JSON policy isn't sent.

Since there is like zero documentation for django-filepicker, I suppose it's good that Python is easily readable and sort of documents itself. I think this is what I need to use to add these params to FPFileField: https://github.com/Ink/django-filepicker/blob/master/django_filepicker/models.py#L39-L40

That parameter is used in some kind of Mixin which is used in the Form: https://github.com/Ink/django-filepicker/blob/master/django_filepicker/forms.py#L53-L54

But of course that's using some form other than what the URL would take. It looks like all it does is take a Python dictionary and maps it to some other set of keys. Here are what the parameter keys should look like in the extended attributes thingy: https://developers.inkfilepicker.com/docs/web/#widgets-pick

So the parms would be added as data-fp-policy and data-fp-signature into FPFileField? Best guess I have given almost no documentation.

btbonval commented 10 years ago

Alright, so valid options for FPFileField are "apikey, mimetypes, services, additional_params". Anything else gets passed to FileField. A bunch of FileFields such as upload_to are currently set, but those are for local upload. I want to upload to S3. No idea how based on this python stuff.

All the stuff statically written into the filepicker partial seems to indicate use of S3 and so on. I guess I'll just draw all 12 fields from that INPUT into the FPFileField model (most of which go into additional_params). https://github.com/FinalsClub/karmaworld/blob/master/karmaworld/templates/partial/filepicker.html#L9-L20

btbonval commented 10 years ago

Okay now there's that "type" field on the input which appears to be very important to the look and feel. Looks like it is taken care of in the widgets which render the FPFileField, as well as importing the right Javascript. Fantastic. https://github.com/Ink/django-filepicker/blob/master/django_filepicker/widgets.py

btbonval commented 10 years ago

data-fp-store-path is based on data that isn't available to the FPFileField definition. As set statically, it is supposed to drop files into /{school}/{course}/{filename}, but looking at S3 fc_filepicker, that isn't happening anyway. It's all {filename} without any prefix. Not that we ever go into there anyway. So that setting is getting dumped in favor of default (which is being used anyway).

btbonval commented 10 years ago

Well it looks like the policy and signature will be generated when the FPFileField model is generated, so let's go with the original plan of valid for a year from when the policy is generated. Perform that in the notes models.

btbonval commented 10 years ago

Need to replace INPUT on the Filepicker template with the Django-Filepicker widget somehow.

There is clearly some javascript that fires up when a file is uploaded. Gotta find what that is and hook it to onchange of the FPFileField.

btbonval commented 10 years ago

Alright well I'll either have to display the whole Document input, which should render the Filepicker thing correctly, or I'll have to create a new Form object which somehow only contains the one field.

Oh goody. I remember looking at this when we had one file uploading multiple times. So this rat's nest of javascript needs to be deconvolved somewhat so that onchange can point at the right thing. https://github.com/FinalsClub/karmaworld/blob/06f8f7a5f500458afac830b7d082e68542e8af59/karmaworld/templates/partial/filepicker.html#L88-L193

btbonval commented 10 years ago

Write a form with only the one field. See the second bit of code that doesn't use Meta here: https://docs.djangoproject.com/en/1.5/topics/forms/modelforms/#a-full-example

Okay, maybe this JS stuff isn't so hard. Turn this from an anonymous function to a named one, and call that from onchange in the FPFileField. Hopefully the rest will fall into place. https://github.com/FinalsClub/karmaworld/blob/06f8f7a5f500458afac830b7d082e68542e8af59/karmaworld/templates/partial/filepicker.html#L185-L189

btbonval commented 10 years ago

It looks like we do not presently use forms directly in our templates. Who decided to use Django only to ignore everything it does? Anyway, here's how to do it: https://docs.djangoproject.com/en/1.5/ref/forms/api/#outputting-forms-as-html

btbonval commented 10 years ago

This error doesn't make a whole lot of sense in the context of not really using FileField's upload functionality in lieu of FPFileField, although even the examples of FPFileField assume one is uploading to the server and not to S3.

One or more models did not validate:
document_upload.rawdocument: "fp_file": FileFields require an "upload_to" attribute.

I could drop a random upload_to attribute to make the problem go away. Seemed to work in the past. Might be worth trying FPUrlField, which is just for storing the Filepicker URL in the database.

omg we're not even using to_python from FPFileField https://github.com/Ink/django-filepicker/blob/master/django_filepicker/forms.py#L99-L120

and instead reimplemented the same darn thing here with get_file(): https://github.com/FinalsClub/karmaworld/blob/9b8fd07613313a6f6ad2c97c21a1c03db768c970/karmaworld/apps/notes/models.py#L110-L137

So we're not using FPFileField's capabilities!

Silver lining: FPUrlField has no equivalent thingy mabob of to_python(), so changing it to get_file() makes that method useful again.

Although if get_file() adds nothing over to_python(), it might be better to say get_file = FPFieldField.to_python. That's a bit hacky. Let's just switch to FPUrlField and see how much breaks.

btbonval commented 10 years ago

FPUrlField is only for views. Alright then. FPFileField with a random and ignored upload_to. Why fight sense and sensibility? Just do stuff.

btbonval commented 10 years ago

fields in the FPFileField are scrubbed, which ruins some bits:

data-fp-button-text="<i class='fa fa-arrow-circle-o-up'></i> add notes"
btbonval commented 10 years ago

still trying to parse the code for above error, but found this:

less hacky way to make use of get_file(), it's defined as a util. https://github.com/Ink/django-filepicker/blob/master/django_filepicker/utils.py#L17-L37

btbonval commented 10 years ago

django.utils.safestring.mark_safe fixed the one attribute.

This was needed to fix the id attribute of the <input>. https://docs.djangoproject.com/en/1.5/ref/forms/widgets/#django.forms.Widget.attrs

the data-fp button is not appearing. It might be because the <input> is wrapped in a <p>? Maybe because a name attribute is being added to the <input>? Looks like everything else is about the same.

Oh, missing javascript input might make some difference! It looks like it should be handled here: https://github.com/Ink/django-filepicker/blob/master/django_filepicker/widgets.py#L23-L24

ah, but the demo shows that must be called explicitly: https://github.com/Ink/django-filepicker/blob/master/demo/templates/home.html#L4

yay. I couldn't trace the code that led form.media back to widget.Media.JS, but it made things look right.

btbonval commented 10 years ago

Filepicker error in the console.

FilepickerException: Error: Cannot pass in both mimetype and extension parameters to the pick function

Annoyingly, I did not specify mimetype, but it defaults to adding '*/*'. Thanks django-filepicker, very useful.

btbonval commented 10 years ago

https://github.com/Ink/django-filepicker/blob/0671c4315b61f897358367a757b6ef8444aa183b/django_filepicker/forms.py#L15 NO! Bad filepicker! No treat!

Guess one must manually specify mimetypes='' when one specifies extension. Will try that out.

btbonval commented 10 years ago

Oh wait. You can't make it go away. There is no possible way to make it go away, because if you tell it not to be there, it just says "oh hey, let's use this default thing instead." https://github.com/Ink/django-filepicker/blob/0671c4315b61f897358367a757b6ef8444aa183b/django_filepicker/forms.py#L33

btbonval commented 10 years ago

Alright maybe I can hack something here and override the default_mimetypes to be empty string in the class before it invokes the referenced method.

I need to file a ticket with them about this.

btbonval commented 10 years ago

win.

    # Hack because mimetypes conflict with extensions, but there is no way to
    # disable mimetypes.
    django_filepicker.forms.FPFieldMixin.default_mimetypes = ''
btbonval commented 10 years ago

Extraneous label autogenerated by widget needs to be removed.

the got_file javascript bit which Filepicker binds to is not in a global scope. it is inside an anonymous function. Need to pull it out one level.

btbonval commented 10 years ago

Make label go away: drop auto_id=False into the Form class. https://docs.djangoproject.com/en/1.5/ref/forms/api/#configuring-html-label-tags

But that'll still display really useless text "Fp file:" generated from the field's name. RAGE!!! Why wouldn't they talk about how to disable that text entirely on the same page?

btbonval commented 10 years ago

Nothing about how to do it with meta, but "label" is a keyword thing for a field. I'm guessing make a dictionary called labels in the Meta class will do the trick. https://docs.djangoproject.com/en/1.5/ref/forms/fields/#django.forms.Field.label

btbonval commented 10 years ago

NOPE

btbonval commented 10 years ago

Okay so you have to refactor field junk out of Meta and into the Form to adjust the label, but you can keep everything in Meta to adjust widgets. I don't get it, but whatever. https://docs.djangoproject.com/en/1.5/topics/forms/modelforms/#overriding-the-default-field-types-or-widgets

so sick of django right now.

btbonval commented 10 years ago

No, refactoring it to set the label messes everything up. Screw that. Let's hope this "label" is the one I'm trying to remove: * The form field’s label is set to the verbose_name of the model field, with the first character capitalized. https://docs.djangoproject.com/en/1.5/topics/forms/modelforms/#field-types

btbonval commented 10 years ago

Finally. Looks like Beta.

btbonval commented 10 years ago

File was chosen, but nothing happened. Javascript did not activate to unhide the bit about tags and such. Wonder why that doesn't happen now.

btbonval commented 10 years ago

Comparing original:

      var fileup = document.getElementById('filepicker-file-upload');
      fileup.onchange = function(event){
        $dropzone_result.text(event);
          for (var i=0; i < event.fpfiles.length; i++){
            makeFileForm(event.fpfiles[i]);
          }
      };

to new thing:

<input ... id="filepicker-file-upload"  onchange="got_file" type="filepicker-dragdrop" />
    var got_file = function(event){
      $dropzone_result.text(event);
        for (var i=0; i < event.fpfiles.length; i++){
          makeFileForm(event.fpfiles[i]);
        }

No errors saying got_file couldn't be found, as it had done earlier. Instead ... just nothing.

btbonval commented 10 years ago

"When the dialog finishes uploading the file, the javascript code in the onchange field will be run with a special 'event' variable". This is going to be one of those dumb things where JavaScript has to call an anonymous function to call a named function, instead of simply being able to pass a reference to the named function?

onchange="function (files) {got_file(files);}"

Javascript at least has the decency to error:

SyntaxError: function statement requires a name

Yup, definitely one of those really subtle Browser / JavaScript things that people who've done this forever totally get, and other people take hours debugging.

btbonval commented 10 years ago

Uhm, so definitely looks like some kind of ECMA thingy. anonymous functions only work when being set? So I guess i'll try the Function thing which seems a bit more explicit way of making an anon func. http://stackoverflow.com/a/1140107/1867779

btbonval commented 10 years ago

back to nothin. No errors, no output, no changes.

onchange="new Function('files','got_file(files);')"
btbonval commented 10 years ago

Ran this in my console and magic.

$('filepicker-file-upload').context.onchange = function (files) {got_file(files);}

I happened to notice that $('filepicker-file-upload').context.onchange was initially null. Does the Input onchange actually do anything? It is documented under Drag-Drop widget for Filepicker. https://developers.inkfilepicker.com/docs/web/#widgets-pick

Am I being dumb or is the documentation being dumb?

btbonval commented 10 years ago

Beta also evaluates $('filepicker-file-upload').context.onchange to null on load, yet it works just fine. Aha, document.getElementById('filepicker-file-upload').onchange evaluates to a function on Beta. And this is the function:

(function (event){
        $dropzone_result.text(event);
          for (var i=0; i < event.fpfiles.length; i++){
            makeFileForm(event.fpfiles[i]);
          }
      })

compare to document.getElementById('filepicker-file-upload').onchange from the input thing:

function onchange(event) {
new Function('files','got_file(files);')
}

oh, so the format is totally different this way. It wraps onchange(event) {}, which is way easier. I should just be able to call got_files(event) for onchange.

btbonval commented 10 years ago

Did it. Okay, so the jQuery interface to onchange is much different than the html attribute onchange, and I found zero helpful tips on that on the internets.

Server isn't as pleased as I am at this moment.

ERROR:django.request:Internal Server Error: /api/upload
Traceback (most recent call last):
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 115, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/home/vagrant/karmaworld/karmaworld/apps/document_upload/views.py", line 16, in save_fp_upload
    if r_d_f.is_valid():
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/forms/forms.py", line 126, in is_valid
...
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django_filepicker/forms.py", line 104, in to_python
    url_fp = urllib2.urlopen(data)
...
HTTPError: HTTP Error 400: BAD REQUEST

That FilePicker bit is here: https://github.com/Ink/django-filepicker/blob/master/django_filepicker/forms.py#L104

Alright well I had no idea that RawDocument validation downloaded the FP file during is_valid(), so that's new. It looks like there is some kind of error coming out of what I presume is Filepicker returning that 400.

btbonval commented 10 years ago

Nothing for it but to edit Filepicker code on my system and add some debug to the data to see what URL is being checked and see what's wrong with it.

yay

btbonval commented 10 years ago

URL is https://www.filepicker.io/api/file/bWhokSChTK6vw5TXh5Nk. Error:

[uuid=45F7FCF656034C5C] This action has been secured by the developer of this website. Error: Proper credentials not provided. Signature: None Policy: None

So basically Filepicker is ignoring the signature and policy, which were supplied to the FPFileField, when it comes time to construct a URL for this sort of stuff. Thanks again, django-filepicker, for being useful. I need to figure out where this URL is constructed in django-filepicker so I can fix it. Open source is beautiful.

btbonval commented 10 years ago

oh right the URL is the raw data. So this is nice, we don't want to store the sign and policy, but we do want to attach them to the URL when needed. They should be appended to the data in the to_python method, assuming they are available to the form from the model.

btbonval commented 10 years ago

Sweet. self.additional_params is available. Time to hack up some more django-filepicker. Might actually do a pull request for this one.

btbonval commented 10 years ago

Okay. Rewrote django-filepicker. I should probably fork their repo, move my change into that, and submit a pull request. But for now, I want our system to work.

celery is trying to read the Filepicker URL without adding the signature and policy. So basically same thing all over again, but easier because it's our code in gdrive.py.

btbonval commented 10 years ago

I'd like to use Filepicker's get_file method, but it ignores signature and policy. So I'd have to write that up as well as part of a fork, then we can switch to using that. We'll have to use a forked Filepicker anyway.

The alternative is to hack it up into our current Document.get_file or wherever, which we should ideally drop anyway.

btbonval commented 10 years ago

Wrote some fixes. https://github.com/btbonval/django-filepicker

Nuking my venv and starting over with my modified version of django-filepicker for testing.

btbonval commented 10 years ago

what on earth? s3boto is saying FPFileField.url is scary? Why is this suddenly a thing?

[2014-01-31 06:27:15,041: ERROR/MainProcess] karmaworld.apps.document_upload.tasks.process_raw_document[6c8c6b2d-8137-4552-b36e-ce7ef91c1c39]: Traceback (most recent call last):
  File "/home/vagrant/karmaworld/karmaworld/apps/document_upload/tasks.py", line 16, in process_raw_document
    convert_raw_document(raw_document, user=user)
  File "/home/vagrant/karmaworld/karmaworld/apps/notes/gdrive.py", line 183, in convert_raw_document
    fp_file = raw_document.get_file()
  File "/home/vagrant/karmaworld/karmaworld/apps/notes/models.py", line 148, in get_file
    fpf = django_filepicker.utils.FilepickerFile(self.fp_file.url)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/django/db/models/fields/files.py", line 64, in _get_url
    return self.storage.url(self.name)
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/storages/backends/s3boto.py", line 257, in url
    name = self._normalize_name(self._clean_name(name))
  File "/var/www/karmaworld/venv/local/lib/python2.7/site-packages/storages/backends/s3boto.py", line 156, in _normalize_name
    raise SuspiciousOperation("Attempted access to '%s' denied." % name)
SuspiciousOperation: Attempted access to 'https:/www.filepicker.io/api/file/VlPtkajZSOUPFcFzc6qA' denied.
btbonval commented 10 years ago

Oh. I guess we were using FPFileField.name before, not FPFileField.url. Weird. whatevs don't care keep going.

btbonval commented 10 years ago

Can't read the File() object returned from FilepickerFile?

[2014-01-31 06:40:12,197: INFO/MainProcess] Got task from broker: karmaworld.apps.document_upload.tasks.process_raw_document[e55b2c6e-b740-4db6-ba9e-11cb7930cb65]
[2014-01-31 06:40:12,595: WARNING/PoolWorker-1] this is the mimetype of the document to check:
[2014-01-31 06:40:12,596: WARNING/PoolWorker-1] text/plain
[2014-01-31 06:40:12,596: ERROR/MainProcess] karmaworld.apps.document_upload.tasks.process_raw_document[e55b2c6e-b740-4db6-ba9e-11cb7930cb65]: Traceback (most recent call last):
  File "/home/vagrant/karmaworld/karmaworld/apps/document_upload/tasks.py", line 16, in process_raw_document
    convert_raw_document(raw_document, user=user)
  File "/home/vagrant/karmaworld/karmaworld/apps/notes/gdrive.py", line 196, in convert_raw_document
    original_content = fp_file.read()
ValueError: I/O operation on closed file