From 07ef1a80a1461123d602c57e366974aed265a91e Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 1 Nov 2022 16:33:32 +0000 Subject: [PATCH] SECURITY: Fix invite link email validation (#18817) See https://github.com/discourse/discourse/security/advisories/GHSA-x8w7-rwmr-w278 Co-authored-by: Martin Brennan --- .../discourse/app/controllers/invites-show.js | 65 +++++- .../discourse/app/templates/invites/show.hbs | 9 +- app/controllers/invites_controller.rb | 208 +++++++++--------- app/controllers/session_controller.rb | 9 +- app/models/email_token.rb | 2 +- app/models/invite.rb | 37 +++- app/models/invite_redeemer.rb | 125 +++++++---- config/locales/client.en.yml | 2 + config/locales/server.en.yml | 1 + ...log_out_invite_redemption_invited_users.rb | 32 +++ spec/models/invite_redeemer_spec.rb | 54 ++++- spec/models/invite_spec.rb | 14 +- spec/requests/invites_controller_spec.rb | 172 +++++++++++---- 13 files changed, 507 insertions(+), 223 deletions(-) create mode 100644 db/post_migrate/20221026035440_security_log_out_invite_redemption_invited_users.rb diff --git a/app/assets/javascripts/discourse/app/controllers/invites-show.js b/app/assets/javascripts/discourse/app/controllers/invites-show.js index 68937d5e7d3..c9ad1157079 100644 --- a/app/assets/javascripts/discourse/app/controllers/invites-show.js +++ b/app/assets/javascripts/discourse/app/controllers/invites-show.js @@ -1,4 +1,4 @@ -import { alias, not, or, readOnly } from "@ember/object/computed"; +import { alias, bool, not, readOnly } from "@ember/object/computed"; import Controller, { inject as controller } from "@ember/controller"; import DiscourseURL from "discourse/lib/url"; import EmberObject from "@ember/object"; @@ -29,6 +29,9 @@ export default Controller.extend( invitedBy: readOnly("model.invited_by"), email: alias("model.email"), accountEmail: alias("email"), + existingUserId: readOnly("model.existing_user_id"), + existingUserCanRedeem: readOnly("model.existing_user_can_redeem"), + existingUserRedeeming: bool("existingUserId"), hiddenEmail: alias("model.hidden_email"), emailVerifiedByLink: alias("model.email_verified_by_link"), differentExternalEmail: alias("model.different_external_email"), @@ -40,13 +43,6 @@ export default Controller.extend( authOptions: null, inviteImageUrl: getUrl("/images/envelope.svg"), isInviteLink: readOnly("model.is_invite_link"), - submitDisabled: or( - "emailValidation.failed", - "usernameValidation.failed", - "passwordValidation.failed", - "nameValidation.failed", - "userFieldsValidation.failed" - ), rejectedEmails: null, init() { @@ -81,6 +77,15 @@ export default Controller.extend( }); }, + @discourseComputed("existingUserId") + subheaderMessage(existingUserId) { + if (existingUserId) { + return I18n.t("invites.existing_user_can_redeem"); + } else { + return I18n.t("create_account.subheader_title"); + } + }, + @discourseComputed("email") yourEmailMessage(email) { return I18n.t("invites.your_email", { email }); @@ -100,6 +105,37 @@ export default Controller.extend( ); }, + @discourseComputed( + "emailValidation.failed", + "usernameValidation.failed", + "passwordValidation.failed", + "nameValidation.failed", + "userFieldsValidation.failed", + "existingUserRedeeming", + "existingUserCanRedeem" + ) + submitDisabled( + emailValidationFailed, + usernameValidationFailed, + passwordValidationFailed, + nameValidationFailed, + userFieldsValidationFailed, + existingUserRedeeming, + existingUserCanRedeem + ) { + if (existingUserRedeeming) { + return !existingUserCanRedeem; + } + + return ( + emailValidationFailed || + usernameValidationFailed || + passwordValidationFailed || + nameValidationFailed || + userFieldsValidationFailed + ); + }, + @discourseComputed( "externalAuthsEnabled", "externalAuthsOnly", @@ -118,13 +154,20 @@ export default Controller.extend( @discourseComputed( "externalAuthsOnly", "authOptions", - "emailValidation.failed" + "emailValidation.failed", + "existingUserRedeeming" ) - shouldDisplayForm(externalAuthsOnly, authOptions, emailValidationFailed) { + shouldDisplayForm( + externalAuthsOnly, + authOptions, + emailValidationFailed, + existingUserRedeeming + ) { return ( (this.siteSettings.enable_local_logins || (externalAuthsOnly && authOptions && !emailValidationFailed)) && - !this.siteSettings.enable_discourse_connect + !this.siteSettings.enable_discourse_connect && + !existingUserRedeeming ); }, diff --git a/app/assets/javascripts/discourse/app/templates/invites/show.hbs b/app/assets/javascripts/discourse/app/templates/invites/show.hbs index 51b11885ee8..c7d9122c4e8 100644 --- a/app/assets/javascripts/discourse/app/templates/invites/show.hbs +++ b/app/assets/javascripts/discourse/app/templates/invites/show.hbs @@ -4,7 +4,7 @@

{{this.welcomeTitle}}

{{#unless this.successMessage}} - + {{/unless}} @@ -131,6 +131,13 @@ {{/if}} {{/if}} + {{#if this.existingUserRedeeming}} + {{#if this.existingUserCanRedeem}} + + {{else}} +
{{i18n "invites.existing_user_cannot_redeem"}}
+ {{/if}} + {{/if}} {{/if}} diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 8f65340be03..5c5275c12e7 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -12,7 +12,6 @@ class InvitesController < ApplicationController before_action :ensure_invites_allowed, only: [:show, :perform_accept_invitation] before_action :ensure_new_registrations_allowed, only: [:show, :perform_accept_invitation] - before_action :ensure_not_logged_in, only: :perform_accept_invitation def show expires_now @@ -22,90 +21,9 @@ class InvitesController < ApplicationController invite = Invite.find_by(invite_key: params[:id]) if invite.present? && invite.redeemable? - if current_user - 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 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("/")) - 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 - email = invite.email - else - different_external_email = true - end - end - - email_verified_by_link = invite.email_token.present? && params[:t] == invite.email_token - - if email_verified_by_link - email = invite.email - end - - hidden_email = email != invite.email - - if hidden_email || invite.email.nil? - username = "" - else - username = UserNameSuggester.suggest(invite.email) - end - - info = { - invited_by: UserNameSerializer.new(invite.invited_by, scope: guardian, root: false), - email: email, - hidden_email: hidden_email, - username: username, - is_invite_link: invite.is_invite_link?, - email_verified_by_link: email_verified_by_link - } - - if different_external_email - info[:different_external_email] = true - end - - if staged_user = User.where(staged: true).with_email(invite.email).first - info[:username] = staged_user.username - info[:user_fields] = staged_user.user_fields - end - - store_preloaded("invite_info", MultiJson.dump(info)) - - secure_session["invite-key"] = invite.invite_key - - render layout: 'application' + show_invite(invite) else - flash.now[:error] = if invite.blank? - I18n.t('invite.not_found', base_url: Discourse.base_url) - elsif invite.redeemed? - if invite.is_invite_link? - I18n.t('invite.not_found_template_link', site_name: SiteSetting.title, base_url: Discourse.base_url) - else - I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url) - end - elsif invite.expired? - I18n.t('invite.expired', base_url: Discourse.base_url) - end - - render layout: 'no_ember' + show_irredeemable_invite(invite) end rescue RateLimiter::LimitExceeded => e flash.now[:error] = e.description @@ -277,24 +195,33 @@ class InvitesController < ApplicationController params.permit(:email, :username, :name, :password, :timezone, :email_token, user_custom_fields: {}) invite = Invite.find_by(invite_key: params[:id]) + redeeming_user = current_user if invite.present? begin attrs = { - username: params[:username], - name: params[:name], - password: params[:password], - user_custom_fields: params[:user_custom_fields], ip_address: request.remote_ip, session: session } - if invite.is_invite_link? - params.require(:email) - attrs[:email] = params[:email] + if redeeming_user + attrs[:redeeming_user] = redeeming_user else - attrs[:email] = invite.email - attrs[:email_token] = params[:email_token] if params[:email_token].present? + attrs[:username] = params[:username] + attrs[:name] = params[:name] + attrs[:password] = params[:password] + attrs[:user_custom_fields] = params[:user_custom_fields] + + # If the invite is not scoped to an email then we allow the + # user to provide it themselves + if invite.is_invite_link? + params.require(:email) + attrs[:email] = params[:email] + else + # Otherwise we always use the email from the invitation. + attrs[:email] = invite.email + attrs[:email_token] = params[:email_token] if params[:email_token].present? + end end user = invite.redeem(**attrs) @@ -306,7 +233,10 @@ class InvitesController < ApplicationController return render json: failed_json.merge(message: I18n.t('invite.not_found_json')), status: 404 end - log_on_user(user) if user.active? && user.guardian.can_access_forum? + if !redeeming_user && user.active? && user.guardian.can_access_forum? + log_on_user(user) + end + user.update_timezone_if_missing(params[:timezone]) post_process_invite(user) create_topic_invite_notifications(invite, user) @@ -316,6 +246,10 @@ class InvitesController < ApplicationController if user.present? if user.active? && user.guardian.can_access_forum? + if redeeming_user + response[:message] = I18n.t("invite.existing_user_success") + end + if user.guardian.can_see?(topic) response[:redirect_to] = path(topic.relative_url) else @@ -424,6 +358,84 @@ class InvitesController < ApplicationController private + def show_invite(invite) + 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 + email = invite.email + else + different_external_email = true + end + end + + email_verified_by_link = invite.email_token.present? && params[:t] == invite.email_token + + if email_verified_by_link + email = invite.email + end + + hidden_email = email != invite.email + + if hidden_email || invite.email.nil? + username = "" + else + username = UserNameSuggester.suggest(invite.email) + end + + info = { + invited_by: UserNameSerializer.new(invite.invited_by, scope: guardian, root: false), + email: email, + hidden_email: hidden_email, + username: username, + is_invite_link: invite.is_invite_link?, + email_verified_by_link: email_verified_by_link + } + + if different_external_email + info[:different_external_email] = true + end + + if staged_user = User.where(staged: true).with_email(invite.email).first + info[:username] = staged_user.username + info[:user_fields] = staged_user.user_fields + end + + if current_user + info[:existing_user_id] = current_user.id + info[:existing_user_can_redeem] = invite.can_be_redeemed_by?(current_user) + info[:email] = current_user.email + info[:username] = current_user.username + end + + store_preloaded("invite_info", MultiJson.dump(info)) + + secure_session["invite-key"] = invite.invite_key + + render layout: 'application' + end + + def show_irredeemable_invite(invite) + flash.now[:error] = \ + if invite.blank? + I18n.t('invite.not_found', base_url: Discourse.base_url) + elsif invite.redeemed? + if invite.is_invite_link? + I18n.t('invite.not_found_template_link', site_name: SiteSetting.title, base_url: Discourse.base_url) + else + I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url) + end + elsif invite.expired? + I18n.t('invite.expired', base_url: Discourse.base_url) + end + + render layout: 'no_ember' + end + def ensure_invites_allowed if (!SiteSetting.enable_local_logins && Discourse.enabled_auth_providers.count == 0 && !SiteSetting.enable_discourse_connect) raise Discourse::NotFound @@ -438,14 +450,6 @@ class InvitesController < ApplicationController end end - def ensure_not_logged_in - if current_user - flash[:error] = I18n.t("login.already_logged_in") - render layout: 'no_ember' - false - end - end - def groups_can_see_topic?(groups, topic) if topic&.read_restricted_category? topic_groups = topic.category.groups diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 68d8a367a82..d05ba4351f6 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -159,7 +159,7 @@ class SessionController < ApplicationController if SiteSetting.must_approve_users? && !user.approved? if invite.present? && user.invited_user.blank? - redeem_invitation(invite, sso) + redeem_invitation(invite, sso, user) end if SiteSetting.discourse_connect_not_approved_url.present? @@ -173,7 +173,7 @@ class SessionController < ApplicationController # the user has not already redeemed an invite # (covers the same SSO user visiting an invite link) elsif invite.present? && user.invited_user.blank? - redeem_invitation(invite, sso) + redeem_invitation(invite, sso, user) # we directly call user.activate here instead of going # through the UserActivator path because we assume the account @@ -772,14 +772,15 @@ class SessionController < ApplicationController invite end - def redeem_invitation(invite, sso) + def redeem_invitation(invite, sso, redeeming_user) InviteRedeemer.new( invite: invite, username: sso.username, name: sso.name, ip_address: request.remote_ip, session: session, - email: sso.email + email: sso.email, + redeeming_user: redeeming_user ).redeem secure_session["invite-key"] = nil diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 0907aaccbed..8cd7dbdd03a 100644 --- a/app/models/email_token.rb +++ b/app/models/email_token.rb @@ -69,7 +69,7 @@ class EmailToken < ActiveRecord::Base user.create_reviewable if !skip_reviewable user.set_automatic_groups DiscourseEvent.trigger(:user_confirmed_email, user) - Invite.redeem_from_email(user.email) if scope == EmailToken.scopes[:signup] + Invite.redeem_for_existing_user(user) if scope == EmailToken.scopes[:signup] user.reload end diff --git a/app/models/invite.rb b/app/models/invite.rb index 4b0c92620eb..917b486f294 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -90,6 +90,22 @@ class Invite < ActiveRecord::Base end end + def email_matches?(email) + email.downcase == self.email.downcase + end + + def domain_matches?(email) + _, domain = email.split('@') + self.domain == domain + end + + def can_be_redeemed_by?(user) + return false if !self.redeemable? + return true if self.email.blank? && self.domain.blank? + return true if self.email.present? && email_matches?(user.email) + self.domain.present? && domain_matches?(user.email) + end + def expired? expires_at < Time.zone.now end @@ -172,7 +188,17 @@ class Invite < ActiveRecord::Base invite.reload end - def redeem(email: nil, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil, email_token: nil) + def redeem( + email: nil, + username: nil, + name: nil, + password: nil, + user_custom_fields: nil, + ip_address: nil, + session: nil, + email_token: nil, + redeeming_user: nil + ) return if !redeemable? email = self.email if email.blank? && !is_invite_link? @@ -186,14 +212,15 @@ class Invite < ActiveRecord::Base user_custom_fields: user_custom_fields, ip_address: ip_address, session: session, - email_token: email_token + email_token: email_token, + redeeming_user: redeeming_user ).redeem end - def self.redeem_from_email(email) - invite = Invite.find_by(email: Email.downcase(email)) + def self.redeem_for_existing_user(user) + invite = Invite.find_by(email: Email.downcase(user.email)) if invite.present? && invite.redeemable? - InviteRedeemer.new(invite: invite, email: invite.email).redeem + InviteRedeemer.new(invite: invite, redeeming_user: user).redeem end invite end diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index ed0a115f4b3..ba823aaf512 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -1,6 +1,41 @@ # frozen_string_literal: true -InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_custom_fields, :ip_address, :session, :email_token, keyword_init: true) do +class InviteRedeemer + attr_reader :invite, + :email, + :username, + :name, + :password, + :user_custom_fields, + :ip_address, + :session, + :email_token, + :redeeming_user + + def initialize( + invite: nil, + email: nil, + username: nil, + name: nil, + password: nil, + user_custom_fields: nil, + ip_address: nil, + session: nil, + email_token: nil, + redeeming_user: nil) + + @invite = invite + @email = email + @username = username + @name = name + @password = password + @user_custom_fields = user_custom_fields + @ip_address = ip_address + @session = session + @email_token = email_token + @redeeming_user = redeeming_user + end + def redeem Invite.transaction do if can_redeem_invite? && mark_invite_redeemed @@ -82,42 +117,64 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ private def can_redeem_invite? - return false unless invite.redeemable? + return false if !invite.redeemable? - # Invite has already been redeemed + # Invite has already been redeemed by anyone. if !invite.is_invite_link? && InvitedUser.exists?(invite_id: invite.id) return false end - validate_invite_email! + # 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 - existing_user = get_existing_user + if invite.email.present? && !invite.email_matches?(email_to_check) + raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.not_matching_email')) + end - if existing_user.present? && InvitedUser.exists?(user_id: existing_user.id, invite_id: invite.id) + if invite.domain.present? && !invite.domain_matches?(email_to_check) + raise ActiveRecord::RecordNotSaved.new(I18n.t('invite.domain_not_allowed')) + end + end + + # Anon user is trying to redeem an invitation, if an existing user already + # redeemed it then we cannot redeem now. + redeeming_user ||= User.where(admin: false, staged: false).find_by_email(email) + if redeeming_user.present? && InvitedUser.exists?(user_id: redeeming_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 + return @invited_user if defined?(@invited_user) + + # The redeeming user is an already logged in user or a user who is + # activating their account who is redeeming the invite, + # which is valid for existing users to be invited to topics or groups. + if redeeming_user.present? + @invited_user = redeeming_user + return @invited_user + end + + # If there was no logged in user then we must attempt to create + # one based on the provided params. + invited_user ||= InviteRedeemer.create_user_from_invite( + email: email, + invite: invite, + username: username, + name: name, + password: password, + user_custom_fields: user_custom_fields, + ip_address: ip_address, + session: session, + email_token: email_token + ) + invited_user.send_welcome_message = false + @invited_user = invited_user + @invited_user end def process_invitation @@ -138,28 +195,6 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ @invited_user_record.present? end - def get_invited_user - result = get_existing_user - - result ||= InviteRedeemer.create_user_from_invite( - email: email, - invite: invite, - username: username, - name: name, - password: password, - user_custom_fields: user_custom_fields, - ip_address: ip_address, - session: session, - email_token: email_token - ) - result.send_welcome_message = false - result - end - - def get_existing_user - User.where(admin: false, staged: false).find_by_email(email) - end - def add_to_private_topics_if_invited topic_ids = Topic.where(archetype: Archetype::private_message).includes(:invites).where(invites: { email: email }).pluck(:id) topic_ids.each do |id| diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 2653d79eb3c..fe77f335f8b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2089,6 +2089,8 @@ en: success: "Your account has been created and you're now logged in." name_label: "Name" password_label: "Password" + existing_user_can_redeem: "Redeem your invitation to a topic or group." + existing_user_cannot_redeem: "This invitation cannot be redeemed. Please ask the person who invited you to send you a new invitation." password_reset: continue: "Continue to %{site_name}" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 6e55cf5ed9a..e853452a019 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -266,6 +266,7 @@ en: max_redemptions_allowed_one: "for email invites should be 1." redemption_count_less_than_max: "should be less than %{max_redemptions_allowed}." email_xor_domain: "Email and domain fields are not allowed at the same time" + existing_user_success: "Invite redeemed successfully" bulk_invite: file_should_be_csv: "The uploaded file should be of csv format." diff --git a/db/post_migrate/20221026035440_security_log_out_invite_redemption_invited_users.rb b/db/post_migrate/20221026035440_security_log_out_invite_redemption_invited_users.rb new file mode 100644 index 00000000000..e87a27e3b0c --- /dev/null +++ b/db/post_migrate/20221026035440_security_log_out_invite_redemption_invited_users.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class SecurityLogOutInviteRedemptionInvitedUsers < ActiveRecord::Migration[7.0] + def up + # 20220606061813 was added shortly before the vulnerability was introduced + vulnerable_since = DB.query_single("SELECT created_at FROM schema_migration_details WHERE version='20220606061813'")[0] + + DB.exec(<<~SQL, vulnerable_since: vulnerable_since) + DELETE FROM user_auth_tokens + WHERE user_id IN ( + SELECT DISTINCT user_id + FROM invited_users + JOIN users ON invited_users.user_id = users.id + WHERE invited_users.redeemed_at > :vulnerable_since + ) + SQL + + DB.exec(<<~SQL, vulnerable_since: vulnerable_since) + DELETE FROM user_api_keys + WHERE user_id IN ( + SELECT DISTINCT user_id + FROM invited_users + JOIN users ON invited_users.user_id = users.id + WHERE invited_users.redeemed_at > :vulnerable_since + ) + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb index fc4c82244fc..e944f459033 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/spec/models/invite_redeemer_spec.rb @@ -242,6 +242,40 @@ RSpec.describe InviteRedeemer do expect(invite.invited_users.first).to be_present end + it "raises an error if the email does not match the invite email" do + redeemer = InviteRedeemer.new(invite: invite, email: "blah@test.com", username: username, name: name) + expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email")) + end + + context "when a redeeming user is passed in" do + fab!(:redeeming_user) { Fabricate(:user, email: "foobar@example.com") } + + it "raises an error if the email does not match the invite email" do + redeeming_user.update!(email: "foo@bar.com") + redeemer = InviteRedeemer.new(invite: invite, redeeming_user: redeeming_user) + expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.not_matching_email")) + end + end + + context 'with domain' do + fab!(:invite) { Fabricate(:invite, email: nil, domain: "test.com") } + + it "raises an error if the email domain does not match the invite domain" do + redeemer = InviteRedeemer.new(invite: invite, email: "blah@somesite.com", username: username, name: name) + expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.domain_not_allowed")) + end + + context "when a redeeming user is passed in" do + fab!(:redeeming_user) { Fabricate(:user, email: "foo@test.com") } + + it "raises an error if the user's email domain does not match the invite domain" do + redeeming_user.update!(email: "foo@bar.com") + redeemer = InviteRedeemer.new(invite: invite, redeeming_user: redeeming_user) + expect { redeemer.redeem }.to raise_error(ActiveRecord::RecordNotSaved, I18n.t("invite.domain_not_allowed")) + end + end + end + context 'with invite_link' do fab!(:invite_link) { Fabricate(:invite, email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required]) } let(:invite_redeemer) { InviteRedeemer.new(invite: invite_link, email: 'foo@example.com') } @@ -257,7 +291,7 @@ RSpec.describe InviteRedeemer do end it "should not redeem the invite if InvitedUser record already exists for email" do - user = invite_redeemer.redeem + invite_redeemer.redeem invite_link.reload another_invite_redeemer = InviteRedeemer.new(invite: invite_link, email: 'foo@example.com') @@ -266,14 +300,28 @@ RSpec.describe InviteRedeemer do end it "should redeem the invite if InvitedUser record does not exists for email" do - user = invite_redeemer.redeem + invite_redeemer.redeem invite_link.reload another_invite_redeemer = InviteRedeemer.new(invite: invite_link, email: 'bar@example.com') another_user = another_invite_redeemer.redeem expect(another_user.is_a?(User)).to eq(true) end - end + it "raises an error if the email is already being used by an existing user" do + Fabricate(:user, email: 'foo@example.com') + expect { invite_redeemer.redeem }.to raise_error(ActiveRecord::RecordInvalid, /Primary email has already been taken/) + end + + context "when a redeeming user is passed in" do + fab!(:redeeming_user) { Fabricate(:user, email: 'foo@example.com') } + + it "does not create a new user" do + expect do + InviteRedeemer.new(invite: invite_link, redeeming_user: redeeming_user).redeem + end.not_to change { User.count } + end + end + end end end diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index 2691659db35..0a1f9de9f35 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -338,38 +338,38 @@ RSpec.describe Invite do end end - describe '#redeem_from_email' do + describe '#redeem_for_existing_user' do fab!(:invite) { Fabricate(:invite, email: 'test@example.com') } fab!(:user) { Fabricate(:user, email: invite.email) } it 'redeems the invite from email' do - Invite.redeem_from_email(user.email) + Invite.redeem_for_existing_user(user) expect(invite.reload).to be_redeemed end it 'does not redeem the invite if email does not match' do - Invite.redeem_from_email('test2@example.com') + user.update!(email: 'test2@example.com') + Invite.redeem_for_existing_user(user) expect(invite.reload).not_to be_redeemed end it 'does not work with expired invites' do invite.update!(expires_at: 1.day.ago) - Invite.redeem_from_email(user.email) + Invite.redeem_for_existing_user(user) expect(invite).not_to be_redeemed end it 'does not work with deleted invites' do invite.trash! - Invite.redeem_from_email(user.email) + Invite.redeem_for_existing_user(user) expect(invite).not_to be_redeemed end it 'does not work with invalidated invites' do invite.update!(invalidated_at: 1.day.ago) - Invite.redeem_from_email(user.email) + Invite.redeem_for_existing_user(user) expect(invite).not_to be_redeemed end - end describe 'scopes' do diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index 01332a0b99b..e450296d870 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -78,63 +78,68 @@ RSpec.describe InvitesController do sign_in(user) end - it "redeems the invite when user's email matches invite's email before redirecting to secured topic url" do + it "shows the accept invite page when user's email matches the invite email" do invite.update_columns(email: user.email) - group.add_owner(invite.invited_by) - secured_category = Fabricate(:category) - secured_category.permissions = { group.name => :full } - secured_category.save! + get "/invites/#{invite.invite_key}" + expect(response.status).to eq(200) + expect(response.body).to have_tag(:script, with: { src: "/assets/discourse.js" }) + expect(response.body).not_to include(I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url)) - topic = Fabricate(:topic, category: secured_category) - TopicInvite.create!(invite: invite, topic: topic) - InvitedGroup.create!(invite: invite, group: group) - - 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) - - expect(Notification.exists?(user: user, notification_type: Notification.types[:invited_to_topic], topic: topic)) - .to eq(true) - - expect(Notification.exists?(user: invite.invited_by, notification_type: Notification.types[:invitee_accepted])) - .to eq(true) + expect(response.body).to have_tag('div#data-preloaded') do |element| + json = JSON.parse(element.current_scope.attribute('data-preloaded').value) + invite_info = JSON.parse(json['invite_info']) + expect(invite_info['username']).to eq(user.username) + expect(invite_info['email']).to eq(user.email) + expect(invite_info['existing_user_id']).to eq(user.id) + expect(invite_info['existing_user_can_redeem']).to eq(true) + end end - it "redeems the invite when user's email domain matches the domain an invite link is restricted to" do + it "shows the accept invite page 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) - 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.status).to eq(200) + expect(response.body).to have_tag(:script, with: { src: "/assets/discourse.js" }) + expect(response.body).not_to include(I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url)) - expect(response).to redirect_to(topic.url) - expect(user.reload.groups).to include(group) + expect(response.body).to have_tag('div#data-preloaded') do |element| + json = JSON.parse(element.current_scope.attribute('data-preloaded').value) + invite_info = JSON.parse(json['invite_info']) + expect(invite_info['username']).to eq(user.username) + expect(invite_info['email']).to eq(user.email) + expect(invite_info['existing_user_id']).to eq(user.id) + expect(invite_info['existing_user_can_redeem']).to eq(true) + end 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 + it "does not allow the user to accept the invite when their email domain does not match the domain of the invite" do user.update!(email: "someguy@discourse.com") invite.update!(email: nil, domain: 'discourse.org') - expect { get "/invites/#{invite.invite_key}" }.not_to change { InvitedUser.count } + get "/invites/#{invite.invite_key}" + expect(response.status).to eq(200) - expect(response).to redirect_to("/") + expect(response.body).to have_tag('div#data-preloaded') do |element| + json = JSON.parse(element.current_scope.attribute('data-preloaded').value) + invite_info = JSON.parse(json['invite_info']) + expect(invite_info['existing_user_can_redeem']).to eq(false) + end end - it "redirects to root if a tries to view an invite meant for a specific email that is not the user's" do + it "does not allow the user to accept the invite when their email does not match the invite" do invite.update_columns(email: "notuseremail@discourse.org") - expect { get "/invites/#{invite.invite_key}" }.not_to change { InvitedUser.count } + get "/invites/#{invite.invite_key}" + expect(response.status).to eq(200) - expect(response).to redirect_to("/") + expect(response.body).to have_tag('div#data-preloaded') do |element| + json = JSON.parse(element.current_scope.attribute('data-preloaded').value) + invite_info = JSON.parse(json['invite_info']) + expect(invite_info['existing_user_can_redeem']).to eq(false) + end end end @@ -848,8 +853,9 @@ RSpec.describe InvitesController do fab!(:invite) { Fabricate(:invite, email: nil, emailed_status: Invite.emailed_status_types[:not_required]) } it 'sends an activation email and does not activate the user' do - expect { put "/invites/show/#{invite.invite_key}.json", params: { email: 'test@example.com', password: 'verystrongpassword' } } - .not_to change { UserAuthToken.count } + expect { + put "/invites/show/#{invite.invite_key}.json", params: { email: 'test@example.com', password: 'verystrongpassword' } + }.not_to change { UserAuthToken.count } expect(response.status).to eq(200) expect(response.parsed_body['message']).to eq(I18n.t('invite.confirm_email')) @@ -870,6 +876,24 @@ RSpec.describe InvitesController do expect(job_args['user_id']).to eq(invited_user.id) expect(EmailToken.hash_token(job_args['email_token'])).to eq(tokens.first.token_hash) end + + it "does not automatically log in the user if their email matches an existing user's and shows an error" do + Fabricate(:user, email: 'test@example.com') + put "/invites/show/#{invite.invite_key}.json", params: { email: 'test@example.com', password: 'verystrongpassword' } + expect(session[:current_user_id]).to be_blank + expect(response.status).to eq(412) + expect(response.parsed_body['message']).to include("Primary email has already been taken") + expect(invite.reload.redemption_count).to eq(0) + end + + it "does not automatically log in the user if their email matches an existing admin's and shows an error" do + Fabricate(:admin, email: 'test@example.com') + put "/invites/show/#{invite.invite_key}.json", params: { email: 'test@example.com', password: 'verystrongpassword' } + expect(session[:current_user_id]).to be_blank + expect(response.status).to eq(412) + expect(response.parsed_body['message']).to include("Primary email has already been taken") + expect(invite.reload.redemption_count).to eq(0) + end end context 'when new registrations are disabled' do @@ -889,15 +913,75 @@ RSpec.describe InvitesController do context 'when user is already logged in' do fab!(:invite) { Fabricate(:invite, email: 'test@example.com') } - fab!(:user) { sign_in(Fabricate(:user)) } + fab!(:user) { Fabricate(:user, email: 'test@example.com') } + fab!(:group) { Fabricate(:group) } - it 'does not redeem the invite' do + before { sign_in(user) } + + it 'redeems the invitation and creates the invite accepted notification' do put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } expect(response.status).to eq(200) + expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success")) invite.reload - expect(invite.invited_users).to be_blank - expect(invite.redeemed?).to be_falsey - expect(response.body).to include(I18n.t('login.already_logged_in', current_user: user.username)) + expect(invite.invited_users.first.user).to eq(user) + expect(invite.redeemed?).to be_truthy + expect( + Notification.exists?( + user: invite.invited_by, notification_type: Notification.types[:invitee_accepted] + ) + ).to eq(true) + end + + it 'redirects to the first topic the user was invited to and creates the topic notification' do + topic = Fabricate(:topic) + TopicInvite.create!(invite: invite, topic: topic) + put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } + expect(response.status).to eq(200) + expect(response.parsed_body['redirect_to']).to eq(topic.relative_url) + expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1) + end + + it "adds the user to the groups specified on the invite and allows them to access the secure topic" do + 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) + + put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } + expect(response.status).to eq(200) + expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success")) + expect(response.parsed_body['redirect_to']).to eq(topic.relative_url) + invite.reload + expect(invite.redeemed?).to be_truthy + expect(user.reload.groups).to include(group) + expect(Notification.where(notification_type: Notification.types[:invited_to_topic], topic: topic).count).to eq(1) + end + + it "does not try to log in the user automatically" do + expect do + put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } + end.not_to change { UserAuthToken.count } + expect(response.status).to eq(200) + expect(response.parsed_body["message"]).to eq(I18n.t("invite.existing_user_success")) + end + + it "errors if the user's email doesn't match the invite email" do + user.update!(email: "blah@test.com") + put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } + expect(response.status).to eq(412) + expect(response.parsed_body["message"]).to eq(I18n.t("invite.not_matching_email")) + end + + it "errors if the user's email domain doesn't match the invite domain" do + user.update!(email: "blah@test.com") + invite.update!(email: nil, domain: "example.com") + put "/invites/show/#{invite.invite_key}.json", params: { id: invite.invite_key } + expect(response.status).to eq(412) + expect(response.parsed_body["message"]).to eq(I18n.t("invite.domain_not_allowed")) end end