Merge branch 'security-415117-confidential-issue-16-1' into '16-1-stable-ee'

Add authorization checks to import status endpoint

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/3515



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Dylan Griffith <dyl.griffith@gmail.com>
Approved-by: Luke Duncalfe <lduncalfe@gitlab.com>
Co-authored-by: bmarjanovic <bmarjanovic@gitlab.com>

app/policies/project_policy.rb

@@ -561,6 +561,7 @@ class ProjectPolicy < BasePolicy
     enable :destroy_upload
     enable :admin_incident_management_timeline_event_tag
     enable :stop_environment
+    enable :read_import_error
   end
   rule { public_project & metrics_dashboard_allowed }.policy do

lib/api/entities/project_import_status.rb

@@ -17,8 +17,15 @@ class ProjectImportStatus < ProjectIdentity
         project.import_state&.relation_hard_failures(limit: 100) || []
       end
-      expose :import_error, documentation: { type: 'string', example: 'Error message' } do |project, _options|
-        project.import_state&.last_error
+      expose :import_error, documentation: { type: 'string', example: 'Error message' } do |project, options|
+        next unless options[:current_user]
+        next unless project.import_state&.last_error
+
+        if Ability.allowed?(options[:current_user], :read_import_error, project)
+          project.import_state&.last_error
+        else
+          _("Ask a maintainer to check the import status for more details.")
+        end
       end
       expose :stats, documentation: { type: 'object' } do |project, _options|

lib/api/project_import.rb

@@ -111,7 +111,7 @@ def filtered_override_params(params)
         ).execute
         if response.success?
-          present(response.payload, with: Entities::ProjectImportStatus)
+          present(response.payload, with: Entities::ProjectImportStatus, current_user: current_user)
         else
           render_api_error!(response.message, response.http_status)
         end
@@ -134,7 +134,7 @@ def filtered_override_params(params)
       end
       route_setting :skip_authentication, true
       get ':id/import' do
-        present user_project, with: Entities::ProjectImportStatus
+        present user_project, with: Entities::ProjectImportStatus, current_user: current_user
       end
       params do
@@ -182,7 +182,7 @@ def filtered_override_params(params)
         ).execute
         if response.success?
-          present(response.payload, with: Entities::ProjectImportStatus)
+          present(response.payload, with: Entities::ProjectImportStatus, current_user: current_user)
         else
           render_api_error!(response.message, response.http_status)
         end
@@ -241,7 +241,7 @@ def filtered_override_params(params)
         ).execute
         if response.success?
-          present(response.payload, with: Entities::ProjectImportStatus)
+          present(response.payload, with: Entities::ProjectImportStatus, current_user: current_user)
         else
           render_api_error!(response.message, response.http_status)
         end

locale/gitlab.pot

@@ -6300,6 +6300,9 @@ msgstr ""
 msgid "AsanaService|User Personal Access Token. User must have access to the task. All comments are attributed to this user."
 msgstr ""
+msgid "Ask a maintainer to check the import status for more details."
+msgstr ""
+
 msgid "Ask again later"
 msgstr ""

spec/lib/api/entities/project_import_status_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
-RSpec.describe API::Entities::ProjectImportStatus, :aggregate_failures do
+RSpec.describe API::Entities::ProjectImportStatus, :aggregate_failures, feature_category: :importers do
   describe '#as_json' do
     subject { entity.as_json }
@@ -67,14 +67,36 @@
     context 'when import has failed' do
       let(:project) { create(:project, :import_failed, import_type: 'import_type', import_correlation_id: correlation_id, import_last_error: 'error') }
-      let(:entity) { described_class.new(project) }
-      it 'includes basic fields with import error' do
-        expect(subject[:import_status]).to eq('failed')
-        expect(subject[:import_type]).to eq('import_type')
-        expect(subject[:correlation_id]).to eq(correlation_id)
-        expect(subject[:import_error]).to eq('error')
-        expect(subject[:failed_relations]).to eq([])
+      let(:current_user) { create(:user) }
+      let(:options) { { current_user: current_user } }
+      let(:entity) { described_class.new(project, options) }
+
+      context 'when user has access to read import status' do
+        before do
+          project.add_maintainer(current_user)
+        end
+
+        it 'includes basic fields with import error' do
+          expect(subject[:import_status]).to eq('failed')
+          expect(subject[:import_type]).to eq('import_type')
+          expect(subject[:correlation_id]).to eq(correlation_id)
+          expect(subject[:import_error]).to eq('error')
+          expect(subject[:failed_relations]).to eq([])
+        end
+      end
+      context 'when user does not have access to read import status' do
+        before do
+          project.add_reporter(current_user)
+        end
+
+        it 'includes basic fields with import error' do
+          expect(subject[:import_status]).to eq('failed')
+          expect(subject[:import_type]).to eq('import_type')
+          expect(subject[:correlation_id]).to eq(correlation_id)
+          expect(subject[:import_error]).to eq('Ask a maintainer to check the import status for more details.')
+          expect(subject[:failed_relations]).to eq([])
+        end
       end
     end

spec/policies/project_policy_spec.rb

@@ -578,6 +578,11 @@ def set_access_level(access_level)
         expect(described_class.new(maintainer, project)).to be_allowed(:admin_incident_management_timeline_event_tag)
         expect(described_class.new(owner, project)).to be_allowed(:admin_incident_management_timeline_event_tag)
       end
+
+      it 'allows to read import error' do
+        expect(described_class.new(maintainer, project)).to be_allowed(:read_import_error)
+        expect(described_class.new(owner, project)).to be_allowed(:read_import_error)
+      end
     end
     context 'when user is a developer/guest/reporter' do
@@ -586,6 +591,12 @@ def set_access_level(access_level)
         expect(described_class.new(guest, project)).to be_disallowed(:admin_incident_management_timeline_event_tag)
         expect(described_class.new(reporter, project)).to be_disallowed(:admin_incident_management_timeline_event_tag)
       end
+
+      it 'disallows reading the import error' do
+        expect(described_class.new(developer, project)).to be_disallowed(:read_import_error)
+        expect(described_class.new(guest, project)).to be_disallowed(:read_import_error)
+        expect(described_class.new(reporter, project)).to be_disallowed(:read_import_error)
+      end
     end
     context 'when user is not a member of the project' do