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