SECURITY: Add confirmation screen when connecting associated accounts

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);
+    }
+  }
+});

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",
@@ -202,7 +205,7 @@ export default Ember.Controller.extend(
       },
 
       revokeAccount(account) {
-        this.set("revoking", true);
+        this.set(`revoking.${account.name}`, true);
 
         this.model
           .revokeAssociatedAccount(account.name)
@@ -214,7 +217,7 @@ export default Ember.Controller.extend(
             }
           })
           .catch(popupAjaxError)
-          .finally(() => this.set("revoking", false));
+          .finally(() => this.set(`revoking.${account.name}`, false));
       },
 
       toggleShowAllAuthTokens() {

app/assets/javascripts/discourse/models/login-method.js.es6

@@ -29,7 +29,7 @@ const LoginMethod = Ember.Object.extend({
         authUrl += "?reconnect=true";
       }
 
-      if (fullScreenLogin || this.full_screen_login) {
+      if (reconnect || fullScreenLogin || this.full_screen_login) {
         document.cookie = "fsl=true";
         window.location = authUrl;
       } else {

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

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)
+      )
+    );
+  }
+});

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}}
+    <div class='alert alert-error'>
+      {{model.error}}
+    </div>
+  {{/if}}
+
+  {{i18n "user.associated_accounts.confirm_description"
+      provider=(i18n (concat "login." model.provider_name ".name"))
+      account_description=model.account_description}}
+{{/d-modal-body}}
+
+<div class="modal-footer">
+  {{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")}}
+</div>

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

@@ -111,9 +111,7 @@
                 <td>{{authProvider.account.description}}</td>
                 <td>
                   {{#if authProvider.method.can_revoke}}
-                    {{#conditional-loading-spinner condition=revoking size='small'}}
-                      {{d-button action=(action "revokeAccount") actionParam=authProvider.account title="user.associated_accounts.revoke" class="btn-danger no-text" icon="trash-alt" }}
-                    {{/conditional-loading-spinner}}
+                    {{d-button action=(action "revokeAccount") actionParam=authProvider.account title="user.associated_accounts.revoke" class="btn-danger no-text" icon="trash-alt" disabled=(get revoking authProvider.method.name) }}
                   {{/if}}
                 </td>
             </tr>

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

app/controllers/users/omniauth_callbacks_controller.rb

@@ -28,20 +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')
-        DiscourseEvent.trigger(:after_auth, authenticator, @auth_result)
-        return redirect_to Discourse.base_uri("/my/preferences/account")
-      else
-        @auth_result.authenticated = true
-        DiscourseEvent.trigger(:after_auth, authenticator, @auth_result)
-        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)
       DiscourseEvent.trigger(:after_auth, authenticator, @auth_result)

config/locales/client.en.yml

@@ -1021,7 +1021,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"

config/routes.rb

@@ -598,6 +598,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

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

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

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

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