Refactor the build_policy() to use same logic to build both canned an…

…d custom policies

botocore/signers.py

@@ -17,7 +17,7 @@
 
 import botocore
 import botocore.auth
-from botocore.compat import six
+from botocore.compat import six, OrderedDict
 from botocore.awsrequest import create_request_object, prepare_request_dict
 from botocore.exceptions import UnknownSignatureVersionError
 from botocore.exceptions import UnknownClientMethodError
@@ -313,29 +313,27 @@ def build_policy(self, resource, date_less_than,
         :rtype: str
         :return: The policy in a compact string.
         """
-        if date_greater_than is None and ip_address is None:
-            policy = (
-                '{"Statement":[{"Resource":"%s",'
-                '"Condition":{"DateLessThan":{"AWS:EpochTime":%s}}}]}') % (
-                resource, int(datetime2timestamp(date_less_than)))
-        else:
-            # SEE: http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-creating-signed-url-custom-policy.html
-            moment = int(datetime2timestamp(date_less_than))
-            condition = {"DateLessThan": {"AWS:EpochTime": moment}}
-            if ip_address:
-                if '/' not in ip_address:
-                    ip_address += '/32'
-                condition["IpAddress"] = {"AWS:SourceIp": ip_address}
-            if date_greater_than:
-                moment = int(datetime2timestamp(date_greater_than))
-                condition["DateGreaterThan"] = {"AWS:EpochTime": moment}
-            custom_policy = {
-                "Statement": [{"Resource": resource, "Condition": condition}]}
-            policy = json.dumps(
-                custom_policy,
-                sort_keys=True,  # Make it stable
-                separators=(',', ':'))
-        return policy
+        # Note:
+        # 1. Order in canned policy is significant. Special care has been taken
+        #    to ensure the output will match the order defined by document.
+        #    There is also a test case in this commit to ensure that order.
+        #    SEE: http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-creating-signed-url-canned-policy.html#private-content-canned-policy-creating-policy-statement
+        # 2. Albeit the order in custom policy is not required by CloudFront,
+        #    we still use OrderedDict internally to ensure the result is stable
+        #    and also matches canned policy requirement.
+        #    SEE: http://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-creating-signed-url-custom-policy.html
+        moment = int(datetime2timestamp(date_less_than))
+        condition = OrderedDict({"DateLessThan": {"AWS:EpochTime": moment}})
+        if ip_address:
+            if '/' not in ip_address:
+                ip_address += '/32'
+            condition["IpAddress"] = {"AWS:SourceIp": ip_address}
+        if date_greater_than:
+            moment = int(datetime2timestamp(date_greater_than))
+            condition["DateGreaterThan"] = {"AWS:EpochTime": moment}
+        ordered_payload = [('Resource', resource), ('Condition', condition)]
+        custom_policy = {"Statement": [OrderedDict(ordered_payload)]}
+        return json.dumps(custom_policy, separators=(',', ':'))
 
     def _url_b64encode(self, data):
         # Required by CloudFront. See also:

tests/unit/test_signers.py

@@ -275,17 +275,17 @@ def test_build_custom_policy(self):
             'foo', datetime.datetime(2016, 1, 1),
             date_greater_than=datetime.datetime(2015, 12, 1),
             ip_address='12.34.56.78/9')
-        expected = (
-            '{"Statement":['
-                '{"Condition":{'
-                    '"DateGreaterThan":{"AWS:EpochTime":1448928000},'
-                    '"DateLessThan":{"AWS:EpochTime":1451606400},'
-                    '"IpAddress":{"AWS:SourceIp":"12.34.56.78/9"}'
-                '},'
-                '"Resource":"foo"}'
-            ']}')
-        self.assertEqual(json.loads(policy), json.loads(expected))
-        self.assertEqual(policy, expected)  # This is to ensure the right order
+        expected = {
+            "Statement": [{
+                "Resource":"foo",
+                "Condition":{
+                    "DateGreaterThan":{"AWS:EpochTime":1448928000},
+                    "DateLessThan":{"AWS:EpochTime":1451606400},
+                    "IpAddress":{"AWS:SourceIp":"12.34.56.78/9"}
+                },
+            }]
+        }
+        self.assertEqual(json.loads(policy), expected)
 
     def _urlparse(self, url):
         if isinstance(url, six.binary_type):
@@ -319,11 +319,11 @@ def test_generate_presign_url_with_custom_policy(self):
             'http://test.com/index.html?foo=bar', policy=policy)
         expected = (
             'http://test.com/index.html?foo=bar'
-            '&Policy=eyJTdGF0ZW1lbnQiOlt7IkNvbmRpdGlvbiI6eyJEYXRlR3JlY'
-                'XRlclRoYW4iOnsiQVdTOkVwb2NoVGltZSI6MTQ0ODkyODAwMH0sIk'
-                'RhdGVMZXNzVGhhbiI6eyJBV1M6RXBvY2hUaW1lIjoxNDUxNjA2NDA'
-                'wfSwiSXBBZGRyZXNzIjp7IkFXUzpTb3VyY2VJcCI6IjEyLjM0LjU2'
-                'Ljc4LzkifX0sIlJlc291cmNlIjoiZm9vIn1dfQ__'
+            '&Policy=eyJTdGF0ZW1lbnQiOlt7IlJlc291cmNlIjoiZm9vIiwiQ29uZ'
+                'Gl0aW9uIjp7IkRhdGVMZXNzVGhhbiI6eyJBV1M6RXBvY2hUaW1lIj'
+                'oxNDUxNjA2NDAwfSwiSXBBZGRyZXNzIjp7IkFXUzpTb3VyY2VJcCI'
+                '6IjEyLjM0LjU2Ljc4LzkifSwiRGF0ZUdyZWF0ZXJUaGFuIjp7IkFX'
+                'UzpFcG9jaFRpbWUiOjE0NDg5MjgwMDB9fX1dfQ__'
             '&Signature=c2lnbmVk&Key-Pair-Id=MY_KEY_ID')
         self.assertEqualUrl(signed_url, expected)