diff options
27 files changed, 300 insertions, 8 deletions
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 4a579892e3f..88c7002b1b6 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -16,6 +16,7 @@ module Ci include ShaAttribute include FromUnion include UpdatedAtFilterable + include EachBatch MAX_OPEN_MERGE_REQUESTS_REFS = 4 diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index a399ffc32de..c2aecc524d4 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -55,6 +55,7 @@ class CommitStatus < ApplicationRecord scope :for_ids, -> (ids) { where(id: ids) } scope :for_ref, -> (ref) { where(ref: ref) } scope :by_name, -> (name) { where(name: name) } + scope :in_pipelines, ->(pipelines) { where(pipeline: pipelines) } scope :for_project_paths, -> (paths) do where(project: Project.where_full_path_in(Array(paths))) diff --git a/app/models/member.rb b/app/models/member.rb index 2e79b50d6b7..62fe757683f 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -47,6 +47,19 @@ class Member < ApplicationRecord }, if: :project_bot? + scope :in_hierarchy, ->(source) do + groups = source.root_ancestor.self_and_descendants + group_members = Member.default_scoped.where(source: groups) + + projects = source.root_ancestor.all_projects + project_members = Member.default_scoped.where(source: projects) + + Member.default_scoped.from_union([ + group_members, + project_members + ]).merge(self) + end + # This scope encapsulates (most of) the conditions a row in the member table # must satisfy if it is a valid permission. Of particular note: # @@ -79,12 +92,18 @@ class Member < ApplicationRecord scope :invite, -> { where.not(invite_token: nil) } scope :non_invite, -> { where(invite_token: nil) } + scope :request, -> { where.not(requested_at: nil) } scope :non_request, -> { where(requested_at: nil) } scope :not_accepted_invitations, -> { invite.where(invite_accepted_at: nil) } scope :not_accepted_invitations_by_user, -> (user) { not_accepted_invitations.where(created_by: user) } scope :not_expired, -> (today = Date.current) { where(arel_table[:expires_at].gt(today).or(arel_table[:expires_at].eq(nil))) } + + scope :created_today, -> do + now = Date.current + where(created_at: now.beginning_of_day..now.end_of_day) + end scope :last_ten_days_excluding_today, -> (today = Date.current) { where(created_at: (today - 10).beginning_of_day..(today - 1).end_of_day) } scope :has_access, -> { active.where('access_level > 0') } diff --git a/app/services/ci/abort_project_pipelines_service.rb b/app/services/ci/abort_project_pipelines_service.rb new file mode 100644 index 00000000000..0b2fa9ed3c0 --- /dev/null +++ b/app/services/ci/abort_project_pipelines_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Ci + class AbortProjectPipelinesService + # Danger: Cancels in bulk without callbacks + # Only for pipeline abandonment scenarios (current example: project delete) + def execute(project) + return unless Feature.enabled?(:abort_deleted_project_pipelines, default_enabled: :yaml) + + pipelines = project.all_pipelines.cancelable + bulk_abort!(pipelines, status: :canceled) + + ServiceResponse.success(message: 'Pipelines canceled') + end + + private + + def bulk_abort!(pipelines, status:) + pipelines.each_batch do |pipeline_batch| + CommitStatus.in_pipelines(pipeline_batch).in_batches.update_all(status: status) # rubocop: disable Cop/InBatches + pipeline_batch.update_all(status: status) + end + end + end +end diff --git a/app/services/ci/cancel_user_pipelines_service.rb b/app/services/ci/cancel_user_pipelines_service.rb index 3a8b5e91088..3d3a8032e8e 100644 --- a/app/services/ci/cancel_user_pipelines_service.rb +++ b/app/services/ci/cancel_user_pipelines_service.rb @@ -6,6 +6,7 @@ module Ci # This is a bug with CodeReuse/ActiveRecord cop # https://gitlab.com/gitlab-org/gitlab/issues/32332 def execute(user) + # TODO: fix N+1 queries https://gitlab.com/gitlab-org/gitlab/-/issues/300685 user.pipelines.cancelable.find_each(&:cancel_running) ServiceResponse.success(message: 'Pipeline canceled') diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb index 5fcf2d711b0..cffccda1a44 100644 --- a/app/services/members/create_service.rb +++ b/app/services/members/create_service.rb @@ -2,12 +2,12 @@ module Members class CreateService < Members::BaseService + include Gitlab::Utils::StrongMemoize + DEFAULT_LIMIT = 100 def execute(source) - return error(s_('AddMember|No users specified.')) if params[:user_ids].blank? - - user_ids = params[:user_ids].split(',').uniq.flatten + return error(s_('AddMember|No users specified.')) if user_ids.blank? return error(s_("AddMember|Too many users specified (limit is %{user_limit})") % { user_limit: user_limit }) if user_limit && user_ids.size > user_limit @@ -47,6 +47,13 @@ module Members private + def user_ids + strong_memoize(:user_ids) do + ids = params[:user_ids] || '' + ids.split(',').uniq.flatten + end + end + def user_limit limit = params.fetch(:limit, DEFAULT_LIMIT) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index bec75657530..c1501625300 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -21,11 +21,14 @@ module Projects def execute return false unless can?(current_user, :remove_project, project) + project.update_attribute(:pending_delete, true) # Flush the cache for both repositories. This has to be done _before_ # removing the physical repositories as some expiration code depends on # Git data (e.g. a list of branch names). flush_caches(project) + ::Ci::AbortProjectPipelinesService.new.execute(project) + Projects::UnlinkForkService.new(project, current_user).execute attempt_destroy(project) diff --git a/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml b/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml new file mode 100644 index 00000000000..de92707cb8f --- /dev/null +++ b/changelogs/unreleased/security-cancel-pipelines-for-deleted-project.yml @@ -0,0 +1,5 @@ +--- +title: Cancel running and pending jobs when a project is deleted +merge_request: 1220 +author: +type: security diff --git a/changelogs/unreleased/security-limit-invitations.yml b/changelogs/unreleased/security-limit-invitations.yml new file mode 100644 index 00000000000..353d1cec727 --- /dev/null +++ b/changelogs/unreleased/security-limit-invitations.yml @@ -0,0 +1,5 @@ +--- +title: Limit daily invitations to groups and projects +merge_request: +author: +type: security diff --git a/config/feature_flags/development/abort_deleted_project_pipelines.yml b/config/feature_flags/development/abort_deleted_project_pipelines.yml new file mode 100644 index 00000000000..f09cc9dd86b --- /dev/null +++ b/config/feature_flags/development/abort_deleted_project_pipelines.yml @@ -0,0 +1,8 @@ +--- +name: abort_deleted_project_pipelines +introduced_by_url: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1220 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/301106 +milestone: '13.9' +type: development +group: group::continuous integration +default_enabled: true diff --git a/db/migrate/20201007033527_add_daily_invites_to_plan_limits.rb b/db/migrate/20201007033527_add_daily_invites_to_plan_limits.rb new file mode 100644 index 00000000000..8f0079cd639 --- /dev/null +++ b/db/migrate/20201007033527_add_daily_invites_to_plan_limits.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddDailyInvitesToPlanLimits < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column(:plan_limits, :daily_invites, :integer, default: 0, null: false) + end +end diff --git a/db/migrate/20201007033723_insert_daily_invites_plan_limits.rb b/db/migrate/20201007033723_insert_daily_invites_plan_limits.rb new file mode 100644 index 00000000000..dcdcbbb0964 --- /dev/null +++ b/db/migrate/20201007033723_insert_daily_invites_plan_limits.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class InsertDailyInvitesPlanLimits < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + return unless Gitlab.com? + + create_or_update_plan_limit('daily_invites', 'free', 20) + create_or_update_plan_limit('daily_invites', 'bronze', 0) + create_or_update_plan_limit('daily_invites', 'silver', 0) + create_or_update_plan_limit('daily_invites', 'gold', 0) + end + + def down + return unless Gitlab.com? + + create_or_update_plan_limit('daily_invites', 'free', 0) + create_or_update_plan_limit('daily_invites', 'bronze', 0) + create_or_update_plan_limit('daily_invites', 'silver', 0) + create_or_update_plan_limit('daily_invites', 'gold', 0) + end +end diff --git a/db/schema_migrations/20201007033527 b/db/schema_migrations/20201007033527 new file mode 100644 index 00000000000..b2cedd57973 --- /dev/null +++ b/db/schema_migrations/20201007033527 @@ -0,0 +1 @@ +1200747265d5095a86250020786d6f1e9e50bc75328a71de497046807afa89d7
\ No newline at end of file diff --git a/db/schema_migrations/20201007033723 b/db/schema_migrations/20201007033723 new file mode 100644 index 00000000000..c874ae0475b --- /dev/null +++ b/db/schema_migrations/20201007033723 @@ -0,0 +1 @@ +febefead6f966960f6493d29add5f35fc4a1080b5118c5526502fa5fe1d29023
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index de4218ed405..c1f13a2fca4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -15254,7 +15254,8 @@ CREATE TABLE plan_limits ( project_feature_flags integer DEFAULT 200 NOT NULL, ci_max_artifact_size_api_fuzzing integer DEFAULT 0 NOT NULL, ci_pipeline_deployments integer DEFAULT 500 NOT NULL, - pull_mirror_interval_seconds integer DEFAULT 300 NOT NULL + pull_mirror_interval_seconds integer DEFAULT 300 NOT NULL, + daily_invites integer DEFAULT 0 NOT NULL ); CREATE SEQUENCE plan_limits_id_seq diff --git a/doc/administration/instance_limits.md b/doc/administration/instance_limits.md index 0eede5f3ca5..a3452a1a605 100644 --- a/doc/administration/instance_limits.md +++ b/doc/administration/instance_limits.md @@ -96,6 +96,13 @@ Read more on the [Rack Attack initializer](../security/rack_attack.md) method of - **Default rate limit** - Disabled +### Member Invitations + +Limit the maximum daily member invitations allowed per group hierarchy. + +- GitLab.com: Free members may invite 20 members per day. +- Self-managed: Invites are not limited. + ## Gitaly concurrency limit Clone traffic can put a large strain on your Gitaly service. To prevent such workloads from overwhelming your Gitaly server, you can set concurrency limits in Gitaly’s configuration file. diff --git a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb index e68d9020a21..55c125e03d5 100644 --- a/lib/gitlab/ci/pipeline/chain/validate/abilities.rb +++ b/lib/gitlab/ci/pipeline/chain/validate/abilities.rb @@ -10,6 +10,10 @@ module Gitlab include Chain::Helpers def perform! + if project.pending_delete? + return error('Project is deleted!') + end + unless project.builds_enabled? return error('Pipelines are disabled!') end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e47bbec804f..0b1b80d59ff 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1807,6 +1807,9 @@ msgstr "" msgid "AddContextCommits|Add/remove" msgstr "" +msgid "AddMember|Invite limit of %{daily_invites} per day exceeded" +msgstr "" + msgid "AddMember|No users specified." msgstr "" diff --git a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb index ae3270cb9b2..7aaeee32f49 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb @@ -74,6 +74,14 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do it 'does not break the chain' do expect(step.break?).to eq false end + + context 'when project is deleted' do + before do + project.update!(pending_delete: true) + end + + specify { expect(step.perform!).to contain_exactly('Project is deleted!') } + end end describe '#allowed_to_write_ref?' do diff --git a/spec/migrations/insert_daily_invites_plan_limits_spec.rb b/spec/migrations/insert_daily_invites_plan_limits_spec.rb new file mode 100644 index 00000000000..3265efcb0ce --- /dev/null +++ b/spec/migrations/insert_daily_invites_plan_limits_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20201007033723_insert_daily_invites_plan_limits.rb') + +RSpec.describe InsertDailyInvitesPlanLimits do + let(:plans) { table(:plans) } + let(:plan_limits) { table(:plan_limits) } + let!(:free_plan) { plans.create!(name: 'free') } + let!(:bronze_plan) { plans.create!(name: 'bronze') } + let!(:silver_plan) { plans.create!(name: 'silver') } + let!(:gold_plan) { plans.create!(name: 'gold') } + + context 'when on Gitlab.com' do + before do + expect(Gitlab).to receive(:com?).at_most(:twice).and_return(true) + end + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect(plan_limits.where.not(daily_invites: 0)).to be_empty + } + + # Expectations will run after the up migration. + migration.after -> { + expect(plan_limits.pluck(:plan_id, :daily_invites)).to contain_exactly( + [free_plan.id, 20], + [bronze_plan.id, 0], + [silver_plan.id, 0], + [gold_plan.id, 0] + ) + } + end + end + end + + context 'when on self hosted' do + before do + expect(Gitlab).to receive(:com?).at_most(:twice).and_return(false) + end + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect(plan_limits.pluck(:daily_invites)).to eq [] + } + + migration.after -> { + expect(plan_limits.pluck(:daily_invites)).to eq [] + } + end + end + end +end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index f5e824bb066..140527e4414 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -34,6 +34,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { is_expected.to have_many(:auto_canceled_jobs) } it { is_expected.to have_many(:sourced_pipelines) } it { is_expected.to have_many(:triggered_pipelines) } + it { is_expected.to have_many(:pipeline_artifacts) } it { is_expected.to have_one(:chat_data) } it { is_expected.to have_one(:source_pipeline) } @@ -41,14 +42,15 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { is_expected.to have_one(:source_job) } it { is_expected.to have_one(:pipeline_config) } - it { is_expected.to validate_presence_of(:sha) } - it { is_expected.to validate_presence_of(:status) } - it { is_expected.to respond_to :git_author_name } it { is_expected.to respond_to :git_author_email } it { is_expected.to respond_to :short_sha } it { is_expected.to delegate_method(:full_path).to(:project).with_prefix } - it { is_expected.to have_many(:pipeline_artifacts) } + + describe 'validations' do + it { is_expected.to validate_presence_of(:sha) } + it { is_expected.to validate_presence_of(:status) } + end describe 'associations' do it 'has a bidirectional relationship with projects' do diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 1a791820f1b..b60af7abade 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -171,6 +171,43 @@ RSpec.describe Member do end end + describe '.in_hierarchy' do + let(:root_ancestor) { create(:group) } + let(:project) { create(:project, group: root_ancestor) } + let(:subgroup) { create(:group, parent: root_ancestor) } + let(:subgroup_project) { create(:project, group: subgroup) } + + let!(:root_ancestor_member) { create(:group_member, group: root_ancestor) } + let!(:project_member) { create(:project_member, project: project) } + let!(:subgroup_member) { create(:group_member, group: subgroup) } + let!(:subgroup_project_member) { create(:project_member, project: subgroup_project) } + + let(:hierarchy_members) do + [ + root_ancestor_member, + project_member, + subgroup_member, + subgroup_project_member + ] + end + + subject { Member.in_hierarchy(project) } + + it { is_expected.to contain_exactly(*hierarchy_members) } + + context 'with scope prefix' do + subject { Member.where.not(source: project).in_hierarchy(subgroup) } + + it { is_expected.to contain_exactly(root_ancestor_member, subgroup_member, subgroup_project_member) } + end + + context 'with scope suffix' do + subject { Member.in_hierarchy(project).where.not(source: project) } + + it { is_expected.to contain_exactly(root_ancestor_member, subgroup_member, subgroup_project_member) } + end + end + describe '.invite' do it { expect(described_class.invite).not_to include @maintainer } it { expect(described_class.invite).to include @invited_member } @@ -251,6 +288,21 @@ RSpec.describe Member do it { is_expected.to include(expiring_tomorrow, not_expiring) } end + describe '.created_today' do + let_it_be(:now) { Time.current } + let_it_be(:created_today) { create(:group_member, created_at: now.beginning_of_day) } + let_it_be(:created_yesterday) { create(:group_member, created_at: now - 1.day) } + + before do + travel_to now + end + + subject { described_class.created_today } + + it { is_expected.not_to include(created_yesterday) } + it { is_expected.to include(created_today) } + end + describe '.last_ten_days_excluding_today' do let_it_be(:now) { Time.current } let_it_be(:created_today) { create(:group_member, created_at: now.beginning_of_day) } diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 67fb11f34e0..4259c8b708b 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -209,6 +209,7 @@ RSpec.describe PlanLimits do ci_pipeline_size ci_active_jobs storage_size_limit + daily_invites ] + disabled_max_artifact_size_columns end diff --git a/spec/services/ci/abort_project_pipelines_service_spec.rb b/spec/services/ci/abort_project_pipelines_service_spec.rb new file mode 100644 index 00000000000..9af909ac2ab --- /dev/null +++ b/spec/services/ci/abort_project_pipelines_service_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::AbortProjectPipelinesService do + let_it_be(:project) { create(:project) } + let_it_be(:pipeline) { create(:ci_pipeline, :running, project: project) } + let_it_be(:build) { create(:ci_build, :running, pipeline: pipeline) } + + describe '#execute' do + it 'cancels all running pipelines and related jobs' do + result = described_class.new.execute(project) + + expect(result).to be_success + expect(pipeline.reload).to be_canceled + expect(build.reload).to be_canceled + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new { described_class.new.execute(project) }.count + + pipelines = create_list(:ci_pipeline, 5, :running, project: project) + create_list(:ci_build, 5, :running, pipeline: pipelines.first) + + expect { described_class.new.execute(project) }.not_to exceed_query_limit(control_count) + end + end + + context 'when feature disabled' do + before do + stub_feature_flags(abort_deleted_project_pipelines: false) + end + + it 'does not abort the pipeline' do + result = described_class.new.execute(project) + + expect(result).to be(nil) + expect(pipeline.reload).to be_running + expect(build.reload).to be_running + end + end +end diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index f0f09218b06..75d1c98923a 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -69,6 +69,12 @@ RSpec.describe Projects::DestroyService, :aggregate_failures do destroy_project(project, user, {}) end + it 'performs cancel for project ci pipelines' do + expect(::Ci::AbortProjectPipelinesService).to receive_message_chain(:new, :execute).with(project) + + destroy_project(project, user, {}) + end + context 'when project has remote mirrors' do let!(:project) do create(:project, :repository, namespace: user.namespace).tap do |project| diff --git a/vendor/gitignore/C++.gitignore b/vendor/gitignore/C++.gitignore index 259148fa18f..259148fa18f 100755..100644 --- a/vendor/gitignore/C++.gitignore +++ b/vendor/gitignore/C++.gitignore diff --git a/vendor/gitignore/Java.gitignore b/vendor/gitignore/Java.gitignore index a1c2a238a96..a1c2a238a96 100755..100644 --- a/vendor/gitignore/Java.gitignore +++ b/vendor/gitignore/Java.gitignore |