From 7a00e5696bb76e68267c15738fae3e3965bbb7c6 Mon Sep 17 00:00:00 2001 From: Gregor Schmidt Date: Thu, 5 Jan 2017 10:05:59 +0100 Subject: [PATCH] #10840 Define token action properties explicitly Tokens differ in validity and whether or not multiple tokens may exist at the same time. These differences have been buried in different parts of the code base. Reusing the token table in plugins is difficult, since one cannot simply define the new tokens properties without monkey-patching multiple Token methods. By explicitly defining properties and differences of the existing core token actions, we are now make the Token methods more generic, which makes it easer for plugin authors to extend the list of tokens. Following #10840 - allow "stay logged in" from multiple browsers - the max number of autologin tokens has been raised to 10. Also fixing the following bugs: * Using action related validity_time when deleting expired tokens --- app/models/token.rb | 60 +++++++++++++++++++++++++++++++++++++++++++++---- test/unit/token_test.rb | 43 +++++++++++++++++++++++------------ 2 files changed, 85 insertions(+), 18 deletions(-) diff --git a/app/models/token.rb b/app/models/token.rb index a5ca18a..7966220 100644 --- a/app/models/token.rb +++ b/app/models/token.rb @@ -25,18 +25,70 @@ class Token < ActiveRecord::Base cattr_accessor :validity_time self.validity_time = 1.day + class << self + attr_reader :actions + + def add_action(name, options) + options.assert_valid_keys(:max_instances, :validity_time) + @actions ||= {} + @actions[name.to_s] = options + end + end + + add_action :api, max_instances: 1, validity_time: nil + add_action :autologin, max_instances: 10, validity_time: Proc.new { Setting.autologin.to_i.days } + add_action :feeds, max_instances: 1, validity_time: nil + add_action :recovery, max_instances: 1, validity_time: Proc.new { Token.validity_time } + add_action :register, max_instances: 1, validity_time: Proc.new { Token.validity_time } + add_action :session, max_instances: 10, validity_time: nil + def generate_new_token self.value = Token.generate_token_value end # Return true if token has expired def expired? - return Time.now > self.created_on + self.class.validity_time + return created_on < self.class.invalid_when_created_before(action) + end + + def max_instances + Token.actions.has_key?(action) ? Token.actions[action][:max_instances] : 1 + end + + def self.invalid_when_created_before(action = nil) + if Token.actions.has_key?(action) + validity_time = Token.actions[action][:validity_time] + validity_time = validity_time.call(action) if validity_time.respond_to? :call + else + validity_time = self.validity_time + end + + if validity_time.nil? + 0 + else + Time.now - validity_time + end end # Delete all expired tokens def self.destroy_expired - Token.where("action NOT IN (?) AND created_on < ?", ['feeds', 'api', 'session'], Time.now - validity_time).delete_all + t = Token.arel_table + + # Unknown actions have default validity_time + condition = t[:action].not_in(self.actions.keys).and(t[:created_on].lt(invalid_when_created_before)) + + self.actions.each do |action, options| + validity_time = invalid_when_created_before(action) + + # Do not delete tokens, which don't become invalid + next if validity_time.nil? + + condition = condition.or( + t[:action].eq(action).and(t[:created_on].lt(validity_time)) + ) + end + + Token.where(condition).delete_all end # Returns the active user who owns the key for the given action @@ -80,8 +132,8 @@ class Token < ActiveRecord::Base def delete_previous_tokens if user scope = Token.where(:user_id => user.id, :action => action) - if action == 'session' - ids = scope.order(:updated_on => :desc).offset(9).ids + if max_instances > 1 + ids = scope.order(:updated_on => :desc).offset(max_instances - 1).ids if ids.any? Token.delete(ids) end diff --git a/test/unit/token_test.rb b/test/unit/token_test.rb index 92a7f12..f25fcfd 100644 --- a/test/unit/token_test.rb +++ b/test/unit/token_test.rb @@ -29,31 +29,34 @@ class TokenTest < ActiveSupport::TestCase def test_create_should_remove_existing_tokens user = User.find(1) - t1 = Token.create(:user => user, :action => 'autologin') - t2 = Token.create(:user => user, :action => 'autologin') + t1 = Token.create(:user => user, :action => 'register') + t2 = Token.create(:user => user, :action => 'register') assert_not_equal t1.value, t2.value assert !Token.exists?(t1.id) assert Token.exists?(t2.id) end - def test_create_session_token_should_keep_last_10_tokens + def test_create_session_or_autologin_token_should_keep_last_10_tokens Token.delete_all user = User.find(1) - assert_difference 'Token.count', 10 do - 10.times { Token.create!(:user => user, :action => 'session') } - end + ["autologin", "session"].each do |action| + assert_difference 'Token.count', 10 do + 10.times { Token.create!(:user => user, :action => action) } + end - assert_no_difference 'Token.count' do - Token.create!(:user => user, :action => 'session') + assert_no_difference 'Token.count' do + Token.create!(:user => user, :action => action) + end end end - def test_destroy_expired_should_not_destroy_feeds_and_api_tokens + def test_destroy_expired_should_not_destroy_session_feeds_and_api_tokens Token.delete_all Token.create!(:user_id => 1, :action => 'api', :created_on => 7.days.ago) Token.create!(:user_id => 1, :action => 'feeds', :created_on => 7.days.ago) + Token.create!(:user_id => 1, :action => 'session', :created_on => 7.days.ago) assert_no_difference 'Token.count' do assert_equal 0, Token.destroy_expired @@ -63,12 +66,24 @@ class TokenTest < ActiveSupport::TestCase def test_destroy_expired_should_destroy_expired_tokens Token.delete_all - Token.create!(:user_id => 1, :action => 'autologin', :created_on => 7.days.ago) - Token.create!(:user_id => 2, :action => 'autologin', :created_on => 3.days.ago) - Token.create!(:user_id => 3, :action => 'autologin', :created_on => 1.hour.ago) + # Expiration of autologin tokens is determined by Setting.autologin + Setting.autologin = "7" + Token.create!(:user_id => 2, :action => 'autologin', :created_on => 3.weeks.ago) + Token.create!(:user_id => 3, :action => 'autologin', :created_on => 3.days.ago) + + # Expiration of register and recovery tokens is determined by Token.validity_time + Token.create!(:user_id => 1, :action => 'register', :created_on => 7.days.ago) + Token.create!(:user_id => 3, :action => 'register', :created_on => 7.hours.ago) + + Token.create!(:user_id => 2, :action => 'recovery', :created_on => 3.days.ago) + Token.create!(:user_id => 3, :action => 'recovery', :created_on => 3.hours.ago) + + # Expiration of tokens with unknown action is determined by Token.validity_time + Token.create!(:user_id => 2, :action => 'unknown_action', :created_on => 2.days.ago) + Token.create!(:user_id => 3, :action => 'unknown_action', :created_on => 2.hours.ago) - assert_difference 'Token.count', -2 do - assert_equal 2, Token.destroy_expired + assert_difference 'Token.count', -4 do + assert_equal 4, Token.destroy_expired end end -- 2.9.2