Review Changes for f4f8a293e7.

app/assets/javascripts/admin/controllers/admin-user-index.js.es6

@@ -19,6 +19,11 @@ export default Ember.Controller.extend(CanCheckEmails, {
 
   primaryGroupDirty: propertyNotEqual('originalPrimaryGroupId', 'model.primary_group_id'),
 
+  canDisableSecondFactor: Ember.computed.and(
+    'model.second_factor_enabled',
+    'model.can_disable_second_factor'
+  ),
+
   automaticGroups: function() {
     return this.get("model.automaticGroups").map((g) => g.name).join(", ");
   }.property("model.automaticGroups"),
@@ -41,11 +46,6 @@ export default Ember.Controller.extend(CanCheckEmails, {
     return userPath(`${username}/preferences`);
   },
 
-  @computed('model.second_factor_enabled','model.can_disable_second_factor')
-  canDisableSecondFactor(secondFactorEnabled, canDisableSecondFactor) {
-    return secondFactorEnabled && canDisableSecondFactor;
-  },
-
   actions: {
 
     impersonate() { return this.get("model").impersonate(); },

app/assets/javascripts/admin/models/admin-user.js.es6

@@ -169,7 +169,7 @@ const AdminUser = Discourse.User.extend({
   },
 
   disableSecondFactor() {
-    return ajax("/admin/users/" + this.get('id') + "/disable_second_factor", {
+    return ajax(`/admin/users/${this.get('id')}/disable_second_factor`, {
       type: 'PUT'
     }).then(() => {
       this.set('second_factor_enabled', false);

app/assets/javascripts/discourse/controllers/login.js.es6

@@ -4,6 +4,7 @@ import showModal from 'discourse/lib/show-modal';
 import { setting } from 'discourse/lib/computed';
 import { findAll } from 'discourse/models/login-method';
 import { escape } from 'pretty-text/sanitizer';
+import computed from 'ember-addons/ember-computed-decorators';
 
 // This is happening outside of the app via popup
 const AuthErrors = [
@@ -41,9 +42,10 @@ export default Ember.Controller.extend(ModalFunctionality, {
     return findAll(this.siteSettings).length > 0;
   }.property(),
 
-  loginButtonText: function() {
+  @computed('loggingIn')
-    return this.get('loggingIn') ? I18n.t('login.logging_in') : I18n.t('login.title');
+  loginButtonLabel(loggingIn) {
-  }.property('loggingIn'),
+    return loggingIn ? 'login.logging_in' : 'login.title';
+  },
 
   loginDisabled: Em.computed.or('loggingIn', 'loggedIn'),
 
@@ -70,20 +72,24 @@ export default Ember.Controller.extend(ModalFunctionality, {
       this.set('loggingIn', true);
 
       ajax("/session", {
-        data: { login: this.get('loginName'), password: this.get('loginPassword'), second_factor_token: this.get('loginSecondFactor') },
+        type: 'POST',
-        type: 'POST'
+        data: {
+          login: this.get('loginName'),
+          password: this.get('loginPassword'),
+          second_factor_token: this.get('loginSecondFactor')
+        },
       }).then(function (result) {
         // Successful login
         if (result && result.error) {
           self.set('loggingIn', false);
-          if(result.reason === 'invalid_second_factor' && !self.get('secondFactorRequired')) {
+
+          if (result.reason === 'invalid_second_factor' && !self.get('secondFactorRequired')) {
             $('#modal-alert').hide();
             self.set('secondFactorRequired', true);
             $("#credentials").hide();
             $("#second-factor").show();
             return;
-          }
+          } else if (result.reason === 'not_activated') {
-          if (result.reason === 'not_activated') {
             self.send('showNotActivated', {
               username: self.get('loginName'),
               sentTo: escape(result.sent_to_email),

app/assets/javascripts/discourse/controllers/password-reset.js.es6

@@ -47,22 +47,22 @@ export default Ember.Controller.extend(PasswordValidation, {
             DiscourseURL.redirectTo(result.redirect_to || '/');
           }
         } else {
-          if (result.errors && result.errors.second_factor) {
+          if (result.errors && result.errors.user_second_factor) {
             this.setProperties({
               secondFactorRequired: true,
               password: null,
               errorMessage: result.message
             });
-          }
+          } else if (this.get('secondFactorRequired')) {
-          else if (this.get('secondFactorRequired')) {
+            this.setProperties({
-            //ok 2factor
+              secondFactorRequired: false,
-            this.set('secondFactorRequired',false);
+              errorMessage: null
-            this.set('errorMessage', null);
+            });
-          }
+          } else if (result.errors && result.errors.password && result.errors.password.length > 0) {
-          else if (result.errors && result.errors.password && result.errors.password.length > 0) {
             this.get('rejectedPasswords').pushObject(this.get('accountPassword'));
             this.get('rejectedPasswordsMessages').set(this.get('accountPassword'), result.errors.password[0]);
           }
+
           if (result.message) {
             this.set('errorMessage', result.message);
           }

app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6

@@ -1,70 +1,71 @@
 import { default as computed } from 'ember-addons/ember-computed-decorators';
-import DiscourseURL from 'discourse/lib/url';
+import { default as DiscourseURL, userPath } from 'discourse/lib/url';
-import { userPath } from 'discourse/lib/url';
 import { popupAjaxError } from 'discourse/lib/ajax-error';
 
 export default Ember.Controller.extend({
-
   loading: false,
   password: null,
   secondFactorImage: null,
   secondFactorKey: null,
   showSecondFactorKey: false,
-
   errorMessage: null,
   newUsername: null,
 
-  @computed('secondFactorImage','secondFactorKey')
+  loaded: Ember.computed.and('secondFactorImage', 'secondFactorKey'),
-  loaded(secondFactorImage, secondFactorKey) {
-    return secondFactorImage && secondFactorKey;
-  },
 
   @computed('loading')
   submitButtonText(loading) {
-    if (loading) return I18n.t('loading');
+    return loading ? 'loading' : 'submit';
-    return I18n.t('submit');
   },
 
   toggleSecondFactor(enable) {
-    if(!this.get('second_factor_token')) {
+    if (!this.get('second_factor_token')) return;
-      return;
-    }
     this.set('loading', true);
-    this.get('content').toggleSecondFactor(this.get('second_factor_token'), enable).then((resp) => {
+
-      if(resp.error) {
+    this.get('content').toggleSecondFactor(this.get('second_factor_token'), enable)
-        this.set('errorMessage',resp.error);
+      .then(response => {
-        return;
+        if (response.error) {
-      }
+          this.set('errorMessage', response.error);
-      this.set('errorMessage',null);
+          return;
-      DiscourseURL.redirectTo(userPath(this.get('content').username.toLowerCase() + "/preferences"));
+        }
-    })
+
+        this.set('errorMessage',null);
+        DiscourseURL.redirectTo(userPath(`${this.get('content').username.toLowerCase()}/preferences`));
+      })
       .catch(popupAjaxError)
       .finally(() => this.set('loading', false));
   },
 
   actions: {
     confirmPassword() {
-      if(!this.get('password')) {
+      if (!this.get('password')) return;
-        return;
-      }
       this.set('loading', true);
-      this.get('content').loadSecondFactorCodes(this.get('password')).then((resp) => {
+
-        if(resp.error) {
+      this.get('content').loadSecondFactorCodes(this.get('password'))
-          this.set('errorMessage',resp.error);
+        .then(response => {
-          return;
+          if(response.error) {
-        }
+            this.set('errorMessage', response.error);
-        this.set('errorMessage',null);
+            return;
-        this.set('secondFactorKey', resp.key);
+          }
-        this.set('secondFactorImage', resp.qr);
+
-      }).catch(popupAjaxError)
+          this.setProperties({
+            errorMessage: null,
+            secondFactorKey: response.key,
+            secondFactorImage: response.qr,
+          });
+        })
+        .catch(popupAjaxError)
         .finally(() => this.set('loading', false));
     },
+
     showSecondFactorKey() {
       this.set('showSecondFactorKey', true);
     },
+
     enableSecondFactor() {
       this.toggleSecondFactor(true);
     },
+
     disableSecondFactor() {
       this.toggleSecondFactor(false);
     }

app/assets/javascripts/discourse/models/user.js.es6

@@ -305,19 +305,16 @@ const User = RestModel.extend({
   },
 
   loadSecondFactorCodes(password) {
-    return ajax("/second_factor/create", {
+    return ajax("/u/second_factors.json", {
-      dataType: 'json',
+      data: { password },
-      data: { login: this.get('username'),
-              password: password},
       type: 'POST'
     });
   },
 
   toggleSecondFactor(token, enable) {
-    return ajax(userPath(`${this.get('username_lower')}/preferences/second-factor`), {
+    return ajax("/u/second_factor.json", {
-      dataType: 'json',
+      data: { second_factor_token: token, enable },
-      data: { token, enable },
+      type: 'PUT'
-      type: 'POST'
     });
   },
 

app/assets/javascripts/discourse/routes/preferences-second-factor.js.es6

@@ -9,13 +9,7 @@ export default RestrictedUserRoute.extend({
     return this.render({ into: 'user' });
   },
 
-  // A bit odd, but if we leave to /preferences we need to re-render that outlet
+  setupController(controller, model) {
-  deactivate() {
+    controller.setProperties({ model, newUsername: model.get('username') });
-    this._super();
-    this.render('preferences', { into: 'user', controller: 'preferences' });
-  },
-
-  setupController(controller, user) {
-    controller.setProperties({ model: user, newUsername: user.get('username') });
   }
 });

app/assets/javascripts/discourse/routes/preferences.js.es6

@@ -18,6 +18,7 @@ export default RestrictedUserRoute.extend({
     showTwoFactorModal() {
       showModal('second-factor-intro');
     },
+
     showAvatarSelector() {
       showModal('avatar-selector');
 

app/assets/javascripts/discourse/templates/components/second-factor-form.hbs

@@ -1,6 +1,7 @@
 <div id="second-factor" style="display: none;">
   <h3>{{i18n 'login.second_factor_title'}}</h3>
   <p>{{i18n 'login.second_factor_description'}}</p>
+
   <table>
     <tr>
       <td>

app/assets/javascripts/discourse/templates/mobile/modal/login.hbs

@@ -30,7 +30,11 @@
           </table>
         </div>
         {{#second-factor-form}}
-          {{text-field value=loginSecondFactor id="login-second-factor" autocorrect="off" autocapitalize="off" autofocus="autofocus"}}
+          {{text-field value=loginSecondFactor
+              id="login-second-factor"
+              autocorrect="off"
+              autocapitalize="off"
+              autofocus="autofocus"}}
         {{/second-factor-form}}
       </form>
     {{/if}}
@@ -44,11 +48,11 @@
     {{/if}}
 
     {{#if canLoginLocal}}
-      <button class='btn btn-large btn-primary'
+      {{d-button action="login"
-              disabled={{loginDisabled}}
+          icon="unlock"
-              {{action "login"}}>
+          label=loginButtonLabel
-        {{d-icon "unlock"}}&nbsp;{{loginButtonText}}
+          disabled=loginDisabled
-      </button>
+          class='btn btn-large btn-primary'}}
 
       {{#if showSignupLink}}
         <button class="btn btn-large" id="new-account-link" {{action "showCreateAccount"}}>

app/assets/javascripts/discourse/templates/modal/login.hbs

@@ -33,9 +33,11 @@
 
   <div class="modal-footer">
     {{#if canLoginLocal}}
-      <button form="login-form" type="submit" class="btn btn-large btn-primary" disabled={{loginDisabled}} {{action "login"}}>
+      {{d-button action="login"
-        {{d-icon "unlock"}}&nbsp;{{loginButtonText}}
+          icon="unlock"
-      </button>
+          label=loginButtonLabel
+          disabled=loginDisabled
+          class='btn btn-large btn-primary'}}
 
       {{#if showSignupLink}}
         <button class="btn btn-large" id="new-account-link" {{action "createAccount"}}>

app/assets/javascripts/discourse/templates/preferences-second-factor.hbs

@@ -1,69 +1,112 @@
-<section class='user-content'>
+<section class='user-content user-preferences'>
-  <form class="form-vertical">
+  <form class="form-horizontal">
-
     <div class="control-group">
-      <label class="control-label">{{i18n 'user.second_factor.title'}}</label>
       <div class="controls">
-        {{#if model.second_factor_enabled}}
+        <h3>{{i18n 'user.second_factor.title'}}</h3>
-          <p>{{i18n 'user.second_factor.disable_description'}}</p>
-          <label>{{i18n 'login.second_factor_label'}}</label>
-          {{text-field value=second_factor_token id="second_factor_token" classNames="input-large" autofocus="autofocus"}}
-          <p>
-            {{#if errorMessage}}
-              <span class="alert alert-error">{{errorMessage}}</span>
-            {{/if}}
-          </p>
-          <button {{action "disableSecondFactor"}} disabled={{loading}} class="btn btn-primary">{{submitButtonText}}</button>
-        {{else}}
-          {{#if loaded}}
-            <p>{{i18n 'user.second_factor.enable_description'}}</p>
-            <div class="control-group">
-              {{{ secondFactorImage }}}
-                <p>
-                {{#if showSecondFactorKey}}
-                    {{ secondFactorKey }}
-                {{else}}
-                    <a {{action "showSecondFactorKey"}}>{{i18n 'user.second_factor.show_key_description'}}</a>
-                {{/if}}
-                </p>
-            </div>
-            <div class="control-group">
-              <label class="input-prepend">{{i18n 'login.second_factor_label'}}</label>
-              {{text-field value=second_factor_token id="second_factor_token" classNames="input-large" autofocus="autofocus"}}
-            </div>
-            <p>
-              {{#if errorMessage}}
-                <span class="alert alert-error">{{errorMessage}}</span>
-              {{/if}}
-            </p>
-            <div class="control-group">
-              <button {{action "enableSecondFactor"}} disabled={{loading}} class="btn btn-primary">{{submitButtonText}}</button>
-            </div>
-          {{else}}
-            <div class="control-group">
-              <p>{{i18n 'user.second_factor.confirm_password_description'}}</p>
-              <label>{{i18n 'user.password.title'}}</label>
-              <div class="controls">
-                {{text-field value=password id="password" type="password" classNames="input-xxlarge" autofocus="autofocus"}}
-              </div>
-              <p>
-                {{#if errorMessage}}
-                  <span class="alert alert-error">{{errorMessage}}</span>
-                {{/if}}
-              </p>
-            </div>
-            
-            <div class="control-group">
-              <div class="controls">
-                <button {{action "confirmPassword"}} disabled={{loading}} class="btn btn-primary">{{submitButtonText}}</button>
-                {{#if saved}}{{i18n 'saved'}}{{/if}}
-              </div>
-            </div>
-          {{/if}}
-        {{/if}}
       </div>
     </div>
 
+    {{#if errorMessage}}
+      <div class="control-group">
+        <div class="instructions">
+          <div class='alert alert-error'>{{errorMessage}}</div>
+        </div>
+      </div>
+    {{/if}}
 
+    {{#if model.second_factor_enabled}}
+      <label class='control-label'>{{i18n 'login.second_factor_label'}}</label>
+
+      <div class="control-group">
+        <div class="controls">
+          {{text-field value=second_factor_token
+              id="second_factor_token"
+              classNames="input-large"
+              autofocus="autofocus"}}
+        </div>
+
+        <div class='instructions'>
+          {{i18n 'user.second_factor.disable_description'}}
+        </div>
+      </div>
+
+      <div class="control-group">
+        <div class="controls">
+          {{d-button action="disableSecondFactor"
+              class="btn btn-primary"
+              disabled=loading
+              label=submitButtonText}}
+        </div>
+      </div>
+    {{else}}
+      {{#if loaded}}
+        <div class="control-group">
+          <div class="controls">
+            {{i18n 'user.second_factor.enable_description'}}
+          </div>
+        </div>
+
+        <div class="control-group">
+          <div class="controls">
+            {{{secondFactorImage}}}
+
+            <p>
+            {{#if showSecondFactorKey}}
+              {{secondFactorKey}}
+            {{else}}
+              <a {{action "showSecondFactorKey"}}>{{i18n 'user.second_factor.show_key_description'}}</a>
+            {{/if}}
+            </p>
+          </div>
+        </div>
+
+        <div class="control-group">
+          <label class="control-label input-prepend">{{i18n 'login.second_factor_label'}}</label>
+
+          <div class="controls">
+            {{text-field value=second_factor_token
+                id="second-factor-token"
+                classNames="input-xxlarge"
+                autofocus="autofocus"}}
+          </div>
+        </div>
+
+        <div class="control-group">
+          <div class="controls">
+            {{d-button action="enableSecondFactor"
+                class="btn btn-primary"
+                disabled=loading
+                label=submitButtonText}}
+          </div>
+        </div>
+      {{else}}
+        <div class="control-group">
+          <label class='control-label'>{{i18n 'user.password.title'}}</label>
+
+          <div class="controls">
+            {{text-field value=password
+                id="password"
+                type="password"
+                classNames="input-xxlarge"
+                autofocus="autofocus"}}
+          </div>
+
+          <div class='instructions'>
+            {{i18n 'user.second_factor.confirm_password_description'}}
+          </div>
+        </div>
+
+        <div class="control-group">
+          <div class="controls">
+            {{d-button action="confirmPassword"
+                class="btn btn-primary"
+                disabled=loading
+                label=submitButtonText}}
+
+            {{#if saved}}{{i18n 'saved'}}{{/if}}
+          </div>
+        </div>
+      {{/if}}
+    {{/if}}
   </form>
 </section>

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

@@ -14,7 +14,6 @@
   {{/if}}
 </div>
 
-
 {{#if canEditName}}
   <div class="control-group pref-name">
     <label class="control-label">{{i18n 'user.name.title'}}</label>
@@ -69,17 +68,19 @@
 </div>
 <div class="control-group pref-second-factor">
   <label class="control-label">{{i18n 'user.second_factor.title'}}</label>
+
   <div class="controls">
-  {{#link-to "preferences.second-factor" class="btn"}}
+    {{#link-to "preferences.second-factor" class="btn"}}
-    {{#if model.second_factor_enabled}}
+      {{#if model.second_factor_enabled}}
         {{d-icon "unlock-alt"}}
-      {{i18n 'user.second_factor.disable'}}
+        {{i18n 'user.second_factor.disable'}}
-    {{else}}
+      {{else}}
         {{d-icon "lock"}}
-      {{i18n 'user.second_factor.enable'}}
+        {{i18n 'user.second_factor.enable'}}
-    {{/if}}
+      {{/if}}
-  {{/link-to}}
+    {{/link-to}}
   </div>
+
   <div class="instructions">
     <a href {{action "showTwoFactorModal"}}>{{i18n 'user.second_factor.info_prompt'}}</a>
   </div>

app/controllers/admin/users_controller.rb

@@ -342,15 +342,20 @@ class Admin::UsersController < Admin::AdminController
   end
 
   def disable_second_factor
-    guardian.ensure_can_disable_second_factor! @user
+    guardian.ensure_can_disable_second_factor!(@user)
-    if @user.user_second_factor.try(:delete)
+    user_second_factor = @user.user_second_factor
-      StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user)
+    raise Discourse::InvalidParameters unless user_second_factor
-    end
+
+    user_second_factor.destroy!
+    StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user)
+
     Jobs.enqueue(
       :critical_user_email,
       type: :account_second_factor_disabled,
       user_id: @user.id
     )
+
+    render json: success_json
   end
 
   def destroy

app/controllers/second_factor_controller.rb

@@ -1,51 +0,0 @@
-class SecondFactorController < ApplicationController
-
-  def create
-    RateLimiter.new(nil, "login-hr-#{request.remote_ip}", SiteSetting.max_logins_per_ip_per_hour, 1.hour).performed!
-    RateLimiter.new(nil, "login-min-#{request.remote_ip}", SiteSetting.max_logins_per_ip_per_minute, 1.minute).performed!
-    if user = User.find_by_username_or_email(params[:login])
-      unless user.confirm_password?(params[:password])
-        return invalid_credentials
-      end
-      qrcode = RQRCode::QRCode.new(SecondFactorHelper.provisioning_uri(user))
-      qrcode_svg = qrcode.as_svg(
-        offset: 0,
-        color: '000',
-        shape_rendering: 'crispEdges',
-        module_size: 4
-      )
-      render json: { key: user.user_second_factor.data, qr: qrcode_svg }
-    end
-  end
-
-  def update
-    params.require(:token)
-    user = fetch_user_from_params
-    unless SecondFactorHelper.authenticate(user, params[:token])
-      RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
-      render json: { error: I18n.t("login.invalid_second_factor_code") }
-      return
-    end
-    if params[:enable] == "true"
-      SecondFactorHelper.create_totp(user)
-      user.user_second_factor.enabled = true
-      user.user_second_factor.save!
-      return render json: { result: "ok", action: "enabled" }
-    else
-      user.user_second_factor.delete
-      Jobs.enqueue(
-        :critical_user_email,
-        type: :account_second_factor_disabled,
-        user_id: user.id
-      )
-      return render json: { result: "ok", action: "disabled" }
-    end
-  end
-
-  private
-
-  def invalid_credentials
-    render json: { error: I18n.t("login.incorrect_username_email_or_password") }
-  end
-
-end

app/controllers/session_controller.rb

@@ -225,12 +225,13 @@ class SessionController < ApplicationController
     if payload = login_error_check(user)
       render json: payload
     else
-
+      if user.totp_enabled? && !user.authenticate_totp(params[:second_factor_token])
-      if SecondFactorHelper.totp_enabled?(user)
+        return render json: failed_json.merge(
-        unless SecondFactorHelper.authenticate(user, params[:second_factor_token])
+          error: I18n.t("login.invalid_second_factor_code"),
-          return render json: { error: I18n.t("login.invalid_second_factor_code"), reason: "invalid_second_factor" }
+          reason: "invalid_second_factor"
-        end
+        )
       end
+
       (user.active && user.email_confirmed?) ? login(user) : not_activated(user)
     end
   end
@@ -242,25 +243,28 @@ class SessionController < ApplicationController
       @error = I18n.t("login.invalid_second_factor_code")
       RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
     end
-    unless EmailToken.second_factor_valid(params[:token], params[:second_factor_token])
+
+    token = params[:token]
+    valid_token = !!EmailToken.valid_token_format?(token)
+    user = EmailToken.confirmable(token)&.user
+
+    if valid_token && user&.totp_enabled? && !user.authenticate_totp(params[:second_factor_token])
       @second_factor_required = true
-      return render layout: 'no_ember'
+      @error = I18n.t('login.invalid_second_factor_code')
-    end
+    elsif user = EmailToken.confirm(token)
-    if EmailToken.valid_token_format?(params[:token]) && (user = EmailToken.confirm(params[:token]))
       if login_not_approved_for?(user)
         @error = login_not_approved[:error]
-        return render layout: 'no_ember'
       elsif payload = login_error_check(user)
         @error = payload[:error]
-        return render layout: 'no_ember'
       else
         log_on_user(user)
-        redirect_to path("/")
+        return redirect_to path("/")
       end
     else
       @error = I18n.t('email_login.invalid_token')
-      return render layout: 'no_ember'
     end
+
+    render layout: 'no_ember'
   end
 
   def forgot_password

app/controllers/users_controller.rb

@@ -12,7 +12,7 @@ class UsersController < ApplicationController
   requires_login only: [
     :username, :update, :user_preferences_redirect, :upload_user_image,
     :pick_avatar, :destroy_user_image, :destroy, :check_emails, :topic_tracking_state,
-    :preferences
+    :preferences, :create_second_factor, :update_second_factor
   ]
 
   skip_before_action :check_xhr, only: [
@@ -470,19 +470,22 @@ class UsersController < ApplicationController
       end
     end
 
-    if @user && (!SecondFactorHelper.totp_enabled?(@user) || SecondFactorHelper.authenticate(@user, params[:second_factor_token]))
+    totp_enabled = @user&.totp_enabled?
+
+    if !totp_enabled || @user.authenticate_totp(params[:second_factor_token])
       secure_session["second-factor-#{token}"] = "true"
     end
-    @valid_second_factor = secure_session["second-factor-#{token}"] == "true"
+
+    valid_second_factor = secure_session["second-factor-#{token}"] == "true"
 
     if !@user
       @error = I18n.t('password_reset.no_token')
     elsif request.put?
       @invalid_password = params[:password].blank? || params[:password].length > User.max_password_length
 
-      if !@valid_second_factor
+      if !valid_second_factor
         RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
-        @user.errors.add(:second_factor, :invalid)
+        @user.errors.add(:user_second_factor, :invalid)
         @error = I18n.t('login.invalid_second_factor_code')
       elsif @invalid_password
         @user.errors.add(:password, :invalid)
@@ -506,9 +509,14 @@ class UsersController < ApplicationController
         else
           store_preloaded(
             "password_reset",
-            MultiJson.dump(is_developer: UsernameCheckerService.is_developer?(@user.email), admin: @user.admin?, second_factor_required: !@valid_second_factor)
+            MultiJson.dump(
+              is_developer: UsernameCheckerService.is_developer?(@user.email),
+              admin: @user.admin?,
+              second_factor_required: !valid_second_factor
+            )
           )
         end
+
         return redirect_to(wizard_path) if request.put? && Wizard.user_requires_completion?(@user)
       end
 
@@ -531,7 +539,11 @@ class UsersController < ApplicationController
             }
           end
         else
-          render json: { is_developer: UsernameCheckerService.is_developer?(@user.email), admin: @user.admin?, second_factor_required: !@valid_second_factor }
+          render json: {
+            is_developer: UsernameCheckerService.is_developer?(@user.email),
+            admin: @user.admin?,
+            second_factor_required: !valid_second_factor
+          }
         end
       end
     end
@@ -571,13 +583,20 @@ class UsersController < ApplicationController
       else
         @message = I18n.t("admin_login.errors.unknown_email_address")
       end
-    elsif params[:token].present?
+    elsif (token = params[:token]).present?
-      if EmailToken.valid_token_format?(params[:token])
+      valid_token = EmailToken.valid_token_format?(token)
+
+      if valid_token
         if params[:second_factor_token].present?
           RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
         end
-        if EmailToken.second_factor_valid(params[:token], params[:second_factor_token])
+
-          @user = EmailToken.confirm(params[:token])
+        email_token_user = EmailToken.confirmable(token)&.user
+        totp_enabled = email_token_user.totp_enabled?
+
+        if !totp_enabled || email_token_user.authenticate_totp(params[:second_factor_token])
+          @user = EmailToken.confirm(token)
+
           if @user && @user.admin?
             log_on_user(@user)
             return redirect_to path("/")
@@ -916,6 +935,60 @@ class UsersController < ApplicationController
     render layout: 'no_ember'
   end
 
+  def create_second_factor
+    RateLimiter.new(nil, "login-hr-#{request.remote_ip}", SiteSetting.max_logins_per_ip_per_hour, 1.hour).performed!
+    RateLimiter.new(nil, "login-min-#{request.remote_ip}", SiteSetting.max_logins_per_ip_per_minute, 1.minute).performed!
+
+    unless current_user.confirm_password?(params[:password])
+      return render json: failed_json.merge(
+        error: I18n.t("login.incorrect_password")
+      )
+    end
+
+    qrcode_svg = RQRCode::QRCode.new(current_user.totp_provisioning_uri).as_svg(
+      offset: 0,
+      color: '000',
+      shape_rendering: 'crispEdges',
+      module_size: 4
+    )
+
+    render json: success_json.merge(
+      key: current_user.user_second_factor.data,
+      qr: qrcode_svg
+    )
+  end
+
+  def update_second_factor
+    params.require(:second_factor_token)
+
+    [request.remote_ip, current_user.id].each do |key|
+      RateLimiter.new(nil, "second-factor-min-#{key}", 3, 1.minute).performed!
+    end
+
+    user_second_factor = current_user.user_second_factor
+    raise Discourse::InvalidParameters unless user_second_factor
+
+    unless current_user.authenticate_totp(params[:second_factor_token])
+      return render json: failed_json.merge(
+        error: I18n.t("login.invalid_second_factor_code")
+      )
+    end
+
+    if params[:enable] == "true"
+      user_second_factor.update!(enabled: true)
+    else
+      user_second_factor.destroy!
+
+      Jobs.enqueue(
+        :critical_user_email,
+        type: :account_second_factor_disabled,
+        user_id: current_user.id
+      )
+    end
+
+    render json: success_json
+  end
+
   private
 
     def honeypot_value

app/controllers/users_email_controller.rb

@@ -33,27 +33,32 @@ class UsersEmailController < ApplicationController
 
   def confirm
     expires_now
-    token = EmailToken.confirmable params[:token]
+
-    change_req = token&.user&.email_change_requests
+    token = EmailToken.confirmable(params[:token])
-      &.where('new_email_token_id = :token_id', token_id: token.id)
+    user = token&.user
-      &.first
+
-    if change_req.try(:change_state) == EmailChangeRequest.states[:authorizing_new] &&
+    change_request =
-       !EmailToken.second_factor_valid(params[:token], params[:second_factor_token])
+      if user
+        user.email_change_requests.where(new_email_token_id: token.id).first
+      end
+
+    if change_request&.change_state == EmailChangeRequest.states[:authorizing_new] &&
+       user.totp_enabled? && !user.authenticate_totp(params[:second_factor_token])
+
       @update_result = :invalid_second_factor
+
       if params[:second_factor_token].present?
         RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed!
         @show_invalid_second_factor_error = true
       end
-      render layout: 'no_ember'
+    else
-      return
+      updater = EmailUpdater.new
-    end
+      @update_result = updater.confirm(params[:token])
 
-    updater = EmailUpdater.new
+      if @update_result == :complete
-    @update_result = updater.confirm(params[:token])
+        updater.user.user_stat.reset_bounce_score!
-
+        log_on_user(updater.user)
-    if @update_result == :complete
+      end
-      updater.user.user_stat.reset_bounce_score!
-      log_on_user(updater.user)
     end
 
     render layout: 'no_ember'

app/helpers/second_factor_helper.rb

@@ -1,35 +0,0 @@
-module SecondFactorHelper
-
-  def self.totp(user)
-    self.create_totp user
-    ROTP::TOTP.new(user.user_second_factor.data, issuer: SiteSetting.title)
-  end
-
-  def self.create_totp(user)
-    if !user.user_second_factor
-      user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: ROTP::Base32.random_base32)
-    end
-  end
-
-  def self.provisioning_uri(user)
-    self.totp(user).provisioning_uri(user.email)
-  end
-
-  def self.authenticate(user, token)
-    totp = self.totp(user)
-    last_used = 0
-    if user.user_second_factor.last_used
-      last_used = user.user_second_factor.last_used.to_i
-    end
-    authenticated = !token.blank? && totp.verify_with_drift_and_prior(token, 0, last_used)
-    if authenticated
-      user.user_second_factor.last_used = DateTime.now
-      user.user_second_factor.save
-    end
-    return authenticated
-  end
-
-  def self.totp_enabled?(user)
-    !!user.user_second_factor && user.user_second_factor.enabled?
-  end
-end

app/models/concerns/second_factor_manager.rb

@@ -0,0 +1,38 @@
+module SecondFactorManager
+  extend ActiveSupport::Concern
+
+  def totp
+    self.create_totp
+    ROTP::TOTP.new(self.user_second_factor.data, issuer: SiteSetting.title)
+  end
+
+  def create_totp(opts = {})
+    if !self.user_second_factor
+      self.create_user_second_factor!({
+        method: UserSecondFactor.methods[:totp],
+        data: ROTP::Base32.random_base32
+      }.merge(opts))
+    end
+  end
+
+  def totp_provisioning_uri
+    self.totp.provisioning_uri(self.email)
+  end
+
+  def authenticate_totp(token)
+    totp = self.totp
+    last_used = 0
+
+    if self.user_second_factor.last_used
+      last_used = self.user_second_factor.last_used.to_i
+    end
+
+    authenticated = !token.blank? && totp.verify_with_drift_and_prior(token, 0, last_used)
+    self.user_second_factor.update!(last_used: DateTime.now) if authenticated
+    !!authenticated
+  end
+
+  def totp_enabled?
+    !!(self&.user_second_factor&.enabled?)
+  end
+end

app/models/email_token.rb

@@ -39,15 +39,6 @@ class EmailToken < ActiveRecord::Base
     token.present? && token =~ /\h{#{token.length / 2}}/i
   end
 
-  def self.second_factor_valid(token, second_factor_token)
-    # Fail only when token is valid, second factor token is required, and does NOT check out.
-    return true unless valid_token_format?(token)
-    email_token = confirmable(token)
-    return true if email_token.blank?
-    return true unless SecondFactorHelper.totp_enabled?(email_token.user)
-    return SecondFactorHelper.authenticate(email_token.user, second_factor_token)
-  end
-
   def self.atomic_confirm(token)
     failure = { success: false }
     return failure unless valid_token_format?(token)

app/models/user.rb

@@ -18,6 +18,7 @@ class User < ActiveRecord::Base
   include Searchable
   include Roleable
   include HasCustomFields
+  include SecondFactorManager
 
   # TODO: Remove this after 7th Jan 2018
   self.ignored_columns = %w{email}
@@ -462,10 +463,6 @@ class User < ActiveRecord::Base
     '' # so that validator doesn't complain that a password attribute doesn't exist
   end
 
-  def second_factor
-    '' # so that validator doesn't complain that a password attribute doesn't exist
-  end
-
   # Indicate that this is NOT a passwordless account for the purposes of validation
   def password_required!
     @password_required = true

app/models/user_second_factor.rb

@@ -1,3 +1,23 @@
 class UserSecondFactor < ActiveRecord::Base
   belongs_to :user
+
+  def self.methods
+    @methods ||= Enum.new(
+      totp: 1,
+    )
+  end
 end
+
+# == Schema Information
+#
+# Table name: user_second_factors
+#
+#  id         :integer          not null, primary key
+#  user_id    :integer          not null
+#  method     :string
+#  data       :string
+#  enabled    :boolean          default(FALSE), not null
+#  last_used  :datetime
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#

app/serializers/admin_detailed_user_serializer.rb

@@ -36,17 +36,12 @@ class AdminDetailedUserSerializer < AdminUserSerializer
   has_one :tl3_requirements, serializer: TrustLevel3RequirementsSerializer, embed: :objects
   has_many :groups, embed: :object, serializer: BasicGroupSerializer
 
-  def include_second_factor_enabled?
+  def second_factor_enabled
-    scope.is_staff?
+    object.totp_enabled?
   end
 
   def can_disable_second_factor
-    (object.id && object.id != scope.user.try(:id)) &&
+    object&.id != scope.user.id
-      scope.is_staff?
-  end
-
-  def second_factor_enabled
-    SecondFactorHelper.totp_enabled?(object)
   end
 
   def can_revoke_admin

app/serializers/user_serializer.rb

@@ -1,5 +1,3 @@
-require 'rqrcode'
-
 class UserSerializer < BasicUserSerializer
 
   attr_accessor :omit_stats,
@@ -149,12 +147,11 @@ class UserSerializer < BasicUserSerializer
   end
 
   def include_second_factor_enabled?
-    (object.id && object.id == scope.user.try(:id)) ||
+    (object&.id == scope.user&.id) || scope.is_staff?
-      scope.is_staff?
   end
 
   def second_factor_enabled
-    SecondFactorHelper.totp_enabled?(object)
+    object.totp_enabled?
   end
 
   def can_change_bio

app/views/session/email_login.html.erb

@@ -3,6 +3,7 @@
     <%= @error %>
   </div>
 <%end%>
+
 <%if @second_factor_required%>
   <div style="display: flex;">
     <div style="margin: auto;">
@@ -10,7 +11,7 @@
         <h2><%=t "login.second_factor_title" %></h2>
         <%= label_tag(:second_factor_token, t("login.second_factor_description")) %>
         <div><%= text_field_tag(:second_factor_token) %></div>
-        <%= submit_tag(t("login.submit"), class: "btn btn-large btn-primary") %>
+        <%= submit_tag(t("submit"), class: "btn btn-large btn-primary") %>
       <%end%>
     </div>
   </div>

app/views/users_email/confirm.html.erb

@@ -16,7 +16,7 @@
       <% if @show_invalid_second_factor_error %>
         <div class='alert alert-error'><%= t('login.invalid_second_factor_code') %></div>
       <% end %>
-      <%= submit_tag t('login.submit'), class: "btn btn-primary" %>
+      <%= submit_tag t('submit'), class: "btn btn-primary" %>
     <% end %>
   <% else %>
     <div class='alert alert-error'>

config/application.rb

@@ -129,13 +129,14 @@ module Discourse
 
     # Configure sensitive parameters which will be filtered from the log file.
     config.filter_parameters += [
-        :password,
+      :password,
-        :pop3_polling_password,
+      :pop3_polling_password,
-        :api_key,
+      :api_key,
-        :s3_secret_access_key,
+      :s3_secret_access_key,
-        :twitter_consumer_secret,
+      :twitter_consumer_secret,
-        :facebook_app_secret,
+      :facebook_app_secret,
-        :github_client_secret
+      :github_client_secret,
+      :second_factor_token,
     ]
 
     # Enable the asset pipeline

config/locales/client.en.yml

@@ -710,14 +710,14 @@ en:
 
       second_factor:
         title: "Two Factor Authentication"
-        enable: "Enable 2-Step Verification"
+        enable: "Enable Two Factor Authentication"
-        disable: "Disable 2-Step Verification"
+        disable: "Disable Two Factor Authentication"
-        confirm_password_description: "Confirm your password to continue enabling 2-Step Verification."
+        confirm_password_description: "Confirm your password to continue enabling Two Factor Authentication."
-        enable_description: "To complete 2-Step Verification setup, scan the following QR code and submit a 2-Step Verification code."
+        enable_description: "To complete Two Factor Authentication setup, scan the following QR code and submit a Two Factor Authentication code."
-        disable_description: "Enter a 2-Step Verification code to disable."
+        disable_description: "Enter a Two Factor Authentication code to disable."
         show_key_description: "Or enter the key manually."
-        info_prompt: "What is Two Factor authentication?"
+        info_prompt: "What is Two Factor Authentication?"
-        extended_description: "Two-factor authentication adds an extra security step to logging in by requiring a one-time token in addition to your password. These tokens are generated by compatible apps for iPhone or Android such as <a href=\"https://play.google.com/store/apps/details?id=com.google.android.apps.authenticator2\">Google Authenticator</a>, <a href=\"https://play.google.com/store/apps/details?id=com.authy.authy\">Authy</a>, and <a href=\"https://play.google.com/store/apps/details?id=org.fedorahosted.freeotp\">FreeOTP</a>."
+        extended_description: "Two Factor Authentication adds an extra security step to logging in by requiring a one-time token in addition to your password. These tokens are generated by compatible apps for iPhone or Android such as <a href=\"https://play.google.com/store/apps/details?id=com.google.android.apps.authenticator2\" target='_blank'>Google Authenticator</a>, <a href=\"https://play.google.com/store/apps/details?id=com.authy.authy\" target='_blank'>Authy</a>, and <a href=\"https://play.google.com/store/apps/details?id=org.fedorahosted.freeotp\" target='_blank'>FreeOTP</a>."
 
       change_about:
         title: "Change About Me"
@@ -1109,7 +1109,7 @@ en:
       title: "Log In"
       username: "User"
       password: "Password"
-      second_factor_title: "2-Step Verification Required"
+      second_factor_title: "Two Factor Authentication Required"
       second_factor_description: "Enter a generated verification code."
       second_factor_label: "Code"
       email_placeholder: "email or username"
@@ -3277,7 +3277,7 @@ en:
             post_locked: "post locked"
             post_unlocked: "post unlocked"
             check_personal_message: "check personal message"
-            disabled_second_factor: "disable 2-step auth"
+            disabled_second_factor: "disable 2 factor authentication"
         screened_emails:
           title: "Screened Emails"
           description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."

config/locales/server.en.yml

@@ -49,6 +49,7 @@ en:
   loading: "Loading"
   powered_by_html: 'Powered by <a href="https://www.discourse.org">Discourse</a>, best viewed with JavaScript enabled'
   log_in: "Log In"
+  submit: "Submit"
 
   purge_reason: "Automatically deleted as abandoned, deactivated account"
   disable_remote_images_download_reason: "Remote images download was disabled because there wasn't enough disk space available."
@@ -1761,6 +1762,7 @@ en:
   login:
     not_approved: "Your account hasn't been approved yet. You will be notified by email when you are ready to log in."
     incorrect_username_email_or_password: "Incorrect username, email or password"
+    incorrect_password: "Incorrect password"
     wait_approval: "Thanks for signing up. We will notify you when your account has been approved."
     active: "Your account is activated and ready to use."
     activate_email: "<p>You’re almost done! We sent an activation mail to <b>%{email}</b>. Please follow the instructions in the mail to activate your account.</p><p>If it doesn’t arrive, check your spam folder.</p>"
@@ -1783,10 +1785,9 @@ en:
     auth_complete: "Authentication is complete."
     click_to_continue: "Click here to continue."
     already_logged_in: "Oops, looks like you are attempting to accept an invitation for another user. If you are not %{current_user}, please log out and try again."
-    second_factor_title: "2-Step Verification Required"
+    second_factor_title: "Two Factor Authentication Required"
-    second_factor_description: "Enter a generated verification code."
+    second_factor_description: "Enter a generated authentication code."
-    invalid_second_factor_code: "Invalid 2-Step Verification Code"
+    invalid_second_factor_code: "Invalid Two Factor Authentication Code"
-    submit: "Submit"
 
   user:
     no_accounts_associated: "No accounts associated"
@@ -2735,10 +2736,10 @@ en:
 
 
     account_second_factor_disabled:
-      title: "2-Step Verification disabled"
+      title: "Two Factor Authentication disabled"
-      subject_template: "[%{email_prefix}] 2-Step Verification disabled"
+      subject_template: "[%{email_prefix}] Two Factor Authentication disabled"
       text_body_template: |
-        Your account’s 2-Step verification at %{site_name} has been disabled. The account no longer needs a 2-Step Verification code to sign in.
+        Your account’s Two Factor Authentication at %{site_name} has been disabled. The account no longer needs a Two Factor Authentication code to sign in.
 
         If you have any questions, [contact our friendly staff](%{base_url}/about).
 

config/routes.rb

@@ -304,7 +304,6 @@ Discourse::Application.routes.draw do
   get "session/csrf" => "session#csrf"
   get "session/email-login/:token" => "session#email_login"
   post "session/email-login/:token" => "session#email_login"
-  post "second_factor/create" => "second_factor#create"
   get "composer_messages" => "composer_messages#index"
   post "composer/parse_html" => "composer#parse_html"
 
@@ -332,6 +331,9 @@ Discourse::Application.routes.draw do
       end
     end
 
+    post "#{root_path}/second_factors" => "users#create_second_factor"
+    put "#{root_path}/second_factor" => "users#update_second_factor"
+
     put "#{root_path}/update-activation-email" => "users#update_activation_email"
     get "#{root_path}/hp" => "users#get_honeypot_value"
     post "#{root_path}/email-login" => "users#email_login"
@@ -385,7 +387,6 @@ Discourse::Application.routes.draw do
     put "#{root_path}/:username/preferences/badge_title" => "users#badge_title", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/preferences/username" => "users#preferences", constraints: { username: RouteFormat.username }
     put "#{root_path}/:username/preferences/username" => "users#username", constraints: { username: RouteFormat.username }
-    post "#{root_path}/:username/preferences/second-factor" => "second_factor#update", constraints: { username: RouteFormat.username }
     get "#{root_path}/:username/preferences/second-factor" => "users#preferences", constraints: { username: RouteFormat.username }
     delete "#{root_path}/:username/preferences/user_image" => "users#destroy_user_image", constraints: { username: RouteFormat.username }
     put "#{root_path}/:username/preferences/avatar/pick" => "users#pick_avatar", constraints: { username: RouteFormat.username }

db/migrate/20180109222722_create_user_second_factors.rb

@@ -2,8 +2,8 @@ class CreateUserSecondFactors < ActiveRecord::Migration[5.1]
   def change
     create_table :user_second_factors do |t|
       t.integer :user_id, null: false
-      t.string :method
+      t.integer :method, null: false
-      t.string :data
+      t.string :data, null: false
       t.boolean :enabled, null: false, default: false
       t.timestamp :last_used
       t.timestamps

spec/components/concern/second_factor_manager_spec.rb

@@ -0,0 +1,93 @@
+require 'rails_helper'
+
+RSpec.describe SecondFactorManager do
+  let(:user_second_factor) { Fabricate(:user_second_factor) }
+  let(:user) { user_second_factor.user }
+  let(:another_user) { Fabricate(:user) }
+
+  describe '#totp' do
+    it 'should return the right data' do
+      totp = nil
+
+      expect do
+        totp = another_user.totp
+      end.to change { UserSecondFactor.count }.by(1)
+
+      expect(totp.issuer).to eq(SiteSetting.title)
+      expect(totp.secret).to eq(another_user.reload.user_second_factor.data)
+    end
+  end
+
+  describe '#create_totp' do
+    it 'should create the right record' do
+      second_factor = another_user.create_totp(enabled: true)
+
+      expect(second_factor.method).to eq(UserSecondFactor.methods[:totp])
+      expect(second_factor.data).to be_present
+      expect(second_factor.enabled).to eq(true)
+    end
+
+    describe 'when user has a second factor' do
+      it 'should return nil' do
+        expect(user.create_totp).to eq(nil)
+      end
+    end
+  end
+
+  describe '#totp_provisioning_uri' do
+    it 'should return the right uri' do
+      expect(user.totp_provisioning_uri).to eq(
+        "otpauth://totp/#{SiteSetting.title}:#{user.email}?secret=#{user_second_factor.data}&issuer=#{SiteSetting.title}"
+      )
+    end
+  end
+
+  describe '#authenticate_totp' do
+    it 'should be able to authenticate a token' do
+      freeze_time do
+        expect(user.user_second_factor.last_used).to eq(nil)
+
+        token = user.totp.now
+
+        expect(user.authenticate_totp(token)).to eq(true)
+        expect(user.user_second_factor.last_used).to eq(DateTime.now)
+        expect(user.authenticate_totp(token)).to eq(false)
+      end
+    end
+
+    describe 'when token is blank' do
+      it 'should be false' do
+        expect(user.authenticate_totp(nil)).to eq(false)
+        expect(user.user_second_factor.last_used).to eq(nil)
+      end
+    end
+
+    describe 'when token is invalid' do
+      it 'should be false' do
+        expect(user.authenticate_totp('111111')).to eq(false)
+        expect(user.user_second_factor.last_used).to eq(nil)
+      end
+    end
+  end
+
+  describe '#totp_enabled?' do
+    describe 'when user does not have a second factor record' do
+      it 'should return false' do
+        expect(another_user.totp_enabled?).to eq(false)
+      end
+    end
+
+    describe "when user's second factor record is disabled" do
+      it 'should return false' do
+        user.user_second_factor.update!(enabled: false)
+        expect(user.totp_enabled?).to eq(false)
+      end
+    end
+
+    describe "when user's second factor record is enabled" do
+      it 'should return true' do
+        expect(user.totp_enabled?).to eq(true)
+      end
+    end
+  end
+end

spec/controllers/admin/users_controller_spec.rb

@@ -265,19 +265,6 @@ describe Admin::UsersController do
       end
     end
 
-    context '#disable_second_factor' do
-      before do
-        @another_user = Fabricate(:user)
-        SecondFactorHelper.create_totp(@another_user)
-      end
-
-      it 'disables the second factor' do
-        expect(User.find(@another_user.id).user_second_factor).not_to eq(nil)
-        put :disable_second_factor, params: { user_id: @another_user.id }, format: :json
-        expect(User.find(@another_user.id).user_second_factor).to eq(nil)
-      end
-    end
-
     context '#add_group' do
       let(:user) { Fabricate(:user) }
       let(:group) { Fabricate(:group) }

spec/controllers/second_factor_controller_spec.rb

@@ -1,69 +0,0 @@
-require 'rails_helper'
-
-RSpec.describe SecondFactorController, type: :controller do
-  # featheredtoast-todo also write qunit tests.
-  describe '.create' do
-
-    let(:user) { Fabricate(:user) }
-
-    describe 'create 2fa request' do
-      it 'fails on incorrect password' do
-        post :create, params: {
-               login: user.username, password: 'wrongpassword'
-             }, format: :json
-        expect(JSON.parse(response.body)['error']).to eq(I18n.t("login.incorrect_username_email_or_password"))
-      end
-
-      it 'succeeds on correct password' do
-        post :create, params: {
-               login: user.username, password: 'myawesomepassword'
-             }, format: :json
-        expect(JSON.parse(response.body).keys).to contain_exactly('key', 'qr')
-      end
-    end
-  end
-
-  describe '.update' do
-    let(:user) { Fabricate(:user) }
-
-    context 'when user has totp setup' do
-      second_factor_data = "rcyryaqage3jexfj"
-      before do
-        user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data)
-      end
-
-      it 'errors on incorrect code' do
-        post :update, params: {
-               username: user.username,
-               token: '000000',
-               enable: 'true'
-             }, format: :json
-        expect(JSON.parse(response.body)['error']).to eq(I18n.t("login.invalid_second_factor_code"))
-        user.reload
-      end
-
-      it 'can be enabled' do
-        post :update, params: {
-               username: user.username,
-               token: ROTP::TOTP.new(second_factor_data).now,
-               enable: 'true'
-             }, format: :json
-        expect(JSON.parse(response.body)['result']).to eq('ok')
-        user.reload
-        expect(user.user_second_factor.enabled).to be true
-      end
-
-      it 'can be disabled' do
-        post :update, params: {
-               username: user.username,
-               enable: 'false',
-               token: ROTP::TOTP.new(second_factor_data).now
-             }, format: :json
-        expect(JSON.parse(response.body)['result']).to eq('ok')
-        user.reload
-        expect(user.user_second_factor).to be_nil
-      end
-    end
-  end
-
-end

spec/controllers/session_controller_spec.rb

@@ -585,34 +585,50 @@ describe SessionController do
       end
 
       context 'when user has 2-factor logins' do
-        second_factor_data = "rcyryaqage3jexfj"
+        let!(:user_second_factor) { Fabricate(:user_second_factor, user: user) }
-        before do
-          user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true)
-        end
 
-        describe 'failure no 2-factor' do
+        describe 'when second factor token is missing' do
-          it 'should return an error' do
+          it 'should return the right response' do
             post :create, params: {
-                   login: user.username, password: 'myawesomepassword'
+              login: user.username,
-                 }, format: :json
+              password: 'myawesomepassword',
-            expect(JSON.parse(response.body)['error']).to eq(I18n.t('login.invalid_second_factor_code'))
+            }, format: :json
+
+            expect(JSON.parse(response.body)['error']).to eq(I18n.t(
+              'login.invalid_second_factor_code'
+            ))
           end
         end
-        describe 'successful 2-factor' do
-          it 'logs in correctly' do
-            events = DiscourseEvent.track_events do
-              post :create, params: {
-                     login: user.username, password: 'myawesomepassword', second_factor_token: ROTP::TOTP.new(second_factor_data).now
-                   }, format: :json
-            end
 
-            expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
+        describe 'when second factor token is invalid' do
+          it 'should return the right response' do
+            post :create, params: {
+              login: user.username,
+              password: 'myawesomepassword',
+              second_factor_token: '00000000'
+            }, format: :json
+
+            expect(JSON.parse(response.body)['error']).to eq(I18n.t(
+              'login.invalid_second_factor_code'
+            ))
+          end
+        end
+
+        describe 'when second factor token is valid' do
+          it 'should log the user in' do
+            post :create, params: {
+              login: user.username,
+              password: 'myawesomepassword',
+              second_factor_token: ROTP::TOTP.new(user_second_factor.data).now
+            }, format: :json
 
             user.reload
 
             expect(session[:current_user_id]).to eq(user.id)
             expect(user.user_auth_tokens.count).to eq(1)
-            expect(UserAuthToken.hash_token(cookies[:_t])).to eq(user.user_auth_tokens.first.auth_token)
+
+            expect(UserAuthToken.hash_token(cookies[:_t]))
+              .to eq(user.user_auth_tokens.first.auth_token)
           end
         end
       end
@@ -810,27 +826,32 @@ describe SessionController do
           login: user.username, password: 'myawesomepassword'
         }, format: :json
 
-        expect(response).not_to be_success
+        expect(response.status).to eq(429)
         json = JSON.parse(response.body)
         expect(json["error_type"]).to eq("rate_limit")
       end
+
       it 'rate limits second factor attempts' do
         RateLimiter.enable
         RateLimiter.clear_all!
 
         3.times do
           post :create, params: {
-                 login: user.username, password: 'myawesomepassword', second_factor_token: '000000'
+            login: user.username,
-               }, format: :json
+            password: 'myawesomepassword',
+            second_factor_token: '000000'
+          }, format: :json
 
           expect(response).to be_success
         end
 
         post :create, params: {
-               login: user.username, password: 'myawesomepassword', second_factor_token: '000000'
+          login: user.username,
-             }, format: :json
+          password: 'myawesomepassword',
+          second_factor_token: '000000'
+        }, format: :json
 
-        expect(response).not_to be_success
+        expect(response.status).to eq(429)
         json = JSON.parse(response.body)
         expect(json["error_type"]).to eq("rate_limit")
       end

spec/controllers/users_controller_spec.rb

@@ -407,17 +407,11 @@ describe UsersController do
         expect(UserAuthToken.where(id: user_token.id).count).to eq(1)
       end
 
-      context '2-factor required' do
+      context '2 factor authentication required' do
-
+        let!(:second_factor) { Fabricate(:user_second_factor, user: user) }
-        second_factor_data = "rcyryaqage3jexfj"
-        let(:user) { Fabricate(:user) }
-
-        before do
-          user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true)
-        end
 
         it 'does not change with an invalid token' do
-          token = user.email_tokens.create(email: user.email).token
+          token = user.email_tokens.create!(email: user.email).token
 
           get :password_reset, params: { token: token }
 
@@ -438,8 +432,11 @@ describe UsersController do
 
           get :password_reset, params: { token: token }
 
-          put :password_reset,
+          put :password_reset, params: {
-              params: { token: token, password: 'hg9ow8yHG32O', second_factor_token: ROTP::TOTP.new(second_factor_data).now }
+            token: token,
+            password: 'hg9ow8yHG32O',
+            second_factor_token: ROTP::TOTP.new(second_factor.data).now
+          }
 
           user.reload
           expect(user.confirm_password?('hg9ow8yHG32O')).to eq(true)
@@ -515,7 +512,7 @@ describe UsersController do
     end
   end
 
-  describe '.admin_login' do
+  describe '#admin_login' do
     let(:admin) { Fabricate(:admin) }
     let(:user) { Fabricate(:user) }
 
@@ -555,14 +552,12 @@ describe UsersController do
         end
       end
 
-      context 'needs 2-factor' do
+      describe 'when 2 factor authentication is enabled' do
+        let(:second_factor) { Fabricate(:user_second_factor, user: admin) }
         render_views
-        second_factor_data = "rcyryaqage3jexfj"
-        before do
-          admin.user_second_factor = UserSecondFactor.create(user_id: admin.id, method: "totp", data: second_factor_data, enabled: true)
-        end
 
         it 'does not log in when token required' do
+          second_factor
           token = admin.email_tokens.create(email: admin.email).token
           get :admin_login, params: { token: token }
           expect(response).not_to redirect_to('/')
@@ -572,7 +567,12 @@ describe UsersController do
 
         it 'logs in when a valid 2-factor token is given' do
           token = admin.email_tokens.create(email: admin.email).token
-          put :admin_login, params: { token: token, second_factor_token: ROTP::TOTP.new(second_factor_data).now }
+
+          put :admin_login, params: {
+            token: token,
+            second_factor_token: ROTP::TOTP.new(second_factor.data).now
+          }
+
           expect(response).to redirect_to('/')
           expect(session[:current_user_id]).to eq(admin.id)
         end

spec/fabricators/user_second_factor_fabricator.rb

@@ -0,0 +1,6 @@
+Fabricator(:user_second_factor) do
+  user
+  data 'rcyryaqage3jexfj'
+  enabled true
+  method UserSecondFactor.methods[:totp]
+end

spec/models/user_second_factor_spec.rb

@@ -0,0 +1,9 @@
+require 'rails_helper'
+
+RSpec.describe UserSecondFactor do
+  describe '.methods' do
+    it 'should retain the right order' do
+      expect(described_class.methods[:totp]).to eq(1)
+    end
+  end
+end

spec/requests/admin/users_controller_spec.rb

@@ -0,0 +1,50 @@
+require 'rails_helper'
+
+RSpec.describe Admin::UsersController do
+  let(:admin) { Fabricate(:admin) }
+  let(:user) { Fabricate(:user) }
+
+  describe '#disable_second_factor' do
+    let(:second_factor) { user.create_totp }
+
+    describe 'as an admin' do
+      before do
+        sign_in(admin)
+        second_factor
+        expect(user.reload.user_second_factor).to eq(second_factor)
+      end
+
+      it 'should able to disable the second factor for another user' do
+        SiteSetting.queue_jobs = true
+
+        expect do
+          put "/admin/users/#{user.id}/disable_second_factor.json"
+        end.to change { Jobs::CriticalUserEmail.jobs.length }.by(1)
+
+        expect(response.status).to eq(200)
+        expect(user.reload.user_second_factor).to eq(nil)
+
+        job_args = Jobs::CriticalUserEmail.jobs.first["args"].first
+
+        expect(job_args["user_id"]).to eq(user.id)
+        expect(job_args["type"]).to eq('account_second_factor_disabled')
+      end
+
+      it 'should not be able to disable the second factor for the current user' do
+        put "/admin/users/#{admin.id}/disable_second_factor.json"
+
+        expect(response.status).to eq(403)
+      end
+
+      describe 'when user does not have second factor enabled' do
+        it 'should raise the right error' do
+          user.user_second_factor.destroy!
+
+          put "/admin/users/#{user.id}/disable_second_factor.json"
+
+          expect(response.status).to eq(400)
+        end
+      end
+    end
+  end
+end

spec/requests/session_controller_spec.rb

@@ -138,10 +138,7 @@ RSpec.describe SessionController do
       end
 
       context 'user has 2-factor logins' do
-        second_factor_data = "rcyryaqage3jexfj"
+        let!(:user_second_factor) { Fabricate(:user_second_factor, user: user) }
-        before do
-          user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true)
-        end
 
         describe 'requires second factor' do
           it 'should return a second factor prompt' do
@@ -149,7 +146,9 @@ RSpec.describe SessionController do
 
             expect(response.status).to eq(200)
 
-            expect(CGI.unescapeHTML(response.body)).to include(I18n.t("login.second_factor_title"))
+            expect(CGI.unescapeHTML(response.body)).to include(I18n.t(
+              "login.second_factor_title"
+            ))
           end
         end
 
@@ -159,13 +158,17 @@ RSpec.describe SessionController do
 
             expect(response.status).to eq(200)
 
-            expect(CGI.unescapeHTML(response.body)).to include(I18n.t("login.invalid_second_factor_code"))
+            expect(CGI.unescapeHTML(response.body)).to include(I18n.t(
+              "login.invalid_second_factor_code"
+            ))
           end
         end
 
         describe 'allows successful 2-factor' do
           it 'logs in correctly' do
-            post "/session/email-login/#{email_token.token}", params: { second_factor_token: ROTP::TOTP.new(second_factor_data).now }
+            post "/session/email-login/#{email_token.token}", params: {
+              second_factor_token: ROTP::TOTP.new(user_second_factor.data).now
+            }
 
             expect(response).to redirect_to("/")
           end

spec/requests/users_controller_spec.rb

@@ -401,4 +401,118 @@ RSpec.describe UsersController do
       end
     end
   end
+
+  describe '#create_second_factor' do
+    context 'when not logged in' do
+      it 'should return the right response' do
+        post "/users/second_factors.json", params: {
+          password: 'wrongpassword'
+        }
+
+        expect(response.status).to eq(403)
+      end
+    end
+
+    context 'when logged in' do
+      before do
+        sign_in(user)
+      end
+
+      describe 'create 2fa request' do
+        it 'fails on incorrect password' do
+          post "/users/second_factors.json", params: {
+            password: 'wrongpassword'
+          }
+
+          expect(response.status).to eq(200)
+
+          expect(JSON.parse(response.body)['error']).to eq(I18n.t(
+            "login.incorrect_password")
+          )
+        end
+
+        it 'succeeds on correct password' do
+          post "/users/second_factors.json", params: {
+            password: 'somecomplicatedpassword'
+          }
+
+          expect(response.status).to eq(200)
+
+          response_body = JSON.parse(response.body)
+
+          expect(response_body['key']).to eq(user.user_second_factor.data)
+          expect(response_body['qr']).to be_present
+        end
+      end
+    end
+  end
+
+  describe '#update_second_factor' do
+    let(:user_second_factor) { Fabricate(:user_second_factor, user: user) }
+
+    context 'when not logged in' do
+      it 'should return the right response' do
+        put "/users/second_factor.json", params: {
+          second_factor_token: ROTP::TOTP.new(user_second_factor.data).now
+        }
+
+        expect(response.status).to eq(403)
+      end
+    end
+
+    context 'when logged in' do
+      before do
+        sign_in(user)
+        user_second_factor
+      end
+
+      context 'when user has totp setup' do
+        context 'when token is missing' do
+          it 'returns the right response' do
+            put "/users/second_factor.json", params: {
+              enable: 'true',
+            }
+
+            expect(response.status).to eq(400)
+          end
+        end
+
+        context 'when token is invalid' do
+          it 'returns the right response' do
+            put "/users/second_factor.json", params: {
+              second_factor_token: '000000',
+              enable: 'true',
+            }
+
+            expect(response.status).to eq(200)
+
+            expect(JSON.parse(response.body)['error']).to eq(I18n.t(
+              "login.invalid_second_factor_code"
+            ))
+          end
+        end
+
+        context 'when token is valid' do
+          it 'should allow second factor for the user to be enabled' do
+            put "/users/second_factor.json", params: {
+              second_factor_token: ROTP::TOTP.new(user_second_factor.data).now,
+              enable: 'true',
+            }
+
+            expect(response.status).to eq(200)
+            expect(user.reload.user_second_factor.enabled).to be true
+          end
+
+          it 'should allow second factor for the user to be disabled' do
+            put "/users/second_factor.json", params: {
+              second_factor_token: ROTP::TOTP.new(user_second_factor.data).now,
+            }
+
+            expect(response.status).to eq(200)
+            expect(user.reload.user_second_factor).to eq(nil)
+          end
+        end
+      end
+    end
+  end
 end

spec/requests/users_email_controller_spec.rb

@@ -2,7 +2,7 @@ require 'rails_helper'
 
 describe UsersEmailController do
 
-  describe '.confirm' do
+  describe '#confirm' do
     it 'errors out for invalid tokens' do
       get "/u/authorize-email/asdfasdf"
 
@@ -62,29 +62,37 @@ describe UsersEmailController do
       end
 
       context 'second factor required' do
-        second_factor_data = "rcyryaqage3jexfj"
+        let!(:second_factor) { Fabricate(:user_second_factor, user: user) }
-        before do
-          user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true)
-        end
 
         it 'requires a second factor token' do
           get "/u/authorize-email/#{user.email_tokens.last.token}"
-          expect(response.body).to include(I18n.t("login.second_factor_title"))
+
-          expect(response.body).not_to include(I18n.t("login.invalid_second_factor_code"))
+          expect(response.status).to eq(200)
+
+          response_body = response.body
+
+          expect(response_body).to include(I18n.t("login.second_factor_title"))
+          expect(response_body).not_to include(I18n.t("login.invalid_second_factor_code"))
         end
 
         it 'adds an error on a second factor attempt' do
           get "/u/authorize-email/#{user.email_tokens.last.token}", params: {
-                second_factor_token: "000000"
+            second_factor_token: "000000"
-              }
+          }
+
+          expect(response.status).to eq(200)
           expect(response.body).to include(I18n.t("login.invalid_second_factor_code"))
         end
 
         it 'confirms with a correct second token' do
           get "/u/authorize-email/#{user.email_tokens.last.token}", params: {
-                second_factor_token: ROTP::TOTP.new(second_factor_data).now
+            second_factor_token: ROTP::TOTP.new(second_factor.data).now
-              }
+          }
-          expect(response).to be_success
+
+          expect(response.status).to eq(200)
+
+          response_body = response.body
+
           expect(response.body).not_to include(I18n.t("login.second_factor_title"))
           expect(response.body).not_to include(I18n.t("login.invalid_second_factor_code"))
         end
@@ -92,17 +100,16 @@ describe UsersEmailController do
     end
   end
 
-  describe '.update' do
+  describe '#update' do
+    let(:user) { Fabricate(:user) }
     let(:new_email) { 'bubblegum@adventuretime.ooo' }
 
     it "requires you to be logged in" do
-      put "/u/asdf/preferences/email.json"
+      put "/u/#{user.username}/preferences/email.json", params: { email: new_email }
       expect(response.status).to eq(403)
     end
 
     context 'when logged in' do
-      let(:user) { Fabricate(:user) }
-
       before do
         sign_in(user)
       end

test/javascripts/acceptance/password-reset-test.js.es6

@@ -26,16 +26,21 @@ acceptance("Password Reset", {
     });
 
     server.get('/u/confirm-email-token/requiretwofactor.json', () => { //eslint-disable-line
-      return response({success: "OK"});
+      return response({ success: "OK" });
     });
+
     server.put('/u/password-reset/requiretwofactor.json', request => { //eslint-disable-line
       const body = parsePostData(request.requestBody);
       if (body.password === "perf3ctly5ecur3" && body.second_factor_token === "123123") {
-        return response({success: "OK", message: I18n.t('password_reset.success')});
+        return response({ success: "OK", message: I18n.t('password_reset.success') });
       } else if (body.second_factor_token === "123123") {
-        return response({success: false, errors: {password: ["invalid"]}});
+        return response({ success: false, errors: { password: ["invalid"] } });
       } else {
-        return response({success: false, message: "invalid token", errors: {second_factor: ["invalid token"]}});
+        return response({
+          success: false,
+          message: "invalid token",
+          errors: { user_second_factor: ["invalid token"] }
+        });
       }
     });
   }
@@ -75,24 +80,33 @@ QUnit.test("Password Reset Page", assert => {
 });
 
 QUnit.test("Password Reset Page With Second Factor", assert => {
-  PreloadStore.store('password_reset', {is_developer: false, second_factor_required: true});
+  PreloadStore.store('password_reset', {
+    is_developer: false,
+    second_factor_required: true
+  });
 
   visit("/u/password-reset/requiretwofactor");
+
   andThen(() => {
     assert.notOk(exists("#new-account-password"), "does not show the input");
     assert.ok(exists("#second-factor"), "shows the second factor prompt");
   });
 
   fillIn('#second-factor', '0000');
-
   click('.password-reset form button');
+
   andThen(() => {
     assert.ok(exists(".alert-error"), "shows 2 factor error");
-    assert.ok(find(".alert-error").html().indexOf("invalid token") > -1, "server validation error message shows");
+
+    assert.ok(
+      find(".alert-error").html().indexOf("invalid token") > -1,
+      "shows server validation error message"
+    );
   });
 
   fillIn('#second-factor', '123123');
   click('.password-reset form button');
+
   andThen(() => {
     assert.notOk(exists(".alert-error"), "hides error");
     assert.ok(exists("#new-account-password"), "shows the input");
@@ -100,6 +114,7 @@ QUnit.test("Password Reset Page With Second Factor", assert => {
 
   fillIn('.password-reset input', 'perf3ctly5ecur3');
   click('.password-reset form button');
+
   andThen(() => {
     assert.ok(!exists(".password-reset form"), "form is gone");
   });

test/javascripts/acceptance/preferences-test.js.es6

@@ -10,8 +10,15 @@ acceptance("User Preferences", {
       ];
     };
 
-    server.post('/second_factor/create', () => { //eslint-disable-line
+    server.post('/u/second_factors.json', () => { //eslint-disable-line
-      return response({key: "rcyryaqage3jexfj", qr: '<div id="test-qr">qr-code</div>'});
+      return response({
+        key: "rcyryaqage3jexfj",
+        qr: '<div id="test-qr">qr-code</div>'
+      });
+    });
+
+    server.put('/u/second_factor.json', () => { //eslint-disable-line
+      return response({ error: 'invalid token' });
     });
   }
 });
@@ -91,13 +98,26 @@ QUnit.test("email", assert => {
 
 QUnit.test("second factor", assert => {
   visit("/u/eviltrout/preferences/second-factor");
+
   andThen(() => {
     assert.ok(exists("#password"), "it has a password input");
   });
+
   fillIn('#password', 'secrets');
   click(".user-content .btn-primary");
+
   andThen(() => {
     assert.ok(exists("#test-qr"), "shows qr code");
     assert.notOk(exists("#password"), "it hides the password input");
   });
+
+  fillIn("#second-factor-token", '111111');
+  click('.btn-primary');
+
+  andThen(() => {
+    assert.ok(
+      find(".alert-error").html().indexOf("invalid token") > -1,
+      "shows server validation error message"
+    );
+  });
 });

test/javascripts/acceptance/sign-in-test.js.es6

@@ -79,14 +79,15 @@ QUnit.test("sign in - not activated - edit email", assert => {
 QUnit.test("second factor", assert => {
   visit("/");
   click("header .login-button");
+
   andThen(() => {
     assert.ok(exists('.login-modal'), "it shows the login modal");
   });
 
-  // Login with username and password only
   fillIn('#login-account-name', 'eviltrout');
   fillIn('#login-account-password', 'need-second-factor');
   click('.modal-footer .btn-primary');
+
   andThen(() => {
     assert.not(exists('#modal-alert:visible'), 'it hides the login error');
     assert.not(exists('#credentials:visible'), 'it hides the username and password prompt');
@@ -94,9 +95,9 @@ QUnit.test("second factor", assert => {
     assert.not(exists('.modal-footer .btn-primary:disabled'), "enables the login button");
   });
 
-  // Login with username, password, and token
   fillIn('#login-second-factor', '123456');
   click('.modal-footer .btn-primary');
+
   andThen(() => {
     assert.ok(exists('.modal-footer .btn-primary:disabled'), "disables the login button");
   });

test/javascripts/helpers/create-pretender.js.es6

@@ -229,8 +229,9 @@ export default function() {
 
       if (data.password === 'need-second-factor') {
         if (data.second_factor_token) {
-          return response({username: 'eviltrout'});
+          return response({ username: 'eviltrout' });
         }
+
         return response({ error: "Invalid Second Factor",
                           reason: "invalid_second_factor",
                           sent_to_email: 'eviltrout@example.com',