keyfile: read and write SSIDs as strings by default

Keep compat with old format if the SSID includes unprintable
characters.  But having to type an int list for an SSID is just silly
and it's about damn time we fix that.

5 files changed, 273 insertions, 1 deletions

diff --git a/system-settings/plugins/keyfile/reader.c b/system-settings/plugins/keyfile/reader.c
index 8a81df0df1..a5b4219c4a 100644
--- a/system-settings/plugins/keyfile/reader.c
+++ b/system-settings/plugins/keyfile/reader.c
@@ -40,6 +40,7 @@
 #include <arpa/inet.h>
 #include <netinet/ether.h>
 #include <string.h>
+#include <ctype.h>
 
 #include "nm-dbus-glib-types.h"
 #include "reader.h"
@@ -731,6 +732,68 @@ read_hash_of_string (GKeyFile *file, NMSetting *setting, const char *key)
 	g_strfreev (keys);
 }
 
+static void
+ssid_parser (NMSetting *setting, const char *key, GKeyFile *keyfile)
+{
+	const char *setting_name = nm_setting_get_name (setting);
+	GByteArray *array = NULL;
+	char *p, *tmp_string;
+	gint *tmp_list;
+	gsize length;
+	int i;
+
+	/* New format: just a string.  We try parsing the new format if there are
+	 * no ';' in the string or it's not just numbers.
+	 */
+	p = tmp_string = g_key_file_get_string (keyfile, setting_name, key, NULL);
+	if (tmp_string) {
+		gboolean new_format = FALSE;
+
+		if (strchr (p, ';') == NULL)
+			new_format = TRUE;
+		else {
+			new_format = TRUE;
+			while (p && *p) {
+				if (!isdigit (*p++)) {
+					new_format = FALSE;
+					break;
+				}
+			}
+		}
+
+		if (new_format) {
+			array = g_byte_array_sized_new (strlen (tmp_string));
+			g_byte_array_append (array, (guint8 *) tmp_string, strlen (tmp_string));
+			goto done;
+		}
+	}
+	g_free (tmp_string);
+
+	/* Old format; list of ints */
+	tmp_list = g_key_file_get_integer_list (keyfile, setting_name, key, &length, NULL);
+	array = g_byte_array_sized_new (length);
+	for (i = 0; i < length; i++) {
+		int val = tmp_list[i];
+		unsigned char v = (unsigned char) (val & 0xFF);
+
+		if (val < 0 || val > 255) {
+			g_warning ("%s: %s / %s ignoring invalid byte element '%d' (not "
+			           " between 0 and 255 inclusive)", __func__, setting_name,
+			           key, val);
+		} else
+			g_byte_array_append (array, (const unsigned char *) &v, sizeof (v));
+	}
+	g_free (tmp_list);
+
+done:
+	if (array->len)
+		g_object_set (setting, key, array, NULL);
+	else {
+		g_warning ("%s: ignoring invalid SSID for %s / %s",
+		           __func__, setting_name, key);
+	}
+	g_byte_array_free (array, TRUE);
+}
 
 typedef struct {
 	const char *setting_name;
@@ -794,6 +857,10 @@ static KeyParser key_parsers[] = {
 	  NM_SETTING_BLUETOOTH_BDADDR,
 	  TRUE,
 	  mac_address_parser },
+	{ NM_SETTING_WIRELESS_SETTING_NAME,
+	  NM_SETTING_WIRELESS_SSID,
+	  TRUE,
+	  ssid_parser },
 	{ NULL, NULL, FALSE }
 };
 
diff --git a/system-settings/plugins/keyfile/tests/keyfiles/Makefile.am b/system-settings/plugins/keyfile/tests/keyfiles/Makefile.am
index 52252f9a61..d6ee018340 100644
--- a/system-settings/plugins/keyfile/tests/keyfiles/Makefile.am
+++ b/system-settings/plugins/keyfile/tests/keyfiles/Makefile.am
@@ -5,7 +5,8 @@ EXTRA_DIST = \
 	Test_Wired_Connection_MAC_Case \
 	Test_Wired_Connection_IP6 \
 	ATT_Data_Connect_BT \
-	ATT_Data_Connect_Plain
+	ATT_Data_Connect_Plain \
+	Test_String_SSID
 
 check-local:
 	@for f in $(EXTRA_DIST); do \
diff --git a/system-settings/plugins/keyfile/tests/keyfiles/Test_String_SSID b/system-settings/plugins/keyfile/tests/keyfiles/Test_String_SSID
new file mode 100644
index 0000000000..4a3b56d24f
--- /dev/null
+++ b/system-settings/plugins/keyfile/tests/keyfiles/Test_String_SSID
@@ -0,0 +1,11 @@
+[connection]
+id=Test 
+uuid=2f962388-e5f3-45af-a62c-ac220b8f7baa
+type=802-11-wireless
+
+[802-11-wireless]
+ssid=blah blah ssid 1234
+
+[ipv4]
+method=auto
+
diff --git a/system-settings/plugins/keyfile/tests/test-keyfile.c b/system-settings/plugins/keyfile/tests/test-keyfile.c
index 18daad1974..7bf63ef539 100644
--- a/system-settings/plugins/keyfile/tests/test-keyfile.c
+++ b/system-settings/plugins/keyfile/tests/test-keyfile.c
@@ -1296,6 +1296,149 @@ test_write_wireless_connection (void)
 	g_object_unref (connection);
 }
 
+#define TEST_STRING_SSID_FILE TEST_KEYFILES_DIR"/Test_String_SSID"
+
+static void
+test_read_string_ssid (void)
+{
+	NMConnection *connection;
+	NMSettingWireless *s_wireless;
+	GError *error = NULL;
+	const GByteArray *array;
+	const char *expected_ssid = "blah blah ssid 1234";
+
+	connection = connection_from_file (TEST_STRING_SSID_FILE, NULL);
+	ASSERT (connection != NULL,
+			"connection-read", "failed to read %s", TEST_STRING_SSID_FILE);
+
+	ASSERT (nm_connection_verify (connection, &error),
+	        "connection-verify", "failed to verify %s: %s", TEST_STRING_SSID_FILE, error->message);
+
+	/* ===== WIRELESS SETTING ===== */
+
+	s_wireless = NM_SETTING_WIRELESS (nm_connection_get_setting (connection, NM_TYPE_SETTING_WIRELESS));
+	ASSERT (s_wireless != NULL,
+	        "connection-verify-wireless", "failed to verify %s: missing %s setting",
+	        TEST_STRING_SSID_FILE,
+	        NM_SETTING_WIRELESS_SETTING_NAME);
+
+	/* SSID */
+	array = nm_setting_wireless_get_ssid (s_wireless);
+	ASSERT (array != NULL,
+	        "connection-verify-wireless", "failed to verify %s: missing %s / %s key",
+	        TEST_STRING_SSID_FILE,
+	        NM_SETTING_WIRELESS_SETTING_NAME,
+	        NM_SETTING_WIRELESS_SSID);
+	ASSERT (memcmp (array->data, expected_ssid, sizeof (expected_ssid)) == 0,
+	        "connection-verify-wireless", "failed to verify %s: unexpected %s / %s key value",
+	        TEST_STRING_SSID_FILE,
+	        NM_SETTING_WIRELESS_SETTING_NAME,
+	        NM_SETTING_WIRELESS_SSID);
+
+	g_object_unref (connection);
+}
+
+static void
+test_write_string_ssid (void)
+{
+	NMConnection *connection;
+	NMSettingConnection *s_con;
+	NMSettingWireless *s_wireless;
+	NMSettingIP4Config *s_ip4;
+	char *uuid, *testfile = NULL, *tmp;
+	GByteArray *ssid;
+	unsigned char tmpssid[] = { 65, 49, 50, 51, 32, 46, 92, 46, 36, 37, 126, 93 };
+	gboolean success;
+	NMConnection *reread;
+	GError *error = NULL;
+	pid_t owner_grp;
+	uid_t owner_uid;
+	GKeyFile *keyfile;
+
+	connection = nm_connection_new ();
+	ASSERT (connection != NULL,
+	        "connection-write", "failed to allocate new connection");
+
+	/* Connection setting */
+
+	s_con = NM_SETTING_CONNECTION (nm_setting_connection_new ());
+	ASSERT (s_con != NULL,
+	        "connection-write", "failed to allocate new %s setting",
+	        NM_SETTING_CONNECTION_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_con));
+
+	uuid = nm_utils_uuid_generate ();
+	g_object_set (s_con,
+	              NM_SETTING_CONNECTION_ID, "String SSID Test",
+	              NM_SETTING_CONNECTION_UUID, uuid,
+	              NM_SETTING_CONNECTION_TYPE, NM_SETTING_WIRELESS_SETTING_NAME,
+	              NULL);
+	g_free (uuid);
+
+	/* Wireless setting */
+
+	s_wireless = NM_SETTING_WIRELESS (nm_setting_wireless_new ());
+	ASSERT (s_wireless != NULL,
+			"connection-write", "failed to allocate new %s setting",
+			NM_SETTING_WIRELESS_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_wireless));
+
+	ssid = g_byte_array_sized_new (sizeof (tmpssid));
+	g_byte_array_append (ssid, &tmpssid[0], sizeof (tmpssid));
+	g_object_set (s_wireless, NM_SETTING_WIRELESS_SSID, ssid, NULL);
+	g_byte_array_free (ssid, TRUE);
+
+	/* IP4 setting */
+
+	s_ip4 = NM_SETTING_IP4_CONFIG (nm_setting_ip4_config_new ());
+	ASSERT (s_ip4 != NULL,
+			"connection-write", "failed to allocate new %s setting",
+			NM_SETTING_IP4_CONFIG_SETTING_NAME);
+	nm_connection_add_setting (connection, NM_SETTING (s_ip4));
+
+	g_object_set (s_ip4,
+	              NM_SETTING_IP4_CONFIG_METHOD, NM_SETTING_IP4_CONFIG_METHOD_AUTO,
+	              NULL);
+
+	/* Write out the connection */
+	owner_uid = geteuid ();
+	owner_grp = getegid ();
+	success = write_connection (connection, TEST_SCRATCH_DIR, owner_uid, owner_grp, &testfile, &error);
+	ASSERT (success == TRUE,
+			"connection-write", "failed to allocate write keyfile: %s",
+			error ? error->message : "(none)");
+
+	ASSERT (testfile != NULL,
+			"connection-write", "didn't get keyfile name back after writing connection");
+
+	/* Ensure the SSID was written out as a string */
+	keyfile = g_key_file_new ();
+	ASSERT (g_key_file_load_from_file (keyfile, testfile, 0, NULL) == TRUE,
+	        "string-ssid-verify", "failed to load keyfile to verify");
+	tmp = g_key_file_get_string (keyfile, NM_SETTING_WIRELESS_SETTING_NAME, NM_SETTING_WIRELESS_SSID, NULL);
+	ASSERT (tmp, "string-ssid-verify", "failed to load 'ssid' key from file");
+	ASSERT (strlen (tmp) == sizeof (tmpssid),
+	        "string-ssid-verify", "reread SSID and expected were different sizes");
+	ASSERT (memcmp (tmp, tmpssid, sizeof (tmpssid)) == 0,
+	        "string-ssid-verify", "reread SSID and expected were different");
+	g_free (tmp);
+	g_key_file_free (keyfile);
+
+	/* Read the connection back in and compare it to the one we just wrote out */
+	reread = connection_from_file (testfile, NULL);
+	ASSERT (reread != NULL, "connection-write", "failed to re-read test connection");
+
+	ASSERT (nm_connection_compare (connection, reread, NM_SETTING_COMPARE_FLAG_EXACT) == TRUE,
+			"connection-write", "written and re-read connection weren't the same");
+
+	g_clear_error (&error);
+	unlink (testfile);
+	g_free (testfile);
+
+	g_object_unref (reread);
+	g_object_unref (connection);
+}
+
 #define TEST_BT_DUN_FILE TEST_KEYFILES_DIR"/ATT_Data_Connect_BT"
 
 static void
@@ -1867,6 +2010,9 @@ int main (int argc, char **argv)
 	test_read_valid_wireless_connection ();
 	test_write_wireless_connection ();
 
+	test_read_string_ssid ();
+	test_write_string_ssid ();
+
 	test_read_bt_dun_connection ();
 	test_write_bt_dun_connection ();
 
diff --git a/system-settings/plugins/keyfile/writer.c b/system-settings/plugins/keyfile/writer.c
index 13dfb17936..fdba70c969 100644
--- a/system-settings/plugins/keyfile/writer.c
+++ b/system-settings/plugins/keyfile/writer.c
@@ -36,6 +36,7 @@
 #include <string.h>
 #include <arpa/inet.h>
 #include <netinet/ether.h>
+#include <ctype.h>
 
 #include "nm-dbus-glib-types.h"
 #include "writer.h"
@@ -453,6 +454,49 @@ write_hash_of_string (GKeyFile *file,
 	g_hash_table_foreach (hash, write_hash_of_string_helper, &info);
 }
 
+static void
+ssid_writer (GKeyFile *file,
+             NMSetting *setting,
+             const char *key,
+             const GValue *value)
+{
+	GByteArray *array;
+	const char *setting_name = nm_setting_get_name (setting);
+	gboolean new_format = TRUE;
+	int i, *tmp_array;
+	char *ssid;
+
+	g_return_if_fail (G_VALUE_HOLDS (value, DBUS_TYPE_G_UCHAR_ARRAY));
+
+	array = (GByteArray *) g_value_get_boxed (value);
+	if (!array || !array->len)
+		return;
+
+	/* Check whether each byte is printable.  If not, we have to use an
+	 * integer list, otherwise we can just use a string.
+	 */
+	for (i = 0; i < array->len; i++) {
+		char c = array->data[i] & 0xFF;
+		if (!isprint (c)) {
+			new_format = FALSE;
+			break;
+		}
+	}
+
+	if (new_format) {
+		ssid = g_malloc0 (array->len + 1);
+		memcpy (ssid, array->data, array->len);
+		g_key_file_set_string (file, setting_name, key, ssid);
+		g_free (ssid);
+	} else {
+		tmp_array = g_new (gint, array->len);
+		for (i = 0; i < array->len; i++)
+			tmp_array[i] = (int) array->data[i];
+		g_key_file_set_integer_list (file, setting_name, key, tmp_array, array->len);
+		g_free (tmp_array);
+	}
+}
+
 typedef struct {
 	const char *setting_name;
 	const char *key;
@@ -502,6 +546,9 @@ static KeyWriter key_writers[] = {
 	{ NM_SETTING_BLUETOOTH_SETTING_NAME,
 	  NM_SETTING_BLUETOOTH_BDADDR,
 	  mac_address_writer },
+	{ NM_SETTING_WIRELESS_SETTING_NAME,
+	  NM_SETTING_WIRELESS_SSID,
+	  ssid_writer },
 	{ NULL, NULL, NULL }
 };