SECURITY: Validate email constraints when trying to redeem an invite

In certain situations, a logged in user can redeem an invite with an email that
either doesn't match the invite's email or does not adhere to the email domain
restriction of an invite link. The impact of this flaw is aggrevated
when the invite has been configured to add the user that accepts the
invite into restricted groups.

app/controllers/invites_controller.rb

@@ -20,37 +20,22 @@ class InvitesController < ApplicationController
     RateLimiter.new(nil, "invites-show-#{request.remote_ip}", 100, 1.minute).performed!
 
     invite = Invite.find_by(invite_key: params[:id])
+
     if invite.present? && invite.redeemable?
       if current_user
-        if current_user != invite.invited_by
+        redeemed = false
-          InvitedUser.transaction do
+
-            invited_user = InvitedUser.find_or_initialize_by(user: current_user, invite: invite)
+        begin
-            if invited_user.new_record?
+          invite.redeem(email: current_user.email)
-              invited_user.save!
+          redeemed = true
-              Invite.increment_counter(:redemption_count, invite.id)
+        rescue ActiveRecord::RecordNotSaved, Invite::UserExists
-              invite.invited_by.notifications.create!(
+          # This is not ideal but `Invite#redeem` raises either `Invite::UserExists` or `ActiveRecord::RecordNotSaved`
-                notification_type: Notification.types[:invitee_accepted],
+          # error when it fails to redeem the invite. If redemption fails for a logged in user, we will just ignore it.
-                data: { display_username: current_user.username }.to_json
-              )
-            end
-          end
         end
 
-        if invite.groups.present?
+        if redeemed && (topic = invite.topics.first) && current_user.guardian.can_see?(topic)
-          invite_by_guardian = Guardian.new(invite.invited_by)
+          create_topic_invite_notifications(invite, current_user)
-          new_group_ids = invite.groups.pluck(:id) - current_user.group_users.pluck(:group_id)
+          return redirect_to(topic.url)
-          new_group_ids.each do |id|
-            if group = Group.find_by(id: id)
-              group.add(current_user) if invite_by_guardian.can_edit_group?(group)
-            end
-          end
-        end
-
-        if topic = invite.topics.first
-          if current_user.guardian.can_see?(topic)
-            create_topic_invite_notifications(invite, current_user)
-            return redirect_to(topic.url)
-          end
         end
 
         return redirect_to(path("/"))
@@ -60,6 +45,7 @@ class InvitesController < ApplicationController
 
       # Show email if the user already authenticated their email
       different_external_email = false
+
       if session[:authentication]
         auth_result = Auth::Result.from_session_data(session[:authentication], user: nil)
         if invite.email == auth_result.email
@@ -70,6 +56,7 @@ class InvitesController < ApplicationController
       end
 
       email_verified_by_link = invite.email_token.present? && params[:t] == invite.email_token
+
       if email_verified_by_link
         email = invite.email
       end

app/models/invite.rb

@@ -165,11 +165,8 @@ class Invite < ActiveRecord::Base
   def redeem(email: nil, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil, email_token: nil)
     return if !redeemable?
 
-    if is_invite_link? && UserEmail.exists?(email: email)
-      raise UserExists.new I18n.t("invite_link.email_taken")
-    end
-
     email = self.email if email.blank? && !is_invite_link?
+
     InviteRedeemer.new(
       invite: self,
       email: email,

app/models/invite_redeemer.rb

@@ -1,10 +1,9 @@
 # frozen_string_literal: true
 
 InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_custom_fields, :ip_address, :session, :email_token, keyword_init: true) do
-
   def redeem
     Invite.transaction do
-      if invite_was_redeemed?
+      if can_redeem_invite? && mark_invite_redeemed
         process_invitation
         invited_user
       end
@@ -19,13 +18,6 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
       available_username = UserNameSuggester.suggest(email)
     end
 
-    if email.present? && invite.domain.present?
-      username, domain = email.split('@')
-      if domain.present? && invite.domain != domain
-        raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed'))
-      end
-    end
-
     user = User.where(staged: true).with_email(email.strip.downcase).first
     user.unstage! if user
     user ||= User.new
@@ -89,6 +81,39 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
 
   private
 
+  def can_redeem_invite?
+    # Invite has already been redeemed
+    if !invite.is_invite_link? && InvitedUser.exists?(invite_id: invite.id)
+      return false
+    end
+
+    validate_invite_email!
+
+    existing_user = get_existing_user
+
+    if existing_user.present? && InvitedUser.exists?(user_id: existing_user.id, invite_id: invite.id)
+      return false
+    end
+
+    true
+  end
+
+  def validate_invite_email!
+    return if email.blank?
+
+    if invite.email.present? && email.downcase != invite.email.downcase
+      raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.not_matching_email'))
+    end
+
+    if invite.domain.present?
+      username, domain = email.split('@')
+
+      if domain.present? && invite.domain != domain
+        raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed'))
+      end
+    end
+  end
+
   def invited_user
     @invited_user ||= get_invited_user
   end
@@ -100,21 +125,9 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
     notify_invitee
   end
 
-  def invite_was_redeemed?
-    mark_invite_redeemed
-  end
-
   def mark_invite_redeemed
-    if !invite.is_invite_link? && InvitedUser.exists?(invite_id: invite.id)
-      return false
-    end
-
-    existing_user = get_existing_user
-    if existing_user.present? && InvitedUser.exists?(user_id: existing_user.id, invite_id: invite.id)
-      return false
-    end
-
     @invited_user_record = InvitedUser.create!(invite_id: invite.id, redeemed_at: Time.zone.now)
+
     if @invited_user_record.present?
       Invite.increment_counter(:redemption_count, invite.id)
       delete_duplicate_invites
@@ -125,6 +138,7 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
 
   def get_invited_user
     result = get_existing_user
+
     result ||= InviteRedeemer.create_user_from_invite(
       email: email,
       invite: invite,

spec/requests/invites_controller_spec.rb

@@ -71,67 +71,71 @@ describe InvitesController do
       end
     end
 
-    it 'adds logged in users to invite groups' do
+    describe 'logged in user viewing an invite' do
-      group = Fabricate(:group)
+      fab!(:group) { Fabricate(:group) }
-      group.add_owner(invite.invited_by)
-      InvitedGroup.create!(group: group, invite: invite)
 
-      sign_in(user)
+      before do
+        sign_in(user)
+      end
 
-      get "/invites/#{invite.invite_key}"
+      it "redeems the invite when user's email matches invite's email before redirecting to secured topic url" do
-      expect(response).to redirect_to("/")
+        invite.update_columns(email: user.email)
-      expect(user.reload.groups).to include(group)
+        group.add_owner(invite.invited_by)
-    end
 
-    it 'redirects logged in users to invite topic if they can see it' do
+        secured_category = Fabricate(:category)
-      topic = Fabricate(:topic)
+        secured_category.permissions = { group.name => :full }
-      TopicInvite.create!(topic: topic, invite: invite)
+        secured_category.save!
 
-      sign_in(user)
+        topic = Fabricate(:topic, category: secured_category)
+        TopicInvite.create!(invite: invite, topic: topic)
+        InvitedGroup.create!(invite: invite, group: group)
 
-      get "/invites/#{invite.invite_key}"
+        expect do
-      expect(response).to redirect_to(topic.url)
+          get "/invites/#{invite.invite_key}"
-      expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1)
+        end.to change { InvitedUser.exists?(invite: invite, user: user) }.to(true)
-    end
 
-    it 'adds logged in user to group and redirects them to invite topic' do
+        expect(response).to redirect_to(topic.url)
-      group = Fabricate(:group)
+        expect(user.reload.groups).to include(group)
-      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)
 
-      sign_in(user)
+        expect(Notification.exists?(user: user, notification_type: Notification.types[:invited_to_topic], topic: topic))
+          .to eq(true)
 
-      get "/invites/#{invite.invite_key}"
+        expect(Notification.exists?(user: invite.invited_by, notification_type: Notification.types[:invitee_accepted]))
-      expect(user.reload.groups).to include(group)
+          .to eq(true)
-      expect(response).to redirect_to(topic.url)
+      end
-      expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1)
-    end
 
-    it 'creates a notification to inviter' do
+      it "redeems the invite when user's email domain matches the domain an invite link is restricted to" do
-      topic = Fabricate(:topic)
+        invite.update!(email: nil, domain: 'discourse.org')
-      TopicInvite.create!(topic: topic, invite: invite)
+        user.update!(email: "someguy@discourse.org")
+        topic = Fabricate(:topic)
+        TopicInvite.create!(invite: invite, topic: topic)
+        group.add_owner(invite.invited_by)
+        InvitedGroup.create!(invite: invite, group: group)
 
-      sign_in(user)
+        expect do
+          get "/invites/#{invite.invite_key}"
+        end.to change { InvitedUser.exists?(invite: invite, user: user) }.to(true)
 
-      get "/invites/#{invite.invite_key}"
+        expect(response).to redirect_to(topic.url)
-      expect(response).to redirect_to(topic.url)
+        expect(user.reload.groups).to include(group)
-      expect(Notification.where(user: user, notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1)
+      end
-      expect(Notification.where(user: invite.invited_by, notification_type: Notification.types[:invitee_accepted]).count).to eq(1)
-    end
 
-    it 'creates an invited user record' do
+      it "redirects to root if a logged in user tries to view an invite link restricted to a certain domain but user's email domain does not match" do
-      sign_in(invite.invited_by)
+        user.update!(email: "someguy@discourse.com")
-      expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(0)
+        invite.update!(email: nil, domain: 'discourse.org')
-      expect(response.status).to eq(302)
 
-      sign_in(user)
+        expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(0)
-      expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(1)
+
-      expect(response.status).to eq(302)
+        expect(response).to redirect_to("/")
+      end
+
+      it "redirects to root if a tries to view an invite meant for a specific email that is not the user's" do
+        invite.update_columns(email: "notuseremail@discourse.org")
+
+        expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(0)
+
+        expect(response).to redirect_to("/")
+      end
     end
 
     it 'fails if invite does not exist' do