Deprecate setting the payload type and encoding

Deprecate manually setting the payload_content_type and
payload_content_encoding properties of a secret.  With this CR a user of
the client only needs to provide the payload, and the client will figure
out what the correct payload_content_type and payload_content_encoding
values should be.

Setting these properties for the user lets us avoid a lot of weird
behaviors such as the one described in Bug #1419166, and also lets us
avoid errors that happen when a user mismatches the payload and an
incorrect content type.

In the interest of backwards compatibility, these properties are still
usable, but will log deprecation warnings.  They should be removed in a
future version after current users have had enough time to update their
code bases.

Change-Id: Ibfe3ad42e11bd83c002d0f1b69fb8a323a7b6f3d
Closes-Bug: #1419166

4 files changed, 200 insertions, 38 deletions

diff --git a/README.rst b/README.rst
index 5759a4a..dd99017 100644
--- a/README.rst
+++ b/README.rst
@@ -31,12 +31,12 @@ with keystone authentication:
     >>> from barbicanclient import client
 
     >>> # We'll use Keystone API v3 for authentication
-    >>> auth = identity.v3.Password(auth_url='http://localhost:5000/v3',
-    ...                             username='admin_user',
-    ...                             user_domain_name='Default',
-    ...                             password='password',
-    ...                             project_name='demo',
-    ...                             project_domain_name='Default')
+    >>> auth = identity.v3.Password(auth_url=u'http://localhost:5000/v3',
+    ...                             username=u'admin_user',
+    ...                             user_domain_name=u'Default',
+    ...                             password=u'password',
+    ...                             project_name=u'demo',
+    ...                             project_domain_name=u'Default')
 
     >>> # Next we'll create a Keystone session using the auth plugin we just created
     >>> sess = session.Session(auth=auth)
@@ -45,9 +45,8 @@ with keystone authentication:
     >>> barbican = client.Client(session=sess)
 
     >>> # Let's create a Secret to store some sensitive data
-    >>> secret = barbican.secrets.create(name='Self destruction sequence',
-    ...                                  payload='the magic words are squeamish ossifrage',
-    ...                                  payload_content_type='text/plain')
+    >>> secret = barbican.secrets.create(name=u'Self destruction sequence',
+    ...                                  payload=u'the magic words are squeamish ossifrage')
 
     >>> # Now let's store the secret by using its store() method. This will send the secret data
     >>> # to Barbican, where it will be encrypted and stored securely in the cloud.
diff --git a/barbicanclient/secrets.py b/barbicanclient/secrets.py
index dca5c56..fe015e7 100644
--- a/barbicanclient/secrets.py
+++ b/barbicanclient/secrets.py
@@ -12,11 +12,12 @@
 # implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import base64
 import functools
 import logging
-import six
 
 from oslo_utils.timeutils import parse_isotime
+import six
 
 from barbicanclient import base
 from barbicanclient import formatter
@@ -209,11 +210,21 @@ class Secret(SecretFormatter):
     @payload_content_type.setter
     @immutable_after_save
     def payload_content_type(self, value):
+        LOG.warning(
+            'DEPRECATION WARNING: Manually setting the payload_content_type '
+            'can lead to unexpected results.  It will be removed in a future '
+            'release.  See Launchpad Bug #1419166.'
+        )
         self._payload_content_type = value
 
     @payload_content_encoding.setter
     @immutable_after_save
     def payload_content_encoding(self, value):
+        LOG.warning(
+            'DEPRECATION WARNING: Manually setting the '
+            'payload_content_encoding can lead to unexpected results.  It '
+            'will be removed in a future release.  See Launchpad Bug #1419166.'
+        )
         self._payload_content_encoding = value
 
     def _fetch_payload(self):
@@ -234,15 +245,41 @@ class Secret(SecretFormatter):
         """
         secret_dict = base.filter_empty_keys({
             'name': self.name,
-            'payload': self.payload,
-            'payload_content_type': self.payload_content_type,
-            'payload_content_encoding': self.payload_content_encoding,
             'algorithm': self.algorithm,
             'mode': self.mode,
             'bit_length': self.bit_length,
             'expiration': self.expiration
         })
 
+        if self.payload_content_type:
+            """
+            Setting the payload_content_type and payload_content_encoding
+            manually is deprecated.  This clause of the if statement is here
+            for backwards compatibility and should be removed in a future
+            release.
+            """
+            secret_dict['payload'] = self.payload
+            secret_dict['payload_content_type'] = self.payload_content_type
+            secret_dict['payload_content_encoding'] = (
+                self.payload_content_encoding
+            )
+        elif type(self.payload) is six.binary_type:
+            """
+            six.binary_type is stored as application/octet-stream
+            and it is base64 encoded for a one-step POST
+            """
+            secret_dict['payload'] = (
+                base64.b64encode(self.payload)
+            ).decode('UTF-8')
+            secret_dict['payload_content_type'] = u'application/octet-stream'
+            secret_dict['payload_content_encoding'] = u'base64'
+        elif type(self.payload) is six.text_type:
+            """
+            six.text_type is stored as text/plain
+            """
+            secret_dict['payload'] = self.payload
+            secret_dict['payload_content_type'] = u'text/plain'
+
         LOG.debug("Request body: {0}".format(secret_dict))
 
         # Save, store secret_ref and return
@@ -332,8 +369,9 @@ class SecretManager(base.BaseEntityManager):
         Retrieve an existing Secret from Barbican
 
         :param str secret_ref: Full HATEOAS reference to a Secret
-        :param str payload_content_type: Content type to use for payload
-            decryption
+        :param str payload_content_type: DEPRECATED: Content type to use for
+            payload decryption. Setting this can lead to unexpected results.
+            See Launchpad Bug #1419166.
         :returns: Secret object retrieved from Barbican
         :rtype: :class:`barbicanclient.secrets.Secret`
         """
@@ -356,8 +394,12 @@ class SecretManager(base.BaseEntityManager):
 
         :param name: A friendly name for the Secret
         :param payload: The unencrypted secret data
-        :param payload_content_type: The format/type of the secret data
-        :param payload_content_encoding: The encoding of the secret data
+        :param payload_content_type: DEPRECATED: The format/type of the secret
+            data. Setting this can lead to unexpected results.  See Launchpad
+            Bug #1419166.
+        :param payload_content_encoding: DEPRECATED: The encoding of the secret
+            data. Setting this can lead to unexpected results.  See Launchpad
+            Bug #1419166.
         :param algorithm: The algorithm associated with this secret key
         :param bit_length: The bit length of this secret key
         :param mode: The algorithm mode used with this secret key
diff --git a/barbicanclient/tests/test_secrets.py b/barbicanclient/tests/test_secrets.py
index abed3c2..c7d4681 100644
--- a/barbicanclient/tests/test_secrets.py
+++ b/barbicanclient/tests/test_secrets.py
@@ -12,7 +12,7 @@
 # implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
+import base64
 import json
 
 from oslo.utils import timeutils
@@ -23,15 +23,14 @@ from barbicanclient import secrets, base
 
 class SecretData(object):
     def __init__(self):
-        self.name = 'Self destruction sequence'
-        self.payload = 'the magic words are squeamish ossifrage'
-        self.payload_content_type = 'text/plain'
-        self.content = 'text/plain'
-        self.algorithm = 'AES'
+        self.name = u'Self destruction sequence'
+        self.payload = u'the magic words are squeamish ossifrage'
+        self.payload_content_type = u'text/plain'
+        self.algorithm = u'AES'
         self.created = str(timeutils.utcnow())
 
         self.secret_dict = {'name': self.name,
-                            'status': 'ACTIVE',
+                            'status': u'ACTIVE',
                             'algorithm': self.algorithm,
                             'created': self.created}
 
@@ -65,8 +64,7 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         self.responses.post(self.entity_base + '/', json=data)
 
         secret = self.manager.create(name=self.secret.name,
-                                     payload=self.secret.payload,
-                                     payload_content_type=self.secret.content)
+                                     payload=self.secret.payload)
         secret_href = secret.store()
         self.assertEqual(self.entity_href, secret_href)
 
@@ -74,8 +72,6 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         secret_req = json.loads(self.responses.last_request.text)
         self.assertEqual(self.secret.name, secret_req['name'])
         self.assertEqual(self.secret.payload, secret_req['payload'])
-        self.assertEqual(self.secret.payload_content_type,
-                         secret_req['payload_content_type'])
 
     def test_should_store_via_attributes(self):
         data = {'secret_ref': self.entity_href}
@@ -84,7 +80,6 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         secret = self.manager.create()
         secret.name = self.secret.name
         secret.payload = self.secret.payload
-        secret.payload_content_type = self.secret.content
         secret_href = secret.store()
         self.assertEqual(self.entity_href, secret_href)
 
@@ -92,16 +87,108 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         secret_req = json.loads(self.responses.last_request.text)
         self.assertEqual(self.secret.name, secret_req['name'])
         self.assertEqual(self.secret.payload, secret_req['payload'])
-        self.assertEqual(self.secret.payload_content_type,
+
+    def test_should_store_binary_type_as_octet_stream(self):
+        """
+        We use six.binary_type as the canonical binary type.
+        The client should base64 encode the payload before sending the
+        request.
+        """
+        data = {'secret_ref': self.entity_href}
+        self.responses.post(self.entity_base + '/', json=data)
+
+        # This literal will have type(str) in Python 2, but will have
+        # type(bytes) in Python 3.  It is six.binary_type in both cases.
+        binary_payload = b'F\x130\x89f\x8e\xd9\xa1\x0e\x1f\r\xf67uu\x8b'
+
+        secret = self.manager.create()
+        secret.name = self.secret.name
+        secret.payload = binary_payload
+        secret.store()
+
+        secret_req = json.loads(self.responses.last_request.text)
+        self.assertEqual(self.secret.name, secret_req['name'])
+        self.assertEqual(u'application/octet-stream',
+                         secret_req['payload_content_type'])
+        self.assertEqual(u'base64',
+                         secret_req['payload_content_encoding'])
+        self.assertNotEqual(binary_payload, secret_req['payload'])
+
+    def test_should_store_text_type_as_text_plain(self):
+        """
+        We use six.text_type as the canonical text type.
+        """
+        data = {'secret_ref': self.entity_href}
+        self.responses.post(self.entity_base + '/', json=data)
+
+        # This literal will have type(unicode) in Python 2, but will have
+        # type(str) in Python 3.  It is six.text_type in both cases.
+        text_payload = u'time for an ice cold \U0001f37a'
+
+        secret = self.manager.create()
+        secret.payload = text_payload
+        secret.store()
+
+        secret_req = json.loads(self.responses.last_request.text)
+        self.assertEqual(text_payload, secret_req['payload'])
+        self.assertEqual(u'text/plain', secret_req['payload_content_type'])
+
+    def test_should_store_with_deprecated_content_type(self):
+        """
+        DEPRECATION WARNING:
+        Manually setting the payload_content_type is deprecated and will be
+        removed in a future release.
+        """
+        data = {'secret_ref': self.entity_href}
+        self.responses.post(self.entity_base + '/', json=data)
+
+        payload = 'I should be octet-stream'
+        payload_content_type = u'text/plain'
+
+        secret = self.manager.create()
+        secret.payload = payload
+        secret.payload_content_type = payload_content_type
+        secret.store()
+
+        secret_req = json.loads(self.responses.last_request.text)
+        self.assertEqual(payload, secret_req['payload'])
+        self.assertEqual(payload_content_type,
+                         secret_req['payload_content_type'])
+
+    def test_should_store_with_deprecated_content_encoding(self):
+        """
+        DEPRECATION WARNING:
+        Manually setting the payload_content_encoding is deprecated and will be
+        removed in a future release.
+        """
+        data = {'secret_ref': self.entity_href}
+        self.responses.post(self.entity_base + '/', json=data)
+
+        encoded_payload = base64.b64encode(
+            b'F\x130\x89f\x8e\xd9\xa1\x0e\x1f\r\xf67uu\x8b'
+        ).decode('UTF-8')
+        payload_content_type = u'application/octet-stream'
+        payload_content_encoding = u'base64'
+
+        secret = self.manager.create()
+        secret.payload = encoded_payload
+        secret.payload_content_type = payload_content_type
+        secret.payload_content_encoding = payload_content_encoding
+        secret.store()
+
+        secret_req = json.loads(self.responses.last_request.text)
+        self.assertEqual(encoded_payload, secret_req['payload'])
+        self.assertEqual(payload_content_type,
                          secret_req['payload_content_type'])
+        self.assertEqual(payload_content_encoding,
+                         secret_req['payload_content_encoding'])
 
     def test_should_be_immutable_after_submit(self):
         data = {'secret_ref': self.entity_href}
         self.responses.post(self.entity_base + '/', json=data)
 
         secret = self.manager.create(name=self.secret.name,
-                                     payload=self.secret.payload,
-                                     payload_content_type=self.secret.content)
+                                     payload=self.secret.payload)
         secret_href = secret.store()
 
         self.assertEqual(self.entity_href, secret_href)
@@ -149,7 +236,12 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         # Verify the correct URL was used to make the GET call
         self.assertEqual(self.entity_href, m.last_request.url)
 
-    def test_should_get_payload_only(self):
+    def test_should_get_payload_only_when_content_type_is_set(self):
+        """
+        DEPRECATION WARNING:
+        Manually setting the payload_content_type is deprecated and will be
+        removed in a future release.
+        """
         m = self.responses.get(self.entity_href,
                                request_headers={'Accept': 'application/json'},
                                json=self.secret.get_dict(self.entity_href))
@@ -182,7 +274,7 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         # Verify the correct URL was used to make the `get_raw` call
         self.assertEqual(self.entity_href, n.last_request.url)
 
-    def test_should_fetch_metadata_to_get_payload_if_no_content_type_set(self):
+    def test_should_fetch_metadata_to_get_payload(self):
         content_types_dict = {'default': 'application/octet-stream'}
 
         data = self.secret.get_dict(self.entity_href,
@@ -219,7 +311,12 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         self.assertEqual(self.entity_href, m.last_request.url)
         self.assertEqual(self.entity_href, n.last_request.url)
 
-    def test_should_decrypt_with_content_type(self):
+    def test_should_decrypt_when_content_type_is_set(self):
+        """
+        DEPRECATION WARNING:
+        Manually setting the payload_content_type is deprecated and will be
+        removed in a future release.
+        """
         decrypted = 'decrypted text here'
 
         request_headers = {'Accept': 'application/octet-stream'}
@@ -238,7 +335,7 @@ class WhenTestingSecrets(test_client.BaseEntityResource):
         # Verify the correct URL was used to make the call.
         self.assertEqual(self.entity_href, m.last_request.url)
 
-    def test_should_decrypt_without_content_type(self):
+    def test_should_decrypt(self):
         content_types_dict = {'default': 'application/octet-stream'}
         json = self.secret.get_dict(self.entity_href, content_types_dict)
         m = self.responses.get(self.entity_href,
diff --git a/doc/source/usage.rst b/doc/source/usage.rst
index 7eb0ea5..2f6211c 100644
--- a/doc/source/usage.rst
+++ b/doc/source/usage.rst
@@ -32,7 +32,7 @@ class that is exposed as the `secrets` attribute of the Client.
 
 Example::
 
-    # Store a random plain text password in Barbican
+    # Store a random text password in Barbican
 
     from barbicanclient import client
     import random
@@ -49,7 +49,6 @@ Example::
     my_secret = barbican.secrets.create()
     my_secret.name = u'Random plain text password'
     my_secret.payload = random_password(24)
-    my_secret.payload_content_type = u'text/plain'
 
     my_secret_ref = my_secret.store()
 
@@ -63,6 +62,31 @@ Example::
     retrieved_secret = barbican.secrets.get(my_secret_ref)
     my_password = retrieved_secret.payload
 
+Secret Content Types
+--------------------
+
+The Barbican service defines a Secret Content Type.  The client will choose
+the correct Content Type based on the type of the data that is set on the
+`Secret.payload` property.  The following table summarizes the mapping of
+Python types to Barbican Secret Content Types:
+
++-----------------+---------------+---------------+--------------------------+
+|    six Type     | Python 2 Type | Python 3 Type |  Barbican Content Type   |
++=================+===============+===============+==========================+
+| six.binary_type |      str      |     bytes     | application/octet-stream |
++-----------------+---------------+---------------+--------------------------+
+| six.text_type   |    unicode    |      str      |        text/plain        |
++-----------------+---------------+---------------+--------------------------+
+
+.. WARNING::
+   Previous versions of python-barbicanclient allowed the user to set the
+   `payload_content_type` and `payload_content_encoding` properties for any
+   secret.  This can lead to unexpected behavior such as changing a unicode
+   string back to a byte string in Python 2, and dropping the base64 encoding
+   of a binary secret as in Launchpad Bug #1419166.
+   Because of this, manually setting the `payload_content_type` and the
+   `payload_content_encoding` has been deprecated.
+
 Orders
 ======