Bring Ticket Stores under test

Write tests for Ticket Store classes
Fixes #30
Update a lot of our gems

Author: soupmatt

.simplecov

@@ -1,6 +1,7 @@
 SimpleCov.start do
   add_filter "/spec/.*_spec\.rb"
   add_filter "/spec/.*/shared_examples.*"
+  add_filter "/spec/.*/.*helper(s?).rb"
 end
 
 # vim: filetype=ruby

Gemfile

@@ -1,15 +1,18 @@
 source "http://rubygems.org"
 
 group :development do
-  gem "json", "~> 1.6.1"
-  gem "rspec", "~> 2.7.0"
+  gem "json"
+  gem "rspec"
   gem "bundler", ">= 1.0"
-  gem "jeweler", "~> 1.6.2"
+  gem "jeweler"
   gem "actionpack"
+  gem "activerecord"
   gem "rake"
   gem "simplecov", :require => false
   gem "guard"
   gem "guard-rspec"
+  gem "sqlite3", :require => false
+  gem "database_cleaner"
 end
 
 gem "activesupport", :require => "active_support"

Gemfile.lock

@@ -4,48 +4,55 @@ GEM
     actionpack (2.3.11)
       activesupport (= 2.3.11)
       rack (~> 1.1.0)
+    activerecord (2.3.11)
+      activesupport (= 2.3.11)
     activesupport (2.3.11)
+    database_cleaner (0.7.1)
     diff-lcs (1.1.3)
     ffi (1.0.11)
     git (1.2.5)
     guard (1.0.0)
       ffi (>= 0.5.0)
       thor (~> 0.14.6)
-    guard-rspec (0.5.11)
-      guard (>= 0.8.4)
+    guard-rspec (0.6.0)
+      guard (>= 0.10.0)
     jeweler (1.6.4)
       bundler (~> 1.0)
       git (>= 1.2.5)
       rake
-    json (1.6.1)
+    json (1.6.5)
     multi_json (1.0.4)
     rack (1.1.2)
     rake (0.9.2.2)
-    rspec (2.7.0)
-      rspec-core (~> 2.7.0)
-      rspec-expectations (~> 2.7.0)
-      rspec-mocks (~> 2.7.0)
-    rspec-core (2.7.1)
-    rspec-expectations (2.7.0)
+    rspec (2.8.0)
+      rspec-core (~> 2.8.0)
+      rspec-expectations (~> 2.8.0)
+      rspec-mocks (~> 2.8.0)
+    rspec-core (2.8.0)
+    rspec-expectations (2.8.0)
       diff-lcs (~> 1.1.2)
-    rspec-mocks (2.7.0)
+    rspec-mocks (2.8.0)
     simplecov (0.5.4)
       multi_json (~> 1.0.3)
       simplecov-html (~> 0.5.3)
     simplecov-html (0.5.3)
+    sqlite3 (1.3.5)
     thor (0.14.6)
 
 PLATFORMS
   ruby
 
 DEPENDENCIES
   actionpack
+  activerecord
   activesupport
   bundler (>= 1.0)
+  database_cleaner
   guard
   guard-rspec
-  jeweler (~> 1.6.2)
-  json (~> 1.6.1)
+  jeweler
+  json
   rake
-  rspec (~> 2.7.0)
+  rspec
   simplecov
+  sqlite3

Guardfile

@@ -5,5 +5,7 @@ guard 'rspec', :version => 2, :cli => '-c -f doc' do
   watch(%r{^spec/.+_spec\.rb$})
   watch(%r{^lib/(.+)\.rb$})     { |m| "spec/lib/#{m[1]}_spec.rb" }
   watch('spec/spec_helper.rb')  { "spec" }
+  watch(%r{^spec/support/(.+)\.rb$})     { |m| "spec/lib/#{m[1]}_spec.rb" }
+  watch(%r{^spec/support/(.+)\.rb$})     { "spec" }
 end
 

lib/casclient.rb

@@ -56,7 +56,7 @@ def set_real_logger(real_logger)
     # Log using the appropriate method if we have a logger
     # if we dont' have a logger, gracefully ignore.
     def method_missing(name, *args)
-      if @real_logger && @real_logger.respond_to?(name)
+      if !@real_logger.nil? && @real_logger.respond_to?(name)
         @real_logger.send(name, *args)
       end
     end

lib/casclient/tickets/storage.rb

@@ -4,31 +4,34 @@ module Storage
       class AbstractTicketStore
 
         attr_accessor :log
-        @log = CASClient::LoggerWrapper.new
+        def log
+          @log ||= CASClient::LoggerWrapper.new
+        end
 
-        def process_single_sign_out(si)
+        def process_single_sign_out(st)
 
-          session_id, session = get_session_for_service_ticket(si)
+          session_id, session = get_session_for_service_ticket(st)
           if session
             session.destroy
-            log.debug("Destroyed #{session.inspect} for session #{session_id.inspect} corresponding to service ticket #{si.inspect}.")
+            log.debug("Destroyed #{session.inspect} for session #{session_id.inspect} corresponding to service ticket #{st.inspect}.")
           else
             log.debug("Data for session #{session_id.inspect} was not found. It may have already been cleared by a local CAS logout request.")
           end
 
           if session_id
             log.info("Single-sign-out for service ticket #{session_id.inspect} completed successfuly.")
           else
-            log.debug("No session id found for CAS ticket #{si}")
+            log.debug("No session id found for CAS ticket #{st}")
           end
         end
 
         def get_session_for_service_ticket(st)
-          session_id = read_service_session_lookup(si)
-          if session_id
-            session = ActiveRecord::SessionStore::Session.find_by_session_id(session_id)
+          session_id = read_service_session_lookup(st)
+          unless session_id.nil?
+            # This feels a bit hackish, but there isn't really a better way to go about it that I am aware of yet
+            session = ActiveRecord::SessionStore.session_class.find_by_session_id(session_id)
           else
-            log.warn("Couldn't destroy session with SessionIndex #{si} because no corresponding session id could be looked up.")
+            log.warn("Couldn't destroy session service ticket #{st} because no corresponding session id could be found.")
           end
           [session_id, session]
         end
@@ -53,6 +56,12 @@ def retrieve_pgt(pgt_iou)
         def read_service_session_lookup(st)
           raise 'Implement this in a subclass!'
         end
+
+        def session_id_from_controller(controller)
+          session_id = controller.request.session_options[:id] || controller.session.session_id
+          raise CASClient::CASException, "Failed to extract session_id from controller" if session_id.nil?
+          session_id
+        end
       end
 
       # A Ticket Store that keeps it's tickets in a directory on the local filesystem.
@@ -83,10 +92,10 @@ def initialize(config={})
         # Rails session id.
         # Returns the filename of the lookup file created.
         def store_service_session_lookup(st, controller)
-          raise CASException, "No service_ticket specified." unless st
-          raise CASException, "No controller specified." unless controller
+          raise CASException, "No service_ticket specified." if st.nil?
+          raise CASException, "No controller specified." if controller.nil?
 
-          sid = controller.request.session_options[:id] || controller.session.session_id
+          sid = session_id_from_controller(controller)
 
           st = st.ticket if st.kind_of? ServiceTicket
           f = File.new(filename_of_service_session_lookup(st), 'w')
@@ -100,11 +109,11 @@ def store_service_session_lookup(st, controller)
         # cas_sess.<session ticket> file created in a prior call to 
         # #store_service_session_lookup.
         def read_service_session_lookup(st)
-          raise CASException, "No service_ticket specified." unless st
+          raise CASException, "No service_ticket specified." if st.nil?
 
           st = st.ticket if st.kind_of? ServiceTicket
           ssl_filename = filename_of_service_session_lookup(st)
-          return File.exists?(ssl_filename) && IO.read(ssl_filename)
+          return IO.read(ssl_filename) if File.exists?(ssl_filename)
         end
 
         # Removes a stored relationship between a ServiceTicket and a local
@@ -113,14 +122,17 @@ def read_service_session_lookup(st)
         #
         # See #store_service_session_lookup.
         def cleanup_service_session_lookup(st)
-          raise CASException, "No service_ticket specified." unless st
+          raise CASException, "No service_ticket specified." if st.nil?
 
           st = st.ticket if st.kind_of? ServiceTicket
           ssl_filename = filename_of_service_session_lookup(st)
           File.delete(ssl_filename) if File.exists?(ssl_filename)
         end
 
         def save_pgt_iou(pgt_iou, pgt)
+          raise CASException, "Invalid pgt_iou" if pgt_iou.nil?
+          raise CASException, "Invalid pgt" if pgt.nil?
+
           # TODO: pstore contents should probably be encrypted...
           pstore = open_pstore
 
@@ -135,17 +147,14 @@ def retrieve_pgt(pgt_iou)
           pstore = open_pstore
 
           pgt = nil
+          # TODO: need to periodically clean the storage, otherwise it will just keep growing
           pstore.transaction do
             pgt = pstore[pgt_iou]
+            pstore.delete pgt_iou
           end
 
           raise CASException, "Invalid pgt_iou specified. Perhaps this pgt has already been retrieved?" unless pgt
 
-          # TODO: need to periodically clean the storage, otherwise it will just keep growing
-          pstore.transaction do
-            pstore.delete pgt_iou
-          end
-
           pgt
         end
 

lib/casclient/tickets/storage/active_record_ticket_store.rb

@@ -19,35 +19,40 @@ def initialize(config={})
         end
 
         def store_service_session_lookup(st, controller)
-          #get the session from the rack env using ActiveRecord::SessionStore::SESSION_RECORD_KEY = 'rack.session.record'
+          raise CASException, "No service_ticket specified." unless st
+          raise CASException, "No controller specified." unless controller
 
           st = st.ticket if st.kind_of? ServiceTicket
           session = controller.session
           session[:service_ticket] = st
         end
 
-        def get_session_for_service_ticket(st)
+        def read_service_session_lookup(st)
+          raise CASException, "No service_ticket specified." unless st
           st = st.ticket if st.kind_of? ServiceTicket
           session = ActiveRecord::SessionStore::Session.find_by_service_ticket(st)
-          session_id = session ? session.session_id : nil
-          [session_id, session]
+          session ? session.session_id : nil
         end
 
         def cleanup_service_session_lookup(st)
           #no cleanup needed for this ticket store
+          #we still raise the exception for API compliance
+          raise CASException, "No service_ticket specified." unless st
         end
 
         def save_pgt_iou(pgt_iou, pgt)
+          raise CASClient::CASException.new("Invalid pgt_iou") if pgt_iou.nil?
+          raise CASClient::CASException.new("Invalid pgt") if pgt.nil?
           pgtiou = CasPgtiou.create(:pgt_iou => pgt_iou, :pgt_id => pgt)
         end
 
         def retrieve_pgt(pgt_iou)
           raise CASException, "No pgt_iou specified. Cannot retrieve the pgt." unless pgt_iou
 
           pgtiou = CasPgtiou.find_by_pgt_iou(pgt_iou)
-          pgt = pgtiou.pgt_id
 
-          raise CASException, "Invalid pgt_iou specified. Perhaps this pgt has already been retrieved?" unless pgt
+          raise CASException, "Invalid pgt_iou specified. Perhaps this pgt has already been retrieved?" unless pgtiou
+          pgt = pgtiou.pgt_id
 
           pgtiou.destroy
 

spec/.gitignore

@@ -0,0 +1 @@
+test.sqlite3