Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions lib/onelogin/ruby-saml/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,21 @@ class Utils

DSIG = "http://www.w3.org/2000/09/xmldsig#"
XENC = "http://www.w3.org/2001/04/xmlenc#"
DURATION_FORMAT = %r(^(-?)P(?:(?:(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?)?)|(?:(\d+)W))$)
DURATION_FORMAT = %r(^
(-?)P # 1: Duration sign
(?:
(?:(\d+)Y)? # 2: Years
(?:(\d+)M)? # 3: Months
(?:(\d+)D)? # 4: Days
(?:T
(?:(\d+)H)? # 5: Hours
(?:(\d+)M)? # 6: Minutes
(?:(\d+(?:[.,]\d+)?)S)? # 7: Seconds
)?
|
(\d+)W # 8: Weeks
)
$)x
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this x?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the "ignore whitespace" regexp flag. It's what allows me to split the regexp over multiple lines, and also insert comments. You can see it here documented for 2.6, but it's supported back to 1.8.x:

https://ruby-doc.org/core-2.6.4/Regexp.html#class-Regexp-label-Options


# Checks if the x509 cert provided is expired
#
Expand All @@ -37,31 +51,18 @@ def self.is_cert_expired(cert)
# current time.
#
# @return [Integer] The new timestamp, after the duration is applied.
#
#
def self.parse_duration(duration, timestamp=Time.now.utc)
matches = duration.match(DURATION_FORMAT)

if matches.nil?
raise Exception.new("Invalid ISO 8601 duration")
end

durYears = matches[2].to_i
durMonths = matches[3].to_i
durDays = matches[4].to_i
durHours = matches[5].to_i
durMinutes = matches[6].to_i
durSeconds = matches[7].to_f
durWeeks = matches[8].to_i

if matches[1] == "-"
durYears = -durYears
durMonths = -durMonths
durDays = -durDays
durHours = -durHours
durMinutes = -durMinutes
durSeconds = -durSeconds
durWeeks = -durWeeks
end
sign = matches[1] == '-' ? -1 : 1

durYears, durMonths, durDays, durHours, durMinutes, durSeconds, durWeeks =
matches[2..8].map { |match| match ? sign * match.tr(',', '.').to_f : 0.0 }

initial_datetime = Time.at(timestamp).utc.to_datetime
final_datetime = initial_datetime.next_year(durYears)
Expand Down
39 changes: 39 additions & 0 deletions test/utils_test.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,45 @@
require File.expand_path(File.join(File.dirname(__FILE__), "test_helper"))

class UtilsTest < Minitest::Test
describe ".parse_duration" do
DURATIONS_FROM_EPOCH = {
# Basic formats
"P1Y1M1D" => "1971-02-02T00:00:00.000Z",
"PT1H1M1S" => "1970-01-01T01:01:01.000Z",
"P1W" => "1970-01-08T00:00:00.000Z",
"P1Y1M1DT1H1M1S" => "1971-02-02T01:01:01.000Z",

# Negative duration
"-P1Y1M1DT1H1M1S" => "1968-11-29T22:58:59.000Z",
Copy link
Copy Markdown
Collaborator

@johnnyshields johnnyshields Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are negative values supported for each unit, e.g. P1Y-1M == 11 months?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain that negative durations are part of the spec, as it mentions them only once, to say:

2.1.6
duration
non-negative quantity attributed to a time interval

From conversations in the moment.js repo (1, 2) it appears that this is an area where implementations vary, some supporting prefix signs, some supporting infix.

For the purposes of this PR, I'm just trying to cover the existing code, which supports a prefix sign alone.

Copy link
Copy Markdown
Collaborator

@pitbulk pitbulk Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some info about the xsd:duration type: http://www.datypic.com/sc/xsd/t-xsd_duration.html
It seems infix should not be valid.

The numbers may be any unsigned integer, with the exception of the number of seconds, which may be an unsigned decimal number. So P15.5Y seems invalid

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, cool, thanks for the extra info!

As a result of the earlier conversations I've permitted decimal hours and minutes; would you like me to back that change out, since the xsd:duration spec is tighter than the ISO one?

Copy link
Copy Markdown
Collaborator

@pitbulk pitbulk Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, and sorry for providing late this info.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, glad to have it resolved! I've updated the branch and referred to the xsd:duration docs in the commit message.


# Nominal wraparounds
"P13M" => "1971-02-01T00:00:00.000Z",
"P31D" => "1970-02-01T00:00:00.000Z",

# Decimal seconds
"PT0.5S" => "1970-01-01T00:00:00.500Z",
"PT0,5S" => "1970-01-01T00:00:00.500Z"
}

def result(duration, reference = 0)
Time.at(
OneLogin::RubySaml::Utils.parse_duration(duration, reference)
).utc.iso8601(3)
end

DURATIONS_FROM_EPOCH.each do |duration, expected|
it "parses #{duration} to return #{expected} from the given timestamp" do
assert_equal expected, result(duration)
end
end

it "returns the last calendar day of the next month when advancing from a longer month to a shorter one" do
initial_timestamp = Time.iso8601("1970-01-31T00:00:00.000Z").to_i

assert_equal "1970-02-28T00:00:00.000Z", result("P1M", initial_timestamp)
end
end

describe ".format_cert" do
let(:formatted_certificate) {read_certificate("formatted_certificate")}
let(:formatted_chained_certificate) {read_certificate("formatted_chained_certificate")}
Expand Down