From d6e47a03c778887d0ad144a5fad48d30d8a3799f Mon Sep 17 00:00:00 2001 From: rhymes Date: Wed, 15 Apr 2020 18:13:10 +0200 Subject: [PATCH] [deploy] Authentication refactoring - step 1 (#7259) * Add Authentication providers * Making strides on refactoring * New usr from Twitter * Existing Twitter user * Existing user Github * Test skip_confirmation! * Refactor a bit and add simplecov groups * Test user already logged in * Rename to Authentication::Authenticator * Refactor to a common class * Add existing user test for twitter login * Refactor global handler * Fix spec and comment Twitter provider * Update comments and strategy TODO * Move providers loading code to Authentication::Providers * add SubclassResponsibility error * Use delegation --- .rubocop.yml | 2 + .../omniauth_callbacks_controller.rb | 7 +- app/errors/subclass_responsibility.rb | 4 + app/models/identity.rb | 21 +- app/services/authentication/authenticator.rb | 130 +++++++ app/services/authentication/errors.rb | 9 + app/services/authentication/providers.rb | 11 + .../authentication/providers/github.rb | 34 ++ .../authentication/providers/provider.rb | 54 +++ .../authentication/providers/twitter.rb | 43 +++ app/services/authorization_service.rb | 115 ------ spec/factories/users.rb | 6 +- spec/labor/badge_rewarder_spec.rb | 1 + spec/models/github_repo_spec.rb | 2 + spec/models/identity_spec.rb | 83 +++- spec/models/user_spec.rb | 7 +- spec/rails_helper.rb | 3 + spec/requests/github_repos_spec.rb | 1 + spec/requests/internal/users_spec.rb | 5 +- spec/requests/user/user_settings_spec.rb | 5 +- .../authentication/authenticator_spec.rb | 354 ++++++++++++++++++ spec/services/authorization_service_spec.rb | 77 ---- .../welcome_notification/generator_spec.rb | 8 +- spec/services/users/delete_spec.rb | 2 + spec/spec_helper.rb | 17 + spec/support/omniauth_macros.rb | 102 +++-- .../user_logs_in_with_github_spec.rb | 104 +++++ .../user_logs_in_with_twitter_spec.rb | 104 +++++ spec/system/user_logs_in_with_twitter_spec.rb | 173 --------- 29 files changed, 1082 insertions(+), 402 deletions(-) create mode 100644 app/errors/subclass_responsibility.rb create mode 100644 app/services/authentication/authenticator.rb create mode 100644 app/services/authentication/errors.rb create mode 100644 app/services/authentication/providers.rb create mode 100644 app/services/authentication/providers/github.rb create mode 100644 app/services/authentication/providers/provider.rb create mode 100644 app/services/authentication/providers/twitter.rb delete mode 100644 app/services/authorization_service.rb create mode 100644 spec/services/authentication/authenticator_spec.rb delete mode 100644 spec/services/authorization_service_spec.rb create mode 100644 spec/system/authentication/user_logs_in_with_github_spec.rb create mode 100644 spec/system/authentication/user_logs_in_with_twitter_spec.rb delete mode 100644 spec/system/user_logs_in_with_twitter_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index b90c11d69..47ae288eb 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -382,6 +382,8 @@ RSpec/DescribeClass: Description: Check that the first argument to the top level describe is a constant. Enabled: true StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/DescribeClass + Exclude: + - 'spec/system/**/*' RSpec/ExampleLength: Description: Checks for long examples. diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 85c67b6bf..672118b2f 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -35,9 +35,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController private + # TODO: [thepracticaldev/oss] test all of this def callback_for(provider) cta_variant = request.env["omniauth.params"]["state"].to_s - @user = AuthorizationService.new(request.env["omniauth.auth"], current_user, cta_variant).get_user + @user = Authentication::Authenticator.call( + request.env["omniauth.auth"], + current_user: current_user, + cta_variant: cta_variant, + ) if persisted_and_valid? # delete legacy session based cookie once the user has logged in again diff --git a/app/errors/subclass_responsibility.rb b/app/errors/subclass_responsibility.rb new file mode 100644 index 000000000..b1d95e2a5 --- /dev/null +++ b/app/errors/subclass_responsibility.rb @@ -0,0 +1,4 @@ +# raised to signal that the implementation of a method is responsibility +# of the subclass +class SubclassResponsibility < StandardError +end diff --git a/app/models/identity.rb b/app/models/identity.rb index 14db87297..6c671e15c 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -1,14 +1,31 @@ class Identity < ApplicationRecord belongs_to :user has_many :backup_data, as: :instance, class_name: "BackupData", dependent: :destroy + validates :uid, :provider, presence: true validates :uid, uniqueness: { scope: :provider }, if: proc { |identity| identity.uid_changed? || identity.provider_changed? } validates :user_id, uniqueness: { scope: :provider }, if: proc { |identity| identity.user_id_changed? || identity.provider_changed? } + # TODO: [thepracticaldev/oss] put the providers somewhere else validates :provider, inclusion: { in: %w[github twitter] } + # TODO: [thepracticaldev/oss] should this be transitioned to JSON? serialize :auth_data_dump - def self.find_for_oauth(auth) - find_or_create_by(uid: auth.uid, provider: auth.provider) + # Builds an identity from OmniAuth's authentication payload + def self.build_from_omniauth(provider) + payload = provider.payload + + identity = find_or_initialize_by( + provider: payload.provider, + uid: payload.uid, + ) + + identity.assign_attributes( + token: payload.credentials.token, + secret: payload.credentials.secret, + auth_data_dump: payload, + ) + + identity end end diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb new file mode 100644 index 000000000..f85d52a96 --- /dev/null +++ b/app/services/authentication/authenticator.rb @@ -0,0 +1,130 @@ +module Authentication + # TODO: [thepracticaldev/oss] use strategy pattern for the three cases + # described below. + # Make the decision early which one of the 3 cases we're dealing with + # and then call either NewUserStrategy, UpdateUserStrategy or + # LoggedInUserStrategy. I think the resulting three classes would be much + # easier to understand and they can still share methods by inheriting + # from a basic AuthStrategy. + + # Authenticator will perform one of these tree operations: + # 1. create a new user and match it to its authentication identity + # 2. update an existing user and align it to its authentication identity + # 3. return the current user if a user is given (already logged in scenario) + class Authenticator + # auth_payload is the payload schema, see https://github.com/omniauth/omniauth/wiki/Auth-Hash-Schema + def initialize(auth_payload, current_user: nil, cta_variant: nil) + @provider = load_authentication_provider(auth_payload) + + @current_user = current_user + @cta_variant = cta_variant + end + + def self.call(*args) + new(*args).call + end + + def call + identity = Identity.build_from_omniauth(provider) + + return current_user if current_user_identity_exists? + + user = proper_user(identity) + user = if user.nil? + build_user + else + update_user(user) + end + + save_identity(identity, user) + + user.skip_confirmation! + + flag_spam_user(user) if account_less_than_a_week_old?(user, identity) + + user.save! + user + end + + private + + attr_reader :provider, :current_user, :cta_variant + + # Loads the proper authentication provider from the available ones + def load_authentication_provider(auth_payload) + provider_class = Authentication::Providers.get!(auth_payload.provider) + provider_class.new(auth_payload) + end + + def current_user_identity_exists? + current_user&.identities&.exists?(provider: provider.name) + end + + def proper_user(identity) + if current_user + current_user + elsif identity.user + identity.user + elsif provider.user_email.present? + User.find_by(email: provider.user_email) + end + end + + def build_user + existing_user = User.where( + provider.user_username_field => provider.user_nickname, + ).take + return existing_user if existing_user + + User.new.tap do |user| + user.assign_attributes(provider.new_user_data) + user.assign_attributes(default_user_fields) + + user.set_remember_fields + end + end + + def default_user_fields + { + password: Devise.friendly_token(20), + signup_cta_variant: cta_variant, + saw_onboarding: false, + editor_version: :v2 + } + end + + def update_user(user) + user.tap do |model| + user.assign_attributes(provider.existing_user_data) + + update_profile_updated_at(model) + + model.set_remember_fields + end + end + + def update_profile_updated_at(user) + field_name = "#{provider.user_username_field}_changed?" + user.profile_updated_at = Time.current if user.public_send(field_name) + end + + def save_identity(identity, user) + identity.user = user if identity.user_id.blank? + identity.save! + end + + def account_less_than_a_week_old?(user, logged_in_identity) + provider_created_at = user.public_send(provider.user_created_at_field) + user_identity_age = provider_created_at || + Time.zone.parse(logged_in_identity.auth_data_dump.extra.raw_info.created_at) + + # last one is a fallback in case both are nil + range = 1.week.ago.beginning_of_day..Time.current + range.cover?(user_identity_age) + end + + def flag_spam_user(user) + Slack::Messengers::PotentialSpammer.call(user: user) + end + end +end diff --git a/app/services/authentication/errors.rb b/app/services/authentication/errors.rb new file mode 100644 index 000000000..1b7346a2e --- /dev/null +++ b/app/services/authentication/errors.rb @@ -0,0 +1,9 @@ +module Authentication + module Errors + class Error < StandardError + end + + class ProviderNotFound < Error + end + end +end diff --git a/app/services/authentication/providers.rb b/app/services/authentication/providers.rb new file mode 100644 index 000000000..83bce303c --- /dev/null +++ b/app/services/authentication/providers.rb @@ -0,0 +1,11 @@ +module Authentication + module Providers + # TODO: [thepracticaldev/oss] raise exception if provider is available but not enabled for this app + # TODO: [thepracticaldev/oss] add available providers, enabled providers + def self.get!(provider_name) + "Authentication::Providers::#{provider_name.titleize}".constantize + rescue NameError => e + raise ::Authentication::Errors::ProviderNotFound, e + end + end +end diff --git a/app/services/authentication/providers/github.rb b/app/services/authentication/providers/github.rb new file mode 100644 index 000000000..367407805 --- /dev/null +++ b/app/services/authentication/providers/github.rb @@ -0,0 +1,34 @@ +module Authentication + module Providers + # GitHub authentication provider, uses omniauth-github as backend + class Github < Provider + CREATED_AT_FIELD = "github_created_at".freeze + USERNAME_FIELD = "github_username".freeze + + def new_user_data + name = raw_info.name.presence || info.name + + { + email: info.email.to_s, + github_created_at: raw_info.created_at, + github_username: info.nickname, + name: name, + remote_profile_image_url: info.image.to_s + } + end + + def existing_user_data + { + github_created_at: raw_info.created_at, + github_username: info.nickname + } + end + + protected + + def cleanup_payload(auth_payload) + auth_payload + end + end + end +end diff --git a/app/services/authentication/providers/provider.rb b/app/services/authentication/providers/provider.rb new file mode 100644 index 000000000..70e6ba079 --- /dev/null +++ b/app/services/authentication/providers/provider.rb @@ -0,0 +1,54 @@ +module Authentication + module Providers + # Authentication provider + class Provider + # name of the field to store the upstream identity's creation date in + CREATED_AT_FIELD = "".freeze + # name of the field to store the upstream identity's username in + USERNAME_FIELD = "".freeze + + delegate :email, :nickname, to: :info, prefix: :user + + def initialize(auth_payload) + @auth_payload = cleanup_payload(auth_payload.dup) + @info = auth_payload.info + @raw_info = auth_payload.extra.raw_info + end + + # Extract data for a brand new user + def new_user_data + raise SubclassResponsibility + end + + # Extract data for an existing user + def existing_user_data + raise SubclassResponsibility + end + + def name + auth_payload.provider + end + + def payload + auth_payload + end + + def user_created_at_field + self.class::CREATED_AT_FIELD + end + + def user_username_field + self.class::USERNAME_FIELD + end + + protected + + # Remove sensible data from the payload + def cleanup_payload(_auth_payload) + raise SubclassResponsibility + end + + attr_reader :auth_payload, :info, :raw_info + end + end +end diff --git a/app/services/authentication/providers/twitter.rb b/app/services/authentication/providers/twitter.rb new file mode 100644 index 000000000..0146e6016 --- /dev/null +++ b/app/services/authentication/providers/twitter.rb @@ -0,0 +1,43 @@ +module Authentication + module Providers + # Twitter authentication provider, uses omniauth-twitter as backend + class Twitter < Provider + CREATED_AT_FIELD = "twitter_created_at".freeze + USERNAME_FIELD = "twitter_username".freeze + + def new_user_data + name = raw_info.name.presence || info.name + remote_profile_image_url = info.image.to_s.gsub("_normal", "") + + { + email: info.email.to_s, + name: name, + remote_profile_image_url: remote_profile_image_url, + twitter_created_at: raw_info.created_at, + twitter_followers_count: raw_info.followers_count.to_i, + twitter_following_count: raw_info.friends_count.to_i, + twitter_username: info.nickname + } + end + + def existing_user_data + { + twitter_created_at: raw_info.created_at, + twitter_followers_count: raw_info.followers_count.to_i, + twitter_following_count: raw_info.friends_count.to_i, + twitter_username: info.nickname + } + end + + protected + + def cleanup_payload(auth_payload) + auth_payload.tap do |auth| + # Twitter sends the server side access token keys in the payload + # for each authentication. We definitely do not want to store those + auth.extra.delete("access_token") + end + end + end + end +end diff --git a/app/services/authorization_service.rb b/app/services/authorization_service.rb deleted file mode 100644 index 3fbde9164..000000000 --- a/app/services/authorization_service.rb +++ /dev/null @@ -1,115 +0,0 @@ -class AuthorizationService - attr_accessor :auth, :signed_in_resource, :cta_variant - - def initialize(auth, signed_in_resource = nil, cta_variant = nil) - @auth = auth - @signed_in_resource = signed_in_resource - @cta_variant = cta_variant - end - - def get_user - identity = build_identity - return signed_in_resource if user_identity_exists(identity) - - user = proper_user(identity) - user = if user.nil? - build_user(identity) - else - update_user(user) - end - set_identity(identity, user) - user.skip_confirmation! - flag_spam_user(user) if account_less_than_a_week_old?(user, identity) - user - end - - def add_social_identity_data(user) - return unless auth&.provider && auth&.extra && auth.extra.raw_info - - if auth.provider == "twitter" - user.twitter_created_at = auth.extra.raw_info.created_at - user.twitter_followers_count = auth.extra.raw_info.followers_count.to_i - user.twitter_following_count = auth.extra.raw_info.friends_count.to_i - else - user.github_created_at = auth.extra.raw_info.created_at - end - end - - def build_identity - identity = Identity.find_for_oauth(auth) - identity.token = auth.credentials.token - identity.secret = auth.credentials.secret - auth["extra"].delete("access_token") if auth["extra"]["access_token"] - identity.auth_data_dump = auth - identity.save - identity - end - - def build_user(identity) - user = User.where("#{identity.provider}_username" => auth.info.nickname).first - if user.nil? - user = User.new( - name: auth.extra.raw_info.name, - remote_profile_image_url: (auth.info.image || "").gsub("_normal", ""), - github_username: (auth.info.nickname if auth.provider == "github"), - signup_cta_variant: cta_variant, - email: auth.info.email || "", - twitter_username: (auth.info.nickname if auth.provider == "twitter"), - password: Devise.friendly_token[0, 20], - ) - user.name = auth.info.nickname if user.name.blank? - user.skip_confirmation! - user.set_remember_fields - add_social_identity_data(user) - user.saw_onboarding = false - user.editor_version = "v2" - user.save! - end - user - end - - def update_user(user) - user.set_remember_fields - user.github_username = auth.info.nickname if auth.provider == "github" && auth.info.nickname != user.github_username - user.twitter_username = auth.info.nickname if auth.provider == "twitter" && auth.info.nickname != user.twitter_username - add_social_identity_data(user) - user.profile_updated_at = Time.current if user.twitter_username_changed? || user.github_username_changed? - user.save - user - end - - def proper_user(identity) - if signed_in_resource - signed_in_resource - elsif identity.user - identity.user - elsif auth.info.email.present? - User.find_by(email: auth.info.email) - end - end - - def set_identity(identity, user) - return if identity.user_id.present? - - identity.user = user - identity.save! - end - - def user_identity_exists(identity) - signed_in_resource && - Identity.where(provider: identity.provider, user_id: signed_in_resource.id).any? - end - - def account_less_than_a_week_old?(user, logged_in_identity) - user_identity_age = user.github_created_at || - user.twitter_created_at || - Time.zone.parse(logged_in_identity.auth_data_dump.extra.raw_info.created_at) - # last one is a fallback in case both are nil - range = 1.week.ago.beginning_of_day..Time.current - range.cover?(user_identity_age) - end - - def flag_spam_user(user) - Slack::Messengers::PotentialSpammer.call(user: user) - end -end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index fcd9e447a..4b2fc3283 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -27,7 +27,11 @@ FactoryBot.define do after(:create) do |user, options| options.identities.each do |provider| - create(:identity, user: user, provider: provider) + auth = OmniAuth.config.mock_auth.fetch(provider.to_sym) + create( + :identity, + user: user, provider: provider, uid: auth.uid, auth_data_dump: auth, + ) end end end diff --git a/spec/labor/badge_rewarder_spec.rb b/spec/labor/badge_rewarder_spec.rb index dda779298..05de77d58 100644 --- a/spec/labor/badge_rewarder_spec.rb +++ b/spec/labor/badge_rewarder_spec.rb @@ -113,6 +113,7 @@ RSpec.describe BadgeRewarder, type: :labor do end before do + mock_github 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 974c132ce..61ee5839b 100644 --- a/spec/models/github_repo_spec.rb +++ b/spec/models/github_repo_spec.rb @@ -4,6 +4,8 @@ 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 } + it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:url) } it { is_expected.to validate_uniqueness_of(:url) } diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index 9169131c6..d4478ec03 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -16,12 +16,83 @@ RSpec.describe Identity, type: :model do it { is_expected.to validate_inclusion_of(:provider).in_array(%w[github twitter]) } it { is_expected.to serialize(:auth_data_dump) } - describe ".find_for_oauth" do - it "works" do - allow(described_class).to receive(:find_or_create_by) - auth = { uid: 0, provider: "github" } - described_class.find_for_oauth(instance_double("request", auth)) - expect(described_class).to have_received(:find_or_create_by).with(auth) + describe ".build_build_from_omniauth" do + let(:user) { create(:user) } + + before do + mock_auth_hash + end + + context "with Github payload" do + let(:auth_payload) { OmniAuth.config.mock_auth[:github] } + let(:provider) { Authentication::Providers::Github.new(auth_payload) } + + it "initializes a new identity from the auth payload" do + identity = described_class.build_from_omniauth(provider) + + expect(identity.new_record?).to be(true) + expect(identity.provider).to eq("github") + expect(identity.uid).to eq(auth_payload.uid) + expect(identity.token).to eq(auth_payload.credentials.token) + expect(identity.secret).to eq(auth_payload.credentials.secret) + expect(identity.auth_data_dump).to eq(provider.payload) + end + + it "finds an existing identity" do + payload = provider.payload + + existing_identity = described_class.create!( + user: user, + provider: payload.provider, + uid: payload.uid, + token: payload.credentials.token, + secret: payload.credentials.secret, + auth_data_dump: payload, + ) + + identity = described_class.build_from_omniauth(provider) + expect(identity).to eq(existing_identity) + end + end + + context "with Twitter payload" do + let(:auth_payload) { OmniAuth.config.mock_auth[:twitter] } + let(:provider) { Authentication::Providers::Twitter.new(auth_payload) } + + it "initializes a new identity from the auth payload" do + identity = described_class.build_from_omniauth(provider) + + expect(identity.new_record?).to be(true) + expect(identity.provider).to eq("twitter") + expect(identity.uid).to eq(auth_payload.uid) + expect(identity.token).to eq(auth_payload.credentials.token) + expect(identity.secret).to eq(auth_payload.credentials.secret) + expect(identity.auth_data_dump).to eq(provider.payload) + end + + it "finds an existing identity" do + payload = provider.payload + + existing_identity = described_class.create!( + user: user, + provider: payload.provider, + uid: payload.uid, + token: payload.credentials.token, + secret: payload.credentials.secret, + auth_data_dump: payload, + ) + + identity = described_class.build_from_omniauth(provider) + expect(identity).to eq(existing_identity) + end + + it "does not store the access token in auth_data_dump" do + expect(auth_payload.extra.access_token).not_to be_nil + + identity = described_class.build_from_omniauth(provider) + + expect(identity.auth_data_dump.extra.access_token).to be_nil + end end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f00590aec..243702c5c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2,8 +2,11 @@ require "rails_helper" def user_from_authorization_service(service_name, signed_in_resource, cta_variant) auth = OmniAuth.config.mock_auth[service_name] - service = AuthorizationService.new(auth, signed_in_resource, cta_variant) - service.get_user + Authentication::Authenticator.call( + auth, + current_user: signed_in_resource, + cta_variant: cta_variant, + ) end RSpec.describe User, type: :model do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index ec48326a6..9788dc827 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -124,6 +124,9 @@ RSpec.configure do |config| stub_request(:any, /api.mailchimp.com/). to_return(status: 200, body: "", headers: {}) + + stub_request(:any, /dummyimage.com/). + to_return(status: 200, body: "", headers: {}) end OmniAuth.config.test_mode = true diff --git a/spec/requests/github_repos_spec.rb b/spec/requests/github_repos_spec.rb index 3d835a833..9d5f66cd5 100644 --- a/spec/requests/github_repos_spec.rb +++ b/spec/requests/github_repos_spec.rb @@ -19,6 +19,7 @@ RSpec.describe "GithubRepos", type: :request do end before do + mock_github 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 bf63fa832..bcf2e5ea6 100644 --- a/spec/requests/internal/users_spec.rb +++ b/spec/requests/internal/users_spec.rb @@ -1,7 +1,10 @@ require "rails_helper" RSpec.describe "internal/users", type: :request do - let!(:user) { create(:user, :with_identity, identities: ["github"]) } + let!(:user) do + mock_github + create(:user, :with_identity, identities: ["github"]) + end let(:admin) { create(:user, :super_admin) } before do diff --git a/spec/requests/user/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb index 1372b0656..c21543ee2 100644 --- a/spec/requests/user/user_settings_spec.rb +++ b/spec/requests/user/user_settings_spec.rb @@ -265,7 +265,10 @@ RSpec.describe "UserSettings", type: :request do context "when user has two identities" do let(:user) { create(:user, :with_identity, identities: %w[github twitter]) } - before { sign_in user } + before do + mock_auth_hash + sign_in user + end it "brings the identity count to 1" do delete "/users/remove_association", params: { provider: "twitter" } diff --git a/spec/services/authentication/authenticator_spec.rb b/spec/services/authentication/authenticator_spec.rb new file mode 100644 index 000000000..5984afc1b --- /dev/null +++ b/spec/services/authentication/authenticator_spec.rb @@ -0,0 +1,354 @@ +require "rails_helper" + +RSpec.describe Authentication::Authenticator, type: :service do + before { mock_auth_hash } + + context "when authenticating through an unknown provider" do + it "raises ProviderNotFound" do + auth_payload = OmniAuth.config.mock_auth[:github].merge(provider: "okta") + expect { described_class.call(auth_payload) }.to raise_error( + Authentication::Errors::ProviderNotFound, + ) + end + end + + context "when authenticating through Github" do + let!(:auth_payload) { OmniAuth.config.mock_auth[:github] } + let!(:service) { described_class.new(auth_payload) } + + 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 "extracts the proper data from the auth payload" 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.github_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i) + expect(user.github_username).to eq(info.nickname) + end + + it "sets default fields" 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.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" 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" do + 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: SlackBotPingWorker) do + described_class.call(auth_payload) + end + end + end + + describe "existing user" do + let(:user) { create(:user, :with_identity, identities: [:github]) } + + 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 "sets remember_me for the existing user" 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 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 + + user = described_class.call(auth_payload) + + expect(user.github_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.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 + end + + describe "user already logged in" do + it "returns the current user if the identity exists" do + user = create(:user, :with_identity, identities: [:github]) + 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 + + context "when authenticating through Twitter" do + let!(:auth_payload) { OmniAuth.config.mock_auth[:twitter] } + let!(:service) { described_class.new(auth_payload) } + + 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 "extracts the proper data from the auth payload" 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.to_s.gsub("_normal", "")) + expect(user.twitter_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i) + expect(user.twitter_followers_count).to eq(raw_info.followers_count.to_i) + expect(user.twitter_following_count).to eq(raw_info.friends_count.to_i) + expect(user.twitter_username).to eq(info.nickname) + end + + it "sets default fields" 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.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" 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" do + 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: SlackBotPingWorker) do + described_class.call(auth_payload) + end + end + end + + describe "existing user" do + let(:user) { create(:user, :with_identity, identities: [:twitter]) } + + 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 "updates the proper data from the auth payload" do + # simulate changing twitter data + auth_payload.extra.raw_info.followers_count = rand(100).to_s + auth_payload.extra.raw_info.friends_count = rand(100).to_s + + user = described_class.call(auth_payload) + + raw_info = auth_payload.extra.raw_info + + expect(user.twitter_created_at.to_i).to eq(Time.zone.parse(raw_info.created_at).to_i) + expect(user.twitter_followers_count).to eq(raw_info.followers_count.to_i) + expect(user.twitter_following_count).to eq(raw_info.friends_count.to_i) + end + + it "sets remember_me for the existing user" 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 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 + + user = described_class.call(auth_payload) + + expect(user.twitter_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.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 + end + + describe "user already logged in" do + it "returns the current user if the identity exists" do + user = create(:user, :with_identity, identities: [:twitter]) + 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 diff --git a/spec/services/authorization_service_spec.rb b/spec/services/authorization_service_spec.rb deleted file mode 100644 index 09d672c25..000000000 --- a/spec/services/authorization_service_spec.rb +++ /dev/null @@ -1,77 +0,0 @@ -require "rails_helper" - -RSpec.describe AuthorizationService, type: :service do - before { mock_auth_hash } - - describe "new user" do - let(:auth) { OmniAuth.config.mock_auth[:github] } - let!(:service) { described_class.new(auth) } - - it "creates a new user" do - expect do - service.get_user - end.to change(User, :count).by(1) - end - - it "sets remember_me for the new user" do - user = service.get_user - user.reload - expect(user.remember_me).to be_truthy - expect(user.remember_token).to be_truthy - expect(user.remember_created_at).to be_truthy - end - - it "queues a slack message to be sent for a user whose identity is brand new" do - service.auth.extra.raw_info.created_at = 1.minute.ago.rfc3339 - - sidekiq_assert_enqueued_with(job: SlackBotPingWorker) do - service.get_user - end - end - end - - describe "existing user" do - let(:auth) { OmniAuth.config.mock_auth[:twitter] } - let(:user) { create(:user) } - - before { OmniAuth.config.mock_auth[:twitter].info.email = user.email } - - it "doesn't create a duplicate user" do - service = described_class.new(auth) - expect do - service.get_user - end.not_to change(User, :count) - end - - it "sets remember_me for the existing user" do - user.update_columns(remember_token: nil, remember_created_at: nil) - service = described_class.new(auth) - service.get_user - user.reload - expect(user.remember_me).to be_truthy - expect(user.remember_token).to be_truthy - expect(user.remember_created_at).to be_truthy - end - - context "when the user has a new Twitter username" do - it "updates their username properly" do - new_username = "new_username#{rand(1000)}" - auth.info.nickname = new_username - service = described_class.new(auth) - service.get_user - user.reload - expect(user.twitter_username).to eq new_username - end - - it "touches the profile_updated_at timestamp" do - original_profile_updated_at = user.profile_updated_at - new_username = "new_username#{rand(1000)}" - auth.info.nickname = new_username - service = described_class.new(auth) - service.get_user - user.reload - expect(user.profile_updated_at).to be > original_profile_updated_at - end - end - end -end diff --git a/spec/services/broadcasts/welcome_notification/generator_spec.rb b/spec/services/broadcasts/welcome_notification/generator_spec.rb index c32ec1b1f..1039a5362 100644 --- a/spec/services/broadcasts/welcome_notification/generator_spec.rb +++ b/spec/services/broadcasts/welcome_notification/generator_spec.rb @@ -13,6 +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 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 @@ -141,10 +142,13 @@ RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do end describe "#send_feed_customization_notification" do - let!(:user) { create(:user, :with_identity, identities: %w[twitter github], created_at: 3.days.ago) } + let!(:user) do + mock_auth_hash + create(:user, :with_identity, identities: %w[twitter github], created_at: 3.days.ago) + end it "does not send a notification to a newly-created user" do - user.update!(created_at: Time.zone.now) + user.update!(created_at: Time.current) sidekiq_perform_enqueued_jobs { described_class.new(user.id).send(:send_feed_customization_notification) } expect(Notification).not_to have_received(:send_welcome_notification) end diff --git a/spec/services/users/delete_spec.rb b/spec/services/users/delete_spec.rb index 7c084ca06..6ab71472c 100644 --- a/spec/services/users/delete_spec.rb +++ b/spec/services/users/delete_spec.rb @@ -1,6 +1,8 @@ require "rails_helper" RSpec.describe Users::Delete, type: :service do + before { mock_github } + let(:user) { create(:user, :with_identity, identities: ["github"]) } it "deletes user" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 031efec0e..0eea785b5 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,4 +1,21 @@ require "simplecov" +SimpleCov.start do + # TODO: [thepracticaldev/oss] enable this when we upgrade to SimpleCov 0.18 + # enable_coverage :branch + + add_group "Decorators", "app/decorators" + add_group "Errors", "app/errors" + add_group "Labor", "app/labor" + add_group "Liquid tags", "app/liquid_tags" + add_group "Policies", "app/policies" + add_group "Queries", "app/queries" + add_group "Sanitizers", "app/sanitizers" + add_group "Serializers", "app/serializers" + add_group "Services", "app/services" + add_group "Uploaders", "app/uploaders" + add_group "View objects", "app/view_objects" +end + require "zonebie/rspec" # This file was generated by the `rails generate rspec:install` command. Conventionally, all diff --git a/spec/support/omniauth_macros.rb b/spec/support/omniauth_macros.rb index d91f54fa0..4d3f397c0 100644 --- a/spec/support/omniauth_macros.rb +++ b/spec/support/omniauth_macros.rb @@ -1,4 +1,6 @@ module OmniauthMacros + OMNIAUTH_DEFAULT_FAILURE_HANDLER = OmniAuth.config.on_failure + INFO = OmniAuth::AuthHash::InfoHash.new( first_name: "fname", last_name: "lname", @@ -7,27 +9,28 @@ module OmniauthMacros nickname: "fname.lname", email: "yourname@email.com", verified: true, - # info.image = "http://graph.facebook.com/123456/picture?type=square”" ) - EXTRA_INFO = Hashie::Mash.new(raw_info: Hashie::Mash.new( - email: "yourname@email.com", - first_name: "fname", - gender: "female", - id: "123456", - last_name: "lname", - link: "http://www.facebook.com/url”", - lang: "fr", - locale: "en_US", - name: "fname lname", - timezone: 5.5, - updated_time: "2012-06-08T13:09:47+0000", - username: "fname.lname", - verified: true, - followers_count: 100, - friends_count: 1000, - created_at: "2017-06-08T13:09:47+0000", - )) + EXTRA_INFO = Hashie::Mash.new( + raw_info: Hashie::Mash.new( + email: "yourname@email.com", + first_name: "fname", + gender: "female", + id: "123456", + last_name: "lname", + link: "http://www.facebook.com/url”", + lang: "fr", + locale: "en_US", + name: "fname lname", + timezone: 5.5, + updated_time: "2012-06-08T13:09:47+0000", + username: "fname.lname", + verified: true, + followers_count: 100, + friends_count: 1000, + created_at: "2017-06-08T13:09:47+0000", + ), + ) CREDENTIAL = OmniAuth::AuthHash::InfoHash.new( token: "2735246777-jlOnuFlGlvybuwDJfyrIyESLUEgoo6CffyJCQUO", @@ -41,20 +44,77 @@ module OmniauthMacros credentials: CREDENTIAL }.freeze + def mock_auth_with_invalid_credentials(provider) + OmniAuth.config.mock_auth[provider] = :invalid_credentials + end + + def setup_omniauth_error(error) + # this hack is needed due to a limitation in how OmniAuth handles + # failures in mocked/testing environments, + # see + # for more details + local_failure_handler = lambda do |env| + env["omniauth.error"] = error + env + end + + # here we compose the two handlers into a single function, + # the result will be global_failure_handler(local_failure_handler(env)) + failure_handler = local_failure_handler >> OMNIAUTH_DEFAULT_FAILURE_HANDLER + + OmniAuth.config.on_failure = failure_handler + end + + def omniauth_failure_args(error, provider, params) + class_name = error.present? ? error.class.name : "" + + [ + tags: [ + "class:#{class_name}", + "message:#{error&.message}", + "reason:#{error.try(:error_reason)}", + "type:#{error.try(:error)}", + "uri:#{error.try(:error_uri)}", + "provider:#{provider}", + "origin:", + "params:#{params}", + ], + ] + end + def mock_auth_hash mock_twitter mock_github end def mock_twitter + info = BASIC_INFO[:info].merge( + image: "https://dummyimage.com/400x400_normal.jpg", + ) + + extra = BASIC_INFO[:extra].merge( + access_token: "value", + ) + OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new( - BASIC_INFO.merge(provider: "twitter"), + 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"), + 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 new file mode 100644 index 000000000..0b88f9ed1 --- /dev/null +++ b/spec/system/authentication/user_logs_in_with_github_spec.rb @@ -0,0 +1,104 @@ +require "rails_helper" + +RSpec.describe "Authenticating with GitHub" do + let(:sign_in_link) { "Sign In With GitHub" } + + before { mock_github } + + context "when a user is new" do + context "when using valid credentials" do + it "logs in and redirects to the onboarding" do + visit root_path + click_link sign_in_link + + expect(page.current_path).to include("/onboarding") + expect(page.html).to include("onboarding-container") + end + end + + context "when using invalid credentials" do + before do + mock_auth_with_invalid_credentials(:github) + + allow(DatadogStatsClient).to receive(:increment) + end + + after do + OmniAuth.config.on_failure = OmniauthMacros.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") + end + + it "does not log in" do + visit root_path + click_link sign_in_link + + expect(page).to have_current_path("/users/sign_in") + expect(page).to have_link("Sign In/Up") + expect(page).to have_link("Via GitHub") + expect(page).to have_link("All about #{ApplicationConfig['COMMUNITY_NAME']}") + end + + it "notifies Datadog about a callback error" do + error = OmniAuth::Strategies::OAuth2::CallbackError.new( + "Callback error", "Error reason", "https://example.com/error" + ) + + setup_omniauth_error(error) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + + it "notifies Datadog about an OAuth unauthorized error" do + request = double + 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) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + + it "notifies Datadog even with no OmniAuth error present" do + error = nil + setup_omniauth_error(error) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + end + end + + context "when a user already exists" do + let!(:auth_payload) { OmniAuth.config.mock_auth[:github] } + let(:user) { create(:user, :with_identity, identities: [:github]) } + + before do + auth_payload.info.email = user.email + sign_in user + end + + context "when using valid credentials" do + it "logs in and redirects to the onboarding" do + visit "/users/auth/github" + + expect(page).to have_current_path("/dashboard?signin=true") + end + end + end +end diff --git a/spec/system/authentication/user_logs_in_with_twitter_spec.rb b/spec/system/authentication/user_logs_in_with_twitter_spec.rb new file mode 100644 index 000000000..8c3c426fb --- /dev/null +++ b/spec/system/authentication/user_logs_in_with_twitter_spec.rb @@ -0,0 +1,104 @@ +require "rails_helper" + +RSpec.describe "Authenticating with Twitter" do + let(:sign_in_link) { "Sign In With Twitter" } + + before { mock_twitter } + + context "when a user is new" do + context "when using valid credentials" do + it "logs in and redirects to the onboarding" do + visit root_path + click_link sign_in_link + + expect(page).to have_current_path("/onboarding?referrer=none") + expect(page.html).to include("onboarding-container") + end + end + + context "when using invalid credentials" do + before do + mock_auth_with_invalid_credentials(:twitter) + + allow(DatadogStatsClient).to receive(:increment) + end + + after do + OmniAuth.config.on_failure = OmniauthMacros.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") + end + + it "does not log in" do + visit root_path + click_link sign_in_link + + expect(page).to have_current_path("/users/sign_in") + expect(page).to have_link("Sign In/Up") + expect(page).to have_link("Via Twitter") + expect(page).to have_link("All about #{ApplicationConfig['COMMUNITY_NAME']}") + end + + it "notifies Datadog about a callback error" do + error = OmniAuth::Strategies::OAuth2::CallbackError.new( + "Callback error", "Error reason", "https://example.com/error" + ) + + setup_omniauth_error(error) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "twitter", "{}") + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + + it "notifies Datadog about an OAuth unauthorized error" do + request = double + 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) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "twitter", "{}") + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + + it "notifies Datadog even with no OmniAuth error present" do + error = nil + setup_omniauth_error(error) + + visit root_path + click_link sign_in_link + + args = omniauth_failure_args(error, "twitter", "{}") + expect(DatadogStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + end + end + + context "when a user already exists" do + let!(:auth_payload) { OmniAuth.config.mock_auth[:twitter] } + let(:user) { create(:user, :with_identity, identities: [:twitter]) } + + before do + auth_payload.info.email = user.email + sign_in user + end + + context "when using valid credentials" do + it "logs in and redirects to the onboarding" do + visit "/users/auth/twitter" + + expect(page).to have_current_path("/dashboard?signin=true") + end + end + end +end diff --git a/spec/system/user_logs_in_with_twitter_spec.rb b/spec/system/user_logs_in_with_twitter_spec.rb deleted file mode 100644 index 4ac4570a9..000000000 --- a/spec/system/user_logs_in_with_twitter_spec.rb +++ /dev/null @@ -1,173 +0,0 @@ -require "rails_helper" - -global_failure_handler = OmniAuth.config.on_failure - -def user_grants_authorization_on_twitter(twitter_callback_hash) - OmniAuth.config.add_mock(:twitter, twitter_callback_hash) -end - -def user_does_not_grant_authorization_on_twitter - OmniAuth.config.mock_auth[:twitter] = :invalid_credentials -end - -def setup_omniauth_error(error) - # this hack is needed due to a limitation in how OmniAuth handles - # failures in mocked/testing environments, - # see - # for more details - global_failure_handler = OmniAuth.config.on_failure - - local_failure_handler = lambda do |env| - env["omniauth.error"] = error - env - end - # here we compose the two handlers into a single function, - # the result will be global_failure_handler(local_failure_handler(env)) - failure_handler = local_failure_handler >> global_failure_handler - - OmniAuth.config.on_failure = failure_handler -end - -def get_failure_args(error) - class_name = error.present? ? error.class.name : "" - - [ - tags: [ - "class:#{class_name}", - "message:#{error&.message}", - "reason:#{error.try(:error_reason)}", - "type:#{error.try(:error)}", - "uri:#{error.try(:error_uri)}", - "provider:twitter", - "origin:", - "params:{}", - ], - ] -end - -RSpec.describe "Authenticating with Twitter" do - let(:twitter_callback_hash) do - { - provider: "twitter", - uid: "111111", - credentials: { - token: "222222", - secret: "333333" - }, - extra: { - access_token: "", - raw_info: { - name: "Bruce Wayne", - created_at: "Thu Jul 4 00:00:00 +0000 2013" # This is mandatory - } - }, - info: { - nickname: "batman", - name: "Bruce Wayne", - email: "batman@batcave.com" - } - } - end - - context "when a user is new" do - let(:twitter_link) { "Sign In With Twitter" } - - context "when using valid credentials" do - before do - user_grants_authorization_on_twitter(twitter_callback_hash) - end - - it "logs in and redirects to the onboarding" do - visit root_path - click_link twitter_link - - expect(page).to have_current_path("/onboarding?referrer=none") - expect(page.html).to include("onboarding-container") - end - end - - context "when using invalid credentials" do - let(:callback_failure) do - [ - "omniauth.failure", - tags: [ - "class:", - "message:", - "reason:", - "type:", - "uri:", - "provider:twitter", - "origin:", - "params:{}", - ], - ] - end - - before do - user_does_not_grant_authorization_on_twitter - - allow(DatadogStatsClient).to receive(:increment) - end - - after do - OmniAuth.config.on_failure = global_failure_handler - end - - it "does not log in" do - visit root_path - click_link twitter_link - - expect(page).to have_current_path("/users/sign_in") - expect(page).to have_link("Sign In/Up") - expect(page).to have_link("Via Twitter") - expect(page).to have_link("Via GitHub") - expect(page).to have_link("All about #{ApplicationConfig['COMMUNITY_NAME']}") - end - - it "notifies Datadog about a callback error" do - error = OmniAuth::Strategies::OAuth2::CallbackError.new( - "Callback error", "Error reason", "https://example.com/error" - ) - - setup_omniauth_error(error) - - visit root_path - click_link twitter_link - - args = get_failure_args(error) - expect(DatadogStatsClient).to have_received(:increment).with( - "omniauth.failure", *args - ) - end - - it "notifies Datadog about an OAuth unauthorized error" do - request = double - 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) - - visit root_path - click_link twitter_link - - args = get_failure_args(error) - expect(DatadogStatsClient).to have_received(:increment).with( - "omniauth.failure", *args - ) - end - - it "notifies Datadog even with no OmniAuth error present" do - error = nil - setup_omniauth_error(error) - - visit root_path - click_link twitter_link - - args = get_failure_args(error) - expect(DatadogStatsClient).to have_received(:increment).with( - "omniauth.failure", *args - ) - end - end - end -end