diff --git a/app/models/site_config.rb b/app/models/site_config.rb index 3b214abe7..6b4635849 100644 --- a/app/models/site_config.rb +++ b/app/models/site_config.rb @@ -91,9 +91,4 @@ class SiteConfig < RailsSettings::Base # Shop field :shop_url, type: :string, default: "https://shop.dev.to" - - # Helpful methods - def self.auth_allowed?(provider) - authentication_providers.include?(provider) - end end diff --git a/app/services/authentication/errors.rb b/app/services/authentication/errors.rb index 1b7346a2e..2c4311a83 100644 --- a/app/services/authentication/errors.rb +++ b/app/services/authentication/errors.rb @@ -5,5 +5,8 @@ module Authentication class ProviderNotFound < Error end + + class ProviderNotEnabled < Error + end end end diff --git a/app/services/authentication/providers.rb b/app/services/authentication/providers.rb index 83bce303c..50c63e69a 100644 --- a/app/services/authentication/providers.rb +++ b/app/services/authentication/providers.rb @@ -1,11 +1,70 @@ +# We require all authentication modules to make sure providers +# are correctly preloaded both in development and in production and +# ready to be used when needed at runtime +Dir[Rails.root.join("app/services/authentication/**/*.rb")].each do |f| + require_dependency(f) +end + 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 + # Retrieves a provider that is both available and enabled def self.get!(provider_name) - "Authentication::Providers::#{provider_name.titleize}".constantize - rescue NameError => e - raise ::Authentication::Errors::ProviderNotFound, e + name = provider_name.to_s.titleize + + unless Authentication::Providers.const_defined?(name) + raise( + ::Authentication::Errors::ProviderNotFound, + "Provider #{name} is not available!", + ) + end + + unless enabled?(provider_name) + raise( + ::Authentication::Errors::ProviderNotEnabled, + "Provider #{name} is not enabled!", + ) + end + + 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 + + # Returns enabled providers + # TODO: [thepracticaldev/oss] ideally this should be "available - disabled" + # we can get there once we have feature flags + def self.enabled + 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 367407805..6d7fecea6 100644 --- a/app/services/authentication/providers/github.rb +++ b/app/services/authentication/providers/github.rb @@ -2,6 +2,7 @@ module Authentication module Providers # GitHub authentication provider, uses omniauth-github as backend class Github < Provider + OFFICIAL_NAME = "GitHub".freeze CREATED_AT_FIELD = "github_created_at".freeze USERNAME_FIELD = "github_username".freeze @@ -24,6 +25,10 @@ module Authentication } end + def self.official_name + OFFICIAL_NAME + end + protected def cleanup_payload(auth_payload) diff --git a/app/services/authentication/providers/provider.rb b/app/services/authentication/providers/provider.rb index 70e6ba079..90d383074 100644 --- a/app/services/authentication/providers/provider.rb +++ b/app/services/authentication/providers/provider.rb @@ -41,6 +41,11 @@ module Authentication self.class::USERNAME_FIELD end + # Returns the official name of the authentication provider + def self.official_name + 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 0146e6016..a823bfc5c 100644 --- a/app/services/authentication/providers/twitter.rb +++ b/app/services/authentication/providers/twitter.rb @@ -2,6 +2,7 @@ module Authentication module Providers # Twitter authentication provider, uses omniauth-twitter as backend class Twitter < Provider + OFFICIAL_NAME = "Twitter".freeze CREATED_AT_FIELD = "twitter_created_at".freeze USERNAME_FIELD = "twitter_username".freeze @@ -29,6 +30,10 @@ module Authentication } end + def self.official_name + OFFICIAL_NAME + end + protected def cleanup_payload(auth_payload) diff --git a/app/views/articles/_sidebar_additional.html.erb b/app/views/articles/_sidebar_additional.html.erb index d6c1bf651..85d82b494 100644 --- a/app/views/articles/_sidebar_additional.html.erb +++ b/app/views/articles/_sidebar_additional.html.erb @@ -7,12 +7,7 @@ Join <%= community_name %>
- <% if SiteConfig.auth_allowed?("twitter") %> - " class="cta cta-button login-cta-button" data-no-instant>Sign In With Twitter - <% end %> - <% if SiteConfig.auth_allowed?("github") %> - - <% end %> + <%= render partial: "shared/authentication/providers_sidebar" %>
<% end %> diff --git a/app/views/devise/registrations/_registration_form.html.erb b/app/views/devise/registrations/_registration_form.html.erb index 2d5fd11dc..c18871674 100644 --- a/app/views/devise/registrations/_registration_form.html.erb +++ b/app/views/devise/registrations/_registration_form.html.erb @@ -74,16 +74,8 @@

Great to have you

<% end %>
- <% if SiteConfig.auth_allowed?("twitter") %> - " class="sign-up-link" data-no-instant> - <%= inline_svg_tag("twitter-logo.svg", class: "icon-img", aria: true, title: "twitter logo") %> Sign In with Twitter - - <% end %> - <% if SiteConfig.auth_allowed?("github") %> - - <% end %> + <%= render partial: "shared/authentication/providers_registration_form" %> +

We require social login to prevent abuse.

Open Source 😇

diff --git a/app/views/layouts/_nav_menu.html.erb b/app/views/layouts/_nav_menu.html.erb index a7981a749..a2a608bca 100644 --- a/app/views/layouts/_nav_menu.html.erb +++ b/app/views/layouts/_nav_menu.html.erb @@ -3,27 +3,29 @@
- Dashboard - Write a Post - Reading List - Settings - Key Links + Dashboard + Write a Post + Reading List + Settings + Key Links
- Sign Out + Sign Out
<% else %>
- Sign In/Up - <% if SiteConfig.auth_allowed?("twitter") %> - " class="crayons-link crayons-link--block pl-5" data-no-instant>Via Twitter - <% end %> - <% if SiteConfig.auth_allowed?("github") %> - Via GitHub - <% end %> + + Sign In/Up + + + <%= render partial: "shared/authentication/providers_nav_menu" %>
- All about <%= community_name %> + All about <%= community_name %>
<% end %> diff --git a/app/views/layouts/_signup_modal.html.erb b/app/views/layouts/_signup_modal.html.erb index acd931434..1560505d4 100644 --- a/app/views/layouts/_signup_modal.html.erb +++ b/app/views/layouts/_signup_modal.html.erb @@ -6,21 +6,18 @@
<%= image_tag(cloudinary(SiteConfig.mascot_image_url, 300), - class: "mascot-image", alt: SiteConfig.mascot_image_description, loading: "lazy") %> + class: "mascot-image", + alt: SiteConfig.mascot_image_description, + loading: "lazy") %> +

Join our <%= community_qualified_name %> :)

+

<%= SiteConfig.tagline %>

- <% if SiteConfig.auth_allowed?("twitter") %> - - <% end %> - <% if SiteConfig.auth_allowed?("github") %> - - <% end %> + + <%= render partial: "shared/authentication/providers_signup_modal" %> +

We strive for transparency and don't collect excess data.

diff --git a/app/views/shared/authentication/_providers_nav_menu.html.erb b/app/views/shared/authentication/_providers_nav_menu.html.erb new file mode 100644 index 000000000..06ddbefff --- /dev/null +++ b/app/views/shared/authentication/_providers_nav_menu.html.erb @@ -0,0 +1,10 @@ +<% Authentication::Providers.enabled.each do |provider_name| %> + <% provider = Authentication::Providers.get!(provider_name) %> + + " + class="crayons-link crayons-link--block pl-5" + data-no-instant> + Via <%= provider.official_name %> + +<% end %> diff --git a/app/views/shared/authentication/_providers_registration_form.html.erb b/app/views/shared/authentication/_providers_registration_form.html.erb new file mode 100644 index 000000000..b24f52461 --- /dev/null +++ b/app/views/shared/authentication/_providers_registration_form.html.erb @@ -0,0 +1,10 @@ +<% Authentication::Providers.enabled.each do |provider_name| %> + <% provider = Authentication::Providers.get!(provider_name) %> + + " + 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 %> + +<% end %> diff --git a/app/views/shared/authentication/_providers_sidebar.html.erb b/app/views/shared/authentication/_providers_sidebar.html.erb new file mode 100644 index 000000000..e09a67f78 --- /dev/null +++ b/app/views/shared/authentication/_providers_sidebar.html.erb @@ -0,0 +1,10 @@ +<% Authentication::Providers.enabled.each do |provider_name| %> + <% provider = Authentication::Providers.get!(provider_name) %> + + " + class="cta cta-button login-cta-button" + data-no-instant> + Sign In With <%= provider.official_name %> + +<% end %> diff --git a/app/views/shared/authentication/_providers_signup_modal.html.erb b/app/views/shared/authentication/_providers_signup_modal.html.erb new file mode 100644 index 000000000..640ad49f5 --- /dev/null +++ b/app/views/shared/authentication/_providers_signup_modal.html.erb @@ -0,0 +1,13 @@ +<% Authentication::Providers.enabled.each do |provider_name| %> + <% provider = Authentication::Providers.get!(provider_name) %> + + " + class="sign-up-link cta" + data-no-instant> + " + class="icon-img" + alt="<%= provider.official_name %> logo" /> Auth With <%= provider.official_name %> + +<% end %> diff --git a/app/views/users/_additional_authentication.html.erb b/app/views/users/_additional_authentication.html.erb index 0e741442d..fa8d86c95 100644 --- a/app/views/users/_additional_authentication.html.erb +++ b/app/views/users/_additional_authentication.html.erb @@ -1,16 +1,17 @@ -<% if (SiteConfig.auth_allowed?("github") || SiteConfig.auth_allowed?("twitter")) && (!@user.identities.exists?(provider: "github") || !@user.identities.exists?(provider: "twitter")) %> -
-
- <% if SiteConfig.auth_allowed?("github") && !@user.identities.exists?(provider: "github") %> - - <%= inline_svg_tag("github.svg", aria: true, class: "crayons-icon", title: "GitHub") %>Connect GitHub Account - - <% end %> - <% if SiteConfig.auth_allowed?("twitter") && !@user.identities.exists?(provider: "twitter") %> - " class="crayons-btn crayons-btn--outlined" data-no-instant> - <%= inline_svg_tag("twitter.svg", aria: true, class: "crayons-icon", title: "GitHub") %>Connect Twitter Account - - <% end %> -
-
+<% Authentication::Providers.enabled.each do |provider_name| %> + <% unless @user.identities.exists?(provider: provider_name) %> + <% provider = Authentication::Providers.get!(provider_name) %> + + <%= inline_svg_tag( + "#{provider_name}.svg", + aria: true, + class: "crayons-icon", + title: provider_name.to_s.titleize.to_s, + ) %> + Connect <%= provider.official_name %> Account + + <% end %> <% end %> diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 4b2fc3283..92729600c 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -23,7 +23,7 @@ FactoryBot.define do email_digest_periodic { false } trait :with_identity do - transient { identities { %i[github twitter] } } + transient { identities { Authentication::Providers.available } } after(:create) do |user, options| options.identities.each do |provider| diff --git a/spec/requests/user/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb index e27f42e93..06a6bc034 100644 --- a/spec/requests/user/user_settings_spec.rb +++ b/spec/requests/user/user_settings_spec.rb @@ -62,22 +62,6 @@ RSpec.describe "UserSettings", type: :request do expect(response.body).to include ghost_account_message end - it "renders CONNECT_WITH_TWITTER and user with only github identity" do - user.identities.where(provider: "twitter").delete_all - get "/settings" - expect(response.body).to include "Connect Twitter Account" - end - - it "renders does not render CONNECT_WITH_TWITTER if SiteConfig does not include Twitter auth" do - user.identities.where(provider: "twitter").destroy_all - current_auth_value = SiteConfig.authentication_providers - SiteConfig.authentication_providers = ["github"] - SiteConfig.clear_cache - get "/settings" - expect(response.body).not_to include "Connect Twitter Account" - SiteConfig.authentication_providers = current_auth_value # restore prior value - end - it "renders the proper organization page" do first_org, second_org = create_list(:organization, 2) create(:organization_membership, user: user, organization: first_org) @@ -92,6 +76,43 @@ RSpec.describe "UserSettings", type: :request do expect(response.body).to include "Editing a response template" end end + + describe "connect providers accounts" do + before do + mock_auth_hash + end + + it "does not render the text for the enabled provider the user has an identity for" do + allow(Authentication::Providers).to receive(:enabled).and_return(Authentication::Providers.available) + user = create(:user, :with_identity, identities: [:github]) + + sign_in user + get "/settings" + + expect(response.body).not_to include("Connect GitHub Account") + end + + it "does not render the text for the disabled provider the user has an identity for" do + providers = Authentication::Providers.available - %i[github] + allow(Authentication::Providers).to receive(:enabled).and_return(providers) + user = create(:user, :with_identity, identities: [:github]) + + sign_in user + get "/settings" + + expect(response.body).not_to include("Connect GitHub Account") + end + + it "renders the text for the enabled provider the user has no identity for" do + allow(Authentication::Providers).to receive(:enabled).and_return(Authentication::Providers.available) + user = create(:user, :with_identity, identities: [:twitter]) + + sign_in user + get "/settings" + + expect(response.body).to include("Connect GitHub Account") + end + end end describe "PUT /update/:id" do diff --git a/spec/services/authentication/providers_spec.rb b/spec/services/authentication/providers_spec.rb new file mode 100644 index 000000000..3de70558f --- /dev/null +++ b/spec/services/authentication/providers_spec.rb @@ -0,0 +1,93 @@ +require "rails_helper" + +RSpec.describe Authentication::Providers, type: :service do + describe ".get!" do + it "raises an exception if a provider is not available" do + expect do + described_class.get!(:unknown) + end.to raise_error(Authentication::Errors::ProviderNotFound) + end + + it "raises an exception if a provider is available but not enabled" do + allow(SiteConfig).to receive(:authentication_providers).and_return(%w[github]) + + expect do + described_class.get!(:twitter) + end.to raise_error(Authentication::Errors::ProviderNotEnabled) + end + + it "loads the correct provider class" do + is_subclass_of = ( + described_class.get!(:twitter) < Authentication::Providers::Provider + ) + expect(is_subclass_of).to be(true) + end + end + + describe ".available" do + it "lists the available providers" do + available_providers = %i[github twitter] + expect(described_class.available).to eq(available_providers) + end + end + + describe ".enabled" do + it "lists all available providers" do + expect(described_class.available).to eq(described_class.enabled) + end + + context "when one of the available providers is disabled" do + it "only lists those that remain enabled" do + allow(SiteConfig).to receive(:authentication_providers).and_return(%w[github]) + + expect(described_class.enabled).to eq(%i[github]) + end + end + end + + describe ".enabled?" do + it "returns true if a provider is enabled" do + allow(SiteConfig).to receive(:authentication_providers).and_return(%w[github]) + + expect(described_class.enabled?(:github)).to be(true) + end + + it "returns false if a provider is not enabled" do + allow(SiteConfig).to receive(:authentication_providers).and_return(%w[twitter]) + + 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/system/authentication/user_logs_in_with_github_spec.rb b/spec/system/authentication/user_logs_in_with_github_spec.rb index d235d3d08..cb3d4b7e1 100644 --- a/spec/system/authentication/user_logs_in_with_github_spec.rb +++ b/spec/system/authentication/user_logs_in_with_github_spec.rb @@ -49,6 +49,10 @@ RSpec.describe "Authenticating with GitHub" do end context "when using invalid credentials" do + let(:params) do + '{"callback_url"=>"http://localhost:3000/users/auth/github/callback", "state"=>"navbar_basic"}' + end + before do mock_auth_with_invalid_credentials(:github) @@ -86,7 +90,7 @@ RSpec.describe "Authenticating with GitHub" do visit root_path click_link sign_in_link - args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + args = omniauth_failure_args(error, "github", params) expect(DatadogStatsClient).to have_received(:increment).with( "omniauth.failure", *args ) @@ -102,7 +106,7 @@ RSpec.describe "Authenticating with GitHub" do visit root_path click_link sign_in_link - args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + args = omniauth_failure_args(error, "github", params) expect(DatadogStatsClient).to have_received(:increment).with( "omniauth.failure", *args ) @@ -115,7 +119,7 @@ RSpec.describe "Authenticating with GitHub" do visit root_path click_link sign_in_link - args = omniauth_failure_args(error, "github", '{"state"=>"navbar_basic"}') + args = omniauth_failure_args(error, "github", params) expect(DatadogStatsClient).to have_received(:increment).with( "omniauth.failure", *args ) diff --git a/spec/views/users/edit.html.erb_spec.rb b/spec/views/users/edit.html.erb_spec.rb deleted file mode 100644 index f6acb9e44..000000000 --- a/spec/views/users/edit.html.erb_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -require "rails_helper" - -RSpec.describe "users/edit", type: :view do - let(:user) { create(:user) } - - context "when on profile edit" do - before do - assign(:tab, "profile") - assign(:user, user) - assign(:tab_list, user.settings_tab_list) - allow(view).to receive(:current_user).and_return(user) - end - - it "asks user to connect with github when it's missing" do - create(:identity, user: user, provider: "twitter") - - render - expect(rendered).to match(/Connect GitHub Account/) - expect(rendered).not_to match(/Connect Twitter Account/) - end - - it "asks user to connect with twitter if it's missing" do - create(:identity, user: user, provider: "github") - - render - expect(rendered).to match(/Connect Twitter Account/) - expect(rendered).not_to match(/Connect GitHub Account/) - end - end -end