From 5c91d9a62990c1234f1608abd0f9f8f1d99c46f1 Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Thu, 2 Jun 2022 16:11:04 +0200 Subject: [PATCH] SECURITY: Remove auto approval when redeeming an invite (#16976) This security fix affects sites which have `SiteSetting.must_approve_users` enabled. There are intentional and unintentional cases where invited users can be auto approved and are deemed to have skipped the staff approval process. Instead of trying to reason about when auto-approval should happen, we have decided that enabling the `must_approve_users` setting going forward will just mean that all new users must be explicitly approved by a staff user in the review queue. The only case where users are auto approved is when the `auto_approve_email_domains` site setting is used. Co-authored-by: Alan Guo Xiang Tan --- app/controllers/session_controller.rb | 8 +- app/models/invite_redeemer.rb | 18 +--- app/models/reviewable_user.rb | 2 +- spec/models/invite_redeemer_spec.rb | 121 +++++++++-------------- spec/models/invite_spec.rb | 8 -- spec/requests/session_controller_spec.rb | 7 +- 6 files changed, 56 insertions(+), 108 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index d24e3e9e03c..dd084563701 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -181,9 +181,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 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 93bc96924e7..0b2a8ace597 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 7fe1cb5dc55..36fe5b2864b 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/spec/models/invite_redeemer_spec.rb @@ -4,14 +4,14 @@ require 'rails_helper' 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 @@ -22,7 +22,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 @@ -49,7 +49,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 @@ -59,7 +59,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 @@ -82,24 +82,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 @@ -110,30 +94,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 @@ -144,17 +143,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 @@ -166,7 +155,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 @@ -228,28 +217,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 994aeeabe51..72af96c60e7 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -292,14 +292,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 be65aec5eb3..fb28a6352d9 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -885,17 +885,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