Bug 399073: Remove the 'loginnetmask' parameter - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

10 files changed, 48 insertions, 179 deletions

diff --git a/Bugzilla/Auth/Login/Cookie.pm b/Bugzilla/Auth/Login/Cookie.pm
index e2cd8f5ee..0b002168e 100644
--- a/Bugzilla/Auth/Login/Cookie.pm
+++ b/Bugzilla/Auth/Login/Cookie.pm
@@ -36,7 +36,6 @@ sub get_login_info {
     my $dbh = Bugzilla->dbh;
 
     my $ip_addr      = $cgi->remote_addr();
-    my $net_addr     = get_netaddr($ip_addr);
     my $login_cookie = $cgi->cookie("Bugzilla_logincookie");
     my $user_id      = $cgi->cookie("Bugzilla_login");
 
@@ -60,24 +59,16 @@ sub get_login_info {
         trick_taint($login_cookie);
         detaint_natural($user_id);
 
-        my $query = "SELECT userid
-                       FROM logincookies
-                      WHERE logincookies.cookie = ?
-                            AND logincookies.userid = ?
-                            AND (logincookies.ipaddr = ?";
-
-        # If we have a network block that's allowed to use this cookie,
-        # as opposed to just a single IP.
-        my @params = ($login_cookie, $user_id, $ip_addr);
-        if (defined $net_addr) {
-            trick_taint($net_addr);
-            $query .= " OR logincookies.ipaddr = ?";
-            push(@params, $net_addr);
-        }
-        $query .= ")";
+        my $is_valid =
+          $dbh->selectrow_array('SELECT 1
+                                   FROM logincookies
+                                  WHERE cookie = ?
+                                        AND userid = ?
+                                        AND (ipaddr = ? OR ipaddr IS NULL)',
+                                 undef, ($login_cookie, $user_id, $ip_addr));
 
         # If the cookie is valid, return a valid username.
-        if ($dbh->selectrow_array($query, undef, @params)) {
+        if ($is_valid) {
             # If we logged in successfully, then update the lastused 
             # time on the login cookie
             $dbh->do("UPDATE logincookies SET lastused = NOW() 
diff --git a/Bugzilla/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm
index 60f90925e..4458e31b5 100644
--- a/Bugzilla/Auth/Persist/Cookie.pm
+++ b/Bugzilla/Auth/Persist/Cookie.pm
@@ -49,17 +49,14 @@ sub persist_login {
     my $dbh = Bugzilla->dbh;
     my $cgi = Bugzilla->cgi;
 
-    my $ip_addr = $cgi->remote_addr;
-    unless ($cgi->param('Bugzilla_restrictlogin') ||
-            Bugzilla->params->{'loginnetmask'} == 32) 
-    {
-        $ip_addr = get_netaddr($ip_addr);
+    my $ip_addr;
+    if ($cgi->param('Bugzilla_restrictlogin')) {
+        $ip_addr = $cgi->remote_addr;
+        # The IP address is valid, at least for comparing with itself in a
+        # subsequent login
+        trick_taint($ip_addr);
     }
 
-    # The IP address is valid, at least for comparing with itself in a
-    # subsequent login
-    trick_taint($ip_addr);
-
     $dbh->bz_start_transaction();
 
     my $login_cookie = 
diff --git a/Bugzilla/Config/Auth.pm b/Bugzilla/Config/Auth.pm
index cbd94617a..1af808eaa 100644
--- a/Bugzilla/Config/Auth.pm
+++ b/Bugzilla/Config/Auth.pm
@@ -91,13 +91,6 @@ sub get_param_list {
   },
 
   {
-   name => 'loginnetmask',
-   type => 't',
-   default => '0',
-   checker => \&check_netmask
-  },
-
-  {
    name => 'requirelogin',
    type => 'b',
    default => '0'
diff --git a/Bugzilla/Config/Common.pm b/Bugzilla/Config/Common.pm
index 90a5a6c76..b722795d4 100644
--- a/Bugzilla/Config/Common.pm
+++ b/Bugzilla/Config/Common.pm
@@ -47,7 +47,7 @@ use base qw(Exporter);
     qw(check_multi check_numeric check_regexp check_url check_group
        check_sslbase check_priority check_severity check_platform
        check_opsys check_shadowdb check_urlbase check_webdotbase
-       check_netmask check_user_verify_class
+       check_user_verify_class
        check_mail_delivery_method check_notification check_utf8
        check_bug_status check_smtp_auth check_theschwartz_available
        check_maxattachmentsize
@@ -248,21 +248,6 @@ sub check_webdotbase {
     return "";
 }
 
-sub check_netmask {
-    my ($mask) = @_;
-    my $res = check_numeric($mask);
-    return $res if $res;
-    if ($mask < 0 || $mask > 32) {
-        return "an IPv4 netmask must be between 0 and 32 bits";
-    }
-    # Note that if we changed the netmask from anything apart from 32, then
-    # existing logincookies which aren't for a single IP won't work
-    # any more. We can't know which ones they are, though, so they'll just
-    # take space until they're periodically cleared, later.
-
-    return "";
-}
-
 sub check_user_verify_class {
     # doeditparams traverses the list of params, and for each one it checks,
     # then updates. This means that if one param checker wants to look at 
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 2bd95d501..c5003f798 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -974,7 +974,7 @@ use constant ABSTRACT_SCHEMA => {
                          REFERENCES => {TABLE  => 'profiles',
                                         COLUMN => 'userid',
                                         DELETE => 'CASCADE'}},
-            ipaddr   => {TYPE => 'varchar(40)', NOTNULL => 1},
+            ipaddr   => {TYPE => 'varchar(40)'},
             lastused => {TYPE => 'DATETIME', NOTNULL => 1},
         ],
         INDEXES => [
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index e6b577526..df6296056 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -580,6 +580,9 @@ sub update_table_definitions {
     # 2009-09-28 LpSolit@gmail.com - Bug 519032
     $dbh->bz_drop_column('series', 'last_viewed');
 
+    # 2009-09-28 LpSolit@gmail.com - Bug 399073
+    _fix_logincookies_ipaddr();
+
     ################################################################
     # New --TABLE-- changes should go *** A B O V E *** this point #
     ################################################################
@@ -1249,7 +1252,7 @@ sub _use_ip_instead_of_hostname_in_logincookies {
         # Now update the logincookies schema
         $dbh->bz_drop_column("logincookies", "hostname");
         $dbh->bz_add_column("logincookies", "ipaddr",
-                            {TYPE => 'varchar(40)', NOTNULL => 1}, '');
+                            {TYPE => 'varchar(40)'});
     }
 }
 
@@ -3207,6 +3210,15 @@ sub _convert_disallownew_to_isactive {
     }
 }
 
+sub _fix_logincookies_ipaddr {
+    my $dbh = Bugzilla->dbh;
+    return if !$dbh->bz_column_info('logincookies', 'ipaddr')->{NOTNULL};
+
+    $dbh->bz_alter_column('logincookies', 'ipaddr', {TYPE => 'varchar(40)'});
+    $dbh->do('UPDATE logincookies SET ipaddr = NULL WHERE ipaddr = ?',
+             undef, '0.0.0.0');
+}
+
 1;
 
 __END__
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 90525b9d4..a36b22c37 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -35,7 +35,7 @@ use base qw(Exporter);
                              detaint_signed
                              html_quote url_quote xml_quote
                              css_class_quote html_light_quote url_decode
-                             i_am_cgi get_netaddr correct_urlbase
+                             i_am_cgi correct_urlbase
                              lsearch do_ssl_redirect_if_required use_attachbase
                              diff_arrays
                              trim wrap_hard wrap_comment find_wrap_point
@@ -601,28 +601,6 @@ sub get_text {
     return $message;
 }
 
-
-sub get_netaddr {
-    my $ipaddr = shift;
-
-    # Check for a valid IPv4 addr which we know how to parse
-    if (!$ipaddr || $ipaddr !~ /^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/) {
-        return undef;
-    }
-
-    my $addr = unpack("N", pack("CCCC", split(/\./, $ipaddr)));
-
-    my $maskbits = Bugzilla->params->{'loginnetmask'};
-
-    # Make Bugzilla ignore the IP address if loginnetmask is set to 0
-    return "0.0.0.0" if ($maskbits == 0);
-
-    $addr >>= (32-$maskbits);
-
-    $addr <<= (32-$maskbits);
-    return join(".", unpack("CCCC", pack("N", $addr)));
-}
-
 sub disable_utf8 {
     if (Bugzilla->params->{'utf8'}) {
         binmode STDOUT, ':bytes'; # Turn off UTF8 encoding.
@@ -657,7 +635,6 @@ Bugzilla::Util - Generic utility functions for bugzilla
 
   # Functions that tell you about your environment
   my $is_cgi   = i_am_cgi();
-  my $net_addr = get_netaddr($ip_addr);
   my $urlbase  = correct_urlbase();
 
   # Functions for searching
@@ -788,13 +765,6 @@ Tells you whether or not you are being run as a CGI script in a web
 server. For example, it would return false if the caller is running
 in a command-line script.
 
-=item C<get_netaddr($ipaddr)>
-
-Given an IP address, this returns the associated network address, using
-C<Bugzilla->params->{'loginnetmask'}> as the netmask. This can be used
-to obtain data in order to restrict weak authentication methods (such as
-cookies) to only some addresses.
-
 =item C<correct_urlbase()>
 
 Returns either the C<sslbase> or C<urlbase> parameter, depending on the
diff --git a/docs/en/xml/troubleshooting.xml b/docs/en/xml/troubleshooting.xml
index 7a4a08ef5..0c20b71d1 100644
--- a/docs/en/xml/troubleshooting.xml
+++ b/docs/en/xml/troubleshooting.xml
@@ -1,5 +1,5 @@
 <!-- <!DOCTYPE chapter PUBLIC "-//OASIS//DTD DocBook XML V4.1.2//EN"> -->
-<!-- $Id: troubleshooting.xml,v 1.11 2008/04/04 06:48:23 uid623 Exp $ -->
+<!-- $Id: troubleshooting.xml,v 1.14 2009/10/18 23:35:00 lpsolit%gmail.com Exp $ -->
 
 <appendix id="troubleshooting">
 <title>Troubleshooting</title>
@@ -22,7 +22,7 @@
     <para>If you have made it all the way through
     <xref linkend="installation"/> (Installation) and
     <xref linkend="configuration"/> (Configuration) but accessing the Bugzilla
-    URL doesn't work, the first thing to do is to check your webserver error
+    URL doesn't work, the first thing to do is to check your web server error
     log. For Apache, this is often located at
     <filename>/etc/logs/httpd/error_log</filename>. The error messages
     you see may be self-explanatory enough to enable you to diagnose and
@@ -32,7 +32,7 @@
 
     <para>
       Bugzilla can also log all user-based errors (and many code-based errors)
-      that occur, without polluting the web server error log.  To enable
+      that occur, without polluting the web server's error log.  To enable
       Bugzilla error logging, create a file that Bugzilla can write to, named
       <filename>errorlog</filename>, in the Bugzilla <filename>data</filename>
       directory.  Errors will be logged as they occur, and will include the type
@@ -45,10 +45,10 @@
   </section>
         
   <section id="trbl-testserver">
-  <title>The Apache webserver is not serving Bugzilla pages</title>
+  <title>The Apache web server is not serving Bugzilla pages</title>
     <para>After you have run <command>checksetup.pl</command> twice,
     run <command>testserver.pl http://yoursite.yourdomain/yoururl</command>
-    to confirm that your webserver is configured properly for
+    to confirm that your web server is configured properly for
     Bugzilla.
     </para>
     <programlisting>
@@ -75,9 +75,9 @@ TEST-OK Webserver is preventing fetch of http://landfill.bugzilla.org/bugzilla-t
         </para>
       </listitem>
       <listitem>
-	<para>The permissions on your library directories are set incorrectly.
-	They must, at the very least, be readable by the webserver user or
-	group. It is recommended that they be world readable.
+        <para>The permissions on your library directories are set incorrectly.
+        They must, at the very least, be readable by the web server user or
+        group. It is recommended that they be world readable.
         </para>
       </listitem>
     </orderedlist>
@@ -139,55 +139,12 @@ TEST-OK Webserver is preventing fetch of http://landfill.bugzilla.org/bugzilla-t
     </para>
   </section>    
 
-  <section id="trouble-filetemp">
-  <title>Your vendor has not defined Fcntl macro O_NOINHERIT</title>
-
-    <para>This is caused by a bug in the version of
-    <productname>File::Temp</productname> that is distributed with perl
-    5.6.0. Many minor variations of this error have been reported:
-    </para>
-
-    <programlisting>Your vendor has not defined Fcntl macro O_NOINHERIT, used 
-at /usr/lib/perl5/site_perl/5.6.0/File/Temp.pm line 208.
-
-Your vendor has not defined Fcntl macro O_EXLOCK, used 
-at /usr/lib/perl5/site_perl/5.6.0/File/Temp.pm line 210.
-
-Your vendor has not defined Fcntl macro O_TEMPORARY, used 
-at /usr/lib/perl5/site_perl/5.6.0/File/Temp.pm line 233.</programlisting>
-
-    <para>Numerous people have reported that upgrading to version 5.6.1
-    or higher solved the problem for them. A less involved fix is to apply
-    the following patch, which is also
-    available as a <ulink url="../xml/filetemp.patch">patch file</ulink>.
-    </para>
-
-    <programlisting><![CDATA[--- File/Temp.pm.orig   Thu Feb  6 16:26:00 2003
-+++ File/Temp.pm        Thu Feb  6 16:26:23 2003
-@@ -205,6 +205,7 @@
-     # eg CGI::Carp
-     local $SIG{__DIE__} = sub {};
-     local $SIG{__WARN__} = sub {};
-+    local *CORE::GLOBAL::die = sub {};
-     $bit = &$func();
-     1;
-   };
-@@ -226,6 +227,7 @@
-     # eg CGI::Carp
-     local $SIG{__DIE__} = sub {};
-     local $SIG{__WARN__} = sub {};
-+    local *CORE::GLOBAL::die = sub {};
-     $bit = &$func();
-     1;
-   };]]></programlisting>
-  </section>
-
   <section id="trbl-relogin-everyone">
   <title>Everybody is constantly being forced to relogin</title>
   
   <para>The most-likely cause is that the <quote>cookiepath</quote> parameter
   is not set correctly in the Bugzilla configuration.  You can change this (if
-  you're a Bugzilla administrator) from the editparams.cgi page via the web.
+  you're a Bugzilla administrator) from the editparams.cgi page via the web interface.
   </para>
 
   <para>The value of the cookiepath parameter should be the actual directory
@@ -256,35 +213,6 @@ at /usr/lib/perl5/site_perl/5.6.0/File/Temp.pm line 233.</programlisting>
     </para>
   </section>
 
-  <section id="trbl-relogin-some">
-  <title>Some users are constantly being forced to relogin</title>
-
-    <para>First, make sure cookies are enabled in the user's browser.
-    </para>
-
-    <para>If that doesn't fix the problem, it may be that the user's ISP
-     implements a rotating proxy server. This causes the user's effective IP
-     address (the address which the Bugzilla server perceives him coming from)
-     to change periodically. Since Bugzilla cookies are tied to a specific IP
-     address, each time the effective address changes, the user will have to
-     log in again.
-     </para>
-
-     <para>If you are using 2.18 (or later), there is a
-     parameter called <quote>loginnetmask</quote>, which you can use to set
-     the number of bits of the user's IP address to require to be matched when
-     authenticating the cookies. If you set this to something less than 32,
-     then the user will be given a checkbox for <quote>Restrict this login to
-     my IP address</quote> on the login screen, which defaults to checked. If
-     they leave the box checked, Bugzilla will behave the same as it did
-     before, requiring an exact match on their IP address to remain logged in.
-     If they uncheck the box, then only the left side of their IP address (up
-     to the number of bits you specified in the parameter) has to match to
-     remain logged in.
-     </para>
-
-   </section>
-
   <section id="trbl-index">
   <title><filename>index.cgi</filename> doesn't show up unless specified in the URL</title>
     <para>
diff --git a/template/en/default/account/auth/login.html.tmpl b/template/en/default/account/auth/login.html.tmpl
index e4adfdcb6..9a043e4f4 100644
--- a/template/en/default/account/auth/login.html.tmpl
+++ b/template/en/default/account/auth/login.html.tmpl
@@ -69,17 +69,15 @@
       </tr>
     [% END %]
 
-    [% IF Param('loginnetmask') < 32 %]
-      <tr>
-        <th>&nbsp;</th>
-        <td>
-          <input type="checkbox" id="Bugzilla_restrictlogin" name="Bugzilla_restrictlogin"
-                 checked="checked">
-          <label for="Bugzilla_restrictlogin">Restrict this session to this IP address
-          (using this option improves security)</label>
-        </td>
-      </tr>
-    [% END %]
+    <tr>
+      <th>&nbsp;</th>
+      <td>
+        <input type="checkbox" id="Bugzilla_restrictlogin" name="Bugzilla_restrictlogin"
+               checked="checked">
+        <label for="Bugzilla_restrictlogin">Restrict this session to this IP address
+        (using this option improves security)</label>
+      </td>
+    </tr>
   </table>
 
   [% PROCESS "global/hidden-fields.html.tmpl"
diff --git a/template/en/default/admin/params/auth.html.tmpl b/template/en/default/admin/params/auth.html.tmpl
index 94a748b25..d2cb3e5b5 100644
--- a/template/en/default/admin/params/auth.html.tmpl
+++ b/template/en/default/admin/params/auth.html.tmpl
@@ -103,11 +103,6 @@
                       </li>
                     </ul>",
 
-  loginnetmask => "The number of bits for the netmask used if a user chooses to " _
-                  "allow a login to be valid for more than a single IP. Setting " _
-                  "this to 32 disables this feature.<br> " _
-                  "Note that enabling this may decrease the security of your system.",
-
   requirelogin => "If this option is set, all access to the system beyond the " _
                   "front page will require a login. No anonymous users will " _
                   "be permitted.",