diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index c802e4694..ae470ac85 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -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) } diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 8f05d4bb4..be0f52c1c 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -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 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 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 diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb index b93767262..d8867f6f6 100644 --- a/app/services/authentication/authenticator.rb +++ b/app/services/authentication/authenticator.rb @@ -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 diff --git a/spec/services/authentication/authenticator_spec.rb b/spec/services/authentication/authenticator_spec.rb index 73995e938..6590e8758 100644 --- a/spec/services/authentication/authenticator_spec.rb +++ b/spec/services/authentication/authenticator_spec.rb @@ -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