Refactor exercise_controller and move more checks to policy

* We introduce a custom handler for Pundit::NotAuthorizedError

app/controllers/exercises_controller.rb

@@ -22,6 +22,8 @@ class ExercisesController < ApplicationController
   skip_after_action :verify_authorized, only: %i[import_exercise import_uuid_check]
   skip_after_action :verify_policy_scoped, only: %i[import_exercise import_uuid_check], raise: false
 
+  rescue_from Pundit::NotAuthorizedError, with: :not_authorized_for_exercise
+
   def authorize!
     authorize(@exercise || @exercises)
   end
@@ -294,8 +296,6 @@ class ExercisesController < ApplicationController
   private :update_exercise_tips
 
   def implement
-    redirect_to(@exercise, alert: t('exercises.implement.unpublished')) if @exercise.unpublished? && current_user.role != 'admin' && current_user.role != 'teacher' # TODO: TESTESTEST
-    redirect_to(@exercise, alert: t('exercises.implement.no_files')) unless @exercise.files.visible.exists?
     user_solved_exercise = @exercise.solved_by?(current_user)
     count_interventions_today = UserExerciseIntervention.where(user: current_user).where('created_at >= ?',
       Time.zone.now.beginning_of_day).count
@@ -434,6 +434,16 @@ class ExercisesController < ApplicationController
     authorize!
   end
 
+  def not_authorized_for_exercise(_exception)
+    if %w[implement working_times intervention search reload].include?(action_name) && (current_user.admin? || current_user.teacher?)
+      redirect_to(@exercise, alert: t('exercises.implement.unpublished')) if @exercise.unpublished?
+      redirect_to(@exercise, alert: t('exercises.implement.no_files')) unless @exercise.files.visible.exists?
+    else
+      render_not_authorized
+    end
+  end
+  private :not_authorized_for_exercise
+
   def set_execution_environments
     @execution_environments = ExecutionEnvironment.all.order(:name)
   end

app/policies/exercise_policy.rb

@@ -30,7 +30,11 @@ class ExercisePolicy < AdminOrAuthorPolicy
   end
 
   %i[implement? working_times? intervention? search? reload?].each do |action|
-    define_method(action) { everyone }
+    define_method(action) do
+      return no_one unless @record.files.visible.exists?
+
+      admin? || teacher_in_study_group? || author? || (everyone && !@record.unpublished?)
+    end
   end
 
   def submit?

spec/controllers/exercises_controller_spec.rb

@@ -187,6 +187,17 @@ describe ExercisesController do
       expect_flash_message(:alert, :'exercises.implement.no_files')
       expect_redirect(:exercise)
     end
+
+    context 'with other users accessing an unpublished exercise' do
+      let(:exercise) { create(:fibonacci, unpublished: true) }
+      let(:user) { create(:teacher) }
+
+      before { perform_request.call }
+
+      expect_assigns(exercise: :exercise)
+      expect_flash_message(:alert, :'exercises.implement.unpublished')
+      expect_redirect(:exercise)
+    end
   end
 
   describe 'GET #index' do
@@ -223,6 +234,8 @@ describe ExercisesController do
 
   describe 'GET #reload' do
     context 'when being anyone' do
+      let(:exercise) { create(:fibonacci) }
+
       before { get :reload, format: :json, params: {id: exercise.id} }
 
       expect_assigns(exercise: :exercise)

spec/policies/exercise_policy_spec.rb

@@ -110,10 +110,54 @@ describe ExercisePolicy do
     end
   end
 
-  permissions :implement? do
+  %i[implement? working_times? intervention? search? reload?].each do |action|
+    permissions(action) do
+      context 'when the exercise has no visible files' do
+        let(:exercise) { create(:dummy) }
+
+        it 'does not grant access to anyone' do
+          %i[admin external_user teacher].each do |factory_name|
+            expect(policy).not_to permit(build(factory_name), exercise)
+          end
+        end
+      end
+
+      context 'when the exercise has visible files' do
+        let(:exercise) { create(:fibonacci) }
+
         it 'grants access to anyone' do
           %i[admin external_user teacher].each do |factory_name|
-        expect(policy).to permit(build(factory_name), Exercise.new)
+            expect(policy).to permit(build(factory_name), exercise)
+          end
+        end
+      end
+
+      context 'when the exercise is published' do
+        let(:exercise) { create(:fibonacci, unpublished: false) }
+
+        it 'grants access to anyone' do
+          %i[admin external_user teacher].each do |factory_name|
+            expect(policy).to permit(build(factory_name), exercise)
+          end
+        end
+      end
+
+      context 'when the exercise is unpublished' do
+        let(:exercise) { create(:fibonacci, unpublished: true) }
+
+        it 'grants access to admins' do
+          expect(policy).to permit(build(:admin), exercise)
+        end
+
+        it 'grants access to the author' do
+          expect(policy).to permit(exercise.author, exercise)
+        end
+
+        it 'does not grant access to everyone' do
+          %i[external_user teacher].each do |factory_name|
+            expect(policy).not_to permit(build(factory_name), exercise)
+          end
+        end
       end
     end
   end