SECURITY: Remove auto approval when redeeming an invite (#16974)

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 <gxtan1990@gmail.com>

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,
+        if SiteSetting.must_approve_users? && !user.approved?
-        # they are already pre-approved because they have been invited
+          if invite.present? && user.invited_user.blank?
-        if SiteSetting.must_approve_users? && !user.approved? && invite.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

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? ||
+    if SiteSetting.must_approve_users? && EmailValidator.can_auto_approve_user?(user.email)
-        (SiteSetting.must_approve_users? && invite.invited_by.staff?) ||
+      ReviewableUser.set_approved_fields!(user, Discourse.system_user)
-        EmailValidator.can_auto_approve_user?(user.email)
-      ReviewableUser.set_approved_fields!(user, invite.invited_by)
     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!(

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"

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
+      expect(user.active).to eq(false)
-
-    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)
     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
+    context "when must_approve_users setting is enabled" do
-      SiteSetting.must_approve_users = true
+      before do
-      inviter = invite.invited_by
+        SiteSetting.must_approve_users = true
-      inviter.admin = true
+      end
-      user = invite_redeemer.redeem
-      invite.reload
 
-      expect(user.name).to eq(name)
+      it "should redeem an invite but not approve the user when invite is created by a staff user" do
-      expect(user.username).to eq(username)
+        inviter = invite.invited_by
-      expect(user.invited_by).to eq(inviter)
+        inviter.update!(admin: true)
-      expect(inviter.notifications.count).to eq(1)
+        user = invite_redeemer.redeem
-      expect(user.approved).to eq(true)
-    end
 
-    it "should redeem the invite if invited by non staff but not approve" do
+        expect(user.name).to eq(name)
-      SiteSetting.must_approve_users = true
+        expect(user.username).to eq(username)
-      inviter = invite.invited_by
+        expect(user.invited_by).to eq(inviter)
-      user = invite_redeemer.redeem
+        expect(user.approved).to eq(false)
 
-      expect(user.name).to eq(name)
+        expect(inviter.notifications.count).to eq(1)
-      expect(user.username).to eq(username)
+      end
-      expect(user.invited_by).to eq(inviter)
+
-      expect(inviter.notifications.count).to eq(1)
+      it "should redeem the invite but not approve the user when invite is created by a regular user" do
-      expect(user.approved).to eq(false)
+        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)
+      expect(user.approved).to eq(false)
-    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)
     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') }

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) }

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.status).to eq(403)
-        expect(response).to redirect_to("/")
+        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