From 355d51afde190b884dbb9b81ebc836a16b48d98c Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 19 Mar 2021 10:20:10 +1000 Subject: [PATCH] FEATURE: Allow using invites when DiscourseConnect SSO is enabled (#12419) This PR allows invitations to be used when the DiscourseConnect SSO is enabled for a site (`enable_discourse_connect`) and local logins are disabled. Previously invites could not be accepted with SSO enabled simply because we did not have the code paths to handle that logic. The invitation methods that are supported include: * Inviting people to groups via email address * Inviting people to topics via email address * Using invitation links generated by the Invite Users UI in the /my/invited/pending route The flow works like this: 1. User visits an invite URL 2. The normal invitation validations (redemptions/expiry) happen at that point 3. We store the invite key in a secure session 4. The user clicks "Accept Invitation and Continue" (see below) 5. The user is redirected to /session/sso then to the SSO provider URL then back to /session/sso_login 6. We retrieve the invite based on the invite key in secure session. We revalidate the invitation. We show an error to the user if it is not valid. An additional check here for invites with an email specified is to check the SSO email matches the invite email 7. If the invite is OK we create the user via the normal SSO methods 8. We redeem the invite and activate the user. We clear the invite key in secure session. 9. If the invite had a topic we redirect the user there, otherwise we redirect to / Note that we decided for SSO-based invites the `must_approve_users` site setting is ignored, because the invite is a form of pre-approval, and because regular non-staff users cannot send out email invites or generally invite to the forum in this case. Also deletes some group invite checks as per https://github.com/discourse/discourse/pull/12353 --- .../discourse/app/controllers/invites-show.js | 17 ++- .../discourse/app/templates/invites/show.hbs | 13 +- .../tests/acceptance/invite-accept-test.js | 47 +++++++ app/controllers/groups_controller.rb | 8 -- app/controllers/invites_controller.rb | 20 +-- app/controllers/session_controller.rb | 92 ++++++++++++-- app/models/invite.rb | 44 +++---- app/services/user_activator.rb | 2 +- config/locales/server.en.yml | 2 + lib/guardian.rb | 26 ++-- lib/single_sign_on.rb | 11 +- spec/components/guardian_spec.rb | 16 +-- spec/models/invite_spec.rb | 11 -- spec/requests/groups_controller_spec.rb | 17 --- spec/requests/invites_controller_spec.rb | 17 ++- spec/requests/session_controller_spec.rb | 115 ++++++++++++++++-- spec/requests/users_controller_spec.rb | 19 --- spec/support/integration_helpers.rb | 6 + 18 files changed, 339 insertions(+), 144 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/invites-show.js b/app/assets/javascripts/discourse/app/controllers/invites-show.js index 875820daf77..0e640be3949 100644 --- a/app/assets/javascripts/discourse/app/controllers/invites-show.js +++ b/app/assets/javascripts/discourse/app/controllers/invites-show.js @@ -63,6 +63,11 @@ export default Controller.extend( this.setProperties(props); }, + @discourseComputed + discourseConnectEnabled() { + return this.siteSettings.enable_discourse_connect; + }, + @discourseComputed welcomeTitle() { return I18n.t("invites.welcome_to", { @@ -83,10 +88,17 @@ export default Controller.extend( @discourseComputed externalAuthsOnly() { return ( - !this.siteSettings.enable_local_logins && this.externalAuthsEnabled + !this.siteSettings.enable_local_logins && + this.externalAuthsEnabled && + !this.siteSettings.enable_discourse_connect ); }, + @discourseComputed("externalAuthsOnly", "discourseConnectEnabled") + showSocialLoginAvailable(externalAuthsOnly, discourseConnectEnabled) { + return !externalAuthsOnly && !discourseConnectEnabled; + }, + @discourseComputed( "externalAuthsOnly", "authOptions", @@ -170,6 +182,9 @@ export default Controller.extend( @discourseComputed wavingHandURL: () => wavingHandURL(), + @discourseComputed + ssoPath: () => getUrl("/session/sso"), + actions: { submit() { const userFields = this.userFields; diff --git a/app/assets/javascripts/discourse/app/templates/invites/show.hbs b/app/assets/javascripts/discourse/app/templates/invites/show.hbs index d4a0f5afc9e..9a45776efa2 100644 --- a/app/assets/javascripts/discourse/app/templates/invites/show.hbs +++ b/app/assets/javascripts/discourse/app/templates/invites/show.hbs @@ -21,15 +21,16 @@

{{user-info user=invitedBy}}

{{#unless isInviteLink}} -

+

{{html-safe yourEmailMessage}} - {{#unless externalAuthsOnly}} + {{#if showSocialLoginAvailable}} {{i18n "invites.social_login_available"}} - {{/unless}} + {{/if}}

{{/unless}} {{#if externalAuthsOnly}} + {{! authOptions are present once the user has followed the OmniAuth flow (e.g. twitter/google/etc) }} {{#if authOptions}} {{#unless isInviteLink}} {{input-tip validation=emailValidation id="account-email-validation"}} @@ -39,6 +40,12 @@ {{/if}} {{/if}} + {{#if discourseConnectEnabled}} + + {{i18n "continue"}} + + {{/if}} + {{#if shouldDisplayForm}}
{{#if isInviteLink}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js b/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js index 1265053c1cb..472e9cb50c5 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/invite-accept-test.js @@ -170,6 +170,53 @@ acceptance("Invite accept when local login is disabled", function (needs) { }); }); +acceptance( + "Invite accept when DiscourseConnect SSO is enabled and local login is disabled", + function (needs) { + needs.settings({ + enable_local_logins: false, + enable_discourse_connect: true, + }); + + test("invite link", async function (assert) { + preloadInvite({ link: true }); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok( + !exists(".btn-social.facebook"), + "does not show Facebook login button" + ); + assert.ok(!exists("form"), "does not display the form"); + assert.ok( + !exists(".email-message"), + "does not show the email message with the prefilled email" + ); + assert.ok(exists(".discourse-connect"), "shows the Continue button"); + }); + + test("email invite link", async function (assert) { + preloadInvite(); + + await visit("/invites/myvalidinvitetoken"); + + assert.ok( + !exists(".btn-social.facebook"), + "does not show Facebook login button" + ); + assert.ok(!exists("form"), "does not display the form"); + assert.ok( + exists(".email-message"), + "shows the email message with the prefilled email" + ); + assert.ok(exists(".discourse-connect"), "shows the Continue button"); + assert.ok( + queryAll(".email-message").text().includes("foobar@example.com") + ); + }); + } +); + acceptance("Invite link with authentication data", function (needs) { needs.settings({ enable_local_logins: false }); diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 8d2becaaae2..0d39f9a8f9b 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -336,14 +336,6 @@ class GroupsController < ApplicationController raise Discourse::InvalidParameters.new(I18n.t("groups.errors.usernames_or_emails_required")) end - if emails.any? - if SiteSetting.enable_discourse_connect? - raise Discourse::InvalidParameters.new(I18n.t("groups.errors.no_invites_with_discourse_connect")) - elsif !SiteSetting.enable_local_logins? - raise Discourse::InvalidParameters.new(I18n.t("groups.errors.no_invites_without_local_logins")) - end - end - if users.length > ADD_MEMBERS_LIMIT return render_json_error( I18n.t("groups.errors.adding_too_many_users", count: ADD_MEMBERS_LIMIT) diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 8843efee479..98210b775fe 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -16,7 +16,7 @@ class InvitesController < ApplicationController expires_now invite = Invite.find_by(invite_key: params[:id]) - if invite.present? && !invite.expired? && !invite.redeemed? + if invite.present? && invite.redeemable? email = Email.obfuscate(invite.email) # Show email if the user already authenticated their email @@ -34,14 +34,16 @@ class InvitesController < ApplicationController is_invite_link: invite.is_invite_link? )) + secure_session["invite-key"] = invite.invite_key + render layout: 'application' else - flash.now[:error] = if invite&.expired? - I18n.t('invite.expired', base_url: Discourse.base_url) - elsif invite&.redeemed? - I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url) - else + flash.now[:error] = if invite.blank? I18n.t('invite.not_found', base_url: Discourse.base_url) + elsif invite.redeemed? + I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url) + elsif invite.expired? + I18n.t('invite.expired', base_url: Discourse.base_url) end render layout: 'no_ember' @@ -165,6 +167,8 @@ class InvitesController < ApplicationController render json: success_json end + # For DiscourseConnect SSO, all invite acceptance is done + # via the SessionController#sso_login route def perform_accept_invitation params.require(:id) params.permit(:email, :username, :name, :password, :timezone, user_custom_fields: {}) @@ -190,7 +194,7 @@ class InvitesController < ApplicationController invite.email end - user = invite.redeem(attrs) + user = invite.redeem(**attrs) rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e return render json: failed_json.merge(errors: e.record&.errors&.to_hash, message: I18n.t('invite.error_message')), status: 412 rescue Invite::UserExists => e @@ -296,7 +300,7 @@ class InvitesController < ApplicationController private def ensure_invites_allowed - if SiteSetting.enable_discourse_connect || (!SiteSetting.enable_local_logins && Discourse.enabled_auth_providers.count == 0) + if (!SiteSetting.enable_local_logins && Discourse.enabled_auth_providers.count == 0 && !SiteSetting.enable_discourse_connect) raise Discourse::NotFound end end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 1b5cf7c2761..ffd938165a5 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -172,6 +172,8 @@ class SessionController < ApplicationController sso.expire_nonce! begin + invite = validate_invitiation!(sso) + if user = sso.lookup_or_create_user(request.remote_ip) if user.suspended? @@ -179,25 +181,38 @@ class SessionController < ApplicationController return end - if SiteSetting.must_approve_users? && !user.approved? + # users logging in via SSO using an invite do not need to be approved, + # they are already pre-approved because they have been invited + if SiteSetting.must_approve_users? && !user.approved? && invite.blank? if SiteSetting.discourse_connect_not_approved_url.present? redirect_to SiteSetting.discourse_connect_not_approved_url else render_sso_error(text: I18n.t("discourse_connect.account_not_approved"), status: 403) end return + + # we only want to redeem the invite if + # 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) + + # we directly call user.activate here instead of going + # through the UserActivator path because we assume the account + # is valid from the SSO provider's POV and do not need to + # send an activation email to the user + user.activate + login_sso_user(sso, user) + + topic = invite.topics.first + return_path = topic.present? ? path(topic.relative_url) : path("/") elsif !user.active? activation = UserActivator.new(user, request, session, cookies) activation.finish session["user_created_message"] = activation.message - redirect_to(users_account_created_path) && (return) + return redirect_to(users_account_created_path) else - if SiteSetting.verbose_discourse_connect_logging - Rails.logger.warn("Verbose SSO log: User was logged on #{user.username}\n\n#{sso.diagnostics}") - end - if user.id != current_user&.id - log_on_user user - end + login_sso_user(sso, user) end # If it's not a relative URL check the host @@ -252,11 +267,14 @@ class SessionController < ApplicationController end render_sso_error(text: text || I18n.t("discourse_connect.unknown_error"), status: 500) - rescue DiscourseSingleSignOn::BlankExternalId - render_sso_error(text: I18n.t("discourse_connect.blank_id_error"), status: 500) - + rescue Invite::ValidationFailed => e + render_sso_error(text: e.message, status: 400) + rescue Invite::RedemptionFailed => e + render_sso_error(text: I18n.t("discourse_connect.invite_redeem_failed"), status: 412) + rescue Invite::UserExists => e + render_sso_error(text: e.message, status: 412) rescue => e message = +"Failed to create or lookup user: #{e}." message << " " @@ -270,6 +288,13 @@ class SessionController < ApplicationController end end + def login_sso_user(sso, user) + if SiteSetting.verbose_discourse_connect_logging + Rails.logger.warn("Verbose SSO log: User was logged on #{user.username}\n\n#{sso.diagnostics}") + end + log_on_user(user) if user.id != current_user&.id + end + def create params.require(:login) params.require(:password) @@ -612,4 +637,49 @@ class SessionController < ApplicationController def sso_url(sso) sso.to_url end + + # the invite_key will be present if set in InvitesController + # when the user visits an /invites/xxxx link; however we do + # not want to complete the SSO process of creating a user + # and redeeming the invite if the invite is not redeemable or + # for the wrong user + def validate_invitiation!(sso) + invite_key = secure_session["invite-key"] + return if invite_key.blank? + + invite = Invite.find_by(invite_key: invite_key) + + if invite.blank? + raise Invite::ValidationFailed.new(I18n.t("invite.not_found", base_url: Discourse.base_url)) + end + + if invite.redeemable? + if !invite.is_invite_link? && sso.email != invite.email + raise Invite::ValidationFailed.new(I18n.t("invite.not_matching_email")) + end + elsif invite.expired? + raise Invite::ValidationFailed.new(I18n.t('invite.expired', base_url: Discourse.base_url)) + elsif invite.redeemed? + raise Invite::ValidationFailed.new(I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url)) + end + + invite + end + + def redeem_invitation(invite, sso) + InviteRedeemer.new( + invite: invite, + username: sso.username, + name: sso.name, + ip_address: request.remote_ip, + session: session, + email: sso.email + ).redeem + secure_session["invite-key"] = nil + + # note - more specific errors are handled in the sso_login method + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e + Rails.logger.warn("SSO invite redemption failed: #{e}") + raise Invite::RedemptionFailed + end end diff --git a/app/models/invite.rb b/app/models/invite.rb index f376db0e653..da46b38dcd8 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -2,6 +2,8 @@ class Invite < ActiveRecord::Base class UserExists < StandardError; end + class RedemptionFailed < StandardError; end + class ValidationFailed < StandardError; end include RateLimiter::OnCreateRecord include Trashable @@ -31,7 +33,6 @@ class Invite < ActiveRecord::Base validates :email, email: true, allow_blank: true validate :ensure_max_redemptions_allowed validate :user_doesnt_already_exist - validate :ensure_no_invalid_email_invites before_create do self.invite_key ||= SecureRandom.hex @@ -68,6 +69,10 @@ class Invite < ActiveRecord::Base email.blank? end + def redeemable? + !redeemed? && !expired? && !destroyed? && link_valid? + end + def redeemed? if is_invite_link? redemption_count >= max_redemptions_allowed @@ -163,20 +168,23 @@ class Invite < ActiveRecord::Base end def redeem(email: nil, username: nil, name: nil, password: nil, user_custom_fields: nil, ip_address: nil, session: nil) - if !expired? && !destroyed? && link_valid? - raise UserExists.new I18n.t("invite_link.email_taken") if is_invite_link? && UserEmail.exists?(email: email) - email = self.email if email.blank? && !is_invite_link? - InviteRedeemer.new( - invite: self, - email: email, - username: username, - name: name, - password: password, - user_custom_fields: user_custom_fields, - ip_address: ip_address, - session: session - ).redeem + 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, + username: username, + name: name, + password: password, + user_custom_fields: user_custom_fields, + ip_address: ip_address, + session: session + ).redeem end def self.redeem_from_email(email) @@ -254,14 +262,6 @@ class Invite < ActiveRecord::Base end end end - - def ensure_no_invalid_email_invites - return if email.blank? - - if SiteSetting.enable_discourse_connect? - errors.add(:email, I18n.t("invite.disabled_errors.discourse_connect_enabled")) - end - end end # == Schema Information diff --git a/app/services/user_activator.rb b/app/services/user_activator.rb index 49672b04ffe..6a7d8ad1b05 100644 --- a/app/services/user_activator.rb +++ b/app/services/user_activator.rb @@ -33,7 +33,7 @@ class UserActivator if !user.active? EmailActivator - elsif SiteSetting.must_approve_users? && !(invite.present? && !invite.expired? && !invite.destroyed? && invite.link_valid?) + elsif SiteSetting.must_approve_users? && !(invite.present? && invite.redeemable?) ApprovalActivator else LoginActivator diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 53541b81a1f..fbed6eee272 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -229,6 +229,7 @@ en: expired: "Your invite token has expired. Please contact staff." not_found: "Your invite token is invalid. Please contact staff." not_found_json: "Your invite token is invalid. Please contact staff." + not_matching_email: "Your email address and the email address associated with the invite token do not match. Please contact staff." not_found_template: |

Your invite to %{site_name} has already been redeemed.

@@ -2359,6 +2360,7 @@ en: blank_id_error: "The `external_id` is required but was blank" email_error: "An account could not be registered with the email address %{email}. Please contact the site's administrator." missing_secret: "Authentication failed due to missing secret. Contact the site administrators to fix this problem." + invite_redeem_failed: "Invite redemption failed. Please contact the site's administrator." original_poster: "Original Poster" most_posts: "Most Posts" diff --git a/lib/guardian.rb b/lib/guardian.rb index f65029bd1b7..f9488f951ce 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -351,14 +351,21 @@ class Guardian end def can_invite_to_forum?(groups = nil) - authenticated? && - (SiteSetting.max_invites_per_day.to_i > 0 || is_staff?) && - !SiteSetting.enable_discourse_connect && - ( - (!SiteSetting.must_approve_users? && @user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i)) || - is_staff? - ) && - (groups.blank? || is_admin? || groups.all? { |g| can_edit_group?(g) }) + return false if !authenticated? + + invites_available = SiteSetting.max_invites_per_day.to_i.positive? + trust_level_requirement_met = !SiteSetting.must_approve_users? && @user.has_trust_level?(SiteSetting.min_trust_level_to_allow_invite.to_i) + + if !is_staff? + return false if !invites_available + return false if !trust_level_requirement_met + end + + if groups.present? + return is_admin? || groups.all? { |g| can_edit_group?(g) } + end + + true end def can_invite_to?(object, groups = nil) @@ -390,7 +397,8 @@ class Guardian def can_invite_via_email?(object) return false unless can_invite_to?(object) - !SiteSetting.enable_discourse_connect && SiteSetting.enable_local_logins && (!SiteSetting.must_approve_users? || is_staff?) + (SiteSetting.enable_local_logins || SiteSetting.enable_discourse_connect) && + (!SiteSetting.must_approve_users? || is_staff?) end def can_bulk_invite_to_forum?(user) diff --git a/lib/single_sign_on.rb b/lib/single_sign_on.rb index 729b3df2006..a4c0bab2e75 100644 --- a/lib/single_sign_on.rb +++ b/lib/single_sign_on.rb @@ -117,6 +117,10 @@ class SingleSignOn OpenSSL::HMAC.hexdigest("sha256", secret, payload) end + def to_json + self.to_h.to_json + end + def to_url(base_url = nil) base = "#{base_url || sso_url}" "#{base}#{base.include?('?') ? '&' : '?'}#{payload}" @@ -128,6 +132,10 @@ class SingleSignOn end def unsigned_payload + Rack::Utils.build_query(self.to_h) + end + + def to_h payload = {} ACCESSORS.each do |k| @@ -139,7 +147,6 @@ class SingleSignOn payload["custom.#{k}"] = v.to_s end - Rack::Utils.build_query(payload) + payload end - end diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 04a051ad3e5..da2b9b59725 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -511,14 +511,6 @@ describe Guardian do expect(Guardian.new(user).can_invite_to_forum?).to be_falsey end - it 'returns false when DiscourseConnect is enabled' do - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - - expect(Guardian.new(user).can_invite_to_forum?).to be_falsey - expect(Guardian.new(moderator).can_invite_to_forum?).to be_falsey - end - context 'with groups' do let(:groups) { [group, another_group] } @@ -691,13 +683,13 @@ describe Guardian do expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_truthy end - it 'returns false for all users when sso is enabled' do + it 'returns true for all users when sso is enabled' do SiteSetting.discourse_connect_url = "https://www.example.com/sso" SiteSetting.enable_discourse_connect = true - expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_falsey - expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_falsey - expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_falsey + expect(Guardian.new(trust_level_2).can_invite_via_email?(topic)).to be_truthy + expect(Guardian.new(moderator).can_invite_via_email?(topic)).to be_truthy + expect(Guardian.new(admin).can_invite_via_email?(topic)).to be_truthy end it 'returns false for all users when local logins are disabled' do diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index a84427a3f97..bda281c9309 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -50,17 +50,6 @@ describe Invite do end end - context "DiscourseConnect validation" do - it "prevents creating an email invite when DiscourseConnect is enabled" do - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - - invite = Fabricate.build(:invite, email: "test@mail.com") - expect(invite).not_to be_valid - expect(invite.errors.details[:email].first[:error]).to eq(I18n.t("invite.disabled_errors.discourse_connect_enabled")) - end - end - context '#create' do context 'saved' do subject { Fabricate(:invite) } diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index f428518328a..8ff7c606bd8 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -1374,23 +1374,6 @@ describe GroupsController do expect(response.status).to eq(200) end - it "rejects unknown emails when DiscourseConnect is enabled" do - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - put "/groups/#{group.id}/members.json", params: { emails: "newuser@example.com" } - - expect(response.status).to eq(400) - expect(response.parsed_body["error_type"]).to eq("invalid_parameters") - end - - it "rejects unknown emails when local logins are disabled" do - SiteSetting.enable_local_logins = false - put "/groups/#{group.id}/members.json", params: { emails: "newuser@example.com" } - - expect(response.status).to eq(400) - expect(response.parsed_body["error_type"]).to eq("invalid_parameters") - end - it "will find users by email, and invite the correct user" do new_user = Fabricate(:user) expect(new_user.group_ids.include?(group.id)).to eq(false) diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index 942bb6046ad..8fd37b887ff 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -51,6 +51,13 @@ describe InvitesController do expect(CGI.unescapeHTML(body)).to_not include(I18n.t('invite.not_found_template', site_name: SiteSetting.title, base_url: Discourse.base_url)) end + it "stores the invite key in the secure session if invite exists" do + get "/invites/#{invite.invite_key}" + expect(response.status).to eq(200) + invite_key = read_secure_session["invite-key"] + expect(invite_key).to eq(invite.invite_key) + end + it "returns error if invite has already been redeemed" do Fabricate(:invited_user, invite: invite, user: Fabricate(:user)) get "/invites/#{invite.invite_key}" @@ -382,16 +389,6 @@ describe InvitesController do expect(response.status).to eq(404) end - it 'returns the right response when DiscourseConnect is enabled' do - invite - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - - put "/invites/show/#{invite.invite_key}.json" - - expect(response.status).to eq(404) - end - describe 'with authentication session' do let(:authenticated_email) { "foobar@example.com" } diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 9fd1ae9edd0..b065fd7e50e 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -659,7 +659,7 @@ RSpec.describe SessionController do def sso_for_ip_specs sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -694,7 +694,7 @@ RSpec.describe SessionController do it 'respects email restrictions' do sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -708,7 +708,7 @@ RSpec.describe SessionController do it 'allows you to create an admin account' do sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -735,7 +735,7 @@ RSpec.describe SessionController do it 'redirects to a non-relative url' do sso = get_sso("#{Discourse.base_url}/b/") - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -748,7 +748,7 @@ RSpec.describe SessionController do SiteSetting.discourse_connect_allows_all_return_paths = true sso = get_sso('https://gusundtrout.com') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -759,7 +759,7 @@ RSpec.describe SessionController do it 'redirects to root if the host of the return_path is different' do sso = get_sso('//eviltrout.com') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -770,7 +770,7 @@ RSpec.describe SessionController do it 'redirects to root if the host of the return_path is different' do sso = get_sso('http://eviltrout.com') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -783,7 +783,7 @@ RSpec.describe SessionController do group = Fabricate(:group, name: :bob, automatic_membership_email_domains: 'bob.com') sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -820,11 +820,106 @@ RSpec.describe SessionController do expect(logged_on_user.custom_fields["bla"]).to eq(nil) end + context "when an invitation is used" do + let(:invite) { Fabricate(:invite, email: invite_email, invited_by: Fabricate(:admin)) } + let(:invite_email) { nil } + + def login_with_sso_and_invite(invite_key = invite.invite_key) + write_secure_session("invite-key", invite_key) + sso = get_sso("/") + sso.external_id = "666" + sso.email = "bob@bob.com" + sso.name = "Sam Saffron" + sso.username = "sam" + + get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers + end + + it "errors if the invite key is invalid" do + login_with_sso_and_invite("wrong") + expect(response.status).to eq(400) + expect(response.body).to include(I18n.t("invite.not_found", base_url: Discourse.base_url)) + expect(invite.reload.redeemed?).to eq(false) + expect(User.find_by_email("bob@bob.com")).to eq(nil) + end + + it "errors if the invite has expired" do + invite.update!(expires_at: 3.days.ago) + login_with_sso_and_invite + expect(response.status).to eq(400) + expect(response.body).to include(I18n.t("invite.expired", base_url: Discourse.base_url)) + expect(invite.reload.redeemed?).to eq(false) + expect(User.find_by_email("bob@bob.com")).to eq(nil) + end + + it "errors if the invite has been redeemed already" do + invite.update!(max_redemptions_allowed: 1, redemption_count: 1) + login_with_sso_and_invite + expect(response.status).to eq(400) + expect(response.body).to include(I18n.t("invite.not_found_template", site_name: SiteSetting.title, base_url: Discourse.base_url)) + expect(invite.reload.redeemed?).to eq(true) + expect(User.find_by_email("bob@bob.com")).to eq(nil) + end + + it "errors if the invite is for a specific email and that email does not match the sso email" do + invite.update!(email: "someotheremail@dave.com") + login_with_sso_and_invite + expect(response.status).to eq(400) + expect(response.body).to include(I18n.t("invite.not_matching_email", base_url: Discourse.base_url)) + expect(invite.reload.redeemed?).to eq(false) + expect(User.find_by_email("bob@bob.com")).to eq(nil) + end + + it "allows you to create an account and redeems the invite successfully, clearing the invite-key session" do + login_with_sso_and_invite + + expect(response.status).to eq(302) + expect(response).to redirect_to("/") + expect(invite.reload.redeemed?).to eq(true) + + user = User.find_by_email("bob@bob.com") + expect(user.active).to eq(true) + expect(session[:current_user_id]).to eq(user.id) + expect(read_secure_session["invite-key"]).to eq(nil) + end + + it "allows you to create an account and redeems the invite successfully even if must_approve_users is enabled" do + SiteSetting.must_approve_users = true + + login_with_sso_and_invite + + expect(response.status).to eq(302) + expect(response).to redirect_to("/") + expect(invite.reload.redeemed?).to eq(true) + + user = User.find_by_email("bob@bob.com") + expect(user.active).to eq(true) + end + + it "redirects to the topic associated to the invite" do + topic_invite = TopicInvite.create!(invite: invite, topic: Fabricate(:topic)) + login_with_sso_and_invite + + expect(response.status).to eq(302) + expect(response).to redirect_to(topic_invite.topic.relative_url) + end + + it "adds the user to the appropriate invite groups" do + invited_group = InvitedGroup.create!(invite: invite, group: Fabricate(:group)) + login_with_sso_and_invite + + expect(invite.reload.redeemed?).to eq(true) + + user = User.find_by_email("bob@bob.com") + expect(GroupUser.exists?(user: user, group: invited_group.group)).to eq(true) + end + end + context 'when sso emails are not trusted' do context 'if you have not activated your account' do it 'does not log you in' do sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' @@ -838,7 +933,7 @@ RSpec.describe SessionController do it 'sends an activation email' do sso = get_sso('/a/') - sso.external_id = '666' # the number of the beast + sso.external_id = '666' sso.email = 'bob@bob.com' sso.name = 'Sam Saffron' sso.username = 'sam' diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index ce974f3e1dd..e0e7dda210e 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1757,25 +1757,6 @@ describe UsersController do end end - context 'when DiscourseConnect has been enabled' do - before do - SiteSetting.discourse_connect_url = "https://www.example.com/sso" - SiteSetting.enable_discourse_connect = true - end - - it 'explains why invites are disabled to staff users' do - inviter = sign_in(Fabricate(:admin)) - Fabricate(:invite, invited_by: inviter, email: nil, max_redemptions_allowed: 5, expires_at: 1.month.from_now, emailed_status: Invite.emailed_status_types[:not_required]) - - get "/u/#{inviter.username}/invited/pending.json" - expect(response.status).to eq(200) - - expect(response.parsed_body['error']).to include(I18n.t( - 'invite.disabled_errors.discourse_connect_enabled' - )) - end - end - context 'with redeemed invites' do it 'returns invites' do sign_in(Fabricate(:moderator)) diff --git a/spec/support/integration_helpers.rb b/spec/support/integration_helpers.rb index 182a5514ee4..45a7b3cfbc6 100644 --- a/spec/support/integration_helpers.rb +++ b/spec/support/integration_helpers.rb @@ -46,4 +46,10 @@ module IntegrationHelpers SecureSession.new(session[:secure_session_id]) end + + def write_secure_session(key, value) + secure_session = read_secure_session + secure_session[key] = value + secure_session + end end