2891: Workbench handles expired tokens more consistently.

Previously Workbench would behave differently depending on whether an
expired token was provided as a query parameter, or living in a
session.  This makes it do the same thing in all cases.  It also fixes
some small bugs around removing api_token from URL paths.

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 22a5c49e0f0c1bce0b4cafb2d906286010029705..59ba2c5b73e176fc57c9c4d0dabc923f07a38ae8 100644 (file)
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -294,11 +294,15 @@ class ApplicationController < ActionController::Base
 
   protected
 
+  def strip_token_from_path(path)
+    path.sub(/([\?&;])api_token=[^&;]*[&;]?/, '\1')
+  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: request.url)
+          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
@@ -361,11 +365,11 @@ class ApplicationController < ActionController::Base
     begin
       try_redirect_to_login = true
       if params[:api_token]
-        try_redirect_to_login = false
         Thread.current[:arvados_api_token] = params[:api_token]
         # Before copying the token into session[], do a simple API
         # call to verify its authenticity.
         if verify_api_token
+          try_redirect_to_login = false
           session[:arvados_api_token] = params[:api_token]
           u = User.current
           session[:user] = {
@@ -382,24 +386,22 @@ class ApplicationController < ActionController::Base
             # cookie instead of the query string.  This prevents API
             # tokens from appearing in (and being inadvisedly copied
             # and pasted from) browser Location bars.
-            redirect_to request.fullpath.sub(%r{([&\?]api_token=)[^&\?]*}, '')
+            redirect_to strip_token_from_path(request.fullpath)
           else
             yield
           end
-        else
-          @errors = ['Invalid API token']
-          self.render_error status: 401
         end
       elsif session[:arvados_api_token]
         # In this case, the token must have already verified at some
         # point, but it might have been revoked since.  We'll try
         # using it, and catch the exception if it doesn't work.
-        try_redirect_to_login = false
         Thread.current[:arvados_api_token] = session[:arvados_api_token]
         begin
           yield
         rescue ArvadosApiClient::NotLoggedInException
-          try_redirect_to_login = true
+          # We'll try to redirect to login later.
+        else
+          try_redirect_to_login = false
         end
       else
         logger.debug "No token received, session is #{session.inspect}"

diff --git a/apps/workbench/test/integration/logins_test.rb b/apps/workbench/test/integration/logins_test.rb
index be7e4e15a5692d5b47ce433fb5647cbb7159bd29..9daf831810de1253496f6c91ee6a931525b5c765 100644 (file)
--- a/apps/workbench/test/integration/logins_test.rb
+++ b/apps/workbench/test/integration/logins_test.rb
@@ -7,15 +7,12 @@ class LoginsTest < ActionDispatch::IntegrationTest
     assert_no_match(/\bapi_token=/, current_path)
   end
 
-  test "can't use expired token" do
+  test "trying to use expired token redirects to login page" do
     visit page_with_token('expired_trustedclient')
-    assert page.has_text? 'Log in'
-  end
-
-  test "expired token yields login page, not error page" do
-    visit page_with_token('expired_trustedclient')
-    # Even the error page has a "Log in" link. We should look for
-    # something that only appears the real login page.
-    assert page.has_text? ' Log in Oh... fiddlesticks. Sorry, I had some trouble handling your request'
+    buttons = all("a.btn", text: /Log in/)
+    assert_equal(1, buttons.size, "Failed to find one login button")
+    login_link = buttons.first[:href]
+    assert_match(%r{//[^/]+/login}, login_link)
+    assert_no_match(/\bapi_token=/, login_link)
   end
 end