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 +++ 21 files changed, 364 insertions(+), 42 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 (limited to 'app') 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 %> -- cgit