Fixes #13557 - Rubocop enforce specifying a timezone

Rubocop can enforce what timezone to store in the database , so we can ensure everything is stored using UTC and we don't miss these things in code reviews. When objects are displayed, they must use the time provided by set_timezone in the controller. This is particularly relevant for Trends, Puppet graphs, etc... to ensure they are stored always properly
theforeman · Feb 4, 2016 · 191e062 · 191e062
1 parent c6d1f78
commit 191e062
Show file tree

Hide file tree

Showing 26 changed files with 65 additions and 61 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -143,11 +143,6 @@ Rails/PluralizationGrammar:
 Rails/ReadWriteAttribute:
   Enabled: false
 
-# Offense count: 48
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-Rails/TimeZone:
-  Enabled: false
-
 # Offense count: 2
 # Cop supports --auto-correct.
 # Configuration parameters: EnforcedStyle, SupportedStyles.

diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
@@ -21,14 +21,14 @@ def status
 
   # check for exception - set the result code and duration time
   def exception_watch(&block)
-    start = Time.now
+    start = Time.now.utc
     result = {}
     begin
       yield
       result[:result] = 'ok'
       result[:status] = :ok
       result[:version] = SETTINGS[:version].full
-      result[:db_duration_ms] = ((Time.now - start) * 1000).round.to_s
+      result[:db_duration_ms] = ((Time.now.utc - start) * 1000).round.to_s
     rescue => e
       result[:result] = 'fail'
       result[:status] = :internal_server_error

diff --git a/app/helpers/trends_helper.rb b/app/helpers/trends_helper.rb
@@ -27,7 +27,7 @@ def trend_title(trend)
     end
   end
 
-  def chart_data(trend, from = Setting.max_trend, to = Time.zone.now)
+  def chart_data(trend, from = Setting.max_trend, to = Time.now.utc)
     chart_colors = ['#4572A7','#AA4643','#89A54E','#80699B','#3D96AE','#DB843D','#92A8CD','#A47D7C','#B5CA92']
     values = trend.values
     labels = {}
@@ -39,7 +39,7 @@ def chart_data(trend, from = Setting.max_trend, to = Time.zone.now)
       value.trend_counters.each do |counter|
         #cut the left side of the graph
         interval_start = (counter.interval_start || from) > from ? counter.interval_start : from
-        next_timestamp = counter.try(:interval_end) || Time.now
+        next_timestamp = counter.try(:interval_end) || Time.now.utc
         #transform the timestamp values to flot format - from seconds in Ruby to milliseconds in flot
         data << [interval_start.to_i*1000, counter.count]
         data << [next_timestamp.to_i*1000 - 1, counter.count]

diff --git a/app/models/concerns/fog_extensions/openstack/server.rb b/app/models/concerns/fog_extensions/openstack/server.rb
@@ -39,7 +39,7 @@ def flavor_with_object
       end
 
       def created_at
-        Time.parse attributes['created']
+        Time.parse(attributes['created']).utc
       end
 
       # the original method requires a server ID, however we want to be able to call this method on new instances too

diff --git a/app/models/concerns/user_time.rb b/app/models/concerns/user_time.rb
@@ -0,0 +1,12 @@
+module UserTime
+  extend ActiveSupport::Concern
+
+  included do
+    validate :validate_timezone
+    attr_accessible :timezone
+
+    def validate_timezone
+      errors.add(:timezone, _("is not valid")) unless timezone.blank? || Time.find_zone(timezone)
+    end
+  end
+end
diff --git a/app/models/host/managed.rb b/app/models/host/managed.rb
@@ -350,7 +350,7 @@ def error_count
   end
 
   def no_report
-    last_report.nil? or last_report < Time.now - (Setting[:puppet_interval] + Setting[:outofsync_interval]).minutes and enabled?
+    last_report.nil? || last_report < Time.now.utc - (Setting[:puppet_interval] + Setting[:outofsync_interval]).minutes and enabled?
   end
 
   def disabled?

diff --git a/app/models/host_status/configuration_status.rb b/app/models/host_status/configuration_status.rb
@@ -17,7 +17,7 @@ def out_of_sync?
       if (host && !host.enabled?) || no_reports?
         false
       else
-        !reported_at.nil? && reported_at < (Time.now - (Setting[:puppet_interval] + Setting[:outofsync_interval]).minutes)
+        !reported_at.nil? && reported_at < (Time.now.utc - (Setting[:puppet_interval] + Setting[:outofsync_interval]).minutes)
       end
     end
 
@@ -105,7 +105,7 @@ def handle_options(options)
     end
 
     def update_timestamp
-      self.reported_at = last_report.try(:reported_at) || Time.now
+      self.reported_at = last_report.try(:reported_at) || Time.now.utc
     end
 
     def calculator

diff --git a/app/models/host_status/status.rb b/app/models/host_status/status.rb
@@ -57,7 +57,7 @@ def relevant?(options = {})
     private
 
     def update_timestamp
-      self.reported_at = Time.now
+      self.reported_at = Time.now.utc
     end
 
     def update_status

diff --git a/app/models/report.rb b/app/models/report.rb
@@ -78,7 +78,7 @@ def self.expire(conditions = {})
     Message.where("id not IN (#{Log.unscoped.select('DISTINCT message_id').to_sql})").delete_all
     Source.where("id not IN (#{Log.unscoped.select('DISTINCT source_id').to_sql})").delete_all
     count = where(cond).delete_all
-    logger.info Time.now.to_s + ": Expired #{count} #{to_s.underscore.humanize.pluralize}"
+    logger.info Time.now.utc.to_s + ": Expired #{count} #{to_s.underscore.humanize.pluralize}"
     count
   end
 

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -8,6 +8,7 @@ class User < ActiveRecord::Base
   include Foreman::ThreadSession::UserModel
   include Taxonomix
   include DirtyAssociations
+  include UserTime
   audited :except => [:last_login_on, :password, :password_hash, :password_salt, :password_confirmation], :allow_mass_assignment => true
 
   ANONYMOUS_ADMIN = 'foreman_admin'
@@ -41,7 +42,7 @@ class User < ActiveRecord::Base
 
   accepts_nested_attributes_for :user_mail_notifications, :allow_destroy => true, :reject_if => :reject_empty_intervals
   attr_accessible :password, :password_confirmation, :login, :firstname,
-    :lastname, :mail, :locale, :timezone, :mail_enabled,
+    :lastname, :mail, :locale, :mail_enabled,
     :default_location_id, :default_organization_id,
     :auth_source_name, :auth_source, :auth_source_id,
     :mail_notification_ids, :mail_notification_names,
@@ -94,7 +95,7 @@ def self.name_format
   validates :firstname, :lastname, :format => {:with => name_format}, :length => {:maximum => 50}, :allow_nil => true
   validate :name_used_in_a_usergroup, :ensure_hidden_users_are_not_renamed, :ensure_hidden_users_remain_admin,
            :ensure_privileges_not_escalated, :default_organization_inclusion, :default_location_inclusion,
-           :ensure_last_admin_remains_admin, :hidden_authsource_restricted, :validate_timezone, :ensure_admin_password_changed_by_admin
+           :ensure_last_admin_remains_admin, :hidden_authsource_restricted, :ensure_admin_password_changed_by_admin
   before_validation :prepare_password, :normalize_mail
   before_save       :set_lower_login
 
@@ -404,7 +405,7 @@ def external_usergroups
 
   def prepare_password
     unless password.blank?
-      self.password_salt = Digest::SHA1.hexdigest([Time.now, rand].join)
+      self.password_salt = Digest::SHA1.hexdigest([Time.now.utc, rand].join)
       self.password_hash = encrypt_password(password)
     end
   end
@@ -552,8 +553,4 @@ def hidden_authsource_restricted
       errors.add :auth_source, _("is not permitted")
     end
   end
-
-  def validate_timezone
-    errors.add(:timezone, _("is not valid")) unless timezone.blank? || Time.find_zone(timezone)
-  end
 end
diff --git a/app/services/orchestration/task.rb b/app/services/orchestration/task.rb
@@ -30,7 +30,7 @@ def as_json(options = {})
     private
 
     def update_ts
-      @timestamp = Time.now
+      @timestamp = Time.now.utc
     end
 
     # sort based on priority

diff --git a/app/services/report_importer.rb b/app/services/report_importer.rb
@@ -34,12 +34,12 @@ def initialize(raw, proxy_id = nil)
   end
 
   def import
-    start_time = Time.now
+    start_time = Time.now.utc
     logger.info "processing report for #{name}"
     logger.debug { "Report: #{raw.inspect}" }
     create_report_and_logs
     if report.persisted?
-      logger.info("Imported report for #{name} in #{(Time.now - start_time).round(2)} seconds")
+      logger.info("Imported report for #{name} in #{(Time.now.utc - start_time).round(2)} seconds")
       host.refresh_statuses
     end
   end

diff --git a/app/services/smart_proxies/puppet_ca_certificate.rb b/app/services/smart_proxies/puppet_ca_certificate.rb
@@ -5,8 +5,8 @@ class SmartProxies::PuppetCACertificate
 
   def initialize(opts)
     @name, @state, @fingerprint, @valid_from, @expires_at, @status_object = opts.flatten
-    @valid_from = Time.parse(@valid_from) unless @valid_from.blank?
-    @expires_at = Time.parse(@expires_at) unless @expires_at.blank?
+    @valid_from = Time.parse(@valid_from).utc unless @valid_from.blank?
+    @expires_at = Time.parse(@expires_at).utc unless @expires_at.blank?
   end
 
   def sign

diff --git a/app/services/trend_importer.rb b/app/services/trend_importer.rb
@@ -21,7 +21,7 @@ def check_values
   end
 
   def update_trend_counters
-    timestamp = Time.now
+    timestamp = Time.now.utc
     counter_hash = {}
     Trend.types.each do |trend|
       if trend.is_a? FactTrend

diff --git a/db/migrate/20150622090115_change_reported_at.rb b/db/migrate/20150622090115_change_reported_at.rb
@@ -1,6 +1,6 @@
 class ChangeReportedAt < ActiveRecord::Migration
   def up
-    Report.where(:reported_at => nil).update_all(:reported_at => Time.at(0))
+    Report.where(:reported_at => nil).update_all(:reported_at => Time.at(0).utc)
     change_column :reports, :reported_at, :datetime, :null => false
   end
 

diff --git a/lib/timed_cached_store.rb b/lib/timed_cached_store.rb
@@ -16,7 +16,7 @@ def read(name)
 
   def write(name, value, options = nil)
     if options and options[:expires_in]
-      time                  = Time.now
+      time                  = Time.now.utc
       ttl                   = time + options[:expires_in].to_i
       @data[ts_field(name)] = { :created_at => time, :expires_at => ttl }
     end
@@ -42,7 +42,7 @@ def delete_if_expired(name)
 
   def expired?(name)
     ts = @data[ts_field(name)]
-    ts and (Time.now >= ts[:expires_at])
+    ts and (Time.now.utc >= ts[:expires_at])
   rescue
     false
   end

diff --git a/test/functional/api/v2/hostgroups_controller_test.rb b/test/functional/api/v2/hostgroups_controller_test.rb
@@ -39,7 +39,7 @@ class Api::V2::HostgroupsControllerTest < ActionController::TestCase
 
   test "should clone hostgroup" do
     assert_difference('Hostgroup.count') do
-      post :clone, { :id => hostgroups(:common).to_param, :name => Time.now.to_s }
+      post :clone, { :id => hostgroups(:common).to_param, :name => Time.now.utc.to_s }
     end
     assert_response :success
   end

diff --git a/test/functional/api/v2/tasks_controller_test.rb b/test/functional/api/v2/tasks_controller_test.rb
@@ -27,6 +27,6 @@ class Api::V2::TasksControllerTest < ActionController::TestCase
     assert_equal 'create something', task['name']
     assert_equal 'pending', task['status']
     assert_equal 10, task['priority']
-    assert Time.now - Time.parse(task['timestamp']) < 5.seconds
+    assert Time.now.utc - Time.parse(task['timestamp']).utc < 5.seconds
   end
 end
diff --git a/test/functional/unattended_controller_test.rb b/test/functional/unattended_controller_test.rb
@@ -245,15 +245,15 @@ class UnattendedControllerTest < ActionController::TestCase
     Setting[:token_duration] = 30
     Setting[:unattended_url]    = "https://test.host"
     @request.env["REMOTE_ADDR"] = @ub_host.ip
-    @ub_host.create_token(:value => token, :expires => Time.now + 5.minutes)
+    @ub_host.create_token(:value => token, :expires => Time.now.utc + 5.minutes)
     get :host_template, {:kind => 'provision', 'token' => @ub_host.token.value }
     assert @response.body.include?("#{Setting[:unattended_url]}/unattended/finish?token=#{token}")
   end
 
   test "hosts with unknown ip and valid token should render a template" do
     Setting[:token_duration] = 30
     @request.env["REMOTE_ADDR"] = '127.0.0.1'
-    @ub_host.create_token(:value => "aaaaaa", :expires => Time.now + 5.minutes)
+    @ub_host.create_token(:value => "aaaaaa", :expires => Time.now.utc + 5.minutes)
     get :host_template, {:kind => 'provision', 'token' => @ub_host.token.value }
     assert_response :success
   end
@@ -275,7 +275,7 @@ class UnattendedControllerTest < ActionController::TestCase
       Setting[:update_ip_from_built_request] = false
       @request.env["REMOTE_ADDR"] = '127.0.0.1'
       h=@ub_host
-      h.create_token(:value => "aaaaaa", :expires => Time.now + 5.minutes)
+      h.create_token(:value => "aaaaaa", :expires => Time.now.utc + 5.minutes)
       get :built, {'token' => h.token.value }
       h_new=Host.find_by_name(h.name)
 
@@ -291,7 +291,7 @@ class UnattendedControllerTest < ActionController::TestCase
       new_ip = h.subnet.network.gsub(/\.0$/,'.100') # Must be in the subnet, which isn't fixed
       @request.env["REMOTE_ADDR"] = new_ip
       refute_equal new_ip, h.ip
-      h.create_token(:value => "aaaaab", :expires => Time.now + 5.minutes)
+      h.create_token(:value => "aaaaab", :expires => Time.now.utc + 5.minutes)
       get :built, {'token' => h.token.value }
       h_new=Host.find_by_name(h.reload.name)
       assert_response :success
@@ -304,7 +304,7 @@ class UnattendedControllerTest < ActionController::TestCase
       Setting[:update_ip_from_built_request] = true
       @request.env["REMOTE_ADDR"] = @rh_host.ip
       h=@ub_host
-      h.create_token(:value => "aaaaac", :expires => Time.now + 5.minutes)
+      h.create_token(:value => "aaaaac", :expires => Time.now.utc + 5.minutes)
       get :built, {'token' => h.token.value }
       assert_response :success
       h_new=Host.find_by_name(h.reload.name)
@@ -316,7 +316,7 @@ class UnattendedControllerTest < ActionController::TestCase
       Setting[:token_duration]    = 30
       Setting[:unattended_url]    = "http://test.host"
       @request.env["REMOTE_ADDR"] = @ub_host.ip
-      @ub_host.create_token(:value => "aaaaaa", :expires => Time.now + 5.minutes)
+      @ub_host.create_token(:value => "aaaaaa", :expires => Time.now.utc + 5.minutes)
       get :host_template, {:kind => 'provision'}
       assert @response.body.include?("http://test.host/unattended/finish?token=aaaaaa")
     end
@@ -326,7 +326,7 @@ class UnattendedControllerTest < ActionController::TestCase
     template_server_from_proxy = 'https://someproxy:8443'
     ProxyAPI::Template.any_instance.stubs(:template_url).returns(template_server_from_proxy)
     @request.env["REMOTE_ADDR"] = '127.0.0.1'
-    @host_with_template_subnet.create_token(:value => "aaaaad", :expires => Time.now + 5.minutes)
+    @host_with_template_subnet.create_token(:value => "aaaaad", :expires => Time.now.utc + 5.minutes)
     get :host_template, {:kind => 'provision', 'token' => @host_with_template_subnet.token.value }
     assert @response.body.include?("#{template_server_from_proxy}/unattended/finish?token=aaaaad")
   end
@@ -335,7 +335,7 @@ class UnattendedControllerTest < ActionController::TestCase
     Setting[:token_duration] = 30
     Setting[:unattended_url]    = "http://test.host"
     @request.env["REMOTE_ADDR"] = '127.0.0.1'
-    @host_with_template_subnet.create_token(:value => "aaaaae", :expires => Time.now + 5.minutes)
+    @host_with_template_subnet.create_token(:value => "aaaaae", :expires => Time.now.utc + 5.minutes)
     get :host_template, {:kind => 'provision', 'token' => @host_with_template_subnet.token.value }
     assert @response.body.include?("#{@host_with_template_subnet.subnet.tftp.url}/unattended/finish?token=aaaaae")
   end

diff --git a/test/lib/tasks/trends_test.rb b/test/lib/tasks/trends_test.rb
@@ -180,7 +180,7 @@ class TrendsTest < ActiveSupport::TestCase
 
   def create_trend_line(trend, values_line)
     point_dates = []
-    point_date = Time.now.beginning_of_day
+    point_date = Time.now.utc.beginning_of_day
     values_line.each do |value|
       point_dates << point_date
       FactoryGirl.create(:trend_counter, :trend => trend, :created_at => point_date, :updated_at => point_date, :count => value)

diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -73,19 +73,19 @@ class ActiveSupport::TestCase
 
     DEFERRED_GC_THRESHOLD = (ENV['DEFER_GC'] || 1.0).to_f
 
-    @@last_gc_run = Time.now
+    @@last_gc_run = Time.now.getlocal
 
     def begin_gc_deferment
       GC.disable if DEFERRED_GC_THRESHOLD > 0
     end
 
     def reconsider_gc_deferment
-      if DEFERRED_GC_THRESHOLD > 0 && Time.now - @@last_gc_run >= DEFERRED_GC_THRESHOLD
+      if DEFERRED_GC_THRESHOLD > 0 && Time.now.getlocal - @@last_gc_run >= DEFERRED_GC_THRESHOLD
         GC.enable
         GC.start
         GC.disable
 
-        @@last_gc_run = Time.now
+        @@last_gc_run = Time.now.getlocal
       end
     end
 

diff --git a/test/unit/host_mailer_test.rb b/test/unit/host_mailer_test.rb
@@ -6,7 +6,7 @@ def setup
     @env = environments(:production)
     @host = FactoryGirl.create(:host, :environment => @env)
     as_admin do
-      @host.last_report = Time.at(0)
+      @host.last_report = Time.at(0).utc
       @host.save(:validate => false)
       @env.hosts << @host
       @env.save

diff --git a/test/unit/host_status/configuration_status_test.rb b/test/unit/host_status/configuration_status_test.rb
@@ -54,7 +54,7 @@ def setup
     @status.refresh!
     assert @status.reported_at.present?
     window = (Setting[:puppet_interval] + Setting[:outofsync_interval]).minutes
-    assert @status.reported_at < Time.now - window
+    assert @status.reported_at < Time.now.utc - window
 
     assert @status.out_of_sync?
   end
@@ -65,7 +65,7 @@ def setup
   end
 
   test '#out_of_sync? is false when window is big enough' do
-    original, Setting[:outofsync_interval] = Setting[:outofsync_interval], (Time.now - @report.reported_at).to_i / 60 + 1
+    original, Setting[:outofsync_interval] = Setting[:outofsync_interval], (Time.now.utc - @report.reported_at).to_i / 60 + 1
     refute @status.out_of_sync?
     Setting[:outofsync_interval] = original
   end