From d3f67ab4c7a430dc782182dcae8644d4c19ac389 Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Tue, 27 Nov 2018 17:05:38 +0100 Subject: [PATCH] Rethink permissions in CodeOcean for usage in schools and adopt views --- app/controllers/application_controller.rb | 1 + app/controllers/exercises_controller.rb | 15 ++++++++------- app/controllers/proxy_exercises_controller.rb | 6 +++--- app/policies/application_policy.rb | 16 +++++++++++----- app/policies/comment_policy.rb | 5 ----- app/policies/consumer_policy.rb | 4 +--- app/policies/execution_environment_policy.rb | 7 +------ app/policies/exercise_policy.rb | 5 ----- app/policies/file_template_policy.rb | 6 +++++- app/policies/file_type_policy.rb | 7 +------ app/policies/intervention_policy.rb | 5 ----- app/policies/proxy_exercise_policy.rb | 5 ----- app/policies/request_for_comment_policy.rb | 5 ----- app/policies/search_policy.rb | 5 ----- app/policies/submission_policy.rb | 16 +++++++++++----- app/policies/tag_policy.rb | 5 ----- app/views/file_types/index.html.slim | 10 +++++----- 17 files changed, 47 insertions(+), 76 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index c58303ba..0988d8f0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -33,6 +33,7 @@ class ApplicationController < ActionController::Base private :set_locale def welcome + # Show root page end def allow_iframe_requests diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 839aef4c..e60797d5 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -7,8 +7,8 @@ class ExercisesController < ApplicationController before_action :handle_file_uploads, only: [:create, :update] before_action :set_execution_environments, only: [:create, :edit, :new, :update] - before_action :set_exercise, only: MEMBER_ACTIONS + [:clone, :implement, :working_times, :intervention, :search, :run, :statistics, :submit, :reload, :feedback] - before_action :set_external_user, only: [:statistics] + before_action :set_exercise_and_authorize, only: MEMBER_ACTIONS + [:clone, :implement, :working_times, :intervention, :search, :run, :statistics, :submit, :reload, :feedback] + before_action :set_external_user_and_authorize, only: [:statistics] before_action :set_file_types, only: [:create, :edit, :new, :update] before_action :set_course_token, only: [:implement] @@ -291,19 +291,19 @@ class ExercisesController < ApplicationController end private :set_execution_environments - def set_exercise + def set_exercise_and_authorize @exercise = Exercise.find(params[:id]) authorize! end - private :set_exercise + private :set_exercise_and_authorize - def set_external_user + def set_external_user_and_authorize if params[:external_user_id] @external_user = ExternalUser.find(params[:external_user_id]) authorize! end end - private :set_exercise + private :set_external_user_and_authorize def set_file_types @file_types = FileType.all.order(:name) @@ -321,10 +321,11 @@ class ExercisesController < ApplicationController private :collect_set_and_unset_exercise_tags def show + # Show exercise details for teachers and admins end - #we might want to think about auth here def reload + # Returns JSON with original file content end def statistics diff --git a/app/controllers/proxy_exercises_controller.rb b/app/controllers/proxy_exercises_controller.rb index 40bd20ca..171fe061 100644 --- a/app/controllers/proxy_exercises_controller.rb +++ b/app/controllers/proxy_exercises_controller.rb @@ -1,7 +1,7 @@ class ProxyExercisesController < ApplicationController include CommonBehavior - before_action :set_exercise, only: MEMBER_ACTIONS + [:clone, :reload] + before_action :set_exercise_and_authorize, only: MEMBER_ACTIONS + [:clone, :reload] def authorize! authorize(@proxy_exercise || @proxy_exercises) @@ -56,11 +56,11 @@ class ProxyExercisesController < ApplicationController authorize! end - def set_exercise + def set_exercise_and_authorize @proxy_exercise = ProxyExercise.find(params[:id]) authorize! end - private :set_exercise + private :set_exercise_and_authorize def show @search = @proxy_exercise.exercises.search diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 6ce391a2..9d8b9cd1 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -9,22 +9,28 @@ class ApplicationPolicy end private :teacher? + def author? + @user == @record.author + end + private :author? + def everyone + # As the ApplicationController forces to have any authorization, `everyone` here means `every user logged in` true end private :everyone + def no_one + false + end + private :no_one + def initialize(user, record) @user = user @record = record require_user! end - def no_one - false - end - private :no_one - def require_user! fail Pundit::NotAuthorizedError unless @user end diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb index 3bdcdda0..41caae9b 100644 --- a/app/policies/comment_policy.rb +++ b/app/policies/comment_policy.rb @@ -1,9 +1,4 @@ class CommentPolicy < ApplicationPolicy - def author? - @user == @record.author - end - private :author? - def create? everyone end diff --git a/app/policies/consumer_policy.rb b/app/policies/consumer_policy.rb index 5cdad33f..8566473d 100644 --- a/app/policies/consumer_policy.rb +++ b/app/policies/consumer_policy.rb @@ -1,5 +1,3 @@ class ConsumerPolicy < AdminOnlyPolicy - def show? - super || @user.consumer == @record - end + end diff --git a/app/policies/execution_environment_policy.rb b/app/policies/execution_environment_policy.rb index 3ee5a1c0..9e36b558 100644 --- a/app/policies/execution_environment_policy.rb +++ b/app/policies/execution_environment_policy.rb @@ -1,14 +1,9 @@ class ExecutionEnvironmentPolicy < AdminOnlyPolicy - def author? - @user == @record.author - end - private :author? - [:execute_command?, :shell?, :statistics?].each do |action| define_method(action) { admin? || author? } end - [:create?, :index?, :new?].each do |action| + [:show?, :index?, :new?].each do |action| define_method(action) { admin? || teacher? } end end diff --git a/app/policies/exercise_policy.rb b/app/policies/exercise_policy.rb index adc246d8..cb9a32cf 100644 --- a/app/policies/exercise_policy.rb +++ b/app/policies/exercise_policy.rb @@ -1,9 +1,4 @@ class ExercisePolicy < AdminOrAuthorPolicy - def author? - @user == @record.author - end - private :author? - def batch_update? admin? end diff --git a/app/policies/file_template_policy.rb b/app/policies/file_template_policy.rb index 92ced442..a50302d9 100644 --- a/app/policies/file_template_policy.rb +++ b/app/policies/file_template_policy.rb @@ -1,7 +1,11 @@ class FileTemplatePolicy < AdminOnlyPolicy + def index? + admin? || teacher? + end + def show? - everyone + admin? || teacher? end def by_file_type? diff --git a/app/policies/file_type_policy.rb b/app/policies/file_type_policy.rb index afa6aa7a..ad6dddd0 100644 --- a/app/policies/file_type_policy.rb +++ b/app/policies/file_type_policy.rb @@ -1,10 +1,5 @@ class FileTypePolicy < AdminOnlyPolicy - def author? - @user == @record.author - end - private :author? - - [:create?, :index?, :new?].each do |action| + [:index?, :show?].each do |action| define_method(action) { admin? || teacher? } end diff --git a/app/policies/intervention_policy.rb b/app/policies/intervention_policy.rb index 7716e667..373adcb2 100644 --- a/app/policies/intervention_policy.rb +++ b/app/policies/intervention_policy.rb @@ -1,9 +1,4 @@ class InterventionPolicy < AdminOrAuthorPolicy - def author? - @user == @record.author - end - private :author? - def batch_update? admin? end diff --git a/app/policies/proxy_exercise_policy.rb b/app/policies/proxy_exercise_policy.rb index c65f1f38..3a7bedf7 100644 --- a/app/policies/proxy_exercise_policy.rb +++ b/app/policies/proxy_exercise_policy.rb @@ -1,9 +1,4 @@ class ProxyExercisePolicy < AdminOrAuthorPolicy - def author? - @user == @record.author - end - private :author? - def batch_update? admin? end diff --git a/app/policies/request_for_comment_policy.rb b/app/policies/request_for_comment_policy.rb index d02c5568..80b95f5f 100644 --- a/app/policies/request_for_comment_policy.rb +++ b/app/policies/request_for_comment_policy.rb @@ -1,9 +1,4 @@ class RequestForCommentPolicy < ApplicationPolicy - def author? - @user == @record.author - end - private :author? - def create? everyone end diff --git a/app/policies/search_policy.rb b/app/policies/search_policy.rb index b45ae00d..7f861c30 100644 --- a/app/policies/search_policy.rb +++ b/app/policies/search_policy.rb @@ -1,9 +1,4 @@ class SearchPolicy < AdminOrAuthorPolicy - def author? - @user == @record.author - end - private :author? - def batch_update? admin? end diff --git a/app/policies/submission_policy.rb b/app/policies/submission_policy.rb index 32916450..fe2137c3 100644 --- a/app/policies/submission_policy.rb +++ b/app/policies/submission_policy.rb @@ -1,9 +1,4 @@ class SubmissionPolicy < ApplicationPolicy - def author? - @user == @record.author - end - private :author? - def create? everyone end @@ -16,4 +11,15 @@ class SubmissionPolicy < ApplicationPolicy def index? admin? end + + def everyone_in_study_group + users_in_same_study_group = @record.study_groups.users + users_in_same_study_group.include? @user + end + private :everyone_in_study_group + + def teacher_in_study_group + teacher? && everyone_in_study_group + end + private :teacher_in_study_group end diff --git a/app/policies/tag_policy.rb b/app/policies/tag_policy.rb index 51665053..7691134a 100644 --- a/app/policies/tag_policy.rb +++ b/app/policies/tag_policy.rb @@ -1,9 +1,4 @@ class TagPolicy < AdminOrAuthorPolicy - def author? - @user == @record.author - end - private :author? - def batch_update? admin? end diff --git a/app/views/file_types/index.html.slim b/app/views/file_types/index.html.slim index 95f1394f..08f850e7 100644 --- a/app/views/file_types/index.html.slim +++ b/app/views/file_types/index.html.slim @@ -11,12 +11,12 @@ h1 = FileType.model_name.human(count: 2) tbody - @file_types.each do |file_type| tr - td = link_to(file_type.name, file_type) - td = link_to(file_type.author, file_type.author) + td = link_to_if(policy(file_type).show?, file_type.name, file_type) + td = link_to_if(policy(file_type.author).show?, file_type.author, file_type.author) td = file_type.file_extension - td = link_to(t('shared.show'), file_type) - td = link_to(t('shared.edit'), edit_file_type_path(file_type)) - td = link_to(t('shared.destroy'), file_type, data: {confirm: t('shared.confirm_destroy')}, method: :delete) + td = link_to(t('shared.show'), file_type) if policy(file_type).show? + td = link_to(t('shared.edit'), edit_file_type_path(file_type)) if policy(file_type).edit? + td = link_to(t('shared.destroy'), file_type, data: {confirm: t('shared.confirm_destroy')}, method: :delete) if policy(file_type).destroy? = render('shared/pagination', collection: @file_types) p = render('shared/new_button', model: FileType)