:allowed_clock_drift should be bidrectional (allow X sec before "NotBefore" and X sec after "NotOnOrAfter"). Also improves readability of associated error messages.

Author: johnnyshields

lib/onelogin/ruby-saml/response.rb

@@ -337,9 +337,9 @@ def audiences
       end
 
       # returns the allowed clock drift on timing validation
-      # @return [Integer]
+      # @return [Float]
       def allowed_clock_drift
-        return options[:allowed_clock_drift].to_f
+        options[:allowed_clock_drift].to_f.abs
       end
 
       # Checks if the SAML Response contains or not an EncryptedAssertion element
@@ -692,13 +692,13 @@ def validate_conditions
 
         now = Time.now.utc
 
-        if not_before && (now_with_drift = now + allowed_clock_drift) < not_before
-          error_msg = "Current time is earlier than NotBefore condition (#{now_with_drift} < #{not_before})"
+        if not_before && now < (not_before - allowed_clock_drift)
+          error_msg = "Current time is earlier than NotBefore condition (#{now} < #{not_before}#{" - #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})")
           return append_error(error_msg)
         end
 
-        if not_on_or_after && now >= (not_on_or_after_with_drift = not_on_or_after + allowed_clock_drift)
-          error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after_with_drift})"
+        if not_on_or_after && now >= (not_on_or_after + allowed_clock_drift)
+          error_msg = "Current time is on or after NotOnOrAfter condition (#{now} >= #{not_on_or_after}#{" + #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})")
           return append_error(error_msg)
         end
 
@@ -740,7 +740,7 @@ def validate_session_expiration(soft = true)
         return true if session_expires_at.nil?
 
         now = Time.now.utc
-        unless (session_expires_at + allowed_clock_drift) > now
+        unless now < (session_expires_at + allowed_clock_drift)
           error_msg = "The attributes have expired, based on the SessionNotOnOrAfter of the AuthnStatement of this Response"
           return append_error(error_msg)
         end
@@ -778,8 +778,8 @@ def validate_subject_confirmation
 
           attrs = confirmation_data_node.attributes
           next if (attrs.include? "InResponseTo" and attrs['InResponseTo'] != in_response_to) ||
-                  (attrs.include? "NotOnOrAfter" and (parse_time(confirmation_data_node, "NotOnOrAfter") + allowed_clock_drift) <= now) ||
-                  (attrs.include? "NotBefore" and parse_time(confirmation_data_node, "NotBefore") > (now + allowed_clock_drift)) ||
+                  (attrs.include? "NotBefore" and now < (parse_time(confirmation_data_node, "NotBefore") - allowed_clock_drift)) ||
+                  (attrs.include? "NotOnOrAfter" and now >= (parse_time(confirmation_data_node, "NotOnOrAfter") + allowed_clock_drift)) ||
                   (attrs.include? "Recipient" and !options[:skip_recipient_check] and settings and attrs['Recipient'] != settings.assertion_consumer_service_url)
 
           valid_subject_confirmation = true

lib/onelogin/ruby-saml/slo_logoutrequest.rb

@@ -130,6 +130,12 @@ def session_indexes
 
       private
 
+      # returns the allowed clock drift on timing validation
+      # @return [Float]
+      def allowed_clock_drift
+        options[:allowed_clock_drift].to_f.abs
+      end
+
       # Hard aux function to validate the Logout Request
       # @param collect_errors [Boolean] Stop validation when first error appears or keep validating. (if soft=true)
       # @return [Boolean] TRUE if the Logout Request is valid
@@ -187,8 +193,8 @@ def validate_version
       #
       def validate_not_on_or_after
         now = Time.now.utc
-        if not_on_or_after && now >= (not_on_or_after + (options[:allowed_clock_drift] || 0))
-          return append_error("Current time is on or after NotOnOrAfter (#{now} >= #{not_on_or_after})")
+        if not_on_or_after && now >= (not_on_or_after + allowed_clock_drift)
+          return append_error("Current time is on or after NotOnOrAfter (#{now} >= #{not_on_or_after}#{" + #{allowed_clock_drift.ceil}s" if allowed_clock_drift > 0})")
         end
 
         true