From 38cdf2e2dd09e68f740370d4b5c37b60276ee2be Mon Sep 17 00:00:00 2001 From: Michael Frister Date: Thu, 29 Aug 2013 17:31:05 +0100 Subject: [PATCH] Add option to log user for all requests Also remove before filter that worked around some session cookie bug in Rails. This workaround wasn't effective anyway, since our session cookie names don't contain chiliproject any more. --- app/controllers/application_controller.rb | 34 +++++--- app/views/settings/_authentication.html.erb | 1 + config/locales/de.yml | 1 + config/locales/en.yml | 1 + config/settings.yml | 3 + doc/CHANGELOG.md | 2 + .../application_controller_spec.rb | 79 +++++++++++++++++++ 7 files changed, 108 insertions(+), 13 deletions(-) create mode 100644 spec/controllers/application_controller_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dd1589690fc..5b7160a84e2 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -46,18 +46,6 @@ class ApplicationController < ActionController::Base cookies.delete(:autologin) end - # Remove broken cookie after upgrade from 0.8.x (#4292) - # See https://rails.lighthouseapp.com/projects/8994/tickets/3360 - # TODO: remove it when Rails is fixed - before_filter :delete_broken_cookies - def delete_broken_cookies - if cookies['_chiliproject_session'] && cookies['_chiliproject_session'] !~ /--/ - cookies.delete '_chiliproject_session' - redirect_to home_path - return false - end - end - # FIXME: Remove this when all of Rack and Rails have learned how to # properly use encodings before_filter :params_filter @@ -76,7 +64,12 @@ class ApplicationController < ActionController::Base end end - before_filter :user_setup, :check_if_login_required, :reset_i18n_fallbacks, :set_localization, :check_session_lifetime + before_filter :user_setup, + :check_if_login_required, + :log_requesting_user, + :reset_i18n_fallbacks, + :set_localization, + :check_session_lifetime rescue_from ActionController::InvalidAuthenticityToken, :with => :invalid_authenticity_token @@ -154,6 +147,21 @@ class ApplicationController < ActionController::Base require_login if Setting.login_required? end + def log_requesting_user + return unless Setting.log_requesting_user? + login_and_mail = " (#{escape_for_logging(User.current.login)} ID: #{User.current.id} " + + "<#{escape_for_logging(User.current.mail)}>)" unless User.current.anonymous? + logger.info "OpenProject User: #{escape_for_logging(User.current.name)}#{login_and_mail}" + end + + # Escape string to prevent log injection + # e.g. setting the user name to contain \r allows overwriting a log line on console + # replaces all invalid characters with # + def escape_for_logging(string) + # only allow numbers, ASCII letters, space and the following characters: @.-"'!?=/ + string.gsub(/[^0-9a-zA-Z@._\-"\'!\?=\/ ]{1}/, '#') + end + def reset_i18n_fallbacks return if I18n.fallbacks.defaults == (fallbacks = [I18n.default_locale] + Setting.available_languages.map(&:to_sym)) I18n.fallbacks = nil diff --git a/app/views/settings/_authentication.html.erb b/app/views/settings/_authentication.html.erb index 8882ef6e3e2..ffab31169df 100644 --- a/app/views/settings/_authentication.html.erb +++ b/app/views/settings/_authentication.html.erb @@ -62,6 +62,7 @@ See doc/COPYRIGHT.rdoc for more details. <%= I18n.t(:other, :scope => [:settings]) %>

<%= setting_check_box :openid, :disabled => !Object.const_defined?(:OpenID) %>

+

<%= setting_check_box :log_requesting_user %>

<%= setting_check_box :rest_api_enabled %>

diff --git a/config/locales/de.yml b/config/locales/de.yml index 1c95957a83d..47db99d8f49 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -1158,6 +1158,7 @@ de: setting_work_package_properties: "Arbeitspaket-Eigenschaften" setting_work_package_startdate_is_adddate: "Neue Arbeitspakete haben \"Heute\" als Anfangsdatum" setting_work_package_export_limit: "Max. Anzahl Arbeitspakete bei CSV/PDF-Export" + setting_log_requesting_user: "Logge Benutzer Login, Name und Mailadresse für alle Anfragen" setting_login_required: "Authentifizierung erforderlich" setting_mail_from: "E-Mail-Absender" setting_mail_handler_api_enabled: "Abruf eingehender E-Mails aktivieren" diff --git a/config/locales/en.yml b/config/locales/en.yml index ba8afb8b087..981977892eb 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1137,6 +1137,7 @@ en: setting_work_package_properties: "Work package properties" setting_work_package_startdate_is_adddate: "Use current date as start date for new work packages" setting_work_packages_export_limit: "Work packages export limit" + setting_log_requesting_user: "Log user login, name, and mail address for all requests" setting_login_required: "Authentication required" setting_mail_from: "Emission email address" setting_mail_handler_api_enabled: "Enable WS for incoming emails" diff --git a/config/settings.yml b/config/settings.yml index ea17e19aac5..77f1463995e 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -13,6 +13,9 @@ brute_force_block_after_failed_logins: format: int welcome_text: default: +log_requesting_user: + default: 0 + format: int login_required: default: 0 self_registration: diff --git a/doc/CHANGELOG.md b/doc/CHANGELOG.md index 61b969bb477..c5bc51317e7 100644 --- a/doc/CHANGELOG.md +++ b/doc/CHANGELOG.md @@ -1,5 +1,7 @@ # Changelog +* `#1808` Add option to log user for each request + ## 3.0.0pre14 * `#825` Migrate Duration diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 00000000000..7af3ac90a96 --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,79 @@ +#-- copyright +# OpenProject is a project management system. +# +# Copyright (C) 2012-2013 the OpenProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe ApplicationController do + let(:user) { FactoryGirl.create(:user, :lastname => "Crazy! Name with \r\n Newline") } + + # Fake controller to test calling an action + controller do + def index + # just do anything that doesn't require an extra template + render_404 + end + end + + describe 'with log_requesting_user enabled' do + before do + Setting.stub(:log_requesting_user?).and_return(true) + end + + it 'should log the current user' do + messages = [] + Rails.logger.should_receive(:info).at_least(:once) do |message| + messages << message + end + + as_logged_in_user(user) do + get(:index) + end + + filtered_messages = messages.select { |message| message.start_with? 'OpenProject User' } + filtered_messages.length.should == 1 + filtered_messages[0].should == "OpenProject User: #{user.firstname} Crazy! Name with \#\# " + + "Newline (#{user.login} ID: #{user.id} <#{user.mail}>)" + end + + it 'should log an anonymous user' do + messages = [] + Rails.logger.should_receive(:info).at_least(:once) do |message| + messages << message + end + + # no login, so this is done as Anonymous + get(:index) + + filtered_messages = messages.select { |message| message.start_with? 'OpenProject User' } + filtered_messages.length.should == 1 + filtered_messages[0].should == "OpenProject User: Anonymous" + end + end + describe 'with log_requesting_user disabled' do + before do + Setting.stub(:log_requesting_user?).and_return(false) + end + + it 'should not log the current user' do + messages = [] + Rails.logger.stub(:info) do |message| + messages << message + end + + as_logged_in_user(user) do + get(:index) + end + + filtered_messages = messages.select { |message| message.start_with? 'OpenProject User' } + filtered_messages.length.should == 0 + end + end +end