Prevent attempts to "filter" list() calls by globally unique IDs
This use case isn't covered by our current APIs: GET /entities?id={entity_id} Because we have a dedicated API for that: GET /entities/{entity_id} But our list() methods generally support **kwargs, which are passed as query parameters to keystone. When an 'id' is passed to keystone as a query parameter, keystone rightly ignores it and returns an unfiltered collection. This change raises a client-side TypeError (as you'd expect when you try to pass a keyword argument that a function isn't expecting), and includes a helpful suggestion to try calling get() instead. Change-Id: I100b69bbf571ad6de49ccc5ad1099c20b877d13d Closes-Bug: 1452298
This commit is contained in:
parent
ae066a828f
commit
98326c72f7
|
@ -356,6 +356,17 @@ class CrudManager(Manager):
|
|||
|
||||
@filter_kwargs
|
||||
def list(self, fallback_to_auth=False, **kwargs):
|
||||
if 'id' in kwargs.keys():
|
||||
# Ensure that users are not trying to call things like
|
||||
# ``domains.list(id='default')`` when they should have used
|
||||
# ``[domains.get(domain_id='default')]`` instead. Keystone supports
|
||||
# ``GET /v3/domains/{domain_id}``, not ``GET
|
||||
# /v3/domains?id={domain_id}``.
|
||||
raise TypeError(
|
||||
_("list() got an unexpected keyword argument 'id'. To "
|
||||
"retrieve a single object using a globally unique "
|
||||
"identifier, try using get() instead."))
|
||||
|
||||
url = self.build_url(dict_args_in_out=kwargs)
|
||||
|
||||
try:
|
||||
|
|
|
@ -30,6 +30,12 @@ class DomainTests(utils.TestCase, utils.CrudTests):
|
|||
kwargs.setdefault('name', uuid.uuid4().hex)
|
||||
return kwargs
|
||||
|
||||
def test_filter_for_default_domain_by_id(self):
|
||||
ref = self.new_ref(id='default')
|
||||
super(DomainTests, self).test_list_by_id(
|
||||
ref=ref,
|
||||
id=ref['id'])
|
||||
|
||||
def test_list_filter_name(self):
|
||||
super(DomainTests, self).test_list(name='adomain123')
|
||||
|
||||
|
|
|
@ -278,6 +278,16 @@ class ProtocolTests(utils.TestCase, utils.CrudTests):
|
|||
for obj, ref_obj in zip(returned, expected):
|
||||
self.assertEqual(obj.to_dict(), ref_obj)
|
||||
|
||||
def test_list_by_id(self):
|
||||
# The test in the parent class needs to be overridden because it
|
||||
# assumes globally unique IDs, which is not the case with protocol IDs
|
||||
# (which are contextualized per identity provider).
|
||||
ref = self.new_ref()
|
||||
super(ProtocolTests, self).test_list_by_id(
|
||||
ref=ref,
|
||||
identity_provider=ref['identity_provider'],
|
||||
id=ref['id'])
|
||||
|
||||
def test_list_params(self):
|
||||
request_args = self.new_ref()
|
||||
filter_kwargs = {uuid.uuid4().hex: uuid.uuid4().hex}
|
||||
|
|
|
@ -71,6 +71,15 @@ class RoleAssignmentsTests(utils.TestCase, utils.CrudTests):
|
|||
self.assertEqual(len(ref_list), len(returned_list))
|
||||
[self.assertIsInstance(r, self.model) for r in returned_list]
|
||||
|
||||
def test_list_by_id(self):
|
||||
# It doesn't make sense to "list role assignments by ID" at all, given
|
||||
# that they don't have globally unique IDs in the first place. But
|
||||
# calling RoleAssignmentsManager.list(id=...) should still raise a
|
||||
# TypeError when given an unexpected keyword argument 'id', so we don't
|
||||
# actually have to modify the test in the superclass... I just wanted
|
||||
# to make a note here in case the superclass changes.
|
||||
super(RoleAssignmentsTests, self).test_list_by_id()
|
||||
|
||||
def test_list_params(self):
|
||||
ref_list = self.TEST_USER_PROJECT_LIST
|
||||
self.stub_entity('GET',
|
||||
|
|
|
@ -245,6 +245,20 @@ class CrudTests(object):
|
|||
|
||||
return expected_path
|
||||
|
||||
def test_list_by_id(self, ref=None, **filter_kwargs):
|
||||
"""Test ``entities.list(id=x)`` being rewritten as ``GET /v3/entities/x``.
|
||||
|
||||
This tests an edge case of each manager's list() implementation, to
|
||||
ensure that it "does the right thing" when users call ``.list()``
|
||||
when they should have used ``.get()``.
|
||||
|
||||
"""
|
||||
if 'id' not in filter_kwargs:
|
||||
ref = ref or self.new_ref()
|
||||
filter_kwargs['id'] = ref['id']
|
||||
|
||||
self.assertRaises(TypeError, self.manager.list, **filter_kwargs)
|
||||
|
||||
def test_list(self, ref_list=None, expected_path=None,
|
||||
expected_query=None, **filter_kwargs):
|
||||
ref_list = ref_list or [self.new_ref(), self.new_ref()]
|
||||
|
|
Loading…
Reference in New Issue