From 270754deffc4e8bd54ba1e2962e2332ee4980df5 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 13 Aug 2019 19:22:51 +0300 Subject: [PATCH 01/23] [Apache v2] New ParserNode interface abstraction (#7246) * New ParserNode interface abstraction * Add the test assertions, and fix interface * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Add dummy tests and change arguments to properties * Add more context to comment property docstring * Add documentation to the main docstring * Streamline the parameter naming * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Add explicit instructions how whitespaces are treated in set_parameters * Add the information about lookups being case insensitive. * Add context about whitespacing to add_ - methods * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren --- certbot-apache/certbot_apache/interfaces.py | 476 ++++++++++++++++++ .../certbot_apache/tests/parsernode_test.py | 86 ++++ 2 files changed, 562 insertions(+) create mode 100644 certbot-apache/certbot_apache/interfaces.py create mode 100644 certbot-apache/certbot_apache/tests/parsernode_test.py diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py new file mode 100644 index 000000000..a614f85c9 --- /dev/null +++ b/certbot-apache/certbot_apache/interfaces.py @@ -0,0 +1,476 @@ +"""ParserNode interface for interacting with configuration tree. + +General description +------------------- + +The ParserNode interfaces are designed to be able to contain all the parsing logic, +while allowing their users to interact with the configuration tree in a Pythonic +and well structured manner. + +The structure allows easy traversal of the tree of ParserNodes. Each ParserNode +stores a reference to its ancestor and immediate children, allowing the user to +traverse the tree using built in interface methods as well as accessing the interface +properties directly. + +ParserNode interface implementation should stand between the actual underlying +parser functionality and the business logic within Configurator code, interfacing +with both. The ParserNode tree is a result of configuration parsing action. + +ParserNode tree will be in charge of maintaining the parser state and hence the +abstract syntax tree (AST). Interactions between ParserNode tree and underlying +parser should involve only parsing the configuration files to this structure, and +writing it back to the filesystem - while preserving the format including whitespaces. + +For some implementations (Apache for example) it's important to keep track of and +to use state information while parsing conditional blocks and directives. This +allows the implementation to set a flag to parts of the parsed configuration +structure as not being in effect in a case of unmatched conditional block. It's +important to store these blocks in the tree as well in order to not to conduct +destructive actions (failing to write back parts of the configuration) while writing +the AST back to the filesystem. + +The ParserNode tree is in charge of maintaining the its own structure while every +child node fetched with find - methods or by iterating its list of children can be +changed in place. When making changes the affected nodes should be flagged as "dirty" +in order for the parser implementation to figure out the parts of the configuration +that need to be written back to disk during the save() operation. + + +Metadata +-------- + +The metadata holds all the implementation specific attributes of the ParserNodes - +things like the positional information related to the AST, file paths, whitespacing, +and any other information relevant to the underlying parser engine. + +Access to the metadata should be handled by implementation specific methods, allowing +the Configurator functionality to access the underlying information where needed. +A good example of this is file path of a node - something that is needed by the +reverter functionality within the Configurator. + + +Apache implementation +--------------------- + +The Apache implementation of ParserNode interface requires some implementation +specific functionalities that are not described by the interface itself. + +Conditional blocks + +Apache configuration can have conditional blocks, for example: , +resulting the directives and subblocks within it being either enabled or disabled. +While find_* interface methods allow including the disabled parts of the configuration +tree in searches a special care needs to be taken while parsing the structure in +order to reflect the active state of configuration. + +Whitespaces + +Each ParserNode object is responsible of storing its prepending whitespace characters +in order to be able to write the AST back to filesystem like it was, preserving the +format, this applies for parameters of BlockNode and DirectiveNode as well. +When parameters of ParserNode are changed, the pre-existing whitespaces in the +parameter sequence are discarded, as the general reason for storing them is to +maintain the ability to write the configuration back to filesystem exactly like +it was. This loses its meaning when we have to change the directives or blocks +parameters for other reasons. + +Searches and matching + +Apache configuration is largely case insensitive, so the Apache implementation of +ParserNode interface needs to provide the user means to match block and directive +names and parameters in case insensitive manner. This does not apply to everything +however, for example the parameters of a conditional statement may be case sensitive. +For this reason the internal representation of data should not ignore the case. +""" + +import abc +import six + + +@six.add_metaclass(abc.ABCMeta) +class ParserNode(object): + """ + ParserNode is the basic building block of the tree of such nodes, + representing the structure of the configuration. It is largely meant to keep + the structure information intact and idiomatically accessible. + + The root node as well as the child nodes of it should be instances of ParserNode. + Nodes keep track of their differences to on-disk representation of configuration + by marking modified ParserNodes as dirty to enable partial write-to-disk for + different files in the configuration structure. + + While for the most parts the usage and the child types are obvious, "include"- + and similar directives are an exception to this rule. This is because of the + nature of include directives - which unroll the contents of another file or + configuration block to their place. While we could unroll the included nodes + to the parent tree, it remains important to keep the context of include nodes + separate in order to write back the original configuration as it was. + + For parsers that require the implementation to keep track of the whitespacing, + it's responsibility of each ParserNode object itself to store its prepending + whitespaces in order to be able to reconstruct the complete configuration file + as it was when originally read from the disk. + """ + + @property + @abc.abstractmethod + def ancestor(self): # pragma: no cover + """ + This property contains a reference to ancestor node, or None if the node + is the root node of the configuration tree. + + :returns: The ancestor BlockNode object, or None for root node. + :rtype: ParserNode + """ + + raise NotImplementedError + + @property + @abc.abstractmethod + def dirty(self): # pragma: no cover + """ + This property contains a boolean value of the information if this node has + been modified since last save (or after the initial parse). + + :returns: True if this node has had changes that have not yet been written + to disk. + :rtype: bool + """ + + raise NotImplementedError + + @abc.abstractmethod + def save(self, msg): + """ + Save traverses the children, and attempts to write the AST to disk for + all the objects that are marked dirty. The actual operation of course + depends on the underlying implementation. save() shouldn't be called + from the Configurator outside of its designated save() method in order + to ensure that the Reverter checkpoints are created properly. + + Note: this approach of keeping internal structure of the configuration + within the ParserNode tree does not represent the file inclusion structure + of actual configuration files that reside in the filesystem. To handle + file writes properly, the file specific temporary trees should be extracted + from the full ParserNode tree where necessary when writing to disk. + + """ + + +@six.add_metaclass(abc.ABCMeta) +class CommentNode(ParserNode): + """ + CommentNode class is used for representation of comments within the parsed + configuration structure. Because of the nature of comments, it is not able + to have child nodes and hence it is always treated as a leaf node. + + CommentNode stores its contents in class variable 'comment' and does not + have a specific name. + + """ + + @property + @abc.abstractmethod + def comment(self): # pragma: no cover + """ + Comment property contains the contents of the comment without the comment + directives (typically # or /* ... */). + + :returns: A string containing the comment + :rtype: str + """ + + raise NotImplementedError + +@six.add_metaclass(abc.ABCMeta) +class DirectiveNode(ParserNode): + """ + DirectiveNode class represents a configuration directive within the configuration. + It can have zero or more parameters attached to it. Because of the nature of + single directives, it is not able to have child nodes and hence it is always + treated as a leaf node. + """ + + @property + @abc.abstractmethod + def enabled(self): # pragma: no cover + """ + Configuration blocks may have conditional statements enabling or disabling + their contents. This property returns the state of this DirectiveNode. + + :returns: True if the DirectiveNode is parsed and enabled in the configuration. + :rtype: bool + """ + + raise NotImplementedError + + @property + @abc.abstractmethod + def name(self): # pragma: no cover + """ + Name property contains the name of the directive. + + :returns: Name of this node + :rtype: str + """ + + raise NotImplementedError + + @property + @abc.abstractmethod + def parameters(self): # pragma: no cover + """ + This property contains a tuple of parameters of this ParserNode object + excluding whitespaces. + + :returns: A tuple of parameters for this node + :rtype: tuple + """ + + raise NotImplementedError + + @abc.abstractmethod + def set_parameters(self, parameters): + """ + Sets the sequence of parameters for this ParserNode object without + whitespaces. While the whitespaces for parameters are discarded when using + this method, the whitespacing preceeding the ParserNode itself should be + kept intact. + + :param list parameters: sequence of parameters + """ + + +@six.add_metaclass(abc.ABCMeta) +class BlockNode(ParserNode): + """ + BlockNode class represents a block of nested configuration directives, comments + and other blocks as its children. A BlockNode can have zero or more parameters + attached to it. + + Configuration blocks typically consist of one or more child nodes of all possible + types. Because of this, the BlockNode class has various discovery and structure + management methods. + + Lists of parameters used as an optional argument for some of the methods should + be lists of strings that are applicable parameters for each specific BlockNode + or DirectiveNode type. As an example, for a following configuration example: + + + ... + + + The node type would be BlockNode, name would be 'VirtualHost' and its parameters + would be: ['*:80']. + + While for the following example: + + LoadModule alias_module /usr/lib/apache2/modules/mod_alias.so + + The node type would be DirectiveNode, name would be 'LoadModule' and its + parameters would be: ['alias_module', '/usr/lib/apache2/modules/mod_alias.so'] + + The applicable parameters are dependent on the underlying configuration language + and its grammar. + """ + + @abc.abstractmethod + def add_child_block(self, name, parameters=None, position=None): + """ + Adds a new BlockNode child node with provided values and marks the callee + BlockNode dirty. This is used to add new children to the AST. The preceeding + whitespaces should not be added based on the ancestor or siblings for the + newly created object. This is to match the current behavior of the legacy + parser implementation. + + :param str name: The name of the child node to add + :param list parameters: list of parameters for the node + :param int position: Position in the list of children to add the new child + node to. Defaults to None, which appends the newly created node to the list. + If an integer is given, the child is inserted before that index in the + list similar to list().insert. + + :returns: BlockNode instance of the created child block + + """ + + @abc.abstractmethod + def add_child_directive(self, name, parameters=None, position=None): + """ + Adds a new DirectiveNode child node with provided values and marks the + callee BlockNode dirty. This is used to add new children to the AST. The + preceeding whitespaces should not be added based on the ancestor or siblings + for the newly created object. This is to match the current behavior of the + legacy parser implementation. + + + :param str name: The name of the child node to add + :param list parameters: list of parameters for the node + :param int position: Position in the list of children to add the new child + node to. Defaults to None, which appends the newly created node to the list. + If an integer is given, the child is inserted before that index in the + list similar to list().insert. + + :returns: DirectiveNode instance of the created child directive + + """ + + @abc.abstractmethod + def add_child_comment(self, comment="", position=None): + """ + Adds a new CommentNode child node with provided value and marks the + callee BlockNode dirty. This is used to add new children to the AST. The + preceeding whitespaces should not be added based on the ancestor or siblings + for the newly created object. This is to match the current behavior of the + legacy parser implementation. + + + :param str comment: Comment contents + :param int position: Position in the list of children to add the new child + node to. Defaults to None, which appends the newly created node to the list. + If an integer is given, the child is inserted before that index in the + list similar to list().insert. + + :returns: CommentNode instance of the created child comment + + """ + + @property + @abc.abstractmethod + def children(self): # pragma: no cover + """ + This property contains a list ParserNode objects that are the children + for this node. The order of children is the same as that of the parsed + configuration block. + + :returns: A tuple of this block's children + :rtype: tuple + """ + + raise NotImplementedError + + @property + @abc.abstractmethod + def enabled(self): + """ + Configuration blocks may have conditional statements enabling or disabling + their contents. This property returns the state of this configuration block. + In case of unmatched conditional statement in block, this block itself should + be set enabled while its children should be set disabled. + + :returns: True if the BlockNode is parsed and enabled in the configuration. + :rtype: bool + """ + + @abc.abstractmethod + def find_blocks(self, name, exclude=True): + """ + Find a configuration block by name. This method walks the child tree of + ParserNodes under the instance it was called from. This way it is possible + to search for the whole configuration tree, when starting from root node or + to do a partial search when starting from a specified branch. The lookup + should be case insensitive. + + :param str name: The name of the directive to search for + :param bool exclude: If the search results should exclude the contents of + ParserNode objects that reside within conditional blocks and because + of current state are not enabled. + + :returns: A list of found BlockNode objects. + """ + + @abc.abstractmethod + def find_directives(self, name, exclude=True): + """ + Find a directive by name. This method walks the child tree of ParserNodes + under the instance it was called from. This way it is possible to search + for the whole configuration tree, when starting from root node, or to do + a partial search when starting from a specified branch. The lookup should + be case insensitive. + + :param str name: The name of the directive to search for + :param bool exclude: If the search results should exclude the contents of + ParserNode objects that reside within conditional blocks and because + of current state are not enabled. + + :returns: A list of found DirectiveNode objects. + + """ + + @abc.abstractmethod + def find_comments(self, comment, exact=False): + """ + Find comments with value containing or being exactly the same as search term. + + This method walks the child tree of ParserNodes under the instance it was + called from. This way it is possible to search for the whole configuration + tree, when starting from root node, or to do a partial search when starting + from a specified branch. The lookup should be case sensitive. + + :param str comment: The content of comment to search for + :param bool exact: If the comment needs to exactly match the search term + + :returns: A list of found CommentNode objects. + + """ + + @abc.abstractmethod + def delete_child(self, child): + """ + Remove a specified child node from the list of children of the called + BlockNode object. + + :param ParserNode child: Child ParserNode object to remove from the list + of children of the callee. + """ + + @property + @abc.abstractmethod + def name(self): # pragma: no cover + """ + Name property contains the name of the block. As an example for config: + ... + the name would be "VirtualHost". + + :returns: Name of this node + :rtype: str + """ + + raise NotImplementedError + + @property + @abc.abstractmethod + def parameters(self): # pragma: no cover + """ + This property contains a tuple of parameters of this ParserNode object + excluding whitespaces. + + :returns: A tuple of parameters for this node + :rtype: tuple + """ + + raise NotImplementedError + + @abc.abstractmethod + def set_parameters(self, parameters): + """ + Sets the sequence of parameters for this ParserNode object without + whitespaces. While the whitespaces for parameters are discarded when using + this method, the whitespacing preceeding the ParserNode itself should be + kept intact. + + :param list parameters: sequence of parameters + """ + + @abc.abstractmethod + def unsaved_files(self): + """ + Returns a list of file paths that have been changed since the last save + (or the initial configuration parse). The intended use for this method + is to tell the Reverter which files need to be included in a checkpoint. + + This is typically called for the root of the ParserNode tree. + + :returns: list of file paths of files that have been changed but not yet + saved to disk. + """ diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py new file mode 100644 index 000000000..6b45a4d6d --- /dev/null +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -0,0 +1,86 @@ +""" Tests for ParserNode interface """ + +import unittest + +from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module + +from certbot_apache import interfaces + + + +class DummyCommentNode(interfaces.CommentNode): + """ A dummy class implementing CommentNode interface """ + ancestor = None + comment = "" + dirty = False + + def save(self, msg): # pragma: no cover + pass + + +class DummyDirectiveNode(interfaces.DirectiveNode): + """ A dummy class implementing DirectiveNode interface """ + ancestor = None + parameters = tuple() # type: Tuple[str, ...] + dirty = False + enabled = True + name = "" + + def save(self, msg): # pragma: no cover + pass + + def set_parameters(self, parameters): # pragma: no cover + pass + + +class DummyBlockNode(interfaces.BlockNode): + """ A dummy class implementing BlockNode interface """ + ancestor = None + parameters = tuple() # type: Tuple[str, ...] + children = tuple() # type: Tuple[interfaces.ParserNode, ...] + dirty = False + enabled = True + name = "" + + def save(self, msg): # pragma: no cover + pass + + def add_child_block(self, name, parameters=None, position=None): # pragma: no cover + pass + + def add_child_directive(self, name, parameters=None, position=None): # pragma: no cover + pass + + def add_child_comment(self, comment="", position=None): # pragma: no cover + pass + + def find_blocks(self, name, exclude=True): # pragma: no cover + pass + + def find_directives(self, name, exclude=True): # pragma: no cover + pass + + def find_comments(self, comment, exact=False): # pragma: no cover + pass + + def delete_child(self, child): # pragma: no cover + pass + + def set_parameters(self, parameters): # pragma: no cover + pass + + def unsaved_files(self): # pragma: no cover + pass + + +class ParserNodeTest(unittest.TestCase): + """Dummy placeholder test case for ParserNode interfaces""" + + def test_dummy(self): + dummyblock = DummyBlockNode() + dummydirective = DummyDirectiveNode() + dummycomment = DummyCommentNode() + + +if __name__ == "__main__": + unittest.main() # pragma: no cover From af1c66b28f5cad7c33c7da06feb2cc3c2bd8dc30 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 30 Aug 2019 23:42:18 +0300 Subject: [PATCH 02/23] [Apache v2] Modifications to ParserNode interfaces (#7330) This PR contains the changes requested in initial pre-review comments of #7308 Move properties to class pydocs in interfaces.py Prefer class ABC register() functionality instead of class inheritance for interface classes Add apache implementation specific functions to interfaces * Move class argument definitions to class pydoc * Add apache specific functionality to the interface * Bring inheritance back * Define initialization for different ParserNode classes * Add parsernode utils to check keyword arguments and document the defaults in pydoc * Fix pydocs and make BlockNode a child of DirectiveNode * Refine docs, and remove unused __init__ from BlockNode * Split parsernode util tests to their own respective file * Skip cover for dummy calls to super * Add types to method documentation * Add documentation for children --- certbot-apache/certbot_apache/interfaces.py | 248 +++++++++--------- .../certbot_apache/parsernode_util.py | 85 ++++++ .../certbot_apache/tests/parsernode_test.py | 101 ++++--- .../tests/parsernode_util_test.py | 87 ++++++ 4 files changed, 360 insertions(+), 161 deletions(-) create mode 100644 certbot-apache/certbot_apache/parsernode_util.py create mode 100644 certbot-apache/certbot_apache/tests/parsernode_util_test.py diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index a614f85c9..919aa7e93 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -86,6 +86,8 @@ For this reason the internal representation of data should not ignore the case. import abc import six +from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module + @six.add_metaclass(abc.ABCMeta) class ParserNode(object): @@ -110,35 +112,42 @@ class ParserNode(object): it's responsibility of each ParserNode object itself to store its prepending whitespaces in order to be able to reconstruct the complete configuration file as it was when originally read from the disk. + + ParserNode objects should have the following attributes: + + # Reference to ancestor node, or None if the node is the root node of the + # configuration tree. + ancestor: Optional[ParserNode] + + # True if this node has been modified since last save. + dirty: bool + + # Filepath of the file where the configuration element for this ParserNode + # object resides. For root node, the value for filepath is the httpd root + # configuration file. Filepath can be None if a configuration directive is + # defined in for example the httpd command line. + filepath: Optional[str] """ - @property @abc.abstractmethod - def ancestor(self): # pragma: no cover + def __init__(self, **kwargs): """ - This property contains a reference to ancestor node, or None if the node - is the root node of the configuration tree. + Initializes the ParserNode instance, and sets the ParserNode specific + instance variables. This is not meant to be used directly, but through + specific classes implementing ParserNode interface. - :returns: The ancestor BlockNode object, or None for root node. - :rtype: ParserNode + :param ancestor: BlockNode ancestor for this CommentNode. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this CommentNode + does or should exist in the filesystem. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this CommentNode has been + created or changed after the last save. Default: False. + :type dirty: bool """ - raise NotImplementedError - - @property - @abc.abstractmethod - def dirty(self): # pragma: no cover - """ - This property contains a boolean value of the information if this node has - been modified since last save (or after the initial parse). - - :returns: True if this node has had changes that have not yet been written - to disk. - :rtype: bool - """ - - raise NotImplementedError - @abc.abstractmethod def save(self, msg): """ @@ -154,10 +163,13 @@ class ParserNode(object): file writes properly, the file specific temporary trees should be extracted from the full ParserNode tree where necessary when writing to disk. + :param str msg: Message describing the reason for the save. + """ -@six.add_metaclass(abc.ABCMeta) +# Linter rule exclusion done because of https://github.com/PyCQA/pylint/issues/179 +@six.add_metaclass(abc.ABCMeta) # pylint: disable=abstract-method class CommentNode(ParserNode): """ CommentNode class is used for representation of comments within the parsed @@ -167,20 +179,38 @@ class CommentNode(ParserNode): CommentNode stores its contents in class variable 'comment' and does not have a specific name. + CommentNode objects should have the following attributes in addition to + the ones described in ParserNode: + + # Contains the contents of the comment without the directive notation + # (typically # or /* ... */). + comment: str + """ - @property @abc.abstractmethod - def comment(self): # pragma: no cover + def __init__(self, **kwargs): """ - Comment property contains the contents of the comment without the comment - directives (typically # or /* ... */). + Initializes the CommentNode instance and sets its instance variables. - :returns: A string containing the comment - :rtype: str + :param comment: Contents of the comment. Required. + :type comment: str + + :param ancestor: BlockNode ancestor for this CommentNode. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this CommentNode + does or should exist in the filesystem. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this CommentNode has been + created or changed after the last save. Default: False. + :type dirty: bool """ + super(CommentNode, self).__init__(ancestor=kwargs['ancestor'], + dirty=kwargs.get('dirty', False), + filepath=kwargs['filepath']) # pragma: no cover - raise NotImplementedError @six.add_metaclass(abc.ABCMeta) class DirectiveNode(ParserNode): @@ -189,45 +219,61 @@ class DirectiveNode(ParserNode): It can have zero or more parameters attached to it. Because of the nature of single directives, it is not able to have child nodes and hence it is always treated as a leaf node. + + If a this directive was defined on the httpd command line, the ancestor instance + variable for this DirectiveNode should be None, and it should be inserted to the + beginning of root BlockNode children sequence. + + DirectiveNode objects should have the following attributes in addition to + the ones described in ParserNode: + + # True if this DirectiveNode is enabled and False if it is inside of an + # inactive conditional block. + enabled: bool + + # Name, or key of the configuration directive. If BlockNode subclass of + # DirectiveNode is the root configuration node, the name should be None. + name: Optional[str] + + # Tuple of parameters of this ParserNode object, excluding whitespaces. + parameters: Tuple[str, ...] + """ - @property @abc.abstractmethod - def enabled(self): # pragma: no cover + def __init__(self, **kwargs): """ - Configuration blocks may have conditional statements enabling or disabling - their contents. This property returns the state of this DirectiveNode. + Initializes the DirectiveNode instance and sets its instance variables. + + :param name: Name or key of the DirectiveNode object. Required. + :type name: str or None + + :param tuple parameters: Tuple of str parameters for this DirectiveNode. + Default: (). + :type parameters: tuple + + :param ancestor: BlockNode ancestor for this DirectiveNode, or None for + root configuration node. Required. + :type ancestor: BlockNode or None + + :param filepath: Filesystem path for the file where this DirectiveNode + does or should exist in the filesystem, or None for directives introduced + in the httpd command line. Required. + :type filepath: str or None + + :param dirty: Boolean flag for denoting if this DirectiveNode has been + created or changed after the last save. Default: False. + :type dirty: bool + + :param enabled: True if this DirectiveNode object is parsed in the active + configuration of the httpd. False if the DirectiveNode exists within a + unmatched conditional configuration block. Default: True. + :type enabled: bool - :returns: True if the DirectiveNode is parsed and enabled in the configuration. - :rtype: bool """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def name(self): # pragma: no cover - """ - Name property contains the name of the directive. - - :returns: Name of this node - :rtype: str - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def parameters(self): # pragma: no cover - """ - This property contains a tuple of parameters of this ParserNode object - excluding whitespaces. - - :returns: A tuple of parameters for this node - :rtype: tuple - """ - - raise NotImplementedError + super(DirectiveNode, self).__init__(ancestor=kwargs['ancestor'], + dirty=kwargs.get('dirty', False), + filepath=kwargs['filepath']) # pragma: no cover @abc.abstractmethod def set_parameters(self, parameters): @@ -242,7 +288,7 @@ class DirectiveNode(ParserNode): @six.add_metaclass(abc.ABCMeta) -class BlockNode(ParserNode): +class BlockNode(DirectiveNode): """ BlockNode class represents a block of nested configuration directives, comments and other blocks as its children. A BlockNode can have zero or more parameters @@ -272,6 +318,15 @@ class BlockNode(ParserNode): The applicable parameters are dependent on the underlying configuration language and its grammar. + + BlockNode objects should have the following attributes in addition to + the ones described in DirectiveNode: + + # Tuple of direct children of this BlockNode object. The order of children + # in this tuple retain the order of elements in the parsed configuration + # block. + children: Tuple[ParserNode, ...] + """ @abc.abstractmethod @@ -335,33 +390,6 @@ class BlockNode(ParserNode): """ - @property - @abc.abstractmethod - def children(self): # pragma: no cover - """ - This property contains a list ParserNode objects that are the children - for this node. The order of children is the same as that of the parsed - configuration block. - - :returns: A tuple of this block's children - :rtype: tuple - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def enabled(self): - """ - Configuration blocks may have conditional statements enabling or disabling - their contents. This property returns the state of this configuration block. - In case of unmatched conditional statement in block, this block itself should - be set enabled while its children should be set disabled. - - :returns: True if the BlockNode is parsed and enabled in the configuration. - :rtype: bool - """ - @abc.abstractmethod def find_blocks(self, name, exclude=True): """ @@ -424,44 +452,6 @@ class BlockNode(ParserNode): of children of the callee. """ - @property - @abc.abstractmethod - def name(self): # pragma: no cover - """ - Name property contains the name of the block. As an example for config: - ... - the name would be "VirtualHost". - - :returns: Name of this node - :rtype: str - """ - - raise NotImplementedError - - @property - @abc.abstractmethod - def parameters(self): # pragma: no cover - """ - This property contains a tuple of parameters of this ParserNode object - excluding whitespaces. - - :returns: A tuple of parameters for this node - :rtype: tuple - """ - - raise NotImplementedError - - @abc.abstractmethod - def set_parameters(self, parameters): - """ - Sets the sequence of parameters for this ParserNode object without - whitespaces. While the whitespaces for parameters are discarded when using - this method, the whitespacing preceeding the ParserNode itself should be - kept intact. - - :param list parameters: sequence of parameters - """ - @abc.abstractmethod def unsaved_files(self): """ diff --git a/certbot-apache/certbot_apache/parsernode_util.py b/certbot-apache/certbot_apache/parsernode_util.py new file mode 100644 index 000000000..2450a1c15 --- /dev/null +++ b/certbot-apache/certbot_apache/parsernode_util.py @@ -0,0 +1,85 @@ +"""ParserNode utils""" + + +def validate_kwargs(kwargs, required_names): + """ + Ensures that the kwargs dict has all the expected values. This function modifies + the kwargs dictionary, and hence the returned dictionary should be used instead + in the caller function instead of the original kwargs. + + :param dict kwargs: Dictionary of keyword arguments to validate. + :param list required_names: List of required parameter names. + """ + + validated_kwargs = dict() + for name in required_names: + try: + validated_kwargs[name] = kwargs.pop(name) + except KeyError: + raise TypeError("Required keyword argument: {} undefined.".format(name)) + + # Raise exception if unknown key word arguments are found. + if kwargs: + unknown = ", ".join(kwargs.keys()) + raise TypeError("Unknown keyword argument(s): {}".format(unknown)) + return validated_kwargs + + +def parsernode_kwargs(kwargs): + """ + Validates keyword arguments for ParserNode. This function modifies the kwargs + dictionary, and hence the returned dictionary should be used instead in the + caller function instead of the original kwargs. + + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments. + """ + kwargs.setdefault("dirty", False) + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath"]) + return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"] + + +def commentnode_kwargs(kwargs): + """ + Validates keyword arguments for CommentNode and sets the default values for + optional kwargs. This function modifies the kwargs dictionary, and hence the + returned dictionary should be used instead in the caller function instead of + the original kwargs. + + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments and ParserNode kwargs. + """ + kwargs.setdefault("dirty", False) + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment"]) + + comment = kwargs.pop("comment") + return comment, kwargs + + +def directivenode_kwargs(kwargs): + """ + Validates keyword arguments for DirectiveNode and BlockNode and sets the + default values for optional kwargs. This function modifies the kwargs + dictionary, and hence the returned dictionary should be used instead in the + caller function instead of the original kwargs. + + :param dict kwargs: Keyword argument dictionary to validate. + + :returns: Tuple of validated and prepared arguments and ParserNode kwargs. + """ + + kwargs.setdefault("dirty", False) + kwargs.setdefault("enabled", True) + kwargs.setdefault("parameters", ()) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "name", + "parameters", "enabled"]) + + name = kwargs.pop("name") + parameters = kwargs.pop("parameters") + enabled = kwargs.pop("enabled") + return name, parameters, enabled, kwargs diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py index 6b45a4d6d..0fba32b98 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -2,84 +2,121 @@ import unittest -from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module - from certbot_apache import interfaces +from certbot_apache import parsernode_util as util +class DummyParserNode(interfaces.ParserNode): + """ A dummy class implementing ParserNode interface """ -class DummyCommentNode(interfaces.CommentNode): + def __init__(self, **kwargs): + """ + Initializes the ParserNode instance. + """ + ancestor, dirty, filepath = util.parsernode_kwargs(kwargs) + self.ancestor = ancestor + self.dirty = dirty + self.filepath = filepath + super(DummyParserNode, self).__init__(**kwargs) + + def save(self, msg): # pragma: no cover + """Save""" + pass + + +class DummyCommentNode(DummyParserNode): """ A dummy class implementing CommentNode interface """ - ancestor = None - comment = "" - dirty = False - def save(self, msg): # pragma: no cover - pass + def __init__(self, **kwargs): + """ + Initializes the CommentNode instance and sets its instance variables. + """ + comment, kwargs = util.commentnode_kwargs(kwargs) + self.comment = comment + super(DummyCommentNode, self).__init__(**kwargs) -class DummyDirectiveNode(interfaces.DirectiveNode): +class DummyDirectiveNode(DummyParserNode): """ A dummy class implementing DirectiveNode interface """ - ancestor = None - parameters = tuple() # type: Tuple[str, ...] - dirty = False - enabled = True - name = "" - def save(self, msg): # pragma: no cover - pass + # pylint: disable=too-many-arguments + def __init__(self, **kwargs): + """ + Initializes the DirectiveNode instance and sets its instance variables. + """ + name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs) + self.name = name + self.parameters = parameters + self.enabled = enabled + + super(DummyDirectiveNode, self).__init__(**kwargs) def set_parameters(self, parameters): # pragma: no cover + """Set parameters""" pass -class DummyBlockNode(interfaces.BlockNode): +class DummyBlockNode(DummyDirectiveNode): """ A dummy class implementing BlockNode interface """ - ancestor = None - parameters = tuple() # type: Tuple[str, ...] - children = tuple() # type: Tuple[interfaces.ParserNode, ...] - dirty = False - enabled = True - name = "" - - def save(self, msg): # pragma: no cover - pass def add_child_block(self, name, parameters=None, position=None): # pragma: no cover + """Add child block""" pass def add_child_directive(self, name, parameters=None, position=None): # pragma: no cover + """Add child directive""" pass def add_child_comment(self, comment="", position=None): # pragma: no cover + """Add child comment""" pass def find_blocks(self, name, exclude=True): # pragma: no cover + """Find blocks""" pass def find_directives(self, name, exclude=True): # pragma: no cover + """Find directives""" pass def find_comments(self, comment, exact=False): # pragma: no cover + """Find comments""" pass def delete_child(self, child): # pragma: no cover - pass - - def set_parameters(self, parameters): # pragma: no cover + """Delete child""" pass def unsaved_files(self): # pragma: no cover + """Unsaved files""" pass +interfaces.CommentNode.register(DummyCommentNode) +interfaces.DirectiveNode.register(DummyDirectiveNode) +interfaces.BlockNode.register(DummyBlockNode) + class ParserNodeTest(unittest.TestCase): """Dummy placeholder test case for ParserNode interfaces""" def test_dummy(self): - dummyblock = DummyBlockNode() - dummydirective = DummyDirectiveNode() - dummycomment = DummyCommentNode() + dummyblock = DummyBlockNode( + name="None", + parameters=(), + ancestor=None, + dirty=False, + filepath="/some/random/path" + ) + dummydirective = DummyDirectiveNode( + name="Name", + ancestor=None, + filepath="/another/path" + ) + dummycomment = DummyCommentNode( + comment="Comment", + ancestor=dummyblock, + filepath="/some/file" + ) if __name__ == "__main__": diff --git a/certbot-apache/certbot_apache/tests/parsernode_util_test.py b/certbot-apache/certbot_apache/tests/parsernode_util_test.py new file mode 100644 index 000000000..acb7a92db --- /dev/null +++ b/certbot-apache/certbot_apache/tests/parsernode_util_test.py @@ -0,0 +1,87 @@ +""" Tests for ParserNode utils """ +import unittest + +from certbot_apache import parsernode_util as util + + +class ParserNodeUtilTest(unittest.TestCase): + """Tests for ParserNode utils""" + + def _setup_parsernode(self): + """ Sets up kwargs dict for ParserNode """ + return { + "ancestor": None, + "dirty": False, + "filepath": "/tmp", + } + + def _setup_commentnode(self): + """ Sets up kwargs dict for CommentNode """ + + pn = self._setup_parsernode() + pn["comment"] = "x" + return pn + + def _setup_directivenode(self): + """ Sets up kwargs dict for DirectiveNode """ + + pn = self._setup_parsernode() + pn["name"] = "Name" + pn["parameters"] = ("first",) + pn["enabled"] = True + return pn + + def test_unknown_parameter(self): + params = self._setup_parsernode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.parsernode_kwargs, params) + + params = self._setup_commentnode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.commentnode_kwargs, params) + + params = self._setup_directivenode() + params["unknown"] = "unknown" + self.assertRaises(TypeError, util.directivenode_kwargs, params) + + def test_parsernode(self): + params = self._setup_parsernode() + ctrl = self._setup_parsernode() + + ancestor, dirty, filepath = util.parsernode_kwargs(params) + self.assertEqual(ancestor, ctrl["ancestor"]) + self.assertEqual(dirty, ctrl["dirty"]) + self.assertEqual(filepath, ctrl["filepath"]) + + def test_commentnode(self): + params = self._setup_commentnode() + ctrl = self._setup_commentnode() + + comment, _ = util.commentnode_kwargs(params) + self.assertEqual(comment, ctrl["comment"]) + + def test_directivenode(self): + params = self._setup_directivenode() + ctrl = self._setup_directivenode() + + name, parameters, enabled, _ = util.directivenode_kwargs(params) + self.assertEqual(name, ctrl["name"]) + self.assertEqual(parameters, ctrl["parameters"]) + self.assertEqual(enabled, ctrl["enabled"]) + + def test_missing_required(self): + c_params = self._setup_commentnode() + c_params.pop("comment") + self.assertRaises(TypeError, util.commentnode_kwargs, c_params) + + d_params = self._setup_directivenode() + d_params.pop("ancestor") + self.assertRaises(TypeError, util.directivenode_kwargs, d_params) + + p_params = self._setup_parsernode() + p_params.pop("filepath") + self.assertRaises(TypeError, util.parsernode_kwargs, p_params) + + +if __name__ == "__main__": + unittest.main() # pragma: no cover From 9620cc75d4125066e4e676e299d3eecce09bbf84 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 9 Sep 2019 22:09:09 +0300 Subject: [PATCH 03/23] [Apache v2] Allow initialization of ParserNode instances using metadata dictionary instead of required arguments (#7366) Add metadata keyword argument to the ParserNode interface, allowing the initialization of the object from contents of the metadata - if the implementation allows it. As an example, Augeas implementation needs nothing more than the Augeas DOM path of a configuration directive to be able to populate the ParserNode instance with all data relevant to the DirectiveNode. The checks also allow skipping the otherwise required keyword arguments if metadata is provided. * Allow creating ParserNode instances using information from metadata dictionary * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren * Address review comments * Fix filepath comment * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: Brad Warren --- certbot-apache/certbot_apache/interfaces.py | 37 +++++++++++-- .../certbot_apache/parsernode_util.py | 52 +++++++++++++++++-- .../certbot_apache/tests/parsernode_test.py | 3 +- .../tests/parsernode_util_test.py | 30 ++++++++++- 4 files changed, 111 insertions(+), 11 deletions(-) diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index 919aa7e93..cdfdfac91 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -45,8 +45,10 @@ and any other information relevant to the underlying parser engine. Access to the metadata should be handled by implementation specific methods, allowing the Configurator functionality to access the underlying information where needed. -A good example of this is file path of a node - something that is needed by the -reverter functionality within the Configurator. + +For some implementations the node can be initialized using the information carried +in metadata alone. This is useful especially when populating the ParserNode tree +while parsing the configuration. Apache implementation @@ -55,6 +57,20 @@ Apache implementation The Apache implementation of ParserNode interface requires some implementation specific functionalities that are not described by the interface itself. +Initialization + +When the user of a ParserNode class is creating these objects, they must specify +the parameters as described in the documentation for the __init__ methods below. +When these objects are created internally, however, some parameters may not be +needed because (possibly more detailed) information is included in the metadata +parameter. In this case, implementations can deviate from the required parameters +from __init__, however, they should still behave the same when metadata is not +provided. + +For consistency internally, if an argument is provided directly in the ParserNode +initialization parameters as well as within metadata it's recommended to establish +clear behavior around this scenario within the implementation. + Conditional blocks Apache configuration can have conditional blocks, for example: , @@ -86,7 +102,7 @@ For this reason the internal representation of data should not ignore the case. import abc import six -from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module +from acme.magic_typing import Any, Dict, Optional, Tuple # pylint: disable=unused-import, no-name-in-module @six.add_metaclass(abc.ABCMeta) @@ -127,6 +143,10 @@ class ParserNode(object): # configuration file. Filepath can be None if a configuration directive is # defined in for example the httpd command line. filepath: Optional[str] + + # Metadata dictionary holds all the implementation specific key-value pairs + # for the ParserNode instance. + metadata: Dict[str, Any] """ @abc.abstractmethod @@ -146,6 +166,11 @@ class ParserNode(object): :param dirty: Boolean flag for denoting if this CommentNode has been created or changed after the last save. Default: False. :type dirty: bool + + :param metadata: Dictionary of metadata values for this ParserNode object. + Metadata information should be used only internally in the implementation. + Default: {} + :type metadata: dict """ @abc.abstractmethod @@ -209,7 +234,8 @@ class CommentNode(ParserNode): """ super(CommentNode, self).__init__(ancestor=kwargs['ancestor'], dirty=kwargs.get('dirty', False), - filepath=kwargs['filepath']) # pragma: no cover + filepath=kwargs['filepath'], + metadata=kwargs.get('metadata', {})) # pragma: no cover @six.add_metaclass(abc.ABCMeta) @@ -273,7 +299,8 @@ class DirectiveNode(ParserNode): """ super(DirectiveNode, self).__init__(ancestor=kwargs['ancestor'], dirty=kwargs.get('dirty', False), - filepath=kwargs['filepath']) # pragma: no cover + filepath=kwargs['filepath'], + metadata=kwargs.get('metadata', {})) # pragma: no cover @abc.abstractmethod def set_parameters(self, parameters): diff --git a/certbot-apache/certbot_apache/parsernode_util.py b/certbot-apache/certbot_apache/parsernode_util.py index 2450a1c15..d9646862a 100644 --- a/certbot-apache/certbot_apache/parsernode_util.py +++ b/certbot-apache/certbot_apache/parsernode_util.py @@ -31,14 +31,28 @@ def parsernode_kwargs(kwargs): dictionary, and hence the returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "filepath" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. Filepath defaults to None. :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments. """ + + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) - kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath"]) - return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"] + kwargs.setdefault("metadata", {}) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "metadata"]) + return kwargs["ancestor"], kwargs["dirty"], kwargs["filepath"], kwargs["metadata"] def commentnode_kwargs(kwargs): @@ -48,13 +62,29 @@ def commentnode_kwargs(kwargs): returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "comment" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments and ParserNode kwargs. """ + + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + kwargs.setdefault("comment", None) + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) - kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment"]) + kwargs.setdefault("metadata", {}) + + kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "comment", + "metadata"]) comment = kwargs.pop("comment") return comment, kwargs @@ -67,17 +97,31 @@ def directivenode_kwargs(kwargs): dictionary, and hence the returned dictionary should be used instead in the caller function instead of the original kwargs. + If metadata is provided, the otherwise required argument "name" may be + omitted if the implementation is able to extract its value from the metadata. + This usecase is handled within this function. + :param dict kwargs: Keyword argument dictionary to validate. :returns: Tuple of validated and prepared arguments and ParserNode kwargs. """ + # As many values of ParserNode instances can be derived from the metadata, + # (ancestor being a common exception here) make sure we permit it here as well. + if "metadata" in kwargs: + kwargs.setdefault("name", None) + # Filepath can be derived from the metadata in Augeas implementation. + # Default is None, as in this case the responsibility of populating this + # variable lies on the implementation. + kwargs.setdefault("filepath", None) + kwargs.setdefault("dirty", False) kwargs.setdefault("enabled", True) kwargs.setdefault("parameters", ()) + kwargs.setdefault("metadata", {}) kwargs = validate_kwargs(kwargs, ["ancestor", "dirty", "filepath", "name", - "parameters", "enabled"]) + "parameters", "enabled", "metadata"]) name = kwargs.pop("name") parameters = kwargs.pop("parameters") diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py index 0fba32b98..1a2288c82 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -13,10 +13,11 @@ class DummyParserNode(interfaces.ParserNode): """ Initializes the ParserNode instance. """ - ancestor, dirty, filepath = util.parsernode_kwargs(kwargs) + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs) self.ancestor = ancestor self.dirty = dirty self.filepath = filepath + self.metadata = metadata super(DummyParserNode, self).__init__(**kwargs) def save(self, msg): # pragma: no cover diff --git a/certbot-apache/certbot_apache/tests/parsernode_util_test.py b/certbot-apache/certbot_apache/tests/parsernode_util_test.py index acb7a92db..a079759ee 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_util_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_util_test.py @@ -48,10 +48,21 @@ class ParserNodeUtilTest(unittest.TestCase): params = self._setup_parsernode() ctrl = self._setup_parsernode() - ancestor, dirty, filepath = util.parsernode_kwargs(params) + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(params) self.assertEqual(ancestor, ctrl["ancestor"]) self.assertEqual(dirty, ctrl["dirty"]) self.assertEqual(filepath, ctrl["filepath"]) + self.assertEqual(metadata, {}) + + def test_parsernode_from_metadata(self): + params = self._setup_parsernode() + params.pop("filepath") + md = {"some": "value"} + params["metadata"] = md + + # Just testing that error from missing required parameters is not raised + _, _, _, metadata = util.parsernode_kwargs(params) + self.assertEqual(metadata, md) def test_commentnode(self): params = self._setup_commentnode() @@ -60,6 +71,14 @@ class ParserNodeUtilTest(unittest.TestCase): comment, _ = util.commentnode_kwargs(params) self.assertEqual(comment, ctrl["comment"]) + def test_commentnode_from_metadata(self): + params = self._setup_commentnode() + params.pop("comment") + params["metadata"] = {} + + # Just testing that error from missing required parameters is not raised + util.commentnode_kwargs(params) + def test_directivenode(self): params = self._setup_directivenode() ctrl = self._setup_directivenode() @@ -69,6 +88,15 @@ class ParserNodeUtilTest(unittest.TestCase): self.assertEqual(parameters, ctrl["parameters"]) self.assertEqual(enabled, ctrl["enabled"]) + def test_directivenode_from_metadata(self): + params = self._setup_directivenode() + params.pop("filepath") + params.pop("name") + params["metadata"] = {"irrelevant": "value"} + + # Just testing that error from missing required parameters is not raised + util.directivenode_kwargs(params) + def test_missing_required(self): c_params = self._setup_commentnode() c_params.pop("comment") From 23fb6d28773a117e6c9456ae64d8aaba4a6fbe56 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 18 Sep 2019 23:31:44 +0300 Subject: [PATCH 04/23] [Apache v2] DualParserNode implementation 1/3 (#7374) * DualParserNode, DualCommentNode and DualDirectiveNode implementations * Address review comments * Address review comments * Simplify isPass --- certbot-apache/certbot_apache/assertions.py | 61 ++++++++++++ certbot-apache/certbot_apache/augeasparser.py | 69 ++++++++++++++ certbot-apache/certbot_apache/dualparser.py | 95 +++++++++++++++++++ .../certbot_apache/tests/dualnode_test.py | 88 +++++++++++++++++ 4 files changed, 313 insertions(+) create mode 100644 certbot-apache/certbot_apache/assertions.py create mode 100644 certbot-apache/certbot_apache/augeasparser.py create mode 100644 certbot-apache/certbot_apache/dualparser.py create mode 100644 certbot-apache/certbot_apache/tests/dualnode_test.py diff --git a/certbot-apache/certbot_apache/assertions.py b/certbot-apache/certbot_apache/assertions.py new file mode 100644 index 000000000..659efee28 --- /dev/null +++ b/certbot-apache/certbot_apache/assertions.py @@ -0,0 +1,61 @@ +"""Dual parser node assertions""" +from certbot_apache import interfaces + + +PASS = "CERTBOT_PASS_ASSERT" + + +def assertEqual(first, second): + """ Equality assertion """ + + if isinstance(first, interfaces.CommentNode): + assertEqualComment(first, second) + elif isinstance(first, interfaces.DirectiveNode): + assertEqualDirective(first, second) + + # Skip tests if filepath includes the pass value. This is done + # because filepath is variable of the base ParserNode interface, and + # unless the implementation is actually done, we cannot assume getting + # correct results from boolean assertion for dirty + if not isPass(first.filepath) and not isPass(second.filepath): + assert first.dirty == second.dirty + # We might want to disable this later if testing with two separate + # (but identical) directory structures. + assert first.filepath == second.filepath + +def assertEqualComment(first, second): # pragma: no cover + """ Equality assertion for CommentNode """ + + assert isinstance(first, interfaces.CommentNode) + assert isinstance(second, interfaces.CommentNode) + + if not isPass(first.comment) and not isPass(second.comment): + assert first.comment == second.comment + +def _assertEqualDirectiveComponents(first, second): # pragma: no cover + """ Handles assertion for instance variables for DirectiveNode and BlockNode""" + + # Enabled value cannot be asserted, because Augeas implementation + # is unable to figure that out. + # assert first.enabled == second.enabled + if not isPass(first.name) and not isPass(second.name): + assert first.name == second.name + + if not isPass(first.parameters) and not isPass(second.parameters): + assert first.parameters == second.parameters + +def assertEqualDirective(first, second): + """ Equality assertion for DirectiveNode """ + + assert isinstance(first, interfaces.DirectiveNode) + assert isinstance(second, interfaces.DirectiveNode) + _assertEqualDirectiveComponents(first, second) + +def isPass(value): # pragma: no cover + """Checks if the value is set to PASS""" + return PASS in value + +def assertEqualSimple(first, second): + """ Simple assertion """ + if not isPass(first) and not isPass(second): + assert first == second diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py new file mode 100644 index 000000000..b7ac5ec3d --- /dev/null +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -0,0 +1,69 @@ +""" Augeas implementation of the ParserNode interface """ +from certbot_apache import assertions +from certbot_apache import interfaces +from certbot_apache import parsernode_util as util + + +class AugeasParserNode(interfaces.ParserNode): + """ Augeas implementation of ParserNode interface """ + + def __init__(self, **kwargs): + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs) # pylint: disable=unused-variable + super(AugeasParserNode, self).__init__(**kwargs) + self.ancestor = ancestor + # self.filepath = filepath + self.filepath = assertions.PASS + self.dirty = dirty + self.metadata = metadata + + def save(self, msg): # pragma: no cover + pass + + +class AugeasCommentNode(AugeasParserNode): + """ Augeas implementation of CommentNode interface """ + + def __init__(self, **kwargs): + comment, kwargs = util.commentnode_kwargs(kwargs) # pylint: disable=unused-variable + super(AugeasCommentNode, self).__init__(**kwargs) + # self.comment = comment + self.comment = assertions.PASS + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.comment == other.comment and + self.filepath == other.filepath and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata) + return False + + +class AugeasDirectiveNode(AugeasParserNode): + """ Augeas implementation of DirectiveNode interface """ + + def __init__(self, **kwargs): + name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs) + super(AugeasDirectiveNode, self).__init__(**kwargs) + self.name = name + self.parameters = parameters + self.enabled = enabled + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.name == other.name and + self.filepath == other.filepath and + self.parameters == other.parameters and + self.enabled == other.enabled and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata) + return False + + def set_parameters(self, parameters): + """Sets the parameters for DirectiveNode""" + self.parameters = parameters + + +interfaces.CommentNode.register(AugeasCommentNode) +interfaces.DirectiveNode.register(AugeasDirectiveNode) diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py new file mode 100644 index 000000000..4bb4b3497 --- /dev/null +++ b/certbot-apache/certbot_apache/dualparser.py @@ -0,0 +1,95 @@ +""" Dual ParserNode implementation """ +from certbot_apache import assertions +from certbot_apache import augeasparser + + +class DualNodeBase(object): + """ Dual parser interface for in development testing. This is used as the + base class for dual parser interface classes. This class handles runtime + attribute value assertions.""" + + def save(self, msg): # pragma: no cover + """ Call save for both parsers """ + self.primary.save(msg) + self.secondary.save(msg) + + def __getattr__(self, aname): + """ Attribute value assertion """ + firstval = getattr(self.primary, aname) + secondval = getattr(self.secondary, aname) + assertions.assertEqualSimple(firstval, secondval) + return firstval + + +class DualCommentNode(DualNodeBase): + """ Dual parser implementation of CommentNode interface """ + + def __init__(self, **kwargs): + """ This initialization implementation allows ordinary initialization + of CommentNode objects as well as creating a DualCommentNode object + using precreated or fetched CommentNode objects if provided as optional + arguments primary and secondary. + + Parameters other than the following are from interfaces.CommentNode: + + :param CommentNode primary: Primary pre-created CommentNode, mainly + used when creating new DualParser nodes using add_* methods. + :param CommentNode secondary: Secondary pre-created CommentNode + """ + + kwargs.setdefault("primary", None) + kwargs.setdefault("secondary", None) + primary = kwargs.pop("primary") + secondary = kwargs.pop("secondary") + + if primary or secondary: + assert primary and secondary + self.primary = primary + self.secondary = secondary + else: + self.primary = augeasparser.AugeasCommentNode(**kwargs) + self.secondary = augeasparser.AugeasCommentNode(**kwargs) + + assertions.assertEqual(self.primary, self.secondary) + + +class DualDirectiveNode(DualNodeBase): + """ Dual parser implementation of DirectiveNode interface """ + + def __init__(self, **kwargs): + """ This initialization implementation allows ordinary initialization + of DirectiveNode objects as well as creating a DualDirectiveNode object + using precreated or fetched DirectiveNode objects if provided as optional + arguments primary and secondary. + + Parameters other than the following are from interfaces.DirectiveNode: + + :param DirectiveNode primary: Primary pre-created DirectiveNode, mainly + used when creating new DualParser nodes using add_* methods. + :param DirectiveNode secondary: Secondary pre-created DirectiveNode + + + """ + + kwargs.setdefault("primary", None) + kwargs.setdefault("secondary", None) + primary = kwargs.pop("primary") + secondary = kwargs.pop("secondary") + + if primary or secondary: + assert primary and secondary + self.primary = primary + self.secondary = secondary + else: + self.primary = augeasparser.AugeasDirectiveNode(**kwargs) + self.secondary = augeasparser.AugeasDirectiveNode(**kwargs) + + assertions.assertEqual(self.primary, self.secondary) + + def set_parameters(self, parameters): + """ Sets parameters and asserts that both implementation successfully + set the parameter sequence """ + + self.primary.set_parameters(parameters) + self.secondary.set_parameters(parameters) + assertions.assertEqual(self.primary, self.secondary) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py new file mode 100644 index 000000000..f8a012280 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -0,0 +1,88 @@ +"""Tests for DualParserNode implementation""" +import unittest + +import mock + +from certbot_apache import assertions +from certbot_apache import dualparser + + +class DualParserNodeTest(unittest.TestCase): + """DualParserNode tests""" + + def setUp(self): # pylint: disable=arguments-differ + self.directive = dualparser.DualDirectiveNode(name="directive", + ancestor=None, + filepath="/tmp/something") + self.comment = dualparser.DualCommentNode(comment="comment", + ancestor=None, + filepath="/tmp/something") + + def test_create_with_precreated(self): + cnode = dualparser.DualCommentNode(comment="comment", + ancestor=None, + filepath="/tmp/something", + primary=self.comment.secondary, + secondary=self.comment.primary) + dnode = dualparser.DualDirectiveNode(name="directive", + ancestor=None, + filepath="/tmp/something", + primary=self.directive.secondary, + secondary=self.directive.primary) + # Switched around + self.assertTrue(cnode.primary is self.comment.secondary) + self.assertTrue(cnode.secondary is self.comment.primary) + self.assertTrue(dnode.primary is self.directive.secondary) + self.assertTrue(dnode.secondary is self.directive.primary) + + def test_set_params(self): + params = ("first", "second") + self.directive.set_parameters(params) + self.assertEqual(self.directive.primary.parameters, params) + self.assertEqual(self.directive.secondary.parameters, params) + + def test_set_parameters(self): + pparams = mock.MagicMock() + sparams = mock.MagicMock() + pparams.parameters = ("a", "b") + sparams.parameters = ("a", "b") + self.directive.primary.set_parameters = pparams + self.directive.secondary.set_parameters = sparams + self.directive.set_parameters(("param", "seq")) + self.assertTrue(pparams.called) + self.assertTrue(sparams.called) + + def test_getattr_equality(self): + self.directive.primary.variableexception = "value" + self.directive.secondary.variableexception = "not_value" + with self.assertRaises(AssertionError): + _ = self.directive.variableexception + + self.directive.primary.variable = "value" + self.directive.secondary.variable = "value" + try: + self.directive.variable + except AssertionError: # pragma: no cover + self.fail("getattr check raised an AssertionError where it shouldn't have") + + def test_parsernode_dirty_assert(self): + # disable assertion pass + self.comment.primary.comment = "value" + self.comment.secondary.comment = "value" + self.comment.primary.filepath = "x" + self.comment.secondary.filepath = "x" + + self.comment.primary.dirty = False + self.comment.secondary.dirty = True + with self.assertRaises(AssertionError): + assertions.assertEqual(self.comment.primary, self.comment.secondary) + + def test_parsernode_filepath_assert(self): + # disable assertion pass + self.comment.primary.comment = "value" + self.comment.secondary.comment = "value" + + self.comment.primary.filepath = "first" + self.comment.secondary.filepath = "second" + with self.assertRaises(AssertionError): + assertions.assertEqual(self.comment.primary, self.comment.secondary) From c22434033033d7268c8e57f9f37998d3e01ce298 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 20 Sep 2019 00:44:50 +0300 Subject: [PATCH 05/23] [Apache v2] DualParserNode implementation 2/3 (#7375) * DualParserNode, DualCommentNode and DualDirectiveNode implementations * Add DualBlockNode * Address review comments * Address review comments * Call the right assertion after name change * Simplify isPass * Add explanation to _create_matching_list pydoc * Break when match was found --- certbot-apache/certbot_apache/augeasparser.py | 67 ++++++++- certbot-apache/certbot_apache/dualparser.py | 138 ++++++++++++++++++ .../certbot_apache/tests/dualnode_test.py | 75 +++++++++- 3 files changed, 275 insertions(+), 5 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index b7ac5ec3d..f396ab76a 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -1,4 +1,4 @@ -""" Augeas implementation of the ParserNode interface """ +""" Augeas implementation of the ParserNode interfaces """ from certbot_apache import assertions from certbot_apache import interfaces from certbot_apache import parsernode_util as util @@ -65,5 +65,70 @@ class AugeasDirectiveNode(AugeasParserNode): self.parameters = parameters +class AugeasBlockNode(AugeasDirectiveNode): + """ Augeas implementation of BlockNode interface """ + + def __init__(self, **kwargs): + super(AugeasBlockNode, self).__init__(**kwargs) + self.children = () + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.name == other.name and + self.filepath == other.filepath and + self.parameters == other.parameters and + self.children == other.children and + self.enabled == other.enabled and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata) + return False + + def add_child_block(self, name, parameters=None, position=None): # pylint: disable=unused-argument + """Adds a new BlockNode to the sequence of children""" + new_block = AugeasBlockNode(name=assertions.PASS, + ancestor=self, + filepath=assertions.PASS) + self.children += (new_block,) + return new_block + + def add_child_directive(self, name, parameters=None, position=None): # pylint: disable=unused-argument + """Adds a new DirectiveNode to the sequence of children""" + new_dir = AugeasDirectiveNode(name=assertions.PASS, + ancestor=self, + filepath=assertions.PASS) + self.children += (new_dir,) + return new_dir + + def add_child_comment(self, comment="", position=None): # pylint: disable=unused-argument + """Adds a new CommentNode to the sequence of children""" + new_comment = AugeasCommentNode(comment=assertions.PASS, + ancestor=self, + filepath=assertions.PASS) + self.children += (new_comment,) + return new_comment + + def find_blocks(self, name, exclude=True): # pragma: no cover + """Recursive search of BlockNodes from the sequence of children""" + pass + + def find_directives(self, name, exclude=True): # pragma: no cover + """Recursive search of DirectiveNodes from the sequence of children""" + pass + + def find_comments(self, comment, exact=False): # pragma: no cover + """Recursive search of DirectiveNodes from the sequence of children""" + pass + + def delete_child(self, child): # pragma: no cover + """Deletes a ParserNode from the sequence of children""" + pass + + def unsaved_files(self): # pragma: no cover + """Returns a list of unsaved filepaths""" + return [assertions.PASS] + + interfaces.CommentNode.register(AugeasCommentNode) interfaces.DirectiveNode.register(AugeasDirectiveNode) +interfaces.BlockNode.register(AugeasBlockNode) diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index 4bb4b3497..5f00d59f9 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -93,3 +93,141 @@ class DualDirectiveNode(DualNodeBase): self.primary.set_parameters(parameters) self.secondary.set_parameters(parameters) assertions.assertEqual(self.primary, self.secondary) + +class DualBlockNode(DualNodeBase): + """ Dual parser implementation of BlockNode interface """ + + def __init__(self, **kwargs): + """ This initialization implementation allows ordinary initialization + of BlockNode objects as well as creating a DualBlockNode object + using precreated or fetched BlockNode objects if provided as optional + arguments primary and secondary. + + Parameters other than the following are from interfaces.BlockNode: + + :param BlockNode primary: Primary pre-created BlockNode, mainly + used when creating new DualParser nodes using add_* methods. + :param BlockNode secondary: Secondary pre-created BlockNode + """ + + kwargs.setdefault("primary", None) + kwargs.setdefault("secondary", None) + primary = kwargs.pop("primary") + secondary = kwargs.pop("secondary") + + if primary or secondary: + assert primary and secondary + self.primary = primary + self.secondary = secondary + else: + self.primary = augeasparser.AugeasBlockNode(**kwargs) + self.secondary = augeasparser.AugeasBlockNode(**kwargs) + + assertions.assertEqual(self.primary, self.secondary) + + def add_child_block(self, name, parameters=None, position=None): + """ Creates a new child BlockNode, asserts that both implementations + did it in a similar way, and returns a newly created DualBlockNode object + encapsulating both of the newly created objects """ + + primary_new = self.primary.add_child_block(name, parameters, position) + secondary_new = self.secondary.add_child_block(name, parameters, position) + assertions.assertEqual(primary_new, secondary_new) + new_block = DualBlockNode(primary=primary_new, secondary=secondary_new) + return new_block + + def add_child_directive(self, name, parameters=None, position=None): + """ Creates a new child DirectiveNode, asserts that both implementations + did it in a similar way, and returns a newly created DualDirectiveNode + object encapsulating both of the newly created objects """ + + primary_new = self.primary.add_child_directive(name, parameters, position) + secondary_new = self.secondary.add_child_directive(name, parameters, position) + assertions.assertEqual(primary_new, secondary_new) + new_dir = DualDirectiveNode(primary=primary_new, secondary=secondary_new) + return new_dir + + def add_child_comment(self, comment="", position=None): + """ Creates a new child CommentNode, asserts that both implementations + did it in a similar way, and returns a newly created DualCommentNode + object encapsulating both of the newly created objects """ + + primary_new = self.primary.add_child_comment(comment, position) + secondary_new = self.secondary.add_child_comment(comment, position) + assertions.assertEqual(primary_new, secondary_new) + new_comment = DualCommentNode(primary=primary_new, secondary=secondary_new) + return new_comment + + def _create_matching_list(self, primary_list, secondary_list): # pragma: no cover + """ Matches the list of primary_list to a list of secondary_list and + returns a list of tuples. This is used to create results for find_ + methods. + + This helper function exists, because we cannot ensure that the list of + search results returned by primary.find_* and secondary.find_* are ordered + in a same way. The function pairs the same search results from both + implementations to a list of tuples. + """ + + matched = list() + for p in primary_list: + match = None + for s in secondary_list: + try: + assertions.assertEqual(p, s) + match = s + break + except AssertionError: + continue + if match: + matched.append((p, match)) + else: + raise AssertionError("Could not find a matching node.") + return matched + + def find_blocks(self, name, exclude=True): # pragma: no cover + """ + Performs a search for BlockNodes using both implementations and does simple + checks for results. This is built upon the assumption that unimplemented + find_* methods return a list with a single assertion passing object. + After the assertion, it creates a list of newly created DualBlockNode + instances that encapsulate the pairs of returned BlockNode objects. + """ + pass + + def find_directives(self, name, exclude=True): # pragma: no cover + """ + Performs a search for DirectiveNodes using both implementations and + checks the results. This is built upon the assumption that unimplemented + find_* methods return a list with a single assertion passing object. + After the assertion, it creates a list of newly created DualDirectiveNode + instances that encapsulate the pairs of returned DirectiveNode objects. + """ + pass + + def find_comments(self, comment, exact=False): # pragma: no cover + """ + Performs a search for CommentNodes using both implementations and + checks the results. This is built upon the assumption that unimplemented + find_* methods return a list with a single assertion passing object. + After the assertion, it creates a list of newly created DualCommentNode + instances that encapsulate the pairs of returned CommentNode objects. + """ + pass + + def delete_child(self, child): # pragma: no cover + """Deletes a child from the ParserNode implementations. The actual + ParserNode implementations are used here directly in order to be able + to match a child to the list of children.""" + + self.primary.delete_child(child.primary) + self.secondary.delete_child(child.secondary) + + def unsaved_files(self): + """ Fetches the list of unsaved file paths and asserts that the lists + match """ + primary_files = self.primary.unsaved_files() + secondary_files = self.secondary.unsaved_files() + assertions.assertEqualSimple(primary_files, secondary_files) + + return primary_files diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index f8a012280..97f58d6c9 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -5,35 +5,49 @@ import mock from certbot_apache import assertions from certbot_apache import dualparser +from certbot_apache import interfaces class DualParserNodeTest(unittest.TestCase): """DualParserNode tests""" def setUp(self): # pylint: disable=arguments-differ + self.block = dualparser.DualBlockNode(name="block", + ancestor=None, + filepath="/tmp/something") + self.block_two = dualparser.DualBlockNode(name="block", + ancestor=self.block, + filepath="/tmp/something") self.directive = dualparser.DualDirectiveNode(name="directive", - ancestor=None, + ancestor=self.block, filepath="/tmp/something") self.comment = dualparser.DualCommentNode(comment="comment", - ancestor=None, + ancestor=self.block, filepath="/tmp/something") def test_create_with_precreated(self): cnode = dualparser.DualCommentNode(comment="comment", - ancestor=None, + ancestor=self.block, filepath="/tmp/something", primary=self.comment.secondary, secondary=self.comment.primary) dnode = dualparser.DualDirectiveNode(name="directive", - ancestor=None, + ancestor=self.block, filepath="/tmp/something", primary=self.directive.secondary, secondary=self.directive.primary) + bnode = dualparser.DualBlockNode(name="block", + ancestor=self.block, + filepath="/tmp/something", + primary=self.block.secondary, + secondary=self.block.primary) # Switched around self.assertTrue(cnode.primary is self.comment.secondary) self.assertTrue(cnode.secondary is self.comment.primary) self.assertTrue(dnode.primary is self.directive.secondary) self.assertTrue(dnode.secondary is self.directive.primary) + self.assertTrue(bnode.primary is self.block.secondary) + self.assertTrue(bnode.secondary is self.block.primary) def test_set_params(self): params = ("first", "second") @@ -52,6 +66,26 @@ class DualParserNodeTest(unittest.TestCase): self.assertTrue(pparams.called) self.assertTrue(sparams.called) + def test_delete_child(self): + pdel = mock.MagicMock() + sdel = mock.MagicMock() + self.block.primary.delete_child = pdel + self.block.secondary.delete_child = sdel + self.block.delete_child(self.comment) + self.assertTrue(pdel.called) + self.assertTrue(sdel.called) + + def test_unsaved_files(self): + puns = mock.MagicMock() + suns = mock.MagicMock() + puns.return_value = assertions.PASS + suns.return_value = assertions.PASS + self.block.primary.unsaved_files = puns + self.block.secondary.unsaved_files = suns + self.block.unsaved_files() + self.assertTrue(puns.called) + self.assertTrue(suns.called) + def test_getattr_equality(self): self.directive.primary.variableexception = "value" self.directive.secondary.variableexception = "not_value" @@ -86,3 +120,36 @@ class DualParserNodeTest(unittest.TestCase): self.comment.secondary.filepath = "second" with self.assertRaises(AssertionError): 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) + 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) + + def test_add_child_directive(self): + self.assertEqual(len(self.block.primary.children), 0) + self.assertEqual(len(self.block.secondary.children), 0) + 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) + + def test_add_child_comment(self): + self.assertEqual(len(self.block.primary.children), 0) + self.assertEqual(len(self.block.secondary.children), 0) + self.block.add_child_comment("Comment") + 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.CommentNode)) + self.assertEqual(self.block.primary.children[0].ancestor, + self.block.primary) From feacbe9671d0514afd87fe098d9a4de4241ef497 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 23 Sep 2019 23:27:48 +0300 Subject: [PATCH 06/23] [Apache v2] DualParserNode implementation 3/3 (#7376) * DualParserNode, DualCommentNode and DualDirectiveNode implementations * Add DualBlockNode * DualBlockNode find_ methods * Address review comments * Address review comments * Simplify isPass * Add explanation to _create_matching_list pydoc * Remove unnecessary conditional block * Address review comments --- certbot-apache/certbot_apache/assertions.py | 43 +++ certbot-apache/certbot_apache/augeasparser.py | 29 +- certbot-apache/certbot_apache/dualparser.py | 62 ++++- .../certbot_apache/tests/dualnode_test.py | 248 +++++++++++++++++- 4 files changed, 361 insertions(+), 21 deletions(-) diff --git a/certbot-apache/certbot_apache/assertions.py b/certbot-apache/certbot_apache/assertions.py index 659efee28..fc2b35e14 100644 --- a/certbot-apache/certbot_apache/assertions.py +++ b/certbot-apache/certbot_apache/assertions.py @@ -13,6 +13,11 @@ def assertEqual(first, second): elif isinstance(first, interfaces.DirectiveNode): assertEqualDirective(first, second) + # Do an extra interface implementation assertion, as the contents were + # already checked for BlockNode in the assertEqualDirective + if isinstance(first, interfaces.BlockNode): + assert isinstance(second, interfaces.BlockNode) + # Skip tests if filepath includes the pass value. This is done # because filepath is variable of the base ParserNode interface, and # unless the implementation is actually done, we cannot assume getting @@ -55,6 +60,44 @@ def isPass(value): # pragma: no cover """Checks if the value is set to PASS""" return PASS in value +def isPassDirective(block): + """ Checks if BlockNode or DirectiveNode should pass the assertion """ + + if isPass(block.name): + return True + if isPass(block.parameters): # pragma: no cover + return True + if isPass(block.filepath): # pragma: no cover + return True + return False + +def isPassComment(comment): + """ Checks if CommentNode should pass the assertion """ + + if isPass(comment.comment): + return True + if isPass(comment.filepath): # pragma: no cover + return True + return False + +def isPassNodeList(nodelist): # pragma: no cover + """ Checks if a ParserNode in the nodelist should pass the assertion, + this function is used for results of find_* methods. Unimplemented find_* + methods should return a sequence containing a single ParserNode instance + with assertion pass string.""" + + try: + node = nodelist[0] + except IndexError: + node = None + + if not node: # pragma: no cover + return False + + if isinstance(node, interfaces.DirectiveNode): + return isPassDirective(node) + return isPassComment(node) + def assertEqualSimple(first, second): """ Simple assertion """ if not isPass(first) and not isPass(second): diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index f396ab76a..9325de5de 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -11,8 +11,7 @@ class AugeasParserNode(interfaces.ParserNode): ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs) # pylint: disable=unused-variable super(AugeasParserNode, self).__init__(**kwargs) self.ancestor = ancestor - # self.filepath = filepath - self.filepath = assertions.PASS + self.filepath = filepath self.dirty = dirty self.metadata = metadata @@ -27,9 +26,9 @@ class AugeasCommentNode(AugeasParserNode): comment, kwargs = util.commentnode_kwargs(kwargs) # pylint: disable=unused-variable super(AugeasCommentNode, self).__init__(**kwargs) # self.comment = comment - self.comment = assertions.PASS + self.comment = comment - def __eq__(self, other): # pragma: no cover + def __eq__(self, other): if isinstance(other, self.__class__): return (self.comment == other.comment and self.filepath == other.filepath and @@ -49,7 +48,7 @@ class AugeasDirectiveNode(AugeasParserNode): self.parameters = parameters self.enabled = enabled - def __eq__(self, other): # pragma: no cover + def __eq__(self, other): if isinstance(other, self.__class__): return (self.name == other.name and self.filepath == other.filepath and @@ -72,7 +71,7 @@ class AugeasBlockNode(AugeasDirectiveNode): super(AugeasBlockNode, self).__init__(**kwargs) self.children = () - def __eq__(self, other): # pragma: no cover + def __eq__(self, other): if isinstance(other, self.__class__): return (self.name == other.name and self.filepath == other.filepath and @@ -108,17 +107,23 @@ class AugeasBlockNode(AugeasDirectiveNode): self.children += (new_comment,) return new_comment - def find_blocks(self, name, exclude=True): # pragma: no cover + def find_blocks(self, name, exclude=True): # pylint: disable=unused-argument """Recursive search of BlockNodes from the sequence of children""" - pass + return [AugeasBlockNode(name=assertions.PASS, + ancestor=self, + filepath=assertions.PASS)] - def find_directives(self, name, exclude=True): # pragma: no cover + def find_directives(self, name, exclude=True): # pylint: disable=unused-argument """Recursive search of DirectiveNodes from the sequence of children""" - pass + return [AugeasDirectiveNode(name=assertions.PASS, + ancestor=self, + filepath=assertions.PASS)] - def find_comments(self, comment, exact=False): # pragma: no cover + def find_comments(self, comment, exact=False): # pylint: disable=unused-argument """Recursive search of DirectiveNodes from the sequence of children""" - pass + return [AugeasCommentNode(comment=assertions.PASS, + ancestor=self, + filepath=assertions.PASS)] def delete_child(self, child): # pragma: no cover """Deletes a ParserNode from the sequence of children""" diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index 5f00d59f9..5fa337a45 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -94,6 +94,7 @@ class DualDirectiveNode(DualNodeBase): self.secondary.set_parameters(parameters) assertions.assertEqual(self.primary, self.secondary) + class DualBlockNode(DualNodeBase): """ Dual parser implementation of BlockNode interface """ @@ -158,7 +159,7 @@ class DualBlockNode(DualNodeBase): new_comment = DualCommentNode(primary=primary_new, secondary=secondary_new) return new_comment - def _create_matching_list(self, primary_list, secondary_list): # pragma: no cover + def _create_matching_list(self, primary_list, secondary_list): """ Matches the list of primary_list to a list of secondary_list and returns a list of tuples. This is used to create results for find_ methods. @@ -185,7 +186,7 @@ class DualBlockNode(DualNodeBase): raise AssertionError("Could not find a matching node.") return matched - def find_blocks(self, name, exclude=True): # pragma: no cover + def find_blocks(self, name, exclude=True): """ Performs a search for BlockNodes using both implementations and does simple checks for results. This is built upon the assumption that unimplemented @@ -193,9 +194,11 @@ class DualBlockNode(DualNodeBase): After the assertion, it creates a list of newly created DualBlockNode instances that encapsulate the pairs of returned BlockNode objects. """ - pass - def find_directives(self, name, exclude=True): # pragma: no cover + return self._find_helper(DualBlockNode, "find_blocks", name, + exclude=exclude) + + def find_directives(self, name, exclude=True): """ Performs a search for DirectiveNodes using both implementations and checks the results. This is built upon the assumption that unimplemented @@ -203,9 +206,11 @@ class DualBlockNode(DualNodeBase): After the assertion, it creates a list of newly created DualDirectiveNode instances that encapsulate the pairs of returned DirectiveNode objects. """ - pass - def find_comments(self, comment, exact=False): # pragma: no cover + return self._find_helper(DualDirectiveNode, "find_directives", name, + exclude=exclude) + + def find_comments(self, comment, exact=False): """ Performs a search for CommentNodes using both implementations and checks the results. This is built upon the assumption that unimplemented @@ -213,9 +218,50 @@ class DualBlockNode(DualNodeBase): After the assertion, it creates a list of newly created DualCommentNode instances that encapsulate the pairs of returned CommentNode objects. """ - pass - def delete_child(self, child): # pragma: no cover + return self._find_helper(DualCommentNode, "find_comments", comment, + exact=exact) + + def _find_helper(self, nodeclass, findfunc, search, **kwargs): + """A helper for find_* functions. The function specific attributes should + be passed as keyword arguments. + + :param interfaces.ParserNode nodeclass: The node class for results. + :param str findfunc: Name of the find function to call + :param str search: The search term + """ + + primary_res = getattr(self.primary, findfunc)(search, **kwargs) + secondary_res = getattr(self.secondary, findfunc)(search, **kwargs) + + # The order of search results for Augeas implementation cannot be + # assured. + + pass_primary = assertions.isPassNodeList(primary_res) + pass_secondary = assertions.isPassNodeList(secondary_res) + new_nodes = list() + + if pass_primary and pass_secondary: + # Both unimplemented + new_nodes.append(nodeclass(primary=primary_res[0], + secondary=secondary_res[0])) + elif pass_primary: + for c in secondary_res: + new_nodes.append(nodeclass(primary=primary_res[0], + secondary=c)) + elif pass_secondary: + for c in primary_res: + new_nodes.append(nodeclass(primary=c, + secondary=secondary_res[0])) + else: + assert len(primary_res) == len(secondary_res) + matches = self._create_matching_list(primary_res, secondary_res) + for p, s in matches: + new_nodes.append(nodeclass(primary=p, secondary=s)) + + return new_nodes + + def delete_child(self, child): """Deletes a child from the ParserNode implementations. The actual ParserNode implementations are used here directly in order to be able to match a child to the list of children.""" diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index 97f58d6c9..eeea7d151 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -4,11 +4,12 @@ import unittest import mock from certbot_apache import assertions +from certbot_apache import augeasparser from certbot_apache import dualparser from certbot_apache import interfaces -class DualParserNodeTest(unittest.TestCase): +class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public-methods """DualParserNode tests""" def setUp(self): # pylint: disable=arguments-differ @@ -153,3 +154,248 @@ class DualParserNodeTest(unittest.TestCase): interfaces.CommentNode)) self.assertEqual(self.block.primary.children[0].ancestor, self.block.primary) + + def test_find_blocks(self): + dblks = self.block.find_blocks("block") + p_dblks = [d.primary for d in dblks] + s_dblks = [d.secondary for d in dblks] + p_blks = self.block.primary.find_blocks("block") + s_blks = self.block.secondary.find_blocks("block") + # Check that every block response is represented in the list of + # DualParserNode instances. + for p in p_dblks: + self.assertTrue(p in p_blks) + for s in s_dblks: + self.assertTrue(s in s_blks) + + def test_find_directives(self): + ddirs = self.block.find_directives("directive") + p_ddirs = [d.primary for d in ddirs] + s_ddirs = [d.secondary for d in ddirs] + p_dirs = self.block.primary.find_directives("directive") + s_dirs = self.block.secondary.find_directives("directive") + # Check that every directive response is represented in the list of + # DualParserNode instances. + for p in p_ddirs: + self.assertTrue(p in p_dirs) + for s in s_ddirs: + self.assertTrue(s in s_dirs) + + def test_find_comments(self): + dcoms = self.block.find_comments("comment") + p_dcoms = [d.primary for d in dcoms] + s_dcoms = [d.secondary for d in dcoms] + p_coms = self.block.primary.find_comments("comment") + s_coms = self.block.secondary.find_comments("comment") + # Check that every comment response is represented in the list of + # DualParserNode instances. + for p in p_dcoms: + self.assertTrue(p in p_coms) + for s in s_dcoms: + self.assertTrue(s in s_coms) + + def test_find_blocks_first_passing(self): + youshallnotpass = [augeasparser.AugeasBlockNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + youshallpass = [augeasparser.AugeasBlockNode(name=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_blocks_primary = mock.MagicMock(return_value=youshallpass) + find_blocks_secondary = mock.MagicMock(return_value=youshallnotpass) + self.block.primary.find_blocks = find_blocks_primary + self.block.secondary.find_blocks = find_blocks_secondary + + blocks = self.block.find_blocks("something") + for block in blocks: + try: + assertions.assertEqual(block.primary, block.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertTrue(assertions.isPassDirective(block.primary)) + self.assertFalse(assertions.isPassDirective(block.secondary)) + + def test_find_blocks_second_passing(self): + youshallnotpass = [augeasparser.AugeasBlockNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + youshallpass = [augeasparser.AugeasBlockNode(name=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_blocks_primary = mock.MagicMock(return_value=youshallnotpass) + find_blocks_secondary = mock.MagicMock(return_value=youshallpass) + self.block.primary.find_blocks = find_blocks_primary + self.block.secondary.find_blocks = find_blocks_secondary + + blocks = self.block.find_blocks("something") + for block in blocks: + try: + assertions.assertEqual(block.primary, block.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertFalse(assertions.isPassDirective(block.primary)) + self.assertTrue(assertions.isPassDirective(block.secondary)) + + def test_find_dirs_first_passing(self): + notpassing = [augeasparser.AugeasDirectiveNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + passing = [augeasparser.AugeasDirectiveNode(name=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_dirs_primary = mock.MagicMock(return_value=passing) + find_dirs_secondary = mock.MagicMock(return_value=notpassing) + self.block.primary.find_directives = find_dirs_primary + self.block.secondary.find_directives = find_dirs_secondary + + directives = self.block.find_directives("something") + for directive in directives: + try: + assertions.assertEqual(directive.primary, directive.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertTrue(assertions.isPassDirective(directive.primary)) + self.assertFalse(assertions.isPassDirective(directive.secondary)) + + def test_find_dirs_second_passing(self): + notpassing = [augeasparser.AugeasDirectiveNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + passing = [augeasparser.AugeasDirectiveNode(name=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_dirs_primary = mock.MagicMock(return_value=notpassing) + find_dirs_secondary = mock.MagicMock(return_value=passing) + self.block.primary.find_directives = find_dirs_primary + self.block.secondary.find_directives = find_dirs_secondary + + directives = self.block.find_directives("something") + for directive in directives: + try: + assertions.assertEqual(directive.primary, directive.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertFalse(assertions.isPassDirective(directive.primary)) + self.assertTrue(assertions.isPassDirective(directive.secondary)) + + def test_find_coms_first_passing(self): + notpassing = [augeasparser.AugeasCommentNode(comment="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + passing = [augeasparser.AugeasCommentNode(comment=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_coms_primary = mock.MagicMock(return_value=passing) + find_coms_secondary = mock.MagicMock(return_value=notpassing) + self.block.primary.find_comments = find_coms_primary + self.block.secondary.find_comments = find_coms_secondary + + comments = self.block.find_comments("something") + for comment in comments: + try: + assertions.assertEqual(comment.primary, comment.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertTrue(assertions.isPassComment(comment.primary)) + self.assertFalse(assertions.isPassComment(comment.secondary)) + + def test_find_coms_second_passing(self): + notpassing = [augeasparser.AugeasCommentNode(comment="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + passing = [augeasparser.AugeasCommentNode(comment=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_coms_primary = mock.MagicMock(return_value=notpassing) + find_coms_secondary = mock.MagicMock(return_value=passing) + self.block.primary.find_comments = find_coms_primary + self.block.secondary.find_comments = find_coms_secondary + + comments = self.block.find_comments("something") + for comment in comments: + try: + assertions.assertEqual(comment.primary, comment.secondary) + except AssertionError: # pragma: no cover + self.fail("Assertion should have passed") + self.assertFalse(assertions.isPassComment(comment.primary)) + self.assertTrue(assertions.isPassComment(comment.secondary)) + + def test_find_blocks_no_pass_equal(self): + notpassing1 = [augeasparser.AugeasBlockNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + notpassing2 = [augeasparser.AugeasBlockNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + find_blocks_primary = mock.MagicMock(return_value=notpassing1) + find_blocks_secondary = mock.MagicMock(return_value=notpassing2) + self.block.primary.find_blocks = find_blocks_primary + self.block.secondary.find_blocks = find_blocks_secondary + + blocks = self.block.find_blocks("anything") + for block in blocks: + self.assertEqual(block.primary, block.secondary) + self.assertTrue(block.primary is not block.secondary) + + def test_find_dirs_no_pass_equal(self): + notpassing1 = [augeasparser.AugeasDirectiveNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + notpassing2 = [augeasparser.AugeasDirectiveNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + find_dirs_primary = mock.MagicMock(return_value=notpassing1) + find_dirs_secondary = mock.MagicMock(return_value=notpassing2) + self.block.primary.find_directives = find_dirs_primary + self.block.secondary.find_directives = find_dirs_secondary + + directives = self.block.find_directives("anything") + for directive in directives: + self.assertEqual(directive.primary, directive.secondary) + self.assertTrue(directive.primary is not directive.secondary) + + def test_find_comments_no_pass_equal(self): + notpassing1 = [augeasparser.AugeasCommentNode(comment="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + notpassing2 = [augeasparser.AugeasCommentNode(comment="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + find_coms_primary = mock.MagicMock(return_value=notpassing1) + find_coms_secondary = mock.MagicMock(return_value=notpassing2) + self.block.primary.find_comments = find_coms_primary + self.block.secondary.find_comments = find_coms_secondary + + comments = self.block.find_comments("anything") + for comment in comments: + self.assertEqual(comment.primary, comment.secondary) + self.assertTrue(comment.primary is not comment.secondary) + + def test_find_blocks_no_pass_notequal(self): + notpassing1 = [augeasparser.AugeasBlockNode(name="notpassing", + ancestor=self.block, + filepath="/path/to/whatever")] + notpassing2 = [augeasparser.AugeasBlockNode(name="different", + ancestor=self.block, + filepath="/path/to/whatever")] + find_blocks_primary = mock.MagicMock(return_value=notpassing1) + find_blocks_secondary = mock.MagicMock(return_value=notpassing2) + self.block.primary.find_blocks = find_blocks_primary + self.block.secondary.find_blocks = find_blocks_secondary + + with self.assertRaises(AssertionError): + _ = self.block.find_blocks("anything") + + def test_parsernode_notequal(self): + ne_block = augeasparser.AugeasBlockNode(name="different", + ancestor=self.block, + filepath="/path/to/whatever") + ne_directive = augeasparser.AugeasDirectiveNode(name="different", + ancestor=self.block, + filepath="/path/to/whatever") + ne_comment = augeasparser.AugeasCommentNode(comment="different", + ancestor=self.block, + filepath="/path/to/whatever") + self.assertFalse(self.block == ne_block) + self.assertFalse(self.directive == ne_directive) + self.assertFalse(self.comment == ne_comment) From 3f36298716e87131e622d82d8e15e4f896357a96 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 21 Oct 2019 22:52:00 +0300 Subject: [PATCH 07/23] [Apache v2] find_blocks and find_directives implementation (#7443) * Implement AugeasDirectiveNode and AugeasBlockNode find and create functions * Add tests for _aug_get_block_name --- certbot-apache/certbot_apache/augeasparser.py | 119 ++++++++++++++++-- certbot-apache/certbot_apache/configurator.py | 16 +++ certbot-apache/certbot_apache/dualparser.py | 3 +- certbot-apache/certbot_apache/parser.py | 4 +- .../certbot_apache/tests/augeasnode_test.py | 65 ++++++++++ .../certbot_apache/tests/dualnode_test.py | 39 ++---- 6 files changed, 203 insertions(+), 43 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/augeasnode_test.py diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 9325de5de..1bbf1ed77 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -1,8 +1,14 @@ """ Augeas implementation of the ParserNode interfaces """ + +from certbot_apache import apache_util from certbot_apache import assertions from certbot_apache import interfaces +from certbot_apache import parser from certbot_apache import parsernode_util as util +from certbot.compat import os +from acme.magic_typing import Set # pylint: disable=unused-import, no-name-in-module + class AugeasParserNode(interfaces.ParserNode): """ Augeas implementation of ParserNode interface """ @@ -14,6 +20,7 @@ class AugeasParserNode(interfaces.ParserNode): self.filepath = filepath self.dirty = dirty self.metadata = metadata + self.parser = self.metadata.get("augeasparser") def save(self, msg): # pragma: no cover pass @@ -85,45 +92,72 @@ class AugeasBlockNode(AugeasDirectiveNode): def add_child_block(self, name, parameters=None, position=None): # pylint: disable=unused-argument """Adds a new BlockNode to the sequence of children""" + new_metadata = {"augeasparser": self.parser} new_block = AugeasBlockNode(name=assertions.PASS, ancestor=self, - filepath=assertions.PASS) + filepath=assertions.PASS, + metadata=new_metadata) self.children += (new_block,) return new_block def add_child_directive(self, name, parameters=None, position=None): # pylint: disable=unused-argument """Adds a new DirectiveNode to the sequence of children""" + new_metadata = {"augeasparser": self.parser} new_dir = AugeasDirectiveNode(name=assertions.PASS, ancestor=self, - filepath=assertions.PASS) + filepath=assertions.PASS, + metadata=new_metadata) self.children += (new_dir,) return new_dir 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_comment = AugeasCommentNode(comment=assertions.PASS, ancestor=self, - filepath=assertions.PASS) + filepath=assertions.PASS, + metadata=new_metadata) self.children += (new_comment,) return new_comment def find_blocks(self, name, exclude=True): # pylint: disable=unused-argument """Recursive search of BlockNodes from the sequence of children""" - return [AugeasBlockNode(name=assertions.PASS, - ancestor=self, - filepath=assertions.PASS)] + + nodes = list() + paths = self._aug_find_blocks(name) + if exclude: + paths = self.parser.exclude_dirs(paths) + for path in paths: + nodes.append(self._create_blocknode(path)) + + return nodes def find_directives(self, name, exclude=True): # pylint: disable=unused-argument """Recursive search of DirectiveNodes from the sequence of children""" - return [AugeasDirectiveNode(name=assertions.PASS, - ancestor=self, - filepath=assertions.PASS)] + + nodes = list() + ownpath = self.metadata.get("augeaspath") + + directives = self.parser.find_dir(name, start=ownpath, exclude=exclude) + already_parsed = set() # type: Set[str] + for directive in directives: + # Remove the /arg part from the Augeas path + directive = directive.partition("/arg")[0] + # find_dir returns an object for each _parameter_ of a directive + # so we need to filter out duplicates. + if directive not in already_parsed: + nodes.append(self._create_directivenode(directive)) + already_parsed.add(directive) + + return nodes def find_comments(self, comment, exact=False): # pylint: disable=unused-argument """Recursive search of DirectiveNodes from the sequence of children""" + new_metadata = {"augeasparser": self.parser} return [AugeasCommentNode(comment=assertions.PASS, ancestor=self, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=new_metadata)] def delete_child(self, child): # pragma: no cover """Deletes a ParserNode from the sequence of children""" @@ -133,6 +167,71 @@ class AugeasBlockNode(AugeasDirectiveNode): """Returns a list of unsaved filepaths""" return [assertions.PASS] + def _create_directivenode(self, path): + """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=self, + filepath=apache_util.get_file_path(path), + metadata=metadata) + + def _create_blocknode(self, path): + """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=self, + filepath=apache_util.get_file_path(path), + metadata=metadata) + + def _aug_find_blocks(self, name): + """Helper function to perform a search to Augeas DOM tree to search + configuration blocks with a given name""" + + # The code here is modified from configurator.get_virtual_hosts() + blk_paths = set() + for vhost_path in list(self.parser.parser_paths): + paths = self.parser.aug.match( + ("/files%s//*[label()=~regexp('%s')]" % + (vhost_path, parser.case_i(name)))) + blk_paths.update([path for path in paths if + 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.""" + + # Remove the ending slash if any + if path[-1] == "/": # pragma: no cover + path = path[:-1] + + # Get the block name + name = path.split("/")[-1] + + # remove [...], it's not allowed in Apache configuration and is used + # for indexing within Augeas + name = name.split("[")[0] + return name + interfaces.CommentNode.register(AugeasCommentNode) interfaces.DirectiveNode.register(AugeasDirectiveNode) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index f7c27bf76..d01b0f5ce 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -30,8 +30,10 @@ from certbot.plugins.util import path_surgery from certbot.plugins.enhancements import AutoHSTSEnhancement from certbot_apache import apache_util +from certbot_apache import assertions from certbot_apache import constants from certbot_apache import display_ops +from certbot_apache import dualparser from certbot_apache import http_01 from certbot_apache import obj from certbot_apache import parser @@ -204,6 +206,7 @@ class ApacheConfigurator(common.Installer): # These will be set in the prepare function self._prepared = False self.parser = None + self.parser_root = None self.version = version self.vhosts = None self.options = copy.deepcopy(self.OS_DEFAULTS) @@ -253,6 +256,10 @@ class ApacheConfigurator(common.Installer): # Perform the actual Augeas initialization to be able to react self.parser = self.get_parser() + # Set up ParserNode root + pn_meta = {"augeasparser": self.parser} + self.parser_root = self.get_parsernode_root(pn_meta) + # Check for errors in parsing files with Augeas self.parser.check_parsing_errors("httpd.aug") @@ -348,6 +355,15 @@ class ApacheConfigurator(common.Installer): self.option("server_root"), self.conf("vhost-root"), self.version, configurator=self) + def get_parsernode_root(self, metadata): + """Initializes the ParserNode parser root instance.""" + return dualparser.DualBlockNode( + name=assertions.PASS, + ancestor=None, + filepath=assertions.PASS, + metadata=metadata + ) + def _wildcard_domain(self, domain): """ Checks if domain is a wildcard domain diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index 5fa337a45..d56e07de2 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -17,7 +17,8 @@ class DualNodeBase(object): """ Attribute value assertion """ firstval = getattr(self.primary, aname) secondval = getattr(self.secondary, aname) - assertions.assertEqualSimple(firstval, secondval) + if not callable(firstval): + assertions.assertEqualSimple(firstval, secondval) return firstval diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index b5f0cd81a..7ae9ac386 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -613,7 +613,7 @@ class ApacheParser(object): "%s//*[self::directive=~regexp('%s')]" % (start, regex)) if exclude: - matches = self._exclude_dirs(matches) + matches = self.exclude_dirs(matches) if arg is None: arg_suffix = "/arg" @@ -680,7 +680,7 @@ class ApacheParser(object): return value - def _exclude_dirs(self, matches): + 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 new file mode 100644 index 000000000..c4631c57c --- /dev/null +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -0,0 +1,65 @@ +"""Tests for AugeasParserNode classes""" +import mock + +from certbot_apache import assertions + +from certbot_apache.tests import util + + +class AugeasParserNodeTest(util.ApacheTest): + """Test AugeasParserNode using available test configurations""" + + def setUp(self): # pylint: disable=arguments-differ + super(AugeasParserNodeTest, self).setUp() + + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir) + self.vh_truth = util.get_vh_truth( + self.temp_dir, "debian_apache_2_4/multiple_vhosts") + + def test_get_block_node_name(self): + from certbot_apache.augeasparser import AugeasBlockNode + block = AugeasBlockNode( + name=assertions.PASS, + ancestor=None, + filepath=assertions.PASS, + metadata={"augeasparser": mock.Mock()} + ) + testcases = { + "/some/path/FirstNode/SecondNode": "SecondNode", + "/some/path/FirstNode/SecondNode/": "SecondNode", + "OnlyPathItem": "OnlyPathItem", + "/files/etc/apache2/apache2.conf/VirtualHost": "VirtualHost", + "/Anything": "Anything", + } + for test in testcases: + self.assertEqual(block._aug_get_block_name(test), testcases[test]) # pylint: disable=protected-access + + def test_find_blocks(self): + blocks = self.config.parser_root.find_blocks("VirtualHost", exclude=False) + self.assertEqual(len(blocks), 12) + + def test_find_blocks_case_insensitive(self): + vhs = self.config.parser_root.find_blocks("VirtualHost") + vhs2 = self.config.parser_root.find_blocks("viRtuAlHoST") + self.assertEqual(len(vhs), len(vhs2)) + + def test_find_directive_found(self): + directives = self.config.parser_root.find_directives("Listen") + self.assertEqual(len(directives), 1) + self.assertTrue(directives[0].filepath.endswith("/apache2/ports.conf")) + self.assertEqual(directives[0].parameters, (u'80',)) + + def test_find_directive_notfound(self): + directives = self.config.parser_root.find_directives("Nonexistent") + self.assertEqual(len(directives), 0) + + def test_find_directive_from_block(self): + blocks = self.config.parser_root.find_blocks("virtualhost") + found = False + for vh in blocks: + if vh.filepath.endswith("sites-enabled/certbot.conf"): + servername = vh.find_directives("servername") + self.assertEqual(servername[0].parameters[0], "certbot.demo") + 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 eeea7d151..b37a7fdf9 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -13,18 +13,23 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- """DualParserNode tests""" def setUp(self): # pylint: disable=arguments-differ + metadata = {"augeasparser": mock.Mock()} self.block = dualparser.DualBlockNode(name="block", ancestor=None, - filepath="/tmp/something") + filepath="/tmp/something", + metadata=metadata) self.block_two = dualparser.DualBlockNode(name="block", ancestor=self.block, - filepath="/tmp/something") + filepath="/tmp/something", + metadata=metadata) self.directive = dualparser.DualDirectiveNode(name="directive", ancestor=self.block, - filepath="/tmp/something") + filepath="/tmp/something", + metadata=metadata) self.comment = dualparser.DualCommentNode(comment="comment", ancestor=self.block, - filepath="/tmp/something") + filepath="/tmp/something", + metadata=metadata) def test_create_with_precreated(self): cnode = dualparser.DualCommentNode(comment="comment", @@ -155,32 +160,6 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- self.assertEqual(self.block.primary.children[0].ancestor, self.block.primary) - def test_find_blocks(self): - dblks = self.block.find_blocks("block") - p_dblks = [d.primary for d in dblks] - s_dblks = [d.secondary for d in dblks] - p_blks = self.block.primary.find_blocks("block") - s_blks = self.block.secondary.find_blocks("block") - # Check that every block response is represented in the list of - # DualParserNode instances. - for p in p_dblks: - self.assertTrue(p in p_blks) - for s in s_dblks: - self.assertTrue(s in s_blks) - - def test_find_directives(self): - ddirs = self.block.find_directives("directive") - p_ddirs = [d.primary for d in ddirs] - s_ddirs = [d.secondary for d in ddirs] - p_dirs = self.block.primary.find_directives("directive") - s_dirs = self.block.secondary.find_directives("directive") - # Check that every directive response is represented in the list of - # DualParserNode instances. - for p in p_ddirs: - self.assertTrue(p in p_dirs) - for s in s_ddirs: - self.assertTrue(s in s_dirs) - def test_find_comments(self): dcoms = self.block.find_comments("comment") p_dcoms = [d.primary for d in dcoms] From 9b2322a573876841064251c1f1155b96e28da962 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sat, 26 Oct 2019 00:54:28 +0300 Subject: [PATCH 08/23] Use dummy values for ancestor (#7462) --- certbot-apache/certbot_apache/augeasparser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 1bbf1ed77..ebbf4120f 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -178,7 +178,7 @@ class AugeasBlockNode(AugeasDirectiveNode): # the complete ParserNode tree, we use the search parent as ancestor return AugeasDirectiveNode(name=name, parameters=params, - ancestor=self, + ancestor=assertions.PASS, filepath=apache_util.get_file_path(path), metadata=metadata) @@ -193,7 +193,7 @@ class AugeasBlockNode(AugeasDirectiveNode): # the complete ParserNode tree, we use the search parent as ancestor return AugeasBlockNode(name=name, parameters=params, - ancestor=self, + ancestor=assertions.PASS, filepath=apache_util.get_file_path(path), metadata=metadata) From d645574839c5791a131d24d90df2b86d2643d9d5 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 30 Oct 2019 20:03:23 +0200 Subject: [PATCH 09/23] [Apache v2] AugeasBlockNode find_comments() implementation (#7457) * find_comments implementation and AugeasCommentNode creation * Use dummy value for ancestor * Add NotImplementedError when calling find_comments with exact parameter * Remove parameter 'exact' from find_comments interface * Fix comment --- certbot-apache/certbot_apache/augeasparser.py | 35 +++++++++++++++---- certbot-apache/certbot_apache/dualparser.py | 7 ++-- certbot-apache/certbot_apache/interfaces.py | 5 ++- .../certbot_apache/tests/augeasnode_test.py | 9 +++++ .../certbot_apache/tests/dualnode_test.py | 11 ++++++ 5 files changed, 53 insertions(+), 14 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index ebbf4120f..04286ef4d 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -151,13 +151,21 @@ class AugeasBlockNode(AugeasDirectiveNode): return nodes - def find_comments(self, comment, exact=False): # pylint: disable=unused-argument - """Recursive search of DirectiveNodes from the sequence of children""" - new_metadata = {"augeasparser": self.parser} - return [AugeasCommentNode(comment=assertions.PASS, - ancestor=self, - filepath=assertions.PASS, - metadata=new_metadata)] + def find_comments(self, comment): + """ + Recursive search of DirectiveNodes from the sequence of children. + + :param str comment: Comment content to search for. + """ + + nodes = list() + ownpath = self.metadata.get("augeaspath") + + comments = self.parser.find_comments(comment, start=ownpath) + for com in comments: + nodes.append(self._create_commentnode(com)) + + return nodes def delete_child(self, child): # pragma: no cover """Deletes a ParserNode from the sequence of children""" @@ -167,6 +175,19 @@ class AugeasBlockNode(AugeasDirectiveNode): """Returns a list of unsaved filepaths""" return [assertions.PASS] + def _create_commentnode(self, path): + """Helper function to create a CommentNode from Augeas path""" + + comment = self.parser.aug.get(path) + metadata = {"augeasparser": self.parser, "augeaspath": path} + + # Because of the dynamic nature of AugeasParser and the fact that we're + # not populating the complete node tree, the ancestor has a dummy value + return AugeasCommentNode(comment=comment, + ancestor=assertions.PASS, + filepath=apache_util.get_file_path(path), + metadata=metadata) + def _create_directivenode(self, path): """Helper function to create a DirectiveNode from Augeas path""" diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index d56e07de2..8897449b6 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -211,7 +211,7 @@ class DualBlockNode(DualNodeBase): return self._find_helper(DualDirectiveNode, "find_directives", name, exclude=exclude) - def find_comments(self, comment, exact=False): + def find_comments(self, comment): """ Performs a search for CommentNodes using both implementations and checks the results. This is built upon the assumption that unimplemented @@ -220,8 +220,7 @@ class DualBlockNode(DualNodeBase): instances that encapsulate the pairs of returned CommentNode objects. """ - return self._find_helper(DualCommentNode, "find_comments", comment, - exact=exact) + return self._find_helper(DualCommentNode, "find_comments", comment) def _find_helper(self, nodeclass, findfunc, search, **kwargs): """A helper for find_* functions. The function specific attributes should @@ -245,7 +244,7 @@ class DualBlockNode(DualNodeBase): if pass_primary and pass_secondary: # Both unimplemented new_nodes.append(nodeclass(primary=primary_res[0], - secondary=secondary_res[0])) + secondary=secondary_res[0])) # pragma: no cover elif pass_primary: for c in secondary_res: new_nodes.append(nodeclass(primary=primary_res[0], diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index cdfdfac91..ecad2d4eb 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -453,9 +453,9 @@ class BlockNode(DirectiveNode): """ @abc.abstractmethod - def find_comments(self, comment, exact=False): + def find_comments(self, comment): """ - Find comments with value containing or being exactly the same as search term. + Find comments with value containing the search term. This method walks the child tree of ParserNodes under the instance it was called from. This way it is possible to search for the whole configuration @@ -463,7 +463,6 @@ class BlockNode(DirectiveNode): from a specified branch. The lookup should be case sensitive. :param str comment: The content of comment to search for - :param bool exact: If the comment needs to exactly match the search term :returns: A list of found CommentNode objects. diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index c4631c57c..acb2d5329 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -63,3 +63,12 @@ class AugeasParserNodeTest(util.ApacheTest): self.assertEqual(servername[0].parameters[0], "certbot.demo") found = True self.assertTrue(found) + + def test_find_comments(self): + rootcomment = self.config.parser_root.find_comments( + "This is the main Apache server configuration file. " + ) + self.assertEqual(len(rootcomment), 1) + self.assertTrue(rootcomment[0].filepath.endswith( + "debian_apache_2_4/multiple_vhosts/apache2/apache2.conf" + )) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index b37a7fdf9..3101a9c6f 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -161,6 +161,17 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- self.block.primary) def test_find_comments(self): + pri_comments = [augeasparser.AugeasCommentNode(comment="some comment", + ancestor=self.block, + filepath="/path/to/whatever")] + sec_comments = [augeasparser.AugeasCommentNode(comment=assertions.PASS, + ancestor=self.block, + filepath=assertions.PASS)] + find_coms_primary = mock.MagicMock(return_value=pri_comments) + find_coms_secondary = mock.MagicMock(return_value=sec_comments) + self.block.primary.find_comments = find_coms_primary + self.block.secondary.find_comments = find_coms_secondary + dcoms = self.block.find_comments("comment") p_dcoms = [d.primary for d in dcoms] s_dcoms = [d.secondary for d in dcoms] From 19de05c72fdb3f2825f2f2b7074f5d5d1010c536 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 6 Nov 2019 20:33:24 +0200 Subject: [PATCH 10/23] [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 From 578ca1c6af600a53f68910667b9a6135b4b14e0c Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 11 Nov 2019 21:33:14 +0200 Subject: [PATCH 11/23] [Apache v2] Adding nodes 1/3 : add_child_block() (#7497) * Implement add_child_block() * Add comments and example * Check augas path inconsistencies in initialization --- certbot-apache/certbot_apache/augeasparser.py | 178 ++++++++++++++++-- .../certbot_apache/tests/augeasnode_test.py | 105 ++++++++++- .../certbot_apache/tests/configurator_test.py | 3 +- .../certbot_apache/tests/dualnode_test.py | 21 ++- 4 files changed, 286 insertions(+), 21 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 5f719f9a3..e340519ce 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -1,4 +1,72 @@ -""" Augeas implementation of the ParserNode interfaces """ +""" +Augeas implementation of the ParserNode interfaces. + +Augeas works internally by using XPATH notation. The following is a short example +of how this all works internally, to better understand what's going on under the +hood. + +A configuration file /etc/apache2/apache2.conf with the following content: + + # First comment line + # Second comment line + WhateverDirective whatevervalue + + DirectiveInABlock dirvalue + + SomeDirective somedirectivevalue + + AnotherDirectiveInABlock dirvalue + + # Yet another comment + + +Translates over to Augeas path notation (of immediate children), when calling +for example: aug.match("/files/etc/apache2/apache2.conf/*") + +[ + "/files/etc/apache2/apache2.conf/#comment[1]", + "/files/etc/apache2/apache2.conf/#comment[2]", + "/files/etc/apache2/apache2.conf/directive[1]", + "/files/etc/apache2/apache2.conf/ABlock[1]", + "/files/etc/apache2/apache2.conf/directive[2]", + "/files/etc/apache2/apache2.conf/ABlock[2]", + "/files/etc/apache2/apache2.conf/#comment[3]" +] + +Regardless of directives name, its key in the Augeas tree is always "directive", +with index where needed of course. Comments work similarly, while blocks +have their own key in the Augeas XPATH notation. + +It's important to note that all of the unique keys have their own indices. + +Augeas paths are case sensitive, while Apache configuration is case insensitive. +It looks like this: + + + directive value + + + Directive Value + + + directive value + + + DiReCtiVe VaLuE + + +Translates over to: + +[ + "/files/etc/apache2/apache2.conf/block[1]", + "/files/etc/apache2/apache2.conf/Block[1]", + "/files/etc/apache2/apache2.conf/block[2]", + "/files/etc/apache2/apache2.conf/bLoCk[1]", +] +""" +from acme.magic_typing import Set # pylint: disable=unused-import, no-name-in-module +from certbot import errors +from certbot.compat import os from certbot_apache import apache_util from certbot_apache import assertions @@ -6,8 +74,6 @@ from certbot_apache import interfaces from certbot_apache import parser from certbot_apache import parsernode_util as util -from certbot.compat import os -from acme.magic_typing import Set # pylint: disable=unused-import, no-name-in-module class AugeasParserNode(interfaces.ParserNode): @@ -21,6 +87,15 @@ class AugeasParserNode(interfaces.ParserNode): self.dirty = dirty self.metadata = metadata self.parser = self.metadata.get("augeasparser") + try: + if self.metadata["augeaspath"].endswith("/"): + raise errors.PluginError( + "Augeas path: {} has a trailing slash".format( + self.metadata["augeaspath"] + ) + ) + except KeyError: + raise errors.PluginError("Augeas path is required") def save(self, msg): # pragma: no cover pass @@ -125,12 +200,22 @@ class AugeasBlockNode(AugeasDirectiveNode): # 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, "augeaspath": assertions.PASS} - new_block = AugeasBlockNode(name=assertions.PASS, - ancestor=self, - filepath=assertions.PASS, + + insertpath, realpath, before = self._aug_resolve_child_position( + name, + position + ) + new_metadata = {"augeasparser": self.parser, "augeaspath": realpath} + + # Create the new block + self.parser.aug.insert(insertpath, name, before) + + # Parameters will be set at the initialization of the new object + new_block = AugeasBlockNode(name=name, + parameters=parameters, + ancestor=assertions.PASS, + filepath=apache_util.get_file_path(realpath), metadata=new_metadata) - self.children += (new_block,) return new_block # pylint: disable=unused-argument @@ -238,7 +323,7 @@ class AugeasBlockNode(AugeasDirectiveNode): def _create_blocknode(self, path): """Helper function to create a BlockNode from Augeas path""" - name = self._aug_get_block_name(path) + name = self._aug_get_name(path) metadata = {"augeasparser": self.parser, "augeaspath": path} # Because of the dynamic nature, and the fact that we're not populating @@ -262,8 +347,10 @@ class AugeasBlockNode(AugeasDirectiveNode): name.lower() in os.path.basename(path).lower()]) return blk_paths - def _aug_get_block_name(self, path): - """Helper function to get name of a configuration block from path.""" + def _aug_get_name(self, path): + """ + Helper function to get name of a configuration block or variable from path. + """ # Remove the ending slash if any if path[-1] == "/": # pragma: no cover @@ -277,6 +364,75 @@ class AugeasBlockNode(AugeasDirectiveNode): name = name.split("[")[0] return name + def _aug_resolve_child_position(self, name, position): + """ + Helper function that iterates through the immediate children and figures + out the insertion path for a new AugeasParserNode. + + Augeas also generalizes indices for directives and comments, simply by + using "directive" or "comment" respectively as their names. + + This function iterates over the existing children of the AugeasBlockNode, + returning their insertion path, resulting Augeas path and if the new node + should be inserted before or after the returned insertion path. + + Note: while Apache is case insensitive, Augeas is not, and blocks like + Nameofablock and NameOfABlock have different indices. + + :param str name: Name of the AugeasBlockNode to insert, "directive" for + AugeasDirectiveNode or "comment" for AugeasCommentNode + :param int position: The position to insert the child AugeasParserNode to + + :returns: Tuple of insert path, resulting path and a boolean if the new + node should be inserted before it. + :rtype: tuple of str, str, bool + """ + + # Default to appending + before = False + + all_children = self.parser.aug.match("{}/*".format( + self.metadata["augeaspath"]) + ) + + # Calculate resulting_path + # Augeas indices start at 1. We use counter to calculate the index to + # be used in resulting_path. + counter = 1 + for i, child in enumerate(all_children): + if position is not None and i >= position: + # We're not going to insert the new node to an index after this + break + childname = self._aug_get_name(child) + if name == childname: + counter += 1 + + resulting_path = "{}/{}[{}]".format( + self.metadata["augeaspath"], + name, + counter + ) + + # Form the correct insert_path + # Inserting the only child and appending as the last child work + # similarly in Augeas. + append = not all_children or position is None or position >= len(all_children) + if append: + insert_path = "{}/*[last()]".format( + self.metadata["augeaspath"] + ) + elif position == 0: + # Insert as the first child, before the current first one. + insert_path = all_children[0] + before = True + else: + insert_path = "{}/*[{}]".format( + self.metadata["augeaspath"], + position + ) + + return (insert_path, resulting_path, before) + interfaces.CommentNode.register(AugeasCommentNode) interfaces.DirectiveNode.register(AugeasDirectiveNode) diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index bbf9093d3..8846c3c07 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -2,6 +2,7 @@ import mock from acme.magic_typing import List # pylint: disable=unused-import, no-name-in-module +from certbot import errors from certbot_apache import assertions @@ -25,7 +26,7 @@ class AugeasParserNodeTest(util.ApacheTest): name=assertions.PASS, ancestor=None, filepath=assertions.PASS, - metadata={"augeasparser": mock.Mock()} + metadata={"augeasparser": mock.Mock(), "augeaspath": "/files/anything"} ) testcases = { "/some/path/FirstNode/SecondNode": "SecondNode", @@ -35,7 +36,7 @@ class AugeasParserNodeTest(util.ApacheTest): "/Anything": "Anything", } for test in testcases: - self.assertEqual(block._aug_get_block_name(test), testcases[test]) # pylint: disable=protected-access + self.assertEqual(block._aug_get_name(test), testcases[test]) # pylint: disable=protected-access def test_find_blocks(self): blocks = self.config.parser_root.find_blocks("VirtualHost", exclude=False) @@ -130,3 +131,103 @@ class AugeasParserNodeTest(util.ApacheTest): servername.set_parameters(["thisshouldnotexistpreviously"]) found = True self.assertTrue(found) + + def test_add_child_block(self): + nb = self.config.parser_root.primary.add_child_block( + "NewBlock", + ["first", "second"] + ) + rpath, _, directive = nb.metadata["augeaspath"].rpartition("/") + self.assertEqual( + rpath, + self.config.parser_root.primary.metadata["augeaspath"] + ) + self.assertTrue(directive.startswith("NewBlock")) + + def test_add_child_block_beginning(self): + self.config.parser_root.primary.add_child_block( + "Beginning", + position=0 + ) + parser = self.config.parser_root.primary.parser + root_path = self.config.parser_root.primary.metadata["augeaspath"] + # Get first child + first = parser.aug.match("{}/*[1]".format(root_path)) + self.assertTrue(first[0].endswith("Beginning")) + + def test_add_child_block_append(self): + self.config.parser_root.primary.add_child_block( + "VeryLast", + ) + parser = self.config.parser_root.primary.parser + root_path = self.config.parser_root.primary.metadata["augeaspath"] + # Get last child + last = parser.aug.match("{}/*[last()]".format(root_path)) + self.assertTrue(last[0].endswith("VeryLast")) + + def test_add_child_block_append_alt(self): + self.config.parser_root.primary.add_child_block( + "VeryLastAlt", + position=99999 + ) + parser = self.config.parser_root.primary.parser + root_path = self.config.parser_root.primary.metadata["augeaspath"] + # Get last child + last = parser.aug.match("{}/*[last()]".format(root_path)) + self.assertTrue(last[0].endswith("VeryLastAlt")) + + def test_add_child_block_middle(self): + self.config.parser_root.primary.add_child_block( + "Middle", + position=5 + ) + parser = self.config.parser_root.primary.parser + root_path = self.config.parser_root.primary.metadata["augeaspath"] + # Augeas indices start at 1 :( + middle = parser.aug.match("{}/*[6]".format(root_path)) + self.assertTrue(middle[0].endswith("Middle")) + + def test_add_child_block_existing_name(self): + parser = self.config.parser_root.primary.parser + root_path = self.config.parser_root.primary.metadata["augeaspath"] + # There already exists a single VirtualHost in the base config + new_block = parser.aug.match("{}/VirtualHost[2]".format(root_path)) + self.assertEqual(len(new_block), 0) + vh = self.config.parser_root.primary.add_child_block( + "VirtualHost", + ) + new_block = parser.aug.match("{}/VirtualHost[2]".format(root_path)) + self.assertEqual(len(new_block), 1) + self.assertTrue(vh.metadata["augeaspath"].endswith("VirtualHost[2]")) + + def test_node_init_error_bad_augeaspath(self): + from certbot_apache.augeasparser import AugeasBlockNode + parameters = { + "name": assertions.PASS, + "ancestor": None, + "filepath": assertions.PASS, + "metadata": { + "augeasparser": mock.Mock(), + "augeaspath": "/files/path/endswith/slash/" + } + } + self.assertRaises( + errors.PluginError, + AugeasBlockNode, + **parameters + ) + def test_node_init_error_missing_augeaspath(self): + from certbot_apache.augeasparser import AugeasBlockNode + parameters = { + "name": assertions.PASS, + "ancestor": None, + "filepath": assertions.PASS, + "metadata": { + "augeasparser": mock.Mock(), + } + } + self.assertRaises( + errors.PluginError, + AugeasBlockNode, + **parameters + ) diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 1eafae982..68d9c90fa 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -78,7 +78,8 @@ class MultipleVhostsTest(util.ApacheTest): @mock.patch("certbot_apache.parser.ApacheParser") @mock.patch("certbot_apache.configurator.util.exe_exists") - def _test_prepare_locked(self, unused_parser, unused_exe_exists): + @mock.patch("certbot_apache.configurator.ApacheConfigurator.get_parsernode_root") + def _test_prepare_locked(self, _node, _exists, _parser): try: self.config.prepare() except errors.PluginError as err: diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index 5d6f827e7..dbce18431 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -164,10 +164,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_comments(self): pri_comments = [augeasparser.AugeasCommentNode(comment="some comment", ancestor=self.block, - filepath="/path/to/whatever")] + filepath="/path/to/whatever", + metadata=self.metadata)] sec_comments = [augeasparser.AugeasCommentNode(comment=assertions.PASS, ancestor=self.block, - filepath=assertions.PASS)] + filepath=assertions.PASS, + metadata=self.metadata)] find_coms_primary = mock.MagicMock(return_value=pri_comments) find_coms_secondary = mock.MagicMock(return_value=sec_comments) self.block.primary.find_comments = find_coms_primary @@ -303,10 +305,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_find_coms_second_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=notpassing) find_coms_secondary = mock.MagicMock(return_value=passing) self.block.primary.find_comments = find_coms_primary @@ -398,13 +402,16 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- def test_parsernode_notequal(self): ne_block = augeasparser.AugeasBlockNode(name="different", ancestor=self.block, - filepath="/path/to/whatever") + filepath="/path/to/whatever", + metadata=self.metadata) ne_directive = augeasparser.AugeasDirectiveNode(name="different", ancestor=self.block, - filepath="/path/to/whatever") + filepath="/path/to/whatever", + metadata=self.metadata) ne_comment = augeasparser.AugeasCommentNode(comment="different", ancestor=self.block, - filepath="/path/to/whatever") + filepath="/path/to/whatever", + metadata=self.metadata) self.assertFalse(self.block == ne_block) self.assertFalse(self.directive == ne_directive) self.assertFalse(self.comment == ne_comment) From bdf24d2bed1976ae5f8938d46709b1cdef424b4f Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 13 Nov 2019 00:19:02 +0200 Subject: [PATCH 12/23] Implement add_child_directive (#7517) --- certbot-apache/certbot_apache/augeasparser.py | 24 +++++++++++++++---- .../certbot_apache/tests/augeasnode_test.py | 22 ++++++++++++++++- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index e340519ce..5fd5247c1 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -221,12 +221,26 @@ class AugeasBlockNode(AugeasDirectiveNode): # 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, "augeaspath": assertions.PASS} - new_dir = AugeasDirectiveNode(name=assertions.PASS, - ancestor=self, - filepath=assertions.PASS, + + if not parameters: + raise errors.PluginError("Directive requires parameters and none were set.") + + insertpath, realpath, before = self._aug_resolve_child_position( + "directive", + position + ) + new_metadata = {"augeasparser": self.parser, "augeaspath": realpath} + + # Create the new directive + self.parser.aug.insert(insertpath, "directive", before) + # Set the directive key + self.parser.aug.set(realpath, name) + + new_dir = AugeasDirectiveNode(name=name, + parameters=parameters, + ancestor=assertions.PASS, + filepath=apache_util.get_file_path(realpath), metadata=new_metadata) - self.children += (new_dir,) return new_dir def add_child_comment(self, comment="", position=None): # pylint: disable=unused-argument diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 8846c3c07..76f46c14d 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -9,7 +9,7 @@ from certbot_apache import assertions from certbot_apache.tests import util -class AugeasParserNodeTest(util.ApacheTest): +class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public-methods """Test AugeasParserNode using available test configurations""" def setUp(self): # pylint: disable=arguments-differ @@ -216,6 +216,7 @@ class AugeasParserNodeTest(util.ApacheTest): AugeasBlockNode, **parameters ) + def test_node_init_error_missing_augeaspath(self): from certbot_apache.augeasparser import AugeasBlockNode parameters = { @@ -231,3 +232,22 @@ class AugeasParserNodeTest(util.ApacheTest): AugeasBlockNode, **parameters ) + + def test_add_child_directive(self): + self.config.parser_root.primary.add_child_directive( + "ThisWasAdded", + ["with", "parameters"], + position=0 + ) + dirs = self.config.parser_root.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].metadata["augeaspath"].endswith("[1]")) + + def test_add_child_directive_exception(self): + self.assertRaises( + errors.PluginError, + self.config.parser_root.primary.add_child_directive, + "ThisRaisesErrorBecauseMissingParameters" + ) From d14eec9ecf89da33dacb4fbac618a226e54526b0 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 13 Nov 2019 00:19:21 +0200 Subject: [PATCH 13/23] [Apache v2] Implement save() and unsaved_files() (#7520) * Implement save() and unsaved_files() * Linter fix --- certbot-apache/certbot_apache/augeasparser.py | 8 ++++---- .../certbot_apache/tests/augeasnode_test.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 5fd5247c1..a1d72fe52 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -97,8 +97,8 @@ class AugeasParserNode(interfaces.ParserNode): except KeyError: raise errors.PluginError("Augeas path is required") - def save(self, msg): # pragma: no cover - pass + def save(self, msg): + self.parser.save(msg) class AugeasCommentNode(AugeasParserNode): @@ -304,9 +304,9 @@ class AugeasBlockNode(AugeasDirectiveNode): """Deletes a ParserNode from the sequence of children""" pass - def unsaved_files(self): # pragma: no cover + def unsaved_files(self): """Returns a list of unsaved filepaths""" - return [assertions.PASS] + return self.parser.unsaved_files() def _create_commentnode(self, path): """Helper function to create a CommentNode from Augeas path""" diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 76f46c14d..39736ee17 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -20,6 +20,18 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.vh_truth = util.get_vh_truth( self.temp_dir, "debian_apache_2_4/multiple_vhosts") + def test_save(self): + with mock.patch('certbot_apache.parser.ApacheParser.save') as mock_save: + self.config.parser_root.save("A save message") + self.assertTrue(mock_save.called) + self.assertEqual(mock_save.call_args[0][0], "A save message") + + def test_unsaved_files(self): + with mock.patch('certbot_apache.parser.ApacheParser.unsaved_files') as mock_uf: + mock_uf.return_value = ["first", "second"] + files = self.config.parser_root.unsaved_files() + self.assertEqual(files, ["first", "second"]) + def test_get_block_node_name(self): from certbot_apache.augeasparser import AugeasBlockNode block = AugeasBlockNode( From 517ff5cb1930e21b1de0d8c5343cb99c622e321f Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Wed, 13 Nov 2019 00:19:35 +0200 Subject: [PATCH 14/23] [Apache v2] Implement delete_child() (#7521) * Implement delete_child * Fix linter --- certbot-apache/certbot_apache/augeasparser.py | 15 ++++++++++++--- .../certbot_apache/tests/augeasnode_test.py | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index a1d72fe52..47b8602ab 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -300,9 +300,18 @@ class AugeasBlockNode(AugeasDirectiveNode): return nodes - def delete_child(self, child): # pragma: no cover - """Deletes a ParserNode from the sequence of children""" - pass + def delete_child(self, child): + """ + Deletes a ParserNode from the sequence of children, and raises an + exception if it's unable to do so. + :param AugeasParserNode: child: A node to delete. + """ + 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"]) + ) def unsaved_files(self): """Returns a list of unsaved filepaths""" diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 39736ee17..a86b618b2 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -144,6 +144,24 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- found = True self.assertTrue(found) + def test_delete_child(self): + listens = self.config.parser_root.find_directives("Listen") + self.assertEqual(len(listens), 1) + self.config.parser_root.primary.delete_child(listens[0]) + + listens = self.config.parser_root.find_directives("Listen") + self.assertEqual(len(listens), 0) + + def test_delete_child_not_found(self): + listen = self.config.parser_root.find_directives("Listen")[0] + listen.primary.metadata["augeaspath"] = "/files/something/nonexistent" + + self.assertRaises( + errors.PluginError, + self.config.parser_root.delete_child, + listen + ) + def test_add_child_block(self): nb = self.config.parser_root.primary.add_child_block( "NewBlock", From b70f9c474481efd8850fc83622ccb6f4312f3021 Mon Sep 17 00:00:00 2001 From: sydneyli Date: Fri, 15 Nov 2019 02:22:18 -0800 Subject: [PATCH 15/23] [Apache v2] Initial ApacheParser skeleton (#7559) * Fix metadata & primary references in Augeas tests. When performing actions only on one of the trees in DualNodeParser, the two trees get out-of-sync. Similarly, we can't expect that the metadata between the two trees will remain the same. Did a pass over the tests to re-wire metadata and primary usage. * Add ApacheParser skeleton. Fix plumbing in configurator & dualparser to initialize ApacheParser alongside AugeasParser. * Silence coverage reports for now --- certbot-apache/certbot_apache/apacheparser.py | 157 ++++++++++++++++++ certbot-apache/certbot_apache/configurator.py | 3 +- certbot-apache/certbot_apache/dualparser.py | 7 +- .../certbot_apache/tests/augeasnode_test.py | 28 ++-- .../certbot_apache/tests/dualnode_test.py | 2 +- 5 files changed, 178 insertions(+), 19 deletions(-) create mode 100644 certbot-apache/certbot_apache/apacheparser.py diff --git a/certbot-apache/certbot_apache/apacheparser.py b/certbot-apache/certbot_apache/apacheparser.py new file mode 100644 index 000000000..6625735b4 --- /dev/null +++ b/certbot-apache/certbot_apache/apacheparser.py @@ -0,0 +1,157 @@ +""" apacheconfig implementation of the ParserNode interfaces """ + +from certbot_apache import assertions +from certbot_apache import interfaces +from certbot_apache import parsernode_util as util + + +class ApacheParserNode(interfaces.ParserNode): + """ apacheconfig implementation of ParserNode interface. + + Expects metadata `ac_ast` to be passed in, where `ac_ast` is the AST provided + by parsing the equivalent configuration text using the apacheconfig library. + """ + + def __init__(self, **kwargs): + ancestor, dirty, filepath, metadata = util.parsernode_kwargs(kwargs) # pylint: disable=unused-variable + super(ApacheParserNode, self).__init__(**kwargs) + self.ancestor = ancestor + self.filepath = filepath + self.dirty = dirty + self.metadata = metadata + self._raw = self.metadata["ac_ast"] + + def save(self, msg): # pragma: no cover + pass + + +class ApacheCommentNode(ApacheParserNode): + """ apacheconfig implementation of CommentNode interface """ + + def __init__(self, **kwargs): + comment, kwargs = util.commentnode_kwargs(kwargs) # pylint: disable=unused-variable + super(ApacheCommentNode, self).__init__(**kwargs) + self.comment = comment + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.comment == other.comment and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata and + self.filepath == other.filepath) + return False + + +class ApacheDirectiveNode(ApacheParserNode): + """ apacheconfig implementation of DirectiveNode interface """ + + def __init__(self, **kwargs): + name, parameters, enabled, kwargs = util.directivenode_kwargs(kwargs) + super(ApacheDirectiveNode, self).__init__(**kwargs) + self.name = name + self.parameters = parameters + self.enabled = enabled + self.include = None + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.name == other.name and + self.filepath == other.filepath and + self.parameters == other.parameters and + self.enabled == other.enabled and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata) + return False + + def set_parameters(self, parameters): + """Sets the parameters for DirectiveNode""" + pass + + +class ApacheBlockNode(ApacheDirectiveNode): + """ apacheconfig implementation of BlockNode interface """ + + def __init__(self, **kwargs): + super(ApacheBlockNode, self).__init__(**kwargs) + self.children = () + + def __eq__(self, other): # pragma: no cover + if isinstance(other, self.__class__): + return (self.name == other.name and + self.filepath == other.filepath and + self.parameters == other.parameters and + self.children == other.children and + self.enabled == other.enabled and + self.dirty == other.dirty and + self.ancestor == other.ancestor and + self.metadata == other.metadata) + return False + + def add_child_block(self, name, parameters=None, position=None): # pylint: disable=unused-argument + """Adds a new BlockNode to the sequence of children""" + new_block = ApacheBlockNode(name=assertions.PASS, + parameters=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata) + self.children += (new_block,) + return new_block + + def add_child_directive(self, name, parameters=None, position=None): # pylint: disable=unused-argument + """Adds a new DirectiveNode to the sequence of children""" + new_dir = ApacheDirectiveNode(name=assertions.PASS, + parameters=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata) + self.children += (new_dir,) + return new_dir + + # pylint: disable=unused-argument + def add_child_comment(self, comment="", position=None): # pragma: no cover + + """Adds a new CommentNode to the sequence of children""" + new_comment = ApacheCommentNode(comment=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata) + self.children += (new_comment,) + return new_comment + + 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, + parameters=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata)] + + 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, + parameters=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata)] + + def find_comments(self, comment, exact=False): # pylint: disable=unused-argument + """Recursive search of DirectiveNodes from the sequence of children""" + return [ApacheCommentNode(comment=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata)] + + def delete_child(self, child): # pragma: no cover + """Deletes a ParserNode from the sequence of children""" + pass + + def unsaved_files(self): # pragma: no cover + """Returns a list of unsaved filepaths""" + return [assertions.PASS] + + +interfaces.CommentNode.register(ApacheCommentNode) +interfaces.DirectiveNode.register(ApacheDirectiveNode) +interfaces.BlockNode.register(ApacheBlockNode) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 808748006..2b20ebb7c 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -258,7 +258,8 @@ class ApacheConfigurator(common.Installer): # Set up ParserNode root pn_meta = {"augeasparser": self.parser, - "augeaspath": self.parser.get_root_augpath()} + "augeaspath": self.parser.get_root_augpath(), + "ac_ast": None} self.parser_root = self.get_parsernode_root(pn_meta) # Check for errors in parsing files with Augeas diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index 8897449b6..ef8e86196 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -1,6 +1,7 @@ """ Dual ParserNode implementation """ from certbot_apache import assertions from certbot_apache import augeasparser +from certbot_apache import apacheparser class DualNodeBase(object): @@ -49,7 +50,7 @@ class DualCommentNode(DualNodeBase): self.secondary = secondary else: self.primary = augeasparser.AugeasCommentNode(**kwargs) - self.secondary = augeasparser.AugeasCommentNode(**kwargs) + self.secondary = apacheparser.ApacheCommentNode(**kwargs) assertions.assertEqual(self.primary, self.secondary) @@ -83,7 +84,7 @@ class DualDirectiveNode(DualNodeBase): self.secondary = secondary else: self.primary = augeasparser.AugeasDirectiveNode(**kwargs) - self.secondary = augeasparser.AugeasDirectiveNode(**kwargs) + self.secondary = apacheparser.ApacheDirectiveNode(**kwargs) assertions.assertEqual(self.primary, self.secondary) @@ -123,7 +124,7 @@ class DualBlockNode(DualNodeBase): self.secondary = secondary else: self.primary = augeasparser.AugeasBlockNode(**kwargs) - self.secondary = augeasparser.AugeasBlockNode(**kwargs) + self.secondary = apacheparser.ApacheBlockNode(**kwargs) assertions.assertEqual(self.primary, self.secondary) diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index a86b618b2..2bf21408f 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -110,7 +110,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- name=servernames[0].name, parameters=["test", "setting", "these"], ancestor=assertions.PASS, - metadata=servernames[0].metadata + metadata=servernames[0].primary.metadata ) self.assertTrue(mock_set.called) self.assertEqual( @@ -145,11 +145,11 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(found) def test_delete_child(self): - listens = self.config.parser_root.find_directives("Listen") + listens = self.config.parser_root.primary.find_directives("Listen") self.assertEqual(len(listens), 1) self.config.parser_root.primary.delete_child(listens[0]) - listens = self.config.parser_root.find_directives("Listen") + listens = self.config.parser_root.primary.find_directives("Listen") self.assertEqual(len(listens), 0) def test_delete_child_not_found(self): @@ -163,11 +163,11 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- ) def test_add_child_block(self): - nb = self.config.parser_root.primary.add_child_block( + nb = self.config.parser_root.add_child_block( "NewBlock", ["first", "second"] ) - rpath, _, directive = nb.metadata["augeaspath"].rpartition("/") + rpath, _, directive = nb.primary.metadata["augeaspath"].rpartition("/") self.assertEqual( rpath, self.config.parser_root.primary.metadata["augeaspath"] @@ -175,7 +175,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(directive.startswith("NewBlock")) def test_add_child_block_beginning(self): - self.config.parser_root.primary.add_child_block( + self.config.parser_root.add_child_block( "Beginning", position=0 ) @@ -186,7 +186,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(first[0].endswith("Beginning")) def test_add_child_block_append(self): - self.config.parser_root.primary.add_child_block( + self.config.parser_root.add_child_block( "VeryLast", ) parser = self.config.parser_root.primary.parser @@ -196,7 +196,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(last[0].endswith("VeryLast")) def test_add_child_block_append_alt(self): - self.config.parser_root.primary.add_child_block( + self.config.parser_root.add_child_block( "VeryLastAlt", position=99999 ) @@ -207,7 +207,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(last[0].endswith("VeryLastAlt")) def test_add_child_block_middle(self): - self.config.parser_root.primary.add_child_block( + self.config.parser_root.add_child_block( "Middle", position=5 ) @@ -223,12 +223,12 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- # There already exists a single VirtualHost in the base config new_block = parser.aug.match("{}/VirtualHost[2]".format(root_path)) self.assertEqual(len(new_block), 0) - vh = self.config.parser_root.primary.add_child_block( + vh = self.config.parser_root.add_child_block( "VirtualHost", ) new_block = parser.aug.match("{}/VirtualHost[2]".format(root_path)) self.assertEqual(len(new_block), 1) - self.assertTrue(vh.metadata["augeaspath"].endswith("VirtualHost[2]")) + self.assertTrue(vh.primary.metadata["augeaspath"].endswith("VirtualHost[2]")) def test_node_init_error_bad_augeaspath(self): from certbot_apache.augeasparser import AugeasBlockNode @@ -264,7 +264,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- ) def test_add_child_directive(self): - self.config.parser_root.primary.add_child_directive( + self.config.parser_root.add_child_directive( "ThisWasAdded", ["with", "parameters"], position=0 @@ -273,11 +273,11 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- 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].metadata["augeaspath"].endswith("[1]")) + self.assertTrue(dirs[0].primary.metadata["augeaspath"].endswith("[1]")) def test_add_child_directive_exception(self): self.assertRaises( errors.PluginError, - self.config.parser_root.primary.add_child_directive, + self.config.parser_root.add_child_directive, "ThisRaisesErrorBecauseMissingParameters" ) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index dbce18431..2878a693e 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -16,7 +16,7 @@ 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"} + self.metadata = {"augeasparser": parser_mock, "augeaspath": "/invalid", "ac_ast": None} self.block = dualparser.DualBlockNode(name="block", ancestor=None, filepath="/tmp/something", From ac1a60ff0b98251930d9f3a8338127c8a0513ffa Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 18 Nov 2019 21:21:51 +0200 Subject: [PATCH 16/23] Implement add_child_comment (#7518) --- certbot-apache/certbot_apache/augeasparser.py | 22 ++++++++++++++----- .../certbot_apache/tests/augeasnode_test.py | 10 +++++++++ .../certbot_apache/tests/dualnode_test.py | 15 +++++-------- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 47b8602ab..3956b1d16 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -243,14 +243,24 @@ class AugeasBlockNode(AugeasDirectiveNode): metadata=new_metadata) return new_dir - def add_child_comment(self, comment="", position=None): # pylint: disable=unused-argument + def add_child_comment(self, comment="", position=None): """Adds a new CommentNode to the sequence of children""" - new_metadata = {"augeasparser": self.parser, "augeaspath": assertions.PASS} - new_comment = AugeasCommentNode(comment=assertions.PASS, - ancestor=self, - filepath=assertions.PASS, + + insertpath, realpath, before = self._aug_resolve_child_position( + "#comment", + position + ) + new_metadata = {"augeasparser": self.parser, "augeaspath": realpath} + + # Create the new comment + self.parser.aug.insert(insertpath, "#comment", before) + # Set the comment content + self.parser.aug.set(realpath, comment) + + new_comment = AugeasCommentNode(comment=comment, + ancestor=assertions.PASS, + filepath=apache_util.get_file_path(realpath), metadata=new_metadata) - self.children += (new_comment,) return new_comment def find_blocks(self, name, exclude=True): # pylint: disable=unused-argument diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 2bf21408f..bf01f9fac 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -144,6 +144,16 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- found = True self.assertTrue(found) + 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") + self.assertEqual(len(comments), 1) + self.assertEqual( + newc.metadata["augeaspath"], + comments[0].primary.metadata["augeaspath"] + ) + self.assertEqual(newc.comment, comments[0].comment) + def test_delete_child(self): listens = self.config.parser_root.primary.find_directives("Listen") self.assertEqual(len(listens), 1) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index 2878a693e..bdfab4fc7 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -6,7 +6,6 @@ import mock from certbot_apache import assertions from certbot_apache import augeasparser from certbot_apache import dualparser -from certbot_apache import interfaces class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public-methods @@ -151,15 +150,13 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- self.assertTrue(mock_second.called) def test_add_child_comment(self): - self.assertEqual(len(self.block.primary.children), 0) - self.assertEqual(len(self.block.secondary.children), 0) + mock_first = mock.MagicMock(return_value=self.comment.primary) + mock_second = mock.MagicMock(return_value=self.comment.secondary) + self.block.primary.add_child_comment = mock_first + self.block.secondary.add_child_comment = mock_second self.block.add_child_comment("Comment") - 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.CommentNode)) - self.assertEqual(self.block.primary.children[0].ancestor, - self.block.primary) + self.assertTrue(mock_first.called) + self.assertTrue(mock_second.called) def test_find_comments(self): pri_comments = [augeasparser.AugeasCommentNode(comment="some comment", From 06fdbf2a55eca3097b489ae9794c48366e5a93ea Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 2 Dec 2019 16:25:39 +0200 Subject: [PATCH 17/23] [Apache v2] Implement find_ancestors (#7561) * Implement find_ancestors * Create the node properly and add assertions * Update certbot-apache/certbot_apache/augeasparser.py Co-Authored-By: ohemorange * Remove comment --- certbot-apache/certbot_apache/apacheparser.py | 8 ++ certbot-apache/certbot_apache/augeasparser.py | 92 ++++++++++++------- certbot-apache/certbot_apache/dualparser.py | 89 ++++++++++-------- certbot-apache/certbot_apache/interfaces.py | 12 +++ .../certbot_apache/tests/augeasnode_test.py | 21 +++++ .../certbot_apache/tests/dualnode_test.py | 9 ++ .../certbot_apache/tests/parsernode_test.py | 4 + 7 files changed, 163 insertions(+), 72 deletions(-) diff --git a/certbot-apache/certbot_apache/apacheparser.py b/certbot-apache/certbot_apache/apacheparser.py index 6625735b4..d9f33f095 100644 --- a/certbot-apache/certbot_apache/apacheparser.py +++ b/certbot-apache/certbot_apache/apacheparser.py @@ -24,6 +24,14 @@ class ApacheParserNode(interfaces.ParserNode): def save(self, msg): # pragma: no cover pass + def find_ancestors(self, name): # pylint: disable=unused-variable + """Find ancestor BlockNodes with a given name""" + return [ApacheBlockNode(name=assertions.PASS, + parameters=assertions.PASS, + ancestor=self, + filepath=assertions.PASS, + metadata=self.metadata)] + class ApacheCommentNode(ApacheParserNode): """ apacheconfig implementation of CommentNode interface """ diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 3956b1d16..8a3a37083 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -75,7 +75,6 @@ from certbot_apache import parser from certbot_apache import parsernode_util as util - class AugeasParserNode(interfaces.ParserNode): """ Augeas implementation of ParserNode interface """ @@ -100,6 +99,63 @@ class AugeasParserNode(interfaces.ParserNode): def save(self, msg): self.parser.save(msg) + def find_ancestors(self, name): + """ + Searches for ancestor BlockNodes with a given name. + + :param str name: Name of the BlockNode parent to search for + + :returns: List of matching ancestor nodes. + :rtype: list of AugeasBlockNode + """ + + ancestors = [] + + parent = self.metadata["augeaspath"] + while True: + # Get the path of ancestor node + parent = parent.rpartition("/")[0] + if not parent: + break + anc = self._create_blocknode(parent) + if anc.name.lower() == name.lower(): + ancestors.append(anc) + + return ancestors + + def _create_blocknode(self, path): + """ + Helper function to create a BlockNode from Augeas path. This is used by + AugeasParserNode.find_ancestors and AugeasBlockNode. + and AugeasBlockNode.find_blocks + + """ + + name = self._aug_get_name(path) + metadata = {"augeasparser": self.parser, "augeaspath": path} + + return AugeasBlockNode(name=name, + ancestor=assertions.PASS, + filepath=apache_util.get_file_path(path), + metadata=metadata) + + def _aug_get_name(self, path): + """ + Helper function to get name of a configuration block or variable from path. + """ + + # Remove the ending slash if any + if path[-1] == "/": # pragma: no cover + path = path[:-1] + + # Get the block name + name = path.split("/")[-1] + + # remove [...], it's not allowed in Apache configuration and is used + # for indexing within Augeas + name = name.split("[")[0] + return name + class AugeasCommentNode(AugeasParserNode): """ Augeas implementation of CommentNode interface """ @@ -263,7 +319,7 @@ class AugeasBlockNode(AugeasDirectiveNode): metadata=new_metadata) return new_comment - def find_blocks(self, name, exclude=True): # pylint: disable=unused-argument + def find_blocks(self, name, exclude=True): """Recursive search of BlockNodes from the sequence of children""" nodes = list() @@ -275,7 +331,7 @@ class AugeasBlockNode(AugeasDirectiveNode): return nodes - def find_directives(self, name, exclude=True): # pylint: disable=unused-argument + def find_directives(self, name, exclude=True): """Recursive search of DirectiveNodes from the sequence of children""" nodes = list() @@ -353,19 +409,6 @@ class AugeasBlockNode(AugeasDirectiveNode): filepath=apache_util.get_file_path(path), metadata=metadata) - def _create_blocknode(self, path): - """Helper function to create a BlockNode from Augeas path""" - - name = self._aug_get_name(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, - ancestor=assertions.PASS, - filepath=apache_util.get_file_path(path), - metadata=metadata) - def _aug_find_blocks(self, name): """Helper function to perform a search to Augeas DOM tree to search configuration blocks with a given name""" @@ -380,23 +423,6 @@ class AugeasBlockNode(AugeasDirectiveNode): name.lower() in os.path.basename(path).lower()]) return blk_paths - def _aug_get_name(self, path): - """ - Helper function to get name of a configuration block or variable from path. - """ - - # Remove the ending slash if any - if path[-1] == "/": # pragma: no cover - path = path[:-1] - - # Get the block name - name = path.split("/")[-1] - - # remove [...], it's not allowed in Apache configuration and is used - # for indexing within Augeas - name = name.split("[")[0] - return name - def _aug_resolve_child_position(self, name, position): """ Helper function that iterates through the immediate children and figures diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index ef8e86196..667462d34 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -18,10 +18,59 @@ class DualNodeBase(object): """ Attribute value assertion """ firstval = getattr(self.primary, aname) secondval = getattr(self.secondary, aname) - if not callable(firstval): + exclusions = [ + # Metadata will inherently be different, as ApacheParserNode does + # not have Augeas paths and so on. + aname == "metadata", + callable(firstval) + ] + if not any(exclusions): assertions.assertEqualSimple(firstval, secondval) return firstval + def find_ancestors(self, name): + """ Traverses the ancestor tree and returns ancestors matching name """ + return self._find_helper(DualBlockNode, "find_ancestors", name) + + def _find_helper(self, nodeclass, findfunc, search, **kwargs): + """A helper for find_* functions. The function specific attributes should + be passed as keyword arguments. + + :param interfaces.ParserNode nodeclass: The node class for results. + :param str findfunc: Name of the find function to call + :param str search: The search term + """ + + primary_res = getattr(self.primary, findfunc)(search, **kwargs) + secondary_res = getattr(self.secondary, findfunc)(search, **kwargs) + + # The order of search results for Augeas implementation cannot be + # assured. + + pass_primary = assertions.isPassNodeList(primary_res) + pass_secondary = assertions.isPassNodeList(secondary_res) + new_nodes = list() + + if pass_primary and pass_secondary: + # Both unimplemented + new_nodes.append(nodeclass(primary=primary_res[0], + secondary=secondary_res[0])) # pragma: no cover + elif pass_primary: + for c in secondary_res: + new_nodes.append(nodeclass(primary=primary_res[0], + secondary=c)) + elif pass_secondary: + for c in primary_res: + new_nodes.append(nodeclass(primary=c, + secondary=secondary_res[0])) + else: + assert len(primary_res) == len(secondary_res) + matches = self._create_matching_list(primary_res, secondary_res) + for p, s in matches: + new_nodes.append(nodeclass(primary=p, secondary=s)) + + return new_nodes + class DualCommentNode(DualNodeBase): """ Dual parser implementation of CommentNode interface """ @@ -223,44 +272,6 @@ class DualBlockNode(DualNodeBase): return self._find_helper(DualCommentNode, "find_comments", comment) - def _find_helper(self, nodeclass, findfunc, search, **kwargs): - """A helper for find_* functions. The function specific attributes should - be passed as keyword arguments. - - :param interfaces.ParserNode nodeclass: The node class for results. - :param str findfunc: Name of the find function to call - :param str search: The search term - """ - - primary_res = getattr(self.primary, findfunc)(search, **kwargs) - secondary_res = getattr(self.secondary, findfunc)(search, **kwargs) - - # The order of search results for Augeas implementation cannot be - # assured. - - pass_primary = assertions.isPassNodeList(primary_res) - pass_secondary = assertions.isPassNodeList(secondary_res) - new_nodes = list() - - if pass_primary and pass_secondary: - # Both unimplemented - new_nodes.append(nodeclass(primary=primary_res[0], - secondary=secondary_res[0])) # pragma: no cover - elif pass_primary: - for c in secondary_res: - new_nodes.append(nodeclass(primary=primary_res[0], - secondary=c)) - elif pass_secondary: - for c in primary_res: - new_nodes.append(nodeclass(primary=c, - secondary=secondary_res[0])) - else: - assert len(primary_res) == len(secondary_res) - matches = self._create_matching_list(primary_res, secondary_res) - for p, s in matches: - new_nodes.append(nodeclass(primary=p, secondary=s)) - - return new_nodes def delete_child(self, child): """Deletes a child from the ParserNode implementations. The actual diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index ecad2d4eb..2d25fad0f 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -192,6 +192,18 @@ class ParserNode(object): """ + @abc.abstractmethod + def find_ancestors(self, name): + """ + Traverses the ancestor tree up, searching for BlockNodes with a specific + name. + + :param str name: Name of the ancestor BlockNode to search for + + :returns: A list of ancestor BlockNodes that match the name + :rtype: list of BlockNode + """ + # Linter rule exclusion done because of https://github.com/PyCQA/pylint/issues/179 @six.add_metaclass(abc.ABCMeta) # pylint: disable=abstract-method diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index bf01f9fac..043f5d248 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -291,3 +291,24 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.config.parser_root.add_child_directive, "ThisRaisesErrorBecauseMissingParameters" ) + + def test_find_ancestors(self): + vhsblocks = self.config.parser_root.find_blocks("VirtualHost") + macro_test = False + nonmacro_test = False + for vh in vhsblocks: + if "/macro/" in vh.metadata["augeaspath"].lower(): + ancs = vh.find_ancestors("Macro") + self.assertEqual(len(ancs), 1) + macro_test = True + else: + ancs = vh.find_ancestors("Macro") + self.assertEqual(len(ancs), 0) + nonmacro_test = True + self.assertTrue(macro_test) + self.assertTrue(nonmacro_test) + + def test_find_ancestors_bad_path(self): + self.config.parser_root.primary.metadata["augeaspath"] = "" + ancs = self.config.parser_root.primary.find_ancestors("Anything") + self.assertEqual(len(ancs), 0) diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index bdfab4fc7..9e5a5e9aa 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -412,3 +412,12 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- self.assertFalse(self.block == ne_block) self.assertFalse(self.directive == ne_directive) self.assertFalse(self.comment == ne_comment) + + def test_find_ancestors(self): + primarymock = mock.MagicMock(return_value=[]) + secondarymock = mock.MagicMock(return_value=[]) + self.block.primary.find_ancestors = primarymock + self.block.secondary.find_ancestors = secondarymock + self.block.find_ancestors("anything") + self.assertTrue(primarymock.called) + self.assertTrue(secondarymock.called) diff --git a/certbot-apache/certbot_apache/tests/parsernode_test.py b/certbot-apache/certbot_apache/tests/parsernode_test.py index 1a2288c82..a6caf4814 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_test.py @@ -24,6 +24,10 @@ class DummyParserNode(interfaces.ParserNode): """Save""" pass + def find_ancestors(self, name): # pragma: no cover + """ Find ancestors """ + return [] + class DummyCommentNode(DummyParserNode): """ A dummy class implementing CommentNode interface """ From 6148e5c35599cc3b39a5be70e4c520414d1e5eb7 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 3 Dec 2019 01:00:53 +0200 Subject: [PATCH 18/23] [Apache v2] Move the apachectl parsing to apache_util (#7569) * Move the Apache CLI parsing to apache_util * Fix test mocks * Address review comments * Fix the parsernode metadata dictionary --- certbot-apache/certbot_apache/apache_util.py | 119 ++++++++++++++++++ certbot-apache/certbot_apache/configurator.py | 7 ++ .../certbot_apache/override_gentoo.py | 2 +- certbot-apache/certbot_apache/parser.py | 77 +----------- .../certbot_apache/tests/centos_test.py | 4 +- .../certbot_apache/tests/configurator_test.py | 4 +- .../certbot_apache/tests/debian_test.py | 2 +- .../certbot_apache/tests/fedora_test.py | 4 +- .../certbot_apache/tests/gentoo_test.py | 4 +- .../certbot_apache/tests/parser_test.py | 12 +- certbot-apache/certbot_apache/tests/util.py | 28 +++-- 11 files changed, 163 insertions(+), 100 deletions(-) diff --git a/certbot-apache/certbot_apache/apache_util.py b/certbot-apache/certbot_apache/apache_util.py index 7a2ecf49b..70febc949 100644 --- a/certbot-apache/certbot_apache/apache_util.py +++ b/certbot-apache/certbot_apache/apache_util.py @@ -1,9 +1,16 @@ """ Utility functions for certbot-apache plugin """ import binascii +import logging +import re +import subprocess +from certbot import errors from certbot import util + from certbot.compat import os +logger = logging.getLogger(__name__) + def get_mod_deps(mod_name): """Get known module dependencies. @@ -105,3 +112,115 @@ def parse_define_file(filepath, varname): def unique_id(): """ Returns an unique id to be used as a VirtualHost identifier""" return binascii.hexlify(os.urandom(16)).decode("utf-8") + + +def parse_defines(apachectl): + """ + Gets Defines from httpd process and returns a dictionary of + the defined variables. + + :param str apachectl: Path to apachectl executable + + :returns: dictionary of defined variables + :rtype: dict + """ + + variables = dict() + define_cmd = [apachectl, "-t", "-D", + "DUMP_RUN_CFG"] + matches = parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)") + try: + matches.remove("DUMP_RUN_CFG") + except ValueError: + return {} + + for match in matches: + if match.count("=") > 1: + logger.error("Unexpected number of equal signs in " + "runtime config dump.") + raise errors.PluginError( + "Error parsing Apache runtime variables") + parts = match.partition("=") + variables[parts[0]] = parts[2] + + return variables + + +def parse_includes(apachectl): + """ + Gets Include directives from httpd process and returns a list of + their values. + + :param str apachectl: Path to apachectl executable + + :returns: list of found Include directive values + :rtype: list of str + """ + + inc_cmd = [apachectl, "-t", "-D", + "DUMP_INCLUDES"] + return parse_from_subprocess(inc_cmd, r"\(.*\) (.*)") + + +def parse_modules(apachectl): + """ + Get loaded modules from httpd process, and return the list + of loaded module names. + + :param str apachectl: Path to apachectl executable + + :returns: list of found LoadModule module names + :rtype: list of str + """ + + mod_cmd = [apachectl, "-t", "-D", + "DUMP_MODULES"] + return parse_from_subprocess(mod_cmd, r"(.*)_module") + + +def parse_from_subprocess(command, regexp): + """Get values from stdout of subprocess command + + :param list command: Command to run + :param str regexp: Regexp for parsing + + :returns: list parsed from command output + :rtype: list + + """ + stdout = _get_runtime_cfg(command) + return re.compile(regexp).findall(stdout) + + +def _get_runtime_cfg(command): + """ + Get runtime configuration info. + + :param command: Command to run + + :returns: stdout from command + + """ + try: + proc = subprocess.Popen( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True) + stdout, stderr = proc.communicate() + + except (OSError, ValueError): + logger.error( + "Error running command %s for runtime parameters!%s", + command, os.linesep) + raise errors.MisconfigurationError( + "Error accessing loaded Apache parameters: {0}".format( + command)) + # Small errors that do not impede + if proc.returncode != 0: + logger.warning("Error in checking parameter list: %s", stderr) + raise errors.MisconfigurationError( + "Apache is unable to check whether or not the module is " + "loaded because Apache is misconfigured.") + + return stdout diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 2b20ebb7c..9a9dec7a8 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -359,6 +359,13 @@ class ApacheConfigurator(common.Installer): def get_parsernode_root(self, metadata): """Initializes the ParserNode parser root instance.""" + + apache_vars = dict() + apache_vars["defines"] = apache_util.parse_defines(self.option("ctl")) + apache_vars["includes"] = apache_util.parse_includes(self.option("ctl")) + apache_vars["modules"] = apache_util.parse_modules(self.option("ctl")) + metadata["apache_vars"] = apache_vars + return dualparser.DualBlockNode( name=assertions.PASS, ancestor=None, diff --git a/certbot-apache/certbot_apache/override_gentoo.py b/certbot-apache/certbot_apache/override_gentoo.py index c358a10fa..fdb1a8d8a 100644 --- a/certbot-apache/certbot_apache/override_gentoo.py +++ b/certbot-apache/certbot_apache/override_gentoo.py @@ -70,6 +70,6 @@ class GentooParser(parser.ApacheParser): def update_modules(self): """Get loaded modules from httpd process, and add them to DOM""" mod_cmd = [self.configurator.option("ctl"), "modules"] - matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module") + matches = apache_util.parse_from_subprocess(mod_cmd, r"(.*)_module") for mod in matches: self.add_mod(mod.strip()) diff --git a/certbot-apache/certbot_apache/parser.py b/certbot-apache/certbot_apache/parser.py index 906b97e72..b9bdaca37 100644 --- a/certbot-apache/certbot_apache/parser.py +++ b/certbot-apache/certbot_apache/parser.py @@ -3,7 +3,6 @@ import copy import fnmatch import logging import re -import subprocess import sys import six @@ -13,6 +12,7 @@ from acme.magic_typing import Dict, List, Set # pylint: disable=unused-import, from certbot import errors from certbot.compat import os +from certbot_apache import apache_util from certbot_apache import constants logger = logging.getLogger(__name__) @@ -291,32 +291,15 @@ class ApacheParser(object): def update_runtime_variables(self): """Update Includes, Defines and Includes from httpd config dump data""" + self.update_defines() self.update_includes() self.update_modules() def update_defines(self): - """Get Defines from httpd process""" + """Updates the dictionary of known variables in the configuration""" - variables = dict() - define_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_RUN_CFG"] - matches = self.parse_from_subprocess(define_cmd, r"Define: ([^ \n]*)") - try: - matches.remove("DUMP_RUN_CFG") - except ValueError: - return - - for match in matches: - if match.count("=") > 1: - logger.error("Unexpected number of equal signs in " - "runtime config dump.") - raise errors.PluginError( - "Error parsing Apache runtime variables") - parts = match.partition("=") - variables[parts[0]] = parts[2] - - self.variables = variables + self.variables = apache_util.parse_defines(self.configurator.option("ctl")) def update_includes(self): """Get includes from httpd process, and add them to DOM if needed""" @@ -326,9 +309,7 @@ class ApacheParser(object): # configuration files _ = self.find_dir("Include") - inc_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_INCLUDES"] - matches = self.parse_from_subprocess(inc_cmd, r"\(.*\) (.*)") + matches = apache_util.parse_includes(self.configurator.option("ctl")) if matches: for i in matches: if not self.parsed_in_current(i): @@ -337,56 +318,10 @@ class ApacheParser(object): def update_modules(self): """Get loaded modules from httpd process, and add them to DOM""" - mod_cmd = [self.configurator.option("ctl"), "-t", "-D", - "DUMP_MODULES"] - matches = self.parse_from_subprocess(mod_cmd, r"(.*)_module") + matches = apache_util.parse_modules(self.configurator.option("ctl")) for mod in matches: self.add_mod(mod.strip()) - def parse_from_subprocess(self, command, regexp): - """Get values from stdout of subprocess command - - :param list command: Command to run - :param str regexp: Regexp for parsing - - :returns: list parsed from command output - :rtype: list - - """ - stdout = self._get_runtime_cfg(command) - return re.compile(regexp).findall(stdout) - - def _get_runtime_cfg(self, command): # pylint: disable=no-self-use - """Get runtime configuration info. - :param command: Command to run - - :returns: stdout from command - - """ - try: - proc = subprocess.Popen( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True) - stdout, stderr = proc.communicate() - - except (OSError, ValueError): - logger.error( - "Error running command %s for runtime parameters!%s", - command, os.linesep) - raise errors.MisconfigurationError( - "Error accessing loaded Apache parameters: {0}".format( - command)) - # Small errors that do not impede - if proc.returncode != 0: - logger.warning("Error in checking parameter list: %s", stderr) - raise errors.MisconfigurationError( - "Apache is unable to check whether or not the module is " - "loaded because Apache is misconfigured.") - - return stdout - def filter_args_num(self, matches, args): # pylint: disable=no-self-use """Filter out directives with specific number of arguments. diff --git a/certbot-apache/certbot_apache/tests/centos_test.py b/certbot-apache/certbot_apache/tests/centos_test.py index dddbf489e..bb97ec144 100644 --- a/certbot-apache/certbot_apache/tests/centos_test.py +++ b/certbot-apache/certbot_apache/tests/centos_test.py @@ -107,7 +107,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_centos.CentOSParser) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -156,7 +156,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_get_sysconfig_vars(self, mock_cfg): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 68d9c90fa..350112f6f 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -807,7 +807,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(mock_restart.call_count, 1) @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_cleanup(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() @@ -823,7 +823,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertFalse(mock_restart.called) @mock.patch("certbot_apache.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_cleanup_no_errors(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() diff --git a/certbot-apache/certbot_apache/tests/debian_test.py b/certbot-apache/certbot_apache/tests/debian_test.py index 54ced2d0b..72f806589 100644 --- a/certbot-apache/certbot_apache/tests/debian_test.py +++ b/certbot-apache/certbot_apache/tests/debian_test.py @@ -47,7 +47,7 @@ class MultipleVhostsTestDebian(util.ApacheTest): @mock.patch("certbot.util.run_script") @mock.patch("certbot.util.exe_exists") - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_enable_mod(self, mock_popen, mock_exe_exists, mock_run_script): mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "") mock_popen().returncode = 0 diff --git a/certbot-apache/certbot_apache/tests/fedora_test.py b/certbot-apache/certbot_apache/tests/fedora_test.py index 4d3f3a313..799c24c20 100644 --- a/certbot-apache/certbot_apache/tests/fedora_test.py +++ b/certbot-apache/certbot_apache/tests/fedora_test.py @@ -101,7 +101,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_fedora.FedoraParser) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -156,7 +156,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_get_sysconfig_vars(self, mock_cfg): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls diff --git a/certbot-apache/certbot_apache/tests/gentoo_test.py b/certbot-apache/certbot_apache/tests/gentoo_test.py index d0d3ba0dd..353a66564 100644 --- a/certbot-apache/certbot_apache/tests/gentoo_test.py +++ b/certbot-apache/certbot_apache/tests/gentoo_test.py @@ -90,7 +90,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): for define in defines: self.assertTrue(define in self.config.parser.variables.keys()) - @mock.patch("certbot_apache.parser.ApacheParser.parse_from_subprocess") + @mock.patch("certbot_apache.apache_util.parse_from_subprocess") def test_no_binary_configdump(self, mock_subprocess): """Make sure we don't call binary dumps other than modules from Apache as this is not supported in Gentoo currently""" @@ -104,7 +104,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): self.config.parser.reset_modules() self.assertTrue(mock_subprocess.called) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): mod_val = ( 'Loaded Modules:\n' diff --git a/certbot-apache/certbot_apache/tests/parser_test.py b/certbot-apache/certbot_apache/tests/parser_test.py index 27d66f680..303ac09e3 100644 --- a/certbot-apache/certbot_apache/tests/parser_test.py +++ b/certbot-apache/certbot_apache/tests/parser_test.py @@ -167,7 +167,7 @@ class BasicParserTest(util.ParserTest): self.assertTrue(mock_logger.debug.called) @mock.patch("certbot_apache.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_variables(self, mock_cfg, _): define_val = ( 'ServerRoot: "/etc/apache2"\n' @@ -273,7 +273,7 @@ class BasicParserTest(util.ParserTest): self.assertEqual(mock_parse.call_count, 25) @mock.patch("certbot_apache.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_variables_alt_values(self, mock_cfg, _): inc_val = ( 'Included configuration files:\n' @@ -295,7 +295,7 @@ class BasicParserTest(util.ParserTest): # path derived from root configuration Include statements self.assertEqual(mock_parse.call_count, 1) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_update_runtime_vars_bad_output(self, mock_cfg): mock_cfg.return_value = "Define: TLS=443=24" self.parser.update_runtime_variables() @@ -305,7 +305,7 @@ class BasicParserTest(util.ParserTest): errors.PluginError, self.parser.update_runtime_variables) @mock.patch("certbot_apache.configurator.ApacheConfigurator.option") - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_ctl(self, mock_popen, mock_opt): mock_popen.side_effect = OSError mock_opt.return_value = "nonexistent" @@ -313,7 +313,7 @@ class BasicParserTest(util.ParserTest): errors.MisconfigurationError, self.parser.update_runtime_variables) - @mock.patch("certbot_apache.parser.subprocess.Popen") + @mock.patch("certbot_apache.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_exit(self, mock_popen): mock_popen().communicate.return_value = ("", "") mock_popen.returncode = -1 @@ -357,7 +357,7 @@ class ParserInitTest(util.ApacheTest): ApacheParser, os.path.relpath(self.config_path), "/dummy/vhostpath", version=(2, 4, 22), configurator=self.config) - @mock.patch("certbot_apache.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache.apache_util._get_runtime_cfg") def test_unparseable(self, mock_cfg): from certbot_apache.parser import ApacheParser mock_cfg.return_value = ('Define: TEST') diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 8e3de04be..4155c93c0 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -111,19 +111,21 @@ def get_apache_configurator( # pylint: disable=too-many-arguments, too-many-loc mock_exe_exists.return_value = True with mock.patch("certbot_apache.parser.ApacheParser." "update_runtime_variables"): - try: - config_class = entrypoint.OVERRIDE_CLASSES[os_info] - except KeyError: - config_class = configurator.ApacheConfigurator - config = config_class(config=mock_le_config, name="apache", - version=version) - if not conf_vhost_path: - config_class.OS_DEFAULTS["vhost_root"] = vhost_path - else: - # Custom virtualhost path was requested - config.config.apache_vhost_root = conf_vhost_path - config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"] - config.prepare() + with mock.patch("certbot_apache.apache_util.parse_from_subprocess") as mock_sp: + mock_sp.return_value = [] + try: + config_class = entrypoint.OVERRIDE_CLASSES[os_info] + except KeyError: + config_class = configurator.ApacheConfigurator + config = config_class(config=mock_le_config, name="apache", + version=version) + if not conf_vhost_path: + config_class.OS_DEFAULTS["vhost_root"] = vhost_path + else: + # Custom virtualhost path was requested + config.config.apache_vhost_root = conf_vhost_path + config.config.apache_ctl = config_class.OS_DEFAULTS["ctl"] + config.prepare() return config From 5c588a6f8ddc7bc5f13b79e90138c6d88c68b0e0 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Tue, 10 Dec 2019 20:20:00 +0200 Subject: [PATCH 19/23] [Apache v2] Implement parsed_files (#7562) * Implement parsed_files * Add parsed_files stub to ApacheParserNodes and fix assertions * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: ohemorange * Add more descriptive comments * Update certbot-apache/certbot_apache/augeasparser.py Co-Authored-By: ohemorange * Update certbot-apache/certbot_apache/dualparser.py Co-Authored-By: ohemorange * Update certbot-apache/certbot_apache/interfaces.py Co-Authored-By: ohemorange --- certbot-apache/certbot_apache/apacheparser.py | 4 ++++ certbot-apache/certbot_apache/assertions.py | 16 ++++++++++++++++ certbot-apache/certbot_apache/augeasparser.py | 14 ++++++++++++++ certbot-apache/certbot_apache/dualparser.py | 19 +++++++++++++++++-- certbot-apache/certbot_apache/interfaces.py | 12 ++++++++++++ .../certbot_apache/tests/augeasnode_test.py | 4 ++++ .../certbot_apache/tests/dualnode_test.py | 19 +++++++++++++++++++ 7 files changed, 86 insertions(+), 2 deletions(-) diff --git a/certbot-apache/certbot_apache/apacheparser.py b/certbot-apache/certbot_apache/apacheparser.py index d9f33f095..969a4ab69 100644 --- a/certbot-apache/certbot_apache/apacheparser.py +++ b/certbot-apache/certbot_apache/apacheparser.py @@ -159,6 +159,10 @@ class ApacheBlockNode(ApacheDirectiveNode): """Returns a list of unsaved filepaths""" return [assertions.PASS] + def parsed_paths(self): # pragma: no cover + """Returns a list of parsed configuration file paths""" + return [assertions.PASS] + interfaces.CommentNode.register(ApacheCommentNode) interfaces.DirectiveNode.register(ApacheDirectiveNode) diff --git a/certbot-apache/certbot_apache/assertions.py b/certbot-apache/certbot_apache/assertions.py index fc2b35e14..c7a61f446 100644 --- a/certbot-apache/certbot_apache/assertions.py +++ b/certbot-apache/certbot_apache/assertions.py @@ -1,4 +1,6 @@ """Dual parser node assertions""" +import fnmatch + from certbot_apache import interfaces @@ -102,3 +104,17 @@ def assertEqualSimple(first, second): """ Simple assertion """ if not isPass(first) and not isPass(second): assert first == second + +def assertEqualPathsList(first, second): # pragma: no cover + """ + Checks that the two lists of file paths match. This assertion allows for wildcard + paths. + """ + if any([isPass(path) for path in first]): + return + if any([isPass(path) for path in second]): + return + for fpath in first: + assert any([fnmatch.fnmatch(fpath, spath) for spath in second]) + for spath in second: + assert any([fnmatch.fnmatch(fpath, spath) for fpath in first]) diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index 8a3a37083..d2771c9d2 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -383,6 +383,20 @@ class AugeasBlockNode(AugeasDirectiveNode): """Returns a list of unsaved filepaths""" return self.parser.unsaved_files() + def parsed_paths(self): + """ + Returns a list of file paths that have currently been parsed into the parser + tree. The returned list may include paths with wildcard characters, for + example: ['/etc/apache2/conf.d/*.load'] + + This is typically called on the root node of the ParserNode tree. + + :returns: list of file paths of files that have been parsed + """ + + parsed_paths = self.parser.aug.match("/augeas/load/Httpd/incl") + return [self.parser.aug.get(path) for path in parsed_paths] + def _create_commentnode(self, path): """Helper function to create a CommentNode from Augeas path""" diff --git a/certbot-apache/certbot_apache/dualparser.py b/certbot-apache/certbot_apache/dualparser.py index 667462d34..ef0800782 100644 --- a/certbot-apache/certbot_apache/dualparser.py +++ b/certbot-apache/certbot_apache/dualparser.py @@ -54,7 +54,7 @@ class DualNodeBase(object): if pass_primary and pass_secondary: # Both unimplemented new_nodes.append(nodeclass(primary=primary_res[0], - secondary=secondary_res[0])) # pragma: no cover + secondary=secondary_res[0])) # pragma: no cover elif pass_primary: for c in secondary_res: new_nodes.append(nodeclass(primary=primary_res[0], @@ -272,7 +272,6 @@ class DualBlockNode(DualNodeBase): return self._find_helper(DualCommentNode, "find_comments", comment) - def delete_child(self, child): """Deletes a child from the ParserNode implementations. The actual ParserNode implementations are used here directly in order to be able @@ -289,3 +288,19 @@ class DualBlockNode(DualNodeBase): assertions.assertEqualSimple(primary_files, secondary_files) return primary_files + + def parsed_paths(self): + """ + Returns a list of file paths that have currently been parsed into the parser + tree. The returned list may include paths with wildcard characters, for + example: ['/etc/apache2/conf.d/*.load'] + + This is typically called on the root node of the ParserNode tree. + + :returns: list of file paths of files that have been parsed + """ + + primary_paths = self.primary.parsed_paths() + secondary_paths = self.secondary.parsed_paths() + assertions.assertEqualPathsList(primary_paths, secondary_paths) + return primary_paths diff --git a/certbot-apache/certbot_apache/interfaces.py b/certbot-apache/certbot_apache/interfaces.py index 2d25fad0f..1b67be5c8 100644 --- a/certbot-apache/certbot_apache/interfaces.py +++ b/certbot-apache/certbot_apache/interfaces.py @@ -502,3 +502,15 @@ class BlockNode(DirectiveNode): :returns: list of file paths of files that have been changed but not yet saved to disk. """ + + @abc.abstractmethod + def parsed_paths(self): + """ + Returns a list of file paths that have currently been parsed into the parser + tree. The returned list may include paths with wildcard characters, for + example: ['/etc/apache2/conf.d/*.load'] + + This is typically called on the root node of the ParserNode tree. + + :returns: list of file paths of files that have been parsed + """ diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 043f5d248..849a468c9 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -292,6 +292,10 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- "ThisRaisesErrorBecauseMissingParameters" ) + def test_parsed_paths(self): + paths = self.config.parser_root.parsed_paths() + self.assertEqual(len(paths), 6) + def test_find_ancestors(self): vhsblocks = self.config.parser_root.find_blocks("VirtualHost") macro_test = False diff --git a/certbot-apache/certbot_apache/tests/dualnode_test.py b/certbot-apache/certbot_apache/tests/dualnode_test.py index 9e5a5e9aa..b5ab588f0 100644 --- a/certbot-apache/certbot_apache/tests/dualnode_test.py +++ b/certbot-apache/certbot_apache/tests/dualnode_test.py @@ -413,6 +413,25 @@ class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public- self.assertFalse(self.directive == ne_directive) self.assertFalse(self.comment == ne_comment) + def test_parsed_paths(self): + mock_p = mock.MagicMock(return_value=['/path/file.conf', + '/another/path', + '/path/other.conf']) + mock_s = mock.MagicMock(return_value=['/path/*.conf', '/another/path']) + self.block.primary.parsed_paths = mock_p + self.block.secondary.parsed_paths = mock_s + self.block.parsed_paths() + self.assertTrue(mock_p.called) + self.assertTrue(mock_s.called) + + def test_parsed_paths_error(self): + mock_p = mock.MagicMock(return_value=['/path/file.conf']) + mock_s = mock.MagicMock(return_value=['/path/*.conf', '/another/path']) + self.block.primary.parsed_paths = mock_p + self.block.secondary.parsed_paths = mock_s + with self.assertRaises(AssertionError): + self.block.parsed_paths() + def test_find_ancestors(self): primarymock = mock.MagicMock(return_value=[]) secondarymock = mock.MagicMock(return_value=[]) From 4401eacaace73583bfeb591f079e1fb2fb68bab2 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Thu, 19 Dec 2019 10:51:41 +0200 Subject: [PATCH 20/23] Implement get_virtual_hosts() for ParserNode interfaces (#7564) --- certbot-apache/certbot_apache/apache_util.py | 17 +++ certbot-apache/certbot_apache/assertions.py | 22 ++++ certbot-apache/certbot_apache/augeasparser.py | 36 +++++- certbot-apache/certbot_apache/configurator.py | 103 +++++++++++++++++- certbot-apache/certbot_apache/obj.py | 3 +- .../tests/parsernode_configurator_test.py | 36 ++++++ 6 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 certbot-apache/certbot_apache/tests/parsernode_configurator_test.py diff --git a/certbot-apache/certbot_apache/apache_util.py b/certbot-apache/certbot_apache/apache_util.py index 70febc949..085ccddc8 100644 --- a/certbot-apache/certbot_apache/apache_util.py +++ b/certbot-apache/certbot_apache/apache_util.py @@ -1,5 +1,6 @@ """ Utility functions for certbot-apache plugin """ import binascii +import fnmatch import logging import re import subprocess @@ -114,6 +115,22 @@ def unique_id(): return binascii.hexlify(os.urandom(16)).decode("utf-8") +def included_in_paths(filepath, paths): + """ + Returns true if the filepath is included in the list of paths + that may contain full paths or wildcard paths that need to be + expanded. + + :param str filepath: Filepath to check + :params list paths: List of paths to check against + + :returns: True if included + :rtype: bool + """ + + return any([fnmatch.fnmatch(filepath, path) for path in paths]) + + def parse_defines(apachectl): """ Gets Defines from httpd process and returns a dictionary of diff --git a/certbot-apache/certbot_apache/assertions.py b/certbot-apache/certbot_apache/assertions.py index c7a61f446..1a5ce2096 100644 --- a/certbot-apache/certbot_apache/assertions.py +++ b/certbot-apache/certbot_apache/assertions.py @@ -60,6 +60,8 @@ def assertEqualDirective(first, second): def isPass(value): # pragma: no cover """Checks if the value is set to PASS""" + if isinstance(value, bool): + return True return PASS in value def isPassDirective(block): @@ -105,6 +107,26 @@ def assertEqualSimple(first, second): if not isPass(first) and not isPass(second): assert first == second +def isEqualVirtualHost(first, second): + """ + Checks that two VirtualHost objects are similar. There are some built + in differences with the implementations: VirtualHost created by ParserNode + implementation doesn't have "path" defined, as it was used for Augeas path + and that cannot obviously be used in the future. Similarly the legacy + version lacks "node" variable, that has a reference to the BlockNode for the + VirtualHost. + """ + return ( + first.name == second.name and + first.aliases == second.aliases and + first.filep == second.filep and + first.addrs == second.addrs and + first.ssl == second.ssl and + first.enabled == second.enabled and + first.modmacro == second.modmacro and + first.ancestor == second.ancestor + ) + def assertEqualPathsList(first, second): # pragma: no cover """ Checks that the two lists of file paths match. This assertion allows for wildcard diff --git a/certbot-apache/certbot_apache/augeasparser.py b/certbot-apache/certbot_apache/augeasparser.py index d2771c9d2..1c6ce6675 100644 --- a/certbot-apache/certbot_apache/augeasparser.py +++ b/certbot-apache/certbot_apache/augeasparser.py @@ -115,7 +115,8 @@ class AugeasParserNode(interfaces.ParserNode): while True: # Get the path of ancestor node parent = parent.rpartition("/")[0] - if not parent: + # Root of the tree + if not parent or parent == "/files": break anc = self._create_blocknode(parent) if anc.name.lower() == name.lower(): @@ -134,7 +135,13 @@ class AugeasParserNode(interfaces.ParserNode): name = self._aug_get_name(path) metadata = {"augeasparser": self.parser, "augeaspath": path} + # Check if the file was included from the root config or initial state + enabled = self.parser.parsed_in_original( + apache_util.get_file_path(path) + ) + return AugeasBlockNode(name=name, + enabled=enabled, ancestor=assertions.PASS, filepath=apache_util.get_file_path(path), metadata=metadata) @@ -265,10 +272,15 @@ class AugeasBlockNode(AugeasDirectiveNode): # Create the new block self.parser.aug.insert(insertpath, name, before) + # Check if the file was included from the root config or initial state + enabled = self.parser.parsed_in_original( + apache_util.get_file_path(realpath) + ) # Parameters will be set at the initialization of the new object new_block = AugeasBlockNode(name=name, parameters=parameters, + enabled=enabled, ancestor=assertions.PASS, filepath=apache_util.get_file_path(realpath), metadata=new_metadata) @@ -291,9 +303,14 @@ class AugeasBlockNode(AugeasDirectiveNode): self.parser.aug.insert(insertpath, "directive", before) # Set the directive key self.parser.aug.set(realpath, name) + # Check if the file was included from the root config or initial state + enabled = self.parser.parsed_in_original( + apache_util.get_file_path(realpath) + ) new_dir = AugeasDirectiveNode(name=name, parameters=parameters, + enabled=enabled, ancestor=assertions.PASS, filepath=apache_util.get_file_path(realpath), metadata=new_metadata) @@ -394,8 +411,14 @@ class AugeasBlockNode(AugeasDirectiveNode): :returns: list of file paths of files that have been parsed """ - parsed_paths = self.parser.aug.match("/augeas/load/Httpd/incl") - return [self.parser.aug.get(path) for path in parsed_paths] + res_paths = [] + + paths = self.parser.existing_paths + for directory in paths: + for filename in paths[directory]: + res_paths.append(os.path.join(directory, filename)) + + return res_paths def _create_commentnode(self, path): """Helper function to create a CommentNode from Augeas path""" @@ -416,10 +439,13 @@ class AugeasBlockNode(AugeasDirectiveNode): name = self.parser.get_arg(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 + # Check if the file was included from the root config or initial state + enabled = self.parser.parsed_in_original( + apache_util.get_file_path(path) + ) return AugeasDirectiveNode(name=name, ancestor=assertions.PASS, + enabled=enabled, filepath=apache_util.get_file_path(path), metadata=metadata) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 9a9dec7a8..d4466cc53 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -202,7 +202,11 @@ class ApacheConfigurator(common.Installer): self._autohsts = {} # type: Dict[str, Dict[str, Union[int, float]]] # Reverter save notes self.save_notes = "" - + # Should we use ParserNode implementation instead of the old behavior + self.USE_PARSERNODE = False + # Saves the list of file paths that were parsed initially, and + # not added to parser tree by self.conf("vhost-root") for example. + self.parsed_paths = [] # type: List[str] # These will be set in the prepare function self._prepared = False self.parser = None @@ -261,6 +265,7 @@ class ApacheConfigurator(common.Installer): "augeaspath": self.parser.get_root_augpath(), "ac_ast": None} self.parser_root = self.get_parsernode_root(pn_meta) + self.parsed_paths = self.parser_root.parsed_paths() # Check for errors in parsing files with Augeas self.parser.check_parsing_errors("httpd.aug") @@ -897,6 +902,29 @@ class ApacheConfigurator(common.Installer): return vhost def get_virtual_hosts(self): + """ + Temporary wrapper for legacy and ParserNode version for + get_virtual_hosts. This should be replaced with the ParserNode + implementation when ready. + """ + + v1_vhosts = self.get_virtual_hosts_v1() + v2_vhosts = self.get_virtual_hosts_v2() + + for v1_vh in v1_vhosts: + found = False + for v2_vh in v2_vhosts: + if assertions.isEqualVirtualHost(v1_vh, v2_vh): + found = True + break + if not found: + raise AssertionError("Equivalent for {} was not found".format(v1_vh.path)) + + if self.USE_PARSERNODE: + return v2_vhosts + return v1_vhosts + + def get_virtual_hosts_v1(self): """Returns list of virtual hosts found in the Apache configuration. :returns: List of :class:`~certbot_apache.obj.VirtualHost` @@ -949,6 +977,79 @@ class ApacheConfigurator(common.Installer): vhs.append(new_vhost) return vhs + def get_virtual_hosts_v2(self): + """Returns list of virtual hosts found in the Apache configuration using + ParserNode interface. + :returns: List of :class:`~certbot_apache.obj.VirtualHost` + objects found in configuration + :rtype: list + """ + + vhs = [] + vhosts = self.parser_root.find_blocks("VirtualHost", exclude=False) + for vhblock in vhosts: + vhs.append(self._create_vhost_v2(vhblock)) + return vhs + + def _create_vhost_v2(self, node): + """Used by get_virtual_hosts_v2 to create vhost objects using ParserNode + interfaces. + :param interfaces.BlockNode node: The BlockNode object of VirtualHost block + :returns: newly created vhost + :rtype: :class:`~certbot_apache.obj.VirtualHost` + """ + addrs = set() + for param in node.parameters: + addrs.add(obj.Addr.fromstring(param)) + + is_ssl = False + sslengine = node.find_directives("SSLEngine") + if sslengine: + for directive in sslengine: + if directive.parameters[0].lower() == "on": + is_ssl = True + break + + # "SSLEngine on" might be set outside of + # Treat vhosts with port 443 as ssl vhosts + for addr in addrs: + if addr.get_port() == "443": + is_ssl = True + + enabled = apache_util.included_in_paths(node.filepath, self.parsed_paths) + + macro = False + # Check if the VirtualHost is contained in a mod_macro block + if node.find_ancestors("Macro"): + macro = True + vhost = obj.VirtualHost( + node.filepath, None, addrs, is_ssl, enabled, modmacro=macro, node=node + ) + self._populate_vhost_names_v2(vhost) + return vhost + + def _populate_vhost_names_v2(self, vhost): + """Helper function that populates the VirtualHost names. + :param host: In progress vhost whose names will be added + :type host: :class:`~certbot_apache.obj.VirtualHost` + """ + + servername_match = vhost.node.find_directives("ServerName", + exclude=False) + serveralias_match = vhost.node.find_directives("ServerAlias", + exclude=False) + + servername = None + if servername_match: + servername = servername_match[-1].parameters[-1] + + if not vhost.modmacro: + for alias in serveralias_match: + for serveralias in alias.parameters: + vhost.aliases.add(serveralias) + vhost.name = servername + + def is_name_vhost(self, target_addr): """Returns if vhost is a name based vhost diff --git a/certbot-apache/certbot_apache/obj.py b/certbot-apache/certbot_apache/obj.py index 22abc85cd..939251802 100644 --- a/certbot-apache/certbot_apache/obj.py +++ b/certbot-apache/certbot_apache/obj.py @@ -124,7 +124,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods strip_name = re.compile(r"^(?:.+://)?([^ :$]*)") def __init__(self, filep, path, addrs, ssl, enabled, name=None, - aliases=None, modmacro=False, ancestor=None): + aliases=None, modmacro=False, ancestor=None, node=None): # pylint: disable=too-many-arguments """Initialize a VH.""" @@ -137,6 +137,7 @@ class VirtualHost(object): # pylint: disable=too-few-public-methods self.enabled = enabled self.modmacro = modmacro self.ancestor = ancestor + self.node = node def get_names(self): """Return a set of all names.""" diff --git a/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py b/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py new file mode 100644 index 000000000..97f07d3d2 --- /dev/null +++ b/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py @@ -0,0 +1,36 @@ +"""Tests for ApacheConfigurator for AugeasParserNode classes""" +import unittest + +import mock + +from certbot_apache.tests import util + + +class ConfiguratorParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public-methods + """Test AugeasParserNode using available test configurations""" + + def setUp(self): # pylint: disable=arguments-differ + super(ConfiguratorParserNodeTest, self).setUp() + + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir) + self.vh_truth = util.get_vh_truth( + self.temp_dir, "debian_apache_2_4/multiple_vhosts") + + def test_parsernode_get_vhosts(self): + self.config.USE_PARSERNODE = True + vhosts = self.config.get_virtual_hosts() + # Legacy get_virtual_hosts() do not set the node + self.assertTrue(vhosts[0].node is not None) + + def test_parsernode_get_vhosts_mismatch(self): + vhosts = self.config.get_virtual_hosts_v2() + # One of the returned VirtualHost objects differs + vhosts[0].name = "IdidntExpectThat" + self.config.get_virtual_hosts_v2 = mock.MagicMock(return_value=vhosts) + with self.assertRaises(AssertionError): + _ = self.config.get_virtual_hosts() + + +if __name__ == "__main__": + unittest.main() # pragma: no cover From 70be256c663be8d4163b4abb5587da79dbd2da2a Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Fri, 3 Jan 2020 01:44:11 +0200 Subject: [PATCH 21/23] Fix gating to ensure that no parsernode functionality is run unless explicitly requested (#7654) --- certbot-apache/certbot_apache/configurator.py | 33 ++++++++++--------- .../certbot_apache/tests/augeasnode_test.py | 2 +- .../tests/parsernode_configurator_test.py | 3 +- certbot-apache/certbot_apache/tests/util.py | 5 +-- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index d4466cc53..c189552d5 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -187,6 +187,7 @@ class ApacheConfigurator(common.Installer): """ version = kwargs.pop("version", None) + use_parsernode = kwargs.pop("use_parsernode", False) super(ApacheConfigurator, self).__init__(*args, **kwargs) # Add name_server association dict @@ -203,7 +204,7 @@ class ApacheConfigurator(common.Installer): # Reverter save notes self.save_notes = "" # Should we use ParserNode implementation instead of the old behavior - self.USE_PARSERNODE = False + self.USE_PARSERNODE = use_parsernode # Saves the list of file paths that were parsed initially, and # not added to parser tree by self.conf("vhost-root") for example. self.parsed_paths = [] # type: List[str] @@ -264,8 +265,9 @@ class ApacheConfigurator(common.Installer): pn_meta = {"augeasparser": self.parser, "augeaspath": self.parser.get_root_augpath(), "ac_ast": None} - self.parser_root = self.get_parsernode_root(pn_meta) - self.parsed_paths = self.parser_root.parsed_paths() + if self.USE_PARSERNODE: + self.parser_root = self.get_parsernode_root(pn_meta) + self.parsed_paths = self.parser_root.parsed_paths() # Check for errors in parsing files with Augeas self.parser.check_parsing_errors("httpd.aug") @@ -909,18 +911,18 @@ class ApacheConfigurator(common.Installer): """ v1_vhosts = self.get_virtual_hosts_v1() - v2_vhosts = self.get_virtual_hosts_v2() - - for v1_vh in v1_vhosts: - found = False - for v2_vh in v2_vhosts: - if assertions.isEqualVirtualHost(v1_vh, v2_vh): - found = True - break - if not found: - raise AssertionError("Equivalent for {} was not found".format(v1_vh.path)) - if self.USE_PARSERNODE: + v2_vhosts = self.get_virtual_hosts_v2() + + for v1_vh in v1_vhosts: + found = False + for v2_vh in v2_vhosts: + if assertions.isEqualVirtualHost(v1_vh, v2_vh): + found = True + break + if not found: + raise AssertionError("Equivalent for {} was not found".format(v1_vh.path)) + return v2_vhosts return v1_vhosts @@ -1003,7 +1005,8 @@ class ApacheConfigurator(common.Installer): addrs.add(obj.Addr.fromstring(param)) is_ssl = False - sslengine = node.find_directives("SSLEngine") + # Exclusion to match the behavior in get_virtual_hosts_v2 + sslengine = node.find_directives("SSLEngine", exclude=False) if sslengine: for directive in sslengine: if directive.parameters[0].lower() == "on": diff --git a/certbot-apache/certbot_apache/tests/augeasnode_test.py b/certbot-apache/certbot_apache/tests/augeasnode_test.py index 849a468c9..d090c4aa5 100644 --- a/certbot-apache/certbot_apache/tests/augeasnode_test.py +++ b/certbot-apache/certbot_apache/tests/augeasnode_test.py @@ -16,7 +16,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- super(AugeasParserNodeTest, self).setUp() self.config = util.get_apache_configurator( - self.config_path, self.vhost_path, self.config_dir, self.work_dir) + self.config_path, self.vhost_path, self.config_dir, self.work_dir, use_parsernode=True) self.vh_truth = util.get_vh_truth( self.temp_dir, "debian_apache_2_4/multiple_vhosts") diff --git a/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py b/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py index 97f07d3d2..96bd63fdd 100644 --- a/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py +++ b/certbot-apache/certbot_apache/tests/parsernode_configurator_test.py @@ -13,7 +13,8 @@ class ConfiguratorParserNodeTest(util.ApacheTest): # pylint: disable=too-many-p super(ConfiguratorParserNodeTest, self).setUp() self.config = util.get_apache_configurator( - self.config_path, self.vhost_path, self.config_dir, self.work_dir) + self.config_path, self.vhost_path, self.config_dir, + self.work_dir, use_parsernode=True) self.vh_truth = util.get_vh_truth( self.temp_dir, "debian_apache_2_4/multiple_vhosts") diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index 4155c93c0..ee941f507 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -85,7 +85,8 @@ def get_apache_configurator( # pylint: disable=too-many-arguments, too-many-loc config_path, vhost_path, config_dir, work_dir, version=(2, 4, 7), os_info="generic", - conf_vhost_path=None): + conf_vhost_path=None, + use_parsernode=False): """Create an Apache Configurator with the specified options. :param conf: Function that returns binary paths. self.conf in Configurator @@ -118,7 +119,7 @@ def get_apache_configurator( # pylint: disable=too-many-arguments, too-many-loc except KeyError: config_class = configurator.ApacheConfigurator config = config_class(config=mock_le_config, name="apache", - version=version) + version=version, use_parsernode=use_parsernode) if not conf_vhost_path: config_class.OS_DEFAULTS["vhost_root"] = vhost_path else: From 3b065238b39a133eb8f450ca3bc7aba2aa04ea33 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Mon, 6 Jan 2020 17:19:33 +0200 Subject: [PATCH 22/23] Modifications needed for merging to master --- .../certbot_apache/_internal/apacheparser.py | 12 ++++++------ .../certbot_apache/_internal/assertions.py | 6 +++--- .../certbot_apache/_internal/augeasparser.py | 10 +++++----- .../certbot_apache/_internal/dualparser.py | 6 +++--- certbot-apache/tests/augeasnode_test.py | 19 ++++++++++--------- certbot-apache/tests/centos_test.py | 4 ++-- certbot-apache/tests/configurator_test.py | 6 +++--- certbot-apache/tests/debian_test.py | 2 +- certbot-apache/tests/dualnode_test.py | 6 +++--- certbot-apache/tests/fedora_test.py | 4 ++-- certbot-apache/tests/gentoo_test.py | 4 ++-- certbot-apache/tests/parser_test.py | 12 ++++++------ .../tests/parsernode_configurator_test.py | 2 +- certbot-apache/tests/parsernode_test.py | 4 ++-- certbot-apache/tests/parsernode_util_test.py | 2 +- certbot-apache/tests/util.py | 2 +- 16 files changed, 51 insertions(+), 50 deletions(-) diff --git a/certbot-apache/certbot_apache/_internal/apacheparser.py b/certbot-apache/certbot_apache/_internal/apacheparser.py index 969a4ab69..77f4517fe 100644 --- a/certbot-apache/certbot_apache/_internal/apacheparser.py +++ b/certbot-apache/certbot_apache/_internal/apacheparser.py @@ -1,8 +1,8 @@ """ apacheconfig implementation of the ParserNode interfaces """ -from certbot_apache import assertions -from certbot_apache import interfaces -from certbot_apache import parsernode_util as util +from certbot_apache._internal import assertions +from certbot_apache._internal import interfaces +from certbot_apache._internal import parsernode_util as util class ApacheParserNode(interfaces.ParserNode): @@ -73,9 +73,9 @@ class ApacheDirectiveNode(ApacheParserNode): self.metadata == other.metadata) return False - def set_parameters(self, parameters): + def set_parameters(self, _parameters): """Sets the parameters for DirectiveNode""" - pass + return class ApacheBlockNode(ApacheDirectiveNode): @@ -153,7 +153,7 @@ class ApacheBlockNode(ApacheDirectiveNode): def delete_child(self, child): # pragma: no cover """Deletes a ParserNode from the sequence of children""" - pass + return def unsaved_files(self): # pragma: no cover """Returns a list of unsaved filepaths""" diff --git a/certbot-apache/certbot_apache/_internal/assertions.py b/certbot-apache/certbot_apache/_internal/assertions.py index 1a5ce2096..e1b4cdcc8 100644 --- a/certbot-apache/certbot_apache/_internal/assertions.py +++ b/certbot-apache/certbot_apache/_internal/assertions.py @@ -1,7 +1,7 @@ """Dual parser node assertions""" import fnmatch -from certbot_apache import interfaces +from certbot_apache._internal import interfaces PASS = "CERTBOT_PASS_ASSERT" @@ -36,8 +36,8 @@ def assertEqualComment(first, second): # pragma: no cover assert isinstance(first, interfaces.CommentNode) assert isinstance(second, interfaces.CommentNode) - if not isPass(first.comment) and not isPass(second.comment): - assert first.comment == second.comment + if not isPass(first.comment) and not isPass(second.comment): # type: ignore + assert first.comment == second.comment # type: ignore def _assertEqualDirectiveComponents(first, second): # pragma: no cover """ Handles assertion for instance variables for DirectiveNode and BlockNode""" diff --git a/certbot-apache/certbot_apache/_internal/augeasparser.py b/certbot-apache/certbot_apache/_internal/augeasparser.py index 1c6ce6675..e1d7c941d 100644 --- a/certbot-apache/certbot_apache/_internal/augeasparser.py +++ b/certbot-apache/certbot_apache/_internal/augeasparser.py @@ -68,11 +68,11 @@ from acme.magic_typing import Set # pylint: disable=unused-import, no-name-in-m from certbot import errors from certbot.compat import os -from certbot_apache import apache_util -from certbot_apache import assertions -from certbot_apache import interfaces -from certbot_apache import parser -from certbot_apache import parsernode_util as util +from certbot_apache._internal import apache_util +from certbot_apache._internal import assertions +from certbot_apache._internal import interfaces +from certbot_apache._internal import parser +from certbot_apache._internal import parsernode_util as util class AugeasParserNode(interfaces.ParserNode): diff --git a/certbot-apache/certbot_apache/_internal/dualparser.py b/certbot-apache/certbot_apache/_internal/dualparser.py index ef0800782..aa66cf84c 100644 --- a/certbot-apache/certbot_apache/_internal/dualparser.py +++ b/certbot-apache/certbot_apache/_internal/dualparser.py @@ -1,7 +1,7 @@ """ Dual ParserNode implementation """ -from certbot_apache import assertions -from certbot_apache import augeasparser -from certbot_apache import apacheparser +from certbot_apache._internal import assertions +from certbot_apache._internal import augeasparser +from certbot_apache._internal import apacheparser class DualNodeBase(object): diff --git a/certbot-apache/tests/augeasnode_test.py b/certbot-apache/tests/augeasnode_test.py index d090c4aa5..9d663a05f 100644 --- a/certbot-apache/tests/augeasnode_test.py +++ b/certbot-apache/tests/augeasnode_test.py @@ -1,12 +1,13 @@ """Tests for AugeasParserNode classes""" import mock +import util + from acme.magic_typing import List # pylint: disable=unused-import, no-name-in-module from certbot import errors -from certbot_apache import assertions +from certbot_apache._internal import assertions -from certbot_apache.tests import util class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public-methods @@ -21,19 +22,19 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.temp_dir, "debian_apache_2_4/multiple_vhosts") def test_save(self): - with mock.patch('certbot_apache.parser.ApacheParser.save') as mock_save: + with mock.patch('certbot_apache._internal.parser.ApacheParser.save') as mock_save: self.config.parser_root.save("A save message") self.assertTrue(mock_save.called) self.assertEqual(mock_save.call_args[0][0], "A save message") def test_unsaved_files(self): - with mock.patch('certbot_apache.parser.ApacheParser.unsaved_files') as mock_uf: + with mock.patch('certbot_apache._internal.parser.ApacheParser.unsaved_files') as mock_uf: mock_uf.return_value = ["first", "second"] files = self.config.parser_root.unsaved_files() self.assertEqual(files, ["first", "second"]) def test_get_block_node_name(self): - from certbot_apache.augeasparser import AugeasBlockNode + from certbot_apache._internal.augeasparser import AugeasBlockNode block = AugeasBlockNode( name=assertions.PASS, ancestor=None, @@ -102,9 +103,9 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue("going_to_set_this" in names) def test_set_parameters_atinit(self): - from certbot_apache.augeasparser import AugeasDirectiveNode + from certbot_apache._internal.augeasparser import AugeasDirectiveNode servernames = self.config.parser_root.find_directives("servername") - setparam = "certbot_apache.augeasparser.AugeasDirectiveNode.set_parameters" + setparam = "certbot_apache._internal.augeasparser.AugeasDirectiveNode.set_parameters" with mock.patch(setparam) as mock_set: AugeasDirectiveNode( name=servernames[0].name, @@ -241,7 +242,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- self.assertTrue(vh.primary.metadata["augeaspath"].endswith("VirtualHost[2]")) def test_node_init_error_bad_augeaspath(self): - from certbot_apache.augeasparser import AugeasBlockNode + from certbot_apache._internal.augeasparser import AugeasBlockNode parameters = { "name": assertions.PASS, "ancestor": None, @@ -258,7 +259,7 @@ class AugeasParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public- ) def test_node_init_error_missing_augeaspath(self): - from certbot_apache.augeasparser import AugeasBlockNode + from certbot_apache._internal.augeasparser import AugeasBlockNode parameters = { "name": assertions.PASS, "ancestor": None, diff --git a/certbot-apache/tests/centos_test.py b/certbot-apache/tests/centos_test.py index 8959d73b8..55fee3faa 100644 --- a/certbot-apache/tests/centos_test.py +++ b/certbot-apache/tests/centos_test.py @@ -106,7 +106,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_centos.CentOSParser) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -155,7 +155,7 @@ class MultipleVhostsTestCentOS(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_get_sysconfig_vars(self, mock_cfg): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls diff --git a/certbot-apache/tests/configurator_test.py b/certbot-apache/tests/configurator_test.py index fc4559e17..cbb052155 100644 --- a/certbot-apache/tests/configurator_test.py +++ b/certbot-apache/tests/configurator_test.py @@ -76,7 +76,7 @@ class MultipleVhostsTest(util.ApacheTest): @mock.patch("certbot_apache._internal.parser.ApacheParser") @mock.patch("certbot_apache._internal.configurator.util.exe_exists") @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.get_parsernode_root") - def _test_prepare_locked(self, unused_parser, unused_exe_exists): + def _test_prepare_locked(self, _node, _exists, _parser): try: self.config.prepare() except errors.PluginError as err: @@ -800,7 +800,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertEqual(mock_restart.call_count, 1) @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_cleanup(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() @@ -816,7 +816,7 @@ class MultipleVhostsTest(util.ApacheTest): self.assertFalse(mock_restart.called) @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.restart") - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_cleanup_no_errors(self, mock_cfg, mock_restart): mock_cfg.return_value = "" _, achalls = self.get_key_and_achalls() diff --git a/certbot-apache/tests/debian_test.py b/certbot-apache/tests/debian_test.py index 6e63a9bd3..400e503fb 100644 --- a/certbot-apache/tests/debian_test.py +++ b/certbot-apache/tests/debian_test.py @@ -46,7 +46,7 @@ class MultipleVhostsTestDebian(util.ApacheTest): @mock.patch("certbot.util.run_script") @mock.patch("certbot.util.exe_exists") - @mock.patch("certbot_apache._internal.parser.subprocess.Popen") + @mock.patch("certbot_apache._internal.apache_util.subprocess.Popen") def test_enable_mod(self, mock_popen, mock_exe_exists, mock_run_script): mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "") mock_popen().returncode = 0 diff --git a/certbot-apache/tests/dualnode_test.py b/certbot-apache/tests/dualnode_test.py index b5ab588f0..0871bac78 100644 --- a/certbot-apache/tests/dualnode_test.py +++ b/certbot-apache/tests/dualnode_test.py @@ -3,9 +3,9 @@ import unittest import mock -from certbot_apache import assertions -from certbot_apache import augeasparser -from certbot_apache import dualparser +from certbot_apache._internal import assertions +from certbot_apache._internal import augeasparser +from certbot_apache._internal import dualparser class DualParserNodeTest(unittest.TestCase): # pylint: disable=too-many-public-methods diff --git a/certbot-apache/tests/fedora_test.py b/certbot-apache/tests/fedora_test.py index 2bfd6babb..cb1614278 100644 --- a/certbot-apache/tests/fedora_test.py +++ b/certbot-apache/tests/fedora_test.py @@ -100,7 +100,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): def test_get_parser(self): self.assertIsInstance(self.config.parser, override_fedora.FedoraParser) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): define_val = ( 'Define: TEST1\n' @@ -155,7 +155,7 @@ class MultipleVhostsTestFedora(util.ApacheTest): raise Exception("Missed: %s" % vhost) # pragma: no cover self.assertEqual(found, 2) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_get_sysconfig_vars(self, mock_cfg): """Make sure we read the sysconfig OPTIONS variable correctly""" # Return nothing for the process calls diff --git a/certbot-apache/tests/gentoo_test.py b/certbot-apache/tests/gentoo_test.py index 90a163fd3..fb5d192d0 100644 --- a/certbot-apache/tests/gentoo_test.py +++ b/certbot-apache/tests/gentoo_test.py @@ -90,7 +90,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): for define in defines: self.assertTrue(define in self.config.parser.variables.keys()) - @mock.patch("certbot_apache._internal.parser.ApacheParser.parse_from_subprocess") + @mock.patch("certbot_apache._internal.apache_util.parse_from_subprocess") def test_no_binary_configdump(self, mock_subprocess): """Make sure we don't call binary dumps other than modules from Apache as this is not supported in Gentoo currently""" @@ -104,7 +104,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest): self.config.parser.reset_modules() self.assertTrue(mock_subprocess.called) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_opportunistic_httpd_runtime_parsing(self, mock_get): mod_val = ( 'Loaded Modules:\n' diff --git a/certbot-apache/tests/parser_test.py b/certbot-apache/tests/parser_test.py index b334ce52e..f5a0a3d11 100644 --- a/certbot-apache/tests/parser_test.py +++ b/certbot-apache/tests/parser_test.py @@ -165,7 +165,7 @@ class BasicParserTest(util.ParserTest): self.assertTrue(mock_logger.debug.called) @mock.patch("certbot_apache._internal.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_update_runtime_variables(self, mock_cfg, _): define_val = ( 'ServerRoot: "/etc/apache2"\n' @@ -271,7 +271,7 @@ class BasicParserTest(util.ParserTest): self.assertEqual(mock_parse.call_count, 25) @mock.patch("certbot_apache._internal.parser.ApacheParser.find_dir") - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_update_runtime_variables_alt_values(self, mock_cfg, _): inc_val = ( 'Included configuration files:\n' @@ -293,7 +293,7 @@ class BasicParserTest(util.ParserTest): # path derived from root configuration Include statements self.assertEqual(mock_parse.call_count, 1) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_update_runtime_vars_bad_output(self, mock_cfg): mock_cfg.return_value = "Define: TLS=443=24" self.parser.update_runtime_variables() @@ -303,7 +303,7 @@ class BasicParserTest(util.ParserTest): errors.PluginError, self.parser.update_runtime_variables) @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.option") - @mock.patch("certbot_apache._internal.parser.subprocess.Popen") + @mock.patch("certbot_apache._internal.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_ctl(self, mock_popen, mock_opt): mock_popen.side_effect = OSError mock_opt.return_value = "nonexistent" @@ -311,7 +311,7 @@ class BasicParserTest(util.ParserTest): errors.MisconfigurationError, self.parser.update_runtime_variables) - @mock.patch("certbot_apache._internal.parser.subprocess.Popen") + @mock.patch("certbot_apache._internal.apache_util.subprocess.Popen") def test_update_runtime_vars_bad_exit(self, mock_popen): mock_popen().communicate.return_value = ("", "") mock_popen.returncode = -1 @@ -355,7 +355,7 @@ class ParserInitTest(util.ApacheTest): ApacheParser, os.path.relpath(self.config_path), "/dummy/vhostpath", version=(2, 4, 22), configurator=self.config) - @mock.patch("certbot_apache._internal.parser.ApacheParser._get_runtime_cfg") + @mock.patch("certbot_apache._internal.apache_util._get_runtime_cfg") def test_unparseable(self, mock_cfg): from certbot_apache._internal.parser import ApacheParser mock_cfg.return_value = ('Define: TEST') diff --git a/certbot-apache/tests/parsernode_configurator_test.py b/certbot-apache/tests/parsernode_configurator_test.py index 96bd63fdd..67d65995a 100644 --- a/certbot-apache/tests/parsernode_configurator_test.py +++ b/certbot-apache/tests/parsernode_configurator_test.py @@ -3,7 +3,7 @@ import unittest import mock -from certbot_apache.tests import util +import util class ConfiguratorParserNodeTest(util.ApacheTest): # pylint: disable=too-many-public-methods diff --git a/certbot-apache/tests/parsernode_test.py b/certbot-apache/tests/parsernode_test.py index a6caf4814..a86952f53 100644 --- a/certbot-apache/tests/parsernode_test.py +++ b/certbot-apache/tests/parsernode_test.py @@ -2,8 +2,8 @@ import unittest -from certbot_apache import interfaces -from certbot_apache import parsernode_util as util +from certbot_apache._internal import interfaces +from certbot_apache._internal import parsernode_util as util class DummyParserNode(interfaces.ParserNode): diff --git a/certbot-apache/tests/parsernode_util_test.py b/certbot-apache/tests/parsernode_util_test.py index a079759ee..715388da5 100644 --- a/certbot-apache/tests/parsernode_util_test.py +++ b/certbot-apache/tests/parsernode_util_test.py @@ -1,7 +1,7 @@ """ Tests for ParserNode utils """ import unittest -from certbot_apache import parsernode_util as util +from certbot_apache._internal import parsernode_util as util class ParserNodeUtilTest(unittest.TestCase): diff --git a/certbot-apache/tests/util.py b/certbot-apache/tests/util.py index 60704ec66..ccd0b274d 100644 --- a/certbot-apache/tests/util.py +++ b/certbot-apache/tests/util.py @@ -111,7 +111,7 @@ def get_apache_configurator( mock_exe_exists.return_value = True with mock.patch("certbot_apache._internal.parser.ApacheParser." "update_runtime_variables"): - with mock.patch("certbot_apache.apache_util.parse_from_subprocess") as mock_sp: + with mock.patch("certbot_apache._internal.apache_util.parse_from_subprocess") as mock_sp: mock_sp.return_value = [] try: config_class = entrypoint.OVERRIDE_CLASSES[os_info] From 0e78436b059e99a5fd07ea7329b911bcb3f0e147 Mon Sep 17 00:00:00 2001 From: sydneyli Date: Tue, 7 Jan 2020 09:57:43 -0800 Subject: [PATCH 23/23] [Apache v2] Add apacheconfig as a dependency (#7643) * Add apacheconfig as a dependency. * Change apacheconfig to a dev dependency * Bump apacheconfig dep to 0.3.1 --- .travis.yml | 2 +- certbot-apache/setup.py | 6 ++++++ tools/dev_constraints.txt | 2 ++ tools/oldest_constraints.txt | 1 + tox.ini | 6 ++++++ 5 files changed, 16 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 63129c9b1..9f18b04ac 100644 --- a/.travis.yml +++ b/.travis.yml @@ -57,7 +57,7 @@ matrix: # cryptography we support cannot be compiled against the version of # OpenSSL in Xenial or newer. dist: trusty - env: TOXENV='py27-{acme,apache,certbot,dns,nginx}-oldest' + env: TOXENV='py27-{acme,apache,apache-v2,certbot,dns,nginx}-oldest' <<: *not-on-master - python: "3.4" env: TOXENV=py34 diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index 204d01620..c48b8a336 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -18,6 +18,9 @@ install_requires = [ 'zope.interface', ] +dev_extras = [ + 'apacheconfig>=0.3.1', +] class PyTest(TestCommand): user_options = [] @@ -69,6 +72,9 @@ setup( packages=find_packages(), include_package_data=True, install_requires=install_requires, + extras_require={ + 'dev': dev_extras, + }, entry_points={ 'certbot.plugins': [ 'apache = certbot_apache._internal.entrypoint:ENTRYPOINT', diff --git a/tools/dev_constraints.txt b/tools/dev_constraints.txt index d5d78c96a..94a59a6dd 100644 --- a/tools/dev_constraints.txt +++ b/tools/dev_constraints.txt @@ -3,6 +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 apipkg==1.4 appnope==0.1.0 asn1crypto==0.22.0 @@ -64,6 +65,7 @@ pexpect==4.7.0 pickleshare==0.7.4 pkginfo==1.4.2 pluggy==0.13.0 +ply==3.4 prompt-toolkit==1.0.15 ptyprocess==0.6.0 py==1.8.0 diff --git a/tools/oldest_constraints.txt b/tools/oldest_constraints.txt index c5a5c5aa0..6154b497a 100644 --- a/tools/oldest_constraints.txt +++ b/tools/oldest_constraints.txt @@ -40,6 +40,7 @@ pytz==2012rc0 google-api-python-client==1.5.5 # Our setup.py constraints +apacheconfig==0.3.1 cloudflare==1.5.1 cryptography==1.2.3 parsedatetime==1.3 diff --git a/tox.ini b/tox.ini index 5f1a9a426..31a8a8578 100644 --- a/tox.ini +++ b/tox.ini @@ -90,6 +90,12 @@ commands = setenv = {[testenv:py27-oldest]setenv} +[testenv:py27-apache-v2-oldest] +commands = + {[base]install_and_test} certbot-apache[dev] +setenv = + {[testenv:py27-oldest]setenv} + [testenv:py27-certbot-oldest] commands = {[base]install_and_test} certbot[dev]