Merge branch '205628-single-file-constraint' into 'master'

Snippets access rights checking single file constraint

See merge request gitlab-org/gitlab!26551

app/models/snippet.rb

@@ -17,6 +17,8 @@ class Snippet < ApplicationRecord
   include HasRepository
   extend ::Gitlab::Utils::Override
 
+  MAX_FILE_COUNT = 1
+
   ignore_column :repository_storage, remove_with: '12.10', remove_after: '2020-03-22'
 
   cache_markdown_field :title, pipeline: :single_line

lib/gitlab/checks/push_file_count_check.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Checks
+    class PushFileCountCheck < BaseChecker
+      attr_reader :repository, :newrev, :limit, :logger
+
+      LOG_MESSAGES = {
+        diff_content_check: "Validating diff contents being single file..."
+      }.freeze
+
+      ERROR_MESSAGES = {
+        upper_limit: "The repository can contain at most %{limit} file(s).",
+        lower_limit: "The repository must contain at least 1 file."
+      }.freeze
+
+      def initialize(change, repository:, limit:, logger:)
+        @repository = repository
+        @newrev = change[:newrev]
+        @limit = limit
+        @logger = logger
+      end
+
+      def validate!
+        file_count = repository.ls_files(newrev).size
+
+        if file_count > limit
+          raise ::Gitlab::GitAccess::ForbiddenError, ERROR_MESSAGES[:upper_limit] % { limit: limit }
+        end
+
+        if file_count == 0
+          raise ::Gitlab::GitAccess::ForbiddenError, ERROR_MESSAGES[:lower_limit]
+        end
+      end
+    end
+  end
+end

lib/gitlab/checks/snippet_check.rb

@@ -20,14 +20,11 @@ module Gitlab
         @logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
       end
 
-      def exec
+      def validate!
         if creation? || deletion?
           raise GitAccess::ForbiddenError, ERROR_MESSAGES[:create_delete_branch]
         end
 
-        # TODO: https://gitlab.com/gitlab-org/gitlab/issues/205628
-        # Check operation will not result in more than one file in the repository
-
         true
       end
 

lib/gitlab/git_access_snippet.rb

@@ -97,9 +97,8 @@ module Gitlab
     end
 
     def check_single_change_access(change)
-      change_access = Checks::SnippetCheck.new(change, logger: logger)
-
-      change_access.exec
+      Checks::SnippetCheck.new(change, logger: logger).validate!
+      Checks::PushFileCountCheck.new(change, repository: repository, limit: Snippet::MAX_FILE_COUNT, logger: logger).validate!
     rescue Checks::TimedLogger::TimeoutError
       raise TimeoutError, logger.full_message
     end

spec/lib/gitlab/checks/push_file_count_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Checks::PushFileCountCheck do
+  let(:snippet) { create(:personal_snippet, :repository) }
+  let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } }
+  let(:timeout) { Gitlab::GitAccess::INTERNAL_TIMEOUT }
+  let(:logger) { Gitlab::Checks::TimedLogger.new(timeout: timeout) }
+
+  subject { described_class.new(changes, repository: snippet.repository, limit: 1, logger: logger) }
+
+  describe '#validate!' do
+    using RSpec::Parameterized::TableSyntax
+
+    before do
+      allow(snippet.repository).to receive(:new_commits).and_return(
+        snippet.repository.commits_between(oldrev, newrev)
+      )
+    end
+
+    context 'initial creation' do
+      let(:oldrev) { Gitlab::Git::EMPTY_TREE_ID }
+      let(:newrev) { TestEnv::BRANCH_SHA["snippet/single-file"] }
+      let(:ref) { "refs/heads/snippet/single-file" }
+
+      it 'allows creation' do
+        expect { subject.validate! }.not_to raise_error
+      end
+    end
+
+    where(:old, :new, :valid, :message) do
+      'single-file' | 'edit-file'            | true  | nil
+      'single-file' | 'multiple-files'       | false | 'The repository can contain at most 1 file(s).'
+      'single-file' | 'no-files'             | false | 'The repository must contain at least 1 file.'
+      'edit-file'   | 'rename-and-edit-file' | true  | nil
+    end
+
+    with_them do
+      let(:oldrev) { TestEnv::BRANCH_SHA["snippet/#{old}"] }
+      let(:newrev) { TestEnv::BRANCH_SHA["snippet/#{new}"] }
+      let(:ref) { "refs/heads/snippet/#{new}" }
+
+      it "verifies" do
+        if valid
+          expect { subject.validate! }.not_to raise_error
+        else
+          expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, message)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/checks/snippet_check_spec.rb

@@ -10,16 +10,16 @@ describe Gitlab::Checks::SnippetCheck do
 
   subject { Gitlab::Checks::SnippetCheck.new(changes, logger: logger) }
 
-  describe '#exec' do
+  describe '#validate!' do
     it 'does not raise any error' do
-      expect { subject.exec }.not_to raise_error
+      expect { subject.validate! }.not_to raise_error
     end
 
     context 'trying to delete the branch' do
       let(:newrev) { '0000000000000000000000000000000000000000' }
 
       it 'raises an error' do
-        expect { subject.exec }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You can not create or delete branches.')
+        expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You can not create or delete branches.')
       end
     end
 
@@ -28,14 +28,14 @@ describe Gitlab::Checks::SnippetCheck do
       let(:ref) { 'refs/heads/feature' }
 
       it 'raises an error' do
-        expect { subject.exec }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You can not create or delete branches.')
+        expect { subject.validate! }.to raise_error(Gitlab::GitAccess::ForbiddenError, 'You can not create or delete branches.')
       end
 
       context "when branch is 'master'" do
         let(:ref) { 'refs/heads/master' }
 
         it "allows the operation" do
-          expect { subject.exec }.not_to raise_error
+          expect { subject.validate! }.not_to raise_error
         end
       end
     end

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -188,12 +188,15 @@ describe Gitlab::GitAccessSnippet do
   end
 
   context 'when changes are specific' do
-    let(:changes) { 'oldrev newrev ref' }
+    let(:changes) { "2d1db523e11e777e49377cfb22d368deec3f0793 ddd0f15ae83993f5cb66a927a28673882e99100b master" }
     let(:user) { snippet.author }
 
     it 'does not raise error if SnippetCheck does not raise error' do
       expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
-        expect(check).to receive(:exec).and_call_original
+        expect(check).to receive(:validate!).and_call_original
+      end
+      expect_next_instance_of(Gitlab::Checks::PushFileCountCheck) do |check|
+        expect(check).to receive(:validate!)
       end
 
       expect { push_access_check }.not_to raise_error
@@ -201,7 +204,7 @@ describe Gitlab::GitAccessSnippet do
 
     it 'raises error if SnippetCheck raises error' do
       expect_next_instance_of(Gitlab::Checks::SnippetCheck) do |check|
-        allow(check).to receive(:exec).and_raise(Gitlab::GitAccess::ForbiddenError, 'foo')
+        allow(check).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError, 'foo')
       end
 
       expect { push_access_check }.to raise_forbidden('foo')

spec/requests/api/internal/base_spec.rb

@@ -12,6 +12,7 @@ describe API::Internal::Base do
 
   let_it_be(:personal_snippet) { create(:personal_snippet, :repository, author: user) }
   let_it_be(:project_snippet) { create(:project_snippet, :repository, author: user, project: project) }
+  let(:snippet_changes) { "#{TestEnv::BRANCH_SHA['snippet/single-file']} #{TestEnv::BRANCH_SHA['snippet/edit-file']} refs/heads/snippet/edit-file" }
 
   describe "GET /internal/check" do
     it do
@@ -336,7 +337,7 @@ describe API::Internal::Base do
       end
 
       context 'git push with personal snippet' do
-        subject { push(key, personal_snippet, env: env.to_json) }
+        subject { push(key, personal_snippet, env: env.to_json, changes: snippet_changes) }
 
         it 'responds with success' do
           subject
@@ -371,7 +372,7 @@ describe API::Internal::Base do
       end
 
       context 'git push with project snippet' do
-        subject { push(key, project_snippet, env: env.to_json) }
+        subject { push(key, project_snippet, env: env.to_json, changes: snippet_changes) }
 
         it 'responds with success' do
           subject
@@ -1104,9 +1105,11 @@ describe API::Internal::Base do
     )
   end
 
-  def push(key, container, protocol = 'ssh', env: nil)
+  def push(key, container, protocol = 'ssh', env: nil, changes: nil)
+    changes ||= 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master'
+
     params = {
-      changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master',
+      changes: changes,
       key_id: key.id,
       project: full_path_for(container),
       gl_repository: gl_repository_for(container),

spec/support/helpers/test_env.rb

@@ -61,6 +61,11 @@ module TestEnv
     'merge-commit-analyze-before'        => '1adbdef',
     'merge-commit-analyze-side-branch'   => '8a99451',
     'merge-commit-analyze-after'         => '646ece5',
+    'snippet/single-file'                => '43e4080',
+    'snippet/multiple-files'             => 'b80faa8',
+    'snippet/rename-and-edit-file'       => '220a1e4',
+    'snippet/edit-file'                  => 'c2f074f',
+    'snippet/no-files'                   => '671aaa8',
     '2-mb-file'                          => 'bf12d25',
     'before-create-delete-modify-move'   => '845009f',
     'between-create-delete-modify-move'  => '3f5f443',