From 169ccbfbca0f78f7322a696bc2a3964f85332168 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 23 Apr 2018 10:39:11 -0400 Subject: [PATCH 1/3] directory creation fixes --- src/allmydata/frontends/magic_folder.py | 103 ++++++++++++++-------- src/allmydata/test/test_magic_folder.py | 110 ++++++++++++++++++++---- 2 files changed, 157 insertions(+), 56 deletions(-) diff --git a/src/allmydata/frontends/magic_folder.py b/src/allmydata/frontends/magic_folder.py index 24fd9cb0f..d7c9cb743 100644 --- a/src/allmydata/frontends/magic_folder.py +++ b/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,11 @@ _DEFAULT_DOWNLOAD_UMASK = 0o077 IN_EXCL_UNLINK = 0x04000000L + +class ConfigurationError(Exception): + pass + + def get_inotify_module(): try: if sys.platform == "win32": @@ -148,22 +154,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 +201,68 @@ def load_magic_folders(node_directory): ) # check configuration - for (name, mf_config) in folders.items(): - if not isinstance(mf_config, dict): - raise Exception( - "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 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") - + folders = dict( + (name, fix_magic_folder_config(yaml_fname, name, config)) + for (name, config) + in folders.items() + ) return folders +def fix_magic_folder_config(yaml_fname, name, config): + if not isinstance(config, dict): + raise Exception( + "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 Exception( + "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"), diff --git a/src/allmydata/test/test_magic_folder.py b/src/allmydata/test/test_magic_folder.py index 0f26c99f2..cd8e6844b 100644 --- a/src/allmydata/test/test_magic_folder.py +++ b/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 @@ -52,21 +55,89 @@ class NewConfigUtilTests(unittest.TestCase): } # we need a bit of tahoe.cfg - with open(join(self.basedir, u"tahoe.cfg"), "w") as f: - f.write( - u"[magic_folder]\n" - u"enabled = True\n" - ) + self.write_tahoe_config( + self.basedir, + u"[magic_folder]\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 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. + self.assertEqual( + perm, + stat.S_IMODE(os.stat(self.local_dir).st_mode), + ) + + 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 +265,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) - self.assertIn( - "there is no directory at that location", - str(ctx.exception) + magic_folder.load_magic_folders(self.basedir) + + self.assertTrue( + isdir(expected), + "magic-folder local directory {} was not created".format( + expected, + ), ) def test_load_legacy_not_a_dir(self): @@ -226,10 +300,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) ) From a7218cb16ffae4e4d039f67989f441b4aa461c53 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Mon, 23 Apr 2018 16:44:50 -0400 Subject: [PATCH 2/3] Some nice documnetation and proper exception types --- src/allmydata/frontends/magic_folder.py | 27 ++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/allmydata/frontends/magic_folder.py b/src/allmydata/frontends/magic_folder.py index d7c9cb743..70b45cbf9 100644 --- a/src/allmydata/frontends/magic_folder.py +++ b/src/allmydata/frontends/magic_folder.py @@ -43,7 +43,9 @@ IN_EXCL_UNLINK = 0x04000000L class ConfigurationError(Exception): - pass + """ + There was something wrong with some magic-folder configuration. + """ def get_inotify_module(): @@ -210,14 +212,33 @@ def load_magic_folders(node_directory): 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 Exception( + 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 Exception( + raise ConfigurationError( "Config for magic folder '{}' is missing '{}'".format( name, k ) From cec37466829288cb2cd7459f82f689d27b99e960 Mon Sep 17 00:00:00 2001 From: meejah Date: Tue, 1 May 2018 16:28:12 -0600 Subject: [PATCH 3/3] explicit umask and skip part of test on windows --- src/allmydata/test/test_magic_folder.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/allmydata/test/test_magic_folder.py b/src/allmydata/test/test_magic_folder.py index cd8e6844b..ef20dd390 100644 --- a/src/allmydata/test/test_magic_folder.py +++ b/src/allmydata/test/test_magic_folder.py @@ -34,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())) @@ -63,6 +66,9 @@ class NewConfigUtilTests(unittest.TestCase): # ..and the 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) @@ -99,10 +105,14 @@ class NewConfigUtilTests(unittest.TestCase): ), ) # It has permissions determined by the configured umask. - self.assertEqual( - perm, - stat.S_IMODE(os.stat(self.local_dir).st_mode), - ) + 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): """