From 19de05c72fdb3f2825f2f2b7074f5d5d1010c536 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 6 Nov 2019 20:33:24 +0200 Subject: [PATCH] [Apache v2] Implement set_parameters() (#7461) * find_comments implementation and AugeasCommentNode creation * set_parameters implementation * Change parameters to a property * Remove parameters property setter * More pythonic iteration handling --- certbot-apache/certbot_apache/augeasparser.py | 60 +++++++---- certbot-apache/certbot_apache/configurator.py | 5 +- certbot-apache/certbot_apache/parser.py | 6 ++ .../certbot_apache/tests/augeasnode_test.py | 58 ++++++++++ .../certbot_apache/tests/dualnode_test.py | 101 +++++++++++------- 5 files changed, 169 insertions(+), 61 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 04286ef4d..5f719f9a3 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -52,8 +52,9 @@ class AugeasDirectiveNode(AugeasParserNode): name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs) super(AugeasDirectiveNode, self).__init__(**kwargs) self.name = name - self.parameters = parameters self.enabled = enabled + if parameters: + self.set_parameters(parameters) def __eq__(self, other): if isinstance(other, self.__class__): @@ -67,8 +68,39 @@ class AugeasDirectiveNode(AugeasParserNode): return False def set_parameters(self, parameters): - """Sets the parameters for DirectiveNode""" - self.parameters = parameters + """ + Sets parameters of a DirectiveNode or BlockNode object. + + :param list parameters: List of all parameters for the node to set. + """ + orig_params = self._aug_get_params(self.metadata["augeaspath"]) + + # Clear out old parameters + for _ in orig_params: + # When the first parameter is removed, the indices get updated + param_path = "{}/arg[1]".format(self.metadata["augeaspath"]) + self.parser.aug.remove(param_path) + # Insert new ones + for pi, param in enumerate(parameters): + param_path = "{}/arg[{}]".format(self.metadata["augeaspath"], pi+1) + self.parser.aug.set(param_path, param) + + @property + def parameters(self): + """ + Fetches the parameters from Augeas tree, ensuring that the sequence always + represents the current state + + :returns: Tuple of parameters for this DirectiveNode + :rtype: tuple: + """ + return tuple(self._aug_get_params(self.metadata["augeaspath"])) + + def _aug_get_params(self, path): + """Helper function to get parameters for DirectiveNodes and BlockNodes""" + + arg_paths = self.parser.aug.match(path + "/arg") + return [self.parser.get_arg(apath) for apath in arg_paths] class AugeasBlockNode(AugeasDirectiveNode): @@ -90,9 +122,10 @@ class AugeasBlockNode(AugeasDirectiveNode): self.metadata == other.metadata) return False - def add_child_block(self, name, parameters=None, position=None): # pylint: disable=unused-argument + # pylint: disable=unused-argument + def add_child_block(self, name, parameters=None, position=None): # pragma: no cover """Adds a new BlockNode to the sequence of children""" - new_metadata = {"augeasparser": self.parser} + new_metadata = {"augeasparser": self.parser, "augeaspath": assertions.PASS} new_block = AugeasBlockNode(name=assertions.PASS, ancestor=self, filepath=assertions.PASS, @@ -100,9 +133,10 @@ class AugeasBlockNode(AugeasDirectiveNode): self.children += (new_block,) return new_block - def add_child_directive(self, name, parameters=None, position=None): # pylint: disable=unused-argument + # pylint: disable=unused-argument + def add_child_directive(self, name, parameters=None, position=None): # pragma: no cover """Adds a new DirectiveNode to the sequence of children""" - new_metadata = {"augeasparser": self.parser} + new_metadata = {"augeasparser": self.parser, "augeaspath": assertions.PASS} new_dir = AugeasDirectiveNode(name=assertions.PASS, ancestor=self, filepath=assertions.PASS, @@ -112,7 +146,7 @@ class AugeasBlockNode(AugeasDirectiveNode): def add_child_comment(self, comment="", position=None): # pylint: disable=unused-argument """Adds a new CommentNode to the sequence of children""" - new_metadata = {"augeasparser": self.parser} + new_metadata = {"augeasparser": self.parser, "augeaspath": assertions.PASS} new_comment = AugeasCommentNode(comment=assertions.PASS, ancestor=self, filepath=assertions.PASS, @@ -192,13 +226,11 @@ class AugeasBlockNode(AugeasDirectiveNode): """Helper function to create a DirectiveNode from Augeas path""" name = self.parser.get_arg(path) - params = tuple(self._aug_get_params(path)) metadata = {"augeasparser": self.parser, "augeaspath": path} # Because of the dynamic nature, and the fact that we're not populating # the complete ParserNode tree, we use the search parent as ancestor return AugeasDirectiveNode(name=name, - parameters=params, ancestor=assertions.PASS, filepath=apache_util.get_file_path(path), metadata=metadata) @@ -207,13 +239,11 @@ class AugeasBlockNode(AugeasDirectiveNode): """Helper function to create a BlockNode from Augeas path""" name = self._aug_get_block_name(path) - params = tuple(self._aug_get_params(path)) metadata = {"augeasparser": self.parser, "augeaspath": path} # Because of the dynamic nature, and the fact that we're not populating # the complete ParserNode tree, we use the search parent as ancestor return AugeasBlockNode(name=name, - parameters=params, ancestor=assertions.PASS, filepath=apache_util.get_file_path(path), metadata=metadata) @@ -232,12 +262,6 @@ class AugeasBlockNode(AugeasDirectiveNode): name.lower() in os.path.basename(path).lower()]) return blk_paths - def _aug_get_params(self, path): - """Helper function to get parameters for BlockNodes""" - - arg_paths = self.parser.aug.match(path + "/arg") - return [self.parser.get_arg(apath) for apath in arg_paths] - def _aug_get_block_name(self, path): """Helper function to get name of a configuration block from path.""" diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index d01b0f5ce..808748006 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -257,7 +257,8 @@ class ApacheConfigurator(common.Installer): self.parser = self.get_parser() # Set up ParserNode root - pn_meta = {"augeasparser": self.parser} + pn_meta = {"augeasparser": self.parser, + "augeaspath": self.parser.get_root_augpath()} self.parser_root = self.get_parsernode_root(pn_meta) # Check for errors in parsing files with Augeas @@ -360,7 +361,7 @@ class ApacheConfigurator(common.Installer): return dualparser.DualBlockNode( name=assertions.PASS, ancestor=None, - filepath=assertions.PASS, + filepath=self.parser.loc["root"], metadata=metadata ) diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index 7ae9ac386..906b97e72 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -680,6 +680,12 @@ class ApacheParser(object): return value + def get_root_augpath(self): + """ + Returns the Augeas path of root configuration. + """ + return get_aug_path(self.loc["root"]) + def exclude_dirs(self, matches): """Exclude directives that are not loaded into the configuration.""" filters = [("ifmodule", self.modules), ("ifdefine", self.variables)] diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index acb2d5329..bbf9093d3 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -1,6 +1,8 @@ """Tests for AugeasParserNode classes""" import mock +from acme.magic_typing import List # pylint: disable=unused-import, no-name-in-module + from certbot_apache import assertions from certbot_apache.tests import util @@ -72,3 +74,59 @@ class AugeasParserNodeTest(util.ApacheTest): self.assertTrue(rootcomment[0].filepath.endswith( "debian_apache_2_4/multiple_vhosts/apache2/apache2.conf" )) + + def test_set_parameters(self): + servernames = self.config.parser_root.find_directives("servername") + names = [] # type: List[str] + for servername in servernames: + names += servername.parameters + self.assertFalse("going_to_set_this" in names) + servernames[0].set_parameters(["something", "going_to_set_this"]) + servernames = self.config.parser_root.find_directives("servername") + names = [] + for servername in servernames: + names += servername.parameters + self.assertTrue("going_to_set_this" in names) + + def test_set_parameters_atinit(self): + from certbot_apache.augeasparser import AugeasDirectiveNode + servernames = self.config.parser_root.find_directives("servername") + setparam = "certbot_apache.augeasparser.AugeasDirectiveNode.set_parameters" + with mock.patch(setparam) as mock_set: + AugeasDirectiveNode( + name=servernames[0].name, + parameters=["test", "setting", "these"], + ancestor=assertions.PASS, + metadata=servernames[0].metadata + ) + self.assertTrue(mock_set.called) + self.assertEqual( + mock_set.call_args_list[0][0][0], + ["test", "setting", "these"] + ) + + def test_set_parameters_delete(self): + # Set params + servername = self.config.parser_root.find_directives("servername")[0] + servername.set_parameters(["thisshouldnotexistpreviously", "another", + "third"]) + + # Delete params + servernames = self.config.parser_root.find_directives("servername") + found = False + for servername in servernames: + if "thisshouldnotexistpreviously" in servername.parameters: + self.assertEqual(len(servername.parameters), 3) + servername.set_parameters(["thisshouldnotexistpreviously"]) + found = True + self.assertTrue(found) + + # Verify params + servernames = self.config.parser_root.find_directives("servername") + found = False + for servername in servernames: + if "thisshouldnotexistpreviously" in servername.parameters: + self.assertEqual(len(servername.parameters), 1) + servername.set_parameters(["thisshouldnotexistpreviously"]) + found = True + self.assertTrue(found) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index 3101a9c6f..5d6f827e7 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -13,23 +13,26 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- """DualParserNode tests""" def setUp(self): # pylint: disable=arguments-differ - metadata = {"augeasparser": mock.Mock()} + parser_mock = mock.MagicMock() + parser_mock.aug.match.return_value = [] + parser_mock.get_arg.return_value = [] + self.metadata = {"augeasparser": parser_mock, "augeaspath": "/invalid"} self.block = dualparser.DualBlockNode(name="block", ancestor=None, filepath="/tmp/something", - metadata=metadata) + metadata=self.metadata) self.block_two = dualparser.DualBlockNode(name="block", ancestor=self.block, filepath="/tmp/something", - metadata=metadata) + metadata=self.metadata) self.directive = dualparser.DualDirectiveNode(name="directive", ancestor=self.block, filepath="/tmp/something", - metadata=metadata) + metadata=self.metadata) self.comment = dualparser.DualCommentNode(comment="comment", ancestor=self.block, filepath="/tmp/something", - metadata=metadata) + metadata=self.metadata) def test_create_with_precreated(self): cnode = dualparser.DualCommentNode(comment="comment", @@ -57,9 +60,11 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_set_params(self): params = ("first", "second") + self.directive.primary.set_parameters = mock.Mock() + self.directive.secondary.set_parameters = mock.Mock() self.directive.set_parameters(params) - self.assertEqual(self.directive.primary.parameters, params) - self.assertEqual(self.directive.secondary.parameters, params) + self.assertTrue(self.directive.primary.set_parameters.called) + self.assertTrue(self.directive.secondary.set_parameters.called) def test_set_parameters(self): pparams = mock.MagicMock() @@ -128,26 +133,22 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- assertions.assertEqual(self.comment.primary, self.comment.secondary) def test_add_child_block(self): - self.assertEqual(len(self.block.primary.children), 0) - self.assertEqual(len(self.block.secondary.children), 0) + mock_first = mock.MagicMock(return_value=self.block.primary) + mock_second = mock.MagicMock(return_value=self.block.secondary) + self.block.primary.add_child_block = mock_first + self.block.secondary.add_child_block = mock_second self.block.add_child_block("Block") - self.assertEqual(len(self.block.primary.children), 1) - self.assertEqual(len(self.block.secondary.children), 1) - self.assertTrue(isinstance(self.block.primary.children[0], - interfaces.BlockNode)) - self.assertEqual(self.block.primary.children[0].ancestor, - self.block.primary) + self.assertTrue(mock_first.called) + self.assertTrue(mock_second.called) def test_add_child_directive(self): - self.assertEqual(len(self.block.primary.children), 0) - self.assertEqual(len(self.block.secondary.children), 0) + mock_first = mock.MagicMock(return_value=self.directive.primary) + mock_second = mock.MagicMock(return_value=self.directive.secondary) + self.block.primary.add_child_directive = mock_first + self.block.secondary.add_child_directive = mock_second self.block.add_child_directive("Directive") - self.assertEqual(len(self.block.primary.children), 1) - self.assertEqual(len(self.block.secondary.children), 1) - self.assertTrue(isinstance(self.block.primary.children[0], - interfaces.DirectiveNode)) - self.assertEqual(self.block.primary.children[0].ancestor, - self.block.primary) + self.assertTrue(mock_first.called) + self.assertTrue(mock_second.called) def test_add_child_comment(self): self.assertEqual(len(self.block.primary.children), 0) @@ -187,10 +188,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_blocks_first_passing(self): youshallnotpass = [augeasparser.AugeasBlockNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] youshallpass = [augeasparser.AugeasBlockNode(name=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_blocks_primary = mock.MagicMock(return_value=youshallpass) find_blocks_secondary = mock.MagicMock(return_value=youshallnotpass) self.block.primary.find_blocks = find_blocks_primary @@ -208,10 +211,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_blocks_second_passing(self): youshallnotpass = [augeasparser.AugeasBlockNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] youshallpass = [augeasparser.AugeasBlockNode(name=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_blocks_primary = mock.MagicMock(return_value=youshallnotpass) find_blocks_secondary = mock.MagicMock(return_value=youshallpass) self.block.primary.find_blocks = find_blocks_primary @@ -229,10 +234,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_dirs_first_passing(self): notpassing = [augeasparser.AugeasDirectiveNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] passing = [augeasparser.AugeasDirectiveNode(name=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_dirs_primary = mock.MagicMock(return_value=passing) find_dirs_secondary = mock.MagicMock(return_value=notpassing) self.block.primary.find_directives = find_dirs_primary @@ -250,10 +257,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_dirs_second_passing(self): notpassing = [augeasparser.AugeasDirectiveNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] passing = [augeasparser.AugeasDirectiveNode(name=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_dirs_primary = mock.MagicMock(return_value=notpassing) find_dirs_secondary = mock.MagicMock(return_value=passing) self.block.primary.find_directives = find_dirs_primary @@ -271,10 +280,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_coms_first_passing(self): notpassing = [augeasparser.AugeasCommentNode(comment="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] passing = [augeasparser.AugeasCommentNode(comment=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_coms_primary = mock.MagicMock(return_value=passing) find_coms_secondary = mock.MagicMock(return_value=notpassing) self.block.primary.find_comments = find_coms_primary @@ -313,10 +324,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_blocks_no_pass_equal(self): notpassing1 = [augeasparser.AugeasBlockNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] notpassing2 = [augeasparser.AugeasBlockNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] find_blocks_primary = mock.MagicMock(return_value=notpassing1) find_blocks_secondary = mock.MagicMock(return_value=notpassing2) self.block.primary.find_blocks = find_blocks_primary @@ -330,10 +343,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_dirs_no_pass_equal(self): notpassing1 = [augeasparser.AugeasDirectiveNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] notpassing2 = [augeasparser.AugeasDirectiveNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] find_dirs_primary = mock.MagicMock(return_value=notpassing1) find_dirs_secondary = mock.MagicMock(return_value=notpassing2) self.block.primary.find_directives = find_dirs_primary @@ -347,10 +362,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_comments_no_pass_equal(self): notpassing1 = [augeasparser.AugeasCommentNode(comment="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] notpassing2 = [augeasparser.AugeasCommentNode(comment="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] find_coms_primary = mock.MagicMock(return_value=notpassing1) find_coms_secondary = mock.MagicMock(return_value=notpassing2) self.block.primary.find_comments = find_coms_primary @@ -364,10 +381,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_blocks_no_pass_notequal(self): notpassing1 = [augeasparser.AugeasBlockNode(name="notpassing", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] notpassing2 = [augeasparser.AugeasBlockNode(name="different", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] find_blocks_primary = mock.MagicMock(return_value=notpassing1) find_blocks_secondary = mock.MagicMock(return_value=notpassing2) self.block.primary.find_blocks = find_blocks_primary