From eda1462b3b8f57aace0c49b1d64edfcf3d1f45b2 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 23 Jul 2018 16:51:57 +0100 Subject: [PATCH] FEATURE: List, revoke and reconnect associated accounts. Phase 1 (#6099) Listing connections is supported for all built-in auth providers. Revoke and reconnect is currently only implemented for Facebook. --- .../admin/controllers/admin-user-index.js.es6 | 12 + .../admin/templates/user-index.hbs | 6 +- .../discourse/controllers/login.js.es6 | 45 +--- .../controllers/preferences/account.js.es6 | 61 +++++ .../discourse/models/login-method.js.es6 | 68 +++++- .../javascripts/discourse/models/user.js.es6 | 10 + .../templates/preferences/account.hbs | 39 ++++ app/assets/stylesheets/common/base/user.scss | 6 + .../users/omniauth_callbacks_controller.rb | 33 +-- app/controllers/users_controller.rb | 26 +++ app/models/user.rb | 22 +- app/serializers/user_serializer.rb | 7 +- app/services/user_anonymizer.rb | 2 +- config/locales/client.en.yml | 16 +- config/locales/server.en.yml | 5 +- config/routes.rb | 1 + lib/auth/authenticator.rb | 39 +++- lib/auth/facebook_authenticator.rb | 55 ++++- lib/auth/github_authenticator.rb | 9 + lib/auth/google_oauth2_authenticator.rb | 9 + lib/auth/instagram_authenticator.rb | 9 + lib/auth/oauth2_authenticator.rb | 4 + lib/auth/open_id_authenticator.rb | 12 +- lib/auth/twitter_authenticator.rb | 9 + lib/discourse.rb | 15 +- lib/discourse_plugin_registry.rb | 15 ++ lib/plugin/auth_provider.rb | 6 +- lib/plugin/instance.rb | 21 ++ .../auth/facebook_authenticator_spec.rb | 84 +++++++ .../auth/open_id_authenticator_spec.rb | 4 +- .../discourse_plugin_registry_spec.rb | 29 +++ spec/components/discourse_spec.rb | 48 ++++ spec/components/plugin/instance_spec.rb | 48 +++- spec/fixtures/plugins/my_plugin/plugin.rb | 2 +- spec/models/user_spec.rb | 5 +- .../omniauth_callbacks_controller_spec.rb | 25 ++- spec/requests/users_controller_spec.rb | 46 +++- spec/services/user_anonymizer_spec.rb | 4 +- .../acceptance/preferences-test.js.es6 | 212 +++++++++--------- .../javascripts/fixtures/user_fixtures.js.es6 | 7 + 40 files changed, 836 insertions(+), 240 deletions(-) diff --git a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 b/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 index c9675f210bc..2fbac162ced 100644 --- a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 @@ -40,6 +40,18 @@ export default Ember.Controller.extend(CanCheckEmails, { .join(", "); }, + @computed("model.associated_accounts") + associatedAccountsLoaded(associatedAccounts) { + return typeof associatedAccounts !== "undefined"; + }, + + @computed("model.associated_accounts") + associatedAccounts(associatedAccounts) { + return associatedAccounts + .map(provider => `${provider.name} (${provider.description})`) + .join(", "); + }, + userFields: function() { const siteUserFields = this.site.get("user_fields"), userFields = this.get("model.user_fields"); diff --git a/app/assets/javascripts/admin/templates/user-index.hbs b/app/assets/javascripts/admin/templates/user-index.hbs index 14bbf89fb85..35daa9560d6 100644 --- a/app/assets/javascripts/admin/templates/user-index.hbs +++ b/app/assets/javascripts/admin/templates/user-index.hbs @@ -109,10 +109,10 @@
-
{{i18n 'user.associated_accounts'}}
+
{{i18n 'user.associated_accounts.title'}}
- {{#if model.associated_accounts}} - {{model.associated_accounts}} + {{#if associatedAccountsLoaded}} + {{associatedAccounts}} {{else}} {{d-button action="checkEmail" actionParam=model icon="envelope-o" label="admin.users.check_email.text" title="admin.users.check_email.title"}} {{/if}} diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index c40b96e6bac..41f909bd359 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/app/assets/javascripts/discourse/controllers/login.js.es6 @@ -193,50 +193,7 @@ export default Ember.Controller.extend(ModalFunctionality, { }, externalLogin: function(loginMethod) { - const name = loginMethod.get("name"); - const customLogin = loginMethod.get("customLogin"); - - if (customLogin) { - customLogin(); - } else { - let authUrl = - loginMethod.get("customUrl") || Discourse.getURL("/auth/" + name); - if (loginMethod.get("fullScreenLogin")) { - document.cookie = "fsl=true"; - window.location = authUrl; - } else { - this.set("authenticate", name); - const left = this.get("lastX") - 400; - const top = this.get("lastY") - 200; - - const height = loginMethod.get("frameHeight") || 400; - const width = loginMethod.get("frameWidth") || 800; - - if (loginMethod.get("displayPopup")) { - authUrl = authUrl + "?display=popup"; - } - - const w = window.open( - authUrl, - "_blank", - "menubar=no,status=no,height=" + - height + - ",width=" + - width + - ",left=" + - left + - ",top=" + - top - ); - const self = this; - const timer = setInterval(function() { - if (!w || w.closed) { - clearInterval(timer); - self.set("authenticate", null); - } - }, 1000); - } - } + loginMethod.doLogin(); }, createAccount: function() { diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 index 8c464e5743d..2ca1efcb831 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 @@ -5,6 +5,7 @@ import PreferencesTabController from "discourse/mixins/preferences-tab-controlle import { setting } from "discourse/lib/computed"; import { popupAjaxError } from "discourse/lib/ajax-error"; import showModal from "discourse/lib/show-modal"; +import { findAll } from "discourse/models/login-method"; export default Ember.Controller.extend( CanCheckEmails, @@ -54,6 +55,44 @@ export default Ember.Controller.extend( ); }, + @computed("model.associated_accounts") + associatedAccountsLoaded(associatedAccounts) { + return typeof associatedAccounts !== "undefined"; + }, + + @computed("model.associated_accounts.[]") + authProviders(accounts) { + const allMethods = findAll( + this.siteSettings, + this.capabilities, + this.site.isMobileDevice + ); + + const result = allMethods.map(method => { + return { + method, + account: accounts.find(account => account.name === method.name) // Will be undefined if no account + }; + }); + + return result.filter(value => { + return value.account || value.method.get("canConnect"); + }); + }, + + @computed("model.id") + disableConnectButtons(userId) { + return userId !== this.get("currentUser.id"); + }, + + @computed() + canUpdateAssociatedAccounts() { + return ( + findAll(this.siteSettings, this.capabilities, this.site.isMobileDevice) + .length > 0 + ); + }, + actions: { save() { this.set("saved", false); @@ -135,6 +174,28 @@ export default Ember.Controller.extend( showTwoFactorModal() { showModal("second-factor-intro"); + }, + + revokeAccount(account) { + const model = this.get("model"); + this.set("revoking", true); + model + .revokeAssociatedAccount(account.name) + .then(result => { + if (result.success) { + model.get("associated_accounts").removeObject(account); + } else { + bootbox.alert(result.message); + } + }) + .catch(popupAjaxError) + .finally(() => { + this.set("revoking", false); + }); + }, + + connectAccount(method) { + method.doLogin(); } } } diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6 index 6a979c81597..da5282a4ab8 100644 --- a/app/assets/javascripts/discourse/models/login-method.js.es6 +++ b/app/assets/javascripts/discourse/models/login-method.js.es6 @@ -12,8 +12,22 @@ const LoginMethod = Ember.Object.extend({ } return ( - this.get("titleOverride") || - I18n.t("login." + this.get("name") + ".title") + this.get("titleOverride") || 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`) ); }, @@ -23,6 +37,52 @@ const LoginMethod = Ember.Object.extend({ this.get("messageOverride") || I18n.t("login." + this.get("name") + ".message") ); + }, + + doLogin() { + const name = this.get("name"); + const customLogin = this.get("customLogin"); + + if (customLogin) { + customLogin(); + } else { + let authUrl = this.get("customUrl") || Discourse.getURL("/auth/" + name); + if (this.get("fullScreenLogin")) { + document.cookie = "fsl=true"; + window.location = authUrl; + } else { + this.set("authenticate", name); + const left = this.get("lastX") - 400; + const top = this.get("lastY") - 200; + + const height = this.get("frameHeight") || 400; + const width = this.get("frameWidth") || 800; + + if (this.get("displayPopup")) { + authUrl = authUrl + "?display=popup"; + } + + const w = window.open( + authUrl, + "_blank", + "menubar=no,status=no,height=" + + height + + ",width=" + + width + + ",left=" + + left + + ",top=" + + top + ); + const self = this; + const timer = setInterval(function() { + if (!w || w.closed) { + clearInterval(timer); + self.set("authenticate", null); + } + }, 1000); + } + } } }); @@ -57,6 +117,10 @@ export function findAll(siteSettings, capabilities, isMobileDevice) { params.displayPopup = true; } + if (["facebook"].includes(name)) { + params.canConnect = true; + } + params.siteSettings = siteSettings; methods.pushObject(LoginMethod.create(params)); } diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index b0d7fe60163..598faf1afd4 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -372,6 +372,16 @@ const User = RestModel.extend({ }); }, + revokeAssociatedAccount(providerName) { + return ajax( + userPath(`${this.get("username")}/preferences/revoke-account`), + { + data: { provider_name: providerName }, + type: "POST" + } + ); + }, + loadUserAction(id) { const stream = this.get("stream"); return ajax(`/user_actions/${id}.json`, { cache: "false" }).then(result => { diff --git a/app/assets/javascripts/discourse/templates/preferences/account.hbs b/app/assets/javascripts/discourse/templates/preferences/account.hbs index 57c8b74cd79..9feb40a5796 100644 --- a/app/assets/javascripts/discourse/templates/preferences/account.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/account.hbs @@ -99,6 +99,45 @@
{{/if}} +{{#if canUpdateAssociatedAccounts}} +
+ + {{#if associatedAccountsLoaded}} + + {{#each authProviders as |authProvider|}} + + + + {{#if authProvider.account}} + + + {{else}} + + {{/if}} + + + {{/each}} +
{{authProvider.method.prettyName}}{{authProvider.account.description}} + {{#if authProvider.account.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}} + {{/if}} + + {{#if authProvider.method.canConnect}} + {{d-button action="connectAccount" actionParam=authProvider.method label="user.associated_accounts.connect" icon="plug" disabled=disableConnectButtons}} + {{else}} + {{i18n 'user.associated_accounts.not_connected'}} + {{/if}} +
+ {{else}} +
+ {{d-button action="checkEmail" actionParam=model title="admin.users.check_email.title" icon="envelope-o" label="admin.users.check_email.text"}} +
+ {{/if}} +
+{{/if}} + {{#unless siteSettings.sso_overrides_avatar}}
diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss index 8fd6c6dd8d5..8aaa84f7dac 100644 --- a/app/assets/stylesheets/common/base/user.scss +++ b/app/assets/stylesheets/common/base/user.scss @@ -629,6 +629,12 @@ } } } + + .pref-associated-accounts table { + td { + padding: 8px; + } + } } .paginated-topics-list { diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index dc7cbfdb60a..2731bb441b3 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -8,7 +8,7 @@ class Users::OmniauthCallbacksController < ApplicationController BUILTIN_AUTH = [ Auth::FacebookAuthenticator.new, Auth::GoogleOAuth2Authenticator.new, - Auth::OpenIdAuthenticator.new("yahoo", "https://me.yahoo.com", trusted: true), + Auth::OpenIdAuthenticator.new("yahoo", "https://me.yahoo.com", 'enable_yahoo_logins', trusted: true), Auth::GithubAuthenticator.new, Auth::TwitterAuthenticator.new, Auth::InstagramAuthenticator.new @@ -18,10 +18,6 @@ class Users::OmniauthCallbacksController < ApplicationController layout 'no_ember' - def self.types - @types ||= Enum.new(:facebook, :instagram, :twitter, :google, :yahoo, :github, :persona, :cas) - end - # need to be able to call this skip_before_action :check_xhr @@ -36,9 +32,13 @@ class Users::OmniauthCallbacksController < ApplicationController auth[:session] = session authenticator = self.class.find_authenticator(params[:provider]) - provider = Discourse.auth_providers && Discourse.auth_providers.find { |p| p.name == params[:provider] } + provider = DiscoursePluginRegistry.auth_providers.find { |p| p.name == params[:provider] } - @auth_result = authenticator.after_authenticate(auth) + if authenticator.can_connect_existing_user? && current_user + @auth_result = authenticator.after_authenticate(auth, existing_account: current_user) + else + @auth_result = authenticator.after_authenticate(auth) + end origin = request.env['omniauth.origin'] @@ -91,23 +91,10 @@ class Users::OmniauthCallbacksController < ApplicationController end def self.find_authenticator(name) - BUILTIN_AUTH.each do |authenticator| - if authenticator.name == name - raise Discourse::InvalidAccess.new(I18n.t("provider_not_enabled")) unless SiteSetting.send("enable_#{name}_logins?") - return authenticator - end + Discourse.enabled_authenticators.each do |authenticator| + return authenticator if authenticator.name == name end - - Discourse.auth_providers.each do |provider| - next if provider.name != name - - unless provider.enabled_setting.nil? || SiteSetting.send(provider.enabled_setting) - raise Discourse::InvalidAccess.new(I18n.t("provider_not_enabled")) - end - return provider.authenticator - end - - raise Discourse::InvalidAccess.new(I18n.t("provider_not_found")) + raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) end protected diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 6382c4bafb1..63d675a3204 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1072,6 +1072,32 @@ class UsersController < ApplicationController render json: success_json end + def revoke_account + user = fetch_user_from_params + guardian.ensure_can_edit!(user) + provider_name = params.require(:provider_name) + + # 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? + + skip_remote = params.permit(:skip_remote) + + # We're likely going to contact the remote auth provider, so hijack request + hijack do + result = authenticator.revoke(user, skip_remote: skip_remote) + if result + return render json: success_json + else + return render json: { + success: false, + message: I18n.t("associated_accounts.revoke_failed", provider_name: provider_name) + } + end + end + end + private def honeypot_value diff --git a/app/models/user.rb b/app/models/user.rb index f0fc6ac22e9..667b9f31508 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -62,7 +62,7 @@ class User < ActiveRecord::Base has_one :twitter_user_info, dependent: :destroy has_one :github_user_info, dependent: :destroy has_one :google_user_info, dependent: :destroy - has_one :oauth2_user_info, dependent: :destroy + has_many :oauth2_user_infos, dependent: :destroy has_one :instagram_user_info, dependent: :destroy has_many :user_second_factors, dependent: :destroy has_one :user_stat, dependent: :destroy @@ -952,18 +952,18 @@ class User < ActiveRecord::Base def associated_accounts result = [] - result << "Twitter(#{twitter_user_info.screen_name})" if twitter_user_info - result << "Facebook(#{facebook_user_info.username})" if facebook_user_info - result << "Google(#{google_user_info.email})" if google_user_info - result << "GitHub(#{github_user_info.screen_name})" if github_user_info - result << "Instagram(#{instagram_user_info.screen_name})" if instagram_user_info - result << "#{oauth2_user_info.provider}(#{oauth2_user_info.email})" if oauth2_user_info - - user_open_ids.each do |oid| - result << "OpenID #{oid.url[0..20]}...(#{oid.email})" + Discourse.authenticators.each do |authenticator| + account_description = authenticator.description_for_user(self) + unless account_description.empty? + result << { + name: authenticator.name, + description: account_description, + can_revoke: authenticator.can_revoke? + } + end end - result.empty? ? I18n.t("user.no_accounts_associated") : result.join(", ") + result end def user_fields diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 3d5a15132a3..08fdd23dea1 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -75,7 +75,8 @@ class UserSerializer < BasicUserSerializer :staged, :second_factor_enabled, :second_factor_backup_enabled, - :second_factor_remaining_backup_codes + :second_factor_remaining_backup_codes, + :associated_accounts has_one :invited_by, embed: :object, serializer: BasicUserSerializer has_many :groups, embed: :object, serializer: BasicGroupSerializer @@ -145,6 +146,10 @@ class UserSerializer < BasicUserSerializer (scope.is_staff? && object.staged?) end + def include_associated_accounts? + (object.id && object.id == scope.user.try(:id)) + end + def include_second_factor_enabled? (object&.id == scope.user&.id) || scope.is_staff? end diff --git a/app/services/user_anonymizer.rb b/app/services/user_anonymizer.rb index 9e9d8667e34..daaa9763010 100644 --- a/app/services/user_anonymizer.rb +++ b/app/services/user_anonymizer.rb @@ -58,7 +58,7 @@ class UserAnonymizer @user.github_user_info.try(:destroy) @user.facebook_user_info.try(:destroy) @user.single_sign_on_record.try(:destroy) - @user.oauth2_user_info.try(:destroy) + @user.oauth2_user_infos.try(:destroy_all) @user.instagram_user_info.try(:destroy) @user.user_open_ids.find_each { |x| x.destroy } @user.api_key.try(:destroy) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 33087ebe2fc..ef98e3f26b4 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -820,6 +820,12 @@ en: one: "We'll only email you if we haven't seen you in the last minute." other: "We'll only email you if we haven't seen you in the last {{count}} minutes." + associated_accounts: + title: "Associated Accounts" + connect: "Connect" + revoke: "Revoke" + not_connected: "(not connected)" + name: title: "Name" instructions: "your full name (optional)" @@ -1007,7 +1013,6 @@ en: topics: "Topics" replies: "Replies" - associated_accounts: "Logins" ip_address: title: "Last IP Address" registration_ip_address: @@ -1198,25 +1203,28 @@ en: preferences: "You need to be logged in to change your user preferences." forgot: "I don't recall my account details" not_approved: "Your account hasn't been approved yet. You will be notified by email when you are ready to log in." - google: - title: "with Google" - message: "Authenticating with Google (make sure pop up blockers are not enabled)" google_oauth2: + name: "Google" title: "with Google" message: "Authenticating with Google (make sure pop up blockers are not enabled)" twitter: + name: "Twitter" title: "with Twitter" message: "Authenticating with Twitter (make sure pop up blockers are not enabled)" instagram: + name: "Instagram" title: "with Instagram" message: "Authenticating with Instagram (make sure pop up blockers are not enabled)" facebook: + name: "Facebook" title: "with Facebook" message: "Authenticating with Facebook (make sure pop up blockers are not enabled)" yahoo: + name: "Yahoo" title: "with Yahoo" message: "Authenticating with Yahoo (make sure pop up blockers are not enabled)" github: + name: "GitHub" title: "with GitHub" message: "Authenticating with GitHub (make sure pop up blockers are not enabled)" invites: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index c66ab97ef43..33d6865f80e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -197,6 +197,7 @@ en: not_logged_in: "You need to be logged in to do that." not_found: "The requested URL or resource could not be found." invalid_access: "You are not permitted to view the requested resource." + authenticator_not_found: "Authentication method does not exist, or has been disabled." invalid_api_credentials: "You are not permitted to view the requested resource. The API username or key is invalid." provider_not_enabled: "You are not permitted to view the requested resource. The authentication provider is not enabled." provider_not_found: "You are not permitted to view the requested resource. The authentication provider does not exist." @@ -701,6 +702,9 @@ en: title: "Thanks for confirming your current email address" description: "We're now emailing your new address for confirmation." + associated_accounts: + revoke_failed: "Failed to revoke your account with %{provider_name}." + activation: action: "Click here to activate your account" already_done: "Sorry, this account confirmation link is no longer valid. Perhaps your account is already active?" @@ -1950,7 +1954,6 @@ en: backup_code: "Log in using a backup code" user: - no_accounts_associated: "No accounts associated" deactivated: "Was deactivated due to too many bounced emails to '%{email}'." deactivated_by_staff: "Deactivated by staff" activated_by_staff: "Activated by staff" diff --git a/config/routes.rb b/config/routes.rb index b09cf206b4f..3b2e17dd22f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -408,6 +408,7 @@ Discourse::Application.routes.draw do delete "#{root_path}/:username/preferences/user_image" => "users#destroy_user_image", constraints: { username: RouteFormat.username } put "#{root_path}/:username/preferences/avatar/pick" => "users#pick_avatar", constraints: { username: RouteFormat.username } put "#{root_path}/:username/preferences/avatar/select" => "users#select_avatar", constraints: { username: RouteFormat.username } + post "#{root_path}/:username/preferences/revoke-account" => "users#revoke_account", constraints: { username: RouteFormat.username } get "#{root_path}/:username/staff-info" => "users#staff_info", constraints: { username: RouteFormat.username } get "#{root_path}/:username/summary" => "users#summary", constraints: { username: RouteFormat.username } get "#{root_path}/:username/invited" => "users#invited", constraints: { username: RouteFormat.username } diff --git a/lib/auth/authenticator.rb b/lib/auth/authenticator.rb index bd4861160fa..b5d795662cc 100644 --- a/lib/auth/authenticator.rb +++ b/lib/auth/authenticator.rb @@ -2,7 +2,17 @@ # an authentication system interacts with our database and middleware class Auth::Authenticator - def after_authenticate(auth_options) + def name + raise NotImplementedError + end + + def enabled? + raise NotImplementedError + end + + # run once the user has completed authentication on the third party system. Should return an instance of Auth::Result. + # If the user has requested to connect an existing account then `existing_account` will be set + def after_authenticate(auth_options, existing_account: nil) raise NotImplementedError end @@ -19,4 +29,31 @@ class Auth::Authenticator def register_middleware(omniauth) raise NotImplementedError end + + # return a string describing the connected account + # for a given user (typically email address). Used to list + # connected accounts under the user's preferences. Empty string + # indicates not connected + def description_for_user(user) + "" + end + + # can authorisation for this provider be revoked? + def can_revoke? + false + end + + # can exising discourse users connect this provider to their accounts + def can_connect_existing_user? + false + end + + # optionally implement the ability for users to revoke + # their link with this authenticator. + # should ideally contact the third party to fully revoke + # permissions. If this fails, return :remote_failed. + # skip remote if skip_remote == true + def revoke(user, skip_remote: false) + raise NotImplementedError + end end diff --git a/lib/auth/facebook_authenticator.rb b/lib/auth/facebook_authenticator.rb index f4ea2f1b852..f26ce628104 100644 --- a/lib/auth/facebook_authenticator.rb +++ b/lib/auth/facebook_authenticator.rb @@ -6,7 +6,47 @@ class Auth::FacebookAuthenticator < Auth::Authenticator "facebook" end - def after_authenticate(auth_token) + def enabled? + SiteSetting.enable_facebook_logins + end + + def description_for_user(user) + info = FacebookUserInfo.find_by(user_id: user.id) + info&.email || info&.username || "" + end + + def can_revoke? + true + end + + def revoke(user, skip_remote: false) + info = FacebookUserInfo.find_by(user_id: user.id) + raise Discourse::NotFound if info.nil? + + if skip_remote + info.destroy! + return true + end + + response = Excon.delete(revoke_url(info.facebook_user_id)) + + if response.status == 200 + info.destroy! + return true + end + + false + end + + def revoke_url(fb_user_id) + "https://graph.facebook.com/#{fb_user_id}/permissions?access_token=#{SiteSetting.facebook_app_id}|#{SiteSetting.facebook_app_secret}" + end + + def can_connect_existing_user? + true + end + + def after_authenticate(auth_token, existing_account: nil) result = Auth::Result.new session_info = parse_auth_token(auth_token) @@ -20,9 +60,16 @@ class Auth::FacebookAuthenticator < Auth::Authenticator user_info = FacebookUserInfo.find_by(facebook_user_id: facebook_hash[:facebook_user_id]) - result.user = user_info.try(:user) + if existing_account && (user_info.nil? || existing_account.id != user_info.user_id) + user_info.destroy! if user_info + result.user = existing_account + user_info = FacebookUserInfo.create!({ user_id: result.user.id }.merge(facebook_hash)) + else + result.user = user_info&.user + end + if !result.user && !email.blank? && result.user = User.find_by_email(email) - FacebookUserInfo.create({ user_id: result.user.id }.merge(facebook_hash)) + FacebookUserInfo.create!({ user_id: result.user.id }.merge(facebook_hash)) end user_info.update_columns(facebook_hash) if user_info @@ -42,7 +89,7 @@ class Auth::FacebookAuthenticator < Auth::Authenticator def after_create_account(user, auth) extra_data = auth[:extra_data] - FacebookUserInfo.create({ user_id: user.id }.merge(extra_data)) + FacebookUserInfo.create!({ user_id: user.id }.merge(extra_data)) retrieve_avatar(user, extra_data) retrieve_profile(user, extra_data) diff --git a/lib/auth/github_authenticator.rb b/lib/auth/github_authenticator.rb index 2d3f705cf30..e5ac4ee30ee 100644 --- a/lib/auth/github_authenticator.rb +++ b/lib/auth/github_authenticator.rb @@ -6,6 +6,15 @@ class Auth::GithubAuthenticator < Auth::Authenticator "github" end + def enabled? + SiteSetting.enable_github_logins + end + + def description_for_user(user) + info = GithubUserInfo.find_by(user_id: user.id) + info&.screen_name || "" + end + class GithubEmailChecker include ::HasErrors diff --git a/lib/auth/google_oauth2_authenticator.rb b/lib/auth/google_oauth2_authenticator.rb index e477c55edc0..3434c6b4b4c 100644 --- a/lib/auth/google_oauth2_authenticator.rb +++ b/lib/auth/google_oauth2_authenticator.rb @@ -4,6 +4,15 @@ class Auth::GoogleOAuth2Authenticator < Auth::Authenticator "google_oauth2" end + def enabled? + SiteSetting.enable_google_oauth2_logins + end + + def description_for_user(user) + info = GoogleUserInfo.find_by(user_id: user.id) + info&.email || info&.name || "" + end + def after_authenticate(auth_hash) session_info = parse_hash(auth_hash) google_hash = session_info[:google] diff --git a/lib/auth/instagram_authenticator.rb b/lib/auth/instagram_authenticator.rb index 507981ef2aa..ca4f41ebc84 100644 --- a/lib/auth/instagram_authenticator.rb +++ b/lib/auth/instagram_authenticator.rb @@ -4,6 +4,15 @@ class Auth::InstagramAuthenticator < Auth::Authenticator "instagram" end + def enabled? + SiteSetting.enable_instagram_logins + end + + def description_for_user(user) + info = InstagramUserInfo.find_by(user_id: user.id) + info&.screen_name || "" + end + # TODO twitter provides all sorts of extra info, like website/bio etc. # it may be worth considering pulling some of it in. def after_authenticate(auth_token) diff --git a/lib/auth/oauth2_authenticator.rb b/lib/auth/oauth2_authenticator.rb index cadd1022a7b..e5288c29df2 100644 --- a/lib/auth/oauth2_authenticator.rb +++ b/lib/auth/oauth2_authenticator.rb @@ -52,4 +52,8 @@ class Auth::OAuth2Authenticator < Auth::Authenticator ) end + def description_for_user(user) + info = Oauth2UserInfo.find_by(user_id: user.id, provider: @name) + info&.email || info&.name || info&.uid || "" + end end diff --git a/lib/auth/open_id_authenticator.rb b/lib/auth/open_id_authenticator.rb index 1bb17447c7f..a51aac840d6 100644 --- a/lib/auth/open_id_authenticator.rb +++ b/lib/auth/open_id_authenticator.rb @@ -2,12 +2,22 @@ class Auth::OpenIdAuthenticator < Auth::Authenticator attr_reader :name, :identifier - def initialize(name, identifier, opts = {}) + def initialize(name, identifier, enabled_site_setting, opts = {}) @name = name @identifier = identifier + @enabled_site_setting = enabled_site_setting @opts = opts end + def enabled? + SiteSetting.send(@enabled_site_setting) + end + + def description_for_user(user) + info = UserOpenId.find_by(user_id: user.id) + info&.email || "" + end + def after_authenticate(auth_token) result = Auth::Result.new diff --git a/lib/auth/twitter_authenticator.rb b/lib/auth/twitter_authenticator.rb index b4ef82d9ebd..c6ffbe2145f 100644 --- a/lib/auth/twitter_authenticator.rb +++ b/lib/auth/twitter_authenticator.rb @@ -4,6 +4,15 @@ class Auth::TwitterAuthenticator < Auth::Authenticator "twitter" end + def enabled? + SiteSetting.enable_twitter_logins + end + + def description_for_user(user) + info = TwitterUserInfo.find_by(user_id: user.id) + info&.email || info&.screen_name || "" + end + def after_authenticate(auth_token) result = Auth::Result.new diff --git a/lib/discourse.rb b/lib/discourse.rb index 693bdb0c07f..9160e887f04 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -199,24 +199,15 @@ module Discourse end def self.authenticators - # TODO: perhaps we don't need auth providers and authenticators maybe one object is enough - # 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 + auth_providers.map(&:authenticator) + Users::OmniauthCallbacksController::BUILTIN_AUTH + DiscoursePluginRegistry.auth_providers.map(&:authenticator) end - def self.auth_providers - providers = [] - plugins.each do |p| - next unless p.auth_providers - p.auth_providers.each do |prov| - providers << prov - end - end - providers + def self.enabled_authenticators + authenticators.select { |authenticator| authenticator.enabled? } end def self.cache diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index 547857838f8..88197b0abd7 100644 --- a/lib/discourse_plugin_registry.rb +++ b/lib/discourse_plugin_registry.rb @@ -5,6 +5,7 @@ class DiscoursePluginRegistry class << self attr_writer :javascripts + attr_writer :auth_providers attr_writer :service_workers attr_writer :admin_javascripts attr_writer :stylesheets @@ -26,6 +27,10 @@ class DiscoursePluginRegistry @javascripts ||= Set.new end + def auth_providers + @auth_providers ||= Set.new + end + def service_workers @service_workers ||= Set.new end @@ -87,6 +92,10 @@ class DiscoursePluginRegistry end end + def self.register_auth_provider(auth_provider) + self.auth_providers << auth_provider + end + def register_js(filename, options = {}) # If we have a server side option, add that too. self.class.javascripts << filename @@ -203,6 +212,10 @@ class DiscoursePluginRegistry self.class.javascripts end + def auth_providers + self.class.auth_providers + end + def service_workers self.class.service_workers end @@ -229,6 +242,7 @@ class DiscoursePluginRegistry def self.clear self.javascripts = nil + self.auth_providers = nil self.service_workers = nil self.stylesheets = nil self.mobile_stylesheets = nil @@ -240,6 +254,7 @@ class DiscoursePluginRegistry def self.reset! javascripts.clear + auth_providers.clear service_workers.clear admin_javascripts.clear stylesheets.clear diff --git a/lib/plugin/auth_provider.rb b/lib/plugin/auth_provider.rb index 996d9ba0413..b44dfc16aa1 100644 --- a/lib/plugin/auth_provider.rb +++ b/lib/plugin/auth_provider.rb @@ -1,8 +1,8 @@ class Plugin::AuthProvider def self.auth_attributes - [:glyph, :background_color, :title, :message, :frame_width, :frame_height, :authenticator, - :title_setting, :enabled_setting, :full_screen_login, :custom_url] + [:glyph, :background_color, :pretty_name, :title, :message, :frame_width, :frame_height, :authenticator, + :pretty_name_setting, :title_setting, :enabled_setting, :full_screen_login, :custom_url] end attr_accessor(*auth_attributes) @@ -14,8 +14,10 @@ class Plugin::AuthProvider 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 diff --git a/lib/plugin/instance.rb b/lib/plugin/instance.rb index 2077ff94a0e..8f47f1fdc43 100644 --- a/lib/plugin/instance.rb +++ b/lib/plugin/instance.rb @@ -451,6 +451,7 @@ JS register_assets! unless assets.blank? register_locales! register_service_workers! + register_auth_providers! seed_data.each do |key, value| DiscoursePluginRegistry.register_seed_data(key, value) @@ -488,6 +489,20 @@ JS Plugin::AuthProvider.auth_attributes.each do |sym| provider.send "#{sym}=", opts.delete(sym) end + + after_initialize do + begin + provider.authenticator.enabled? + rescue NotImplementedError + provider.authenticator.define_singleton_method(:enabled?) do + Rails.logger.warn("Auth::Authenticator subclasses should define an `enabled?` function. Patching for now.") + return SiteSetting.send(provider.enabled_setting) if provider.enabled_setting + Rails.logger.warn("Plugin::AuthProvider has not defined an enabled_setting. Defaulting to true.") + true + end + end + end + auth_providers << provider end @@ -567,6 +582,12 @@ JS end end + def register_auth_providers! + auth_providers.each do |auth_provider| + DiscoursePluginRegistry.register_auth_provider(auth_provider) + end + end + def register_locales! root_path = File.dirname(@path) diff --git a/spec/components/auth/facebook_authenticator_spec.rb b/spec/components/auth/facebook_authenticator_spec.rb index c7d26b6e869..ccdd3865cbb 100644 --- a/spec/components/auth/facebook_authenticator_spec.rb +++ b/spec/components/auth/facebook_authenticator_spec.rb @@ -38,6 +38,36 @@ describe Auth::FacebookAuthenticator do expect(result.user.user_profile.location).to eq("America") end + it 'can connect to a different existing user account' do + authenticator = Auth::FacebookAuthenticator.new + user1 = Fabricate(:user) + user2 = Fabricate(:user) + + FacebookUserInfo.create!(user_id: user1.id, facebook_user_id: 100) + + hash = { + "extra" => { + "raw_info" => { + "username" => "bob" + } + }, + "info" => { + "location" => "America", + "description" => "bio", + "urls" => { + "Website" => "https://awesome.com" + } + }, + "uid" => "100" + } + + result = authenticator.after_authenticate(hash, existing_account: user2) + + expect(result.user.id).to eq(user2.id) + expect(FacebookUserInfo.exists?(user_id: user1.id)).to eq(false) + expect(FacebookUserInfo.exists?(user_id: user2.id)).to eq(true) + end + it 'can create a proper result for non existing users' do hash = { @@ -62,4 +92,58 @@ describe Auth::FacebookAuthenticator do end end + context 'description_for_user' do + let(:user) { Fabricate(:user) } + let(:authenticator) { Auth::FacebookAuthenticator.new } + + it 'returns empty string if no entry for user' do + expect(authenticator.description_for_user(user)).to eq("") + end + + it 'returns correct information' do + FacebookUserInfo.create!(user_id: user.id, facebook_user_id: 12345, email: 'someuser@somedomain.tld') + expect(authenticator.description_for_user(user)).to eq('someuser@somedomain.tld') + end + end + + context 'revoke' do + let(:user) { Fabricate(:user) } + let(:authenticator) { Auth::FacebookAuthenticator.new } + + it 'raises exception if no entry for user' do + expect { authenticator.revoke(user) }.to raise_error(Discourse::NotFound) + end + + context "with valid record" do + before do + SiteSetting.facebook_app_id = '123' + SiteSetting.facebook_app_secret = 'abcde' + FacebookUserInfo.create!(user_id: user.id, facebook_user_id: 12345, email: 'someuser@somedomain.tld') + end + + it 'revokes correctly' do + stub = stub_request(:delete, authenticator.revoke_url(12345)).to_return(body: "true") + + expect(authenticator.can_revoke?).to eq(true) + expect(authenticator.revoke(user)).to eq(true) + + expect(stub).to have_been_requested.once + expect(authenticator.description_for_user(user)).to eq("") + end + + it 'handles errors correctly' do + stub = stub_request(:delete, authenticator.revoke_url(12345)).to_return(status: 404) + + expect(authenticator.revoke(user)).to eq(false) + expect(stub).to have_been_requested.once + expect(authenticator.description_for_user(user)).to eq('someuser@somedomain.tld') + + expect(authenticator.revoke(user, skip_remote: true)).to eq(true) + expect(stub).to have_been_requested.once + expect(authenticator.description_for_user(user)).to eq("") + + end + end + end + end diff --git a/spec/components/auth/open_id_authenticator_spec.rb b/spec/components/auth/open_id_authenticator_spec.rb index 983fea379a3..0ae354ebb9f 100644 --- a/spec/components/auth/open_id_authenticator_spec.rb +++ b/spec/components/auth/open_id_authenticator_spec.rb @@ -9,7 +9,7 @@ load 'auth/open_id_authenticator.rb' describe Auth::OpenIdAuthenticator do it "can lookup pre-existing user if trusted" do - auth = Auth::OpenIdAuthenticator.new("test", "id", trusted: true) + auth = Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) user = Fabricate(:user) response = OpenStruct.new(identity_url: 'abc') @@ -18,7 +18,7 @@ describe Auth::OpenIdAuthenticator do end it "raises an exception when email is missing" do - auth = Auth::OpenIdAuthenticator.new("test", "id", trusted: true) + auth = Auth::OpenIdAuthenticator.new("test", "id", "enable_yahoo_logins", trusted: true) response = OpenStruct.new(identity_url: 'abc') expect { auth.after_authenticate(info: {}, extra: { response: response }) }.to raise_error(Discourse::InvalidParameters) end diff --git a/spec/components/discourse_plugin_registry_spec.rb b/spec/components/discourse_plugin_registry_spec.rb index 82ecbbe680b..bcba59e407d 100644 --- a/spec/components/discourse_plugin_registry_spec.rb +++ b/spec/components/discourse_plugin_registry_spec.rb @@ -29,6 +29,13 @@ describe DiscoursePluginRegistry do end end + context '#auth_providers' do + it 'defaults to an empty Set' do + registry.auth_providers = nil + expect(registry.auth_providers).to eq(Set.new) + end + end + context '#admin_javascripts' do it 'defaults to an empty Set' do registry.admin_javascripts = nil @@ -92,6 +99,28 @@ describe DiscoursePluginRegistry do end end + context '.register_auth_provider' do + let(:registry) { DiscoursePluginRegistry } + let(:auth_provider) do + provider = Plugin::AuthProvider.new + provider.authenticator = Auth::Authenticator.new + provider + end + + before do + registry.register_auth_provider(auth_provider) + end + + after do + registry.reset! + end + + it 'is returned by DiscoursePluginRegistry.auth_providers' do + expect(registry.auth_providers.include?(auth_provider)).to eq(true) + end + + end + context '.register_service_worker' do let(:registry) { DiscoursePluginRegistry } diff --git a/spec/components/discourse_spec.rb b/spec/components/discourse_spec.rb index b8030d84f75..9cbd4c7e3f8 100644 --- a/spec/components/discourse_spec.rb +++ b/spec/components/discourse_spec.rb @@ -59,6 +59,54 @@ describe Discourse do end end + context 'authenticators' do + it 'returns inbuilt authenticators' do + expect(Discourse.authenticators).to match_array(Users::OmniauthCallbacksController::BUILTIN_AUTH) + end + + context 'with authentication plugin installed' do + let(:plugin_auth_provider) do + authenticator_class = Class.new(Auth::Authenticator) do + def name + 'pluginauth' + end + + def enabled + true + end + end + + provider = Plugin::AuthProvider.new + provider.authenticator = authenticator_class.new + provider + end + + before do + DiscoursePluginRegistry.register_auth_provider(plugin_auth_provider) + end + + after do + DiscoursePluginRegistry.reset! + end + + it 'returns inbuilt and plugin authenticators' do + expect(Discourse.authenticators).to match_array( + Users::OmniauthCallbacksController::BUILTIN_AUTH + [plugin_auth_provider.authenticator]) + end + + end + end + + context 'enabled_authenticators' do + it 'only returns enabled authenticators' do + expect(Discourse.enabled_authenticators.length).to be(0) + expect { SiteSetting.enable_twitter_logins = true } + .to change { Discourse.enabled_authenticators.length }.by(1) + expect(Discourse.enabled_authenticators.length).to be(1) + expect(Discourse.enabled_authenticators.first).to be_instance_of(Auth::TwitterAuthenticator) + end + end + context '#site_contact_user' do let!(:admin) { Fabricate(:admin) } diff --git a/spec/components/plugin/instance_spec.rb b/spec/components/plugin/instance_spec.rb index 44456c818ae..61accafbf48 100644 --- a/spec/components/plugin/instance_spec.rb +++ b/spec/components/plugin/instance_spec.rb @@ -125,7 +125,40 @@ describe Plugin::Instance do end end + it 'patches the enabled? function for auth_providers if not defined' do + SiteSetting.stubs(:ubuntu_login_enabled).returns(false) + + plugin = Plugin::Instance.new + + # No enabled_site_setting + authenticator = Auth::Authenticator.new + plugin.auth_provider(authenticator: authenticator) + plugin.notify_after_initialize + expect(authenticator.enabled?).to eq(true) + + # With enabled site setting + authenticator = Auth::Authenticator.new + plugin.auth_provider(enabled_setting: 'ubuntu_login_enabled', authenticator: authenticator) + plugin.notify_after_initialize + expect(authenticator.enabled?).to eq(false) + + # Defines own method + SiteSetting.stubs(:ubuntu_login_enabled).returns(true) + authenticator = Class.new(Auth::Authenticator) do + def enabled? + false + end + end.new + plugin.auth_provider(enabled_setting: 'ubuntu_login_enabled', authenticator: authenticator) + plugin.notify_after_initialize + expect(authenticator.enabled?).to eq(false) + end + context "activate!" do + before do + SiteSetting.stubs(:ubuntu_login_enabled).returns(false) + end + it "can activate plugins correctly" do plugin = Plugin::Instance.new plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" @@ -135,10 +168,6 @@ describe Plugin::Instance do File.open("#{plugin.auto_generated_path}/junk", "w") { |f| f.write("junk") } plugin.activate! - expect(plugin.auth_providers.count).to eq(1) - auth_provider = plugin.auth_providers[0] - expect(auth_provider.authenticator.name).to eq('ubuntu') - # calls ensure_assets! make sure they are there expect(plugin.assets.count).to eq(1) plugin.assets.each do |a, opts| @@ -149,6 +178,17 @@ describe Plugin::Instance do expect(File.exists?(junk_file)).to eq(false) end + it "registers auth providers correctly" do + plugin = Plugin::Instance.new + plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" + plugin.activate! + + expect(plugin.auth_providers.count).to eq(1) + auth_provider = plugin.auth_providers[0] + expect(auth_provider.authenticator.name).to eq('ubuntu') + expect(DiscoursePluginRegistry.auth_providers.count).to eq(1) + end + it "finds all the custom assets" do plugin = Plugin::Instance.new plugin.path = "#{Rails.root}/spec/fixtures/plugins/my_plugin/plugin.rb" diff --git a/spec/fixtures/plugins/my_plugin/plugin.rb b/spec/fixtures/plugins/my_plugin/plugin.rb index db3f2f891f3..2df4eadf7fd 100644 --- a/spec/fixtures/plugins/my_plugin/plugin.rb +++ b/spec/fixtures/plugins/my_plugin/plugin.rb @@ -4,7 +4,7 @@ # authors: Frank Zappa auth_provider title: 'with Ubuntu', - authenticator: Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', trusted: true), + authenticator: Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', 'ubuntu_login_enabled', trusted: true), 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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a8ed543876a..771f3fb9ddb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -416,17 +416,16 @@ describe User do describe 'associated_accounts' do it 'should correctly find social associations' do user = Fabricate(:user) - expect(user.associated_accounts).to eq(I18n.t("user.no_accounts_associated")) + expect(user.associated_accounts).to eq([]) TwitterUserInfo.create(user_id: user.id, screen_name: "sam", twitter_user_id: 1) FacebookUserInfo.create(user_id: user.id, username: "sam", facebook_user_id: 1) GoogleUserInfo.create(user_id: user.id, email: "sam@sam.com", google_user_id: 1) GithubUserInfo.create(user_id: user.id, screen_name: "sam", github_user_id: 1) - Oauth2UserInfo.create(user_id: user.id, provider: "linkedin", email: "sam@sam.com", uid: 1) InstagramUserInfo.create(user_id: user.id, screen_name: "sam", instagram_user_id: "examplel123123") user.reload - expect(user.associated_accounts).to eq("Twitter(sam), Facebook(sam), Google(sam@sam.com), GitHub(sam), Instagram(sam), linkedin(sam@sam.com)") + expect(user.associated_accounts.map { |a| a[:name] }).to contain_exactly('twitter', 'facebook', 'google_oauth2', 'github', 'instagram') end end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 4ff4c58a977..d902b25a881 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -38,13 +38,26 @@ RSpec.describe Users::OmniauthCallbacksController do let :provider do provider = Plugin::AuthProvider.new - provider.authenticator = Auth::OpenIdAuthenticator.new('ubuntu', 'https://login.ubuntu.com', trusted: true) + provider.authenticator = Class.new(Auth::Authenticator) do + def name + 'ubuntu' + end + + def enabled? + SiteSetting.ubuntu_login_enabled + end + end.new + provider.enabled_setting = "ubuntu_login_enabled" provider end before do - Discourse.stubs(:auth_providers).returns [provider] + DiscoursePluginRegistry.register_auth_provider(provider) + end + + after do + DiscoursePluginRegistry.reset! end it "finds an authenticator when enabled" do @@ -60,14 +73,6 @@ RSpec.describe Users::OmniauthCallbacksController do expect { Users::OmniauthCallbacksController.find_authenticator("ubuntu") } .to raise_error(Discourse::InvalidAccess) end - - it "succeeds if an authenticator does not have a site setting" do - provider.enabled_setting = nil - SiteSetting.stubs(:ubuntu_login_enabled).returns(false) - - expect(Users::OmniauthCallbacksController.find_authenticator("ubuntu")) - .to be(provider.authenticator) - end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 9b213c5aaba..4967b0d49c2 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1965,7 +1965,7 @@ describe UsersController do json = JSON.parse(response.body) expect(json["email"]).to eq(user.email) expect(json["secondary_emails"]).to eq(user.secondary_emails) - expect(json["associated_accounts"]).to be_present + expect(json["associated_accounts"]).to eq([]) end it "works on inactive users" do @@ -1978,7 +1978,7 @@ describe UsersController do json = JSON.parse(response.body) expect(json["email"]).to eq(inactive_user.email) expect(json["secondary_emails"]).to eq(inactive_user.secondary_emails) - expect(json["associated_accounts"]).to be_present + expect(json["associated_accounts"]).to eq([]) end end end @@ -3068,4 +3068,46 @@ describe UsersController do end end end + + describe '#revoke_account' do + let(:other_user) { Fabricate(:user) } + it 'errors for unauthorised users' do + post "/u/#{user.username}/preferences/revoke-account.json", params: { + provider_name: 'facebook' + } + expect(response.status).to eq(403) + + sign_in(other_user) + + post "/u/#{user.username}/preferences/revoke-account.json", params: { + provider_name: 'facebook' + } + expect(response.status).to eq(403) + end + + context 'while logged in' do + before do + sign_in(user) + end + + it 'returns an error when there is no matching account' do + post "/u/#{user.username}/preferences/revoke-account.json", params: { + provider_name: 'facebook' + } + 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") + + post "/u/#{user.username}/preferences/revoke-account.json", params: { + provider_name: 'facebook' + } + expect(response.status).to eq(200) + end + + end + + end end diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 9212958d23c..5b61f8b1d15 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -181,7 +181,7 @@ describe UserAnonymizer do user.github_user_info = GithubUserInfo.create(user_id: user.id, screen_name: "example", github_user_id: "examplel123123") user.facebook_user_info = FacebookUserInfo.create(user_id: user.id, facebook_user_id: "example") user.single_sign_on_record = SingleSignOnRecord.create(user_id: user.id, external_id: "example", last_payload: "looks good") - user.oauth2_user_info = Oauth2UserInfo.create(user_id: user.id, uid: "example", provider: "example") + user.oauth2_user_infos = [Oauth2UserInfo.create(user_id: user.id, uid: "example", provider: "example")] user.instagram_user_info = InstagramUserInfo.create(user_id: user.id, screen_name: "example", instagram_user_id: "examplel123123") UserOpenId.create(user_id: user.id, email: user.email, url: "http://example.com/openid", active: true) make_anonymous @@ -191,7 +191,7 @@ describe UserAnonymizer do expect(user.github_user_info).to eq(nil) expect(user.facebook_user_info).to eq(nil) expect(user.single_sign_on_record).to eq(nil) - expect(user.oauth2_user_info).to eq(nil) + expect(user.oauth2_user_infos).to be_empty expect(user.instagram_user_info).to eq(nil) expect(user.user_open_ids.count).to eq(0) end diff --git a/test/javascripts/acceptance/preferences-test.js.es6 b/test/javascripts/acceptance/preferences-test.js.es6 index cfc58e01423..dff59326aa4 100644 --- a/test/javascripts/acceptance/preferences-test.js.es6 +++ b/test/javascripts/acceptance/preferences-test.js.es6 @@ -1,43 +1,42 @@ import { acceptance } from "helpers/qunit-helpers"; acceptance("User Preferences", { loggedIn: true, - beforeEach() { - const response = object => { - return [200, { "Content-Type": "application/json" }, object]; - }; - - // prettier-ignore - server.post("/u/second_factors.json", () => { //eslint-disable-line - return response({ + pretend(server, helper) { + server.post("/u/second_factors.json", () => { + return helper.response({ key: "rcyryaqage3jexfj", qr: '
qr-code
' }); }); - // prettier-ignore - server.put("/u/second_factor.json", () => { //eslint-disable-line - return response({ error: "invalid token" }); + server.put("/u/second_factor.json", () => { + return helper.response({ error: "invalid token" }); }); - // prettier-ignore - server.put("/u/second_factors_backup.json", () => { //eslint-disable-line - return response({ backup_codes: ["dsffdsd", "fdfdfdsf", "fddsds"] }); + server.put("/u/second_factors_backup.json", () => { + return helper.response({ + backup_codes: ["dsffdsd", "fdfdfdsf", "fddsds"] + }); + }); + + server.post("/u/eviltrout/preferences/revoke-account", () => { + return helper.response({ + success: true + }); }); } }); -QUnit.test("update some fields", assert => { - visit("/u/eviltrout/preferences"); +QUnit.test("update some fields", async assert => { + await visit("/u/eviltrout/preferences"); - andThen(() => { - assert.ok($("body.user-preferences-page").length, "has the body class"); - assert.equal( - currentURL(), - "/u/eviltrout/preferences/account", - "defaults to account tab" - ); - assert.ok(exists(".user-preferences"), "it shows the preferences"); - }); + assert.ok($("body.user-preferences-page").length, "has the body class"); + assert.equal( + currentURL(), + "/u/eviltrout/preferences/account", + "defaults to account tab" + ); + assert.ok(exists(".user-preferences"), "it shows the preferences"); const savePreferences = () => { click(".save-user"); @@ -48,25 +47,25 @@ QUnit.test("update some fields", assert => { }; fillIn(".pref-name input[type=text]", "Jon Snow"); - savePreferences(); + await savePreferences(); click(".preferences-nav .nav-profile a"); fillIn("#edit-location", "Westeros"); - savePreferences(); + await savePreferences(); click(".preferences-nav .nav-emails a"); click(".pref-activity-summary input[type=checkbox]"); - savePreferences(); + await savePreferences(); click(".preferences-nav .nav-notifications a"); selectKit(".control-group.notifications .combo-box.duration") .expand() .selectRowByValue(1440); - savePreferences(); + await savePreferences(); click(".preferences-nav .nav-categories a"); fillIn(".category-controls .category-selector", "faq"); - savePreferences(); + await savePreferences(); assert.ok( !exists(".preferences-nav .nav-tags a"), @@ -84,83 +83,87 @@ QUnit.test("update some fields", assert => { ); }); -QUnit.test("username", assert => { - visit("/u/eviltrout/preferences/username"); - andThen(() => { - assert.ok(exists("#change_username"), "it has the input element"); - }); +QUnit.test("username", async assert => { + await visit("/u/eviltrout/preferences/username"); + assert.ok(exists("#change_username"), "it has the input element"); }); -QUnit.test("about me", assert => { - visit("/u/eviltrout/preferences/about-me"); - andThen(() => { - assert.ok(exists(".raw-bio"), "it has the input element"); - }); +QUnit.test("about me", async assert => { + await visit("/u/eviltrout/preferences/about-me"); + assert.ok(exists(".raw-bio"), "it has the input element"); }); -QUnit.test("email", assert => { - visit("/u/eviltrout/preferences/email"); - andThen(() => { - assert.ok(exists("#change-email"), "it has the input element"); - }); +QUnit.test("email", async assert => { + await visit("/u/eviltrout/preferences/email"); - fillIn("#change-email", "invalidemail"); + assert.ok(exists("#change-email"), "it has the input element"); - andThen(() => { - assert.equal( - find(".tip.bad") - .text() - .trim(), - I18n.t("user.email.invalid"), - "it should display invalid email tip" - ); - }); + await fillIn("#change-email", "invalidemail"); + + assert.equal( + find(".tip.bad") + .text() + .trim(), + I18n.t("user.email.invalid"), + "it should display invalid email tip" + ); }); -QUnit.test("second factor", assert => { - visit("/u/eviltrout/preferences/second-factor"); +QUnit.test("connected accounts", async assert => { + await visit("/u/eviltrout/preferences/account"); - andThen(() => { - assert.ok(exists("#password"), "it has a password input"); - }); + assert.ok( + exists(".pref-associated-accounts"), + "it has the connected accounts section" + ); + assert.ok( + find(".pref-associated-accounts table tr:first td:first") + .html() + .indexOf("Facebook") > -1, + "it lists facebook" + ); - fillIn("#password", "secrets"); - click(".user-preferences .btn-primary"); + await click(".pref-associated-accounts table tr:first td:last button"); - andThen(() => { - assert.ok(exists("#test-qr"), "shows qr code"); - assert.notOk(exists("#password"), "it hides the password input"); - }); - - fillIn("#second-factor-token", "111111"); - click(".btn-primary"); - - andThen(() => { - assert.ok( - find(".alert-error") - .html() - .indexOf("invalid token") > -1, - "shows server validation error message" - ); - }); + find(".pref-associated-accounts table tr:first td:last button") + .html() + .indexOf("Connect") > -1; }); -QUnit.test("second factor backup", assert => { - visit("/u/eviltrout/preferences/second-factor-backup"); +QUnit.test("second factor", async assert => { + await visit("/u/eviltrout/preferences/second-factor"); - andThen(() => { - assert.ok( - exists("#second-factor-token"), - "it has a authentication token input" - ); - }); + assert.ok(exists("#password"), "it has a password input"); - fillIn("#second-factor-token", "111111"); - click(".user-preferences .btn-primary"); + await fillIn("#password", "secrets"); + await click(".user-preferences .btn-primary"); - andThen(() => { - assert.ok(exists(".backup-codes-area"), "shows backup codes"); - }); + assert.ok(exists("#test-qr"), "shows qr code"); + assert.notOk(exists("#password"), "it hides the password input"); + + await fillIn("#second-factor-token", "111111"); + await click(".btn-primary"); + + assert.ok( + find(".alert-error") + .html() + .indexOf("invalid token") > -1, + "shows server validation error message" + ); +}); + +QUnit.test("second factor backup", async assert => { + await visit("/u/eviltrout/preferences/second-factor-backup"); + + assert.ok( + exists("#second-factor-token"), + "it has a authentication token input" + ); + + await fillIn("#second-factor-token", "111111"); + await click(".user-preferences .btn-primary"); + + assert.ok(exists(".backup-codes-area"), "shows backup codes"); }); QUnit.test("default avatar selector", assert => { @@ -175,26 +178,23 @@ QUnit.test("default avatar selector", assert => { acceptance("Avatar selector when selectable avatars is enabled", { loggedIn: true, settings: { selectable_avatars_enabled: true }, - beforeEach() { - // prettier-ignore - server.get("/site/selectable-avatars.json", () => { //eslint-disable-line - return [200, { "Content-Type": "application/json" }, [ - "https://www.discourse.org", - "https://meta.discourse.org", - ]]; + pretend(server) { + server.get("/site/selectable-avatars.json", () => { + return [ + 200, + { "Content-Type": "application/json" }, + ["https://www.discourse.org", "https://meta.discourse.org"] + ]; }); } }); -QUnit.test("selectable avatars", assert => { - visit("/u/eviltrout/preferences"); +QUnit.test("selectable avatars", async assert => { + await visit("/u/eviltrout/preferences"); - click(".pref-avatar .btn"); - andThen(() => { - assert.ok( - exists(".selectable-avatars", "opens the avatar selection modal") - ); - }); + await click(".pref-avatar .btn"); + + assert.ok(exists(".selectable-avatars", "opens the avatar selection modal")); }); acceptance("User Preferences when badges are disabled", { diff --git a/test/javascripts/fixtures/user_fixtures.js.es6 b/test/javascripts/fixtures/user_fixtures.js.es6 index fc1cee83cb5..e397ce918c7 100644 --- a/test/javascripts/fixtures/user_fixtures.js.es6 +++ b/test/javascripts/fixtures/user_fixtures.js.es6 @@ -114,6 +114,13 @@ export default { "/letter_avatar/eviltrout/{size}/3_f9720745f5ce6dfc2b5641fca999d934.png", name: "Robin Ward", email: "robin.ward@example.com", + associated_accounts: [ + { + name: "facebook", + description: "robin.ward@example.com", + can_revoke: true + } + ], last_posted_at: "2015-05-07T15:23:35.074Z", last_seen_at: "2015-05-13T14:34:23.188Z", bio_raw: