Merge pull request #244 from opf/feature/fix-session-lifetime

[#1314] Log out users with empty session activity time when session lifetime is ...

app/controllers/application_controller.rb

@@ -141,6 +141,7 @@ 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
@@ -616,27 +617,32 @@ 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)
-        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 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
 
+  def session_expired?
+    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
     @permitted_params ||= PermittedParams.new(params, current_user)
   end

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

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"

features/step_definitions/general_steps.rb

@@ -366,13 +366,21 @@
   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}')]")
 
   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

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