[ARVADOS] created: 7067c9a96ac36a7bb28d9efc45c70167d8761676
git at public.curoverse.com
git at public.curoverse.com
Wed Jun 25 16:42:24 EDT 2014
at 7067c9a96ac36a7bb28d9efc45c70167d8761676 (commit)
commit 7067c9a96ac36a7bb28d9efc45c70167d8761676
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jun 25 16:39:24 2014 -0400
2891: Guard against API server errors in Workbench layout.
The goal here is to generally avoid situations like #3031 where
trouble talking to the API server prevents us from even rendering an
error page. Previous commits made us smarter about logged in status.
This guards against other API errors.
diff --git a/apps/workbench/app/views/layouts/application.html.erb b/apps/workbench/app/views/layouts/application.html.erb
index 45e1ae2..cccef64 100644
--- a/apps/workbench/app/views/layouts/application.html.erb
+++ b/apps/workbench/app/views/layouts/application.html.erb
@@ -156,6 +156,7 @@
</nav>
<% if current_user.andand.is_active %>
+ <% begin %>
<nav class="navbar navbar-default breadcrumbs" role="navigation">
<ul class="nav navbar-nav navbar-left">
<li class="dropdown">
@@ -180,6 +181,10 @@
<% end %>
</ul>
</nav>
+ <% rescue ArvadosApiClient::NotLoggedInException %>
+ <% raise # Let ApplicationController handle this. %>
+ <% rescue ArvadosApiClient::ApiError # Just skip rendering projects. %>
+ <% end %>
<% end %>
<div id="page-wrapper">
commit e18593e917c4031f59d4ca6c6dc3639e89b95b73
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jun 25 16:31:57 2014 -0400
Clean redundant conditional from Workbench layout.
diff --git a/apps/workbench/app/views/layouts/application.html.erb b/apps/workbench/app/views/layouts/application.html.erb
index 23e0715..45e1ae2 100644
--- a/apps/workbench/app/views/layouts/application.html.erb
+++ b/apps/workbench/app/views/layouts/application.html.erb
@@ -74,7 +74,6 @@
</li>
-->
- <% if current_user %>
<li class="dropdown notification-menu">
<a href="#" class="dropdown-toggle" data-toggle="dropdown" id="notifications-menu">
<span class="badge badge-alert notification-count"><%= @notification_count %></span>
@@ -97,7 +96,6 @@
<% end %>
</ul>
</li>
- <% end %>
<li class="dropdown selection-menu">
<a href="#" class="dropdown-toggle" data-toggle="dropdown">
commit 0b8cc6ed510fd0d2b12b4b70ed6d971df8038f1c
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jun 25 16:07:27 2014 -0400
2891: Workbench displays more info about API errors.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 27fac6f..533163a 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -50,17 +50,22 @@ class ApplicationController < ActionController::Base
def render_exception(e)
logger.error e.inspect
logger.error e.backtrace.collect { |x| x + "\n" }.join('') if e.backtrace
- if @object.andand.errors.andand.full_messages.andand.any?
+ err_opts = {status: 422}
+ if e.is_a?(ArvadosApiClient::ApiError)
+ err_opts.merge!(action: 'api_error', locals: {api_error: e})
+ @errors = e.api_response[:errors]
+ elsif @object.andand.errors.andand.full_messages.andand.any?
@errors = @object.errors.full_messages
else
@errors = [e.to_s]
end
if e.is_a? ArvadosApiClient::NotLoggedInException
- self.render_error status: 422
+ prep_token = :thread_clear
else
- set_thread_api_token do
- self.render_error status: 422
- end
+ prep_token = :set_thread_api_token
+ end
+ send(prep_token) do
+ render_error(err_opts)
end
end
diff --git a/apps/workbench/app/views/application/api_error.html.erb b/apps/workbench/app/views/application/api_error.html.erb
new file mode 100644
index 0000000..10ffbb7
--- /dev/null
+++ b/apps/workbench/app/views/application/api_error.html.erb
@@ -0,0 +1,23 @@
+<h2>Oh... fiddlesticks.</h2>
+
+<p>An error occurred when Workbench sent a request to the Arvados API server. Try reloading this page. If the problem is temporary, your request might go through next time.
+
+<% if not api_error %>
+</p>
+<% else %>
+If that doesn't work, the information below can help system administrators track down the problem.
+</p>
+
+<dl>
+ <dt>API request URL</dt>
+ <dd><code><%= api_error.request_url %></code></dd>
+
+ <% if api_error.api_response.empty? %>
+ <dt>Invalid API response</dt>
+ <dd><%= api_error.api_response_s %></dd>
+ <% else %>
+ <dt>API response</dt>
+ <dd><pre><%= Oj.dump(api_error.api_response, indent: 2) %></pre></dd>
+ <% end %>
+</dl>
+<% end %>
diff --git a/apps/workbench/app/views/application/api_error.json.erb b/apps/workbench/app/views/application/api_error.json.erb
new file mode 100644
index 0000000..8371ff9
--- /dev/null
+++ b/apps/workbench/app/views/application/api_error.json.erb
@@ -0,0 +1 @@
+{"errors":<%= raw @errors.to_json %>}
\ No newline at end of file
diff --git a/apps/workbench/test/integration/errors_test.rb b/apps/workbench/test/integration/errors_test.rb
index 26c8783..f6a34bc 100644
--- a/apps/workbench/test/integration/errors_test.rb
+++ b/apps/workbench/test/integration/errors_test.rb
@@ -30,4 +30,27 @@ class ErrorsTest < ActionDispatch::IntegrationTest
assert(all("a").any? { |a| a[:href] =~ %r{/collections/?(\?|$)} },
"no search link found on 404 page")
end
+
+ def now_timestamp
+ Time.now.utc.to_i
+ end
+
+ def page_has_error_token?(start_stamp)
+ matching_stamps = (start_stamp .. now_timestamp).to_a.join("|")
+ # Check the page HTML because we really don't care how it's presented.
+ # I think it would even be reasonable to put it in a comment.
+ page.html =~ /\b(#{matching_stamps})\+[0-9A-Fa-f]{8}\b/
+ end
+
+ # We use API tokens with limited scopes as the quickest way to get the API
+ # server to return an error. If Workbench gets smarter about coping when
+ # it has a too-limited token, these tests will need to be adjusted.
+ test "API error page includes error token" do
+ start_stamp = now_timestamp
+ visit(page_with_token("active_readonly", "/authorized_keys"))
+ click_on "Add a new authorized key"
+ assert(page.has_text?(/fiddlesticks/i),
+ "Not on an error page after making an SSH key out of scope")
+ assert(page_has_error_token?(start_stamp), "no error token on 404 page")
+ end
end
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index 71b6388..383e97c 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -87,6 +87,13 @@ active_apitokens:
scopes: ["GET /arvados/v1/api_client_authorizations",
"POST /arvados/v1/api_client_authorizations"]
+active_readonly:
+ api_client: untrusted
+ user: active
+ api_token: activereadonlyabcdefghijklmnopqrstuvwxyz1234568790
+ expires_at: 2038-01-01 00:00:00
+ scopes: ["GET /"]
+
spectator:
api_client: untrusted
user: spectator
commit 34f60c96240605b77ef4ebe3fd066fa9ac416acb
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jun 25 14:11:10 2014 -0400
2891: Add Workbench test for expired API token.
diff --git a/apps/workbench/test/integration/errors_test.rb b/apps/workbench/test/integration/errors_test.rb
index 12c18f4..26c8783 100644
--- a/apps/workbench/test/integration/errors_test.rb
+++ b/apps/workbench/test/integration/errors_test.rb
@@ -11,6 +11,14 @@ class ErrorsTest < ActionDispatch::IntegrationTest
"Logged in user prompted to log in on error page")
end
+ test "no user navigation with expired token" do
+ visit(page_with_token("expired", "/collections/#{BAD_UUID}"))
+ assert(page.has_no_text?(api_fixture("users")["active"]["email"]),
+ "Page visited with expired token included user information")
+ assert(page.has_selector?("a", text: /log ?in/i),
+ "Login prompt missing on expired token 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/),
commit 57d48418b4d3f333f8acae2cfd4afa47cebf9042
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 17:31:57 2014 -0400
2891: Add index link to Workbench 404 responses.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index d746bd9..27fac6f 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -35,15 +35,15 @@ class ApplicationController < ActionController::Base
render_error status: 422
end
- def render_error(opts)
- opts = {status: 500}.merge opts
+ def render_error(opts={})
+ opts[:status] ||= 500
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
# ajax:error callback parse the response correctly, even though
# the browser can't.
f.json { render opts.merge(json: {success: false, errors: @errors}) }
- f.html { render opts.merge(controller: 'application', action: 'error') }
+ f.html { render({action: 'error'}.merge(opts)) }
end
end
@@ -68,7 +68,7 @@ class ApplicationController < ActionController::Base
logger.error e.inspect
@errors = ["Path not found"]
set_thread_api_token do
- self.render_error status: 404
+ self.render_error(action: '404', status: 404)
end
end
diff --git a/apps/workbench/app/views/application/404.html.erb b/apps/workbench/app/views/application/404.html.erb
new file mode 100644
index 0000000..658271f
--- /dev/null
+++ b/apps/workbench/app/views/application/404.html.erb
@@ -0,0 +1,12 @@
+<h2>Not Found</h2>
+
+<p>The item you requested was not found.
+
+<% if params[:uuid] and (model_class = resource_class_for_uuid(params[:uuid]))
+ then class_name = model_class.to_s.pluralize.downcase %>
+Perhaps you'd like to
+<%= link_to("browse all #{class_name}", send("#{class_name}_path".to_sym)) %>?
+<% end %>
+
+</p>
+
diff --git a/apps/workbench/app/views/application/404.json.erb b/apps/workbench/app/views/application/404.json.erb
new file mode 100644
index 0000000..8371ff9
--- /dev/null
+++ b/apps/workbench/app/views/application/404.json.erb
@@ -0,0 +1 @@
+{"errors":<%= raw @errors.to_json %>}
\ No newline at end of file
diff --git a/apps/workbench/test/integration/errors_test.rb b/apps/workbench/test/integration/errors_test.rb
index 8da6de1..12c18f4 100644
--- a/apps/workbench/test/integration/errors_test.rb
+++ b/apps/workbench/test/integration/errors_test.rb
@@ -1,7 +1,7 @@
require 'integration_helper'
class ErrorsTest < ActionDispatch::IntegrationTest
- BAD_UUID = "zzzzz-zzzzz-zzzzzzzzzzzzzzz"
+ BAD_UUID = "ffffffffffffffffffffffffffffffff+0"
test "error page renders user navigation" do
visit(page_with_token("active", "/collections/#{BAD_UUID}"))
@@ -16,4 +16,10 @@ class ErrorsTest < ActionDispatch::IntegrationTest
assert(page.has_no_text?(/\b500\b/),
"Error page without login returned 500")
end
+
+ test "'object not found' page includes search link" do
+ visit(page_with_token("active", "/collections/#{BAD_UUID}"))
+ assert(all("a").any? { |a| a[:href] =~ %r{/collections/?(\?|$)} },
+ "no search link found on 404 page")
+ end
end
commit 947e48e6c58515d99a94be264fa88ce2854f5557
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 15:53:18 2014 -0400
2891: Workbench returns 404 when API object doesn't exist.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index d0496cb..d746bd9 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -344,22 +344,22 @@ class ApplicationController < ActionController::Base
if params[:id] and params[:id].match /\D/
params[:uuid] = params.delete :id
end
- if not model_class
- @object = nil
- elsif params[:uuid].is_a? String
- if params[:uuid].empty?
+ begin
+ if not model_class
@object = nil
+ elsif not params[:uuid].is_a?(String)
+ @object = model_class.where(uuid: params[:uuid]).first
+ elsif params[:uuid].empty?
+ @object = nil
+ elsif (model_class != Link and
+ resource_class_for_uuid(params[:uuid]) == Link)
+ @name_link = Link.find(params[:uuid])
+ @object = model_class.find(@name_link.head_uuid)
else
- if (model_class != Link and
- resource_class_for_uuid(params[:uuid]) == Link)
- @name_link = Link.find(params[:uuid])
- @object = model_class.find(@name_link.head_uuid)
- else
- @object = model_class.find(params[:uuid])
- end
+ @object = model_class.find(params[:uuid])
end
- else
- @object = model_class.where(uuid: params[:uuid]).first
+ rescue ArvadosApiClient::NotFoundException => error
+ @object = nil
end
end
diff --git a/apps/workbench/test/functional/application_controller_test.rb b/apps/workbench/test/functional/application_controller_test.rb
index f3dbcb5..50f990a 100644
--- a/apps/workbench/test/functional/application_controller_test.rb
+++ b/apps/workbench/test/functional/application_controller_test.rb
@@ -296,4 +296,11 @@ class ApplicationControllerTest < ActionController::TestCase
assert users.size == 3, 'Expected two objects in the preloaded hash'
end
+ test "requesting a nonexistent object returns 404" do
+ # We're really testing ApplicationController's find_object_by_uuid.
+ # It's easiest to do that by instantiating a concrete controller.
+ @controller = NodesController.new
+ get(:show, {id: "zzzzz-zzzzz-zzzzzzzzzzzzzzz"}, session_for(:admin))
+ assert_response 404
+ end
end
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index babe4fb..4745d1b 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -1,6 +1,8 @@
require 'test_helper'
class CollectionsControllerTest < ActionController::TestCase
+ NONEXISTENT_COLLECTION = "ffffffffffffffffffffffffffffffff+0"
+
def collection_params(collection_name, file_name=nil)
uuid = api_fixture('collections')[collection_name.to_s]['uuid']
params = {uuid: uuid, id: uuid}
@@ -173,4 +175,9 @@ class CollectionsControllerTest < ActionController::TestCase
"when showing the user agreement.")
assert_response :success
end
+
+ test "requesting nonexistent Collection returns 404" do
+ show_collection({uuid: NONEXISTENT_COLLECTION, id: NONEXISTENT_COLLECTION},
+ :active, 404)
+ end
end
commit 7610ed3457bac39bada7183d7a2bd14752d2f293
Author: Brett Smith <brett at curoverse.com>
Date: Mon Jun 23 15:28:48 2014 -0400
2891: Workbench API client raises structured exceptions.
These changes will make it easier for the rest of the Workbench code
to cope with these errors more gracefully—even if that just means
friendlier reporting of the error.
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 95aee92..7556f3c 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -242,14 +242,10 @@ class CollectionsController < ApplicationController
begin
yield
return api_token
- rescue ArvadosApiClient::NotLoggedInException => error
- status = 401
- rescue => error
- status = (error.message =~ /\[API: (\d+)\]$/) ? $1.to_i : nil
- raise unless [401, 403, 404].include?(status)
- end
- if status >= most_specific_error.first
- most_specific_error = [status, error]
+ rescue ArvadosApiClient::ApiError => error
+ if error.api_status >= most_specific_error.first
+ most_specific_error = [error.api_status, error]
+ end
end
end
end
diff --git a/apps/workbench/app/helpers/application_helper.rb b/apps/workbench/app/helpers/application_helper.rb
index 66267e0..2314c41 100644
--- a/apps/workbench/app/helpers/application_helper.rb
+++ b/apps/workbench/app/helpers/application_helper.rb
@@ -100,7 +100,7 @@ module ApplicationHelper
else
link_name = object_for_dataclass(resource_class, link_uuid).andand.friendly_link_name
end
- rescue RuntimeError
+ rescue ArvadosApiClient::NotFoundException
# If that lookup failed, the link will too. So don't make one.
return attrvalue
end
@@ -392,7 +392,7 @@ module ApplicationHelper
render opts.merge(partial: "application/#{partial}")
end
end
-
+
def fa_icon_class_for_object object
case object.class.to_s.to_sym
when :User
diff --git a/apps/workbench/app/models/arvados_api_client.rb b/apps/workbench/app/models/arvados_api_client.rb
index 25c54d1..a079f56 100644
--- a/apps/workbench/app/models/arvados_api_client.rb
+++ b/apps/workbench/app/models/arvados_api_client.rb
@@ -2,13 +2,53 @@ require 'httpclient'
require 'thread'
class ArvadosApiClient
- class NotLoggedInException < StandardError
- end
- class InvalidApiResponseException < StandardError
+ class ApiError < StandardError
+ attr_reader :api_response, :api_response_s, :api_status, :request_url
+
+ def initialize(request_url, api_response)
+ @request_url = request_url
+ @api_status = api_response.status_code
+ @api_response_s = api_response.content
+ @api_response = parse_response
+ super("#{error_message} [API: #{@api_status}]")
+ end
+
+ protected
+
+ def parse_response
+ Oj.load(@api_response_s, :symbol_keys => true)
+ end
+
+ def error_message
+ errors = @api_response[:errors]
+ if errors.respond_to?(:join)
+ errors.join("\n\n")
+ else
+ errors.to_s
+ end
+ end
end
- class AccessForbiddenException < StandardError
+
+ class InvalidApiResponseException < ApiError
+ def parse_response
+ {} # We already know it's not parseable.
+ end
+
+ def error_message
+ "Unparseable response from API server"
+ end
end
+ class AccessForbiddenException < ApiError; end
+ class NotFoundException < ApiError; end
+ class NotLoggedInException < ApiError; end
+
+ ERROR_CODE_CLASSES = {
+ 401 => NotLoggedInException,
+ 403 => AccessForbiddenException,
+ 404 => NotFoundException,
+ }
+
@@profiling_enabled = Rails.configuration.profiling_enabled
@@discovery = nil
@@ -84,29 +124,18 @@ class ArvadosApiClient
end
profile_checkpoint 'API transaction'
- if msg.status_code == 401
- raise NotLoggedInException.new
- end
-
- json = msg.content
-
begin
- resp = Oj.load(json, :symbol_keys => true)
+ resp = Oj.load(msg.content, :symbol_keys => true)
rescue Oj::ParseError
- raise InvalidApiResponseException.new json
+ resp = nil
end
if not resp.is_a? Hash
- raise InvalidApiResponseException.new json
- end
- if msg.status_code != 200
- errors = resp[:errors]
- errors = errors.join("\n\n") if errors.is_a? Array
- if msg.status_code == 403
- raise AccessForbiddenException.new "#{errors} [API: #{msg.status_code}]"
- else
- raise "#{errors} [API: #{msg.status_code}]"
- end
+ raise InvalidApiResponseException.new(url, msg)
+ elsif msg.status_code != 200
+ error_class = ERROR_CODE_CLASSES.fetch(msg.status_code, ApiError)
+ raise error_class.new(url, msg)
end
+
if resp[:_profile]
Rails.logger.info "API client: " \
"#{resp.delete(:_profile)[:request_time]} request_time"
commit 495c923dedc737b44e032925d8df9a2c2d0c0773
Author: Brett Smith <brett at curoverse.com>
Date: Tue Jun 24 11:38:50 2014 -0400
2891: API server assigns error tokens.
These unique tokens are both logged and sent along with the response,
making it easier to cross-reference what the client does with what
happens on the API server.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 51b0c3c..7e5b316 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -110,16 +110,29 @@ class ApplicationController < ActionController::Base
errors = [e.inspect]
end
status = e.respond_to?(:http_status) ? e.http_status : 422
- render json: { errors: errors }, status: status
+ send_error(*errors, status: status)
end
def render_not_found(e=ActionController::RoutingError.new("Path not found"))
logger.error e.inspect
- render json: { errors: ["Path not found"] }, status: 404
+ send_error("Path not found", status: 404)
end
protected
+ def send_error(*args)
+ if args.last.is_a? Hash
+ err = args.pop
+ else
+ err = {}
+ end
+ err[:errors] ||= args
+ err[:error_token] = [Time.now.utc.to_i, "%08x" % rand(16 ** 8)].join("+")
+ status = err.delete(:status) || 422
+ logger.error "Error #{err[:error_token]}: #{status}"
+ render json: err, status: status
+ end
+
def find_objects_for_index
@objects ||= model_class.readable_by(*@read_users)
apply_where_limit_order_params
@@ -242,12 +255,8 @@ class ApplicationController < ActionController::Base
def require_login
if not current_user
respond_to do |format|
- format.json {
- render :json => { errors: ['Not logged in'] }.to_json, status: 401
- }
- format.html {
- redirect_to '/auth/joshid'
- }
+ format.json { send_error("Not logged in", status: 401) }
+ format.html { redirect_to '/auth/joshid' }
end
false
end
@@ -255,14 +264,14 @@ class ApplicationController < ActionController::Base
def admin_required
unless current_user and current_user.is_admin
- render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ send_error("Forbidden", status: 403)
end
end
def require_auth_scope
if @read_auths.empty?
if require_login != false
- render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ send_error("Forbidden", status: 403)
end
false
end
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 76a228d..f365a7f 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -82,7 +82,8 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
def current_api_client_is_trusted
unless Thread.current[:api_client].andand.is_trusted
- render :json => { errors: ['Forbidden: this API client cannot manipulate other clients\' access tokens.'] }.to_json, status: 403
+ send_error('Forbidden: this API client cannot manipulate other clients\' access tokens.',
+ status: 403)
end
end
end
diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index d38f257..feeb82d 100644
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -8,9 +8,8 @@ class Arvados::V1::JobsController < ApplicationController
def create
[:repository, :script, :script_version, :script_parameters].each do |r|
if !resource_attrs[r]
- return render json: {
- :errors => ["#{r} attribute must be specified"]
- }, status: :unprocessable_entity
+ return send_error("#{r} attribute must be specified",
+ status: :unprocessable_entity)
end
end
diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index 188ecfc..0772227 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -2,7 +2,8 @@ class Arvados::V1::LinksController < ApplicationController
def check_uuid_kind uuid, kind
if kind and ArvadosModel::resource_class_for_uuid(uuid).andand.kind != kind
- render :json => { errors: ["'#{kind}' does not match uuid '#{uuid}', expected '#{ArvadosModel::resource_class_for_uuid(uuid).andand.kind}'"] }.to_json, status: 422
+ send_error("'#{kind}' does not match uuid '#{uuid}', expected '#{ArvadosModel::resource_class_for_uuid(uuid).andand.kind}'",
+ status: 422)
nil
else
true
diff --git a/services/api/test/functional/application_controller_test.rb b/services/api/test/functional/application_controller_test.rb
new file mode 100644
index 0000000..4144d0a
--- /dev/null
+++ b/services/api/test/functional/application_controller_test.rb
@@ -0,0 +1,49 @@
+require 'test_helper'
+
+class ApplicationControllerTest < ActionController::TestCase
+ BAD_UUID = "zzzzz-zzzzz-zzzzzzzzzzzzzzz"
+
+ def now_timestamp
+ Time.now.utc.to_i
+ end
+
+ setup do
+ # These tests are meant to check behavior in ApplicationController.
+ # We instantiate a small concrete controller for convenience.
+ @controller = Arvados::V1::SpecimensController.new
+ @start_stamp = now_timestamp
+ end
+
+ def check_error_token
+ token = json_response['error_token']
+ assert_not_nil token
+ token_time = token.split('+', 2).first.to_i
+ assert_operator(token_time, :>=, @start_stamp, "error token too old")
+ assert_operator(token_time, :<=, now_timestamp, "error token too new")
+ end
+
+ def check_404(errmsg="Path not found")
+ assert_response 404
+ assert_equal([errmsg], json_response['errors'])
+ check_error_token
+ end
+
+ test "requesting nonexistent object returns 404 error" do
+ authorize_with :admin
+ get(:show, id: BAD_UUID)
+ check_404
+ end
+
+ test "requesting object without read permission returns 404 error" do
+ authorize_with :spectator
+ get(:show, id: specimens(:owned_by_active_user).uuid)
+ check_404
+ end
+
+ test "submitting bad object returns error" do
+ authorize_with :spectator
+ post(:create, specimen: {badattr: "badvalue"})
+ assert_response 422
+ check_error_token
+ end
+end
commit ff7251f5923d4703f9d0c482c6d0de434599224f
Author: Brett Smith <brett at curoverse.com>
Date: Tue Jun 24 10:22:25 2014 -0400
2891: API server ApplicationController style updates.
This ports over some of the niceties we've recently implemented in the
Workbench ApplicationController.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index fe5598e..51b0c3c 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -20,12 +20,11 @@ class ApplicationController < ActionController::Base
include LoadParam
include RecordFilters
- ERROR_ACTIONS = [:render_error, :render_not_found]
-
-
respond_to :json
protect_from_forgery
+ ERROR_ACTIONS = [:render_error, :render_not_found]
+
before_filter :respond_with_json_by_default
before_filter :remote_ip
before_filter :load_read_auths
@@ -46,6 +45,17 @@ class ApplicationController < ActionController::Base
attr_accessor :resource_attrs
+ begin
+ rescue_from(Exception,
+ ArvadosModel::PermissionDeniedError,
+ :with => :render_error)
+ rescue_from(ActiveRecord::RecordNotFound,
+ ActionController::RoutingError,
+ ActionController::UnknownController,
+ AbstractController::ActionNotFound,
+ :with => :render_not_found)
+ end
+
def index
@objects.uniq!(&:id) if @select.nil? or @select.include? "id"
if params[:eager] and params[:eager] != '0' and params[:eager] != 0 and params[:eager] != ''
@@ -85,21 +95,6 @@ class ApplicationController < ActionController::Base
end
end
- begin
- rescue_from Exception,
- :with => :render_error
- rescue_from ActiveRecord::RecordNotFound,
- :with => :render_not_found
- rescue_from ActionController::RoutingError,
- :with => :render_not_found
- rescue_from ActionController::UnknownController,
- :with => :render_not_found
- rescue_from AbstractController::ActionNotFound,
- :with => :render_not_found
- rescue_from ArvadosModel::PermissionDeniedError,
- :with => :render_error
- end
-
def render_404_if_no_object
render_not_found "Object not found" if !@object
end
commit 72ee1c20b4f563fb580dd49e21e44465e50e1aa8
Author: Brett Smith <brett at curoverse.com>
Date: Wed Jun 25 13:47:13 2014 -0400
2891: Fix bug in Workbench error page test.
diff --git a/apps/workbench/test/integration/errors_test.rb b/apps/workbench/test/integration/errors_test.rb
index 092041d..8da6de1 100644
--- a/apps/workbench/test/integration/errors_test.rb
+++ b/apps/workbench/test/integration/errors_test.rb
@@ -5,7 +5,7 @@ class ErrorsTest < ActionDispatch::IntegrationTest
test "error page renders user navigation" do
visit(page_with_token("active", "/collections/#{BAD_UUID}"))
- assert(page.has_text?(@@API_AUTHS["active"]["email"]),
+ assert(page.has_text?(api_fixture("users")["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")
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list