Add latest changes from gitlab-org/gitlab@13-1-stable-ee

8 files changed, 157 insertions, 11 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 32bebc1a3a7..482c6b00ee2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,11 @@ entry.
 
 ## 13.1.7 (2020-08-17)
 
-- No changes.
+### Security (2 changes)
+
+- Stop deploy token being mis-used as user in ProjectPolicy and GroupPolicy.
+- Project access is checked during deploy token authentication.
+
 
 ## 13.1.6 (2020-08-05)
 
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index b1b52d62b85..861105520a8 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -149,6 +149,7 @@ class GroupPolicy < BasePolicy
 
   def access_level
     return GroupMember::NO_ACCESS if @user.nil?
+    return GroupMember::NO_ACCESS unless user_is_user?
 
     @access_level ||= lookup_access_level!
   end
@@ -156,6 +157,12 @@ class GroupPolicy < BasePolicy
   def lookup_access_level!
     @subject.max_member_access_for_user(@user)
   end
+
+  private
+
+  def user_is_user?
+    user.is_a?(User)
+  end
 end
 
 GroupPolicy.prepend_if_ee('EE::GroupPolicy')
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index f87c72007ec..f4a7d6f8fc6 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -578,8 +578,13 @@ class ProjectPolicy < BasePolicy
 
   private
 
+  def user_is_user?
+    user.is_a?(User)
+  end
+
   def team_member?
     return false if @user.nil?
+    return false unless user_is_user?
 
     greedy_load_subject = false
 
@@ -607,6 +612,7 @@ class ProjectPolicy < BasePolicy
   # rubocop: disable CodeReuse/ActiveRecord
   def project_group_member?
     return false if @user.nil?
+    return false unless user_is_user?
 
     project.group &&
       (
@@ -618,6 +624,7 @@ class ProjectPolicy < BasePolicy
 
   def team_access_level
     return -1 if @user.nil?
+    return -1 unless user_is_user?
 
     lookup_access_level!
   end
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 44e8c9c04b9..1a23814959d 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -218,6 +218,9 @@ module Gitlab
         return unless token && login
         return if login != token.username
 
+        # Registry access (with jwt) does not have access to project
+        return if project && !token.has_access_to?(project)
+
         scopes = abilities_for_scopes(token.scopes)
 
         if valid_scoped_token?(token, all_available_scopes)
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 870f02b6933..21767af9abf 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -541,7 +541,7 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
 
         it 'fails if token is not related to project' do
           another_deploy_token = create(:deploy_token)
-          expect(gl_auth.find_for_git_client(login, another_deploy_token.token, project: project, ip: 'ip'))
+          expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project, ip: 'ip'))
             .to eq(auth_failure)
         end
 
@@ -566,6 +566,13 @@ describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
 
           expect(subject).to eq(auth_success)
         end
+
+        it 'fails if token is not related to group' do
+          another_deploy_token = create(:deploy_token, :group, read_repository: true)
+
+          expect(gl_auth.find_for_git_client(another_deploy_token.username, another_deploy_token.token, project: project_with_group, ip: 'ip'))
+            .to eq(auth_failure)
+        end
       end
 
       context 'when the deploy token has read_registry as a scope' do
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 6b17a8285a2..796b5ef23d7 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -63,6 +63,24 @@ describe GroupPolicy do
     end
   end
 
+  shared_examples 'deploy token does not get confused with user' do
+    before do
+      deploy_token.update!(id: user_id)
+    end
+
+    let(:deploy_token) { create(:deploy_token) }
+    let(:current_user) { deploy_token }
+
+    it do
+      expect_disallowed(*read_group_permissions)
+      expect_disallowed(*guest_permissions)
+      expect_disallowed(*reporter_permissions)
+      expect_disallowed(*developer_permissions)
+      expect_disallowed(*maintainer_permissions)
+      expect_disallowed(*owner_permissions)
+    end
+  end
+
   context 'guests' do
     let(:current_user) { guest }
 
@@ -74,6 +92,10 @@ describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { guest.id }
+    end
   end
 
   context 'reporter' do
@@ -87,6 +109,10 @@ describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { reporter.id }
+    end
   end
 
   context 'developer' do
@@ -100,6 +126,10 @@ describe GroupPolicy do
       expect_disallowed(*maintainer_permissions)
       expect_disallowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { developer.id }
+    end
   end
 
   context 'maintainer' do
@@ -136,6 +166,10 @@ describe GroupPolicy do
         expect_disallowed(*owner_permissions)
       end
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { maintainer.id }
+    end
   end
 
   context 'owner' do
@@ -149,6 +183,10 @@ describe GroupPolicy do
       expect_allowed(*maintainer_permissions)
       expect_allowed(*owner_permissions)
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { owner.id }
+    end
   end
 
   context 'admin' do
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index f3fa5e36fec..e726d513a3d 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -547,12 +547,6 @@ describe 'Git LFS API and storage' do
           project.lfs_objects << lfs_object
         end
 
-        context 'when Deploy Token is valid' do
-          let(:deploy_token) { create(:deploy_token, projects: [project]) }
-
-          it_behaves_like 'an authorized request', renew_authorization: false
-        end
-
         context 'when Deploy Token is not valid' do
           let(:deploy_token) { create(:deploy_token, projects: [project], read_repository: false) }
 
@@ -562,7 +556,14 @@ describe 'Git LFS API and storage' do
         context 'when Deploy Token is not related to the project' do
           let(:deploy_token) { create(:deploy_token, projects: [other_project]) }
 
-          it_behaves_like 'LFS http 404 response'
+          it_behaves_like 'LFS http 401 response'
+        end
+
+        # TODO: We should fix this test case that causes flakyness by alternating the result of the above test cases.
+        context 'when Deploy Token is valid' do
+          let(:deploy_token) { create(:deploy_token, projects: [project]) }
+
+          it_behaves_like 'an authorized request', renew_authorization: false
         end
       end
 
diff --git a/spec/support/shared_examples/policies/project_policy_shared_examples.rb b/spec/support/shared_examples/policies/project_policy_shared_examples.rb
index 4dd0152e3d1..e1fb65057a3 100644
--- a/spec/support/shared_examples/policies/project_policy_shared_examples.rb
+++ b/spec/support/shared_examples/policies/project_policy_shared_examples.rb
@@ -75,6 +75,28 @@ RSpec.shared_examples 'project policies as anonymous' do
   end
 end
 
+RSpec.shared_examples 'deploy token does not get confused with user' do
+  before do
+    deploy_token.update!(id: user_id)
+
+    # Project with public builds are available to all
+    project.update!(public_builds: false)
+  end
+
+  let(:deploy_token) { create(:deploy_token) }
+
+  subject { described_class.new(deploy_token, project) }
+
+  it do
+    expect_disallowed(*guest_permissions)
+    expect_disallowed(*reporter_permissions)
+    expect_disallowed(*team_member_reporter_permissions)
+    expect_disallowed(*developer_permissions)
+    expect_disallowed(*maintainer_permissions)
+    expect_disallowed(*owner_permissions)
+  end
+end
+
 RSpec.shared_examples 'project policies as guest' do
   subject { described_class.new(guest, project) }
 
@@ -93,6 +115,10 @@ RSpec.shared_examples 'project policies as guest' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { guest.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { guest_permissions }
     end
@@ -106,7 +132,7 @@ RSpec.shared_examples 'project policies as guest' do
 
     context 'when public builds disabled' do
       before do
-        project.update(public_builds: false)
+        project.update!(public_builds: false)
       end
 
       it do
@@ -117,7 +143,7 @@ RSpec.shared_examples 'project policies as guest' do
 
     context 'when builds are disabled' do
       before do
-        project.project_feature.update(builds_access_level: ProjectFeature::DISABLED)
+        project.project_feature.update!(builds_access_level: ProjectFeature::DISABLED)
       end
 
       it do
@@ -143,6 +169,10 @@ RSpec.shared_examples 'project policies as reporter' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { reporter.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { reporter_permissions }
     end
@@ -164,6 +194,10 @@ RSpec.shared_examples 'project policies as developer' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { developer.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { developer_permissions }
     end
@@ -185,6 +219,10 @@ RSpec.shared_examples 'project policies as maintainer' do
       expect_disallowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { maintainer.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { maintainer_permissions }
     end
@@ -206,6 +244,10 @@ RSpec.shared_examples 'project policies as owner' do
       expect_allowed(*owner_permissions)
     end
 
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { owner.id }
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { owner_permissions }
     end
@@ -227,6 +269,28 @@ RSpec.shared_examples 'project policies as admin with admin mode' do
       expect_allowed(*owner_permissions)
     end
 
+    context 'deploy token does not get confused with user' do
+      before do
+        allow(deploy_token).to receive(:id).and_return(admin.id)
+
+        # Project with public builds are available to all
+        project.update!(public_builds: false)
+      end
+
+      let(:deploy_token) { create(:deploy_token) }
+
+      subject { described_class.new(deploy_token, project) }
+
+      it do
+        expect_disallowed(*guest_permissions)
+        expect_disallowed(*reporter_permissions)
+        expect_disallowed(*team_member_reporter_permissions)
+        expect_disallowed(*developer_permissions)
+        expect_disallowed(*maintainer_permissions)
+        expect_disallowed(*owner_permissions)
+      end
+    end
+
     it_behaves_like 'archived project policies' do
       let(:regular_abilities) { owner_permissions }
     end
@@ -240,5 +304,20 @@ RSpec.shared_examples 'project policies as admin without admin mode' do
     subject { described_class.new(admin, project) }
 
     it { is_expected.to be_banned }
+
+    context 'deploy token does not get confused with user' do
+      before do
+        allow(deploy_token).to receive(:id).and_return(admin.id)
+
+        # Project with public builds are available to all
+        project.update!(public_builds: false)
+      end
+
+      let(:deploy_token) { create(:deploy_token) }
+
+      subject { described_class.new(deploy_token, project) }
+
+      it { is_expected.to be_banned }
+    end
   end
 end