From: Jean-Philippe Lang Date: Fri, 5 Feb 2016 07:33:24 +0000 (+0000) Subject: Security notifications when password or email adress is changed (#21421). X-Git-Tag: 3.3.0~254 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=5d70fce6ce4c481f058fc1b89d567c1389cb7e54;p=redmine.git Security notifications when password or email adress is changed (#21421). Patch by Jan Schulz-Hofen. git-svn-id: http://svn.redmine.org/redmine/trunk@15145 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index d6c69a3a5..195bae30e 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -73,6 +73,12 @@ class AccountController < ApplicationController @user.password, @user.password_confirmation = params[:new_password], params[:new_password_confirmation] if @user.save @token.destroy + Mailer.security_notification(@user, + message: :mail_body_security_notification_change, + field: :field_password, + title: :button_change_password, + url: {controller: 'my', action: 'password'} + ).deliver flash[:notice] = l(:notice_account_password_updated) redirect_to signin_path return diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1fae635fc..113eb9b9c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -133,6 +133,8 @@ class ApplicationController < ActionController::Base end end end + # store current ip address in user object ephemerally + user.remote_ip = request.remote_ip if user user end diff --git a/app/controllers/my_controller.rb b/app/controllers/my_controller.rb index 9fdc14314..38f015747 100644 --- a/app/controllers/my_controller.rb +++ b/app/controllers/my_controller.rb @@ -105,6 +105,12 @@ class MyController < ApplicationController if @user.save # The session token was destroyed by the password change, generate a new one session[:tk] = @user.generate_session_token + Mailer.security_notification(@user, + message: :mail_body_security_notification_change, + field: :field_password, + title: :button_change_password, + url: {controller: 'my', action: 'password'} + ).deliver flash[:notice] = l(:notice_account_password_updated) redirect_to my_account_path end diff --git a/app/models/email_address.rb b/app/models/email_address.rb index 01fd75b1a..c63974e2a 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -19,8 +19,9 @@ class EmailAddress < ActiveRecord::Base belongs_to :user attr_protected :id - after_update :destroy_tokens - after_destroy :destroy_tokens + after_create :deliver_security_notification_create + after_update :destroy_tokens, :deliver_security_notification_update + after_destroy :destroy_tokens, :deliver_security_notification_destroy validates_presence_of :address validates_format_of :address, :with => /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i, :allow_blank => true @@ -42,6 +43,58 @@ class EmailAddress < ActiveRecord::Base private + # send a security notification to user that a new email address was added + def deliver_security_notification_create + # only deliver if this isn't the only address. + # in that case, the user is just being created and + # should not receive this email. + if user.mails != [address] + deliver_security_notification(user, + message: :mail_body_security_notification_add, + field: :field_mail, + value: address + ) + end + end + + # send a security notification to user that an email has been changed (notified/not notified) + def deliver_security_notification_update + if address_changed? + recipients = [user, address_was] + options = { + message: :mail_body_security_notification_change_to, + field: :field_mail, + value: address + } + elsif notify_changed? + recipients = [user, address] + options = { + message: notify_was ? :mail_body_security_notification_notify_disabled : :mail_body_security_notification_notify_enabled, + value: address + } + end + deliver_security_notification(recipients, options) + end + + # send a security notification to user that an email address was deleted + def deliver_security_notification_destroy + deliver_security_notification([user, address], + message: :mail_body_security_notification_remove, + field: :field_mail, + value: address + ) + end + + # generic method to send security notifications for email addresses + def deliver_security_notification(recipients, options={}) + Mailer.security_notification(recipients, + options.merge( + title: :label_my_account, + url: {controller: 'my', action: 'account'} + ) + ).deliver + end + # Delete all outstanding password reset tokens on email change. # This helps to keep the account secure in case the associated email account # was compromised. diff --git a/app/models/mailer.rb b/app/models/mailer.rb index ac4cc3a04..a803a35c2 100644 --- a/app/models/mailer.rb +++ b/app/models/mailer.rb @@ -318,6 +318,20 @@ class Mailer < ActionMailer::Base :subject => l(:mail_subject_register, Setting.app_title) end + def security_notification(recipients, options={}) + redmine_headers 'Sender' => User.current.login + @user = Array(recipients).detect{|r| r.is_a? User } + set_language_if_valid(@user.try :language) + @message = l(options[:message], + field: (options[:field] && l(options[:field])), + value: options[:value] + ) + @title = options[:title] && l(options[:title]) + @url = options[:url] && (options[:url].is_a?(Hash) ? url_for(options[:url]) : options[:url]) + mail :to => recipients, + :subject => l(:mail_subject_security_notification) + end + def test_email(user) set_language_if_valid(user.language) @url = url_for(:controller => 'welcome') diff --git a/app/models/user.rb b/app/models/user.rb index 7e5da46c8..c210df324 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -97,6 +97,8 @@ class User < Principal attr_accessor :password, :password_confirmation, :generate_password attr_accessor :last_before_login_on + attr_accessor :remote_ip + # Prevents unauthorized assignments attr_protected :login, :admin, :password, :password_confirmation, :hashed_password diff --git a/app/views/mailer/security_notification.html.erb b/app/views/mailer/security_notification.html.erb new file mode 100644 index 000000000..53bf0a0d5 --- /dev/null +++ b/app/views/mailer/security_notification.html.erb @@ -0,0 +1,13 @@ +

<%= @message %>
+<% if @url && @title -%> +<%= link_to @title, @url -%> +<% elsif @url -%> +<%= link_to @url -%> +<% elsif @title -%> +<%= content_tag :h1, @title -%> +<% end %>

+ +

<%= l(:field_user) %>: <%= User.current.login %>
+<%= l(:field_remote_ip) %>: <%= User.current.remote_ip %>
+<%= l(:label_date) %>: <%= format_time Time.now, true, @user %>

+ diff --git a/app/views/mailer/security_notification.text.erb b/app/views/mailer/security_notification.text.erb new file mode 100644 index 000000000..17fd6ef67 --- /dev/null +++ b/app/views/mailer/security_notification.text.erb @@ -0,0 +1,8 @@ +<%= @message %> + +<%= @url || @title %> + +<%= l(:field_user) %>: <%= User.current.login %> +<%= l(:field_remote_ip) %>: <%= User.current.remote_ip %> +<%= l(:label_date) %>: <%= format_time Time.now, true, @user %> + diff --git a/config/locales/de.yml b/config/locales/de.yml index 515ee6ae1..808a61681 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -848,6 +848,13 @@ de: mail_subject_reminder: "%{count} Tickets müssen in den nächsten %{days} Tagen abgegeben werden" mail_subject_wiki_content_added: "Wiki-Seite '%{id}' hinzugefügt" mail_subject_wiki_content_updated: "Wiki-Seite '%{id}' erfolgreich aktualisiert" + mail_subject_security_notification: "Sicherheitshinweis" + mail_body_security_notification_change: "%{field} wurde geändert." + mail_body_security_notification_change_to: "%{field} wurde geändert zu %{value}." + mail_body_security_notification_add: "%{field} %{value} wurde hinzugefügt." + mail_body_security_notification_remove: "%{field} %{value} wurde entfernt." + mail_body_security_notification_notify_enabled: "E-Mail-Adresse %{value} erhält nun Benachrichtigungen." + mail_body_security_notification_notify_disabled: "E-Mail-Adresse %{value} erhält keine Benachrichtigungen mehr." notice_account_activated: Ihr Konto ist aktiviert. Sie können sich jetzt anmelden. notice_account_deleted: Ihr Benutzerkonto wurde unwiderruflich gelöscht. @@ -1148,6 +1155,7 @@ de: error_password_expired: Your password has expired or the administrator requires you to change it. field_time_entries_visibility: Time logs visibility + field_remote_ip: IP-Adresse label_parent_task_attributes: Parent tasks attributes label_parent_task_attributes_derived: Calculated from subtasks label_parent_task_attributes_independent: Independent of subtasks diff --git a/config/locales/en.yml b/config/locales/en.yml index 5ae94054b..a9973786a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -228,6 +228,13 @@ en: mail_body_wiki_content_added: "The '%{id}' wiki page has been added by %{author}." mail_subject_wiki_content_updated: "'%{id}' wiki page has been updated" mail_body_wiki_content_updated: "The '%{id}' wiki page has been updated by %{author}." + mail_subject_security_notification: "Security notification" + mail_body_security_notification_change: "%{field} was changed." + mail_body_security_notification_change_to: "%{field} was changed to %{value}." + mail_body_security_notification_add: "%{field} %{value} was added." + mail_body_security_notification_remove: "%{field} %{value} was removed." + mail_body_security_notification_notify_enabled: "Email address %{value} now receives notifications." + mail_body_security_notification_notify_disabled: "Email address %{value} no longer receives notifications." field_name: Name field_description: Description @@ -352,6 +359,7 @@ en: field_time_entries_visibility: Time logs visibility field_total_estimated_hours: Total estimated time field_default_version: Default version + field_remote_ip: IP address setting_app_title: Application title setting_app_subtitle: Application subtitle diff --git a/test/functional/account_controller_test.rb b/test/functional/account_controller_test.rb index f308c935f..d623081a3 100644 --- a/test/functional/account_controller_test.rb +++ b/test/functional/account_controller_test.rb @@ -400,6 +400,7 @@ class AccountControllerTest < ActionController::TestCase end def test_post_lost_password_with_token_should_change_the_user_password + ActionMailer::Base.deliveries.clear user = User.find(2) token = Token.create!(:action => 'recovery', :user => user) @@ -408,6 +409,10 @@ class AccountControllerTest < ActionController::TestCase user.reload assert user.check_password?('newpass123') assert_nil Token.find_by_id(token.id), "Token was not deleted" + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_select_email do + assert_select 'a[href^=?]', 'http://localhost:3000/my/password', :text => 'Change password' + end end def test_post_lost_password_with_token_for_non_active_user_should_fail diff --git a/test/functional/email_addresses_controller_test.rb b/test/functional/email_addresses_controller_test.rb index 7c52d9c1d..3d2d6dea2 100644 --- a/test/functional/email_addresses_controller_test.rb +++ b/test/functional/email_addresses_controller_test.rb @@ -92,6 +92,22 @@ class EmailAddressesControllerTest < ActionController::TestCase end end + def test_create_should_send_security_notification + @request.session[:user_id] = 2 + ActionMailer::Base.deliveries.clear + post :create, :user_id => 2, :email_address => {:address => 'something@example.fr'} + + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_mail_body_match '0.0.0.0', mail + assert_mail_body_match I18n.t(:mail_body_security_notification_add, field: I18n.t(:field_mail), value: 'something@example.fr'), mail + assert_select_email do + assert_select 'a[href^=?]', 'http://localhost:3000/my/account', :text => 'My account' + end + # The old email address should be notified about a new address for security purposes + assert [mail.bcc, mail.cc].flatten.include?(User.find(2).mail) + assert [mail.bcc, mail.cc].flatten.include?('something@example.fr') + end + def test_update @request.session[:user_id] = 2 email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') @@ -112,6 +128,21 @@ class EmailAddressesControllerTest < ActionController::TestCase assert_equal false, email.reload.notify end + def test_update_should_send_security_notification + @request.session[:user_id] = 2 + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + + ActionMailer::Base.deliveries.clear + xhr :put, :update, :user_id => 2, :id => email.id, :notify => '0' + + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_mail_body_match I18n.t(:mail_body_security_notification_notify_disabled, value: 'another@somenet.foo'), mail + + # The changed address should be notified for security purposes + assert [mail.bcc, mail.cc].flatten.include?('another@somenet.foo') + end + + def test_destroy @request.session[:user_id] = 2 email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') @@ -141,4 +172,18 @@ class EmailAddressesControllerTest < ActionController::TestCase assert_response 404 end end + + def test_destroy_should_send_security_notification + @request.session[:user_id] = 2 + email = EmailAddress.create!(:user_id => 2, :address => 'another@somenet.foo') + + ActionMailer::Base.deliveries.clear + xhr :delete, :destroy, :user_id => 2, :id => email.id + + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_mail_body_match I18n.t(:mail_body_security_notification_remove, field: I18n.t(:field_mail), value: 'another@somenet.foo'), mail + + # The removed address should be notified for security purposes + assert [mail.bcc, mail.cc].flatten.include?('another@somenet.foo') + end end diff --git a/test/functional/my_controller_test.rb b/test/functional/my_controller_test.rb index 92ee24781..4f3f2e247 100644 --- a/test/functional/my_controller_test.rb +++ b/test/functional/my_controller_test.rb @@ -117,6 +117,24 @@ class MyControllerTest < ActionController::TestCase assert user.groups.empty? end + def test_update_account_should_send_security_notification + ActionMailer::Base.deliveries.clear + post :account, + :user => { + :mail => 'foobar@example.com' + } + + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_mail_body_match '0.0.0.0', mail + assert_mail_body_match I18n.t(:mail_body_security_notification_change_to, field: I18n.t(:field_mail), value: 'foobar@example.com'), mail + assert_select_email do + assert_select 'a[href^=?]', 'http://localhost:3000/my/account', :text => 'My account' + end + # The old email address should be notified about the change for security purposes + assert [mail.bcc, mail.cc].flatten.include?(User.find(2).mail) + assert [mail.bcc, mail.cc].flatten.include?('foobar@example.com') + end + def test_my_account_should_show_destroy_link get :account assert_select 'a[href="/my/account/destroy"]' @@ -193,6 +211,19 @@ class MyControllerTest < ActionController::TestCase assert_redirected_to '/my/account' end + def test_change_password_should_send_security_notification + ActionMailer::Base.deliveries.clear + post :password, :password => 'jsmith', + :new_password => 'secret123', + :new_password_confirmation => 'secret123' + + assert_not_nil (mail = ActionMailer::Base.deliveries.last) + assert_mail_body_no_match 'secret123', mail # just to be sure: pw should never be sent! + assert_select_email do + assert_select 'a[href^=?]', 'http://localhost:3000/my/password', :text => 'Change password' + end + end + def test_page_layout get :page_layout assert_response :success diff --git a/test/unit/mailer_test.rb b/test/unit/mailer_test.rb index 8de5bfe56..9ee179400 100644 --- a/test/unit/mailer_test.rb +++ b/test/unit/mailer_test.rb @@ -666,6 +666,51 @@ class MailerTest < ActiveSupport::TestCase end end + def test_security_notification + set_language_if_valid User.find(1).language + with_settings :emails_footer => "footer without link" do + User.current.remote_ip = '192.168.1.1' + assert Mailer.security_notification(User.find(1), message: :notice_account_password_updated).deliver + mail = last_email + assert_not_nil mail + assert_mail_body_match '192.168.1.1', mail + assert_mail_body_match I18n.t(:notice_account_password_updated), mail + assert_select_email do + assert_select "h1", false + assert_select "a", false + end + end + end + + def test_security_notification_should_include_title + set_language_if_valid User.find(2).language + with_settings :emails_footer => "footer without link" do + assert Mailer.security_notification(User.find(2), + message: :notice_account_password_updated, + title: :label_my_account + ).deliver + assert_select_email do + assert_select "a", false + assert_select "h1", :text => I18n.t(:label_my_account) + end + end + end + + def test_security_notification_should_include_link + set_language_if_valid User.find(3).language + with_settings :emails_footer => "footer without link" do + assert Mailer.security_notification(User.find(3), + message: :notice_account_password_updated, + title: :label_my_account, + url: {controller: 'my', action: 'account'} + ).deliver + assert_select_email do + assert_select "h1", false + assert_select 'a[href=?]', 'http://mydomain.foo/my/account', :text => I18n.t(:label_my_account) + end + end + end + def test_mailer_should_not_change_locale # Set current language to italian set_language_if_valid 'it'