Merge lp:~alecu/ubuntu-sso-client/keep-old-credentials into lp:ubuntu-sso-client

[Alejandro J. Cura]

Use a proper dictionary with different attributes for each key so old keys are not overwritten nor deleted.

The gnome keyring needs to be passed a dictionary containing attributes to single out each key in a given keyring. An empty dictionary was being used instead, which lead to old keys being deleted.

To solve this issue we are using a proper dictionary with different attributes for each key.

[Rodrigo Moya (rodrigo-moya)]

review: Approve

[Natalia Bidart (nataliabidart)]

I think we should starting adding some test cases to this module. From a talk with alecu on IRC:

(04:21:22 PM) alecu: nessita, should I add info on how to run it to the merge proposal?
(04:22:03 PM) nessita: alecu: instructions would be nice
(04:22:28 PM) nessita: alecu: but also some unit tests. Could you please explain why is not possible to add unittests?
(04:24:09 PM) alecu: nessita, I can add unit tests, sure. But I can't find a way to add a unit test that checks if this problem is fixed, because checking it requires accessing the gnomekeyring
(04:25:13 PM) nessita: alecu: maybe I don't fully understand this problem :-), so, could you please explain?
(04:34:18 PM) alecu: nessita, the problem was that when adding new credentials to the keyring, old credentials with the same set of attributes (in this case, an empty dict of attributes) were being erased. The way to check if old credentials are kept is by adding a new sets of credentials, and checking that they are still there after adding a second new set of credentials.
(04:38:03 PM) nessita: alecu: I don't see why that can't be tested with a mock. We can create a faked gnome keyring class and add two set of credentials, and assert the both are there
(04:38:30 PM) nessita: and this mock will behave like the real gnome keyirng when the set of attrs is the same (the emtpy dict)
(04:39:02 PM) nessita: we can also assert that the gnomekeyring.item_delete_sync is not called while doing a set_ubuntusso_attr
(04:39:24 PM) nessita: that last check I think is the most important one, if I understood correctly

review: Needs Fixing

[lp:~alecu/ubuntu-sso-client/keep-old-credentials]

577. By Alejandro J. Cura on 2010-08-19

tests for the keyring module

[Alejandro J. Cura (alecu)]

Added tests as per nessita's suggestion.

[Natalia Bidart (nataliabidart)]

Very nice, thanks!

review: Approve

=== modified file 'ubuntu_sso/keyring.py'
--- ubuntu_sso/keyring.py	2010-08-06 22:40:02 +0000
+++ ubuntu_sso/keyring.py	2010-08-19 05:52:55 +0000
@@ -2,6 +2,7 @@
 #
 # Authors:
 #  Andrew Higginson
+#  Alejandro J. Cura <[e-mail]>
 #
 # This program is free software; you can redistribute it and/or modify it under
 # the terms of the GNU General Public License as published by the Free Software
@@ -39,14 +40,7 @@
         keyring_names = gnomekeyring.list_keyring_names_sync()
         if not name in keyring_names:
             gnomekeyring.create_sync(name)
-
-    def _item_exists(self, sync, name):
-        """
-        Returns whether a named item exists in a
-        named keyring
-        """
-        return self._get_item_id_from_name(sync, name) != None
-
+
     def _get_item_id_from_name(self, sync, name):
         """
         Returns the ID for a named item
@@ -69,25 +63,25 @@
                 return item_info
         return None
 
-    def set_ubuntusso_attr(self, attr):
-        """
-        Sets the attributes of the Ubuntu SSO item
-        """
-        # Creates the secret from the attributes
-        secret = urlencode(attr)
+    def set_ubuntusso_attr(self, cred):
+        """
+        Sets the credentials of the Ubuntu SSO item
+        """
+        # Creates the secret from the credentials
+        secret = urlencode(cred)
 
         # Create the keyring
         self._create_keyring(self.KEYRING_NAME)
 
-        # If the item already exists, delete it
-        if self._item_exists(self.KEYRING_NAME, self.app_name):
-            item_id = self._get_item_id_from_name(self.KEYRING_NAME,
-                                                  self.app_name)
-            gnomekeyring.item_delete_sync(self.KEYRING_NAME, item_id)
+        # No need to delete the item if it already exists
 
-        # Add our SSO item
+        # A set of attributes for this credentials
+        attr = {"key-type": "Ubuntu SSO credentials",
+                "token-name": cred["name"].encode("utf8")}
+
+        # Add our SSO credentials to the keyring
         gnomekeyring.item_create_sync(self.KEYRING_NAME,
-                gnomekeyring.ITEM_GENERIC_SECRET, self.app_name, {},
+                gnomekeyring.ITEM_GENERIC_SECRET, self.app_name, attr,
                 secret, True)
 
     def get_ubuntusso_attr(self):
@@ -101,11 +95,31 @@
         secret = self._get_item_info_from_name(self.KEYRING_NAME,
                                                self.app_name).get_secret()
         return dict(parse_qsl(secret))
+
+    def delete_ubuntusso_attr(self):
+        """
+        Delete a set of credentials from the keyring
+        """
+        item_id = self._get_item_id_from_name(self.KEYRING_NAME,
+                                              self.app_name)
+        if item_id is not None:
+            gnomekeyring.item_delete_sync(self.KEYRING_NAME, item_id)
 
 if __name__ == "__main__":
-    SSO_ITEM_NAME = "Software Center UbuntuSSO token"
-    keyring = Keyring(SSO_ITEM_NAME)
-    keyring.set_ubuntusso_attr({"ha":"hehddddeff", "hi":"hggehes", "ho":"he"})
-    print keyring.get_ubuntusso_attr()
-
-
+    kr1 = Keyring("Test key 1")
+    kr2 = Keyring("Test key 2")
+
+    kr1.delete_ubuntusso_attr()
+    kr2.delete_ubuntusso_attr()
+
+    d1 = {"name":"test-1", "ha":"hehddddeff", "hi":"hggehes", "ho":"he"}
+    d2 = {"name":"test-2", "hi":"ho", "let's":"go"}
+
+    kr1.set_ubuntusso_attr(d1)
+    kr2.set_ubuntusso_attr(d2)
+
+    r1 = kr1.get_ubuntusso_attr()
+    r2 = kr2.get_ubuntusso_attr()
+
+    assert r1 == d1
+    assert r2 == d2

=== added file 'ubuntu_sso/tests/test_keyring.py'
--- ubuntu_sso/tests/test_keyring.py	1970-01-01 00:00:00 +0000
+++ ubuntu_sso/tests/test_keyring.py	2010-08-19 05:52:55 +0000
@@ -0,0 +1,115 @@
+# test_keyring - tests for ubuntu_sso.keyring
+#
+# Author: Alejandro J. Cura <[e-mail]>
+#
+# Copyright 2010 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""Tests for the keyring.py module."""
+
+import gnomekeyring
+from twisted.trial.unittest import TestCase
+
+from ubuntu_sso import keyring
+
+
+class MockInfoSync(object):
+    """A mock object that represents a fake key."""
+
+    def __init__(self, (key, key_name, secret)):
+        """Initialize this instance."""
+        self.key = key
+        self.key_name = key_name
+        self.secret = secret
+
+    def get_display_name(self):
+        """Return info on the mocked object."""
+        return self.key_name
+
+
+class MockGnomeKeyring(object):
+    """A mock keyring that stores keys according to a given attr."""
+
+    def __init__(self):
+        """Initialize this instance."""
+        self.keyrings = set()
+        self.store = []
+        self.deleted = []
+
+    def item_create_sync(self, keyring_name, item_type, key_name, attr,
+                         secret, update_if_exists):
+        """Sets a value in the keyring."""
+        # we'll use the attr as the dict key, so make it a tuple
+        key = tuple(attr.items())
+        self.store.append((key, key_name, secret))
+
+    def item_delete_sync(self, keyring_name, item_id):
+        """Makes a list of deleted items."""
+        key, key_name, secret = self.store.pop(item_id)
+        self.deleted.append(key_name)
+
+    def list_item_ids_sync(self, keyring_name):
+        """Return a list of ids for all the items."""
+        return [n for n, v in enumerate(self.store)]
+
+    def item_get_info_sync(self, keyring_name, item_id):
+        """Return an info sync object for the item."""
+        return MockInfoSync(self.store[item_id])
+
+    def list_keyring_names_sync(self):
+        """The keyring you are looking for may be here."""
+        return self.keyrings
+
+    def create_sync(self, name):
+        """Thanks, I'll create that keyring for you."""
+        self.keyrings.add(name)
+
+    def is_available(self):
+        """A very available keyring."""
+        return True
+
+
+class TestKeyring(TestCase):
+    """Test the gnome keyring related functions."""
+    def setUp(self):
+        """Initialize the mock used in these tests."""
+        self.mgk = MockGnomeKeyring()
+        self.patch(gnomekeyring, "item_create_sync", self.mgk.item_create_sync)
+        self.patch(gnomekeyring, "is_available", self.mgk.is_available)
+        self.patch(gnomekeyring, "create_sync", self.mgk.create_sync)
+        self.patch(gnomekeyring, "list_keyring_names_sync",
+                                              self.mgk.list_keyring_names_sync)
+        self.patch(gnomekeyring, "list_item_ids_sync",
+                                                   self.mgk.list_item_ids_sync)
+        self.patch(gnomekeyring, "item_get_info_sync",
+                                                   self.mgk.item_get_info_sync)
+        self.patch(gnomekeyring, "item_delete_sync", self.mgk.item_delete_sync)
+
+    def test_set_ubuntusso(self):
+        """Test that the set method does not erase previous keys."""
+        sample_creds = {"name": "sample creds name"}
+        sample_creds2 = {"name": "sample creds name 2"}
+        keyring.Keyring("appname").set_ubuntusso_attr(sample_creds)
+        keyring.Keyring("appname").set_ubuntusso_attr(sample_creds2)
+
+        self.assertEqual(len(self.mgk.store), 2)
+        self.assertEqual(len(self.mgk.deleted), 0)
+
+    def test_delete_ubuntusso(self):
+        """Test that a given key is deleted."""
+        sample_creds = {"name": "sample creds name"}
+        keyring.Keyring("appname").set_ubuntusso_attr(sample_creds)
+        keyring.Keyring("appname").delete_ubuntusso_attr()
+
+        self.assertEqual(len(self.mgk.store), 0)
+        self.assertEqual(len(self.mgk.deleted), 1)