From 2eea4553e23ff3c0d4d367316ea634253e11c10a Mon Sep 17 00:00:00 2001
From: Dolph Mathews <dolph.mathews@gmail.com>
Date: Wed, 7 Nov 2012 15:17:52 -0600
Subject: Include 'extra' attributes twice (bug 1076120)

In order to maintain backwards-compatibility with the output of the
previously-broken SQL driver, non-indexed attributes are included in the
update user/tenant response in both the correct and expected locations.

Change-Id: I54f69c0c4cb3ade349190bc0c61539dcc1846267
---
 keystone/common/sql/core.py       | 13 ++++++++-
 keystone/identity/backends/sql.py |  4 +--
 keystone/identity/core.py         |  5 ++++
 tests/test_backend_sql.py         | 57 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/keystone/common/sql/core.py b/keystone/common/sql/core.py
index 1a9d595f..d8e11c3a 100644
--- a/keystone/common/sql/core.py
+++ b/keystone/common/sql/core.py
@@ -68,10 +68,21 @@ class DictBase(object):
 
         return cls(**new_d)
 
-    def to_dict(self):
+    def to_dict(self, include_extra_dict=False):
+        """Returns the model's attributes as a dictionary.
+
+        If include_extra_dict is True, 'extra' attributes are literally
+        included in the resulting dictionary twice, for backwards-compatibility
+        with a broken implementation.
+
+        """
         d = self.extra.copy()
         for attr in self.__class__.attributes:
             d[attr] = getattr(self, attr)
+
+        if include_extra_dict:
+            d['extra'] = self.extra.copy()
+
         return d
 
     def __setitem__(self, key, value):
diff --git a/keystone/identity/backends/sql.py b/keystone/identity/backends/sql.py
index 7c3eb975..753a30e2 100644
--- a/keystone/identity/backends/sql.py
+++ b/keystone/identity/backends/sql.py
@@ -337,7 +337,7 @@ class Identity(sql.Base, identity.Driver):
             user_ref.name = new_user.name
             user_ref.extra = new_user.extra
             session.flush()
-        return identity.filter_user(user_ref.to_dict())
+        return identity.filter_user(user_ref.to_dict(include_extra_dict=True))
 
     def delete_user(self, user_id):
         session = self.get_session()
@@ -378,7 +378,7 @@ class Identity(sql.Base, identity.Driver):
             tenant_ref.name = new_tenant.name
             tenant_ref.extra = new_tenant.extra
             session.flush()
-        return tenant_ref.to_dict()
+        return tenant_ref.to_dict(include_extra_dict=True)
 
     def delete_tenant(self, tenant_id):
         session = self.get_session()
diff --git a/keystone/identity/core.py b/keystone/identity/core.py
index 89663047..5bd2f446 100644
--- a/keystone/identity/core.py
+++ b/keystone/identity/core.py
@@ -45,6 +45,11 @@ def filter_user(user_ref):
         user_ref = user_ref.copy()
         user_ref.pop('password', None)
         user_ref.pop('tenants', None)
+        try:
+            user_ref['extra'].pop('password', None)
+            user_ref['extra'].pop('tenants', None)
+        except KeyError:
+            pass
     return user_ref
 
 
diff --git a/tests/test_backend_sql.py b/tests/test_backend_sql.py
index bc318d07..c484f9ea 100644
--- a/tests/test_backend_sql.py
+++ b/tests/test_backend_sql.py
@@ -155,6 +155,63 @@ class SqlIdentity(SqlTests, test_backend.IdentityTests):
                           user['id'],
                           self.tenant_bar['id'])
 
+    def test_update_tenant_returns_extra(self):
+        """This tests for backwards-compatibility with an essex/folsom bug.
+
+        Non-indexed attributes were returned in an 'extra' attribute, instead
+        of on the entity itself; for consistency and backwards compatibility,
+        those attributes should be included twice.
+
+        This behavior is specific to the SQL driver.
+
+        """
+        tenant_id = uuid.uuid4().hex
+        arbitrary_key = uuid.uuid4().hex
+        arbitrary_value = uuid.uuid4().hex
+        tenant = {
+            'id': tenant_id,
+            'name': uuid.uuid4().hex,
+            arbitrary_key: arbitrary_value}
+        ref = self.identity_api.create_tenant(tenant_id, tenant)
+        self.assertEqual(arbitrary_value, ref[arbitrary_key])
+        self.assertIsNone(ref.get('extra'))
+
+        tenant['name'] = uuid.uuid4().hex
+        ref = self.identity_api.update_tenant(tenant_id, tenant)
+        self.assertEqual(arbitrary_value, ref[arbitrary_key])
+        self.assertEqual(arbitrary_value, ref['extra'][arbitrary_key])
+
+    def test_update_user_returns_extra(self):
+        """This tests for backwards-compatibility with an essex/folsom bug.
+
+        Non-indexed attributes were returned in an 'extra' attribute, instead
+        of on the entity itself; for consistency and backwards compatibility,
+        those attributes should be included twice.
+
+        This behavior is specific to the SQL driver.
+
+        """
+        user_id = uuid.uuid4().hex
+        arbitrary_key = uuid.uuid4().hex
+        arbitrary_value = uuid.uuid4().hex
+        user = {
+            'id': user_id,
+            'name': uuid.uuid4().hex,
+            'password': uuid.uuid4().hex,
+            arbitrary_key: arbitrary_value}
+        ref = self.identity_api.create_user(user_id, user)
+        self.assertEqual(arbitrary_value, ref[arbitrary_key])
+        self.assertIsNone(ref.get('password'))
+        self.assertIsNone(ref.get('extra'))
+
+        user['name'] = uuid.uuid4().hex
+        user['password'] = uuid.uuid4().hex
+        ref = self.identity_api.update_user(user_id, user)
+        self.assertIsNone(ref.get('password'))
+        self.assertIsNone(ref['extra'].get('password'))
+        self.assertEqual(arbitrary_value, ref[arbitrary_key])
+        self.assertEqual(arbitrary_value, ref['extra'][arbitrary_key])
+
 
 class SqlToken(SqlTests, test_backend.TokenTests):
     pass
-- 
cgit