From fb75f188ba9904f25a4031fbaabec13e7224759c Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 1 Mar 2018 15:47:07 +0800 Subject: [PATCH] FEATURE: Disallow login via omniauth when user has 2FA enabled. --- .../discourse/controllers/login.js.es6 | 30 ++++++++++++++----- .../templates/mobile/modal/login.hbs | 12 ++++---- .../discourse/templates/modal/login.hbs | 12 ++++---- .../users/omniauth_callbacks_controller.rb | 5 ++++ config/locales/client.en.yml | 1 + lib/auth/result.rb | 25 +++++++++++----- .../omniauth_callbacks_controller_spec.rb | 17 +++++++++++ 7 files changed, 77 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index 7646c8ecb34..7067dbc11ba 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/app/assets/javascripts/discourse/controllers/login.js.es6 @@ -27,16 +27,20 @@ export default Ember.Controller.extend(ModalFunctionality, { loggingIn: false, loggedIn: false, processingEmailLink: false, + showLoginButtons: true, canLoginLocal: setting('enable_local_logins'), canLoginLocalWithEmail: setting('enable_local_logins_via_email'), loginRequired: Em.computed.alias('application.loginRequired'), resetForm: function() { - this.set('authenticate', null); - this.set('loggingIn', false); - this.set('loggedIn', false); - this.set('secondFactorRequired', false); + this.setProperties({ + 'authenticate': null, + 'loggingIn': false, + 'loggedIn': false, + 'secondFactorRequired': false, + 'showLoginButtons': true, + }); $("#credentials").show(); $("#second-factor").hide(); }, @@ -250,16 +254,28 @@ export default Ember.Controller.extend(ModalFunctionality, { }).property('authenticate'), authenticationComplete(options) { - const self = this; - function loginError(errorMsg, className) { + function loginError(errorMsg, className, callback) { showModal('login'); - Ember.run.next(function() { + + Ember.run.next(() => { + callback(); self.flash(errorMsg, className || 'success'); self.set('authenticate', null); }); } + if (options.omniauth_disallow_totp) { + return loginError(I18n.t('login.omniauth_disallow_totp'), 'error', () => { + this.setProperties({ + 'loginName': options.email, + 'showLoginButtons': false, + }); + + $('#login-account-password').focus(); + }); + } + for (let i=0; i diff --git a/app/assets/javascripts/discourse/templates/modal/login.hbs b/app/assets/javascripts/discourse/templates/modal/login.hbs index 35f7015629c..17a38baac9e 100644 --- a/app/assets/javascripts/discourse/templates/modal/login.hbs +++ b/app/assets/javascripts/discourse/templates/modal/login.hbs @@ -1,10 +1,12 @@ {{#login-modal screenX=lastX screenY=lastY loginName=loginName loginPassword=loginPassword loginSecondFactor=loginSecondFactor action="login"}} {{#d-modal-body title="login.title" class="login-modal"}} - {{login-buttons - canLoginLocalWithEmail=canLoginLocalWithEmail - processingEmailLink=processingEmailLink - emailLogin='emailLogin' - externalLogin='externalLogin'}} + {{#if showLoginButtons}} + {{login-buttons + canLoginLocalWithEmail=canLoginLocalWithEmail + processingEmailLink=processingEmailLink + emailLogin='emailLogin' + externalLogin='externalLogin'}} + {{/if}} {{#if canLoginLocal}}
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 45410715a30..768898585ab 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -114,6 +114,11 @@ class Users::OmniauthCallbacksController < ApplicationController end def user_found(user) + if user.totp_enabled? + @auth_result.omniauth_disallow_totp = true + return + end + # automatically activate/unstage any account if a provider marked the email valid if @auth_result.email_valid && @auth_result.email == user.email user.update!(staged: false) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 132fef53a6b..b9c4b83a79d 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1140,6 +1140,7 @@ en: not_allowed_from_ip_address: "You can't login from that IP address." admin_not_allowed_from_ip_address: "You can't log in as admin from that IP address." resend_activation_email: "Click here to send the activation email again." + omniauth_disallow_totp: "Your account has 2FA enabled. Please login with your password." resend_title: "Resend Activation Email" change_email: "Change Email Address" diff --git a/lib/auth/result.rb b/lib/auth/result.rb index 911dca0a28b..9f34d2c62e2 100644 --- a/lib/auth/result.rb +++ b/lib/auth/result.rb @@ -4,7 +4,7 @@ class Auth::Result :awaiting_approval, :authenticated, :authenticator_name, :requires_invite, :not_allowed_from_ip_address, :admin_not_allowed_from_ip_address, :omit_username, - :skip_email_validation, :destination_url + :skip_email_validation, :destination_url, :omniauth_disallow_totp attr_accessor( :failed, @@ -42,13 +42,22 @@ class Auth::Result date: I18n.l(user.suspended_till, format: :date_only), reason: user.suspend_reason) } else - result = { - authenticated: !!authenticated, - awaiting_activation: !!awaiting_activation, - awaiting_approval: !!awaiting_approval, - not_allowed_from_ip_address: !!not_allowed_from_ip_address, - admin_not_allowed_from_ip_address: !!admin_not_allowed_from_ip_address - } + result = + if omniauth_disallow_totp + { + omniauth_disallow_totp: !!omniauth_disallow_totp, + email: email + } + else + { + authenticated: !!authenticated, + awaiting_activation: !!awaiting_activation, + awaiting_approval: !!awaiting_approval, + not_allowed_from_ip_address: !!not_allowed_from_ip_address, + admin_not_allowed_from_ip_address: !!admin_not_allowed_from_ip_address + } + end + result[:destination_url] = destination_url if authenticated && destination_url.present? result end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index a648d4952cb..f2860cc6940 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -132,6 +132,23 @@ RSpec.describe Users::OmniauthCallbacksController do expect(user.registration_ip_address).to be_present end + context 'when user has second factor enabled' do + before do + user.create_totp(enabled: true) + end + + it 'should return the right response' do + get "/auth/google_oauth2/callback.json" + + expect(response).to be_success + + response_body = JSON.parse(response.body) + + expect(response_body["email"]).to eq(user.email) + expect(response_body["omniauth_disallow_totp"]).to eq(true) + end + end + context 'when user has not verified his email' do before do GoogleUserInfo.create!(google_user_id: '12345', user: user)