Merge branch 'kassio/bulkimports-import-group-avatar' into 'master'

BulkImports: Import Group Avatar

See merge request gitlab-org/gitlab!62714

app/models/concerns/avatarable.rb

@@ -9,13 +9,18 @@ module Avatarable
 
   ALLOWED_IMAGE_SCALER_WIDTHS = (USER_AVATAR_SIZES | PROJECT_AVATAR_SIZES | GROUP_AVATAR_SIZES).freeze
 
+  # This value must not be bigger than then: https://gitlab.com/gitlab-org/gitlab/-/blob/master/workhorse/config.toml.example#L20
+  #
+  # https://docs.gitlab.com/ee/development/image_scaling.html
+  MAXIMUM_FILE_SIZE = 200.kilobytes.to_i
+
   included do
     prepend ShadowMethods
     include ObjectStorage::BackgroundMove
     include Gitlab::Utils::StrongMemoize
 
     validate :avatar_type, if: ->(user) { user.avatar.present? && user.avatar_changed? }
-    validates :avatar, file_size: { maximum: 200.kilobytes.to_i }, if: :avatar_changed?
+    validates :avatar, file_size: { maximum: MAXIMUM_FILE_SIZE }, if: :avatar_changed?
 
     mount_uploader :avatar, AvatarUploader
 

app/services/bulk_imports/file_download_service.rb

 # frozen_string_literal: true
 
+# Downloads a remote file. If no filename is given, it'll use the remote filename
 module BulkImports
   class FileDownloadService
-    FILE_SIZE_LIMIT = 5.gigabytes
-    ALLOWED_CONTENT_TYPES = %w(application/gzip application/octet-stream).freeze
-
     ServiceError = Class.new(StandardError)
 
-    def initialize(configuration:, relative_url:, dir:, filename:)
+    REMOTE_FILENAME_PATTERN = %r{filename="(?<filename>[^"]+)"}.freeze
+    FILENAME_SIZE_LIMIT = 255 # chars before the extension
+
+    def initialize(configuration:, relative_url:, dir:, file_size_limit:, allowed_content_types:, filename: nil)
       @configuration = configuration
       @relative_url = relative_url
       @filename = filename
       @dir = dir
-      @filepath = File.join(@dir, @filename)
+      @file_size_limit = file_size_limit
+      @allowed_content_types = allowed_content_types
     end
 
     def execute
@@ -30,7 +32,7 @@ module BulkImports
 
     private
 
-    attr_reader :configuration, :relative_url, :dir, :filename, :filepath
+    attr_reader :configuration, :relative_url, :dir, :file_size_limit, :allowed_content_types
 
     def download_file
       File.open(filepath, 'wb') do |file|
@@ -39,7 +41,7 @@ module BulkImports
         http_client.stream(relative_url) do |chunk|
           bytes_downloaded += chunk.size
 
-          raise(ServiceError, 'Invalid downloaded file') if bytes_downloaded > FILE_SIZE_LIMIT
+          validate_size!(bytes_downloaded)
           raise(ServiceError, "File download error #{chunk.code}") unless chunk.code == 200
 
           file.write(chunk)
@@ -88,15 +90,59 @@ module BulkImports
     end
 
     def validate_content_length
-      content_size = headers['content-length']
+      validate_size!(headers['content-length'])
+    end
 
-      raise(ServiceError, 'Invalid content length') if content_size.blank? || content_size.to_i > FILE_SIZE_LIMIT
+    def validate_size!(size)
+      if size.blank?
+        raise ServiceError, 'Missing content-length header'
+      elsif size.to_i > file_size_limit
+        raise ServiceError, "File size %{size} exceeds limit of %{limit}" % {
+          size: ActiveSupport::NumberHelper.number_to_human_size(size),
+          limit: ActiveSupport::NumberHelper.number_to_human_size(file_size_limit)
+        }
+      end
     end
 
     def validate_content_type
       content_type = headers['content-type']
 
-      raise(ServiceError, 'Invalid content type') if content_type.blank? || ALLOWED_CONTENT_TYPES.exclude?(content_type)
+      raise(ServiceError, 'Invalid content type') if content_type.blank? || allowed_content_types.exclude?(content_type)
+    end
+
+    def filepath
+      @filepath ||= File.join(@dir, filename)
+    end
+
+    def filename
+      @filename.presence || remote_filename
+    end
+
+    # Fetch the remote filename information from the request content-disposition header
+    # - Raises if the filename does not exist
+    # - If the filename is longer then 255 chars truncate it
+    #   to be a total of 255 chars (with the extension)
+    def remote_filename
+      @remote_filename ||=
+        headers['content-disposition'].to_s
+        .match(REMOTE_FILENAME_PATTERN)               # matches the filename pattern
+        .then { |match| match&.named_captures || {} } # ensures the match is a hash
+        .fetch('filename')                            # fetches the 'filename' key or raise KeyError
+        .then(&File.method(:basename))                # Ensures to remove path from the filename (../ for instance)
+        .then(&method(:ensure_filename_size))         # Ensures the filename is within the FILENAME_SIZE_LIMIT
+    rescue KeyError
+      raise ServiceError, 'Remote filename not provided in content-disposition header'
+    end
+
+    def ensure_filename_size(filename)
+      if filename.length <= FILENAME_SIZE_LIMIT
+        filename
+      else
+        extname = File.extname(filename)
+        basename = File.basename(filename, extname)[0, FILENAME_SIZE_LIMIT]
+
+        "#{basename}#{extname}"
+      end
     end
   end
 end

doc/user/group/import/index.md

@@ -22,6 +22,7 @@ The following resources are migrated to the target instance:
   - description
   - attributes
   - subgroups
+  - avatar ([Introduced in 14.0](https://gitlab.com/gitlab-org/gitlab/-/issues/322904))
 - Group Labels ([Introduced in 13.9](https://gitlab.com/gitlab-org/gitlab/-/issues/292429))
   - title
   - description

ee/spec/lib/ee/bulk_imports/stage_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe BulkImports::Stage do
   let(:pipelines) do
     [
       [0, BulkImports::Groups::Pipelines::GroupPipeline],
+      [1, BulkImports::Groups::Pipelines::GroupAvatarPipeline],
       [1, BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline],
       [1, BulkImports::Groups::Pipelines::MembersPipeline],
       [1, BulkImports::Groups::Pipelines::LabelsPipeline],

lib/bulk_imports/common/extractors/ndjson_extractor.rb

@@ -7,6 +7,8 @@ module BulkImports
         include Gitlab::ImportExport::CommandLineUtil
         include Gitlab::Utils::StrongMemoize
 
+        FILE_SIZE_LIMIT = 5.gigabytes
+        ALLOWED_CONTENT_TYPES = %w(application/gzip application/octet-stream).freeze
         EXPORT_DOWNLOAD_URL_PATH = "/%{resource}/%{full_path}/export_relations/download?relation=%{relation}"
 
         def initialize(relation:)
@@ -39,7 +41,9 @@ module BulkImports
             configuration: context.configuration,
             relative_url: relative_resource_url(context),
             dir: tmp_dir,
-            filename: filename
+            filename: filename,
+            file_size_limit: FILE_SIZE_LIMIT,
+            allowed_content_types: ALLOWED_CONTENT_TYPES
           )
         end
 

lib/bulk_imports/groups/pipelines/group_avatar_pipeline.rb

+# frozen_string_literal: true
+
+module BulkImports
+  module Groups
+    module Pipelines
+      class GroupAvatarPipeline
+        include Pipeline
+
+        ALLOWED_AVATAR_DOWNLOAD_TYPES = (AvatarUploader::MIME_WHITELIST + %w(application/octet-stream)).freeze
+
+        GroupAvatarLoadingError = Class.new(StandardError)
+
+        def extract(context)
+          context.extra[:tmpdir] = Dir.mktmpdir
+
+          filepath = BulkImports::FileDownloadService.new(
+            configuration: context.configuration,
+            relative_url: "/groups/#{context.entity.encoded_source_full_path}/avatar",
+            dir: context.extra[:tmpdir],
+            file_size_limit: Avatarable::MAXIMUM_FILE_SIZE,
+            allowed_content_types: ALLOWED_AVATAR_DOWNLOAD_TYPES
+          ).execute
+
+          BulkImports::Pipeline::ExtractedData.new(data: { filepath: filepath })
+        end
+
+        def transform(_, data)
+          data
+        end
+
+        def load(context, data)
+          return if data.blank?
+
+          File.open(data[:filepath]) do |avatar|
+            service = ::Groups::UpdateService.new(
+              portable,
+              current_user,
+              avatar: avatar
+            )
+
+            unless service.execute
+              raise GroupAvatarLoadingError, portable.errors.full_messages.first
+            end
+          end
+        end
+
+        def after_run(context, _)
+          FileUtils.remove_entry(context.extra[:tmpdir]) if context.extra[:tmpdir].present?
+        end
+      end
+    end
+  end
+end

lib/bulk_imports/stage.rb

@@ -9,6 +9,10 @@ module BulkImports
         pipeline: BulkImports::Groups::Pipelines::GroupPipeline,
         stage: 0
       },
+      avatar: {
+        pipeline: BulkImports::Groups::Pipelines::GroupAvatarPipeline,
+        stage: 1
+      },
       subgroups: {
         pipeline: BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline,
         stage: 1

spec/lib/bulk_imports/groups/pipelines/group_avatar_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BulkImports::Groups::Pipelines::GroupAvatarPipeline do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:bulk_import) { create(:bulk_import, user: user) }
+
+  let_it_be(:entity) do
+    create(
+      :bulk_import_entity,
+      group: group,
+      bulk_import: bulk_import,
+      source_full_path: 'source/full/path',
+      destination_name: 'My Destination Group',
+      destination_namespace: group.full_path
+    )
+  end
+
+  let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) }
+  let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
+
+  subject { described_class.new(context) }
+
+  describe '#extract' do
+    it 'downloads the group avatar' do
+      expect_next_instance_of(
+        BulkImports::FileDownloadService,
+        configuration: context.configuration,
+        relative_url: "/groups/source%2Ffull%2Fpath/avatar",
+        dir: an_instance_of(String),
+        file_size_limit: Avatarable::MAXIMUM_FILE_SIZE,
+        allowed_content_types: described_class::ALLOWED_AVATAR_DOWNLOAD_TYPES
+      ) do |downloader|
+        expect(downloader).to receive(:execute)
+      end
+
+      subject.run
+    end
+  end
+
+  describe '#transform' do
+    it 'returns the given data' do
+      expect(subject.transform(nil, :value)).to eq(:value)
+    end
+  end
+
+  describe '#load' do
+    it 'updates the group avatar' do
+      avatar_path = 'spec/fixtures/dk.png'
+      data = { filepath: fixture_file_upload(avatar_path) }
+
+      expect { subject.load(context, data) }.to change(group, :avatar)
+
+      expect(FileUtils.identical?(avatar_path, group.avatar.file.file)).to eq(true)
+    end
+
+    it 'raises an error when the avatar upload fails' do
+      avatar_path = 'spec/fixtures/aosp_manifest.xml'
+      data = { filepath: fixture_file_upload(avatar_path) }
+
+      expect { subject.load(context, data) }.to raise_error(
+        described_class::GroupAvatarLoadingError,
+        "Avatar file format is not supported. Please try one of the following supported formats: image/png, image/jpeg, image/gif, image/bmp, image/tiff, image/vnd.microsoft.icon"
+      )
+    end
+  end
+end

spec/lib/bulk_imports/stage_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe BulkImports::Stage do
   let(:pipelines) do
     [
       [0, BulkImports::Groups::Pipelines::GroupPipeline],
+      [1, BulkImports::Groups::Pipelines::GroupAvatarPipeline],
       [1, BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline],
       [1, BulkImports::Groups::Pipelines::MembersPipeline],
       [1, BulkImports::Groups::Pipelines::LabelsPipeline],

spec/services/bulk_imports/file_download_service_spec.rb

@@ -4,26 +4,41 @@ require 'spec_helper'
 
 RSpec.describe BulkImports::FileDownloadService do
   describe '#execute' do
+    let_it_be(:allowed_content_types) { %w(application/gzip application/octet-stream) }
+    let_it_be(:file_size_limit) { 5.gigabytes }
     let_it_be(:config) { build(:bulk_import_configuration) }
     let_it_be(:content_type) { 'application/octet-stream' }
+    let_it_be(:content_disposition) { nil }
     let_it_be(:filename) { 'file_download_service_spec' }
     let_it_be(:tmpdir) { Dir.tmpdir }
     let_it_be(:filepath) { File.join(tmpdir, filename) }
+    let_it_be(:content_length) { 1000 }
+
+    let(:chunk_double) { double('chunk', size: 100, code: 200) }
 
-    let(:chunk_double) { double('chunk', size: 1000, code: 200) }
     let(:response_double) do
       double(
         code: 200,
         success?: true,
         parsed_response: {},
         headers: {
-          'content-length' => 100,
-          'content-type' => content_type
+          'content-length' => content_length,
+          'content-type' => content_type,
+          'content-disposition' => content_disposition
         }
       )
     end
 
-    subject { described_class.new(configuration: config, relative_url: '/test', dir: tmpdir, filename: filename) }
+    subject do
+      described_class.new(
+        configuration: config,
+        relative_url: '/test',
+        dir: tmpdir,
+        filename: filename,
+        file_size_limit: file_size_limit,
+        allowed_content_types: allowed_content_types
+      )
+    end
 
     before do
       allow_next_instance_of(BulkImports::Clients::HTTP) do |client|
@@ -54,7 +69,14 @@ RSpec.describe BulkImports::FileDownloadService do
         stub_application_setting(allow_local_requests_from_web_hooks_and_services: false)
 
         double = instance_double(BulkImports::Configuration, url: 'https://localhost', access_token: 'token')
-        service = described_class.new(configuration: double, relative_url: '/test', dir: tmpdir, filename: filename)
+        service = described_class.new(
+          configuration: double,
+          relative_url: '/test',
+          dir: tmpdir,
+          filename: filename,
+          file_size_limit: file_size_limit,
+          allowed_content_types: allowed_content_types
+        )
 
         expect { service.execute }.to raise_error(Gitlab::UrlBlocker::BlockedUrlError)
       end
@@ -70,31 +92,46 @@ RSpec.describe BulkImports::FileDownloadService do
 
     context 'when content-length is not valid' do
       context 'when content-length exceeds limit' do
-        before do
-          stub_const("#{described_class}::FILE_SIZE_LIMIT", 1)
-        end
+        let(:file_size_limit) { 1 }
 
         it 'raises an error' do
-          expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content length')
+          expect { subject.execute }.to raise_error(
+            described_class::ServiceError,
+            'File size 1000 Bytes exceeds limit of 1 Byte'
+          )
         end
       end
 
       context 'when content-length is missing' do
-        let(:response_double) { double(success?: true, headers: { 'content-type' => content_type }) }
+        let(:content_length) { nil }
 
         it 'raises an error' do
-          expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid content length')
+          expect { subject.execute }.to raise_error(
+            described_class::ServiceError,
+            'Missing content-length header'
+          )
         end
       end
     end
 
-    context 'when partially downloaded file exceeds limit' do
-      before do
-        stub_const("#{described_class}::FILE_SIZE_LIMIT", 150)
+    context 'when content-length is equals the file size limit' do
+      let(:content_length) { 150 }
+      let(:file_size_limit) { 150 }
+
+      it 'does not raise an error' do
+        expect { subject.execute }.not_to raise_error
       end
+    end
+
+    context 'when partially downloaded file exceeds limit' do
+      let(:content_length) { 151 }
+      let(:file_size_limit) { 150 }
 
       it 'raises an error' do
-        expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid downloaded file')
+        expect { subject.execute }.to raise_error(
+          described_class::ServiceError,
+          'File size 151 Bytes exceeds limit of 150 Bytes'
+        )
       end
     end
 
@@ -102,7 +139,10 @@ RSpec.describe BulkImports::FileDownloadService do
       let(:chunk_double) { double('chunk', size: 1000, code: 307) }
 
       it 'raises an error' do
-        expect { subject.execute }.to raise_error(described_class::ServiceError, 'File download error 307')
+        expect { subject.execute }.to raise_error(
+          described_class::ServiceError,
+          'File download error 307'
+        )
       end
     end
 
@@ -110,23 +150,88 @@ RSpec.describe BulkImports::FileDownloadService do
       let_it_be(:symlink) { File.join(tmpdir, 'symlink') }
 
       before do
-        FileUtils.ln_s(File.join(tmpdir, filename), symlink)
+        FileUtils.ln_s(File.join(tmpdir, filename), symlink, force: true)
       end
 
-      subject { described_class.new(configuration: config, relative_url: '/test', dir: tmpdir, filename: 'symlink') }
+      subject do
+        described_class.new(
+          configuration: config,
+          relative_url: '/test',
+          dir: tmpdir,
+          filename: 'symlink',
+          file_size_limit: file_size_limit,
+          allowed_content_types: allowed_content_types
+        )
+      end
 
       it 'raises an error and removes the file' do
-        expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid downloaded file')
+        expect { subject.execute }.to raise_error(
+          described_class::ServiceError,
+          'Invalid downloaded file'
+        )
 
         expect(File.exist?(symlink)).to eq(false)
       end
     end
 
     context 'when dir is not in tmpdir' do
-      subject { described_class.new(configuration: config, relative_url: '/test', dir: '/etc', filename: filename) }
+      subject do
+        described_class.new(
+          configuration: config,
+          relative_url: '/test',
+          dir: '/etc',
+          filename: filename,
+          file_size_limit: file_size_limit,
+          allowed_content_types: allowed_content_types
+        )
+      end
 
       it 'raises an error' do
-        expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid target directory')
+        expect { subject.execute }.to raise_error(
+          described_class::ServiceError,
+          'Invalid target directory'
+        )
+      end
+    end
+
+    context 'when using the remote filename' do
+      let_it_be(:filename) { nil }
+
+      context 'when no filename is given' do
+        it 'raises an error when the filename is not provided in the request header' do
+          expect { subject.execute }.to raise_error(
+            described_class::ServiceError,
+            'Remote filename not provided in content-disposition header'
+          )
+        end
+      end
+
+      context 'with a given filename' do
+        let_it_be(:content_disposition) { 'filename="avatar.png"' }
+
+        it 'uses the given filename' do
+          expect(subject.execute).to eq(File.join(tmpdir, "avatar.png"))
+        end
+      end
+
+      context 'when the filename is a path' do
+        let_it_be(:content_disposition) { 'filename="../../avatar.png"' }
+
+        it 'raises an error when the filename is not provided in the request header' do
+          expect(subject.execute).to eq(File.join(tmpdir, "avatar.png"))
+        end
+      end
+
+      context 'when the filename is longer the the limit' do
+        let_it_be(:content_disposition) { 'filename="../../xxx.b"' }
+
+        before do
+          stub_const("#{described_class}::FILENAME_SIZE_LIMIT", 1)
+        end
+
+        it 'raises an error when the filename is not provided in the request header' do
+          expect(subject.execute).to eq(File.join(tmpdir, "x.b"))
+        end
       end
     end
   end