Bug 917669 - invalid or expired authentication tokens and cookies should throw errors, not be silently ignored

r/a=glob

4 files changed, 36 insertions, 14 deletions

diff --git a/Bugzilla/Auth/Login/Cookie.pm b/Bugzilla/Auth/Login/Cookie.pm
index 130fab8e3..3068331ea 100644
--- a/Bugzilla/Auth/Login/Cookie.pm
+++ b/Bugzilla/Auth/Login/Cookie.pm
@@ -14,6 +14,7 @@ use parent qw(Bugzilla::Auth::Login);
 
 use Bugzilla::Constants;
 use Bugzilla::Util;
+use Bugzilla::Error;
 
 use List::Util qw(first);
 
@@ -73,7 +74,9 @@ sub get_login_info {
                                         AND (ipaddr = ? OR ipaddr IS NULL)',
                                  undef, ($login_cookie, $user_id, $ip_addr));
 
-        # If the cookie is valid, return a valid username.
+        # If the cookie or token is valid, return a valid username.
+        # If they were not valid and we are using a webservice, then
+        # throw an error notifying the client.
         if ($is_valid) {
             # If we logged in successfully, then update the lastused 
             # time on the login cookie
@@ -81,12 +84,16 @@ sub get_login_info {
                        WHERE cookie = ?", undef, $login_cookie);
             return { user_id => $user_id };
         }
+        elsif (i_am_webservice()) {
+            ThrowUserError('invalid_cookies_or_token');
+        }
     }
 
-    # Either the he cookie is invalid, or we got no cookie. We don't want 
-    # to ever return AUTH_LOGINFAILED, because we don't want Bugzilla to 
-    # actually throw an error when it gets a bad cookie. It should just 
-    # look like there was no cookie to begin with.
+    # Either the cookie or token is invalid and we are not authenticating
+    # via a webservice, or we did not receive a cookie or token. We don't
+    # want to ever return AUTH_LOGINFAILED, because we don't want Bugzilla to
+    # actually throw an error when it gets a bad cookie or token. It should just
+    # look like there was no cookie or token to begin with.
     return { failure => AUTH_NODATA };
 }
 
@@ -97,9 +104,7 @@ sub login_token {
 
     return $self->{'_login_token'} if exists $self->{'_login_token'};
 
-    if ($usage_mode ne USAGE_MODE_XMLRPC
-        && $usage_mode ne USAGE_MODE_JSON
-        && $usage_mode ne USAGE_MODE_REST) {
+    if (!i_am_webservice()) {
         return $self->{'_login_token'} = undef;
     }
 
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index 4b227e9d2..e1f7f3b60 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -837,10 +837,7 @@ sub create {
                 # (Wrapping the message in the WebService is unnecessary
                 # and causes awkward things like \n's appearing in error
                 # messages in JSON-RPC.)
-                unless (Bugzilla->usage_mode == USAGE_MODE_JSON
-                        or Bugzilla->usage_mode == USAGE_MODE_XMLRPC
-                        or Bugzilla->usage_mode == USAGE_MODE_REST)
-                {
+                unless (i_am_webservice()) {
                     $var = wrap_comment($var, 72);
                 }
                 $var =~ s/\ / /g;
diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm
index a3651182a..a9ff86c8b 100644
--- a/Bugzilla/Util.pm
+++ b/Bugzilla/Util.pm
@@ -14,8 +14,8 @@ use parent qw(Exporter);
 @Bugzilla::Util::EXPORT = qw(trick_taint detaint_natural detaint_signed
                              html_quote url_quote xml_quote
                              css_class_quote html_light_quote
-                             i_am_cgi correct_urlbase remote_ip validate_ip
-                             do_ssl_redirect_if_required use_attachbase
+                             i_am_cgi i_am_webservice correct_urlbase remote_ip
+                             validate_ip do_ssl_redirect_if_required use_attachbase
                              diff_arrays on_main_db
                              trim wrap_hard wrap_comment find_wrap_point
                              format_time validate_date validate_time datetime_from
@@ -237,6 +237,13 @@ sub i_am_cgi {
     return exists $ENV{'SERVER_SOFTWARE'} ? 1 : 0;
 }
 
+sub i_am_webservice {
+    my $usage_mode = Bugzilla->usage_mode;
+    return $usage_mode == USAGE_MODE_XMLRPC
+           || $usage_mode == USAGE_MODE_JSON
+           || $usage_mode == USAGE_MODE_REST;
+}
+
 # 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).
@@ -847,6 +854,7 @@ Bugzilla::Util - Generic utility functions for bugzilla
 
   # Functions that tell you about your environment
   my $is_cgi   = i_am_cgi();
+  my $is_webservice = i_am_webservice();
   my $urlbase  = correct_urlbase();
 
   # Data manipulation
@@ -974,6 +982,11 @@ 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<i_am_webservice()>
+
+Tells you whether or not the current usage mode is WebServices related
+such as JSONRPC, XMLRPC, or REST.
+
 =item C<correct_urlbase()>
 
 Returns either the C<sslbase> or C<urlbase> parameter, depending on the
diff --git a/Bugzilla/WebService.pm b/Bugzilla/WebService.pm
index 03548d257..92ffed659 100644
--- a/Bugzilla/WebService.pm
+++ b/Bugzilla/WebService.pm
@@ -181,6 +181,13 @@ For REST, you may also use the C<username> and C<password> variable
 names instead of C<Bugzilla_login> and C<Bugzilla_password> as a
 convenience.
 
+=item B<Added in Bugzilla 5.0>
+
+An error is now thrown if you pass invalid cookies or an invalid token.
+You will need to log in again to get new cookies or a new token. Previous
+releases simply ignored invalid cookies and token support was added in
+Bugzilla B<5.0>.
+
 =back
 
 =head1 STABLE, EXPERIMENTAL, and UNSTABLE