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.

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");
       });
     },
 

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

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;

app/assets/javascripts/discourse/templates/preferences/account.hbs

@@ -111,7 +111,7 @@
           {{#if authProvider.account}}
             <td>{{authProvider.account.description}}</td>
             <td>
-              {{#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 @@
             </td>
           {{else}}
             <td colspan=2>
-              {{#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'}}

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'

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)
 

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

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

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

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

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: ''

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'

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

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

lib/plugin/auth_provider.rb

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

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 << <<JS
-define("discourse/initializers/login-method-#{hash}",
-  ["discourse/models/login-method", "exports"],
-  function(module, __exports__) {
-    "use strict";
-    __exports__["default"] = {
-      name: "login-method-#{hash}",
-      after: "inject-objects",
-      initialize: function(container) {
-        if (Ember.testing) { return; }
-
-        var authOpts = #{auth_json};
-        authOpts.siteSettings = container.lookup('site-settings:main');
-        module.register(authOpts);
-      }
-    };
-  });
-JS
-
-      if auth.glyph
-        css << ".btn-social.#{auth.name}:before{ content: '#{auth.glyph}'; }\n"
-      end
-
-      if auth.background_color
-        css << ".btn-social.#{auth.name}{ background: #{auth.background_color}; }\n"
-      end
-    end
-
     # Generate an IIFE for the JS
     js = "(function(){#{js}})();" if js.present?
 
@@ -495,9 +463,9 @@ JS
   end
 
   def auth_provider(opts)
-    provider = Plugin::AuthProvider.new
+    provider = Auth::AuthProvider.new
 
-    Plugin::AuthProvider.auth_attributes.each do |sym|
+    Auth::AuthProvider.auth_attributes.each do |sym|
       provider.send "#{sym}=", opts.delete(sym)
     end
 

spec/components/discourse_plugin_registry_spec.rb

@@ -102,7 +102,7 @@ describe DiscoursePluginRegistry do
   context '.register_auth_provider' do
     let(:registry) { DiscoursePluginRegistry }
     let(:auth_provider) do
-      provider = Plugin::AuthProvider.new
+      provider = Auth::AuthProvider.new
       provider.authenticator = Auth::Authenticator.new
       provider
     end

spec/components/discourse_spec.rb

@@ -61,7 +61,7 @@ describe Discourse do
 
   context 'authenticators' do
     it 'returns inbuilt authenticators' do
-      expect(Discourse.authenticators).to match_array(Users::OmniauthCallbacksController::BUILTIN_AUTH)
+      expect(Discourse.authenticators).to match_array(Discourse::BUILTIN_AUTH.map(&:authenticator))
     end
 
     context 'with authentication plugin installed' do
@@ -76,7 +76,7 @@ describe Discourse do
           end
         end
 
-        provider = Plugin::AuthProvider.new
+        provider = Auth::AuthProvider.new
         provider.authenticator = authenticator_class.new
         provider
       end
@@ -91,7 +91,7 @@ describe Discourse do
 
       it 'returns inbuilt and plugin authenticators' do
         expect(Discourse.authenticators).to match_array(
-          Users::OmniauthCallbacksController::BUILTIN_AUTH + [plugin_auth_provider.authenticator])
+          Discourse::BUILTIN_AUTH.map(&:authenticator) + [plugin_auth_provider.authenticator])
       end
 
     end

spec/fixtures/plugins/my_plugin/plugin.rb

@@ -8,3 +8,7 @@ auth_provider title: 'with Ubuntu',
               message: 'Authenticating with Ubuntu (make sure pop up blockers are not enbaled)',
               frame_width: 1000,   # the frame size used for the pop up window, overrides default
               frame_height: 800
+
+register_javascript <<JS
+  console.log("Hello world")
+JS

spec/models/site_spec.rb

@@ -66,4 +66,10 @@ describe Site do
     expect(Site.new(guardian).categories).not_to include(sub_category)
   end
 
+  it "includes all enabled authentication providers" do
+    SiteSetting.enable_twitter_logins = true
+    SiteSetting.enable_facebook_logins = true
+    expect(Site.new(Guardian.new).auth_providers.map(&:name)).to contain_exactly('facebook', 'twitter')
+  end
+
 end

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe Users::OmniauthCallbacksController do
     context "with a plugin-contributed auth provider" do
 
       let :provider do
-        provider = Plugin::AuthProvider.new
+        provider = Auth::AuthProvider.new
         provider.authenticator = Class.new(Auth::Authenticator) do
           def name
             'ubuntu'

spec/requests/users_controller_spec.rb

@@ -3097,16 +3097,65 @@ describe UsersController do
         expect(response.status).to eq(404)
       end
 
-      it 'works' do
-        FacebookUserInfo.create!(user_id: user.id, facebook_user_id: 12345, email: 'someuser@somedomain.tld')
-        stub = stub_request(:delete, 'https://graph.facebook.com/12345/permissions?access_token=123%7Cabcde').to_return(body: "true")
+      context "with fake provider" do
+        let(:authenticator) do
+          Class.new(Auth::Authenticator) do
+            attr_accessor :can_revoke
+            def name
+              "testprovider"
+            end
+
+            def enabled?
+              true
+            end
+
+            def description_for_user(user)
+              "an account"
+            end
+
+            def can_revoke?
+              can_revoke
+            end
+
+            def revoke(user, skip_remote: false)
+              true
+            end
+          end.new
+        end
+
+        before do
+          DiscoursePluginRegistry.register_auth_provider(Auth::AuthProvider.new(authenticator: authenticator))
+        end
+
+        after do
+          DiscoursePluginRegistry.reset!
+        end
+
+        it 'returns an error when revoking is not allowed' do
+          authenticator.can_revoke = false
 
           post "/u/#{user.username}/preferences/revoke-account.json", params: {
-          provider_name: 'facebook'
+            provider_name: 'testprovider'
+          }
+          expect(response.status).to eq(404)
+
+          authenticator.can_revoke = true
+          post "/u/#{user.username}/preferences/revoke-account.json", params: {
+            provider_name: 'testprovider'
           }
           expect(response.status).to eq(200)
         end
 
+        it 'works' do
+          authenticator.can_revoke = true
+
+          post "/u/#{user.username}/preferences/revoke-account.json", params: {
+            provider_name: 'testprovider'
+          }
+          expect(response.status).to eq(200)
+        end
+      end
+
     end
 
   end

test/javascripts/controllers/preferences-second-factor-test.js.es6

@@ -1,18 +1,5 @@
 moduleFor("controller:preferences/second-factor");
 
-QUnit.test(
-  "displayOAuthWarning when OAuth login methods are disabled",
-  function(assert) {
-    const controller = this.subject({
-      siteSettings: {
-        enable_google_oauth2_logins: false
-      }
-    });
-
-    assert.equal(controller.get("displayOAuthWarning"), false);
-  }
-);
-
 QUnit.test("displayOAuthWarning when OAuth login methods are enabled", function(
   assert
 ) {

test/javascripts/fixtures/site-fixtures.js.es6

@@ -634,6 +634,20 @@ export default {
           name: "translation missing: en.archetypes.banner.title",
           options: []
         }
+      ],
+      auth_providers: [
+        {
+          name: "facebook",
+          custom_url: null,
+          pretty_name_override: null,
+          title_override: null,
+          message_override: null,
+          frame_width: 580,
+          frame_height: 400,
+          full_screen_login: false,
+          can_connect: true,
+          can_revoke: true
+        }
       ]
     }
   }