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

9 files changed, 211 insertions, 11 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 395ff400fae..a2c56abb2f2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +4,11 @@ entry.
 
 ## 13.2.5 (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.2.4 (2020-08-11)
 
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 62f66093875..92cba5f8f7d 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -166,6 +166,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
@@ -173,6 +174,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 39b39bd2fce..3a245119cb7 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -603,8 +603,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
 
@@ -632,6 +637,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 &&
       (
@@ -643,6 +649,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 0b391c8cba9..b62f9b55b64 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -541,7 +541,7 @@ RSpec.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 @@ RSpec.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 733cc9bd9cb..9bd692b45c3 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -63,6 +63,24 @@ RSpec.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 @@ RSpec.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 @@ RSpec.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 @@ RSpec.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 @@ RSpec.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 @@ RSpec.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
@@ -166,6 +204,14 @@ RSpec.describe GroupPolicy do
     context 'with admin mode', :enable_admin_mode do
       specify { expect_allowed(*admin_permissions) }
     end
+
+    it_behaves_like 'deploy token does not get confused with user' do
+      let(:user_id) { admin.id }
+
+      context 'with admin mode', :enable_admin_mode do
+        it { expect_disallowed(*admin_permissions) }
+      end
+    end
   end
 
   describe 'private nested group use the highest access level from the group and inherited permissions' do
diff --git a/spec/requests/api/maven_packages_spec.rb b/spec/requests/api/maven_packages_spec.rb
index 189d6a4c1a4..b9351308545 100644
--- a/spec/requests/api/maven_packages_spec.rb
+++ b/spec/requests/api/maven_packages_spec.rb
@@ -193,6 +193,24 @@ RSpec.describe API::MavenPackages do
       it_behaves_like 'downloads with a job token'
 
       it_behaves_like 'downloads with a deploy token'
+
+      it 'does not allow download by a unauthorized deploy token with same id as a user with access' do
+        unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+        another_user = create(:user)
+        project.add_developer(another_user)
+
+        # We force the id of the deploy token and the user to be the same
+        unauthorized_deploy_token.update!(id: another_user.id)
+
+        download_file(
+          package_file.file_name,
+          {},
+          Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token
+        )
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
     end
 
     context 'project name is different from a package name' do
@@ -451,6 +469,20 @@ RSpec.describe API::MavenPackages do
       expect(response).to have_gitlab_http_status(:ok)
     end
 
+    it 'rejects requests by a unauthorized deploy token with same id as a user with access' do
+      unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+      another_user = create(:user)
+      project.add_developer(another_user)
+
+      # We force the id of the deploy token and the user to be the same
+      unauthorized_deploy_token.update!(id: another_user.id)
+
+      authorize_upload({}, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token))
+
+      expect(response).to have_gitlab_http_status(:forbidden)
+    end
+
     def authorize_upload(params = {}, request_headers = headers)
       put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/maven-metadata.xml/authorize"), params: params, headers: request_headers
     end
@@ -538,6 +570,20 @@ RSpec.describe API::MavenPackages do
         expect(response).to have_gitlab_http_status(:ok)
       end
 
+      it 'rejects uploads by a unauthorized deploy token with same id as a user with access' do
+        unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true)
+
+        another_user = create(:user)
+        project.add_developer(another_user)
+
+        # We force the id of the deploy token and the user to be the same
+        unauthorized_deploy_token.update!(id: another_user.id)
+
+        upload_file(params, headers.merge(Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token))
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+
       context 'version is not correct' do
         let(:version) { '$%123' }
 
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index f7771c7b0f9..082857ab738 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -547,12 +547,6 @@ RSpec.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 @@ RSpec.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 f8526ec68dc..df8e4bc96dd 100644
--- a/spec/support/shared_examples/policies/project_policy_shared_examples.rb
+++ b/spec/support/shared_examples/policies/project_policy_shared_examples.rb
@@ -97,6 +97,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) }
 
@@ -115,6 +137,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
@@ -128,7 +154,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
@@ -139,7 +165,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
@@ -165,6 +191,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
@@ -186,6 +216,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
@@ -207,6 +241,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
@@ -228,6 +266,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
@@ -249,6 +291,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
@@ -268,5 +332,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