FEATURE: Allow connecting associated accounts when two-factor is enabled (#6754)

FEATURE: Allow connecting associated accounts when two-factor is enabled (#6754)

Previously the ‘reconnect’ process was a bit magic - IF you were already logged into discourse, and followed the auth flow, your account would be reconnected and you would be ‘logged in again’.

Now, we explicitly check for a reconnect=true parameter when the flow is started, store it in the session, and then only follow the reconnect logic if that variable is present. Setting this parameter also skips the ‘logged in again’ step, which means reconnect now works with 2fa enabled.

From c7c56af397c1286f24827afeb2221a1c6160f6f7 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 11 Dec 2018 13:19:00 +0000
Subject: [PATCH] FEATURE: Allow connecting associated accounts when two-factor
 is enabled (#6754)

Previously the 'reconnect' process was a bit magic - IF you were already logged into discourse, and followed the auth flow, your account would be reconnected and you would be 'logged in again'.

Now, we explicitly check for a reconnect=true parameter when the flow is started, store it in the session, and then only follow the reconnect logic if that variable is present. Setting this parameter also skips the 'logged in again' step, which means reconnect now works with 2fa enabled.

diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6
index 935d52c..5ec24d7 100644
--- a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6
+++ b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6
@@ -243,7 +243,7 @@ export default Ember.Controller.extend(
       },
 
       connectAccount(method) {
-        method.doLogin();
+        method.doLogin(true);
       }
     }
   }
diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6
index 0365a27..7207ad5 100644
--- a/app/assets/javascripts/discourse/models/login-method.js.es6
+++ b/app/assets/javascripts/discourse/models/login-method.js.es6
@@ -24,7 +24,7 @@ const LoginMethod = Ember.Object.extend({
     );
   },
 
-  doLogin() {
+  doLogin(reconnect = false) {
     const name = this.get("name");
     const customLogin = this.get("customLogin");
 
@@ -33,6 +33,10 @@ const LoginMethod = Ember.Object.extend({
     } else {
       let authUrl = this.get("custom_url") || Discourse.getURL("/auth/" + name);
 
+      if (reconnect) {
+        authUrl += "?reconnect=true";
+      }
+
       if (this.get("full_screen_login")) {
         document.cookie = "fsl=true";
         window.location = authUrl;
@@ -45,7 +49,8 @@ const LoginMethod = Ember.Object.extend({
         const width = this.get("frame_width") || 800;
 
         if (name === "facebook") {
-          authUrl = authUrl + "?display=popup";
+          authUrl += authUrl.includes("?") ? "&" : "?";
+          authUrl += "display=popup";
         }
 
         const w = window.open(
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index 65310a1..b34e751 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -25,8 +25,18 @@ class Users::OmniauthCallbacksController < ApplicationController
     authenticator = self.class.find_authenticator(params[:provider])
     provider = DiscoursePluginRegistry.auth_providers.find { |p| p.name == params[:provider] }
 
-    if authenticator.can_connect_existing_user? && current_user
+    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
+        return respond_to do |format|
+          format.html
+          format.json { render json: @auth_result.to_client_hash }
+        end
+      end
     else
       @auth_result = authenticator.after_authenticate(auth)
     end
@@ -64,7 +74,7 @@ class Users::OmniauthCallbacksController < ApplicationController
       @auth_result.authenticator_name = authenticator.name
       complete_response_data
 
-      if (provider && provider.full_screen_login) || cookies['fsl']
+      if provider&.full_screen_login || cookies['fsl']
         cookies.delete('fsl')
         cookies['_bypass_cache'] = true
         cookies[:authentication_data] = @auth_result.to_client_hash.to_json
diff --git a/lib/middleware/omniauth_bypass_middleware.rb b/lib/middleware/omniauth_bypass_middleware.rb
index 55d8a1f..e039489 100644
--- a/lib/middleware/omniauth_bypass_middleware.rb
+++ b/lib/middleware/omniauth_bypass_middleware.rb
@@ -17,6 +17,12 @@ class Middleware::OmniauthBypassMiddleware
         authenticator.register_middleware(self)
       end
     end
+
+    @omniauth.before_request_phase do |env|
+      # If the user is trying to reconnect to an existing account, store in session
+      request = ActionDispatch::Request.new(env)
+      request.session[:auth_reconnect] = !!request.params["reconnect"]
+    end
   end
 
   def call(env)
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index 6185da3..97bd64b 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -338,6 +338,86 @@ RSpec.describe Users::OmniauthCallbacksController do
       end
     end
 
+    context 'when attempting reconnect' do
+      let(:user2) { Fabricate(:user) }
+      before do
+        GoogleUserInfo.create!(google_user_id: '12345', user: user)
+        GoogleUserInfo.create!(google_user_id: '123456', user: user2)
+
+        OmniAuth.config.mock_auth[:google_oauth2] = OmniAuth::AuthHash.new(
+          provider: 'google_oauth2',
+          uid: '12345',
+          info: OmniAuth::AuthHash::InfoHash.new(
+            email: 'someother_email@test.com',
+            name: 'Some name'
+          ),
+          extra: {
+            raw_info: OmniAuth::AuthHash.new(
+              email_verified: true,
+              email: 'someother_email@test.com',
+              family_name: 'Huh',
+              given_name: user.name,
+              gender: 'male',
+              name: "#{user.name} Huh",
+            )
+          },
+        )
+
+        Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2]
+      end
+
+      it 'should not reconnect normally' do
+        # Log in normally
+        get "/auth/google_oauth2"
+        expect(response.status).to eq(302)
+        expect(session[:auth_reconnect]).to eq(false)
+
+        get "/auth/google_oauth2/callback.json"
+        expect(response.status).to eq(200)
+        expect(session[:current_user_id]).to eq(user.id)
+
+        # Log into another user
+        OmniAuth.config.mock_auth[:google_oauth2].uid = "123456"
+        get "/auth/google_oauth2"
+        expect(response.status).to eq(302)
+        expect(session[:auth_reconnect]).to eq(false)
+
+        get "/auth/google_oauth2/callback.json"
+        expect(response.status).to eq(200)
+        expect(session[:current_user_id]).to eq(user2.id)
+        expect(GoogleUserInfo.count).to eq(2)
+      end
+
+      it 'should reconnect if parameter supplied' do
+        # Log in normally
+        get "/auth/google_oauth2?reconnect=true"
+        expect(response.status).to eq(302)
+        expect(session[:auth_reconnect]).to eq(true)
+
+        get "/auth/google_oauth2/callback.json"
+        expect(response.status).to eq(200)
+        expect(session[:current_user_id]).to eq(user.id)
+
+        # Clear cookie after login
+        expect(session[:auth_reconnect]).to eq(nil)
+
+        # Disconnect
+        GoogleUserInfo.find_by(user_id: user.id).destroy
+
+        # 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(200)
+        expect(session[:current_user_id]).to eq(user.id)
+        expect(GoogleUserInfo.count).to eq(1)
+      end
+
+    

GitHub

(follow up comments from PR: https://github.com/discourse/discourse/pull/6754)