diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index a33009bbd..1171d8618 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -1,5 +1,4 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController - # Don't need a policy for this since this is our sign up/in route include Devise::Controllers::Rememberable # Called upon successful redirect from Twitter @@ -36,22 +35,25 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController private - # TODO: [thepracticaldev/oss] investigate if there's a way to simplify this - # by using Devise+OmniAuth builtin integration - # def callback_for(provider) + auth_payload = request.env["omniauth.auth"] cta_variant = request.env["omniauth.params"]["state"].to_s + @user = Authentication::Authenticator.call( - request.env["omniauth.auth"], + auth_payload, current_user: current_user, cta_variant: cta_variant, ) if user_persisted_and_valid? + # Devise's Omniauthable does not automatically remember users + # see remember_me(@user) - set_flash_message(:notice, :success, kind: provider.to_s.capitalize) if is_navigational_format? + 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 @@ -65,6 +67,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # here in case of validation errors, see: # https://github.com/thepracticaldev/dev.to/blob/80737b540453afe8775128cb37bd379b7c09c7e8/app/services/authorization_service.rb#L77 else + # Devise will clean this data when the user is not persisted session["devise.#{provider}_data"] = request.env["omniauth.auth"] user_errors = @user.errors.full_messages diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 7ef02297a..edf06bce5 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -1,10 +1,9 @@ class RegistrationsController < Devise::RegistrationsController prepend_before_action :require_no_authentication, only: [] - # No authorization required for public registration route def new if user_signed_in? - redirect_to "/dashboard?signed-in-already&t=#{Time.current.to_i}" + redirect_to dashboard_path else super end diff --git a/app/helpers/authentication_helper.rb b/app/helpers/authentication_helper.rb new file mode 100644 index 000000000..3113fb229 --- /dev/null +++ b/app/helpers/authentication_helper.rb @@ -0,0 +1,11 @@ +module AuthenticationHelper + def authentication_provider(provider_name) + Authentication::Providers.get!(provider_name) + end + + def authentication_enabled_providers + Authentication::Providers.enabled.map do |provider_name| + Authentication::Providers.get!(provider_name) + end + end +end diff --git a/app/services/authentication/paths.rb b/app/services/authentication/paths.rb new file mode 100644 index 000000000..223141753 --- /dev/null +++ b/app/services/authentication/paths.rb @@ -0,0 +1,23 @@ +module Authentication + # These are meant to be called from the specific providers + module Paths + # Returns the authentication path for the given provider + def self.authentication_path(provider_name, params = {}) + Rails.application.routes.url_helpers.public_send( + "user_#{provider_name}_omniauth_authorize_path", params + ) + end + + # Returns the sign in URL for the given provider + def self.sign_in_path(provider_name, params = {}) + url_helpers = Rails.application.routes.url_helpers + + callback_url_helper = "user_#{provider_name}_omniauth_callback_path" + mandatory_params = { + callback_url: URL.url(url_helpers.public_send(callback_url_helper)) + } + + authentication_path(provider_name, params.merge(mandatory_params)) + end + end +end diff --git a/app/services/authentication/providers.rb b/app/services/authentication/providers.rb index 50c63e69a..83dd07228 100644 --- a/app/services/authentication/providers.rb +++ b/app/services/authentication/providers.rb @@ -11,7 +11,7 @@ module Authentication def self.get!(provider_name) name = provider_name.to_s.titleize - unless Authentication::Providers.const_defined?(name) + unless available?(provider_name) raise( ::Authentication::Errors::ProviderNotFound, "Provider #{name} is not available!", @@ -28,14 +28,16 @@ module Authentication Authentication::Providers.const_get(name) end - # Returns available providers def self.available - # the magic is done in Authentication::Providers::Provider.subclasses.map do |subclass| subclass.name.demodulize.downcase.to_sym end.sort end + def self.available?(provider_name) + Authentication::Providers.const_defined?(provider_name.to_s.titleize) + end + # Returns enabled providers # TODO: [thepracticaldev/oss] ideally this should be "available - disabled" # we can get there once we have feature flags @@ -43,28 +45,8 @@ module Authentication SiteConfig.authentication_providers.map(&:to_sym).sort end - # Returns true if a provider is enabled, false otherwise def self.enabled?(provider_name) enabled.include?(provider_name.to_sym) end - - # Returns the authentication path for the given provider - def self.authentication_path(provider_name, params = {}) - Rails.application.routes.url_helpers.public_send( - "user_#{provider_name}_omniauth_authorize_path", params - ) - end - - # Returns the sign in URL for the given provider - def self.sign_in_path(provider_name, params = {}) - url_helpers = Rails.application.routes.url_helpers - - callback_url_helper = "user_#{provider_name}_omniauth_callback_path" - mandatory_params = { - callback_url: URL.url(url_helpers.public_send(callback_url_helper)) - } - - authentication_path(provider_name, params.merge(mandatory_params)) - end end end diff --git a/app/services/authentication/providers/github.rb b/app/services/authentication/providers/github.rb index 6d7fecea6..c2ebe7239 100644 --- a/app/services/authentication/providers/github.rb +++ b/app/services/authentication/providers/github.rb @@ -29,6 +29,13 @@ module Authentication OFFICIAL_NAME end + def self.sign_in_path(params = {}) + ::Authentication::Paths.sign_in_path( + provider_name, + params, + ) + end + protected def cleanup_payload(auth_payload) diff --git a/app/services/authentication/providers/provider.rb b/app/services/authentication/providers/provider.rb index 90d383074..c22d4a4ce 100644 --- a/app/services/authentication/providers/provider.rb +++ b/app/services/authentication/providers/provider.rb @@ -41,11 +41,23 @@ module Authentication self.class::USERNAME_FIELD end + def self.provider_name + name.demodulize.downcase.to_sym + end + # Returns the official name of the authentication provider def self.official_name raise SubclassResponsibility end + def self.authentication_path(params = {}) + ::Authentication::Paths.authentication_path(provider_name, params) + end + + def self.sign_in_path(_params = {}) + raise SubclassResponsibility + end + protected # Remove sensible data from the payload diff --git a/app/services/authentication/providers/twitter.rb b/app/services/authentication/providers/twitter.rb index a823bfc5c..e2c4bbcf0 100644 --- a/app/services/authentication/providers/twitter.rb +++ b/app/services/authentication/providers/twitter.rb @@ -34,6 +34,16 @@ module Authentication OFFICIAL_NAME end + def self.sign_in_path(params = {}) + # see https://github.com/arunagw/omniauth-twitter#authentication-options + mandatory_params = { secure_image_url: true } + + ::Authentication::Paths.sign_in_path( + provider_name, + params.merge(mandatory_params), + ) + end + protected def cleanup_payload(auth_payload) diff --git a/app/views/shared/authentication/_providers_nav_menu.html.erb b/app/views/shared/authentication/_providers_nav_menu.html.erb index 06ddbefff..a95c35f82 100644 --- a/app/views/shared/authentication/_providers_nav_menu.html.erb +++ b/app/views/shared/authentication/_providers_nav_menu.html.erb @@ -1,8 +1,6 @@ -<% Authentication::Providers.enabled.each do |provider_name| %> - <% provider = Authentication::Providers.get!(provider_name) %> - +<% authentication_enabled_providers.each do |provider| %> " + href="<%= provider.sign_in_path(state: "navbar_basic") %>" class="crayons-link crayons-link--block pl-5" data-no-instant> Via <%= provider.official_name %> diff --git a/app/views/shared/authentication/_providers_registration_form.html.erb b/app/views/shared/authentication/_providers_registration_form.html.erb index b24f52461..970ffa4e0 100644 --- a/app/views/shared/authentication/_providers_registration_form.html.erb +++ b/app/views/shared/authentication/_providers_registration_form.html.erb @@ -1,10 +1,8 @@ -<% Authentication::Providers.enabled.each do |provider_name| %> - <% provider = Authentication::Providers.get!(provider_name) %> - +<% authentication_enabled_providers.each do |provider| %> " + href="<%= provider.sign_in_path(state: "join-club-page_basic") %>" class="sign-up-link" data-no-instant> - <%= inline_svg_tag("#{provider_name}-logo.svg", class: "icon-img", aria: true, title: "#{provider_name} logo") %> Sign In with <%= provider.official_name %> + <%= inline_svg_tag("#{provider.provider_name}-logo.svg", class: "icon-img", aria: true, title: "#{provider.name} logo") %> Sign In with <%= provider.official_name %> <% end %> diff --git a/app/views/shared/authentication/_providers_sidebar.html.erb b/app/views/shared/authentication/_providers_sidebar.html.erb index e09a67f78..f8d3ba5ab 100644 --- a/app/views/shared/authentication/_providers_sidebar.html.erb +++ b/app/views/shared/authentication/_providers_sidebar.html.erb @@ -1,8 +1,6 @@ -<% Authentication::Providers.enabled.each do |provider_name| %> - <% provider = Authentication::Providers.get!(provider_name) %> - +<% authentication_enabled_providers.each do |provider| %> " + href="<%= provider.sign_in_path(state: "navbar_basic") %>" class="cta cta-button login-cta-button" data-no-instant> Sign In With <%= provider.official_name %> diff --git a/app/views/shared/authentication/_providers_signup_modal.html.erb b/app/views/shared/authentication/_providers_signup_modal.html.erb index 640ad49f5..1a1e24a27 100644 --- a/app/views/shared/authentication/_providers_signup_modal.html.erb +++ b/app/views/shared/authentication/_providers_signup_modal.html.erb @@ -1,13 +1,11 @@ -<% Authentication::Providers.enabled.each do |provider_name| %> - <% provider = Authentication::Providers.get!(provider_name) %> - +<% authentication_enabled_providers.each do |provider| %> " + href="<%= provider.sign_in_path(state: "signup-modal") %>" class="sign-up-link cta" data-no-instant> " + src="<%= asset_path("#{provider.provider_name}-logo.svg") %>" class="icon-img" - alt="<%= provider.official_name %> logo" /> Auth With <%= provider.official_name %> + alt="<%= provider.official_name %> logo" /> Sign In With <%= provider.official_name %> <% end %> diff --git a/app/views/users/_additional_authentication.html.erb b/app/views/users/_additional_authentication.html.erb index fa8d86c95..060597c6d 100644 --- a/app/views/users/_additional_authentication.html.erb +++ b/app/views/users/_additional_authentication.html.erb @@ -1,15 +1,14 @@ -<% Authentication::Providers.enabled.each do |provider_name| %> - <% unless @user.identities.exists?(provider: provider_name) %> - <% provider = Authentication::Providers.get!(provider_name) %> +<% authentication_enabled_providers.each do |provider| %> + <% unless @user.identities.exists?(provider: provider.provider_name) %> <%= inline_svg_tag( - "#{provider_name}.svg", + "#{provider.provider_name}.svg", aria: true, class: "crayons-icon", - title: provider_name.to_s.titleize.to_s, + title: provider.official_name, ) %> Connect <%= provider.official_name %> Account diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index b07e1f643..2c78a0fb8 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -235,6 +235,8 @@ Devise.setup do |config| # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' + config.omniauth :github, ApplicationConfig["GITHUB_KEY"], ApplicationConfig["GITHUB_SECRET"], scope: "user:email" + config.omniauth :twitter, ApplicationConfig["TWITTER_KEY"], ApplicationConfig["TWITTER_SECRET"] # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or @@ -258,8 +260,4 @@ Devise.setup do |config| # When using OmniAuth, Devise cannot automatically set OmniAuth path, # so you need to do it manually. For the users scope, it would be: # config.omniauth_path_prefix = '/my_engine/users/auth' - - config.omniauth :github, - ApplicationConfig["GITHUB_KEY"], ApplicationConfig["GITHUB_SECRET"], scope: "user:email" - config.omniauth :twitter, ApplicationConfig["TWITTER_KEY"], ApplicationConfig["TWITTER_SECRET"] end diff --git a/config/initializers/persistent_csrf_token_cookie.rb b/config/initializers/persistent_csrf_token_cookie.rb index 2bbf9773d..a0306ee32 100644 --- a/config/initializers/persistent_csrf_token_cookie.rb +++ b/config/initializers/persistent_csrf_token_cookie.rb @@ -1,5 +1,3 @@ -# config/initializers/monkey_patches/persistent_csrf_token_cookie.rb -# # Workaround for CSRF protection bug. # # https://github.com/rails/rails/issues/21948 diff --git a/config/routes.rb b/config/routes.rb index 2c0b3a5c4..0db835cef 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -10,6 +10,11 @@ Rails.application.routes.draw do registrations: "registrations" } + devise_scope :user do + get "/enter", to: "registrations#new", as: :sign_up + delete "/sign_out", to: "devise/sessions#destroy" + end + require "sidekiq/web" require "sidekiq_unique_jobs/web" @@ -23,11 +28,6 @@ Rails.application.routes.draw do mount FieldTest::Engine, at: "abtests" end - devise_scope :user do - delete "/sign_out" => "devise/sessions#destroy" - get "/enter" => "registrations#new", :as => :sign_up - end - namespace :admin do # Check administrate gem docs DashboardManifest::DASHBOARDS.each do |dashboard_resource| diff --git a/spec/labor/badge_rewarder_spec.rb b/spec/labor/badge_rewarder_spec.rb index 05de77d58..8fe0455df 100644 --- a/spec/labor/badge_rewarder_spec.rb +++ b/spec/labor/badge_rewarder_spec.rb @@ -113,7 +113,7 @@ RSpec.describe BadgeRewarder, type: :labor do end before do - mock_github + omniauth_mock_github_payload allow(Octokit::Client).to receive(:new).and_return(my_ocktokit_client) allow(my_ocktokit_client).to receive(:commits).and_return(stubbed_github_commit) create(:badge, title: "DEV Contributor") diff --git a/spec/models/github_repo_spec.rb b/spec/models/github_repo_spec.rb index 61ee5839b..5f4c83a2b 100644 --- a/spec/models/github_repo_spec.rb +++ b/spec/models/github_repo_spec.rb @@ -4,7 +4,7 @@ RSpec.describe GithubRepo, type: :model do let(:user) { create(:user, :with_identity, identities: ["github"]) } let(:repo) { build(:github_repo, user_id: user.id) } - before { mock_github } + before { omniauth_mock_github_payload } it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:url) } diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 619f9302b..3ded4962d 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -27,7 +27,7 @@ RSpec.describe Identity, type: :model do let(:user) { create(:user) } before do - mock_auth_hash + omniauth_mock_providers_payload end context "with Github payload" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f96343154..966d5db4f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -15,7 +15,7 @@ RSpec.describe User, type: :model do let(:user_with_user_optional_fields) { create(:user, :with_user_optional_fields) } let(:org) { create(:organization) } - before { mock_auth_hash } + before { omniauth_mock_providers_payload } describe "validations" do describe "builtin validations" do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index f2a368707..bb2ba5278 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -67,7 +67,7 @@ RSpec.configure do |config| config.include Devise::Test::IntegrationHelpers, type: :system config.include Devise::Test::IntegrationHelpers, type: :request config.include FactoryBot::Syntax::Methods - config.include OmniauthMacros + config.include OmniauthHelpers config.include SidekiqTestHelpers config.include ElasticsearchHelpers diff --git a/spec/requests/github_repos_spec.rb b/spec/requests/github_repos_spec.rb index 9d5f66cd5..22ae06456 100644 --- a/spec/requests/github_repos_spec.rb +++ b/spec/requests/github_repos_spec.rb @@ -19,7 +19,7 @@ RSpec.describe "GithubRepos", type: :request do end before do - mock_github + omniauth_mock_github_payload allow(Octokit::Client).to receive(:new).and_return(my_octokit_client) allow(my_octokit_client).to receive(:repositories) { stubbed_github_repos } end diff --git a/spec/requests/internal/users_spec.rb b/spec/requests/internal/users_spec.rb index bcf2e5ea6..4a157e526 100644 --- a/spec/requests/internal/users_spec.rb +++ b/spec/requests/internal/users_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" RSpec.describe "internal/users", type: :request do let!(:user) do - mock_github + omniauth_mock_github_payload create(:user, :with_identity, identities: ["github"]) end let(:admin) { create(:user, :super_admin) } diff --git a/spec/requests/registration_spec.rb b/spec/requests/registrations_spec.rb similarity index 71% rename from spec/requests/registration_spec.rb rename to spec/requests/registrations_spec.rb index c8d160712..b890ad1ee 100644 --- a/spec/requests/registration_spec.rb +++ b/spec/requests/registrations_spec.rb @@ -15,10 +15,8 @@ RSpec.describe "Registrations", type: :request do it "redirects to /dashboard" do sign_in user - Timecop.freeze(Time.current) do - get "/enter" - expect(response).to redirect_to("/dashboard?signed-in-already&t=#{Time.current.to_i}") - end + get "/enter" + expect(response).to redirect_to("/dashboard") end end end diff --git a/spec/requests/user/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb index 06a6bc034..504e54bf6 100644 --- a/spec/requests/user/user_settings_spec.rb +++ b/spec/requests/user/user_settings_spec.rb @@ -79,7 +79,7 @@ RSpec.describe "UserSettings", type: :request do describe "connect providers accounts" do before do - mock_auth_hash + omniauth_mock_providers_payload end it "does not render the text for the enabled provider the user has an identity for" do @@ -296,7 +296,7 @@ RSpec.describe "UserSettings", type: :request do let(:user) { create(:user, :with_identity, identities: %w[github twitter]) } before do - mock_auth_hash + omniauth_mock_providers_payload sign_in user end diff --git a/spec/services/authentication/authenticator_spec.rb b/spec/services/authentication/authenticator_spec.rb index 31dbd4104..448c33cf8 100644 --- a/spec/services/authentication/authenticator_spec.rb +++ b/spec/services/authentication/authenticator_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Authentication::Authenticator, type: :service do - before { mock_auth_hash } + before { omniauth_mock_providers_payload } context "when authenticating through an unknown provider" do it "raises ProviderNotFound" do diff --git a/spec/services/authentication/providers/github_spec.rb b/spec/services/authentication/providers/github_spec.rb new file mode 100644 index 000000000..a06950f2e --- /dev/null +++ b/spec/services/authentication/providers/github_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +RSpec.describe Authentication::Providers::Github, type: :service do + describe ".authentication_path" do + it "returns the correct authentication path" do + expected_path = Rails.application.routes.url_helpers.user_github_omniauth_authorize_path + expect(described_class.authentication_path).to eq(expected_path) + end + + it "supports additional parameters" do + path = described_class.authentication_path(state: "state") + expect(path).to include("state=state") + end + end + + describe ".sign_in_path" do + let(:expected_path) do + "/users/auth/github?callback_url=http%3A%2F%2Flocalhost%3A3000%2Fusers%2Fauth%2Fgithub%2Fcallback" + end + + it "returns the correct sign in path" do + expect(described_class.sign_in_path).to eq(expected_path) + end + + it "supports additional parameters" do + path = described_class.sign_in_path(state: "state") + expect(path).to include("state=state") + end + + it "does not override the callback_url parameter" do + path = described_class.sign_in_path(callback_url: "https://example.com/callback") + expect(path).to eq(expected_path) + end + end +end diff --git a/spec/services/authentication/providers/twitter_spec.rb b/spec/services/authentication/providers/twitter_spec.rb new file mode 100644 index 000000000..9781e7b3e --- /dev/null +++ b/spec/services/authentication/providers/twitter_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +RSpec.describe Authentication::Providers::Twitter, type: :service do + describe ".authentication_path" do + it "returns the correct authentication path" do + expected_path = Rails.application.routes.url_helpers.user_twitter_omniauth_authorize_path + expect(described_class.authentication_path).to eq(expected_path) + end + + it "supports additional parameters" do + path = described_class.authentication_path(state: "state") + expect(path).to include("state=state") + end + end + + describe ".sign_in_path" do + let(:expected_path) do + "/users/auth/twitter?callback_url=http%3A%2F%2Flocalhost%3A3000%2Fusers%2Fauth%2Ftwitter%2Fcallback&secure_image_url=true" + end + + it "returns the correct sign in path" do + expect(described_class.sign_in_path).to eq(expected_path) + end + + it "supports additional parameters" do + path = described_class.sign_in_path(state: "state") + expect(path).to include("state=state") + end + + it "does not override the callback_url parameter" do + path = described_class.sign_in_path(callback_url: "https://example.com/callback") + expect(path).to eq(expected_path) + end + + it "does not override the secure_image_url parameter" do + path = described_class.sign_in_path(secure_image_url: "https://dummyimage.com/300") + expect(path).to eq(expected_path) + end + end +end diff --git a/spec/services/authentication/providers_spec.rb b/spec/services/authentication/providers_spec.rb index 3de70558f..dc1b5776a 100644 --- a/spec/services/authentication/providers_spec.rb +++ b/spec/services/authentication/providers_spec.rb @@ -58,36 +58,4 @@ RSpec.describe Authentication::Providers, type: :service do expect(described_class.enabled?(:github)).to be(false) end end - - describe ".authentication_path" do - it "returns the correct authentication path for given provider" do - expected_path = Rails.application.routes.url_helpers.user_github_omniauth_authorize_path - expect(described_class.authentication_path(:github)).to eq(expected_path) - end - - it "supports additional parameters" do - path = described_class.authentication_path(:github, state: "state") - expect(path).to include("state=state") - end - end - - describe ".sign_in_path" do - let(:expected_path) do - "/users/auth/github?callback_url=http%3A%2F%2Flocalhost%3A3000%2Fusers%2Fauth%2Fgithub%2Fcallback" - end - - it "returns the correct sign in URL for given provider" do - expect(described_class.sign_in_path(:github)).to eq(expected_path) - end - - it "supports additional parameters" do - path = described_class.sign_in_path(:github, state: "state") - expect(path).to include("state=state") - end - - it "does not override the callback_url parameter" do - path = described_class.sign_in_path(:github, callback_url: "https://example.com/callback") - expect(path).to eq(expected_path) - end - end end diff --git a/spec/services/broadcasts/welcome_notification/generator_spec.rb b/spec/services/broadcasts/welcome_notification/generator_spec.rb index 1039a5362..61301e455 100644 --- a/spec/services/broadcasts/welcome_notification/generator_spec.rb +++ b/spec/services/broadcasts/welcome_notification/generator_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do let_it_be_readonly(:customize_ux_broadcast) { create(:customize_ux_broadcast) } before do - mock_auth_hash + omniauth_mock_providers_payload allow(Notification).to receive(:send_welcome_notification).and_call_original allow(User).to receive(:mascot_account).and_return(mascot_account) SiteConfig.staff_user_id = mascot_account.id @@ -143,7 +143,7 @@ RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do describe "#send_feed_customization_notification" do let!(:user) do - mock_auth_hash + omniauth_mock_providers_payload create(:user, :with_identity, identities: %w[twitter github], created_at: 3.days.ago) end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 524d8de38..e8df1d7a6 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Users::Delete, type: :service do - before { mock_github } + before { omniauth_mock_github_payload } let(:user) { create(:user, :with_identity, identities: ["github"]) } diff --git a/spec/support/omniauth_macros.rb b/spec/support/omniauth_helpers.rb similarity index 68% rename from spec/support/omniauth_macros.rb rename to spec/support/omniauth_helpers.rb index 4d3f397c0..61088cfb2 100644 --- a/spec/support/omniauth_macros.rb +++ b/spec/support/omniauth_helpers.rb @@ -1,7 +1,7 @@ -module OmniauthMacros +module OmniauthHelpers OMNIAUTH_DEFAULT_FAILURE_HANDLER = OmniAuth.config.on_failure - INFO = OmniAuth::AuthHash::InfoHash.new( + OMNIAUTH_INFO = OmniAuth::AuthHash::InfoHash.new( first_name: "fname", last_name: "lname", location: "location,state,country", @@ -11,8 +11,8 @@ module OmniauthMacros verified: true, ) - EXTRA_INFO = Hashie::Mash.new( - raw_info: Hashie::Mash.new( + OMNIAUTH_EXTRA_INFO = OmniAuth::AuthHash::InfoHash.new( + raw_info: OmniAuth::AuthHash::InfoHash.new( email: "yourname@email.com", first_name: "fname", gender: "female", @@ -32,23 +32,21 @@ module OmniauthMacros ), ) - CREDENTIAL = OmniAuth::AuthHash::InfoHash.new( - token: "2735246777-jlOnuFlGlvybuwDJfyrIyESLUEgoo6CffyJCQUO", - secret: "o0cu6ACtypMQfLyWhme3Vj99uSds7rjr4szuuTiykSYcN", - ) - - BASIC_INFO = { + OMNIAUTH_BASIC_INFO = { uid: "1234567", - info: INFO, - extra: EXTRA_INFO, - credentials: CREDENTIAL + info: OMNIAUTH_INFO, + extra: OMNIAUTH_EXTRA_INFO, + credentials: { + token: SecureRandom.hex, + secret: SecureRandom.hex + } }.freeze - def mock_auth_with_invalid_credentials(provider) + def omniauth_setup_invalid_credentials(provider) OmniAuth.config.mock_auth[provider] = :invalid_credentials end - def setup_omniauth_error(error) + def omniauth_setup_authentication_error(error) # this hack is needed due to a limitation in how OmniAuth handles # failures in mocked/testing environments, # see @@ -82,39 +80,46 @@ module OmniauthMacros ] end - def mock_auth_hash - mock_twitter - mock_github + def omniauth_mock_providers_payload + Authentication::Providers.available.each do |provider_name| + public_send("omniauth_mock_#{provider_name}_payload") + end end - def mock_twitter - info = BASIC_INFO[:info].merge( + def omniauth_reset_mock + Authentication::Providers.available.each do |provider_name| + OmniAuth.config.mock_auth[provider_name] = nil + end + end + + def omniauth_mock_github_payload + info = OMNIAUTH_BASIC_INFO[:info].merge( + image: "https://dummyimage.com/400x400.jpg", + ) + + OmniAuth.config.mock_auth[:github] = OmniAuth::AuthHash.new( + OMNIAUTH_BASIC_INFO.merge( + provider: "github", + info: info, + ), + ) + end + + def omniauth_mock_twitter_payload + info = OMNIAUTH_BASIC_INFO[:info].merge( image: "https://dummyimage.com/400x400_normal.jpg", ) - extra = BASIC_INFO[:extra].merge( + extra = OMNIAUTH_BASIC_INFO[:extra].merge( access_token: "value", ) OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new( - BASIC_INFO.merge( + OMNIAUTH_BASIC_INFO.merge( provider: "twitter", info: info, extra: extra, ), ) end - - def mock_github - info = BASIC_INFO[:info].merge( - image: "https://dummyimage.com/400x400.jpg", - ) - - OmniAuth.config.mock_auth[:github] = OmniAuth::AuthHash.new( - BASIC_INFO.merge( - provider: "github", - info: info, - ), - ) - end end diff --git a/spec/system/authentication/user_logs_in_with_github_spec.rb b/spec/system/authentication/user_logs_in_with_github_spec.rb index cb3d4b7e1..ee6eb503c 100644 --- a/spec/system/authentication/user_logs_in_with_github_spec.rb +++ b/spec/system/authentication/user_logs_in_with_github_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe "Authenticating with GitHub" do let(:sign_in_link) { "Sign In With GitHub" } - before { mock_github } + before { omniauth_mock_github_payload } context "when a user is new" do context "when using valid credentials" do @@ -54,13 +54,13 @@ RSpec.describe "Authenticating with GitHub" do end before do - mock_auth_with_invalid_credentials(:github) + omniauth_setup_invalid_credentials(:github) allow(DatadogStatsClient).to receive(:increment) end after do - OmniAuth.config.on_failure = OmniauthMacros.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") + OmniAuth.config.on_failure = OmniauthHelpers.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") end it "does not create a new user" do @@ -85,7 +85,7 @@ RSpec.describe "Authenticating with GitHub" do "Callback error", "Error reason", "https://example.com/error" ) - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link @@ -101,7 +101,7 @@ RSpec.describe "Authenticating with GitHub" do allow(request).to receive(:code).and_return(401) allow(request).to receive(:message).and_return("unauthorized") error = OAuth::Unauthorized.new(request) - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link @@ -114,7 +114,7 @@ RSpec.describe "Authenticating with GitHub" do it "notifies Datadog even with no OmniAuth error present" do error = nil - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link diff --git a/spec/system/authentication/user_logs_in_with_twitter_spec.rb b/spec/system/authentication/user_logs_in_with_twitter_spec.rb index aea01c2b5..7c81cbdaa 100644 --- a/spec/system/authentication/user_logs_in_with_twitter_spec.rb +++ b/spec/system/authentication/user_logs_in_with_twitter_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe "Authenticating with Twitter" do let(:sign_in_link) { "Sign In With Twitter" } - before { mock_twitter } + before { omniauth_mock_twitter_payload } context "when a user is new" do context "when using valid credentials" do @@ -50,13 +50,13 @@ RSpec.describe "Authenticating with Twitter" do context "when using invalid credentials" do before do - mock_auth_with_invalid_credentials(:twitter) + omniauth_setup_invalid_credentials(:twitter) allow(DatadogStatsClient).to receive(:increment) end after do - OmniAuth.config.on_failure = OmniauthMacros.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") + OmniAuth.config.on_failure = OmniauthHelpers.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") end it "does not create a new user" do @@ -81,7 +81,7 @@ RSpec.describe "Authenticating with Twitter" do "Callback error", "Error reason", "https://example.com/error" ) - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link @@ -97,7 +97,7 @@ RSpec.describe "Authenticating with Twitter" do allow(request).to receive(:code).and_return(401) allow(request).to receive(:message).and_return("unauthorized") error = OAuth::Unauthorized.new(request) - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link @@ -110,7 +110,7 @@ RSpec.describe "Authenticating with Twitter" do it "notifies Datadog even with no OmniAuth error present" do error = nil - setup_omniauth_error(error) + omniauth_setup_authentication_error(error) visit root_path click_link sign_in_link