Merge pull request #492 from exarkun/2879.magic-folder-create-and-umask.0

Create the magic-folder local directory if necessary

src/allmydata/frontends/magic_folder.py

@@ -1,6 +1,7 @@
 
 import sys, os
 import os.path
+from errno import EEXIST
 from collections import deque
 from datetime import datetime
 import time
@@ -40,6 +41,13 @@ _DEFAULT_DOWNLOAD_UMASK = 0o077
 
 IN_EXCL_UNLINK = 0x04000000L
 
+
+class ConfigurationError(Exception):
+    """
+    There was something wrong with some magic-folder configuration.
+    """
+
+
 def get_inotify_module():
     try:
         if sys.platform == "win32":
@@ -148,22 +156,6 @@ def load_magic_folders(node_directory):
                 interval = int(config.get("magic_folder", "poll_interval"))
             except ConfigParser.NoOptionError:
                 interval = 60
-            dir_fp = to_filepath(directory)
-
-            if not dir_fp.exists():
-                raise Exception(
-                    "The '[magic_folder] local.directory' parameter is {} "
-                    "but there is no directory at that location.".format(
-                        quote_local_unicode_path(directory),
-                    )
-                )
-            if not dir_fp.isdir():
-                raise Exception(
-                    "The '[magic_folder] local.directory' parameter is {} "
-                    "but the thing at that location is not a directory.".format(
-                        quote_local_unicode_path(directory)
-                    )
-                )
 
             if config.has_option("magic_folder", "download.umask"):
                 umask = int(config.get("magic_folder", "download.umask"), 8)
@@ -211,31 +203,87 @@ def load_magic_folders(node_directory):
                 )
 
     # check configuration
-    for (name, mf_config) in folders.items():
+    folders = dict(
-        if not isinstance(mf_config, dict):
+        (name, fix_magic_folder_config(yaml_fname, name, config))
-            raise Exception(
+        for (name, config)
-                "Each item in '{}' must itself be a dict".format(yaml_fname)
+        in folders.items()
-            )
+    )
-        for k in ['collective_dircap', 'upload_dircap', 'directory', 'poll_interval']:
-            if k not in mf_config:
-                raise Exception(
-                    "Config for magic folder '{}' is missing '{}'".format(
-                        name, k
-                    )
-                )
-        for k in ['collective_dircap', 'upload_dircap']:
-            if isinstance(mf_config[k], unicode):
-                mf_config[k] = mf_config[k].encode('ascii')
-
-        if not isinstance(
-            mf_config.setdefault(u"umask", _DEFAULT_DOWNLOAD_UMASK),
-            int,
-        ):
-            raise Exception("magic-folder download umask must be an integer")
-
     return folders
 
 
+def fix_magic_folder_config(yaml_fname, name, config):
+    """
+    Check the given folder configuration for validity.
+
+    If it refers to a local directory which does not exist, create that
+    directory with the configured permissions.
+
+    :param unicode yaml_fname: The configuration file from which the
+        configuration was read.
+
+    :param unicode name: The name of the magic-folder this particular
+        configuration blob is associated with.
+
+    :param config: The configuration for a single magic-folder.  This is
+        expected to be a ``dict`` with certain keys and values of certain
+        types but these properties will be checked.
+
+    :raise ConfigurationError: If the given configuration object does not
+        conform to some magic-folder configuration requirement.
+    """
+    if not isinstance(config, dict):
+        raise ConfigurationError(
+            "Each item in '{}' must itself be a dict".format(yaml_fname)
+        )
+
+    for k in ['collective_dircap', 'upload_dircap', 'directory', 'poll_interval']:
+        if k not in config:
+            raise ConfigurationError(
+                "Config for magic folder '{}' is missing '{}'".format(
+                    name, k
+                )
+            )
+
+    if not isinstance(
+        config.setdefault(u"umask", _DEFAULT_DOWNLOAD_UMASK),
+        int,
+    ):
+        raise Exception("magic-folder download umask must be an integer")
+
+    # make sure directory for magic folder exists
+    dir_fp = to_filepath(config['directory'])
+    umask = config.setdefault('umask', 0077)
+
+    try:
+        os.mkdir(dir_fp.path, 0777 & (~ umask))
+    except OSError as e:
+        if EEXIST != e.errno:
+            # Report some unknown problem.
+            raise ConfigurationError(
+                "magic-folder {} configured path {} could not be created: "
+                "{}".format(
+                    name,
+                    dir_fp.path,
+                    str(e),
+                ),
+            )
+        elif not dir_fp.isdir():
+            # Tell the user there's a collision.
+            raise ConfigurationError(
+                "magic-folder {} configured path {} exists and is not a "
+                "directory".format(
+                    name, dir_fp.path,
+                ),
+            )
+
+    result_config = config.copy()
+    for k in ['collective_dircap', 'upload_dircap']:
+        if isinstance(config[k], unicode):
+            result_config[k] = config[k].encode('ascii')
+    return result_config
+
+
+
 def save_magic_folders(node_directory, folders):
     fileutil.write_atomically(
         os.path.join(node_directory, u"private", u"magic_folders.yaml"),

src/allmydata/test/test_magic_folder.py

@@ -1,8 +1,8 @@
 
 import os, sys, time
-import shutil, json
+import stat, shutil, json
 import mock
-from os.path import join, exists
+from os.path import join, exists, isdir
 
 from twisted.trial import unittest
 from twisted.internet import defer, task, reactor
@@ -19,7 +19,10 @@ from allmydata.test.common import ShouldFailMixin
 from .cli.test_magic_folder import MagicFolderCLITestMixin
 
 from allmydata.frontends import magic_folder
-from allmydata.frontends.magic_folder import MagicFolder, WriteFileMixin
+from allmydata.frontends.magic_folder import (
+    MagicFolder, WriteFileMixin,
+    ConfigurationError,
+)
 from allmydata import magicfolderdb, magicpath
 from allmydata.util.fileutil import get_pathinfo
 from allmydata.util.fileutil import abspath_expanduser_unicode
@@ -31,6 +34,9 @@ _debug = False
 class NewConfigUtilTests(unittest.TestCase):
 
     def setUp(self):
+        # some tests look at the umask of created directories or files
+        # so we set an explicit one
+        self._old_umask = os.umask(0o022)
         self.basedir = abspath_expanduser_unicode(unicode(self.mktemp()))
         os.mkdir(self.basedir)
         self.local_dir = abspath_expanduser_unicode(unicode(self.mktemp()))
@@ -52,21 +58,96 @@ class NewConfigUtilTests(unittest.TestCase):
         }
 
         # we need a bit of tahoe.cfg
-        with open(join(self.basedir, u"tahoe.cfg"), "w") as f:
+        self.write_tahoe_config(
-            f.write(
+            self.basedir,
-                u"[magic_folder]\n"
+            u"[magic_folder]\n"
-                u"enabled = True\n"
+            u"enabled = True\n",
-            )
+        )
         # ..and the yaml
-        yaml_fname = join(self.basedir, u"private", u"magic_folders.yaml")
+        self.write_magic_folder_config(self.basedir, self.folders)
+
+    def tearDown(self):
+        os.umask(self._old_umask)
+
+    def write_tahoe_config(self, basedir, tahoe_config):
+        with open(join(basedir, u"tahoe.cfg"), "w") as f:
+            f.write(tahoe_config)
+
+    def write_magic_folder_config(self, basedir, folder_configuration):
+        yaml_fname = join(basedir, u"private", u"magic_folders.yaml")
         with open(yaml_fname, "w") as f:
-            f.write(yamlutil.safe_dump({u"magic-folders": self.folders}))
+            f.write(yamlutil.safe_dump({u"magic-folders": folder_configuration}))
 
     def test_load(self):
         folders = magic_folder.load_magic_folders(self.basedir)
         self.assertEqual(['default'], list(folders.keys()))
         self.assertEqual(folders['default'][u'umask'], 0o077)
 
+    def test_load_makes_directory(self):
+        """
+        If the *directory* does not exist then it is created by
+        ``load_magic_folders``.
+        """
+        os.rmdir(self.local_dir)
+        # Just pick some arbitrary bits.
+        # rwxr-xr--
+        perm = stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH
+        self.folders[u"default"][u"umask"] = (0o777 & ~perm)
+        self.write_magic_folder_config(self.basedir, self.folders)
+
+        magic_folder.load_magic_folders(self.basedir)
+
+        # It is created.
+        self.assertTrue(
+            isdir(self.local_dir),
+            "magic-folder local directory {} was not created".format(
+                self.local_dir,
+            ),
+        )
+        # It has permissions determined by the configured umask.
+        if sys.platform != "win32":
+            self.assertEqual(
+                perm,
+                stat.S_IMODE(os.stat(self.local_dir).st_mode),
+            )
+        else:
+            # Do directories even have permissions on Windows?
+            print("Not asserting directory-creation mode on windows")
+
+    def test_directory_collision(self):
+        """
+        If a non-directory already exists at the magic folder's configured local
+        directory path, ``load_magic_folders`` raises an exception.
+        """
+        os.rmdir(self.local_dir)
+        open(self.local_dir, "w").close()
+
+        with self.assertRaises(ConfigurationError) as ctx:
+            magic_folder.load_magic_folders(self.basedir)
+        self.assertIn(
+            "exists and is not a directory",
+            str(ctx.exception),
+        )
+
+    def test_directory_creation_error(self):
+        """
+        If a directory at the magic folder's configured local directory path
+        cannot be created for some other reason, ``load_magic_folders`` raises
+        an exception.
+        """
+        os.rmdir(self.local_dir)
+        open(self.local_dir, "w").close()
+        self.folders[u"default"][u"directory"] = self.local_dir + "/foo"
+        self.write_magic_folder_config(self.basedir, self.folders)
+
+        with self.assertRaises(ConfigurationError) as ctx:
+            magic_folder.load_magic_folders(self.basedir)
+        self.assertIn(
+            "could not be created",
+            str(ctx.exception),
+        )
+
+
     def test_both_styles_of_config(self):
         os.unlink(join(self.basedir, u"private", u"magic_folders.yaml"))
         with self.assertRaises(Exception) as ctx:
@@ -194,22 +275,25 @@ class LegacyConfigUtilTests(unittest.TestCase):
             pass
 
     def test_load_legacy_no_dir(self):
+        expected = self.local_dir + 'foo'
         with open(join(self.basedir, u"tahoe.cfg"), "w") as f:
             f.write(
                 u"[magic_folder]\n"
                 u"enabled = True\n"
                 u"local.directory = {}\n"
                 u"poll_interval = {}\n".format(
-                    self.local_dir + 'foo',
+                    expected,
                     self.poll_interval,
                 )
             )
 
-        with self.assertRaises(Exception) as ctx:
+        magic_folder.load_magic_folders(self.basedir)
-            magic_folder.load_magic_folders(self.basedir)
+
-        self.assertIn(
+        self.assertTrue(
-            "there is no directory at that location",
+            isdir(expected),
-            str(ctx.exception)
+            "magic-folder local directory {} was not created".format(
+                expected,
+            ),
         )
 
     def test_load_legacy_not_a_dir(self):
@@ -226,10 +310,10 @@ class LegacyConfigUtilTests(unittest.TestCase):
         with open(self.local_dir + "foo", "w") as f:
             f.write("not a directory")
 
-        with self.assertRaises(Exception) as ctx:
+        with self.assertRaises(ConfigurationError) as ctx:
             magic_folder.load_magic_folders(self.basedir)
         self.assertIn(
-            "location is not a directory",
+            "is not a directory",
             str(ctx.exception)
         )