From 36650584355de0eef5a9fecb110547c4235b570d Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Fri, 30 Nov 2018 15:44:45 +0100 Subject: [PATCH] Fix tests and slightly refactor some policies --- app/controllers/concerns/lti.rb | 2 +- app/policies/execution_environment_policy.rb | 4 ++-- app/policies/file_template_policy.rb | 8 ------- app/policies/file_type_policy.rb | 3 --- app/policies/intervention_policy.rb | 4 ++-- app/policies/proxy_exercise_policy.rb | 2 +- app/policies/request_for_comment_policy.rb | 4 ++-- app/policies/subscription_policy.rb | 4 ---- app/policies/tag_policy.rb | 22 +++---------------- config/locales/de.yml | 1 + config/locales/en.yml | 1 + spec/features/authorization_spec.rb | 4 ++-- .../execution_environment_policy_spec.rb | 6 ++--- spec/policies/file_type_policy_spec.rb | 18 +-------------- 14 files changed, 19 insertions(+), 64 deletions(-) diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb index 124b32cb..ff5e6ccf 100644 --- a/app/controllers/concerns/lti.rb +++ b/app/controllers/concerns/lti.rb @@ -63,7 +63,7 @@ module Lti else # 'learner' next end - end + end unless provider.roles.blank? result end diff --git a/app/policies/execution_environment_policy.rb b/app/policies/execution_environment_policy.rb index 9e36b558..19da368e 100644 --- a/app/policies/execution_environment_policy.rb +++ b/app/policies/execution_environment_policy.rb @@ -1,9 +1,9 @@ class ExecutionEnvironmentPolicy < AdminOnlyPolicy - [:execute_command?, :shell?, :statistics?].each do |action| + [:execute_command?, :shell?, :statistics?, :show?].each do |action| define_method(action) { admin? || author? } end - [:show?, :index?, :new?].each do |action| + [:index?].each do |action| define_method(action) { admin? || teacher? } end end diff --git a/app/policies/file_template_policy.rb b/app/policies/file_template_policy.rb index a50302d9..67aad7c0 100644 --- a/app/policies/file_template_policy.rb +++ b/app/policies/file_template_policy.rb @@ -1,13 +1,5 @@ class FileTemplatePolicy < AdminOnlyPolicy - def index? - admin? || teacher? - end - - def show? - admin? || teacher? - end - def by_file_type? everyone end diff --git a/app/policies/file_type_policy.rb b/app/policies/file_type_policy.rb index ad6dddd0..57cd4940 100644 --- a/app/policies/file_type_policy.rb +++ b/app/policies/file_type_policy.rb @@ -1,6 +1,3 @@ class FileTypePolicy < AdminOnlyPolicy - [:index?, :show?].each do |action| - define_method(action) { admin? || teacher? } - end end diff --git a/app/policies/intervention_policy.rb b/app/policies/intervention_policy.rb index 373adcb2..b8f482d5 100644 --- a/app/policies/intervention_policy.rb +++ b/app/policies/intervention_policy.rb @@ -11,8 +11,8 @@ class InterventionPolicy < AdminOrAuthorPolicy define_method(action) { admin? || author?} end - [:reload?].each do |action| - define_method(action) { everyone } + def reload? + everyone end class Scope < Scope diff --git a/app/policies/proxy_exercise_policy.rb b/app/policies/proxy_exercise_policy.rb index 3a7bedf7..14981c8e 100644 --- a/app/policies/proxy_exercise_policy.rb +++ b/app/policies/proxy_exercise_policy.rb @@ -20,7 +20,7 @@ class ProxyExercisePolicy < AdminOrAuthorPolicy if @user.admin? @scope.all elsif @user.teacher? - @scope.where('user_id = ?', @user.id) + @scope.where('user_id = ? OR public = TRUE', @user.id) else @scope.none end diff --git a/app/policies/request_for_comment_policy.rb b/app/policies/request_for_comment_policy.rb index 80b95f5f..5ecf488f 100644 --- a/app/policies/request_for_comment_policy.rb +++ b/app/policies/request_for_comment_policy.rb @@ -11,8 +11,8 @@ class RequestForCommentPolicy < ApplicationPolicy everyone end - [:destroy?].each do |action| - define_method(action) { admin? } + def destroy? + admin? end def mark_as_solved? diff --git a/app/policies/subscription_policy.rb b/app/policies/subscription_policy.rb index 289bd5ac..6c759837 100644 --- a/app/policies/subscription_policy.rb +++ b/app/policies/subscription_policy.rb @@ -7,10 +7,6 @@ class SubscriptionPolicy < ApplicationPolicy author? || admin? end - def show_error? - everyone - end - def author? @user == @record.user end diff --git a/app/policies/tag_policy.rb b/app/policies/tag_policy.rb index 7691134a..88bb28c9 100644 --- a/app/policies/tag_policy.rb +++ b/app/policies/tag_policy.rb @@ -1,29 +1,13 @@ -class TagPolicy < AdminOrAuthorPolicy - def batch_update? - admin? - end - - def show? - admin? || teacher? - end - - [:clone?, :destroy?, :edit?, :update?].each do |action| - define_method(action) { admin? || author?} - end - - [:reload?].each do |action| - define_method(action) { everyone } - end +class TagPolicy < AdminOnlyPolicy class Scope < Scope def resolve - if @user.admin? + if @user.admin? || @user.teacher? @scope.all - elsif @user.teacher? - @scope.where('user_id = ? OR public = TRUE', @user.id) else @scope.none end end end + end diff --git a/config/locales/de.yml b/config/locales/de.yml index 4cbd120c..1c001661 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -47,6 +47,7 @@ de: consumer: Konsument email: E-Mail name: Name + role: Rolle file: content: Inhalt feedback_message: Feedback-Nachricht diff --git a/config/locales/en.yml b/config/locales/en.yml index 40922a02..c82839d4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -47,6 +47,7 @@ en: consumer: Consumer email: Email name: Name + role: Role file: content: Content feedback_message: Feedback Message diff --git a/spec/features/authorization_spec.rb b/spec/features/authorization_spec.rb index 7535634f..4ce5432f 100644 --- a/spec/features/authorization_spec.rb +++ b/spec/features/authorization_spec.rb @@ -23,11 +23,11 @@ describe 'Authorization' do let(:user) { FactoryBot.create(:teacher) } before(:each) { allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(user) } - [Consumer, InternalUser].each do |model| + [Consumer, InternalUser, ExecutionEnvironment, FileType].each do |model| expect_forbidden_path(:"new_#{model.model_name.singular}_path") end - [ExecutionEnvironment, Exercise, FileType].each do |model| + [Exercise].each do |model| expect_permitted_path(:"new_#{model.model_name.singular}_path") end end diff --git a/spec/policies/execution_environment_policy_spec.rb b/spec/policies/execution_environment_policy_spec.rb index ef5787b9..50bd57ab 100644 --- a/spec/policies/execution_environment_policy_spec.rb +++ b/spec/policies/execution_environment_policy_spec.rb @@ -5,7 +5,7 @@ describe ExecutionEnvironmentPolicy do let(:execution_environment) { FactoryBot.build(:ruby) } - [:create?, :index?, :new?].each do |action| + [:index?].each do |action| permissions(action) do it 'grants access to admins' do expect(subject).to permit(FactoryBot.build(:admin), execution_environment) @@ -21,7 +21,7 @@ describe ExecutionEnvironmentPolicy do end end - [:execute_command?, :shell?, :statistics?].each do |action| + [:execute_command?, :shell?, :statistics?, :show?].each do |action| permissions(action) do it 'grants access to admins' do expect(subject).to permit(FactoryBot.build(:admin), execution_environment) @@ -39,7 +39,7 @@ describe ExecutionEnvironmentPolicy do end end - [:destroy?, :edit?, :show?, :update?].each do |action| + [:destroy?, :edit?, :update?, :new?, :create?].each do |action| permissions(action) do it 'grants access to admins' do expect(subject).to permit(FactoryBot.build(:admin), execution_environment) diff --git a/spec/policies/file_type_policy_spec.rb b/spec/policies/file_type_policy_spec.rb index 55ec4120..951512d5 100644 --- a/spec/policies/file_type_policy_spec.rb +++ b/spec/policies/file_type_policy_spec.rb @@ -5,23 +5,7 @@ describe FileTypePolicy do let(:file_type) { FactoryBot.build(:dot_rb) } - [:create?, :index?, :new?].each do |action| - permissions(action) do - it 'grants access to admins' do - expect(subject).to permit(FactoryBot.build(:admin), file_type) - end - - it 'grants access to teachers' do - expect(subject).to permit(FactoryBot.build(:teacher), file_type) - end - - it 'does not grant access to external users' do - expect(subject).not_to permit(FactoryBot.build(:external_user), file_type) - end - end - end - - [:destroy?, :edit?, :show?, :update?].each do |action| + [:destroy?, :edit?, :update?, :new?, :create?, :index?, :show?].each do |action| permissions(action) do it 'grants access to admins' do expect(subject).to permit(FactoryBot.build(:admin), file_type)