Merge branch '247910-invite-tokens-not-cleared-after-invite-is-accepted' into 'master'

Cleanup invalid membership invites

See merge request gitlab-org/gitlab!67760

app/models/member.rb

@@ -168,7 +168,7 @@ class Member < ApplicationRecord
 
   scope :on_project_and_ancestors, ->(project) { where(source: [project] + project.ancestors) }
 
-  before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? }
+  before_validation :generate_invite_token, on: :create, if: -> (member) { member.invite_email.present? && !member.invite_accepted_at? }
 
   after_create :send_invite, if: :invite?, unless: :importing?
   after_create :send_request, if: :request?, unless: :importing?

db/post_migrate/20210809123658_orphaned_invite_tokens_cleanup.rb

+# frozen_string_literal: true
+
+class OrphanedInviteTokensCleanup < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  TMP_INDEX_NAME = 'tmp_idx_orphaned_invite_tokens'
+  QUERY_CONDITION = "invite_token IS NOT NULL and invite_accepted_at IS NOT NULL and invite_accepted_at < created_at"
+
+  def up
+    membership = define_batchable_model('members')
+
+    add_concurrent_index('members', :id, where: QUERY_CONDITION, name: TMP_INDEX_NAME)
+
+    membership.where(QUERY_CONDITION).pluck(:id).each_slice(10) do |group|
+      membership.where(id: group).where(QUERY_CONDITION).update_all(invite_token: nil)
+    end
+
+    remove_concurrent_index_by_name('members', TMP_INDEX_NAME)
+  end
+
+  def down
+    remove_concurrent_index_by_name('members', TMP_INDEX_NAME) if index_exists_by_name?('members', TMP_INDEX_NAME)
+
+    # This migration is irreversible
+  end
+end

db/schema_migrations/20210809123658

+f4a1963c8f21b8c767766c3a18037bae223efce8452c87f570cf9789d6f666d6
\ No newline at end of file

spec/migrations/orphaned_invite_tokens_cleanup_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration! 'orphaned_invite_tokens_cleanup'
+
+RSpec.describe OrphanedInviteTokensCleanup, :migration do
+  def create_member(**extra_attributes)
+    defaults = {
+      access_level: 10,
+      source_id: 1,
+      source_type: "Project",
+      notification_level: 0,
+      type: 'ProjectMember'
+    }
+
+    table(:members).create!(defaults.merge(extra_attributes))
+  end
+
+  describe '#up', :aggregate_failures do
+    it 'removes invite tokens for accepted records with invite_accepted_at < created_at' do
+      record1 = create_member(invite_token: 'foo', invite_accepted_at: 1.day.ago, created_at: 1.hour.ago)
+      record2 = create_member(invite_token: 'foo2', invite_accepted_at: nil, created_at: 1.hour.ago)
+      record3 = create_member(invite_token: 'foo3', invite_accepted_at: 1.day.ago, created_at: 1.year.ago)
+
+      migrate!
+
+      expect(table(:members).find(record1.id).invite_token).to eq nil
+      expect(table(:members).find(record2.id).invite_token).to eq 'foo2'
+      expect(table(:members).find(record3.id).invite_token).to eq 'foo3'
+    end
+  end
+end

spec/models/member_spec.rb

@@ -667,7 +667,23 @@ RSpec.describe Member do
     let!(:member) { create(:project_member, invite_email: "user@example.com", user: nil) }
 
     it "sets the invite token" do
-      expect { member.generate_invite_token }.to change { member.invite_token}
+      expect { member.generate_invite_token }.to change { member.invite_token }
+    end
+  end
+
+  describe 'generate invite token on create' do
+    let!(:member) { build(:project_member, invite_email: "user@example.com") }
+
+    it "sets the invite token" do
+      expect { member.save! }.to change { member.invite_token }.to(kind_of(String))
+    end
+
+    context 'when invite was already accepted' do
+      it "does not set invite token" do
+        member.invite_accepted_at = 1.day.ago
+
+        expect { member.save! }.not_to change { member.invite_token }.from(nil)
+      end
     end
   end