From 66a777c24c6596453b7826c3b42198216ba66684 Mon Sep 17 00:00:00 2001 From: Katherine Thiessen Date: Fri, 31 May 2024 09:29:47 -0500 Subject: [PATCH] [DFCT0010059] Fixing user permissions for project workflow executions (#618) * updating the view_workflow_execution policy within project namespace * adding a couple tests * updating can_create_export policy to include analysts * fixing flaky submission tests --- app/models/member.rb | 10 ++++--- .../namespaces/project_namespace_policy.rb | 3 +- app/views/layouts/projects.html.erb | 29 ++++++++++--------- config/locales/en.yml | 1 + .../workflow_executions_controller_test.rb | 17 +++++++++++ test/fixtures/members.yml | 6 ++++ .../data_exports/create_service_test.rb | 19 ++++++++++++ .../workflow_executions/submissions_test.rb | 14 +++++++++ 8 files changed, 80 insertions(+), 19 deletions(-) diff --git a/app/models/member.rb b/app/models/member.rb index 53c58c731d..d285a0ef42 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -29,7 +29,7 @@ class Member < ApplicationRecord # rubocop:disable Metrics/ClassLength scope :not_expired, -> { where('expires_at IS NULL OR expires_at > ?', Time.zone.now.beginning_of_day) } - class << self # rubocop:disable Metrics/ClassLength + class << self def access_levels(member) case member.access_level when AccessLevel::OWNER @@ -121,6 +121,10 @@ def can_update_namespace_with_group_link?(user, object_namespace) can_modify?(user, object_namespace) end + def can_view_workflows?(user, object_namespace) + effective_access_level(object_namespace, user) >= Member::AccessLevel::ANALYST + end + def can_submit_workflow?(user, object_namespace) effective_access_level(object_namespace, user) >= Member::AccessLevel::ANALYST end @@ -134,9 +138,7 @@ def user_has_namespace_maintainer_access?(user, namespace, include_group_links = end def can_create_export?(user, object_namespace) - Member::AccessLevel.manageable.include?( - effective_access_level(object_namespace, user) - ) + effective_access_level(object_namespace, user) >= Member::AccessLevel::ANALYST end def access_level_in_namespace_group_links(user, namespace) diff --git a/app/policies/namespaces/project_namespace_policy.rb b/app/policies/namespaces/project_namespace_policy.rb index 72bfe64cf7..fb142cd2db 100644 --- a/app/policies/namespaces/project_namespace_policy.rb +++ b/app/policies/namespaces/project_namespace_policy.rb @@ -155,8 +155,7 @@ def submit_workflow? end def view_workflow_executions? - return true if record.parent.user_namespace? && record.parent.owner == user - return true if Member.can_view?(user, record) == true + return true if Member.can_view_workflows?(user, record) == true details[:name] = record.name false diff --git a/app/views/layouts/projects.html.erb b/app/views/layouts/projects.html.erb index 599555f4ba..c63e1c0ca6 100644 --- a/app/views/layouts/projects.html.erb +++ b/app/views/layouts/projects.html.erb @@ -9,48 +9,51 @@ url: project_path(@project), icon: "clipboard_document", label: t(:"projects.sidebar.details"), - selected: @current_page == t(:"projects.sidebar.details").downcase + selected: @current_page == t(:"projects.sidebar.details").downcase, ) %> <%= render section.with_item( url: namespace_project_members_path, icon: "users", label: t(:"projects.sidebar.members"), - selected: @current_page == t(:"projects.sidebar.members").downcase + selected: @current_page == t(:"projects.sidebar.members").downcase, ) %> <%= render section.with_item( url: project_samples_path(@project), icon: "beaker", label: t(:"projects.sidebar.samples"), - selected: @current_page == t(:"projects.sidebar.samples").downcase + selected: @current_page == t(:"projects.sidebar.samples").downcase, ) %> <% if allowed_to?(:view_history?, @project) %> <%= render section.with_item( url: project_history_path(@project), icon: "list_bullet", label: t(:"projects.sidebar.history"), - selected: @current_page == t(:"projects.sidebar.history").downcase + selected: @current_page == t(:"projects.sidebar.history").downcase, + ) %> + <% end %> + <% if allowed_to?(:view_workflow_executions?, @project.namespace) %> + <%= render section.with_item( + url: project_workflow_executions_path(@project), + icon: "command_line", + label: t(:"projects.sidebar.workflow_executions"), + selected: + @current_page == t(:"projects.sidebar.workflow_executions").downcase, ) %> <% end %> - <%= render section.with_item( - url: project_workflow_executions_path(@project), - icon: "command_line", - label: t(:"projects.sidebar.workflow_executions"), - selected: @current_page == t(:"projects.sidebar.workflow_executions").downcase - ) %> <% if allowed_to?(:update?, @project) %> <%= render section.with_multi_level_menu(title: t(:"projects.sidebar.settings"), icon: "cog_6_tooth") do |multi_level_menu| %> <% multi_level_menu.with_menu_item( url: project_edit_path(@project), - label: t(:"projects.sidebar.general") + label: t(:"projects.sidebar.general"), ) %> <% multi_level_menu.with_menu_item( url: project_bots_path(@project), - label: t(:"projects.sidebar.bot_accounts") + label: t(:"projects.sidebar.bot_accounts"), ) %> <% multi_level_menu.with_menu_item( url: project_automated_workflow_executions_path(@project), - label: t(:"projects.sidebar.automated_workflow_executions") + label: t(:"projects.sidebar.automated_workflow_executions"), ) %> <% end %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 2fe0804d4d..cabddcd784 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -107,6 +107,7 @@ en: update_automated_workflow_executions?: "You are not authorized to update automated workflow executions for project %{name}" view_automated_workflow_executions?: "You are not authorized to view automated workflow executions for project %{name}" submit_workflow?: You are not authorized to submit workflows for samples within project %{name} on this server. + view_workflow_executions?: You are not authorized to view workflow executions for project %{name} on this server. namespaces/user_namespace: create?: You are not authorized to create a project under the %{name} namespace transfer_into_namespace?: You are not authorized to transfer to the user %{name} namespace diff --git a/test/controllers/projects/workflow_executions_controller_test.rb b/test/controllers/projects/workflow_executions_controller_test.rb index 9466b5ef9f..6cbd4783e9 100644 --- a/test/controllers/projects/workflow_executions_controller_test.rb +++ b/test/controllers/projects/workflow_executions_controller_test.rb @@ -27,6 +27,14 @@ class WorfklowExecutionsControllerTest < ActionDispatch::IntegrationTest assert_response :unauthorized end + test 'should not show a listing of project workflow executions for guests' do + sign_in users(:ryan_doe) + + get namespace_project_workflow_executions_path(@namespace, @project, format: :turbo_stream) + + assert_response :unauthorized + end + test 'should show workflow execution' do workflow_execution = workflow_executions(:automated_workflow_execution) @@ -44,6 +52,15 @@ class WorfklowExecutionsControllerTest < ActionDispatch::IntegrationTest assert_response :unauthorized end + test 'should not show project workflow execution for guests' do + sign_in users(:ryan_doe) + workflow_execution = workflow_executions(:automated_workflow_execution) + + get namespace_project_workflow_execution_path(@namespace, @project, workflow_execution) + + assert_response :unauthorized + end + test 'should cancel a new workflow with valid params' do workflow_execution = workflow_executions(:automated_example_new) diff --git a/test/fixtures/members.yml b/test/fixtures/members.yml index 0227259ed0..20d2e9fb2f 100644 --- a/test/fixtures/members.yml +++ b/test/fixtures/members.yml @@ -92,6 +92,12 @@ project_one_member_john_doe: namespace_id: <%= ActiveRecord::FixtureSet.identify(:project1_namespace, :uuid) %> access_level: <%= Member::AccessLevel::OWNER %> +project_one_member_james_doe: + user_id: <%= ActiveRecord::FixtureSet.identify(:james_doe, :uuid) %> + created_by_id: <%= ActiveRecord::FixtureSet.identify(:john_doe, :uuid) %> + namespace_id: <%= ActiveRecord::FixtureSet.identify(:project1_namespace, :uuid) %> + access_level: <%= Member::AccessLevel::ANALYST %> + project_one_member_ryan_doe: user_id: <%= ActiveRecord::FixtureSet.identify(:ryan_doe, :uuid) %> created_by_id: <%= ActiveRecord::FixtureSet.identify(:john_doe, :uuid) %> diff --git a/test/services/data_exports/create_service_test.rb b/test/services/data_exports/create_service_test.rb index 3437682ebd..ae890c4e5b 100644 --- a/test/services/data_exports/create_service_test.rb +++ b/test/services/data_exports/create_service_test.rb @@ -134,6 +134,25 @@ def setup end end + test 'analyst authorized to create workflow execution export' do + valid_params = { 'export_type' => 'analysis', + 'export_parameters' => { 'ids' => [@workflow_execution1.id] } } + user = users(:james_doe) + + assert_authorized_to(:export_workflow_execution_data?, @workflow_execution1, with: WorkflowExecutionPolicy, + context: { user: }) do + DataExports::CreateService.new(user, valid_params).execute + end + end + + test 'guest unauthorized to create workflow execution export' do + valid_params = { 'export_type' => 'analysis', + 'export_parameters' => { 'ids' => [@workflow_execution1.id] } } + user = users(:ryan_doe) + + assert_raises(ActionPolicy::Unauthorized) { DataExports::CreateService.new(user, valid_params).execute } + end + test 'data export with valid parameters but unauthorized for workflow execution export' do valid_params = { 'export_type' => 'analysis', 'export_parameters' => { 'ids' => [@workflow_execution1.id] } } diff --git a/test/system/workflow_executions/submissions_test.rb b/test/system/workflow_executions/submissions_test.rb index 0958908954..c0df87c0ee 100644 --- a/test/system/workflow_executions/submissions_test.rb +++ b/test/system/workflow_executions/submissions_test.rb @@ -16,6 +16,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit namespace_project_samples_url(namespace_id: @namespace.path, project_id: @project.path) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click @@ -46,6 +48,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit namespace_project_samples_url(namespace_id: @namespace.path, project_id: @project.path) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click @@ -76,6 +80,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit namespace_project_samples_url(namespace_id: @namespace.path, project_id: @project.path) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click @@ -110,6 +116,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit namespace_project_samples_url(namespace_id: namespace.path, project_id: project.path) + assert_text 'Displaying 1 item' + within 'table' do find("input[type='checkbox'][value='#{sample.id}']").click end @@ -146,6 +154,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit group_samples_url(@namespace) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click @@ -176,6 +186,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit group_samples_url(@namespace) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click @@ -206,6 +218,8 @@ class SubmissionsTest < ApplicationSystemTestCase visit group_samples_url(@namespace) + assert_text 'Displaying 3 items' + within 'table' do find("input[type='checkbox'][value='#{@sample43.id}']").click find("input[type='checkbox'][value='#{@sample44.id}']").click