Merge branch '212662-edit-snippet-images' into 'master'

Resolve "Snippets actions with binary data"

Closes #212662

See merge request gitlab-org/gitlab!28191

app/controllers/concerns/snippets_actions.rb

@@ -4,6 +4,10 @@ module SnippetsActions
   extend ActiveSupport::Concern
   include SendsBlob
 
+  included do
+    before_action :redirect_if_binary, only: [:edit, :update]
+  end
+
   def edit
     # We need to load some info from the existing blob
     snippet.content = blob.data
@@ -67,4 +71,8 @@ module SnippetsActions
     flash.now[:alert] = repository_errors.first if repository_errors.present?
     recaptcha_check_with_fallback(repository_errors.empty?) { render :edit }
   end
+
+  def redirect_if_binary
+    redirect_to gitlab_snippet_path(snippet) if blob&.binary?
+  end
 end

changelogs/unreleased/212662-edit-snippet-images.yml

+---
+title: Resolve Snippet actions with binary data
+merge_request: 28191
+author:
+type: fixed

spec/controllers/projects/snippets_controller_spec.rb

@@ -116,7 +116,7 @@ describe Projects::SnippetsController do
       end
 
       context 'when the snippet is public' do
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }
             .not_to change { Snippet.count }
           expect(response).to render_template(:new)
@@ -164,6 +164,7 @@ describe Projects::SnippetsController do
 
   describe 'PUT #update' do
     let(:project) { create :project, :public }
+    let(:visibility_level) { Snippet::PUBLIC }
     let(:snippet) { create :project_snippet, author: user, project: project, visibility_level: visibility_level }
 
     def update_snippet(snippet_params = {}, additional_params = {})
@@ -174,13 +175,27 @@ describe Projects::SnippetsController do
       put :update, params: {
         namespace_id: project.namespace.to_param,
         project_id: project,
-        id: snippet.id,
+        id: snippet,
         project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params)
       }.merge(additional_params)
 
       snippet.reload
     end
 
+    it_behaves_like 'updating snippet checks blob is binary' do
+      let_it_be(:title) { 'Foo' }
+      let(:params) do
+        {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: snippet.id,
+          project_snippet: { title: title }
+        }
+      end
+
+      subject { put :update, params: params }
+    end
+
     context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
@@ -198,9 +213,7 @@ describe Projects::SnippetsController do
       end
 
       context 'when the snippet is public' do
-        let(:visibility_level) { Snippet::PUBLIC }
-
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo') }
             .not_to change { snippet.reload.title }
         end
@@ -245,7 +258,7 @@ describe Projects::SnippetsController do
       context 'when the private snippet is made public' do
         let(:visibility_level) { Snippet::PRIVATE }
 
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }
             .not_to change { snippet.reload.title }
         end
@@ -581,4 +594,19 @@ describe Projects::SnippetsController do
       end
     end
   end
+
+  describe 'GET #edit' do
+    it_behaves_like 'editing snippet checks blob is binary' do
+      let(:snippet) { create(:project_snippet, :private, project: project, author: user) }
+      let(:params) do
+        {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: snippet
+        }
+      end
+
+      subject { get :edit, params: params }
+    end
+  end
 end

spec/controllers/snippets_controller_spec.rb

@@ -308,7 +308,7 @@ describe SnippetsController do
       end
 
       context 'when the snippet is public' do
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { create_snippet(visibility_level: Snippet::PUBLIC) }
             .not_to change { Snippet.count }
         end
@@ -354,6 +354,7 @@ describe SnippetsController do
 
   describe 'PUT #update' do
     let(:project) { create :project }
+    let(:visibility_level) { Snippet::PUBLIC }
     let(:snippet) { create :personal_snippet, author: user, project: project, visibility_level: visibility_level }
 
     def update_snippet(snippet_params = {}, additional_params = {})
@@ -367,6 +368,12 @@ describe SnippetsController do
       snippet.reload
     end
 
+    it_behaves_like 'updating snippet checks blob is binary' do
+      let_it_be(:title) { 'Foo' }
+
+      subject { put :update, params: { id: snippet, personal_snippet: { title: title } } }
+    end
+
     context 'when the snippet is spam' do
       before do
         allow_next_instance_of(Spam::AkismetService) do |instance|
@@ -429,9 +436,7 @@ describe SnippetsController do
       end
 
       context 'when the snippet is public' do
-        let(:visibility_level) { Snippet::PUBLIC }
-
-        it 'rejects the shippet' do
+        it 'rejects the snippet' do
           expect { update_snippet(title: 'Foo') }
             .not_to change { snippet.reload.title }
         end
@@ -793,4 +798,12 @@ describe SnippetsController do
       end
     end
   end
+
+  describe 'GET #edit' do
+    it_behaves_like 'editing snippet checks blob is binary' do
+      let_it_be(:snippet) { create(:personal_snippet, :public, :repository, author: user) }
+
+      subject { get :edit, params: { id: snippet } }
+    end
+  end
 end

spec/support/shared_examples/controllers/binary_blob_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'editing snippet checks blob is binary' do
+  before do
+    sign_in(user)
+
+    allow_next_instance_of(Blob) do |blob|
+      allow(blob).to receive(:binary?).and_return(binary)
+    end
+
+    subject
+  end
+
+  context 'when blob is text' do
+    let(:binary) { false }
+
+    it 'responds with status 200' do
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to render_template(:edit)
+    end
+  end
+
+  context 'when blob is binary' do
+    let(:binary) { true }
+
+    it 'redirects away' do
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+    end
+  end
+end
+
+RSpec.shared_examples 'updating snippet checks blob is binary' do
+  before do
+    sign_in(user)
+
+    allow_next_instance_of(Blob) do |blob|
+      allow(blob).to receive(:binary?).and_return(binary)
+    end
+
+    subject
+  end
+
+  context 'when blob is text' do
+    let(:binary) { false }
+
+    it 'updates successfully' do
+      expect(snippet.reload.title).to eq title
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+    end
+  end
+
+  context 'when blob is binary' do
+    let(:binary) { true }
+
+    it 'redirects away without updating' do
+      expect(response).to redirect_to(gitlab_snippet_path(snippet))
+      expect(snippet.reload.title).not_to eq title
+    end
+  end
+end