From 3956f024d0cd86dcd6b9e145f2e0a09b525d4bbf Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Thu, 11 Jul 2013 11:30:02 +0200 Subject: [PATCH 1/7] Log out users with empty session activity time when session lifetime is enabled. --- app/controllers/application_controller.rb | 10 ++++++---- spec/controllers/users_controller_spec.rb | 21 +++++++++++++++++---- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d12807aa3a8c..779b915d7e61 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -616,10 +616,8 @@ def disable_api ActiveSupport.run_load_hooks(:application_controller, self) def check_session_lifetime - session_ttl_value = Setting.session_ttl.to_i - - if Setting.session_ttl_enabled? && session_ttl_value >= 5 - if session[:updated_at] && User.current.logged? && ((session[:updated_at] + (session_ttl_value * 60)) < Time.now) + if Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 + if session[:updated_at].nil? || session_expired? self.logged_user = nil if request.get? url = url_for(params) @@ -637,6 +635,10 @@ def check_session_lifetime private + def session_expired? + session[:updated_at] && User.current.logged? && ((session[:updated_at] + (Setting.session_ttl.to_i * 60)) < Time.now) + end + def permitted_params @permitted_params ||= PermittedParams.new(params, current_user) end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 57a95490a538..664cf0f2f9ef 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -241,6 +241,14 @@ end end + shared_examples_for 'index action with enabled session lifetime and inactivity exceeded' do + it "logs out the user and redirects with a warning that he has been locked out" do + response.redirect_url.should == (signin_url + "?back_url=" + CGI::escape(@controller.url_for(:controller => "users", :action => "index"))) + User.current.should_not == admin + flash[:warning].should == I18n.t(:notice_forced_logout, :ttl_time => Setting.session_ttl) + end + end + context "disabled" do before do Setting.stub!(:session_ttl_enabled?).and_return(false) @@ -272,11 +280,16 @@ session[:updated_at] = Time.now - 3.hours get :index end - it "logs out the user and redirects with a warning that he has been locked out" do - response.redirect_url.should == (signin_url + "?back_url=" + CGI::escape(@controller.url_for(:controller => "users", :action => "index"))) - User.current.should_not == admin - flash[:warning].should == I18n.t(:notice_forced_logout, :ttl_time => Setting.session_ttl) + it_should_behave_like 'index action with enabled session lifetime and inactivity exceeded' + end + + context "without last activity time in the session" do + before do + Setting.stub!(:session_ttl).and_return("60") + session[:updated_at] = nil + get :index end + it_should_behave_like 'index action with enabled session lifetime and inactivity exceeded' end context "with ttl = 0" do From 1ba73c4ef9c08c439eb8ea900338673e0de4c411 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Thu, 11 Jul 2013 17:23:51 +0200 Subject: [PATCH 2/7] Always set session[:updated_at] to avoid logging out users when the administrator turns on session expiration. --- app/controllers/application_controller.rb | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 779b915d7e61..f3c233a11605 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -616,21 +616,18 @@ def disable_api ActiveSupport.run_load_hooks(:application_controller, self) def check_session_lifetime - if Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 - if session[:updated_at].nil? || session_expired? - self.logged_user = nil - if request.get? - url = url_for(params) - else - url = url_for(:controller => params[:controller], :action => params[:action], :id => params[:id], :project_id => params[:project_id]) - end - - flash[:warning] = I18n.t('notice_forced_logout', :ttl_time => Setting.session_ttl) - redirect_to(:controller => "account", :action => "login", :back_url => url) + if Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 && (session[:updated_at].nil? || session_expired?) + self.logged_user = nil + if request.get? + url = url_for(params) else - session[:updated_at] = Time.now + url = url_for(:controller => params[:controller], :action => params[:action], :id => params[:id], :project_id => params[:project_id]) end + + flash[:warning] = I18n.t('notice_forced_logout', :ttl_time => Setting.session_ttl) + redirect_to(:controller => "account", :action => "login", :back_url => url) end + session[:updated_at] = Time.now end private From d42c9eb1a99e58c5d46a30aff11a9648d0483af8 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 16 Jul 2013 11:50:53 +0200 Subject: [PATCH 3/7] Refactor session_expired? conditions. --- app/controllers/application_controller.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f3c233a11605..b4240925ea15 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -616,14 +616,14 @@ def disable_api ActiveSupport.run_load_hooks(:application_controller, self) def check_session_lifetime - if Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 && (session[:updated_at].nil? || session_expired?) + if session_expired? self.logged_user = nil if request.get? url = url_for(params) else - url = url_for(:controller => params[:controller], :action => params[:action], :id => params[:id], :project_id => params[:project_id]) + url = url_for(:controller => params[:controller], :action => params[:action], + :id => params[:id], :project_id => params[:project_id]) end - flash[:warning] = I18n.t('notice_forced_logout', :ttl_time => Setting.session_ttl) redirect_to(:controller => "account", :action => "login", :back_url => url) end @@ -633,7 +633,13 @@ def check_session_lifetime private def session_expired? - session[:updated_at] && User.current.logged? && ((session[:updated_at] + (Setting.session_ttl.to_i * 60)) < Time.now) + current_user.logged? && + (session_ttl_enabled? && (session[:updated_at].nil? || + (session[:updated_at] + Setting.session_ttl.to_i.minutes) < Time.now)) + end + + def session_ttl_enabled? + Setting.session_ttl_enabled? && Setting.session_ttl.to_i >= 5 end def permitted_params From 44184492e1702b506d2f5066119b209c8049db87 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 16 Jul 2013 13:28:14 +0200 Subject: [PATCH 4/7] Add changelog entry. --- doc/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/CHANGELOG.md b/doc/CHANGELOG.md index 6f8011515f3c..ab11ceda9ac4 100644 --- a/doc/CHANGELOG.md +++ b/doc/CHANGELOG.md @@ -1,5 +1,6 @@ # Changelog +* `#1314` Always set last activity timestamp and check session expiry if ttl-setting is enabled * `#1414` Remove start & due date requirement from planning elements ## 3.0.0pre8 From c9d1e660a40f88303cf2f07d7f8e6fcd101b6eb0 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 16 Jul 2013 17:36:24 +0200 Subject: [PATCH 5/7] Set updated_at immediately after login. --- app/controllers/application_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b4240925ea15..e5bb92a01927 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -144,6 +144,7 @@ def logged_user=(user) else User.current = User.anonymous end + session[:updated_at] = Time.now end # check if login is globally required to access the application From 705b76f0ce8f08ef931c0835b7ddb570ab615253 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Tue, 16 Jul 2013 17:39:31 +0200 Subject: [PATCH 6/7] Add cuke for login/logout and session lifetime. --- features/session/user_session.feature | 49 ++++++++++++++++++++++ features/step_definitions/general_steps.rb | 12 ++++++ 2 files changed, 61 insertions(+) create mode 100644 features/session/user_session.feature diff --git a/features/session/user_session.feature b/features/session/user_session.feature new file mode 100644 index 000000000000..c02d661803c7 --- /dev/null +++ b/features/session/user_session.feature @@ -0,0 +1,49 @@ +#-- 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. +#++ + +Feature: User session + Background: + Given there is 1 project with the following: + | name | Awesome Project | + | identifier | awesome-project | + And project "Awesome Project" uses the following modules: + | calendar | + And there is a role "member" + And the role "member" may have the following rights: + | view_calendar | + And there is 1 user with the following: + | login | bob | + And the user "bob" is a "member" in the project "Awesome Project" + + Scenario: A user can log in + When I login as "bob" + Then I should be logged in as "bob" + + Scenario: A user can log out + Given I am logged in as "bob" + When I logout + Then I should be logged out + + Scenario: A user is logged out when their session is expired + Given the "session_ttl_enabled" setting is set to true + And the "session_ttl" setting is set to 5 + When I login as "bob" + And I wait for "10" minutes + And I go to the home page + Then I should be logged out + + Scenario: A user is logged in as long as their session is valid + Given the "session_ttl_enabled" setting is set to true + And the "session_ttl" setting is set to 5 + When I login as "bob" + And I wait for "4" minutes + And I go to the home page + Then I should be logged in as "bob" diff --git a/features/step_definitions/general_steps.rb b/features/step_definitions/general_steps.rb index d0b456569f2f..2ad019dee5d3 100644 --- a/features/step_definitions/general_steps.rb +++ b/features/step_definitions/general_steps.rb @@ -366,6 +366,10 @@ login(username, password) end +When "I logout" do + visit "/logout" +end + Then /^I should be logged in as "([^\"]*)"?$/ do |username| user = User.find_by_login(username) || User.anonymous page.should have_xpath("//div[contains(., 'Logged in as #{username}')] | //a[contains(.,'#{user.name}')]") @@ -373,6 +377,10 @@ User.current = user end +Then "I should be logged out" do + page.should have_css("a.login") +end + When /^I satisfy the "(.+)" plugin to (.+)$/ do |plugin_name, action| if plugin_loaded?(plugin_name) action_name = action.gsub("\"", "") @@ -409,6 +417,10 @@ end end +When(/^I wait for "(.*?)" minutes$/) do |number_of_minutes| + page.set_rack_session(updated_at: Time.now - number_of_minutes.to_i.minutes) +end + def get_project(project_name = nil) if project_name.blank? project = @project From 8acc54811b2dfba9c3110631a62a6d157deaa5b7 Mon Sep 17 00:00:00 2001 From: Christian Rijke Date: Fri, 19 Jul 2013 16:19:56 +0200 Subject: [PATCH 7/7] Only set updated_at for logged in users and not for anonymous users. --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e5bb92a01927..0d1cf36145bc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -141,10 +141,10 @@ def logged_user=(user) if user && user.is_a?(User) User.current = user session[:user_id] = user.id + session[:updated_at] = Time.now else User.current = User.anonymous end - session[:updated_at] = Time.now end # check if login is globally required to access the application