Remove redundant/legacy is_admin logic

The original model used was to assert is_admin on the object context
which was actually used in only one place in ironic's code. Redudnantly
of course.

This is an excess call of is_admin on all API invocations, and is
simply not necessary as individual calls have API policy checking
and is_admin was only being consulted in the glance service utils...
However, the glance service utils also confirmed it should be able
to access glance if there was an auth_token present on the request
which should also always be the case. This was somewhat identified
as redundant/possible bug during the Wallaby cycle and appears to
be fine to remove

This does *not* remove the deprecated rule. At present, it appears
that rule may not be removed until after Xena.

Change-Id: I5a176f51db93d2a2238496f6955c1c7d9a79c548
This commit is contained in:
Julia Kreger
2021-06-15 17:26:44 -07:00
parent 0f865495b8
commit f91915d4cd
6 changed files with 5 additions and 29 deletions

View File

@@ -100,9 +100,6 @@ class ContextHook(hooks.PecanHook):
if cfg.CONF.auth_strategy != 'keystone':
ctx.auth_token = None
creds = ctx.to_policy_values()
is_admin = policy.check('is_admin', creds, creds)
ctx.is_admin = is_admin
policy_deprecation_check()
state.request.context = ctx

View File

@@ -52,11 +52,7 @@ class RequestContext(context.RequestContext):
def get_admin_context():
"""Create an administrator context."""
# TODO(TheJulia): Revise in Xena, is_admin should
# no longer be a default, much less passed as it is
# deprecated.
context = RequestContext(auth_token=None,
project_id=None,
is_admin=True,
overwrite=False)
return context

View File

@@ -114,12 +114,7 @@ def is_image_available(context, image):
# be able to be used.
return True
# TODO(TheJulia): This is potentially a bug below. Admin context doesn't
# necessarilly mean the object is *actually* accessible. We should likely
# just ask glance... Although everything should also have an auth_token
# as noted above. Ultimately we need to tease the is_admin logic apart
# and treat things appropriately by checking them as needed.
if getattr(image, 'visibility', None) == 'public' or context.is_admin:
if getattr(image, 'visibility', None) == 'public':
return True
return (context.project_id

View File

@@ -167,7 +167,6 @@ class TestListNodes(test_api_base.BaseApiTest):
mock.call('baremetal:node:get:filter_threshold',
mock.ANY, mock.ANY)])
mock_check.assert_has_calls([
mock.call('is_admin', mock.ANY, mock.ANY),
mock.call('show_password', mock.ANY, mock.ANY),
mock.call('show_instance_secrets', mock.ANY, mock.ANY),
# Last error is populated above and should trigger a check.

View File

@@ -220,13 +220,11 @@ class TestContextHook(base.BaseApiTest):
mock_ctx.from_environ.return_value = ctx
policy_dict = {'user_id': 'foo'} # Lots of other values here
ctx.to_policy_values.return_value = policy_dict
mock_policy.return_value = is_admin
mock_policy.return_value = False
context_hook.before(reqstate)
creds_dict = {'is_public_api': is_public_api}
mock_ctx.from_environ.assert_called_once_with(environ, **creds_dict)
mock_policy.assert_called_once_with('is_admin', policy_dict,
policy_dict)
self.assertIs(is_admin, ctx.is_admin)
mock_policy.assert_not_called()
if auth_strategy == 'noauth':
self.assertIsNone(ctx.auth_token)
return context_hook, reqstate
@@ -234,18 +232,14 @@ class TestContextHook(base.BaseApiTest):
def test_context_hook_not_admin(self):
self._test_context_hook()
def test_context_hook_admin(self):
self._test_context_hook(is_admin=True)
def test_context_hook_public_api(self):
self._test_context_hook(is_admin=True, is_public_api=True)
self._test_context_hook(is_public_api=True)
def test_context_hook_noauth_token_removed(self):
self._test_context_hook(auth_strategy='noauth')
def test_context_hook_after_add_request_id(self):
context_hook, reqstate = self._test_context_hook(is_admin=True,
request_id='fake-id')
context_hook, reqstate = self._test_context_hook(request_id='fake-id')
context_hook.after(reqstate)
self.assertEqual('fake-id',
reqstate.response.headers['Openstack-Request-Id'])

View File

@@ -26,7 +26,6 @@ class RequestContextTestCase(tests_base.TestCase):
"user_id": "user1",
"project_id": "project1",
"project_name": "somename",
'is_admin': True,
'read_only': True,
'show_deleted': True,
'request_id': 'id1',
@@ -51,10 +50,6 @@ class RequestContextTestCase(tests_base.TestCase):
self.assertEqual('somename', ctx_dict['project_name'])
self.assertTrue(ctx_dict['is_public_api'])
def test_get_admin_context(self):
admin_context = context.get_admin_context()
self.assertTrue(admin_context.is_admin)
@mock.patch.object(oslo_context, 'get_current', autospec=True)
def test_thread_without_context(self, context_get_mock):
self.context.update_store = mock.Mock()