mirror of
https://github.com/discourse/discourse.git
synced 2025-06-01 09:08:10 +08:00
DEV: Hash tokens stored from email_tokens (#14493)
This commit adds token_hash and scopes columns to email_tokens table. token_hash is a replacement for the token column to avoid storing email tokens in plaintext as it can pose a security risk. The new scope column ensures that email tokens cannot be used to perform a different action than the one intended. To sum up, this commit: * Adds token_hash and scope to email_tokens * Reuses code that schedules critical_user_email * Refactors EmailToken.confirm and EmailToken.atomic_confirm methods * Periodically cleans old, unconfirmed or expired email tokens
This commit is contained in:
Dan Ungureanu
@ -6,18 +6,19 @@ require 'rotp'
|
||||
describe UsersEmailController do
|
||||
|
||||
fab!(:user) { Fabricate(:user) }
|
||||
let!(:email_token) { Fabricate(:email_token, user: user) }
|
||||
fab!(:moderator) { Fabricate(:moderator) }
|
||||
|
||||
describe "#confirm-new-email" do
|
||||
it 'does not redirect to login for signed out accounts, this route works fine as anon user' do
|
||||
get "/u/confirm-new-email/asdfasdf"
|
||||
get "/u/confirm-new-email/invalidtoken"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
end
|
||||
|
||||
it 'does not redirect to login for signed out accounts on login_required sites, this route works fine as anon user' do
|
||||
SiteSetting.login_required = true
|
||||
get "/u/confirm-new-email/asdfasdf"
|
||||
get "/u/confirm-new-email/invalidtoken"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
end
|
||||
@ -25,7 +26,7 @@ describe UsersEmailController do
|
||||
it 'errors out for invalid tokens' do
|
||||
sign_in(user)
|
||||
|
||||
get "/u/confirm-new-email/asdfasdf"
|
||||
get "/u/confirm-new-email/invalidtoken"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.body).to include(I18n.t('change_email.already_done'))
|
||||
@ -33,18 +34,14 @@ describe UsersEmailController do
|
||||
|
||||
it 'does not change email if accounts mismatch for a signed in user' do
|
||||
updater = EmailUpdater.new(guardian: user.guardian, user: user)
|
||||
updater.change_to('new.n.cool@example.com')
|
||||
updater.change_to('bubblegum@adventuretime.ooo')
|
||||
|
||||
old_email = user.email
|
||||
|
||||
sign_in(moderator)
|
||||
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: "#{user.email_tokens.last.token}"
|
||||
}
|
||||
|
||||
user.reload
|
||||
expect(user.email).to eq(old_email)
|
||||
put "/u/confirm-new-email", params: { token: "#{email_token.token}" }
|
||||
expect(user.reload.email).to eq(old_email)
|
||||
end
|
||||
|
||||
context "with a valid user" do
|
||||
@ -52,14 +49,14 @@ describe UsersEmailController do
|
||||
|
||||
before do
|
||||
sign_in(user)
|
||||
updater.change_to('new.n.cool@example.com')
|
||||
updater.change_to('bubblegum@adventuretime.ooo')
|
||||
end
|
||||
|
||||
it 'includes security_key_allowed_credential_ids in a hidden field' do
|
||||
key1 = Fabricate(:user_security_key_with_random_credential, user: user)
|
||||
key2 = Fabricate(:user_security_key_with_random_credential, user: user)
|
||||
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
|
||||
doc = Nokogiri::HTML5(response.body)
|
||||
credential_ids = doc.css("#security-key-allowed-credential-ids").first["value"].split(",")
|
||||
@ -70,17 +67,15 @@ describe UsersEmailController do
|
||||
user.user_stat.update_columns(bounce_score: 42, reset_bounce_score_after: 1.week.from_now)
|
||||
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: "#{user.email_tokens.last.token}"
|
||||
token: "#{updater.change_req.new_email_token.token}"
|
||||
}
|
||||
|
||||
expect(response.status).to eq(302)
|
||||
expect(response.redirect_url).to include("done")
|
||||
|
||||
user.reload
|
||||
|
||||
expect(user.user_stat.bounce_score).to eq(0)
|
||||
expect(user.user_stat.reset_bounce_score_after).to eq(nil)
|
||||
expect(user.email).to eq("new.n.cool@example.com")
|
||||
expect(user.email).to eq('bubblegum@adventuretime.ooo')
|
||||
end
|
||||
|
||||
context 'second factor required' do
|
||||
@ -88,29 +83,23 @@ describe UsersEmailController do
|
||||
fab!(:backup_code) { Fabricate(:user_second_factor_backup, user: user) }
|
||||
|
||||
it 'requires a second factor token' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
response_body = response.body
|
||||
|
||||
expect(response_body).to include(I18n.t("login.second_factor_title"))
|
||||
expect(response_body).not_to include(I18n.t("login.invalid_second_factor_code"))
|
||||
expect(response.body).to include(I18n.t("login.second_factor_title"))
|
||||
expect(response.body).not_to include(I18n.t("login.invalid_second_factor_code"))
|
||||
end
|
||||
|
||||
it 'requires a backup token' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_backup=true"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_backup=true"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
response_body = response.body
|
||||
|
||||
expect(response_body).to include(I18n.t("login.second_factor_backup_title"))
|
||||
expect(response.body).to include(I18n.t("login.second_factor_backup_title"))
|
||||
end
|
||||
|
||||
it 'adds an error on a second factor attempt' do
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: user.email_tokens.last.token,
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: "000000",
|
||||
second_factor_method: UserSecondFactor.methods[:totp]
|
||||
}
|
||||
@ -123,13 +112,11 @@ describe UsersEmailController do
|
||||
put "/u/confirm-new-email", params: {
|
||||
second_factor_token: ROTP::TOTP.new(second_factor.data).now,
|
||||
second_factor_method: UserSecondFactor.methods[:totp],
|
||||
token: user.email_tokens.last.token
|
||||
token: updater.change_req.new_email_token.token
|
||||
}
|
||||
|
||||
expect(response.status).to eq(302)
|
||||
|
||||
user.reload
|
||||
expect(user.email).to eq("new.n.cool@example.com")
|
||||
expect(user.reload.email).to eq('bubblegum@adventuretime.ooo')
|
||||
end
|
||||
|
||||
context "rate limiting" do
|
||||
@ -163,7 +150,7 @@ describe UsersEmailController do
|
||||
6.times do |x|
|
||||
user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:complete])
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: user.email_tokens.last.token,
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: "000000",
|
||||
second_factor_method: UserSecondFactor.methods[:totp]
|
||||
}, env: { "REMOTE_ADDR": "1.2.3.#{x}" }
|
||||
@ -173,7 +160,7 @@ describe UsersEmailController do
|
||||
|
||||
user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:authorizing_new])
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: user.email_tokens.last.token,
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: "000000",
|
||||
second_factor_method: UserSecondFactor.methods[:totp]
|
||||
}, env: { "REMOTE_ADDR": "1.2.3.4" }
|
||||
@ -198,14 +185,11 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it 'requires a security key' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
response_body = response.body
|
||||
|
||||
expect(response_body).to include(I18n.t("login.security_key_authenticate"))
|
||||
expect(response_body).to include(I18n.t("login.security_key_description"))
|
||||
expect(response.body).to include(I18n.t("login.security_key_authenticate"))
|
||||
expect(response.body).to include(I18n.t("login.security_key_description"))
|
||||
end
|
||||
|
||||
context "if the user has a TOTP enabled and wants to use that instead" do
|
||||
@ -214,21 +198,18 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it 'allows entering the totp code instead' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}?show_totp=true"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}?show_totp=true"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
response_body = response.body
|
||||
|
||||
expect(response_body).to include(I18n.t("login.second_factor_title"))
|
||||
expect(response_body).not_to include(I18n.t("login.security_key_authenticate"))
|
||||
expect(response.body).to include(I18n.t("login.second_factor_title"))
|
||||
expect(response.body).not_to include(I18n.t("login.security_key_authenticate"))
|
||||
end
|
||||
end
|
||||
|
||||
it 'adds an error on a security key attempt' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: user.email_tokens.last.token,
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: "{}",
|
||||
second_factor_method: UserSecondFactor.methods[:security_key]
|
||||
}
|
||||
@ -238,26 +219,24 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it 'confirms with a correct security key token' do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: valid_security_key_auth_post_data.to_json,
|
||||
second_factor_method: UserSecondFactor.methods[:security_key],
|
||||
token: user.email_tokens.last.token
|
||||
second_factor_method: UserSecondFactor.methods[:security_key]
|
||||
}
|
||||
|
||||
expect(response.status).to eq(302)
|
||||
|
||||
user.reload
|
||||
expect(user.email).to eq("new.n.cool@example.com")
|
||||
expect(user.reload.email).to eq('bubblegum@adventuretime.ooo')
|
||||
end
|
||||
|
||||
context "if the security key data JSON is garbled" do
|
||||
it "raises an invalid parameters error" do
|
||||
get "/u/confirm-new-email/#{user.email_tokens.last.token}"
|
||||
get "/u/confirm-new-email/#{updater.change_req.new_email_token.token}"
|
||||
put "/u/confirm-new-email", params: {
|
||||
token: updater.change_req.new_email_token.token,
|
||||
second_factor_token: "{someweird: 8notjson}",
|
||||
second_factor_method: UserSecondFactor.methods[:security_key],
|
||||
token: user.email_tokens.last.token
|
||||
second_factor_method: UserSecondFactor.methods[:security_key]
|
||||
}
|
||||
|
||||
expect(response.status).to eq(400)
|
||||
@ -268,9 +247,8 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
describe '#confirm-old-email' do
|
||||
|
||||
it 'redirects to login for signed out accounts' do
|
||||
get "/u/confirm-old-email/asdfasdf"
|
||||
get "/u/confirm-old-email/invalidtoken"
|
||||
|
||||
expect(response.status).to eq(302)
|
||||
expect(response.redirect_url).to eq("http://test.localhost/login")
|
||||
@ -279,75 +257,62 @@ describe UsersEmailController do
|
||||
it 'errors out for invalid tokens' do
|
||||
sign_in(user)
|
||||
|
||||
get "/u/confirm-old-email/asdfasdf"
|
||||
get "/u/confirm-old-email/invalidtoken"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.body).to include(I18n.t('change_email.already_done'))
|
||||
end
|
||||
|
||||
it 'bans change when accounts do not match' do
|
||||
|
||||
sign_in(user)
|
||||
updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
|
||||
updater.change_to('new.n.cool@example.com')
|
||||
email_change_request = updater.change_to('bubblegum@adventuretime.ooo')
|
||||
|
||||
get "/u/confirm-old-email/#{moderator.email_tokens.last.token}"
|
||||
get "/u/confirm-old-email/#{email_change_request.old_email_token.token}"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(body).to include("alert-error")
|
||||
end
|
||||
|
||||
context 'valid old address token' do
|
||||
|
||||
context 'valid old token' do
|
||||
it 'confirms with a correct token' do
|
||||
# NOTE: only moderators need to confirm both old and new
|
||||
sign_in(moderator)
|
||||
updater = EmailUpdater.new(guardian: moderator.guardian, user: moderator)
|
||||
updater.change_to('new.n.cool@example.com')
|
||||
email_change_request = updater.change_to('bubblegum@adventuretime.ooo')
|
||||
|
||||
get "/u/confirm-old-email/#{moderator.email_tokens.last.token}"
|
||||
get "/u/confirm-old-email/#{email_change_request.old_email_token.token}"
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
|
||||
body = CGI.unescapeHTML(response.body)
|
||||
|
||||
expect(body)
|
||||
.to include(I18n.t('change_email.authorizing_old.title'))
|
||||
|
||||
expect(body)
|
||||
.to include(I18n.t('change_email.authorizing_old.description'))
|
||||
expect(body).to include(I18n.t('change_email.authorizing_old.title'))
|
||||
expect(body).to include(I18n.t('change_email.authorizing_old.description'))
|
||||
|
||||
put "/u/confirm-old-email", params: {
|
||||
token: moderator.email_tokens.last.token
|
||||
token: email_change_request.old_email_token.token
|
||||
}
|
||||
|
||||
expect(response.status).to eq(302)
|
||||
expect(response.redirect_url).to include("done=true")
|
||||
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '#create' do
|
||||
let(:new_email) { 'bubblegum@adventuretime.ooo' }
|
||||
|
||||
it 'has an email token' do
|
||||
sign_in(user)
|
||||
|
||||
expect { post "/u/#{user.username}/preferences/email.json", params: { email: new_email } }
|
||||
expect { post "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' } }
|
||||
.to change(EmailChangeRequest, :count)
|
||||
|
||||
emailChangeRequest = EmailChangeRequest.last
|
||||
expect(emailChangeRequest.old_email).to eq(nil)
|
||||
expect(emailChangeRequest.new_email).to eq(new_email)
|
||||
expect(emailChangeRequest.new_email).to eq('bubblegum@adventuretime.ooo')
|
||||
end
|
||||
end
|
||||
|
||||
describe '#update' do
|
||||
let(:new_email) { 'bubblegum@adventuretime.ooo' }
|
||||
|
||||
it "requires you to be logged in" do
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: new_email }
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' }
|
||||
expect(response.status).to eq(403)
|
||||
end
|
||||
|
||||
@ -368,10 +333,9 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it "raises an error if you can't edit the user's email" do
|
||||
Guardian.any_instance.expects(:can_edit_email?).with(user).returns(false)
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: new_email }
|
||||
SiteSetting.email_editable = false
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' }
|
||||
expect(response).to be_forbidden
|
||||
end
|
||||
|
||||
@ -384,18 +348,12 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it 'raises an error' do
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: other_user.email
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email }
|
||||
expect(response).to_not be_successful
|
||||
end
|
||||
|
||||
it 'raises an error if there is whitespace too' do
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: "#{other_user.email} "
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: "#{other_user.email} " }
|
||||
expect(response).to_not be_successful
|
||||
end
|
||||
end
|
||||
@ -406,10 +364,7 @@ describe UsersEmailController do
|
||||
end
|
||||
|
||||
it 'responds with success' do
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: other_user.email
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email }
|
||||
expect(response.status).to eq(200)
|
||||
end
|
||||
end
|
||||
@ -419,10 +374,7 @@ describe UsersEmailController do
|
||||
fab!(:other_user) { Fabricate(:user, email: 'case.insensitive@gmail.com') }
|
||||
|
||||
it 'raises an error' do
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: other_user.email.upcase
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: other_user.email.upcase }
|
||||
expect(response).to_not be_successful
|
||||
end
|
||||
end
|
||||
@ -430,34 +382,24 @@ describe UsersEmailController do
|
||||
it 'raises an error when new email domain is present in blocked_email_domains site setting' do
|
||||
SiteSetting.blocked_email_domains = "mailinator.com"
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: "not_good@mailinator.com"
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: "not_good@mailinator.com" }
|
||||
expect(response).to_not be_successful
|
||||
end
|
||||
|
||||
it 'raises an error when new email domain is not present in allowed_email_domains site setting' do
|
||||
SiteSetting.allowed_email_domains = "discourse.org"
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: new_email
|
||||
}
|
||||
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' }
|
||||
expect(response).to_not be_successful
|
||||
end
|
||||
|
||||
context 'success' do
|
||||
it 'has an email token' do
|
||||
expect do
|
||||
put "/u/#{user.username}/preferences/email.json", params: {
|
||||
email: new_email
|
||||
}
|
||||
put "/u/#{user.username}/preferences/email.json", params: { email: 'bubblegum@adventuretime.ooo' }
|
||||
end.to change(EmailChangeRequest, :count)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
end
|
||||
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user