From 701a312eb72a71fcae921e1a304ebaa0da960574 Mon Sep 17 00:00:00 2001 From: panoplie <59100-panoplie@users.noreply.gitlab.gnome.org> Date: Fri, 1 Apr 2022 16:24:46 +0200 Subject: [PATCH 1/3] async-search: unlock keyring before getting secret attributes In gnome-keyring, the secret items attributes are not visible until the keyring is unlocked. But in libsecret, the asynchronous secret search function unlocks the keyring after and not before the attributes dbus pull. So when the keyring is locked and you run secret_service_search(), you get hashed or empty attributes because the keyring was locked at the time these attributes were pulled. If you run this function when the keyring is already unlocked, there is no problem. This commit moves the unlock routine before the attributes pull to make the asynchronous search function working correctly when the keyring is locked initially. The secret_search_unlock_load_or_complete function should be renamed to secret_service_load_or_complete. To keep this commit readable, it is done in the next commit. Issues #6 gnome-shell#4780 --- libsecret/secret-methods.c | 101 ++++++++++++++++++------------------- 1 file changed, 49 insertions(+), 52 deletions(-) diff --git a/libsecret/secret-methods.c b/libsecret/secret-methods.c index c84419e..7618346 100644 --- a/libsecret/secret-methods.c +++ b/libsecret/secret-methods.c @@ -97,34 +97,6 @@ on_search_secrets (GObject *source, g_clear_object (&task); } -static void -on_search_unlocked (GObject *source, - GAsyncResult *result, - gpointer user_data) -{ - GTask *task = G_TASK (user_data); - SearchClosure *search = g_task_get_task_data (task); - GCancellable *cancellable = g_task_get_cancellable (task); - GList *items; - - /* Note that we ignore any unlock failure */ - secret_service_unlock_finish (search->service, result, NULL, NULL); - - /* If loading secrets ... locked items automatically ignored */ - if (search->flags & SECRET_SEARCH_LOAD_SECRETS) { - items = g_hash_table_get_values (search->items); - secret_item_load_secrets (items, cancellable, - on_search_secrets, g_object_ref (task)); - g_list_free (items); - - /* No additional options, just complete */ - } else { - g_task_return_boolean (task, TRUE); - } - - g_clear_object (&task); -} - static void secret_search_unlock_load_or_complete (GTask *task, SearchClosure *search) @@ -132,15 +104,8 @@ secret_search_unlock_load_or_complete (GTask *task, GCancellable *cancellable = g_task_get_cancellable (task); GList *items; - /* If unlocking then unlock all the locked items */ - if (search->flags & SECRET_SEARCH_UNLOCK) { - items = search_closure_build_items (search, search->locked); - secret_service_unlock (search->service, items, cancellable, - on_search_unlocked, g_object_ref (task)); - g_list_free_full (items, g_object_unref); - /* If loading secrets ... locked items automatically ignored */ - } else if (search->flags & SECRET_SEARCH_LOAD_SECRETS) { + if (search->flags & SECRET_SEARCH_LOAD_SECRETS) { items = g_hash_table_get_values (search->items); secret_item_load_secrets (items, cancellable, on_search_secrets, g_object_ref (task)); @@ -200,6 +165,44 @@ search_load_item_async (SecretService *self, } } +static void +load_items (SearchClosure *closure, + GTask *task) +{ + SecretService *self = closure->service; + gint want = 1; + gint count = 0; + gint i; + + if (closure->flags & SECRET_SEARCH_ALL) + want = G_MAXINT; + + for (i = 0; count < want && closure->unlocked[i] != NULL; i++, count++) + search_load_item_async (self, task, closure, closure->unlocked[i]); + for (i = 0; count < want && closure->locked[i] != NULL; i++, count++) + search_load_item_async (self, task, closure, closure->locked[i]); + + /* No items loading, complete operation now */ + if (closure->loading == 0) + secret_search_unlock_load_or_complete (task, closure); +} + +static void +on_unlock_paths (GObject *source, + GAsyncResult *result, + gpointer user_data) +{ + GTask *task = G_TASK (user_data); + SearchClosure *closure = g_task_get_task_data (task); + SecretService *self = closure->service; + + /* Note that we ignore any unlock failure */ + secret_service_unlock_dbus_paths_finish (self, result, NULL, NULL); + + load_items (closure, task); + g_clear_object (&task); +} + static void on_search_paths (GObject *source, GAsyncResult *result, @@ -209,27 +212,21 @@ on_search_paths (GObject *source, SearchClosure *closure = g_task_get_task_data (task); SecretService *self = closure->service; GError *error = NULL; - gint want = 1; - gint count; - gint i; secret_service_search_for_dbus_paths_finish (self, result, &closure->unlocked, &closure->locked, &error); if (error == NULL) { - want = 1; - if (closure->flags & SECRET_SEARCH_ALL) - want = G_MAXINT; - count = 0; - - for (i = 0; count < want && closure->unlocked[i] != NULL; i++, count++) - search_load_item_async (self, task, closure, closure->unlocked[i]); - for (i = 0; count < want && closure->locked[i] != NULL; i++, count++) - search_load_item_async (self, task, closure, closure->locked[i]); - - /* No items loading, complete operation now */ - if (closure->loading == 0) - secret_search_unlock_load_or_complete (task, closure); + /* If unlocking then unlock all the locked items */ + if (closure->flags & SECRET_SEARCH_UNLOCK) { + GCancellable *cancellable = g_task_get_cancellable (task); + const gchar **const_locked = (const gchar**) closure->locked; + secret_service_unlock_dbus_paths (self, const_locked, cancellable, + on_unlock_paths, + g_steal_pointer (&task)); + } else { + load_items (closure, task); + } } else { g_task_return_error (task, g_steal_pointer (&error)); } From 31ea8cb41d30dddcbcc1ec07a4120027602c6319 Mon Sep 17 00:00:00 2001 From: panoplie <59100-panoplie@users.noreply.gitlab.gnome.org> Date: Sun, 11 Sep 2022 16:54:54 +0200 Subject: [PATCH 2/3] Rename secret_search_unlock_load_or_complete() This function does not unlock the keyring anymore so we remove the "unlock" term from its name. --- libsecret/secret-methods.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libsecret/secret-methods.c b/libsecret/secret-methods.c index 7618346..75dbed3 100644 --- a/libsecret/secret-methods.c +++ b/libsecret/secret-methods.c @@ -98,8 +98,8 @@ on_search_secrets (GObject *source, } static void -secret_search_unlock_load_or_complete (GTask *task, - SearchClosure *search) +secret_search_load_or_complete (GTask *task, + SearchClosure *search) { GCancellable *cancellable = g_task_get_cancellable (task); GList *items; @@ -141,7 +141,7 @@ on_search_loaded (GObject *source, /* We're done loading, lets go to the next step */ if (closure->loading == 0) - secret_search_unlock_load_or_complete (task, closure); + secret_search_load_or_complete (task, closure); g_clear_object (&task); } @@ -184,7 +184,7 @@ load_items (SearchClosure *closure, /* No items loading, complete operation now */ if (closure->loading == 0) - secret_search_unlock_load_or_complete (task, closure); + secret_search_load_or_complete (task, closure); } static void From 7387774263cd344d08a68b06e1bf10e9ce5e6a53 Mon Sep 17 00:00:00 2001 From: panoplie <59100-panoplie@users.noreply.gitlab.gnome.org> Date: Sat, 12 Nov 2022 16:50:47 +0100 Subject: [PATCH 3/3] sync-search: unlock keyring before getting secret attributes In gnome-keyring, the secret items attributes are not visible until the keyring is unlocked. But in libsecret, the synchronous secret search function unlocks the keyring after and not before the attributes dbus pull. So when the keyring is locked and you run secret_service_search_sync(), you get hashed or empty attributes because the keyring was locked at the time these attributes were pulled. If you run this function when the keyring is already unlocked, there is no problem. This commit moves the unlock routine before the attributes pull to make the synchronous search function working correctly when the keyring is locked initially. Issues #6 gnome-shell#4780 --- libsecret/secret-methods.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libsecret/secret-methods.c b/libsecret/secret-methods.c index 75dbed3..e0cb3eb 100644 --- a/libsecret/secret-methods.c +++ b/libsecret/secret-methods.c @@ -475,6 +475,10 @@ secret_service_search_sync (SecretService *service, return NULL; } + if (flags & SECRET_SEARCH_UNLOCK) + secret_service_unlock_dbus_paths_sync (service, (const gchar**) locked_paths, + cancellable, NULL, NULL); + ret = TRUE; want = 1; @@ -509,9 +513,6 @@ secret_service_search_sync (SecretService *service, items = g_list_concat (items, g_list_copy (unlocked)); items = g_list_reverse (items); - if (flags & SECRET_SEARCH_UNLOCK) - secret_service_unlock_sync (service, locked, cancellable, NULL, NULL); - if (flags & SECRET_SEARCH_LOAD_SECRETS) secret_item_load_secrets_sync (items, NULL, NULL);