libnm-util: fix hashing of secrets broken by 4d326182

4d326182 changed connection hashing slightly such that now base type settings
are always returned even if they are empty. Unfortunately a bunch of code in
the settings hashed connections with the ONLY_SECRETS flag and then checked
whether the returned hash was NULL or not to determine whether there were
any secrets, and then called nm_connection_update_secrets() with the hash.

nm_connection_update_secrets() would fail in the case where a setting
name was given, but the passed-in secrets hash did not contain any secrets
for the requested setting.  Instead, the function should return success
to match the semantics of passing in an entire connection hash which may
not have any secrets either.

3 files changed, 137 insertions, 29 deletions

diff --git a/libnm-util/nm-connection.c b/libnm-util/nm-connection.c
index be9d874db2..fe55b44c51 100644
--- a/libnm-util/nm-connection.c
+++ b/libnm-util/nm-connection.c
@@ -644,56 +644,93 @@ nm_connection_update_secrets (NMConnection *connection,
                               GError **error)
 {
 	NMSetting *setting;
-	gboolean success;
-	GHashTable *tmp;
+	gboolean success = FALSE, updated = FALSE;
+	GHashTable *setting_hash = NULL;
+	GHashTableIter iter;
+	const char *key;
+	gboolean hashed_connection = FALSE;
 
 	g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE);
 	g_return_val_if_fail (secrets != NULL, FALSE);
 	if (error)
 		g_return_val_if_fail (*error == NULL, FALSE);
 
+	/* Empty @secrets means success */
+	if (g_hash_table_size (secrets) == 0)
+		return TRUE;
+
+	/* For backwards compatibility, this function accepts either a hashed
+	 * connection (GHashTable of GHashTables of GValues) or a single hashed
+	 * setting (GHashTable of GValues).
+	 */
+	g_hash_table_iter_init (&iter, secrets);
+	while (g_hash_table_iter_next (&iter, (gpointer) &key, NULL)) {
+		if (_nm_setting_lookup_setting_type (key) != G_TYPE_INVALID) {
+			/* @secrets looks like a hashed connection */
+			hashed_connection = TRUE;
+			break;
+		}
+	}
+
 	if (setting_name) {
-		/* Update just one setting */
+		/* Update just one setting's secrets */
 		setting = nm_connection_get_setting_by_name (connection, setting_name);
 		if (!setting) {
 			g_set_error_literal (error,
-				                 NM_CONNECTION_ERROR,
-				                 NM_CONNECTION_ERROR_SETTING_NOT_FOUND,
-				                 setting_name);
+			                     NM_CONNECTION_ERROR,
+			                     NM_CONNECTION_ERROR_SETTING_NOT_FOUND,
+			                     setting_name);
 			return FALSE;
 		}
 
-		/* Check if this is a hash of hashes, ie a full deserialized connection,
-		 * not just a single hashed setting.
-		 */
-		tmp = g_hash_table_lookup (secrets, setting_name);
-		success = nm_setting_update_secrets (setting, tmp ? tmp : secrets, error);
-	} else {
-		GHashTableIter iter;
-		const char *name;
+		if (hashed_connection) {
+			setting_hash = g_hash_table_lookup (secrets, setting_name);
+			if (!setting_hash) {
+				/* The hashed connection that didn't contain any secrets for
+				 * @setting_name; just return success.
+				 */
+				success = TRUE;
+			}
+		}
 
-		success = TRUE; /* Just in case 'secrets' has no elements */
+		if (!success) {
+			updated = success = nm_setting_update_secrets (setting,
+			                                               setting_hash ? setting_hash : secrets,
+			                                               error);
+		}
+	} else {
+		if (!hashed_connection) {
+			g_set_error_literal (error,
+			                     NM_CONNECTION_ERROR,
+			                     NM_CONNECTION_ERROR_SETTING_NOT_FOUND,
+			                     key);
+			return FALSE;
+		}
 
-		/* Try as a serialized connection (GHashTable of GHashTables) */
+		/* Update each setting with any secrets from the hashed connection */
 		g_hash_table_iter_init (&iter, secrets);
-		while (g_hash_table_iter_next (&iter, (gpointer) &name, (gpointer) &tmp)) {
-			setting = nm_connection_get_setting_by_name (connection, name);
+		while (g_hash_table_iter_next (&iter, (gpointer) &key, (gpointer) &setting_hash)) {
+			setting = nm_connection_get_setting_by_name (connection, key);
 			if (!setting) {
 				g_set_error_literal (error,
-						             NM_CONNECTION_ERROR,
-						             NM_CONNECTION_ERROR_SETTING_NOT_FOUND,
-						             name);
+				                     NM_CONNECTION_ERROR,
+				                     NM_CONNECTION_ERROR_SETTING_NOT_FOUND,
+				                     key);
 				return FALSE;
 			}
 
 			/* Update the secrets for this setting */
-			success = nm_setting_update_secrets (setting, tmp, error);
-			if (success == FALSE)
+			success = nm_setting_update_secrets (setting, setting_hash, error);
+			if (success)
+				updated = TRUE;
+			else
 				break;
 		}
 	}
-	if (success)
+
+	if (updated)
 		g_signal_emit (connection, signals[SECRETS_UPDATED], 0, setting_name);
+
 	return success;
 }
 
diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c
index d8fa84d8e2..a36da29428 100644
--- a/libnm-util/nm-setting.c
+++ b/libnm-util/nm-setting.c
@@ -196,11 +196,7 @@ _nm_setting_lookup_setting_type (const char *name)
 	_ensure_registered ();
 
 	info = g_hash_table_lookup (registered_settings, name);
-	if (info)
-		return info->type;
-
-	g_warning ("Unknown setting '%s'", name);
-	return G_TYPE_INVALID;
+	return info ? info->type : G_TYPE_INVALID;
 }
 
 GType
diff --git a/libnm-util/tests/test-secrets.c b/libnm-util/tests/test-secrets.c
index 43156f100b..f770d53e3e 100644
--- a/libnm-util/tests/test-secrets.c
+++ b/libnm-util/tests/test-secrets.c
@@ -484,6 +484,8 @@ test_update_secrets_wifi_single_setting (void)
 	const char *wepkey = "11111111111111111111111111";
 	const char *tmp;
 
+	/* Test update with a hashed setting of 802-11-wireless secrets */
+
 	connection = wifi_connection_new ();
 
 	/* Build up the secrets hash */
@@ -518,6 +520,10 @@ test_update_secrets_wifi_full_hash (void)
 	const char *wepkey = "11111111111111111111111111";
 	const char *tmp;
 
+	/* Test update with a hashed connection containing only 802-11-wireless
+	 * setting and secrets.
+	 */
+
 	connection = wifi_connection_new ();
 
 	/* Build up the secrets hash */
@@ -552,6 +558,10 @@ test_update_secrets_wifi_bad_setting_name (void)
 	gboolean success;
 	const char *wepkey = "11111111111111111111111111";
 
+	/* Test that passing an invalid setting name to
+	 * nm_connection_update_secrets() fails with the correct error.
+	 */
+
 	connection = wifi_connection_new ();
 
 	/* Build up the secrets hash */
@@ -579,6 +589,10 @@ test_update_secrets_whole_connection (void)
 	gboolean success;
 	const char *wepkey = "11111111111111111111111111";
 
+	/* Test calling nm_connection_update_secrets() with an entire hashed
+	 * connection including non-secrets.
+	 */
+
 	connection = wifi_connection_new ();
 
 	/* Build up the secrets hash */
@@ -606,6 +620,8 @@ test_update_secrets_whole_connection_empty_hash (void)
 	GError *error = NULL;
 	gboolean success;
 
+	/* Test that updating secrets with an empty hash returns success */
+
 	connection = wifi_connection_new ();
 	secrets = g_hash_table_new (g_str_hash, g_str_equal);
 	success = nm_connection_update_secrets (connection, NULL, secrets, &error);
@@ -623,6 +639,10 @@ test_update_secrets_whole_connection_bad_setting (void)
 	gboolean success;
 	const char *wepkey = "11111111111111111111111111";
 
+	/* Test that sending a hashed connection containing an invalid setting
+	 * name fails with the right error.
+	 */
+
 	connection = wifi_connection_new ();
 
 	/* Build up the secrets hash */
@@ -646,6 +666,59 @@ test_update_secrets_whole_connection_bad_setting (void)
 	g_object_unref (connection);
 }
 
+static void
+test_update_secrets_whole_connection_empty_base_setting (void)
+{
+	NMConnection *connection;
+	GHashTable *secrets;
+	GError *error = NULL;
+	gboolean success;
+
+	/* Test that a hashed connection which does not have any hashed secrets
+	 * for the requested setting returns success.
+	 */
+
+	connection = wifi_connection_new ();
+	secrets = nm_connection_to_hash (connection, NM_SETTING_HASH_FLAG_ONLY_SECRETS);
+	g_assert_cmpint (g_hash_table_size (secrets), ==, 1);
+	g_assert (g_hash_table_lookup (secrets, NM_SETTING_WIRELESS_SETTING_NAME));
+
+	success = nm_connection_update_secrets (connection,
+	                                        NM_SETTING_WIRELESS_SECURITY_SETTING_NAME,
+	                                        secrets,
+	                                        &error);
+	g_assert_no_error (error);
+	g_assert (success);
+
+	g_hash_table_destroy (secrets);
+	g_object_unref (connection);
+}
+
+static void
+test_update_secrets_null_setting_name_with_setting_hash (void)
+{
+	NMConnection *connection;
+	GHashTable *secrets;
+	GError *error = NULL;
+	gboolean success;
+	const char *wepkey = "11111111111111111111111111";
+
+	/* Ensure that a NULL setting name and only a hashed setting fails */
+
+	connection = wifi_connection_new ();
+
+	secrets = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, value_destroy);
+	g_hash_table_insert (secrets, NM_SETTING_WIRELESS_SECURITY_WEP_KEY0, string_to_gvalue (wepkey));
+	g_hash_table_insert (secrets, NM_SETTING_WIRELESS_SECURITY_WEP_KEY_TYPE, uint_to_gvalue (NM_WEP_KEY_TYPE_KEY));
+
+	success = nm_connection_update_secrets (connection, NULL, secrets, &error);
+	g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_SETTING_NOT_FOUND);
+	g_assert (!success);
+
+	g_hash_table_destroy (secrets);
+	g_object_unref (connection);
+}
+
 int main (int argc, char **argv)
 {
 	GError *error = NULL;
@@ -669,6 +742,8 @@ int main (int argc, char **argv)
 	test_update_secrets_whole_connection ();
 	test_update_secrets_whole_connection_empty_hash ();
 	test_update_secrets_whole_connection_bad_setting ();
+	test_update_secrets_whole_connection_empty_base_setting ();
+	test_update_secrets_null_setting_name_with_setting_hash ();
 
 	base = g_path_get_basename (argv[0]);
 	fprintf (stdout, "%s: SUCCESS\n", base);