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
(cherry picked from commit fe8107a817
)
This commit is contained in:
parent
671cbad9b9
commit
63f6732d8c
|
@ -139,10 +139,10 @@ class PolicyHook(hooks.PecanHook):
|
||||||
# If a tenant is modifying it's own object, it's safe to
|
# If a tenant is modifying it's own object, it's safe to
|
||||||
# return a 403. Otherwise, pretend that it doesn't exist
|
# return a 403. Otherwise, pretend that it doesn't exist
|
||||||
# to avoid giving away information.
|
# to avoid giving away information.
|
||||||
orig_item_tenant_id = item.get('tenant_id')
|
controller = utils.get_controller(state)
|
||||||
if (needs_prefetch and
|
s_action = controller.plugin_handlers[controller.SHOW]
|
||||||
(neutron_context.tenant_id != orig_item_tenant_id or
|
if not policy.check(neutron_context, s_action, item,
|
||||||
orig_item_tenant_id is None)):
|
pluralized=collection):
|
||||||
ctxt.reraise = False
|
ctxt.reraise = False
|
||||||
msg = _('The resource could not be found.')
|
msg = _('The resource could not be found.')
|
||||||
raise webob.exc.HTTPNotFound(msg)
|
raise webob.exc.HTTPNotFound(msg)
|
||||||
|
@ -190,10 +190,10 @@ class PolicyHook(hooks.PecanHook):
|
||||||
except oslo_policy.PolicyNotAuthorized:
|
except oslo_policy.PolicyNotAuthorized:
|
||||||
# This exception must be explicitly caught as the exception
|
# This exception must be explicitly caught as the exception
|
||||||
# translation hook won't be called if an error occurs in the
|
# 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
|
# we have to set the status_code here to prevent the catch_errors
|
||||||
# middleware from turning this into a 500.
|
# middleware from turning this into a 500.
|
||||||
state.response.status_code = 403
|
state.response.status_code = 404
|
||||||
return
|
return
|
||||||
|
|
||||||
if is_single:
|
if is_single:
|
||||||
|
|
|
@ -209,6 +209,23 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest):
|
||||||
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||||
self.assertEqual(0, self.mock_plugin.update_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):
|
def test_before_on_delete_not_authorized(self):
|
||||||
# The policy hook here should load the resource, and therefore we must
|
# The policy hook here should load the resource, and therefore we must
|
||||||
# mock a get response
|
# 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(1, self.mock_plugin.get_meh.call_count)
|
||||||
self.assertEqual(0, self.mock_plugin.delete_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
|
# 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 = {
|
self.mock_plugin.get_meh.return_value = {
|
||||||
'id': 'yyy',
|
'id': 'yyy',
|
||||||
'attr': 'meh',
|
'attr': 'meh',
|
||||||
|
@ -240,7 +258,7 @@ class TestPolicyEnforcementHook(test_functional.PecanFunctionalTest):
|
||||||
response = self.app.get('/v2.0/mehs/yyy.json',
|
response = self.app.get('/v2.0/mehs/yyy.json',
|
||||||
headers={'X-Project-Id': 'tenid'},
|
headers={'X-Project-Id': 'tenid'},
|
||||||
expect_errors=True)
|
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)
|
self.assertEqual(1, self.mock_plugin.get_meh.call_count)
|
||||||
|
|
||||||
def test_after_on_get_excludes_admin_attribute(self):
|
def test_after_on_get_excludes_admin_attribute(self):
|
||||||
|
|
Loading…
Reference in New Issue