Bug 35239

Created attachment 44376 [details] [review]
Filter out invalid user names

Filter out invalid user names as per the RFC. A client can handle this in its own UI too, but it is trivial have the check in Idle. Just in case. :-)

Created attachment 44377 [details] [review]
Handle ERROR messages

When an invalid user name is used the server responds with an ERROR message and closes the connection. Currently Idle only notices this when it tries to send another message and notices that the connection has been terminated.

This just tries to handle the ERROR message gracefully. However, I wonder if there is a way to send the specific reason why this happened to the client. Maybe we can add a new TpConnectionStatusReason -- TP_CONNECTION_STATUS_REASON_NAME_INVALID (or something similar)?

This has also been raised in https://bugs.freedesktop.org/30833

Review of attachment 44376 [details] [review]:

::: src/protocol.c
@@ +75,3 @@
+      const char ch = username[i];
+
+  g_assert (value);

Any reason not to use '\0', '\n', '\r', ' ' and '@' rather than their numeric forms here?

Review of attachment 44376 [details] [review]:

::: src/protocol.c
@@ +75,3 @@
+      const char ch = username[i];
+
+  g_assert (value);

Any reason not to use '\0', '\n', '\r', ' ' and '@' rather than their numeric forms here?

(In reply to comment #2)
> Currently Idle only notices this when it tries to send
> another message and notices that the connection has been terminated.

Huh, it doesn't notice the connection being closed by the other end? That sounds … not good.

Review of attachment 44377 [details] [review]:

This seems better than not handling ERROR at all.

::: src/idle-connection.c
@@ +857,3 @@
+	IdleConnection *conn = IDLE_CONNECTION(user_data);
+
+	connection_connect_cb(conn, FALSE, TP_CONNECTION_STATUS_REASON_NETWORK_ERROR);

It'd be best to have Idle emit the <http://telepathy.freedesktop.org/spec-snapshot/Connection.html#Signal:ConnectionError> signal, which would let us include more information about what went wrong. Specifically in this case, we could include the message the server sent us as the 'server-message'. We could also define a new D-Bus error name, "org.freedesktop.Telepathy.Error.InvalidParameter", and specify that when used in ConnectionError, a 'parameters' key in the details would map to a list of strings which are the named parameters which were rejected by the server.

This information ends up on the Account interface <http://telepathy.freedesktop.org/spec-snapshot/Account.html#Property:ConnectionError> and so Empathy's accounts dialog (for instance) could highlight the relevant fields in red.

Can 'ERROR' be recieved after we're happily connected? If so it strikes me that we might want to use different connection status reasons in different cases: Authentication_Failed if it's while we're connecting, and Network_Error if it's while we're already connected, for instance. (Also, does connection_connect_cb handle being called while the connection's open?)

(In reply to comment #3)
> Review of attachment 44376 [details] [review]:
> 
> ::: src/protocol.c
> @@ +75,3 @@
> +      const char ch = username[i];
> +
> +  g_assert (value);
> 
> Any reason not to use '\0', '\n', '\r', ' ' and '@' rather than their numeric
> forms here?

No. Changed to the character equivalents.

Created attachment 44697 [details] [review]
Filter out invalid user names

(In reply to comment #6)

> It'd be best to have Idle emit the
> <http://telepathy.freedesktop.org/spec-snapshot/Connection.html#Signal:ConnectionError>
> signal

Done.

Created attachment 44854 [details] [review]
Handle ERROR messages

Review of attachment 44697 [details] [review]:

Looks good.

Review of attachment 44854 [details] [review]:

::: src/idle-connection.c
@@ +858,3 @@
+	IdleConnection *conn = IDLE_CONNECTION(user_data);
+	GHashTable *details = NULL;
+	TpConnectionStatus status = conn->parent.status;

If status is already TP_CONNECTION_STATUS_DISCONNECTED, then this function should early-return. This will happen in the case where Disconnect() has already been called before the ERROR is received from the server. Otherwise, calling tp_base_connection_disconnect_with_dbus_error () on the connection—which already knows it's disconnected/ing—will trigger a warning from tp-glib.

@@ +875,3 @@
+		server_msg = g_new0(gchar, length + 1);
+		strncpy(server_msg, begin, length);
+	const gchar *end = strrchr(msg, ')');

I drew some boxes on some paper to check this code. :) I think this would be clearer and equivalent:

  begin++;
  end--;
  length = end - begin;
  server_msg = g_strndup (server_msg + begin, length);

@@ +876,3 @@
+		strncpy(server_msg, begin, length);
+		server_msg[length] = '\0';
+	const gchar *error_name = (status == TP_CONNECTION_STATUS_CONNECTING) ? TP_ERROR_STR_AUTHENTICATION_FAILED : TP_ERROR_STR_NETWORK_ERROR;

You should g_free (server_msg); here. ... ah, I see that you free it later. It would be clearer, I think, to define gchar *server_msg only within this block, and free it right after the call to tp_asv_new().

@@ +879,3 @@
+	}
+
+	gchar *server_msg = NULL;

As you noticed on IRC, the hash table needs to be unreffed if it's not NULL.

Comment on attachment 44697 [details] [review]
Filter out invalid user names

Committed as http://cgit.freedesktop.org/telepathy/telepathy-idle/commit/?id=418996606b95598d9392f1ec07d7115fac040580

Along with a simple smoke-test: http://cgit.freedesktop.org/telepathy/telepathy-idle/commit/?id=f246140fac18edc2975f85933d0203a797244ec4

(In reply to comment #12)

> Review of attachment 44854 [details] [review]:
> 
> ::: src/idle-connection.c
> @@ +858,3 @@
> +    IdleConnection *conn = IDLE_CONNECTION(user_data);
> +    GHashTable *details = NULL;
> +    TpConnectionStatus status = conn->parent.status;
> 
> If status is already TP_CONNECTION_STATUS_DISCONNECTED, then this function
> should early-return.

Done.

> @@ +875,3 @@
> +        server_msg = g_new0(gchar, length + 1);
> +        strncpy(server_msg, begin, length);
> +    const gchar *end = strrchr(msg, ')');
> 
> I drew some boxes on some paper to check this code. :) I think this would be
> clearer and equivalent:
> 
>   begin++;
>   end--;
>   length = end - begin;
>   server_msg = g_strndup (server_msg + begin, length);

Right. But the length should still be end - begin + 1. Else we will loose a character.

> @@ +876,3 @@
> +        strncpy(server_msg, begin, length);
> +        server_msg[length] = '\0';
> +    const gchar *error_name = (status == TP_CONNECTION_STATUS_CONNECTING) ?
> TP_ERROR_STR_AUTHENTICATION_FAILED : TP_ERROR_STR_NETWORK_ERROR;
> 
> You should g_free (server_msg); here. ... ah, I see that you free it later. It
> would be clearer, I think, to define gchar *server_msg only within this block,
> and free it right after the call to tp_asv_new().

Done.

> @@ +879,3 @@
> +    }
> +
> +    gchar *server_msg = NULL;
> 
> As you noticed on IRC, the hash table needs to be unreffed if it's not NULL.

Done.

Created attachment 44855 [details] [review]
Handle ERROR messages

Created attachment 44856 [details] [review]
Handle ERROR messages

Unref the hash table only when it is not NULL.

Created attachment 44857 [details] [review]
Handle ERROR messages

Using a case & a default because case, case & an empty default will cause the compiler to scream that reason and error_message might be used without initialization.

Created attachment 44858 [details] [review]
Handle ERROR messages

Put exit early if into the switch's default.

*** Bug 30833 has been marked as a duplicate of this bug. ***

Review of attachment 44858 [details] [review]:

Looks good!