From ec414d01e371e4fcdd959836e04bb4c2b3be857f Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Tue, 24 Jan 2023 13:46:05 +0100 Subject: [PATCH] Add deploy_key associations and validations to protected tags Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/325415 After DB migration: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109795 * Add validations for 'create_access_level' * Add associations for 'create_access_level' * Split protected_tag_create_access_level factories between FOSS and EE Changelog: added --- app/models/deploy_key.rb | 1 + .../protected_tag/create_access_level.rb | 34 +++++ .../protected_tags/create_access_levels.rb | 4 +- .../import_export/project/import_export.yml | 1 + .../protected_tags/create_access_levels.rb | 9 ++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/deploy_key_spec.rb | 1 + .../protected_tag/create_access_level_spec.rb | 144 ++++++++++++++++++ 8 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 spec/factories/protected_tags/create_access_levels.rb create mode 100644 spec/models/protected_tag/create_access_level_spec.rb diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb index 47829a07495d62..ef31bedc3a8251 100644 --- a/app/models/deploy_key.rb +++ b/app/models/deploy_key.rb @@ -12,6 +12,7 @@ class DeployKey < Key has_many :deploy_keys_projects_with_write_access, -> { with_write_access }, class_name: "DeployKeysProject", inverse_of: :deploy_key has_many :projects_with_write_access, -> { includes(:route) }, class_name: 'Project', through: :deploy_keys_projects_with_write_access, source: :project has_many :protected_branch_push_access_levels, class_name: '::ProtectedBranch::PushAccessLevel', inverse_of: :deploy_key + has_many :protected_tag_create_access_levels, class_name: '::ProtectedTag::CreateAccessLevel', inverse_of: :deploy_key scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where(deploy_keys_projects: { project_id: projects }) } scope :with_write_access, -> { joins(:deploy_keys_projects).merge(DeployKeysProject.with_write_access) } diff --git a/app/models/protected_tag/create_access_level.rb b/app/models/protected_tag/create_access_level.rb index 5d8b1fb4f71c57..abb233d3800fd1 100644 --- a/app/models/protected_tag/create_access_level.rb +++ b/app/models/protected_tag/create_access_level.rb @@ -4,9 +4,43 @@ class ProtectedTag::CreateAccessLevel < ApplicationRecord include Importable include ProtectedTagAccess + belongs_to :deploy_key + + validates :access_level, uniqueness: { scope: :protected_tag_id, if: :role?, + conditions: -> { where(user_id: nil, group_id: nil, deploy_key_id: nil) } } + validates :deploy_key_id, uniqueness: { scope: :protected_tag_id, allow_nil: true } + validate :validate_deploy_key_membership + + def type + if deploy_key.present? + :deploy_key + else + super + end + end + def check_access(user) return false if access_level == Gitlab::Access::NO_ACCESS + if user && deploy_key.present? + return user.can?(:read_project, project) && enabled_deploy_key_for_user?(deploy_key, user) + end + super end + + private + + def validate_deploy_key_membership + return unless deploy_key + + return if project.deploy_keys_projects.where(deploy_key: deploy_key).exists? + + errors.add(:deploy_key, 'is not enabled for this project') + end + + def enabled_deploy_key_for_user?(deploy_key, user) + deploy_key.user_id == user.id && + DeployKey.with_write_access_for_project(protected_tag.project, deploy_key: deploy_key).any? + end end diff --git a/ee/spec/factories/protected_tags/create_access_levels.rb b/ee/spec/factories/protected_tags/create_access_levels.rb index e90d649bf981b6..3bc6a3155a30b1 100644 --- a/ee/spec/factories/protected_tags/create_access_levels.rb +++ b/ee/spec/factories/protected_tags/create_access_levels.rb @@ -1,10 +1,8 @@ # frozen_string_literal: true -FactoryBot.define do +FactoryBot.modify do factory :protected_tag_create_access_level, class: 'ProtectedTag::CreateAccessLevel' do user { nil } group { nil } - protected_tag - access_level { Gitlab::Access::DEVELOPER } end end diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 9936499686443a..7c24bf9569554c 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -1005,6 +1005,7 @@ excluded_attributes: - :protected_branch_id create_access_levels: - :protected_tag_id + - :deploy_key_id deploy_access_levels: - :protected_environment_id boards: diff --git a/spec/factories/protected_tags/create_access_levels.rb b/spec/factories/protected_tags/create_access_levels.rb new file mode 100644 index 00000000000000..07450b2789c105 --- /dev/null +++ b/spec/factories/protected_tags/create_access_levels.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :protected_tag_create_access_level, class: 'ProtectedTag::CreateAccessLevel' do + deploy_key { nil } + protected_tag + access_level { Gitlab::Access::DEVELOPER } + end +end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 8750bf4387cee9..2208eba3c10bce 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -386,6 +386,7 @@ create_access_levels: - user - protected_tag - group +- deploy_key container_repositories: - project - name diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index a74da896cd8400..337fa40b4ba2bd 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -22,6 +22,7 @@ it { is_expected.to have_many(:projects) } it { is_expected.to have_many(:protected_branch_push_access_levels).inverse_of(:deploy_key) } + it { is_expected.to have_many(:protected_tag_create_access_levels).inverse_of(:deploy_key) } end describe 'notification' do diff --git a/spec/models/protected_tag/create_access_level_spec.rb b/spec/models/protected_tag/create_access_level_spec.rb new file mode 100644 index 00000000000000..566f8695388394 --- /dev/null +++ b/spec/models/protected_tag/create_access_level_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ProtectedTag::CreateAccessLevel, feature_category: :source_code_management do + describe 'associations' do + it { is_expected.to belong_to(:deploy_key) } + end + + describe 'validations', :aggregate_failures do + let_it_be(:protected_tag) { create(:protected_tag) } + + it 'verifies access levels' do + is_expected.to validate_inclusion_of(:access_level).in_array( + [ + Gitlab::Access::MAINTAINER, + Gitlab::Access::DEVELOPER, + Gitlab::Access::NO_ACCESS + ] + ) + end + + context 'when deploy key enabled for the project' do + let(:deploy_key) { create(:deploy_key, projects: [protected_tag.project]) } + + it 'is valid' do + level = build(:protected_tag_create_access_level, protected_tag: protected_tag, deploy_key: deploy_key) + + expect(level).to be_valid + end + end + + context 'when a record exists with the same access level' do + before do + create(:protected_tag_create_access_level, protected_tag: protected_tag) + end + + it 'is not valid' do + level = build(:protected_tag_create_access_level, protected_tag: protected_tag) + + expect(level).to be_invalid + expect(level.errors.full_messages).to include('Access level has already been taken') + end + end + + context 'when a deploy key already added for this access level' do + let!(:create_access_level) do + create(:protected_tag_create_access_level, protected_tag: protected_tag, deploy_key: deploy_key) + end + + let(:deploy_key) { create(:deploy_key, projects: [protected_tag.project]) } + + it 'is not valid' do + level = build(:protected_tag_create_access_level, protected_tag: protected_tag, deploy_key: deploy_key) + + expect(level).to be_invalid + expect(level.errors.full_messages).to contain_exactly('Deploy key has already been taken') + end + end + + context 'when deploy key is not enabled for the project' do + let(:create_access_level) do + build(:protected_tag_create_access_level, protected_tag: protected_tag, deploy_key: create(:deploy_key)) + end + + it 'returns an error' do + expect(create_access_level).to be_invalid + expect(create_access_level.errors.full_messages).to contain_exactly( + 'Deploy key is not enabled for this project' + ) + end + end + end + + describe '#check_access' do + let_it_be(:project) { create(:project) } + let_it_be(:protected_tag) { create(:protected_tag, :no_one_can_create, project: project) } + let_it_be(:user) { create(:user) } + let_it_be(:deploy_key) { create(:deploy_key, user: user) } + + let!(:deploy_keys_project) do + create(:deploy_keys_project, project: project, deploy_key: deploy_key, can_push: can_push) + end + + let(:create_access_level) { protected_tag.create_access_levels.first } + let(:can_push) { true } + + before_all do + project.add_maintainer(user) + end + + it { expect(create_access_level.check_access(user)).to be_falsey } + + context 'when this create_access_level is tied to a deploy key' do + let(:create_access_level) do + create(:protected_tag_create_access_level, protected_tag: protected_tag, deploy_key: deploy_key) + end + + context 'when the deploy key is among the active keys for this project' do + it { expect(create_access_level.check_access(user)).to be_truthy } + end + + context 'when user is missing' do + it { expect(create_access_level.check_access(nil)).to be_falsey } + end + + context 'when deploy key does not belong to the user' do + let(:another_user) { create(:user) } + + it { expect(create_access_level.check_access(another_user)).to be_falsey } + end + + context 'when user cannot access the project' do + before do + allow(user).to receive(:can?).with(:read_project, project).and_return(false) + end + + it { expect(create_access_level.check_access(user)).to be_falsey } + end + + context 'when the deploy key is not among the active keys of this project' do + let(:can_push) { false } + + it { expect(create_access_level.check_access(user)).to be_falsey } + end + end + end + + describe '#type' do + let(:create_access_level) { build(:protected_tag_create_access_level) } + + it 'returns :role by default' do + expect(create_access_level.type).to eq(:role) + end + + context 'when a deploy key is tied to the protected branch' do + let(:create_access_level) { build(:protected_tag_create_access_level, deploy_key: build(:deploy_key)) } + + it 'returns :deploy_key' do + expect(create_access_level.type).to eq(:deploy_key) + end + end + end +end -- GitLab