diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2020-08-18 08:51:48 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2020-08-18 08:51:48 +0000 |
commit | 692ef21eec707375f60b2a850be78e1dc2e8e3aa (patch) | |
tree | bb00ea4038bb647d1d4f887c71253f3db4f0e033 | |
parent | b03c3aceeb00deff189822dbda0d241a67878ce1 (diff) | |
download | gitlab-ce-692ef21eec707375f60b2a850be78e1dc2e8e3aa.tar.gz |
Add latest changes from gitlab-org/gitlab@13-1-stable-ee
-rw-r--r-- | CHANGELOG.md | 6 | ||||
-rw-r--r-- | app/policies/group_policy.rb | 7 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 7 | ||||
-rw-r--r-- | lib/gitlab/auth.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/auth_spec.rb | 9 | ||||
-rw-r--r-- | spec/policies/group_policy_spec.rb | 38 | ||||
-rw-r--r-- | spec/requests/lfs_http_spec.rb | 15 | ||||
-rw-r--r-- | spec/support/shared_examples/policies/project_policy_shared_examples.rb | 83 |
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 |