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

5 files changed, 50 insertions, 0 deletions

diff --git a/keystoneclient/base.py b/keystoneclient/base.py
index 025362b..eabbdc4 100644
--- a/keystoneclient/base.py
+++ b/keystoneclient/base.py
@@ -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:
diff --git a/keystoneclient/tests/unit/v3/test_domains.py b/keystoneclient/tests/unit/v3/test_domains.py
index 9cc23e7..4dbfd73 100644
--- a/keystoneclient/tests/unit/v3/test_domains.py
+++ b/keystoneclient/tests/unit/v3/test_domains.py
@@ -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')
 
diff --git a/keystoneclient/tests/unit/v3/test_federation.py b/keystoneclient/tests/unit/v3/test_federation.py
index 4cbae8d..5782aa6 100644
--- a/keystoneclient/tests/unit/v3/test_federation.py
+++ b/keystoneclient/tests/unit/v3/test_federation.py
@@ -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}
diff --git a/keystoneclient/tests/unit/v3/test_role_assignments.py b/keystoneclient/tests/unit/v3/test_role_assignments.py
index 79d2585..e77bdcc 100644
--- a/keystoneclient/tests/unit/v3/test_role_assignments.py
+++ b/keystoneclient/tests/unit/v3/test_role_assignments.py
@@ -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',
diff --git a/keystoneclient/tests/unit/v3/utils.py b/keystoneclient/tests/unit/v3/utils.py
index 7f2d633..b5fb7ce 100644
--- a/keystoneclient/tests/unit/v3/utils.py
+++ b/keystoneclient/tests/unit/v3/utils.py
@@ -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()]