diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb
index d6c69a3..195bae3 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 1fae635..113eb9b 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 9fdc143..38f0157 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 01fd75b..c63974e 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 ac4cc3a..a803a35 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 7e5da46..c210df3 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 0000000..53bf0a0
--- /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 0000000..17fd6ef
--- /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 515ee6a..808a616 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 5ae9405..a997378 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 f308c93..d623081 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 7c52d9c..3d2d6de 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 92ee247..4f3f2e2 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 8de5bfe..9ee1794 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'