From 14f3594f9f39dce7f6a5dda44287560b0a6e53e8 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 20 Feb 2018 14:44:51 +0800 Subject: [PATCH] Review Changes for https://github.com/discourse/discourse/pull/5612/commits/f4f8a293e74e6a37c7bee7645d9ca1d72a7d5bd3. --- .../admin/controllers/admin-user-index.js.es6 | 10 +- .../admin/models/admin-user.js.es6 | 2 +- .../discourse/controllers/login.js.es6 | 22 ++- .../controllers/password-reset.js.es6 | 16 +- .../preferences/second-factor.js.es6 | 67 +++---- .../javascripts/discourse/models/user.js.es6 | 13 +- .../routes/preferences-second-factor.js.es6 | 10 +- .../discourse/routes/preferences.js.es6 | 1 + .../components/second-factor-form.hbs | 1 + .../templates/mobile/modal/login.hbs | 16 +- .../discourse/templates/modal/login.hbs | 8 +- .../templates/preferences-second-factor.hbs | 165 +++++++++++------- .../templates/preferences/account.hbs | 17 +- app/controllers/admin/users_controller.rb | 13 +- app/controllers/second_factor_controller.rb | 51 ------ app/controllers/session_controller.rb | 30 ++-- app/controllers/users_controller.rb | 95 ++++++++-- app/controllers/users_email_controller.rb | 35 ++-- app/helpers/second_factor_helper.rb | 35 ---- app/models/concerns/second_factor_manager.rb | 38 ++++ app/models/email_token.rb | 9 - app/models/user.rb | 5 +- app/models/user_second_factor.rb | 20 +++ .../admin_detailed_user_serializer.rb | 11 +- app/serializers/user_serializer.rb | 7 +- app/views/session/email_login.html.erb | 3 +- app/views/users_email/confirm.html.erb | 2 +- config/application.rb | 15 +- config/locales/client.en.yml | 18 +- config/locales/server.en.yml | 15 +- config/routes.rb | 5 +- ...180109222722_create_user_second_factors.rb | 4 +- .../concern/second_factor_manager_spec.rb | 93 ++++++++++ .../admin/users_controller_spec.rb | 13 -- .../second_factor_controller_spec.rb | 69 -------- spec/controllers/session_controller_spec.rb | 69 +++++--- spec/controllers/users_controller_spec.rb | 36 ++-- .../user_second_factor_fabricator.rb | 6 + spec/models/user_second_factor_spec.rb | 9 + spec/requests/admin/users_controller_spec.rb | 50 ++++++ spec/requests/session_controller_spec.rb | 17 +- spec/requests/users_controller_spec.rb | 114 ++++++++++++ spec/requests/users_email_controller_spec.rb | 39 +++-- .../acceptance/password-reset-test.js.es6 | 29 ++- .../acceptance/preferences-test.js.es6 | 24 ++- .../acceptance/sign-in-test.js.es6 | 5 +- .../helpers/create-pretender.js.es6 | 3 +- 47 files changed, 843 insertions(+), 492 deletions(-) delete mode 100644 app/controllers/second_factor_controller.rb delete mode 100644 app/helpers/second_factor_helper.rb create mode 100644 app/models/concerns/second_factor_manager.rb create mode 100644 spec/components/concern/second_factor_manager_spec.rb delete mode 100644 spec/controllers/second_factor_controller_spec.rb create mode 100644 spec/fabricators/user_second_factor_fabricator.rb create mode 100644 spec/models/user_second_factor_spec.rb create mode 100644 spec/requests/admin/users_controller_spec.rb diff --git a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 b/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 index b232d5bc288..770d5588b06 100644 --- a/app/assets/javascripts/admin/controllers/admin-user-index.js.es6 +++ b/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(); }, diff --git a/app/assets/javascripts/admin/models/admin-user.js.es6 b/app/assets/javascripts/admin/models/admin-user.js.es6 index 52348e4019e..99aec6aaf71 100644 --- a/app/assets/javascripts/admin/models/admin-user.js.es6 +++ b/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); diff --git a/app/assets/javascripts/discourse/controllers/login.js.es6 b/app/assets/javascripts/discourse/controllers/login.js.es6 index 51033172214..31d339b7c05 100644 --- a/app/assets/javascripts/discourse/controllers/login.js.es6 +++ b/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() { - return this.get('loggingIn') ? I18n.t('login.logging_in') : I18n.t('login.title'); - }.property('loggingIn'), + @computed('loggingIn') + loginButtonLabel(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; - } - if (result.reason === 'not_activated') { + } else if (result.reason === 'not_activated') { self.send('showNotActivated', { username: self.get('loginName'), sentTo: escape(result.sent_to_email), diff --git a/app/assets/javascripts/discourse/controllers/password-reset.js.es6 b/app/assets/javascripts/discourse/controllers/password-reset.js.es6 index 0c1a9e0faf5..bcf40ca88cf 100644 --- a/app/assets/javascripts/discourse/controllers/password-reset.js.es6 +++ b/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')) { - //ok 2factor - this.set('secondFactorRequired',false); - this.set('errorMessage', null); - } - else if (result.errors && result.errors.password && result.errors.password.length > 0) { + } else if (this.get('secondFactorRequired')) { + this.setProperties({ + secondFactorRequired: false, + errorMessage: null + }); + } 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); } diff --git a/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 b/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 index 4365bdd1688..990d2eb6f9b 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/second-factor.js.es6 +++ b/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 { userPath } from 'discourse/lib/url'; +import { default as DiscourseURL, 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(secondFactorImage, secondFactorKey) { - return secondFactorImage && secondFactorKey; - }, + loaded: Ember.computed.and('secondFactorImage', 'secondFactorKey'), @computed('loading') submitButtonText(loading) { - if (loading) return I18n.t('loading'); - return I18n.t('submit'); + return loading ? 'loading' : 'submit'; }, toggleSecondFactor(enable) { - if(!this.get('second_factor_token')) { - return; - } + if (!this.get('second_factor_token')) return; this.set('loading', true); - this.get('content').toggleSecondFactor(this.get('second_factor_token'), enable).then((resp) => { - if(resp.error) { - this.set('errorMessage',resp.error); - return; - } - this.set('errorMessage',null); - DiscourseURL.redirectTo(userPath(this.get('content').username.toLowerCase() + "/preferences")); - }) + + this.get('content').toggleSecondFactor(this.get('second_factor_token'), enable) + .then(response => { + if (response.error) { + this.set('errorMessage', response.error); + return; + } + + 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')) { - return; - } + if (!this.get('password')) return; this.set('loading', true); - this.get('content').loadSecondFactorCodes(this.get('password')).then((resp) => { - if(resp.error) { - this.set('errorMessage',resp.error); - return; - } - this.set('errorMessage',null); - this.set('secondFactorKey', resp.key); - this.set('secondFactorImage', resp.qr); - }).catch(popupAjaxError) + + this.get('content').loadSecondFactorCodes(this.get('password')) + .then(response => { + if(response.error) { + this.set('errorMessage', response.error); + return; + } + + 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); } diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index ad06bb36e4d..eef39aa8acc 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -305,19 +305,16 @@ const User = RestModel.extend({ }, loadSecondFactorCodes(password) { - return ajax("/second_factor/create", { - dataType: 'json', - data: { login: this.get('username'), - password: password}, + return ajax("/u/second_factors.json", { + data: { password }, type: 'POST' }); }, toggleSecondFactor(token, enable) { - return ajax(userPath(`${this.get('username_lower')}/preferences/second-factor`), { - dataType: 'json', - data: { token, enable }, - type: 'POST' + return ajax("/u/second_factor.json", { + data: { second_factor_token: token, enable }, + type: 'PUT' }); }, diff --git a/app/assets/javascripts/discourse/routes/preferences-second-factor.js.es6 b/app/assets/javascripts/discourse/routes/preferences-second-factor.js.es6 index fdc9a5fac75..b688ec813bc 100644 --- a/app/assets/javascripts/discourse/routes/preferences-second-factor.js.es6 +++ b/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 - deactivate() { - this._super(); - this.render('preferences', { into: 'user', controller: 'preferences' }); - }, - - setupController(controller, user) { - controller.setProperties({ model: user, newUsername: user.get('username') }); + setupController(controller, model) { + controller.setProperties({ model, newUsername: model.get('username') }); } }); diff --git a/app/assets/javascripts/discourse/routes/preferences.js.es6 b/app/assets/javascripts/discourse/routes/preferences.js.es6 index ef42132ddcf..128a3017315 100644 --- a/app/assets/javascripts/discourse/routes/preferences.js.es6 +++ b/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'); diff --git a/app/assets/javascripts/discourse/templates/components/second-factor-form.hbs b/app/assets/javascripts/discourse/templates/components/second-factor-form.hbs index 0535a9578cd..a1d5e033405 100644 --- a/app/assets/javascripts/discourse/templates/components/second-factor-form.hbs +++ b/app/assets/javascripts/discourse/templates/components/second-factor-form.hbs @@ -1,6 +1,7 @@ {{#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}} {{/if}} @@ -44,11 +48,11 @@ {{/if}} {{#if canLoginLocal}} - + {{d-button action="login" + icon="unlock" + label=loginButtonLabel + disabled=loginDisabled + class='btn btn-large btn-primary'}} {{#if showSignupLink}} + {{d-button action="login" + icon="unlock" + label=loginButtonLabel + disabled=loginDisabled + class='btn btn-large btn-primary'}} {{#if showSignupLink}} - {{else}} - {{#if loaded}} -

{{i18n 'user.second_factor.enable_description'}}

-
- {{{ secondFactorImage }}} -

- {{#if showSecondFactorKey}} - {{ secondFactorKey }} - {{else}} - {{i18n 'user.second_factor.show_key_description'}} - {{/if}} -

-
-
- - {{text-field value=second_factor_token id="second_factor_token" classNames="input-large" autofocus="autofocus"}} -
-

- {{#if errorMessage}} - {{errorMessage}} - {{/if}} -

-
- -
- {{else}} -
-

{{i18n 'user.second_factor.confirm_password_description'}}

- -
- {{text-field value=password id="password" type="password" classNames="input-xxlarge" autofocus="autofocus"}} -
-

- {{#if errorMessage}} - {{errorMessage}} - {{/if}} -

-
- -
-
- - {{#if saved}}{{i18n 'saved'}}{{/if}} -
-
- {{/if}} - {{/if}} +

{{i18n 'user.second_factor.title'}}

+ {{#if errorMessage}} +
+
+
{{errorMessage}}
+
+
+ {{/if}} + {{#if model.second_factor_enabled}} + + +
+
+ {{text-field value=second_factor_token + id="second_factor_token" + classNames="input-large" + autofocus="autofocus"}} +
+ +
+ {{i18n 'user.second_factor.disable_description'}} +
+
+ +
+
+ {{d-button action="disableSecondFactor" + class="btn btn-primary" + disabled=loading + label=submitButtonText}} +
+
+ {{else}} + {{#if loaded}} +
+
+ {{i18n 'user.second_factor.enable_description'}} +
+
+ +
+
+ {{{secondFactorImage}}} + +

+ {{#if showSecondFactorKey}} + {{secondFactorKey}} + {{else}} + {{i18n 'user.second_factor.show_key_description'}} + {{/if}} +

+
+
+ +
+ + +
+ {{text-field value=second_factor_token + id="second-factor-token" + classNames="input-xxlarge" + autofocus="autofocus"}} +
+
+ +
+
+ {{d-button action="enableSecondFactor" + class="btn btn-primary" + disabled=loading + label=submitButtonText}} +
+
+ {{else}} +
+ + +
+ {{text-field value=password + id="password" + type="password" + classNames="input-xxlarge" + autofocus="autofocus"}} +
+ +
+ {{i18n 'user.second_factor.confirm_password_description'}} +
+
+ +
+
+ {{d-button action="confirmPassword" + class="btn btn-primary" + disabled=loading + label=submitButtonText}} + + {{#if saved}}{{i18n 'saved'}}{{/if}} +
+
+ {{/if}} + {{/if}} diff --git a/app/assets/javascripts/discourse/templates/preferences/account.hbs b/app/assets/javascripts/discourse/templates/preferences/account.hbs index a37f525e9c6..225db632b9c 100644 --- a/app/assets/javascripts/discourse/templates/preferences/account.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/account.hbs @@ -14,7 +14,6 @@ {{/if}} - {{#if canEditName}}
@@ -69,17 +68,19 @@
+
- {{#link-to "preferences.second-factor" class="btn"}} - {{#if model.second_factor_enabled}} + {{#link-to "preferences.second-factor" class="btn"}} + {{#if model.second_factor_enabled}} {{d-icon "unlock-alt"}} - {{i18n 'user.second_factor.disable'}} - {{else}} + {{i18n 'user.second_factor.disable'}} + {{else}} {{d-icon "lock"}} - {{i18n 'user.second_factor.enable'}} - {{/if}} - {{/link-to}} + {{i18n 'user.second_factor.enable'}} + {{/if}} + {{/link-to}}
+
{{i18n 'user.second_factor.info_prompt'}}
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 75615c23b08..298161a88d1 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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 - if @user.user_second_factor.try(:delete) - StaffActionLogger.new(current_user).log_disable_second_factor_auth(@user) - end + guardian.ensure_can_disable_second_factor!(@user) + user_second_factor = @user.user_second_factor + raise Discourse::InvalidParameters unless user_second_factor + + 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 diff --git a/app/controllers/second_factor_controller.rb b/app/controllers/second_factor_controller.rb deleted file mode 100644 index ca22f0d25d5..00000000000 --- a/app/controllers/second_factor_controller.rb +++ /dev/null @@ -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 diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 61f93af2764..063ea7d2476 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -225,12 +225,13 @@ class SessionController < ApplicationController if payload = login_error_check(user) render json: payload else - - if SecondFactorHelper.totp_enabled?(user) - unless SecondFactorHelper.authenticate(user, params[:second_factor_token]) - return render json: { error: I18n.t("login.invalid_second_factor_code"), reason: "invalid_second_factor" } - end + if user.totp_enabled? && !user.authenticate_totp(params[:second_factor_token]) + return render json: failed_json.merge( + error: I18n.t("login.invalid_second_factor_code"), + reason: "invalid_second_factor" + ) 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' - end - if EmailToken.valid_token_format?(params[:token]) && (user = EmailToken.confirm(params[:token])) + @error = I18n.t('login.invalid_second_factor_code') + elsif user = EmailToken.confirm(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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4a9ccd31b00..6fdbb5ad924 100644 --- a/app/controllers/users_controller.rb +++ b/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? - if EmailToken.valid_token_format?(params[:token]) + elsif (token = params[:token]).present? + 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 diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index 45b7920a9a4..2fd289edb07 100644 --- a/app/controllers/users_email_controller.rb +++ b/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 - &.where('new_email_token_id = :token_id', token_id: token.id) - &.first - if change_req.try(:change_state) == EmailChangeRequest.states[:authorizing_new] && - !EmailToken.second_factor_valid(params[:token], params[:second_factor_token]) + + token = EmailToken.confirmable(params[:token]) + user = token&.user + + change_request = + 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' - return - end + else + updater = EmailUpdater.new + @update_result = updater.confirm(params[:token]) - updater = EmailUpdater.new - @update_result = updater.confirm(params[:token]) - - if @update_result == :complete - updater.user.user_stat.reset_bounce_score! - log_on_user(updater.user) + if @update_result == :complete + updater.user.user_stat.reset_bounce_score! + log_on_user(updater.user) + end end render layout: 'no_ember' diff --git a/app/helpers/second_factor_helper.rb b/app/helpers/second_factor_helper.rb deleted file mode 100644 index 0d2feb304a1..00000000000 --- a/app/helpers/second_factor_helper.rb +++ /dev/null @@ -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 diff --git a/app/models/concerns/second_factor_manager.rb b/app/models/concerns/second_factor_manager.rb new file mode 100644 index 00000000000..a9a04cd390c --- /dev/null +++ b/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 diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 7cfe74dab58..2a10dd4e234 100644 --- a/app/models/email_token.rb +++ b/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) diff --git a/app/models/user.rb b/app/models/user.rb index 0514b7038c6..f993bddab13 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/models/user_second_factor.rb b/app/models/user_second_factor.rb index da48d2302f3..acd16cf134a 100644 --- a/app/models/user_second_factor.rb +++ b/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 +# diff --git a/app/serializers/admin_detailed_user_serializer.rb b/app/serializers/admin_detailed_user_serializer.rb index f1a0e1c93f5..e0ad2abcfc7 100644 --- a/app/serializers/admin_detailed_user_serializer.rb +++ b/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? - scope.is_staff? + def second_factor_enabled + object.totp_enabled? end def can_disable_second_factor - (object.id && object.id != scope.user.try(:id)) && - scope.is_staff? - end - - def second_factor_enabled - SecondFactorHelper.totp_enabled?(object) + object&.id != scope.user.id end def can_revoke_admin diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 63677173ce2..1e45721b685 100644 --- a/app/serializers/user_serializer.rb +++ b/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)) || - scope.is_staff? + (object&.id == scope.user&.id) || scope.is_staff? end def second_factor_enabled - SecondFactorHelper.totp_enabled?(object) + object.totp_enabled? end def can_change_bio diff --git a/app/views/session/email_login.html.erb b/app/views/session/email_login.html.erb index 03704e0a441..43d988162f9 100644 --- a/app/views/session/email_login.html.erb +++ b/app/views/session/email_login.html.erb @@ -3,6 +3,7 @@ <%= @error %>
<%end%> + <%if @second_factor_required%>
@@ -10,7 +11,7 @@

<%=t "login.second_factor_title" %>

<%= label_tag(:second_factor_token, t("login.second_factor_description")) %>
<%= text_field_tag(:second_factor_token) %>
- <%= submit_tag(t("login.submit"), class: "btn btn-large btn-primary") %> + <%= submit_tag(t("submit"), class: "btn btn-large btn-primary") %> <%end%>
diff --git a/app/views/users_email/confirm.html.erb b/app/views/users_email/confirm.html.erb index 9411b3473ce..35877cd95d0 100644 --- a/app/views/users_email/confirm.html.erb +++ b/app/views/users_email/confirm.html.erb @@ -16,7 +16,7 @@ <% if @show_invalid_second_factor_error %>
<%= t('login.invalid_second_factor_code') %>
<% end %> - <%= submit_tag t('login.submit'), class: "btn btn-primary" %> + <%= submit_tag t('submit'), class: "btn btn-primary" %> <% end %> <% else %>
diff --git a/config/application.rb b/config/application.rb index eb2b23be4d8..bb98916354c 100644 --- a/config/application.rb +++ b/config/application.rb @@ -129,13 +129,14 @@ module Discourse # Configure sensitive parameters which will be filtered from the log file. config.filter_parameters += [ - :password, - :pop3_polling_password, - :api_key, - :s3_secret_access_key, - :twitter_consumer_secret, - :facebook_app_secret, - :github_client_secret + :password, + :pop3_polling_password, + :api_key, + :s3_secret_access_key, + :twitter_consumer_secret, + :facebook_app_secret, + :github_client_secret, + :second_factor_token, ] # Enable the asset pipeline diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 835cbfbe4f4..a0069439d1e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -710,14 +710,14 @@ en: second_factor: title: "Two Factor Authentication" - enable: "Enable 2-Step Verification" - disable: "Disable 2-Step Verification" - confirm_password_description: "Confirm your password to continue enabling 2-Step Verification." - enable_description: "To complete 2-Step Verification setup, scan the following QR code and submit a 2-Step Verification code." - disable_description: "Enter a 2-Step Verification code to disable." + enable: "Enable Two Factor Authentication" + disable: "Disable Two Factor Authentication" + confirm_password_description: "Confirm your password to continue enabling Two Factor Authentication." + enable_description: "To complete Two Factor Authentication setup, scan the following QR code and submit a Two Factor Authentication code." + 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?" - 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 Google Authenticator, Authy, and FreeOTP." + 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 Google Authenticator, Authy, and FreeOTP." 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." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 42e61730f49..0f0d44035c7 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -49,6 +49,7 @@ en: loading: "Loading" powered_by_html: 'Powered by Discourse, 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: "

You’re almost done! We sent an activation mail to %{email}. Please follow the instructions in the mail to activate your account.

If it doesn’t arrive, check your spam folder.

" @@ -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_description: "Enter a generated verification code." - invalid_second_factor_code: "Invalid 2-Step Verification Code" - submit: "Submit" + second_factor_title: "Two Factor Authentication Required" + second_factor_description: "Enter a generated authentication code." + invalid_second_factor_code: "Invalid Two Factor Authentication Code" user: no_accounts_associated: "No accounts associated" @@ -2735,10 +2736,10 @@ en: account_second_factor_disabled: - title: "2-Step Verification disabled" - subject_template: "[%{email_prefix}] 2-Step Verification disabled" + title: "Two Factor Authentication 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). diff --git a/config/routes.rb b/config/routes.rb index bf838b8ff46..a3b09d8fcac 100644 --- a/config/routes.rb +++ b/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 } diff --git a/db/migrate/20180109222722_create_user_second_factors.rb b/db/migrate/20180109222722_create_user_second_factors.rb index 46abb216d19..ff038c17762 100644 --- a/db/migrate/20180109222722_create_user_second_factors.rb +++ b/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.string :data + t.integer :method, null: false + t.string :data, null: false t.boolean :enabled, null: false, default: false t.timestamp :last_used t.timestamps diff --git a/spec/components/concern/second_factor_manager_spec.rb b/spec/components/concern/second_factor_manager_spec.rb new file mode 100644 index 00000000000..afc969d915e --- /dev/null +++ b/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 diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 0c81cde5969..703021890fc 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/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) } diff --git a/spec/controllers/second_factor_controller_spec.rb b/spec/controllers/second_factor_controller_spec.rb deleted file mode 100644 index d74ba44ce7a..00000000000 --- a/spec/controllers/second_factor_controller_spec.rb +++ /dev/null @@ -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 diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 0b83ab8ab2c..1f980a1fcb3 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/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" - before do - user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true) - end + let!(:user_second_factor) { Fabricate(:user_second_factor, user: user) } - describe 'failure no 2-factor' do - it 'should return an error' do + describe 'when second factor token is missing' do + it 'should return the right response' do post :create, params: { - login: user.username, password: 'myawesomepassword' - }, format: :json - expect(JSON.parse(response.body)['error']).to eq(I18n.t('login.invalid_second_factor_code')) + login: user.username, + password: 'myawesomepassword', + }, 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' - }, format: :json + login: user.username, + 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' - }, format: :json + login: user.username, + 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 diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 206fd6bcf59..19a324d7f4b 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/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 - - 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 + context '2 factor authentication required' do + let!(:second_factor) { Fabricate(:user_second_factor, user: user) } 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, - params: { token: token, password: 'hg9ow8yHG32O', second_factor_token: ROTP::TOTP.new(second_factor_data).now } + put :password_reset, params: { + 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 diff --git a/spec/fabricators/user_second_factor_fabricator.rb b/spec/fabricators/user_second_factor_fabricator.rb new file mode 100644 index 00000000000..1bb88567873 --- /dev/null +++ b/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 diff --git a/spec/models/user_second_factor_spec.rb b/spec/models/user_second_factor_spec.rb new file mode 100644 index 00000000000..2a61b064222 --- /dev/null +++ b/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 diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb new file mode 100644 index 00000000000..8d665aea2ea --- /dev/null +++ b/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 diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 65004efe145..246f30c647f 100644 --- a/spec/requests/session_controller_spec.rb +++ b/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" - before do - user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true) - end + let!(:user_second_factor) { Fabricate(:user_second_factor, user: user) } 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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index ba41c482ecd..cd2628ba3c7 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index e7feb560dbd..624ba871e54 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/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" - before do - user.user_second_factor = UserSecondFactor.create(user_id: user.id, method: "totp", data: second_factor_data, enabled: true) - end + let!(:second_factor) { Fabricate(:user_second_factor, user: user) } 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 - } - expect(response).to be_success + second_factor_token: ROTP::TOTP.new(second_factor.data).now + } + + 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 diff --git a/test/javascripts/acceptance/password-reset-test.js.es6 b/test/javascripts/acceptance/password-reset-test.js.es6 index 211ef5f47b8..b105315b47d 100644 --- a/test/javascripts/acceptance/password-reset-test.js.es6 +++ b/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"); }); diff --git a/test/javascripts/acceptance/preferences-test.js.es6 b/test/javascripts/acceptance/preferences-test.js.es6 index c8fbee7aae0..5176eed55cd 100644 --- a/test/javascripts/acceptance/preferences-test.js.es6 +++ b/test/javascripts/acceptance/preferences-test.js.es6 @@ -10,8 +10,15 @@ acceptance("User Preferences", { ]; }; - server.post('/second_factor/create', () => { //eslint-disable-line - return response({key: "rcyryaqage3jexfj", qr: '
qr-code
'}); + server.post('/u/second_factors.json', () => { //eslint-disable-line + return response({ + key: "rcyryaqage3jexfj", + qr: '
qr-code
' + }); + }); + + 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" + ); + }); }); diff --git a/test/javascripts/acceptance/sign-in-test.js.es6 b/test/javascripts/acceptance/sign-in-test.js.es6 index ffe8c9cab7a..1320be56ee2 100644 --- a/test/javascripts/acceptance/sign-in-test.js.es6 +++ b/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"); }); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index db8b8520a1c..28e2607b21b 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/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',