Limit invitations to groups and projects

Free plan on .com will limit invites to 20 per day.

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') }

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)
 

changelogs/unreleased/security-limit-invitations.yml

+---
+title: Limit daily invitations to groups and projects
+merge_request:
+author:
+type: security

db/migrate/20201007033527_add_daily_invites_to_plan_limits.rb

+# 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

db/migrate/20201007033723_insert_daily_invites_plan_limits.rb

+# 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

db/schema_migrations/20201007033527

+1200747265d5095a86250020786d6f1e9e50bc75328a71de497046807afa89d7
\ No newline at end of file

db/schema_migrations/20201007033723

+febefead6f966960f6493d29add5f35fc4a1080b5118c5526502fa5fe1d29023
\ No newline at end of file

db/structure.sql

@@ -15520,6 +15520,7 @@ CREATE TABLE plan_limits (
     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,
+    daily_invites integer DEFAULT 0 NOT NULL,
     rubygems_max_file_size bigint DEFAULT '3221225472'::bigint NOT NULL
 );
 

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.

ee/app/services/ee/members/create_service.rb

@@ -3,6 +3,17 @@
 module EE
   module Members
     module CreateService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(source)
+        if invite_quota_exceeded?(source, user_ids)
+          return error(s_("AddMember|Invite limit of %{daily_invites} per day exceeded") % { daily_invites: source.actual_limits.daily_invites })
+        end
+
+        super(source)
+      end
+
       def after_execute(member:)
         super
 
@@ -18,6 +29,14 @@ module EE
           action: :create
         ).for_member(member).security_event
       end
+
+      def invite_quota_exceeded?(source, user_ids)
+        return unless source.actual_limits.daily_invites
+
+        invite_count = ::Member.invite.created_today.in_hierarchy(source).count
+
+        source.actual_limits.exceeded?(:daily_invites, invite_count + user_ids.count)
+      end
     end
   end
 end

ee/spec/services/ee/members/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::CreateService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:root_ancestor) { create(:group) }
+  let_it_be(:project, reload: true) { create(:project, group: root_ancestor) }
+  let_it_be(:subgroup) { create(:group, parent: root_ancestor) }
+  let_it_be(:subgroup_project) { create(:project, group: subgroup) }
+  let_it_be(:project_users) { create_list(:user, 2) }
+
+  let(:params) { { user_ids: project_users.map(&:id).join(','), access_level: Gitlab::Access::GUEST } }
+
+  subject { described_class.new(user, params).execute(project) }
+
+  before_all do
+    project.add_maintainer(user)
+
+    create(:project_member, :invited, project: subgroup_project, created_at: 2.days.ago)
+    create(:project_member, :invited, project: subgroup_project)
+    create(:group_member, :invited, group: subgroup, created_at: 2.days.ago)
+    create(:group_member, :invited, group: subgroup)
+  end
+
+  context 'with group plan' do
+    let(:plan_limits) { create(:plan_limits, daily_invites: daily_invites) }
+    let(:plan) { create(:plan, limits: plan_limits) }
+    let!(:subscription) do
+      create(
+        :gitlab_subscription,
+        namespace: root_ancestor,
+        hosted_plan: plan
+      )
+    end
+
+    shared_examples 'quota limit exceeded' do |limit|
+      it { expect(subject).to include(status: :error, message: "Invite limit of #{limit} per day exceeded") }
+      it { expect { subject }.not_to change { Member.count } }
+    end
+
+    context 'already exceeded invite quota limit' do
+      let(:daily_invites) { 2 }
+
+      it_behaves_like 'quota limit exceeded', 2
+    end
+
+    context 'will exceed invite quota limit' do
+      let(:daily_invites) { 3 }
+
+      it_behaves_like 'quota limit exceeded', 3
+    end
+
+    context 'within invite quota limit' do
+      let(:daily_invites) { 5 }
+
+      it { expect(subject).to eq({ status: :success }) }
+      it do
+        subject
+        expect(project.users).to include(*project_users)
+      end
+    end
+
+    context 'infinite invite quota limit' do
+      let(:daily_invites) { 0 }
+
+      it { expect(subject).to eq({ status: :success }) }
+      it do
+        subject
+        expect(project.users).to include(*project_users)
+      end
+    end
+  end
+
+  context 'without a plan' do
+    let(:plan) { nil }
+
+    it { expect(subject).to eq({ status: :success }) }
+    it do
+      subject
+      expect(project.users).to include(*project_users)
+    end
+  end
+end

locale/gitlab.pot

@@ -1830,6 +1830,9 @@ msgstr ""
 msgid "AddContextCommits|Add/remove"
 msgstr ""
 
+msgid "AddMember|Invite limit of %{daily_invites} per day exceeded"
+msgstr ""
+
 msgid "AddMember|No users specified."
 msgstr ""
 

spec/migrations/insert_daily_invites_plan_limits_spec.rb

+# 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

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) }

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