[ARVADOS] created: 62f9bb4bb85fc644e2b005b6f953b642b832b21c

git at public.curoverse.com git at public.curoverse.com
Fri Jun 20 14:32:55 EDT 2014


        at  62f9bb4bb85fc644e2b005b6f953b642b832b21c (commit)


commit 62f9bb4bb85fc644e2b005b6f953b642b832b21c
Author: Brett Smith <brett at curoverse.com>
Date:   Fri Jun 20 14:33:18 2014 -0400

    2891: Workbench error page shows login status.
    
    The tests here check that an error page shows the usual user
    information for someone logged in, and renders okay for someone who
    isn't.

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 28d6593..07edc36 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -55,13 +55,21 @@ class ApplicationController < ActionController::Base
     else
       @errors = [e.to_s]
     end
-    self.render_error status: 422
+    if e.is_a? ArvadosApiClient::NotLoggedInException
+      self.render_error status: 422
+    else
+      thread_with_optional_api_token do
+        self.render_error status: 422
+      end
+    end
   end
 
   def render_not_found(e=ActionController::RoutingError.new("Path not found"))
     logger.error e.inspect
     @errors = ["Path not found"]
-    self.render_error status: 404
+    thread_with_optional_api_token do
+      self.render_error status: 404
+    end
   end
 
   def find_objects_for_index
diff --git a/apps/workbench/test/integration/errors_test.rb b/apps/workbench/test/integration/errors_test.rb
new file mode 100644
index 0000000..092041d
--- /dev/null
+++ b/apps/workbench/test/integration/errors_test.rb
@@ -0,0 +1,19 @@
+require 'integration_helper'
+
+class ErrorsTest < ActionDispatch::IntegrationTest
+  BAD_UUID = "zzzzz-zzzzz-zzzzzzzzzzzzzzz"
+
+  test "error page renders user navigation" do
+    visit(page_with_token("active", "/collections/#{BAD_UUID}"))
+    assert(page.has_text?(@@API_AUTHS["active"]["email"]),
+           "User information missing from error page")
+    assert(page.has_no_text?(/log ?in/i),
+           "Logged in user prompted to log in on error page")
+  end
+
+  test "error page renders without login" do
+    visit "/collections/download/#{BAD_UUID}/#{@@API_AUTHS['active']['api_token']}"
+    assert(page.has_no_text?(/\b500\b/),
+           "Error page without login returned 500")
+  end
+end

commit af60f396e5a4d313bbfeb810178885b69550f7ba
Author: Brett Smith <brett at curoverse.com>
Date:   Fri Jun 20 14:31:33 2014 -0400

    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 9e918f5..28d6593 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -296,11 +296,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
@@ -363,11 +367,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] = {
@@ -384,24 +388,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 be7e4e1..9daf831 100644
--- 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

commit 7917aacd2736ab3e41613cbab3c113518964fd9d
Author: Brett Smith <brett at curoverse.com>
Date:   Fri Jun 20 11:19:20 2014 -0400

    2891: Workbench integration tests cope when screenshots aren't supported.

diff --git a/apps/workbench/test/integration_helper.rb b/apps/workbench/test/integration_helper.rb
index ebfbc58..81ea67c 100644
--- a/apps/workbench/test/integration_helper.rb
+++ b/apps/workbench/test/integration_helper.rb
@@ -54,11 +54,17 @@ class ActionDispatch::IntegrationTest
     end
   end
 
-  @@screenshot_count = 0
+  @@screenshot_count = 1
   def screenshot
-    image_file = "./tmp/workbench-fail-#{@@screenshot_count += 1}.png"
-    page.save_screenshot image_file
-    puts "Saved #{image_file}"
+    image_file = "./tmp/workbench-fail-#{@@screenshot_count}.png"
+    begin
+      page.save_screenshot image_file
+    rescue Capybara::NotSupportedByDriverError
+      # C'est la vie.
+    else
+      puts "Saved #{image_file}"
+      @@screenshot_count += 1
+    end
   end
 
   teardown do

commit 355ebf1f21ba7344ac6e70dcf7e1717fe92f33ed
Author: Brett Smith <brett at curoverse.com>
Date:   Fri Jun 20 14:33:28 2014 -0400

    Remove debug prints from Workbench.

diff --git a/apps/workbench/app/views/collections/_choose_rows.html.erb b/apps/workbench/app/views/collections/_choose_rows.html.erb
index 5cce19a..d87f56f 100644
--- a/apps/workbench/app/views/collections/_choose_rows.html.erb
+++ b/apps/workbench/app/views/collections/_choose_rows.html.erb
@@ -1,7 +1,5 @@
 <% @name_links.each do |name_link| %>
-  <% puts "looking up #{name_link.head_uuid}" %>
   <% if (object = get_object(name_link.head_uuid)) %>
-    <% puts "got #{object}" %>
     <div class="row filterable selectable <%= 'multiple' if multiple %>" data-object-uuid="<%= name_link.uuid %>"
          data-preview-href="<%= url_for object %>?tab_pane=chooser_preview"
          style="margin-left: 1em; border-bottom-style: solid; border-bottom-width: 1px; border-bottom-color: #DDDDDD">

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list