maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[deploy] Authentication refactoring: add available and enabled providers (#7505)

Browse source 
* Add Providers(.available|.enabled|.enabled?)

* Add provider_sidebar partial

* Add providers_signup_modal partial and Providers.sign_in_url

* Use path helpers

* Add providers_nav_menu partial

* Add providers_registration_form partial

* Generalize users/additional_authentication

* Refactor sign_in_path and authentication_path

* Add .official_name and fix specs

* Preload authentication providers correctly and use less Ruby magic

* Put require_dependency in the correct place
This commit is contained in:
rhymes 2020-04-28 16:16:28 +02:00 • committed by GitHub
parent 8f2c32249c
commit 883eb170de
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
20 changed files with 306 additions and 116 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
app/models/site_config.rb
View file

@ -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

3
app/services/authentication/errors.rb
View file

@ -5,5 +5,8 @@ module Authentication
class ProviderNotFound < Error
end
class ProviderNotEnabled < Error
end
end
end

69
app/services/authentication/providers.rb
View file

@ -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 <config/initializers/authentication_providers.rb>
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

5
app/services/authentication/providers/github.rb
View file

@ -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)

5
app/services/authentication/providers/provider.rb
View file

@ -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

5
app/services/authentication/providers/twitter.rb
View file

@ -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)

7
app/views/articles/_sidebar_additional.html.erb
View file

@ -7,12 +7,7 @@
Join <%= community_name %>
</header>
<div class="widget-body">
<% if SiteConfig.auth_allowed?("twitter") %>
<a href="/users/auth/twitter?callback_url=<%= app_url("/users/auth/twitter/callback") %>" class="cta cta-button login-cta-button" data-no-instant>Sign In With Twitter</a>
<% end %>
<% if SiteConfig.auth_allowed?("github") %>
<a href="/users/auth/github?state=navbar_basic" class="cta cta-button login-cta-button" data-no-instant>Sign In With GitHub</a>
<% end %>
<%= render partial: "shared/authentication/providers_sidebar" %>
</div>
</div>
<% end %>

12
app/views/devise/registrations/_registration_form.html.erb
View file

@ -74,16 +74,8 @@
<h1>Great to have you</h1>
<% end %>
<div class="links">
<% if SiteConfig.auth_allowed?("twitter") %>
<a href="/users/auth/twitter?callback_url=<%= app_url("/users/auth/twitter/callback") %>" 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
</a>
<% end %>
<% if SiteConfig.auth_allowed?("github") %>
<a href="/users/auth/github?state=join-club-page_basic" class="sign-up-link" data-no-instant>
<%= inline_svg_tag("github-logo.svg", class: "icon-img", aria: true, title: "github logo") %> Sign In with GitHub
</a>
<% end %>
<%= render partial: "shared/authentication/providers_registration_form" %>
<p><em>We require social login to prevent abuse.</em></p>
</div>
<p>Open Source 😇</p>

30
app/views/layouts/_nav_menu.html.erb
View file

@ -3,27 +3,29 @@
<div id="user-profile-link-placeholder" class="border-0 border-b-1 border-solid border-base-20 p-1">...</div>
<div class="border-0 border-b-1 border-solid border-base-20 p-1">
<a href="/dashboard" class="crayons-link crayons-link--block">Dashboard</a>
<a href="/new" class="crayons-link crayons-link--block">Write a Post</a>
<a href="/readinglist" class="crayons-link crayons-link--block">Reading List</a>
<a href="/settings" class="crayons-link crayons-link--block">Settings</a>
<a href="/p/information" class="crayons-link crayons-link--block" id="second-last-nav-link">Key Links</a>
<a href="<%= dashboard_path %>" class="crayons-link crayons-link--block">Dashboard</a>
<a href="<%= new_path %>" class="crayons-link crayons-link--block">Write a Post</a>
<a href="<%= readinglist_path %>" class="crayons-link crayons-link--block">Reading List</a>
<a href="<%= user_settings_path %>" class="crayons-link crayons-link--block">Settings</a>
<a href="<%= information_path %>" class="crayons-link crayons-link--block" id="second-last-nav-link">Key Links</a>
</div>
<div class="p-1">
<a href="/signout_confirm" class="crayons-link crayons-link--block" id="last-nav-link">Sign Out</a>
<a href="<%= signout_confirm_path %>" class="crayons-link crayons-link--block" id="last-nav-link">Sign Out</a>
</div>
<% else %>
<div class="border-0 border-b-1 border-solid border-base-20 p-1">
<a href="/enter" data-no-instant id="first-nav-link" class="crayons-link crayons-link--block fw-medium">Sign In/Up</a>
<% if SiteConfig.auth_allowed?("twitter") %>
<a href="/users/auth/twitter?callback_url=<%= app_url("/users/auth/twitter/callback") %>" class="crayons-link crayons-link--block pl-5" data-no-instant>Via Twitter</a>
<% end %>
<% if SiteConfig.auth_allowed?("github") %>
<a href="/users/auth/github?state=navbar_basic" class="crayons-link crayons-link--block pl-5" data-no-instant id="second-last-nav-link">Via GitHub</a>
<% end %>
<a
href="<%= sign_up_path %>"
id="first-nav-link"
class="crayons-link crayons-link--block fw-medium"
data-no-instant>
Sign In/Up
</a>
<%= render partial: "shared/authentication/providers_nav_menu" %>
</div>
<div class="p-1">
<a href="/p/information" class="crayons-link crayons-link--block" id="last-nav-link">All about <%= community_name %></a>
<a href="<%= information_path %>" class="crayons-link crayons-link--block" id="last-nav-link">All about <%= community_name %></a>
</div>
<% end %>
</div>

19
app/views/layouts/_signup_modal.html.erb
View file

@ -6,21 +6,18 @@
</div>
<div class="global-signup-modal--inner-a">
<%= 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") %>
<h1>Join our <%= community_qualified_name %> :)</h1>
<p>
<%= SiteConfig.tagline %>
</p>
<% if SiteConfig.auth_allowed?("twitter") %>
<a href="/users/auth/twitter?callback_url=<%= app_url %>/users/auth/twitter/callback" class="sign-up-link cta" data-no-instant>
<img src="<%= asset_path("twitter-logo.svg") %>" class="icon-img" alt="Twitter logo" /> Auth With Twitter
</a>
<% end %>
<% if SiteConfig.auth_allowed?("github") %>
<a href="/users/auth/github?state=signup-modal" class="sign-up-link cta" data-no-instant>
<img src="<%= asset_path("github-logo.svg") %>" class="icon-img" alt="GitHub logo" /> Auth With GitHub
</a>
<% end %>
<%= render partial: "shared/authentication/providers_signup_modal" %>
<p>
<em>We strive for transparency and don't collect excess data.</em>
</p>

10
app/views/shared/authentication/_providers_nav_menu.html.erb Normal file
View file

@ -0,0 +1,10 @@
<% Authentication::Providers.enabled.each do |provider_name| %>
<% provider = Authentication::Providers.get!(provider_name) %>
<a
href="<%= Authentication::Providers.sign_in_path(provider_name, state: "navbar_basic") %>"
class="crayons-link crayons-link--block pl-5"
data-no-instant>
Via <%= provider.official_name %>
</a>
<% end %>

10
app/views/shared/authentication/_providers_registration_form.html.erb Normal file
View file

@ -0,0 +1,10 @@
<% Authentication::Providers.enabled.each do |provider_name| %>
<% provider = Authentication::Providers.get!(provider_name) %>
<a
href="<%= Authentication::Providers.sign_in_path(provider_name, state: "join-club-page_basic") %>"
class="sign-up-link"
data-no-instant>
<%= inline_svg_tag("#{provider_name}-logo.svg", class: "icon-img", aria: true, title: "#{provider_name} logo") %> Sign In with <%= provider.official_name %>
</a>
<% end %>

10
app/views/shared/authentication/_providers_sidebar.html.erb Normal file
View file

@ -0,0 +1,10 @@
<% Authentication::Providers.enabled.each do |provider_name| %>
<% provider = Authentication::Providers.get!(provider_name) %>
<a
href="<%= Authentication::Providers.sign_in_path(provider_name, state: "navbar_basic") %>"
class="cta cta-button login-cta-button"
data-no-instant>
Sign In With <%= provider.official_name %>
</a>
<% end %>

13
app/views/shared/authentication/_providers_signup_modal.html.erb Normal file
View file

@ -0,0 +1,13 @@
<% Authentication::Providers.enabled.each do |provider_name| %>
<% provider = Authentication::Providers.get!(provider_name) %>
<a
href="<%= Authentication::Providers.sign_in_path(provider_name, state: "signup-modal") %>"
class="sign-up-link cta"
data-no-instant>
<img
src="<%= asset_path("#{provider_name}-logo.svg") %>"
class="icon-img"
alt="<%= provider.official_name %> logo" /> Auth With <%= provider.official_name %>
</a>
<% end %>

31
app/views/users/_additional_authentication.html.erb
View file

@ -1,16 +1,17 @@
<% if (SiteConfig.auth_allowed?("github") || SiteConfig.auth_allowed?("twitter")) && (!@user.identities.exists?(provider: "github") || !@user.identities.exists?(provider: "twitter")) %>
<div class="crayons-card mb-6 p-6">
<div class="flex">
<% if SiteConfig.auth_allowed?("github") && !@user.identities.exists?(provider: "github") %>
<a href="/users/auth/github" class="crayons-btn crayons-btn--outlined mr-2" data-no-instant>
<%= inline_svg_tag("github.svg", aria: true, class: "crayons-icon", title: "GitHub") %>Connect GitHub Account
</a>
<% end %>
<% if SiteConfig.auth_allowed?("twitter") && !@user.identities.exists?(provider: "twitter") %>
<a href="/users/auth/twitter?callback_url=<%= app_url("/users/auth/twitter/callback") %>" class="crayons-btn crayons-btn--outlined" data-no-instant>
<%= inline_svg_tag("twitter.svg", aria: true, class: "crayons-icon", title: "GitHub") %>Connect Twitter Account
</a>
<% end %>
</div>
</div>
<% Authentication::Providers.enabled.each do |provider_name| %>
<% unless @user.identities.exists?(provider: provider_name) %>
<% provider = Authentication::Providers.get!(provider_name) %>
<a
href="<%= Authentication::Providers.authentication_path(provider_name) %>"
class="crayons-btn crayons-btn--outlined mr-2"
data-no-instant>
<%= inline_svg_tag(
"#{provider_name}.svg",
aria: true,
class: "crayons-icon",
title: provider_name.to_s.titleize.to_s,
) %>
Connect <%= provider.official_name %> Account
</a>
<% end %>
<% end %>

2
spec/factories/users.rb
View file

@ -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|

53
spec/requests/user/user_settings_spec.rb
View file

@ -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

93
spec/services/authentication/providers_spec.rb Normal file
View file

@ -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

10
spec/system/authentication/user_logs_in_with_github_spec.rb
View file

@ -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
)

30
spec/views/users/edit.html.erb_spec.rb
View file

@ -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