From 73541e8c42daaeb93b5821783cd4c55f94b6eabf Mon Sep 17 00:00:00 2001 From: Martin Artnik Date: Wed, 31 Jan 2024 13:52:56 +0100 Subject: [PATCH] Fix experiment bulk move permission checks, specs [SCI-9840] --- app/controllers/experiments_controller.rb | 27 +++++++++---------- app/permissions/experiment.rb | 3 ++- app/views/experiments/_move_modal.html.erb | 12 ++++----- config/routes.rb | 4 +-- .../experiments_controller_spec.rb | 5 ++-- 5 files changed, 25 insertions(+), 26 deletions(-) diff --git a/app/controllers/experiments_controller.rb b/app/controllers/experiments_controller.rb index 76997ff2a..f7ded1a67 100644 --- a/app/controllers/experiments_controller.rb +++ b/app/controllers/experiments_controller.rb @@ -8,15 +8,17 @@ class ExperimentsController < ApplicationController include Rails.application.routes.url_helpers include Breadcrumbs - before_action :load_project, only: %i(new create archive_group restore_group) + before_action :load_project, only: %i(new create archive_group restore_group move) before_action :load_experiment, except: %i(new create archive_group restore_group - inventory_assigning_experiment_filter actions_toolbar move_modal move) + inventory_assigning_experiment_filter actions_toolbar + move move_modal) before_action :load_experiments, only: %i(move_modal move) - before_action :check_read_permissions, except: %i(edit archive clone move new + before_action :check_move_permissions, only: %i(move_modal move) + before_action :check_read_permissions, except: %i(edit archive clone move move_modal new create archive_group restore_group - inventory_assigning_experiment_filter actions_toolbar move_modal) + inventory_assigning_experiment_filter actions_toolbar) before_action :check_canvas_read_permissions, only: %i(canvas) - before_action :check_create_permissions, only: %i(new create) + before_action :check_create_permissions, only: %i(new create move) before_action :check_manage_permissions, only: %i(edit batch_clone_my_modules) before_action :check_update_permissions, only: %i(update) before_action :check_archive_permissions, only: :archive @@ -254,7 +256,7 @@ class ExperimentsController < ApplicationController # POST: clone_experiment(id) def clone - project = current_team.projects.find(move_experiment_param) + @project = current_team.projects.find(move_experiment_param) return render_403 unless can_create_project_experiments?(project) service = Experiments::CopyExperimentAsTemplateService.call(experiment: @experiment, @@ -297,10 +299,7 @@ class ExperimentsController < ApplicationController # POST: move_experiment(id) def move - project = Project.viewable_by_user(current_user, current_team) - .find_by(id: params[:project_id]) - - project.transaction do + @project.transaction do @experiments.each do |experiment| service = Experiments::MoveToProjectService .call(experiment_id: experiment.id, @@ -309,14 +308,14 @@ class ExperimentsController < ApplicationController raise StandardError unless service.succeed? end - flash[:success] = t('experiments.table.move_success_flash', project: escape_input(project.name)) + flash[:success] = t('experiments.table.move_success_flash', project: escape_input(@project.name)) render json: { message: t('experiments.table.move_success_flash', - project: escape_input(project.name)), path: project_path(project) } + project: escape_input(@project.name)), path: project_path(@project) } rescue StandardError => e Rails.logger.error(e.message) Rails.logger.error(e.backtrace.join("\n")) render json: { - message: t('experiments.table.move_error_flash', project: escape_input(project.name)) + message: t('experiments.table.move_error_flash', project: escape_input(@project.name)) }, status: :unprocessable_entity raise ActiveRecord::Rollback end @@ -596,7 +595,7 @@ class ExperimentsController < ApplicationController end def check_move_permissions - render_403 unless can_move_experiment?(@experiment) + render_403 unless @experiments.all? { |e| can_move_experiment?(e) } end def set_inline_name_editing diff --git a/app/permissions/experiment.rb b/app/permissions/experiment.rb index bc7c3c9d6..595b2053c 100644 --- a/app/permissions/experiment.rb +++ b/app/permissions/experiment.rb @@ -82,7 +82,8 @@ Canaid::Permissions.register_for(Experiment) do end can :move_experiment do |user, experiment| - experiment.permission_granted?(user, ExperimentPermissions::MANAGE) + experiment.permission_granted?(user, ExperimentPermissions::MANAGE) && + can_manage_all_experiment_my_modules?(experiment) end can :designate_users_to_new_task do |user, experiment| diff --git a/app/views/experiments/_move_modal.html.erb b/app/views/experiments/_move_modal.html.erb index e2cc90aa1..bce372516 100644 --- a/app/views/experiments/_move_modal.html.erb +++ b/app/views/experiments/_move_modal.html.erb @@ -1,10 +1,10 @@

<%= t("experiments.move.notice") %>

- <% if @projects.any? && @experiments.all? { |experiment| can_manage_all_experiment_my_modules?(experiment) } %> + <% if @projects.any? && @experiments.all? { |experiment| can_move_experiment?(experiment) } %> <%= f.select :project_id, options_for_select(@projects.collect { |p| [ p.name, p.id ] }), { label: t("experiments.move.target_project") }, { class: "form-control selectpicker", "data-role" => "clear" } %> <% else %> @@ -24,14 +24,14 @@ <% if @projects.blank? %> <%= t("experiments.move.no_projects") %> - <% elsif !@experiments.all? { |experiment| can_manage_all_experiment_my_modules?(experiment) } %> + <% elsif !@experiments.all? { |experiment| can_move_experiment?(experiment) } %> <%= t("experiments.move.task_permission") %> <% end %>
<% end %> - <% @experiments.each do |experiment| %> - <%= f.hidden_field :ids, multiple: true, value: experiment.id %> + <% params[:ids].each do |id| %> + <%= f.hidden_field :ids, multiple: true, value: id %> <% end %>
diff --git a/config/routes.rb b/config/routes.rb index 3d425dc7c..dcf007686 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -387,6 +387,8 @@ Rails.application.routes.draw do get 'clone_modal', action: :clone_modal get 'move_modal', action: :move_modal get 'actions_toolbar' + get 'move_modal' # return modal with move options + post 'move' # move experiment end member do get 'permissions' @@ -410,8 +412,6 @@ Rails.application.routes.draw do post 'archive' # archive experiment get 'clone_modal' # return modal with clone options post 'clone' # clone experiment - get 'move_modal' # return modal with move options - post 'move' # move experiment get 'fetch_workflow_img' # Get updated workflow img get 'modules/new', to: 'my_modules#new' post 'modules', to: 'my_modules#create' diff --git a/spec/permissions/controllers/experiments_controller_spec.rb b/spec/permissions/controllers/experiments_controller_spec.rb index ec53040ee..6a1cacff0 100644 --- a/spec/permissions/controllers/experiments_controller_spec.rb +++ b/spec/permissions/controllers/experiments_controller_spec.rb @@ -16,7 +16,6 @@ describe ExperimentsController, type: :controller do archive_group: { project_id: 1 }, restore_group: { project_id: 1 }, clone: { id: 1 }, - move: { id: 1 }, module_archive: { id: 1 }, fetch_workflow_img: { id: 1 }, sidebar: { id: 1 }, @@ -112,13 +111,13 @@ describe ExperimentsController, type: :controller do it_behaves_like "a controller action with permissions checking", :get, :move_modal do let(:testable) { experiment } let(:permissions) { [ExperimentPermissions::MANAGE] } - let(:action_params) { { id: experiment.id } } + let(:action_params) { { ids: [experiment.id] } } end it_behaves_like "a controller action with permissions checking", :post, :move do let(:testable) { experiment } let(:permissions) { [ExperimentPermissions::MANAGE] } - let(:action_params) { { id: experiment.id } } + let(:action_params) { { ids: [experiment.id], project_id: project.id } } end it_behaves_like "a controller action with permissions checking", :get, :module_archive do