5105: Treat not-logged-in AJAX requests as errors, instead of redirecting to login...

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index b52591bc0caa5ff1b14da6ac7485a829eaeab1d8..e0786418ec736aa9e3bb790dd77409c11781ca18 100644 (file)
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -441,19 +441,14 @@ class ApplicationController < ActionController::Base
   end
 
   def redirect_to_login
-    respond_to do |f|
-      f.html {
-        if request.method.in? ['GET', 'HEAD']
-          redirect_to arvados_api_client.arvados_login_url(return_to: strip_token_from_path(request.url))
-        else
-          flash[:error] = "Either you are not logged in, or your session has timed out. I can't automatically log you in and re-attempt this request."
-          redirect_to :back
-        end
-      }
-      f.json {
-        @errors = ['You do not seem to be logged in. You did not supply an API token with this request, and your session (if any) has timed out.']
-        self.render_error status: 422
-      }
+    if request.xhr? or request.format.json?
+      @errors = ['You are not logged in. Most likely your session has timed out and you need to log in again.']
+      render_error status: 401
+    elsif request.method.in? ['GET', 'HEAD']
+      redirect_to arvados_api_client.arvados_login_url(return_to: strip_token_from_path(request.url))
+    else
+      flash[:error] = "Either you are not logged in, or your session has timed out. I can't automatically log you in and re-attempt this request."
+      redirect_to :back
     end
     false  # For convenience to return from callbacks
   end
@@ -594,7 +589,8 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  # Redirect to login/welcome if client provided expired API token (or none at all)
+  # Redirect to login/welcome if client provided expired API token (or
+  # none at all)
   def require_thread_api_token
     if Thread.current[:arvados_api_token]
       yield
@@ -604,15 +600,26 @@ class ApplicationController < ActionController::Base
       # log in" page instead of getting stuck in a redirect loop.
       session.delete :arvados_api_token
       redirect_to_login
+    elsif request.xhr?
+      # If we redirect to the welcome page, the browser will handle
+      # the 302 by itself and the client code will end up rendering
+      # the "welcome" page in some content area where it doesn't make
+      # sense. Instead, we send 401 ("authenticate and try again" or
+      # "display error", depending on how smart the client side is).
+      @errors = ['You are not logged in.']
+      render_error status: 401
     else
       redirect_to welcome_users_path(return_to: request.fullpath)
     end
   end
 
   def ensure_current_user_is_admin
-    unless current_user and current_user.is_admin
+    if not current_user
+      @errors = ['Not logged in']
+      render_error status: 401
+    elsif not current_user.is_admin
       @errors = ['Permission denied']
-      self.render_error status: 401
+      render_error status: 403
     end
   end
 

diff --git a/apps/workbench/test/integration/ajax_errors_test.rb b/apps/workbench/test/integration/ajax_errors_test.rb
new file mode 100644 (file)
index 0000000..de2dfc7
--- /dev/null
+++ b/apps/workbench/test/integration/ajax_errors_test.rb
@@ -0,0 +1,45 @@
+require 'integration_helper'
+
+class AjaxErrorsTest < ActionDispatch::IntegrationTest
+  setup do
+    # Regrettably...
+    need_selenium 'to assert_text in iframe'
+  end
+
+  test 'load pane with deleted session' do
+    # Simulate loading a page in browser-tab A, hitting "Log out" in
+    # browser-tab B, then returning to browser-tab A and choosing a
+    # different tab. (Automatic tab refreshes will behave similarly.)
+    visit page_with_token('active', '/projects/' + api_fixture('groups')['aproject']['uuid'])
+    ActionDispatch::Request::Session.any_instance.stubs(:[]).returns(nil)
+    click_link "Subprojects"
+    wait_for_ajax
+    assert_no_selector '.container-fluid .container-fluid'
+    assert_no_text 'If you have never used'
+    assert_text 'Reload tab'
+    page.driver.browser.switch_to.frame 0
+    assert_text 'You are not logged in.'
+  end
+
+  test 'load pane with expired token' do
+    # Similar to 'deleted session'. Here, the session cookie is still
+    # alive, but it contains a token which has expired. This uses a
+    # different code path because Workbench cannot detect that
+    # anything is amiss until it actually uses the token in an API
+    # request.
+    visit page_with_token('active', '/projects/' + api_fixture('groups')['aproject']['uuid'])
+    use_token :active_trustedclient do
+      # Go behind Workbench's back to expire the "active" token.
+      token = api_fixture('api_client_authorizations')['active']['api_token']
+      auth = ApiClientAuthorization.find(token)
+      auth.update_attributes(expires_at: '1999-12-31T23:59:59Z')
+    end
+    click_link "Subprojects"
+    wait_for_ajax
+    assert_no_selector '.container-fluid .container-fluid'
+    assert_no_text 'If you have never used'
+    assert_text 'Reload tab'
+    page.driver.browser.switch_to.frame 0
+    assert_text 'You are not logged in.'
+  end
+end

diff --git a/apps/workbench/test/test_helper.rb b/apps/workbench/test/test_helper.rb
index 335fcd0ee51b76fd04077cfbcbfc0bb39d381ced..c2ff207ab0255955b850d6b358d568c23253dcea 100644 (file)
--- a/apps/workbench/test/test_helper.rb
+++ b/apps/workbench/test/test_helper.rb
@@ -32,8 +32,13 @@ class ActiveSupport::TestCase
   # in integration tests -- they do not yet inherit this setting
   fixtures :all
   def use_token token_name
+    was = Thread.current[:arvados_api_token]
     auth = api_fixture('api_client_authorizations')[token_name.to_s]
     Thread.current[:arvados_api_token] = auth['api_token']
+    if block_given?
+      yield
+      Thread.current[:arvados_api_token] = was
+    end
   end
 
   setup do