[Freeciv-Dev] Re: (PR#9996) is_allowed_city_name and notifications

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9996 >

This patch fixes everything!

Rule for usernames:

- Only ascii names are allowed (see is_ascii_name).
- The name may not begin with a digit.
- Some reserved names aren't allowed.

Rule for player names:

- Any name from the ruleset is allowed.
- Otherwise only ascii names are allowed (see is_ascii_name).

Rule for city names:

- Any name from the ruleset is allowed.
- Otherwise only ascii names (is_ascii_name) are allowed.
- Other rules may be imposed depending on the cityname setting.


This fixes some bugs:

- Currently there is no check on usernames.
- There is a memory bug in the playername check (with real_name).

As well as solving future issues:

- We want to allow non-ascii names for players and cities in the ruleset.
- We don't want to allow players and cities to have just any name; it's 
too easy to make a garbage name.


jason


Index: client/gui-ftwl/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-ftwl/dialogs.c,v
retrieving revision 1.1
diff -u -r1.1 dialogs.c
--- client/gui-ftwl/dialogs.c   29 Jul 2004 14:10:13 -0000      1.1
+++ client/gui-ftwl/dialogs.c   9 Sep 2004 19:53:21 -0000
@@ -67,7 +67,7 @@
   int leader_count, leader=sw_list_get_selected_row(leaders_list);
   struct leader *leaders = get_nation_leaders(selected_nation, &leader_count);
 
-  if (!is_sane_name(leaders[leader].name)) {
+  if (strlen(leaders[leader].name) == 0) {
     append_output_window(_("You must type a legal name."));
     return;
   }
Index: client/gui-gtk/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/dialogs.c,v
retrieving revision 1.144
diff -u -r1.144 dialogs.c
--- client/gui-gtk/dialogs.c    13 Jul 2004 22:52:16 -0000      1.144
+++ client/gui-gtk/dialogs.c    9 Sep 2004 19:53:21 -0000
@@ -2347,7 +2347,7 @@
   s = gtk_entry_get_text(GTK_ENTRY(GTK_COMBO(leader_name)->entry));
 
   /* perform a minimum of sanity test on the name */
-  if (!is_sane_name(s)) {
+  if (strlen(s) == 0) {
     append_output_window(_("You must type a legal name."));
     return;
   }
Index: client/gui-gtk-2.0/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk-2.0/dialogs.c,v
retrieving revision 1.70
diff -u -r1.70 dialogs.c
--- client/gui-gtk-2.0/dialogs.c        13 Jul 2004 22:52:16 -0000      1.70
+++ client/gui-gtk-2.0/dialogs.c        9 Sep 2004 19:53:22 -0000
@@ -2164,7 +2164,8 @@
     s = gtk_entry_get_text(GTK_ENTRY(GTK_COMBO(races_leader)->entry));
 
     /* Perform a minimum of sanity test on the name. */
-    if (!is_sane_name(s)) {
+    /* This could call is_allowed_player_name if it were available. */
+    if (strlen(s) == 0) {
       append_output_window(_("You must type a legal name."));
       return;
     }
Index: client/gui-mui/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/dialogs.c,v
retrieving revision 1.56
diff -u -r1.56 dialogs.c
--- client/gui-mui/dialogs.c    13 Jul 2004 22:52:16 -0000      1.56
+++ client/gui-mui/dialogs.c    9 Sep 2004 19:53:22 -0000
@@ -1574,7 +1574,7 @@
 
   sz_strlcpy(packet.name, (char*)s);
   
-  if (!is_sane_name(packet.name)) {
+  if (strlen(packet.name) == 0) {
     append_output_window(_("You must type a legal name."));
     return;
   }
Index: client/gui-sdl/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-sdl/dialogs.c,v
retrieving revision 1.47
diff -u -r1.47 dialogs.c
--- client/gui-sdl/dialogs.c    13 Jul 2004 22:52:16 -0000      1.47
+++ client/gui-sdl/dialogs.c    9 Sep 2004 19:53:22 -0000
@@ -3799,7 +3799,7 @@
   char *pStr = convert_to_chars(pSetup->pName_Edit->string16->text);
 
   /* perform a minimum of sanity test on the name */
-  if (!is_sane_name(pStr)) {
+  if (strlen(pStr) == 0) {
     append_output_window(_("You must type a legal name."));
     pSellected_Widget = pStart_Button;
     set_wstate(pStart_Button, FC_WS_SELLECTED);
Index: client/gui-win32/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-win32/dialogs.c,v
retrieving revision 1.51
diff -u -r1.51 dialogs.c
--- client/gui-win32/dialogs.c  18 Jul 2004 04:50:23 -0000      1.51
+++ client/gui-win32/dialogs.c  9 Sep 2004 19:53:23 -0000
@@ -342,7 +342,7 @@
   ComboBox_GetText(GetDlgItem(hWnd,ID_RACESDLG_LEADER),
                   name,MAX_LEN_NAME);
  
-  if (!is_sane_name(name)) {
+  if (strlen(name) == 0) {
     append_output_window(_("You must type a legal name."));
     return;
   }
Index: client/gui-xaw/dialogs.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/dialogs.c,v
retrieving revision 1.99
diff -u -r1.99 dialogs.c
--- client/gui-xaw/dialogs.c    13 Jul 2004 22:52:17 -0000      1.99
+++ client/gui-xaw/dialogs.c    9 Sep 2004 19:53:24 -0000
@@ -2202,7 +2202,7 @@
   XtVaGetValues(races_leader, XtNstring, &dp, NULL);
 
   /* perform a minimum of sanity test on the name */
-  if (!is_sane_name(dp)) {
+  if (strlen(dp) == 0) {
     append_output_window(_("You must type a legal name."));
     return;
   }
Index: server/cityhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityhand.c,v
retrieving revision 1.135
diff -u -r1.135 cityhand.c
--- server/cityhand.c   6 Sep 2004 02:05:31 -0000       1.135
+++ server/cityhand.c   9 Sep 2004 19:53:25 -0000
@@ -371,17 +371,15 @@
 void handle_city_rename(struct player *pplayer, int city_id, char *name)
 {
   struct city *pcity = player_find_city_by_id(pplayer, city_id);
+  char message[1024];
 
   if (!pcity) {
     return;
   }
 
-  if (!is_sane_name(name)) {
-    notify_player(pplayer, _("Game: %s is not a valid name."), name);
-    return;
-  }
-
-  if (!is_allowed_city_name(pplayer, name, pcity->x, pcity->y, TRUE)) {
+  if (!is_allowed_city_name(pplayer, name, message, sizeof(message))) {
+    notify_player_ex(pplayer, pcity->x, pcity->y, E_NOEVENT,
+                    _("Game: %s"),  message);
     return;
   }
 
Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.271
diff -u -r1.271 citytools.c
--- server/citytools.c  31 Aug 2004 04:40:50 -0000      1.271
+++ server/citytools.c  9 Sep 2004 19:53:25 -0000
@@ -181,7 +181,7 @@
 
   for (choice = 0; city_names[choice].name; choice++) {
     if (!game_find_city_by_name(city_names[choice].name) &&
-       is_allowed_city_name(pplayer, city_names[choice].name, x, y, FALSE)) {
+       is_allowed_city_name(pplayer, city_names[choice].name, NULL, 0)) {
       int priority = evaluate_city_name_priority(x, y, &city_names[choice],
                                                 choice);
 
@@ -202,32 +202,43 @@
 1: a city name has to be unique to player
 2: a city name has to be globally unique
 3: a city name has to be globally unique, and players can't use names
-   that are in another player's default city names. (E.g. Swedish may not
+   that are in another player's default city names. (E.g., Swedish may not
    call new cities or rename old cities as Helsinki, because it's in
-   Finns' default city names)
+   Finns' default city names.  Duplicated names may be used by
+   either nation.)
 **************************************************************************/
 bool is_allowed_city_name(struct player *pplayer, const char *city_name,
-                         int x, int y, bool notify_player)
+                         char *error_buf, size_t bufsz)
 {
-  /* Mode 0: No restrictions to city names */
-  if (game.allowed_city_names == 0) {
-    return TRUE;
-  }
-
-  /* Mode 1: A city name has to be unique to player */
+  /* Mode 1: A city name has to be unique for each player. */
   if (game.allowed_city_names == 1 &&
       city_list_find_name(&pplayer->cities, city_name)) {
-    if (notify_player) {
-      notify_player_ex(pplayer, x, y, E_NOEVENT,
-                      _("Game: You already have a city called %s"),
-                      city_name);
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz, _("You already have a city called %s."),
+                 city_name);
     }
     return FALSE;
   }
 
+  /* Modes 2,3: A city name has to be globally unique. */
+  if ((game.allowed_city_names == 2 || game.allowed_city_names == 3)
+      && game_find_city_by_name(city_name)) {
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz,
+                 _("A city called %s already exists."), city_name);
+    }
+    return FALSE;
+  }
+
+  /* General rule: any name in our ruleset is allowed. */
+  if (is_default_city_name(city_name, pplayer)) {
+    return TRUE;
+  }
+
   /* 
-   * Mode 3: Check first that the proposed city name is not in another
-   * player's default city names.
+   * Mode 3: Check that the proposed city name is not in another
+   * player's default city names.  Note the name will already have been
+   * allowed if it is in this player's default city names list.
    */
   if (game.allowed_city_names == 3) {
     struct player *pother = NULL;
@@ -240,27 +251,25 @@
     } players_iterate_end;
 
     if (pother != NULL) {
-      if (notify_player) {
-       notify_player_ex(pplayer, x, y, E_NOEVENT,
-                        _("Game: Can't use %s as a city name. "
-                          "It is reserved for %s."),
-                        city_name, get_nation_name_plural(pother->nation));
+      if (error_buf) {
+       my_snprintf(error_buf, bufsz, _("Can't use %s as a city name. It is "
+                                       "reserved for %s."),
+                   city_name, get_nation_name_plural(pother->nation));
       }
       return FALSE;
     }
   }
 
-  /* Modes 2,3: A city name has to be globally unique */
-  if ((game.allowed_city_names == 2 ||
-       game.allowed_city_names == 3) && game_find_city_by_name(city_name)) {
-    if (notify_player) {
-      notify_player_ex(pplayer, x, y, E_NOEVENT,
-                      _("Game: A city called %s already exists."),
-                      city_name);
+  if (!is_ascii_name(city_name)) {
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz, _("%s is not a valid name. Only ASCII "
+                                     "names are allowed for cities."),
+                 city_name);
     }
     return FALSE;
   }
 
+
   return TRUE;
 }
 
Index: server/citytools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.h,v
retrieving revision 1.55
diff -u -r1.55 citytools.h
--- server/citytools.h  6 Aug 2004 16:46:24 -0000       1.55
+++ server/citytools.h  9 Sep 2004 19:53:25 -0000
@@ -77,7 +77,7 @@
                         int target, bool is_unit, enum event_type event);
 
 bool is_allowed_city_name(struct player *pplayer, const char *city_name,
-                         int x, int y, bool notify_player);
+                         char *error_buf, size_t bufsz);
 char *city_name_suggestion(struct player *pplayer, int x, int y);
 
 
Index: server/connecthand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/connecthand.c,v
retrieving revision 1.24
diff -u -r1.24 connecthand.c
--- server/connecthand.c        6 Sep 2004 15:53:21 -0000       1.24
+++ server/connecthand.c        9 Sep 2004 19:53:26 -0000
@@ -251,7 +251,9 @@
   remove_leading_trailing_spaces(req->username);
 
   /* Name-sanity check: could add more checks? */
-  if (strlen(req->username) == 0 || my_isdigit(req->username[0])
+  if (strlen(req->username) == 0
+      || my_isdigit(req->username[0])
+      || !is_ascii_name(req->username)
       || mystrcasecmp(req->username, "all") == 0
       || mystrcasecmp(req->username, "none") == 0
       || mystrcasecmp(req->username, ANON_USER_NAME) == 0) {
@@ -512,7 +514,7 @@
     return FALSE;
   }
 
-  if (!is_sane_name(password)) {
+  if (!is_ascii_name(password)) {
     return FALSE;
   }
 
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.187
diff -u -r1.187 srv_main.c
--- server/srv_main.c   6 Sep 2004 17:13:07 -0000       1.187
+++ server/srv_main.c   9 Sep 2004 19:53:26 -0000
@@ -1007,6 +1007,89 @@
 }
 
 /**************************************************************************
+  Checks if the player name belongs to the default player names of a
+  particular player.
+**************************************************************************/
+static bool is_default_nation_name(const char *name,
+                                  Nation_Type_id nation_id)
+{
+  const struct nation_type *nation = get_nation_by_idx(nation_id);
+
+  int choice;
+
+  for (choice = 0; choice < nation->leader_count; choice++) {
+    if (mystrcasecmp(name, nation->leaders[choice].name) == 0) {
+      return TRUE;
+    }
+  }
+
+  return FALSE;
+}
+
+/**************************************************************************
+  Check if this name is allowed for the player.  Fill out the error message
+  (a translated string to be sent to the client) if not.
+**************************************************************************/
+static bool is_allowed_player_name(struct player *pplayer,
+                                  Nation_Type_id nation,
+                                  const char *name,
+                                  char *error_buf, size_t bufsz)
+{
+  /* An empty name is surely not allowed. */
+  if (strlen(name) == 0) {
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz, _("Please choose a non-blank name."));
+    }
+    return FALSE;
+  }
+
+  /* Any name already taken is not allowed. */
+  players_iterate(other_player) {
+    if (other_player->nation == nation) {
+      if (error_buf) {
+       my_snprintf(error_buf, bufsz, _("That nation is already in use."));
+      }
+      return FALSE;
+    } else {
+      /* Check to see if name has been taken.
+       * Ignore case because matches elsewhere are case-insenstive.
+       * Don't limit this check to just players with allocated nation:
+       * otherwise could end up with same name as pre-created AI player
+       * (which have no nation yet, but will keep current player name).
+       * Also want to keep all player names strictly distinct at all
+       * times (for server commands etc), including during nation
+       * allocation phase.
+       */
+      if (other_player->player_no != pplayer->player_no
+         && mystrcasecmp(other_player->name, name) == 0) {
+       if (error_buf) {
+         my_snprintf(error_buf, bufsz,
+                     _("Another player already has the name '%s'.  Please "
+                       "choose another name."), name);
+       }
+       return FALSE;
+      }
+    }
+  } players_iterate_end;
+
+  /* Any name from the default list is always allowed. */
+  if (is_default_nation_name(name, nation)) {
+    return TRUE;
+  }
+
+  /* Otherwise only ascii names are allowed. */
+  if (!is_ascii_name(name)) {
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz, _("Please choose a name containing "
+                                     "only ASCII characters."));
+    }
+    return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**************************************************************************
 ...
 **************************************************************************/
 void handle_nation_select_req(struct player *pplayer,
@@ -1014,7 +1097,7 @@
                              char *name, int city_style)
 {
   int nation_used_count;
-  char real_name[MAX_LEN_NAME];
+  char message[1024];
 
   if (server_state != SELECT_RACES_STATE) {
     freelog(LOG_ERROR, _("Trying to alloc nation outside "
@@ -1031,44 +1114,15 @@
 
   remove_leading_trailing_spaces(name);
 
-  if (strlen(name)==0) {
-    notify_player(pplayer, _("Please choose a non-blank name."));
-    send_select_nation(pplayer);
-    return;
-  }
-
-  if (!is_sane_name(name)) {
-    notify_player(pplayer, _("Please choose a legal name."));
+  if (!is_allowed_player_name(pplayer, nation_no, name,
+                             message, sizeof(message))) {
+    notify_player(pplayer, "%s", message);
     send_select_nation(pplayer);
     return;
   }
 
   name[0] = my_toupper(name[0]);
 
-  players_iterate(other_player) {
-    if (other_player->nation == nation_no) {
-       send_select_nation(pplayer); /* it failed - nation taken */
-       return;
-    } else
-      /* Check to see if name has been taken.
-       * Ignore case because matches elsewhere are case-insenstive.
-       * Don't limit this check to just players with allocated nation:
-       * otherwise could end up with same name as pre-created AI player
-       * (which have no nation yet, but will keep current player name).
-       * Also want to keep all player names strictly distinct at all
-       * times (for server commands etc), including during nation
-       * allocation phase.
-       */
-      if (other_player->player_no != pplayer->player_no
-         && mystrcasecmp(other_player->name, real_name) == 0) {
-       notify_player(pplayer,
-                    _("Another player already has the name '%s'.  "
-                      "Please choose another name."), real_name);
-       send_select_nation(pplayer);
-       return;
-    }
-  } players_iterate_end;
-
   notify_conn_ex(&game.game_connections, -1, -1, E_NATION_SELECTED,
                 _("Game: %s is the %s ruler %s."), pplayer->username,
                 get_nation_name(nation_no), name);
Index: server/stdinhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/stdinhand.c,v
retrieving revision 1.345
diff -u -r1.345 stdinhand.c
--- server/stdinhand.c  7 Sep 2004 08:04:15 -0000       1.345
+++ server/stdinhand.c  9 Sep 2004 19:53:27 -0000
@@ -1981,8 +1981,9 @@
     goto cleanup;
   }
 
-  if (!is_sane_name(arg[1])) {
-    cmd_reply(CMD_TEAM, caller, C_SYNTAX, _("Bad team name."));
+  if (!is_ascii_name(arg[1])) {
+    cmd_reply(CMD_TEAM, caller, C_SYNTAX,
+             _("Only ASCII characters are allowed for team names."));
     goto cleanup;
   }
   if (is_barbarian(pplayer)) {
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.303
diff -u -r1.303 unithand.c
--- server/unithand.c   26 Aug 2004 18:37:52 -0000      1.303
+++ server/unithand.c   9 Sep 2004 19:53:27 -0000
@@ -497,14 +497,11 @@
 static void city_build(struct player *pplayer, struct unit *punit,
                       char *name)
 {
-  if (!is_sane_name(name)) {
-    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
-                    _("Game: Let's not build a city with "
-                      "such a stupid name."));
-    return;
-  }
+  char message[1024];
 
-  if (!is_allowed_city_name(pplayer, name, punit->x, punit->y, TRUE)) {
+  if (!is_allowed_city_name(pplayer, name, message, sizeof(message))) {
+    notify_player_ex(pplayer, punit->x, punit->y, E_NOEVENT,
+                    _("Game: %s"), message);
     return;
   }
 
Index: utility/shared.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/shared.c,v
retrieving revision 1.117
diff -u -r1.117 shared.c
--- utility/shared.c    9 Sep 2004 18:07:28 -0000       1.117
+++ utility/shared.c    9 Sep 2004 19:53:27 -0000
@@ -367,7 +367,7 @@
   Returns TRUE iff the name is acceptable.
   FIXME:  Not internationalised.
 ***************************************************************/
-bool is_sane_name(const char *name)
+bool is_ascii_name(const char *name)
 {
   const char illegal_chars[] = {'|', '%', '"', ',', '*', '\0'};
   int i, j;
@@ -731,7 +731,7 @@
 
     if (env) {
       sz_strlcpy(username, env);
-      if (is_sane_name(username)) {
+      if (is_ascii_name(username)) {
        freelog(LOG_VERBOSE, "USER username is %s", username);
        return username;
       }
@@ -746,7 +746,7 @@
 
     if (pwent) {
       sz_strlcpy(username, pwent->pw_name);
-      if (is_sane_name(username)) {
+      if (is_ascii_name(username)) {
        freelog(LOG_VERBOSE, "getpwuid username is %s", username);
        return username;
       }
@@ -762,7 +762,7 @@
 
     if (GetUserName(name, &length)) {
       sz_strlcpy(username, name);
-      if (is_sane_name(username)) {
+      if (is_ascii_name(username)) {
        freelog(LOG_VERBOSE, "GetUserName username is %s", username);
        return username;
       }
@@ -776,7 +776,7 @@
   my_snprintf(username, MAX_LEN_NAME, "name%d", (int)getuid());
 #endif
   freelog(LOG_VERBOSE, "fake username is %s", username);
-  assert(is_sane_name(username));
+  assert(is_ascii_name(username));
   return username;
 }
 
Index: utility/shared.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/shared.h,v
retrieving revision 1.132
diff -u -r1.132 shared.h
--- utility/shared.h    7 Sep 2004 08:04:16 -0000       1.132
+++ utility/shared.h    9 Sep 2004 19:53:27 -0000
@@ -182,7 +182,7 @@
 const char *big_int_to_text(unsigned int mantissa, unsigned int exponent);
 const char *int_to_text(unsigned int number);
 
-bool is_sane_name(const char *name);
+bool is_ascii_name(const char *name);
 const char *textyear(int year);
 int compare_strings(const void *first, const void *second);
 int compare_strings_ptrs(const void *first, const void *second);