Merge branch 'fj-forbid-push-when-snippet-read-only' into 'master'

Forbid snippet pushes when repo is read-only

See merge request gitlab-org/gitlab!51318

changelogs/unreleased/fj-forbid-push-when-snippet-read-only.yml

+---
+title: Forbid snippet pushes when repo is read-only
+merge_request: 51318
+author:
+type: fixed

lib/gitlab/git_access_snippet.rb

@@ -59,7 +59,7 @@ module Gitlab
       # TODO: Investigate if expanding actor/authentication types are needed.
       # https://gitlab.com/gitlab-org/gitlab/issues/202190
       if actor && !allowed_actor?
-        raise ForbiddenError, ERROR_MESSAGES[:authentication_mechanism]
+        raise ForbiddenError, error_message(:authentication_mechanism)
       end
 
       super
@@ -71,14 +71,18 @@ module Gitlab
 
     override :check_push_access!
     def check_push_access!
-      raise ForbiddenError, ERROR_MESSAGES[:update_snippet] unless user
+      raise ForbiddenError, error_message(:update_snippet) unless user
+
+      if snippet&.repository_read_only?
+        raise ForbiddenError, error_message(:read_only)
+      end
 
       check_change_access!
     end
 
     def check_snippet_accessibility!
       if snippet.blank?
-        raise NotFoundError, ERROR_MESSAGES[:snippet_not_found]
+        raise NotFoundError, error_message(:snippet_not_found)
       end
     end
 
@@ -94,14 +98,14 @@ module Gitlab
       passed = guest_can_download_code? || user_can_download_code?
 
       unless passed
-        raise ForbiddenError, ERROR_MESSAGES[:read_snippet]
+        raise ForbiddenError, error_message(:read_snippet)
       end
     end
 
     override :check_change_access!
     def check_change_access!
       unless user_can_push?
-        raise ForbiddenError, ERROR_MESSAGES[:update_snippet]
+        raise ForbiddenError, error_message(:update_snippet)
       end
 
       check_size_before_push!

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -29,8 +29,17 @@ RSpec.describe Gitlab::GitAccessSnippet do
     let(:actor) { build(:deploy_key) }
 
     it 'does not allow push and pull access' do
-      expect { push_access_check }.to raise_forbidden(described_class::ERROR_MESSAGES[:authentication_mechanism])
-      expect { pull_access_check }.to raise_forbidden(described_class::ERROR_MESSAGES[:authentication_mechanism])
+      expect { push_access_check }.to raise_forbidden(:authentication_mechanism)
+      expect { pull_access_check }.to raise_forbidden(:authentication_mechanism)
+    end
+  end
+
+  describe 'when snippet repository is read-only' do
+    it 'does not allow push and allows pull access' do
+      allow(snippet).to receive(:repository_read_only?).and_return(true)
+
+      expect { push_access_check }.to raise_forbidden(:read_only)
+      expect { pull_access_check }.not_to raise_error
     end
   end
 
@@ -58,7 +67,7 @@ RSpec.describe Gitlab::GitAccessSnippet do
       let(:snippet) { nil }
 
       it 'blocks access with "not found"' do
-        expect { pull_access_check }.to raise_snippet_not_found
+        expect { pull_access_check }.to raise_not_found(:snippet_not_found)
       end
     end
 
@@ -66,7 +75,7 @@ RSpec.describe Gitlab::GitAccessSnippet do
       let(:snippet) { build_stubbed(:personal_snippet) }
 
       it 'blocks access with "not found"' do
-        expect { pull_access_check }.to raise_snippet_not_found
+        expect { pull_access_check }.to raise_not_found(:no_repo)
       end
     end
   end
@@ -81,8 +90,8 @@ RSpec.describe Gitlab::GitAccessSnippet do
     it 'blocks access when the user did not accept terms' do
       message = /must accept the Terms of Service in order to perform this action/
 
-      expect { push_access_check }.to raise_forbidden(message)
-      expect { pull_access_check }.to raise_forbidden(message)
+      expect { push_access_check }.to raise_forbidden_with_message(message)
+      expect { pull_access_check }.to raise_forbidden_with_message(message)
     end
 
     it 'allows access when the user accepted the terms' do
@@ -149,8 +158,8 @@ RSpec.describe Gitlab::GitAccessSnippet do
           let(:membership) { membership }
 
           it 'respects accessibility' do
-            expect { push_access_check }.to raise_snippet_not_found
-            expect { pull_access_check }.to raise_snippet_not_found
+            expect { push_access_check }.to raise_not_found(:project_not_found)
+            expect { pull_access_check }.to raise_not_found(:project_not_found)
           end
         end
       end
@@ -273,7 +282,7 @@ RSpec.describe Gitlab::GitAccessSnippet do
           allow(check).to receive(:validate!).and_raise(Gitlab::GitAccess::ForbiddenError, 'foo')
         end
 
-        expect { push_access_check }.to raise_forbidden('foo')
+        expect { push_access_check }.to raise_forbidden_with_message('foo')
       end
 
       it 'sets the file count limit from Snippet class' do
@@ -424,15 +433,15 @@ RSpec.describe Gitlab::GitAccessSnippet do
 
   private
 
-  def raise_snippet_not_found
-    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:snippet_not_found])
+  def raise_not_found(message_key)
+    raise_error(described_class::NotFoundError, described_class.error_message(message_key))
   end
 
-  def raise_project_not_found
-    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
+  def raise_forbidden(message_key)
+    raise_error(Gitlab::GitAccess::ForbiddenError, described_class.error_message(message_key))
   end
 
-  def raise_forbidden(message)
+  def raise_forbidden_with_message(message)
     raise_error(Gitlab::GitAccess::ForbiddenError, message)
   end
 end