Merge branch 'master' into 258-new-tab

remoteappmanager/spawners.py

@@ -1,6 +1,6 @@
 import os
-import shutil
-import tempfile
+import escapism
+import string
 
 from traitlets import Any, Unicode
 from tornado import gen
@@ -153,8 +153,9 @@ def start(self):
         # Create the temporary directory as the user's workspace
         if self.workspace_dir and not self._virtual_workspace:
             try:
-                self._virtual_workspace = tempfile.mkdtemp(
-                    dir=self.workspace_dir)
+                workspace = _user_workspace(self.workspace_dir, self.user.name)
+                os.makedirs(workspace, 0o755, exist_ok=True)
+                self._virtual_workspace = workspace
             except Exception as exception:
                 # A whole lot of reasons why temporary directory cannot
                 # be created. e.g. workspace_dir does not exist
@@ -178,7 +179,6 @@ def start(self):
         try:
             return (yield super().start())
         except Exception:
-            self._clean_up_workspace_dir()
             raise
 
     @gen.coroutine
@@ -188,29 +188,8 @@ def stop(self, now=False):
         If virtual user has a temporary home directory,
         clean up the directory.
         """
-        self._clean_up_workspace_dir()
         yield super().stop(now=now)
 
-    def _clean_up_workspace_dir(self):
-        """ Clean up the virtual user's temporary directory, if exists
-        """
-        if not self._virtual_workspace:
-            return
-
-        # Clean up the directory
-        # Make sure the temporary directory is not /, ./ or ../
-        if os.path.normpath(self._virtual_workspace) in ('/', '.', '..'):
-            self.log.warning("Virtual workspace is '%s'.  Seriously? "
-                             "Not removing.", self._virtual_workspace)
-        else:
-            try:
-                shutil.rmtree(self._virtual_workspace)
-            except Exception as exception:
-                self.log.error("Failed to remove %s, error %s",
-                               self._virtual_workspace, str(exception))
-            else:
-                self.log.info('Removed %s', self._virtual_workspace)
-
 
 def _docker_envvars():
     """Returns a dictionary containing the docker environment variables, if
@@ -223,3 +202,29 @@ def _docker_envvars():
            if envvar in os.environ}
 
     return env
+
+
+# Used by escape
+_ESCAPE_SAFE_CHARS = set(string.ascii_letters + string.digits + '-.')
+_ESCAPE_CHAR = '_'
+
+
+# Note: copied from container_manager.py, but we want to keep the
+# spawners module completely separated from the remoteappmanager.
+def escape(s):
+    """Trivial escaping wrapper for well established stuff.
+    Works for containers, file names. Note that it is not destructive,
+    so it won't generate collisions."""
+    return escapism.escape(s, _ESCAPE_SAFE_CHARS, _ESCAPE_CHAR)
+
+
+def _user_workspace(base_dir, user_name):
+    """Returns the appropriate user workspace for the given username.
+    Raises ValueError if the user_name is only spaces after basenaming.
+    """
+
+    name = os.path.basename(user_name).strip()
+    if len(name) == 0:
+        raise ValueError("User name is invalid")
+
+    return os.path.join(base_dir, escape(name))

remoteappmanager/tests/test_spawners.py

@@ -171,12 +171,38 @@ def test_spawner_with_workspace_dir(self):
             self.assertIn(os.path.basename(virtual_directory),
                           os.listdir(self.tempdir))
 
-        # The temporary directory should be removed upon stop
-        self.assertFalse(os.listdir(self.tempdir))
+        self.assertIn(os.path.basename(virtual_directory),
+                      os.listdir(self.tempdir))
 
         status = self.io_loop.run_sync(self.spawner.poll)
         self.assertEqual(status, 1)
 
+    def test_spawner_with_workspace_dir_already_existent(self):
+        self.spawner.workspace_dir = self.tempdir
+        os.mkdir(os.path.join(self.tempdir, username()))
+
+        with spawner_start_and_stop(self.io_loop, self.spawner):
+            status = self.io_loop.run_sync(self.spawner.poll)
+            self.assertIsNone(status)
+
+            # There should be a temporary directory created
+            # and it should be assigned to _virtual_workspace
+            virtual_directory = self.spawner._virtual_workspace
+            self.assertIn(os.path.basename(virtual_directory),
+                          os.listdir(self.tempdir))
+
+        self.assertIn(os.path.basename(virtual_directory),
+                      os.listdir(self.tempdir))
+
+    def test_spawner_with_workspace_dir_as_file(self):
+        self.spawner.workspace_dir = self.tempdir
+
+        with open(os.path.join(self.tempdir, username()), 'w'):
+            pass
+
+        with spawner_start_and_stop(self.io_loop, self.spawner):
+            self.assertIsNone(self.spawner.get_env().get('HOME'))
+
     def test_env_has_proxy_api_token(self):
         env = self.spawner.get_env()
         self.assertIn("PROXY_API_TOKEN", env)
@@ -213,22 +239,6 @@ def test_state_if_workspace_not_defined(self):
             state = self.spawner.get_state()
             self.assertNotIn('virtual_workspace', state)
 
-    def test_clean_up_temporary_dir_if_start_fails(self):
-        self.spawner.workspace_dir = self.tempdir
-
-        # mock LocalProcessSpawner.start to fail
-        def start_fail(instance):
-            raise Exception
-
-        with mock.patch('jupyterhub.spawner.LocalProcessSpawner.start',
-                        start_fail), \
-                self.assertRaises(Exception), \
-                spawner_start_and_stop(self.io_loop, self.spawner):
-            pass
-
-        # The temporary directory should be cleaned up
-        self.assertFalse(os.listdir(self.tempdir))
-
     def test_start_if_workspace_path_not_exists(self):
         self.spawner.workspace_dir = '/no_way/this_exists'