[gnome-keyring/gnome-3-2] secret-store: Return results with the most recent result first

[Stefan Walter <stefw src gnome org>]

commit bc01094411d724470ebda56d4515f45760f8d008
Author: Stef Walter <stefw collabora co uk>
Date:   Thu Oct 20 16:38:00 2011 +0200

    secret-store: Return results with the most recent result first
    
     * This is to help programs that have erroneusly stored more than
       one password with given attributes, and keep finding the old one.
     * The most recent one is usually correct.

 pkcs11/secret-store/gkm-secret-search.c        |   79 +++++++++++++----------
 pkcs11/secret-store/tests/test-secret-search.c |   73 ++++++++++++++++++++++
 2 files changed, 118 insertions(+), 34 deletions(-)
---
diff --git a/pkcs11/secret-store/gkm-secret-search.c b/pkcs11/secret-store/gkm-secret-search.c
index fd86056..02eb9e9 100644
--- a/pkcs11/secret-store/gkm-secret-search.c
+++ b/pkcs11/secret-store/gkm-secret-search.c
@@ -48,15 +48,11 @@ struct _GkmSecretSearch {
 	gchar *collection_id;
 	GHashTable *fields;
 	GList *managers;
-	GHashTable *handles;
+	GHashTable *objects;
 };
 
 G_DEFINE_TYPE (GkmSecretSearch, gkm_secret_search, GKM_TYPE_OBJECT);
 
-/* -----------------------------------------------------------------------------
- * INTERNAL
- */
-
 static gboolean
 match_object_against_criteria (GkmSecretSearch *self, GkmObject *object)
 {
@@ -89,17 +85,13 @@ static void
 on_manager_added_object (GkmManager *manager, GkmObject *object, gpointer user_data)
 {
 	GkmSecretSearch *self = user_data;
-	CK_OBJECT_HANDLE handle;
 
 	g_return_if_fail (GKM_IS_SECRET_SEARCH (self));
 
-	handle = gkm_object_get_handle (object);
-	g_return_if_fail (handle);
-
-	g_return_if_fail (g_hash_table_lookup (self->handles, &handle) == NULL);
+	g_return_if_fail (g_hash_table_lookup (self->objects, object) == NULL);
 
 	if (match_object_against_criteria (self, object)) {
-		g_hash_table_replace (self->handles, gkm_util_ulong_alloc (handle), "unused");
+		g_hash_table_replace (self->objects, g_object_ref (object), "unused");
 		gkm_object_notify_attribute (GKM_OBJECT (self), CKA_G_MATCHED);
 	}
 }
@@ -108,17 +100,11 @@ static void
 on_manager_removed_object (GkmManager *manager, GkmObject *object, gpointer user_data)
 {
 	GkmSecretSearch *self = user_data;
-	CK_OBJECT_HANDLE handle;
 
 	g_return_if_fail (GKM_IS_SECRET_SEARCH (self));
 
-	handle = gkm_object_get_handle (object);
-	g_return_if_fail (handle);
-
-	if (g_hash_table_lookup (self->handles, &handle) != NULL) {
-		g_hash_table_remove (self->handles, &handle);
+	if (g_hash_table_remove (self->objects, object))
 		gkm_object_notify_attribute (GKM_OBJECT (self), CKA_G_MATCHED);
-	}
 }
 
 static void
@@ -138,17 +124,15 @@ on_manager_changed_object (GkmManager *manager, GkmObject *object,
 
 	/* Should we have this object? */
 	if (match_object_against_criteria (self, object)) {
-		if (g_hash_table_lookup (self->handles, &handle) == NULL) {
-			g_hash_table_replace (self->handles, gkm_util_ulong_alloc (handle), "unused");
+		if (g_hash_table_lookup (self->objects, object) == NULL) {
+			g_hash_table_replace (self->objects, g_object_ref (object), "unused");
 			gkm_object_notify_attribute (GKM_OBJECT (self), CKA_G_MATCHED);
 		}
 
 	/* Should we not have this object? */
 	} else {
-		if (g_hash_table_lookup (self->handles, &handle) != NULL) {
-			g_hash_table_remove (self->handles, &handle);
+		if (g_hash_table_remove (self->objects, object))
 			gkm_object_notify_attribute (GKM_OBJECT (self), CKA_G_MATCHED);
-		}
 	}
 }
 
@@ -250,34 +234,57 @@ factory_create_search (GkmSession *session, GkmTransaction *transaction,
 	return GKM_OBJECT (search);
 }
 
-static void
-add_each_handle_to_array (gpointer key, gpointer value, gpointer user_data)
+static gint
+on_matched_sort_modified (gconstpointer a,
+                          gconstpointer b)
 {
-	GArray *array = user_data;
-	CK_OBJECT_HANDLE *handle = key;
-	g_array_append_val (array, *handle);
+	glong modified_a;
+	glong modified_b;
+
+	/* Sorting in reverse order */
+
+	modified_a = gkm_secret_object_get_modified (GKM_SECRET_OBJECT (a));
+	modified_b = gkm_secret_object_get_modified (GKM_SECRET_OBJECT (b));
+
+	if (modified_a < modified_b)
+		return 1;
+	if (modified_a > modified_b)
+		return -1;
+
+	return 0;
 }
 
 static CK_RV
-attribute_set_handles (GHashTable *handles, CK_ATTRIBUTE_PTR attr)
+attribute_set_handles (GHashTable *objects,
+                       CK_ATTRIBUTE_PTR attr)
 {
+	GList *list, *l;
 	GArray *array;
+	gulong handle;
 	CK_RV rv;
 
-	g_assert (handles);
+	g_assert (objects);
 	g_assert (attr);
 
 	/* Want the length */
 	if (!attr->pValue) {
-		attr->ulValueLen = sizeof (CK_OBJECT_HANDLE) * g_hash_table_size (handles);
+		attr->ulValueLen = sizeof (CK_OBJECT_HANDLE) * g_hash_table_size (objects);
 		return CKR_OK;
 	}
 
 	/* Get the actual values */
+	list = g_list_sort (g_hash_table_get_keys (objects), on_matched_sort_modified);
 	array = g_array_new (FALSE, TRUE, sizeof (CK_OBJECT_HANDLE));
-	g_hash_table_foreach (handles, add_each_handle_to_array, array);
+
+	for (l = list; l != NULL; l = g_list_next (l)) {
+		handle = gkm_object_get_handle (l->data);
+		g_array_append_val (array, handle);
+	}
+
 	rv = gkm_attribute_set_data (attr, array->data, array->len * sizeof (CK_OBJECT_HANDLE));
 	g_array_free (array, TRUE);
+	g_list_free (list);
+
 	return rv;
 }
 
@@ -302,7 +309,7 @@ gkm_secret_search_get_attribute (GkmObject *base, GkmSession *session, CK_ATTRIB
 	case CKA_G_FIELDS:
 		return gkm_secret_fields_serialize (attr, self->fields);
 	case CKA_G_MATCHED:
-		return attribute_set_handles (self->handles, attr);
+		return attribute_set_handles (self->objects, attr);
 	}
 
 	return GKM_OBJECT_CLASS (gkm_secret_search_parent_class)->get_attribute (base, session, attr);
@@ -312,7 +319,7 @@ gkm_secret_search_get_attribute (GkmObject *base, GkmSession *session, CK_ATTRIB
 static void
 gkm_secret_search_init (GkmSecretSearch *self)
 {
-	self->handles = g_hash_table_new_full (gkm_util_ulong_hash, gkm_util_ulong_equal, gkm_util_ulong_free, NULL);
+	self->objects = g_hash_table_new_full (g_direct_hash, g_direct_equal, g_object_unref, NULL);
 }
 
 static GObject*
@@ -384,6 +391,8 @@ gkm_secret_search_dispose (GObject *obj)
 	g_free (self->collection_id);
 	self->collection_id = NULL;
 
+	g_hash_table_remove_all (self->objects);
+
 	G_OBJECT_CLASS (gkm_secret_search_parent_class)->dispose (obj);
 }
 
@@ -398,6 +407,8 @@ gkm_secret_search_finalize (GObject *obj)
 		g_hash_table_destroy (self->fields);
 	self->fields = NULL;
 
+	g_hash_table_destroy (self->objects);
+
 	G_OBJECT_CLASS (gkm_secret_search_parent_class)->finalize (obj);
 }
 
diff --git a/pkcs11/secret-store/tests/test-secret-search.c b/pkcs11/secret-store/tests/test-secret-search.c
index 303a314..fc75996 100644
--- a/pkcs11/secret-store/tests/test-secret-search.c
+++ b/pkcs11/secret-store/tests/test-secret-search.c
@@ -31,6 +31,7 @@
 
 #include "gkm/gkm-session.h"
 #include "gkm/gkm-transaction.h"
+#include "gkm/gkm-test.h"
 
 #include "pkcs11/pkcs11i.h"
 
@@ -360,6 +361,77 @@ test_for_collection_no_match (Test *test, gconstpointer unused)
 	g_object_unref (ocoll);
 }
 
+static void
+test_order (Test *test,
+            gconstpointer unused)
+{
+	CK_ATTRIBUTE attrs[] = {
+	        { CKA_G_FIELDS, "test\0value", 11 },
+	        { CKA_G_COLLECTION, "other-collection", 16 },
+	};
+
+	GkmObject *object = NULL;
+	GkmSecretCollection *collection;
+	GkmSecretItem *item;
+	GHashTable *fields;
+	gulong *matched;
+	gsize vsize;
+	gchar *identifier;
+	glong modified;
+	glong last;
+	gint i;
+	CK_RV rv;
+
+	collection = g_object_new (GKM_TYPE_SECRET_COLLECTION,
+	                           "module", test->module,
+	                           "manager", gkm_session_get_manager (test->session),
+	                           "identifier", "other-collection",
+	                           NULL);
+
+	gkm_object_expose (GKM_OBJECT (collection), TRUE);
+
+	/* Add a bunch of items */
+	for (i = 0; i < 2000; i++) {
+		identifier = g_strdup_printf ("item-%d", i);
+		item = gkm_secret_collection_new_item (collection, identifier);
+		g_free (identifier);
+
+		/* Make it match, but remember, wrong collection*/
+		fields = gkm_secret_fields_new ();
+		gkm_secret_fields_add (fields, "test", "value");
+		gkm_secret_item_set_fields (item, fields);
+		g_hash_table_unref (fields);
+
+		gkm_secret_object_set_modified (GKM_SECRET_OBJECT (item),
+		                                (glong)g_random_int ());
+		gkm_object_expose (GKM_OBJECT (item), TRUE);
+	}
+
+	object = gkm_session_create_object_for_factory (test->session, test->factory, NULL, attrs, 2);
+	g_assert (object != NULL);
+	g_assert (GKM_IS_SECRET_SEARCH (object));
+
+	/* No objects matched */
+	matched = gkm_object_get_attribute_data (object, test->session, CKA_G_MATCHED, &vsize);
+	g_assert (matched != NULL);
+	gkm_assert_cmpulong (vsize, ==, sizeof (gulong) * 2000);
+
+	last = G_MAXLONG;
+	for (i = 0; i < vsize / sizeof (gulong); i++) {
+		rv = gkm_session_lookup_readable_object (test->session, matched[i], (GkmObject **)&item);
+		gkm_assert_cmprv (rv, ==, CKR_OK);
+
+		modified = gkm_secret_object_get_modified (GKM_SECRET_OBJECT (item));
+		g_assert (last > modified);
+		last = modified;
+	}
+
+	g_free (matched);
+
+	g_object_unref (object);
+	g_object_unref (collection);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -375,6 +447,7 @@ main (int argc, char **argv)
 	g_test_add ("/secret-store/search/for_bad_collection", Test, NULL, setup, test_for_bad_collection, teardown);
 	g_test_add ("/secret-store/search/for_collection", Test, NULL, setup, test_for_collection, teardown);
 	g_test_add ("/secret-store/search/for_collection_no_match", Test, NULL, setup, test_for_collection_no_match, teardown);
+	g_test_add ("/secret-store/search/order", Test, NULL, setup, test_order, teardown);
 
 	return g_test_run ();
 }