Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Delete email tokens when a user's email is changed or delet…
…ed (#19736)

Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
  • Loading branch information
tgxworld and OsamaSayegh committed Jan 5, 2023
1 parent b9e2e99 commit 4bf306f
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 10 deletions.
8 changes: 8 additions & 0 deletions app/models/user_email.rb
Expand Up @@ -18,6 +18,10 @@ class UserEmail < ActiveRecord::Base

scope :secondary, -> { where(primary: false) }

before_save ->() { destroy_email_tokens(self.email_was) }, if: :will_save_change_to_email?

after_destroy { destroy_email_tokens(self.email) }

def normalize_email
self.normalized_email = if self.email.present?
username, domain = self.email.split('@', 2)
Expand Down Expand Up @@ -62,6 +66,10 @@ def user_id_not_changed
)
end
end

def destroy_email_tokens(email)
EmailToken.where(email: email).destroy_all
end
end

# == Schema Information
Expand Down
19 changes: 9 additions & 10 deletions spec/requests/users_controller_spec.rb
Expand Up @@ -3063,17 +3063,18 @@ def post_user(extra_params = {})
expect(user1.email_change_requests).to contain_exactly(request_1)
end

it "can destroy associated email tokens" do
it "destroys associated email tokens and email change requests" do
new_email = 'new.n.cool@example.com'
updater = EmailUpdater.new(guardian: user1.guardian, user: user1)
updater.change_to(new_email)

expect { updater.change_to(new_email) }
.to change { user1.email_tokens.count }.by(1)
email_token = updater.change_req.new_email_token
expect(email_token).to be_present

expect { delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email } }
.to change { user1.email_tokens.count }.by(-1)
delete "/u/#{user1.username}/preferences/email.json", params: { email: new_email }

expect(user1.email_tokens.first.email).to eq(user1.email)
expect(EmailToken.find_by(id: email_token.id)).to eq(nil)
expect(EmailChangeRequest.find_by(id: updater.change_req.id)).to eq(nil)
end
end

Expand Down Expand Up @@ -3439,8 +3440,7 @@ def create_and_like_post(likee, liker)
expect(user.email).to eq('updatedemail@example.com')
expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present

token.reload
expect(token.expired?).to eq(true)
expect(EmailToken.find_by(id: token.id)).to eq(nil)
end

it 'tells the user to slow down after many requests' do
Expand Down Expand Up @@ -3530,8 +3530,7 @@ def create_and_like_post(likee, liker)
expect(user.email).to eq('updatedemail@example.com')
expect(user.email_tokens.where(email: 'updatedemail@example.com', expired: false)).to be_present

token.reload
expect(token.expired?).to eq(true)
expect(EmailToken.find_by(id: token.id)).to eq(nil)
end

it 'tells the user to slow down after many requests' do
Expand Down
22 changes: 22 additions & 0 deletions spec/requests/users_email_controller_spec.rb
Expand Up @@ -244,6 +244,28 @@
end
end
end

it "destroys email tokens associated with the old email after the new email is confirmed" do
SiteSetting.enable_secondary_emails = true

email_token = user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])

updater = EmailUpdater.new(guardian: user.guardian, user: user)
updater.change_to('bubblegum@adventuretime.ooo')

sign_in(user)
put "/u/confirm-new-email", params: {
token: "#{updater.change_req.new_email_token.token}"
}

new_password = SecureRandom.hex
put "/u/password-reset/#{email_token.token}.json", params: {
password: new_password
}
expect(response.parsed_body["success"]).to eq(false)
expect(response.parsed_body["message"]).to eq(I18n.t("password_reset.no_token", base_url: Discourse.base_url))
expect(user.reload.confirm_password?(new_password)).to eq(false)
end
end

describe '#confirm-old-email' do
Expand Down

0 comments on commit 4bf306f

Please sign in to comment.