Pecan: fix logic of hiding authZ failures as 404s
Change [1] altered the behavior of the legacy API controller to do the sane thing and return an HTTP 403 instead of a 404 whenever a user got a policy authorization failure when trying to mutate a resource they have the permission to view. This carries the same logic over to the pecan API. This also adjusts the logic for GET requests to return 404s instead of 403s to match the resource hiding behavior of the old controller. 1. I7a5b0a9e89c8a71490dd74497794a52489f46cd2 Closes-Bug: #1714388 Change-Id: I9e0d288a42bc63c2927bebe9c581b83e6fbe010b
This commit is contained in:
parent
a34c26b916
commit
fe8107a817
|
@ -139,10 +139,10 @@ class PolicyHook(hooks.PecanHook):
|
|||
# If a tenant is modifying it's own object, it's safe to
|
||||
# return a 403. Otherwise, pretend that it doesn't exist
|
||||
# to avoid giving away information.
|
||||
orig_item_tenant_id = item.get('tenant_id')
|
||||
if (needs_prefetch and
|
||||
(neutron_context.tenant_id != orig_item_tenant_id or
|
||||
orig_item_tenant_id is None)):
|
||||
controller = utils.get_controller(state)
|
||||
s_action = controller.plugin_handlers[controller.SHOW]
|
||||
if not policy.check(neutron_context, s_action, item,
|
||||
pluralized=collection):
|
||||
ctxt.reraise = False
|
||||
msg = _('The resource could not be found.')
|
||||
raise webob.exc.HTTPNotFound(msg)
|
||||
|
@ -190,10 +190,10 @@ class PolicyHook(hooks.PecanHook):
|
|||
except oslo_policy.PolicyNotAuthorized:
|
||||
# This exception must be explicitly caught as the exception
|
||||
# translation hook won't be called if an error occurs in the
|
||||
# 'after' handler. Instead of raising an HTTPForbidden exception,
|
||||
# 'after' handler. Instead of raising an HTTPNotFound exception,
|
||||
# we have to set the status_code here to prevent the catch_errors
|
||||
# middleware from turning this into a 500.
|
||||
state.response.status_code = 403
|
||||
state.response.status_code = 404
|
||||
return
|
||||
|
||||
if is_single:
|
||||
|
|
|
@ -209,6 +209,23 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest):
|
|||
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||
self.assertEqual(0, self.mock_plugin.update_meh.call_count)
|
||||
|
||||
def test_before_on_put_not_found_when_not_authorized_to_get(self):
|
||||
# the user won't even have permission to view this resource
|
||||
# so the error on unauthorized updates should be translated into
|
||||
# a 404
|
||||
self.mock_plugin.get_meh.return_value = {
|
||||
'id': 'yyy',
|
||||
'attr': 'meh',
|
||||
'restricted_attr': '',
|
||||
'tenant_id': 'tenid'}
|
||||
response = self.app.put_json('/v2.0/mehs/yyy.json',
|
||||
params={'meh': {'attr': 'meh'}},
|
||||
headers={'X-Project-Id': 'tenid'},
|
||||
expect_errors=True)
|
||||
self.assertEqual(404, response.status_int)
|
||||
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||
self.assertEqual(0, self.mock_plugin.update_meh.call_count)
|
||||
|
||||
def test_before_on_delete_not_authorized(self):
|
||||
# The policy hook here should load the resource, and therefore we must
|
||||
# mock a get response
|
||||
|
@ -227,9 +244,10 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest):
|
|||
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||
self.assertEqual(0, self.mock_plugin.delete_meh.call_count)
|
||||
|
||||
def test_after_on_get_not_authorized(self):
|
||||
def test_after_on_get_not_found(self):
|
||||
# The GET test policy will deny access to anything whose id is not
|
||||
# 'xxx', so the following request should be forbidden
|
||||
# 'xxx', so the following request should be forbidden and presented
|
||||
# to the user as an HTTPNotFound
|
||||
self.mock_plugin.get_meh.return_value = {
|
||||
'id': 'yyy',
|
||||
'attr': 'meh',
|
||||
|
@ -240,7 +258,7 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest):
|
|||
response = self.app.get('/v2.0/mehs/yyy.json',
|
||||
headers={'X-Project-Id': 'tenid'},
|
||||
expect_errors=True)
|
||||
self.assertEqual(403, response.status_int)
|
||||
self.assertEqual(404, response.status_int)
|
||||
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||
|
||||
def test_after_on_get_excludes_admin_attribute(self):
|
||||
|
|
Loading…
Reference in New Issue