Merge branch '2388-bogus-token-error-page'
authorTom Clegg <tom@curoverse.com>
Tue, 15 Apr 2014 03:56:04 +0000 (23:56 -0400)
committerTom Clegg <tom@curoverse.com>
Tue, 15 Apr 2014 03:56:04 +0000 (23:56 -0400)
closes #2388

apps/workbench/app/controllers/application_controller.rb
apps/workbench/test/functional/users_controller_test.rb
apps/workbench/test/integration/logins_test.rb
apps/workbench/test/integration_helper.rb
apps/workbench/test/test_helper.rb
services/api/app/controllers/application_controller.rb
services/api/test/fixtures/api_client_authorizations.yml
services/api/test/functional/arvados/v1/users_controller_test.rb

index c169be27044ec5db6e0e8da8b67a0e75f8a1cf8d..1e4094dbc1498d5925719aaff97e88c995515235 100644 (file)
@@ -30,6 +30,7 @@ class ApplicationController < ActionController::Base
   end
 
   def render_error(opts)
+    opts = {status: 500}.merge opts
     respond_to do |f|
       # json must come before html here, so it gets used as the
       # default format when js is requested by the client. This lets
index c67c56b5c7edf6631f2c093e343d40e4f66affc3..aadee36f656bfbd99bef0f113651b86f9701d2fc 100644 (file)
@@ -1,4 +1,20 @@
 require 'test_helper'
 
 class UsersControllerTest < ActionController::TestCase
+  test "ignore previously valid token (for deleted user), don't crash" do
+    get :welcome, {}, session_for(:valid_token_deleted_user)
+    assert_response :success
+    assert_nil assigns(:my_jobs)
+    assert_nil assigns(:my_ssh_keys)
+  end
+
+  test "expired token redirects to api server login" do
+    get :show, {
+      id: api_fixture('users')['active']['uuid']
+    }, session_for(:expired_trustedclient)
+    assert_response :redirect
+    assert_match /^#{Rails.configuration.arvados_login_base}/, @response.redirect_url
+    assert_nil assigns(:my_jobs)
+    assert_nil assigns(:my_ssh_keys)
+  end
 end
index 185d9cb017f55ee66412d7df5e616bc96d55a881..6e5389e7cc11d0a4d6c7c4808023476346e9c978 100644 (file)
@@ -11,4 +11,12 @@ class LoginsTest < ActionDispatch::IntegrationTest
     visit page_with_token('expired_trustedclient')
     assert page.has_text? 'Log in'
   end
+
+  test "expired token yields login page, not error page" do
+    skip
+    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? 'Please log in'
+  end
 end
index 30643bc7224ee81c91f41d6fa1452dfa460f104d..88aec2ca6948ef8512b3b5604efe4b315d107560 100644 (file)
@@ -4,20 +4,12 @@ require 'capybara/poltergeist'
 require 'uri'
 require 'yaml'
 
-$ARV_API_SERVER_DIR = File.expand_path('../../../../services/api', __FILE__)
-SERVER_PID_PATH = 'tmp/pids/server.pid'
-
 class ActionDispatch::IntegrationTest
   # Make the Capybara DSL available in all integration tests
   include Capybara::DSL
+  include ApiFixtureLoader
 
-  def self.api_fixture(name)
-    # Returns the data structure from the named API server test fixture.
-    path = File.join($ARV_API_SERVER_DIR, 'test', 'fixtures', "#{name}.yml")
-    YAML.load(IO.read(path))
-  end
-
-  @@API_AUTHS = api_fixture('api_client_authorizations')
+  @@API_AUTHS = self.api_fixture('api_client_authorizations')
 
   def page_with_token(token, path='/')
     # Generate a page path with an embedded API token.
@@ -31,48 +23,3 @@ class ActionDispatch::IntegrationTest
     "#{path}#{sep}#{q_string}"
   end
 end
-
-class IntegrationTestRunner < MiniTest::Unit
-  # Make a hash that unsets Bundle's environment variables.
-  # We'll use this environment when we launch Bundle commands in the API
-  # server.  Otherwise, those commands will try to use Workbench's gems, etc.
-  @@APIENV = Hash[ENV.map { |key, val|
-                    (key =~ /^BUNDLE_/) ? [key, nil] : nil
-                  }.compact]
-
-  def _system(*cmd)
-    if not system(@@APIENV, *cmd)
-      raise RuntimeError, "#{cmd[0]} returned exit code #{$?.exitstatus}"
-    end
-  end
-
-  def _run(args=[])
-    Capybara.javascript_driver = :poltergeist
-    server_pid = Dir.chdir($ARV_API_SERVER_DIR) do |apidir|
-      _system('bundle', 'exec', 'rake', 'db:test:load')
-      _system('bundle', 'exec', 'rake', 'db:fixtures:load')
-      _system('bundle', 'exec', 'rails', 'server', '-d')
-      timeout = Time.now.tv_sec + 10
-      begin
-        sleep 0.2
-        begin
-          server_pid = IO.read(SERVER_PID_PATH).to_i
-          good_pid = (server_pid > 0) and (Process.kill(0, pid) rescue false)
-        rescue Errno::ENOENT
-          good_pid = false
-        end
-      end while (not good_pid) and (Time.now.tv_sec < timeout)
-      if not good_pid
-        raise RuntimeError, "could not find API server Rails pid"
-      end
-      server_pid
-    end
-    begin
-      super(args)
-    ensure
-      Process.kill('TERM', server_pid)
-    end
-  end
-end
-
-MiniTest::Unit.runner = IntegrationTestRunner.new
index 8bf1192ffec252a4562218bdf299891a319b9cb9..145914f7414f4006d3cb648d327e90cc56fe94c6 100644 (file)
@@ -2,6 +2,9 @@ ENV["RAILS_ENV"] = "test"
 require File.expand_path('../../config/environment', __FILE__)
 require 'rails/test_help'
 
+$ARV_API_SERVER_DIR = File.expand_path('../../../../services/api', __FILE__)
+SERVER_PID_PATH = 'tmp/pids/server.pid'
+
 class ActiveSupport::TestCase
   # Setup all fixtures in test/fixtures/*.(yml|csv) for all tests in alphabetical order.
   #
@@ -11,3 +14,78 @@ class ActiveSupport::TestCase
 
   # Add more helper methods to be used by all tests here...
 end
+
+module ApiFixtureLoader
+  def self.included(base)
+    base.extend(ClassMethods)
+  end
+
+  module ClassMethods
+    @@api_fixtures = {}
+    def api_fixture(name)
+      # Returns the data structure from the named API server test fixture.
+      @@api_fixtures[name] ||= \
+      begin
+        path = File.join($ARV_API_SERVER_DIR, 'test', 'fixtures', "#{name}.yml")
+        YAML.load(IO.read(path))
+      end
+    end
+  end
+  def api_fixture name
+    self.class.api_fixture name
+  end
+end
+
+class ActiveSupport::TestCase
+  include ApiFixtureLoader
+  def session_for api_client_auth_name
+    {
+      arvados_api_token: api_fixture('api_client_authorizations')[api_client_auth_name.to_s]['api_token']
+    }
+  end
+end
+
+class ApiServerBackedTestRunner < MiniTest::Unit
+  # Make a hash that unsets Bundle's environment variables.
+  # We'll use this environment when we launch Bundle commands in the API
+  # server.  Otherwise, those commands will try to use Workbench's gems, etc.
+  @@APIENV = Hash[ENV.map { |key, val|
+                    (key =~ /^BUNDLE_/) ? [key, nil] : nil
+                  }.compact]
+
+  def _system(*cmd)
+    if not system(@@APIENV, *cmd)
+      raise RuntimeError, "#{cmd[0]} returned exit code #{$?.exitstatus}"
+    end
+  end
+
+  def _run(args=[])
+    Capybara.javascript_driver = :poltergeist
+    server_pid = Dir.chdir($ARV_API_SERVER_DIR) do |apidir|
+      _system('bundle', 'exec', 'rake', 'db:test:load')
+      _system('bundle', 'exec', 'rake', 'db:fixtures:load')
+      _system('bundle', 'exec', 'rails', 'server', '-d')
+      timeout = Time.now.tv_sec + 10
+      begin
+        sleep 0.2
+        begin
+          server_pid = IO.read(SERVER_PID_PATH).to_i
+          good_pid = (server_pid > 0) and (Process.kill(0, pid) rescue false)
+        rescue Errno::ENOENT
+          good_pid = false
+        end
+      end while (not good_pid) and (Time.now.tv_sec < timeout)
+      if not good_pid
+        raise RuntimeError, "could not find API server Rails pid"
+      end
+      server_pid
+    end
+    begin
+      super(args)
+    ensure
+      Process.kill('TERM', server_pid)
+    end
+  end
+end
+
+MiniTest::Unit.runner = ApiServerBackedTestRunner.new
index 5a9c90c324eac9085fece0000825311b51a509c0..4b13fca1de45757292592e80152700efe8c83954 100644 (file)
@@ -352,6 +352,9 @@ class ApplicationController < ActionController::Base
           session[:api_client_authorization_id] = api_client_auth.id
           user = api_client_auth.user
           api_client = api_client_auth.api_client
+        else
+          # Token seems valid, but points to a non-existent (deleted?) user.
+          api_client_auth = nil
         end
       elsif session[:user_id]
         user = User.find(session[:user_id]) rescue nil
index f60ba010eaf9d642aef06302d2aba2be47d1240f..5cada908a4f43ccbf38744595b467c6d77e0a3f5 100644 (file)
@@ -71,3 +71,9 @@ expired_trustedclient:
   user: active
   api_token: 5hpni7izokzcatku2896xxwqdbt5ptomn04r6auc7fohnli82v
   expires_at: 1970-01-01 00:00:00
+
+valid_token_deleted_user:
+  api_client: trusted_workbench
+  user_id: 1234567
+  api_token: tewfa58099sndckyqhlgd37za6e47o6h03r9l1vpll23hudm8b
+  expires_at: 2038-01-01 00:00:00
index 799cbfa3dd35d5e87e9fa18de8f74073c52ed37f..6dc5950dd81a18f32a7ad5175d5b1defc06fea65 100644 (file)
@@ -44,6 +44,12 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
     assert_equal true, me['is_active']
   end
 
+  test "respond 401 if given token exists but user record is missing" do
+    authorize_with :valid_token_deleted_user
+    get :current, {format: :json}
+    assert_response 401
+  end
+
   test "create new user with user as input" do
     authorize_with :admin
     post :create, user: {