invalidate all sessions on explicit logout

when a user explicitly logs out of one pseudonym session, invalidate all
the others

fixes CNVS-1923

test-plan:
  - create a user in two different accounts
  - log them in to both accounts
  - click "log out" in one account
  - should be logged out of both accounts

Change-Id: I79e70017d753c8201429901421e015f5d20e2000
Reviewed-on: https://gerrit.instructure.com/20096
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
QA-Review: Clare Strong <clare@instructure.com>
Product-Review: Brian Palmer <brianp@instructure.com>

Gemfile

@@ -27,7 +27,7 @@ end
 gem 'daemons',        '1.1.0'
 gem 'diff-lcs',       '1.1.3',  :require => 'diff/lcs'
 if !CANVAS_RAILS3
-  gem 'encrypted_cookie_store-instructure', '1.0.2', :require => 'encrypted_cookie_store'
+  gem 'encrypted_cookie_store-instructure', '1.0.4', :require => 'encrypted_cookie_store'
 end
 gem 'erubis',         CANVAS_RAILS3 ? '2.6.6' : '2.7.0'
 if !CANVAS_RAILS3

app/controllers/application_controller.rb

@@ -1312,6 +1312,16 @@ class ApplicationController < ActionController::Base
     super
   end
 
+  def destroy_session
+    @pseudonym_session.destroy rescue true
+    reset_session
+  end
+
+  def logout_current_user
+    @current_user.try(:stamp_logout_time!)
+    destroy_session
+  end
+
   def set_layout_options
     @embedded_view = params[:embedded]
     @headers = false if params[:no_headers]

app/controllers/pseudonym_sessions_controller.rb

@@ -192,7 +192,6 @@ class PseudonymSessionsController < ApplicationController
   def destroy
     # the saml message has to survive a couple redirects and reset_session calls
     message = session[:delegated_message]
-    @pseudonym_session.destroy rescue true
 
     if @domain_root_account.saml_authentication? and session[:saml_unique_id]
       # logout at the saml identity provider
@@ -209,21 +208,21 @@ class PseudonymSessionsController < ApplicationController
           aac.debug_set(:debugging, t('debug.logout_redirect', "LogoutRequest sent to IdP"))
         end
 
-        reset_session
+        logout_current_user
         session[:delegated_message] = message if message
         redirect_to(forward_url)
         return
       else
-        reset_session
+        logout_current_user
         flash[:message] = t('errors.logout_errors.no_idp_found', "Canvas was unable to log you out at your identity provider")
       end
     elsif @domain_root_account.cas_authentication? and session[:cas_login]
-      reset_session
+      logout_current_user
       session[:delegated_message] = message if message
       redirect_to(cas_client.logout_url(cas_login_url))
       return
     else
-      reset_session
+      logout_current_user
       flash[:delegated_message] = message if message
     end
 
@@ -260,8 +259,7 @@ class PseudonymSessionsController < ApplicationController
         aac = @domain_root_account.account_authorization_configs.find_by_idp_entity_id(response.issuer)
         if aac.nil?
           logger.error "Attempted SAML login for #{response.issuer} on account without that IdP"
-          @pseudonym_session.destroy rescue true
-          reset_session
+          destroy_session
           if @domain_root_account.auth_discovery_url
             message = t('errors.login_errors.unrecognized_idp', "Canvas did not recognize your identity provider")
             redirect_to @domain_root_account.auth_discovery_url + "?message=#{URI.escape message}"
@@ -364,21 +362,18 @@ class PseudonymSessionsController < ApplicationController
           aac.debug_set(:login_response_validation_error, response.validation_error)
         end
         logger.error "Failed to verify SAML signature."
-        @pseudonym_session.destroy rescue true
-        reset_session
+        destroy_session
         flash[:delegated_message] = login_error_message
         redirect_to login_url(:no_auto=>'true')
       end
     elsif !params[:SAMLResponse]
       logger.error "saml_consume request with no SAMLResponse parameter"
-      @pseudonym_session.destroy rescue true
-      reset_session
+      destroy_session
       flash[:delegated_message] = login_error_message
       redirect_to login_url(:no_auto=>'true')
     else
       logger.error "Attempted SAML login on non-SAML enabled account."
-      @pseudonym_session.destroy rescue true
-      reset_session
+      destroy_session
       flash[:delegated_message] = login_error_message
       redirect_to login_url(:no_auto=>'true')
     end

app/controllers/users_controller.rb

@@ -847,19 +847,6 @@ class UsersController < ApplicationController
     end
   end
 
-  def registered
-    @pseudonym_session.destroy if @pseudonym_session
-    @pseudonym = Pseudonym.find_by_id(flash[:pseudonym_id]) if flash[:pseudonym_id].present?
-    if flash[:user_id] && (@user = User.find(flash[:user_id]))
-      @email_address = @pseudonym && @pseudonym.communication_channel && @pseudonym.communication_channel.path
-      @email_address ||= @user.email
-      @pseudonym ||= @user.pseudonym
-      @cc = @pseudonym.communication_channel || @user.communication_channel
-    else
-      redirect_to root_url
-    end
-  end
-
   # @API Edit a user
   # Modify an existing user. To modify a user's login, see the documentation for logins.
   #
@@ -1096,8 +1083,7 @@ class UsersController < ApplicationController
     if authorized_action(@user, @current_user, [:manage, :manage_logins])
       @user.destroy(@user.grants_right?(@current_user, session, :manage_logins))
       if @user == @current_user
-        @pseudonym_session.destroy rescue true
-        reset_session
+        logout_current_user
       end
 
       respond_to do |format|

app/models/user.rb

@@ -2417,4 +2417,12 @@ class User < ActiveRecord::Base
   def prefers_gradebook2?
     preferences[:use_gradebook2] != false
   end
+
+  def stamp_logout_time!
+    if Rails.version < '3.0'
+      User.update_all({ :last_logged_out => Time.zone.now }, :id => self)
+    else
+      User.where(:id => self).update_all(:last_logged_out => Time.zone.now)
+    end
+  end
 end

config/routes.rb

@@ -512,7 +512,6 @@ ActionController::Routing::Routes.draw do |map|
   map.clear_file_session "file_session/clear", :controller => "pseudonym_sessions", :action => "clear_file_session"
   map.register "register", :controller => "users", :action => "new"
   map.register_from_website "register_from_website", :controller => "users", :action => "new"
-  map.registered "registered", :controller => "users", :action => "registered"
   map.enroll 'enroll/:self_enrollment_code', :controller => 'self_enrollments', :action => 'new', :conditions => {:method => :get}
   map.enroll_frd 'enroll/:self_enrollment_code', :controller => 'self_enrollments', :action => 'create', :conditions => {:method => :post}
   map.services 'services', :controller => 'users', :action => 'services'

db/migrate/20130425230856_add_last_logged_out_to_users.rb

@@ -0,0 +1,11 @@
+class AddLastLoggedOutToUsers < ActiveRecord::Migration
+  tag :predeploy
+
+  def self.up
+    add_column :users, :last_logged_out, :timestamp
+  end
+
+  def self.down
+    remove_column :users, :last_logged_out
+  end
+end

lib/authentication_methods.rb

@@ -84,9 +84,19 @@ module AuthenticationMethods
     if !@current_pseudonym
       if @policy_pseudonym_id
         @current_pseudonym = Pseudonym.find_by_id(@policy_pseudonym_id)
-      else
-        @pseudonym_session = PseudonymSession.find
-        @current_pseudonym = @pseudonym_session && @pseudonym_session.record
+      elsif @pseudonym_session = PseudonymSession.find
+        @current_pseudonym = @pseudonym_session.record
+
+        # if the session was created before the last time the user explicitly
+        # logged out (of any session for any of their pseudonyms), invalidate
+        # this session
+        if (invalid_before = @current_pseudonym.user.last_logged_out) &&
+          (session_refreshed_at = request.env['encrypted_cookie_store.session_refreshed_at']) &&
+          session_refreshed_at < invalid_before
+
+          destroy_session
+          @current_pseudonym = nil
+        end
       end
       if params[:login_success] == '1' && !@current_pseudonym
         # they just logged in successfully, but we can't find the pseudonym now?

spec/lib/authentication_methods_spec.rb

@@ -87,6 +87,38 @@ describe AuthenticationMethods do
       end
     end
   end
+
+  describe "#load_user" do
+    before do
+      @request = stub(:env => {'encrypted_cookie_store.session_refreshed_at' => 5.minutes.ago})
+      @controller = Spec::MockController.new(nil, @request)
+      @controller.stubs(:load_pseudonym_from_access_token)
+      @controller.stubs(:api_request?).returns(false)
+    end
+
+    context "with active session" do
+      before do
+        user_with_pseudonym
+        @pseudonym_session = stub(:record => @pseudonym)
+        PseudonymSession.stubs(:find).returns(@pseudonym_session)
+      end
+
+      it "should set the user and pseudonym" do
+        @controller.send(:load_user).should == @user
+        @controller.instance_variable_get(:@current_user).should == @user
+        @controller.instance_variable_get(:@current_pseudonym).should == @pseudonym
+      end
+
+      it "should destroy session if user was explicitly logged out" do
+        @user.stamp_logout_time!
+        @pseudonym.reload
+        @controller.expects(:destroy_session).once
+        @controller.send(:load_user).should be_nil
+        @controller.instance_variable_get(:@current_user).should be_nil
+        @controller.instance_variable_get(:@current_pseudonym).should be_nil
+      end
+    end
+  end
 end
 
 class Spec::MockController

spec/models/user_spec.rb

@@ -2269,4 +2269,25 @@ describe User do
       end
     end
   end
+
+  describe "#stamp_logout_time!" do
+    before do
+      user_model
+    end
+
+    it "should update last_logged_out" do
+      now = Time.zone.now
+      Timecop.freeze(now) { @user.stamp_logout_time! }
+      @user.reload.last_logged_out.to_i.should == now.to_i
+    end
+
+    context "sharding" do
+      specs_require_sharding
+
+      it "should update regardless of current shard" do
+        @shard1.activate{ @user.stamp_logout_time! }
+        @user.reload.last_logged_out.should_not be_nil
+      end
+    end
+  end
 end