Repository: minio/minio-py
Pull request #475 (2017-02-14 22:09:07)

get_bucket_policy is broken
1. `response.read()` in `_get_bucket_policy` is incorrect. We should be using `response.data` instead.
2. ~`Resource` is a list of the form `["arn:aws:s3:::test-bucket"]`. Thus using 0th index instead.~
3. Added python 2/3 compatible code.
4. Added tests.
5. Fix bug in `_get_permissions`

harshavardhana: Thanks @indradhanush 
harshavardhana: > Resource is a list of the form ["arn:aws:s3:::test-bucket"]. Thus using 0th index instead.

This is not true always a Resource can be a string and a array.  Take a look at - https://github.com/minio/minio-py/pull/456/commits/2db1e751389584103e03891bbc0183554229a063 once merged should fix your problem. 
harshavardhana: > response.read() in _get_bucket_policy is incorrect. We should be using response.data instead.

response.read() - reads the entire 'response.data' what was the issue that you observed? 
harshavardhana: Can you rebase @indradhanush there was another conflicting PR that was merged. 
indradhanush: @harshavardhana Rebased!

> response.read() - reads the entire 'response.data' what was the issue that you observed?

I though so too. But if I use this in a python script to get a bucket policy, `response.read()` returns an empty string while `response.data` returns the actual response. I'm unable to reproduce in my tests though.
harshavardhana: > I though so too. But if I use this in a python script to get a bucket policy, response.read() returns an empty string while response.data returns the actual response. I'm unable to reproduce this in my tests though.

This is something odd and shouldn't happen. `response.data` is fine for now. 
indradhanush: > This is something odd and shouldn't happen. response.data is fine for now.

So do we want to use `python-requests` instead? Just a thought.
harshavardhana: > So do we want to use python-requests instead? Just a thought.

We thought about it python-requests was too high level and didn't allow certain things for us to do long time ago.  We could certainly take a look at it since the code is self contained.. it is possible to migrate easily. 
indradhanush: Aha. Sounds doable then. Merge for this? 😄 

Patch:
diff --git a/minio/api.py b/minio/api.py
index 41b4bbd..280efa5 100644
--- a/minio/api.py
+++ b/minio/api.py
@@ -324,12 +324,19 @@ class Minio(object):
                                       bucket_name=bucket_name,
                                       query={"policy": ""},
                                       headers={})
-            policy_dict = json.loads(response.read().decode('utf-8'))
         except ResponseError as e:
             # Ignore 'NoSuchBucketPolicy' error.
             if e.code != 'NoSuchBucketPolicy':
                 raise
 
+        data = response.data
+        if isinstance(data, bytes) and isinstance(data, str):  # Python 2
+            policy_dict = json.loads(data.decode('utf-8'))
+        elif isinstance(data, str):  # Python 3
+            policy_dict = json.loads(data)
+        else:
+            policy_dict = json.loads(str(data, 'utf-8'))
+
         return policy_dict
 
     def get_bucket_policy(self, bucket_name, prefix=""):
diff --git a/minio/policy.py b/minio/policy.py
index ba36569..c4f63a7 100644
--- a/minio/policy.py
+++ b/minio/policy.py
@@ -468,6 +468,8 @@ def _get_object_policy(statement):
 def _get_permissions(s, resource, object_resource, matched_resource,
                      bucket_resource, prefix, bucket_common_found,
                      bucket_read_only, bucket_write_only):
+
+    obj_read_only, obj_write_only = False, False
     if (resource == object_resource or
             fnmatch.fnmatch(object_resource, resource)):
         read_only, write_only = _get_object_policy(s)


Test patch:
diff --git a/tests/__init__.py b/tests/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/unit/get_bucket_policy_test.py b/tests/unit/get_bucket_policy_test.py
new file mode 100644
index 0000000..3619cfb
--- /dev/null
+++ b/tests/unit/get_bucket_policy_test.py
@@ -0,0 +1,105 @@
+# -*- coding: utf-8 -*-
+# Minio Python Library for Amazon S3 Compatible Cloud Storage, (C) 2015 Minio, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+from unittest import TestCase
+
+import mock
+from nose.tools import eq_, raises
+
+from minio import Minio
+from minio.api import _DEFAULT_USER_AGENT
+from minio.policy import Policy
+from minio.error import ResponseError
+from tests.unit.minio_mocks import (
+    MockConnection,
+    MockResponse
+)
+
+
+class GetBucketPolicyTest(TestCase):
+    @mock.patch('urllib3.PoolManager')
+    @raises(ResponseError)
+    def test_get_policy_for_non_existent_bucket(self, mock_connection):
+        mock_server = MockConnection()
+        mock_connection.return_value = mock_server
+
+        bucket_name = 'non-existent-bucket'
+
+        mock_server.mock_add_request(
+            MockResponse(
+                'GET',
+                'https://localhost:9000/' + bucket_name + '/?policy=',
+                {'User-Agent': _DEFAULT_USER_AGENT},
+                404,
+            )
+        )
+
+        client = Minio('localhost:9000')
+
+        client.get_bucket_policy(bucket_name)
+
+    @mock.patch('urllib3.PoolManager')
+    def test_get_policy_for_existent_bucket_with_no_prefix(
+            self, mock_connection
+    ):
+        mock_data = '{"Version":"2012-10-17","Statement":[{"Action":["s3:GetBucketLocation"],"Effect":"Allow","Principal":{"AWS":["*"]},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:GetBucketLocation"],"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:ListBucket"],"Condition":{"StringEquals":{"s3:prefix":["test-prefix-readonly"]}},"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:GetObject"],"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket/test-prefix-readonly*"],"Sid":""}]}'  # NOQA
+
+        mock_server = MockConnection()
+        mock_connection.return_value = mock_server
+
+        bucket_name = 'test-bucket'
+
+        mock_server.mock_add_request(
+            MockResponse(
+                'GET',
+                'https://localhost:9000/' + bucket_name + '/?policy=',
+                {'User-Agent': _DEFAULT_USER_AGENT},
+                200,
+                content=mock_data
+            )
+        )
+
+        client = Minio('localhost:9000')
+
+        response = client.get_bucket_policy(bucket_name)
+        eq_(response, Policy.NONE)
+
+    @mock.patch('urllib3.PoolManager')
+    def test_get_policy_for_existent_bucket_with_prefix(
+            self, mock_connection
+    ):
+        mock_data = '{"Version":"2012-10-17","Statement":[{"Action":["s3:GetBucketLocation"],"Effect":"Allow","Principal":{"AWS":["*"]},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:GetBucketLocation"],"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:ListBucket"],"Condition":{"StringEquals":{"s3:prefix":["test-prefix-readonly"]}},"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket"],"Sid":""},{"Action":["s3:GetObject"],"Effect":"Allow","Principal":{"AWS":"*"},"Resource":["arn:aws:s3:::test-bucket/test-prefix-readonly*"],"Sid":""}]}'  # NOQA
+
+        mock_server = MockConnection()
+        mock_connection.return_value = mock_server
+
+        bucket_name = 'test-bucket'
+        prefix_name = 'test-prefix-readonly'
+
+        mock_server.mock_add_request(
+            MockResponse(
+                'GET',
+                'https://localhost:9000/' + bucket_name + '/?policy=',
+                {'User-Agent': _DEFAULT_USER_AGENT},
+                200,
+                content=mock_data
+            )
+        )
+
+        client = Minio('localhost:9000')
+
+        response = client.get_bucket_policy(bucket_name, prefix_name)
+        eq_(response, Policy.READ_ONLY)