From 8e3075e84eb9997bcf8a1d36239938c538f7a187 Mon Sep 17 00:00:00 2001 From: cablett Date: Fri, 3 Mar 2023 20:37:21 +1300 Subject: [PATCH] Add rails endpoint for work item import - Add specs - Documentation - Workhorse/Go authorisation - Support Workhorse upload acceleration Changelog: added --- .../projects/work_items_controller.rb | 56 ++++++ app/services/import_csv/base_service.rb | 1 + .../work_items/prepare_import_csv_service.rb | 19 ++ app/workers/all_queues.yml | 9 + .../import_work_items_csv_worker.rb | 29 +++ config/routes/project.rb | 7 + config/sidekiq_queues.yml | 2 + locale/gitlab.pot | 3 + .../projects/work_items_controller_spec.rb | 156 ++++++++++++++++ spec/requests/projects/work_items_spec.rb | 176 +++++++++++++++++- .../prepare_import_csv_service_spec.rb | 52 ++++++ spec/workers/every_sidekiq_worker_spec.rb | 1 + .../import_work_items_csv_worker_spec.rb | 44 +++++ workhorse/internal/upstream/routes.go | 3 + workhorse/upload_test.go | 2 + 15 files changed, 557 insertions(+), 3 deletions(-) create mode 100644 app/services/work_items/prepare_import_csv_service.rb create mode 100644 app/workers/work_items/import_work_items_csv_worker.rb create mode 100644 spec/controllers/projects/work_items_controller_spec.rb create mode 100644 spec/services/work_items/prepare_import_csv_service_spec.rb create mode 100644 spec/workers/work_items/import_work_items_csv_worker_spec.rb diff --git a/app/controllers/projects/work_items_controller.rb b/app/controllers/projects/work_items_controller.rb index 34a71dbbb91b2a..7da31c199a12b9 100644 --- a/app/controllers/projects/work_items_controller.rb +++ b/app/controllers/projects/work_items_controller.rb @@ -1,6 +1,12 @@ # frozen_string_literal: true class Projects::WorkItemsController < Projects::ApplicationController + include WorkhorseAuthorization + extend Gitlab::Utils::Override + + EXTENSION_ALLOWLIST = %w[csv].map(&:downcase).freeze + + before_action :authorize_import_access!, only: [:import_csv, :authorize] # rubocop:disable Rails/LexicallyScopedActionFilter before_action do push_force_frontend_feature_flag(:work_items, project&.work_items_feature_flag_enabled?) push_force_frontend_feature_flag(:work_items_mvc, project&.work_items_mvc_feature_flag_enabled?) @@ -9,7 +15,57 @@ class Projects::WorkItemsController < Projects::ApplicationController end feature_category :team_planning + urgency :high, [:authorize] urgency :low + + def import_csv + file = import_params[:file] + return render json: { errors: invalid_file_message }, status: :bad_request unless file_is_valid?(file) + + result = WorkItems::PrepareImportCsvService.new(project, current_user, file: file).execute + + if result.status == :error + render json: { errors: result.message }, status: :bad_request + else + render json: { message: result.message }, status: :ok + end + end + + private + + def import_params + params.permit(:file) + end + + def authorize_import_access! + can_import = can?(current_user, :import_work_items, project) + import_csv_feature_available = Feature.enabled?(:import_export_work_items_csv, project) + return if can_import && import_csv_feature_available + + if current_user || action_name == 'authorize' + render_404 + else + authenticate_user! + end + end + + def invalid_file_message + supported_file_extensions = ".#{EXTENSION_ALLOWLIST.join(', .')}" + format(_("The uploaded file was invalid. Supported file extensions are %{extensions}."), + { extensions: supported_file_extensions }) + end + + def uploader_class + FileUploader + end + + def maximum_size + Gitlab::CurrentSettings.max_attachment_size.megabytes + end + + def file_extension_allowlist + EXTENSION_ALLOWLIST + end end Projects::WorkItemsController.prepend_mod diff --git a/app/services/import_csv/base_service.rb b/app/services/import_csv/base_service.rb index 1d27a5811c7c62..70834b8a85ac5c 100644 --- a/app/services/import_csv/base_service.rb +++ b/app/services/import_csv/base_service.rb @@ -42,6 +42,7 @@ def create_object_class def validate_structure! header_line = csv_data.lines.first + raise CSV::MalformedCSVError.new('File is empty, no headers found', 1) if header_line.blank? validate_headers_presence!(header_line) detect_col_sep diff --git a/app/services/work_items/prepare_import_csv_service.rb b/app/services/work_items/prepare_import_csv_service.rb new file mode 100644 index 00000000000000..a331b2870f405f --- /dev/null +++ b/app/services/work_items/prepare_import_csv_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module WorkItems + class PrepareImportCsvService < Import::PrepareService + extend ::Gitlab::Utils::Override + + private + + override :worker + def worker + ImportWorkItemsCsvWorker + end + + override :success_message + def success_message + _("Your work items are being imported. Once finished, you'll receive a confirmation email.") + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 1624538152e5d6..1436828019c036 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3504,6 +3504,15 @@ :weight: 1 :idempotent: false :tags: [] +- :name: work_items_import_work_items_csv + :worker_name: WorkItems::ImportWorkItemsCsvWorker + :feature_category: :team_planning + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: x509_certificate_revoke :worker_name: X509CertificateRevokeWorker :feature_category: :source_code_management diff --git a/app/workers/work_items/import_work_items_csv_worker.rb b/app/workers/work_items/import_work_items_csv_worker.rb new file mode 100644 index 00000000000000..be7294866df73c --- /dev/null +++ b/app/workers/work_items/import_work_items_csv_worker.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module WorkItems + class ImportWorkItemsCsvWorker + include ApplicationWorker + + data_consistency :always + + sidekiq_options retry: 3 + + idempotent! + feature_category :team_planning + + sidekiq_retries_exhausted do |job| + Upload.find(job['args'][2]).destroy + end + + def perform(current_user_id, project_id, upload_id) + upload = Upload.find(upload_id) + user = User.find(current_user_id) + project = Project.find(project_id) + + WorkItems::ImportCsvService.new(user, project, upload.retrieve_uploader).execute + upload.destroy! + rescue ActiveRecord::RecordNotFound + # Resources have been removed, job should not be retried + end + end +end diff --git a/config/routes/project.rb b/config/routes/project.rb index 285f01538d1e74..eb96d6ba101fe8 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -360,6 +360,13 @@ get 'work_items/*work_items_path' => 'work_items#index', as: :work_items get 'work_items/*work_items_path' => 'work_items#index', as: :work_item + resources :work_items, only: [] do + collection do + post :import_csv + post 'import_csv/authorize', to: 'work_items#authorize' + end + end + post 'incidents/integrations/pagerduty', to: 'incident_management/pager_duty_incidents#create' resources :incidents, only: [:index] diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 862696d39bcf38..d2badbc5f1d74e 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -561,6 +561,8 @@ - 1 - - wikis_git_garbage_collect - 1 +- - work_items_import_work_items_csv + - 1 - - work_items_update_parent_objectives_progress - 1 - - x509_certificate_revoke diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8f16eef3a4be80..69fb61260f2e88 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -50749,6 +50749,9 @@ msgstr "" msgid "Your work" msgstr "" +msgid "Your work items are being imported. Once finished, you'll receive a confirmation email." +msgstr "" + msgid "You’re about to permanently delete the %{issuableType} ‘%{strongOpen}%{issuableTitle}%{strongClose}’. To avoid data loss, consider %{strongOpen}closing this %{issuableType}%{strongClose} instead. Once deleted, it cannot be undone or recovered." msgstr "" diff --git a/spec/controllers/projects/work_items_controller_spec.rb b/spec/controllers/projects/work_items_controller_spec.rb new file mode 100644 index 00000000000000..e0f61a4977be2c --- /dev/null +++ b/spec/controllers/projects/work_items_controller_spec.rb @@ -0,0 +1,156 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::WorkItemsController, feature_category: :team_planning do + let_it_be(:reporter) { create(:user) } + let_it_be(:guest) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:work_item) { create(:work_item, project: project) } + + let(:file) { 'file' } + + before do + project.add_reporter(reporter) + project.add_guest(guest) + end + + shared_examples 'response with 404 status' do + it 'renders a not found message' do + expect(WorkItems::ImportWorkItemsCsvWorker).not_to receive(:perform_async) + + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + shared_examples 'redirects to new session path' do + it 'redirects to sign in' do + subject + + expect(response).to have_gitlab_http_status(:found) + expect(response).to redirect_to(new_user_session_path) + end + end + + describe 'GET index' do + specify do + expect( + get(:index, params: { namespace_id: project.namespace, project_id: project, work_items_path: work_item.id }) + ).to have_request_urgency(:low) + end + end + + describe 'POST authorize' do + subject do + post(:authorize, params: { namespace_id: project.namespace, project_id: project, file: file }) + end + + specify do + expect(subject).to have_request_urgency(:high) + end + + context 'when user is anonymous' do + it_behaves_like 'redirects to new session path' + end + end + + describe 'POST import_csv' do + subject { post :import_csv, params: { namespace_id: project.namespace, project_id: project, file: file } } + + let(:upload_service) { double } + let(:uploader) { double } + let(:upload) { double } + let(:upload_id) { 99 } + + specify do + expect(subject).to have_request_urgency(:low) + end + + context 'with authorized user' do + before do + sign_in(reporter) + allow(controller).to receive(:file_is_valid?).and_return(true) + end + + context 'when feature is available' do + context 'when the upload is processed successfully' do + before do + mock_upload + end + + it 'renders the correct message' do + expect(WorkItems::ImportWorkItemsCsvWorker).to receive(:perform_async) + .with(reporter.id, project.id, upload_id) + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['message']).to eq( + "Your work items are being imported. Once finished, you'll receive a confirmation email." + ) + end + end + + context 'when file is not valid' do + before do + allow(controller).to receive(:file_is_valid?).and_return(false) + end + + it 'renders the error message' do + expect(WorkItems::ImportWorkItemsCsvWorker).not_to receive(:perform_async) + + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['errors']) + .to eq('The uploaded file was invalid. Supported file extensions are .csv.') + end + end + + context 'when service response includes errors' do + before do + mock_upload(false) + end + + it 'renders the error message' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['errors']).to eq('File upload error.') + end + end + end + + context 'when feature is not available' do + before do + stub_feature_flags(import_export_work_items_csv: false) + end + + it_behaves_like 'response with 404 status' + end + end + + context 'with unauthorised user' do + before do + mock_upload + sign_in(guest) + allow(controller).to receive(:file_is_valid?).and_return(true) + end + + it_behaves_like 'response with 404 status' + end + + context 'with anonymous user' do + it 'redirects to sign in page' do + expect(WorkItems::ImportWorkItemsCsvWorker).not_to receive(:perform_async) + + subject + + expect(response).to have_gitlab_http_status(:found) + expect(response).to redirect_to(new_user_session_path) + end + end + end +end diff --git a/spec/requests/projects/work_items_spec.rb b/spec/requests/projects/work_items_spec.rb index 056416d380d6b8..c13707e1945b60 100644 --- a/spec/requests/projects/work_items_spec.rb +++ b/spec/requests/projects/work_items_spec.rb @@ -3,16 +3,41 @@ require 'spec_helper' RSpec.describe 'Work Items', feature_category: :team_planning do + include WorkhorseHelpers + + include_context 'workhorse headers' + let_it_be(:work_item) { create(:work_item) } - let_it_be(:developer) { create(:user) } + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project) } + + let(:file) { fixture_file_upload("spec/fixtures/#{filename}") } before_all do - work_item.project.add_developer(developer) + work_item.project.add_developer(current_user) + end + + shared_examples 'response with 404 status' do + it 'returns 404' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + shared_examples 'safely handles uploaded files', :aggregate_failures do + it 'ensures the upload is handled safely' do + allow(Gitlab::Utils).to receive(:check_path_traversal!).and_call_original + expect(Gitlab::Utils).to receive(:check_path_traversal!).with(filename).at_least(:once) + expect(FileUploader).not_to receive(:cache) + + subject + end end describe 'GET /:namespace/:project/work_items/:id' do before do - sign_in(developer) + sign_in(current_user) end it 'renders index' do @@ -21,4 +46,149 @@ expect(response).to have_gitlab_http_status(:ok) end end + + describe 'POST /:namespace/:project/work_items/import_csv' do + let(:filename) { 'work_items_valid_types.csv' } + let(:params) { { namespace_id: project.namespace.id, path: 'test' } } + + subject { upload_file(file, workhorse_headers, params) } + + shared_examples 'handles authorisation' do + context 'when unauthorized' do + context 'with non-member' do + let_it_be(:current_user) { create(:user) } + + before do + sign_in(current_user) + end + + it 'responds with error' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'with anonymous user' do + it 'responds with error' do + subject + + expect(response).to have_gitlab_http_status(:found) + expect(response).to be_redirect + end + end + end + + context 'when authorized' do + before do + sign_in(current_user) + project.add_reporter(current_user) + end + + context 'when import/export work items feature is available and member is a reporter' do + shared_examples 'response with success status' do + it 'returns 200 status and success message' do + subject + + expect(response).to have_gitlab_http_status(:success) + expect(json_response).to eq( + 'message' => "Your work items are being imported. Once finished, you'll receive a confirmation email.") + end + end + + it_behaves_like 'response with success status' + it_behaves_like 'safely handles uploaded files' + + it 'shows error when upload fails' do + expect_next_instance_of(UploadService) do |upload_service| + expect(upload_service).to receive(:execute).and_return(nil) + end + + subject + + expect(json_response).to eq('errors' => 'File upload error.') + end + + context 'when file extension is not csv' do + let(:filename) { 'sample_doc.md' } + + it 'returns error message' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response).to eq( + 'errors' => "The uploaded file was invalid. Supported file extensions are .csv.") + end + end + end + + context 'when work items import/export feature is not available' do + before do + stub_feature_flags(import_export_work_items_csv: false) + end + + it_behaves_like 'response with 404 status' + end + end + end + + context 'with public project' do + let_it_be(:project) { create(:project, :public) } + + it_behaves_like 'handles authorisation' + end + + context 'with private project' do + it_behaves_like 'handles authorisation' + end + + def upload_file(file, headers = {}, params = {}) + workhorse_finalize( + import_csv_project_work_items_path(project), + method: :post, + file_key: :file, + params: params.merge(file: file), + headers: headers, + send_rewritten_field: true + ) + end + end + + describe 'POST #authorize' do + subject do + post import_csv_authorize_project_work_items_path(project), + headers: workhorse_headers + end + + before do + sign_in(current_user) + end + + context 'with authorized user' do + before do + project.add_reporter(current_user) + end + + context 'when work items import/export feature is enabled' do + let(:user) { current_user } + + it_behaves_like 'handle uploads authorize request' do + let(:uploader_class) { FileUploader } + let(:maximum_size) { Gitlab::CurrentSettings.max_attachment_size.megabytes } + end + end + + context 'when work items import/export feature is disabled' do + before do + stub_feature_flags(import_export_work_items_csv: false) + end + + it_behaves_like 'response with 404 status' + end + end + + context 'with unauthorized user' do + it_behaves_like 'response with 404 status' + end + end end diff --git a/spec/services/work_items/prepare_import_csv_service_spec.rb b/spec/services/work_items/prepare_import_csv_service_spec.rb new file mode 100644 index 00000000000000..6a657120690857 --- /dev/null +++ b/spec/services/work_items/prepare_import_csv_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::PrepareImportCsvService, feature_category: :team_planning do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let(:file) { double } + let(:upload_service) { double } + let(:uploader) { double } + let(:upload) { double } + + let(:subject) do + described_class.new(project, user, file: file).execute + end + + context 'when file is uploaded correctly' do + let(:upload_id) { 99 } + + before do + mock_upload + end + + it 'returns a success message' do + result = subject + + expect(result[:status]).to eq(:success) + expect(result[:message]).to eq( + "Your work items are being imported. Once finished, you'll receive a confirmation email.") + end + + it 'enqueues the ImportWorkItemsCsvWorker' do + expect(WorkItems::ImportWorkItemsCsvWorker).to receive(:perform_async).with(user.id, project.id, upload_id) + + subject + end + end + + context 'when file upload fails' do + before do + mock_upload(false) + end + + it 'returns an error message' do + result = subject + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('File upload error.') + end + end +end diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 9155c04dd01745..ece36ad39a58e3 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -478,6 +478,7 @@ 'WebHooks::DestroyWorker' => 3, 'WebHooks::LogExecutionWorker' => 3, 'Wikis::GitGarbageCollectWorker' => false, + 'WorkItems::ImportWorkItemsCsvWorker' => 3, 'X509CertificateRevokeWorker' => 3, 'ComplianceManagement::MergeRequests::ComplianceViolationsWorker' => 3 } diff --git a/spec/workers/work_items/import_work_items_csv_worker_spec.rb b/spec/workers/work_items/import_work_items_csv_worker_spec.rb new file mode 100644 index 00000000000000..056960fbcf2d1f --- /dev/null +++ b/spec/workers/work_items/import_work_items_csv_worker_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::ImportWorkItemsCsvWorker, feature_category: :team_planning do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let(:upload) { create(:upload, :with_file) } + + before_all do + project.add_reporter(user) + end + + subject { described_class.new.perform(user.id, project.id, upload.id) } + + describe '#perform' do + it 'calls #execute on WorkItems::ImportCsvService and destroys upload' do + expect_next_instance_of(WorkItems::ImportCsvService) do |instance| + expect(instance).to receive(:execute).and_return({ success: 5, error_lines: [], parse_error: false }) + end + + subject + + expect { upload.reload }.to raise_error ActiveRecord::RecordNotFound + end + + it_behaves_like 'an idempotent worker' do + let(:job_args) { [user.id, project.id, upload.id] } + end + end + + describe '.sidekiq_retries_exhausted' do + let_it_be(:job) { { 'args' => [user.id, project.id, create(:upload, :with_file).id] } } + + subject(:sidekiq_retries_exhausted) do + described_class.sidekiq_retries_exhausted_block.call(job) + end + + it 'destroys upload' do + expect { sidekiq_retries_exhausted }.to change { Upload.count }.by(-1) + end + end +end diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index 982f3a5b5f8804..8f9d056b8f31f0 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -325,6 +325,9 @@ func configureRoutes(u *upstream) { // Requirements Import via UI upload acceleration u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, mimeMultipartUploader), + // Work items Import via UI upload acceleration + u.route("POST", projectPattern+`work_items/import_csv`, mimeMultipartUploader), + // Uploads via API u.route("POST", apiProjectPattern+`/uploads\z`, mimeMultipartUploader), diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index 333301091c7299..cf308504e21582 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -176,6 +176,8 @@ func TestAcceleratedUpload(t *testing.T) { {"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/issues/30/metric_images`, true}, {"POST", `/my/project/-/requirements_management/requirements/import_csv`, true}, {"POST", `/my/project/-/requirements_management/requirements/import_csv/`, true}, + {"POST", `/my/project/-/work_items/import_csv`, true}, + {"POST", `/my/project/-/work_items/import_csv/`, true}, {"POST", "/api/v4/projects/2412/packages/helm/api/stable/charts", true}, {"POST", "/api/v4/projects/group%2Fproject/packages/helm/api/stable/charts", true}, {"POST", "/api/v4/projects/group%2Fsubgroup%2Fproject/packages/helm/api/stable/charts", true}, -- GitLab