Merge branch 'security-improper-access-control-on-deploy-key' into 'master'

Improper Access Control on Deploy-Key

See merge request gitlab-org/security/gitlab!840

changelogs/unreleased/security-improper-access-control-on-deploy-key.yml

+---
+title: Fix Improper Access Control on Deploy-Key
+merge_request:
+author:
+type: security

lib/gitlab/git_access.rb

@@ -98,6 +98,13 @@ module Gitlab
       Guest.can?(download_ability, container)
     end
 
+    def deploy_key_can_download_code?
+      authentication_abilities.include?(:download_code) &&
+        deploy_key? &&
+        deploy_key.has_access_to?(container) &&
+        (project? && project&.repository_access_level != ::Featurable::DISABLED)
+    end
+
     def user_can_download_code?
       authentication_abilities.include?(:download_code) &&
         user_access.can_do_action?(download_ability)
@@ -257,7 +264,7 @@ module Gitlab
     end
 
     def check_download_access!
-      passed = deploy_key? ||
+      passed = deploy_key_can_download_code? ||
         deploy_token? ||
         user_can_download_code? ||
         build_can_download_code? ||

spec/lib/gitlab/git_access_spec.rb

@@ -472,31 +472,135 @@ RSpec.describe Gitlab::GitAccess do
       let(:actor) { key }
 
       context 'pull code' do
-        context 'when project is authorized' do
-          before do
-            key.projects << project
+        context 'when project is public' do
+          let(:project) { create(:project, :public, :repository, *options) }
+
+          context 'when deploy key exists in the project' do
+            before do
+              key.projects << project
+            end
+
+            context 'when the repository is public' do
+              let(:options) { %i[repository_enabled] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') }
+            end
           end
 
-          it { expect { pull_access_check }.not_to raise_error }
+          context 'when deploy key does not exist in the project' do
+            context 'when the repository is public' do
+              let(:options) { %i[repository_enabled] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') }
+            end
+          end
         end
 
-        context 'when unauthorized' do
-          context 'from public project' do
-            let(:project) { create(:project, :public, :repository) }
+        context 'when project is internal' do
+          let(:project) { create(:project, :internal, :repository, *options) }
 
-            it { expect { pull_access_check }.not_to raise_error }
+          context 'when deploy key exists in the project' do
+            before do
+              key.projects << project
+            end
+
+            context 'when the repository is public' do
+              let(:options) { %i[repository_enabled] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') }
+            end
           end
 
-          context 'from internal project' do
-            let(:project) { create(:project, :internal, :repository) }
+          context 'when deploy key does not exist in the project' do
+            context 'when the repository is public' do
+              let(:options) { %i[repository_enabled] }
 
-            it { expect { pull_access_check }.to raise_not_found }
+              it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') }
+            end
+
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') }
+            end
           end
+        end
 
-          context 'from private project' do
-            let(:project) { create(:project, :private, :repository) }
+        context 'when project is private' do
+          let(:project) { create(:project, :private, :repository, *options) }
 
-            it { expect { pull_access_check }.to raise_not_found }
+          context 'when deploy key exists in the project' do
+            before do
+              key.projects << project
+            end
+
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.not_to raise_error }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('You are not allowed to download code from this project.') }
+            end
+          end
+
+          context 'when deploy key does not exist in the project' do
+            context 'when the repository is private' do
+              let(:options) { %i[repository_private] }
+
+              it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') }
+            end
+
+            context 'when the repository is disabled' do
+              let(:options) { %i[repository_disabled] }
+
+              it { expect { pull_access_check }.to raise_error('The project you were looking for could not be found.') }
+            end
           end
         end
       end