From 964ae8eee593687f922c873fa7b378bb6e3e39bb Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 22 Jul 2019 10:48:50 +0200 Subject: Change unconfirmed user login behaviour (#11375) Allow access to account settings, 2FA, authorized applications, and account deletions to unconfirmed and pending users, as well as users who had their accounts disabled. Suspended users cannot update their e-mail or password or delete their account. Display account status on account settings page, for example, when an account is frozen, limited, unconfirmed or pending review. After sign up, login users straight away and show a simple page that tells them the status of their account with links to account settings and logout, to reduce onboarding friction and allow users to correct wrongly typed e-mail addresses. Move the final sign-up step of SSO integrations to be the same as above to reduce code duplication. --- spec/controllers/auth/sessions_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/controllers/auth/sessions_controller_spec.rb') diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 71fcc1a6e..87ef4f2bb 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -160,8 +160,8 @@ RSpec.describe Auth::SessionsController, type: :controller do let(:unconfirmed_user) { user.tap { |u| u.update!(confirmed_at: nil) } } let(:accept_language) { 'fr' } - it 'shows a translated login error' do - expect(flash[:alert]).to eq(I18n.t('devise.failure.unconfirmed', locale: accept_language)) + it 'redirects to home' do + expect(response).to redirect_to(root_path) end end -- cgit From c707ef49d9b13932f4d98c127ec3148a5cdc3479 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 15 Sep 2019 21:08:39 +0200 Subject: Fix 2FA challenge and password challenge for non-database users (#11831) * Fix 2FA challenge not appearing for non-database users Fix #11685 * Fix account deletion not working when using external login Fix #11691 --- app/controllers/auth/sessions_controller.rb | 61 ++++++++++------------- app/controllers/settings/deletes_controller.rb | 25 +++++++--- app/models/form/delete_confirmation.rb | 2 +- app/views/settings/deletes/show.html.haml | 5 +- config/initializers/devise.rb | 7 ++- config/locales/en.yml | 3 +- spec/controllers/auth/sessions_controller_spec.rb | 24 +++------ 7 files changed, 66 insertions(+), 61 deletions(-) (limited to 'spec/controllers/auth/sessions_controller_spec.rb') diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 7e6dbf19e..3e93b2e68 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -8,8 +8,6 @@ class Auth::SessionsController < Devise::SessionsController skip_before_action :require_no_authentication, only: [:create] skip_before_action :require_functional! - prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create] - before_action :set_instance_presenter, only: [:new] before_action :set_body_classes @@ -22,9 +20,22 @@ class Auth::SessionsController < Devise::SessionsController end def create - super do |resource| - remember_me(resource) - flash.delete(:notice) + self.resource = begin + if user_params[:email].blank? && session[:otp_user_id].present? + User.find(session[:otp_user_id]) + else + warden.authenticate!(auth_options) + end + end + + if resource.otp_required_for_login? + if user_params[:otp_attempt].present? && session[:otp_user_id].present? + authenticate_with_two_factor_via_otp(resource) + else + prompt_for_two_factor(resource) + end + else + authenticate_and_respond(resource) end end @@ -37,18 +48,6 @@ class Auth::SessionsController < Devise::SessionsController protected - def find_user - if session[:otp_user_id] - User.find(session[:otp_user_id]) - elsif user_params[:email] - if use_seamless_external_login? && Devise.check_at_sign && user_params[:email].index('@').nil? - User.joins(:account).find_by(accounts: { username: user_params[:email] }) - else - User.find_for_authentication(email: user_params[:email]) - end - end - end - def user_params params.require(:user).permit(:email, :password, :otp_attempt) end @@ -71,32 +70,17 @@ class Auth::SessionsController < Devise::SessionsController super end - def two_factor_enabled? - find_user.try(:otp_required_for_login?) - end - def valid_otp_attempt?(user) user.validate_and_consume_otp!(user_params[:otp_attempt]) || user.invalidate_otp_backup_code!(user_params[:otp_attempt]) - rescue OpenSSL::Cipher::CipherError => _error + rescue OpenSSL::Cipher::CipherError false end - def authenticate_with_two_factor - user = self.resource = find_user - - if user_params[:otp_attempt].present? && session[:otp_user_id] - authenticate_with_two_factor_via_otp(user) - elsif user&.valid_password?(user_params[:password]) - prompt_for_two_factor(user) - end - end - def authenticate_with_two_factor_via_otp(user) if valid_otp_attempt?(user) session.delete(:otp_user_id) - remember_me(user) - sign_in(user) + authenticate_and_respond(user) else flash.now[:alert] = I18n.t('users.invalid_otp_token') prompt_for_two_factor(user) @@ -108,6 +92,13 @@ class Auth::SessionsController < Devise::SessionsController render :two_factor end + def authenticate_and_respond(user) + sign_in(user) + remember_me(user) + + respond_with user, location: after_sign_in_path_for(user) + end + private def set_instance_presenter @@ -120,9 +111,11 @@ class Auth::SessionsController < Devise::SessionsController def home_paths(resource) paths = [about_path] + if single_user_mode? && resource.is_a?(User) paths << short_account_path(username: resource.account) end + paths end diff --git a/app/controllers/settings/deletes_controller.rb b/app/controllers/settings/deletes_controller.rb index 97fe4d328..15a59c999 100644 --- a/app/controllers/settings/deletes_controller.rb +++ b/app/controllers/settings/deletes_controller.rb @@ -14,12 +14,11 @@ class Settings::DeletesController < Settings::BaseController end def destroy - if current_user.valid_password?(delete_params[:password]) - Admin::SuspensionWorker.perform_async(current_user.account_id, true) - sign_out + if challenge_passed? + destroy_account! redirect_to new_user_session_path, notice: I18n.t('deletes.success_msg') else - redirect_to settings_delete_path, alert: I18n.t('deletes.bad_password_msg') + redirect_to settings_delete_path, alert: I18n.t('deletes.challenge_not_passed') end end @@ -29,11 +28,25 @@ class Settings::DeletesController < Settings::BaseController redirect_to root_path unless Setting.open_deletion end - def delete_params - params.require(:form_delete_confirmation).permit(:password) + def resource_params + params.require(:form_delete_confirmation).permit(:password, :username) end def require_not_suspended! forbidden if current_account.suspended? end + + def challenge_passed? + if current_user.encrypted_password.blank? + current_account.username == resource_params[:username] + else + current_user.valid_password?(resource_params[:password]) + end + end + + def destroy_account! + current_account.suspend! + Admin::SuspensionWorker.perform_async(current_user.account_id, true) + sign_out + end end diff --git a/app/models/form/delete_confirmation.rb b/app/models/form/delete_confirmation.rb index 0884a09b8..99d04b331 100644 --- a/app/models/form/delete_confirmation.rb +++ b/app/models/form/delete_confirmation.rb @@ -3,5 +3,5 @@ class Form::DeleteConfirmation include ActiveModel::Model - attr_accessor :password + attr_accessor :password, :username end diff --git a/app/views/settings/deletes/show.html.haml b/app/views/settings/deletes/show.html.haml index 6e2ff31c5..08792e0af 100644 --- a/app/views/settings/deletes/show.html.haml +++ b/app/views/settings/deletes/show.html.haml @@ -20,7 +20,10 @@ %hr.spacer/ - = f.input :password, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_password') + - if current_user.encrypted_password.present? + = f.input :password, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_password') + - else + = f.input :username, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_username') .actions = f.button :button, t('deletes.proceed'), type: :submit, class: 'negative' diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index cd9bacf68..311583820 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -71,10 +71,13 @@ end Devise.setup do |config| config.warden do |manager| + manager.default_strategies(scope: :user).unshift :database_authenticatable manager.default_strategies(scope: :user).unshift :ldap_authenticatable if Devise.ldap_authentication manager.default_strategies(scope: :user).unshift :pam_authenticatable if Devise.pam_authentication - manager.default_strategies(scope: :user).unshift :two_factor_authenticatable - manager.default_strategies(scope: :user).unshift :two_factor_backupable + + # We handle 2FA in our own sessions controller so this gets in the way + manager.default_strategies(scope: :user).delete :two_factor_backupable + manager.default_strategies(scope: :user).delete :two_factor_authenticatable end # The secret key used by Devise. Devise uses this key to generate diff --git a/config/locales/en.yml b/config/locales/en.yml index 0a5ca31c1..8c9fe89f8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -632,8 +632,9 @@ en: x_months: "%{count}mo" x_seconds: "%{count}s" deletes: - bad_password_msg: The password you entered was incorrect + challenge_not_passed: The information you entered was not correct confirm_password: Enter your current password to verify your identity + confirm_username: Enter your username to confirm the procedure proceed: Delete account success_msg: Your account was successfully deleted warning: diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 87ef4f2bb..7ed5edde0 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -5,11 +5,11 @@ require 'rails_helper' RSpec.describe Auth::SessionsController, type: :controller do render_views - describe 'GET #new' do - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end + before do + request.env['devise.mapping'] = Devise.mappings[:user] + end + describe 'GET #new' do it 'returns http success' do get :new expect(response).to have_http_status(200) @@ -19,10 +19,6 @@ RSpec.describe Auth::SessionsController, type: :controller do describe 'DELETE #destroy' do let(:user) { Fabricate(:user) } - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end - context 'with a regular user' do it 'redirects to home after sign out' do sign_in(user, scope: :user) @@ -51,10 +47,6 @@ RSpec.describe Auth::SessionsController, type: :controller do end describe 'POST #create' do - before do - request.env['devise.mapping'] = Devise.mappings[:user] - end - context 'using PAM authentication', if: ENV['PAM_ENABLED'] == 'true' do context 'using a valid password' do before do @@ -191,11 +183,11 @@ RSpec.describe Auth::SessionsController, type: :controller do end context 'using two-factor authentication' do - let(:user) do - Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', - otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + let!(:user) do + Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) end - let(:recovery_codes) do + + let!(:recovery_codes) do codes = user.generate_otp_backup_codes! user.save return codes -- cgit From e1066cd4319a220d5be16e51ffaf5236a2f6e866 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 18 Sep 2019 16:37:27 +0200 Subject: Add password challenge to 2FA settings, e-mail notifications (#11878) Fix #3961 --- .../admin/two_factor_authentications_controller.rb | 1 + app/controllers/auth/challenges_controller.rb | 22 ++++ app/controllers/auth/sessions_controller.rb | 1 + app/controllers/concerns/challengable_concern.rb | 65 ++++++++++++ .../confirmations_controller.rb | 5 + .../recovery_codes_controller.rb | 6 ++ .../two_factor_authentications_controller.rb | 4 + app/javascript/styles/mastodon/admin.scss | 43 ++++---- app/javascript/styles/mastodon/forms.scss | 4 + app/mailers/user_mailer.rb | 33 ++++++ app/models/form/challenge.rb | 8 ++ app/models/user.rb | 9 +- app/views/auth/challenges/new.html.haml | 15 +++ app/views/auth/shared/_links.html.haml | 2 +- .../two_factor_authentications/show.html.haml | 38 +++---- .../user_mailer/two_factor_disabled.html.haml | 43 ++++++++ app/views/user_mailer/two_factor_disabled.text.erb | 7 ++ app/views/user_mailer/two_factor_enabled.html.haml | 43 ++++++++ app/views/user_mailer/two_factor_enabled.text.erb | 7 ++ .../two_factor_recovery_codes_changed.html.haml | 43 ++++++++ .../two_factor_recovery_codes_changed.text.erb | 7 ++ config/locales/devise.en.yml | 12 +++ config/locales/en.yml | 5 + config/locales/simple_form.en.yml | 2 + config/routes.rb | 1 + .../controllers/auth/challenges_controller_spec.rb | 46 +++++++++ spec/controllers/auth/sessions_controller_spec.rb | 2 +- .../concerns/challengable_concern_spec.rb | 114 +++++++++++++++++++++ .../confirmations_controller_spec.rb | 10 +- .../recovery_codes_controller_spec.rb | 2 +- .../two_factor_authentications_controller_spec.rb | 2 +- spec/mailers/previews/user_mailer_preview.rb | 15 +++ 32 files changed, 567 insertions(+), 50 deletions(-) create mode 100644 app/controllers/auth/challenges_controller.rb create mode 100644 app/controllers/concerns/challengable_concern.rb create mode 100644 app/models/form/challenge.rb create mode 100644 app/views/auth/challenges/new.html.haml create mode 100644 app/views/user_mailer/two_factor_disabled.html.haml create mode 100644 app/views/user_mailer/two_factor_disabled.text.erb create mode 100644 app/views/user_mailer/two_factor_enabled.html.haml create mode 100644 app/views/user_mailer/two_factor_enabled.text.erb create mode 100644 app/views/user_mailer/two_factor_recovery_codes_changed.html.haml create mode 100644 app/views/user_mailer/two_factor_recovery_codes_changed.text.erb create mode 100644 spec/controllers/auth/challenges_controller_spec.rb create mode 100644 spec/controllers/concerns/challengable_concern_spec.rb (limited to 'spec/controllers/auth/sessions_controller_spec.rb') diff --git a/app/controllers/admin/two_factor_authentications_controller.rb b/app/controllers/admin/two_factor_authentications_controller.rb index 2577a4b17..0652c3a7a 100644 --- a/app/controllers/admin/two_factor_authentications_controller.rb +++ b/app/controllers/admin/two_factor_authentications_controller.rb @@ -8,6 +8,7 @@ module Admin authorize @user, :disable_2fa? @user.disable_two_factor! log_action :disable_2fa, @user + UserMailer.two_factor_disabled(@user).deliver_later! redirect_to admin_accounts_path end diff --git a/app/controllers/auth/challenges_controller.rb b/app/controllers/auth/challenges_controller.rb new file mode 100644 index 000000000..060944240 --- /dev/null +++ b/app/controllers/auth/challenges_controller.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class Auth::ChallengesController < ApplicationController + include ChallengableConcern + + layout 'auth' + + before_action :authenticate_user! + + skip_before_action :require_functional! + + def create + if challenge_passed? + session[:challenge_passed_at] = Time.now.utc + redirect_to challenge_params[:return_to] + else + @challenge = Form::Challenge.new(return_to: challenge_params[:return_to]) + flash.now[:alert] = I18n.t('challenge.invalid_password') + render_challenge + end + end +end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 3e93b2e68..b3113bbef 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -42,6 +42,7 @@ class Auth::SessionsController < Devise::SessionsController def destroy tmp_stored_location = stored_location_for(:user) super + session.delete(:challenge_passed_at) flash.delete(:notice) store_location_for(:user, tmp_stored_location) if continue_after? end diff --git a/app/controllers/concerns/challengable_concern.rb b/app/controllers/concerns/challengable_concern.rb new file mode 100644 index 000000000..b29d90b3c --- /dev/null +++ b/app/controllers/concerns/challengable_concern.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +# This concern is inspired by "sudo mode" on GitHub. It +# is a way to re-authenticate a user before allowing them +# to see or perform an action. +# +# Add `before_action :require_challenge!` to actions you +# want to protect. +# +# The user will be shown a page to enter the challenge (which +# is either the password, or just the username when no +# password exists). Upon passing, there is a grace period +# during which no challenge will be asked from the user. +# +# Accessing challenge-protected resources during the grace +# period will refresh the grace period. +module ChallengableConcern + extend ActiveSupport::Concern + + CHALLENGE_TIMEOUT = 1.hour.freeze + + def require_challenge! + return if skip_challenge? + + if challenge_passed_recently? + session[:challenge_passed_at] = Time.now.utc + return + end + + @challenge = Form::Challenge.new(return_to: request.url) + + if params.key?(:form_challenge) + if challenge_passed? + session[:challenge_passed_at] = Time.now.utc + return + else + flash.now[:alert] = I18n.t('challenge.invalid_password') + render_challenge + end + else + render_challenge + end + end + + def render_challenge + @body_classes = 'lighter' + render template: 'auth/challenges/new', layout: 'auth' + end + + def challenge_passed? + current_user.valid_password?(challenge_params[:current_password]) + end + + def skip_challenge? + current_user.encrypted_password.blank? + end + + def challenge_passed_recently? + session[:challenge_passed_at].present? && session[:challenge_passed_at] >= CHALLENGE_TIMEOUT.ago + end + + def challenge_params + params.require(:form_challenge).permit(:current_password, :return_to) + end +end diff --git a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb index 46c90bf74..ef4df3339 100644 --- a/app/controllers/settings/two_factor_authentication/confirmations_controller.rb +++ b/app/controllers/settings/two_factor_authentication/confirmations_controller.rb @@ -3,9 +3,12 @@ module Settings module TwoFactorAuthentication class ConfirmationsController < BaseController + include ChallengableConcern + layout 'admin' before_action :authenticate_user! + before_action :require_challenge! before_action :ensure_otp_secret skip_before_action :require_functional! @@ -22,6 +25,8 @@ module Settings @recovery_codes = current_user.generate_otp_backup_codes! current_user.save! + UserMailer.two_factor_enabled(current_user).deliver_later! + render 'settings/two_factor_authentication/recovery_codes/index' else flash.now[:alert] = I18n.t('two_factor_authentication.wrong_code') diff --git a/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb index 09a759860..0c4f5bff7 100644 --- a/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb +++ b/app/controllers/settings/two_factor_authentication/recovery_codes_controller.rb @@ -3,16 +3,22 @@ module Settings module TwoFactorAuthentication class RecoveryCodesController < BaseController + include ChallengableConcern + layout 'admin' before_action :authenticate_user! + before_action :require_challenge!, on: :create skip_before_action :require_functional! def create @recovery_codes = current_user.generate_otp_backup_codes! current_user.save! + + UserMailer.two_factor_recovery_codes_changed(current_user).deliver_later! flash.now[:notice] = I18n.t('two_factor_authentication.recovery_codes_regenerated') + render :index end end diff --git a/app/controllers/settings/two_factor_authentications_controller.rb b/app/controllers/settings/two_factor_authentications_controller.rb index c93b17577..9118a7933 100644 --- a/app/controllers/settings/two_factor_authentications_controller.rb +++ b/app/controllers/settings/two_factor_authentications_controller.rb @@ -2,10 +2,13 @@ module Settings class TwoFactorAuthenticationsController < BaseController + include ChallengableConcern + layout 'admin' before_action :authenticate_user! before_action :verify_otp_required, only: [:create] + before_action :require_challenge!, only: [:create] skip_before_action :require_functional! @@ -23,6 +26,7 @@ module Settings if acceptable_code? current_user.otp_required_for_login = false current_user.save! + UserMailer.two_factor_disabled(current_user).deliver_later! redirect_to settings_two_factor_authentication_path else flash.now[:alert] = I18n.t('two_factor_authentication.wrong_code') diff --git a/app/javascript/styles/mastodon/admin.scss b/app/javascript/styles/mastodon/admin.scss index 5d4fe4ef8..074eee2cd 100644 --- a/app/javascript/styles/mastodon/admin.scss +++ b/app/javascript/styles/mastodon/admin.scss @@ -233,32 +233,35 @@ hr.spacer { height: 1px; } -.muted-hint { - color: $darker-text-color; +body, +.admin-wrapper .content { + .muted-hint { + color: $darker-text-color; - a { - color: $highlight-text-color; + a { + color: $highlight-text-color; + } } -} -.positive-hint { - color: $valid-value-color; - font-weight: 500; -} + .positive-hint { + color: $valid-value-color; + font-weight: 500; + } -.negative-hint { - color: $error-value-color; - font-weight: 500; -} + .negative-hint { + color: $error-value-color; + font-weight: 500; + } -.neutral-hint { - color: $dark-text-color; - font-weight: 500; -} + .neutral-hint { + color: $dark-text-color; + font-weight: 500; + } -.warning-hint { - color: $gold-star; - font-weight: 500; + .warning-hint { + color: $gold-star; + font-weight: 500; + } } .filters { diff --git a/app/javascript/styles/mastodon/forms.scss b/app/javascript/styles/mastodon/forms.scss index 16352340b..80ef8797d 100644 --- a/app/javascript/styles/mastodon/forms.scss +++ b/app/javascript/styles/mastodon/forms.scss @@ -254,6 +254,10 @@ code { &-6 { max-width: 50%; } + + .actions { + margin-top: 27px; + } } .fields-group:last-child, diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index b41004acc..6b81f6873 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -57,6 +57,39 @@ class UserMailer < Devise::Mailer end end + def two_factor_enabled(user, **) + @resource = user + @instance = Rails.configuration.x.local_domain + + return if @resource.disabled? + + I18n.with_locale(@resource.locale || I18n.default_locale) do + mail to: @resource.email, subject: I18n.t('devise.mailer.two_factor_enabled.subject') + end + end + + def two_factor_disabled(user, **) + @resource = user + @instance = Rails.configuration.x.local_domain + + return if @resource.disabled? + + I18n.with_locale(@resource.locale || I18n.default_locale) do + mail to: @resource.email, subject: I18n.t('devise.mailer.two_factor_disabled.subject') + end + end + + def two_factor_recovery_codes_changed(user, **) + @resource = user + @instance = Rails.configuration.x.local_domain + + return if @resource.disabled? + + I18n.with_locale(@resource.locale || I18n.default_locale) do + mail to: @resource.email, subject: I18n.t('devise.mailer.two_factor_recovery_codes_changed.subject') + end + end + def welcome(user) @resource = user @instance = Rails.configuration.x.local_domain diff --git a/app/models/form/challenge.rb b/app/models/form/challenge.rb new file mode 100644 index 000000000..40c99649c --- /dev/null +++ b/app/models/form/challenge.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class Form::Challenge + include ActiveModel::Model + + attr_accessor :current_password, :current_username, + :return_to +end diff --git a/app/models/user.rb b/app/models/user.rb index 78b82a68f..b48455802 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -264,17 +264,20 @@ class User < ApplicationRecord end def password_required? - return false if Devise.pam_authentication || Devise.ldap_authentication + return false if external? + super end def send_reset_password_instructions - return false if encrypted_password.blank? && (Devise.pam_authentication || Devise.ldap_authentication) + return false if encrypted_password.blank? + super end def reset_password!(new_password, new_password_confirmation) - return false if encrypted_password.blank? && (Devise.pam_authentication || Devise.ldap_authentication) + return false if encrypted_password.blank? + super end diff --git a/app/views/auth/challenges/new.html.haml b/app/views/auth/challenges/new.html.haml new file mode 100644 index 000000000..9aef2c35d --- /dev/null +++ b/app/views/auth/challenges/new.html.haml @@ -0,0 +1,15 @@ +- content_for :page_title do + = t('challenge.prompt') + += simple_form_for @challenge, url: request.get? ? auth_challenge_path : '' do |f| + = f.input :return_to, as: :hidden + + .field-group + = f.input :current_password, wrapper: :with_block_label, input_html: { :autocomplete => 'off', :autofocus => true }, label: t('challenge.prompt'), required: true + + .actions + = f.button :button, t('challenge.confirm'), type: :submit + + %p.hint.subtle-hint= t('challenge.hint_html') + +.form-footer= render 'auth/shared/links' diff --git a/app/views/auth/shared/_links.html.haml b/app/views/auth/shared/_links.html.haml index e6c3f7cca..66ed5b93f 100644 --- a/app/views/auth/shared/_links.html.haml +++ b/app/views/auth/shared/_links.html.haml @@ -11,7 +11,7 @@ - if controller_name != 'passwords' && controller_name != 'registrations' %li= link_to t('auth.forgot_password'), new_user_password_path - - if controller_name != 'confirmations' + - if controller_name != 'confirmations' && (!user_signed_in? || !current_user.confirmed? || current_user.unconfirmed_email.present?) %li= link_to t('auth.didnt_get_confirmation'), new_user_confirmation_path - if user_signed_in? && controller_name != 'setup' diff --git a/app/views/settings/two_factor_authentications/show.html.haml b/app/views/settings/two_factor_authentications/show.html.haml index 93509e022..f1eecd000 100644 --- a/app/views/settings/two_factor_authentications/show.html.haml +++ b/app/views/settings/two_factor_authentications/show.html.haml @@ -2,33 +2,35 @@ = t('settings.two_factor_authentication') - if current_user.otp_required_for_login - %p.positive-hint - = fa_icon 'check' - = ' ' - = t 'two_factor_authentication.enabled' + %p.hint + %span.positive-hint + = fa_icon 'check' + = ' ' + = t 'two_factor_authentication.enabled' - %hr/ + %hr.spacer/ = simple_form_for @confirmation, url: settings_two_factor_authentication_path, method: :delete do |f| - = f.input :otp_attempt, wrapper: :with_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true + .fields-group + = f.input :otp_attempt, wrapper: :with_block_label, hint: t('two_factor_authentication.code_hint'), label: t('simple_form.labels.defaults.otp_attempt'), input_html: { :autocomplete => 'off' }, required: true .actions - = f.button :button, t('two_factor_authentication.disable'), type: :submit + = f.button :button, t('two_factor_authentication.disable'), type: :submit, class: 'negative' - %hr/ + %hr.spacer/ - %h6= t('two_factor_authentication.recovery_codes') - %p.muted-hint - = t('two_factor_authentication.lost_recovery_codes') - = link_to t('two_factor_authentication.generate_recovery_codes'), - settings_two_factor_authentication_recovery_codes_path, - data: { method: :post } + %h3= t('two_factor_authentication.recovery_codes') + %p.muted-hint= t('two_factor_authentication.lost_recovery_codes') + + %hr.spacer/ + + .simple_form + = link_to t('two_factor_authentication.generate_recovery_codes'), settings_two_factor_authentication_recovery_codes_path, data: { method: :post }, class: 'block-button' - else .simple_form %p.hint= t('two_factor_authentication.description_html') - = link_to t('two_factor_authentication.setup'), - settings_two_factor_authentication_path, - data: { method: :post }, - class: 'block-button' + %hr.spacer/ + + = link_to t('two_factor_authentication.setup'), settings_two_factor_authentication_path, data: { method: :post }, class: 'block-button' diff --git a/app/views/user_mailer/two_factor_disabled.html.haml b/app/views/user_mailer/two_factor_disabled.html.haml new file mode 100644 index 000000000..651c6f940 --- /dev/null +++ b/app/views/user_mailer/two_factor_disabled.html.haml @@ -0,0 +1,43 @@ +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.hero + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center.padded + %table.hero-icon.alert-icon{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td + = image_tag full_pack_url('media/images/mailer/icon_lock_open.png'), alt: '' + + %h1= t 'devise.mailer.two_factor_disabled.title' + %p.lead= t 'devise.mailer.two_factor_disabled.explanation' + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.content-start + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.button-cell + %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.button-primary + = link_to edit_user_registration_url do + %span= t('settings.account_settings') diff --git a/app/views/user_mailer/two_factor_disabled.text.erb b/app/views/user_mailer/two_factor_disabled.text.erb new file mode 100644 index 000000000..73be1ddc2 --- /dev/null +++ b/app/views/user_mailer/two_factor_disabled.text.erb @@ -0,0 +1,7 @@ +<%= t 'devise.mailer.two_factor_disabled.title' %> + +=== + +<%= t 'devise.mailer.two_factor_disabled.explanation' %> + +=> <%= edit_user_registration_url %> diff --git a/app/views/user_mailer/two_factor_enabled.html.haml b/app/views/user_mailer/two_factor_enabled.html.haml new file mode 100644 index 000000000..fc31bd979 --- /dev/null +++ b/app/views/user_mailer/two_factor_enabled.html.haml @@ -0,0 +1,43 @@ +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.hero + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center.padded + %table.hero-icon{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td + = image_tag full_pack_url('media/images/mailer/icon_lock_open.png'), alt: '' + + %h1= t 'devise.mailer.two_factor_enabled.title' + %p.lead= t 'devise.mailer.two_factor_enabled.explanation' + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.content-start + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.button-cell + %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.button-primary + = link_to edit_user_registration_url do + %span= t('settings.account_settings') diff --git a/app/views/user_mailer/two_factor_enabled.text.erb b/app/views/user_mailer/two_factor_enabled.text.erb new file mode 100644 index 000000000..4319dddbf --- /dev/null +++ b/app/views/user_mailer/two_factor_enabled.text.erb @@ -0,0 +1,7 @@ +<%= t 'devise.mailer.two_factor_enabled.title' %> + +=== + +<%= t 'devise.mailer.two_factor_enabled.explanation' %> + +=> <%= edit_user_registration_url %> diff --git a/app/views/user_mailer/two_factor_recovery_codes_changed.html.haml b/app/views/user_mailer/two_factor_recovery_codes_changed.html.haml new file mode 100644 index 000000000..833708868 --- /dev/null +++ b/app/views/user_mailer/two_factor_recovery_codes_changed.html.haml @@ -0,0 +1,43 @@ +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.hero + .email-row + .col-6 + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.text-center.padded + %table.hero-icon.alert-icon{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td + = image_tag full_pack_url('media/images/mailer/icon_lock_open.png'), alt: '' + + %h1= t 'devise.mailer.two_factor_recovery_codes_changed.title' + %p.lead= t 'devise.mailer.two_factor_recovery_codes_changed.explanation' + +%table.email-table{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.email-body + .email-container + %table.content-section{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.content-cell.content-start + %table.column{ cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.column-cell.button-cell + %table.button{ align: 'center', cellspacing: 0, cellpadding: 0 } + %tbody + %tr + %td.button-primary + = link_to edit_user_registration_url do + %span= t('settings.account_settings') diff --git a/app/views/user_mailer/two_factor_recovery_codes_changed.text.erb b/app/views/user_mailer/two_factor_recovery_codes_changed.text.erb new file mode 100644 index 000000000..6ed12fc08 --- /dev/null +++ b/app/views/user_mailer/two_factor_recovery_codes_changed.text.erb @@ -0,0 +1,7 @@ +<%= t 'devise.mailer.two_factor_recovery_codes_changed.title' %> + +=== + +<%= t 'devise.mailer.two_factor_recovery_codes_changed.explanation' %> + +=> <%= edit_user_registration_url %> diff --git a/config/locales/devise.en.yml b/config/locales/devise.en.yml index 5defa6624..726d2426a 100644 --- a/config/locales/devise.en.yml +++ b/config/locales/devise.en.yml @@ -46,6 +46,18 @@ en: extra: If you didn't request this, please ignore this email. Your password won't change until you access the link above and create a new one. subject: 'Mastodon: Reset password instructions' title: Password reset + two_factor_disabled: + explanation: Two-factor authentication for your account has been disabled. Login is now possible using only e-mail address and password. + subject: 'Mastodon: Two-factor authentication disabled' + title: 2FA disabled + two_factor_enabled: + explanation: Two-factor authentication has been enabled for your account. A token generated by the paired TOTP app will be required for login. + subject: 'Mastodon: Two-factor authentication enabled' + title: 2FA enabled + two_factor_recovery_codes_changed: + explanation: The previous recovery codes have been invalidated and new ones generated. + subject: 'Mastodon: Two-factor recovery codes re-generated' + title: 2FA recovery codes changed unlock_instructions: subject: 'Mastodon: Unlock instructions' omniauth_callbacks: diff --git a/config/locales/en.yml b/config/locales/en.yml index f05fdd48b..da06b0e51 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -621,6 +621,11 @@ en: return: Show the user's profile web: Go to web title: Follow %{acct} + challenge: + confirm: Continue + hint_html: "Tip: We won't ask you for your password again for the next hour." + invalid_password: Invalid password + prompt: Confirm password to continue datetime: distance_in_words: about_x_hours: "%{count}h" diff --git a/config/locales/simple_form.en.yml b/config/locales/simple_form.en.yml index c542377a9..c9ffcfc13 100644 --- a/config/locales/simple_form.en.yml +++ b/config/locales/simple_form.en.yml @@ -43,6 +43,8 @@ en: domain: This domain will be able to fetch data from this server and incoming data from it will be processed and stored featured_tag: name: 'You might want to use one of these:' + form_challenge: + current_password: You are entering a secure area imports: data: CSV file exported from another Mastodon server invite_request: diff --git a/config/routes.rb b/config/routes.rb index a4dee2842..9ad1ea65d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -41,6 +41,7 @@ Rails.application.routes.draw do namespace :auth do resource :setup, only: [:show, :update], controller: :setup + resource :challenge, only: [:create], controller: :challenges end end diff --git a/spec/controllers/auth/challenges_controller_spec.rb b/spec/controllers/auth/challenges_controller_spec.rb new file mode 100644 index 000000000..2a6ca301e --- /dev/null +++ b/spec/controllers/auth/challenges_controller_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Auth::ChallengesController, type: :controller do + render_views + + let(:password) { 'foobar12345' } + let(:user) { Fabricate(:user, password: password) } + + before do + sign_in user + end + + describe 'POST #create' do + let(:return_to) { edit_user_registration_path } + + context 'with correct password' do + before { post :create, params: { form_challenge: { return_to: return_to, current_password: password } } } + + it 'redirects back' do + expect(response).to redirect_to(return_to) + end + + it 'sets session' do + expect(session[:challenge_passed_at]).to_not be_nil + end + end + + context 'with incorrect password' do + before { post :create, params: { form_challenge: { return_to: return_to, current_password: 'hhfggjjd562' } } } + + it 'renders challenge' do + expect(response).to render_template('auth/challenges/new') + end + + it 'displays error' do + expect(response.body).to include 'Invalid password' + end + + it 'does not set session' do + expect(session[:challenge_passed_at]).to be_nil + end + end + end +end diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 7ed5edde0..1950c173a 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -80,7 +80,7 @@ RSpec.describe Auth::SessionsController, type: :controller do let(:user) do account = Fabricate.build(:account, username: 'pam_user1') account.save!(validate: false) - user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account) + user = Fabricate(:user, email: 'pam@example.com', password: nil, account: account, external: true) user end diff --git a/spec/controllers/concerns/challengable_concern_spec.rb b/spec/controllers/concerns/challengable_concern_spec.rb new file mode 100644 index 000000000..4db3b740d --- /dev/null +++ b/spec/controllers/concerns/challengable_concern_spec.rb @@ -0,0 +1,114 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ChallengableConcern, type: :controller do + controller(ApplicationController) do + include ChallengableConcern + + before_action :require_challenge! + + def foo + render plain: 'foo' + end + + def bar + render plain: 'bar' + end + end + + before do + routes.draw do + get 'foo' => 'anonymous#foo' + post 'bar' => 'anonymous#bar' + end + end + + context 'with a no-password user' do + let(:user) { Fabricate(:user, external: true, password: nil) } + + before do + sign_in user + end + + context 'for GET requests' do + before { get :foo } + + it 'does not ask for password' do + expect(response.body).to eq 'foo' + end + end + + context 'for POST requests' do + before { post :bar } + + it 'does not ask for password' do + expect(response.body).to eq 'bar' + end + end + end + + context 'with recent challenge in session' do + let(:password) { 'foobar12345' } + let(:user) { Fabricate(:user, password: password) } + + before do + sign_in user + end + + context 'for GET requests' do + before { get :foo, session: { challenge_passed_at: Time.now.utc } } + + it 'does not ask for password' do + expect(response.body).to eq 'foo' + end + end + + context 'for POST requests' do + before { post :bar, session: { challenge_passed_at: Time.now.utc } } + + it 'does not ask for password' do + expect(response.body).to eq 'bar' + end + end + end + + context 'with a password user' do + let(:password) { 'foobar12345' } + let(:user) { Fabricate(:user, password: password) } + + before do + sign_in user + end + + context 'for GET requests' do + before { get :foo } + + it 'renders challenge' do + expect(response).to render_template('auth/challenges/new') + end + + # See Auth::ChallengesControllerSpec + end + + context 'for POST requests' do + before { post :bar } + + it 'renders challenge' do + expect(response).to render_template('auth/challenges/new') + end + + it 'accepts correct password' do + post :bar, params: { form_challenge: { current_password: password } } + expect(response.body).to eq 'bar' + expect(session[:challenge_passed_at]).to_not be_nil + end + + it 'rejects wrong password' do + post :bar, params: { form_challenge: { current_password: 'dddfff888123' } } + expect(response.body).to render_template('auth/challenges/new') + expect(session[:challenge_passed_at]).to be_nil + end + end + end +end diff --git a/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb b/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb index 2e5a9325c..336f13127 100644 --- a/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication/confirmations_controller_spec.rb @@ -24,7 +24,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do context 'when signed in' do subject do sign_in user, scope: :user - get :new + get :new, session: { challenge_passed_at: Time.now.utc } end include_examples 'renders :new' @@ -37,7 +37,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do it 'redirects if user do not have otp_secret' do sign_in user_without_otp_secret, scope: :user - get :new + get :new, session: { challenge_passed_at: Time.now.utc } expect(response).to redirect_to('/settings/two_factor_authentication') end end @@ -50,7 +50,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do describe 'when form_two_factor_confirmation parameter is not provided' do it 'raises ActionController::ParameterMissing' do - post :create, params: {} + post :create, params: {}, session: { challenge_passed_at: Time.now.utc } expect(response).to have_http_status(400) end end @@ -68,7 +68,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do true end - post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } + post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }, session: { challenge_passed_at: Time.now.utc } expect(assigns(:recovery_codes)).to eq otp_backup_codes expect(flash[:notice]).to eq 'Two-factor authentication successfully enabled' @@ -85,7 +85,7 @@ describe Settings::TwoFactorAuthentication::ConfirmationsController do false end - post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } } + post :create, params: { form_two_factor_confirmation: { otp_attempt: '123456' } }, session: { challenge_passed_at: Time.now.utc } end it 'renders the new view' do diff --git a/spec/controllers/settings/two_factor_authentication/recovery_codes_controller_spec.rb b/spec/controllers/settings/two_factor_authentication/recovery_codes_controller_spec.rb index c04760e53..630cec428 100644 --- a/spec/controllers/settings/two_factor_authentication/recovery_codes_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentication/recovery_codes_controller_spec.rb @@ -15,7 +15,7 @@ describe Settings::TwoFactorAuthentication::RecoveryCodesController do end sign_in user, scope: :user - post :create + post :create, session: { challenge_passed_at: Time.now.utc } expect(assigns(:recovery_codes)).to eq otp_backup_codes expect(flash[:notice]).to eq 'Recovery codes successfully regenerated' diff --git a/spec/controllers/settings/two_factor_authentications_controller_spec.rb b/spec/controllers/settings/two_factor_authentications_controller_spec.rb index 922231ded..9df9763fd 100644 --- a/spec/controllers/settings/two_factor_authentications_controller_spec.rb +++ b/spec/controllers/settings/two_factor_authentications_controller_spec.rb @@ -58,7 +58,7 @@ describe Settings::TwoFactorAuthenticationsController do describe 'when creation succeeds' do it 'updates user secret' do before = user.otp_secret - post :create + post :create, session: { challenge_passed_at: Time.now.utc } expect(user.reload.otp_secret).not_to eq(before) expect(response).to redirect_to(new_settings_two_factor_authentication_confirmation_path) diff --git a/spec/mailers/previews/user_mailer_preview.rb b/spec/mailers/previews/user_mailer_preview.rb index ead3b3baa..464f177d0 100644 --- a/spec/mailers/previews/user_mailer_preview.rb +++ b/spec/mailers/previews/user_mailer_preview.rb @@ -18,6 +18,21 @@ class UserMailerPreview < ActionMailer::Preview UserMailer.password_change(User.first) end + # Preview this email at http://localhost:3000/rails/mailers/user_mailer/two_factor_disabled + def two_factor_disabled + UserMailer.two_factor_disabled(User.first) + end + + # Preview this email at http://localhost:3000/rails/mailers/user_mailer/two_factor_enabled + def two_factor_enabled + UserMailer.two_factor_enabled(User.first) + end + + # Preview this email at http://localhost:3000/rails/mailers/user_mailer/two_factor_recovery_codes_changed + def two_factor_recovery_codes_changed + UserMailer.two_factor_recovery_codes_changed(User.first) + end + # Preview this email at http://localhost:3000/rails/mailers/user_mailer/reconfirmation_instructions def reconfirmation_instructions user = User.first -- cgit