From b3d177cd92035b728002f6bfe0cf8e795d3a8817 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Fri, 17 Jul 2020 19:04:57 +0200 Subject: [PATCH] Update compare_policies to add a Version string when missing before comparisons (#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: https://github.com/ansible-collections/community.aws/issues/115 * Cope with missing/None policies * update comment to match what's tested Co-authored-by: Jill R <4121322+jillr@users.noreply.github.com> Co-authored-by: Jill R <4121322+jillr@users.noreply.github.com> --- plugins/module_utils/ec2.py | 10 ++++- tests/unit/module_utils/test_ec2.py | 69 +++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/plugins/module_utils/ec2.py b/plugins/module_utils/ec2.py index 3e3f3c6a71d..b1b97deba6c 100644 --- 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 index 1d2767cf899..5011216ede4 100644 --- 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))