Skip to content
Permalink
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
gschlager and tgxworld committed Jun 2, 2022
1 parent 9d577be commit 7c4e2d3
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 108 deletions.
8 changes: 5 additions & 3 deletions app/controllers/session_controller.rb
Expand Up @@ -156,9 +156,11 @@ def sso_login
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
Expand Down
18 changes: 2 additions & 16 deletions app/models/invite_redeemer.rb
Expand Up @@ -40,10 +40,8 @@ def self.create_user_from_invite(email:, invite:, username: nil, name: nil, pass
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
Expand Down Expand Up @@ -95,7 +93,6 @@ def invited_user
end

def process_invitation
approve_account_if_needed
add_to_private_topics_if_invited
add_user_to_groups
send_welcome_message
Expand Down Expand Up @@ -170,17 +167,6 @@ def send_welcome_message
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!(
Expand Down
2 changes: 1 addition & 1 deletion app/models/reviewable_user.rb
Expand Up @@ -12,7 +12,7 @@ def self.create_for(user)
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"
Expand Down
121 changes: 44 additions & 77 deletions spec/models/invite_redeemer_spec.rb
Expand Up @@ -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

Expand All @@ -20,7 +20,7 @@
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
Expand All @@ -47,7 +47,7 @@
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
Expand All @@ -57,7 +57,7 @@
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

Expand All @@ -80,24 +80,8 @@
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

Expand All @@ -108,30 +92,45 @@
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
Expand All @@ -142,17 +141,7 @@
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
Expand All @@ -164,7 +153,7 @@
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
Expand Down Expand Up @@ -226,28 +215,6 @@
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') }
Expand Down
8 changes: 0 additions & 8 deletions spec/models/invite_spec.rb
Expand Up @@ -287,14 +287,6 @@
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) }
Expand Down
7 changes: 4 additions & 3 deletions spec/requests/session_controller_spec.rb
Expand Up @@ -931,17 +931,18 @@ def login_with_sso_and_invite(invite_key = invite.invite_key)
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
Expand Down

1 comment on commit 7c4e2d3

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/staff-generated-invites-bypass-the-must-approve-users-requirement/228199/28

Please sign in to comment.