Explorar o código

HUE-1094 [fb] Fail early when creating files in a folder without permissions

Do the upload as the current user as we can authenticate it before and are uploading
to the destination path.
Upload a file gets triggered by the is_ajax middleware. request.is_ajax() is not set
for some reason.
Upload archive works as expected but could not spot the different. This could be useful
for bubbling up the exception in the notification.
Romain Rigaux %!s(int64=12) %!d(string=hai) anos
pai
achega
89218aedca

+ 4 - 2
apps/filebrowser/src/filebrowser/templates/listdir_components.mako

@@ -1241,9 +1241,11 @@ from django.utils.translation import ugettext as _
             dest:self.currentPath(),
             fileFieldLabel:"hdfs_file"
           },
-          onComplete:function (id, fileName, responseJSON) {
+          onComplete:function (id, fileName, response) {
             num_of_pending_uploads--;
-            if (num_of_pending_uploads == 0) {
+            if (response.status != 0) {
+              $.jHueNotify.error("${ _('Error: ') }" + (response['data'] ? response['data'] : "${ _('Check file permissions') }"));
+            } else if (num_of_pending_uploads == 0) {
               window.location = "/filebrowser/view" + self.currentPath();
             }
           },

+ 10 - 14
apps/filebrowser/src/filebrowser/views.py

@@ -1123,7 +1123,7 @@ def trash_purge(request):
 
 def upload_file(request):
     """
-    A wrapper around the actual upload view function to clean up the temporary file afterwards.
+    A wrapper around the actual upload view function to clean up the temporary file afterwards if it fails.
 
     Returns JSON.
     e.g. {'status' 0/1, data:'message'...}
@@ -1132,12 +1132,10 @@ def upload_file(request):
 
     if request.method == 'POST':
         try:
-            try:
-                resp = _upload_file(request)
-                response.update(resp)
-            except Exception, ex:
-                response['data'] = str(ex)
-        finally:
+            resp = _upload_file(request)
+            response.update(resp)
+        except Exception, ex:
+            response['data'] = str(ex)
             hdfs_file = request.FILES.get('hdfs_file')
             if hdfs_file:
                 hdfs_file.remove()
@@ -1154,7 +1152,9 @@ def upload_file(request):
 def _upload_file(request):
     """
     Handles file uploaded by HDFSfileUploadHandler.
-    The uploaded file is stored in HDFS. We just need to rename it to the destination path.
+
+    The uploaded file is stored in HDFS at its destination with a .tmp suffix.
+    We just need to rename it to the destination path.
     """
     form = UploadFileForm(request.POST, request.FILES)
 
@@ -1170,12 +1170,8 @@ def _upload_file(request):
         username = request.user.username
 
         try:
-            # Temp file is created by superuser. Chown the file.
-            request.fs.do_as_superuser(request.fs.chmod, tmp_file, 0644)
-            request.fs.do_as_superuser(request.fs.chown, tmp_file, username, username)
-
-            # Move the file to where it belongs
-            request.fs.rename(tmp_file, dest)
+            # Remove tmp suffix of the file
+            request.fs.do_as_user(username, request.fs.rename, tmp_file, dest)
         except IOError, ex:
             already_exists = False
             try:

+ 16 - 9
apps/filebrowser/src/filebrowser/views_test.py

@@ -28,7 +28,6 @@ import os
 import re
 import urlparse
 from avro import schema, datafile, io
-from StringIO import StringIO
 
 from django.utils.encoding import smart_str
 from nose.plugins.attrib import attr
@@ -459,7 +458,7 @@ def test_listdir():
 
     for dirent in dir_listing:
       path = dirent['name']
-      if path == '..':
+      if path in ('.', '..'):
         continue
 
       assert_true(path in orig_paths)
@@ -469,7 +468,8 @@ def test_listdir():
       resp = c.get(url)
 
       # We are actually reading a directory
-      assert_equal('..', resp.context['files'][0]['name'], "'%s' should be a directory" % (path,))
+      assert_equal('.', resp.context['files'][0]['name'])
+      assert_equal('..', resp.context['files'][1]['name'])
 
     # Test's home directory now exists. Should be returned.
     c = make_logged_in_client()
@@ -984,8 +984,12 @@ def test_upload_file():
     cluster.fs.do_as_superuser(cluster.fs.chown, HDFS_DEST_DIR, USER_NAME, USER_NAME)
     cluster.fs.do_as_superuser(cluster.fs.chmod, HDFS_DEST_DIR, 0700)
 
+    stats = cluster.fs.stats(HDFS_DEST_DIR)
+    assert_equal(stats['user'], USER_NAME)
+    assert_equal(stats['group'], USER_NAME)
+
     # Just upload the current python file
-    resp = client.post('/filebrowser/upload/file',
+    resp = client.post('/filebrowser/upload/file?dest=%s' % HDFS_DEST_DIR,
                        dict(dest=HDFS_DEST_DIR, hdfs_file=file(LOCAL_FILE)))
     response = json.loads(resp.content)
 
@@ -1000,16 +1004,19 @@ def test_upload_file():
     assert_equal(actual, expected)
 
     # Upload again and so fails because file already exits
-    resp = client.post('/filebrowser/upload/file',
+    resp = client.post('/filebrowser/upload/file?dest=%s' % HDFS_DEST_DIR,
                        dict(dest=HDFS_DEST_DIR, hdfs_file=file(LOCAL_FILE)))
     response = json.loads(resp.content)
     assert_equal(-1, response['status'], response)
+    assert_true('already exists' in response['data'], response)
 
     # Upload in tmp and fails because of missing permissions
-    resp = client_not_me.post('/filebrowser/upload/file',
-                              dict(dest=HDFS_DEST_DIR, hdfs_file=file(LOCAL_FILE)))
-    response = json.loads(resp.content)
-    assert_equal(-1, response['status'], response)
+    try:
+      resp = client_not_me.post('/filebrowser/upload/file?dest=%s' % HDFS_DEST_DIR,
+                                dict(dest=HDFS_DEST_DIR, hdfs_file=file(LOCAL_FILE)))
+      raise Exception('Should have sent a permissions exception!')
+    except Exception, e:
+      assert_true('Permission denied' in str(e), e)
   finally:
     try:
       cluster.fs.remove(HDFS_DEST_DIR)

+ 4 - 4
desktop/core/src/desktop/settings.py

@@ -95,9 +95,9 @@ TEMPLATE_LOADERS = (
     'desktop.lib.template_loader.load_template_source',
 )
 
-MIDDLEWARE_CLASSES = [
+MIDDLEWARE_CLASSES = (
+    # The order matters
     'desktop.middleware.DatabaseLoggingMiddleware',
-
     'django.middleware.common.CommonMiddleware',
     'desktop.middleware.SessionOverPostMiddleware',
     'django.contrib.sessions.middleware.SessionMiddleware',
@@ -107,7 +107,7 @@ MIDDLEWARE_CLASSES = [
     'django.middleware.locale.LocaleMiddleware',
     'babeldjango.middleware.LocaleMiddleware',
     'desktop.middleware.AjaxMiddleware',
-    # Must be after Session, Auth, and Ajax.  Before everything else.
+    # Must be after Session, Auth, and Ajax. Before everything else.
     'desktop.middleware.LoginAndPermissionMiddleware',
     'django.contrib.messages.middleware.MessageMiddleware',
     'desktop.middleware.NotificationMiddleware',
@@ -117,7 +117,7 @@ MIDDLEWARE_CLASSES = [
     'desktop.middleware.AppSpecificMiddleware',
     'django.middleware.transaction.TransactionMiddleware',
     # 'debug_toolbar.middleware.DebugToolbarMiddleware'
-]
+)
 
 if os.environ.get(ENV_DESKTOP_DEBUG):
   MIDDLEWARE_CLASSES.append('desktop.middleware.HtmlValidationMiddleware')

+ 21 - 17
desktop/libs/hadoop/src/hadoop/fs/upload.py

@@ -31,8 +31,10 @@ import time
 
 from django.core.files.uploadhandler import \
     FileUploadHandler, StopFutureHandlers, StopUpload
+from django.utils.translation import ugettext as _
 import hadoop.cluster
 from hadoop.conf import UPLOAD_CHUNK_SIZE
+from hadoop.fs.exceptions import WebHdfsException
 
 UPLOAD_SUBDIR = 'hue-uploads'
 LOG = logging.getLogger(__name__)
@@ -59,15 +61,13 @@ class HDFStemporaryUploadedFile(object):
     if not self._fs:
       raise HDFSerror("No HDFS found")
 
-    # We want to set the user to be the superuser. But any operation
-    # in the fs needs a username, including the retrieval of the superuser.
-    # So we first set it to the DEFAULT_USER to break this chicken-&-egg.
-    self._fs.setuser(self._fs.DEFAULT_USER)
-    self._fs.setuser(self._fs.superuser)
-
-    self._path = self._fs.mkswap(name, suffix='tmp', basedir=destination)
-
-    self._file = self._fs.open(self._path, 'w')
+    # We want to set the user to be the user doing the upload
+    self._fs.setuser(request.user.username)
+    try:
+      self._path = self._fs.mkswap(name, suffix='tmp', basedir=destination)
+      self._file = self._fs.open(self._path, 'w')
+    except WebHdfsException, e:
+      raise e
     self._do_cleanup = True
 
   def __del__(self):
@@ -114,6 +114,13 @@ class HDFSfileUploadHandler(FileUploadHandler):
 
   This handler is triggered by any upload field whose name starts with
   "HDFS" (case insensitive).
+
+  In practice, the middlewares (which access the request.REQUEST/POST/FILES objects) triggers
+  the upload before reaching the view in case of permissions error. Read about Django
+  uploading documentation.
+  
+  This might trigger the upload before executing the hue auth middleware. HDFS destination
+  permissions will be doing the checks.
   """
   def __init__(self, request):
     FileUploadHandler.__init__(self, request)
@@ -126,19 +133,16 @@ class HDFSfileUploadHandler(FileUploadHandler):
 
   def new_file(self, field_name, file_name, *args, **kwargs):
     # Detect "HDFS" in the field name.
-    # NOTE: The user is not authenticated at this point, and it's
-    #       very difficult to do so because we handle upload before
-    #       running the auth middleware.
     if field_name.upper().startswith('HDFS'):
       try:
         self._file = HDFStemporaryUploadedFile(self.request, file_name, self._destination)
-      except (HDFSerror, IOError), ex:
+        LOG.debug('Upload attempt to %s' % (self._file.get_temp_path(),))
+        self._activated = True
+        self._starttime = time.time()
+      except Exception, ex:
         LOG.error("Not using HDFS upload handler: %s" % (ex,))
-        return
+        raise ex
 
-      LOG.debug('Upload attempt to %s' % (self._file.get_temp_path(),))
-      self._activated = True
-      self._starttime = time.time()
       raise StopFutureHandlers()
 
   def receive_data_chunk(self, raw_data, start):