Merge branch 'fj-forbid-snippet-request-when-flag-not-enabled' into 'master'

Move snippet feature flag check from git access class to controllers

See merge request gitlab-org/gitlab!26583

app/controllers/repositories/git_http_controller.rb

@@ -12,6 +12,8 @@ module Repositories
     rescue_from Gitlab::GitAccess::ProjectCreationError, with: :render_422_with_exception
     rescue_from Gitlab::GitAccess::TimeoutError, with: :render_503_with_exception
 
+    before_action :snippet_request_allowed?
+
     # GET /foo/bar.git/info/refs?service=git-upload-pack (git pull)
     # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
     def info_refs
@@ -116,6 +118,12 @@ module Repositories
     def log_user_activity
       Users::ActivityService.new(user).execute
     end
+
+    def snippet_request_allowed?
+      if repo_type.snippet? && Feature.disabled?(:version_snippets, user)
+        render plain: 'The project you were looking for could not be found.', status: :not_found
+      end
+    end
   end
 end
 

lib/api/internal/base.rb

@@ -108,6 +108,10 @@ module API
         #   check_ip - optional, only in EE version, may limit access to
         #     group resources based on its IP restrictions
         post "/allowed" do
+          if repo_type.snippet? && Feature.disabled?(:version_snippets, actor.user)
+            break response_with_status(code: 404, success: false, message: 'The project you were looking for could not be found.')
+          end
+
           # It was moved to a separate method so that EE can alter its behaviour more
           # easily.
           check_allowed(params)

lib/gitlab/git_access_snippet.rb

@@ -8,7 +8,6 @@ module Gitlab
       authentication_mechanism: 'The authentication mechanism is not supported.',
       read_snippet: 'You are not allowed to read this snippet.',
       update_snippet: 'You are not allowed to update this snippet.',
-      project_not_found: 'The project you were looking for could not be found.',
       snippet_not_found: 'The snippet you were looking for could not be found.',
       repository_not_found: 'The snippet repository you were looking for could not be found.'
     }.freeze
@@ -31,10 +30,6 @@ module Gitlab
         raise ForbiddenError, ERROR_MESSAGES[:authentication_mechanism]
       end
 
-      unless Feature.enabled?(:version_snippets, user)
-        raise NotFoundError, ERROR_MESSAGES[:project_not_found]
-      end
-
       check_snippet_accessibility!
 
       super

spec/controllers/repositories/git_http_controller_spec.rb

@@ -135,6 +135,38 @@ describe Repositories::GitHttpController do
     end
   end
 
+  shared_examples 'snippet feature flag disabled behavior' do
+    before do
+      stub_feature_flags(version_snippets: false)
+
+      request.headers.merge! auth_env(user.username, user.password, nil)
+    end
+
+    describe 'GET #info_refs' do
+      let(:params) { container_params.merge(service: 'git-upload-pack') }
+
+      it 'returns 404' do
+        get :info_refs, params: params
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    describe 'POST #git_upload_pack' do
+      before do
+        allow(controller).to receive(:authenticate_user).and_return(true)
+        allow(controller).to receive(:verify_workhorse_api!).and_return(true)
+        allow(controller).to receive(:access_check).and_return(nil)
+      end
+
+      it 'returns 404' do
+        post :git_upload_pack, params: params
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
   context 'when repository container is a project' do
     it_behaves_like 'info_refs behavior' do
       let(:user) { project.owner }
@@ -158,6 +190,9 @@ describe Repositories::GitHttpController do
       let(:expected_class) { Gitlab::GitAccessSnippet }
       let(:expected_object) { personal_snippet }
     end
+    it_behaves_like 'snippet feature flag disabled behavior' do
+      let(:user) { personal_snippet.author }
+    end
   end
 
   context 'when repository container is a project snippet' do
@@ -172,5 +207,8 @@ describe Repositories::GitHttpController do
       let(:expected_class) { Gitlab::GitAccessSnippet }
       let(:expected_object) { project_snippet }
     end
+    it_behaves_like 'snippet feature flag disabled behavior' do
+      let(:user) { project_snippet.author }
+    end
   end
 end

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -31,12 +31,15 @@ describe Gitlab::GitAccessSnippet do
   end
 
   describe 'when feature flag :version_snippets is disabled' do
+    let(:user) { snippet.author }
+
     before do
       stub_feature_flags(version_snippets: false)
     end
 
-    it 'does not allow push and pull access' do
-      expect { pull_access_check }.to raise_project_not_found
+    it 'allows push and pull access' do
+      expect { pull_access_check }.not_to raise_error
+      expect { push_access_check }.not_to raise_error
     end
   end
 

spec/requests/api/internal/base_spec.rb

@@ -315,6 +315,18 @@ describe API::Internal::Base do
         end
       end
 
+      shared_examples 'snippets with disabled feature flag' do
+        context 'when feature flag :version_snippets is disabled' do
+          it 'returns 404' do
+            stub_feature_flags(version_snippets: false)
+
+            subject
+
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
+
       context 'git push with personal snippet' do
         it 'responds with success' do
           push(key, personal_snippet)
@@ -325,6 +337,10 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{personal_snippet.id}")
           expect(user.reload.last_activity_on).to be_nil
         end
+
+        it_behaves_like 'snippets with disabled feature flag' do
+          subject { push(key, personal_snippet) }
+        end
       end
 
       context 'git pull with personal snippet' do
@@ -337,6 +353,10 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{personal_snippet.id}")
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
+
+        it_behaves_like 'snippets with disabled feature flag' do
+          subject { pull(key, personal_snippet) }
+        end
       end
 
       context 'git push with project snippet' do
@@ -349,6 +369,10 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{project_snippet.id}")
           expect(user.reload.last_activity_on).to be_nil
         end
+
+        it_behaves_like 'snippets with disabled feature flag' do
+          subject { push(key, project_snippet) }
+        end
       end
 
       context 'git pull with project snippet' do
@@ -361,6 +385,10 @@ describe API::Internal::Base do
           expect(json_response["gl_repository"]).to eq("snippet-#{project_snippet.id}")
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
+
+        it_behaves_like 'snippets with disabled feature flag' do
+          subject { pull(key, project_snippet) }
+        end
       end
 
       context "git pull" do