diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index c66252cb3a5..be2535241f6 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -156,9 +156,11 @@ class SessionController < ApplicationController return end - # 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.must_approve_users? && !user.approved? + if invite.present? && user.invited_user.blank? + redeem_invitation(invite, sso) + end + if SiteSetting.discourse_connect_not_approved_url.present? redirect_to SiteSetting.discourse_connect_not_approved_url, allow_other_host: true else diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index e8eec563b7c..bd0fd609ca6 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -40,10 +40,8 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ registration_ip_address: ip_address } - if !SiteSetting.must_approve_users? || - (SiteSetting.must_approve_users? && invite.invited_by.staff?) || - EmailValidator.can_auto_approve_user?(user.email) - ReviewableUser.set_approved_fields!(user, invite.invited_by) + if SiteSetting.must_approve_users? && EmailValidator.can_auto_approve_user?(user.email) + ReviewableUser.set_approved_fields!(user, Discourse.system_user) end user_fields = UserField.all @@ -95,7 +93,6 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ end def process_invitation - approve_account_if_needed add_to_private_topics_if_invited add_user_to_groups send_welcome_message @@ -170,17 +167,6 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_ invited_user.send_welcome_message = true end - def approve_account_if_needed - if invited_user.present? && reviewable_user = ReviewableUser.find_by(target: invited_user, status: Reviewable.statuses[:pending]) - reviewable_user.perform( - invite.invited_by, - :approve_user, - send_email: false, - approved_by_invite: true - ) - end - end - def notify_invitee if inviter = invite.invited_by inviter.notifications.create!( diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 02cbfe4bf0f..5e8c188d5af 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -12,7 +12,7 @@ class ReviewableUser < Reviewable def build_actions(actions, guardian, args) return unless pending? - if guardian.can_approve?(target) || args[:approved_by_invite] + if guardian.can_approve?(target) actions.add(:approve_user) do |a| a.icon = 'user-plus' a.label = "reviewables.actions.approve_user.title" diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb index 981f8f998f2..e782f84bdce 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/spec/models/invite_redeemer_spec.rb @@ -2,14 +2,14 @@ describe InviteRedeemer do - describe '#create_user_from_invite' do + describe '.create_user_from_invite' do it "should be created correctly" do invite = Fabricate(:invite, email: 'walter.white@email.com') user = InviteRedeemer.create_user_from_invite(invite: invite, email: invite.email, username: 'walter', name: 'Walter White') expect(user.username).to eq('walter') expect(user.name).to eq('Walter White') expect(user.email).to eq('walter.white@email.com') - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) expect(user.active).to eq(false) end @@ -20,7 +20,7 @@ describe InviteRedeemer do user = InviteRedeemer.create_user_from_invite(invite: invite, email: invite.email, username: 'walter', name: 'Walter White', password: password, ip_address: ip_address) expect(user).to have_password expect(user.confirm_password?(password)).to eq(true) - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) expect(user.ip_address).to eq(ip_address) expect(user.registration_ip_address).to eq(ip_address) end @@ -47,7 +47,7 @@ describe InviteRedeemer do expect(user.name).to eq('Walter White') expect(user.staged).to eq(false) expect(user.email).to eq('staged@account.com') - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) end it "activates user invited via email with a token" do @@ -57,7 +57,7 @@ describe InviteRedeemer do expect(user.username).to eq('walter') expect(user.name).to eq('Walter White') expect(user.email).to eq('walter.white@email.com') - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) expect(user.active).to eq(true) end @@ -80,24 +80,8 @@ describe InviteRedeemer do expect(user.username).to eq('walter') expect(user.name).to eq('Walter White') expect(user.email).to eq('walter.white@email.com') - expect(user.approved).to eq(true) - expect(user.active).to eq(false) - end - - it "does not automatically approve users if must_approve_users is true" do - SiteSetting.must_approve_users = true - - invite = Fabricate(:invite, email: 'test@example.com') - user = InviteRedeemer.create_user_from_invite(invite: invite, email: invite.email, username: 'test') expect(user.approved).to eq(false) - end - - it "approves user if invited by staff" do - SiteSetting.must_approve_users = true - - invite = Fabricate(:invite, email: 'test@example.com', invited_by: Fabricate(:admin)) - user = InviteRedeemer.create_user_from_invite(invite: invite, email: invite.email, username: 'test') - expect(user.approved).to eq(true) + expect(user.active).to eq(false) end end @@ -108,30 +92,45 @@ describe InviteRedeemer do let(:password) { 'know5nOthiNG' } let(:invite_redeemer) { InviteRedeemer.new(invite: invite, email: invite.email, username: username, name: name) } - it "should redeem the invite if invited by staff" do - SiteSetting.must_approve_users = true - inviter = invite.invited_by - inviter.admin = true - user = invite_redeemer.redeem - invite.reload + context "when must_approve_users setting is enabled" do + before do + SiteSetting.must_approve_users = true + end - expect(user.name).to eq(name) - expect(user.username).to eq(username) - expect(user.invited_by).to eq(inviter) - expect(inviter.notifications.count).to eq(1) - expect(user.approved).to eq(true) - end + it "should redeem an invite but not approve the user when invite is created by a staff user" do + inviter = invite.invited_by + inviter.update!(admin: true) + user = invite_redeemer.redeem - it "should redeem the invite if invited by non staff but not approve" do - SiteSetting.must_approve_users = true - inviter = invite.invited_by - user = invite_redeemer.redeem + expect(user.name).to eq(name) + expect(user.username).to eq(username) + expect(user.invited_by).to eq(inviter) + expect(user.approved).to eq(false) - expect(user.name).to eq(name) - expect(user.username).to eq(username) - expect(user.invited_by).to eq(inviter) - expect(inviter.notifications.count).to eq(1) - expect(user.approved).to eq(false) + expect(inviter.notifications.count).to eq(1) + end + + it "should redeem the invite but not approve the user when invite is created by a regular user" do + inviter = invite.invited_by + user = invite_redeemer.redeem + + expect(user.name).to eq(name) + expect(user.username).to eq(username) + expect(user.invited_by).to eq(inviter) + expect(user.approved).to eq(false) + + expect(inviter.notifications.count).to eq(1) + end + + it "should redeem the invite and approve the user when user email is in auto_approve_email_domains setting" do + SiteSetting.auto_approve_email_domains = "example.com" + user = invite_redeemer.redeem + + expect(user.name).to eq(name) + expect(user.username).to eq(username) + expect(user.approved).to eq(true) + expect(user.approved_by).to eq(Discourse.system_user) + end end it "should redeem the invite if invited by non staff and approve if staff not required to approve" do @@ -142,17 +141,7 @@ describe InviteRedeemer do expect(user.username).to eq(username) expect(user.invited_by).to eq(inviter) expect(inviter.notifications.count).to eq(1) - expect(user.approved).to eq(true) - end - - it "should redeem the invite if invited by non staff and approve if email in auto_approve_email_domains setting" do - SiteSetting.must_approve_users = true - SiteSetting.auto_approve_email_domains = "example.com" - user = invite_redeemer.redeem - - expect(user.name).to eq(name) - expect(user.username).to eq(username) - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) end it "should delete invite if invited_by user has been removed" do @@ -164,7 +153,7 @@ describe InviteRedeemer do user = InviteRedeemer.new(invite: invite, email: invite.email, username: username, name: name, password: password).redeem expect(user).to have_password expect(user.confirm_password?(password)).to eq(true) - expect(user.approved).to eq(true) + expect(user.approved).to eq(false) end it "can set custom fields" do @@ -226,28 +215,6 @@ describe InviteRedeemer do expect(invite.invited_users.first).to be_present end - context "ReviewableUser" do - it "approves pending record" do - reviewable = ReviewableUser.needs_review!(target: Fabricate(:user, email: invite.email), created_by: invite.invited_by) - reviewable.status = Reviewable.statuses[:pending] - reviewable.save! - invite_redeemer.redeem - - reviewable.reload - expect(reviewable.status).to eq(Reviewable.statuses[:approved]) - end - - it "does not raise error if record is not pending" do - reviewable = ReviewableUser.needs_review!(target: Fabricate(:user, email: invite.email), created_by: invite.invited_by) - reviewable.status = Reviewable.statuses[:ignored] - reviewable.save! - invite_redeemer.redeem - - reviewable.reload - expect(reviewable.status).to eq(Reviewable.statuses[:ignored]) - end - end - context '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') } diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index 5efa9c02503..2f9dff38047 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -287,14 +287,6 @@ describe Invite do end end - it 'activates user when must_approve_users? is enabled' do - SiteSetting.must_approve_users = true - invite.invited_by = Fabricate(:admin) - - user = invite.redeem - expect(user.approved?).to eq(true) - end - context 'invite to a topic' do fab!(:topic) { Fabricate(:private_message_topic) } fab!(:another_topic) { Fabricate(:private_message_topic) } diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 4b01607d2dd..e9b02ced1a5 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -931,17 +931,18 @@ describe SessionController do 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 + it "creates the user account and redeems the invite but does not approve the user 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(response.status).to eq(403) + expect(response.parsed_body).to include(I18n.t("discourse_connect.account_not_approved")) expect(invite.reload.redeemed?).to eq(true) user = User.find_by_email("bob@bob.com") expect(user.active).to eq(true) + expect(user.approved).to eq(false) end it "redirects to the topic associated to the invite" do