Gracefully handle 403 errors if they occur in HEAD request

If the file doesn't exist, the HEAD request will return a 403. We should
notify the user of this state rather than throw up a 500 page.

Move common code into ImportExportUpload model.

app/controllers/groups_controller.rb

@@ -181,13 +181,13 @@ class GroupsController < Groups::ApplicationController
   end
 
   def download_export
-    db_status, object_status = @group.export_file_exists_and_stored?
-
-    if db_status && object_status
-      send_upload(@group.export_file, attachment: @group.export_file.filename)
-    elsif db_status && !object_status
-      redirect_to edit_group_path(@group),
-        alert: _('The file containing the export is not available yet; it may still be transferring. Please try again later.')
+    if @group.export_file_exists?
+      if @group.export_archive_exists?
+        send_upload(@group.export_file, attachment: @group.export_file.filename)
+      else
+        redirect_to edit_group_path(@group),
+                    alert: _('The file containing the export is not available yet; it may still be transferring. Please try again later.')
+      end
     else
       redirect_to edit_group_path(@group),
         alert: _('Group export link has expired. Please generate a new export from your group settings.')

app/controllers/projects_controller.rb

@@ -225,15 +225,15 @@ class ProjectsController < Projects::ApplicationController
   end
 
   def download_export
-    db_status, object_status = @project.export_file_exists_and_stored?
-
-    if db_status && object_status
-      send_upload(@project.export_file, attachment: @project.export_file.filename)
-    elsif db_status && !object_status
-      redirect_to(
-        edit_project_path(@project, anchor: 'js-export-project'),
-        alert: _("The file containing the export is not available yet; it may still be transferring. Please try again later.")
-      )
+    if @project.export_file_exists?
+      if @project.export_archive_exists?
+        send_upload(@project.export_file, attachment: @project.export_file.filename)
+      else
+        redirect_to(
+          edit_project_path(@project, anchor: 'js-export-project'),
+          alert: _("The file containing the export is not available yet; it may still be transferring. Please try again later.")
+        )
+      end
     else
       redirect_to(
         edit_project_path(@project, anchor: 'js-export-project'),

app/models/group.rb

@@ -645,20 +645,15 @@ class Group < Namespace
   end
 
   def export_file_exists?
-    export_file&.file
+    import_export_upload&.export_file_exists?
   end
 
   def export_file
     import_export_upload&.export_file
   end
 
-  # This method returns two values: whether a record of the export file exists in the
-  # database and whether the object is actually stored. The latter requires a HEAD request
-  # if object storage is used.
-  def export_file_exists_and_stored?
-    file = export_file&.file
-
-    [file.present?, !!file&.exists?]
+  def export_archive_exists?
+    import_export_upload&.export_archive_exists?
   end
 
   def adjourned_deletion?

app/models/import_export_upload.rb

@@ -25,4 +25,28 @@ class ImportExportUpload < ApplicationRecord
   def retrieve_upload(_identifier, paths)
     Upload.find_by(model: self, path: paths)
   end
+
+  def export_file_exists?
+    !!carrierwave_export_file
+  end
+
+  # This checks if the export archive is actually stored on disk. It
+  # requires a HEAD request if object storage is used.
+  def export_archive_exists?
+    !!carrierwave_export_file&.exists?
+  # Handle any HTTP unexpected error
+  # https://github.com/excon/excon/blob/bbb5bd791d0bb2251593b80e3bce98dbec6e8f24/lib/excon/error.rb#L129-L169
+  rescue Excon::Error => e
+    # The HEAD request will fail with a 403 Forbidden if the file does not
+    # exist, and the user does not have permission to list the object
+    # storage bucket.
+    Gitlab::ErrorTracking.track_exception(e)
+    false
+  end
+
+  private
+
+  def carrierwave_export_file
+    export_file&.file
+  end
 end

app/models/project.rb

@@ -1993,20 +1993,15 @@ class Project < ApplicationRecord
   end
 
   def export_file_exists?
-    export_file&.file
+    import_export_upload&.export_file_exists?
   end
 
-  def export_file
-    import_export_upload&.export_file
+  def export_archive_exists?
+    import_export_upload&.export_archive_exists?
   end
 
-  # This method returns two values: whether a record of the export file exists in the
-  # database and whether the object is actually stored. The latter requires a HEAD request
-  # if object storage is used.
-  def export_file_exists_and_stored?
-    file = export_file&.file
-
-    [file.present?, !!file&.exists?]
+  def export_file
+    import_export_upload&.export_file
   end
 
   def full_path_slug

lib/api/group_export.rb

@@ -22,12 +22,12 @@ module API
       get ':id/export/download' do
         check_rate_limit! :group_download_export, [current_user, user_group]
 
-        db_status, object_status = user_group.export_file_exists_and_stored?
-
-        if db_status && object_status
-          present_carrierwave_file!(user_group.export_file)
-        elsif db_status && !object_status
-          render_api_error!('The group export file is not available yet', 404)
+        if user_group.export_file_exists?
+          if user_group.export_archive_exists?
+            present_carrierwave_file!(user_group.export_file)
+          else
+            render_api_error!('The group export file is not available yet', 404)
+          end
         else
           render_api_error!('404 Not found or has expired', 404)
         end

lib/api/project_export.rb

@@ -29,12 +29,12 @@ module API
       get ':id/export/download' do
         check_rate_limit! :project_download_export, [current_user, user_project]
 
-        db_status, object_status = user_project.export_file_exists_and_stored?
-
-        if db_status && object_status
-          present_carrierwave_file!(user_project.export_file)
-        elsif db_status && !object_status
-          render_api_error!('The project export file is not available yet', 404)
+        if user_project.export_file_exists?
+          if user_project.export_archive_exists?
+            present_carrierwave_file!(user_project.export_file)
+          else
+            render_api_error!('The project export file is not available yet', 404)
+          end
         else
           render_api_error!('404 Not found or has expired', 404)
         end

spec/models/group_spec.rb

@@ -2614,27 +2614,15 @@ RSpec.describe Group do
     end
   end
 
-  describe '#export_file_exists_and_stored?' do
-    it 'returns false and false' do
-      expect(group.export_file_exists_and_stored?).to eq([false, false])
-    end
-
-    context 'with export' do
-      let(:group) { create(:group, :with_export) }
+  context 'with export' do
+    let(:group) { create(:group, :with_export) }
 
-      it 'returns true and true' do
-        expect(group.export_file_exists_and_stored?).to eq([true, true])
-      end
-
-      context 'when object file does not exist' do
-        before do
-          group.export_file.file.delete
-        end
+    it '#export_file_exists returns true' do
+      expect(group.export_file_exists?).to be true
+    end
 
-        it 'returns true and false' do
-          expect(group.reload.export_file_exists_and_stored?).to eq([true, false])
-        end
-      end
+    it '#export_archive_exists? returns true' do
+      expect(group.export_archive_exists?).to be true
     end
   end
 end

spec/models/import_export_upload_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe ImportExportUpload do
-  subject { described_class.new(project: create(:project)) }
+  let(:project) { create(:project) }
+
+  subject { described_class.new(project: project) }
 
   shared_examples 'stores the Import/Export file' do |method|
     it 'stores the import file' do
@@ -62,4 +64,61 @@ RSpec.describe ImportExportUpload do
       expect(find_callback(after_commit_callbacks, :store_import_file!)).to be_nil
     end
   end
+
+  describe 'export file' do
+    it '#export_file_exists? returns false' do
+      expect(subject.export_file_exists?).to be false
+    end
+
+    it '#export_archive_exists? returns false' do
+      expect(subject.export_archive_exists?).to be false
+    end
+
+    context 'with export' do
+      let(:project_with_export) { create(:project, :with_export) }
+
+      subject { described_class.with_export_file.find_by(project: project_with_export) }
+
+      it '#export_file_exists? returns true' do
+        expect(subject.export_file_exists?).to be true
+      end
+
+      it '#export_archive_exists? returns false' do
+        expect(subject.export_archive_exists?).to be true
+      end
+
+      context 'when object file does not exist' do
+        before do
+          subject.export_file.file.delete
+        end
+
+        it '#export_file_exists? returns true' do
+          expect(subject.export_file_exists?).to be true
+        end
+
+        it '#export_archive_exists? returns false' do
+          expect(subject.export_archive_exists?).to be false
+        end
+      end
+
+      context 'when checking object existence raises a error' do
+        let(:exception) { Excon::Error::Forbidden.new('not allowed') }
+
+        before do
+          file = double
+          allow(file).to receive(:exists?).and_raise(exception)
+          allow(subject).to receive(:carrierwave_export_file).and_return(file)
+        end
+
+        it '#export_file_exists? returns true' do
+          expect(subject.export_file_exists?).to be true
+        end
+
+        it '#export_archive_exists? returns false' do
+          expect(Gitlab::ErrorTracking).to receive(:track_exception).with(exception)
+          expect(subject.export_archive_exists?).to be false
+        end
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -4346,29 +4346,15 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#export_file_exists_and_stored?' do
-    let(:project) { create(:project) }
+  context 'with export' do
+    let(:project) { create(:project, :with_export) }
 
-    it 'returns false and false' do
-      expect(project.export_file_exists_and_stored?).to eq([false, false])
+    it '#export_file_exists? returns true' do
+      expect(project.export_file_exists?).to be true
     end
 
-    context 'with export' do
-      let(:project) { create(:project, :with_export) }
-
-      it 'returns true and true' do
-        expect(project.export_file_exists_and_stored?).to eq([true, true])
-      end
-
-      context 'when object file does not exist' do
-        before do
-          project.export_file.file.delete
-        end
-
-        it 'returns true and false' do
-          expect(project.export_file_exists_and_stored?).to eq([true, false])
-        end
-      end
+    it '#export_archive_exists? returns false' do
+      expect(project.export_archive_exists?).to be true
     end
   end
 
@@ -6634,7 +6620,7 @@ RSpec.describe Project, factory_default: :keep do
     context 'when project export is completed' do
       before do
         finish_job(project_export_job)
-        allow(project).to receive(:export_file).and_return(double(ImportExportUploader, file: 'exists.zip'))
+        allow(project).to receive(:export_file_exists?).and_return(true)
       end
 
       it { expect(project.export_status).to eq :finished }
@@ -6645,7 +6631,7 @@ RSpec.describe Project, factory_default: :keep do
 
       before do
         finish_job(project_export_job)
-        allow(project).to receive(:export_file).and_return(double(ImportExportUploader, file: 'exists.zip'))
+        allow(project).to receive(:export_file_exists?).and_return(true)
       end
 
       it { expect(project.export_status).to eq :regeneration_in_progress }