From 5cbac8783c8d6cf3024103a2b357cc087f6ba2f8 Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Fri, 24 Jun 2016 08:32:17 -0500
Subject: [PATCH 1/6] Removes abilities from admins.

---
 dashboard/app/models/ability.rb       | 14 +++++---
 dashboard/test/models/ability_test.rb | 46 +++++++++++++++------------
 2 files changed, 35 insertions(+), 25 deletions(-)

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/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)

From b1df7ca1041f3abc03bad070a804c7b9d9aff90d Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Fri, 24 Jun 2016 10:57:04 -0500
Subject: [PATCH 2/6] Fix tests with fallout.

---
 .../controllers/levels_controller_test.rb     | 65 ++++++++++---------
 .../script_levels_controller_test.rb          | 16 ++---
 .../controllers/scripts_controller_test.rb    | 41 +++++++-----
 dashboard/test/factories.rb                   |  7 ++
 dashboard/test/helpers/levels_helper_test.rb  |  4 +-
 5 files changed, 79 insertions(+), 54 deletions(-)

diff --git a/dashboard/test/controllers/levels_controller_test.rb b/dashboard/test/controllers/levels_controller_test.rb
index c9371ae70c74a..ccd17bb73e1e9 100644
--- a/dashboard/test/controllers/levels_controller_test.rb
+++ b/dashboard/test/controllers/levels_controller_test.rb
@@ -8,8 +8,9 @@ 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)
+    @admin = create(:admin)
+    @levelbuilder = create(:levelbuilder)
+    sign_in(@levelbuilder)
     @program = '<hey/>'
 
     @not_admin = create(:user)
@@ -48,22 +49,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 +139,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 +206,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 +422,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 +446,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 +527,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 +540,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 +550,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..a0329c61c3cc7 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,12 @@ 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 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 +999,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?
 

From 497abc4e3a5850f6a497b8a5be78c6b7e4998ca9 Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Mon, 27 Jun 2016 14:38:04 -0500
Subject: [PATCH 3/6] Trial and error.

---
 dashboard/app/models/ability.rb                      | 12 +++++++++++-
 dashboard/test/controllers/levels_controller_test.rb |  3 +--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/dashboard/app/models/ability.rb b/dashboard/app/models/ability.rb
index 342adbdbe38f6..05e6f4c07b460 100644
--- a/dashboard/app/models/ability.rb
+++ b/dashboard/app/models/ability.rb
@@ -171,10 +171,20 @@ def initialize(user)
     # permissions.
     if user.persisted? && user.permission?(UserPermission::LEVELBUILDER)
       can :manage, [
+        Applab,
+        Artist,
+        Calc,
+        Craft,
+        DSLDefined,
+        Eval,
         Game,
+        Gamelab,
         Level,
+        Maze,
+        NetSim,
         Script,
-        ScriptLevel
+        ScriptLevel,
+        Studio,
       ]
 
       # Only custom levels are editable.
diff --git a/dashboard/test/controllers/levels_controller_test.rb b/dashboard/test/controllers/levels_controller_test.rb
index ccd17bb73e1e9..839fb2bae7f02 100644
--- a/dashboard/test/controllers/levels_controller_test.rb
+++ b/dashboard/test/controllers/levels_controller_test.rb
@@ -9,11 +9,10 @@ class LevelsControllerTest < ActionController::TestCase
 
     @level = create(:level)
     @admin = create(:admin)
+    @not_admin = create(:user)
     @levelbuilder = create(:levelbuilder)
     sign_in(@levelbuilder)
     @program = '<hey/>'
-
-    @not_admin = create(:user)
   end
 
   test "should get index" do

From 384d2845d0b07234ac9605101b768d5346feae1d Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Mon, 27 Jun 2016 16:11:02 -0500
Subject: [PATCH 4/6] Change :level to Level, resolving existing bug and
 current woes.

---
 dashboard/app/controllers/levels_controller.rb |  4 ++--
 dashboard/app/models/ability.rb                | 10 ----------
 2 files changed, 2 insertions(+), 12 deletions(-)

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 05e6f4c07b460..8bb3f7a8c10a5 100644
--- a/dashboard/app/models/ability.rb
+++ b/dashboard/app/models/ability.rb
@@ -171,20 +171,10 @@ def initialize(user)
     # permissions.
     if user.persisted? && user.permission?(UserPermission::LEVELBUILDER)
       can :manage, [
-        Applab,
-        Artist,
-        Calc,
-        Craft,
-        DSLDefined,
-        Eval,
         Game,
-        Gamelab,
         Level,
-        Maze,
-        NetSim,
         Script,
         ScriptLevel,
-        Studio,
       ]
 
       # Only custom levels are editable.

From 1a764dd1f93b44241eaa75609e54e82ed0470cc5 Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Tue, 28 Jun 2016 10:32:17 -0500
Subject: [PATCH 5/6] PR feedback (remove trailing comma, adds TODO).

---
 dashboard/app/models/ability.rb                             | 2 +-
 dashboard/test/controllers/script_levels_controller_test.rb | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/dashboard/app/models/ability.rb b/dashboard/app/models/ability.rb
index 8bb3f7a8c10a5..342adbdbe38f6 100644
--- a/dashboard/app/models/ability.rb
+++ b/dashboard/app/models/ability.rb
@@ -174,7 +174,7 @@ def initialize(user)
         Game,
         Level,
         Script,
-        ScriptLevel,
+        ScriptLevel
       ]
 
       # Only custom levels are editable.
diff --git a/dashboard/test/controllers/script_levels_controller_test.rb b/dashboard/test/controllers/script_levels_controller_test.rb
index a0329c61c3cc7..63c3aadbddc3b 100644
--- a/dashboard/test/controllers/script_levels_controller_test.rb
+++ b/dashboard/test/controllers/script_levels_controller_test.rb
@@ -962,6 +962,7 @@ def create_admin_script
     assert_response :forbidden
   end
 
+  # TODO(asher): Consolidate the tests when the user is an admin.
   test "should not get show of admin script if signed in as admin" do
     admin_script = create_admin_script
 

From c4044fd0b20d29170ec2060936c4aa7e3c5f2644 Mon Sep 17 00:00:00 2001
From: Asher Kach <asher@code.org>
Date: Tue, 28 Jun 2016 11:00:37 -0500
Subject: [PATCH 6/6] Add detail to TODO.

---
 dashboard/test/controllers/script_levels_controller_test.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dashboard/test/controllers/script_levels_controller_test.rb b/dashboard/test/controllers/script_levels_controller_test.rb
index 63c3aadbddc3b..76aec7f65a1f5 100644
--- a/dashboard/test/controllers/script_levels_controller_test.rb
+++ b/dashboard/test/controllers/script_levels_controller_test.rb
@@ -962,7 +962,9 @@ def create_admin_script
     assert_response :forbidden
   end
 
-  # TODO(asher): Consolidate the tests when the user is an admin.
+  # 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