From ecda6eccf6b87a499960b8caf9d869b961132637 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 30 Apr 2019 00:45:44 +0200 Subject: [PATCH 1/2] Remove password reset and sign up link below two factor form They don't make sense on that page, because at this stage, the user already has an account and also has already entered their password. closes #8005 --- app/views/sessions/two_factor.html.haml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/views/sessions/two_factor.html.haml b/app/views/sessions/two_factor.html.haml index f7fd7f592..76c29bbd4 100644 --- a/app/views/sessions/two_factor.html.haml +++ b/app/views/sessions/two_factor.html.haml @@ -29,11 +29,3 @@ = f.button t("devise.sessions.new.sign_in"), type: :submit, class: "btn btn-large btn-block btn-primary" - - .text-center - - if display_password_reset_link? - = link_to t("devise.shared.links.forgot_your_password"), - new_password_path(resource_name), id: "forgot_password_link" - %br - - if display_registration_link? - = link_to t("devise.shared.links.sign_up"), new_registration_path(resource_name) From 54fd4846c0607ea1a3de07f65e9a39920cf4ba8a Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Tue, 30 Apr 2019 02:36:23 +0200 Subject: [PATCH 2/2] Use password to disable 2FA instead of a token Using token doesn't make much sense when you can generate new tokens right below. closes #8006 --- .../two_factor_authentications_controller.rb | 9 ++----- .../_deactivate.haml | 5 ++-- config/locales/diaspora/en.yml | 2 +- .../desktop/two_factor_authentication.feature | 24 ++++++------------- features/step_definitions/session_steps.rb | 2 +- features/step_definitions/two_factor_steps.rb | 16 ------------- ..._factor_authentications_controller_spec.rb | 11 ++++----- 7 files changed, 18 insertions(+), 51 deletions(-) diff --git a/app/controllers/two_factor_authentications_controller.rb b/app/controllers/two_factor_authentications_controller.rb index e5350b38e..8dafbf3c4 100644 --- a/app/controllers/two_factor_authentications_controller.rb +++ b/app/controllers/two_factor_authentications_controller.rb @@ -37,12 +37,12 @@ class TwoFactorAuthenticationsController < ApplicationController end def destroy - if acceptable_code? + if current_user.valid_password?(params[:two_factor_authentication][:password]) current_user.otp_required_for_login = false current_user.save! flash[:notice] = t("two_factor_auth.flash.success_deactivation") else - flash.now[:alert] = t("two_factor_auth.flash.error_token") + flash[:alert] = t("users.destroy.wrong_password") end redirect_to two_factor_authentication_path end @@ -52,9 +52,4 @@ class TwoFactorAuthenticationsController < ApplicationController def verify_otp_required redirect_to two_factor_authentication_path if current_user.otp_required_for_login? end - - def acceptable_code? - current_user.validate_and_consume_otp!(params[:two_factor_authentication][:code]) || - current_user.invalidate_otp_backup_code!(params[:two_factor_authentication][:code]) - end end diff --git a/app/views/two_factor_authentications/_deactivate.haml b/app/views/two_factor_authentications/_deactivate.haml index c982265de..501f2cb2c 100644 --- a/app/views/two_factor_authentications/_deactivate.haml +++ b/app/views/two_factor_authentications/_deactivate.haml @@ -13,10 +13,9 @@ = form_for "two_factor_authentication", url: two_factor_authentication_path, html: {method: :delete, class: "form-horizontal"} do |f| .form-group - = f.label :code, t("two_factor_auth.input_token.label"), class: "control-label col-sm-6" + = f.label :password, t("users.edit.current_password"), class: "control-label col-sm-6" .col-sm-6 - = f.text_field :code, placeholder: t("two_factor_auth.input_token.placeholder"), class: "form-control" - = t("two_factor_auth.recovery.reminder") + = f.password_field :password, class: "form-control" .clearfix= f.submit t("two_factor_auth.activated.change_button"), class: "btn btn-primary pull-right" %hr diff --git a/config/locales/diaspora/en.yml b/config/locales/diaspora/en.yml index 007c05efc..6e8a8a096 100644 --- a/config/locales/diaspora/en.yml +++ b/config/locales/diaspora/en.yml @@ -1316,7 +1316,7 @@ en: explanation: "Two-factor authentication is a powerful way to ensure you are the only one able to sign in to your account. When signing in, you will enter a 6-digit code along with your password to prove your identity. Be careful though: if you lose your phone and the recovery codes created when you activate this feature, access to your diaspora* account will be blocked forever." activated: status: "Two-factor authentication activated" - change_label: "Deactivate two-factor authentication by entering a TOTP token." + change_label: "Deactivate two-factor authentication by entering your password" change_button: "Deactivate" deactivated: status: "Two-factor authentication not activated" diff --git a/features/desktop/two_factor_authentication.feature b/features/desktop/two_factor_authentication.feature index ae8e2d2ef..1e2e8a61f 100644 --- a/features/desktop/two_factor_authentication.feature +++ b/features/desktop/two_factor_authentication.feature @@ -51,40 +51,30 @@ Feature: Two-factor autentication Scenario: Regenerating recovery codes Given a user with email "alice@test.com" - When I sign in as "alice@test.com" And 2fa is activated for "alice@test.com" + When I sign in as "alice@test.com" When I go to the two-factor authentication page Then I should see "Generate new recovery codes" When I press the recovery code generate button Then I should see a list of recovery codes - Scenario: Deactivating 2fa with correct token + Scenario: Deactivating 2fa with correct password Given a user with email "alice@test.com" - When I sign in as "alice@test.com" And 2fa is activated for "alice@test.com" + When I sign in as "alice@test.com" When I go to the two-factor authentication page Then I should see "Deactivate" - When I fill in a valid TOTP token to deactivate for "alice@test.com" + When I put in my password in "two_factor_authentication_password" And I press "Deactivate" Then I should see "Two-factor authentication not activated" - Scenario: Deactivating 2fa with recovery token + Scenario: Trying to deactivate with incorrect password Given a user with email "alice@test.com" - When I sign in as "alice@test.com" And 2fa is activated for "alice@test.com" + When I sign in as "alice@test.com" When I go to the two-factor authentication page Then I should see "Deactivate" - When I fill in a recovery code to deactivate from "alice@test.com" - And I press "Deactivate" - Then I should see "Two-factor authentication not activated" - - Scenario: Trying to deactivate with incorrect token - Given a user with email "alice@test.com" - When I sign in as "alice@test.com" - And 2fa is activated for "alice@test.com" - When I go to the two-factor authentication page - Then I should see "Deactivate" - When I fill in an invalid TOTP token to deactivate + When I fill in "two_factor_authentication_password" with "incorrect" And I press "Deactivate" Then I should see "Two-factor authentication activated" And I should see "Deactivate" diff --git a/features/step_definitions/session_steps.rb b/features/step_definitions/session_steps.rb index a31de421f..4b08d1498 100644 --- a/features/step_definitions/session_steps.rb +++ b/features/step_definitions/session_steps.rb @@ -31,7 +31,7 @@ When /^I (?:sign|log) in with password "([^"]*)"( on the mobile website)?$/ do | end When /^I put in my password in "([^"]*)"$/ do |field| - step %(I fill in "#{field}" with "#{@me.password}") + step %(I fill in "#{field}" with "#{@me.password}") end When /^I fill out change password section with my password and "([^"]*)" and "([^"]*)"$/ do |new_pass, confirm_pass| diff --git a/features/step_definitions/two_factor_steps.rb b/features/step_definitions/two_factor_steps.rb index 7b5ab2319..40e3cd7de 100644 --- a/features/step_definitions/two_factor_steps.rb +++ b/features/step_definitions/two_factor_steps.rb @@ -14,15 +14,6 @@ When /^I fill in an invalid TOTP token$/ do fill_in "user_otp_attempt", with: "c0ffee" end -When /^I fill in a valid TOTP token to deactivate for "([^"]*)"$/ do |username| - @me = find_user username - fill_in "two_factor_authentication_code", with: @me.current_otp -end - -When /^I fill in an invalid TOTP token to deactivate$/ do - fill_in "two_factor_authentication_code", with: "c0ffee" -end - When /^I fill in a recovery code from "([^"]*)"$/ do |username| @me = find_user username @codes = @me.generate_otp_backup_codes! @@ -30,13 +21,6 @@ When /^I fill in a recovery code from "([^"]*)"$/ do |username| fill_in "user_otp_attempt", with: @codes.first end -When /^I fill in a recovery code to deactivate from "([^"]*)"$/ do |username| - @me = find_user username - @codes = @me.generate_otp_backup_codes! - @me.save! - fill_in "two_factor_authentication_code", with: @codes.first -end - When /^I confirm activation$/ do find(".btn-primary", match: :first).click end diff --git a/spec/controllers/two_factor_authentications_controller_spec.rb b/spec/controllers/two_factor_authentications_controller_spec.rb index 585061385..e17a6e3ca 100644 --- a/spec/controllers/two_factor_authentications_controller_spec.rb +++ b/spec/controllers/two_factor_authentications_controller_spec.rb @@ -19,7 +19,6 @@ describe TwoFactorAuthenticationsController, type: :controller do get :show expect(response.body).to match I18n.t("two_factor_auth.title") expect(response.body).to match I18n.t("two_factor_auth.activated.status") - expect(response.body).to match I18n.t("two_factor_auth.input_token.label") expect(response.body).to match I18n.t("two_factor_auth.recovery.button") expect(@user).to have_attributes(otp_required_for_login: true) end @@ -90,16 +89,16 @@ describe TwoFactorAuthenticationsController, type: :controller do before do activate_2fa end - it "deactivates 2fa if token is correct" do - delete :destroy, params: {two_factor_authentication: {code: @user.current_otp}} + it "deactivates 2fa if password is correct" do + delete :destroy, params: {two_factor_authentication: {password: @user.password}} expect(response).to be_redirect expect(flash[:notice]).to match I18n.t("two_factor_auth.flash.success_deactivation") end - it "does nothing if token is wrong" do - delete :destroy, params: {two_factor_authentication: {code: "a wrong code"}} + it "does nothing if password is wrong" do + delete :destroy, params: {two_factor_authentication: {password: "a wrong password"}} expect(response).to be_redirect - expect(flash[:alert]).to match I18n.t("two_factor_auth.flash.error_token") + expect(flash[:alert]).to match I18n.t("users.destroy.wrong_password") end end