SECURITY: Prevent email from being nil in InviteRedeemer (#19005)

This commit adds some protections in InviteRedeemer to ensure that email
can never be nil, which could cause issues with inviting the invited
person to private topics since there was an incorrect inner join.

If the email is nil and the invite is scoped to an email, we just use
that invite.email unconditionally.  If a redeeming_user (an existing
    user) is passed in when redeeming an email, we use their email to
override the passed in email.  Otherwise we just use the passed in
email.  We now raise an error after all this if the email is still nil.
This commit also adds some tests to catch the private topic fix, and
some general improvements and comments around the invite code.

This commit also includes a migration to delete TopicAllowedUser records
for users who were mistakenly added to topics as part of the invite
redemption process.

app/controllers/session_controller.rb

@@ -648,7 +648,7 @@ class SessionController < ApplicationController
     end
 
     if invite.redeemable?
-      if !invite.is_invite_link? && sso.email != invite.email
+      if invite.is_email_invite? && sso.email != invite.email
         raise Invite::ValidationFailed.new(I18n.t("invite.not_matching_email"))
       end
     elsif invite.expired?

app/models/invite.rb

@@ -66,8 +66,16 @@ class Invite < ActiveRecord::Base
     end
   end
 
+  # Even if a domain is specified on the invite, it still counts as
+  # an invite link.
   def is_invite_link?
-    email.blank?
+    self.email.blank?
+  end
+
+  # Email invites have specific behaviour and it's easier to visually
+  # parse is_email_invite? than !is_invite_link?
+  def is_email_invite?
+    self.email.present?
   end
 
   def redeemable?

app/models/invite_redeemer.rb

@@ -1,7 +1,21 @@
 # frozen_string_literal: true
 
+# NOTE: There are a _lot_ of complicated rules and conditions for our
+# invite system, and the code is spread out through a lot of places.
+# Tread lightly and read carefully when modifying this code. You may
+# also want to look at:
+#
+# * InvitesController
+# * SessionController
+# * Invite model
+# * User model
+#
+# Invites that are scoped to a specific email (email IS NOT NULL on the Invite
+# model) have different rules to invites that are considered an "invite link",
+# (email IS NULL) on the Invite model.
 InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_custom_fields, :ip_address, :session, :email_token, :redeeming_user, keyword_init: true) do
   def redeem
+    ensure_email_is_present!(email)
     Invite.transaction do
       if can_redeem_invite? && mark_invite_redeemed
         process_invitation
@@ -10,6 +24,27 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
     end
   end
 
+  # The email must be present in some form since many of the methods
+  # for processing + redemption rely on it. If it's still nil after
+  # these checks then we have hit an edge case and should not proceed!
+  def ensure_email_is_present!(email)
+    if email.blank?
+      Rails.logger.warn(
+        "email param was blank in InviteRedeemer for invite ID #{invite.id}. The `redeeming_user` was #{self.redeeming_user.present? ? "(ID: #{self.redeeming_user.id})" : "not"} present.",
+      )
+    end
+
+    if email.blank? && self.invite.is_email_invite?
+      self.email = self.invite.email
+    elsif self.redeeming_user.present?
+      self.email = self.redeeming_user.email
+    else
+      self.email = email
+    end
+
+    raise Discourse::InvalidParameters if self.email.blank?
+  end
+
   # extracted from User cause it is very specific to invites
   def self.create_user_from_invite(email:, invite:, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil, email_token: nil)
     if username && UsernameValidator.new(username).valid_format? && User.username_available?(username, email)
@@ -72,7 +107,10 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
     user.save!
     authenticator.finish
 
-    if invite.emailed_status != Invite.emailed_status_types[:not_required] && email == invite.email && invite.email_token.present? && email_token == invite.email_token
+    if invite.emailed_status != Invite.emailed_status_types[:not_required] &&
+        email == invite.email &&
+        invite.email_token.present? &&
+        email_token == invite.email_token
       user.activate
     end
 
@@ -83,15 +121,18 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
 
   def can_redeem_invite?
     return false if !invite.redeemable?
+    return false if email.blank?
 
-    # Invite has already been redeemed by anyone.
-    if !invite.is_invite_link? && InvitedUser.exists?(invite_id: invite.id)
+    # Invite scoped to email has already been redeemed by anyone.
+    if invite.is_email_invite? && InvitedUser.exists?(invite_id: invite.id)
       return false
     end
 
-    # Email will not be present if we are claiming an invite link, which
-    # does not have an email or domain scope on the invitation.
-    if email.present? || redeeming_user.present?
+    # The email will be present for either an invite link (where the user provides
+    # us the email manually) or for an invite scoped to an email, where we
+    # prefill the email and do not let the user modify it.
+    #
+    # Note that an invite link can also have a domain scope which must be checked.
     email_to_check = redeeming_user&.email || email
 
     if invite.email.present? && !invite.email_matches?(email_to_check)
@@ -101,7 +142,6 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
     if invite.domain.present? && !invite.domain_matches?(email_to_check)
       raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed'))
     end
-    end
 
     # Anon user is trying to redeem an invitation, if an existing user already
     # redeemed it then we cannot redeem now.
@@ -161,9 +201,18 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
   end
 
   def add_to_private_topics_if_invited
-    topic_ids = Topic.where(archetype: Archetype::private_message).includes(:invites).where(invites: { email: email }).pluck(:id)
+    # Should not happen because of ensure_email_is_present!, but better to cover bases.
+    return if email.blank?
+
+    topic_ids = TopicInvite.joins(:invite)
+      .joins(:topic)
+      .where("topics.archetype = ?", Archetype::private_message)
+      .where("invites.email = ?", email)
+      .pluck(:topic_id)
     topic_ids.each do |id|
-      TopicAllowedUser.create!(user_id: invited_user.id, topic_id: id) unless TopicAllowedUser.exists?(user_id: invited_user.id, topic_id: id)
+      if !TopicAllowedUser.exists?(user_id: invited_user.id, topic_id: id)
+        TopicAllowedUser.create!(user_id: invited_user.id, topic_id: id)
+      end
     end
   end
 
@@ -185,15 +234,17 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
   end
 
   def notify_invitee
-    if inviter = invite.invited_by
-      inviter.notifications.create!(
+    return if invite.invited_by.blank?
+    invite.invited_by.notifications.create!(
       notification_type: Notification.types[:invitee_accepted],
       data: { display_username: invited_user.username }.to_json
     )
   end
-  end
 
   def delete_duplicate_invites
+    # Should not happen because of ensure_email_is_present!, but better to cover bases.
+    return if email.blank?
+
     Invite
       .where('invites.max_redemptions_allowed = 1')
       .joins("LEFT JOIN invited_users ON invites.id = invited_users.invite_id")

db/migrate/20221103051248_remove_invalid_topic_allowed_users_from_invites.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+class RemoveInvalidTopicAllowedUsersFromInvites < ActiveRecord::Migration[6.1]
+  def up
+    # We are getting all the topic_allowed_users records that
+    # match an invited user, which is created as part of the invite
+    # redemption flow. The original invite would _not_ have had a topic_invite
+    # record, and the user should have been added to the topic in the brief
+    # period between creation of the invited_users record and the update of
+    # that record.
+    #
+    # Having > 2 topic allowed users disqualifies messages sent only
+    # by the system or an admin to the user.
+    subquery_sql = <<~SQL
+      SELECT DISTINCT id
+      FROM (
+               SELECT tau.id, tau.user_id, COUNT(*) OVER (PARTITION BY tau.user_id)
+               FROM topic_allowed_users tau
+                    JOIN invited_users iu ON iu.user_id = tau.user_id
+                    LEFT JOIN topic_invites ti ON ti.invite_id = iu.invite_id AND tau.topic_id = ti.topic_id
+               WHERE ti.id IS NULL
+                 AND tau.created_at BETWEEN iu.created_at AND iu.updated_at
+                 AND iu.redeemed_at > '2022-10-27'
+           ) AS matching_topic_allowed_users
+      WHERE matching_topic_allowed_users.count > 2
+    SQL
+
+    # Back up the records we are going to change in case we are too
+    # brutal, and for further inspection.
+    #
+    # TODO DROP this table (topic_allowed_users_backup_nov_2022) in a later migration.
+    DB.exec(<<~SQL)
+      CREATE TABLE topic_allowed_users_backup_nov_2022
+      (
+          id       INT NOT NULL,
+          user_id  INT NOT NULL,
+          topic_id INT NOT NULL
+      );
+      INSERT INTO topic_allowed_users_backup_nov_2022(id, user_id, topic_id)
+      SELECT id, user_id, topic_id
+      FROM topic_allowed_users
+      WHERE id IN (
+                      #{subquery_sql}
+                  )
+    SQL
+
+    # Delete the invalid topic allowed users that should not be there.
+    DB.query(<<~SQL)
+      DELETE
+      FROM topic_allowed_users
+      WHERE id IN (
+                      #{subquery_sql}
+                  )
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/invite_redeemer_spec.rb

@@ -5,6 +5,85 @@ require 'rails_helper'
 describe InviteRedeemer do
   fab!(:admin) { Fabricate(:admin) }
 
+  describe "#initialize" do
+    fab!(:redeeming_user) { Fabricate(:user, email: "redeemer@test.com") }
+
+    context "for invite link" do
+      fab!(:invite) { Fabricate(:invite, email: nil) }
+
+      context "when an email is passed in without a redeeming user" do
+        it "uses that email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: "blah@test.com")
+          expect { redeemer.redeem }.to change { User.count }
+          expect(redeemer.email).to eq("blah@test.com")
+          expect(User.find_by_email(redeemer.email)).to be_present
+        end
+      end
+
+      context "when an email is passed in with a redeeming user" do
+        it "uses the redeeming user's email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: "blah@test.com", redeeming_user: redeeming_user)
+          expect { redeemer.redeem }.not_to change { User.count }
+          expect(redeemer.email).to eq(redeeming_user.email)
+        end
+      end
+
+      context "when an email is not passed in with a redeeming user" do
+        it "uses the redeeming user's email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: nil, redeeming_user: redeeming_user)
+          expect { redeemer.redeem }.not_to change { User.count }
+          expect(redeemer.email).to eq(redeeming_user.email)
+        end
+      end
+
+      context "when no email and no redeeming user is passed in" do
+        it "raises an error" do
+          expect { described_class.new(invite: invite, email: nil, redeeming_user: nil).redeem }.to raise_error(
+            Discourse::InvalidParameters
+          )
+        end
+      end
+    end
+
+    context "for invite with email" do
+      fab!(:invite) { Fabricate(:invite, email: "foobar@example.com") }
+
+      context "when an email is passed in without a redeeming user" do
+        it "uses that email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: "foobar@example.com")
+          expect { redeemer.redeem }.to change { User.count }
+          expect(redeemer.email).to eq("foobar@example.com")
+          expect(User.find_by_email(redeemer.email)).to be_present
+        end
+      end
+
+      context "when an email is passed in with a redeeming user" do
+        it "uses the redeeming user's email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: "blah@test.com", redeeming_user: redeeming_user)
+          expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email"))
+          expect(redeemer.email).to eq(redeeming_user.email)
+        end
+      end
+
+      context "when an email is not passed in with a redeeming user" do
+        it "uses the invite email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: nil, redeeming_user: redeeming_user)
+          expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email"))
+          expect(redeemer.email).to eq("foobar@example.com")
+        end
+      end
+
+      context "when no email and no redeeming user is passed in" do
+        it "uses the invite email for invite redemption" do
+          redeemer = described_class.new(invite: invite, email: nil, redeeming_user: nil)
+          expect { redeemer.redeem }.to change { User.count }
+          expect(redeemer.email).to eq("foobar@example.com")
+          expect(User.find_by_email(redeemer.email)).to be_present
+        end
+      end
+    end
+  end
+
   describe '.create_user_from_invite' do
     it "should be created correctly" do
       invite = Fabricate(:invite, email: 'walter.white@email.com')
@@ -121,6 +200,8 @@ describe InviteRedeemer do
     let(:password) { 'know5nOthiNG' }
     let(:invite_redeemer) { InviteRedeemer.new(invite: invite, email: invite.email, username: username, name: name) }
 
+    context "with email" do
+      fab!(:invite) { Fabricate(:invite, email: "foobar@example.com") }
       context "when must_approve_users setting is enabled" do
         before do
           SiteSetting.must_approve_users = true
@@ -249,6 +330,15 @@ describe InviteRedeemer do
         expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email"))
       end
 
+      it "adds the user to the appropriate private topic and no others" do
+        topic1 = Fabricate(:private_message_topic)
+        topic2 = Fabricate(:private_message_topic)
+        TopicInvite.create(invite: invite, topic: topic1)
+        user = InviteRedeemer.new(invite: invite, email: invite.email, username: username, name: name, password: password).redeem
+        expect(TopicAllowedUser.exists?(topic: topic1, user: user)).to eq(true)
+        expect(TopicAllowedUser.exists?(topic: topic2, user: user)).to eq(false)
+      end
+
       context "when a redeeming user is passed in" do
         fab!(:redeeming_user) { Fabricate(:user, email: "foobar@example.com") }
 
@@ -257,8 +347,22 @@ describe InviteRedeemer do
           redeemer = InviteRedeemer.new(invite: invite, redeeming_user: redeeming_user)
           expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email"))
         end
+
+        it "adds the user to the appropriate private topic and no others" do
+          topic1 = Fabricate(:private_message_topic)
+          topic2 = Fabricate(:private_message_topic)
+          TopicInvite.create(invite: invite, topic: topic1)
+          InviteRedeemer.new(invite: invite, redeeming_user: redeeming_user).redeem
+          expect(TopicAllowedUser.exists?(topic: topic1, user: redeeming_user)).to eq(true)
+          expect(TopicAllowedUser.exists?(topic: topic2, user: redeeming_user)).to eq(false)
         end
 
+        it "does not create a topic allowed user record if the invited user is already in the topic" do
+          topic1 = Fabricate(:private_message_topic)
+          TopicInvite.create(invite: invite, topic: topic1)
+          TopicAllowedUser.create(topic: topic1, user: redeeming_user)
+          expect { InviteRedeemer.new(invite: invite, redeeming_user: redeeming_user).redeem }.not_to change { TopicAllowedUser.count }
+        end
         context 'with domain' do
           fab!(:invite) { Fabricate(:invite, email: nil, domain: "test.com") }
 
@@ -325,6 +429,7 @@ describe InviteRedeemer do
             end
           end
         end
-
+      end
+    end
   end
 end

spec/requests/invites_controller_spec.rb

@@ -801,11 +801,93 @@ describe InvitesController do
     end
 
     context 'when user is already logged in' do
+      before { sign_in(user) }
+
+      context "for an email invite" do
         fab!(:invite) { Fabricate(:invite, email: 'test@example.com') }
         fab!(:user) { Fabricate(:user, email: 'test@example.com') }
         fab!(:group) { Fabricate(:group) }
 
-      before { sign_in(user) }
+        it 'redeems the invitation and creates the invite accepted notification' do
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(200)
+          expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success"))
+          invite.reload
+          expect(invite.invited_users.first.user).to eq(user)
+          expect(invite.redeemed?).to be_truthy
+          expect(
+            Notification.exists?(
+              user: invite.invited_by, notification_type: Notification.types[:invitee_accepted]
+            )
+          ).to eq(true)
+        end
+
+        it 'redirects to the first topic the user was invited to and creates the topic notification' do
+          topic = Fabricate(:topic)
+          TopicInvite.create!(invite: invite, topic: topic)
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(200)
+          expect(response.parsed_body['redirect_to']).to eq(topic.relative_url)
+          expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1)
+        end
+
+        it "adds the user to the private topic" do
+          topic = Fabricate(:private_message_topic)
+          TopicInvite.create!(invite: invite, topic: topic)
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(200)
+          expect(response.parsed_body['redirect_to']).to eq(topic.relative_url)
+          expect(TopicAllowedUser.exists?(user: user, topic: topic)).to eq(true)
+        end
+
+        it "adds the user to the groups specified on the invite and allows them to access the secure topic" do
+          group.add_owner(invite.invited_by)
+          secured_category = Fabricate(:category)
+          secured_category.permissions = { group.name => :full }
+          secured_category.save!
+
+          topic = Fabricate(:topic, category: secured_category)
+          TopicInvite.create!(invite: invite, topic: topic)
+          InvitedGroup.create!(invite: invite, group: group)
+
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(200)
+          expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success"))
+          expect(response.parsed_body['redirect_to']).to eq(topic.relative_url)
+          invite.reload
+          expect(invite.redeemed?).to be_truthy
+          expect(user.reload.groups).to include(group)
+          expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1)
+        end
+
+        it "does not try to log in the user automatically" do
+          expect do
+            put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          end.not_to change { UserAuthToken.count }
+          expect(response.status).to eq(200)
+          expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success"))
+        end
+
+        it "errors if the user's email doesn't match the invite email" do
+          user.update!(email: "blah@test.com")
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(412)
+          expect(response.parsed_body["message"]).to eq(I18n.t("invite.not_matching_email"))
+        end
+
+        it "errors if the user's email domain doesn't match the invite domain" do
+          user.update!(email: "blah@test.com")
+          invite.update!(email: nil, domain: "example.com")
+          put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
+          expect(response.status).to eq(412)
+          expect(response.parsed_body["message"]).to eq(I18n.t("invite.domain_not_allowed"))
+        end
+      end
+
+      context "for an invite link" do
+        fab!(:invite) { Fabricate(:invite, email: nil) }
+        fab!(:user) { Fabricate(:user, email: 'test@example.com') }
+        fab!(:group) { Fabricate(:group) }
 
         it 'redeems the invitation and creates the invite accepted notification' do
           put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
@@ -857,20 +939,6 @@ describe InvitesController do
           expect(response.status).to eq(200)
           expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success"))
         end
-
-      it "errors if the user's email doesn't match the invite email" do
-        user.update!(email: "blah@test.com")
-        put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
-        expect(response.status).to eq(412)
-        expect(response.parsed_body["message"]).to eq(I18n.t("invite.not_matching_email"))
-      end
-
-      it "errors if the user's email domain doesn't match the invite domain" do
-        user.update!(email: "blah@test.com")
-        invite.update!(email: nil, domain: "example.com")
-        put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key }
-        expect(response.status).to eq(412)
-        expect(response.parsed_body["message"]).to eq(I18n.t("invite.domain_not_allowed"))
       end
     end