Thoroughly document procedures and clean-up TODOs

certbot-apache/certbot_apache/_internal/apacheparser.py

@@ -1,5 +1,11 @@
 """ apacheconfig implementation of the ParserNode interfaces """
 
+import glob
+
+from acme.magic_typing import Optional  # pylint: disable=unused-import, no-name-in-module
+
+from certbot.compat import os
+
 from certbot_apache._internal import assertions
 from certbot_apache._internal import interfaces
 from certbot_apache._internal import parsernode_util as util
@@ -51,6 +57,17 @@ class ApacheCommentNode(ApacheParserNode):
         return False
 
 
+def _load_included_file(filename, metadata):
+    with open(filename) as f:
+        ast = metadata['loader'].loads(f.read())
+    metadata = metadata.copy()
+    metadata['ac_ast'] = ast
+    return ApacheBlockNode(name=assertions.PASS,
+                           ancestor=None,
+                           filepath=filename,
+                           metadata=metadata)
+
+
 class ApacheDirectiveNode(ApacheParserNode):
     """ apacheconfig implementation of DirectiveNode interface """
 
@@ -62,6 +79,24 @@ class ApacheDirectiveNode(ApacheParserNode):
         self.enabled = enabled
         self.include = None
 
+        # LoadModule processing
+        if self.name and self.name.lower() in ["loadmodule"]:
+            mod_name, mod_filename = self.parameters
+            self.metadata["apache_vars"]["modules"].add(mod_name)
+            self.metadata["apache_vars"]["modules"].add(
+                os.path.basename(mod_filename)[:-2] + "c")
+
+        # Include processing
+        if self.name and self.name.lower() in ["include", "includeoptional"]:
+            value = self.parameters[0]
+            path = os.path.join(self.metadata['serverroot'], value)
+            filepaths = glob.glob(path)
+            for filepath in filepaths:
+                if filepath not in self.metadata['parsed_files']:
+                    node = _load_included_file(filepath, self.metadata)
+                    self.metadata['parsed_files'][filepath] = node
+            self.include = set(filepaths)
+
     def __eq__(self, other):  # pragma: no cover
         if isinstance(other, self.__class__):
             return (self.name == other.name and
@@ -75,7 +110,58 @@ class ApacheDirectiveNode(ApacheParserNode):
 
     def set_parameters(self, _parameters):
         """Sets the parameters for DirectiveNode"""
-        return
+        self.parameters = tuple(_parameters)
+        self._raw.value = tuple(" ".join(_parameters))
+
+
+def _recursive_generator(node, exclude=True, files_visited=None):
+    """Recursive generator over all children of given block node, expanding includes.
+
+    :param ApacheBlockNode node: node whose children will be yielded by this generator
+    :param bool exclude: If True, excludes nodes disabled by conditional blocks like
+        IfDefine and IfModule.
+    :param dict files_visited: bookkeeping dict for recursion to ensure we don't visit
+        the same file twice (to avoid double-counting nodes)
+    """
+    if not files_visited:
+        files_visited = set([node.filepath])
+    for child in node.children:
+        yield child
+        if isinstance(child, ApacheBlockNode):
+            if not exclude or child.enabled:
+                for subchild in _recursive_generator(child, exclude, files_visited):
+                    yield subchild
+        if isinstance(child, ApacheDirectiveNode) and child.include:
+            for filename in child.include:
+                if filename not in files_visited:
+                    files_visited.add(filename)
+                    file_ast = node.metadata['parsed_files'][filename]
+                    for subchild in _recursive_generator(file_ast, exclude, files_visited):
+                        yield subchild
+
+
+def _is_enabled(block_node, apache_vars):
+    """Returns False if this block disables its children given loaded Apache data.
+
+    Checks to see whether this block_node is a conditional IfDefine or IfModule,
+    and returns what its argument evaluates to.
+
+    :param ApacheBlockNode block_node: block node to check.
+    :param dict apache_vars: dict that includes set of loaded modules and variables, under keys
+        "modules" and "defines", respectively.
+    """
+    filters = {
+        "ifdefine": apache_vars["defines"],
+        "ifmodule": apache_vars["modules"]
+    }
+    if not block_node.name or block_node.name.lower() not in filters:
+        return True
+    loaded_set = filters[block_node.name.lower()]
+    name = block_node.parameters[0]
+    expect_loaded = not name.startswith("!")
+    name = name.lstrip("!")
+    loaded = (name in loaded_set)
+    return expect_loaded == loaded
 
 
 class ApacheBlockNode(ApacheDirectiveNode):
@@ -83,7 +169,32 @@ class ApacheBlockNode(ApacheDirectiveNode):
 
     def __init__(self, **kwargs):
         super(ApacheBlockNode, self).__init__(**kwargs)
-        self.children = ()
+        self._raw_children = self._raw
+        children = []
+
+        self.enabled = self.enabled and _is_enabled(self, self.metadata["apache_vars"])
+        for raw_node in self._raw_children:
+            node = None  # type: Optional[ApacheParserNode]
+            metadata = self.metadata.copy()
+            metadata['ac_ast'] = raw_node
+            if raw_node.typestring == "comment":
+                node = ApacheCommentNode(comment=raw_node.name[2:],
+                                         metadata=metadata, ancestor=self,
+                                         filepath=self.filepath)
+            elif raw_node.typestring == "block":
+                parameters = util.parameters_from_string(raw_node.arguments)
+                node = ApacheBlockNode(name=raw_node.tag, parameters=parameters,
+                                       metadata=metadata, ancestor=self,
+                                       filepath=self.filepath, enabled=self.enabled)
+            else:
+                parameters = ()
+                if raw_node.value:
+                    parameters = util.parameters_from_string(raw_node.value)
+                node = ApacheDirectiveNode(name=raw_node.name, parameters=parameters,
+                                           metadata=metadata, ancestor=self,
+                                           filepath=self.filepath, enabled=self.enabled)
+            children.append(node)
+        self.children = tuple(children)
 
     def __eq__(self, other):  # pragma: no cover
         if isinstance(other, self.__class__):
@@ -130,26 +241,27 @@ class ApacheBlockNode(ApacheDirectiveNode):
 
     def find_blocks(self, name, exclude=True): # pylint: disable=unused-argument
         """Recursive search of BlockNodes from the sequence of children"""
-        return [ApacheBlockNode(name=assertions.PASS,
+        blocks = []
-                                parameters=assertions.PASS,
+        for child in _recursive_generator(self, exclude=exclude):
-                                ancestor=self,
+            if isinstance(child, ApacheBlockNode) and child.name.lower() == name.lower():
-                                filepath=assertions.PASS,
+                blocks.append(child)
-                                metadata=self.metadata)]
+        return blocks
 
     def find_directives(self, name, exclude=True): # pylint: disable=unused-argument
         """Recursive search of DirectiveNodes from the sequence of children"""
-        return [ApacheDirectiveNode(name=assertions.PASS,
+        directives = []
-                                    parameters=assertions.PASS,
+        for child in _recursive_generator(self, exclude=exclude):
-                                    ancestor=self,
+            if isinstance(child, ApacheDirectiveNode) and child.name.lower() == name.lower():
-                                    filepath=assertions.PASS,
+                directives.append(child)
-                                    metadata=self.metadata)]
+        return directives
 
-    def find_comments(self, comment, exact=False): # pylint: disable=unused-argument
+    def find_comments(self, comment): # pylint: disable=unused-argument
         """Recursive search of DirectiveNodes from the sequence of children"""
-        return [ApacheCommentNode(comment=assertions.PASS,
+        comments = []
-                                  ancestor=self,
+        for child in _recursive_generator(self):
-                                  filepath=assertions.PASS,
+            if isinstance(child, ApacheCommentNode) and comment in child.comment:
-                                  metadata=self.metadata)]
+                comments.append(child)
+        return comments
 
     def delete_child(self, child):  # pragma: no cover
         """Deletes a ParserNode from the sequence of children"""

certbot-apache/certbot_apache/_internal/augeasparser.py

@@ -389,8 +389,8 @@ class AugeasBlockNode(AugeasDirectiveNode):
         exception if it's unable to do so.
         :param AugeasParserNode: child: A node to delete.
         """
-        if not self.parser.aug.remove(child.metadata["augeaspath"]):
 
+        if not self.parser.aug.remove(child.metadata["augeaspath"]):
             raise errors.PluginError(
                 ("Could not delete child node, the Augeas path: {} doesn't " +
                  "seem to exist.").format(child.metadata["augeaspath"])

certbot-apache/certbot_apache/_internal/configurator.py

@@ -12,6 +12,11 @@ import pkg_resources
 import six
 import zope.component
 import zope.interface
+try:
+    import apacheconfig
+    HAS_APACHECONFIG = True
+except ImportError:  # pragma: no cover
+    HAS_APACHECONFIG = False
 
 from acme import challenges
 from acme.magic_typing import DefaultDict
@@ -260,8 +265,9 @@ class ApacheConfigurator(common.Installer):
         # Set up ParserNode root
         pn_meta = {"augeasparser": self.parser,
                    "augeaspath": self.parser.get_root_augpath(),
+                   "serverroot": self.option("server_root"),
                    "ac_ast": None}
-        if self.USE_PARSERNODE:
+        if self.USE_PARSERNODE and HAS_APACHECONFIG:
             self.parser_root = self.get_parsernode_root(pn_meta)
             self.parsed_paths = self.parser_root.parsed_paths()
 
@@ -364,17 +370,24 @@ class ApacheConfigurator(common.Installer):
         """Initializes the ParserNode parser root instance."""
 
         apache_vars = dict()
-        apache_vars["defines"] = apache_util.parse_defines(self.option("ctl"))
+        apache_vars["defines"] = set(apache_util.parse_defines(self.option("ctl")))
-        apache_vars["includes"] = apache_util.parse_includes(self.option("ctl"))
+        apache_vars["includes"] = set(apache_util.parse_includes(self.option("ctl")))
-        apache_vars["modules"] = apache_util.parse_modules(self.option("ctl"))
+        apache_vars["modules"] = set(apache_util.parse_modules(self.option("ctl")))
         metadata["apache_vars"] = apache_vars
 
-        return dualparser.DualBlockNode(
+        with apacheconfig.make_loader(writable=True,
-            name=assertions.PASS,
+              **apacheconfig.flavors.NATIVE_APACHE) as loader:
-            ancestor=None,
+            with open(self.parser.loc["root"]) as f:
-            filepath=self.parser.loc["root"],
+                metadata["ac_ast"] = loader.loads(f.read())
-            metadata=metadata
+            metadata["loader"] = loader
-        )
+            metadata["parsed_files"] = {}
+
+            return dualparser.DualBlockNode(
+                name=assertions.PASS,
+                ancestor=None,
+                filepath=self.parser.loc["root"],
+                metadata=metadata
+            )
 
     def _wildcard_domain(self, domain):
         """
@@ -907,7 +920,7 @@ class ApacheConfigurator(common.Installer):
         """
 
         v1_vhosts = self.get_virtual_hosts_v1()
-        if self.USE_PARSERNODE:
+        if self.USE_PARSERNODE and HAS_APACHECONFIG:
             v2_vhosts = self.get_virtual_hosts_v2()
 
             for v1_vh in v1_vhosts:

certbot-apache/certbot_apache/_internal/parsernode_util.py

@@ -127,3 +127,45 @@ def directivenode_kwargs(kwargs):
     parameters = kwargs.pop("parameters")
     enabled = kwargs.pop("enabled")
     return name, parameters, enabled, kwargs
+
+
+def parameters_from_string(text):
+    """Transforms a whitespace-separated string of parameters into a tuple of strings.
+
+    Ignores all whitespace outside quotations (matched single quotes or double quotes)
+    e.g. "parameter1 'parameter two'" => ("parameter1", "parameter two")
+    Mirrors parsing code in apache/httpd.
+
+    ap_getword_conf procedure for retrieving next token from a line:
+    https://github.com/apache/httpd/blob/5515e790adba6414c35ac19f8b6ffa0d7fc0051d/server/util.c#L787
+
+    substring_conf procedure for retrieving text between quotes:
+    https://github.com/apache/httpd/blob/5515e790adba6414c35ac19f8b6ffa0d7fc0051d/server/util.c#L759
+
+    :param str text: whitespace-separated string of apache arguments
+
+    :returns Tuple of strings extracted as parameters from text
+    """
+    text = text.lstrip()
+    words = []
+    word = ""
+    escape = False
+    quote = None
+    for c in text:
+        if c.isspace() and not quote:
+            if word:
+                words.append(word)
+            word = ""
+        else:
+            word += c
+        if not quote and c in "\"\'":
+            quote = c
+        elif c == quote and not escape:
+            words.append(word[1:-1].replace("\\\\", "\\")
+                                   .replace("\\" + quote, quote))
+            word = ""
+            quote = None
+        escape = not escape and c == "\\"
+    if word:
+        words.append(word)
+    return tuple(words)

certbot-apache/setup.py

@@ -19,7 +19,7 @@ install_requires = [
 ]
 
 dev_extras = [
-    'apacheconfig>=0.3.1',
+    'apacheconfig>=0.3.2',
 ]
 
 class PyTest(TestCommand):

certbot-apache/tests/augeasnode_test.py

@@ -1,8 +1,15 @@
 """Tests for AugeasParserNode classes"""
 import mock
 
+import unittest
 import util
 
+try:
+    import apacheconfig  # pylint: disable=import-error,unused-import
+    HAS_APACHECONFIG = True
+except ImportError:  # pragma: no cover
+    HAS_APACHECONFIG = False
+
 from acme.magic_typing import List  # pylint: disable=unused-import, no-name-in-module
 from certbot import errors
 
@@ -10,6 +17,7 @@ from certbot_apache._internal import assertions
 
 
 
+@unittest.skipIf(not HAS_APACHECONFIG, reason='Tests require apacheconfig dependency')
 class AugeasParserNodeTest(util.ApacheTest):  # pylint: disable=too-many-public-methods
     """Test AugeasParserNode using available test configurations"""
 
@@ -147,11 +155,11 @@ class AugeasParserNodeTest(util.ApacheTest):  # pylint: disable=too-many-public-
 
     def test_add_child_comment(self):
         newc = self.config.parser_root.primary.add_child_comment("The content")
-        comments = self.config.parser_root.find_comments("The content")
+        comments = self.config.parser_root.primary.find_comments("The content")
         self.assertEqual(len(comments), 1)
         self.assertEqual(
             newc.metadata["augeaspath"],
-            comments[0].primary.metadata["augeaspath"]
+            comments[0].metadata["augeaspath"]
         )
         self.assertEqual(newc.comment, comments[0].comment)
 
@@ -280,11 +288,11 @@ class AugeasParserNodeTest(util.ApacheTest):  # pylint: disable=too-many-public-
             ["with", "parameters"],
             position=0
         )
-        dirs = self.config.parser_root.find_directives("ThisWasAdded")
+        dirs = self.config.parser_root.primary.find_directives("ThisWasAdded")
         self.assertEqual(len(dirs), 1)
         self.assertEqual(dirs[0].parameters, ("with", "parameters"))
         # The new directive was added to the very first line of the config
-        self.assertTrue(dirs[0].primary.metadata["augeaspath"].endswith("[1]"))
+        self.assertTrue(dirs[0].metadata["augeaspath"].endswith("[1]"))
 
     def test_add_child_directive_exception(self):
         self.assertRaises(

certbot-apache/tests/dualnode_test.py

@@ -15,7 +15,11 @@ class DualParserNodeTest(unittest.TestCase):  # pylint: disable=too-many-public-
         parser_mock = mock.MagicMock()
         parser_mock.aug.match.return_value = []
         parser_mock.get_arg.return_value = []
-        self.metadata = {"augeasparser": parser_mock, "augeaspath": "/invalid", "ac_ast": None}
+        ast_mock = mock.MagicMock()
+        ast_mock.__iter__.return_value = iter([])
+        self.metadata = {"augeasparser": parser_mock,
+                         "augeaspath": "/invalid", "ac_ast": ast_mock,
+                         "apache_vars": { "modules": set(), "defines": set(), "includes": set(), }}
         self.block = dualparser.DualBlockNode(name="block",
                                               ancestor=None,
                                               filepath="/tmp/something",

certbot-apache/tests/parsernode_configurator_test.py

@@ -5,7 +5,14 @@ import mock
 
 import util
 
+try:
+    import apacheconfig
+    HAS_APACHECONFIG = True
+except ImportError:  # pragma: no cover
+    HAS_APACHECONFIG = False
 
+
+@unittest.skipIf(not HAS_APACHECONFIG, reason='Tests require apacheconfig dependency')
 class ConfiguratorParserNodeTest(util.ApacheTest):  # pylint: disable=too-many-public-methods
     """Test AugeasParserNode using available test configurations"""
 

certbot-apache/tests/parsernode_util_test.py

@@ -110,6 +110,31 @@ class ParserNodeUtilTest(unittest.TestCase):
         p_params.pop("filepath")
         self.assertRaises(TypeError, util.parsernode_kwargs, p_params)
 
+    def test_parameters_from_string(self):
+        test_cases = [
+            # Whitespace between tokens is usually ignored
+            ("one two", ("one", "two")),
+            ("\t\none \n\ntwo \t", ("one", "two")),
+            # Quotes preserve whitespace
+            ("one 'param\ttwo'", ("one", "param\ttwo")),
+            ("\t  one \t\n ' param\t2\t'\n", ("one", " param\t2\t")),
+            # Empty/edge cases
+            ("one '' ", ("one", "")),
+            ("one", ("one",)),
+            ("\t\n ", ()),
+            ("", ()),
+            # End-quote can be escaped
+            ("one ' \\'two'", ("one", " 'two")),
+            ("one \" \\\"two\"", ("one", " \"two")),
+            # Escapes are escaped within quotes
+            ("one 'two\\\\ '", ("one", "two\\ ")),
+            # Unmatched quotations
+            ("one 'two ", ("one", "'two ")),
+            ("one 'two\" ", ("one", "'two\" ")),
+        ]
+        for text, expected in test_cases:
+            self.assertEqual(util.parameters_from_string(text), expected)
+
 
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover

tools/dev_constraints.txt

@@ -3,7 +3,7 @@
 # Some dev package versions specified here may be overridden by higher level constraints
 # files during tests (eg. letsencrypt-auto-source/pieces/dependency-requirements.txt).
 alabaster==0.7.10
-apacheconfig==0.3.1
+apacheconfig==0.3.2
 apipkg==1.4
 appnope==0.1.0
 asn1crypto==0.22.0

tools/oldest_constraints.txt

@@ -39,7 +39,7 @@ pytz==2012rc0
 google-api-python-client==1.5.5
 
 # Our setup.py constraints
-apacheconfig==0.3.1
+apacheconfig==0.3.2
 cloudflare==1.5.1
 cryptography==1.2.3
 parsedatetime==1.3

tox.ini

@@ -120,12 +120,14 @@ setenv =
 basepython = python2.7
 commands =
     {[base]install_packages}
+    {[base]pip_install} certbot-apache[dev]
     python tox.cover.py
 
 [testenv:py37-cover]
 basepython = python3.7
 commands =
     {[base]install_packages}
+    {[base]pip_install} certbot-apache[dev]
     python tox.cover.py
 
 [testenv:lint]