From 9cfe3f9948709648a58a372312d42e2c122cc458 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 17 Jul 2019 12:34:02 +0100 Subject: [PATCH] SECURITY: Add confirmation screen when connecting associated accounts --- .../associate-account-confirm.js.es6 | 26 ++++++ .../controllers/preferences/account.js.es6 | 7 +- .../discourse/models/login-method.js.es6 | 2 +- .../discourse/routes/app-route-map.js.es6 | 1 + .../discourse/routes/associate-account.js.es6 | 16 ++++ .../modal/associate-account-confirm.hbs | 21 +++++ .../users/associate_accounts_controller.rb | 45 ++++++++++ .../users/omniauth_callbacks_controller.rb | 16 +--- config/locales/client.en.yml | 3 + config/routes.rb | 2 + lib/auth/authenticator.rb | 7 ++ lib/auth/managed_authenticator.rb | 12 ++- spec/requests/associate_accounts_spec.rb | 89 +++++++++++++++++++ .../omniauth_callbacks_controller_spec.rb | 9 +- 14 files changed, 234 insertions(+), 22 deletions(-) create mode 100644 app/assets/javascripts/discourse/controllers/associate-account-confirm.js.es6 create mode 100644 app/assets/javascripts/discourse/routes/associate-account.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/modal/associate-account-confirm.hbs create mode 100644 app/controllers/users/associate_accounts_controller.rb create mode 100644 spec/requests/associate_accounts_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/associate-account-confirm.js.es6 b/app/assets/javascripts/discourse/controllers/associate-account-confirm.js.es6 new file mode 100644 index 00000000000..e9685e0cf90 --- /dev/null +++ b/app/assets/javascripts/discourse/controllers/associate-account-confirm.js.es6 @@ -0,0 +1,26 @@ +import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import ModalFunctionality from "discourse/mixins/modal-functionality"; + +export default Ember.Controller.extend(ModalFunctionality, { + actions: { + finishConnect() { + ajax({ + url: `/associate/${encodeURIComponent(this.model.token)}`, + type: "POST" + }) + .then(result => { + if (result.success) { + this.transitionToRoute( + "preferences.account", + this.currentUser.findDetails() + ); + this.send("closeModal"); + } else { + this.set("model.error", result.error); + } + }) + .catch(popupAjaxError); + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 index ac56e0ec377..952788273ff 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 @@ -20,6 +20,7 @@ export default Ember.Controller.extend( this._super(...arguments); this.saveAttrNames = ["name", "title"]; + this.set("revoking", {}); }, canEditName: setting("enable_names"), @@ -32,6 +33,8 @@ export default Ember.Controller.extend( showAllAuthTokens: false, + revoking: null, + cannotDeleteAccount: Ember.computed.not("currentUser.can_delete_account"), deleteDisabled: Ember.computed.or( "model.isSaving", @@ -200,7 +203,7 @@ export default Ember.Controller.extend( }, revokeAccount(account) { - this.set("revoking", true); + this.set(`revoking.${account.name}`, true); this.model .revokeAssociatedAccount(account.name) @@ -212,7 +215,7 @@ export default Ember.Controller.extend( } }) .catch(popupAjaxError) - .finally(() => this.set("revoking", false)); + .finally(() => this.set(`revoking.${account.name}`, false)); }, toggleShowAllAuthTokens() { diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6 index 244e75b6259..63fb07eb225 100644 --- a/app/assets/javascripts/discourse/models/login-method.js.es6 +++ b/app/assets/javascripts/discourse/models/login-method.js.es6 @@ -29,7 +29,7 @@ const LoginMethod = Ember.Object.extend({ authUrl += "?reconnect=true"; } - if (fullScreenLogin) { + if (reconnect || fullScreenLogin) { document.cookie = "fsl=true"; window.location = authUrl; } else { diff --git a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 index bfec70df96a..654c683a45e 100644 --- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 +++ b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 @@ -178,6 +178,7 @@ export default function() { this.route("signup", { path: "/signup" }); this.route("login", { path: "/login" }); this.route("email-login", { path: "/session/email-login/:token" }); + this.route("associate-account", { path: "/associate/:token" }); this.route("login-preferences"); this.route("forgot-password", { path: "/password-reset" }); this.route("faq", { path: "/faq" }); diff --git a/app/assets/javascripts/discourse/routes/associate-account.js.es6 b/app/assets/javascripts/discourse/routes/associate-account.js.es6 new file mode 100644 index 00000000000..dfbe2e52f75 --- /dev/null +++ b/app/assets/javascripts/discourse/routes/associate-account.js.es6 @@ -0,0 +1,16 @@ +import { ajax } from "discourse/lib/ajax"; +import showModal from "discourse/lib/show-modal"; +import { popupAjaxError } from "discourse/lib/ajax-error"; + +export default Discourse.Route.extend({ + beforeModel() { + const params = this.paramsFor("associate-account"); + this.replaceWith(`preferences.account`, this.currentUser).then(() => + Ember.run.next(() => + ajax(`/associate/${encodeURIComponent(params.token)}`) + .then(model => showModal("associate-account-confirm", { model })) + .catch(popupAjaxError) + ) + ); + } +}); diff --git a/app/assets/javascripts/discourse/templates/modal/associate-account-confirm.hbs b/app/assets/javascripts/discourse/templates/modal/associate-account-confirm.hbs new file mode 100644 index 00000000000..3fec07f02e3 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/modal/associate-account-confirm.hbs @@ -0,0 +1,21 @@ +{{#d-modal-body + rawTitle=( + i18n "user.associated_accounts.confirm_modal_title" + provider=(i18n (concat "login." model.provider_name ".name")) + ) +}} + {{#if model.error}} +
+ {{model.error}} +
+ {{/if}} + + {{i18n "user.associated_accounts.confirm_description" + provider=(i18n (concat "login." model.provider_name ".name")) + account_description=model.account_description}} +{{/d-modal-body}} + +
+ {{d-button label="user.associated_accounts.connect" action=(action "finishConnect") class="btn-primary" icon="plug"}} + {{d-button label="user.associated_accounts.cancel" action=(action "closeModal")}} +
diff --git a/app/controllers/users/associate_accounts_controller.rb b/app/controllers/users/associate_accounts_controller.rb new file mode 100644 index 00000000000..6505afa6a7b --- /dev/null +++ b/app/controllers/users/associate_accounts_controller.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class Users::AssociateAccountsController < ApplicationController + REDIS_PREFIX ||= "omniauth_reconnect" + + ## + # Presents a confirmation screen to the user. Accessed via GET, with no CSRF checks + def connect_info + auth = get_auth_hash + + provider_name = auth.provider + authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name } + raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) if authenticator.nil? + + account_description = authenticator.description_for_auth_hash(auth) + + render json: { token: params[:token], provider_name: provider_name, account_description: account_description } + end + + ## + # Presents a confirmation screen to the user. Accessed via GET, with no CSRF checks + def connect + auth = get_auth_hash + $redis.del "#{REDIS_PREFIX}_#{current_user&.id}_#{params[:token]}" + + provider_name = auth.provider + authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name } + raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) if authenticator.nil? + + auth_result = authenticator.after_authenticate(auth, existing_account: current_user) + DiscourseEvent.trigger(:after_auth, authenticator, auth_result) + + render json: success_json + end + + private + + def get_auth_hash + token = params[:token] + json = $redis.get "#{REDIS_PREFIX}_#{current_user&.id}_#{token}" + raise Discourse::NotFound if json.nil? + + OmniAuth::AuthHash.new(JSON.parse(json)) + end +end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index d7473a51f6e..65b3e4452de 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -28,18 +28,10 @@ class Users::OmniauthCallbacksController < ApplicationController provider = DiscoursePluginRegistry.auth_providers.find { |p| p.name == params[:provider] } if session.delete(:auth_reconnect) && authenticator.can_connect_existing_user? && current_user - # If we're reconnecting, don't actually try and log the user in - @auth_result = authenticator.after_authenticate(auth, existing_account: current_user) - if provider&.full_screen_login || cookies['fsl'] - cookies.delete('fsl') - return redirect_to Discourse.base_uri("/my/preferences/account") - else - @auth_result.authenticated = true - return respond_to do |format| - format.html - format.json { render json: @auth_result.to_client_hash } - end - end + # Save to redis, with a secret token, then redirect to confirmation screen + token = SecureRandom.hex + $redis.setex "#{Users::AssociateAccountsController::REDIS_PREFIX}_#{current_user.id}_#{token}", 10.minutes, auth.to_json + return redirect_to Discourse.base_uri("/associate/#{token}") else @auth_result = authenticator.after_authenticate(auth) end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9b5b3feba8f..69616436a3c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1012,7 +1012,10 @@ en: title: "Associated Accounts" connect: "Connect" revoke: "Revoke" + cancel: "Cancel" not_connected: "(not connected)" + confirm_modal_title: "Connect %{provider} Account" + confirm_description: "Your %{provider} account '%{account_description}' will be used for authentication." name: title: "Name" diff --git a/config/routes.rb b/config/routes.rb index d6132b9da29..78d9fd07d9a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -585,6 +585,8 @@ Discourse::Application.routes.draw do match "/auth/:provider/callback", to: "users/omniauth_callbacks#complete", via: [:get, :post] match "/auth/failure", to: "users/omniauth_callbacks#failure", via: [:get, :post] + get "/associate/:token", to: "users/associate_accounts#connect_info", constraints: { token: /\h{32}/ } + post "/associate/:token", to: "users/associate_accounts#connect", constraints: { token: /\h{32}/ } resources :clicks do collection do diff --git a/lib/auth/authenticator.rb b/lib/auth/authenticator.rb index b306b2c206d..cd6fd4fd5e6 100644 --- a/lib/auth/authenticator.rb +++ b/lib/auth/authenticator.rb @@ -40,6 +40,13 @@ class Auth::Authenticator "" end + # return a string describing the connected account + # for a given OmniAuth::AuthHash. Used in the confirmation screen + # when connecting accounts + def description_for_auth_hash(user) + "" + end + # can authorisation for this provider be revoked? def can_revoke? false diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb index bdc7f0951c9..9fad79f0c45 100644 --- a/lib/auth/managed_authenticator.rb +++ b/lib/auth/managed_authenticator.rb @@ -2,9 +2,15 @@ class Auth::ManagedAuthenticator < Auth::Authenticator def description_for_user(user) - info = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id)&.info - return "" if info.nil? - info["email"] || info["nickname"] || info["name"] || I18n.t("associated_accounts.connected") + associated_account = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id) + return "" if associated_account.nil? + description_for_auth_hash(associated_account) || I18n.t("associated_accounts.connected") + end + + def description_for_auth_hash(auth_token) + return if auth_token&.info.nil? + info = auth_token.info + info["email"] || info["nickname"] || info["name"] end # These three methods are designed to be overriden by child classes diff --git a/spec/requests/associate_accounts_spec.rb b/spec/requests/associate_accounts_spec.rb new file mode 100644 index 00000000000..41b199c838d --- /dev/null +++ b/spec/requests/associate_accounts_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Users::AssociateAccountsController do + fab!(:user) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + + before do + OmniAuth.config.test_mode = true + end + + after do + Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2] = nil + OmniAuth.config.test_mode = false + end + + context 'when attempting reconnect' do + before do + SiteSetting.enable_google_oauth2_logins = true + OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new( + provider: 'google_oauth2', + uid: '12345', + info: { + email: 'someemail@test.com', + }, + extra: { + raw_info: { + email_verified: true, + } + }, + ) + + Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2] + end + + it 'should work correctly' do + sign_in(user) + + # Reconnect flow: + get "/auth/google_oauth2?reconnect=true" + expect(response.status).to eq(302) + expect(session[:auth_reconnect]).to eq(true) + + OmniAuth.config.mock_auth[:google_oauth2].uid = "123456" + get "/auth/google_oauth2/callback.json" + expect(response.status).to eq(302) + + expect(session[:current_user_id]).to eq(user.id) # Still logged in + expect(UserAssociatedAccount.count).to eq(0) # Reconnect has not yet happened + + # Request associate info + uri = URI.parse(response.redirect_url) + get "#{uri.path}.json" + data = JSON.parse(response.body) + expect(data["provider_name"]).to eq("google_oauth2") + expect(data["account_description"]).to eq("someemail@test.com") + + # Request as different user, should not work + sign_in(user2) + get "#{uri.path}.json" + expect(response.status).to eq(404) + + # Back to first user + sign_in(user) + get "#{uri.path}.json" + data = JSON.parse(response.body) + expect(data["provider_name"]).to eq("google_oauth2") + + # Make the connection + post "#{uri.path}.json" + expect(response.status).to eq(200) + expect(UserAssociatedAccount.count).to eq(1) + + # Token cannot be reused + get "#{uri.path}.json" + expect(response.status).to eq(404) + end + + it "returns the correct response for non-existant tokens" do + get "/associate/12345678901234567890123456789012.json" + expect(response.status).to eq(404) + + get "/associate/shorttoken.json" + expect(response.status).to eq(404) + end + + end +end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 0cb2d54a400..8f3ad02c1a7 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -460,7 +460,7 @@ RSpec.describe Users::OmniauthCallbacksController do expect(UserAssociatedAccount.count).to eq(2) end - it 'should reconnect if parameter supplied' do + it 'should redirect to associate URL if parameter supplied' do # Log in normally get "/auth/google_oauth2?reconnect=true" expect(response.status).to eq(302) @@ -483,10 +483,11 @@ RSpec.describe Users::OmniauthCallbacksController do OmniAuth.config.mock_auth[:google_oauth2].uid = "123456" get "/auth/google_oauth2/callback.json" - expect(response.status).to eq(200) - expect(JSON.parse(response.body)["authenticated"]).to eq(true) + expect(response.status).to eq(302) + expect(response.redirect_url).to start_with("http://test.localhost/associate/") + expect(session[:current_user_id]).to eq(user.id) - expect(UserAssociatedAccount.count).to eq(1) + expect(UserAssociatedAccount.count).to eq(0) # Reconnect has not yet happened end end