فهرست منبع

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 12 سال پیش
والد
کامیت
89218ae

+ 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):