Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Prevent email from being nil in InviteRedeemer (#19004)
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.
  • Loading branch information
martin-brennan committed Nov 14, 2022
1 parent 78157b4 commit a414520
Show file tree
Hide file tree
Showing 6 changed files with 506 additions and 213 deletions.
2 changes: 1 addition & 1 deletion app/controllers/session_controller.rb
Expand Up @@ -760,7 +760,7 @@ def validate_invitiation!(sso)
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?
Expand Down
12 changes: 9 additions & 3 deletions app/models/invite.rb
Expand Up @@ -74,8 +74,16 @@ def email_xor_domain
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?
Expand Down Expand Up @@ -201,8 +209,6 @@ def redeem(
)
return if !redeemable?

email = self.email if email.blank? && !is_invite_link?

InviteRedeemer.new(
invite: self,
email: email,
Expand Down
105 changes: 80 additions & 25 deletions app/models/invite_redeemer.rb
@@ -1,5 +1,18 @@
# 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.
class InviteRedeemer
attr_reader :invite,
:email,
Expand All @@ -13,7 +26,7 @@ class InviteRedeemer
:redeeming_user

def initialize(
invite: nil,
invite:,
email: nil,
username: nil,
name: nil,
Expand All @@ -23,9 +36,7 @@ def initialize(
session: nil,
email_token: nil,
redeeming_user: nil)

@invite = invite
@email = email
@username = username
@name = name
@password = password
Expand All @@ -34,6 +45,8 @@ def initialize(
@session = session
@email_token = email_token
@redeeming_user = redeeming_user

ensure_email_is_present!(email)
end

def redeem
Expand All @@ -45,7 +58,29 @@ def redeem
end
end

# extracted from User cause it is very specific to invites
# 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 #{@redeeming_user.present? ? "(ID: #{@redeeming_user.id})" : "not"} present.",
)
end

if email.blank? && @invite.is_email_invite?
@email = @invite.email
elsif @redeeming_user.present?
@email = @redeeming_user.email
else
@email = email
end

raise Discourse::InvalidParameters if @email.blank?
end

# This will _never_ be called if there is a redeeming_user being passed
# in to InviteRedeemer -- see invited_user below.
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)
available_username = username
Expand Down Expand Up @@ -107,7 +142,10 @@ def self.create_user_from_invite(email:, invite:, username: nil, name: nil, pass
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

Expand All @@ -118,24 +156,26 @@ def self.create_user_from_invite(email:, invite:, username: nil, name: nil, pass

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?
email_to_check = redeeming_user&.email || email
# 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)
raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.not_matching_email'))
end
if invite.email.present? && !invite.email_matches?(email_to_check)
raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.not_matching_email'))
end

if invite.domain.present? && !invite.domain_matches?(email_to_check)
raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed'))
end
if invite.domain.present? && !invite.domain_matches?(email_to_check)
raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed'))
end

# Anon user is trying to redeem an invitation, if an existing user already
Expand All @@ -148,6 +188,10 @@ def can_redeem_invite?
true
end

# Note that the invited_user is returned by #redeemed, so other places
# (e.g. the InvitesController) can perform further actions on it, this
# is why things like send_welcome_message are set without being saved
# on the model.
def invited_user
return @invited_user if defined?(@invited_user)

Expand Down Expand Up @@ -196,9 +240,18 @@ def mark_invite_redeemed
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

Expand All @@ -221,15 +274,17 @@ def send_welcome_message
end

def notify_invitee
if inviter = invite.invited_by
inviter.notifications.create!(
notification_type: Notification.types[:invitee_accepted],
data: { display_username: invited_user.username }.to_json
)
end
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

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")
Expand Down
@@ -0,0 +1,60 @@
# frozen_string_literal: true

class RemoveInvalidTopicAllowedUsersFromInvites < ActiveRecord::Migration[7.0]
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

0 comments on commit a414520

Please sign in to comment.