Merge branch 'fj-47229-fix-logo-lfs-tracked' into 'master'

Fixed project logo when it is LFS tracked

Closes #47229

See merge request gitlab-org/gitlab-ce!20948

app/controllers/concerns/sends_blob.rb

+# frozen_string_literal: true
+
+module SendsBlob
+  extend ActiveSupport::Concern
+
+  included do
+    include BlobHelper
+    include SendFileUpload
+  end
+
+  def send_blob(blob, params = {})
+    if blob
+      headers['X-Content-Type-Options'] = 'nosniff'
+
+      return if cached_blob?(blob)
+
+      if blob.stored_externally?
+        send_lfs_object(blob)
+      else
+        send_git_blob(repository, blob, params)
+      end
+    else
+      render_404
+    end
+  end
+
+  private
+
+  def cached_blob?(blob)
+    stale = stale?(etag: blob.id) # The #stale? method sets cache headers.
+
+    # Because we are opinionated we set the cache headers ourselves.
+    response.cache_control[:public] = project.public?
+
+    response.cache_control[:max_age] =
+      if @ref && @commit && @ref == @commit.id # rubocop:disable Gitlab/ModuleWithInstanceVariables
+        # This is a link to a commit by its commit SHA. That means that the blob
+        # is immutable. The only reason to invalidate the cache is if the commit
+        # was deleted or if the user lost access to the repository.
+        Blob::CACHE_TIME_IMMUTABLE
+      else
+        # A branch or tag points at this blob. That means that the expected blob
+        # value may change over time.
+        Blob::CACHE_TIME
+      end
+
+    response.etag = blob.id
+    !stale
+  end
+
+  def send_lfs_object(blob)
+    lfs_object = find_lfs_object(blob)
+
+    if lfs_object && lfs_object.project_allowed_access?(project)
+      send_upload(lfs_object.file, attachment: blob.name)
+    else
+      render_404
+    end
+  end
+
+  def find_lfs_object(blob)
+    lfs_object = LfsObject.find_by_oid(blob.lfs_oid)
+    if lfs_object && lfs_object.file.exists?
+      lfs_object
+    else
+      nil
+    end
+  end
+end

app/controllers/projects/avatars_controller.rb

 class Projects::AvatarsController < Projects::ApplicationController
-  include BlobHelper
+  include SendsBlob
 
   before_action :authorize_admin_project!, only: [:destroy]
 
   def show
     @blob = @repository.blob_at_branch(@repository.root_ref, @project.avatar_in_git)
-    if @blob
-      headers['X-Content-Type-Options'] = 'nosniff'
 
-      return if cached_blob?
-
-      send_git_blob @repository, @blob
-    else
-      render_404
-    end
+    send_blob(@blob)
   end
 
   def destroy
     @project.remove_avatar!
-
     @project.save
 
     redirect_to edit_project_path(@project, anchor: 'js-general-project-settings'), status: :found

app/controllers/projects/raw_controller.rb

 # Controller for viewing a file's raw
 class Projects::RawController < Projects::ApplicationController
   include ExtractsPath
-  include BlobHelper
-  include SendFileUpload
+  include SendsBlob
 
   before_action :require_non_empty_project
   before_action :assign_ref_vars
@@ -10,39 +9,7 @@ class Projects::RawController < Projects::ApplicationController
 
   def show
     @blob = @repository.blob_at(@commit.id, @path)
-    if @blob
-      headers['X-Content-Type-Options'] = 'nosniff'
 
-      return if cached_blob?
-
-      if @blob.stored_externally?
-        send_lfs_object
-      else
-        send_git_blob @repository, @blob, inline: (params[:inline] != 'false')
-      end
-    else
-      render_404
-    end
-  end
-
-  private
-
-  def send_lfs_object
-    lfs_object = find_lfs_object
-
-    if lfs_object && lfs_object.project_allowed_access?(@project)
-      send_upload(lfs_object.file, attachment: @blob.name)
-    else
-      render_404
-    end
-  end
-
-  def find_lfs_object
-    lfs_object = LfsObject.find_by_oid(@blob.lfs_oid)
-    if lfs_object && lfs_object.file.exists?
-      lfs_object
-    else
-      nil
-    end
+    send_blob(@blob, inline: (params[:inline] != 'false'))
   end
 end

app/helpers/blob_helper.rb

@@ -157,28 +157,6 @@ module BlobHelper
     end
   end
 
-  def cached_blob?
-    stale = stale?(etag: @blob.id) # The #stale? method sets cache headers.
-
-    # Because we are opionated we set the cache headers ourselves.
-    response.cache_control[:public] = @project.public?
-
-    response.cache_control[:max_age] =
-      if @ref && @commit && @ref == @commit.id
-        # This is a link to a commit by its commit SHA. That means that the blob
-        # is immutable. The only reason to invalidate the cache is if the commit
-        # was deleted or if the user lost access to the repository.
-        Blob::CACHE_TIME_IMMUTABLE
-      else
-        # A branch or tag points at this blob. That means that the expected blob
-        # value may change over time.
-        Blob::CACHE_TIME
-      end
-
-    response.etag = @blob.id
-    !stale
-  end
-
   def licenses_for_select
     return @licenses_for_select if defined?(@licenses_for_select)
 

app/serializers/diff_file_entity.rb

@@ -2,7 +2,6 @@
 
 class DiffFileEntity < Grape::Entity
   include RequestAwareEntity
-  include BlobHelper
   include CommitsHelper
   include DiffHelper
   include SubmoduleHelper

changelogs/unreleased/fj-47229-fix-logo-lfs-tracked.yml

+---
+title: Fixed bug when the project logo file is stored in LFS
+merge_request: 20948
+author:
+type: fixed

spec/controllers/projects/avatars_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::AvatarsController do
-  let(:project) { create(:project, :repository, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) }
-  let(:user)    { create(:user) }
+  let(:project) { create(:project, :repository) }
 
   before do
-    sign_in(user)
-    project.add_maintainer(user)
     controller.instance_variable_set(:@project, project)
   end
 
-  it 'GET #show' do
-    get :show, namespace_id: project.namespace.id, project_id: project.id
+  describe 'GET #show' do
+    subject { get :show, namespace_id: project.namespace, project_id: project }
+
+    context 'when repository has no avatar' do
+      it 'shows 404' do
+        subject
 
         expect(response).to have_gitlab_http_status(404)
       end
+    end
+
+    context 'when repository has an avatar' do
+      before do
+        allow(project).to receive(:avatar_in_git).and_return(filepath)
+      end
 
+      context 'when the avatar is stored in the repository' do
+        let(:filepath) { 'files/images/logo-white.png' }
+
+        it 'sends the avatar' do
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(response.header['Content-Type']).to eq('image/png')
+          expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+        end
+      end
+
+      context 'when the avatar is stored in lfs' do
+        it_behaves_like 'repository lfs file load' do
+          let(:filename) { 'lfs_object.iso' }
+          let(:filepath) { "files/lfs/#{filename}" }
+        end
+      end
+    end
+  end
+
+  describe 'DELETE #destroy' do
     it 'removes avatar from DB by calling destroy' do
       delete :destroy, namespace_id: project.namespace.id, project_id: project.id
+
       expect(project.avatar.present?).to be_falsey
       expect(project).to be_valid
     end
+  end
 end

spec/controllers/projects/raw_controller_spec.rb

 require 'spec_helper'
 
 describe Projects::RawController do
-  let(:public_project) { create(:project, :public, :repository) }
+  let(:project) { create(:project, :public, :repository) }
+
+  describe 'GET #show' do
+    subject do
+      get(:show,
+          namespace_id: project.namespace,
+          project_id: project,
+          id: filepath)
+    end
 
-  describe '#show' do
     context 'regular filename' do
-      let(:id) { 'master/README.md' }
+      let(:filepath) { 'master/README.md' }
 
       it 'delivers ASCII file' do
-        get_show(public_project, id)
+        subject
 
         expect(response).to have_gitlab_http_status(200)
         expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
@@ -19,10 +26,10 @@ describe Projects::RawController do
     end
 
     context 'image header' do
-      let(:id) { 'master/files/images/6049019_460s.jpg' }
+      let(:filepath) { 'master/files/images/6049019_460s.jpg' }
 
       it 'sets image content type header' do
-        get_show(public_project, id)
+        subject
 
         expect(response).to have_gitlab_http_status(200)
         expect(response.header['Content-Type']).to eq('image/jpeg')
@@ -30,85 +37,9 @@ describe Projects::RawController do
       end
     end
 
-    context 'lfs object' do
-      let(:id) { 'be93687/files/lfs/lfs_object.iso' }
-      let!(:lfs_object) { create(:lfs_object, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') }
-
-      context 'when lfs is enabled' do
-        before do
-          allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
-        end
-
-        context 'when project has access' do
-          before do
-            public_project.lfs_objects << lfs_object
-            allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true)
-            allow(controller).to receive(:send_file) { controller.head :ok }
-          end
-
-          it 'serves the file' do
-            expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: 'lfs_object.iso', disposition: 'attachment')
-            get_show(public_project, id)
-
-            expect(response).to have_gitlab_http_status(200)
-          end
-
-          context 'and lfs uses object storage' do
-            let(:lfs_object) { create(:lfs_object, :with_file, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') }
-
-            before do
-              stub_lfs_object_storage
-              lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
-            end
-
-            it 'responds with redirect to file' do
-              get_show(public_project, id)
-
-              expect(response).to have_gitlab_http_status(302)
-              expect(response.location).to include(lfs_object.reload.file.path)
-            end
-
-            it 'sets content disposition' do
-              get_show(public_project, id)
-
-              file_uri = URI.parse(response.location)
-              params = CGI.parse(file_uri.query)
-
-              expect(params["response-content-disposition"].first).to eq 'attachment;filename="lfs_object.iso"'
-            end
-          end
+    it_behaves_like 'repository lfs file load' do
+      let(:filename) { 'lfs_object.iso' }
+      let(:filepath) { "be93687/files/lfs/#{filename}" }
     end
-
-        context 'when project does not have access' do
-          it 'does not serve the file' do
-            get_show(public_project, id)
-
-            expect(response).to have_gitlab_http_status(404)
-          end
-        end
-      end
-
-      context 'when lfs is not enabled' do
-        before do
-          allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false)
-        end
-
-        it 'delivers ASCII file' do
-          get_show(public_project, id)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
-          expect(response.header['Content-Disposition'])
-              .to eq('inline')
-          expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
-        end
-      end
-    end
-  end
-
-  def get_show(project, id)
-    get(:show, namespace_id: project.namespace.to_param,
-               project_id: project,
-               id: id)
   end
 end

spec/support/shared_examples/controllers/repository_lfs_file_load_examples.rb

+# frozen_string_literal: true
+
+# Shared examples for controllers that load and send files from the git repository
+# (like Projects::RawController or Projects::AvatarsController)
+
+# These examples requires the following variables:
+# - `project`
+# - `filename`: filename of the file
+# - `filepath`: path of the file (contains filename)
+# - `subject`: the request to be made to the controller. Example:
+# subject { get :show, namespace_id: project.namespace, project_id: project }
+shared_examples 'repository lfs file load' do
+  context 'when file is stored in lfs' do
+    let(:lfs_oid) { '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897' }
+    let(:lfs_size) { '1575078' }
+    let!(:lfs_object) { create(:lfs_object, oid: lfs_oid, size: lfs_size) }
+
+    context 'when lfs is enabled' do
+      before do
+        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(true)
+      end
+
+      context 'when project has access' do
+        before do
+          project.lfs_objects << lfs_object
+          allow_any_instance_of(LfsObjectUploader).to receive(:exists?).and_return(true)
+          allow(controller).to receive(:send_file) { controller.head :ok }
+        end
+
+        it 'serves the file' do
+          expect(controller).to receive(:send_file).with("#{LfsObjectUploader.root}/91/ef/f75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897", filename: filename, disposition: 'attachment')
+
+          subject
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+
+        context 'and lfs uses object storage' do
+          let(:lfs_object) { create(:lfs_object, :with_file, oid: lfs_oid, size: lfs_size) }
+
+          before do
+            stub_lfs_object_storage
+            lfs_object.file.migrate!(LfsObjectUploader::Store::REMOTE)
+          end
+
+          it 'responds with redirect to file' do
+            subject
+
+            expect(response).to have_gitlab_http_status(302)
+            expect(response.location).to include(lfs_object.reload.file.path)
+          end
+
+          it 'sets content disposition' do
+            subject
+
+            file_uri = URI.parse(response.location)
+            params = CGI.parse(file_uri.query)
+
+            expect(params["response-content-disposition"].first).to eq "attachment;filename=\"#{filename}\""
+          end
+        end
+      end
+
+      context 'when project does not have access' do
+        it 'does not serve the file' do
+          subject
+
+          expect(response).to have_gitlab_http_status(404)
+        end
+      end
+    end
+
+    context 'when lfs is not enabled' do
+      before do
+        allow_any_instance_of(Project).to receive(:lfs_enabled?).and_return(false)
+      end
+
+      it 'delivers ASCII file' do
+        subject
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
+        expect(response.header['Content-Disposition'])
+            .to eq('inline')
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+      end
+    end
+  end
+end