Merge pull request #491 from exarkun/consolidate-magicfolder-config

Consolidate magic-folder configuration-loading code.

src/allmydata/client.py

@@ -593,23 +593,7 @@ class _Client(node.Node, pollmixin.PollMixin):
 
             for (name, mf_config) in magic_folders.items():
                 self.log("Starting magic_folder '{}'".format(name))
-                db_filename = os.path.join(self.basedir, "private", "magicfolder_{}.sqlite".format(name))
+                s = magic_folder.MagicFolder.from_config(self, name, mf_config)
-                local_dir_config = mf_config['directory']
-                try:
-                    poll_interval = int(mf_config["poll_interval"])
-                except ValueError:
-                    raise ValueError("'poll_interval' option must be an int")
-
-                s = magic_folder.MagicFolder(
-                    client=self,
-                    upload_dircap=mf_config["upload_dircap"],
-                    collective_dircap=mf_config["collective_dircap"],
-                    local_path_u=abspath_expanduser_unicode(local_dir_config, base=self.basedir),
-                    dbfile=abspath_expanduser_unicode(db_filename),
-                    umask=self.get_config("magic_folder", "download.umask", 0077),
-                    name=name,
-                    downloader_delay=poll_interval,
-                )
                 self._magic_folders[name] = s
                 s.setServiceParent(self)
                 s.startService()

src/allmydata/frontends/magic_folder.py

@@ -18,7 +18,12 @@ from zope.interface import Interface, Attribute, implementer
 from allmydata.util import fileutil, configutil, yamlutil
 from allmydata.interfaces import IDirectoryNode
 from allmydata.util import log
-from allmydata.util.fileutil import precondition_abspath, get_pathinfo, ConflictError
+from allmydata.util.fileutil import (
+    precondition_abspath,
+    get_pathinfo,
+    ConflictError,
+    abspath_expanduser_unicode,
+)
 from allmydata.util.assertutil import precondition, _assert
 from allmydata.util.deferredutil import HookMixin
 from allmydata.util.progress import PercentProgress
@@ -30,6 +35,9 @@ from allmydata.immutable.upload import FileName, Data
 from allmydata import magicfolderdb, magicpath
 
 
+# Mask off all non-owner permissions for magic-folders files by default.
+_DEFAULT_DOWNLOAD_UMASK = 0o077
+
 IN_EXCL_UNLINK = 0x04000000L
 
 def get_inotify_module():
@@ -119,6 +127,7 @@ def load_magic_folders(node_directory):
     old-style to new-style config (but WILL read old-style config and
     return in the same way as if it was new-style).
 
+    :param node_directory: path where node data is stored
     :returns: dict mapping magic-folder-name to its config (also a dict)
     """
     yaml_fname = os.path.join(node_directory, u"private", u"magic_folders.yaml")
@@ -156,11 +165,17 @@ def load_magic_folders(node_directory):
                     )
                 )
 
+            if config.has_option("magic_folder", "download.umask"):
+                umask = int(config.get("magic_folder", "download.umask"), 8)
+            else:
+                umask = _DEFAULT_DOWNLOAD_UMASK
+
             folders[u"default"] = {
                 u"directory": directory,
                 u"upload_dircap": fileutil.read(up_fname),
                 u"collective_dircap": fileutil.read(coll_fname),
                 u"poll_interval": interval,
+                u"umask": umask,
             }
         else:
             # without any YAML file AND no local.directory option it's
@@ -212,6 +227,12 @@ def load_magic_folders(node_directory):
             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
 
 
@@ -228,6 +249,43 @@ def save_magic_folders(node_directory, folders):
 
 class MagicFolder(service.MultiService):
 
+    @classmethod
+    def from_config(cls, client_node, name, config):
+        """
+        Create a ``MagicFolder`` from a client node and magic-folder
+        configuration.
+
+        :param _Client client_node: The client node the magic-folder is
+            attached to.
+
+        :param dict config: Magic-folder configuration like that in the list
+            returned by ``load_magic_folders``.
+        """
+        db_filename = os.path.join(
+            client_node.basedir,
+            "private",
+            "magicfolder_{}.sqlite".format(name),
+        )
+        local_dir_config = config['directory']
+        try:
+            poll_interval = int(config["poll_interval"])
+        except ValueError:
+            raise ValueError("'poll_interval' option must be an int")
+
+        return cls(
+            client=client_node,
+            upload_dircap=config["upload_dircap"],
+            collective_dircap=config["collective_dircap"],
+            local_path_u=abspath_expanduser_unicode(
+                local_dir_config,
+                base=client_node.basedir,
+            ),
+            dbfile=abspath_expanduser_unicode(db_filename),
+            umask=config["umask"],
+            name=name,
+            downloader_delay=poll_interval,
+        )
+
     def __init__(self, client, upload_dircap, collective_dircap, local_path_u, dbfile, umask,
                  name, uploader_delay=1.0, clock=None, downloader_delay=60):
         precondition_abspath(local_path_u)

src/allmydata/test/test_client.py

@@ -401,11 +401,18 @@ class Basic(testutil.ReallyEqualMixin, testutil.NonASCIIPathMixin, unittest.Test
         _check("helper.furl = pb://blah\n", "pb://blah")
 
     def test_create_magic_folder_service(self):
-        class MockMagicFolder(service.MultiService):
+        boom = False
+        class Boom(Exception):
+            pass
+
+        class MockMagicFolder(allmydata.frontends.magic_folder.MagicFolder):
             name = 'magic-folder'
 
             def __init__(self, client, upload_dircap, collective_dircap, local_path_u, dbfile, umask, name,
                          inotify=None, uploader_delay=1.0, clock=None, downloader_delay=3):
+                if boom:
+                    raise Boom()
+
                 service.MultiService.__init__(self)
                 self.client = client
                 self._umask = umask
@@ -415,6 +422,12 @@ class Basic(testutil.ReallyEqualMixin, testutil.NonASCIIPathMixin, unittest.Test
                 self.dbfile = dbfile
                 self.inotify = inotify
 
+            def startService(self):
+                self.running = True
+
+            def stopService(self):
+                self.running = False
+
             def ready(self):
                 pass
 
@@ -461,12 +474,8 @@ class Basic(testutil.ReallyEqualMixin, testutil.NonASCIIPathMixin, unittest.Test
         self.failUnless(magicfolder.inotify is None, magicfolder.inotify)
         self.failUnless(magicfolder.running)
 
-        class Boom(Exception):
+        # See above.
-            pass
+        boom = True
-        def BoomMagicFolder(client, upload_dircap, collective_dircap, local_path_u, dbfile, name,
-                            umask, inotify=None, uploader_delay=1.0, clock=None, downloader_delay=3):
-            raise Boom()
-        self.patch(allmydata.frontends.magic_folder, 'MagicFolder', BoomMagicFolder)
 
         basedir2 = "test_client.Basic.test_create_magic_folder_service2"
         os.mkdir(basedir2)

src/allmydata/test/test_magic_folder.py

@@ -65,6 +65,7 @@ class NewConfigUtilTests(unittest.TestCase):
     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_both_styles_of_config(self):
         os.unlink(join(self.basedir, u"private", u"magic_folders.yaml"))
@@ -115,6 +116,23 @@ class NewConfigUtilTests(unittest.TestCase):
             str(ctx.exception)
         )
 
+    def test_wrong_umask_obj(self):
+        """
+        If a umask is given for a magic-folder that is not an integer, an
+        exception is raised.
+        """
+        self.folders[u"default"][u"umask"] = "0077"
+        yaml_fname = join(self.basedir, u"private", u"magic_folders.yaml")
+        with open(yaml_fname, "w") as f:
+            f.write(yamlutil.safe_dump({u"magic-folders": self.folders}))
+
+        with self.assertRaises(Exception) as ctx:
+            magic_folder.load_magic_folders(self.basedir)
+        self.assertIn(
+            "umask must be an integer",
+            str(ctx.exception)
+        )
+
     def test_wrong_sub_obj(self):
         yaml_fname = join(self.basedir, u"private", u"magic_folders.yaml")
         with open(yaml_fname, "w") as f: