[ARVADOS] updated: f2756832f844d78d782ff35e46b650c5501b0c47
git at public.curoverse.com
git at public.curoverse.com
Mon Jun 23 13:20:11 EDT 2014
Summary of changes:
.../app/controllers/application_controller.rb | 147 ++++++++++-----------
.../app/controllers/collections_controller.rb | 2 +-
.../app/controllers/sessions_controller.rb | 4 +-
apps/workbench/test/integration/errors_test.rb | 19 +++
apps/workbench/test/integration/logins_test.rb | 15 +--
apps/workbench/test/integration_helper.rb | 14 +-
6 files changed, 108 insertions(+), 93 deletions(-)
create mode 100644 apps/workbench/test/integration/errors_test.rb
via f2756832f844d78d782ff35e46b650c5501b0c47 (commit)
via c73e7333d5b55808c68d4f9a39501869e7baf009 (commit)
via 07b681140d714b8df660ba247380e33208fa74cf (commit)
via 36dcc9443ab7a079e6bcb6f874a19c740e5b8441 (commit)
via cf2a30aa6449f7ed9ac6c455842a23af44fb1509 (commit)
via bdc88877d3b4df3e9196e112b7e0bdce2f731998 (commit)
from b72f2ecd46d9723d59f3a826ecac16e7dbd2a00a (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit f2756832f844d78d782ff35e46b650c5501b0c47
Merge: b72f2ec c73e733
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 13:20:46 2014 -0400
Merge branch '2891-workbench-errors-retain-login'
Closes #3032, #3074. Refs #2891.
commit c73e7333d5b55808c68d4f9a39501869e7baf009
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 11:14:29 2014 -0400
2891: Rename and comment Workbench API token filters.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 75b1cd2..d0496cb 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -8,8 +8,10 @@ class ApplicationController < ActionController::Base
ERROR_ACTIONS = [:render_error, :render_not_found]
around_filter :thread_clear
- around_filter :thread_with_api_token
- around_filter :thread_with_mandatory_api_token, except: ERROR_ACTIONS
+ around_filter :set_thread_api_token
+ # Methods that don't require login should
+ # skip_around_filter :require_thread_api_token
+ around_filter :require_thread_api_token, except: ERROR_ACTIONS
before_filter :check_user_agreements, except: ERROR_ACTIONS
before_filter :check_user_notifications, except: ERROR_ACTIONS
before_filter :find_object_by_uuid, except: [:index, :choose] + ERROR_ACTIONS
@@ -56,7 +58,7 @@ class ApplicationController < ActionController::Base
if e.is_a? ArvadosApiClient::NotLoggedInException
self.render_error status: 422
else
- thread_with_api_token do
+ set_thread_api_token do
self.render_error status: 422
end
end
@@ -65,7 +67,7 @@ class ApplicationController < ActionController::Base
def render_not_found(e=ActionController::RoutingError.new("Path not found"))
logger.error e.inspect
@errors = ["Path not found"]
- thread_with_api_token do
+ set_thread_api_token do
self.render_error status: 404
end
end
@@ -369,7 +371,12 @@ class ApplicationController < ActionController::Base
Rails.cache.delete_matched(/^request_#{Thread.current.object_id}_/)
end
- def thread_with_api_token
+ # Save the session API token in thread-local storage, and yield.
+ # This method also takes care of session setup if the request
+ # provides a valid api_token parameter.
+ # If a token is unavailable or expired, the block is still run, with
+ # a nil token.
+ def set_thread_api_token
# If an API token has already been found, pass it through.
if Thread.current[:arvados_api_token]
yield
@@ -419,7 +426,8 @@ class ApplicationController < ActionController::Base
end
end
- def thread_with_mandatory_api_token
+ # Reroute this request if an API token is unavailable.
+ def require_thread_api_token
if Thread.current[:arvados_api_token]
yield
elsif session[:arvados_api_token]
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 9179848..95aee92 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -1,5 +1,5 @@
class CollectionsController < ApplicationController
- skip_around_filter(:thread_with_mandatory_api_token,
+ skip_around_filter(:require_thread_api_token,
only: [:show_file, :show_file_links])
skip_before_filter(:find_object_by_uuid,
only: [:provenance, :show_file, :show_file_links])
diff --git a/apps/workbench/app/controllers/sessions_controller.rb b/apps/workbench/app/controllers/sessions_controller.rb
index 9cd1e1c..97c8d5a 100644
--- a/apps/workbench/app/controllers/sessions_controller.rb
+++ b/apps/workbench/app/controllers/sessions_controller.rb
@@ -1,6 +1,6 @@
class SessionsController < ApplicationController
- skip_around_filter :thread_with_mandatory_api_token, :only => [:destroy, :index]
- skip_around_filter :thread_with_api_token, :only => [:destroy, :index]
+ skip_around_filter :require_thread_api_token, :only => [:destroy, :index]
+ skip_around_filter :set_thread_api_token, :only => [:destroy, :index]
skip_before_filter :find_object_by_uuid, :only => [:destroy, :index]
def destroy
commit 07b681140d714b8df660ba247380e33208fa74cf
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 10:43:02 2014 -0400
2891: Refactor Workbench API token filters.
Remove dead code and simplify the implementation, especially the
relationship between the "required" and "optional" filters, per
discussion with Peter.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 521e48e..75b1cd2 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -8,8 +8,8 @@ class ApplicationController < ActionController::Base
ERROR_ACTIONS = [:render_error, :render_not_found]
around_filter :thread_clear
+ around_filter :thread_with_api_token
around_filter :thread_with_mandatory_api_token, except: ERROR_ACTIONS
- around_filter :thread_with_optional_api_token
before_filter :check_user_agreements, except: ERROR_ACTIONS
before_filter :check_user_notifications, except: ERROR_ACTIONS
before_filter :find_object_by_uuid, except: [:index, :choose] + ERROR_ACTIONS
@@ -56,7 +56,7 @@ class ApplicationController < ActionController::Base
if e.is_a? ArvadosApiClient::NotLoggedInException
self.render_error status: 422
else
- thread_with_optional_api_token do
+ thread_with_api_token do
self.render_error status: 422
end
end
@@ -65,7 +65,7 @@ class ApplicationController < ActionController::Base
def render_not_found(e=ActionController::RoutingError.new("Path not found"))
logger.error e.inspect
@errors = ["Path not found"]
- thread_with_optional_api_token do
+ thread_with_api_token do
self.render_error status: 404
end
end
@@ -369,56 +369,46 @@ class ApplicationController < ActionController::Base
Rails.cache.delete_matched(/^request_#{Thread.current.object_id}_/)
end
- def thread_with_api_token(login_optional = false)
+ def thread_with_api_token
+ # If an API token has already been found, pass it through.
+ if Thread.current[:arvados_api_token]
+ yield
+ return
+ end
+
begin
- try_redirect_to_login = true
- if params[:api_token]
- 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] = {
- uuid: u.uuid,
- email: u.email,
- first_name: u.first_name,
- last_name: u.last_name,
- is_active: u.is_active,
- is_admin: u.is_admin,
- prefs: u.prefs
- }
- if !request.format.json? and request.method.in? ['GET', 'HEAD']
- # Repeat this request with api_token in the (new) session
- # 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 strip_token_from_path(request.fullpath)
- else
- yield
- end
- 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.
- Thread.current[:arvados_api_token] = session[:arvados_api_token]
- begin
- yield
- rescue ArvadosApiClient::NotLoggedInException
- # We'll try to redirect to login later.
- else
- try_redirect_to_login = false
+ # If there's a valid api_token parameter, use it to set up the session.
+ if (Thread.current[:arvados_api_token] = params[:api_token]) and
+ verify_api_token
+ session[:arvados_api_token] = params[:api_token]
+ u = User.current
+ session[:user] = {
+ uuid: u.uuid,
+ email: u.email,
+ first_name: u.first_name,
+ last_name: u.last_name,
+ is_active: u.is_active,
+ is_admin: u.is_admin,
+ prefs: u.prefs
+ }
+ if !request.format.json? and request.method.in? ['GET', 'HEAD']
+ # Repeat this request with api_token in the (new) session
+ # 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 strip_token_from_path(request.fullpath)
+ return
end
- else
- logger.debug "No token received, session is #{session.inspect}"
end
- if try_redirect_to_login
- unless login_optional
- redirect_to_login
- else
- # login is optional for this route so go on to the regular controller
+
+ # With setup done, handle the request using the session token.
+ Thread.current[:arvados_api_token] = session[:arvados_api_token]
+ begin
+ yield
+ rescue ArvadosApiClient::NotLoggedInException
+ # If we got this error with a token, it must've expired.
+ # Retry the request without a token.
+ unless Thread.current[:arvados_api_token].nil?
Thread.current[:arvados_api_token] = nil
yield
end
@@ -430,31 +420,16 @@ class ApplicationController < ActionController::Base
end
def thread_with_mandatory_api_token
- thread_with_api_token(true) do
- if Thread.current[:arvados_api_token]
- yield
- elsif session[:arvados_api_token]
- # Expired session. Clear it before refreshing login so that,
- # if this login procedure fails, we end up showing the "please
- # log in" page instead of getting stuck in a redirect loop.
- session.delete :arvados_api_token
- redirect_to_login
- else
- render 'users/welcome'
- end
- end
- end
-
- # This runs after thread_with_mandatory_api_token in the filter chain.
- def thread_with_optional_api_token
if Thread.current[:arvados_api_token]
- # We are already inside thread_with_mandatory_api_token.
yield
+ elsif session[:arvados_api_token]
+ # Expired session. Clear it before refreshing login so that,
+ # if this login procedure fails, we end up showing the "please
+ # log in" page instead of getting stuck in a redirect loop.
+ session.delete :arvados_api_token
+ redirect_to_login
else
- # We skipped thread_with_mandatory_api_token. Use the optional version.
- thread_with_api_token(true) do
- yield
- end
+ render 'users/welcome'
end
end
diff --git a/apps/workbench/app/controllers/sessions_controller.rb b/apps/workbench/app/controllers/sessions_controller.rb
index 591373b..9cd1e1c 100644
--- a/apps/workbench/app/controllers/sessions_controller.rb
+++ b/apps/workbench/app/controllers/sessions_controller.rb
@@ -1,6 +1,6 @@
class SessionsController < ApplicationController
skip_around_filter :thread_with_mandatory_api_token, :only => [:destroy, :index]
- skip_around_filter :thread_with_optional_api_token, :only => [:destroy, :index]
+ skip_around_filter :thread_with_api_token, :only => [:destroy, :index]
skip_before_filter :find_object_by_uuid, :only => [:destroy, :index]
def destroy
commit 36dcc9443ab7a079e6bcb6f874a19c740e5b8441
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 59ba2c5..521e48e 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -53,13 +53,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 cf2a30aa6449f7ed9ac6c455842a23af44fb1509
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 22a5c49..59ba2c5 100644
--- 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 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 bdc88877d3b4df3e9196e112b7e0bdce2f731998
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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list