From e582de5721238ef42099ac3e1fa49bfed2ff8e1e Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 03:51:17 -0500 Subject: [PATCH 01/24] Implemented Appraisal tests for Rails 4.2, 5.0, and 5.1 --- .gitignore | 50 ++++++++++++++++++-------------------- .travis.yml | 10 ++++++++ Appraisals | 14 +++++++++++ Gemfile | 17 +++++++++++++ README.md | 11 +++++++++ casino.gemspec | 11 +-------- gemfiles/rails_4.2.gemfile | 23 ++++++++++++++++++ gemfiles/rails_5.0.gemfile | 23 ++++++++++++++++++ gemfiles/rails_5.1.gemfile | 23 ++++++++++++++++++ 9 files changed, 145 insertions(+), 37 deletions(-) create mode 100644 Appraisals create mode 100644 gemfiles/rails_4.2.gemfile create mode 100644 gemfiles/rails_5.0.gemfile create mode 100644 gemfiles/rails_5.1.gemfile diff --git a/.gitignore b/.gitignore index 90ad19cb..4fcd38c4 100644 --- a/.gitignore +++ b/.gitignore @@ -1,28 +1,24 @@ -# See http://help.github.com/ignore-files/ for more about ignoring files. -# -# If you find yourself ignoring temporary files generated by your text editor -# or operating system, you probably want to add a global ignore instead: -# git config --global core.excludesfile ~/.gitignore_global - -# Ignore bundler config -/.bundle - -# Ignore the default SQLite database. -/db/*.sqlite3 - -# Ignore all logfiles and tempfiles. -/log/*.log -/tmp +*.gem +*.rbc +.bundle +.config +.yardoc .rails_generators~ - -/coverage - -/pkg - -# http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/ -/Gemfile.lock - -# Dummy application crap -/spec/dummy/log/*.log -/spec/dummy/tmp -/spec/dummy/db/*.sqlite3 +gemfiles/vendor +Gemfile.lock +InstalledFiles +_yardoc +coverage +doc/ +lib/bundler/man +pkg +rdoc +spec/reports +test/tmp +test/version_tmp +tmp +*.lock +.idea/ +.ruby-version +*.sqlite* +*.log diff --git a/.travis.yml b/.travis.yml index 7b4db7e8..84130e9a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,6 +4,16 @@ rvm: - 2.2.2 - 2.4.0 - 2.4.1 +bundler_args: --without development +gemfile: + - gemfiles/rails_4.2.gemfile + - gemfiles/rails_5.0.gemfile + - gemfiles/rails_5.1.gemfile +matrix: + allow_failures: + - rvm: 2.4.1 + # gemfile: gemfiles/rails_5.0.gemfile + # gemfile: gemfiles/rails_5.1.gemfile notifications: hipchat: rooms: diff --git a/Appraisals b/Appraisals new file mode 100644 index 00000000..857098b5 --- /dev/null +++ b/Appraisals @@ -0,0 +1,14 @@ +appraise 'rails-4.2' do + gem 'activerecord', '~> 4.2.0' + gem 'rspec-rails', '>= 3.0' +end + +appraise 'rails-5.0' do + gem 'activerecord', '~> 5.0.0' + gem 'rspec-rails', '>= 3.0' +end + +appraise 'rails-5.1' do + gem 'activerecord', '~> 5.1.0' + gem 'rspec-rails', '>= 3.0' +end diff --git a/Gemfile b/Gemfile index 851fabc2..178ba7a2 100644 --- a/Gemfile +++ b/Gemfile @@ -1,2 +1,19 @@ source 'https://rubygems.org' + +group :test do + gem 'appraisal', '>= 2.1' + gem 'capybara', '>= 2.1' + gem 'coveralls', '>= 0.7' + gem 'factory_bot', '>= 4.1' + gem 'rake', '>= 10.0' + gem 'rspec-its', '>= 1.0' + gem 'rspec-rails', '>= 3.0' + gem 'webmock', '>= 1.9' +end + +# Specify your gem's dependencies in groupify.gemspec gemspec + +platforms :ruby do + gem 'sqlite3', '>= 1.3' +end diff --git a/README.md b/README.md index e6d35746..13e0fb1e 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,17 @@ It currently supports [CAS 1.0 and CAS 2.0](http://apereo.github.io/cas) as well Please check our [documentation](http://casino.rbcas.com/) for setup and configuration instructions. +## Test Suite + +Run the RSpec test suite by installing the `appraisal` gem and dependencies: + + $ gem install appraisal + $ appraisal install + +And then running tests using `appraisal`: + + $ appraisal rake + ## License CASino is released under the [MIT License](http://www.opensource.org/licenses/MIT). See LICENSE.txt for further details. diff --git a/casino.gemspec b/casino.gemspec index 7a1f3ebe..a008d6dd 100644 --- a/casino.gemspec +++ b/casino.gemspec @@ -23,22 +23,13 @@ Gem::Specification.new do |s| s.cert_chain = ['casino-public_cert.pem'] end - s.add_development_dependency 'capybara', '>= 2.1' - s.add_development_dependency 'coveralls', '>= 0.7' - s.add_development_dependency 'factory_bot', '>= 4.1' - s.add_development_dependency 'rake', '>= 10.0' - s.add_development_dependency 'rspec', '>= 3.0' - s.add_development_dependency 'rspec-its', '>= 1.0' - s.add_development_dependency 'rspec-rails', '>= 3.0' - s.add_development_dependency 'sqlite3', '>= 1.3' - s.add_development_dependency 'webmock', '>= 1.9' s.add_runtime_dependency 'addressable', '>= 2.3' s.add_runtime_dependency 'faraday', '>= 0.8' s.add_runtime_dependency 'grape', '>= 0.8' s.add_runtime_dependency 'grape-entity', '>= 0.4' s.add_runtime_dependency 'kaminari', '~> 0.16' - s.add_runtime_dependency 'rails', '~> 4.2' + s.add_runtime_dependency 'rails', '>= 4.2' s.add_runtime_dependency 'rotp', '>= 2.0' s.add_runtime_dependency 'rqrcode_png', '>= 0.1' s.add_runtime_dependency 'sass-rails', '>= 4.0.0', '< 6.0.0' diff --git a/gemfiles/rails_4.2.gemfile b/gemfiles/rails_4.2.gemfile new file mode 100644 index 00000000..31d104df --- /dev/null +++ b/gemfiles/rails_4.2.gemfile @@ -0,0 +1,23 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "activerecord", "~> 4.2.0" +gem "rspec-rails", ">= 3.0" + +group :test do + gem "appraisal", ">= 2.1" + gem "capybara", ">= 2.1" + gem "coveralls", ">= 0.7" + gem "factory_bot", ">= 4.1" + gem "rake", ">= 10.0" + gem "rspec-its", ">= 1.0" + gem "rspec-rails", ">= 3.0" + gem "webmock", ">= 1.9" +end + +platforms :ruby do + gem "sqlite3", ">= 1.3" +end + +gemspec path: "../" diff --git a/gemfiles/rails_5.0.gemfile b/gemfiles/rails_5.0.gemfile new file mode 100644 index 00000000..b094b629 --- /dev/null +++ b/gemfiles/rails_5.0.gemfile @@ -0,0 +1,23 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "activerecord", "~> 5.0.0" +gem "rspec-rails", ">= 3.0" + +group :test do + gem "appraisal", ">= 2.1" + gem "capybara", ">= 2.1" + gem "coveralls", ">= 0.7" + gem "factory_bot", ">= 4.1" + gem "rake", ">= 10.0" + gem "rspec-its", ">= 1.0" + gem "rspec-rails", ">= 3.0" + gem "webmock", ">= 1.9" +end + +platforms :ruby do + gem "sqlite3", ">= 1.3" +end + +gemspec path: "../" diff --git a/gemfiles/rails_5.1.gemfile b/gemfiles/rails_5.1.gemfile new file mode 100644 index 00000000..a24e6fb4 --- /dev/null +++ b/gemfiles/rails_5.1.gemfile @@ -0,0 +1,23 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "activerecord", "~> 5.1.0" +gem "rspec-rails", ">= 3.0" + +group :test do + gem "appraisal", ">= 2.1" + gem "capybara", ">= 2.1" + gem "coveralls", ">= 0.7" + gem "factory_bot", ">= 4.1" + gem "rake", ">= 10.0" + gem "rspec-its", ">= 1.0" + gem "rspec-rails", ">= 3.0" + gem "webmock", ">= 1.9" +end + +platforms :ruby do + gem "sqlite3", ">= 1.3" +end + +gemspec path: "../" From f5b77327394621654764abfabb3bdc23087b2454 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 03:51:40 -0500 Subject: [PATCH 02/24] Don't explicitly set this option that is not available in Rails 5.x --- spec/dummy/config/application.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/dummy/config/application.rb b/spec/dummy/config/application.rb index 8d23c5ea..0c65f4b3 100644 --- a/spec/dummy/config/application.rb +++ b/spec/dummy/config/application.rb @@ -21,6 +21,6 @@ class Application < Rails::Application # config.i18n.default_locale = :de # Do not swallow errors in after_commit/after_rollback callbacks. - config.active_record.raise_in_transactional_callbacks = true + # config.active_record.raise_in_transactional_callbacks = true end end From 32cd2ef6732aa1da49e3c2467711c001aa871863 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Fri, 25 Sep 2015 11:49:52 -0400 Subject: [PATCH 03/24] Reference Rails Engine path (when mounted inside one) --- lib/generators/casino/install/install_generator.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/generators/casino/install/install_generator.rb b/lib/generators/casino/install/install_generator.rb index c2628277..f30c63ef 100644 --- a/lib/generators/casino/install/install_generator.rb +++ b/lib/generators/casino/install/install_generator.rb @@ -27,11 +27,11 @@ def copy_config_files return unless options['config_files'] copy_file 'cas.yml', 'config/cas.yml' - copy_file 'casino_and_overrides.scss', 'app/assets/stylesheets/casino_and_overrides.scss' + copy_file 'casino_and_overrides.scss', build_file_path('app/assets/stylesheets', '/casino_and_overrides.scss') end def insert_assets_loader - insert_into_file 'app/assets/javascripts/application.js', :after => %r{//= require +['"]?jquery_ujs['"]?} do + insert_into_file build_file_path('app/assets/javascripts', '/application.js'), :after => %r{//= require +['"]?jquery_ujs['"]?} do "\n//= require casino" end end @@ -43,5 +43,14 @@ def insert_engine_routes def show_readme readme 'README' end + + private + def build_file_path(root, path) + engine_name = Rails::Generators.namespace.to_s.underscore + engine_path = "/#{engine_name}" unless engine_name.blank? + + [root, engine_path, path].compact.join + end + end end From b78aa7436a733ebea9ac55795b864df8a53f0015 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Fri, 25 Sep 2015 11:50:28 -0400 Subject: [PATCH 04/24] Copy migrations with a generator because `rake casino:install:migrations` doesn't work from Rails Engine --- .../casino/install/install_generator.rb | 2 +- lib/generators/casino/migration_generator.rb | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 lib/generators/casino/migration_generator.rb diff --git a/lib/generators/casino/install/install_generator.rb b/lib/generators/casino/install/install_generator.rb index f30c63ef..d823486a 100644 --- a/lib/generators/casino/install/install_generator.rb +++ b/lib/generators/casino/install/install_generator.rb @@ -20,7 +20,7 @@ class InstallGenerator < Rails::Generators::Base def install_migrations return unless options['migration'] - rake 'casino:install:migrations' + generate 'casino:migration' end def copy_config_files diff --git a/lib/generators/casino/migration_generator.rb b/lib/generators/casino/migration_generator.rb new file mode 100644 index 00000000..46885edd --- /dev/null +++ b/lib/generators/casino/migration_generator.rb @@ -0,0 +1,25 @@ +require 'rails/generators/active_record' + +module CASino + class MigrationGenerator < ::Rails::Generators::Base + include Rails::Generators::Migration + source_root File.expand_path('../../../../db/migrate', __FILE__) + + namespace 'casino:migration' + + desc 'Installs CASino migration files.' + + def install + source_paths.each do |source_path| + Dir["#{source_path}/*.rb"].each do |filename| + puts "MIGRATION TEMPLATE: #{File.basename(filename)}" + migration_template File.basename(filename), "db/migrate/#{File.basename(filename).sub(/^\d+_/, '')}" + end + end + end + + def self.next_migration_number(dirname) + ActiveRecord::Generators::Base.next_migration_number(dirname) + end + end +end \ No newline at end of file From 65d2192e487d3f486edb5facb9389fd594db34a5 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Sat, 26 Sep 2015 13:51:09 -0400 Subject: [PATCH 05/24] Simplified adding namespace to paths --- lib/generators/casino/install/install_generator.rb | 11 ++++------- lib/generators/casino/migration_generator.rb | 1 - 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/generators/casino/install/install_generator.rb b/lib/generators/casino/install/install_generator.rb index d823486a..16c10ea5 100644 --- a/lib/generators/casino/install/install_generator.rb +++ b/lib/generators/casino/install/install_generator.rb @@ -27,11 +27,11 @@ def copy_config_files return unless options['config_files'] copy_file 'cas.yml', 'config/cas.yml' - copy_file 'casino_and_overrides.scss', build_file_path('app/assets/stylesheets', '/casino_and_overrides.scss') + copy_file 'casino_and_overrides.scss', "app/assets/stylesheets/#{namespace_name}/casino_and_overrides.scss".squeeze('/') end def insert_assets_loader - insert_into_file build_file_path('app/assets/javascripts', '/application.js'), :after => %r{//= require +['"]?jquery_ujs['"]?} do + insert_into_file "app/assets/javascripts/#{namespace_name}/application.js".squeeze('/'), :after => %r{//= require +['"]?jquery_ujs['"]?} do "\n//= require casino" end end @@ -45,11 +45,8 @@ def show_readme end private - def build_file_path(root, path) - engine_name = Rails::Generators.namespace.to_s.underscore - engine_path = "/#{engine_name}" unless engine_name.blank? - - [root, engine_path, path].compact.join + def namespace_name + Rails::Generators.namespace.to_s.underscore end end diff --git a/lib/generators/casino/migration_generator.rb b/lib/generators/casino/migration_generator.rb index 46885edd..868b43b9 100644 --- a/lib/generators/casino/migration_generator.rb +++ b/lib/generators/casino/migration_generator.rb @@ -12,7 +12,6 @@ class MigrationGenerator < ::Rails::Generators::Base def install source_paths.each do |source_path| Dir["#{source_path}/*.rb"].each do |filename| - puts "MIGRATION TEMPLATE: #{File.basename(filename)}" migration_template File.basename(filename), "db/migrate/#{File.basename(filename).sub(/^\d+_/, '')}" end end From 3acb52efac70084ac7b8fabdf94483000696e85e Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 04:11:50 -0500 Subject: [PATCH 06/24] Rails 5.0 deprecations fixed --- app/models/casino/two_factor_authenticator.rb | 4 ++-- spec/dummy/config/environments/production.rb | 2 +- spec/dummy/config/environments/test.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/casino/two_factor_authenticator.rb b/app/models/casino/two_factor_authenticator.rb index ed93c604..d35f23fc 100644 --- a/app/models/casino/two_factor_authenticator.rb +++ b/app/models/casino/two_factor_authenticator.rb @@ -5,7 +5,7 @@ class CASino::TwoFactorAuthenticator < ActiveRecord::Base scope :active, -> { where(active: true) } def self.cleanup - self.delete_all(['(created_at < ?) AND active = ?', self.lifetime.ago, false]) + all.delete_all(['(created_at < ?) AND active = ?', lifetime.ago, false]) end def self.lifetime @@ -13,6 +13,6 @@ def self.lifetime end def expired? - !self.active? && (Time.now - (self.created_at || Time.now)) > self.class.lifetime + !active? && (Time.now - (created_at || Time.now)) > self.class.lifetime end end diff --git a/spec/dummy/config/environments/production.rb b/spec/dummy/config/environments/production.rb index 5c1b32e4..5580b3cd 100644 --- a/spec/dummy/config/environments/production.rb +++ b/spec/dummy/config/environments/production.rb @@ -22,7 +22,7 @@ # Disable serving static files from the `/public` folder by default since # Apache or NGINX already handles this. - config.serve_static_files = ENV['RAILS_SERVE_STATIC_FILES'].present? + # config.serve_static_files = ENV['RAILS_SERVE_STATIC_FILES'].present? # Compress JavaScripts and CSS. config.assets.js_compressor = :uglifier diff --git a/spec/dummy/config/environments/test.rb b/spec/dummy/config/environments/test.rb index 1c19f08b..1e09ecab 100644 --- a/spec/dummy/config/environments/test.rb +++ b/spec/dummy/config/environments/test.rb @@ -13,8 +13,8 @@ config.eager_load = false # Configure static file server for tests with Cache-Control for performance. - config.serve_static_files = true - config.static_cache_control = 'public, max-age=3600' + # config.serve_static_files = true + # config.static_cache_control = 'public, max-age=3600' # Show full error reports and disable caching. config.consider_all_requests_local = true From 5aa6f5d738ef22b2ab252cf3e00c89efc4d8f8a7 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 04:12:14 -0500 Subject: [PATCH 07/24] Add extracted gem to fix failing tests --- Appraisals | 2 ++ gemfiles/rails_5.0.gemfile | 1 + gemfiles/rails_5.1.gemfile | 1 + 3 files changed, 4 insertions(+) diff --git a/Appraisals b/Appraisals index 857098b5..1b7b02e8 100644 --- a/Appraisals +++ b/Appraisals @@ -6,9 +6,11 @@ end appraise 'rails-5.0' do gem 'activerecord', '~> 5.0.0' gem 'rspec-rails', '>= 3.0' + gem 'rails-controller-testing' end appraise 'rails-5.1' do gem 'activerecord', '~> 5.1.0' gem 'rspec-rails', '>= 3.0' + gem 'rails-controller-testing' end diff --git a/gemfiles/rails_5.0.gemfile b/gemfiles/rails_5.0.gemfile index b094b629..c2f190ba 100644 --- a/gemfiles/rails_5.0.gemfile +++ b/gemfiles/rails_5.0.gemfile @@ -4,6 +4,7 @@ source "https://rubygems.org" gem "activerecord", "~> 5.0.0" gem "rspec-rails", ">= 3.0" +gem "rails-controller-testing" group :test do gem "appraisal", ">= 2.1" diff --git a/gemfiles/rails_5.1.gemfile b/gemfiles/rails_5.1.gemfile index a24e6fb4..d11dbd0d 100644 --- a/gemfiles/rails_5.1.gemfile +++ b/gemfiles/rails_5.1.gemfile @@ -4,6 +4,7 @@ source "https://rubygems.org" gem "activerecord", "~> 5.1.0" gem "rspec-rails", ">= 3.0" +gem "rails-controller-testing" group :test do gem "appraisal", ">= 2.1" From 875a389c676c704d76a554ba0ed256edf43a6250 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 04:12:29 -0500 Subject: [PATCH 08/24] Updated lambda stynax --- spec/controllers/sessions_controller_spec.rb | 66 +++++++------------- 1 file changed, 22 insertions(+), 44 deletions(-) diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index e2456e68..0e0fe03b 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -5,8 +5,8 @@ routes { CASino::Engine.routes } - let(:params) { { } } - let(:user_agent) { 'YOLOBrowser 420.00'} + let(:params) { {} } + let(:user_agent) { 'YOLOBrowser 420.00' } before(:each) do request.user_agent = user_agent @@ -93,9 +93,7 @@ end it 'generates a service ticket' do - lambda do - get :new, params - end.should change(CASino::ServiceTicket, :count).by(1) + -> { get :new, params }.should change(CASino::ServiceTicket, :count).by(1) end it 'does not set the issued_from_credentials flag on the service ticket' do @@ -138,9 +136,7 @@ end it 'does not generate a service ticket' do - lambda do - get :new, params - end.should change(CASino::ServiceTicket, :count).by(0) + -> { get :new, params }.should change(CASino::ServiceTicket, :count).by(0) end context 'with a changed browser' do @@ -176,7 +172,7 @@ context 'with an expired login ticket' do let(:expired_login_ticket) { FactoryBot.create :login_ticket, :expired } - let(:params) { { lt: expired_login_ticket.ticket }} + let(:params) { { lt: expired_login_ticket.ticket } } it 'renders the new template' do post :create, params @@ -187,7 +183,7 @@ context 'with a valid login ticket' do let(:login_ticket) { FactoryBot.create :login_ticket } let(:username) { 'testuser' } - let(:params) { { lt: login_ticket.ticket, username: username, password: 'wrrooonnng' }} + let(:params) { { lt: login_ticket.ticket, username: username, password: 'wrrooonnng' } } let!(:user) { FactoryBot.create :user, username: username } context 'with invalid credentials' do @@ -304,16 +300,12 @@ end it 'generates a ticket-granting ticket' do - lambda do - post :create, params - end.should change(CASino::TicketGrantingTicket, :count).by(1) + -> { post :create, params }.should change(CASino::TicketGrantingTicket, :count).by(1) end context 'when the user does not exist yet' do it 'generates exactly one user' do - lambda do - post :create, params - end.should change(CASino::User, :count).by(1) + -> { post :create, params }.should change(CASino::User, :count).by(1) end it 'sets the users attributes' do @@ -328,9 +320,7 @@ let!(:user) { CASino::User.create! username: username, authenticator: authenticator } it 'does not regenerate the user' do - lambda do - post :create, params - end.should_not change(CASino::User, :count) + -> { post :create, params }.should_not change(CASino::User, :count) end it 'updates the extra attributes' do @@ -351,9 +341,7 @@ end it 'generates a service ticket' do - lambda do - post :create, params - end.should change(CASino::ServiceTicket, :count).by(1) + -> { post :create, params }.should change(CASino::ServiceTicket, :count).by(1) end it 'does set the issued_from_credentials flag on the service ticket' do @@ -362,9 +350,7 @@ end it 'generates a ticket-granting ticket' do - lambda do - post :create, params - end.should change(CASino::TicketGrantingTicket, :count).by(1) + -> { post :create, params }.should change(CASino::TicketGrantingTicket, :count).by(1) end end end @@ -379,7 +365,7 @@ let(:user_agent) { ticket_granting_ticket.user_agent } let(:otp) { '123456' } let(:service) { 'http://www.example.com/testing' } - let(:params) { { tgt: tgt, otp: otp, service: service }} + let(:params) { { tgt: tgt, otp: otp, service: service } } context 'with an active authenticator' do let!(:two_factor_authenticator) { FactoryBot.create :two_factor_authenticator, user: user } @@ -454,7 +440,7 @@ describe 'GET "logout"' do let(:url) { nil } - let(:params) { { :url => url } } + let(:params) { { url: url } } context 'with an existing ticket-granting ticket' do let(:ticket_granting_ticket) { FactoryBot.create :ticket_granting_ticket } @@ -465,7 +451,7 @@ it 'deletes the ticket-granting ticket' do get :logout, params - CASino::TicketGrantingTicket.where(id: ticket_granting_ticket.id).first.should == nil + CASino::TicketGrantingTicket.where(id: ticket_granting_ticket.id).first.should.nil? end it 'renders the logout template' do @@ -483,7 +469,7 @@ end context 'with a service' do - let(:params) { { :service => url } } + let(:params) { { service: url } } let(:url) { 'http://www.example.org' } context 'when whitelisted' do @@ -586,7 +572,7 @@ let(:login_attempts) do 6.times.map do |counter| FactoryBot.create :login_attempt, user: ticket_granting_ticket.user, - created_at: counter.minutes.ago + created_at: counter.minutes.ago end end @@ -627,9 +613,7 @@ let(:params) { { id: ticket_granting_ticket.id } } it 'deletes exactly one ticket-granting ticket' do - lambda do - delete :destroy, params - end.should change(CASino::TicketGrantingTicket, :count).by(-1) + -> { delete :destroy, params }.should change(CASino::TicketGrantingTicket, :count).by(-1) end it 'deletes the ticket-granting ticket' do @@ -644,11 +628,9 @@ end context 'with an invalid ticket-granting ticket' do - let(:params) { { id: 99999 } } + let(:params) { { id: 99_999 } } it 'does not delete a ticket-granting ticket' do - lambda do - delete :destroy, params - end.should_not change(CASino::TicketGrantingTicket, :count) + -> { delete :destroy, params }.should_not change(CASino::TicketGrantingTicket, :count) end it 'redirects to the session overview' do @@ -662,9 +644,7 @@ let(:params) { { id: ticket_granting_ticket.id } } it 'does not delete a ticket-granting ticket' do - lambda do - delete :destroy, params - end.should_not change(CASino::TicketGrantingTicket, :count) + -> { delete :destroy, params }.should_not change(CASino::TicketGrantingTicket, :count) end it 'redirects to the session overview' do @@ -676,7 +656,7 @@ describe 'GET "destroy_others"' do let(:url) { nil } - let(:params) { { :service => url } } + let(:params) { { service: url } } context 'with an existing ticket-granting ticket' do let(:user) { FactoryBot.create :user } @@ -689,9 +669,7 @@ end it 'deletes all other ticket-granting tickets' do - lambda do - get :destroy_others, params - end.should change(CASino::TicketGrantingTicket, :count).by(-3) + -> { get :destroy_others, params }.should change(CASino::TicketGrantingTicket, :count).by(-3) end it 'redirects to the session overview' do From e0f9ec822d1388c4c1c72273f3d32f20cc908b25 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 04:20:36 -0500 Subject: [PATCH 09/24] Use `all` scope for scope chaining instead of class --- app/models/casino/auth_token_ticket.rb | 5 ++--- app/models/casino/login_ticket.rb | 4 ++-- app/models/casino/service_ticket.rb | 21 +++++++++++---------- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/models/casino/auth_token_ticket.rb b/app/models/casino/auth_token_ticket.rb index dce6f077..28f372cf 100644 --- a/app/models/casino/auth_token_ticket.rb +++ b/app/models/casino/auth_token_ticket.rb @@ -5,11 +5,10 @@ class CASino::AuthTokenTicket < ActiveRecord::Base self.ticket_prefix = 'ATT'.freeze def self.cleanup - delete_all(['created_at < ?', CASino.config.auth_token_ticket[:lifetime].seconds.ago]) + all.delete_all(['created_at < ?', CASino.config.auth_token_ticket[:lifetime].seconds.ago]) end def expired? - (Time.now - (self.created_at || Time.now)) > CASino.config.auth_token_ticket[:lifetime].seconds + (Time.now - (created_at || Time.now)) > CASino.config.auth_token_ticket[:lifetime].seconds end - end diff --git a/app/models/casino/login_ticket.rb b/app/models/casino/login_ticket.rb index afc789ac..6768494f 100644 --- a/app/models/casino/login_ticket.rb +++ b/app/models/casino/login_ticket.rb @@ -5,10 +5,10 @@ class CASino::LoginTicket < ActiveRecord::Base self.ticket_prefix = 'LT'.freeze def self.cleanup - delete_all(['created_at < ?', CASino.config.login_ticket[:lifetime].seconds.ago]) + all.delete_all(['created_at < ?', CASino.config.login_ticket[:lifetime].seconds.ago]) end def expired? - (Time.now - (self.created_at || Time.now)) > CASino.config.login_ticket[:lifetime].seconds + (Time.now - (created_at || Time.now)) > CASino.config.login_ticket[:lifetime].seconds end end diff --git a/app/models/casino/service_ticket.rb b/app/models/casino/service_ticket.rb index 6e56fa4f..a2dbd5c2 100644 --- a/app/models/casino/service_ticket.rb +++ b/app/models/casino/service_ticket.rb @@ -10,15 +10,15 @@ class CASino::ServiceTicket < ActiveRecord::Base has_many :proxy_granting_tickets, as: :granter, dependent: :destroy def self.cleanup_unconsumed - self.delete_all(['created_at < ? AND consumed = ?', CASino.config.service_ticket[:lifetime_unconsumed].seconds.ago, false]) + all.delete_all(['created_at < ? AND consumed = ?', CASino.config.service_ticket[:lifetime_unconsumed].seconds.ago, false]) end def self.cleanup_consumed - self.destroy_all(['(ticket_granting_ticket_id IS NULL OR created_at < ?) AND consumed = ?', CASino.config.service_ticket[:lifetime_consumed].seconds.ago, true]) + all.destroy_all(['(ticket_granting_ticket_id IS NULL OR created_at < ?) AND consumed = ?', CASino.config.service_ticket[:lifetime_consumed].seconds.ago, true]) end def self.cleanup_consumed_hard - self.delete_all(['created_at < ? AND consumed = ?', (CASino.config.service_ticket[:lifetime_consumed] * 2).seconds.ago, true]) + all.delete_all(['created_at < ? AND consumed = ?', (CASino.config.service_ticket[:lifetime_consumed] * 2).seconds.ago, true]) end def service=(service) @@ -27,21 +27,22 @@ def service=(service) end def service_with_ticket_url - service_uri = Addressable::URI.parse(self.service) - service_uri.query_values = (service_uri.query_values(Array) || []) << ['ticket', self.ticket] + service_uri = Addressable::URI.parse(service) + service_uri.query_values = (service_uri.query_values(Array) || []) << ['ticket', ticket] service_uri.to_s end def expired? lifetime = if consumed? - CASino.config.service_ticket[:lifetime_consumed] - else - CASino.config.service_ticket[:lifetime_unconsumed] - end - (Time.now - (self.created_at || Time.now)) > lifetime + CASino.config.service_ticket[:lifetime_consumed] + else + CASino.config.service_ticket[:lifetime_unconsumed] + end + (Time.now - (created_at || Time.now)) > lifetime end private + def send_single_sign_out_notification notifier = SingleSignOutNotifier.new(self) notifier.notify From 8b4ddf4d025ca053747be73d6ddd94f46efc7d93 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 04:48:24 -0500 Subject: [PATCH 10/24] Updated RSpec version for specific Rails versions --- Appraisals | 4 ++-- Gemfile | 1 - gemfiles/rails_4.2.gemfile | 1 - gemfiles/rails_5.0.gemfile | 3 +-- gemfiles/rails_5.1.gemfile | 3 +-- 5 files changed, 4 insertions(+), 8 deletions(-) diff --git a/Appraisals b/Appraisals index 1b7b02e8..fc9fd3e4 100644 --- a/Appraisals +++ b/Appraisals @@ -5,12 +5,12 @@ end appraise 'rails-5.0' do gem 'activerecord', '~> 5.0.0' - gem 'rspec-rails', '>= 3.0' gem 'rails-controller-testing' + gem 'rspec-rails', '>= 3.5' end appraise 'rails-5.1' do gem 'activerecord', '~> 5.1.0' - gem 'rspec-rails', '>= 3.0' gem 'rails-controller-testing' + gem 'rspec-rails', '>= 3.5' end diff --git a/Gemfile b/Gemfile index 178ba7a2..f3f929d8 100644 --- a/Gemfile +++ b/Gemfile @@ -7,7 +7,6 @@ group :test do gem 'factory_bot', '>= 4.1' gem 'rake', '>= 10.0' gem 'rspec-its', '>= 1.0' - gem 'rspec-rails', '>= 3.0' gem 'webmock', '>= 1.9' end diff --git a/gemfiles/rails_4.2.gemfile b/gemfiles/rails_4.2.gemfile index 31d104df..c82adb7d 100644 --- a/gemfiles/rails_4.2.gemfile +++ b/gemfiles/rails_4.2.gemfile @@ -12,7 +12,6 @@ group :test do gem "factory_bot", ">= 4.1" gem "rake", ">= 10.0" gem "rspec-its", ">= 1.0" - gem "rspec-rails", ">= 3.0" gem "webmock", ">= 1.9" end diff --git a/gemfiles/rails_5.0.gemfile b/gemfiles/rails_5.0.gemfile index c2f190ba..b7fc5e9e 100644 --- a/gemfiles/rails_5.0.gemfile +++ b/gemfiles/rails_5.0.gemfile @@ -3,8 +3,8 @@ source "https://rubygems.org" gem "activerecord", "~> 5.0.0" -gem "rspec-rails", ">= 3.0" gem "rails-controller-testing" +gem "rspec-rails", ">= 3.5" group :test do gem "appraisal", ">= 2.1" @@ -13,7 +13,6 @@ group :test do gem "factory_bot", ">= 4.1" gem "rake", ">= 10.0" gem "rspec-its", ">= 1.0" - gem "rspec-rails", ">= 3.0" gem "webmock", ">= 1.9" end diff --git a/gemfiles/rails_5.1.gemfile b/gemfiles/rails_5.1.gemfile index d11dbd0d..19134c6f 100644 --- a/gemfiles/rails_5.1.gemfile +++ b/gemfiles/rails_5.1.gemfile @@ -3,8 +3,8 @@ source "https://rubygems.org" gem "activerecord", "~> 5.1.0" -gem "rspec-rails", ">= 3.0" gem "rails-controller-testing" +gem "rspec-rails", ">= 3.5" group :test do gem "appraisal", ">= 2.1" @@ -13,7 +13,6 @@ group :test do gem "factory_bot", ">= 4.1" gem "rake", ">= 10.0" gem "rspec-its", ">= 1.0" - gem "rspec-rails", ">= 3.0" gem "webmock", ">= 1.9" end From 83c84ff469b047f5c722bb38333e099783e6d51a Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 12:21:03 -0500 Subject: [PATCH 11/24] Remove version constraint to allow for Rails 5.x --- casino.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/casino.gemspec b/casino.gemspec index a008d6dd..b7dbd7be 100644 --- a/casino.gemspec +++ b/casino.gemspec @@ -32,7 +32,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'rails', '>= 4.2' s.add_runtime_dependency 'rotp', '>= 2.0' s.add_runtime_dependency 'rqrcode_png', '>= 0.1' - s.add_runtime_dependency 'sass-rails', '>= 4.0.0', '< 6.0.0' + s.add_runtime_dependency 'sass-rails', '>= 4.0.0' s.add_runtime_dependency 'terminal-table', '>= 1.4' s.add_runtime_dependency 'useragent', '>= 0.4' end From 53de3b7f8dd06bbcc5309a6899da4dc907b8c815 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 20:17:27 -0500 Subject: [PATCH 12/24] Add ApplicationRecord base class for models --- app/models/casino/application_record.rb | 3 +++ app/models/casino/auth_token_ticket.rb | 2 +- app/models/casino/login_attempt.rb | 2 +- app/models/casino/login_ticket.rb | 2 +- app/models/casino/proxy_granting_ticket.rb | 2 +- app/models/casino/proxy_ticket.rb | 2 +- app/models/casino/service_rule.rb | 2 +- app/models/casino/service_ticket.rb | 2 +- app/models/casino/ticket_granting_ticket.rb | 2 +- app/models/casino/two_factor_authenticator.rb | 2 +- app/models/casino/user.rb | 2 +- 11 files changed, 13 insertions(+), 10 deletions(-) create mode 100644 app/models/casino/application_record.rb diff --git a/app/models/casino/application_record.rb b/app/models/casino/application_record.rb new file mode 100644 index 00000000..6d054639 --- /dev/null +++ b/app/models/casino/application_record.rb @@ -0,0 +1,3 @@ +class CASino::ApplicationRecord < ActiveRecord::Base + self.abstract_class = true +end diff --git a/app/models/casino/auth_token_ticket.rb b/app/models/casino/auth_token_ticket.rb index 28f372cf..52713cea 100644 --- a/app/models/casino/auth_token_ticket.rb +++ b/app/models/casino/auth_token_ticket.rb @@ -1,4 +1,4 @@ -class CASino::AuthTokenTicket < ActiveRecord::Base +class CASino::AuthTokenTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket include CASino::ModelConcern::ConsumableTicket diff --git a/app/models/casino/login_attempt.rb b/app/models/casino/login_attempt.rb index 73ca13ca..4914fea9 100644 --- a/app/models/casino/login_attempt.rb +++ b/app/models/casino/login_attempt.rb @@ -1,4 +1,4 @@ -class CASino::LoginAttempt < ActiveRecord::Base +class CASino::LoginAttempt < CASino::ApplicationRecord include CASino::ModelConcern::BrowserInfo belongs_to :user diff --git a/app/models/casino/login_ticket.rb b/app/models/casino/login_ticket.rb index 6768494f..00942e93 100644 --- a/app/models/casino/login_ticket.rb +++ b/app/models/casino/login_ticket.rb @@ -1,4 +1,4 @@ -class CASino::LoginTicket < ActiveRecord::Base +class CASino::LoginTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket include CASino::ModelConcern::ConsumableTicket diff --git a/app/models/casino/proxy_granting_ticket.rb b/app/models/casino/proxy_granting_ticket.rb index 07dece3d..33ee6525 100644 --- a/app/models/casino/proxy_granting_ticket.rb +++ b/app/models/casino/proxy_granting_ticket.rb @@ -1,5 +1,5 @@ -class CASino::ProxyGrantingTicket < ActiveRecord::Base +class CASino::ProxyGrantingTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket self.ticket_prefix = 'PGT'.freeze diff --git a/app/models/casino/proxy_ticket.rb b/app/models/casino/proxy_ticket.rb index e64b358a..05235255 100644 --- a/app/models/casino/proxy_ticket.rb +++ b/app/models/casino/proxy_ticket.rb @@ -1,6 +1,6 @@ require 'addressable/uri' -class CASino::ProxyTicket < ActiveRecord::Base +class CASino::ProxyTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket self.ticket_prefix = 'PT'.freeze diff --git a/app/models/casino/service_rule.rb b/app/models/casino/service_rule.rb index 27b9b1f9..6188d2fe 100644 --- a/app/models/casino/service_rule.rb +++ b/app/models/casino/service_rule.rb @@ -1,5 +1,5 @@ -class CASino::ServiceRule < ActiveRecord::Base +class CASino::ServiceRule < CASino::ApplicationRecord validates :name, presence: true validates :url, uniqueness: true, presence: true diff --git a/app/models/casino/service_ticket.rb b/app/models/casino/service_ticket.rb index a2dbd5c2..c1e99a88 100644 --- a/app/models/casino/service_ticket.rb +++ b/app/models/casino/service_ticket.rb @@ -1,6 +1,6 @@ require 'addressable/uri' -class CASino::ServiceTicket < ActiveRecord::Base +class CASino::ServiceTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket self.ticket_prefix = 'ST'.freeze diff --git a/app/models/casino/ticket_granting_ticket.rb b/app/models/casino/ticket_granting_ticket.rb index 0def6e1e..d9069d3f 100644 --- a/app/models/casino/ticket_granting_ticket.rb +++ b/app/models/casino/ticket_granting_ticket.rb @@ -1,6 +1,6 @@ require 'user_agent' -class CASino::TicketGrantingTicket < ActiveRecord::Base +class CASino::TicketGrantingTicket < CASino::ApplicationRecord include CASino::ModelConcern::Ticket include CASino::ModelConcern::BrowserInfo diff --git a/app/models/casino/two_factor_authenticator.rb b/app/models/casino/two_factor_authenticator.rb index d35f23fc..ff617af8 100644 --- a/app/models/casino/two_factor_authenticator.rb +++ b/app/models/casino/two_factor_authenticator.rb @@ -1,5 +1,5 @@ -class CASino::TwoFactorAuthenticator < ActiveRecord::Base +class CASino::TwoFactorAuthenticator < CASino::ApplicationRecord belongs_to :user scope :active, -> { where(active: true) } diff --git a/app/models/casino/user.rb b/app/models/casino/user.rb index 2bd2a72b..b65685b6 100644 --- a/app/models/casino/user.rb +++ b/app/models/casino/user.rb @@ -1,5 +1,5 @@ -class CASino::User < ActiveRecord::Base +class CASino::User < CASino::ApplicationRecord serialize :extra_attributes, Hash has_many :ticket_granting_tickets From 93ae39b572312e380f99a12ebc0eb6d7a776d8c5 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 20:17:36 -0500 Subject: [PATCH 13/24] Add migration version to migrations --- db/migrate/20130809135400_create_core_schema.rb | 4 ++-- db/migrate/20130809135401_rename_base_models.rb | 2 +- db/migrate/20131022110146_cleanup_indexes.rb | 2 +- db/migrate/20131022110246_fix_long_index_names.rb | 2 +- db/migrate/20131022110346_change_service_to_text.rb | 2 +- db/migrate/20140821142611_change_user_agent_to_text.rb | 2 +- db/migrate/20140827183611_fix_length_of_text_fields.rb | 2 +- db/migrate/20140831205255_create_auth_token_tickets.rb | 2 +- .../20151022192752_add_user_ip_to_ticket_granting_ticket.rb | 2 +- db/migrate/20160502074450_create_login_attempts.rb | 2 +- .../20160524121117_remove_username_from_login_attempts.rb | 2 +- 11 files changed, 12 insertions(+), 12 deletions(-) diff --git a/db/migrate/20130809135400_create_core_schema.rb b/db/migrate/20130809135400_create_core_schema.rb index 57ddba69..5dee53cc 100644 --- a/db/migrate/20130809135400_create_core_schema.rb +++ b/db/migrate/20130809135400_create_core_schema.rb @@ -1,6 +1,6 @@ # In order to support pre-2.0 installations of CASino that included CASinoCore, # we must rebuild the un-namespaced CASinoCore schema so that we can upgrade -class CreateCoreSchema < ActiveRecord::Migration +class CreateCoreSchema < ActiveRecord::Migration[4.1] CoreTables = %w{login_tickets proxy_granting_tickets proxy_tickets service_rules service_tickets ticket_granting_tickets two_factor_authenticators users} def up @@ -114,4 +114,4 @@ def create_users end add_index :users, [:authenticator, :username], :unique => true end -end \ No newline at end of file +end diff --git a/db/migrate/20130809135401_rename_base_models.rb b/db/migrate/20130809135401_rename_base_models.rb index c44c40cf..8d6fe406 100644 --- a/db/migrate/20130809135401_rename_base_models.rb +++ b/db/migrate/20130809135401_rename_base_models.rb @@ -1,4 +1,4 @@ -class RenameBaseModels < ActiveRecord::Migration +class RenameBaseModels < ActiveRecord::Migration[4.1] def up # Login Tickets rename_table :login_tickets, :casino_login_tickets diff --git a/db/migrate/20131022110146_cleanup_indexes.rb b/db/migrate/20131022110146_cleanup_indexes.rb index 0488cb8f..c267dd04 100644 --- a/db/migrate/20131022110146_cleanup_indexes.rb +++ b/db/migrate/20131022110146_cleanup_indexes.rb @@ -1,4 +1,4 @@ -class CleanupIndexes < ActiveRecord::Migration +class CleanupIndexes < ActiveRecord::Migration[4.1] def change # delete some leftovers in migrated CASino 1.x installations remove_deprecated_index_if_exists :login_tickets, [:ticket] diff --git a/db/migrate/20131022110246_fix_long_index_names.rb b/db/migrate/20131022110246_fix_long_index_names.rb index ba655754..1de923f6 100644 --- a/db/migrate/20131022110246_fix_long_index_names.rb +++ b/db/migrate/20131022110246_fix_long_index_names.rb @@ -1,4 +1,4 @@ -class FixLongIndexNames < ActiveRecord::Migration +class FixLongIndexNames < ActiveRecord::Migration[4.1] def change # Long names prevent us from doing some migrations, because the resulting # temporary index names would be longer than 64 characters: diff --git a/db/migrate/20131022110346_change_service_to_text.rb b/db/migrate/20131022110346_change_service_to_text.rb index 03352954..182c38fc 100644 --- a/db/migrate/20131022110346_change_service_to_text.rb +++ b/db/migrate/20131022110346_change_service_to_text.rb @@ -1,4 +1,4 @@ -class ChangeServiceToText < ActiveRecord::Migration +class ChangeServiceToText < ActiveRecord::Migration[4.1] def change change_column :casino_proxy_tickets, :service, :text change_column :casino_service_tickets, :service, :text diff --git a/db/migrate/20140821142611_change_user_agent_to_text.rb b/db/migrate/20140821142611_change_user_agent_to_text.rb index 89d7e432..deaad50f 100644 --- a/db/migrate/20140821142611_change_user_agent_to_text.rb +++ b/db/migrate/20140821142611_change_user_agent_to_text.rb @@ -1,4 +1,4 @@ -class ChangeUserAgentToText < ActiveRecord::Migration +class ChangeUserAgentToText < ActiveRecord::Migration[4.1] def change change_column :casino_ticket_granting_tickets, :user_agent, :text end diff --git a/db/migrate/20140827183611_fix_length_of_text_fields.rb b/db/migrate/20140827183611_fix_length_of_text_fields.rb index b9b38657..f6aac322 100644 --- a/db/migrate/20140827183611_fix_length_of_text_fields.rb +++ b/db/migrate/20140827183611_fix_length_of_text_fields.rb @@ -1,4 +1,4 @@ -class FixLengthOfTextFields < ActiveRecord::Migration +class FixLengthOfTextFields < ActiveRecord::Migration[4.1] def change change_column :casino_proxy_tickets, :service, :text, :limit => nil change_column :casino_service_tickets, :service, :text, :limit => nil diff --git a/db/migrate/20140831205255_create_auth_token_tickets.rb b/db/migrate/20140831205255_create_auth_token_tickets.rb index 5e6a0987..a1865e63 100644 --- a/db/migrate/20140831205255_create_auth_token_tickets.rb +++ b/db/migrate/20140831205255_create_auth_token_tickets.rb @@ -1,4 +1,4 @@ -class CreateAuthTokenTickets < ActiveRecord::Migration +class CreateAuthTokenTickets < ActiveRecord::Migration[4.1] def change create_table :casino_auth_token_tickets do |t| t.string :ticket, :null => false diff --git a/db/migrate/20151022192752_add_user_ip_to_ticket_granting_ticket.rb b/db/migrate/20151022192752_add_user_ip_to_ticket_granting_ticket.rb index 92b5a15f..011b75d6 100644 --- a/db/migrate/20151022192752_add_user_ip_to_ticket_granting_ticket.rb +++ b/db/migrate/20151022192752_add_user_ip_to_ticket_granting_ticket.rb @@ -1,4 +1,4 @@ -class AddUserIpToTicketGrantingTicket < ActiveRecord::Migration +class AddUserIpToTicketGrantingTicket < ActiveRecord::Migration[4.1] def up add_column :casino_ticket_granting_tickets, :user_ip, :string end diff --git a/db/migrate/20160502074450_create_login_attempts.rb b/db/migrate/20160502074450_create_login_attempts.rb index 802fe0c4..8b86c56c 100644 --- a/db/migrate/20160502074450_create_login_attempts.rb +++ b/db/migrate/20160502074450_create_login_attempts.rb @@ -1,4 +1,4 @@ -class CreateLoginAttempts < ActiveRecord::Migration +class CreateLoginAttempts < ActiveRecord::Migration[4.1] def change create_table :casino_login_attempts do |t| t.integer :user_id, null: true diff --git a/db/migrate/20160524121117_remove_username_from_login_attempts.rb b/db/migrate/20160524121117_remove_username_from_login_attempts.rb index d9a94bcd..a31063df 100644 --- a/db/migrate/20160524121117_remove_username_from_login_attempts.rb +++ b/db/migrate/20160524121117_remove_username_from_login_attempts.rb @@ -1,4 +1,4 @@ -class RemoveUsernameFromLoginAttempts < ActiveRecord::Migration +class RemoveUsernameFromLoginAttempts < ActiveRecord::Migration[4.1] def up remove_column :casino_login_attempts, :username change_column_null :casino_login_attempts, :user_id, false From df6f60d052a90bfccdc959373d2d9d89b3a327e2 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 20:27:35 -0500 Subject: [PATCH 14/24] Use `data_source_exists?` to check for tables and views - gets rid of deprecation warning that `table_exists?` will only check tables in Rails 5.1 --- db/migrate/20130809135400_create_core_schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20130809135400_create_core_schema.rb b/db/migrate/20130809135400_create_core_schema.rb index 5dee53cc..8aec23e2 100644 --- a/db/migrate/20130809135400_create_core_schema.rb +++ b/db/migrate/20130809135400_create_core_schema.rb @@ -5,7 +5,7 @@ class CreateCoreSchema < ActiveRecord::Migration[4.1] def up CoreTables.each do |table_name| - if !ActiveRecord::Base.connection.table_exists? table_name + unless connection.data_source_exists? table_name send "create_#{table_name}" end end From 7ce8f379a2fffd72a6091303588a8550358c8767 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 21:03:22 -0500 Subject: [PATCH 15/24] Set new option explicitly --- spec/dummy/app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/dummy/app/controllers/application_controller.rb b/spec/dummy/app/controllers/application_controller.rb index e8065d95..ae44c518 100644 --- a/spec/dummy/app/controllers/application_controller.rb +++ b/spec/dummy/app/controllers/application_controller.rb @@ -1,3 +1,3 @@ class ApplicationController < ActionController::Base - protect_from_forgery + protect_from_forgery prepend: true end From 4bf1512db8949e6f097e9a235ab83171b291badb Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 21:52:19 -0500 Subject: [PATCH 16/24] Fix deprecated queries for Rails 5.0 --- app/models/casino/auth_token_ticket.rb | 2 +- app/models/casino/login_ticket.rb | 2 +- app/models/casino/proxy_ticket.rb | 4 ++-- app/models/casino/service_ticket.rb | 6 +++--- app/models/casino/two_factor_authenticator.rb | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/casino/auth_token_ticket.rb b/app/models/casino/auth_token_ticket.rb index 52713cea..01fc4e96 100644 --- a/app/models/casino/auth_token_ticket.rb +++ b/app/models/casino/auth_token_ticket.rb @@ -5,7 +5,7 @@ class CASino::AuthTokenTicket < CASino::ApplicationRecord self.ticket_prefix = 'ATT'.freeze def self.cleanup - all.delete_all(['created_at < ?', CASino.config.auth_token_ticket[:lifetime].seconds.ago]) + where(['created_at < ?', CASino.config.auth_token_ticket[:lifetime].seconds.ago]).delete_all end def expired? diff --git a/app/models/casino/login_ticket.rb b/app/models/casino/login_ticket.rb index 00942e93..a18782af 100644 --- a/app/models/casino/login_ticket.rb +++ b/app/models/casino/login_ticket.rb @@ -5,7 +5,7 @@ class CASino::LoginTicket < CASino::ApplicationRecord self.ticket_prefix = 'LT'.freeze def self.cleanup - all.delete_all(['created_at < ?', CASino.config.login_ticket[:lifetime].seconds.ago]) + where(['created_at < ?', CASino.config.login_ticket[:lifetime].seconds.ago]).delete_all end def expired? diff --git a/app/models/casino/proxy_ticket.rb b/app/models/casino/proxy_ticket.rb index 05235255..caef26c9 100644 --- a/app/models/casino/proxy_ticket.rb +++ b/app/models/casino/proxy_ticket.rb @@ -10,11 +10,11 @@ class CASino::ProxyTicket < CASino::ApplicationRecord has_many :proxy_granting_tickets, as: :granter, dependent: :destroy def self.cleanup_unconsumed - self.destroy_all(['created_at < ? AND consumed = ?', CASino.config.proxy_ticket[:lifetime_unconsumed].seconds.ago, false]) + where(['created_at < ? AND consumed = ?', CASino.config.proxy_ticket[:lifetime_unconsumed].seconds.ago, false]).destroy_all end def self.cleanup_consumed - self.destroy_all(['created_at < ? AND consumed = ?', CASino.config.proxy_ticket[:lifetime_consumed].seconds.ago, true]) + where(['created_at < ? AND consumed = ?', CASino.config.proxy_ticket[:lifetime_consumed].seconds.ago, true]).destroy_all end def expired? diff --git a/app/models/casino/service_ticket.rb b/app/models/casino/service_ticket.rb index c1e99a88..3a5ddd38 100644 --- a/app/models/casino/service_ticket.rb +++ b/app/models/casino/service_ticket.rb @@ -10,15 +10,15 @@ class CASino::ServiceTicket < CASino::ApplicationRecord has_many :proxy_granting_tickets, as: :granter, dependent: :destroy def self.cleanup_unconsumed - all.delete_all(['created_at < ? AND consumed = ?', CASino.config.service_ticket[:lifetime_unconsumed].seconds.ago, false]) + where(['created_at < ? AND consumed = ?', CASino.config.service_ticket[:lifetime_unconsumed].seconds.ago, false]).delete_all end def self.cleanup_consumed - all.destroy_all(['(ticket_granting_ticket_id IS NULL OR created_at < ?) AND consumed = ?', CASino.config.service_ticket[:lifetime_consumed].seconds.ago, true]) + where(['(ticket_granting_ticket_id IS NULL OR created_at < ?) AND consumed = ?', CASino.config.service_ticket[:lifetime_consumed].seconds.ago, true]).destroy_all end def self.cleanup_consumed_hard - all.delete_all(['created_at < ? AND consumed = ?', (CASino.config.service_ticket[:lifetime_consumed] * 2).seconds.ago, true]) + where(['created_at < ? AND consumed = ?', (CASino.config.service_ticket[:lifetime_consumed] * 2).seconds.ago, true]).delete_all end def service=(service) diff --git a/app/models/casino/two_factor_authenticator.rb b/app/models/casino/two_factor_authenticator.rb index ff617af8..16435b06 100644 --- a/app/models/casino/two_factor_authenticator.rb +++ b/app/models/casino/two_factor_authenticator.rb @@ -5,7 +5,7 @@ class CASino::TwoFactorAuthenticator < CASino::ApplicationRecord scope :active, -> { where(active: true) } def self.cleanup - all.delete_all(['(created_at < ?) AND active = ?', lifetime.ago, false]) + where(['(created_at < ?) AND active = ?', lifetime.ago, false]).delete_all end def self.lifetime From f13ea3d95d10798bafbb19159c76bc8f7269cf90 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 21:53:12 -0500 Subject: [PATCH 17/24] Use keyword arguments in integration tests for Rails 5.0 --- .../auth_tokens_controller_spec.rb | 16 +- .../proxy_tickets_controller_spec.rb | 30 ++-- ...rvice_and_proxy_tickets_controller_spec.rb | 46 +++--- .../service_tickets_controller_spec.rb | 16 +- spec/controllers/sessions_controller_spec.rb | 145 +++++++++--------- ...o_factor_authenticators_controller_spec.rb | 54 +++---- 6 files changed, 155 insertions(+), 152 deletions(-) diff --git a/spec/controllers/auth_tokens_controller_spec.rb b/spec/controllers/auth_tokens_controller_spec.rb index 88d1ab05..1c95c5d6 100644 --- a/spec/controllers/auth_tokens_controller_spec.rb +++ b/spec/controllers/auth_tokens_controller_spec.rb @@ -4,7 +4,7 @@ routes { CASino::Engine.routes } let(:params) { {} } - let(:request_options) { params } + let(:request_options) { {params: params} } before(:each) do CASino::AuthTokenValidationService.any_instance.stub(:validation_result).and_return(validation_result) @@ -17,7 +17,7 @@ let(:params) { { service: service } } it 'redirects to the login' do - get :login, request_options + get :login, **request_options response.should redirect_to(login_path(service: service)) end end @@ -34,7 +34,7 @@ let(:params) { { service: service } } it 'renders the service_not_allowed template' do - get :login, request_options + get :login, **request_options response.should render_template(:service_not_allowed) end end @@ -44,30 +44,30 @@ let(:params) { { service: service } } it 'redirects to the service' do - get :login, request_options + get :login, **request_options response.location.should =~ /^#{Regexp.escape service}\?ticket=ST-/ end it 'generates a service ticket' do lambda do - get :login, request_options + get :login, **request_options end.should change(CASino::ServiceTicket, :count).by(1) end end it 'creates a cookie' do - get :login, request_options + get :login, **request_options response.cookies['tgt'].should_not be_nil end it 'generates a ticket-granting ticket' do lambda do - get :login, request_options + get :login, **request_options end.should change(CASino::TicketGrantingTicket, :count).by(1) end it 'redirects to the session overview' do - get :login, request_options + get :login, **request_options response.should redirect_to(sessions_path) end end diff --git a/spec/controllers/proxy_tickets_controller_spec.rb b/spec/controllers/proxy_tickets_controller_spec.rb index d6a81783..e0bd7a32 100644 --- a/spec/controllers/proxy_tickets_controller_spec.rb +++ b/spec/controllers/proxy_tickets_controller_spec.rb @@ -3,7 +3,7 @@ describe CASino::ProxyTicketsController do routes { CASino::Engine.routes } - let(:request_options) { params } + let(:request_options) { {params: params} } describe 'GET "proxyValidate"' do let(:proxy_ticket) { FactoryBot.create :proxy_ticket } @@ -19,12 +19,12 @@ let(:regex_proxy) { /\s*#{proxy_ticket.proxy_granting_ticket.pgt_url}<\/cas:proxy>\s*<\/cas:proxies>/ } it 'answers with the success text' do - get :proxy_validate, request_options + get :proxy_validate, **request_options response.body.should =~ regex_success end it 'includes the proxy in the response' do - get :proxy_validate, request_options + get :proxy_validate, **request_options response.body.should =~ regex_proxy end @@ -34,7 +34,7 @@ end it 'answers with the failure text' do - get :proxy_validate, request_options + get :proxy_validate, **request_options response.body.should =~ regex_failure end end @@ -43,7 +43,7 @@ let(:params) { parameters.merge(service: 'this_is_another_service') } it 'answers with the failure text' do - get :proxy_validate, request_options + get :proxy_validate, **request_options response.body.should =~ regex_failure end end @@ -52,7 +52,7 @@ let(:params) { { ticket: 'PT-1234', service: 'https://www.example.com/' } } it 'answers with the failure text' do - get :proxy_validate, request_options + get :proxy_validate, **request_options response.body.should =~ regex_failure end end @@ -67,13 +67,13 @@ context 'without proxy-granting ticket' do it 'answers with the failure text' do - get :create, request_options + get :create, **request_options response.body.should =~ regex_failure end it 'does not create a proxy ticket' do lambda do - get :create, request_options + get :create, **request_options end.should_not change(CASino::ProxyTicket, :count) end end @@ -82,13 +82,13 @@ let(:params) { parameters.merge(pgt: 'PGT-123453789') } it 'answers with the failure text' do - get :create, request_options + get :create, **request_options response.body.should =~ regex_failure end it 'does not create a proxy ticket' do lambda do - get :create, request_options + get :create, **request_options end.should_not change(CASino::ProxyTicket, :count) end end @@ -98,18 +98,18 @@ let(:params) { parameters.merge(pgt: proxy_granting_ticket.ticket) } it 'answers with the success text' do - get :create, request_options + get :create, **request_options response.body.should =~ regex_success end it 'does create a proxy ticket' do lambda do - get :create, request_options + get :create, **request_options end.should change(proxy_granting_ticket.proxy_tickets, :count).by(1) end it 'includes the proxy ticket in the response' do - get :create, request_options + get :create, **request_options proxy_ticket = CASino::ProxyTicket.last response.body.should =~ /#{proxy_ticket.ticket}<\/cas:proxyTicket>/ end @@ -118,13 +118,13 @@ let(:params) { parameters.merge(pgt: proxy_granting_ticket.ticket, targetService: nil) } it 'answers with the failure text' do - get :create, request_options + get :create, **request_options response.body.should =~ regex_failure end it 'does not create a proxy ticket' do lambda do - get :create, request_options + get :create, **request_options end.should_not change(CASino::ProxyTicket, :count) end end diff --git a/spec/controllers/service_and_proxy_tickets_controller_spec.rb b/spec/controllers/service_and_proxy_tickets_controller_spec.rb index 1ec8393e..c8fe72e0 100644 --- a/spec/controllers/service_and_proxy_tickets_controller_spec.rb +++ b/spec/controllers/service_and_proxy_tickets_controller_spec.rb @@ -3,7 +3,7 @@ shared_examples_for 'a service ticket validator' do routes { CASino::Engine.routes } - let(:request_options) { params } + let(:request_options) { {params: params} } let(:service_ticket) { FactoryBot.create :service_ticket } let(:service) { service_ticket.service } let(:parameters) { { service: service, ticket: service_ticket.ticket }} @@ -21,7 +21,7 @@ context "without '#{missing_parameter}'" do it 'answers with the failure text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_failure end end @@ -35,7 +35,7 @@ end it 'includes the extra attributes' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ /1234<\/cas\:id\>/ end end @@ -46,7 +46,7 @@ end it 'includes all values' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ /test<\/cas\:memberOf\>/ response.body.should =~ /yolo<\/cas\:memberOf\>/ end @@ -58,19 +58,19 @@ end it 'includes the long-term flag in the answer' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ /true<\/cas\:longTermAuthenticationRequestTokenUsed>/ end end context 'without renew flag' do it 'consumes the service ticket' do - get validation_action, request_options + get validation_action, **request_options service_ticket.reload.consumed.should == true end it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end end @@ -79,7 +79,7 @@ let(:service) { "#{service_ticket.service}?" } it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end end @@ -89,12 +89,12 @@ context 'with a service ticket without issued_from_credentials flag' do it 'consumes the service ticket' do - get validation_action, request_options + get validation_action, **request_options service_ticket.reload.consumed.should == true end it 'answers with the failure text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_failure end end @@ -106,12 +106,12 @@ end it 'consumes the service ticket' do - get validation_action, request_options + get validation_action, **request_options service_ticket.reload.consumed.should == true end it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end end @@ -129,35 +129,35 @@ let(:pgt_url) { 'http://www.example.org' } it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end it 'does not create a proxy-granting ticket' do lambda do - get validation_action, request_options + get validation_action, **request_options end.should_not change(service_ticket.proxy_granting_tickets, :count) end end it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end it 'includes the PGTIOU in the response' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ /\\n?\s*PGTIOU-.+/ end it 'creates a proxy-granting ticket' do lambda do - get validation_action, request_options + get validation_action, **request_options end.should change(service_ticket.proxy_granting_tickets, :count).by(1) end it 'contacts the callback server' do - get validation_action, request_options + get validation_action, **request_options proxy_granting_ticket = CASino::ProxyGrantingTicket.last WebMock.should have_requested(:get, 'https://www.example.org').with(query: { pgtId: proxy_granting_ticket.ticket, @@ -171,13 +171,13 @@ end it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end it 'does not create a proxy-granting ticket' do lambda do - get validation_action, request_options + get validation_action, **request_options end.should_not change(service_ticket.proxy_granting_tickets, :count) end end @@ -188,13 +188,13 @@ end it 'answers with the success text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_success end it 'does not create a proxy-granting ticket' do lambda do - get validation_action, request_options + get validation_action, **request_options end.should_not change(service_ticket.proxy_granting_tickets, :count) end end @@ -207,7 +207,7 @@ end it 'answers with the failure text' do - get validation_action, request_options + get validation_action, **request_options response.body.should =~ regex_failure end end diff --git a/spec/controllers/service_tickets_controller_spec.rb b/spec/controllers/service_tickets_controller_spec.rb index f61466c8..6483ac70 100644 --- a/spec/controllers/service_tickets_controller_spec.rb +++ b/spec/controllers/service_tickets_controller_spec.rb @@ -2,7 +2,7 @@ routes { CASino::Engine.routes } describe 'GET "validate"' do - let(:request_options) { params } + let(:request_options) { {params: params} } let(:service_ticket) { FactoryBot.create :service_ticket } let(:service) { service_ticket.service } let(:parameters) { { service: service, ticket: service_ticket.ticket }} @@ -16,12 +16,12 @@ context 'with an unconsumed service ticket' do context 'without renew flag' do it 'consumes the service ticket' do - get :validate, request_options + get :validate, **request_options service_ticket.reload.consumed.should == true end it 'answers with the expected response text' do - get :validate, request_options + get :validate, **request_options response.body.should == response_text_success end end @@ -31,12 +31,12 @@ context 'with a service ticket without issued_from_credentials flag' do it 'consumes the service ticket' do - get :validate, request_options + get :validate, **request_options service_ticket.reload.consumed.should == true end it 'answers with the expected response text' do - get :validate, request_options + get :validate, **request_options response.body.should == response_text_failure end end @@ -47,12 +47,12 @@ end it 'consumes the service ticket' do - get :validate, request_options + get :validate, **request_options service_ticket.reload.consumed.should == true end it 'answers with the expected response text' do - get :validate, request_options + get :validate, **request_options response.body.should == response_text_success end end @@ -65,7 +65,7 @@ end it 'answers with the expected response text' do - get :validate, request_options + get :validate, **request_options response.body.should == response_text_failure end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 0e0fe03b..6ff51e25 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -6,6 +6,7 @@ routes { CASino::Engine.routes } let(:params) { {} } + let(:request_options) { {params: params} } let(:user_agent) { 'YOLOBrowser 420.00' } before(:each) do @@ -22,14 +23,14 @@ let(:params) { { service: service } } it 'renders the service_not_allowed template' do - get :new, params + get :new, **request_options response.should render_template(:service_not_allowed) end end context 'when logged out' do it 'renders the new template' do - get :new, params + get :new, **request_options response.should render_template(:new) end @@ -39,7 +40,7 @@ let(:params) { { service: service, gateway: 'true' } } it 'redirects to the service' do - get :new, params + get :new, **request_options response.should redirect_to(service) end end @@ -48,7 +49,7 @@ let(:params) { { gateway: 'true' } } it 'renders the new template' do - get :new, params + get :new, **request_options response.should render_template(:new) end end @@ -66,7 +67,7 @@ let(:ticket_granting_ticket) { FactoryBot.create :ticket_granting_ticket, :awaiting_two_factor_authentication } it 'renders the new template' do - get :new, params + get :new, **request_options response.should render_template(:new) end end @@ -78,7 +79,7 @@ end it 'renders the new template' do - get :new, params + get :new, **request_options response.should render_template(:new) end end @@ -88,22 +89,24 @@ let(:params) { { service: service } } it 'redirects to the service' do - get :new, params + get :new, **request_options response.location.should =~ /^#{Regexp.escape service}\?ticket=ST-/ end it 'generates a service ticket' do - -> { get :new, params }.should change(CASino::ServiceTicket, :count).by(1) + -> { get :new, **request_options }.should change(CASino::ServiceTicket, :count).by(1) end it 'does not set the issued_from_credentials flag on the service ticket' do - get :new, params + get :new, **request_options CASino::ServiceTicket.last.should_not be_issued_from_credentials end context 'with renew parameter' do + let(:params) { super.merge(renew: 'true') } + it 'renders the new template' do - get :new, params.merge(renew: 'true') + get :new, **request_options response.should render_template(:new) end end @@ -114,7 +117,7 @@ let(:params) { { service: service } } it 'does not remove the attributes' do - get :new, params + get :new, **request_options response.location.should =~ /^#{Regexp.escape service}&ticket=ST-/ end end @@ -124,19 +127,19 @@ let(:params) { { service: service } } it 'redirects to the session overview' do - get :new, params + get :new, **request_options response.should redirect_to(sessions_path) end end context 'without a service' do it 'redirects to the session overview' do - get :new, params + get :new, **request_options response.should redirect_to(sessions_path) end it 'does not generate a service ticket' do - -> { get :new, params }.should change(CASino::ServiceTicket, :count).by(0) + -> { get :new, **request_options }.should change(CASino::ServiceTicket, :count).by(0) end context 'with a changed browser' do @@ -147,7 +150,7 @@ end it 'renders the new template' do - get :new, params + get :new, **request_options response.should render_template(:new) end end @@ -165,7 +168,7 @@ describe 'POST "create"' do context 'without a valid login ticket' do it 'renders the new template' do - post :create, params + post :create, **request_options response.should render_template(:new) end end @@ -175,7 +178,7 @@ let(:params) { { lt: expired_login_ticket.ticket } } it 'renders the new template' do - post :create, params + post :create, **request_options response.should render_template(:new) end end @@ -188,18 +191,18 @@ context 'with invalid credentials' do it 'renders the new template' do - post :create, params + post :create, **request_options response.should render_template(:new) end it 'creates session log' do expect do - post :create, params + post :create, **request_options end.to change { CASino::LoginAttempt.count }.by 1 end it 'assigns session log the correct attributes' do - post :create, params + post :create, **request_options expect(CASino::LoginAttempt.last.user).to eq user expect(CASino::LoginAttempt.last.successful).to eq false @@ -213,24 +216,24 @@ let(:params) { { lt: login_ticket.ticket, username: username, password: 'foobar123', service: service } } it 'creates a cookie' do - post :create, params + post :create, **request_options response.cookies['tgt'].should_not be_nil end it 'saves user_ip' do - post :create, params + post :create, **request_options tgt = CASino::TicketGrantingTicket.last tgt.user_ip.should == '0.0.0.0' end it 'creates session log' do expect do - post :create, params + post :create, **request_options end.to change { CASino::LoginAttempt.count }.by 1 end it 'assigns session log the correct attributes' do - post :create, params + post :create, **request_options expect(CASino::LoginAttempt.last.user.username).to eq username expect(CASino::LoginAttempt.last.successful).to eq true @@ -245,12 +248,12 @@ end it 'creates a cookie with an expiration date set' do - post :create, params + post :create, **request_options cookie_jar['tgt']['expires'].should be_kind_of(Time) end it 'creates a long-term ticket-granting ticket' do - post :create, params + post :create, **request_options tgt = CASino::TicketGrantingTicket.last tgt.long_term.should == true end @@ -261,7 +264,7 @@ let!(:two_factor_authenticator) { FactoryBot.create :two_factor_authenticator, user: user } it 'renders the validate_otp template' do - post :create, params + post :create, **request_options response.should render_template(:validate_otp) end end @@ -273,7 +276,7 @@ let(:service) { 'http://www.example.org/' } it 'renders the service_not_allowed template' do - post :create, params + post :create, **request_options response.should render_template(:service_not_allowed) end end @@ -286,7 +289,7 @@ end it 'renders the new template' do - post :create, params + post :create, **request_options response.should render_template(:new) end end @@ -295,21 +298,21 @@ let(:service) { nil } it 'redirects to the session overview' do - post :create, params + post :create, **request_options response.should redirect_to(sessions_path) end it 'generates a ticket-granting ticket' do - -> { post :create, params }.should change(CASino::TicketGrantingTicket, :count).by(1) + -> { post :create, **request_options }.should change(CASino::TicketGrantingTicket, :count).by(1) end context 'when the user does not exist yet' do it 'generates exactly one user' do - -> { post :create, params }.should change(CASino::User, :count).by(1) + -> { post :create, **request_options }.should change(CASino::User, :count).by(1) end it 'sets the users attributes' do - post :create, params + post :create, **request_options user = CASino::User.last user.username.should == username user.authenticator.should == authenticator @@ -320,12 +323,12 @@ let!(:user) { CASino::User.create! username: username, authenticator: authenticator } it 'does not regenerate the user' do - -> { post :create, params }.should_not change(CASino::User, :count) + -> { post :create, **request_options }.should_not change(CASino::User, :count) end it 'updates the extra attributes' do lambda do - post :create, params + post :create, **request_options user.reload end.should change(user, :extra_attributes) end @@ -336,21 +339,21 @@ let(:service) { 'https://www.example.com' } it 'redirects to the service' do - post :create, params + post :create, **request_options response.location.should =~ /^#{Regexp.escape service}\/\?ticket=ST-/ end it 'generates a service ticket' do - -> { post :create, params }.should change(CASino::ServiceTicket, :count).by(1) + -> { post :create, **request_options }.should change(CASino::ServiceTicket, :count).by(1) end it 'does set the issued_from_credentials flag on the service ticket' do - post :create, params + post :create, **request_options CASino::ServiceTicket.last.should be_issued_from_credentials end it 'generates a ticket-granting ticket' do - -> { post :create, params }.should change(CASino::TicketGrantingTicket, :count).by(1) + -> { post :create, **request_options }.should change(CASino::TicketGrantingTicket, :count).by(1) end end end @@ -376,12 +379,12 @@ end it 'redirects to the service' do - post :validate_otp, params + post :validate_otp, **request_options response.location.should =~ /^#{Regexp.escape service}\?ticket=ST-/ end it 'does activate the ticket-granting ticket' do - post :validate_otp, params + post :validate_otp, **request_options ticket_granting_ticket.reload.should_not be_awaiting_two_factor_authentication end @@ -394,7 +397,7 @@ end it 'creates a cookie with an expiration date set' do - post :validate_otp, params + post :validate_otp, **request_options cookie_jar['tgt']['expires'].should be_kind_of(Time) end end @@ -406,7 +409,7 @@ let(:service) { 'http://www.example.org/' } it 'renders the service_not_allowed template' do - post :validate_otp, params + post :validate_otp, **request_options response.should render_template(:service_not_allowed) end end @@ -418,12 +421,12 @@ end it 'renders the validate_otp template' do - post :validate_otp, params + post :validate_otp, **request_options response.should render_template(:validate_otp) end it 'does not activate the ticket-granting ticket' do - post :validate_otp, params + post :validate_otp, **request_options ticket_granting_ticket.reload.should be_awaiting_two_factor_authentication end end @@ -432,7 +435,7 @@ context 'without a ticket-granting ticket' do it 'redirects to the login page' do - post :validate_otp, params + post :validate_otp, **request_options response.should redirect_to(login_path) end end @@ -450,12 +453,12 @@ end it 'deletes the ticket-granting ticket' do - get :logout, params + get :logout, **request_options CASino::TicketGrantingTicket.where(id: ticket_granting_ticket.id).first.should.nil? end it 'renders the logout template' do - get :logout, params + get :logout, **request_options response.should render_template(:logout) end @@ -463,7 +466,7 @@ let(:url) { 'http://www.example.com' } it 'assigns the URL' do - get :logout, params + get :logout, **request_options assigns(:url).should == url end end @@ -474,7 +477,7 @@ context 'when whitelisted' do it 'redirects to the service' do - get :logout, params + get :logout, **request_options response.should redirect_to(url) end end @@ -485,12 +488,12 @@ end it 'renders the logout template' do - get :logout, params + get :logout, **request_options response.should render_template(:logout) end it 'does not assign the URL' do - get :logout, params + get :logout, **request_options assigns(:url).should be_nil end end @@ -499,7 +502,7 @@ context 'without a ticket-granting ticket' do it 'renders the logout template' do - get :logout, params + get :logout, **request_options response.should render_template(:logout) end end @@ -517,7 +520,7 @@ context 'without a two-factor authenticator registered' do it 'does not assign any two-factor authenticators' do - get :index, params + get :index, **request_options assigns(:two_factor_authenticators).should == [] end end @@ -526,7 +529,7 @@ let!(:two_factor_authenticator) { FactoryBot.create :two_factor_authenticator, :inactive, user: user } it 'does not assign any two-factor authenticators' do - get :index, params + get :index, **request_options assigns(:two_factor_authenticators).should == [] end end @@ -536,7 +539,7 @@ let!(:other_two_factor_authenticator) { FactoryBot.create :two_factor_authenticator } it 'does assign the two-factor authenticator' do - get :index, params + get :index, **request_options assigns(:two_factor_authenticators).should == [two_factor_authenticator] end end @@ -550,7 +553,7 @@ let(:ticket_granting_ticket) { FactoryBot.create :ticket_granting_ticket, user: user } it 'assigns both ticket granting tickets' do - get :index, params + get :index, **request_options assigns(:ticket_granting_tickets).should == [ticket_granting_ticket, other_ticket_granting_ticket] end end @@ -560,7 +563,7 @@ let(:tgt) { ticket_granting_ticket.ticket } it 'does not assign the other ticket granting ticket' do - get :index, params + get :index, **request_options assigns(:ticket_granting_tickets).should == [ticket_granting_ticket] end end @@ -583,7 +586,7 @@ end it 'assigns last five login attempts' do - get :index, params + get :index, **request_options expect(assigns(:login_attempts)).to eq login_attempts.sort_by(&:created_at).from(1).to(6).reverse end @@ -592,7 +595,7 @@ context 'without a ticket-granting ticket' do it 'redirects to the login page' do - get :index, params + get :index, **request_options response.should redirect_to(login_path) end end @@ -613,16 +616,16 @@ let(:params) { { id: ticket_granting_ticket.id } } it 'deletes exactly one ticket-granting ticket' do - -> { delete :destroy, params }.should change(CASino::TicketGrantingTicket, :count).by(-1) + -> { delete :destroy, **request_options }.should change(CASino::TicketGrantingTicket, :count).by(-1) end it 'deletes the ticket-granting ticket' do - delete :destroy, params + delete :destroy, **request_options CASino::TicketGrantingTicket.where(id: params[:id]).length.should == 0 end it 'redirects to the session overview' do - delete :destroy, params + delete :destroy, **request_options response.should redirect_to(sessions_path) end end @@ -630,11 +633,11 @@ context 'with an invalid ticket-granting ticket' do let(:params) { { id: 99_999 } } it 'does not delete a ticket-granting ticket' do - -> { delete :destroy, params }.should_not change(CASino::TicketGrantingTicket, :count) + -> { delete :destroy, **request_options }.should_not change(CASino::TicketGrantingTicket, :count) end it 'redirects to the session overview' do - delete :destroy, params + delete :destroy, **request_options response.should redirect_to(sessions_path) end end @@ -644,11 +647,11 @@ let(:params) { { id: ticket_granting_ticket.id } } it 'does not delete a ticket-granting ticket' do - -> { delete :destroy, params }.should_not change(CASino::TicketGrantingTicket, :count) + -> { delete :destroy, **request_options }.should_not change(CASino::TicketGrantingTicket, :count) end it 'redirects to the session overview' do - delete :destroy, params + delete :destroy, **request_options response.should redirect_to(sessions_path) end end @@ -669,11 +672,11 @@ end it 'deletes all other ticket-granting tickets' do - -> { get :destroy_others, params }.should change(CASino::TicketGrantingTicket, :count).by(-3) + -> { get :destroy_others, **request_options }.should change(CASino::TicketGrantingTicket, :count).by(-3) end it 'redirects to the session overview' do - get :destroy_others, params + get :destroy_others, **request_options response.should redirect_to(sessions_path) end @@ -681,7 +684,7 @@ let(:url) { 'http://www.example.com' } it 'redirects to the service' do - get :destroy_others, params + get :destroy_others, **request_options response.should redirect_to(url) end end @@ -692,7 +695,7 @@ let(:url) { 'http://www.example.com' } it 'redirects to the service' do - get :destroy_others, params + get :destroy_others, **request_options response.should redirect_to(url) end end diff --git a/spec/controllers/two_factor_authenticators_controller_spec.rb b/spec/controllers/two_factor_authenticators_controller_spec.rb index e369b9dd..386bb1d4 100644 --- a/spec/controllers/two_factor_authenticators_controller_spec.rb +++ b/spec/controllers/two_factor_authenticators_controller_spec.rb @@ -4,7 +4,7 @@ routes { CASino::Engine.routes } let(:params) { Hash.new } - let(:request_options) { params } + let(:request_options) { {params: params} } describe 'GET "new"' do context 'with an existing ticket-granting ticket' do @@ -18,22 +18,22 @@ it 'creates exactly one authenticator' do lambda do - get :new, request_options + get :new, **request_options end.should change(CASino::TwoFactorAuthenticator, :count).by(1) end it 'assigns the two_factor_authenticator' do - get :new, request_options + get :new, **request_options assigns(:two_factor_authenticator).should be_kind_of(CASino::TwoFactorAuthenticator) end it 'creates an inactive two-factor authenticator' do - get :new, request_options + get :new, **request_options CASino::TwoFactorAuthenticator.last.should_not be_active end it 'renders the new template' do - get :new, request_options + get :new, **request_options response.should render_template(:new) end @@ -45,7 +45,7 @@ render_views it 'renders the new template' do - get :new, request_options + get :new, **request_options response.should render_template(:new) end end @@ -53,7 +53,7 @@ context 'without a ticket-granting ticket' do it 'redirects to the login page' do - get :new, request_options + get :new, **request_options response.should redirect_to(login_path) end end @@ -81,12 +81,12 @@ end it 'redirects to the two-factor authenticator new page' do - post :create, request_options + post :create, **request_options response.should redirect_to(new_two_factor_authenticator_path) end it 'adds a error message' do - post :create, request_options + post :create, **request_options flash[:error].should == I18n.t('two_factor_authenticators.invalid_two_factor_authenticator') end end @@ -100,7 +100,7 @@ end it 'redirects to the two-factor authenticator new page' do - post :create, request_options + post :create, **request_options response.should redirect_to(new_two_factor_authenticator_path) end end @@ -115,17 +115,17 @@ end it 'redirects to the session overview' do - post :create, request_options + post :create, **request_options response.should redirect_to(sessions_path) end it 'adds a notice' do - post :create, request_options + post :create, **request_options flash[:notice].should == I18n.t('two_factor_authenticators.successfully_activated') end it 'does activate the authenticator' do - post :create, request_options + post :create, **request_options two_factor_authenticator.reload.should be_active end @@ -133,12 +133,12 @@ let!(:other_two_factor_authenticator) { FactoryBot.create :two_factor_authenticator, user: user } it 'does activate the authenticator' do - post :create, request_options + post :create, **request_options two_factor_authenticator.reload.should be_active end it 'does delete the other authenticator' do - post :create, request_options + post :create, **request_options lambda do other_two_factor_authenticator.reload end.should raise_error(ActiveRecord::RecordNotFound) @@ -153,22 +153,22 @@ end it 'rerenders the new page' do - post :create, request_options + post :create, **request_options response.should render_template(:new) end it 'adds a error message' do - post :create, request_options + post :create, **request_options flash[:error].should == I18n.t('two_factor_authenticators.invalid_one_time_password') end it 'assigns the two-factor authenticator' do - post :create, request_options + post :create, **request_options assigns(:two_factor_authenticator).should be_kind_of(CASino::TwoFactorAuthenticator) end it 'does not activate the authenticator' do - post :create, request_options + post :create, **request_options two_factor_authenticator.reload.should_not be_active end end @@ -177,7 +177,7 @@ context 'without a ticket-granting ticket' do it 'redirects to the login page' do - post :create, request_options + post :create, **request_options response.should redirect_to(login_path) end end @@ -198,17 +198,17 @@ let!(:other_two_factor_authenticator) { FactoryBot.create :two_factor_authenticator } it 'redirects to the session overview' do - delete :destroy, request_options + delete :destroy, **request_options response.should redirect_to(sessions_path) end it 'adds a notice' do - delete :destroy, request_options + delete :destroy, **request_options flash[:notice].should == I18n.t('two_factor_authenticators.successfully_deleted') end it 'deletes the two-factor authenticator' do - delete :destroy, request_options + delete :destroy, **request_options lambda do two_factor_authenticator.reload end.should raise_error(ActiveRecord::RecordNotFound) @@ -216,7 +216,7 @@ it 'does not delete other two-factor authenticators' do lambda do - delete :destroy, request_options + delete :destroy, **request_options end.should change(CASino::TwoFactorAuthenticator, :count).by(-1) end end @@ -225,13 +225,13 @@ let!(:two_factor_authenticator) { FactoryBot.create :two_factor_authenticator } it 'redirects to the session overview' do - delete :destroy, request_options + delete :destroy, **request_options response.should redirect_to(sessions_path) end it 'does not delete two-factor authenticators' do lambda do - delete :destroy, request_options + delete :destroy, **request_options end.should_not change(CASino::TwoFactorAuthenticator, :count) end end @@ -241,7 +241,7 @@ let(:params) { { id: 0 } } it 'redirects to the login page' do - delete :destroy, request_options + delete :destroy, **request_options response.should redirect_to(login_path) end end From ef11cfcdc9f000e4813cf40d0bcb005ea357f267 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 22:13:34 -0500 Subject: [PATCH 18/24] Fixing check for empty params - IntegrationTest converts `nil` params to blank strings: https://github.com/rails/rails/issues/28129 --- app/controllers/casino/controller_concern/ticket_validator.rb | 4 ++-- app/controllers/casino/proxy_tickets_controller.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/casino/controller_concern/ticket_validator.rb b/app/controllers/casino/controller_concern/ticket_validator.rb index 53bfbb55..b7155d8c 100644 --- a/app/controllers/casino/controller_concern/ticket_validator.rb +++ b/app/controllers/casino/controller_concern/ticket_validator.rb @@ -7,7 +7,7 @@ def validate_ticket(ticket) validation_result = validate_ticket_for_service(ticket, params[:service], renew: params[:renew]) if validation_result.success? options = { ticket: ticket } - options[:proxy_granting_ticket] = acquire_proxy_granting_ticket(params[:pgtUrl], ticket) unless params[:pgtUrl].nil? + options[:proxy_granting_ticket] = acquire_proxy_granting_ticket(params[:pgtUrl], ticket) if params[:pgtUrl].present? build_ticket_validation_response(true, options) else build_ticket_validation_response(false, @@ -21,7 +21,7 @@ def build_ticket_validation_response(success, options = {}) end def ensure_service_ticket_parameters_present - if params[:ticket].nil? || params[:service].nil? + if params[:ticket].blank? || params[:service].blank? build_ticket_validation_response(false, error_code: 'INVALID_REQUEST', error_message: '"ticket" and "service" parameters are both required') diff --git a/app/controllers/casino/proxy_tickets_controller.rb b/app/controllers/casino/proxy_tickets_controller.rb index 7e95134e..b1a28525 100644 --- a/app/controllers/casino/proxy_tickets_controller.rb +++ b/app/controllers/casino/proxy_tickets_controller.rb @@ -31,7 +31,7 @@ def build_proxy_response(success, options = {}) end def ensure_proxy_parameters_present - if params[:pgt].nil? || params[:targetService].nil? + if params[:pgt].blank? || params[:targetService].blank? build_proxy_response(false, error_code: 'INVALID_REQUEST', error_message: '"pgt" and "targetService" parameters are both required') From e748e1e04ecd0afaa87cdb1ed6eddb578836bf8e Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 22:55:46 -0500 Subject: [PATCH 19/24] Call `super` without implicit arguments --- spec/controllers/sessions_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 6ff51e25..a2e13dd3 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -103,7 +103,7 @@ end context 'with renew parameter' do - let(:params) { super.merge(renew: 'true') } + let(:params) { super().merge(renew: 'true') } it 'renders the new template' do get :new, **request_options From f0c6be174d0c5074db20632e9011a16df3099d74 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 22:56:15 -0500 Subject: [PATCH 20/24] Invert logic --- app/helpers/casino/sessions_helper.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/helpers/casino/sessions_helper.rb b/app/helpers/casino/sessions_helper.rb index 99ddfa29..1077a33c 100644 --- a/app/helpers/casino/sessions_helper.rb +++ b/app/helpers/casino/sessions_helper.rb @@ -26,7 +26,7 @@ def current_user def current_authenticator_context CASino.config.authenticator_context_builder.call(params, request) end - + def ensure_signed_in redirect_to login_path unless signed_in? end @@ -87,12 +87,12 @@ def handle_signed_in(tgt, options = {}) end def handle_signed_in_with_service(tgt, options) - if !service_allowed?(params[:service]) - @service = params[:service] - render 'casino/sessions/service_not_allowed', status: 403 - else + if service_allowed?(params[:service]) url = acquire_service_ticket(tgt, params[:service], options).service_with_ticket_url redirect_to url, status: :see_other + else + @service = params[:service] + render 'casino/sessions/service_not_allowed', status: 403 end end end From 7b20d83f34eaca89bc059633b8544ec33a5ae3f3 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 23:17:07 -0500 Subject: [PATCH 21/24] Fix checks to look for blank values instead of `nil` to address IntegrationTest change AND potential security issues --- app/controllers/casino/sessions_controller.rb | 6 +++--- app/views/casino/sessions/new.html.erb | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/controllers/casino/sessions_controller.rb b/app/controllers/casino/sessions_controller.rb index 471eaff1..a5d3c274 100755 --- a/app/controllers/casino/sessions_controller.rb +++ b/app/controllers/casino/sessions_controller.rb @@ -16,8 +16,8 @@ def index def new tgt = current_ticket_granting_ticket - return handle_signed_in(tgt) unless params[:renew] || tgt.nil? - redirect_to(params[:service]) if params[:gateway] && params[:service].present? + return handle_signed_in(tgt) unless params[:renew].present? || tgt.nil? + redirect_to(params[:service]) if params[:gateway].present? && params[:service].present? end def create @@ -41,7 +41,7 @@ def destroy_others .ticket_granting_tickets .where('id != ?', current_ticket_granting_ticket.id) .destroy_all if signed_in? - redirect_to params[:service] || sessions_path + redirect_to params[:service].present? ? params[:service] : sessions_path end def logout diff --git a/app/views/casino/sessions/new.html.erb b/app/views/casino/sessions/new.html.erb index 71050fc3..30607949 100644 --- a/app/views/casino/sessions/new.html.erb +++ b/app/views/casino/sessions/new.html.erb @@ -14,7 +14,7 @@
<%= form_tag(login_path, method: :post, id: 'login-form') do %> <%= hidden_field_tag :lt, CASino::LoginTicket.create.ticket %> - <%= hidden_field_tag :service, params[:service] unless params[:service].nil? %> + <%= hidden_field_tag :service, params[:service] unless params[:service].blank? %> <%= label_tag :username, t('login.label_username') %> <%= text_field_tag :username, params[:username], autofocus:true %> <%= label_tag :password, t('login.label_password') %> @@ -28,4 +28,3 @@
<%= render 'footer' %> - From 45bc71890f73ce994482f433742ad8ce0f56e0d5 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 23:17:38 -0500 Subject: [PATCH 22/24] Explicitly respond with 406 status because this changed in Rails 5.0 - https://github.com/rails/rails/issues/20666 --- app/controllers/casino/application_controller.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/controllers/casino/application_controller.rb b/app/controllers/casino/application_controller.rb index 151495cc..4f548fc0 100644 --- a/app/controllers/casino/application_controller.rb +++ b/app/controllers/casino/application_controller.rb @@ -5,6 +5,10 @@ class CASino::ApplicationController < ::ApplicationController layout 'application' + rescue_from ActionController::UnknownFormat do + head :not_acceptable + end + unless Rails.env.development? rescue_from ActionView::MissingTemplate, with: :missing_template end From 0781ba74e526ed0a3bf5bd3c74ecf719bcb3f737 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 23:21:33 -0500 Subject: [PATCH 23/24] Drop Rails 4.2 support - mainly because of keyword arguments in RSpec integration tests --- .travis.yml | 11 +++++------ Appraisals | 5 ----- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/.travis.yml b/.travis.yml index 84130e9a..80df3c1a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,5 @@ language: ruby rvm: -- 2.1.0 - 2.2.2 - 2.4.0 - 2.4.1 @@ -9,11 +8,11 @@ gemfile: - gemfiles/rails_4.2.gemfile - gemfiles/rails_5.0.gemfile - gemfiles/rails_5.1.gemfile -matrix: - allow_failures: - - rvm: 2.4.1 - # gemfile: gemfiles/rails_5.0.gemfile - # gemfile: gemfiles/rails_5.1.gemfile +# matrix: +# allow_failures: +# - rvm: 2.4.1 +# # gemfile: gemfiles/rails_5.0.gemfile +# # gemfile: gemfiles/rails_5.1.gemfile notifications: hipchat: rooms: diff --git a/Appraisals b/Appraisals index fc9fd3e4..901c13f3 100644 --- a/Appraisals +++ b/Appraisals @@ -1,8 +1,3 @@ -appraise 'rails-4.2' do - gem 'activerecord', '~> 4.2.0' - gem 'rspec-rails', '>= 3.0' -end - appraise 'rails-5.0' do gem 'activerecord', '~> 5.0.0' gem 'rails-controller-testing' From 3cc12e8b072d4655764df432f21aa72f228e9473 Mon Sep 17 00:00:00 2001 From: Joel Van Horn Date: Wed, 15 Nov 2017 23:25:24 -0500 Subject: [PATCH 24/24] Version bump v5.0.0 (Rails 5.0+ support) --- lib/casino/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/casino/version.rb b/lib/casino/version.rb index 68c0bad2..67c3a443 100644 --- a/lib/casino/version.rb +++ b/lib/casino/version.rb @@ -1,3 +1,3 @@ module CASino - VERSION = '4.2.0'.freeze + VERSION = '5.0.0'.freeze end