Bug 514913: Eliminate ssl="authenticated sessions"

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=dkl, a=mkanat

15 files changed, 74 insertions, 153 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index 43a9b39ae..62b1af659 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -85,7 +85,6 @@ use constant SHUTDOWNHTML_EXIT_SILENTLY => [
 sub init_page {
     (binmode STDOUT, ':utf8') if Bugzilla->params->{'utf8'};
 
-
     if (${^TAINT}) {
         # Some environment variables are not taint safe
         delete @::ENV{'PATH', 'IFS', 'CDPATH', 'ENV', 'BASH_ENV'};
@@ -94,6 +93,12 @@ sub init_page {
         $ENV{'PATH'} = '';
     }
 
+    # Because this function is run live from perl "use" commands of
+    # other scripts, we're skipping the rest of this function if we get here
+    # during a perl syntax check (perl -c, like we do during the
+    # 001compile.t test).
+    return if $^C;
+
     # IIS prints out warnings to the webpage, so ignore them, or log them
     # to a file if the file exists.
     if ($ENV{SERVER_SOFTWARE} && $ENV{SERVER_SOFTWARE} =~ /microsoft-iis/i) {
@@ -108,18 +113,15 @@ sub init_page {
         };
     }
 
+    do_ssl_redirect_if_required();
+
     # If Bugzilla is shut down, do not allow anything to run, just display a
     # message to the user about the downtime and log out.  Scripts listed in 
     # SHUTDOWNHTML_EXEMPT are exempt from this message.
     #
-    # Because this is code which is run live from perl "use" commands of other
-    # scripts, we're skipping this part if we get here during a perl syntax 
-    # check -- runtests.pl compiles scripts without running them, so we 
-    # need to make sure that this check doesn't apply to 'perl -c' calls.
-    #
     # This code must go here. It cannot go anywhere in Bugzilla::CGI, because
     # it uses Template, and that causes various dependency loops.
-    if (!$^C && Bugzilla->params->{"shutdownhtml"} 
+    if (Bugzilla->params->{"shutdownhtml"} 
         && lsearch(SHUTDOWNHTML_EXEMPT, basename($0)) == -1)
     {
         # Allow non-cgi scripts to exit silently (without displaying any
@@ -318,14 +320,6 @@ sub login {
         $class->set_user($authenticated_user);
     }
 
-    # We run after the login has completed since
-    # some of the checks in ssl_require_redirect
-    # look for Bugzilla->user->id to determine 
-    # if redirection is required.
-    if (i_am_cgi() && ssl_require_redirect()) {
-        $class->cgi->require_https($class->params->{'sslbase'});
-    }
-    
     return $class->user;
 }
 
diff --git a/Bugzilla/Auth/Login/CGI.pm b/Bugzilla/Auth/Login/CGI.pm
index 5be98aa7a..a93bc3d3a 100644
--- a/Bugzilla/Auth/Login/CGI.pm
+++ b/Bugzilla/Auth/Login/CGI.pm
@@ -65,17 +65,6 @@ sub fail_nodata {
             ->faultstring('Login Required');
     }
 
-    # If system is not configured to never require SSL connections
-    # we want to always redirect to SSL since passing usernames and
-    # passwords over an unprotected connection is a bad idea. If we 
-    # get here then a login form will be provided to the user so we
-    # want this to be protected if possible.
-    if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
-        && Bugzilla->params->{'ssl'} ne 'never')
-    {
-        $cgi->require_https(Bugzilla->params->{'sslbase'});
-    }
-
     print $cgi->header();
     $template->process("account/auth/login.html.tmpl",
                        { 'target' => $cgi->url(-relative=>1) }) 
diff --git a/Bugzilla/Auth/Persist/Cookie.pm b/Bugzilla/Auth/Persist/Cookie.pm
index c533252d3..60f90925e 100644
--- a/Bugzilla/Auth/Persist/Cookie.pm
+++ b/Bugzilla/Auth/Persist/Cookie.pm
@@ -89,11 +89,9 @@ sub persist_login {
         # Not a session cookie, so set an infinite expiry
         $cookieargs{'-expires'} = 'Fri, 01-Jan-2038 00:00:00 GMT';
     }
-    if (Bugzilla->params->{'ssl'} ne 'never'
-        && Bugzilla->params->{'sslbase'} ne '')
-    {
-        # Bugzilla->login will automatically redirect to https://,
-        # so it's safe to turn on the 'secure' bit.
+    if (Bugzilla->params->{'ssl_redirect'}) {
+        # Make these cookies only be sent to us by the browser during 
+        # HTTPS sessions, if we're using SSL.
         $cookieargs{'-secure'} = 1;
     }
 
diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm
index a00d632c3..c30e13618 100644
--- a/Bugzilla/CGI.pm
+++ b/Bugzilla/CGI.pm
@@ -368,22 +368,23 @@ sub remove_cookie {
                        '-value'   => 'X');
 }
 
-# Redirect to https if required
-sub require_https {
-    my ($self, $url) = @_;
-    # Do not create query string if data submitted via XMLRPC
-    # since we want the data to be resubmitted over POST method.
-    my $query = Bugzilla->usage_mode == USAGE_MODE_XMLRPC ? 0 : 1;
-    # XMLRPC clients (SOAP::Lite at least) requires 301 to redirect properly
-    # and do not work with 302.
-    my $status = Bugzilla->usage_mode == USAGE_MODE_XMLRPC ? 301 : 302;
-    if (defined $url) {
-        $url .= $self->url('-path_info' => 1, '-query' => $query, '-relative' => 1);
-    } else {
-        $url = $self->self_url;
-        $url =~ s/^http:/https:/i;
-    }
-    print $self->redirect(-location => $url, -status => $status);
+sub redirect_to_https {
+    my $self = shift;
+    my $sslbase = Bugzilla->params->{'sslbase'};
+    # If this is a POST, we don't want ?POSTDATA in the query string.
+    # We expect the client to re-POST, which may be a violation of
+    # the HTTP spec, but the only time we're expecting it often is
+    # in the WebService, and WebService clients usually handle this
+    # correctly.
+    $self->delete('POSTDATA');
+    my $url = $sslbase . $self->url('-path_info' => 1, '-query' => 1, 
+                                    '-relative' => 1);
+
+    # XML-RPC clients (SOAP::Lite at least) require a 301 to redirect properly
+    # and do not work with 302. Our redirect really is permanent anyhow, so
+    # it doesn't hurt to make it a 301.
+    print $self->redirect(-location => $url, -status => 301);
+
     # When using XML-RPC with mod_perl, we need the headers sent immediately.
     $self->r->rflush if $ENV{MOD_PERL};
     exit;
@@ -459,13 +460,13 @@ effectively removing the cookie.
 
 As its only argument, it takes the name of the cookie to expire.
 
-=item C<require_https($baseurl)>
+=item C<redirect_to_https>
 
-This routine redirects the client to a different location using the https protocol. 
-If the client is using XMLRPC, it will not retain the QUERY_STRING since XMLRPC uses POST.
+This routine redirects the client to the https version of the page that
+they're looking at, using the C<sslbase> parameter for the redirection.
 
-It takes an optional argument which will be used as the base URL.  If $baseurl
-is not provided, the current URL is used.
+Generally you should use L<Bugzilla::Util/do_ssl_redirect_if_required>
+instead of calling this directly.
 
 =item C<redirect_to_urlbase>
 
diff --git a/Bugzilla/Config.pm b/Bugzilla/Config.pm
index a20751737..f56ffd78a 100644
--- a/Bugzilla/Config.pm
+++ b/Bugzilla/Config.pm
@@ -192,6 +192,11 @@ sub update_params {
         $param->{'mail_delivery_method'} = $translation{$method};
     }
 
+    # Convert the old "ssl" parameter to the new "ssl_redirect" parameter.
+    # Both "authenticated sessions" and "always" turn on "ssl_redirect"
+    # when upgrading.
+    $param->{'ssl_redirect'} = 1 if $param->{'ssl'} ne 'never';
+
     # --- DEFAULTS FOR NEW PARAMS ---
 
     _load_params unless %params;
diff --git a/Bugzilla/Config/Core.pm b/Bugzilla/Config/Core.pm
index 6d413b965..91426b30a 100644
--- a/Bugzilla/Config/Core.pm
+++ b/Bugzilla/Config/Core.pm
@@ -68,10 +68,9 @@ sub get_param_list {
   },
 
   {
-   name => 'ssl',
-   type => 's',
-   choices => ['never', 'authenticated sessions', 'always'],
-   default => 'never'
+   name => 'ssl_redirect',
+   type => 'b',
+   default => 0
   },
 
 
diff --git a/Bugzilla/Mailer.pm b/Bugzilla/Mailer.pm
index 610523b8a..83ae5a600 100644
--- a/Bugzilla/Mailer.pm
+++ b/Bugzilla/Mailer.pm
@@ -82,10 +82,7 @@ sub MessageToMTA {
     #
     # We don't use correct_urlbase, because we want this URL to
     # *always* be the same for this Bugzilla, in every email,
-    # and some emails we send when we're logged out (in which case
-    # some emails might get urlbase while the logged-in emails might 
-    # get sslbase). Also, we want this to stay the same even if
-    # the admin changes the "ssl" parameter.
+    # even if the admin changes the "ssl_redirect" parameter some day.
     $email->header_set('X-Bugzilla-URL', Bugzilla->params->{'urlbase'});
     
     # We add this header to mark the mail as "auto-generated" and
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index 55ec6dcf8..90525b9d4 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -36,7 +36,7 @@ use base qw(Exporter);
                              html_quote url_quote xml_quote
                              css_class_quote html_light_quote url_decode
                              i_am_cgi get_netaddr correct_urlbase
-                             lsearch ssl_require_redirect use_attachbase
+                             lsearch do_ssl_redirect_if_required use_attachbase
                              diff_arrays
                              trim wrap_hard wrap_comment find_wrap_point
                              format_time format_time_decimal validate_date
@@ -264,60 +264,28 @@ sub i_am_cgi {
     return exists $ENV{'SERVER_SOFTWARE'} ? 1 : 0;
 }
 
-sub ssl_require_redirect {
-    my $method = shift;
-
-    # If currently not in a protected SSL 
-    # connection, determine if a redirection is 
-    # needed based on value in Bugzilla->params->{ssl}.
-    # If we are already in a protected connection or
-    # sslbase is not set then no action is required.
-    if (uc($ENV{'HTTPS'}) ne 'ON' 
-        && $ENV{'SERVER_PORT'} != 443 
-        && Bugzilla->params->{'sslbase'} ne '')
-    {
-        # System is configured to never require SSL 
-        # so no redirection is needed.
-        return 0 
-            if Bugzilla->params->{'ssl'} eq 'never';
-            
-        # System is configured to always require a SSL
-        # connection so we need to redirect.
-        return 1
-            if Bugzilla->params->{'ssl'} eq 'always';
-
-        # System is configured such that if we are inside
-        # of an authenticated session, then we need to make
-        # sure that all of the connections are over SSL. Non
-        # authenticated sessions SSL is not mandatory.
-        # For XMLRPC requests, if the method is User.login
-        # then we always want the connection to be over SSL
-        # if the system is configured for authenticated
-        # sessions since the user's username and password
-        # will be passed before the user is logged in.
-        return 1 
-            if Bugzilla->params->{'ssl'} eq 'authenticated sessions'
-                && (Bugzilla->user->id 
-                    || (defined $method && $method eq 'User.login'));
-    }
+# This exists as a separate function from Bugzilla::CGI::redirect_to_https
+# because we don't want to create a CGI object during XML-RPC calls
+# (doing so can mess up XML-RPC).
+sub do_ssl_redirect_if_required {
+    return if !i_am_cgi();
+    return if !Bugzilla->params->{'ssl_redirect'};
 
-    return 0;
+    my $sslbase = Bugzilla->params->{'sslbase'};
+    
+    # If we're already running under SSL, never redirect.
+    return if uc($ENV{HTTPS} || '') eq 'ON';
+    # Never redirect if there isn't an sslbase.
+    return if !$sslbase;
+    Bugzilla->cgi->redirect_to_https();
 }
 
 sub correct_urlbase {
-    my $ssl = Bugzilla->params->{'ssl'};
-    return Bugzilla->params->{'urlbase'} if $ssl eq 'never';
-
+    my $ssl = Bugzilla->params->{'ssl_redirect'};
+    my $urlbase = Bugzilla->params->{'urlbase'};
     my $sslbase = Bugzilla->params->{'sslbase'};
-    if ($sslbase) {
-        return $sslbase if $ssl eq 'always';
-        # Authenticated Sessions
-        return $sslbase if Bugzilla->user->id;
-    }
 
-    # Set to "authenticated sessions" but nobody's logged in, or
-    # sslbase isn't set.
-    return Bugzilla->params->{'urlbase'};
+    return ($ssl && $sslbase) ? $sslbase : $urlbase;
 }
 
 sub use_attachbase {
@@ -830,7 +798,7 @@ cookies) to only some addresses.
 =item C<correct_urlbase()>
 
 Returns either the C<sslbase> or C<urlbase> parameter, depending on the
-current setting for the C<ssl> parameter.
+current setting for the C<ssl_redirect> parameter.
 
 =item C<use_attachbase()>
 
diff --git a/Bugzilla/WebService/Server.pm b/Bugzilla/WebService/Server.pm
index dfb9f559a..2db182fd4 100644
--- a/Bugzilla/WebService/Server.pm
+++ b/Bugzilla/WebService/Server.pm
@@ -17,26 +17,12 @@
 
 package Bugzilla::WebService::Server;
 use strict;
-use Bugzilla::Util qw(ssl_require_redirect);
 
 sub handle_login {
     my ($self, $class, $method, $full_method) = @_;
     eval "require $class";
     return if $class->login_exempt($method);
     Bugzilla->login();
-
-    # Even though we check for the need to redirect in
-    # Bugzilla->login() we check here again since Bugzilla->login()
-    # does not know what the current XMLRPC method is. Therefore
-    # ssl_require_redirect in Bugzilla->login() will have returned 
-    # false if system was configured to redirect for authenticated 
-    # sessions and the user was not yet logged in.
-    # So here we pass in the method name to ssl_require_redirect so
-    # it can then check for the extra case where the method equals
-    # User.login, which we would then need to redirect if not
-    # over a secure connection. 
-    Bugzilla->cgi->require_https(Bugzilla->params->{'sslbase'})
-        if ssl_require_redirect($full_method);
 }
 
 1;
diff --git a/docs/en/xml/administration.xml b/docs/en/xml/administration.xml
index 6b6ff5fda..0c9a60ce2 100644
--- a/docs/en/xml/administration.xml
+++ b/docs/en/xml/administration.xml
@@ -100,13 +100,13 @@
 
           <varlistentry>
             <term>
-              ssl
+              ssl_redirect
             </term>
             <listitem>
               <para>
-                Determines when Bugzilla will force HTTPS (SSL) connections, using
-                the URL defined in <command>sslbase</command>. 
-                Options include "always", "never", and "authenticated sessions". 
+                If enabled, Bugzilla will force HTTPS (SSL) connections, by
+                automatically redirecting any users who try to use a non-SSL
+                connection.
               </para>
             </listitem>
           </varlistentry>
diff --git a/index.cgi b/index.cgi
index 660909452..fac26434a 100755
--- a/index.cgi
+++ b/index.cgi
@@ -56,14 +56,6 @@ if ($cgi->param('logout')) {
 # Main Body Execution
 ###############################################################################
 
-# Force to use HTTPS unless Bugzilla->params->{'ssl'} equals 'never'.
-# This is required because the user may want to log in from here.
-if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
-    && Bugzilla->params->{'ssl'} ne 'never')
-{
-    $cgi->require_https(Bugzilla->params->{'sslbase'});
-}
-
 # Return the appropriate HTTP response headers.
 print $cgi->header();
 
diff --git a/template/en/default/account/auth/login-small.html.tmpl b/template/en/default/account/auth/login-small.html.tmpl
index 752aa64df..710d7d0a5 100644
--- a/template/en/default/account/auth/login-small.html.tmpl
+++ b/template/en/default/account/auth/login-small.html.tmpl
@@ -28,12 +28,7 @@
  [% login_target = "index.cgi" %]
 [% END %]
 
-[%# If SSL is in use, use 'sslbase', else use 'urlbase'. %]
-[% IF Param("sslbase") != "" && Param("ssl") != "never" %]
-  [% login_target = Param("sslbase") _ login_target %]
-[% ELSE %]
-  [% login_target = Param("urlbase") _ login_target %]
-[% END %]
+[% login_target = urlbase _ login_target %]
 
 <li id="mini_login_container[% qs_suffix %]">
   <span class="separator">| </span>
diff --git a/template/en/default/admin/params/core.html.tmpl b/template/en/default/admin/params/core.html.tmpl
index d66c4a51b..b65dde233 100644
--- a/template/en/default/admin/params/core.html.tmpl
+++ b/template/en/default/admin/params/core.html.tmpl
@@ -42,8 +42,12 @@
   sslbase => "The URL that is the common initial leading part of all HTTPS " _
              "(SSL) $terms.Bugzilla URLs.",
 
-  ssl => "Controls when $terms.Bugzilla should enforce sessions to use HTTPS by " _
-         "using <tt>sslbase</tt>.",
+  ssl_redirect => 
+    "When this is enabled, $terms.Bugzilla will ensure that every page is"
+    _ " accessed over SSL, by redirecting any plain HTTP requests to HTTPS"
+    _ " using the <tt>sslbase</tt> parameter. Also, when this is enabled,"
+    _ " $terms.Bugzilla will send out links using <tt>sslbase</tt> in emails"
+    _ " instead of <tt>urlbase</tt>.",
 
   cookiedomain => "The domain for $terms.Bugzilla cookies. Normally blank. " _
                   "If your website is at 'www.foo.com', setting this to " _
diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl
index 76ca259e5..d5a345182 100644
--- a/template/en/default/bug/edit.html.tmpl
+++ b/template/en/default/bug/edit.html.tmpl
@@ -277,7 +277,8 @@
             <legend>Note</legend>
             <p>
               You need to
-              <a href="[% IF Param('ssl') != 'never' %][% Param('sslbase') %][% END %]show_bug.cgi?id=[% bug.bug_id %]&amp;GoAheadAndLogIn=1">log in</a>
+              <a href="show_bug.cgi?id=
+                       [%- bug.bug_id %]&amp;GoAheadAndLogIn=1">log in</a>
               before you can comment on or make changes to this [% terms.bug %].
             </p>
           </fieldset>
diff --git a/token.cgi b/token.cgi
index 614feefa9..d4298cde7 100755
--- a/token.cgi
+++ b/token.cgi
@@ -360,15 +360,7 @@ sub request_create_account {
     $vars->{'email'} = $login_name . Bugzilla->params->{'emailsuffix'};
     $vars->{'expiration_ts'} = ctime(str2time($date) + MAX_TOKEN_AGE * 86400);
 
-    # When 'ssl' equals 'always' or 'authenticated sessions', 
-    # we want this form to always be over SSL.
-    if ($cgi->protocol ne 'https' && Bugzilla->params->{'sslbase'} ne ''
-        && Bugzilla->params->{'ssl'} ne 'never')
-    {
-        $cgi->require_https(Bugzilla->params->{'sslbase'});
-    }
     print $cgi->header();
-
     $template->process('account/email/confirm-new.html.tmpl', $vars)
       || ThrowTemplateError($template->error());
 }