diff --git a/dashboard/app/controllers/levels_controller.rb b/dashboard/app/controllers/levels_controller.rb index 06cbdf33806d8..d847b2d4b8834 100644 --- a/dashboard/app/controllers/levels_controller.rb +++ b/dashboard/app/controllers/levels_controller.rb @@ -113,7 +113,7 @@ def update # POST /levels # POST /levels.json def create - authorize! :create, :level + authorize! :create, Level type_class = level_params[:type].constantize # Set some defaults. @@ -153,7 +153,7 @@ def destroy end def new - authorize! :create, :level + authorize! :create, Level if params.key? :type @type_class = params[:type].constantize if @type_class == Artist diff --git a/dashboard/app/models/ability.rb b/dashboard/app/models/ability.rb index d8a36642a450f..342adbdbe38f6 100644 --- a/dashboard/app/models/ability.rb +++ b/dashboard/app/models/ability.rb @@ -171,6 +171,7 @@ def initialize(user) # permissions. if user.persisted? && user.permission?(UserPermission::LEVELBUILDER) can :manage, [ + Game, Level, Script, ScriptLevel @@ -185,10 +186,15 @@ def initialize(user) if user.admin? can :manage, :all - # Only custom levels are editable - cannot [:update, :destroy], Level do |level| - !level.custom? - end + cannot :manage, [ + Activity, + Game, + Level, + Script, + ScriptLevel, + UserLevel, + UserScript + ] end end end diff --git a/dashboard/test/controllers/levels_controller_test.rb b/dashboard/test/controllers/levels_controller_test.rb index c9371ae70c74a..839fb2bae7f02 100644 --- a/dashboard/test/controllers/levels_controller_test.rb +++ b/dashboard/test/controllers/levels_controller_test.rb @@ -8,11 +8,11 @@ class LevelsControllerTest < ActionController::TestCase Level.any_instance.stubs(:write_to_file?).returns(false) # don't write to level files @level = create(:level) - @user = create(:admin) - sign_in(@user) - @program = '' - + @admin = create(:admin) @not_admin = create(:user) + @levelbuilder = create(:levelbuilder) + sign_in(@levelbuilder) + @program = '' end test "should get index" do @@ -48,22 +48,24 @@ class LevelsControllerTest < ActionController::TestCase end test "should alphanumeric order custom levels on new" do - Level.where(user_id: @user.id).map(&:destroy) - level_1 = create(:level, user: @user, name: "BBBB") - level_2 = create(:level, user: @user, name: "AAAA") - level_3 = create(:level, user: @user, name: "Z1") - level_4 = create(:level, user: @user, name: "Z10") - level_5 = create(:level, user: @user, name: "Z2") + Level.where(user_id: @levelbuilder.id).map(&:destroy) + level_1 = create(:level, user: @levelbuilder, name: "BBBB") + level_2 = create(:level, user: @levelbuilder, name: "AAAA") + level_3 = create(:level, user: @levelbuilder, name: "Z1") + level_4 = create(:level, user: @levelbuilder, name: "Z10") + level_5 = create(:level, user: @levelbuilder, name: "Z2") get :new, game_id: @level.game assert_equal [level_2, level_1, level_3, level_5, level_4], assigns(:levels) end - test "should not get builder if not admin" do - sign_in @not_admin - get :new, game_id: @level.game - assert_response :forbidden + test "should not get builder if not levelbuilder" do + [@not_admin, @admin].each do |user| + sign_in user + get :new, game_id: @level.game + assert_response :forbidden + end end test "should create maze level" do @@ -136,7 +138,7 @@ class LevelsControllerTest < ActionController::TestCase assert assigns(:level) assert assigns(:level).game - assert_equal @user, assigns(:level).user + assert_equal @levelbuilder, assigns(:level).user assert_equal edit_level_path(assigns(:level)), JSON.parse(@response.body)["redirect"] end @@ -203,28 +205,32 @@ class LevelsControllerTest < ActionController::TestCase assert_equal level.properties[:toolbox_blocks.to_s], @program end - test "should not update blocks if not admin" do - sign_in @not_admin - post :update_blocks, :level_id => @level.id, :game_id => @level.game.id, :type => 'toolbox_blocks', :program => @program - assert_response :forbidden + test "should not update blocks if not levelbuilder" do + [@not_admin, @admin].each do |user| + sign_in user + post :update_blocks, :level_id => @level.id, :game_id => @level.game.id, :type => 'toolbox_blocks', :program => @program + assert_response :forbidden + end end test "should not edit level if not custom level" do level = Script.twenty_hour_script.levels.first - can_edit = Ability.new(@user).can? :edit, level + can_edit = Ability.new(@levelbuilder).can? :edit, level assert_equal false, can_edit post :update_blocks, :level_id => level.id, :game_id => level.game.id, :type => 'toolbox_blocks', :program => @program assert_response :forbidden end - test "should not create level if not admin" do - sign_in @not_admin - assert_no_difference('Level.count') do - post :create, :name => "NewCustomLevel", :program => @program - end + test "should not create level if not levelbuilder" do + [@not_admin, @admin].each do |user| + sign_in user + assert_no_difference('Level.count') do + post :create, :name => "NewCustomLevel", :program => @program + end - assert_response :forbidden + assert_response :forbidden + end end # This should represent the behavior on production. @@ -415,7 +421,7 @@ class LevelsControllerTest < ActionController::TestCase test 'should hide legacy unplugged pdf download button for students' do level = create :unplugged, name: 'OldUnplugged', type: 'Unplugged' teacher = create(:teacher) - sign_out(@user) + sign_out(@levelbuilder) sign_in(teacher) get :show, id: level, game_id: level.game assert_select '.pdf-button' @@ -439,7 +445,7 @@ class LevelsControllerTest < ActionController::TestCase test 'should hide unplugged pdf download section for students' do level = create :unplugged, name: 'NewUnplugged', type: 'Unplugged' teacher = create(:teacher) - sign_out(@user) + sign_out(@levelbuilder) sign_in(teacher) get :show, id: level, game_id: level.game assert_select '.pdf-button' @@ -520,7 +526,7 @@ class LevelsControllerTest < ActionController::TestCase set_env :test level = create :artist - sign_out @user + sign_out @levelbuilder get :edit, id: level assert_response :redirect @@ -533,7 +539,7 @@ class LevelsControllerTest < ActionController::TestCase set_env :test level = create :artist - sign_out @user + sign_out @levelbuilder get :embed_level, level_id: level assert_response :success @@ -543,7 +549,7 @@ class LevelsControllerTest < ActionController::TestCase set_env :test level = create :artist - sign_out @user + sign_out @levelbuilder get :embed_blocks, level_id: level, block_type: :solution_blocks assert_response :success diff --git a/dashboard/test/controllers/script_levels_controller_test.rb b/dashboard/test/controllers/script_levels_controller_test.rb index 0afbccd8d74ce..76aec7f65a1f5 100644 --- a/dashboard/test/controllers/script_levels_controller_test.rb +++ b/dashboard/test/controllers/script_levels_controller_test.rb @@ -753,23 +753,23 @@ def assert_caching_enabled(cache_control_header, max_age, proxy_max_age) end test 'includes makerlab javascript dependencies when makerlab level' do - sign_in @admin + sign_in @teacher level = create :makerlab script_level = create :script_level, levels: [level] - get :show, script_id: script_level.script, stage_id: script_level.stage, id: script_level.position, user_id: @admin.id + get :show, script_id: script_level.script, stage_id: script_level.stage, id: script_level.position, user_id: @teacher.id assert_select 'script[src=?]', ActionController::Base.helpers.javascript_path('js/makerlab') end test 'excludes makerlab javascript dependencies when applab level' do - sign_in @admin + sign_in @teacher level = create :applab script_level = create :script_level, levels: [level] - get :show, script_id: script_level.script, stage_id: script_level.stage, id: script_level.position, user_id: @admin.id + get :show, script_id: script_level.script, stage_id: script_level.stage, id: script_level.position, user_id: @teacher.id assert_select 'script[src=?]', ActionController::Base.helpers.javascript_path('js/makerlab'), false end @@ -962,12 +962,15 @@ def create_admin_script assert_response :forbidden end - test "should get show of admin script if signed in as admin" do + # TODO(asher): Consolidate the tests when the user is an admin. In particular, + # these scripts should no longer exist, should be accessible to noone, and + # will go away. + test "should not get show of admin script if signed in as admin" do admin_script = create_admin_script sign_in create(:admin) get :show, script_id: admin_script.name, stage_id: 1, id: 1 - assert_response :success + assert_response :forbidden end def create_student_of_admin_script @@ -999,12 +1002,12 @@ def create_student_of_admin_script assert_response :success end - test "should get show of student_of_admin script if signed in as admin" do + test "should not get show of student_of_admin script if signed in as admin" do script = create_student_of_admin_script sign_in create(:admin) get :show, script_id: script.name, stage_id: 1, id: 1 - assert_response :success + assert_response :forbidden end test "should have milestone posting disabled if Milestone is set" do diff --git a/dashboard/test/controllers/scripts_controller_test.rb b/dashboard/test/controllers/scripts_controller_test.rb index e2e2f9958a769..f0993e4d113fd 100644 --- a/dashboard/test/controllers/scripts_controller_test.rb +++ b/dashboard/test/controllers/scripts_controller_test.rb @@ -7,6 +7,7 @@ class ScriptsControllerTest < ActionController::TestCase setup do @admin = create(:admin) @not_admin = create(:user) + @levelbuilder = create(:levelbuilder) Rails.application.config.stubs(:levelbuilder_mode).returns false end @@ -14,7 +15,7 @@ class ScriptsControllerTest < ActionController::TestCase test "should get index" do Rails.application.config.stubs(:levelbuilder_mode).returns true - sign_in(@admin) + sign_in(@levelbuilder) get :index assert_response :success assert_not_nil assigns(:scripts) @@ -27,12 +28,14 @@ class ScriptsControllerTest < ActionController::TestCase assert_redirected_to_sign_in end - test "should not get index if not admin" do - sign_in @not_admin + test "should not get index if not levelbuilder" do + [@admin, @not_admin].each do |user| + sign_in user - get :index + get :index - assert_response :forbidden + assert_response :forbidden + end end test "should get show of hoc" do @@ -125,6 +128,12 @@ class ScriptsControllerTest < ActionController::TestCase assert_response :success end + test 'should not get show if admin' do + sign_in @admin + get :show, id: Script::FLAPPY_NAME + assert_response :forbidden + end + test "should use script name as param where script name is words but looks like a number" do script = create(:script, name: '15-16') get :show, id: "15-16" @@ -153,9 +162,9 @@ class ScriptsControllerTest < ActionController::TestCase end end - test "should not get edit if not levelbuilder" do + test "should not get edit if not levelbuilder mode" do Rails.application.config.stubs(:levelbuilder_mode).returns false - sign_in @admin + sign_in @levelbuilder get :edit, id: 'course1' assert_response :forbidden @@ -168,17 +177,19 @@ class ScriptsControllerTest < ActionController::TestCase assert_redirected_to_sign_in end - test "should not get edit if not admin" do + test "should not get edit if not levelbuilder" do Rails.application.config.stubs(:levelbuilder_mode).returns true - sign_in @not_admin - get :edit, id: 'course1' + [@not_admin, @admin].each do |user| + sign_in user + get :edit, id: 'course1' - assert_response :forbidden + assert_response :forbidden + end end test "edit" do Rails.application.config.stubs(:levelbuilder_mode).returns true - sign_in @admin + sign_in @levelbuilder script = Script.find_by_name('course1') get :edit, id: script.name @@ -200,7 +211,7 @@ class ScriptsControllerTest < ActionController::TestCase end test "edit forbidden if not on levelbuilder" do - sign_in @admin + sign_in @levelbuilder get :edit, id: 'course1' assert_response :forbidden end @@ -224,11 +235,11 @@ def create_admin_script assert_response :forbidden end - test "should get show of admin script if signed in as admin" do + test "should not get show of admin script if signed in as admin" do admin_script = create_admin_script sign_in @admin get :show, id: admin_script.name - assert_response :success + assert_response :forbidden end end diff --git a/dashboard/test/factories.rb b/dashboard/test/factories.rb index 547b079e53b94..706b897ed7f1b 100644 --- a/dashboard/test/factories.rb +++ b/dashboard/test/factories.rb @@ -13,6 +13,13 @@ admin true end + factory :levelbuilder do + after(:create) do |levelbuilder| + levelbuilder.permission = UserPermission::LEVELBUILDER + levelbuilder.save + end + end + factory :teacher do user_type User::TYPE_TEACHER birthday Date.new(1980, 03, 14) diff --git a/dashboard/test/helpers/levels_helper_test.rb b/dashboard/test/helpers/levels_helper_test.rb index 323aa5c4ce35b..6ba57d37f1b18 100644 --- a/dashboard/test/helpers/levels_helper_test.rb +++ b/dashboard/test/helpers/levels_helper_test.rb @@ -311,7 +311,7 @@ def stub_country(code) assert_not can_view_solution? sign_out user - user = create :admin + user = create :levelbuilder sign_in user assert can_view_solution? @@ -333,7 +333,7 @@ def stub_country(code) @script = create(:script) @script_level = create(:script_level, level: @level, script: @script) - user = create :admin + user = create :levelbuilder sign_in user assert can_view_solution? diff --git a/dashboard/test/models/ability_test.rb b/dashboard/test/models/ability_test.rb index 5d1a78888a6ae..7f14886c18c61 100644 --- a/dashboard/test/models/ability_test.rb +++ b/dashboard/test/models/ability_test.rb @@ -103,27 +103,30 @@ class AbilityTest < ActiveSupport::TestCase test "as admin" do ability = Ability.new(create(:admin)) - assert ability.can?(:read, Game) - assert ability.can?(:read, Level) - assert ability.can?(:read, Activity) - - assert ability.can?(:destroy, Game) - # Can only destroy custom levels - assert ability.can?(:destroy, Level.where.not(user_id: nil).first) - assert ability.can?(:destroy, Activity) - - assert ability.can?(:read, Script.find_by_name('ECSPD')) - assert ability.can?(:read, Script.find_by_name('flappy')) - - assert ability.can?(:read, @public_script) - assert ability.can?(:read, @login_required_script) - assert ability.can?(:read, @student_of_admin_script) - assert ability.can?(:read, @admin_script) - - assert ability.can?(:read, @public_script_level) - assert ability.can?(:read, @login_required_script_level) - assert ability.can?(:read, @student_of_admin_script_level) - assert ability.can?(:read, @admin_script_level) + assert ability.cannot?(:read, Activity) + assert ability.cannot?(:read, Game) + assert ability.cannot?(:read, Level) + assert ability.cannot?(:read, Script) + assert ability.cannot?(:read, ScriptLevel) + assert ability.cannot?(:read, UserLevel) + assert ability.cannot?(:read, UserScript) + + assert ability.cannot?(:destroy, Game) + assert ability.cannot?(:destroy, Level) + assert ability.cannot?(:destroy, Activity) + + assert ability.cannot?(:read, Script.find_by_name('ECSPD')) + assert ability.cannot?(:read, Script.find_by_name('flappy')) + + assert ability.cannot?(:read, @public_script) + assert ability.cannot?(:read, @login_required_script) + assert ability.cannot?(:read, @student_of_admin_script) + assert ability.cannot?(:read, @admin_script) + + assert ability.cannot?(:read, @public_script_level) + assert ability.cannot?(:read, @login_required_script_level) + assert ability.cannot?(:read, @student_of_admin_script_level) + assert ability.cannot?(:read, @admin_script_level) end test 'with hint_access manage LevelSourceHint and FrequentUnsuccessfulLevelSource' do @@ -183,6 +186,7 @@ class AbilityTest < ActiveSupport::TestCase user_id: user.id, permission: UserPermission::LEVELBUILDER) ability = Ability.new user + assert ability.can?(:manage, Game) assert ability.can?(:manage, Level) assert ability.can?(:manage, Script) assert ability.can?(:manage, ScriptLevel)