token: consistently decode binary types

Ensure that any binary types unpacked from message payloads
are correctly converted from binary to text type.

Under Python 3 msgpack returns the serialized input as a
byte string. Similar to other msgpack'd values in the payload,
we need to explicitly decode it to a string value.

This is specifically more of an issue under Python 3; however
the decode operation is safe back to Python 2 so there is no
need to limit the decode codepath to just Python 3.

Conflicts:
    keystone/token/token_formatters.py

Note: the file conflict is caused by patch
I9529d6bee3e5bb1f618f40f225f69e2ad7e3f64a which is only present in
stable/train.

Change-Id: Ib1073acf5677a60714d0a386de3bcd14ce6cd134
Closes-Bug: 1832265
(cherry picked from commit ffa0918f5a92fd18c86703916d768012b0bea61b)

3 files changed, 111 insertions, 68 deletions

diff --git a/keystone/tests/unit/token/test_fernet_provider.py b/keystone/tests/unit/token/test_fernet_provider.py
index b00de49c3..5f4927cc6 100644
--- a/keystone/tests/unit/token/test_fernet_provider.py
+++ b/keystone/tests/unit/token/test_fernet_provider.py
@@ -287,21 +287,73 @@ class TestPayloads(unit.TestCase):
             actual_time_float)
         self.assertEqual(expected_time_str, actual_time_str)
 
+    def test_convert_or_decode_uuid_bytes(self):
+        payload_cls = token_formatters.BasePayload
+
+        expected_hex_uuid = uuid.uuid4().hex
+        uuid_obj = uuid.UUID(expected_hex_uuid)
+        expected_uuid_in_bytes = uuid_obj.bytes
+
+        actual_hex_uuid = payload_cls._convert_or_decode(
+            is_stored_as_bytes=True,
+            value=expected_uuid_in_bytes
+        )
+
+        self.assertEqual(expected_hex_uuid, actual_hex_uuid)
+
+    def test_convert_or_decode_binary_type(self):
+        payload_cls = token_formatters.BasePayload
+
+        expected_hex_uuid = uuid.uuid4().hex
+
+        actual_hex_uuid = payload_cls._convert_or_decode(
+            is_stored_as_bytes=False,
+            value=expected_hex_uuid.encode('utf-8')
+        )
+
+        self.assertEqual(expected_hex_uuid, actual_hex_uuid)
+
+    def test_convert_or_decode_text_type(self):
+        payload_cls = token_formatters.BasePayload
+
+        expected_hex_uuid = uuid.uuid4().hex
+
+        actual_hex_uuid = payload_cls._convert_or_decode(
+            is_stored_as_bytes=False,
+            value=expected_hex_uuid
+        )
+
+        self.assertEqual(expected_hex_uuid, actual_hex_uuid)
+
     def _test_payload(self, payload_class, exp_user_id=None, exp_methods=None,
                       exp_system=None, exp_project_id=None, exp_domain_id=None,
                       exp_trust_id=None, exp_federated_group_ids=None,
                       exp_identity_provider_id=None, exp_protocol_id=None,
-                      exp_access_token_id=None, exp_app_cred_id=None):
+                      exp_access_token_id=None, exp_app_cred_id=None,
+                      encode_ids=False):
+        def _encode_id(value):
+            if value is not None and six.text_type(value) and encode_ids:
+                return value.encode('utf-8')
+            return value
         exp_user_id = exp_user_id or uuid.uuid4().hex
         exp_methods = exp_methods or ['password']
         exp_expires_at = utils.isotime(timeutils.utcnow(), subsecond=True)
         exp_audit_ids = [provider.random_urlsafe_str()]
 
         payload = payload_class.assemble(
-            exp_user_id, exp_methods, exp_system, exp_project_id,
-            exp_domain_id, exp_expires_at, exp_audit_ids, exp_trust_id,
-            exp_federated_group_ids, exp_identity_provider_id, exp_protocol_id,
-            exp_access_token_id, exp_app_cred_id)
+            _encode_id(exp_user_id),
+            exp_methods,
+            _encode_id(exp_system),
+            _encode_id(exp_project_id),
+            exp_domain_id,
+            exp_expires_at,
+            exp_audit_ids,
+            exp_trust_id,
+            _encode_id(exp_federated_group_ids),
+            _encode_id(exp_identity_provider_id),
+            exp_protocol_id,
+            _encode_id(exp_access_token_id),
+            _encode_id(exp_app_cred_id))
 
         (user_id, methods, system, project_id,
          domain_id, expires_at, audit_ids,
@@ -364,6 +416,12 @@ class TestPayloads(unit.TestCase):
                            exp_user_id='0123456789abcdef',
                            exp_project_id='0123456789abcdef')
 
+    def test_project_scoped_payload_with_binary_encoded_ids(self):
+        self._test_payload(token_formatters.ProjectScopedPayload,
+                           exp_user_id='someNonUuidUserId',
+                           exp_project_id='someNonUuidProjectId',
+                           encode_ids=True)
+
     def test_domain_scoped_payload_with_non_uuid_user_id(self):
         self._test_payload(token_formatters.DomainScopedPayload,
                            exp_user_id='nonUuidUserId',
diff --git a/keystone/token/token_formatters.py b/keystone/token/token_formatters.py
index ed080193b..182755cc6 100644
--- a/keystone/token/token_formatters.py
+++ b/keystone/token/token_formatters.py
@@ -313,9 +313,11 @@ class BasePayload(object):
         """
         try:
             return (True, cls.convert_uuid_hex_to_bytes(value))
-        except ValueError:
-            # this might not be a UUID, depending on the situation (i.e.
-            # federation)
+        except (ValueError, TypeError):
+            # ValueError: this might not be a UUID, depending on the
+            # situation (i.e. federation)
+            # TypeError: the provided value may be binary encoded
+            # in which case just return the value (i.e. Python 3)
             return (False, value)
 
     @classmethod
@@ -344,6 +346,22 @@ class BasePayload(object):
         # restore the padding (==) at the end of the string
         return base64.urlsafe_b64decode(s + '==')
 
+    @classmethod
+    def _convert_or_decode(cls, is_stored_as_bytes, value):
+        """Convert a value to text type, translating uuid -> hex if required.
+
+        :param is_stored_as_bytes: whether value is already bytes
+        :type is_stored_as_bytes: six.boolean
+        :param value: value to attempt to convert to bytes
+        :type value: six.text_type or six.binary_type
+        :rtype: six.text_type
+        """
+        if is_stored_as_bytes:
+            return cls.convert_uuid_bytes_to_hex(value)
+        elif isinstance(value, six.binary_type):
+            return value.decode('utf-8')
+        return value
+
 
 class UnscopedPayload(BasePayload):
     version = 0
@@ -363,8 +381,7 @@ class UnscopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         expires_at_str = cls._convert_float_to_time_string(payload[2])
         audit_ids = list(map(cls.base64_encode, payload[3]))
@@ -409,8 +426,7 @@ class DomainScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         try:
             domain_id = cls.convert_uuid_bytes_to_hex(payload[2])
@@ -457,12 +473,10 @@ class ProjectScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         (is_stored_as_bytes, project_id) = payload[2]
-        if is_stored_as_bytes:
-            project_id = cls.convert_uuid_bytes_to_hex(project_id)
+        project_id = cls._convert_or_decode(is_stored_as_bytes, project_id)
         expires_at_str = cls._convert_float_to_time_string(payload[3])
         audit_ids = list(map(cls.base64_encode, payload[4]))
         system = None
@@ -501,12 +515,10 @@ class TrustScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         (is_stored_as_bytes, project_id) = payload[2]
-        if is_stored_as_bytes:
-            project_id = cls.convert_uuid_bytes_to_hex(project_id)
+        project_id = cls._convert_or_decode(is_stored_as_bytes, project_id)
         expires_at_str = cls._convert_float_to_time_string(payload[3])
         audit_ids = list(map(cls.base64_encode, payload[4]))
         trust_id = cls.convert_uuid_bytes_to_hex(payload[5])
@@ -533,8 +545,7 @@ class FederatedUnscopedPayload(BasePayload):
     @classmethod
     def unpack_group_id(cls, group_id_in_bytes):
         (is_stored_as_bytes, group_id) = group_id_in_bytes
-        if is_stored_as_bytes:
-            group_id = cls.convert_uuid_bytes_to_hex(group_id)
+        group_id = cls._convert_or_decode(is_stored_as_bytes, group_id)
         return {'id': group_id}
 
     @classmethod
@@ -556,15 +567,11 @@ class FederatedUnscopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         group_ids = list(map(cls.unpack_group_id, payload[2]))
         (is_stored_as_bytes, idp_id) = payload[3]
-        if is_stored_as_bytes:
-            idp_id = cls.convert_uuid_bytes_to_hex(idp_id)
-        else:
-            idp_id = idp_id.decode('utf-8')
+        idp_id = cls._convert_or_decode(is_stored_as_bytes, idp_id)
         protocol_id = payload[4]
         if isinstance(protocol_id, six.binary_type):
             protocol_id = protocol_id.decode('utf-8')
@@ -605,29 +612,10 @@ class FederatedScopedPayload(FederatedUnscopedPayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         (is_stored_as_bytes, scope_id) = payload[2]
-        if is_stored_as_bytes:
-            scope_id = cls.convert_uuid_bytes_to_hex(scope_id)
-        else:
-            # NOTE(lbragstad): We assembled the token payload scope as a tuple
-            # (False, domain_id) for cases like (False, 'default'), since the
-            # default domain ID isn't converted to a byte string when it's not
-            # in UUID format. Despite the boolean indicator in the tuple that
-            # denotes if the value is stored as a byte string or not, msgpack
-            # apparently returns the serialized input as byte strings anyway.
-            # For example, this means what we though we were passing in as
-            # (False, 'default') during token creation actually comes out as
-            # (False, b'default') in token validation through msgpack, which
-            # clearly isn't correct according to our boolean indicator. This
-            # causes comparison issues due to different string types (e.g.,
-            # b'default' != 'default') with python 3. See bug 1813085 for
-            # details. We use this pattern for other strings in the payload
-            # like idp_id and protocol_id for the same reason.
-            if six.PY3 and isinstance(scope_id, six.binary_type):
-                scope_id = scope_id.decode('utf-8')
+        scope_id = cls._convert_or_decode(is_stored_as_bytes, scope_id)
         project_id = (
             scope_id
             if cls.version == FederatedProjectScopedPayload.version else None)
@@ -636,11 +624,7 @@ class FederatedScopedPayload(FederatedUnscopedPayload):
             if cls.version == FederatedDomainScopedPayload.version else None)
         group_ids = list(map(cls.unpack_group_id, payload[3]))
         (is_stored_as_bytes, idp_id) = payload[4]
-        if is_stored_as_bytes:
-            idp_id = cls.convert_uuid_bytes_to_hex(idp_id)
-        else:
-            if six.PY3 and isinstance(idp_id, six.binary_type):
-                idp_id = idp_id.decode('utf-8')
+        idp_id = cls._convert_or_decode(is_stored_as_bytes, idp_id)
         protocol_id = payload[5]
         if six.PY3 and isinstance(protocol_id, six.binary_type):
             protocol_id = protocol_id.decode('utf-8')
@@ -685,15 +669,13 @@ class OauthScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         (is_stored_as_bytes, project_id) = payload[2]
-        if is_stored_as_bytes:
-            project_id = cls.convert_uuid_bytes_to_hex(project_id)
+        project_id = cls._convert_or_decode(is_stored_as_bytes, project_id)
         (is_stored_as_bytes, access_token_id) = payload[3]
-        if is_stored_as_bytes:
-            access_token_id = cls.convert_uuid_bytes_to_hex(access_token_id)
+        access_token_id = cls._convert_or_decode(is_stored_as_bytes,
+                                                 access_token_id)
         expires_at_str = cls._convert_float_to_time_string(payload[4])
         audit_ids = list(map(cls.base64_encode, payload[5]))
         system = None
@@ -728,8 +710,7 @@ class SystemScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         system = payload[2]
         expires_at_str = cls._convert_float_to_time_string(payload[3])
@@ -769,12 +750,10 @@ class ApplicationCredentialScopedPayload(BasePayload):
     @classmethod
     def disassemble(cls, payload):
         (is_stored_as_bytes, user_id) = payload[0]
-        if is_stored_as_bytes:
-            user_id = cls.convert_uuid_bytes_to_hex(user_id)
+        user_id = cls._convert_or_decode(is_stored_as_bytes, user_id)
         methods = auth_plugins.convert_integer_to_method_list(payload[1])
         (is_stored_as_bytes, project_id) = payload[2]
-        if is_stored_as_bytes:
-            project_id = cls.convert_uuid_bytes_to_hex(project_id)
+        project_id = cls._convert_or_decode(is_stored_as_bytes, project_id)
         expires_at_str = cls._convert_float_to_time_string(payload[3])
         audit_ids = list(map(cls.base64_encode, payload[4]))
         system = None
@@ -785,8 +764,7 @@ class ApplicationCredentialScopedPayload(BasePayload):
         protocol_id = None
         access_token_id = None
         (is_stored_as_bytes, app_cred_id) = payload[5]
-        if is_stored_as_bytes:
-            app_cred_id = cls.convert_uuid_bytes_to_hex(app_cred_id)
+        app_cred_id = cls._convert_or_decode(is_stored_as_bytes, app_cred_id)
         return (user_id, methods, system, project_id, domain_id,
                 expires_at_str, audit_ids, trust_id, federated_group_ids,
                 identity_provider_id, protocol_id, access_token_id,
diff --git a/releasenotes/notes/bug-1832265-cb76ccf505c2d9d1.yaml b/releasenotes/notes/bug-1832265-cb76ccf505c2d9d1.yaml
new file mode 100644
index 000000000..9dcf16aa7
--- /dev/null
+++ b/releasenotes/notes/bug-1832265-cb76ccf505c2d9d1.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1832265 <https://bugs.launchpad.net/keystone/+bug/1832265>`_]
+    Binary msgpack payload types are now consistently and correctly decoded
+    when running Keystone under Python 3, avoiding any TypeErrors when
+    attempting to convert binary encoded strings into UUID's.