Update compare_policies to add a Version string when missing before c…

…omparisons (ansible-collections#98) * Update compare_policies to add a Version string when missing The Version component of an IAM policy is optional. However, AWS will automatically add a Version entry once a policy is uploaded. This means that comparing a 'live' policy to the policy that created it only gives the correct result if we add a Version entry in when missing. fixes: ansible-collections#115 * Cope with missing/None policies * update comment to match what's tested Co-authored-by: Jill R <[email protected]> Co-authored-by: Jill R <[email protected]>
alinabuzachis · Jul 17, 2020 · b3d177c · b3d177c
1 parent d6857b3
commit b3d177c
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 1 deletion.
diff --git a/plugins/module_utils/ec2.py b/plugins/module_utils/ec2.py
@@ -641,10 +641,18 @@ def py3cmp(a, b):
         raise
 
 
-def compare_policies(current_policy, new_policy):
+def compare_policies(current_policy, new_policy, default_version="2008-10-17"):
     """ Compares the existing policy and the updated policy
         Returns True if there is a difference between policies.
     """
+    if default_version:
+        if isinstance(current_policy, dict):
+            current_policy = current_policy.copy()
+            current_policy.setdefault("Version", default_version)
+        if isinstance(new_policy, dict):
+            new_policy = new_policy.copy()
+            new_policy.setdefault("Version", default_version)
+
     return set(_hashable_policy(new_policy, [])) != set(_hashable_policy(current_policy, []))
 
 

diff --git a/tests/unit/module_utils/test_ec2.py b/tests/unit/module_utils/test_ec2.py
@@ -105,6 +105,44 @@ def setUp(self):
             ]
         }
 
+        self.version_policy_missing = {
+            'Statement': [
+                {
+                    'Action': 's3:PutObjectAcl',
+                    'Sid': 'AddCannedAcl2',
+                    'Resource': 'arn:aws:s3:::test_policy/*',
+                    'Effect': 'Allow',
+                    'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}
+                }
+            ]
+        }
+
+        self.version_policy_old = {
+            'Version': '2008-10-17',
+            'Statement': [
+                {
+                    'Action': 's3:PutObjectAcl',
+                    'Sid': 'AddCannedAcl2',
+                    'Resource': 'arn:aws:s3:::test_policy/*',
+                    'Effect': 'Allow',
+                    'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}
+                }
+            ]
+        }
+
+        self.version_policy_new = {
+            'Version': '2012-10-17',
+            'Statement': [
+                {
+                    'Action': 's3:PutObjectAcl',
+                    'Sid': 'AddCannedAcl2',
+                    'Resource': 'arn:aws:s3:::test_policy/*',
+                    'Effect': 'Allow',
+                    'Principal': {'AWS': ['arn:aws:iam::XXXXXXXXXXXX:user/username1', 'arn:aws:iam::XXXXXXXXXXXX:user/username2']}
+                }
+            ]
+        }
+
         self.larger_policy_one = {
             "Version": "2012-10-17",
             "Statement": [
@@ -232,3 +270,34 @@ def test_compare_boolean_policy_bool_and_string_are_equal(self):
     def test_compare_numeric_policy_number_and_string_are_equal(self):
         """ Testing two policies one using a quoted number, the other an int """
         self.assertFalse(compare_policies(self.numeric_policy_string, self.numeric_policy_number))
+
+    def test_compare_version_policies_defaults_old(self):
+        """ Testing that a policy without Version is considered identical to one
+        with the 'old' Version (by default)
+        """
+        self.assertFalse(compare_policies(self.version_policy_old, self.version_policy_missing))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing))
+
+    def test_compare_version_policies_default_disabled(self):
+        """ Testing that a policy without Version not considered identical when default_version=None
+        """
+        self.assertFalse(compare_policies(self.version_policy_missing, self.version_policy_missing, default_version=None))
+        self.assertTrue(compare_policies(self.version_policy_old, self.version_policy_missing, default_version=None))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing, default_version=None))
+
+    def test_compare_version_policies_default_set(self):
+        """ Testing that a policy without Version is only considered identical
+        when default_version="2008-10-17"
+        """
+        self.assertFalse(compare_policies(self.version_policy_missing, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertTrue(compare_policies(self.version_policy_old, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertFalse(compare_policies(self.version_policy_old, self.version_policy_missing, default_version="2008-10-17"))
+        self.assertFalse(compare_policies(self.version_policy_new, self.version_policy_missing, default_version="2012-10-17"))
+        self.assertTrue(compare_policies(self.version_policy_new, self.version_policy_missing, default_version="2008-10-17"))
+
+    def test_compare_version_policies_with_none(self):
+        """ Testing that comparing with no policy works
+        """
+        self.assertTrue(compare_policies(self.small_policy_one, None))
+        self.assertTrue(compare_policies(None, self.small_policy_one))
+        self.assertFalse(compare_policies(None, None))