From c74121b0c09a01d8bbd9ba415ce00cadbf5e6857 Mon Sep 17 00:00:00 2001 From: Fernando Valverde Date: Thu, 30 Sep 2021 06:47:45 -0600 Subject: [PATCH] Forem passport (cont) (#14759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial forem omniauth strategy setup work * Finish basic raw proof of concept * Some playing around * use OAuth payloads + PASSPORT_OAUTH_URL for local dev * Use FeatureFlag for Forem Passport Auth * Working on tests * Fix tests 🤞🏼 & some cleanup * Use correct namespace within lib directory (match class namespace) * Test to ensure Forem Passport auth is restricted by FeatureFlag * Add broadcast + work on tests * Update spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb Co-authored-by: Michael Kohl * Hash format * Schema cleanup + inline comments * Use temprorary Heroku domain * More cleanup * Missed one * Back to passport.forem.com * Require correct path in lib * Apply suggestions from code review Co-authored-by: Michael Kohl * Use with_indifferent_access for symbol hash access in Forem strategy Co-authored-by: benhalpern Co-authored-by: Michael Kohl --- app/assets/images/forem.svg | 22 ++ .../stylesheets/components/buttons.scss | 15 ++ app/lib/constants/settings/authentication.rb | 8 + app/models/settings/authentication.rb | 2 + .../authentication/providers/forem.rb | 51 ++++ .../settings/forms/_authentication.html.erb | 1 + .../_providers_registration_form.html.erb | 1 + config/initializers/devise.rb | 12 +- ...10709180819_add_forem_username_to_users.rb | 5 + db/schema.rb | 1 + ..._insert_forem_connect_broadcast_message.rb | 17 ++ lib/omni_auth/strategies/forem.rb | 36 +++ spec/factories/broadcasts.rb | 9 + ...rt_forem_connect_broadcast_message_spec.rb | 19 ++ spec/models/identity_spec.rb | 32 +++ spec/models/user_spec.rb | 10 +- .../authentication/providers/facebook_spec.rb | 3 +- .../authentication/providers/forem_spec.rb | 38 +++ .../authentication/providers/github_spec.rb | 3 +- .../authentication/providers/twitter_spec.rb | 4 +- .../services/authentication/providers_spec.rb | 2 +- .../welcome_notification/generator_spec.rb | 2 + spec/support/omniauth_helpers.rb | 31 +++ .../creator_config_edit_spec.rb | 1 + .../omniauth_redirect_uri_spec.rb | 1 + .../user_logs_in_with_facebook_spec.rb | 9 - .../user_logs_in_with_forem_spec.rb | 250 ++++++++++++++++++ 27 files changed, 568 insertions(+), 17 deletions(-) create mode 100644 app/assets/images/forem.svg create mode 100644 app/services/authentication/providers/forem.rb create mode 100644 db/migrate/20210709180819_add_forem_username_to_users.rb create mode 100644 lib/data_update_scripts/20210921235050_insert_forem_connect_broadcast_message.rb create mode 100644 lib/omni_auth/strategies/forem.rb create mode 100644 spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb create mode 100644 spec/services/authentication/providers/forem_spec.rb create mode 100644 spec/system/authentication/user_logs_in_with_forem_spec.rb diff --git a/app/assets/images/forem.svg b/app/assets/images/forem.svg new file mode 100644 index 000000000..9da323e88 --- /dev/null +++ b/app/assets/images/forem.svg @@ -0,0 +1,22 @@ + + + + + \ No newline at end of file diff --git a/app/assets/stylesheets/components/buttons.scss b/app/assets/stylesheets/components/buttons.scss index 6ad37b17f..88308c2f1 100644 --- a/app/assets/stylesheets/components/buttons.scss +++ b/app/assets/stylesheets/components/buttons.scss @@ -17,6 +17,10 @@ --brand-facebook-color: #fff; --brand-facebook-bg-hover: #476fbf; + --brand-forem-bg: #0a3e4a; + --brand-forem-color: #fff; + --brand-forem-bg-hover: #05313b; + --brand-email-bg: #24292e; --brand-email-color: #fff; --brand-email-bg-hover: #000; @@ -291,6 +295,17 @@ --color-inverted-hover: var(--brand-facebook-color); } +.crayons-btn--brand-forem { + --bg: var(--brand-forem-bg); + --bg-hover: var(--brand-forem-bg-hover); + --color: var(--brand-forem-color); + --color-hover: var(--brand-forem-color); + --bg-inverted: var(--brand-forem-bg); + --bg-inverted-hover: var(--brand-forem-bg-hover); + --color-inverted: var(--brand-forem-color); + --color-inverted-hover: var(--brand-forem-color); +} + .crayons-btn--brand-email { --bg: var(--brand-email-bg); --bg-hover: var(--brand-email-bg-hover); diff --git a/app/lib/constants/settings/authentication.rb b/app/lib/constants/settings/authentication.rb index e9be53bc2..fca778e43 100644 --- a/app/lib/constants/settings/authentication.rb +++ b/app/lib/constants/settings/authentication.rb @@ -39,6 +39,14 @@ module Constants "The \"App Secret\" portion of the Basic Settings section of the App page on the Facebook Developer Portal", placeholder: "" }, + forem_key: { + description: "Forem key", + placeholder: "" + }, + forem_secret: { + description: "Forem secret", + placeholder: "" + }, github_key: { description: "The \"Client ID\" portion of the GitHub Oauth Apps portal", placeholder: "" diff --git a/app/models/settings/authentication.rb b/app/models/settings/authentication.rb index dea690689..e98f0afd3 100644 --- a/app/models/settings/authentication.rb +++ b/app/models/settings/authentication.rb @@ -14,6 +14,8 @@ module Settings setting :display_email_domain_allow_list_publicly, type: :boolean, default: false setting :facebook_key, type: :string setting :facebook_secret, type: :string + setting :forem_key, type: :string + setting :forem_secret, type: :string setting :github_key, type: :string, default: ApplicationConfig["GITHUB_KEY"] setting :github_secret, type: :string, default: ApplicationConfig["GITHUB_SECRET"] setting :invite_only_mode, type: :boolean, default: false diff --git a/app/services/authentication/providers/forem.rb b/app/services/authentication/providers/forem.rb new file mode 100644 index 000000000..08b22b382 --- /dev/null +++ b/app/services/authentication/providers/forem.rb @@ -0,0 +1,51 @@ +module Authentication + module Providers + class Forem < Provider + OFFICIAL_NAME = "Forem".freeze + DOMAIN_URL = ApplicationConfig["PASSPORT_OAUTH_URL"] || "https://passport.forem.com".freeze + SETTINGS_URL = "#{DOMAIN_URL}/oauth/authorized_applications".freeze + + def new_user_data + { + email: info.email, + name: info.name, + remote_profile_image_url: raw_info.remote_profile_image_url, + forem_username: info.user_nickname + } + end + + def existing_user_data + { + email: info.email, + name: info.name, + forem_username: info.user_nickname + } + end + + delegate :user_nickname, to: :info + + def self.official_name + OFFICIAL_NAME + end + + def self.settings_url + SETTINGS_URL + end + + def self.sign_in_path(**kwargs) + ::Authentication::Paths.sign_in_path( + provider_name, + **kwargs, + ) + end + + protected + + # Remove sensible data from the payload: None in this case so return as-is + # For more details see Authentication::Providers::Provider#cleanup_payload + def cleanup_payload(auth_payload) + auth_payload + end + end + end +end diff --git a/app/views/admin/settings/forms/_authentication.html.erb b/app/views/admin/settings/forms/_authentication.html.erb index 8b895cced..ef08d0bdb 100644 --- a/app/views/admin/settings/forms/_authentication.html.erb +++ b/app/views/admin/settings/forms/_authentication.html.erb @@ -161,6 +161,7 @@ readonly: true %> <% authentication_available_providers.each do |provider| %> + <% next if provider.provider_name == :forem && !FeatureFlag.enabled?(:forem_passport) %>
<%= inline_svg_tag("#{provider.provider_name}.svg", class: "crayons-icon", aria: true, title: "#{provider.official_name} logo") %> diff --git a/app/views/shared/authentication/_providers_registration_form.html.erb b/app/views/shared/authentication/_providers_registration_form.html.erb index 982bb0282..9646352ca 100644 --- a/app/views/shared/authentication/_providers_registration_form.html.erb +++ b/app/views/shared/authentication/_providers_registration_form.html.erb @@ -1,6 +1,7 @@
<% authentication_enabled_providers.each do |provider| %> <% next unless display_social_login? %> + <% next if provider.provider_name == :forem && !FeatureFlag.enabled?(:forem_passport) %> <%= form_with url: provider.sign_in_path(state: "navbar_basic"), class: "flex w-100", local: true do |f| %> <%= f.button type: :submit, class: "crayons-btn crayons-btn--l crayons-btn--brand-#{provider.provider_name} crayons-btn--icon-left grow-1 whitespace-nowrap" do %> <%= inline_svg_tag("#{provider.provider_name}.svg", aria_hidden: true, class: "crayons-icon", title: provider.provider_name) %> diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 804b07952..04e5e183e 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,3 +1,12 @@ +Dir.glob(Rails.root.join("lib/omni_auth/strategies/*.rb")).each do |filename| + require_dependency filename +end + +FOREM_OMNIAUTH_SETUP = lambda do |env| + env["omniauth.strategy"].options[:client_id] = Settings::Authentication.forem_key + env["omniauth.strategy"].options[:client_secret] = Settings::Authentication.forem_secret +end + TWITTER_OMNIAUTH_SETUP = lambda do |env| env["omniauth.strategy"].options[:consumer_key] = Settings::Authentication.twitter_key env["omniauth.strategy"].options[:consumer_secret] = Settings::Authentication.twitter_secret @@ -312,8 +321,9 @@ Devise.setup do |config| # Fun fact, unless Twitter is last, it doesn't work for some reason. config.omniauth :facebook, setup: FACEBOOK_OMNIAUTH_SETUP config.omniauth :github, setup: GITHUB_OMNIUATH_SETUP - config.omniauth :twitter, setup: TWITTER_OMNIAUTH_SETUP config.omniauth :apple, setup: APPLE_OMNIAUTH_SETUP + config.omniauth :forem, setup: FOREM_OMNIAUTH_SETUP, strategy_class: OmniAuth::Strategies::Forem + config.omniauth :twitter, setup: TWITTER_OMNIAUTH_SETUP # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or diff --git a/db/migrate/20210709180819_add_forem_username_to_users.rb b/db/migrate/20210709180819_add_forem_username_to_users.rb new file mode 100644 index 000000000..f59052d36 --- /dev/null +++ b/db/migrate/20210709180819_add_forem_username_to_users.rb @@ -0,0 +1,5 @@ +class AddForemUsernameToUsers < ActiveRecord::Migration[6.1] + def change + add_column :users, :forem_username, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 671729c1b..c827b80b0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1296,6 +1296,7 @@ ActiveRecord::Schema.define(version: 2021_08_30_062627) do t.integer "following_orgs_count", default: 0, null: false t.integer "following_tags_count", default: 0, null: false t.integer "following_users_count", default: 0, null: false + t.string "forem_username" t.datetime "github_repos_updated_at", default: "2017-01-01 05:00:00" t.string "github_username" t.datetime "invitation_accepted_at" diff --git a/lib/data_update_scripts/20210921235050_insert_forem_connect_broadcast_message.rb b/lib/data_update_scripts/20210921235050_insert_forem_connect_broadcast_message.rb new file mode 100644 index 000000000..0efb6a6bb --- /dev/null +++ b/lib/data_update_scripts/20210921235050_insert_forem_connect_broadcast_message.rb @@ -0,0 +1,17 @@ +module DataUpdateScripts + class InsertForemConnectBroadcastMessage + def run + return if Broadcast.find_by(title: "Welcome Notification: forem_connect") + + message = "You're on a roll! 🎉 Do you have a Forem account? " \ + "Consider connecting it." + + Broadcast.create!( + title: "Welcome Notification: forem_connect", + processed_html: message, + type_of: "Welcome", + active: true, + ) + end + end +end diff --git a/lib/omni_auth/strategies/forem.rb b/lib/omni_auth/strategies/forem.rb new file mode 100644 index 000000000..e43c9800e --- /dev/null +++ b/lib/omni_auth/strategies/forem.rb @@ -0,0 +1,36 @@ +require "omniauth-oauth2" + +module OmniAuth + module Strategies + class Forem < OmniAuth::Strategies::OAuth2 + option :name, :forem + + option :client_options, { + site: ApplicationConfig["PASSPORT_OAUTH_URL"] || "https://passport.forem.com".freeze, + authorize_url: "/oauth/authorize" + } + + uid { raw_info[:id] } + + info do + { + email: raw_info[:email], + name: raw_info[:name], + user_nickname: raw_info[:username] + } + end + + extra do + skip_info? ? {} : { raw_info: raw_info } + end + + def raw_info + @raw_info ||= access_token.get("/api/v0/me").parsed.with_indifferent_access + end + + def callback_url + "#{full_host}#{script_name}#{callback_path}" + end + end + end +end diff --git a/spec/factories/broadcasts.rb b/spec/factories/broadcasts.rb index aced2cf3b..eaa5b3299 100644 --- a/spec/factories/broadcasts.rb +++ b/spec/factories/broadcasts.rb @@ -48,6 +48,15 @@ FactoryBot.define do end end + factory :forem_connect_broadcast do + title { "Welcome Notification: forem_connect" } + type_of { "Welcome" } + processed_html do + "You're on a roll! 🎉 Do you have a Forem account? Consider " \ + "connecting it." + end + end + factory :twitter_connect_broadcast do title { "Welcome Notification: twitter_connect" } type_of { "Welcome" } diff --git a/spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb b/spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb new file mode 100644 index 000000000..5fe477a2c --- /dev/null +++ b/spec/lib/data_update_scripts/insert_forem_connect_broadcast_message_spec.rb @@ -0,0 +1,19 @@ +require "rails_helper" +require Rails.root.join( + "lib/data_update_scripts/20210921235050_insert_forem_connect_broadcast_message.rb", +) + +describe DataUpdateScripts::InsertForemConnectBroadcastMessage do + it "works without a Broadcast" do + expect do + described_class.new.run + end.to change(Broadcast, :count).by(1) + end + + it "works when a Broadcast already exists" do + create(:forem_connect_broadcast) + expect do + described_class.new.run + end.not_to change(Broadcast, :count) + end +end diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index b27c446d5..a250bfc5e 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -124,6 +124,38 @@ RSpec.describe Identity, type: :model do end end + context "with Forem payload" do + let(:auth_payload) { OmniAuth.config.mock_auth[:forem] } + let(:provider) { Authentication::Providers::Forem.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("forem") + 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) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c49c2ae96..2b24dc1a8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,8 +11,11 @@ RSpec.describe User, type: :model do end def mock_username(provider_name, username) - if provider_name == :apple + case provider_name + when :apple OmniAuth.config.mock_auth[provider_name].info.first_name = username + when :forem + OmniAuth.config.mock_auth[provider_name].info.user_nickname = username else OmniAuth.config.mock_auth[provider_name].info.nickname = username end @@ -422,7 +425,10 @@ RSpec.describe User, type: :model do describe "user registration", vcr: { cassette_name: "fastly_sloan" } do let(:user) { create(:user) } - before { omniauth_mock_providers_payload } + before do + allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(true) + omniauth_mock_providers_payload + end Authentication::Providers.available.each do |provider_name| it "finds user by email and assigns identity to that if exists for #{provider_name}" do diff --git a/spec/services/authentication/providers/facebook_spec.rb b/spec/services/authentication/providers/facebook_spec.rb index 51465f4ca..7a30bf6c8 100644 --- a/spec/services/authentication/providers/facebook_spec.rb +++ b/spec/services/authentication/providers/facebook_spec.rb @@ -15,7 +15,8 @@ RSpec.describe Authentication::Providers::Facebook, type: :service do describe ".sign_in_path" do let(:expected_path) do - "/users/auth/facebook?callback_url=http%3A%2F%2Flocalhost%3A3000%2Fusers%2Fauth%2Ffacebook%2Fcallback" + expected_callback_url = CGI.escape(URL.url("/users/auth/facebook/callback")) + "/users/auth/facebook?callback_url=#{expected_callback_url}" end it "returns the correct sign in path" do diff --git a/spec/services/authentication/providers/forem_spec.rb b/spec/services/authentication/providers/forem_spec.rb new file mode 100644 index 000000000..6195310a8 --- /dev/null +++ b/spec/services/authentication/providers/forem_spec.rb @@ -0,0 +1,38 @@ +require "rails_helper" + +RSpec.describe Authentication::Providers::Forem, type: :service do + before { allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(true) } + + describe ".authentication_path" do + it "returns the correct authentication path" do + expected_path = Rails.application.routes.url_helpers.user_forem_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 + expected_callback_url = CGI.escape(URL.url("/users/auth/forem/callback")) + "/users/auth/forem?callback_url=#{expected_callback_url}" + 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/github_spec.rb b/spec/services/authentication/providers/github_spec.rb index a06950f2e..22ced9758 100644 --- a/spec/services/authentication/providers/github_spec.rb +++ b/spec/services/authentication/providers/github_spec.rb @@ -15,7 +15,8 @@ RSpec.describe Authentication::Providers::Github, type: :service do describe ".sign_in_path" do let(:expected_path) do - "/users/auth/github?callback_url=http%3A%2F%2Flocalhost%3A3000%2Fusers%2Fauth%2Fgithub%2Fcallback" + expected_callback_url = CGI.escape(URL.url("/users/auth/github/callback")) + "/users/auth/github?callback_url=#{expected_callback_url}" end it "returns the correct sign in path" do diff --git a/spec/services/authentication/providers/twitter_spec.rb b/spec/services/authentication/providers/twitter_spec.rb index 9d4589778..ac29f7803 100644 --- a/spec/services/authentication/providers/twitter_spec.rb +++ b/spec/services/authentication/providers/twitter_spec.rb @@ -15,8 +15,8 @@ RSpec.describe Authentication::Providers::Twitter, type: :service do 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" + expected_callback_url = CGI.escape(URL.url("/users/auth/twitter/callback")) + "/users/auth/twitter?callback_url=#{expected_callback_url}&secure_image_url=true" end it "returns the correct sign in path" do diff --git a/spec/services/authentication/providers_spec.rb b/spec/services/authentication/providers_spec.rb index 8d6268616..370548989 100644 --- a/spec/services/authentication/providers_spec.rb +++ b/spec/services/authentication/providers_spec.rb @@ -28,7 +28,7 @@ RSpec.describe Authentication::Providers, type: :service do describe ".available" do it "lists the available providers" do - available_providers = %i[apple facebook github twitter] + available_providers = %i[apple facebook forem github twitter] expect(described_class.available).to eq(available_providers) end end diff --git a/spec/services/broadcasts/welcome_notification/generator_spec.rb b/spec/services/broadcasts/welcome_notification/generator_spec.rb index af70da3d0..3d03e37dc 100644 --- a/spec/services/broadcasts/welcome_notification/generator_spec.rb +++ b/spec/services/broadcasts/welcome_notification/generator_spec.rb @@ -10,6 +10,7 @@ RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do let!(:github_connect_broadcast) { create(:github_connect_broadcast) } let!(:facebook_connect_broadcast) { create(:facebook_connect_broadcast) } let!(:apple_connect_broadcast) { create(:apple_connect_broadcast) } + let!(:forem_connect_broadcast) { create(:forem_connect_broadcast) } let!(:customize_feed_broadcast) { create(:customize_feed_broadcast) } let!(:discuss_and_ask_broadcast) { create(:discuss_and_ask_broadcast) } let!(:customize_ux_broadcast) { create(:customize_ux_broadcast) } @@ -66,6 +67,7 @@ RSpec.describe Broadcasts::WelcomeNotification::Generator, type: :service do facebook_connect_broadcast, twitter_connect_broadcast, apple_connect_broadcast, + forem_connect_broadcast, ].include?(user.notifications.last.notifiable) expect(not_github).to be(true) diff --git a/spec/support/omniauth_helpers.rb b/spec/support/omniauth_helpers.rb index 98abe8fc2..878e98286 100644 --- a/spec/support/omniauth_helpers.rb +++ b/spec/support/omniauth_helpers.rb @@ -68,6 +68,33 @@ module OmniauthHelpers }, ).freeze + OMNIAUTH_PAYLOAD_FOREM = OmniAuth::AuthHash::InfoHash.new( + { + provider: "forem", + uid: SecureRandom.hex, + info: { + email: "sloan@dev.to", + name: "Sloan", + image: "https://dummyimage.com/400x400.jpg", + user_nickname: "sloan" + }, + credentials: { + token: SecureRandom.hex, + refresh_token: SecureRandom.hex, + expires_at: 1_589_475_606, + expires: true + }, + extra: { + raw_info: { + email: "slaon@dev.to", + id: "31047", + name: "Sloan", + remote_profile_image_url: "https://dummyimage.com/400x400.jpg" + } + } + }, + ).freeze + OMNIAUTH_PAYLOAD_APPLE = OmniAuth::AuthHash::InfoHash.new( { provider: "apple", @@ -163,6 +190,10 @@ module OmniauthHelpers OmniAuth.config.mock_auth[:facebook] = OMNIAUTH_PAYLOAD_FACEBOOK.dup end + def omniauth_mock_forem_payload + OmniAuth.config.mock_auth[:forem] = OMNIAUTH_PAYLOAD_FOREM.dup + end + def omniauth_mock_apple_payload OmniAuth.config.mock_auth[:apple] = OMNIAUTH_PAYLOAD_APPLE.dup end diff --git a/spec/system/authentication/creator_config_edit_spec.rb b/spec/system/authentication/creator_config_edit_spec.rb index 10cfbda49..85079c98f 100644 --- a/spec/system/authentication/creator_config_edit_spec.rb +++ b/spec/system/authentication/creator_config_edit_spec.rb @@ -7,6 +7,7 @@ RSpec.describe "Creator config edit", type: :system, js: true do before do sign_in admin allow(ForemInstance).to receive(:private?).and_return(false) + allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(true) end it "presents all available OAuth providers" do diff --git a/spec/system/authentication/omniauth_redirect_uri_spec.rb b/spec/system/authentication/omniauth_redirect_uri_spec.rb index 444d0ce4b..f2882e849 100644 --- a/spec/system/authentication/omniauth_redirect_uri_spec.rb +++ b/spec/system/authentication/omniauth_redirect_uri_spec.rb @@ -18,6 +18,7 @@ RSpec.describe "Omniauth redirect_uri", type: :system do it "relies on Settings::General.app_domain to generate correct callbacks_url" do allow(Settings::Authentication).to receive(:providers).and_return(Authentication::Providers.available) + allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(true) visit sign_up_path Authentication::Providers.available.each do |provider_name| provider_auth_button = find("button.crayons-btn--brand-#{provider_name}") diff --git a/spec/system/authentication/user_logs_in_with_facebook_spec.rb b/spec/system/authentication/user_logs_in_with_facebook_spec.rb index 83246eeb7..b81bacd82 100644 --- a/spec/system/authentication/user_logs_in_with_facebook_spec.rb +++ b/spec/system/authentication/user_logs_in_with_facebook_spec.rb @@ -220,15 +220,6 @@ RSpec.describe "Authenticating with Facebook" do expect(page).to have_current_path("/?signin=true") end - - it "renders the facebook icon on the profile" do - sign_in user - visit user_facebook_omniauth_authorize_path - - visit user_profile_path(user.username) - - expect(page).to have_css("svg.crayons-icon.shrink-0", text: "facebook website") - end end end diff --git a/spec/system/authentication/user_logs_in_with_forem_spec.rb b/spec/system/authentication/user_logs_in_with_forem_spec.rb new file mode 100644 index 000000000..1ac78081a --- /dev/null +++ b/spec/system/authentication/user_logs_in_with_forem_spec.rb @@ -0,0 +1,250 @@ +require "rails_helper" + +RSpec.describe "Authenticating with Forem" do + let(:sign_in_link) { "Continue with Forem" } + + before do + allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(true) + omniauth_mock_forem_payload + allow(Settings::Authentication).to receive(:providers).and_return(Authentication::Providers.available) + end + + describe "FeatureFlag hides the Forem Passport auth" do + it "shows Forem auth when enabled" do + visit sign_up_path + expect(page).to have_text(sign_in_link) + end + + it "doesn't show the Forem auth when disabled" do + allow(FeatureFlag).to receive(:enabled?).with(:forem_passport).and_return(false) + visit sign_up_path + expect(page).not_to have_text(sign_in_link) + end + end + + context "when a user is new" do + context "when using valid credentials" do + it "creates a new user" do + expect do + visit sign_up_path + click_on(sign_in_link, match: :first) + end.to change(User, :count).by(1) + end + + it "logs in and redirects to the onboarding" do + visit sign_up_path + click_on(sign_in_link, match: :first) + expect(page).to have_current_path("/onboarding", ignore_query: true) + expect(page.html).to include("onboarding-container") + end + + it "remembers the user" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + user = User.last + + expect(user.remember_token).to be_present + expect(user.remember_created_at).to be_present + end + end + + context "when using valid credentials but witholding email address" do + before do + OmniAuth.config.mock_auth[:forem][:info].delete(:email) + OmniAuth.config.mock_auth[:forem][:extra][:raw_info].delete(:email) + end + + it "creates a new user" do + expect do + visit sign_up_path + click_on(sign_in_link, match: :first) + end.to change(User, :count).by(1) + end + + it "logs in and redirects to the onboarding" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + expect(page).to have_current_path("/onboarding", ignore_query: true) + expect(page.html).to include("onboarding-container") + end + + it "remembers the user" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + user = User.last + + expect(user.remember_token).to be_present + expect(user.remember_created_at).to be_present + end + end + + context "when trying to register with an already existing username" do + it "creates a new user with a temporary username" do + # see Authentication::Providers::Forem#new_user_data + username = OmniAuth.config.mock_auth[:forem].info.name.downcase.sub(" ", "_") + user = create(:user, username: username) + + expect do + visit sign_up_path + click_on(sign_in_link, match: :first) + end.to change(User, :count).by(1) + + expect(page).to have_current_path("/onboarding", ignore_query: true) + expect(User.last.username).to include(user.username) + end + end + + context "when using invalid credentials" do + let(:params) do + '{"callback_url"=>"http://localhost:3000/users/auth/forem/callback", "state"=>"navbar_basic"}' + end + + before do + omniauth_setup_invalid_credentials(:forem) + + allow(ForemStatsClient).to receive(:increment) + end + + after do + OmniAuth.config.on_failure = OmniauthHelpers.const_get("OMNIAUTH_DEFAULT_FAILURE_HANDLER") + end + + it "does not create a new user" do + expect do + visit sign_up_path + click_on(sign_in_link, match: :first) + end.not_to change(User, :count) + end + + it "does not log in" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + expect(page).to have_current_path("/users/sign_in") + expect(page).to have_button(sign_in_link) + end + + it "notifies Datadog about a callback error" do + error = OmniAuth::Strategies::OAuth2::CallbackError.new( + "Callback error", "Error reason", "https://example.com/error" + ) + + omniauth_setup_authentication_error(error, params) + + visit sign_up_path + click_on(sign_in_link, match: :first) + + args = omniauth_failure_args(error, "forem", params) + expect(ForemStatsClient).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) + omniauth_setup_authentication_error(error, params) + + visit sign_up_path + click_on(sign_in_link, match: :first) + + args = omniauth_failure_args(error, "forem", params) + expect(ForemStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + + it "notifies Datadog even with no OmniAuth error present" do + error = nil + omniauth_setup_authentication_error(error, params) + + visit sign_up_path + click_on(sign_in_link, match: :first) + + args = omniauth_failure_args(error, "forem", params) + expect(ForemStatsClient).to have_received(:increment).with( + "omniauth.failure", *args + ) + end + end + + context "when a validation failure occurrs" do + before do + # A User is invalid if their name is more than 100 chars long + OmniAuth.config.mock_auth[:forem].info.name = "X" * 101 + end + + it "does not create a new user" do + expect do + visit sign_up_path + click_on(sign_in_link, match: :first) + end.not_to change(User, :count) + end + + it "redirects to the registration page" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + expect(page).to have_current_path("/users/sign_up") + end + + it "logs errors" do + allow(Honeybadger).to receive(:notify) + + visit sign_up_path + click_on(sign_in_link, match: :first) + + expect(Honeybadger).to have_received(:notify).once + end + end + end + + context "when a user already exists" do + let!(:auth_payload) { OmniAuth.config.mock_auth[:forem] } + let(:user) { create(:user, :with_identity, identities: [:forem]) } + + before do + auth_payload.info.name = user.name + auth_payload.info.email = user.email + end + + after do + sign_out user + end + + context "when using valid credentials" do + it "logs in" do + visit sign_up_path + click_on(sign_in_link, match: :first) + + expect(page).to have_current_path("/?signin=true") + end + end + + context "when already signed in" do + it "redirects to the dashboard" do + sign_in user + visit user_forem_omniauth_authorize_path + + expect(page).to have_current_path("/?signin=true") + end + end + end + + context "when community is in invite-only mode" do + before do + allow(ForemInstance).to receive(:invitation_only?).and_return(true) + end + + it "doesn't present the authentication option" do + visit sign_up_path(state: "new-user") + expect(page).not_to have_text(sign_in_link) + expect(page).to have_text("invite only") + end + end +end