Merge branch 'sh-fix-orphaned-invited-members' into 'master'

Gracefully handle orphaned member invites

Closes #23198

See merge request gitlab-org/gitlab!30355

app/helpers/members_helper.rb

@@ -15,7 +15,18 @@ module MembersHelper
       elsif member.invite?
         "revoke the invitation for #{member.invite_email} to join"
       else
+        if member.user
           "remove #{member.user.name} from"
+        else
+          e = RuntimeError.new("Data integrity error: no associated user for member ID #{member.id}")
+          Gitlab::ErrorTracking.track_exception(e,
+            member_id: member.id,
+            invite_email: member.invite_email,
+            invite_accepted_at: member.invite_accepted_at,
+            source_id: member.source_id,
+            source_type: member.source_type)
+          "remove this orphaned member from"
+        end
       end
 
     "#{text} #{action} the #{member.source.human_name} #{source_text(member)}?"

changelogs/unreleased/sh-fix-orphaned-invited-members.yml

+---
+title: Gracefully handle orphaned member invites
+merge_request: 30355
+author:
+type: fixed

db/migrate/20200424050250_remove_orphaned_invited_members.rb

+# frozen_string_literal: true
+
+class RemoveOrphanedInvitedMembers < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    # As of 2020-04-23, there are 19 entries on GitLab.com that match this criteria.
+    execute "DELETE FROM members WHERE user_id IS NULL AND invite_token IS NULL AND invite_accepted_at IS NOT NULL"
+  end
+
+  def down
+  end
+end

db/structure.sql

@@ -13462,5 +13462,6 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200423080334
 20200423080607
 20200423081409
+20200424050250
 \.
 

spec/helpers/members_helper_spec.rb

@@ -22,6 +22,17 @@ describe MembersHelper do
     it { expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to revoke the invitation for #{group_member_invite.invite_email} to join the #{group.name} group?" }
     it { expect(remove_member_message(group_member_request)).to eq "Are you sure you want to deny #{requester.name}'s request to join the #{group.name} group?" }
     it { expect(remove_member_message(group_member_request, user: requester)).to eq "Are you sure you want to withdraw your access request for the #{group.name} group?" }
+
+    context 'an accepted user invitation with no user associated' do
+      before do
+        group_member_invite.update(invite_email: "#{SecureRandom.hex}@example.com", invite_token: nil, user_id: nil)
+      end
+
+      it 'logs an exception and shows orphaned status' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(anything, hash_including(:member_id, :invite_email, :invite_accepted_at))
+        expect(remove_member_message(group_member_invite)).to eq "Are you sure you want to remove this orphaned member from the #{group.name} group and any subresources?"
+      end
+    end
   end
 
   describe '#remove_member_title' do

spec/migrations/remove_orphaned_invited_members_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200424050250_remove_orphaned_invited_members.rb')
+
+describe RemoveOrphanedInvitedMembers do
+  let(:members_table) { table(:members) }
+  let(:users_table) { table(:users) }
+  let(:namespaces_table) { table(:namespaces) }
+  let(:projects_table) { table(:projects) }
+
+  let!(:user1) { users_table.create!(name: 'user1', email: 'user1@example.com', projects_limit: 1) }
+  let!(:user2) { users_table.create!(name: 'user2', email: 'user2@example.com', projects_limit: 1) }
+  let!(:group) { namespaces_table.create!(type: 'Group', name: 'group', path: 'group') }
+  let!(:project) { projects_table.create!(name: 'project', path: 'project', namespace_id: group.id) }
+
+  let!(:member1) { create_member(user_id: user1.id, source_type: 'Project', source_id: project.id, access_level: 10) }
+  let!(:member2) { create_member(user_id: user2.id, source_type: 'Group', source_id: group.id, access_level: 20) }
+
+  let!(:invited_member1) do
+    create_member(user_id: nil, source_type: 'Project', source_id: project.id,
+                  invite_token: SecureRandom.hex, invite_accepted_at: Time.now,
+                  access_level: 20)
+  end
+  let!(:invited_member2) do
+    create_member(user_id: nil, source_type: 'Group', source_id: group.id,
+                  invite_token: SecureRandom.hex, invite_accepted_at: Time.now,
+                  access_level: 20)
+  end
+
+  let!(:orphaned_member1) do
+    create_member(user_id: nil, source_type: 'Project', source_id: project.id,
+                  invite_accepted_at: Time.now, access_level: 30)
+  end
+  let!(:orphaned_member2) do
+    create_member(user_id: nil, source_type: 'Group', source_id: group.id,
+                  invite_accepted_at: Time.now, access_level: 20)
+  end
+
+  it 'removes orphaned invited members but keeps current members' do
+    expect { migrate! }.to change { members_table.count }.from(6).to(4)
+
+    expect(members_table.all.pluck(:id)).to contain_exactly(member1.id, member2.id, invited_member1.id, invited_member2.id)
+  end
+
+  def create_member(options)
+    members_table.create!(
+      {
+        notification_level: 0,
+        ldap: false,
+        override: false
+      }.merge(options)
+    )
+  end
+end