Update tests to match recent permission changes; dry up "404 if no

object found" logic; fix status codes in some error responses.

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 78284c89da0fa3ded93517ba972ff9506354ef44..7ffa38af93188292ec751dfdb67bfd7c35b01740 100644 (file)
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -14,6 +14,9 @@ class ApplicationController < ActionController::Base
                                                   :render_error,
                                                   :render_not_found]
   before_filter :reload_object_before_update, :only => :update
+  before_filter :render_404_if_no_object, except: [:index, :create,
+                                                   :render_error,
+                                                   :render_not_found]
 
   attr_accessor :resource_attrs
 
@@ -26,11 +29,7 @@ class ApplicationController < ActionController::Base
   end
 
   def show
-    if @object
-      render json: @object.as_api_response
-    else
-      render_not_found("object not found")
-    end
+    render json: @object.as_api_response
   end
 
   def create
@@ -43,9 +42,6 @@ class ApplicationController < ActionController::Base
   end
 
   def update
-    if !@object
-      return render_not_found("object not found")
-    end
     attrs_to_update = resource_attrs.reject { |k,v|
       [:kind, :etag, :href].index k
     }
@@ -84,6 +80,10 @@ class ApplicationController < ActionController::Base
     :with => :render_error
   end
 
+  def render_404_if_no_object
+    render_not_found "Object not found" if !@object
+  end
+
   def render_error(e)
     logger.error e.inspect
     logger.error e.backtrace.collect { |x| x + "\n" }.join('') if e.backtrace

diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index 42501d14cd3ff1db2e9d720c719842231ac6fc67..6c45f88e61b1915a955d1e2895c22234965e2657 100644 (file)
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -3,6 +3,7 @@ class Arvados::V1::JobsController < ApplicationController
   accept_attribute_as_json :runtime_constraints, Hash
   accept_attribute_as_json :tasks_summary, Hash
   skip_before_filter :find_object_by_uuid, :only => :queue
+  skip_before_filter :render_404_if_no_object, :only => :queue
 
   def index
     want_ancestor = @where[:script_version_descends_from]

diff --git a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
index cfe657e4511ba6e66f2d49041b593f44fa50534c..7db295dbb2250be51f524969227bd3b7af086fc7 100644 (file)
--- a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
+++ b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
@@ -1,6 +1,5 @@
 class Arvados::V1::KeepDisksController < ApplicationController
   skip_before_filter :require_auth_scope_all, :only => :ping
-  skip_before_filter :find_object_by_uuid, :only => :ping
 
   def self._ping_requires_parameters
     {
@@ -14,27 +13,13 @@ class Arvados::V1::KeepDisksController < ApplicationController
     }
   end
   def ping
-    @object = Node.where(uuid: (params[:id] || params[:uuid])).first
-    if !@object
-      if current_user.andand.is_admin
-        @object = KeepDisk.new(filesystem_uuid: params[:filesystem_uuid])
-        @object.save!
-
-        # In the first ping from this new filesystem_uuid, we can't
-        # expect the keep node to know the ping_secret so we made sure
-        # we got an admin token. Here we add ping_secret to params so
-        # KeepNode.ping() understands this update is properly
-        # authenticated.
-        params[:ping_secret] = @object.ping_secret
-      else
-        return render_not_found "object not found"
-      end
-    end
-
     params[:service_host] ||= request.env['REMOTE_ADDR']
     if not @object.ping params
       return render_not_found "object not found"
     end
+    # Render the :superuser view (i.e., include the ping_secret) even
+    # if !current_user.is_admin. This is safe because @object.ping's
+    # success implies the ping_secret was already known by the client.
     render json: @object.as_api_response(:superuser)
   end
 
@@ -43,4 +28,20 @@ class Arvados::V1::KeepDisksController < ApplicationController
     @objects = model_class.where('1=1')
     super
   end
+
+  def find_object_by_uuid
+    @object = KeepDisk.where(uuid: (params[:id] || params[:uuid])).first
+    if !@object && current_user.andand.is_admin
+      # Create a new KeepDisk and ping it.
+      @object = KeepDisk.new(filesystem_uuid: params[:filesystem_uuid])
+      @object.save!
+
+      # In the first ping from this new filesystem_uuid, we can't
+      # expect the keep node to know the ping_secret so we made sure
+      # we got an admin token. Here we add ping_secret to params so
+      # KeepNode.ping() understands this update is properly
+      # authenticated.
+      params[:ping_secret] = @object.ping_secret
+    end
+  end
 end

diff --git a/services/api/app/controllers/arvados/v1/nodes_controller.rb b/services/api/app/controllers/arvados/v1/nodes_controller.rb
index 8f5b097ccea9e3d5b222ae69c213c2941acea2d5..1461eeccaa1481fc568eb2a0a8d91a8be8b18562 100644 (file)
--- a/services/api/app/controllers/arvados/v1/nodes_controller.rb
+++ b/services/api/app/controllers/arvados/v1/nodes_controller.rb
@@ -1,6 +1,7 @@
 class Arvados::V1::NodesController < ApplicationController
   skip_before_filter :require_auth_scope_all, :only => :ping
   skip_before_filter :find_object_by_uuid, :only => :ping
+  skip_before_filter :render_404_if_no_object, :only => :ping
 
   def create
     @object = Node.new

diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb
index fc3c5768594214f48cc9c4d0223cff5ae3cc4118..1c754888ce911a93d056f4ba04cbd6210b8a7a56 100644 (file)
--- a/services/api/app/controllers/arvados/v1/schema_controller.rb
+++ b/services/api/app/controllers/arvados/v1/schema_controller.rb
@@ -1,5 +1,6 @@
 class Arvados::V1::SchemaController < ApplicationController
   skip_before_filter :find_object_by_uuid
+  skip_before_filter :render_404_if_no_object
   skip_before_filter :require_auth_scope_all
 
   def show

diff --git a/services/api/app/controllers/arvados/v1/user_agreements_controller.rb b/services/api/app/controllers/arvados/v1/user_agreements_controller.rb
index c1b81dda68e5cad79c89539e5b5b0024851a4a02..4ad959e86aae9079554c9f0e24d77c840dea7482 100644 (file)
--- a/services/api/app/controllers/arvados/v1/user_agreements_controller.rb
+++ b/services/api/app/controllers/arvados/v1/user_agreements_controller.rb
@@ -1,6 +1,7 @@
 class Arvados::V1::UserAgreementsController < ApplicationController
   before_filter :admin_required, except: [:index, :sign, :signatures]
-  skip_before_filter :find_object, only: [:sign, :signatures]
+  skip_before_filter :find_object_by_uuid, only: [:sign, :signatures]
+  skip_before_filter :render_404_if_no_object, only: [:sign, :signatures]
 
   def model_class
     Link

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 441db9947e88480f5bc4de3968f889164601302b..133df0f62c17125ead845cbb64331b3cb79290a2 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -1,4 +1,9 @@
 class Arvados::V1::UsersController < ApplicationController
+  skip_before_filter :find_object_by_uuid, only:
+    [:activate, :event_stream, :current, :system]
+  skip_before_filter :render_404_if_no_object, only:
+    [:activate, :event_stream, :current, :system]
+
   def current
     @object = current_user
     show
@@ -75,7 +80,7 @@ class Arvados::V1::UsersController < ApplicationController
         else
           logger.warn "User #{@object.uuid} called users.activate " +
             "before signing agreements #{todo_uuids.inspect}"
-          raise ArgumentError.new \
+          raise ArvadosModel::PermissionDeniedError.new \
           "Cannot activate without user agreements #{todo_uuids.inspect}."
         end
       end

diff --git a/services/api/app/controllers/arvados/v1/virtual_machines_controller.rb b/services/api/app/controllers/arvados/v1/virtual_machines_controller.rb
index 67b693b33718aa1210f93b7d64bd88dc70006103..10b4bd8cc60e2675e714afc321d74183556acc01 100644 (file)
--- a/services/api/app/controllers/arvados/v1/virtual_machines_controller.rb
+++ b/services/api/app/controllers/arvados/v1/virtual_machines_controller.rb
@@ -1,5 +1,6 @@
 class Arvados::V1::VirtualMachinesController < ApplicationController
   skip_before_filter :find_object_by_uuid, :only => :get_all_logins
+  skip_before_filter :render_404_if_no_object, :only => :get_all_logins
   skip_before_filter(:require_auth_scope_all,
                      :only => [:logins, :get_all_logins])
   before_filter(:admin_required,

diff --git a/services/api/app/controllers/static_controller.rb b/services/api/app/controllers/static_controller.rb
index f64e9a41fadabb3b94047f40a6238b6b50861c5b..77654990e08d2dde16f592b1222ef79e19f3a98b 100644 (file)
--- a/services/api/app/controllers/static_controller.rb
+++ b/services/api/app/controllers/static_controller.rb
@@ -1,6 +1,7 @@
 class StaticController < ApplicationController
 
   skip_before_filter :find_object_by_uuid
+  skip_before_filter :render_404_if_no_object
   skip_before_filter :require_auth_scope_all, :only => [ :home, :login_failure ]
 
   def home

diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb
index 3ac47d46cf221bd7bf9b549281892e2f6e9326d7..046da5ca48f8674d6dcba184a6db5c6cc2314913 100644 (file)
--- a/services/api/app/controllers/user_sessions_controller.rb
+++ b/services/api/app/controllers/user_sessions_controller.rb
@@ -2,6 +2,7 @@ class UserSessionsController < ApplicationController
   before_filter :require_auth_scope_all, :only => [ :destroy ]
 
   skip_before_filter :find_object_by_uuid
+  skip_before_filter :render_404_if_no_object
 
   respond_to :html
 

diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index c9b52dcfc7c3337d169fd604413135ec366a7a4b..1a5125a5c55c254779c9baca7b4db13bdc96ea6b 100644 (file)
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -4,6 +4,12 @@ public:
   name: Public
   description: Public Group
 
+private:
+  uuid: zzzzz-j7d0g-rew6elm53kancon
+  owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  name: Private
+  description: Private Group
+
 all_users:
   uuid: zzzzz-j7d0g-fffffffffffffff
   owner_uuid: zzzzz-tpzed-d9tiejq69daie8f

diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index e871c9bcc1b9cd2e70f9ff2e49945971d0f812fc..24b76c61b27dae0210770140342f92dea52ea9c0 100644 (file)
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -14,6 +14,38 @@ user_agreement_required:
   head_uuid: b519d9cb706a29fc7ea24dbea2f05851
   properties: {}
 
+user_agreement_readable:
+  uuid: zzzzz-o0j2j-qpf60gg4fwjlmex
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_kind: arvados#group
+  tail_uuid: zzzzz-j7d0g-fffffffffffffff
+  link_class: permission
+  name: can_read
+  head_kind: arvados#collection
+  head_uuid: b519d9cb706a29fc7ea24dbea2f05851
+  properties: {}
+
+active_user_member_of_all_users_group:
+  uuid: zzzzz-o0j2j-ctbysaduejxfrs5
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_kind: arvados#user
+  tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  link_class: permission
+  name: can_read
+  head_kind: arvados#group
+  head_uuid: zzzzz-j7d0g-fffffffffffffff
+  properties: {}
+
 user_agreement_signed_by_active:
   uuid: zzzzz-o0j2j-4x85a69tqlrud1z
   owner_uuid: zzzzz-tpzed-000000000000000

diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 6530181b68fb9c6a35024d37c88994af2bfe5860..15bdd7ef5b93dcfcd86e44a38a5f324f0d7cd947 100644 (file)
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -2,9 +2,15 @@ require 'test_helper'
 
 class Arvados::V1::GroupsControllerTest < ActionController::TestCase
 
+  test "attempt to delete group without read or write access" do
+    authorize_with :active
+    post :destroy, id: groups(:private).uuid
+    assert_response 404
+  end
+
   test "attempt to delete group without write access" do
     authorize_with :active
-    post :destroy, id: groups(:public).uuid
+    post :destroy, id: groups(:all_users).uuid
     assert_response 403
   end
 

diff --git a/services/api/test/functional/arvados/v1/keep_disks_controller_test.rb b/services/api/test/functional/arvados/v1/keep_disks_controller_test.rb
index 57cbd0024e6d4b5170011d3e8ee75511af275106..3ccfa055e705dd03ff80dc694d798db24fe76efc 100644 (file)
--- a/services/api/test/functional/arvados/v1/keep_disks_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/keep_disks_controller_test.rb
@@ -2,7 +2,7 @@ require 'test_helper'
 
 class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
 
-  test "add keep node with admin token" do
+  test "add keep disk with admin token" do
     authorize_with :admin
     post :ping, {
       ping_secret: '',          # required by discovery doc, but ignored
@@ -13,13 +13,13 @@ class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
     }
     assert_response :success
     assert_not_nil assigns(:object)
-    new_keep_node = JSON.parse(@response.body)
-    assert_not_nil new_keep_node['uuid']
-    assert_not_nil new_keep_node['ping_secret']
-    assert_not_equal '', new_keep_node['ping_secret']
+    new_keep_disk = JSON.parse(@response.body)
+    assert_not_nil new_keep_disk['uuid']
+    assert_not_nil new_keep_disk['ping_secret']
+    assert_not_equal '', new_keep_disk['ping_secret']
   end
 
-  test "add keep node with no filesystem_uuid" do
+  test "add keep disk with no filesystem_uuid" do
     authorize_with :admin
     opts = {
       ping_secret: '',
@@ -36,7 +36,7 @@ class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
     assert_not_nil JSON.parse(@response.body)['uuid']
   end
 
-  test "refuse to add keep node without admin token" do
+  test "refuse to add keep disk without admin token" do
     post :ping, {
       ping_secret: '',
       service_host: '::1',
@@ -46,7 +46,7 @@ class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
     assert_response 404
   end
 
-  test "ping from keep node" do
+  test "ping keep disk" do
     post :ping, {
       uuid: keep_disks(:nonfull).uuid,
       ping_secret: keep_disks(:nonfull).ping_secret,
@@ -54,12 +54,12 @@ class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
     }
     assert_response :success
     assert_not_nil assigns(:object)
-    keep_node = JSON.parse(@response.body)
-    assert_not_nil keep_node['uuid']
-    assert_not_nil keep_node['ping_secret']
+    keep_disk = JSON.parse(@response.body)
+    assert_not_nil keep_disk['uuid']
+    assert_not_nil keep_disk['ping_secret']
   end
 
-  test "should get index with ping_secret" do
+  test "admin should get index with ping_secret" do
     authorize_with :admin
     get :index
     assert_response :success
@@ -69,13 +69,13 @@ class Arvados::V1::KeepDisksControllerTest < ActionController::TestCase
     assert_not_nil items[0]['ping_secret']
   end
 
-  # inactive user does not see any keep disks
-  test "inactive user should get empty index" do
+  # inactive user sees keep disks
+  test "inactive user should get index" do
     authorize_with :inactive
     get :index
     assert_response :success
     items = JSON.parse(@response.body)['items']
-    assert_equal 0, items.size
+    assert_not_equal 0, items.size
   end
 
   # active user sees non-secret attributes of keep disks

diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 4b52c9b31a56d4501f5a07ccb3917c097be081c8..6d129d898fc370d0f8b02d0276f7ecfbcf5fd082 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -20,7 +20,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
     assert_response :success
     me = JSON.parse(@response.body)
     post :activate, uuid: me['uuid']
-    assert_response 422
+    assert_response 403
     get :current
     assert_response :success
     me = JSON.parse(@response.body)