mbraak/django-file-form

check_permission() - anonymous? when to fail?

sebhaase opened this issue · 10 comments

I'm trying to extend my permissions scenario - who should be allowed to upload...

  1. I noticed that non-logged-in users get only "Upload failed" message without any info why. - I traced that down to utils.py check_permissions() which in turn gets called by

    def start_upload(request):

  2. then I logged in as a user who should not be allowed to upload -- there I found that the upload still happend into the temp folder - since I only check permissions in my form clean(self) function

What is the best way of providing meaningful error message and doing the checks as early as possible? is this docuemented (I did not see it...)

Thx, Sebastian

I'll check if the upload to the temp folder can be prevented.

About the permissions: one solution is to disable uploads. Would that be a solution? I think it's not possible at this moment to disable uploads. I'll check that.

A possible solution is to make the field disabled when the user doesn't have the permission to upload. Pr #680 adds support for disabled fields.

Another solution is to hide the field.

I'll check if it's possible to display an error message.

It's a good idea to check as early as possible if a user has permission to upload; the easiest solution is to make the check_permission function configurable.

In the settings file:

def custom_check_permission(request):
  if not request.user.is_authenticated:
    raise PermissionDenied()

FILE_FORM_CHECK_PERMISSIONS_FUNCTION = custom_check_permission

Would that work in your situation?

I have another issue: to really determine if the upload should be allowed, I would need to get values from other fields of the form... I don't see how that would be at all possible...
So, guess that part comes down to the question of how to delete temp uploads on failed (or never triggered) save...

The form has a delete_temporary_files method that removes all temporary files. So that's in the right direction.

I can have a look later if it's possible to remove files for a single field.

I added the method delete_temporary_files_for_field in pr #689.

mbraak commented

Does pr #689 fix the issue?