Populate existing policy tests
Testing every combination of role, endpoint and policy rule would result in a huge test count, so to make testing the existing policy rules complete and practical, the following guidelines are suggested: - Only the default policy is tested, so inactive rules such as is_node_owner, is_node_lessee are ignored. - Each rule is tested completely on one endpoint which uses it. - A rule (such as baremetal:node:list) which inherits a parent rule (baremetal:node:get) is considered covered by the parent test. - All endpoints need at least one test, but other endpoints which share a fully tested rule only need one denied test which shows that they are covered by some policy. Also adds the initial pass of contributor documentation on how the rbac testing works to try and express the mechanics and what to expect to aid in reviewing/updating/editing the rules. Co-Authored-By: Julia Kreger <juliaashleykreger@gmail.com> Change-Id: I1cd88210e40e42f86464e6a817354620f5ab1d9c
This commit is contained in:
Steve Baker
Julia Kreger
parent
4e5c034187
commit
606549c1c9
@ -32,6 +32,7 @@ primarily for developers.
|
||||
Developing New Notifications <notifications>
|
||||
OSProfiler Tracing <osprofiler-support>
|
||||
Rolling Upgrades <rolling-upgrades>
|
||||
Role Based Access Control Testing <rbac-testing>
|
||||
|
||||
These pages contain information for PTLs, cross-project liaisons, and core
|
||||
reviewers.
|
||||
|
||||
119
doc/source/contributor/rbac-testing.rst
Normal file
119
doc/source/contributor/rbac-testing.rst
Normal file
@ -0,0 +1,119 @@
|
||||
==================================
|
||||
Role Based Acces Control - Testing
|
||||
==================================
|
||||
|
||||
.. todo: This entire file is being added in to provide context for
|
||||
reviewers so we can keep in-line comments to the necessary points
|
||||
in the yaml files. It *IS* written with a forward awareness of the
|
||||
later patches, but it is also broad in nature attempting to provide
|
||||
context to aid in review.
|
||||
|
||||
The Role Based Access control testing is a minor departure from the ironic
|
||||
standard pattern of entirely python based unit testing. In part this was done
|
||||
for purposes of speed and to keep the declaration of the test context.
|
||||
|
||||
This also lended itself to be very useful due to the nature of A/B testing
|
||||
which is requried to properly migrate the Ironic project from a project
|
||||
scoped universe where an ``admin project`` is utilized as the authenticating
|
||||
factor coupled with two custom roles, ``baremetal_admin``, and
|
||||
``baremetal_observer``.
|
||||
|
||||
As a contributor looking back after getting a over a thousand additional tests
|
||||
in place using this method, it definitely helped the speed at which these
|
||||
were created, and then ported to support additional.
|
||||
|
||||
How these tests work
|
||||
====================
|
||||
|
||||
These tests execute API calls through the API layer, using the appropriate
|
||||
verb and header, which settings to prevent the ``keystonemiddleware`` from
|
||||
intercepting and replacing the headers we're passing. Ultimately this is a
|
||||
feature, and it helps quite a bit.
|
||||
|
||||
The second aspect of how this works is we're mocking the conductor RPC
|
||||
``get_topic_for`` and ``get_random_topic_for`` methods. These calls raise
|
||||
Temporary Unavailable, since trying to execute the entire interaction into
|
||||
the conductor is moderately pointless because all policy enforement is
|
||||
located with-in the API layer.
|
||||
|
||||
At the same time wiring everything up to go from API to conductor code and
|
||||
back would have been a heavier lift. As such, the tests largely look for
|
||||
one of the following error codes.
|
||||
|
||||
* 200 - Got the item from the API - This is an database driven interaction.
|
||||
* 201 - Created - This is databaes driven interaction. These are rare.
|
||||
* 204 - Accepted - This is a database driven interaction. These are rare.
|
||||
* 403 - Forbidden - This tells us the policy worked as expected where
|
||||
access was denied.
|
||||
* 404 - NotFound - This is typically when objects were not found. Before
|
||||
ironic becomes scope aware, these are generally only in the drivers
|
||||
API endpoint's behavior. In System scope aware Project scoped
|
||||
configuration, i.e. later RBAC tests, this will become the dominant
|
||||
response for project scoped users as responding with a 403 if they
|
||||
could be an owner or lessee would provide insight into the existence
|
||||
of a node.
|
||||
* 503 - Service Unavailable - In the context of our tests, we expect this
|
||||
when a request *has* been successfully authenticated and would have
|
||||
been sent along to the conductor.
|
||||
|
||||
How to make changes or review these tests?
|
||||
==========================================
|
||||
|
||||
The tests cycle through the various endpoints, and repeating patterns
|
||||
are clearly visible. Typically this means a given endpoint is cycled
|
||||
through with the same basic test using slightly different parameters
|
||||
such as different authentication parameters. When it comes to system
|
||||
scope aware tests supporting node ``owners`` and ``lessee``, these
|
||||
tests will cycle a little more with slightly different attributes
|
||||
as the operation is not general against a shared common node, but
|
||||
different nodes.
|
||||
|
||||
Some tests will test body contents, or attributes. some will validate
|
||||
the number of records returned. This is important later with ``owner``
|
||||
and ``lessee`` having slightly different views of the universe.
|
||||
|
||||
Some general rules apply
|
||||
|
||||
* Admins can do things
|
||||
* Members can do some things, but not everything
|
||||
* Readers can always read, but as we get into sensitive data later on
|
||||
such as fields containing infrastucture internal addresses, these values
|
||||
will become hidden and additional tests will examine this.
|
||||
* Third party, or external/other Admins will find nothing but sadness
|
||||
in empty lists, 403, 404, or even 500 errors.
|
||||
|
||||
What is/will be tested?
|
||||
=======================
|
||||
|
||||
The idea is to in essence test as much as possible, however as these
|
||||
tests Role Based Access Control related capabilities will come in a
|
||||
series of phases, styles vary a little.
|
||||
|
||||
The first phase is ``"legacy"``. In essence these are partially
|
||||
programatically generated and then human reviewed and values populated
|
||||
with expected values.
|
||||
|
||||
The second phase is remarkably similar to ``legacy``. It is the safety net
|
||||
where we execute the ``legacy`` tests with the updated ``oslo.policy``
|
||||
configuration to help enforce scopes. These tests will intentionally begin to
|
||||
fail in phase three.
|
||||
|
||||
The third phase is the implementation of System scope awareness for the
|
||||
API. In this process, as various portions of the API are made system scope
|
||||
aware. The ``legacy`` tests are marked as ``deprecated`` which signals to
|
||||
the second phase test sequences that they are **expected** to fail. New
|
||||
``system scoped`` tests are also implemented which are matched up by name
|
||||
to the ``legacy`` tests. The major difference being some header values,
|
||||
and a user with a ``member`` role in the ``system`` scope now has some
|
||||
rights.
|
||||
|
||||
The forth phase, is implementaiton of ``owner`` and ``lessee`` aware
|
||||
project scoping. The testing approach is similar, however it is much more of
|
||||
a "shotgun" approach. We test what we know should work, and what know should
|
||||
not work, but we do not have redundant testing for each role as ``admin``
|
||||
users are also ``members``, and since the policy rules are designed around
|
||||
thresholds of access, it just made no sense to run the same test for admin
|
||||
and members, where member was the threshold. These thresholds will vary with
|
||||
the proposed default policy. The forth scope also tests a third party external
|
||||
admin as a negative test to ensure that we are also denying access to
|
||||
resources appropriately.
|
||||
@ -112,7 +112,7 @@ class BaseApiTest(db_base.DbTestCase):
|
||||
return response
|
||||
|
||||
def put_json(self, path, params, expect_errors=False, headers=None,
|
||||
extra_environ=None, status=None):
|
||||
extra_environ=None, status=None, path_prefix=PATH_PREFIX):
|
||||
"""Sends simulated HTTP PUT request to Pecan test app.
|
||||
|
||||
:param path: url path of target service
|
||||
@ -127,10 +127,11 @@ class BaseApiTest(db_base.DbTestCase):
|
||||
return self._request_json(path=path, params=params,
|
||||
expect_errors=expect_errors,
|
||||
headers=headers, extra_environ=extra_environ,
|
||||
status=status, method="put")
|
||||
status=status, method="put",
|
||||
path_prefix=path_prefix)
|
||||
|
||||
def post_json(self, path, params, expect_errors=False, headers=None,
|
||||
extra_environ=None, status=None):
|
||||
extra_environ=None, status=None, path_prefix=PATH_PREFIX):
|
||||
"""Sends simulated HTTP POST request to Pecan test app.
|
||||
|
||||
:param path: url path of target service
|
||||
@ -145,10 +146,11 @@ class BaseApiTest(db_base.DbTestCase):
|
||||
return self._request_json(path=path, params=params,
|
||||
expect_errors=expect_errors,
|
||||
headers=headers, extra_environ=extra_environ,
|
||||
status=status, method="post")
|
||||
status=status, method="post",
|
||||
path_prefix=path_prefix)
|
||||
|
||||
def patch_json(self, path, params, expect_errors=False, headers=None,
|
||||
extra_environ=None, status=None):
|
||||
extra_environ=None, status=None, path_prefix=PATH_PREFIX):
|
||||
"""Sends simulated HTTP PATCH request to Pecan test app.
|
||||
|
||||
:param path: url path of target service
|
||||
@ -163,7 +165,8 @@ class BaseApiTest(db_base.DbTestCase):
|
||||
return self._request_json(path=path, params=params,
|
||||
expect_errors=expect_errors,
|
||||
headers=headers, extra_environ=extra_environ,
|
||||
status=status, method="patch")
|
||||
status=status, method="patch",
|
||||
path_prefix=path_prefix)
|
||||
|
||||
def delete(self, path, expect_errors=False, headers=None,
|
||||
extra_environ=None, status=None, path_prefix=PATH_PREFIX):
|
||||
|
||||
@ -23,6 +23,9 @@ import ddt
|
||||
from keystonemiddleware import auth_token
|
||||
from oslo_config import cfg
|
||||
|
||||
from ironic.api.controllers.v1 import versions as api_versions
|
||||
from ironic.common import exception
|
||||
from ironic.conductor import rpcapi
|
||||
from ironic.tests.unit.api import base
|
||||
from ironic.tests.unit.db import utils as db_utils
|
||||
|
||||
@ -42,6 +45,17 @@ class TestACLBase(base.BaseApiTest):
|
||||
self.mock_auth = mock_auth.start()
|
||||
self.addCleanup(mock_auth.stop)
|
||||
|
||||
topic = mock.patch.object(
|
||||
rpcapi.ConductorAPI, 'get_topic_for', autospec=True)
|
||||
self.mock_topic = topic.start()
|
||||
self.mock_topic.side_effect = exception.TemporaryFailure
|
||||
self.addCleanup(topic.stop)
|
||||
rtopic = mock.patch.object(rpcapi.ConductorAPI, 'get_random_topic',
|
||||
autospec=True)
|
||||
self.mock_random_topic = rtopic.start()
|
||||
self.mock_random_topic.side_effect = exception.TemporaryFailure
|
||||
self.addCleanup(rtopic.stop)
|
||||
|
||||
def _make_app(self):
|
||||
cfg.CONF.set_override('auth_strategy', 'keystone')
|
||||
return super(TestACLBase, self)._make_app()
|
||||
@ -53,23 +67,70 @@ class TestACLBase(base.BaseApiTest):
|
||||
def _check_skip(self, **kwargs):
|
||||
if kwargs.get('skip_reason'):
|
||||
self.skipTest(kwargs.get('skip_reason'))
|
||||
# Remove ASAP, but as a few hundred tests use this, we can
|
||||
# rip it out later.
|
||||
if kwargs.get('skip'):
|
||||
self.skipTest(kwargs.get('skip_reason', 'Not implemented'))
|
||||
|
||||
def _fake_process_request(self, request, auth_token_request):
|
||||
pass
|
||||
|
||||
def _test_request(self, path, params=None, headers=None, method='get',
|
||||
assert_status=None, assert_dict_contains=None):
|
||||
body=None, assert_status=None,
|
||||
assert_dict_contains=None,
|
||||
assert_list_length=None):
|
||||
path = path.format(**self.format_data)
|
||||
self.mock_auth.side_effect = self._fake_process_request
|
||||
|
||||
# always request the latest api version
|
||||
version = api_versions.max_version_string()
|
||||
rheaders = {
|
||||
'X-OpenStack-Ironic-API-Version': version
|
||||
}
|
||||
# NOTE(TheJulia): Logging the test request to aid
|
||||
# in troubleshooting ACL testing. This is a pattern
|
||||
# followed in API unit testing in ironic, and
|
||||
# really does help.
|
||||
print('API ACL Testing Path %s %s' % (method, path))
|
||||
if headers:
|
||||
for k, v in headers.items():
|
||||
rheaders[k] = v.format(**self.format_data)
|
||||
|
||||
if method == 'get':
|
||||
response = self.get_json(
|
||||
path,
|
||||
headers=headers,
|
||||
headers=rheaders,
|
||||
expect_errors=True,
|
||||
extra_environ=self.environ,
|
||||
path_prefix=''
|
||||
)
|
||||
elif method == 'put':
|
||||
response = self.put_json(
|
||||
path,
|
||||
headers=rheaders,
|
||||
expect_errors=True,
|
||||
extra_environ=self.environ,
|
||||
path_prefix='',
|
||||
params=body
|
||||
)
|
||||
elif method == 'post':
|
||||
response = self.post_json(
|
||||
path,
|
||||
headers=rheaders,
|
||||
expect_errors=True,
|
||||
extra_environ=self.environ,
|
||||
path_prefix='',
|
||||
params=body
|
||||
)
|
||||
elif method == 'patch':
|
||||
response = self.patch_json(
|
||||
path,
|
||||
params=body,
|
||||
headers=rheaders,
|
||||
expect_errors=True,
|
||||
extra_environ=self.environ,
|
||||
path_prefix=''
|
||||
)
|
||||
elif method == 'delete':
|
||||
response = self.delete(
|
||||
path,
|
||||
headers=rheaders,
|
||||
expect_errors=True,
|
||||
extra_environ=self.environ,
|
||||
path_prefix=''
|
||||
@ -77,12 +138,10 @@ class TestACLBase(base.BaseApiTest):
|
||||
else:
|
||||
assert False, 'Unimplemented test method: %s' % method
|
||||
|
||||
other_asserts = bool(assert_dict_contains)
|
||||
|
||||
if assert_status:
|
||||
self.assertEqual(assert_status, response.status_int)
|
||||
else:
|
||||
self.assertIsNotNone(other_asserts,
|
||||
self.assertIsNotNone(assert_status,
|
||||
'Tests must include an assert_status')
|
||||
|
||||
if assert_dict_contains:
|
||||
@ -91,6 +150,22 @@ class TestACLBase(base.BaseApiTest):
|
||||
self.assertEqual(v.format(**self.format_data),
|
||||
response.json[k])
|
||||
|
||||
if assert_list_length:
|
||||
for root, length in assert_list_length.items():
|
||||
# root - object to look inside
|
||||
# length - number of expected elements which will be
|
||||
# important for owner/lessee testing.
|
||||
items = response.json[root]
|
||||
self.assertIsInstance(items, list)
|
||||
self.assertEqual(length, len(items))
|
||||
|
||||
# NOTE(TheJulia): API tests in Ironic tend to have a pattern
|
||||
# to print request and response data to aid in development
|
||||
# and troubleshooting. As such the prints should remain,
|
||||
# at least until we are through primary development of the
|
||||
# this test suite.
|
||||
print('ACL Test GOT %s' % response)
|
||||
|
||||
|
||||
@ddt.ddt
|
||||
class TestRBACBasic(TestACLBase):
|
||||
@ -110,8 +185,61 @@ class TestRBACBasic(TestACLBase):
|
||||
class TestRBACModelBeforeScopes(TestACLBase):
|
||||
|
||||
def _create_test_data(self):
|
||||
fake_db_node = db_utils.create_test_node(chassis_id=None)
|
||||
self.format_data['node_ident'] = fake_db_node['uuid']
|
||||
allocated_node_id = 31
|
||||
fake_db_allocation = db_utils.create_test_allocation(
|
||||
node_id=allocated_node_id,
|
||||
resource_class="CUSTOM_TEST")
|
||||
fake_db_node = db_utils.create_test_node(
|
||||
chassis_id=None,
|
||||
driver='fake-driverz')
|
||||
fake_db_node_alloced = db_utils.create_test_node(
|
||||
id=allocated_node_id,
|
||||
chassis_id=None,
|
||||
allocation_id=fake_db_allocation['id'],
|
||||
uuid='22e26c0b-03f2-4d2e-ae87-c02d7f33c000',
|
||||
driver='fake-driverz')
|
||||
fake_vif_port_id = "ee21d58f-5de2-4956-85ff-33935ea1ca00"
|
||||
fake_db_port = db_utils.create_test_port(
|
||||
node_id=fake_db_node['id'],
|
||||
internal_info={'tenant_vif_port_id': fake_vif_port_id})
|
||||
fake_db_portgroup = db_utils.create_test_portgroup(
|
||||
node_id=fake_db_node['id'])
|
||||
fake_db_chassis = db_utils.create_test_chassis()
|
||||
fake_db_deploy_template = db_utils.create_test_deploy_template()
|
||||
fake_db_conductor = db_utils.create_test_conductor()
|
||||
fake_db_volume_target = db_utils.create_test_volume_target(
|
||||
node_id=fake_db_allocation['id'])
|
||||
fake_db_volume_connector = db_utils.create_test_volume_connector(
|
||||
node_id=fake_db_allocation['id'])
|
||||
# Trait name aligns with create_test_node_trait.
|
||||
fake_trait = 'trait'
|
||||
fake_setting = 'FAKE_SETTING'
|
||||
db_utils.create_test_bios_setting(
|
||||
node_id=fake_db_node['id'],
|
||||
name=fake_setting,
|
||||
value=fake_setting)
|
||||
db_utils.create_test_node_trait(
|
||||
node_id=fake_db_node['id'])
|
||||
|
||||
self.format_data.update({
|
||||
'node_ident': fake_db_node['uuid'],
|
||||
'allocated_node_ident': fake_db_node_alloced['uuid'],
|
||||
'port_ident': fake_db_port['uuid'],
|
||||
'portgroup_ident': fake_db_portgroup['uuid'],
|
||||
'chassis_ident': fake_db_chassis['uuid'],
|
||||
'deploy_template_ident': fake_db_deploy_template['uuid'],
|
||||
'allocation_ident': fake_db_allocation['uuid'],
|
||||
'conductor_ident': fake_db_conductor['hostname'],
|
||||
'vif_ident': fake_vif_port_id,
|
||||
# Can't use the same fake-driver as other tests can
|
||||
# pollute a global method cache in the API that is in the
|
||||
# test runner, resulting in false positives.
|
||||
'driver_name': 'fake-driverz',
|
||||
'bios_setting': fake_setting,
|
||||
'trait': fake_trait,
|
||||
'volume_target_ident': fake_db_volume_target['uuid'],
|
||||
'volume_connector_ident': fake_db_volume_connector['uuid'],
|
||||
})
|
||||
|
||||
@ddt.file_data('test_rbac_legacy.yaml')
|
||||
@ddt.unpack
|
||||
|
||||
@ -19,6 +19,7 @@ project_admin_can_get_node:
|
||||
assert_dict_contains:
|
||||
uuid: '{node_uuid}'
|
||||
driver: 'fake-hardware'
|
||||
assert_status: 200
|
||||
|
||||
project_member_cannot_get_node:
|
||||
path: *node_path
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
Loading…
x
Reference in New Issue
Block a user