From 7616e9b5407015e3909e793eba97571b83c9c4ff Mon Sep 17 00:00:00 2001 From: Daniel Waterworth Date: Tue, 21 Jun 2022 13:25:10 -0500 Subject: [PATCH] SECURITY: Validate email constraints when trying to redeem an invite (#17182) 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. Co-authored-by: Alan Guo Xiang Tan --- app/controllers/invites_controller.rb | 34 ++++----- app/models/invite.rb | 5 +- app/models/invite_redeemer.rb | 58 +++++++++------ spec/requests/invites_controller_spec.rb | 93 ++++++++++++++---------- 4 files changed, 106 insertions(+), 84 deletions(-) diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 2c75d65911..bd21384ae7 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -20,39 +20,32 @@ 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 - added_to_group = false + redeemed = false - 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) - if invite_by_guardian.can_edit_group?(group) - group.add(current_user) - added_to_group = true - end - end - end + 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 - create_topic_invite_notifications(invite, current_user) - - if topic = invite.topics.first - new_guardian = Guardian.new(current_user) - return redirect_to(topic.url) if new_guardian.can_see?(topic) - elsif added_to_group - return redirect_to(path("/")) + 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 ensure_not_logged_in + return redirect_to(path('/')) end email = Email.obfuscate(invite.email) # 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 @@ -63,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 1594a109ab..87936a2353 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -167,11 +167,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 e046dd032c..1993ad01a8 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 7110cf1733..42132b2cd3 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -64,54 +64,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 'fails for logged in users' do - sign_in(Fabricate(:user)) + 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) - get "/invites/#{invite.invite_key}" - expect(response.status).to eq(200) - expect(response.body).to_not have_tag(:script, with: { src: '/assets/application.js' }) - expect(response.body).to include(I18n.t('login.already_logged_in')) + expect do + get "/invites/#{invite.invite_key}" + end.to change { InvitedUser.exists?(invite: invite, user: user) }.to(true) + + expect(response).to redirect_to(topic.url) + expect(user.reload.groups).to include(group) + end + + 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') + + 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