Merge branch '39514-access-check' into 'master'

Version Control for Snippets: Access Rights Check for project snippet

See merge request gitlab-org/gitlab!23492

lib/gitlab/checks/snippet_check.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Checks
+    class SnippetCheck < BaseChecker
+      ERROR_MESSAGES = {
+        create_delete_branch: 'You can not create or delete branches.'
+      }.freeze
+
+      ATTRIBUTES = %i[oldrev newrev ref branch_name tag_name logger].freeze
+      attr_reader(*ATTRIBUTES)
+
+      def initialize(change, logger:)
+        @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
+        @branch_name = Gitlab::Git.branch_name(@ref)
+        @tag_name = Gitlab::Git.tag_name(@ref)
+
+        @logger = logger
+        @logger.append_message("Running checks for ref: #{@branch_name || @tag_name}")
+      end
+
+      def exec
+        if creation? || deletion?
+          raise GitAccess::UnauthorizedError, 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
+    end
+  end
+end

lib/gitlab/git_access.rb

@@ -60,7 +60,6 @@ module Gitlab
       @logger = Checks::TimedLogger.new(timeout: INTERNAL_TIMEOUT, header: LOG_HEADER)
       @changes = changes
 
-      check_namespace!
       check_protocol!
       check_valid_actor!
       check_active_user!
@@ -72,11 +71,7 @@ module Gitlab
       return custom_action if custom_action
 
       check_db_accessibility!(cmd)
-
-      ensure_project_on_push!(cmd, changes)
-
-      check_project_accessibility!
-      add_project_moved_message!
+      check_project!(changes, cmd)
       check_repository_existence!
 
       case cmd
@@ -113,6 +108,13 @@ module Gitlab
 
     private
 
+    def check_project!(changes, cmd)
+      check_namespace!
+      ensure_project_on_push!(cmd, changes)
+      check_project_accessibility!
+      add_project_moved_message!
+    end
+
     def check_custom_action(cmd)
       nil
     end

lib/gitlab/git_access_snippet.rb

@@ -2,7 +2,13 @@
 
 module Gitlab
   class GitAccessSnippet < GitAccess
+    extend ::Gitlab::Utils::Override
+
     ERROR_MESSAGES = {
+      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
@@ -12,25 +18,47 @@ module Gitlab
     def initialize(actor, snippet, protocol, **kwargs)
       @snippet = snippet
 
-      super(actor, project, protocol, **kwargs)
+      super(actor, snippet&.project, protocol, **kwargs)
+
+      @auth_result_type = nil
+      @authentication_abilities &= [:download_code, :push_code]
     end
 
-    def check(cmd, _changes)
+    def check(cmd, changes)
+      # TODO: Investigate if expanding actor/authentication types are needed.
+      # https://gitlab.com/gitlab-org/gitlab/issues/202190
+      if actor && !actor.is_a?(User) && !actor.instance_of?(Key)
+        raise UnauthorizedError, ERROR_MESSAGES[:authentication_mechanism]
+      end
+
       unless Feature.enabled?(:version_snippets, user)
-        raise NotFoundError, ERROR_MESSAGES[:snippet_not_found]
+        raise NotFoundError, ERROR_MESSAGES[:project_not_found]
       end
 
       check_snippet_accessibility!
 
-      success_result(cmd)
+      super
     end
 
-    def project
-      snippet&.project
+    private
+
+    override :check_project!
+    def check_project!(cmd, changes)
+      if snippet.is_a?(ProjectSnippet)
+        check_namespace!
+        check_project_accessibility!
+        # TODO add add_project_moved_message! to handle non-project repo https://gitlab.com/gitlab-org/gitlab/issues/205646
+      end
     end
 
-    private
+    override :check_push_access!
+    def check_push_access!
+      raise UnauthorizedError, ERROR_MESSAGES[:update_snippet] unless user
+
+      check_change_access!
+    end
 
+    override :repository
     def repository
       snippet&.repository
     end
@@ -39,10 +67,64 @@ module Gitlab
       if snippet.blank?
         raise NotFoundError, ERROR_MESSAGES[:snippet_not_found]
       end
+    end
+
+    override :check_download_access!
+    def check_download_access!
+      passed = guest_can_download_code? || user_can_download_code?
+
+      unless passed
+        raise UnauthorizedError, ERROR_MESSAGES[:read_snippet]
+      end
+    end
+
+    override :guest_can_download_code?
+    def guest_can_download_code?
+      Guest.can?(:read_snippet, snippet)
+    end
+
+    override :user_can_download_code?
+    def user_can_download_code?
+      authentication_abilities.include?(:download_code) && user_access.can_do_action?(:read_snippet)
+    end
+
+    override :check_change_access!
+    def check_change_access!
+      unless user_access.can_do_action?(:update_snippet)
+        raise UnauthorizedError, ERROR_MESSAGES[:update_snippet]
+      end
+
+      changes_list.each do |change|
+        # If user does not have access to make at least one change, cancel all
+        # push by allowing the exception to bubble up
+        check_single_change_access(change)
+      end
+    end
+
+    def check_single_change_access(change)
+      change_access = Checks::SnippetCheck.new(change, logger: logger)
+
+      change_access.exec
+    rescue Checks::TimedLogger::TimeoutError
+      raise TimeoutError, logger.full_message
+    end
 
-      unless repository&.exists?
+    override :check_repository_existence!
+    def check_repository_existence!
+      unless repository.exists?
         raise NotFoundError, ERROR_MESSAGES[:repository_not_found]
       end
     end
+
+    override :user_access
+    def user_access
+      @user_access ||= UserAccessSnippet.new(user, snippet: snippet)
+    end
+
+    # TODO: Implement EE/Geo https://gitlab.com/gitlab-org/gitlab/issues/205629
+    override :check_custom_action
+    def check_custom_action(cmd)
+      nil
+    end
   end
 end

lib/gitlab/user_access.rb

@@ -98,7 +98,7 @@ module Gitlab
       @permission_cache ||= {}
     end
 
-    def can_access_git?
+    request_cache def can_access_git?
       user && user.can?(:access_git)
     end
 

lib/gitlab/user_access_snippet.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class UserAccessSnippet < UserAccess
+    extend ::Gitlab::Cache::RequestCache
+    # TODO: apply override check https://gitlab.com/gitlab-org/gitlab/issues/205677
+
+    request_cache_key do
+      [user&.id, snippet&.id]
+    end
+
+    attr_reader :snippet
+
+    def initialize(user, snippet: nil)
+      @user = user
+      @snippet = snippet
+      @project = snippet&.project
+    end
+
+    def can_do_action?(action)
+      return false unless can_access_git?
+
+      permission_cache[action] =
+        permission_cache.fetch(action) do
+          Ability.allowed?(user, action, snippet)
+        end
+    end
+
+    def can_create_tag?(ref)
+      false
+    end
+
+    def can_delete_branch?(ref)
+      false
+    end
+
+    def can_push_to_branch?(ref)
+      super
+      return false unless snippet
+      return false unless can_do_action?(:update_snippet)
+
+      true
+    end
+
+    def can_merge_to_branch?(ref)
+      false
+    end
+  end
+end

spec/lib/gitlab/checks/snippet_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Checks::SnippetCheck do
+  include_context 'change access checks context'
+
+  let(:snippet) { create(:personal_snippet, :repository) }
+  let(:user_access) { Gitlab::UserAccessSnippet.new(user, snippet: snippet) }
+
+  subject { Gitlab::Checks::SnippetCheck.new(changes, logger: logger) }
+
+  describe '#exec' do
+    it 'does not raise any error' do
+      expect { subject.exec }.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::UnauthorizedError, 'You can not create or delete branches.')
+      end
+    end
+
+    context 'trying to create the branch' do
+      let(:oldrev) { '0000000000000000000000000000000000000000' }
+
+      it 'raises an error' do
+        expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can not create or delete branches.')
+      end
+    end
+  end
+end

spec/lib/gitlab/git_access_snippet_spec.rb

@@ -3,24 +3,30 @@
 require 'spec_helper'
 
 describe Gitlab::GitAccessSnippet do
-  include GitHelpers
+  include ProjectHelpers
+  include TermsHelper
+  include_context 'ProjectPolicyTable context'
+  using RSpec::Parameterized::TableSyntax
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:personal_snippet) { create(:personal_snippet, :private, :repository) }
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:snippet) { create(:project_snippet, :public, :repository, project: project) }
 
+  let(:actor) { user }
   let(:protocol) { 'ssh' }
   let(:changes) { Gitlab::GitAccess::ANY }
+  let(:authentication_abilities) { [:download_code, :push_code] }
+
   let(:push_access_check) { access.check('git-receive-pack', changes) }
   let(:pull_access_check) { access.check('git-upload-pack', changes) }
-  let(:snippet) { personal_snippet }
-  let(:actor) { personal_snippet.author }
 
-  describe 'when feature flag :version_snippets is enabled' do
-    it 'allows push and pull access' do
-      aggregate_failures do
-        expect { pull_access_check }.not_to raise_error
-        expect { push_access_check }.not_to raise_error
-      end
+  subject(:access) { Gitlab::GitAccessSnippet.new(actor, snippet, protocol, authentication_abilities: authentication_abilities) }
+
+  describe 'when actor is a DeployKey' do
+    let(:actor) { build(:deploy_key) }
+
+    it 'does not allow push and pull access' do
+      expect { pull_access_check }.to raise_unauthorized(described_class::ERROR_MESSAGES[:authentication_mechanism])
     end
   end
 
@@ -30,56 +36,186 @@ describe Gitlab::GitAccessSnippet do
     end
 
     it 'does not allow push and pull access' do
-      aggregate_failures do
-        expect { push_access_check }.to raise_snippet_not_found
-        expect { pull_access_check }.to raise_snippet_not_found
-      end
+      expect { pull_access_check }.to raise_project_not_found
     end
   end
 
   describe '#check_snippet_accessibility!' do
     context 'when the snippet exists' do
-      it 'allows push and pull access' do
-        aggregate_failures do
-          expect { pull_access_check }.not_to raise_error
-          expect { push_access_check }.not_to raise_error
-        end
+      it 'allows access' do
+        project.add_developer(actor)
+
+        expect { pull_access_check }.not_to raise_error
       end
     end
 
     context 'when the snippet is nil' do
       let(:snippet) { nil }
 
-      it 'blocks push and pull with "not found"' do
-        aggregate_failures do
-          expect { pull_access_check }.to raise_snippet_not_found
-          expect { push_access_check }.to raise_snippet_not_found
-        end
+      it 'blocks access with "not found"' do
+        expect { pull_access_check }.to raise_snippet_not_found
       end
     end
 
     context 'when the snippet does not have a repository' do
       let(:snippet) { build_stubbed(:personal_snippet) }
 
-      it 'blocks push and pull with "not found"' do
-        aggregate_failures do
-          expect { pull_access_check }.to raise_snippet_not_found
-          expect { push_access_check }.to raise_snippet_not_found
+      it 'blocks access with "not found"' do
+        expect { pull_access_check }.to raise_snippet_not_found
+      end
+    end
+  end
+
+  context 'terms are enforced', :aggregate_failures do
+    before do
+      enforce_terms
+    end
+
+    let(:user) { snippet.author }
+
+    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_unauthorized(message)
+      expect { pull_access_check }.to raise_unauthorized(message)
+    end
+
+    it 'allows access when the user accepted the terms' do
+      accept_terms(user)
+
+      expect { push_access_check }.not_to raise_error
+      expect { pull_access_check }.not_to raise_error
+    end
+  end
+
+  context 'project snippet accessibility', :aggregate_failures do
+    let(:snippet) { create(:project_snippet, :private, :repository, project: project) }
+    let(:user) { membership == :author ? snippet.author : create_user_from_membership(project, membership) }
+
+    shared_examples_for 'checks accessibility' do
+      [:anonymous, :non_member, :guest, :reporter, :maintainer, :admin, :author].each do |membership|
+        context membership.to_s do
+          let(:membership) { membership }
+
+          it 'respects accessibility' do
+            if Ability.allowed?(user, :update_snippet, snippet)
+              expect { push_access_check }.not_to raise_error
+            else
+              expect { push_access_check }.to raise_error(described_class::UnauthorizedError)
+            end
+
+            if Ability.allowed?(user, :read_snippet, snippet)
+              expect { pull_access_check }.not_to raise_error
+            else
+              expect { pull_access_check }.to raise_error(described_class::UnauthorizedError)
+            end
+          end
+        end
+      end
+    end
+
+    context 'when project is public' do
+      it_behaves_like 'checks accessibility'
+    end
+
+    context 'when project is public but snippet feature is private' do
+      let(:project) { create(:project, :public) }
+
+      before do
+        update_feature_access_level(project, :private)
+      end
+
+      it_behaves_like 'checks accessibility'
+    end
+
+    context 'when project is not accessible' do
+      let(:project) { create(:project, :private) }
+
+      [:anonymous, :non_member].each do |membership|
+        context membership.to_s do
+          let(:membership) { membership }
+
+          it 'respects accessibility' do
+            expect { push_access_check }.to raise_error(described_class::NotFoundError)
+            expect { pull_access_check }.to raise_error(described_class::NotFoundError)
+          end
         end
       end
     end
   end
 
-  private
+  context 'personal snippet accessibility', :aggregate_failures do
+    let(:snippet) { create(:personal_snippet, snippet_level, :repository) }
+    let(:user) { membership == :author ? snippet.author : create_user_from_membership(nil, membership) }
+
+    where(:snippet_level, :membership, :_expected_count) do
+      permission_table_for_personal_snippet_access
+    end
 
-  def access
-    described_class.new(actor, snippet, protocol,
-                        authentication_abilities: [],
-                        namespace_path: nil, project_path: nil,
-                        redirected_path: nil, auth_result_type: nil)
+    with_them do
+      it "respects accessibility" do
+        error_class = described_class::UnauthorizedError
+
+        if Ability.allowed?(user, :update_snippet, snippet)
+          expect { push_access_check }.not_to raise_error
+        else
+          expect { push_access_check }.to raise_error(error_class)
+        end
+
+        if Ability.allowed?(user, :read_snippet, snippet)
+          expect { pull_access_check }.not_to raise_error
+        else
+          expect { pull_access_check }.to raise_error(error_class)
+        end
+      end
+    end
   end
 
+  context 'when geo is enabled', if: Gitlab.ee? do
+    let(:user) { snippet.author }
+    let!(:primary_node) { FactoryBot.create(:geo_node, :primary) }
+
+    # Without override, push access would return Gitlab::GitAccessResult::CustomAction
+    it 'skips geo for snippet' do
+      allow(::Gitlab::Database).to receive(:read_only?).and_return(true)
+      allow(::Gitlab::Geo).to receive(:secondary_with_primary?).and_return(true)
+
+      expect { push_access_check }.to raise_unauthorized(/You can't push code to a read-only GitLab instance/)
+    end
+  end
+
+  context 'when changes are specific' do
+    let(:changes) { 'oldrev newrev ref' }
+    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
+      end
+
+      expect { push_access_check }.not_to raise_error
+    end
+
+    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::UnauthorizedError, 'foo')
+      end
+
+      expect { push_access_check }.to raise_unauthorized('foo')
+    end
+  end
+
+  private
+
   def raise_snippet_not_found
     raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:snippet_not_found])
   end
+
+  def raise_project_not_found
+    raise_error(Gitlab::GitAccess::NotFoundError, Gitlab::GitAccess::ERROR_MESSAGES[:project_not_found])
+  end
+
+  def raise_unauthorized(message)
+    raise_error(Gitlab::GitAccess::UnauthorizedError, message)
+  end
 end

spec/lib/gitlab/user_access_snippet_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::UserAccessSnippet do
+  subject(:access) { described_class.new(user, snippet: snippet) }
+
+  let_it_be(:project) { create(:project, :private) }
+  let_it_be(:snippet) { create(:project_snippet, :private, project: project) }
+  let(:user) { create(:user) }
+
+  describe '#can_do_action?' do
+    before do
+      allow(Ability).to receive(:allowed?).and_call_original
+      allow(Ability).to receive(:allowed?).with(user, :ability, snippet).and_return(:foo)
+    end
+
+    context 'when can access_git' do
+      it 'calls Ability#allowed? and returns its result' do
+        expect(access.can_do_action?(:ability)).to eq(:foo)
+      end
+    end
+
+    context 'when can not access_git' do
+      it 'disallows access' do
+        expect(Ability).to receive(:allowed?).with(user, :access_git, :global).and_return(false)
+
+        expect(access.can_do_action?(:ability)).to eq(false)
+      end
+    end
+
+    context 'when user is nil' do
+      let(:user) { nil }
+
+      it 'disallows access' do
+        expect(access.can_do_action?(:ability)).to eq(false)
+      end
+    end
+  end
+
+  describe '#can_push_to_branch?' do
+    include ProjectHelpers
+
+    [:anonymous, :non_member, :guest, :reporter, :maintainer, :admin, :author].each do |membership|
+      context membership.to_s do
+        let(:user) do
+          membership == :author ? snippet.author : create_user_from_membership(project, membership)
+        end
+
+        context 'when can access_git' do
+          it 'respects accessibility' do
+            expected_result = Ability.allowed?(user, :update_snippet, snippet)
+
+            expect(access.can_push_to_branch?('random_branch')).to eq(expected_result)
+          end
+        end
+
+        context 'when can not access_git' do
+          it 'disallows access' do
+            expect(Ability).to receive(:allowed?).with(user, :access_git, :global).and_return(false) if user
+
+            expect(access.can_push_to_branch?('random_branch')).to eq(false)
+          end
+        end
+      end
+    end
+
+    context 'when snippet is nil' do
+      let(:user) { create_user_from_membership(project, :admin) }
+      let(:snippet) { nil }
+
+      it 'disallows access' do
+        expect(access.can_push_to_branch?('random_branch')).to eq(false)
+      end
+    end
+  end
+
+  describe '#can_create_tag?' do
+    it 'returns false' do
+      expect(access.can_create_tag?('random_tag')).to be_falsey
+    end
+  end
+
+  describe '#can_delete_branch?' do
+    it 'returns false' do
+      expect(access.can_delete_branch?('random_branch')).to be_falsey
+    end
+  end
+
+  describe '#can_merge_to_branch?' do
+    it 'returns false' do
+      expect(access.can_merge_to_branch?('random_branch')).to be_falsey
+    end
+  end
+end