Merge pull request #41 from RadiusNetworks/eric/redis-session

Modifies cookie cache value check to also check with redis-based value

Author: lankyfrenchman

app/controllers/kracken/sessions_controller.rb

@@ -7,8 +7,10 @@ class SessionsController < ActionController::Base
     def create
       @user = user_class.find_or_create_from_auth_hash(auth_hash)
       session[:user_id] = @user.id
-      session[:user_cache_key] = cookies[:_radius_user_cache_key]
+      session[:user_uid] = @user.uid
+      session[:user_cache_key] = Kracken::SessionManager.get(@user.uid)
       session[:token_expires_at] = Time.zone.at(auth_hash[:credentials][:expires_at])
+
       redirect_to return_to_path
     end
 

lib/kracken.rb

@@ -12,6 +12,7 @@
 require "kracken/authenticator"
 require "kracken/registration"
 require "kracken/railtie" if defined?(Rails)
+require "kracken/session_manager"
 
 module Kracken
   mattr_accessor :config

lib/kracken/controllers/authenticatable.rb

@@ -6,7 +6,7 @@ module Authenticatable
 
       def self.included(base)
         base.instance_exec do
-          before_action :handle_user_cache_cookie!
+          before_action :handle_user_cache_key!
           before_action :authenticate_user!
           helper_method :sign_out_path, :sign_up_path, :sign_in_path,
                         :current_user, :user_signed_in?
@@ -49,7 +49,7 @@ def authenticate_user!
 
       def check_token_expiry!
         if session[:token_expires_at].nil? || session[:token_expires_at] < Time.zone.now
-          session.delete :user_id
+          delete_session_data
         end
       end
 
@@ -64,31 +64,18 @@ def check_token_expiry!
       #
       # This method will:
       #
-      #  - Check for the `_radius_user_cache_key` tld cookie
-      #  - If the key is "none" log them out
+      #  - Check for the presence of a user cache key in Redis
       #  - Compare it to the `user_cache_key` in the session
       #  - If they don't match, redirect them to the oauth provider and
-      #    delete the cookie
+      #    delete the session
       #
-      def handle_user_cache_cookie!
-        if cookies[:_radius_user_cache_key]
-          if cookies[:_radius_user_cache_key] == "none"
-            # Sign out current user
-            session.delete :user_id
-
-            # Clear that user's cache key
-            session.delete :user_cache_key
-
-          elsif session[:user_cache_key] != cookies[:_radius_user_cache_key]
-            # Delete the cookie to prevent redirect loops
-            cookies.delete :_radius_user_cache_key
-
-            # Redirect to the account app
-            redirect_to_sign_in
-          end
-        end
-      end
+      def handle_user_cache_key!
+        return unless session_present?
+        return if session_and_redis_match?
 
+        delete_session_data
+        redirect_to_sign_in
+      end
 
       def current_user=(u)
         @current_user = u
@@ -124,6 +111,23 @@ def user_signed_in?
 
       private
 
+      def session_present?
+        session[:user_uid] && session[:user_cache_key]
+      end
+
+      def session_and_redis_match?
+        Kracken::SessionManager.get(session[:user_uid]) == session[:user_cache_key]
+      end
+
+      def delete_session_data
+        # Sign out current user
+        session.delete :user_id
+
+        # Clear that user's cache data
+        session.delete :user_uid
+        session.delete :user_cache_key
+      end
+
       def user_class
         Kracken.config.user_class
       end

lib/kracken/session_manager.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Kracken
+  module SessionManager
+    def self.conn
+      @conn ||= if ENV["REDIS_SESSION_URL"].present?
+                  Redis.new(url: ENV["REDIS_SESSION_URL"])
+                else
+                  NullRedis.new
+                end
+    end
+
+    def self.get(user_id)
+      conn.get(user_session_key(user_id))
+    end
+
+    def self.del(user_id)
+      conn.del(user_session_key(user_id))
+    end
+
+    def self.update(user_id, value)
+      conn.set(user_session_key(user_id), value)
+    end
+
+    def self.user_session_key(user_id)
+      "rnsession:#{user_id}"
+    end
+
+    class NullRedis
+      # rubocop:disable Style/EmptyMethod
+      def initialize(*); end
+
+      def del(*); end
+
+      def get(*); end
+
+      def set(*); end
+      # rubocop:enable Style/EmptyMethod
+    end
+  end
+end

spec/kracken/controllers/authenticatable_spec.rb

@@ -136,47 +136,43 @@ class ControllerDouble < BaseControllerDouble
           expect(controller).to_not have_received(:redirect_to)
         end
 
-        context "user cache cookie" do
-          it "nothing if the cache cookie does not exist" do
+        context "user cache key" do
+          it "does nothing if the session does not exist" do
             allow(controller).to receive(:request).and_return(double(format: nil, fullpath: nil))
             allow(controller).to receive(:redirect_to)
-            controller.session[:user_cache_key] = "123"
 
-            controller.handle_user_cache_cookie!
+            controller.handle_user_cache_key!
 
             expect(controller).to_not have_received(:redirect_to)
           end
 
-          it "signs the current user out when the cache cookie is 'none'" do
-            allow(controller).to receive(:request).and_return(double(format: nil, fullpath: nil))
-            allow(controller).to receive(:redirect_to)
-            controller.cookies[:_radius_user_cache_key] = "123"
+          it "ends session and redirects if stored key does not match session key" do
             controller.session[:user_cache_key] = "123"
+            controller.session[:user_uid] = "123"
 
-            controller.handle_user_cache_cookie!
-
-            expect(controller).to_not have_received(:redirect_to)
-          end
-
-          it "redirects when the cache cookie is different than the session" do
             allow(controller).to receive(:request).and_return(double(format: nil, fullpath: nil))
-            allow(controller).to receive(:cookies).and_return({_radius_user_cache_key: "123"})
             allow(controller).to receive(:redirect_to)
-            controller.handle_user_cache_cookie!
+            allow(Kracken::SessionManager).to receive(:get).and_return("456")
+
+            expect(controller).to receive(:redirect_to).with("/")
+            expect(controller.session).to receive(:delete).with(:user_id)
+            expect(controller.session).to receive(:delete).with(:user_uid)
+            expect(controller.session).to receive(:delete).with(:user_cache_key)
 
-            expect(controller).to have_received(:redirect_to).with("/")
+            controller.handle_user_cache_key!
           end
 
-          it "does not redirect when the cache cookie matches the session" do
-            controller.session = spy
+          it "does nothing if session keys match" do
+            controller.session[:user_cache_key] = "123"
+            controller.session[:user_uid] = "123"
+
+            allow(controller).to receive(:request).and_return(double(format: nil, fullpath: nil))
             allow(controller).to receive(:redirect_to)
-            controller.cookies[:_radius_user_cache_key] = "none"
+            allow(Kracken::SessionManager).to receive(:get).and_return("123")
 
-            controller.handle_user_cache_cookie!
+            expect(controller).to_not receive(:redirect_to).with("/")
 
-            expect(controller).to_not have_received(:redirect_to)
-            expect(controller.session).to have_received(:delete).with(:user_id)
-            expect(controller.session).to have_received(:delete).with(:user_cache_key)
+            controller.handle_user_cache_key!
           end
         end
       end