Move webroot processing to webroot.py

letsencrypt/cli.py

@@ -2,7 +2,6 @@
 from __future__ import print_function
 import argparse
 import glob
-import json
 import logging
 import logging.handlers
 import os
@@ -270,12 +269,6 @@ class HelpfulArgumentParser(object):
 
         # Do any post-parsing homework here
 
-        # we get domains from -d, but also from the webroot map...
-        if parsed_args.webroot_map:
-            for domain in parsed_args.webroot_map.keys():
-                if domain not in parsed_args.domains:
-                    parsed_args.domains.append(domain)
-
         if parsed_args.staging or parsed_args.dry_run:
             if parsed_args.server not in (flag_default("server"), constants.STAGING_URI):
                 conflicts = ["--staging"] if parsed_args.staging else []
@@ -311,7 +304,7 @@ class HelpfulArgumentParser(object):
     def handle_csr(self, parsed_args):
         """
         Process a --csr flag. This needs to happen early enough that the
-        webroot plugin can know about the calls to process_domain
+        webroot plugin can know about the calls to add_domains.
         """
         if parsed_args.verb != "certonly":
             raise errors.Error("Currently, a CSR file may only be specified "
@@ -333,14 +326,9 @@ class HelpfulArgumentParser(object):
                 logger.debug("DER CSR parse error %s", e1)
                 logger.debug("PEM CSR parse error %s", traceback.format_exc())
                 raise errors.Error("Failed to parse CSR file: {0}".format(parsed_args.csr[0]))
-        for d in domains:
-            process_domain(parsed_args, d)
 
         for d in domains:
-            sanitised = le_util.enforce_domain_sanity(d)
-            if d.lower() != sanitised:
-                raise errors.ConfigurationError(
-                    "CSR domain {0} needs to be sanitised to {1}.".format(d, sanitised))
+            add_domains(parsed_args, d)
 
         if not domains:
             # TODO: add CN to domains instead:
@@ -572,7 +560,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):
     #for subparser in parser_run, parser_auth, parser_install:
     #    subparser.add_argument("domains", nargs="*", metavar="domain")
     helpful.add(None, "-d", "--domains", "--domain", dest="domains",
-                metavar="DOMAIN", action=DomainFlagProcessor, default=[],
+                metavar="DOMAIN", action=_DomainsAction, default=[],
                 help="Domain names to apply. For multiple domains you can use "
                 "multiple -d flags or enter a comma separated list of domains "
                 "as a parameter.")
@@ -829,51 +817,13 @@ def _plugins_parsing(helpful, plugins):
 
     helpful.add_plugin_args(plugins)
 
-    # These would normally be a flag within the webroot plugin, but because
-    # they are parsed in conjunction with --domains, they live here for
-    # legibility. helpful.add_plugin_ags must be called first to add the
-    # "webroot" topic
-    helpful.add("webroot", "-w", "--webroot-path", default=[], action=WebrootPathProcessor,
-                help="public_html / webroot path. This can be specified multiple times to "
-                     "handle different domains; each domain will have the webroot path that"
-                     " preceded it.  For instance: `-w /var/www/example -d example.com -d "
-                     "www.example.com -w /var/www/thing -d thing.net -d m.thing.net`")
-    # --webroot-map still has some awkward properties, so it is undocumented
-    helpful.add("webroot", "--webroot-map", default={}, action=WebrootMapProcessor,
-                help="JSON dictionary mapping domains to webroot paths; this "
-                     "implies -d for each entry. You may need to escape this "
-                     "from your shell. E.g.: --webroot-map "
-                     """'{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}' """
-                     "This option is merged with, but takes precedence over, "
-                     "-w / -d entries. At present, if you put webroot-map in "
-                     "a config file, it needs to be on a single line, like: "
-                     'webroot-map = {"example.com":"/var/www"}.')
 
+class _DomainsAction(argparse.Action):
+    """Action class for parsing domains."""
 
-class WebrootPathProcessor(argparse.Action):  # pylint: disable=missing-docstring
-    def __init__(self, *args, **kwargs):
-        self.domain_before_webroot = False
-        argparse.Action.__init__(self, *args, **kwargs)
-
-    def __call__(self, parser, args, webroot, option_string=None):
-        """
-        Keep a record of --webroot-path / -w flags during processing, so that
-        we know which apply to which -d flags
-        """
-        if not args.webroot_path:      # first -w flag encountered
-            # if any --domain flags preceded the first --webroot-path flag,
-            # apply that webroot path to those; subsequent entries in
-            # args.webroot_map are filled in by cli.DomainFlagProcessor
-            if args.domains:
-                self.domain_before_webroot = True
-                for d in args.domains:
-                    args.webroot_map.setdefault(d, webroot)
-        elif self.domain_before_webroot:
-            # FIXME if you set domains in a args file, you should get a different error
-            # here, pointing you to --webroot-map
-            raise errors.Error("If you specify multiple webroot paths, one of "
-                               "them must precede all domain flags")
-        args.webroot_path.append(webroot)
+    def __call__(self, parser, namespace, domain, option_string=None):
+        """Just wrap add_domains in argparseese."""
+        add_domains(namespace, domain)
 
 
 def add_domains(args_or_config, domains):
@@ -899,38 +849,3 @@ def add_domains(args_or_config, domains):
             args_or_config.domains.append(domain)
 
     return validated_domains
-
-
-def process_domain(args_or_config, domain_arg, webroot_path=None):
-    """
-    Process a new -d flag, helping the webroot plugin construct a map of
-    {domain : webrootpath} if -w / --webroot-path is in use
-
-    :param args_or_config: may be an argparse args object, or a NamespaceConfig object
-    :param str domain_arg: a string representing 1+ domains, eg: "eg.is, example.com"
-    :param str webroot_path: (optional) the webroot_path for these domains
-
-    """
-    webroot_path = webroot_path if webroot_path else args_or_config.webroot_path
-
-    for domain in (d.strip() for d in domain_arg.split(",")):
-        domain = le_util.enforce_domain_sanity(domain)
-        if domain not in args_or_config.domains:
-            args_or_config.domains.append(domain)
-            # Each domain has a webroot_path of the most recent -w flag
-            # unless it was explicitly included in webroot_map
-            if webroot_path:
-                args_or_config.webroot_map.setdefault(domain, webroot_path[-1])
-
-
-class WebrootMapProcessor(argparse.Action):  # pylint: disable=missing-docstring
-    def __call__(self, parser, args, webroot_map_arg, option_string=None):
-        webroot_map = json.loads(webroot_map_arg)
-        for domains, webroot_path in six.iteritems(webroot_map):
-            process_domain(args, domains, [webroot_path])
-
-
-class DomainFlagProcessor(argparse.Action):  # pylint: disable=missing-docstring
-    def __call__(self, parser, args, domain_arg, option_string=None):
-        """Just wrap process_domain in argparseese."""
-        process_domain(args, domain_arg)

letsencrypt/main.py

@@ -288,14 +288,10 @@ def _find_duplicative_certs(config, domains):
 
 
 def _find_domains(config, installer):
-    if not config.domains:
-        domains = display_ops.choose_names(installer)
-        # record in config.domains (so that it can be serialised in renewal config files),
-        # and set webroot_map entries if applicable
-        for d in domains:
-            cli.process_domain(config, d)
-    else:
+    if config.domains:
         domains = config.domains
+    else:
+        domains = display_ops.choose_names(installer)
 
     if not domains:
         raise errors.Error("Please specify --domains, or --installer that "

letsencrypt/plugins/webroot.py

@@ -38,9 +38,21 @@ to serve all files under specified web root ({0})."""
 
     @classmethod
     def add_parser_arguments(cls, add):
-        # --webroot-path and --webroot-map are added in cli.py because they
-        # are parsed in conjunction with --domains
-        pass
+        add("path", "-w", default=[], action=_WebrootPathAction,
+            help="public_html / webroot path. This can be specified multiple "
+                 "times to handle different domains; each domain will have "
+                 "the webroot path that preceded it.  For instance: `-w "
+                 "/var/www/example -d example.com -d www.example.com -w "
+                 "/var/www/thing -d thing.net -d m.thing.net`")
+        add("map", default={}, action=_WebrootMapAction,
+            help="JSON dictionary mapping domains to webroot paths; this "
+                 "implies -d for each entry. You may need to escape this from "
+                 "your shell. E.g.: --webroot-map "
+                 '\'{"eg1.is,m.eg1.is":"/www/eg1/", "eg2.is":"/www/eg2"}\' '
+                 "This option is merged with, but takes precedence over, -w / "
+                 "-d entries. At present, if you put webroot-map in a config "
+                 "file, it needs to be on a single line, like: webroot-map = "
+                 '{"example.com":"/var/www"}.')
 
     def get_chall_pref(self, domain):  # pragma: no cover
         # pylint: disable=missing-docstring,no-self-use,unused-argument
@@ -52,8 +64,10 @@ to serve all files under specified web root ({0})."""
         self.performed = defaultdict(set)
 
     def prepare(self):  # pylint: disable=missing-docstring
-        path_map = self.conf("map")
+        if self.conf("path"):
+            _match_webroot_with_domains(self.config)
 
+        path_map = self.conf("map")
         if not path_map:
             raise errors.PluginError(
                 "Missing parts of webroot configuration; please set either "
@@ -173,6 +187,7 @@ class _WebrootMapAction(argparse.Action):
 
 class _WebrootPathAction(argparse.Action):
     """Action class for parsing webroot_path."""
+
     def __init__(self, *args, **kwargs):
         super(_WebrootPathAction, self).__init__(*args, **kwargs)
         self._domain_before_webroot = False
@@ -218,4 +233,4 @@ def _match_webroot_with_domains(args_or_config):
     """
     webroot_path = args_or_config.webroot_path[-1]
     for domain in args_or_config.domains:
-        args_or_config.webroot_map.set_default(domain, webroot_path)
+        args_or_config.webroot_map.setdefault(domain, webroot_path)

letsencrypt/plugins/webroot_test.py

@@ -53,7 +53,7 @@ class AuthenticatorTest(unittest.TestCase):
     def test_add_parser_arguments(self):
         add = mock.MagicMock()
         self.auth.add_parser_arguments(add)
-        self.assertEqual(0, add.call_count)  # args moved to cli.py!
+        self.assertEqual(2, add.call_count)
 
     def test_prepare_missing_root(self):
         self.config.webroot_path = None

letsencrypt/renewal.py

@@ -12,6 +12,7 @@ import zope.component
 from letsencrypt import configuration
 from letsencrypt import cli
 from letsencrypt import errors
+from letsencrypt import le_util
 from letsencrypt import storage
 from letsencrypt.plugins import disco as plugins_disco
 
@@ -78,8 +79,8 @@ def _reconstitute(config, full_path):
         return None
 
     try:
-        for d in renewal_candidate.names():
-            cli.process_domain(config, d)
+        config.domains = [le_util.enforce_domain_sanity(d)
+                          for d in renewal_candidate.names()]
     except errors.ConfigurationError as error:
         logger.warning("Renewal configuration file %s references a cert "
                        "that contains an invalid domain name. The problem "

letsencrypt/tests/cli_test.py

@@ -459,60 +459,6 @@ class CLITest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         conflicts += ['--staging']
         self._check_server_conflict_message(short_args, conflicts)
 
-    def _webroot_map_test(self, map_arg, path_arg, domains_arg, # pylint: disable=too-many-arguments
-                          expected_map, expectect_domains, extra_args=None):
-        parse = self._get_argument_parser()
-        webroot_map_args = extra_args if extra_args else []
-        if map_arg:
-            webroot_map_args.extend(["--webroot-map", map_arg])
-        if path_arg:
-            webroot_map_args.extend(["-w", path_arg])
-        if domains_arg:
-            webroot_map_args.extend(["-d", domains_arg])
-        namespace = parse(webroot_map_args)
-        domains = main._find_domains(namespace, mock.MagicMock()) # pylint: disable=protected-access
-        self.assertEqual(namespace.webroot_map, expected_map)
-        self.assertEqual(set(domains), set(expectect_domains))
-
-    def test_parse_webroot(self):
-        parse = self._get_argument_parser()
-        webroot_args = ['--webroot', '-w', '/var/www/example',
-            '-d', 'example.com,www.example.com', '-w', '/var/www/superfluous',
-            '-d', 'superfluo.us', '-d', 'www.superfluo.us']
-        namespace = parse(webroot_args)
-        self.assertEqual(namespace.webroot_map, {
-            'example.com': '/var/www/example',
-            'www.example.com': '/var/www/example',
-            'www.superfluo.us': '/var/www/superfluous',
-            'superfluo.us': '/var/www/superfluous'})
-
-        webroot_args = ['-d', 'stray.example.com'] + webroot_args
-        self.assertRaises(errors.Error, parse, webroot_args)
-
-        simple_map = '{"eg.com" : "/tmp"}'
-        expected_map = {"eg.com": "/tmp"}
-        self._webroot_map_test(simple_map, None, None, expected_map, ["eg.com"])
-
-        # test merging webroot maps from the cli and a webroot map
-        expected_map["eg2.com"] = "/tmp2"
-        domains = ["eg.com", "eg2.com"]
-        self._webroot_map_test(simple_map, "/tmp2", "eg2.com,eg.com", expected_map, domains)
-
-        # test inclusion of interactively specified domains in the webroot map
-        with mock.patch('letsencrypt.display.ops.choose_names') as mock_choose:
-            mock_choose.return_value = domains
-            expected_map["eg2.com"] = "/tmp"
-            self._webroot_map_test(None, "/tmp", None, expected_map, domains)
-
-        extra_args = ['-c', test_util.vector_path('webrootconftest.ini')]
-        self._webroot_map_test(None, None, None, expected_map, domains, extra_args)
-
-        webroot_map_args = ['--webroot-map',
-                            '{"eg.com.,www.eg.com": "/tmp", "eg.is.": "/tmp2"}']
-        namespace = parse(webroot_map_args)
-        self.assertEqual(namespace.webroot_map,
-                         {"eg.com": "/tmp", "www.eg.com": "/tmp", "eg.is": "/tmp2"})
-
     def _certonly_new_request_common(self, mock_client, args=None):
         with mock.patch('letsencrypt.main._treat_as_renewal') as mock_renewal:
             mock_renewal.return_value = ("newcert", None)

letsencrypt/tests/client_test.py

@@ -135,8 +135,8 @@ class ClientTest(unittest.TestCase):
 
     # FIXME move parts of this to test_cli.py...
     @mock.patch("letsencrypt.client.logger")
-    @mock.patch("letsencrypt.cli.process_domain")
-    def test_obtain_certificate_from_csr(self, mock_process_domain, mock_logger):
+    @mock.patch("letsencrypt.cli.add_domains")
+    def test_obtain_certificate_from_csr(self, mock_add_domains, mock_logger):
         self._mock_obtain_certificate()
         from letsencrypt import cli
         test_csr = le_util.CSR(form="der", file=None, data=CSR_SAN)
@@ -151,7 +151,7 @@ class ClientTest(unittest.TestCase):
             cli.HelpfulArgumentParser.handle_csr(mock_parser, mock_parsed_args)
 
             # make sure cli processing occurred
-            cli_processed = (call[0][1] for call in mock_process_domain.call_args_list)
+            cli_processed = (call[0][1] for call in mock_add_domains.call_args_list)
             self.assertEqual(set(cli_processed), set(("example.com", "www.example.com")))
             # Now provoke an inconsistent domains error...
             mock_parsed_args.domains.append("hippopotamus.io")