From 82101a36d348ad0c3de10f455878828f4263ffe2 Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Tue, 1 Jul 2014 16:55:11 -0700 Subject: [PATCH] HEAD responses should return same status as GET According to the HTTP spec, a HEAD request should return the same status and headers as the GET request (including content-type and content-length). The HEAD request simply strips out the body and returns no body. Any case where HEAD routing returned a different status code than GET, now returns the same status and headers. Any case where HEAD was supported where GET was not supported now supports both GET and HEAD. The wsgi.render_response code now handles HEAD appropriately and will maintain headers while enforcing no body data is returned. The bulk of this change is to support the same behavior between deploying Keystone under eventlet and under HTTPD + mod_wsgi. In the case of deploying under HTTPD + mod_wsgi, there are cases where mod_wsgi will turn a HEAD request into a GET request to ensure that the proper response is rendered. With these changes all HEAD responses will respond in the same manner under either eventlet or mod_wsgi. Change-Id: I13ce159cbe9739d4bf5d321fc4bd069245f32734 Closes-Bug: #1334368 --- keystone/assignment/routers.py | 12 ++-- keystone/auth/controllers.py | 7 +- keystone/auth/routers.py | 4 ++ keystone/common/wsgi.py | 28 +++++++- keystone/contrib/endpoint_filter/routers.py | 2 +- keystone/identity/routers.py | 2 +- keystone/tests/test_content_types.py | 2 +- keystone/tests/test_v3.py | 1 + keystone/tests/test_v3_auth.py | 80 ++++++++++----------- keystone/tests/test_v3_identity.py | 2 +- keystone/tests/test_wsgi.py | 7 ++ keystone/token/controllers.py | 5 +- keystone/token/routers.py | 4 ++ keystone/trust/routers.py | 7 +- 14 files changed, 102 insertions(+), 61 deletions(-) diff --git a/keystone/assignment/routers.py b/keystone/assignment/routers.py index 5bab5fd822..74d3baed59 100644 --- a/keystone/assignment/routers.py +++ b/keystone/assignment/routers.py @@ -82,11 +82,11 @@ def append_v3_routers(mapper, routers): mapper.connect('/projects/{project_id}/users/{user_id}/roles/{role_id}', controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect('/projects/{project_id}/groups/{group_id}/roles/{role_id}', controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect('/projects/{project_id}/users/{user_id}/roles', controller=role_controller, action='list_grants', @@ -114,11 +114,11 @@ def append_v3_routers(mapper, routers): mapper.connect('/domains/{domain_id}/users/{user_id}/roles/{role_id}', controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect('/domains/{domain_id}/groups/{group_id}/roles/{role_id}', controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect('/domains/{domain_id}/users/{user_id}/roles', controller=role_controller, action='list_grants', @@ -151,12 +151,12 @@ def append_v3_routers(mapper, routers): '/roles/{role_id}/inherited_to_projects'), controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect(('/OS-INHERIT/domains/{domain_id}/groups/{group_id}' '/roles/{role_id}/inherited_to_projects'), controller=role_controller, action='check_grant', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect(('/OS-INHERIT/domains/{domain_id}/users/{user_id}' '/roles/inherited_to_projects'), controller=role_controller, diff --git a/keystone/auth/controllers.py b/keystone/auth/controllers.py index f3c1b00a54..14dbd87cb2 100644 --- a/keystone/auth/controllers.py +++ b/keystone/auth/controllers.py @@ -490,7 +490,12 @@ class Auth(controller.V3Controller): @controller.protected() def check_token(self, context): token_id = context.get('subject_token_id') - self.token_provider_api.check_v3_token(token_id) + token_data = self.token_provider_api.validate_v3_token( + token_id) + # NOTE(morganfainberg): The code in + # ``keystone.common.wsgi.render_response`` will remove the content + # body. + return render_token_data_response(token_id, token_data) @controller.protected() def revoke_token(self, context): diff --git a/keystone/auth/routers.py b/keystone/auth/routers.py index c15e5476f5..9ec2227848 100644 --- a/keystone/auth/routers.py +++ b/keystone/auth/routers.py @@ -22,6 +22,10 @@ def append_v3_routers(mapper, routers): controller=auth_controller, action='authenticate_for_token', conditions=dict(method=['POST'])) + # NOTE(morganfainberg): For policy enforcement reasons, the + # ``validate_token_head`` method is still used for HEAD requests. + # The controller method makes the same call as the validate_token + # call and lets wsgi.render_response remove the body data. mapper.connect('/auth/tokens', controller=auth_controller, action='check_token', diff --git a/keystone/common/wsgi.py b/keystone/common/wsgi.py index f570d5798d..a103deb3ce 100644 --- a/keystone/common/wsgi.py +++ b/keystone/common/wsgi.py @@ -200,6 +200,11 @@ class Application(BaseApplication): # TODO(termie): do some basic normalization on methods method = getattr(self, action) + # NOTE(morganfainberg): use the request method to normalize the + # response code between GET and HEAD requests. The HTTP status should + # be the same. + req_method = req.environ['REQUEST_METHOD'].upper() + # NOTE(vish): make sure we have no unicode keys for py2.6. params = self._normalize_dict(params) @@ -236,7 +241,8 @@ class Application(BaseApplication): return result response_code = self._get_response_code(req) - return render_response(body=result, status=response_code) + return render_response(body=result, status=response_code, + method=req_method) def _get_response_code(self, req): req_method = req.environ['REQUEST_METHOD'] @@ -598,7 +604,7 @@ class ExtensionRouter(Router): return _factory -def render_response(body=None, status=None, headers=None): +def render_response(body=None, status=None, headers=None, method=None): """Forms a WSGI response.""" if headers is None: headers = [] @@ -621,10 +627,26 @@ def render_response(body=None, status=None, headers=None): headers.append(('Content-Type', 'application/json')) status = status or (200, 'OK') - return webob.Response(body=body, + resp = webob.Response(body=body, status='%s %s' % status, headerlist=headers) + if method == 'HEAD': + # NOTE(morganfainberg): HEAD requests should return the same status + # as a GET request and same headers (including content-type and + # content-length). The webob.Response object automatically changes + # content-length (and other headers) if the body is set to b''. Capture + # all headers and reset them on the response object after clearing the + # body. The body can only be set to a binary-type (not TextType or + # NoneType), so b'' is used here and should be compatible with + # both py2x and py3x. + stored_headers = resp.headers.copy() + resp.body = b'' + for header, value in six.iteritems(stored_headers): + resp.headers[header] = value + + return resp + def render_exception(error, context=None, request=None, user_locale=None): """Forms a WSGI response based on the current error.""" diff --git a/keystone/contrib/endpoint_filter/routers.py b/keystone/contrib/endpoint_filter/routers.py index 91f09bd059..3ca0ca0ea8 100644 --- a/keystone/contrib/endpoint_filter/routers.py +++ b/keystone/contrib/endpoint_filter/routers.py @@ -34,7 +34,7 @@ class EndpointFilterExtension(wsgi.ExtensionRouter): mapper.connect(self.PATH_PREFIX + self.PATH_PROJECT_ENDPOINT, controller=endpoint_filter_controller, action='check_endpoint_in_project', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect(self.PATH_PREFIX + '/projects/{project_id}/endpoints', controller=endpoint_filter_controller, action='list_endpoints_for_project', diff --git a/keystone/identity/routers.py b/keystone/identity/routers.py index 6306dd718a..a892b0c45c 100644 --- a/keystone/identity/routers.py +++ b/keystone/identity/routers.py @@ -50,7 +50,7 @@ def append_v3_routers(mapper, routers): mapper.connect('/groups/{group_id}/users/{user_id}', controller=user_controller, action='check_user_in_group', - conditions=dict(method=['HEAD'])) + conditions=dict(method=['GET', 'HEAD'])) mapper.connect('/groups/{group_id}/users/{user_id}', controller=user_controller, diff --git a/keystone/tests/test_content_types.py b/keystone/tests/test_content_types.py index 73df5b404c..ab7b1d8216 100644 --- a/keystone/tests/test_content_types.py +++ b/keystone/tests/test_content_types.py @@ -246,7 +246,7 @@ class CoreApiTests(object): 'token_id': token, }, token=token, - expected_status=204) + expected_status=200) def test_endpoints(self): token = self.get_scoped_token() diff --git a/keystone/tests/test_v3.py b/keystone/tests/test_v3.py index 9b0c97b37d..972b63081e 100644 --- a/keystone/tests/test_v3.py +++ b/keystone/tests/test_v3.py @@ -465,6 +465,7 @@ class RestfulTestCase(tests.SQLDriverOverrides, rest.RestfulTestCase, r = self.v3_request(method='HEAD', path=path, **kwargs) if 'expected_status' not in kwargs: self.assertResponseStatus(r, 204) + self.assertEqual('', r.body) return r def post(self, path, **kwargs): diff --git a/keystone/tests/test_v3_auth.py b/keystone/tests/test_v3_auth.py index 704d3be627..414077ea6b 100644 --- a/keystone/tests/test_v3_auth.py +++ b/keystone/tests/test_v3_auth.py @@ -376,7 +376,7 @@ class TokenAPITests(object): self.assertEqual(expires, r.result['token']['expires_at']) def test_check_token(self): - self.head('/auth/tokens', headers=self.headers, expected_status=204) + self.head('/auth/tokens', headers=self.headers, expected_status=200) def test_validate_token(self): r = self.get('/auth/tokens', headers=self.headers) @@ -509,9 +509,9 @@ class TestTokenRevokeSelfAndAdmin(test_v3.RestfulTestCase): domain_name=self.domainA['name'])) adminA_token = r.headers.get('X-Subject-Token') - self.head('/auth/tokens', headers=headers, expected_status=204, + self.head('/auth/tokens', headers=headers, expected_status=200, token=adminA_token) - self.head('/auth/tokens', headers=headers, expected_status=204, + self.head('/auth/tokens', headers=headers, expected_status=200, token=user_token) self.delete('/auth/tokens', headers=headers, expected_status=204, token=user_token) @@ -548,9 +548,9 @@ class TestTokenRevokeSelfAndAdmin(test_v3.RestfulTestCase): domain_name=self.domainA['name'])) adminA_token = r.headers.get('X-Subject-Token') - self.head('/auth/tokens', headers=headers, expected_status=204, + self.head('/auth/tokens', headers=headers, expected_status=200, token=adminA_token) - self.head('/auth/tokens', headers=headers, expected_status=204, + self.head('/auth/tokens', headers=headers, expected_status=200, token=user_token) self.delete('/auth/tokens', headers=headers, expected_status=204, token=adminA_token) @@ -725,10 +725,10 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # confirm both tokens are valid self.head('/auth/tokens', headers={'X-Subject-Token': unscoped_token}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': scoped_token}, - expected_status=204) + expected_status=200) # create a new role role = self.new_role_ref() @@ -744,10 +744,10 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # both tokens should remain valid self.head('/auth/tokens', headers={'X-Subject-Token': unscoped_token}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': scoped_token}, - expected_status=204) + expected_status=200) def test_deleting_user_grant_revokes_token(self): """Test deleting a user grant revokes token. @@ -768,7 +768,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm token is valid self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # Delete the grant, which should invalidate the token grant_url = ( '/projects/%(project_id)s/users/%(user_id)s/' @@ -875,19 +875,19 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm tokens are valid self.head('/auth/tokens', headers={'X-Subject-Token': tokenA}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': tokenB}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': tokenC}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': tokenD}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': tokenE}, - expected_status=204) + expected_status=200) # Delete the role, which should invalidate the tokens role_url = '/roles/%s' % self.role1['id'] @@ -910,7 +910,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # ...but the one using role2 is still valid self.head('/auth/tokens', headers={'X-Subject-Token': tokenC}, - expected_status=204) + expected_status=200) def test_domain_user_role_assignment_maintains_token(self): """Test user-domain role assignment maintains existing token. @@ -931,7 +931,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm token is valid self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # Assign a role, which should not affect the token grant_url = ( '/domains/%(domain_id)s/users/%(user_id)s/' @@ -942,7 +942,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): self.put(grant_url) self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) def test_disabling_project_revokes_token(self): resp = self.post( @@ -956,7 +956,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # confirm token is valid self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # disable the project, which should invalidate the token self.patch( @@ -987,7 +987,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # confirm token is valid self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # delete the project, which should invalidate the token self.delete( @@ -1040,13 +1040,13 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm tokens are valid self.head('/auth/tokens', headers={'X-Subject-Token': token1}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': token2}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': token3}, - expected_status=204) + expected_status=200) # Delete the group grant, which should invalidate the # tokens for user1 and user2 grant_url = ( @@ -1065,7 +1065,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # But user3's token should still be valid self.head('/auth/tokens', headers={'X-Subject-Token': token3}, - expected_status=204) + expected_status=200) def test_domain_group_role_assignment_maintains_token(self): """Test domain-group role assignment maintains existing token. @@ -1086,7 +1086,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm token is valid self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # Delete the grant, which should invalidate the token grant_url = ( '/domains/%(domain_id)s/groups/%(group_id)s/' @@ -1097,7 +1097,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): self.put(grant_url) self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) def test_group_membership_changes_revokes_token(self): """Test add/removal to/from group revokes token. @@ -1129,10 +1129,10 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # Confirm tokens are valid self.head('/auth/tokens', headers={'X-Subject-Token': token1}, - expected_status=204) + expected_status=200) self.head('/auth/tokens', headers={'X-Subject-Token': token2}, - expected_status=204) + expected_status=200) # Remove user1 from group1, which should invalidate # the token self.delete('/groups/%(group_id)s/users/%(user_id)s' % { @@ -1144,14 +1144,14 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # But user2's token should still be valid self.head('/auth/tokens', headers={'X-Subject-Token': token2}, - expected_status=204) + expected_status=200) # Adding user2 to a group should not invalidate token self.put('/groups/%(group_id)s/users/%(user_id)s' % { 'group_id': self.group2['id'], 'user_id': self.user2['id']}) self.head('/auth/tokens', headers={'X-Subject-Token': token2}, - expected_status=204) + expected_status=200) def test_removing_role_assignment_does_not_affect_other_users(self): """Revoking a role from one user should not affect other users.""" @@ -1198,7 +1198,7 @@ class TestTokenRevokeById(test_v3.RestfulTestCase): # authorization for the second user should still succeed self.head('/auth/tokens', headers={'X-Subject-Token': user3_token}, - expected_status=204) + expected_status=200) self.post( '/auth/tokens', body=self.build_authentication_request( @@ -1279,7 +1279,7 @@ class TestTokenRevokeApi(TestTokenRevokeById): def test_revoke_token(self): scoped_token = self.get_scoped_token() headers = {'X-Subject-Token': scoped_token} - self.head('/auth/tokens', headers=headers, expected_status=204) + self.head('/auth/tokens', headers=headers, expected_status=200) self.delete('/auth/tokens', headers=headers, expected_status=204) self.head('/auth/tokens', headers=headers, expected_status=404) events_response = self.get('/OS-REVOKE/events', @@ -1301,7 +1301,7 @@ class TestTokenRevokeApi(TestTokenRevokeById): def test_revoke_v2_token(self): token = self.get_v2_token() headers = {'X-Subject-Token': token} - self.head('/auth/tokens', headers=headers, expected_status=204) + self.head('/auth/tokens', headers=headers, expected_status=200) self.delete('/auth/tokens', headers=headers, expected_status=204) self.head('/auth/tokens', headers=headers, expected_status=404) events_response = self.get('/OS-REVOKE/events', @@ -1369,11 +1369,11 @@ class TestTokenRevokeApi(TestTokenRevokeById): scoped_token = self.get_scoped_token() headers_unrevoked = {'X-Subject-Token': scoped_token} - self.head('/auth/tokens', headers=headers, expected_status=204) - self.head('/auth/tokens', headers=headers2, expected_status=204) - self.head('/auth/tokens', headers=headers3, expected_status=204) + self.head('/auth/tokens', headers=headers, expected_status=200) + self.head('/auth/tokens', headers=headers2, expected_status=200) + self.head('/auth/tokens', headers=headers3, expected_status=200) self.head('/auth/tokens', headers=headers_unrevoked, - expected_status=204) + expected_status=200) self.delete('/auth/tokens', headers=headers, expected_status=204) # NOTE(ayoung): not deleting token3, as it should be deleted @@ -1390,7 +1390,7 @@ class TestTokenRevokeApi(TestTokenRevokeById): self.head('/auth/tokens', headers=headers2, expected_status=404) self.head('/auth/tokens', headers=headers3, expected_status=404) self.head('/auth/tokens', headers=headers_unrevoked, - expected_status=204) + expected_status=200) def test_list_with_filter(self): @@ -2643,7 +2643,7 @@ class TestTrustAuth(test_v3.RestfulTestCase): '/OS-TRUST/trusts/%(trust_id)s/roles/%(role_id)s' % { 'trust_id': trust['id'], 'role_id': self.role['id']}, - expected_status=204) + expected_status=200) r = self.get( '/OS-TRUST/trusts/%(trust_id)s/roles/%(role_id)s' % { 'trust_id': trust['id'], @@ -3137,7 +3137,7 @@ class TestTrustAuth(test_v3.RestfulTestCase): 'trust_id': trust['id'], 'role_id': self.role['id']}, auth=auth_data, - expected_status=204) + expected_status=200) r = self.get( '/OS-TRUST/trusts/%(trust_id)s/roles/%(role_id)s' % { diff --git a/keystone/tests/test_v3_identity.py b/keystone/tests/test_v3_identity.py index 87987ecb34..d64c3c858b 100644 --- a/keystone/tests/test_v3_identity.py +++ b/keystone/tests/test_v3_identity.py @@ -646,7 +646,7 @@ class IdentityTestCase(test_v3.RestfulTestCase): # Confirm token is valid for now self.head('/auth/tokens', headers={'X-Subject-Token': token}, - expected_status=204) + expected_status=200) # Now delete the user self.delete('/users/%(user_id)s' % { diff --git a/keystone/tests/test_wsgi.py b/keystone/tests/test_wsgi.py index 469ef9ac56..c09b50a0e2 100644 --- a/keystone/tests/test_wsgi.py +++ b/keystone/tests/test_wsgi.py @@ -156,6 +156,13 @@ class ApplicationTest(BaseWSGITest): self.assertEqual(resp.headers.get('Content-Length'), '0') self.assertIsNone(resp.headers.get('Content-Type')) + def test_render_response_head_with_body(self): + resp = wsgi.render_response({'id': uuid.uuid4().hex}, method='HEAD') + self.assertEqual(resp.status_int, 200) + self.assertEqual(resp.body, b'') + self.assertNotEqual(resp.headers.get('Content-Length'), '0') + self.assertEqual(resp.headers.get('Content-Type'), 'application/json') + def test_application_local_config(self): class FakeApp(wsgi.Application): def __init__(self, *args, **kwargs): diff --git a/keystone/token/controllers.py b/keystone/token/controllers.py index 30d941f395..b345de9f8f 100644 --- a/keystone/token/controllers.py +++ b/keystone/token/controllers.py @@ -396,10 +396,13 @@ class Auth(controller.V2Controller): Identical to ``validate_token``, except does not return a response. + The code in ``keystone.common.wsgi.render_response`` will remove + the content body. + """ # TODO(ayoung) validate against revocation API belongs_to = context['query_string'].get('belongsTo') - self.token_provider_api.check_v2_token(token_id, belongs_to) + return self.token_provider_api.validate_v2_token(token_id, belongs_to) @controller.v2_deprecated @controller.protected() diff --git a/keystone/token/routers.py b/keystone/token/routers.py index b106cb7aca..bcd40ee4da 100644 --- a/keystone/token/routers.py +++ b/keystone/token/routers.py @@ -30,6 +30,10 @@ class Router(wsgi.ComposableRouter): controller=token_controller, action='validate_token', conditions=dict(method=['GET'])) + # NOTE(morganfainberg): For policy enforcement reasons, the + # ``validate_token_head`` method is still used for HEAD requests. + # The controller method makes the same call as the validate_token + # call and lets wsgi.render_response remove the body data. mapper.connect('/tokens/{token_id}', controller=token_controller, action='validate_token_head', diff --git a/keystone/trust/routers.py b/keystone/trust/routers.py index 279f740abf..5f3c66f4b4 100644 --- a/keystone/trust/routers.py +++ b/keystone/trust/routers.py @@ -44,12 +44,7 @@ def append_v3_routers(mapper, routers): action='list_roles_for_trust', conditions=dict(method=['GET'])) - mapper.connect('/OS-TRUST/trusts/{trust_id}/roles/{role_id}', - controller=trust_controller, - action='check_role_for_trust', - conditions=dict(method=['HEAD'])) - mapper.connect('/OS-TRUST/trusts/{trust_id}/roles/{role_id}', controller=trust_controller, action='get_role_for_trust', - conditions=dict(method=['GET'])) + conditions=dict(method=['GET', 'HEAD']))