GNOME Bugzilla – Bug 629261
Shouldn't allow jid without domain
Last modified: 2010-10-14 10:01:19 UTC
In bug #629209 an user managed to enter a JID without domain. Empathy shouldn't allow us to do that. <smcv> cassidy: the reporter shouldn't have been able to enter a domainless JID without Empathy complaining, surely? <smcv> cassidy: the UI is obviously a bit confusing if people think that overriding the server removes the need to have it in the JID box <smcv> cassidy: labelling it JID rather than Username in the XMPP case might be enough of a clue for people who've used other XMPP clients :-)
(I think Gabble will actually be interpreting { 'account': 'foo.bar' } as a JID with no node and domain 'foo.bar'!)
Created attachment 171481 [details] [review] proposed fix
Review of attachment 171481 [details] [review]: Thanks for the patch! Here is my review. You should really try to use git :) ::: empathy-2.31.92/libempathy/empathy-account-settings.c @@ +70,2 @@ GHashTable *parameters; + GHashTable *param_regexps; Please document the semantic of this hash table, the type of the keys and values and that they are owned. @@ +94,3 @@ + priv->param_regexps = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, (GDestroyNotify) tp_g_value_slice_free); You seem to always store strings as value. No point to use GValue. @@ +1341,3 @@ + } + + g_hash_table_iter_init (&iter, (GHashTable *)priv->param_regexps); Style is wrong. "make check" should catch that. @@ +1346,3 @@ + current_regex = g_value_get_string (v); + current_value = empathy_account_settings_get_string (settings, current); + is_valid &= g_regex_match_simple (current_regex, current_value, 0, 0); I'm not sure here is the best place to do the check. I think we should check one param at the time and UI should be able to dispay which param(s) is/are wrong.
*** Bug 631676 has been marked as a duplicate of this bug. ***
Bug #631676 is a similar bug about IRC nick names. It seems freenode refuses "__badger". Would be good to check what the IRC standard says.
Created attachment 172094 [details] [review] updated patch
>You should really try to use git :) Will do in next bugs. I updated the patch in accordance with your notes. Regarding #631676. The "__badger" nickname matches the IRC standard: http://tools.ietf.org/html/rfc2812 (section 2.3.1) The first character in a nick can be a letter or a special character. "_" is a special character. So this seems to be a restriction of this particular IRC server. I think it's not reasonable to set per-server rules.
Review of attachment 172094 [details] [review]: Cool, thanks! ::: empathy-2.32.0.1/libempathy/empathy-account-settings.c @@ +1309,3 @@ + +GList * +empathy_account_settings_get_invalid_parameters ( This API looks weird to me. I think we should have a gboolean empathy_account_settings_check_parameter (self, param_name), no need to use a list. We could call this function each time the entry of a parameter is changed. @@ +1353,3 @@ + if (!current_value) + continue; + if (!g_regex_match_simple (current_regex, current_value, 0, 0)) Wouldn't it be more effecient to store a GRegex * in the hash and use g_regex_match() ? @@ +1363,3 @@ +empathy_account_settings_is_valid (EmpathyAccountSettings *settings) +{ + return empathy_account_settings_get_invalid_parameters (settings) == NULL; You leak the returned list. ::: empathy-2.32.0.1/libempathy/empathy-account-settings.h @@ +177,3 @@ + const gchar *regex); + +GList *empathy_account_settings_get_invalid_parameters ( Should be "GList * empathy_" ::: empathy-2.32.0.1/libempathy-gtk/empathy-account-widget.c @@ +144,3 @@ +#define MSN_USER_NAME "(["ALPHADIGITDASH"_\\.]+)" +#define YAHOO_USER_NAME "(["ALPHA"]["ALPHADIGIT"_\\.]{3,31})" + It would be nice to have a comment linking to a reference of the protocol documenting the format accepted.
(In reply to comment #7) > I think it's not reasonable to set per-server rules. I agree.
(In reply to comment #9) > (In reply to comment #7) > > I think it's not reasonable to set per-server rules. > > I agree. How do you warn the user about it though ? Echo the server message ? "Network error" isn't descriptive enough for the user to fix it, he must go through the debug log to have a clue. It's unlikely that lambda user uses IRC anyway, but well.
Sorry about the double-post, but I just though maybe the dialog error could say something like "you might check the <link to the debug window>debug log</> for more info".
Ideally, telepathy-idle should raise a more useful error than NetworkError. Feel free to open a bug on bugs.freedesktop.org about that.
Created attachment 172265 [details] [review] lucky third
>Ideally, telepathy-idle should raise a more useful error than NetworkError. >Feel free to open a bug on bugs.freedesktop.org about that. https://bugs.freedesktop.org/show_bug.cgi?id=30833
Looks great! I merged it to master. Thanks a lot for this very good work :) This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.