From c7c56af397c1286f24827afeb2221a1c6160f6f7 Mon Sep 17 00:00:00 2001 From: David Taylor 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. --- .../controllers/preferences/account.js.es6 | 2 +- .../discourse/models/login-method.js.es6 | 9 ++- .../users/omniauth_callbacks_controller.rb | 14 +++- lib/middleware/omniauth_bypass_middleware.rb | 6 ++ .../omniauth_callbacks_controller_spec.rb | 80 +++++++++++++++++++ 5 files changed, 106 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/account.js.es6 index 935d52c1a69..5ec24d7aca0 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 0365a276585..7207ad5f3c6 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 65310a13c08..b34e751f121 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 55d8a1fc242..e039489310f 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 6185da3eb1a..97bd64bb420 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 + + end + context 'after changing email' do require_dependency 'email_updater'