Improve unit test coverage of module_utils.policy

changelogs/fragments/1136-DEPRECATE-sort_json_policy_dict.yml

@@ -0,0 +1,7 @@
+deprecated_features:
+- module_utils.policy - ``ansible_collections.amazon.aws.module_utils.policy.sort_json_policy_dict``
+  has been deprecated consider using ``ansible_collections.amazon.aws.module_utils.poilcies.compare_policies`` instead
+  (https://github.com/ansible-collections/amazon.aws/pull/1136).
+minor_changes:
+- module_utils.policy - minor refacter of code to reduce complexity and improve test coverage
+  (https://github.com/ansible-collections/amazon.aws/pull/1136).

plugins/module_utils/policy.py

@@ -35,6 +35,34 @@
 from ansible.module_utils.six import binary_type
 from ansible.module_utils.six import string_types
 
+import ansible.module_utils.common.warnings as ansible_warnings
+
+
+def _canonify_root_arn(arn):
+    # There are multiple ways to specifiy delegation of access to an account
+    # https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-accounts
+    if arn.startswith('arn:aws:iam::') and arn.endswith(':root'):
+        arn = arn.split(':')[4]
+    return arn
+
+
+def _canonify_policy_dict_item(item, key):
+    """
+    Converts special cases where there are multiple ways to write the same thing into a single form
+    """
+    # There are multiple ways to specify anonymous principals
+    # https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#principal-anonymous
+    if key in ["NotPrincipal", "Principal"]:
+        if item == "*":
+            return {"AWS": "*"}
+    return item
+
+
+def _tuplify_list(element):
+    if isinstance(element, list):
+        return tuple(element)
+    return element
+
 
 def _hashable_policy(policy, policy_list):
     """
@@ -63,30 +91,24 @@ def _hashable_policy(policy, policy_list):
 
     if isinstance(policy, list):
         for each in policy:
-            tupleified = _hashable_policy(each, [])
-            if isinstance(tupleified, list):
-                tupleified = tuple(tupleified)
+            hashed_policy = _hashable_policy(each, [])
+            tupleified = _tuplify_list(hashed_policy)
             policy_list.append(tupleified)
     elif isinstance(policy, string_types) or isinstance(policy, binary_type):
         policy = to_text(policy)
         # convert root account ARNs to just account IDs
-        if policy.startswith('arn:aws:iam::') and policy.endswith(':root'):
-            policy = policy.split(':')[4]
+        policy = _canonify_root_arn(policy)
         return [policy]
     elif isinstance(policy, dict):
+        # Sort the keys to ensure a consistent order for later comparison
         sorted_keys = list(policy.keys())
         sorted_keys.sort()
         for key in sorted_keys:
-            element = policy[key]
-            # Special case defined in
-            # https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html
-            if key in ["NotPrincipal", "Principal"] and policy[key] == "*":
-                element = {"AWS": "*"}
-            tupleified = _hashable_policy(element, [])
-            if isinstance(tupleified, list):
-                tupleified = tuple(tupleified)
+            # Converts special cases to a consistent form
+            element = _canonify_policy_dict_item(policy[key], key)
+            hashed_policy = _hashable_policy(element, [])
+            tupleified = _tuplify_list(hashed_policy)
             policy_list.append((key, tupleified))
-
     # ensure we aren't returning deeply nested structures of length 1
     if len(policy_list) == 1 and isinstance(policy_list[0], tuple):
         policy_list = policy_list[0]
@@ -135,7 +157,10 @@ def compare_policies(current_policy, new_policy, default_version="2008-10-17"):
 
 def sort_json_policy_dict(policy_dict):
 
-    """ Sort any lists in an IAM JSON policy so that comparison of two policies with identical values but
+    """
+    DEPRECATED - will be removed in amazon.aws 8.0.0
+
+    Sort any lists in an IAM JSON policy so that comparison of two policies with identical values but
     different orders will return true
     Args:
         policy_dict (dict): Dict representing IAM JSON policy.
@@ -151,6 +176,11 @@ def sort_json_policy_dict(policy_dict):
         }
     """
 
+    ansible_warnings.deprecate(
+        'amazon.aws.module_utils.policy.sort_json_policy_dict has been deprecated, consider using '
+        'amazon.aws.module_utils.policy.compare_policies instead',
+        version='8.0.0', collection_name='amazon.aws')
+
     def value_is_list(my_list):
 
         checked_list = []

tests/unit/module_utils/policy/test_canonicalize.py

@@ -0,0 +1,41 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+from ansible_collections.amazon.aws.plugins.module_utils.policy import _canonify_root_arn
+from ansible_collections.amazon.aws.plugins.module_utils.policy import _canonify_policy_dict_item
+from ansible_collections.amazon.aws.plugins.module_utils.policy import _tuplify_list
+
+from ansible_collections.amazon.aws.tests.unit.compat.mock import sentinel
+
+
+def test_tuplify_list():
+    my_list = ["one", 2, sentinel.list_item, False]
+    # Lists are tuplified
+    assert _tuplify_list(my_list) == tuple(my_list)
+    # Other types are not
+    assert _tuplify_list("one") == "one"
+    assert _tuplify_list(2) == 2
+    assert _tuplify_list(sentinel.single_item) is sentinel.single_item
+    assert _tuplify_list(False) is False
+
+
+def test_canonify_root_arn():
+    assert _canonify_root_arn("Some String") == "Some String"
+    assert _canonify_root_arn("123456789012") == "123456789012"
+    assert _canonify_root_arn("arn:aws:iam::123456789012:root") == "123456789012"
+
+
+def test_canonify_policy_dict_item_principal():
+    assert _canonify_policy_dict_item("*", "Principal") == {"AWS": "*"}
+    assert _canonify_policy_dict_item("*", "NotPrincipal") == {"AWS": "*"}
+    assert _canonify_policy_dict_item("*", "AnotherKey") == "*"
+    assert _canonify_policy_dict_item("NotWildCard", "Principal") == "NotWildCard"
+    assert _canonify_policy_dict_item("NotWildCard", "NotPrincipal") == "NotWildCard"
+    assert _canonify_policy_dict_item(sentinel.single_item, "Principal") is sentinel.single_item
+    assert _canonify_policy_dict_item(False, "Principal") is False
+    assert _canonify_policy_dict_item(True, "Principal") is True

tests/unit/module_utils/policy/test_py3cmp.py

@@ -0,0 +1,43 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import pytest
+
+from ansible_collections.amazon.aws.plugins.module_utils.policy import _py3cmp
+
+
+def test_py3cmp_simple():
+    assert _py3cmp(1, 1) == 0
+    assert _py3cmp(1, 2) == -1
+    assert _py3cmp(2, 1) == 1
+    assert _py3cmp("1", "1") == 0
+    assert _py3cmp("1", "2") == -1
+    assert _py3cmp("2", "1") == 1
+    assert _py3cmp("a", "a") == 0
+    assert _py3cmp("a", "b") == -1
+    assert _py3cmp("b", "a") == 1
+    assert _py3cmp(("a",), ("a",)) == 0
+    assert _py3cmp(("a",), ("b",)) == -1
+    assert _py3cmp(("b",), ("a",)) == 1
+
+
+def test_py3cmp_mixed():
+    # Replicates the Python2 comparison behaviour of placing strings before tuples
+    assert _py3cmp(("a",), "a") == 1
+    assert _py3cmp("a", ("a",)) == -1
+
+    assert _py3cmp(("a",), "b") == 1
+    assert _py3cmp("b", ("a",)) == -1
+    assert _py3cmp(("b",), "a") == 1
+    assert _py3cmp("a", ("b",)) == -1
+
+    # intended for use by _hashable_policy, so expects either a string or a tuple
+    with pytest.raises(TypeError) as context:
+        _py3cmp((1,), 1)
+    with pytest.raises(TypeError) as context:
+        _py3cmp(1, (1,))

tests/unit/module_utils/policy/test_simple_hashable_policy.py

@@ -0,0 +1,32 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+from ansible_collections.amazon.aws.plugins.module_utils.policy import _hashable_policy
+from ansible_collections.amazon.aws.tests.unit.compat.mock import sentinel
+
+
+def test_hashable_policy_none():
+    assert _hashable_policy(None, []) == []
+
+
+def test_hashable_policy_boolean():
+    assert _hashable_policy(True, []) == ("true", )
+    assert _hashable_policy(False, []) == ("false", )
+
+
+def test_hashable_policy_int():
+    assert _hashable_policy(1, []) == ("1", )
+    assert _hashable_policy(42, []) == ("42", )
+    assert _hashable_policy(0, []) == ("0", )
+
+
+def test_hashable_policy_string():
+    assert _hashable_policy("simple_string", []) == ["simple_string"]
+    assert _hashable_policy("123456789012", []) == ["123456789012"]
+    # This is a special case, we generally expect to have gone via _canonify_root_arn
+    assert _hashable_policy("arn:aws:iam::123456789012:root", []) == ["123456789012"]

tests/unit/module_utils/policy/test_sort_json_policy_dict.py

@@ -0,0 +1,64 @@
+# (c) 2022 Red Hat Inc.
+#
+# This file is part of Ansible
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+from ansible_collections.amazon.aws.plugins.module_utils.policy import sort_json_policy_dict
+
+
+def test_nothing_to_sort():
+    simple_dict = {'key1': 'a'}
+    nested_dict = {'key1': {'key2': 'a'}}
+    very_nested_dict = {'key1': {'key2': {'key3': 'a'}}}
+    assert sort_json_policy_dict(simple_dict) == simple_dict
+    assert sort_json_policy_dict(nested_dict) == nested_dict
+    assert sort_json_policy_dict(very_nested_dict) == very_nested_dict
+
+
+def test_basic_sort():
+    simple_dict = {'key1': [1, 2, 3, 4], 'key2': [9, 8, 7, 6]}
+    sorted_dict = {'key1': [1, 2, 3, 4], 'key2': [6, 7, 8, 9]}
+    assert sort_json_policy_dict(simple_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+    simple_dict = {'key1': ["a", "b", "c", "d"], 'key2': ["z", "y", "x", "w"]}
+    sorted_dict = {'key1': ["a", "b", "c", "d"], 'key2': ["w", "x", "y", "z"]}
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+
+
+def test_nested_list_sort():
+    nested_dict = {'key1': {'key2': [9, 8, 7, 6]}}
+    sorted_dict = {'key1': {'key2': [6, 7, 8, 9]}}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+    nested_dict = {'key1': {'key2': ["z", "y", "x", "w"]}}
+    sorted_dict = {'key1': {'key2': ["w", "x", "y", "z"]}}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+
+
+def test_nested_dict_list_sort():
+    nested_dict = {'key1': {'key2': {'key3': [9, 8, 7, 6]}}}
+    sorted_dict = {'key1': {'key2': {'key3': [6, 7, 8, 9]}}}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+    nested_dict = {'key1': {'key2': {'key3': ["z", "y", "x", "w"]}}}
+    sorted_dict = {'key1': {'key2': {'key3': ["w", "x", "y", "z"]}}}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+
+
+def test_list_of_dict_sort():
+    nested_dict = {'key1': [{'key2': [4, 3, 2, 1]}, {'key3': [9, 8, 7, 6]}]}
+    sorted_dict = {'key1': [{'key2': [1, 2, 3, 4]}, {'key3': [6, 7, 8, 9]}]}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict
+
+
+def test_list_of_list_sort():
+    nested_dict = {'key1': [[4, 3, 2, 1], [9, 8, 7, 6]]}
+    sorted_dict = {'key1': [[1, 2, 3, 4], [6, 7, 8, 9]]}
+    assert sort_json_policy_dict(nested_dict) == sorted_dict
+    assert sort_json_policy_dict(sorted_dict) == sorted_dict