From 145c4aa8d51a3da11f563de7a67859c2ce62851e Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Thu, 18 Aug 2022 15:06:36 +0200 Subject: [PATCH] Refactor various ruby files * Insights based on brakeman report --- .rubocop_todo.yml | 2 +- .../code_ocean/files_controller.rb | 2 +- .../codeharbor_links_controller.rb | 3 +- app/controllers/comments_controller.rb | 6 +- .../community_solutions_controller.rb | 4 +- app/controllers/concerns/redirect_behavior.rb | 7 +-- app/controllers/consumers_controller.rb | 2 +- .../error_template_attributes_controller.rb | 2 +- app/controllers/error_templates_controller.rb | 6 +- .../execution_environments_controller.rb | 4 +- .../exercise_collections_controller.rb | 2 +- app/controllers/exercises_controller.rb | 63 ++++++++++++------- app/controllers/external_users_controller.rb | 6 +- app/controllers/file_templates_controller.rb | 2 +- app/controllers/file_types_controller.rb | 2 +- app/controllers/internal_users_controller.rb | 2 +- app/controllers/proxy_exercises_controller.rb | 8 +-- .../request_for_comments_controller.rb | 4 +- app/controllers/sessions_controller.rb | 8 ++- app/controllers/study_groups_controller.rb | 4 +- app/controllers/submissions_controller.rb | 2 +- app/controllers/subscriptions_controller.rb | 4 +- app/controllers/tags_controller.rb | 2 +- app/controllers/tips_controller.rb | 2 +- .../user_exercise_feedbacks_controller.rb | 6 +- app/helpers/statistics_helper.rb | 6 +- app/models/exercise.rb | 12 ++-- app/models/request_for_comment.rb | 25 +------- .../_breadcrumbs_and_title.html.slim | 3 +- .../application/_locale_selector.html.slim | 2 +- .../sessions/destroy_through_lti.html.slim | 8 ++- config/environments/production.rb | 2 +- config/environments/staging.rb | 2 +- lib/assessor.rb | 2 +- spec/controllers/sessions_controller_spec.rb | 3 +- 35 files changed, 113 insertions(+), 107 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0bddfa3f..160eceee 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -19,7 +19,7 @@ Metrics/ClassLength: Max: 600 Metrics/ModuleLength: - Max: 220 + Max: 225 # It's a very complicated application... # diff --git a/app/controllers/code_ocean/files_controller.rb b/app/controllers/code_ocean/files_controller.rb index 79331f6a..0bb2c63c 100644 --- a/app/controllers/code_ocean/files_controller.rb +++ b/app/controllers/code_ocean/files_controller.rb @@ -41,7 +41,7 @@ module CodeOcean end def destroy - @file = CodeOcean::File.find(params[:id]) + @file = CodeOcean::File.find_by(id: params[:id]) authorize! destroy_and_respond(object: @file, path: @file.context) end diff --git a/app/controllers/codeharbor_links_controller.rb b/app/controllers/codeharbor_links_controller.rb index 1257596d..8772c8f0 100644 --- a/app/controllers/codeharbor_links_controller.rb +++ b/app/controllers/codeharbor_links_controller.rb @@ -43,8 +43,7 @@ class CodeharborLinksController < ApplicationController end def set_codeharbor_link - @codeharbor_link = CodeharborLink.find(params[:id]) - @codeharbor_link.user = current_user + @codeharbor_link = CodeharborLink.find_by(id: params[:id]) authorize! end diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 201246ce..1b23cf20 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -10,7 +10,7 @@ class CommentsController < ApplicationController # GET /comments.json def index - file = CodeOcean::File.find(params[:file_id]) + file = CodeOcean::File.find_by(id: params[:file_id]) # there might be no submission yet, so dont use find submission = Submission.find_by(id: file.context_id) if submission @@ -38,7 +38,7 @@ class CommentsController < ApplicationController if @comment.save if comment_params[:request_id] - request_for_comment = RequestForComment.find(comment_params[:request_id]) + request_for_comment = RequestForComment.find_by(id: comment_params[:request_id]) send_mail_to_author @comment, request_for_comment send_mail_to_subscribers @comment, request_for_comment end @@ -71,7 +71,7 @@ class CommentsController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_comment - @comment = Comment.find(params[:id]) + @comment = Comment.find_by(id: params[:id]) end def comment_params_for_update diff --git a/app/controllers/community_solutions_controller.rb b/app/controllers/community_solutions_controller.rb index 170bb0e5..d26b728f 100644 --- a/app/controllers/community_solutions_controller.rb +++ b/app/controllers/community_solutions_controller.rb @@ -90,11 +90,11 @@ class CommunitySolutionsController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_community_solution - @community_solution = CommunitySolution.find(params[:id]) + @community_solution = CommunitySolution.find_by(id: params[:id]) end def set_community_solution_lock - @community_solution_lock = CommunitySolutionLock.find(params[:lock_id]) + @community_solution_lock = CommunitySolutionLock.find_by(id: params[:lock_id]) end def set_exercise_and_submission diff --git a/app/controllers/concerns/redirect_behavior.rb b/app/controllers/concerns/redirect_behavior.rb index a3d6fd24..fabfa8c3 100644 --- a/app/controllers/concerns/redirect_behavior.rb +++ b/app/controllers/concerns/redirect_behavior.rb @@ -129,12 +129,7 @@ module RedirectBehavior lti_parameters_id: session[:lti_parameters_id] ) - lti_parameter = LtiParameter.where(external_users_id: @submission.user_id, - exercises_id: @submission.exercise_id).last - - path = lti_return_path(submission_id: @submission.id, - url: consumer_return_url(build_tool_provider(consumer: @submission.user.consumer, - parameters: lti_parameter&.lti_parameters))) + path = lti_return_path(submission_id: @submission.id) clear_lti_session_data(@submission.exercise_id, @submission.user_id) respond_to do |format| format.html { redirect_to(path) } diff --git a/app/controllers/consumers_controller.rb b/app/controllers/consumers_controller.rb index 2bf09d5b..af1072aa 100644 --- a/app/controllers/consumers_controller.rb +++ b/app/controllers/consumers_controller.rb @@ -38,7 +38,7 @@ class ConsumersController < ApplicationController end def set_consumer - @consumer = Consumer.find(params[:id]) + @consumer = Consumer.find_by(id: params[:id]) authorize! end private :set_consumer diff --git a/app/controllers/error_template_attributes_controller.rb b/app/controllers/error_template_attributes_controller.rb index 3ad29480..28a41e1c 100644 --- a/app/controllers/error_template_attributes_controller.rb +++ b/app/controllers/error_template_attributes_controller.rb @@ -86,7 +86,7 @@ class ErrorTemplateAttributesController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_error_template_attribute - @error_template_attribute = ErrorTemplateAttribute.find(params[:id]) + @error_template_attribute = ErrorTemplateAttribute.find_by(id: params[:id]) end def error_template_attribute_params diff --git a/app/controllers/error_templates_controller.rb b/app/controllers/error_templates_controller.rb index e459cef4..17e7d8da 100644 --- a/app/controllers/error_templates_controller.rb +++ b/app/controllers/error_templates_controller.rb @@ -77,7 +77,7 @@ class ErrorTemplatesController < ApplicationController def add_attribute authorize! - @error_template.error_template_attributes << ErrorTemplateAttribute.find(params['error_template_attribute_id']) + @error_template.error_template_attributes << ErrorTemplateAttribute.find_by(id: params[:error_template_attribute_id]) respond_to do |format| format.html { redirect_to @error_template } format.json { head :no_content } @@ -86,7 +86,7 @@ class ErrorTemplatesController < ApplicationController def remove_attribute authorize! - @error_template.error_template_attributes.delete(ErrorTemplateAttribute.find(params['error_template_attribute_id'])) + @error_template.error_template_attributes.delete(ErrorTemplateAttribute.find_by(id: params[:error_template_attribute_id])) respond_to do |format| format.html { redirect_to @error_template } format.json { head :no_content } @@ -97,7 +97,7 @@ class ErrorTemplatesController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_error_template - @error_template = ErrorTemplate.find(params[:id]) + @error_template = ErrorTemplate.find_by(id: params[:id]) end def error_template_params diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb index babd665e..1e91c922 100644 --- a/app/controllers/execution_environments_controller.rb +++ b/app/controllers/execution_environments_controller.rb @@ -141,7 +141,7 @@ class ExecutionEnvironmentsController < ApplicationController private :set_docker_images def set_execution_environment - @execution_environment = ExecutionEnvironment.find(params[:id]) + @execution_environment = ExecutionEnvironment.find_by(id: params[:id]) authorize! end private :set_execution_environment @@ -158,7 +158,7 @@ class ExecutionEnvironmentsController < ApplicationController def show if @execution_environment.testing_framework? - @testing_framework_adapter = Kernel.const_get(@execution_environment.testing_framework) + @testing_framework_adapter = TestingFrameworkAdapter.descendants.find {|klass| klass.name == @execution_environment.testing_framework } end end diff --git a/app/controllers/exercise_collections_controller.rb b/app/controllers/exercise_collections_controller.rb index 40d49e64..ce6c7882 100644 --- a/app/controllers/exercise_collections_controller.rb +++ b/app/controllers/exercise_collections_controller.rb @@ -41,7 +41,7 @@ class ExerciseCollectionsController < ApplicationController private def set_exercise_collection - @exercise_collection = ExerciseCollection.find(params[:id]) + @exercise_collection = ExerciseCollection.find_by(id: params[:id]) authorize! end diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index 91259c68..f0f07212 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -25,6 +25,7 @@ class ExercisesController < ApplicationController def authorize! authorize(@exercise || @exercises) end + private :authorize! def max_intervention_count_per_day @@ -39,7 +40,7 @@ class ExercisesController < ApplicationController @exercises = Exercise.all authorize! @exercises = params[:exercises].values.map do |exercise_params| - exercise = Exercise.find(exercise_params.delete(:id)) + exercise = Exercise.find_by(id: exercise_params.delete(:id)) exercise.update(exercise_params) exercise end @@ -50,7 +51,7 @@ class ExercisesController < ApplicationController exercise = @exercise.duplicate(public: false, token: nil, user: current_user) exercise.send(:generate_token) if exercise.save - redirect_to(exercise, notice: t('shared.object_cloned', model: Exercise.model_name.human)) + redirect_to(exercise_path(exercise), notice: t('shared.object_cloned', model: Exercise.model_name.human)) else flash[:danger] = t('shared.message_failure') redirect_to(@exercise) @@ -66,6 +67,7 @@ class ExercisesController < ApplicationController end subpaths.flatten.uniq end + private :collect_paths def create @@ -192,12 +194,14 @@ class ExercisesController < ApplicationController api_key = authorization_header&.split(' ')&.second user_by_codeharbor_token(api_key) end + private :user_from_api_key def user_by_codeharbor_token(api_key) link = CodeharborLink.find_by(api_key: api_key) link&.user end + private :user_by_codeharbor_token def exercise_params @@ -225,6 +229,7 @@ class ExercisesController < ApplicationController ) end end + private :exercise_params def handle_file_uploads @@ -241,6 +246,7 @@ class ExercisesController < ApplicationController end end end + private :handle_file_uploads def handle_exercise_tips @@ -258,6 +264,7 @@ class ExercisesController < ApplicationController redirect_to(edit_exercise_path(@exercise)) end end + private :handle_exercise_tips def update_exercise_tips(exercise_tips, parent_exercise_tip_id, rank) @@ -283,6 +290,7 @@ class ExercisesController < ApplicationController end result end + private :update_exercise_tips def implement @@ -348,6 +356,7 @@ class ExercisesController < ApplicationController @course_token = '702cbd2a-c84c-4b37-923a-692d7d1532d0' end end + private :set_course_token def set_available_tips @@ -374,13 +383,14 @@ class ExercisesController < ApplicationController # Return an array with top-level tips @tips = nested_tips.values.select {|tip| tip.parent_exercise_tip_id.nil? } end + private :set_available_tips def working_times working_time_accumulated = @exercise.accumulated_working_time_for_only(current_user) working_time_75_percentile = @exercise.get_quantiles([0.75]).first render(json: {working_time_75_percentile: working_time_75_percentile, -working_time_accumulated: working_time_accumulated}) + working_time_accumulated: working_time_accumulated}) end def intervention @@ -401,8 +411,9 @@ working_time_accumulated: working_time_accumulated}) search_text = params[:search_text] search = Search.new(user: current_user, exercise: @exercise, search: search_text) - begin search.save - render(json: {success: 'true'}) + begin + search.save + render(json: {success: 'true'}) rescue StandardError render(json: {success: 'false', error: "could not save search: #{$ERROR_INFO}"}) end @@ -424,25 +435,29 @@ working_time_accumulated: working_time_accumulated}) def set_execution_environments @execution_environments = ExecutionEnvironment.all.order(:name) end + private :set_execution_environments def set_exercise_and_authorize - @exercise = Exercise.find(params[:id]) + @exercise = Exercise.find_by(id: params[:id]) authorize! end + private :set_exercise_and_authorize def set_external_user_and_authorize if params[:external_user_id] - @external_user = ExternalUser.find(params[:external_user_id]) + @external_user = ExternalUser.find_by(id: params[:external_user_id]) authorize! end end + private :set_external_user_and_authorize def set_file_types @file_types = FileType.all.order(:name) end + private :set_file_types def collect_set_and_unset_exercise_tags @@ -452,9 +467,10 @@ working_time_accumulated: working_time_accumulated}) checked_tags = checked_exercise_tags.collect(&:tag).to_set unchecked_tags = Tag.all.to_set.subtract checked_tags @exercise_tags = checked_exercise_tags + unchecked_tags.collect do |tag| - ExerciseTag.new(exercise: @exercise, tag: tag) - end + ExerciseTag.new(exercise: @exercise, tag: tag) + end end + private :collect_set_and_unset_exercise_tags def show @@ -468,20 +484,24 @@ working_time_accumulated: working_time_accumulated}) def statistics # Show general statistic page for specific exercise user_statistics = {'InternalUser' => {}, 'ExternalUser' => {}} - additional_filter = if policy(@exercise).detailed_statistics? - '' - elsif !policy(@exercise).detailed_statistics? && current_user.study_groups.count.positive? - "AND study_group_id IN (#{current_user.study_groups.pluck(:id).join(', ')}) AND cause = 'submit'" - else - # e.g. internal user without any study groups, show no submissions - 'AND FALSE' - end - query = "SELECT user_id, user_type, MAX(score) AS maximum_score, COUNT(id) AS runs - FROM submissions WHERE exercise_id = #{@exercise.id} #{additional_filter} - GROUP BY user_id, user_type;" - ApplicationRecord.connection.execute(query).each do |tuple| + + query = Submission.select('user_id, user_type, MAX(score) AS maximum_score, COUNT(id) AS runs') + .where(exercise_id: @exercise.id) + .group('user_id, user_type') + + query = if policy(@exercise).detailed_statistics? + query + elsif !policy(@exercise).detailed_statistics? && current_user.study_groups.count.positive? + query.where(study_groups: current_user.study_groups.pluck(:id), cause: 'submit') + else + # e.g. internal user without any study groups, show no submissions + query.where('false') + end + + query.each do |tuple| user_statistics[tuple['user_type']][tuple['user_id'].to_i] = tuple end + render locals: { user_statistics: user_statistics, } @@ -554,6 +574,7 @@ working_time_accumulated: working_time_accumulated}) end end end + private :transmit_lti_score def update diff --git a/app/controllers/external_users_controller.rb b/app/controllers/external_users_controller.rb index 2072d195..52cdc2ef 100644 --- a/app/controllers/external_users_controller.rb +++ b/app/controllers/external_users_controller.rb @@ -15,7 +15,7 @@ class ExternalUsersController < ApplicationController end def show - @user = ExternalUser.find(params[:id]) + @user = ExternalUser.find_by(id: params[:id]) authorize! end @@ -58,7 +58,7 @@ class ExternalUsersController < ApplicationController end def statistics - @user = ExternalUser.find(params[:id]) + @user = ExternalUser.find_by(id: params[:id]) authorize! statistics = {} @@ -73,7 +73,7 @@ class ExternalUsersController < ApplicationController end def tag_statistics - @user = ExternalUser.find(params[:id]) + @user = ExternalUser.find_by(id: params[:id]) authorize! statistics = [] diff --git a/app/controllers/file_templates_controller.rb b/app/controllers/file_templates_controller.rb index e8813e1c..0333f23c 100644 --- a/app/controllers/file_templates_controller.rb +++ b/app/controllers/file_templates_controller.rb @@ -87,7 +87,7 @@ class FileTemplatesController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_file_template - @file_template = FileTemplate.find(params[:id]) + @file_template = FileTemplate.find_by(id: params[:id]) end def file_template_params diff --git a/app/controllers/file_types_controller.rb b/app/controllers/file_types_controller.rb index 2f7fa37d..7bafd157 100644 --- a/app/controllers/file_types_controller.rb +++ b/app/controllers/file_types_controller.rb @@ -51,7 +51,7 @@ class FileTypesController < ApplicationController private :set_editor_modes def set_file_type - @file_type = FileType.find(params[:id]) + @file_type = FileType.find_by(id: params[:id]) authorize! end private :set_file_type diff --git a/app/controllers/internal_users_controller.rb b/app/controllers/internal_users_controller.rb index f7f0e086..27bcf79d 100644 --- a/app/controllers/internal_users_controller.rb +++ b/app/controllers/internal_users_controller.rb @@ -121,7 +121,7 @@ class InternalUsersController < ApplicationController private :set_up_password def set_user - @user = InternalUser.find(params[:id]) + @user = InternalUser.find_by(id: params[:id]) authorize! end private :set_user diff --git a/app/controllers/proxy_exercises_controller.rb b/app/controllers/proxy_exercises_controller.rb index b591c3d4..930a18de 100644 --- a/app/controllers/proxy_exercises_controller.rb +++ b/app/controllers/proxy_exercises_controller.rb @@ -15,7 +15,7 @@ class ProxyExercisesController < ApplicationController user: current_user) proxy_exercise.send(:generate_token) if proxy_exercise.save - redirect_to(proxy_exercise, notice: t('shared.object_cloned', model: ProxyExercise.model_name.human)) + redirect_to(proxy_exercise_path(proxy_exercise), notice: t('shared.object_cloned', model: ProxyExercise.model_name.human)) else flash[:danger] = t('shared.message_failure') redirect_to(@proxy_exercise) @@ -24,7 +24,7 @@ class ProxyExercisesController < ApplicationController def create myparams = proxy_exercise_params - myparams[:exercises] = Exercise.find(myparams[:exercise_ids].compact_blank) + myparams[:exercises] = Exercise.find_by(id: myparams[:exercise_ids].compact_blank) @proxy_exercise = ProxyExercise.new(myparams) authorize! @@ -63,7 +63,7 @@ class ProxyExercisesController < ApplicationController end def set_exercise_and_authorize - @proxy_exercise = ProxyExercise.find(params[:id]) + @proxy_exercise = ProxyExercise.find_by(id: params[:id]) authorize! end private :set_exercise_and_authorize @@ -78,7 +78,7 @@ class ProxyExercisesController < ApplicationController def update myparams = proxy_exercise_params - myparams[:exercises] = Exercise.find(myparams[:exercise_ids].compact_blank) + myparams[:exercises] = Exercise.find_by(id: myparams[:exercise_ids].compact_blank) update_and_respond(object: @proxy_exercise, params: myparams) end end diff --git a/app/controllers/request_for_comments_controller.rb b/app/controllers/request_for_comments_controller.rb index 435ad2d6..c4ef5ef3 100644 --- a/app/controllers/request_for_comments_controller.rb +++ b/app/controllers/request_for_comments_controller.rb @@ -58,7 +58,7 @@ class RequestForCommentsController < ApplicationController # GET /exercises/:id/request_for_comments def rfcs_for_exercise - exercise = Exercise.find(params[:exercise_id]) + exercise = Exercise.find_by(id: params[:exercise_id]) @search = RequestForComment .with_last_activity .where(exercise_id: exercise.id) @@ -141,7 +141,7 @@ class RequestForCommentsController < ApplicationController # Use callbacks to share common setup or constraints between actions. def set_request_for_comment - @request_for_comment = RequestForComment.find(params[:id]) + @request_for_comment = RequestForComment.find_by(id: params[:id]) end def request_for_comment_params diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 4a9b16c6..ca2b084d 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -24,7 +24,7 @@ class SessionsController < ApplicationController store_lti_session_data(consumer: @consumer, parameters: params) store_nonce(params[:oauth_nonce]) if params[:custom_redirect_target] - redirect_to(params[:custom_redirect_target]) + redirect_to(URI.parse(params[:custom_redirect_target].to_s).path) else redirect_to(implement_exercise_path(@exercise), notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, @current_user.id) ? 'with' : 'without'}_outcome", @@ -42,7 +42,11 @@ class SessionsController < ApplicationController end def destroy_through_lti - @submission = Submission.find(params[:submission_id]) + @submission = Submission.find_by(id: params[:submission_id]) + authorize(@submission, :show?) + lti_parameter = LtiParameter.where(external_users_id: @submission.user_id, exercises_id: @submission.exercise_id).last + @url = consumer_return_url(build_tool_provider(consumer: @submission.user.consumer, parameters: lti_parameter&.lti_parameters)) + clear_lti_session_data(@submission.exercise_id, @submission.user_id) end diff --git a/app/controllers/study_groups_controller.rb b/app/controllers/study_groups_controller.rb index de84a945..15fb5157 100644 --- a/app/controllers/study_groups_controller.rb +++ b/app/controllers/study_groups_controller.rb @@ -23,7 +23,7 @@ class StudyGroupsController < ApplicationController def update myparams = study_group_params myparams[:external_users] = - StudyGroupMembership.find(myparams[:study_group_membership_ids].compact_blank).map(&:user) + StudyGroupMembership.find_by(id: myparams[:study_group_membership_ids].compact_blank).map(&:user) myparams.delete(:study_group_membership_ids) update_and_respond(object: @study_group, params: myparams) end @@ -38,7 +38,7 @@ class StudyGroupsController < ApplicationController private :study_group_params def set_group - @study_group = StudyGroup.find(params[:id]) + @study_group = StudyGroup.find_by(id: params[:id]) authorize! end private :set_group diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index b24f9527..757163fd 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -373,7 +373,7 @@ class SubmissionsController < ApplicationController end def set_submission - @submission = Submission.find(params[:id]) + @submission = Submission.find_by(id: params[:id]) authorize! end diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index bff28ac4..7cf59f9e 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -22,7 +22,7 @@ class SubscriptionsController < ApplicationController # DELETE /subscriptions/1 # DELETE /subscriptions/1.json def destroy - @subscription = Subscription.find(params[:id]) + @subscription = Subscription.find_by(id: params[:id]) rescue StandardError skip_authorization respond_to do |format| @@ -47,7 +47,7 @@ class SubscriptionsController < ApplicationController end def set_subscription - @subscription = Subscription.find(params[:id]) + @subscription = Subscription.find_by(id: params[:id]) authorize! end private :set_subscription diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 6a60099b..91b2e9cb 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -38,7 +38,7 @@ class TagsController < ApplicationController end def set_tag - @tag = Tag.find(params[:id]) + @tag = Tag.find_by(id: params[:id]) authorize! end private :set_tag diff --git a/app/controllers/tips_controller.rb b/app/controllers/tips_controller.rb index 7d808d08..3c20f466 100644 --- a/app/controllers/tips_controller.rb +++ b/app/controllers/tips_controller.rb @@ -44,7 +44,7 @@ class TipsController < ApplicationController end def set_tip - @tip = Tip.find(params[:id]) + @tip = Tip.find_by(id: params[:id]) authorize! end private :set_tip diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index e07b9c4a..c17e1a26 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/app/controllers/user_exercise_feedbacks_controller.rb @@ -25,7 +25,7 @@ class UserExerciseFeedbacksController < ApplicationController def create Sentry.set_extras(params: uef_params) - @exercise = Exercise.find(uef_params[:exercise_id]) + @exercise = Exercise.find_by(id: uef_params[:exercise_id]) rfc = RequestForComment.unsolved.where(exercise_id: @exercise.id, user_id: current_user.id).first submission = begin current_user.submissions.where(exercise_id: @exercise.id).order('created_at DESC').first @@ -67,7 +67,7 @@ class UserExerciseFeedbacksController < ApplicationController else params[:user_exercise_feedback][:exercise_id] end - @exercise = Exercise.find(exercise_id) + @exercise = Exercise.find_by(id: exercise_id) @uef = UserExerciseFeedback.find_or_initialize_by(user: current_user, exercise: @exercise) authorize! end @@ -105,7 +105,7 @@ class UserExerciseFeedbacksController < ApplicationController end def set_user_exercise_feedback - @uef = UserExerciseFeedback.find(params[:id]) + @uef = UserExerciseFeedback.find_by(id: params[:id]) @exercise = @uef.exercise end diff --git a/app/helpers/statistics_helper.rb b/app/helpers/statistics_helper.rb index f5b159dc..049c5a42 100644 --- a/app/helpers/statistics_helper.rb +++ b/app/helpers/statistics_helper.rb @@ -2,7 +2,7 @@ module StatisticsHelper WORKING_TIME_DELTA_IN_SECONDS = 5.minutes - WORKING_TIME_DELTA_IN_SQL_INTERVAL = "'0:05:00'" # yes, a string with quotes + WORKING_TIME_DELTA_IN_SQL_INTERVAL = ActiveRecord::Base.sanitize_sql("'0:05:00'") # yes, a string with quotes def statistics_data [ @@ -174,6 +174,8 @@ module StatisticsHelper end def ranged_rfc_data(interval = 'year', from = DateTime.new(0), to = DateTime.now) + interval = ActiveRecord::Base.sanitize_sql(interval) + [ { key: 'rfcs', @@ -209,6 +211,8 @@ module StatisticsHelper end def ranged_user_data(interval = 'year', from = DateTime.new(0), to = DateTime.now) + interval = ActiveRecord::Base.sanitize_sql(interval) + [ { key: 'active', diff --git a/app/models/exercise.rb b/app/models/exercise.rb index d0aa9605..308dc7d7 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -103,7 +103,7 @@ class Exercise < ApplicationRecord (created_at - lag(created_at) over (PARTITION BY user_id, exercise_id ORDER BY created_at)) AS working_time FROM submissions - WHERE exercise_id=#{id}) AS foo) AS bar + WHERE exercise_id=#{self.class.sanitize_sql(id)}) AS foo) AS bar GROUP BY user_id, user_type " end @@ -118,7 +118,7 @@ class Exercise < ApplicationRecord (created_at - lag(created_at) over (PARTITION BY submissions.user_type, submissions.user_id, exercise_id ORDER BY created_at)) AS working_time FROM submissions - WHERE exercise_id = #{exercise_id} AND study_group_id = #{study_group_id} #{additional_filter}), + WHERE exercise_id = #{self.class.sanitize_sql(exercise_id)} AND study_group_id = #{self.class.sanitize_sql(study_group_id)} #{self.class.sanitize_sql(additional_filter)}), working_time_with_deltas_ignored AS ( SELECT user_id, user_type, @@ -251,7 +251,7 @@ class Exercise < ApplicationRecord end def get_quantiles(quantiles) - quantiles_str = "[#{quantiles.join(',')}]" + quantiles_str = self.class.sanitize_sql("[#{quantiles.join(',')}]") result = ActiveRecord::Base.transaction do self.class.connection.execute(" SET LOCAL intervalstyle = 'iso_8601'; @@ -263,7 +263,7 @@ class Exercise < ApplicationRecord Max(score) AS max_score, (created_at - Lag(created_at) OVER (partition BY user_id, exercise_id ORDER BY created_at)) AS working_time FROM submissions - WHERE exercise_id = #{id} + WHERE exercise_id = #{self.class.sanitize_sql(id)} AND user_type = 'ExternalUser' GROUP BY user_id, id, @@ -273,7 +273,7 @@ class Exercise < ApplicationRecord Sum(weight) AS max_points FROM files WHERE context_type = 'Exercise' - AND context_id = #{id} + AND context_id = #{self.class.sanitize_sql(id)} AND role IN ('teacher_defined_test', 'teacher_defined_linter') GROUP BY context_id), -- filter for rows containing max points @@ -387,7 +387,7 @@ class Exercise < ApplicationRecord self.class.connection.execute(" SELECT avg(working_time) as average_time FROM - (#{user_working_time_query}) AS baz; + (#{self.class.sanitize_sql(user_working_time_query)}) AS baz; ").first['average_time'] end end diff --git a/app/models/request_for_comment.rb b/app/models/request_for_comment.rb index d32bb894..7668a8f2 100644 --- a/app/models/request_for_comment.rb +++ b/app/models/request_for_comment.rb @@ -22,27 +22,6 @@ class RequestForComment < ApplicationRecord # after_save :trigger_rfc_action_cable - # not used right now, finds the last submission for the respective user and exercise. - # might be helpful to check whether the exercise has been solved in the meantime. - def last_submission - Submission.find_by_sql(" select * from submissions - where exercise_id = #{exercise_id} AND - user_id = #{user_id} - order by created_at desc - limit 1").first - end - - # not used any longer, since we directly saved the submission_id now. - # Was used before that to determine the submission belonging to the request_for_comment. - def last_submission_before_creation - Submission.find_by_sql(" select * from submissions - where exercise_id = #{exercise_id} AND - user_id = #{user_id} AND - '#{created_at.localtime}' > created_at - order by created_at desc - limit 1").first - end - def comments_count submission.files.sum {|file| file.comments.size } end @@ -89,7 +68,7 @@ class RequestForComment < ApplicationRecord end def last_per_user(count = 5) - from("(#{row_number_user_sql}) as request_for_comments") + from(row_number_user_sql, :request_for_comments) .where('row_number <= ?', count) .group('request_for_comments.id, request_for_comments.user_id, request_for_comments.user_type, ' \ 'request_for_comments.exercise_id, request_for_comments.file_id, request_for_comments.question, ' \ @@ -101,7 +80,7 @@ class RequestForComment < ApplicationRecord private def row_number_user_sql - select('id, user_id, user_type, exercise_id, file_id, question, created_at, updated_at, solved, full_score_reached, submission_id, row_number() OVER (PARTITION BY user_id, user_type ORDER BY created_at DESC) as row_number').to_sql + select('id, user_id, user_type, exercise_id, file_id, question, created_at, updated_at, solved, full_score_reached, submission_id, row_number() OVER (PARTITION BY user_id, user_type ORDER BY created_at DESC) as row_number') end end end diff --git a/app/views/application/_breadcrumbs_and_title.html.slim b/app/views/application/_breadcrumbs_and_title.html.slim index 5829ec88..f823dbeb 100644 --- a/app/views/application/_breadcrumbs_and_title.html.slim +++ b/app/views/application/_breadcrumbs_and_title.html.slim @@ -1,4 +1,5 @@ -- if model = Kernel.const_get(controller_path.classify) rescue nil +- model = controller_path.classify.constantize rescue nil +- if model - object = model.find_by(id: params[:id]) - if model.try(:nested_resource?) - root_element = model.model_name.human(count: 2) diff --git a/app/views/application/_locale_selector.html.slim b/app/views/application/_locale_selector.html.slim index 0401e98d..4e3ff592 100644 --- a/app/views/application/_locale_selector.html.slim +++ b/app/views/application/_locale_selector.html.slim @@ -4,4 +4,4 @@ li.nav-item.dropdown span.caret ul.dropdown-menu.p-0.mt-1 role='menu' - I18n.available_locales.sort_by { |locale| t("locales.#{locale}") }.each do |locale| - li = link_to(t("locales.#{locale}"), url_for(params.permit!.merge(locale: locale)), 'data-turbolinks' => "false", class: 'dropdown-item') + li = link_to(t("locales.#{locale}"), url_for(request.query_parameters.merge(locale: locale)), 'data-turbolinks' => "false", class: 'dropdown-item') diff --git a/app/views/sessions/destroy_through_lti.html.slim b/app/views/sessions/destroy_through_lti.html.slim index cd996afa..e95ab6b4 100644 --- a/app/views/sessions/destroy_through_lti.html.slim +++ b/app/views/sessions/destroy_through_lti.html.slim @@ -1,9 +1,11 @@ h1 = t('.headline') +- consumer = @submission.user.consumer + p - == t(".success_#{params[:outcome] ? 'with' : 'without'}_outcome", consumer: @consumer) - ==< t(".finished_#{@consumer ? 'with' : 'without'}_consumer", consumer: @consumer, url: params[:url]) - ==< t(".do_not_use_backbutton", consumer: @consumer) + = t(".success_#{consumer ? 'with' : 'without'}_outcome", consumer: consumer) + ==< t(".finished_#{consumer ? 'with' : 'without'}_consumer", consumer: h(consumer.name), url: @url) + =< t(".do_not_use_backbutton") h2 = t('shared.statistics') diff --git a/config/environments/production.rb b/config/environments/production.rb index 39abff0a..a06058c7 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -49,7 +49,7 @@ Rails.application.configure do # config.action_cable.allowed_request_origins = [ 'http://example.com', /http:\/\/example.*/ ] # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - # config.force_ssl = true + config.force_ssl = true # Include generic and useful information about system operation, but avoid logging too much # information to avoid inadvertent exposure of personally identifiable information (PII). diff --git a/config/environments/staging.rb b/config/environments/staging.rb index 73832c26..f098937f 100644 --- a/config/environments/staging.rb +++ b/config/environments/staging.rb @@ -60,7 +60,7 @@ Rails.application.configure do # config.action_dispatch.x_sendfile_header = 'X-Accel-Redirect' # for nginx # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - # config.force_ssl = true + config.force_ssl = true # Set to :debug to see everything in the log. config.log_level = :info diff --git a/lib/assessor.rb b/lib/assessor.rb index b7f68e16..5b47e711 100644 --- a/lib/assessor.rb +++ b/lib/assessor.rb @@ -23,7 +23,7 @@ class Assessor def initialize(options = {}) if options[:execution_environment].testing_framework? - @testing_framework_adapter = Kernel.const_get(options[:execution_environment].testing_framework).new + @testing_framework_adapter = options[:execution_environment].testing_framework.constantize.new else raise Error.new('No testing framework adapter set!') end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index df3a7351..0e026c5b 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -201,13 +201,14 @@ describe SessionsController do end describe 'GET #destroy_through_lti' do - let(:perform_request) { proc { get :destroy_through_lti, params: {consumer_id: consumer.id, submission_id: submission.id} } } + let(:perform_request) { proc { get :destroy_through_lti, params: {submission_id: submission.id} } } let(:submission) { create(:submission, exercise: create(:dummy)) } before do # Todo replace session with lti_parameter # Todo create LtiParameter Object # session[:lti_parameters] = {} + allow(controller).to receive(:current_user).and_return(submission.user) perform_request.call end