From 115859964d6e3e92d6d933ffe8e1b330b12a3aca Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 21 Jun 2022 11:56:50 +0800 Subject: [PATCH] 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 | 41 ++++------ app/models/invite.rb | 5 +- app/models/invite_redeemer.rb | 58 ++++++++----- spec/requests/invites_controller_spec.rb | 100 ++++++++++++----------- 4 files changed, 103 insertions(+), 101 deletions(-) diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 8b642ff24ca..221aa2a138e 100644 --- a/app/controllers/invites_controller.rb +++ b/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 - InvitedUser.transaction do - invited_user = InvitedUser.find_or_initialize_by(user: current_user, invite: invite) - if invited_user.new_record? - invited_user.save! - Invite.increment_counter(:redemption_count, invite.id) - invite.invited_by.notifications.create!( - notification_type: Notification.types[:invitee_accepted], - data: { display_username: current_user.username }.to_json - ) - end - end + redeemed = false + + begin + invite.redeem(email: current_user.email) + redeemed = true + rescue ActiveRecord::RecordNotSaved, Invite::UserExists + # This is not ideal but `Invite#redeem` raises either `Invite::UserExists` or `ActiveRecord::RecordNotSaved` + # error when it fails to redeem the invite. If redemption fails for a logged in user, we will just ignore it. end - if invite.groups.present? - invite_by_guardian = Guardian.new(invite.invited_by) - new_group_ids = invite.groups.pluck(:id) - current_user.group_users.pluck(:group_id) - 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 + if redeemed && (topic = invite.topics.first) && current_user.guardian.can_see?(topic) + create_topic_invite_notifications(invite, current_user) + return redirect_to(topic.url) 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 diff --git a/app/models/invite.rb b/app/models/invite.rb index c7236e9caa6..f3c124436c3 100644 --- a/app/models/invite.rb +++ b/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, diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index e046dd032cf..1993ad01a8e 100644 --- a/app/models/invite_redeemer.rb +++ b/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, diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index b310e7f75ea..d04d69906de 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -71,67 +71,71 @@ describe InvitesController do end end - it 'adds logged in users to invite groups' do - group = Fabricate(:group) - group.add_owner(invite.invited_by) - InvitedGroup.create!(group: group, invite: invite) + describe 'logged in user viewing an invite' do + fab!(:group) { Fabricate(:group) } - sign_in(user) + before do + sign_in(user) + end - get "/invites/#{invite.invite_key}" - expect(response).to redirect_to("/") - expect(user.reload.groups).to include(group) - end + it "redeems the invite when user's email matches invite's email before redirecting to secured topic url" do + invite.update_columns(email: user.email) + group.add_owner(invite.invited_by) - it 'redirects logged in users to invite topic if they can see it' do - topic = Fabricate(:topic) - TopicInvite.create!(topic: topic, invite: invite) + secured_category = Fabricate(:category) + secured_category.permissions = { group.name => :full } + 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(response).to redirect_to(topic.url) - expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1) - end + expect do + get "/invites/#{invite.invite_key}" + end.to change { InvitedUser.exists?(invite: invite, user: user) }.to(true) - it 'adds logged in user to group and redirects them to invite topic' do - group = Fabricate(: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) + expect(response).to redirect_to(topic.url) + expect(user.reload.groups).to include(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(user.reload.groups).to include(group) - expect(response).to redirect_to(topic.url) - expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1) - end + expect(Notification.exists?(user: invite.invited_by, notification_type: Notification.types[:invitee_accepted])) + .to eq(true) + end - it 'creates a notification to inviter' do - topic = Fabricate(:topic) - TopicInvite.create!(topic: topic, invite: invite) + it "redeems the invite when user's email domain matches the domain an invite link is restricted to" do + invite.update!(email: nil, domain: 'discourse.org') + 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(Notification.where(user: user, notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1) - expect(Notification.where(user: invite.invited_by, notification_type: Notification.types[:invitee_accepted]).count).to eq(1) - end + expect(response).to redirect_to(topic.url) + expect(user.reload.groups).to include(group) + end - it 'creates an invited user record' do - sign_in(invite.invited_by) - expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(0) - expect(response.status).to eq(302) + 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 + user.update!(email: "someguy@discourse.com") + invite.update!(email: nil, domain: 'discourse.org') - sign_in(user) - expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(1) - expect(response.status).to eq(302) + expect { get "/invites/#{invite.invite_key}" }.to change { InvitedUser.count }.by(0) + + 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