maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix email confirmation logic when registering via Omniauth (#17878)

Browse source 
* Move .skip_confirmation! call to .find_or_create_user! method

* Add regression tests to avoid email confirmation delivery

* Keep the original place where we had user.skip_confirmation! too

* Update logic to require email confirmation from omniauth

* test confirmation is required with SMTP

* Keep :notice instead of :global_notice

* Forem Account bypass email confirmation reorg

* inline comment reorder + clarification

* Apply suggestions from code review

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

Co-authored-by: Fernando Valverde <fernando@visualcosita.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>
This commit is contained in:
Fernando Valverde 2022-06-23 14:26:26 -06:00 • committed by GitHub
parent 8c836430ca
commit 1eafd7388e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 337 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
app/controllers/confirmations_controller.rb
View file

@ -6,8 +6,9 @@ class ConfirmationsController < Devise::ConfirmationsController
if resource.errors.empty?
set_flash_message!(:notice, :confirmed)
sign_in(resource)
if resource.creator?
sign_in(resource)
redirect_to new_admin_creator_setting_path
else
respond_with_navigational(resource) { redirect_to after_confirmation_path_for(resource_name, resource) }

26
app/controllers/omniauth_callbacks_controller.rb
View file

@ -56,27 +56,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
cta_variant: cta_variant,
)
if user_persisted_and_valid?
if user_persisted_and_valid? && @user.confirmed?
# User is allowed to start onboarding
set_flash_message(:notice, :success, kind: provider.to_s.titleize) if is_navigational_format?
# Devise's Omniauthable does not automatically remember users
# see <https://github.com/heartcombo/devise/wiki/Omniauthable,-sign-out-action-and-rememberable>
remember_me(@user)
set_flash_message(:notice, :success, kind: provider.to_s.titleize) if is_navigational_format?
# `event: authentication` is only needed for Warden callbacks
# see <config/initializers/persistent_csrf_token_cookie.rb>
sign_in_and_redirect(@user, event: :authentication)
# NOTE: I can't find a way to test this path
# as `User` will assign a temporary username if the username already exists
# see https://github.com/forem/forem/blob/27131f6f420df347a467f8e9afc84a6af2fcb13a/app/models/user.rb#L532-L555
elsif user_persisted_but_username_taken?
redirect_to "/settings?state=previous-registration"
# NOTE: I can't find a way to test this path
# as `Authentication::Authenticator.call` invokes `User.save!` which will
# raise errors for a validation error.
# In the past we had 1 path (update_user) which would have ended up
# here in case of validation errors, see:
# https://github.com/forem/forem/blob/80737b540453afe8775128cb37bd379b7c09c7e8/app/services/authorization_service.rb#L77
elsif user_persisted_and_valid?
redirect_to confirm_email_path(email: @user.email)
else
# Devise will clean this data when the user is not persisted
session["devise.#{provider}_data"] = request.env["omniauth.auth"]
@ -109,10 +99,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
@user.persisted? && @user.valid?
end
def user_persisted_but_username_taken?
@user.persisted? && @user.errors_as_sentence.include?(I18n.t("omniauth_callbacks_controller.username_taken"))
end
# We only bypass CSRF checks on Apple callback path & Apple trusted ORIGIN
def safe_apple_callback_request?
trusted_origin = Authentication::Providers::Apple::TRUSTED_CALLBACK_ORIGIN

10
app/services/authentication/authenticator.rb
View file

@ -59,8 +59,6 @@ module Authentication
log_to_datadog = new_identity && successful_save
id_provider = identity.provider
user.skip_confirmation!
flag_spam_user(user) if account_less_than_a_week_old?(user, identity)
user.save!
@ -130,6 +128,7 @@ module Authentication
user.assign_attributes(default_user_fields)
user.set_remember_fields
user.skip_confirmation! unless requires_email_confirmation?
# The user must be saved in the database before
# we assign the user to a new identity.
@ -195,6 +194,13 @@ module Authentication
end
end
# If SMTP is enabled we require email confirmation to start onboarding,
# otherwise we skip this required step because we can't confirm them.
# Forem Account auth doesn't require email confirmation (already confirmed)
def requires_email_confirmation?
ForemInstance.smtp_enabled? && provider.class.name != "Authentication::Providers::Forem"
end
def flag_spam_user(user)
Slack::Messengers::PotentialSpammer.call(user: user)
end

372
spec/services/authentication/authenticator_spec.rb
View file

@ -58,6 +58,18 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.apple_username).to match(/#{info.first_name.downcase}_\w+/)
end
it "persists the user as confirmed when SMTP isn't enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
user = service.call
expect(user.confirmed?).to be(true)
end
it "persists the user as unconfirmed when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed?).to be(false)
end
it "sets default fields" do
user = service.call
@ -81,10 +93,20 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "sets confirmed_at" do
user = service.call
it "sets confirmed_at and doesn't deliver email without SMTP enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
expect(user.confirmed_at).to be_present
sidekiq_assert_no_enqueued_jobs(only: Devise.mailer.delivery_job) do
user = service.call
expect(user.confirmed_at).to be_present
end
end
it "sets confirmed_at as nil when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed_at).to be_nil
end
it "queues a slack message to be sent for a user whose identity is brand new" do
@ -179,19 +201,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "updates confirmed_at with the current UTC time" do
original_confirmed_at = user.confirmed_at
Timecop.travel(1.minute.from_now) do
service.call
end
user.reload
expect(
user.confirmed_at.utc.to_i > original_confirmed_at.utc.to_i,
).to be(true)
end
it "updates the username when it is changed on the provider" do
new_username = "new_username#{rand(1000)}"
auth_payload.info.first_name = new_username
@ -261,6 +270,18 @@ RSpec.describe Authentication::Authenticator, type: :service do
end.to change(Identity, :count).by(1)
end
it "persists the user as confirmed when SMTP isn't enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
user = service.call
expect(user.confirmed?).to be(true)
end
it "persists the user as unconfirmed when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed?).to be(false)
end
it "extracts the proper data from the auth payload" do
user = service.call
@ -296,10 +317,20 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "sets confirmed_at" do
user = service.call
it "sets confirmed_at and doesn't deliver email without SMTP enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
expect(user.confirmed_at).to be_present
sidekiq_assert_no_enqueued_jobs(only: Devise.mailer.delivery_job) do
user = service.call
expect(user.confirmed_at).to be_present
end
end
it "sets confirmed_at as nil when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed_at).to be_nil
end
it "queues a slack message to be sent for a user whose identity is brand new" do
@ -379,19 +410,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "updates confirmed_at with the current UTC time" do
original_confirmed_at = user.confirmed_at
Timecop.travel(1.minute.from_now) do
service.call
end
user.reload
expect(
user.confirmed_at.utc.to_i > original_confirmed_at.utc.to_i,
).to be(true)
end
it "updates the username when it is changed on the provider" do
new_username = "new_username#{rand(1000)}"
auth_payload.info.nickname = new_username
@ -473,6 +491,18 @@ RSpec.describe Authentication::Authenticator, type: :service do
end.to change(Identity, :count).by(1)
end
it "persists the user as confirmed when SMTP isn't enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
user = service.call
expect(user.confirmed?).to be(true)
end
it "persists the user as unconfirmed when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed?).to be(false)
end
it "extracts the proper data from the auth payload" do
user = service.call
@ -508,10 +538,20 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "sets confirmed_at" do
user = service.call
it "sets confirmed_at and doesn't deliver email without SMTP enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
expect(user.confirmed_at).to be_present
sidekiq_assert_no_enqueued_jobs(only: Devise.mailer.delivery_job) do
user = service.call
expect(user.confirmed_at).to be_present
end
end
it "sets confirmed_at as nil when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed_at).to be_nil
end
it "queues a slack message to be sent for a user whose identity is brand new" do
@ -564,6 +604,18 @@ RSpec.describe Authentication::Authenticator, type: :service do
end.to change(Identity, :count).by(1)
end
it "persists the user as confirmed when SMTP isn't enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
user = service.call
expect(user.confirmed?).to be(true)
end
it "persists the user as unconfirmed when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed?).to be(false)
end
it "extracts the proper data from the auth payload" do
user = service.call
@ -599,10 +651,20 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "sets confirmed_at" do
user = service.call
it "sets confirmed_at and doesn't deliver email without SMTP enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
expect(user.confirmed_at).to be_present
sidekiq_assert_no_enqueued_jobs(only: Devise.mailer.delivery_job) do
user = service.call
expect(user.confirmed_at).to be_present
end
end
it "sets confirmed_at as nil when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed_at).to be_nil
end
it "queues a slack message to be sent for a user whose identity is brand new" do
@ -679,19 +741,6 @@ RSpec.describe Authentication::Authenticator, type: :service do
expect(user.remember_created_at).to be_present
end
it "updates confirmed_at with the current UTC time" do
original_confirmed_at = user.confirmed_at
Timecop.travel(1.minute.from_now) do
service.call
end
user.reload
expect(
user.confirmed_at.utc.to_i > original_confirmed_at.utc.to_i,
).to be(true)
end
it "updates the username when it is changed on the provider" do
new_username = "new_username#{rand(1000)}"
auth_payload.info.nickname = new_username
@ -741,4 +790,225 @@ RSpec.describe Authentication::Authenticator, type: :service do
end
end
end
context "when authenticating through Forem Account" do
let!(:auth_payload) { OmniAuth.config.mock_auth[:forem] }
let!(:service) { described_class.new(auth_payload) }
include_context "spam handling"
describe "new user" do
it "creates a new user" do
expect do
service.call
end.to change(User, :count).by(1)
end
it "creates a new identity" do
expect do
service.call
end.to change(Identity, :count).by(1)
end
it "persists the user as confirmed when SMTP isn't enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
user = service.call
expect(user.confirmed?).to be(true)
end
it "persists the user as confirmed (special provider provider) when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed?).to be(true)
end
it "extracts the proper data from the auth payload", :aggregate_failures do
user = service.call
info = auth_payload.info
raw_info = auth_payload.extra.raw_info
expect(user.email).to eq(info.email)
expect(user.name).to eq(raw_info.name)
expect(user.remote_profile_image_url).to eq(info.image)
expect(user.forem_username).to eq(info.user_nickname)
end
it "sets default fields", :aggregate_failures do
user = service.call
expect(user.password).to be_present
expect(user.signup_cta_variant).to be_nil
expect(user.saw_onboarding).to be(false)
expect(user.setting.editor_version).to eq("v2")
end
it "sets the correct sign up cta variant" do
user = described_class.call(auth_payload, cta_variant: "awesome")
expect(user.signup_cta_variant).to eq("awesome")
end
it "sets remember_me for the new user", :aggregate_failures do
user = service.call
expect(user.remember_me).to be(true)
expect(user.remember_token).to be_present
expect(user.remember_created_at).to be_present
end
it "sets confirmed_at and doesn't deliver email without SMTP enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(false)
sidekiq_assert_no_enqueued_jobs(only: Devise.mailer.delivery_job) do
user = service.call
expect(user.confirmed_at).to be_present
end
end
it "sets confirmed_at (special provider) when SMTP is enabled" do
allow(ForemInstance).to receive(:smtp_enabled?).and_return(true)
user = service.call
expect(user.confirmed_at).to be_present
end
it "queues a slack message to be sent for a user whose identity is brand new" do
auth_payload.extra.raw_info.created_at = 1.minute.ago.rfc3339
sidekiq_assert_enqueued_with(job: Slack::Messengers::Worker) do
described_class.call(auth_payload)
end
end
it "records successful identity creation metric" do
allow(ForemStatsClient).to receive(:increment)
service.call
expect(ForemStatsClient).to have_received(:increment).with(
"identity.created", tags: ["provider:forem"]
)
end
it "increments identity.errors if any errors occur in the transaction", :aggregate_failures do
# rubocop:disable RSpec/AnyInstance
allow_any_instance_of(Identity).to receive(:save!).and_raise(StandardError)
# rubocop:enable RSpec/AnyInstance
allow(ForemStatsClient).to receive(:increment)
expect { described_class.call(auth_payload) }.to raise_error(StandardError)
tags = hash_including(tags: array_including("error:StandardError"))
expect(ForemStatsClient).to have_received(:increment).with("identity.errors", tags)
end
end
describe "existing user" do
let(:user) { create(:user, :with_identity, identities: [:forem]) }
before do
auth_payload.info.email = user.email
end
it "doesn't create a new user" do
expect do
service.call
end.not_to change(User, :count)
end
it "creates a new identity if the user doesn't have one" do
user = create(:user)
auth_payload.info.email = user.email
auth_payload.uid = "#{user.email}-#{rand(10_000)}"
expect do
described_class.call(auth_payload)
end.to change(Identity, :count).by(1)
end
it "does not create a new identity if the user has one" do
expect do
service.call
end.not_to change(Identity, :count)
end
it "does not record an identity creation metric" do
allow(ForemStatsClient).to receive(:increment)
service.call
expect(ForemStatsClient).not_to have_received(:increment)
end
it "sets remember_me for the existing user", :aggregate_failures do
user.update_columns(remember_token: nil, remember_created_at: nil)
service.call
user.reload
expect(user.remember_me).to be(true)
expect(user.remember_token).to be_present
expect(user.remember_created_at).to be_present
end
it "updates the username when it is changed on the provider" do
new_username = "new_username#{rand(1000)}"
auth_payload.info.user_nickname = new_username
user = described_class.call(auth_payload)
expect(user.forem_username).to eq(new_username)
end
it "updates profile_updated_at when the username is changed" do
original_profile_updated_at = user.profile_updated_at
new_username = "new_username#{rand(1000)}"
auth_payload.info.user_nickname = new_username
Timecop.travel(1.minute.from_now) do
described_class.call(auth_payload)
end
user.reload
expect(
user.profile_updated_at.to_i > original_profile_updated_at.to_i,
).to be(true)
end
it "increments identity.errors if any errors occur in the transaction", :aggregate_failures do
# rubocop:disable RSpec/AnyInstance
allow_any_instance_of(Identity).to receive(:save!).and_raise(StandardError)
# rubocop:enable RSpec/AnyInstance
allow(ForemStatsClient).to receive(:increment)
expect { described_class.call(auth_payload) }.to raise_error(StandardError)
tags = hash_including(tags: array_including("error:StandardError"))
expect(ForemStatsClient).to have_received(:increment).with("identity.errors", tags)
end
it "does not update their forem_username if the user is suspended" do
new_username = "new_username#{rand(1000)}"
auth_payload.info.nickname = new_username
user.add_role :suspended
user = described_class.call(auth_payload)
expect(user.forem_username).not_to eq(new_username)
end
end
describe "user already logged in" do
it "returns the current user if the identity exists" do
user = create(:user, :with_identity, identities: [:forem])
expect(described_class.call(auth_payload, current_user: user)).to eq(user)
end
it "creates the identity if for any reason it does not exist" do
user = create(:user)
expect do
described_class.call(auth_payload, current_user: user)
end.to change(Identity, :count).by(1)
end
end
end
end