Merge "We don't have to keep the retrieved token anymore"

6 files changed, 70 insertions, 76 deletions

diff --git a/swift/common/middleware/s3api/s3request.py b/swift/common/middleware/s3api/s3request.py
index 3c9542eb9..c3e2c9463 100644
--- a/swift/common/middleware/s3api/s3request.py
+++ b/swift/common/middleware/s3api/s3request.py
@@ -537,7 +537,6 @@ class S3Request(swob.Request):
             'string_to_sign': self.string_to_sign,
             'check_signature': self.check_signature,
         }
-        self.token = None
         self.account = None
         self.user_id = None
         self.slo_enabled = slo_enabled
@@ -1136,8 +1135,6 @@ class S3Request(swob.Request):
         if method is not None:
             env['REQUEST_METHOD'] = method
 
-        env['HTTP_X_AUTH_TOKEN'] = self.token
-
         if obj:
             path = '/v1/%s/%s/%s' % (account, container, obj)
         elif container:
@@ -1329,7 +1326,7 @@ class S3Request(swob.Request):
         except swob.HTTPException as err:
             sw_resp = err
         else:
-            # reuse account and tokens
+            # reuse account
             _, self.account, _ = split_path(sw_resp.environ['PATH_INFO'],
                                             2, 3, True)
 
@@ -1339,10 +1336,11 @@ class S3Request(swob.Request):
         if not self.user_id:
             if 'HTTP_X_USER_NAME' in sw_resp.environ:
                 # keystone
-                self.user_id = \
-                    utf8encode("%s:%s" %
-                               (sw_resp.environ['HTTP_X_TENANT_NAME'],
-                                sw_resp.environ['HTTP_X_USER_NAME']))
+                self.user_id = "%s:%s" % (
+                    sw_resp.environ['HTTP_X_TENANT_NAME'],
+                    sw_resp.environ['HTTP_X_USER_NAME'])
+                if six.PY2 and not isinstance(self.user_id, bytes):
+                    self.user_id = self.user_id.encode('utf8')
             else:
                 # tempauth
                 self.user_id = self.access_key
@@ -1505,8 +1503,8 @@ class S3AclRequest(S3Request):
             # keystone
             self.user_id = "%s:%s" % (sw_resp.environ['HTTP_X_TENANT_NAME'],
                                       sw_resp.environ['HTTP_X_USER_NAME'])
-            self.user_id = utf8encode(self.user_id)
-            self.token = sw_resp.environ.get('HTTP_X_AUTH_TOKEN')
+            if six.PY2 and not isinstance(self.user_id, bytes):
+                self.user_id = self.user_id.encode('utf8')
         else:
             # tempauth
             self.user_id = self.access_key
diff --git a/swift/common/middleware/s3api/s3token.py b/swift/common/middleware/s3api/s3token.py
index 503f27692..04ea0543a 100644
--- a/swift/common/middleware/s3api/s3token.py
+++ b/swift/common/middleware/s3api/s3token.py
@@ -111,10 +111,7 @@ def parse_v2_response(token):
         'X-Project-Id': access_info['token']['tenant']['id'],
         'X-Project-Name': access_info['token']['tenant']['name'],
     }
-    return (
-        headers,
-        access_info['token'].get('id'),
-        access_info['token']['tenant'])
+    return headers, access_info['token']['tenant']
 
 
 def parse_v3_response(token):
@@ -134,7 +131,7 @@ def parse_v3_response(token):
         'X-Project-Domain-Id': token['project']['domain']['id'],
         'X-Project-Domain-Name': token['project']['domain']['name'],
     }
-    return headers, None, token['project']
+    return headers, token['project']
 
 
 class S3Token(object):
@@ -308,7 +305,13 @@ class S3Token(object):
         if memcache_client:
             cached_auth_data = memcache_client.get(memcache_token_key)
             if cached_auth_data:
-                headers, token_id, tenant, secret = cached_auth_data
+                if len(cached_auth_data) == 4:
+                    # Old versions of swift may have cached token, too,
+                    # but we don't need it
+                    headers, _token, tenant, secret = cached_auth_data
+                else:
+                    headers, tenant, secret = cached_auth_data
+
                 if s3_auth_details['check_signature'](secret):
                     self._logger.debug("Cached creds valid")
                 else:
@@ -348,9 +351,9 @@ class S3Token(object):
             try:
                 token = resp.json()
                 if 'access' in token:
-                    headers, token_id, tenant = parse_v2_response(token)
+                    headers, tenant = parse_v2_response(token)
                 elif 'token' in token:
-                    headers, token_id, tenant = parse_v3_response(token)
+                    headers, tenant = parse_v3_response(token)
                 else:
                     raise ValueError
                 if memcache_client:
@@ -363,7 +366,7 @@ class S3Token(object):
                             access=access)
                         memcache_client.set(
                             memcache_token_key,
-                            (headers, token_id, tenant, cred_ref.secret),
+                            (headers, tenant, cred_ref.secret),
                             time=self._secret_cache_duration)
                         self._logger.debug("Cached keystone credentials")
                     except Exception:
@@ -391,7 +394,6 @@ class S3Token(object):
                         environ, start_response)
 
         req.headers.update(headers)
-        req.headers['X-Auth-Token'] = token_id
         tenant_to_connect = force_tenant or tenant['id']
         if six.PY2 and isinstance(tenant_to_connect, six.text_type):
             tenant_to_connect = tenant_to_connect.encode('utf-8')
diff --git a/swift/common/middleware/s3api/subresource.py b/swift/common/middleware/s3api/subresource.py
index c85d0b7b3..c941040d8 100644
--- a/swift/common/middleware/s3api/subresource.py
+++ b/swift/common/middleware/s3api/subresource.py
@@ -232,6 +232,8 @@ class Owner(object):
     """
     def __init__(self, id, name):
         self.id = id
+        if not (name is None or isinstance(name, six.string_types)):
+            raise TypeError('name must be a string or None')
         self.name = name
 
 
diff --git a/test/unit/common/middleware/s3api/test_s3api.py b/test/unit/common/middleware/s3api/test_s3api.py
index 9edd72108..b89bb9907 100644
--- a/test/unit/common/middleware/s3api/test_s3api.py
+++ b/test/unit/common/middleware/s3api/test_s3api.py
@@ -22,7 +22,6 @@ import hashlib
 import mock
 import requests
 import json
-import copy
 import six
 from six.moves.urllib.parse import unquote, quote
 
@@ -34,6 +33,7 @@ from swift.common.swob import Request
 from keystonemiddleware.auth_token import AuthProtocol
 from keystoneauth1.access import AccessInfoV2
 
+from test.unit import debug_logger
 from test.unit.common.middleware.s3api import S3ApiTestCase
 from test.unit.common.middleware.s3api.helpers import FakeSwift
 from test.unit.common.middleware.s3api.test_s3token import \
@@ -335,7 +335,7 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         self.assertEqual(status.split()[0], '200', body)
         for _, _, headers in self.swift.calls_with_headers:
             self.assertNotIn('Authorization', headers)
-            self.assertIsNone(headers['X-Auth-Token'])
+            self.assertNotIn('X-Auth-Token', headers)
 
     def test_signed_urls_v4_bad_credential(self):
         def test(credential, message, extra=b''):
@@ -767,7 +767,7 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         self.assertEqual(status.split()[0], '200', body)
         for _, _, headers in self.swift.calls_with_headers:
             self.assertEqual(authz_header, headers['Authorization'])
-            self.assertIsNone(headers['X-Auth-Token'])
+            self.assertNotIn('X-Auth-Token', headers)
 
     def test_signature_v4_no_date(self):
         environ = {
@@ -1096,6 +1096,7 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         self.s3_token = S3Token(
             self.keystone_auth, {'auth_uri': 'https://fakehost/identity'})
         self.s3api = S3ApiMiddleware(self.s3_token, self.conf)
+        self.s3api.logger = debug_logger()
         req = Request.blank(
             '/bucket',
             environ={'REQUEST_METHOD': 'PUT'},
@@ -1122,6 +1123,7 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         self.s3_token = S3Token(
             self.keystone_auth, {'auth_uri': 'https://fakehost/identity'})
         self.s3api = S3ApiMiddleware(self.s3_token, self.conf)
+        self.s3api.logger = debug_logger()
         req = Request.blank(
             '/bucket',
             environ={'REQUEST_METHOD': 'PUT'},
@@ -1150,6 +1152,7 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         self.s3_token = S3Token(
             self.auth_token, {'auth_uri': 'https://fakehost/identity'})
         self.s3api = S3ApiMiddleware(self.s3_token, self.conf)
+        self.s3api.logger = debug_logger()
         req = Request.blank(
             '/bucket',
             environ={'REQUEST_METHOD': 'PUT'},
@@ -1162,6 +1165,8 @@ class TestS3ApiMiddleware(S3ApiTestCase):
         with patch.object(self.s3_token, '_json_request') as mock_req:
             with patch.object(self.auth_token,
                               '_do_fetch_token') as mock_fetch:
+                # sanity check
+                self.assertIn('id', GOOD_RESPONSE_V2['access']['token'])
                 mock_resp = requests.Response()
                 mock_resp._content = json.dumps(
                     GOOD_RESPONSE_V2).encode('ascii')
@@ -1174,21 +1179,28 @@ class TestS3ApiMiddleware(S3ApiTestCase):
                 mock_fetch.return_value = (MagicMock(), mock_access_info)
 
                 status, headers, body = self.call_s3api(req)
-                self.assertEqual(body, b'')
+                # Even though s3token got a token back from keystone, we drop
+                # it on the floor, resulting in a 401 Unauthorized at
+                # `swift.common.middleware.keystoneauth` because
+                # keystonemiddleware's auth_token strips out all auth headers,
+                # significantly 'X-Identity-Status'. Without a token, it then
+                # sets 'X-Identity-Status: Invalid' and never contacts
+                # Keystone.
+                self.assertEqual('403 Forbidden', status)
                 self.assertEqual(1, mock_req.call_count)
-                # With X-Auth-Token, auth_token will call _do_fetch_token to
-                # connect to keystone in auth_token, again
-                self.assertEqual(1, mock_fetch.call_count)
+                # it never even tries to contact keystone
+                self.assertEqual(0, mock_fetch.call_count)
 
-    def test_s3api_with_s3_token_no_pass_token_to_auth_token(self):
+    def test_s3api_with_only_s3_token_in_s3acl(self):
         self.swift = FakeSwift()
         self.keystone_auth = KeystoneAuth(
             self.swift, {'operator_roles': 'swift-user'})
-        self.auth_token = AuthProtocol(
-            self.keystone_auth, {'delay_auth_decision': 'True'})
         self.s3_token = S3Token(
-            self.auth_token, {'auth_uri': 'https://fakehost/identity'})
+            self.keystone_auth, {'auth_uri': 'https://fakehost/identity'})
+
+        self.conf['s3_acl'] = True
         self.s3api = S3ApiMiddleware(self.s3_token, self.conf)
+        self.s3api.logger = debug_logger()
         req = Request.blank(
             '/bucket',
             environ={'REQUEST_METHOD': 'PUT'},
@@ -1196,41 +1208,21 @@ class TestS3ApiMiddleware(S3ApiTestCase):
                      'Date': self.get_date_header()})
         self.swift.register('PUT', '/v1/AUTH_TENANT_ID/bucket',
                             swob.HTTPCreated, {}, None)
-        self.swift.register('HEAD', '/v1/AUTH_TENANT_ID',
-                            swob.HTTPOk, {}, None)
+        # For now, s3 acl commits the bucket owner acl via POST
+        # after PUT container so we need to register the resposne here
+        self.swift.register('POST', '/v1/AUTH_TENANT_ID/bucket',
+                            swob.HTTPNoContent, {}, None)
+        self.swift.register('TEST', '/v1/AUTH_TENANT_ID',
+                            swob.HTTPMethodNotAllowed, {}, None)
         with patch.object(self.s3_token, '_json_request') as mock_req:
-            with patch.object(self.auth_token,
-                              '_do_fetch_token') as mock_fetch:
-                mock_resp = requests.Response()
-                no_token_id_good_resp = copy.deepcopy(GOOD_RESPONSE_V2)
-                # delete token id
-                del no_token_id_good_resp['access']['token']['id']
-                mock_resp._content = json.dumps(
-                    no_token_id_good_resp).encode('ascii')
-                mock_resp.status_code = 201
-                mock_req.return_value = mock_resp
-
-                mock_access_info = AccessInfoV2(GOOD_RESPONSE_V2)
-                mock_access_info.will_expire_soon = \
-                    lambda stale_duration: False
-                mock_fetch.return_value = (MagicMock(), mock_access_info)
-
-                status, headers, body = self.call_s3api(req)
-                # No token provided from keystone result in 401 Unauthorized
-                # at `swift.common.middleware.keystoneauth` because auth_token
-                # will remove all auth headers including 'X-Identity-Status'[1]
-                # and then, set X-Identity-Status: Invalid at [2]
-                #
-                # 1: https://github.com/openstack/keystonemiddleware/blob/
-                #    master/keystonemiddleware/auth_token/__init__.py#L620
-                # 2: https://github.com/openstack/keystonemiddleware/blob/
-                #    master/keystonemiddleware/auth_token/__init__.py#L627-L629
+            mock_resp = requests.Response()
+            mock_resp._content = json.dumps(GOOD_RESPONSE_V2).encode('ascii')
+            mock_resp.status_code = 201
+            mock_req.return_value = mock_resp
 
-                self.assertEqual('403 Forbidden', status)
-                self.assertEqual(1, mock_req.call_count)
-                # if no token provided from keystone, we can skip the call to
-                # fetch the token
-                self.assertEqual(0, mock_fetch.call_count)
+            status, headers, body = self.call_s3api(req)
+            self.assertEqual(body, b'')
+            self.assertEqual(1, mock_req.call_count)
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/test/unit/common/middleware/s3api/test_s3request.py b/test/unit/common/middleware/s3api/test_s3request.py
index f9e4acbf5..bc8b64841 100644
--- a/test/unit/common/middleware/s3api/test_s3request.py
+++ b/test/unit/common/middleware/s3api/test_s3request.py
@@ -249,9 +249,11 @@ class TestRequest(S3ApiTestCase):
             m_swift_resp.return_value = FakeSwiftResponse()
             s3_req = S3AclRequest(req.environ, MagicMock())
             self.assertNotIn('s3api.auth_details', s3_req.environ)
-            self.assertEqual(s3_req.token, 'token')
 
     def test_to_swift_req_Authorization_not_exist_in_swreq(self):
+        # the difference from
+        # test_authenticate_delete_Authorization_from_s3req_headers above is
+        # this method asserts *to_swift_req* method.
         container = 'bucket'
         obj = 'obj'
         method = 'GET'
@@ -264,9 +266,12 @@ class TestRequest(S3ApiTestCase):
 
             m_swift_resp.return_value = FakeSwiftResponse()
             s3_req = S3AclRequest(req.environ, MagicMock())
+            # Yes, we *want* to assert this
             sw_req = s3_req.to_swift_req(method, container, obj)
+            # So since the result of S3AclRequest init tests and with this
+            # result to_swift_req doesn't add Authorization header and token
             self.assertNotIn('s3api.auth_details', sw_req.environ)
-            self.assertEqual(sw_req.headers['X-Auth-Token'], 'token')
+            self.assertNotIn('X-Auth-Token', sw_req.headers)
 
     def test_to_swift_req_subrequest_proxy_access_log(self):
         container = 'bucket'
diff --git a/test/unit/common/middleware/s3api/test_s3token.py b/test/unit/common/middleware/s3api/test_s3token.py
index aaf15cba6..6b43d1acc 100644
--- a/test/unit/common/middleware/s3api/test_s3token.py
+++ b/test/unit/common/middleware/s3api/test_s3token.py
@@ -196,11 +196,11 @@ class S3TokenMiddlewareTestGood(S3TokenMiddlewareTestBase):
         self.middleware(req.environ, self.start_fake_response)
         self.assertEqual(self.response_status, 200)
 
-    def _assert_authorized(self, req, expect_token=True,
-                           account_path='/v1/AUTH_TENANT_ID/'):
+    def _assert_authorized(self, req, account_path='/v1/AUTH_TENANT_ID/'):
         self.assertTrue(
             req.path.startswith(account_path),
             '%r does not start with %r' % (req.path, account_path))
+        self.assertNotIn('X-Auth-Token', req.headers)
         expected_headers = {
             'X-Identity-Status': 'Confirmed',
             'X-Roles': 'swift-user,_member_',
@@ -210,12 +210,8 @@ class S3TokenMiddlewareTestGood(S3TokenMiddlewareTestBase):
             'X-Tenant-Name': 'TENANT_NAME',
             'X-Project-Id': 'TENANT_ID',
             'X-Project-Name': 'TENANT_NAME',
-            'X-Auth-Token': 'TOKEN_ID',
         }
         for header, value in expected_headers.items():
-            if header == 'X-Auth-Token' and not expect_token:
-                self.assertNotIn(header, req.headers)
-                continue
             self.assertIn(header, req.headers)
             self.assertEqual(value, req.headers[header])
             # WSGI wants native strings for headers
@@ -253,7 +249,7 @@ class S3TokenMiddlewareTestGood(S3TokenMiddlewareTestBase):
             'string_to_sign': u'token',
         }
         req.get_response(self.middleware)
-        self._assert_authorized(req, expect_token=False)
+        self._assert_authorized(req)
 
     def test_authorized_bytes(self):
         req = Request.blank('/v1/AUTH_cfa/c/o')
@@ -533,7 +529,7 @@ class S3TokenMiddlewareTestGood(S3TokenMiddlewareTestBase):
 
         cache = MOCK_CACHE_FROM_ENV.return_value
 
-        fake_cache_response = ({}, 'token_id', {'id': 'tenant_id'}, 'secret')
+        fake_cache_response = ({}, {'id': 'tenant_id'}, 'secret')
         cache.get.return_value = fake_cache_response
 
         MOCK_REQUEST.return_value = TestResponse({
@@ -600,8 +596,7 @@ class S3TokenMiddlewareTestGood(S3TokenMiddlewareTestBase):
 
         self.assertTrue(MOCK_REQUEST.called)
         tenant = GOOD_RESPONSE_V2['access']['token']['tenant']
-        token = GOOD_RESPONSE_V2['access']['token']['id']
-        expected_cache = (expected_headers, token, tenant, 'secret')
+        expected_cache = (expected_headers, tenant, 'secret')
         cache.set.assert_called_once_with('s3secret/access', expected_cache,
                                           time=20)