Merge branch 'fj-snippet-image-blob-render' into 'master'

Render snippet blobs

Closes #212584

See merge request gitlab-org/gitlab!28085

app/controllers/concerns/sends_blob.rb

@@ -8,16 +8,16 @@ module SendsBlob
     include SendFileUpload
   end
 
-  def send_blob(repository, blob, params = {})
+  def send_blob(repository, blob, inline: true, allow_caching: false)
     if blob
       headers['X-Content-Type-Options'] = 'nosniff'
 
-      return if cached_blob?(blob)
+      return if cached_blob?(blob, allow_caching: allow_caching)
 
       if blob.stored_externally?
-        send_lfs_object(blob)
+        send_lfs_object(blob, repository.project)
       else
-        send_git_blob(repository, blob, params)
+        send_git_blob(repository, blob, inline: inline)
       end
     else
       render_404
@@ -26,11 +26,11 @@ module SendsBlob
 
   private
 
-  def cached_blob?(blob)
+  def cached_blob?(blob, allow_caching: false)
     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[:public] = allow_caching
 
     response.cache_control[:max_age] =
       if @ref && @commit && @ref == @commit.id # rubocop:disable Gitlab/ModuleWithInstanceVariables
@@ -48,7 +48,7 @@ module SendsBlob
     !stale
   end
 
-  def send_lfs_object(blob)
+  def send_lfs_object(blob, project)
     lfs_object = find_lfs_object(blob)
 
     if lfs_object && lfs_object.project_allowed_access?(project)

app/controllers/concerns/snippets_actions.rb

@@ -2,6 +2,7 @@
 
 module SnippetsActions
   extend ActiveSupport::Concern
+  include SendsBlob
 
   def edit
     # We need to load some info from the existing blob
@@ -12,16 +13,26 @@ module SnippetsActions
   end
 
   def raw
-    disposition = params[:inline] == 'false' ? 'attachment' : 'inline'
-
     workhorse_set_content_type!
 
-    send_data(
-      convert_line_endings(blob.data),
-      type: 'text/plain; charset=utf-8',
-      disposition: disposition,
-      filename: Snippet.sanitized_file_name(blob.name)
-    )
+    # Until we don't migrate all snippets to version
+    # snippets we need to support old `SnippetBlob`
+    # blobs
+    if defined?(blob.snippet)
+      send_data(
+        convert_line_endings(blob.data),
+        type: 'text/plain; charset=utf-8',
+        disposition: content_disposition,
+        filename: Snippet.sanitized_file_name(blob.name)
+      )
+    else
+      send_blob(
+        snippet.repository,
+        blob,
+        inline: content_disposition == 'inline',
+        allow_caching: snippet.public?
+      )
+    end
   end
 
   def js_request?
@@ -30,6 +41,10 @@ module SnippetsActions
 
   private
 
+  def content_disposition
+    @disposition ||= params[:inline] == 'false' ? 'attachment' : 'inline'
+  end
+
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def blob
     return unless snippet

app/controllers/projects/avatars_controller.rb

@@ -8,7 +8,7 @@ class Projects::AvatarsController < Projects::ApplicationController
   def show
     @blob = @repository.blob_at_branch(@repository.root_ref, @project.avatar_in_git)
 
-    send_blob(@repository, @blob)
+    send_blob(@repository, @blob, allow_caching: @project.public?)
   end
 
   def destroy

app/controllers/projects/raw_controller.rb

@@ -17,7 +17,7 @@ class Projects::RawController < Projects::ApplicationController
   def show
     @blob = @repository.blob_at(@commit.id, @path)
 
-    send_blob(@repository, @blob, inline: (params[:inline] != 'false'))
+    send_blob(@repository, @blob, inline: (params[:inline] != 'false'), allow_caching: @project.public?)
   end
 
   private

app/controllers/projects/wikis_controller.rb

@@ -45,7 +45,7 @@ class Projects::WikisController < Projects::ApplicationController
 
       render 'show'
     elsif file_blob
-      send_blob(@project_wiki.repository, file_blob)
+      send_blob(@project_wiki.repository, file_blob, allow_caching: @project.public?)
     elsif show_create_form?
       # Assign a title to the WikiPage unless `id` is a randomly generated slug from #new
       title = params[:id] unless params[:random_title].present?

changelogs/unreleased/fj-snippet-image-blob-render.yml

+---
+title: Render snippet repository blobs
+merge_request: 28085
+author:
+type: changed

ee/app/controllers/projects/design_management/designs/raw_images_controller.rb

@@ -12,7 +12,7 @@ module Projects
         def show
           blob = design_repository.blob_at(ref, design.full_path)
 
-          send_blob(design_repository, blob, { inline: false })
+          send_blob(design_repository, blob, inline: false, allow_caching: project.public?)
         end
 
         private

ee/spec/controllers/projects/design_management/designs/raw_images_controller_spec.rb

@@ -52,6 +52,8 @@ describe Projects::DesignManagement::Designs::RawImagesController do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
+      it_behaves_like 'project cache control headers'
+
       context 'when the user does not have permission' do
         let_it_be(:viewer) { create(:user) }
 

lib/gitlab/workhorse.rb

@@ -225,8 +225,8 @@ module Gitlab
 
       def gitaly_server_hash(repository)
         {
-          address: Gitlab::GitalyClient.address(repository.project.repository_storage),
-          token: Gitlab::GitalyClient.token(repository.project.repository_storage),
+          address: Gitlab::GitalyClient.address(repository.container.repository_storage),
+          token: Gitlab::GitalyClient.token(repository.container.repository_storage),
           features: Feature::Gitaly.server_feature_flags
         }
       end

spec/controllers/projects/avatars_controller_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Projects::AvatarsController do
-  let(:project) { create(:project, :repository) }
+  let_it_be(:project) { create(:project, :repository) }
 
   before do
     controller.instance_variable_set(:@project, project)
@@ -34,15 +34,18 @@ describe Projects::AvatarsController do
           expect(response).to have_gitlab_http_status(:ok)
           expect(response.header['Content-Disposition']).to eq('inline')
           expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
-          expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
+          expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
         end
+
+        it_behaves_like 'project cache control headers'
       end
 
       context 'when the avatar is stored in lfs' do
-        it_behaves_like 'a controller that can serve LFS files' do
-          let(:filename) { 'lfs_object.iso' }
-          let(:filepath) { "files/lfs/#{filename}" }
-        end
+        let(:filename) { 'lfs_object.iso' }
+        let(:filepath) { "files/lfs/#{filename}" }
+
+        it_behaves_like 'a controller that can serve LFS files'
+        it_behaves_like 'project cache control headers'
       end
     end
   end

spec/controllers/projects/raw_controller_spec.rb

@@ -6,6 +6,7 @@ describe Projects::RawController do
   include RepoHelpers
 
   let(:project) { create(:project, :public, :repository) }
+  let(:inline) { nil }
 
   describe 'GET #show' do
     subject do
@@ -13,7 +14,8 @@ describe Projects::RawController do
           params: {
             namespace_id: project.namespace,
             project_id: project,
-            id: filepath
+            id: filepath,
+            inline: inline
           })
     end
 
@@ -25,10 +27,12 @@ describe Projects::RawController do
 
         expect(response).to have_gitlab_http_status(:ok)
         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::DETECT_HEADER]).to eq "true"
+        expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
         expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
       end
+
+      it_behaves_like 'project cache control headers'
+      it_behaves_like 'content disposition headers'
     end
 
     context 'image header' do
@@ -38,15 +42,20 @@ describe Projects::RawController do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)
-        expect(response.header['Content-Disposition']).to eq('inline')
         expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
         expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
       end
+
+      it_behaves_like 'project cache control headers'
+      it_behaves_like 'content disposition headers'
     end
 
-    it_behaves_like 'a controller that can serve LFS files' do
+    context 'with LFS files' do
       let(:filename) { 'lfs_object.iso' }
       let(:filepath) { "be93687/files/lfs/#{filename}" }
+
+      it_behaves_like 'a controller that can serve LFS files'
+      it_behaves_like 'project cache control headers'
     end
 
     context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do

spec/controllers/projects/snippets_controller_spec.rb

@@ -449,44 +449,76 @@ describe Projects::SnippetsController do
   end
 
   describe 'GET #raw' do
-    let(:content) { "first line\r\nsecond line\r\nthird line" }
-    let(:formatted_content) { content.gsub(/\r\n/, "\n") }
-    let(:project_snippet) do
-      create(
-        :project_snippet, :public, :repository,
-        project: project,
-        author: user,
-        content: content
-      )
+    let(:inline) { nil }
+    let(:line_ending) { nil }
+    let(:params) do
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: project_snippet.to_param,
+        inline: inline,
+        line_ending: line_ending
+      }
     end
-    let(:blob) { project_snippet.blobs.first }
 
-    context 'CRLF line ending' do
-      let(:params) do
-        {
-          namespace_id: project.namespace,
-          project_id: project,
-          id: project_snippet.to_param
-        }
+    subject { get :raw, params: params }
+
+    context 'when repository is empty' do
+      let(:content) { "first line\r\nsecond line\r\nthird line" }
+      let(:formatted_content) { content.gsub(/\r\n/, "\n") }
+      let(:project_snippet) do
+        create(
+          :project_snippet, :public, :empty_repo,
+          project: project,
+          author: user,
+          content: content
+        )
       end
 
-      before do
-        allow_next_instance_of(Blob) do |instance|
-          allow(instance).to receive(:data).and_return(content)
+      context 'CRLF line ending' do
+        before do
+          allow_next_instance_of(Blob) do |instance|
+            allow(instance).to receive(:data).and_return(content)
+          end
         end
-      end
 
-      it 'returns LF line endings by default' do
-        get :raw, params: params
+        it 'returns LF line endings by default' do
+          subject
 
-        expect(response.body).to eq(formatted_content)
+          expect(response.body).to eq(formatted_content)
+        end
+
+        context 'when line_ending parameter present' do
+          let(:line_ending) { :raw }
+
+          it 'does not convert line endings' do
+            subject
+
+            expect(response.body).to eq(content)
+          end
+        end
+      end
+    end
+
+    context 'when repository is not empty' do
+      let(:project_snippet) do
+        create(
+          :project_snippet, :public, :repository,
+          project: project,
+          author: user
+        )
       end
 
-      it 'does not convert line endings when parameter present' do
-        get :raw, params: params.merge(line_ending: :raw)
+      it 'sends the blob' do
+        subject
 
-        expect(response.body).to eq(content)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+        expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
       end
+
+      it_behaves_like 'project cache control headers'
+      it_behaves_like 'content disposition headers'
     end
   end
 

spec/controllers/projects/wikis_controller_spec.rb

@@ -144,14 +144,12 @@ describe Projects::WikisController do
 
       let(:id) { upload_file_to_wiki(project, user, file_name) }
 
-      before do
-        subject
-      end
-
       context 'when file is an image' do
         let(:file_name) { 'dk.png' }
 
         it 'delivers the image' do
+          subject
+
           expect(response.headers['Content-Disposition']).to match(/^inline/)
           expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
         end
@@ -160,19 +158,27 @@ describe Projects::WikisController do
           let(:file_name) { 'unsanitized.svg' }
 
           it 'delivers the image' do
+            subject
+
             expect(response.headers['Content-Disposition']).to match(/^inline/)
             expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
           end
         end
+
+        it_behaves_like 'project cache control headers'
       end
 
       context 'when file is a pdf' do
         let(:file_name) { 'git-cheat-sheet.pdf' }
 
         it 'sets the content type to sets the content response headers' do
+          subject
+
           expect(response.headers['Content-Disposition']).to match(/^inline/)
           expect(response.headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true"
         end
+
+        it_behaves_like 'project cache control headers'
       end
     end
   end

spec/controllers/snippets_controller_spec.rb

@@ -501,6 +501,11 @@ describe SnippetsController do
   end
 
   describe "GET #raw" do
+    let(:inline) { nil }
+    let(:params) { { id: snippet.to_param, inline: inline } }
+
+    subject { get :raw, params: params }
+
     shared_examples '200 status' do
       before do
         subject
@@ -511,11 +516,6 @@ describe SnippetsController do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
-      it 'has expected headers' do
-        expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
-        expect(response.header['Content-Disposition']).to match(/inline/)
-      end
-
       it "sets #{Gitlab::Workhorse::DETECT_HEADER} header" do
         expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
       end
@@ -551,12 +551,20 @@ describe SnippetsController do
 
     shared_examples 'successful response' do
       it_behaves_like '200 status'
-      it_behaves_like 'CRLF line ending'
 
-      it 'returns snippet first blob data' do
+      it 'has expected blob headers' do
         subject
 
-        expect(response.body).to eq snippet.blobs.first.data
+        expect(response.header[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with('git-blob:')
+        expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
+      end
+
+      it_behaves_like 'content disposition headers'
+
+      it 'sets cache_control public header based on snippet visibility' do
+        subject
+
+        expect(response.cache_control[:public]).to eq snippet.public?
       end
 
       context 'when feature flag version_snippets is disabled' do
@@ -571,12 +579,33 @@ describe SnippetsController do
           subject
 
           expect(response.body).to eq snippet.content
+          expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
         end
+
+        it_behaves_like 'content disposition headers'
+      end
+
+      context 'when snippet repository is empty' do
+        before do
+          allow_any_instance_of(Repository).to receive(:empty?).and_return(true)
+        end
+
+        it_behaves_like '200 status'
+        it_behaves_like 'CRLF line ending'
+
+        it 'returns snippet database content' do
+          subject
+
+          expect(response.body).to eq snippet.content
+          expect(response.header['Content-Type']).to eq('text/plain; charset=utf-8')
+        end
+
+        it_behaves_like 'content disposition headers'
       end
     end
 
     context 'when the personal snippet is private' do
-      let_it_be(:personal_snippet) { create(:personal_snippet, :private, :repository, author: user) }
+      let_it_be(:snippet) { create(:personal_snippet, :private, :repository, author: user) }
 
       context 'when signed in' do
         before do
@@ -595,18 +624,13 @@ describe SnippetsController do
         end
 
         context 'when signed in user is the author' do
-          it_behaves_like 'successful response' do
-            let(:snippet) { personal_snippet }
-            let(:params) { { id: snippet.to_param } }
-
-            subject { get :raw, params: params }
-          end
+          it_behaves_like 'successful response'
         end
       end
 
       context 'when not signed in' do
         it 'redirects to the sign in page' do
-          get :raw, params: { id: personal_snippet.to_param }
+          subject
 
           expect(response).to redirect_to(new_user_session_path)
         end
@@ -614,24 +638,19 @@ describe SnippetsController do
     end
 
     context 'when the personal snippet is internal' do
-      let_it_be(:personal_snippet) { create(:personal_snippet, :internal, :repository, author: user) }
+      let_it_be(:snippet) { create(:personal_snippet, :internal, :repository, author: user) }
 
       context 'when signed in' do
         before do
           sign_in(user)
         end
 
-        it_behaves_like 'successful response' do
-          let(:snippet) { personal_snippet }
-          let(:params) { { id: snippet.to_param } }
-
-          subject { get :raw, params: params }
-        end
+        it_behaves_like 'successful response'
       end
 
       context 'when not signed in' do
         it 'redirects to the sign in page' do
-          get :raw, params: { id: personal_snippet.to_param }
+          subject
 
           expect(response).to redirect_to(new_user_session_path)
         end
@@ -639,26 +658,21 @@ describe SnippetsController do
     end
 
     context 'when the personal snippet is public' do
-      let_it_be(:personal_snippet) { create(:personal_snippet, :public, :repository, author: user) }
+      let_it_be(:snippet) { create(:personal_snippet, :public, :repository, author: user) }
 
       context 'when signed in' do
         before do
           sign_in(user)
         end
 
-        it_behaves_like 'successful response' do
-          let(:snippet) { personal_snippet }
-          let(:params) { { id: snippet.to_param } }
-
-          subject { get :raw, params: params }
-        end
+        it_behaves_like 'successful response'
       end
 
       context 'when not signed in' do
         it 'responds with status 200' do
-          get :raw, params: { id: personal_snippet.to_param }
+          subject
 
-          expect(assigns(:snippet)).to eq(personal_snippet)
+          expect(assigns(:snippet)).to eq(snippet)
           expect(response).to have_gitlab_http_status(:ok)
         end
       end

spec/support/shared_examples/controllers/cache_control_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'project cache control headers' do
+  before do
+    project.update(visibility_level: visibility_level)
+  end
+
+  context 'when project is public' do
+    let(:visibility_level) { Gitlab::VisibilityLevel::PUBLIC }
+
+    it 'returns cache_control public header to true' do
+      subject
+
+      expect(response.cache_control[:public]).to be_truthy
+    end
+  end
+
+  context 'when project is private' do
+    let(:visibility_level) { Gitlab::VisibilityLevel::PRIVATE }
+
+    it 'returns cache_control public header to true' do
+      subject
+
+      expect(response.cache_control[:public]).to be_falsey
+    end
+  end
+
+  context 'when project is internal' do
+    let(:visibility_level) { Gitlab::VisibilityLevel::INTERNAL }
+
+    it 'returns cache_control public header to true' do
+      subject
+
+      expect(response.cache_control[:public]).to be_falsey
+    end
+  end
+end

spec/support/shared_examples/controllers/content_disposition_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'content disposition headers' do
+  it 'sets content disposition to inline' do
+    subject
+
+    expect(response).to have_gitlab_http_status(:ok)
+    expect(response.header['Content-Disposition']).to match(/inline/)
+  end
+
+  context 'when inline param is false' do
+    let(:inline) { 'false' }
+
+    it 'sets content disposition to attachment' do
+      subject
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response.header['Content-Disposition']).to match(/attachment/)
+    end
+  end
+end