From 812add18bdb071d757a6faff4358f0252122f946 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 31 Jul 2018 16:18:50 +0100 Subject: [PATCH] REFACTOR: Serve auth provider information in the site serializer. At the moment core providers are hard-coded in Javascript, and plugin providers get added to the JS payload at compile time. This refactor means that we only ship enabled providers to the client. --- .../controllers/preferences/account.js.es6 | 2 +- .../preferences/second-factor.js.es6 | 6 +- .../discourse/models/login-method.js.es6 | 108 ++---------------- .../templates/preferences/account.hbs | 4 +- .../users/omniauth_callbacks_controller.rb | 9 -- app/controllers/users_controller.rb | 2 +- app/models/site.rb | 4 + app/models/user.rb | 1 - app/serializers/auth_provider_serializer.rb | 25 ++++ app/serializers/site_serializer.rb | 3 +- config/site_settings.yml | 6 - lib/auth.rb | 1 + lib/auth/auth_provider.rb | 28 +++++ lib/discourse.rb | 21 +++- lib/plugin/auth_provider.rb | 31 ----- lib/plugin/instance.rb | 38 +----- .../discourse_plugin_registry_spec.rb | 2 +- spec/components/discourse_spec.rb | 6 +- spec/fixtures/plugins/my_plugin/plugin.rb | 4 + spec/models/site_spec.rb | 6 + .../omniauth_callbacks_controller_spec.rb | 2 +- spec/requests/users_controller_spec.rb | 63 ++++++++-- .../preferences-second-factor-test.js.es6 | 13 --- .../javascripts/fixtures/site-fixtures.js.es6 | 14 +++ 24 files changed, 184 insertions(+), 215 deletions(-) create mode 100644 app/serializers/auth_provider_serializer.rb create mode 100644 lib/auth/auth_provider.rb delete mode 100644 lib/plugin/auth_provider.rb diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 index 2ca1efcb83..1dd6e3d949 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 @@ -76,7 +76,7 @@ export default Ember.Controller.extend( }); return result.filter(value => { - return value.account || value.method.get("canConnect"); + return value.account || value.method.get("can_connect"); }); }, diff --git a/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 index 809f80278d..e483885c91 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 @@ -1,7 +1,7 @@ import { default as computed } from "ember-addons/ember-computed-decorators"; import { default as DiscourseURL, userPath } from "discourse/lib/url"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { LOGIN_METHODS } from "discourse/models/login-method"; +import { findAll } from "discourse/models/login-method"; export default Ember.Controller.extend({ loading: false, @@ -33,9 +33,7 @@ export default Ember.Controller.extend({ @computed displayOAuthWarning() { - return LOGIN_METHODS.some(name => { - return this.siteSettings[`enable_${name}_logins`]; - }); + return findAll().length > 0; }, toggleSecondFactor(enable) { diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6 index 9b4d83d423..143deec081 100644 --- a/app/assets/javascripts/discourse/models/login-method.js.es6 +++ b/app/assets/javascripts/discourse/models/login-method.js.es6 @@ -3,38 +3,23 @@ import computed from "ember-addons/ember-computed-decorators"; const LoginMethod = Ember.Object.extend({ @computed title() { - const titleSetting = this.get("titleSetting"); - if (!Ember.isEmpty(titleSetting)) { - const result = this.siteSettings[titleSetting]; - if (!Ember.isEmpty(result)) { - return result; - } - } - return ( - this.get("titleOverride") || I18n.t(`login.${this.get("name")}.title`) + this.get("title_override") || I18n.t(`login.${this.get("name")}.title`) ); }, @computed prettyName() { - const prettyNameSetting = this.get("prettyNameSetting"); - if (!Ember.isEmpty(prettyNameSetting)) { - const result = this.siteSettings[prettyNameSetting]; - if (!Ember.isEmpty(result)) { - return result; - } - } - return ( - this.get("prettyNameOverride") || I18n.t(`login.${this.get("name")}.name`) + this.get("pretty_name_override") || + I18n.t(`login.${this.get("name")}.name`) ); }, @computed message() { return ( - this.get("messageOverride") || + this.get("message_override") || I18n.t("login." + this.get("name") + ".message") ); }, @@ -46,18 +31,9 @@ const LoginMethod = Ember.Object.extend({ if (customLogin) { customLogin(); } else { - let authUrl = this.get("customUrl") || Discourse.getURL("/auth/" + name); + let authUrl = this.get("custom_url") || Discourse.getURL("/auth/" + name); - // first check if this plugin has a site setting for full screen login before using the static setting - let fullScreenLogin = false; - const fullScreenLoginSetting = this.get("fullScreenLoginSetting"); - if (!Ember.isEmpty(fullScreenLoginSetting)) { - fullScreenLogin = this.siteSettings[fullScreenLoginSetting]; - } else { - fullScreenLogin = this.get("fullScreenLogin"); - } - - if (fullScreenLogin) { + if (this.get("full_screen_login")) { document.cookie = "fsl=true"; window.location = authUrl; } else { @@ -65,10 +41,10 @@ const LoginMethod = Ember.Object.extend({ const left = this.get("lastX") - 400; const top = this.get("lastY") - 200; - const height = this.get("frameHeight") || 400; - const width = this.get("frameWidth") || 800; + const height = this.get("frame_height") || 400; + const width = this.get("frame_width") || 800; - if (this.get("displayPopup")) { + if (name === "facebook") { authUrl = authUrl + "?display=popup"; } @@ -97,16 +73,6 @@ const LoginMethod = Ember.Object.extend({ }); let methods; -let preRegister; - -export const LOGIN_METHODS = [ - "google_oauth2", - "facebook", - "twitter", - "yahoo", - "instagram", - "github" -]; export function findAll(siteSettings, capabilities, isMobileDevice) { if (methods) { @@ -115,66 +81,16 @@ export function findAll(siteSettings, capabilities, isMobileDevice) { methods = []; - LOGIN_METHODS.forEach(name => { - if (siteSettings["enable_" + name + "_logins"]) { - const params = { name }; - if (name === "google_oauth2") { - params.frameWidth = 850; - params.frameHeight = 500; - } else if (name === "facebook") { - params.frameWidth = 580; - params.frameHeight = 400; - params.displayPopup = true; - } - - if ( - [ - "facebook", - "google_oauth2", - "twitter", - "yahoo", - "github", - "instagram" - ].includes(name) - ) { - params.canConnect = true; - } - - params.siteSettings = siteSettings; - methods.pushObject(LoginMethod.create(params)); - } + Discourse.Site.currentProp("auth_providers").forEach(provider => { + methods.pushObject(LoginMethod.create(provider)); }); - if (preRegister) { - preRegister.forEach(method => { - const enabledSetting = method.get("enabledSetting"); - if (enabledSetting) { - if (siteSettings[enabledSetting]) { - methods.pushObject(method); - } - } else { - methods.pushObject(method); - } - }); - preRegister = undefined; - } - // On Mobile, Android or iOS always go with full screen if (isMobileDevice || capabilities.isIOS || capabilities.isAndroid) { - methods.forEach(m => m.set("fullScreenLogin", true)); + methods.forEach(m => m.set("full_screen_login", true)); } return methods; } -export function register(method) { - method = LoginMethod.create(method); - if (methods) { - methods.pushObject(method); - } else { - preRegister = preRegister || []; - preRegister.push(method); - } -} - export default LoginMethod; diff --git a/app/assets/javascripts/discourse/templates/preferences/account.hbs b/app/assets/javascripts/discourse/templates/preferences/account.hbs index 2a793ee5f4..276caf9c0f 100644 --- a/app/assets/javascripts/discourse/templates/preferences/account.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/account.hbs @@ -111,7 +111,7 @@ {{#if authProvider.account}} {{authProvider.account.description}} - {{#if authProvider.account.can_revoke}} + {{#if authProvider.method.can_revoke}} {{#conditional-loading-spinner condition=revoking size='small'}} {{d-button action="revokeAccount" actionParam=authProvider.account title="user.associated_accounts.revoke" icon="times-circle" }} {{/conditional-loading-spinner}} @@ -119,7 +119,7 @@ {{else}} - {{#if authProvider.method.canConnect}} + {{#if authProvider.method.can_connect}} {{d-button action="connectAccount" actionParam=authProvider.method label="user.associated_accounts.connect" icon="plug" disabled=disableConnectButtons}} {{else}} {{i18n 'user.associated_accounts.not_connected'}} diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 2731bb441b..c102d861de 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -5,15 +5,6 @@ require_dependency 'user_name_suggester' class Users::OmniauthCallbacksController < ApplicationController - BUILTIN_AUTH = [ - Auth::FacebookAuthenticator.new, - Auth::GoogleOAuth2Authenticator.new, - Auth::OpenIdAuthenticator.new("yahoo", "https://me.yahoo.com", 'enable_yahoo_logins', trusted: true), - Auth::GithubAuthenticator.new, - Auth::TwitterAuthenticator.new, - Auth::InstagramAuthenticator.new - ] - skip_before_action :redirect_to_login_if_required layout 'no_ember' diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 63d675a320..13f4f604a2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1080,7 +1080,7 @@ class UsersController < ApplicationController # Using Discourse.authenticators rather than Discourse.enabled_authenticators so users can # revoke permissions even if the admin has temporarily disabled that type of login authenticator = Discourse.authenticators.find { |authenticator| authenticator.name == provider_name } - raise Discourse::NotFound if authenticator.nil? + raise Discourse::NotFound if authenticator.nil? || !authenticator.can_revoke? skip_remote = params.permit(:skip_remote) diff --git a/app/models/site.rb b/app/models/site.rb index b3001e49d9..ec5cec19d4 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -79,6 +79,10 @@ class Site Archetype.list.reject { |t| t.id == Archetype.private_message } end + def auth_providers + Discourse.enabled_auth_providers + end + def self.json_for(guardian) if guardian.anonymous? && SiteSetting.login_required diff --git a/app/models/user.rb b/app/models/user.rb index 8a370224ea..08ec67d06c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -958,7 +958,6 @@ class User < ActiveRecord::Base result << { name: authenticator.name, description: account_description, - can_revoke: authenticator.can_revoke? } end end diff --git a/app/serializers/auth_provider_serializer.rb b/app/serializers/auth_provider_serializer.rb new file mode 100644 index 0000000000..8e5d8b410d --- /dev/null +++ b/app/serializers/auth_provider_serializer.rb @@ -0,0 +1,25 @@ +class AuthProviderSerializer < ApplicationSerializer + + attributes :name, :custom_url, :pretty_name_override, :title_override, :message_override, + :frame_width, :frame_height, :full_screen_login, :can_connect, :can_revoke + + def title_override + return SiteSetting.send(object.title_setting) if object.title_setting + object.title + end + + def pretty_name_override + return SiteSetting.send(object.pretty_name_setting) if object.pretty_name_setting + object.pretty_name + end + + def full_screen_login + return SiteSetting.send(object.full_screen_login_setting) if object.full_screen_login_setting + false + end + + def message_override + object.message + end + +end diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 467542ab36..4b162bf128 100644 --- a/app/serializers/site_serializer.rb +++ b/app/serializers/site_serializer.rb @@ -35,7 +35,8 @@ class SiteSerializer < ApplicationSerializer has_many :categories, serializer: BasicCategorySerializer, embed: :objects has_many :trust_levels, embed: :objects has_many :archetypes, embed: :objects, serializer: ArchetypeSerializer - has_many :user_fields, embed: :objects, serialzer: UserFieldSerializer + has_many :user_fields, embed: :objects, serializer: UserFieldSerializer + has_many :auth_providers, embed: :objects, serializer: AuthProviderSerializer def user_themes cache_fragment("user_themes") do diff --git a/config/site_settings.yml b/config/site_settings.yml index 577e983b0d..da0e32d891 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -271,7 +271,6 @@ login: client: true default: true enable_google_oauth2_logins: - client: true default: false google_oauth2_client_id: '' google_oauth2_client_secret: @@ -288,10 +287,8 @@ login: google_oauth2_hd: default: '' enable_yahoo_logins: - client: true default: false enable_twitter_logins: - client: true default: false twitter_consumer_key: default: '' @@ -301,7 +298,6 @@ login: regex: "^[\\w+-]+$" secret: true enable_instagram_logins: - client: true default: false instagram_consumer_key: default: '' @@ -311,7 +307,6 @@ login: regex: "^[a-z0-9]+$" secret: true enable_facebook_logins: - client: true default: false facebook_app_id: default: '' @@ -321,7 +316,6 @@ login: regex: "^[a-f0-9]+$" secret: true enable_github_logins: - client: true default: false github_client_id: default: '' diff --git a/lib/auth.rb b/lib/auth.rb index f94ba5cf43..7d0c47db99 100644 --- a/lib/auth.rb +++ b/lib/auth.rb @@ -1,5 +1,6 @@ module Auth; end +require_dependency 'auth/auth_provider' require_dependency 'auth/result' require_dependency 'auth/authenticator' require_dependency 'auth/facebook_authenticator' diff --git a/lib/auth/auth_provider.rb b/lib/auth/auth_provider.rb new file mode 100644 index 0000000000..5f74af2e44 --- /dev/null +++ b/lib/auth/auth_provider.rb @@ -0,0 +1,28 @@ +class Auth::AuthProvider + include ActiveModel::Serialization + + def initialize(params = {}) + params.each { |key, value| send "#{key}=", value } + end + + def self.auth_attributes + [:pretty_name, :title, :message, :frame_width, :frame_height, :authenticator, + :pretty_name_setting, :title_setting, :enabled_setting, :full_screen_login, :full_screen_login_setting, + :custom_url] + end + + attr_accessor(*auth_attributes) + + def name + authenticator.name + end + + def can_connect + authenticator.can_connect_existing_user? + end + + def can_revoke + authenticator.can_revoke? + end + +end diff --git a/lib/discourse.rb b/lib/discourse.rb index a00aaea1e4..4ebb87eb72 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -200,12 +200,27 @@ module Discourse end end + BUILTIN_AUTH = [ + Auth::AuthProvider.new(authenticator: Auth::FacebookAuthenticator.new, frame_width: 580, frame_height: 400), + Auth::AuthProvider.new(authenticator: Auth::GoogleOAuth2Authenticator.new, frame_width: 850, frame_height: 500), + Auth::AuthProvider.new(authenticator: Auth::OpenIdAuthenticator.new("yahoo", "https://me.yahoo.com", 'enable_yahoo_logins', trusted: true)), + Auth::AuthProvider.new(authenticator: Auth::GithubAuthenticator.new), + Auth::AuthProvider.new(authenticator: Auth::TwitterAuthenticator.new), + Auth::AuthProvider.new(authenticator: Auth::InstagramAuthenticator.new, frame_width: 1, frame_height: 1) + ] + + def self.auth_providers + BUILTIN_AUTH + DiscoursePluginRegistry.auth_providers.to_a + end + + def self.enabled_auth_providers + auth_providers.select { |provider| provider.authenticator.enabled? } + end + def self.authenticators # NOTE: this bypasses the site settings and gives a list of everything, we need to register every middleware # for the cases of multisite - # In future we may change it so we don't include them all for cases where we are not a multisite, but we would - # require a restart after site settings change - Users::OmniauthCallbacksController::BUILTIN_AUTH + DiscoursePluginRegistry.auth_providers.map(&:authenticator) + auth_providers.map(&:authenticator) end def self.enabled_authenticators diff --git a/lib/plugin/auth_provider.rb b/lib/plugin/auth_provider.rb deleted file mode 100644 index 574962f942..0000000000 --- a/lib/plugin/auth_provider.rb +++ /dev/null @@ -1,31 +0,0 @@ -class Plugin::AuthProvider - - def self.auth_attributes - [:glyph, :background_color, :pretty_name, :title, :message, :frame_width, :frame_height, :authenticator, - :pretty_name_setting, :title_setting, :enabled_setting, :full_screen_login, :full_screen_login_setting, - :custom_url] - end - - attr_accessor(*auth_attributes) - - def name - authenticator.name - end - - def to_json - result = { name: name } - result['customUrl'] = custom_url if custom_url - result['prettyNameOverride'] = pretty_name || name - result['titleOverride'] = title if title - result['titleSetting'] = title_setting if title_setting - result['prettyNameSetting'] = pretty_name_setting if pretty_name_setting - result['enabledSetting'] = enabled_setting if enabled_setting - result['messageOverride'] = message if message - result['frameWidth'] = frame_width if frame_width - result['frameHeight'] = frame_height if frame_height - result['fullScreenLogin'] = full_screen_login if full_screen_login - result['fullScreenLoginSetting'] = full_screen_login_setting if full_screen_login_setting - result.to_json - end - -end diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 3eb543c1fc..2d5a3c0a56 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -1,7 +1,7 @@ require 'digest/sha1' require 'fileutils' require_dependency 'plugin/metadata' -require_dependency 'plugin/auth_provider' +require_dependency 'lib/auth' class Plugin::CustomEmoji def self.cache_key @@ -393,38 +393,6 @@ class Plugin::Instance css = styles.join("\n") js = javascripts.join("\n") - auth_providers.each do |auth| - - auth_json = auth.to_json - hash = Digest::SHA1.hexdigest(auth_json) - js << <