Re: [evolution-patches] seeking review for patch for bug 55102

[Calvin Liu <calvin liu sun com>]

Hi, Jeff,

I renew the patch according to your comments. Please review it. Thanks.

Calvin

On Thu, 2004-03-04 at 03:26, Jeffrey Stedfast wrote:
> looks mostly ok, but a few comments below (and PLEASE, send patches with
> Content-Disposition: inline so it is easier to reply to them and comment
> on the patch)
> 
> On Wed, 2004-03-03 at 03:54, Calvin Liu wrote:
> > Hi, there,
> > 
> > This bug will affect all platforms. If the mail account name contains
> > special characters like "#", user can't access that account.
> > 
> > The patch is coming from the code of shell folder name validation.
> > Please review it.
> > 
> > Thanks.
> > Calvin
> 
> Index: mail/ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
> retrieving revision 1.3142
> diff -u -r1.3142 ChangeLog
> --- mail/ChangeLog      3 Mar 2004 08:11:15 -0000       1.3142
> +++ mail/ChangeLog      3 Mar 2004 09:01:38 -0000
> @@ -1,3 +1,9 @@
> +2004-03-03  Calvin Liu <calvin liu sun com>
> +       * mail-account-gui.c
> +       add mail_account_name_is_valid () and change
> mail_account_gui_save
> +       to check if account name is valid before save it.
> +       fixes bug 55102
> +
>  2004-03-03  Not Zed  <NotZed Ximian com>
>  
>         ** See bug #54924.
> 
> Index: mail/mail-account-gui.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/mail-account-gui.c,v
> retrieving revision 1.148
> diff -u -r1.148 mail-account-gui.c
> --- mail/mail-account-gui.c     24 Feb 2004 21:31:19 -0000      1.148
> +++ mail/mail-account-gui.c     3 Mar 2004 08:58:01 -0000
> @@ -1949,6 +1949,7 @@
>         const char *new_name;
>         gboolean is_storage;
>         GSList *signatures;
> +       const char *reason = NULL;
> 
> 
> probably doesn't need to be set to NULL here, but ok...
> 
>         
>         if (!mail_account_gui_identity_complete (gui, NULL) ||
>             !mail_account_gui_source_complete (gui, NULL) ||
> @@ -1963,6 +1964,12 @@
>          * here. */
>         
>         new_name = gtk_entry_get_text (gui->account_name);
> +        if (! mail_account_name_is_valid (new_name, &reason)){
> 
> remove the space after the ! and add a space before the {
> (to be consistent with the rest of the code formatting)
> 
> +                e_notice (gui->account_name, GTK_MESSAGE_ERROR,
> +                                  _("The specified account name is not
> valid: %s"), reason);
> 
> looks like you used spaces here rather than tab to indent the wrapped
> line (use tab)
> 
> 
> +                return FALSE;
> +        }
> +
>         account = mail_config_get_account_by_name (new_name);
>         
>         if (account && account != new) {
> @@ -2092,3 +2099,43 @@
>         g_free (gui->sent_folder_uri);
>         g_free (gui);
>  }
> +
> +gboolean
> +mail_account_name_is_valid (const char *name,
> +                              const char **reason_return)
> 
> 
> don't bother wrapping this line. this is the age of 1024x768
> resolutions, we don't need to wrap at 80 columns :-)
> 
> also, "reason_return" is inconsistent naming with the way we do this
> everywhere else in the code. Just call it "reason".
> 
> 
> +{
> +        if (name == NULL || *name == '\0') {
> +                if (reason_return != NULL)
> 
> since this function is only ever called from one place which always
> passes in &reason, do we really need this check? I don't think we do...
> 
> +                        *reason_return = _("No account name
> specified.");
> +                return FALSE;
> +        }
> +                                                                                                                                         
> 
> instead of doing the following check, what I would do is check for \r
> above the empty-name check and simply remove \r's. No need to trouble
> the user about characters he clearly didn't mean to have as part of his
> account name.
> 
> +        /* GtkEntry is broken - if you hit KP_ENTER you get a \r
> inserted... */
> +        if (strchr (name, '\r')) {
> +                if (reason_return != NULL)
> +                        *reason_return = _("Account name cannot contain
> the Return character.");
> +                return FALSE;
> +        }
> +                                                                                                                                         
> +        if (strchr (name, '/') != NULL) {
> +                if (reason_return != NULL)
> +                        *reason_return = _("Account name cannot contain
> the character \"/\".");
> +                return FALSE;
> +        }
> +                                                                                                                                         
> +        if (strchr (name, '#') != NULL) {
> +                if (reason_return != NULL)
> +                        *reason_return = _("Account name cannot contain
> the character \"#\".");
> +                return FALSE;
> +        }
> +                                                                                                                                         
> +        if (strcmp (name, ".") == 0 || strcmp (name, "..") == 0) {
> +                if (reason_return != NULL)
> +                        *reason_return = _("Account name cannot contain
> the character \".\".");
> +                return FALSE;
> +        }
> +                                                                                                                                         
> +        *reason_return = NULL;
> 
> hehe, you went thru all the trouble of checking that reason_return !=
> NULL in the error conditions above and didn't bother checking it here?
> Shame on you :-)
> 
> also, get rid of this. it is unneeded (reason_return only gets used if
> this function returns FALSE, so no need to ever set it if we return
> TRUE).
> 
> +                                                                                                                                         
> +        return TRUE;
> +}
> 

Index: mail/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
retrieving revision 1.3142
diff -u -r1.3142 ChangeLog
--- mail/ChangeLog	3 Mar 2004 08:11:15 -0000	1.3142
+++ mail/ChangeLog	3 Mar 2004 09:01:38 -0000
@@ -1,3 +1,9 @@
+2004-03-03  Calvin Liu <calvin liu sun com>
+	* mail-account-gui.c
+	add mail_account_name_is_valid () and change mail_account_gui_save
+	to check if account name is valid before save it.
+	fixes bug 55102
+
 2004-03-03  Not Zed  <NotZed Ximian com>
 
 	** See bug #54924.

Index: mail/mail-account-gui.c
===================================================================
RCS file: /cvs/gnome/evolution/mail/mail-account-gui.c,v
retrieving revision 1.148
diff -u -r1.148 mail-account-gui.c
--- mail/mail-account-gui.c	24 Feb 2004 21:31:19 -0000	1.148
+++ mail/mail-account-gui.c	3 Mar 2004 08:58:01 -0000
@@ -1949,6 +1949,7 @@
 	const char *new_name;
 	gboolean is_storage;
 	GSList *signatures;
+	const char *reason;
 	
 	if (!mail_account_gui_identity_complete (gui, NULL) ||
 	    !mail_account_gui_source_complete (gui, NULL) ||
@@ -1963,6 +1964,12 @@
 	 * here. */
 	
 	new_name = gtk_entry_get_text (gui->account_name);
+	if (!mail_account_name_is_valid (new_name, &reason)) {
+		e_notice (gui->account_name, GTK_MESSAGE_ERROR,
+			_("The specified account name is not valid: %s"), reason);
+		return FALSE;
+	}
+
 	account = mail_config_get_account_by_name (new_name);
 	
 	if (account && account != new) {
@@ -2092,3 +2099,43 @@
 	g_free (gui->sent_folder_uri);
 	g_free (gui);
 }
+
+gboolean
+mail_account_name_is_valid (const char *name, const char **reason)
+{
+	if (name == NULL || *name == '\0') {
+		*reason = _("No account name specified.");
+		return FALSE;
+	}
+	
+	/* GtkEntry is broken - if you hit KP_ENTER you get a \r inserted... */
+	if (strchr (name, '\r')) {
+		*reason = _("Account name cannot contain the Return character.");
+		return FALSE;
+	}
+
+	if (strchr (name, '/') != NULL) {
+		*reason = _("Account name cannot contain the character \"/\".");
+		return FALSE;
+	}
+
+	if (strchr (name, '#') != NULL) {
+		*reason = _("Account name cannot contain the character \"#\".");
+		return FALSE;
+	}
+
+	if (strcmp (name, ".") == 0 || strcmp (name, "..") == 0) {
+		*reason = _("Account name cannot contain the character \".\".");
+		return FALSE;
+	}
+
+	return TRUE;
+}