CVE-2022-31025: Other (NVD-CWE-Other)
value: Discourse is an open source platform for community discussion. Prior to version 2.8.4 on the `stable` branch and 2.9.0beta5 on the `beta` and `tests-passed` branches, inviting users on sites that use single sign-on could bypass the `must_approve_users` check and invites by staff are always approved automatically. The issue is patched in Discourse version 2.8.4 on the `stable` branch and version `2.9.0.beta5` on the `beta` and `tests-passed` branches. As a workaround, disable invites or increase `min_trust_level_to_allow_invite` to reduce the attack surface to more trusted users.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 5.3
Published: 2022-06-07T15:15Z
Repository: https://github.com/discourse/discourse
Language: Ruby
Files changed: app/controllers/session_controller.rb, app/models/invite_redeemer.rb, app/models/reviewable_user.rb, spec/models/invite_redeemer_spec.rb, spec/models/invite_spec.rb, spec/requests/session_controller_spec.rb
Commit date: 2022-06-02 16:10:48 +0200
Commit message: 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>
Diff: diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index c66252cb3a52..be2535241f62 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -154,13 +154,15 @@ class SessionController < ApplicationController
         if user.suspended?
           render_sso_error(text: failed_to_login(user)[:error], status: 403)
           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
             render_sso_error(text: I18n.t("discourse_connect.account_not_approved"), status: 403)
           end
diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb
index e8eec563b7cb..bd0fd609ca69 100644
--- a/app/models/invite_redeemer.rb
+++ b/app/models/invite_redeemer.rb
@@ -38,14 +38,12 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
       trust_level: SiteSetting.default_invitee_trust_level,
       ip_address: ip_address,
       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
     if user_custom_fields.present? && user_fields.present?
       field_params = user_custom_fields || {}
@@ -93,11 +91,10 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
   def invited_user
     @invited_user ||= get_invited_user
   end
 
   def process_invitation
-    approve_account_if_needed
     add_to_private_topics_if_invited
     add_user_to_groups
     send_welcome_message
     notify_invitee
   end
@@ -168,21 +165,10 @@ InviteRedeemer = Struct.new(:invite, :email, :username, :name, :password, :user_
   def send_welcome_message
     @invited_user_record.update!(user_id: invited_user.id)
     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!(
         notification_type: Notification.types[:invitee_accepted],
         data: { display_username: invited_user.username }.to_json
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index 02cbfe4bf0f3..5e8c188d5af8 100644
--- a/app/models/reviewable_user.rb
+++ b/app/models/reviewable_user.rb
@@ -10,11 +10,11 @@ class ReviewableUser < Reviewable
   end
 
   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"
       end
     end
diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb
index 981f8f998f24..e782f84bdce0 100644
--- a/spec/models/invite_redeemer_spec.rb
+++ b/spec/models/invite_redeemer_spec.rb
@@ -1,28 +1,28 @@
 # frozen_string_literal: true
 
 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
 
     it "can set the password and ip_address" do
       password = 's3cure5tpasSw0rD'
       ip_address = '192.168.1.1'
       invite = Fabricate(:invite, email: 'walter.white@email.com')
       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
 
     it "raises exception with record and errors" do
@@ -45,21 +45,21 @@ describe InviteRedeemer do
       expect(user.id).to eq(staged_user.id)
       expect(user.username).to eq('walter')
       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
       invite = Fabricate(:invite, invited_by: Fabricate(:admin), email: 'walter.white@email.com', emailed_status: Invite.emailed_status_types[:sent])
       user = InviteRedeemer.create_user_from_invite(invite: invite, email: invite.email, username: 'walter', name: 'Walter White', email_token: invite.email_token)
 
       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
 
     it "does not activate user invited via email with a wrong token" do
       invite = Fabricate(:invite, invited_by: Fabricate(:user), email: 'walter.white@email.com', emailed_status: Invite.emailed_status_types[:sent])
@@ -78,83 +78,72 @@ describe InviteRedeemer do
       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.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
 
   describe "#redeem" do
     fab!(:invite) { Fabricate(:invite, email: "foobar@example.com") }
     let(:name) { 'john snow' }
     let(:username) { 'kingofthenorth' }
     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
       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(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
       invite.invited_by.destroy!
       expect { invite.reload }.to raise_error(ActiveRecord::RecordNotFound)
@@ -162,11 +151,11 @@ describe InviteRedeemer do
 
     it "can set password" 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
       required_field = Fabricate(:user_field)
       optional_field = Fabricate(:user_field, required: false)
@@ -224,32 +213,10 @@ describe InviteRedeemer do
       expect(user.invited_by).to eq(inviter)
       expect(inviter.notifications.count).to eq(1)
       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') }
 
       it 'works as expected' do
diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb
index 5efa9c02503a..2f9dff380477 100644
--- a/spec/models/invite_spec.rb
+++ b/spec/models/invite_spec.rb
@@ -285,18 +285,10 @@ describe Invite do
         user = invite.redeem
         expect(user.groups).to contain_exactly(group)
       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) }
 
       before do
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index 4b01607d2dd8..e9b02ced1a54 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -929,21 +929,22 @@ describe SessionController do
         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
+      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
         topic_invite = TopicInvite.create!(invite: invite, topic: Fabricate(:topic))
         login_with_sso_and_invite

Vulnerable code:         # 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? ||
        (SiteSetting.must_approve_users? && invite.invited_by.staff?) ||
        EmailValidator.can_auto_approve_user?(user.email)
      ReviewableUser.set_approved_fields!(user, invite.invited_by)
    approve_account_if_needed
  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

    if guardian.can_approve?(target) || args[:approved_by_invite]
  describe '#create_user_from_invite' do
      expect(user.approved).to eq(true)
      expect(user.approved).to eq(true)
      expect(user.approved).to eq(true)
      expect(user.approved).to eq(true)
      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')
    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)
    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
      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 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(inviter.notifications.count).to eq(1)
      expect(user.approved).to eq(false)
      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(true)
    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

    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

      it "allows you to create an account and redeems the invite successfully even if must_approve_users is enabled" do
        expect(response.status).to eq(302)
        expect(response).to redirect_to("/")
Fixed code:         if SiteSetting.must_approve_users? && !user.approved?
          if invite.present? && user.invited_user.blank?
            redeem_invitation(invite, sso)
          end

    if SiteSetting.must_approve_users? && EmailValidator.can_auto_approve_user?(user.email)
      ReviewableUser.set_approved_fields!(user, Discourse.system_user)
    if guardian.can_approve?(target)
  describe '.create_user_from_invite' do
      expect(user.approved).to eq(false)
      expect(user.approved).to eq(false)
      expect(user.approved).to eq(false)
      expect(user.approved).to eq(false)
      expect(user.active).to eq(false)
    context "when must_approve_users setting is enabled" do
      before do
        SiteSetting.must_approve_users = 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
        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 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
      expect(user.approved).to eq(false)
      expect(user.approved).to eq(false)
      it "creates the user account and redeems the invite but does not approve the user if must_approve_users is enabled" do
        expect(response.status).to eq(403)
        expect(response.parsed_body).to include(I18n.t("discourse_connect.account_not_approved"))
        expect(user.approved).to eq(false)