Bug 514913: Eliminate ssl="authenticated sessions"

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

8 files changed, 52 insertions, 109 deletions

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;