Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookie expiration #5042

Closed
wants to merge 1 commit into from
Closed

Conversation

bmmcginty
Copy link
Contributor

@bmmcginty bmmcginty commented Sep 26, 2017

As mentioned in #5033, this PR adds consistent handling for the expires parameter for cookies. It changes #expires to #expiration_time and allows #expires to act as a property for the expires parameter. It also adds #max_age to allow access to the max-age parameter. Finally, documentation has been added for #expiration_time.

delta.should be > 9.seconds
delta.should be < 11.seconds
delta = cookie.max_age
delta.should eq 10.seconds
Copy link
Contributor

@Sija Sija Sep 26, 2017

Choose a reason for hiding this comment

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

you could skip delta intermediary variable altogether since there's one call only.

delta.should be > 0.seconds
delta.should be < 1.seconds
delta = cookie.max_age
delta.should eq 0.seconds
Copy link
Contributor

@Sija Sija Sep 26, 2017

Choose a reason for hiding this comment

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

ditto

@@ -42,9 +47,29 @@ module HTTP
"#{@name}=#{URI.escape value}"
end

# Returns the `Time` at which this cookie will expire, or `nil` if it will not expire.
# Uses *max-age* and *expires* values to ccalculate the time.
Copy link
Contributor

@Sija Sija Sep 26, 2017

Choose a reason for hiding this comment

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

typo: ccalculate


describe "expiration_time" do
it "by max-age=0" do
c = parse_set_cookie("bla=1; max-age=0")
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

c -> cookie plz

end

it "by old date" do
c = parse_set_cookie("bla=1; expires=Thu, 01 Jan 1970 00:00:00 -0000")
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

ditto

# Returns the `Time` at which this cookie will expire, or `nil` if it will not expire.
# Uses *max-age* and *expires* values to calculate the time.
def expiration_time
ma = @max_age
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

ma -> max_age

# Uses *max-age* and *expires* values to calculate the time.
def expiration_time
ma = @max_age
ex = @expires
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

ditto

def expired?
if e = expires
e < Time.now
ma = @max_age
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

ditto

if e = expires
e < Time.now
ma = @max_age
ex = @expires
Copy link
Contributor

@Sija Sija Sep 27, 2017

Choose a reason for hiding this comment

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

ditto

parse_time(match["expires"]?)
end
expires = parse_time(match["expires"]?)
max_age = match["max_age"]? ? match["max_age"].to_i.seconds : nil
Copy link
Contributor

@Sija Sija Oct 5, 2017

Choose a reason for hiding this comment

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

max_age = match["max_age"]?.try(&.to_i.seconds)

Copy link
Member

@straight-shoota straight-shoota Oct 10, 2017

Choose a reason for hiding this comment

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

This suggestion seems better than the current code.

describe "expiration_time" do
it "by max-age=0" do
cookie = parse_set_cookie("bla=1; max-age=0")
(cookie.expiration_time.not_nil! <= (Time.now + 1.seconds)).should eq true
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

eq true should be written as be_true in all specs.
And the tested value is probably better understandable as Time.now - cookie.expiration_time <= 1.seconds

end

it "not expired" do
(parse_set_cookie("bla=1; max-age=1").expiration_time.not_nil! > Time.now).should eq true
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

Please use a cookie variable like in the other examples. This makes it better readable.
The same goes for the following specs.

cookie.expiration_time.should eq Time.new(1970, 1, 1, 0, 0, 0)
end

it "not expired" do
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

It would be great to add more detail to the spec name, e.g. with max-age

(parse_set_cookie("bla=1; max-age=1").expiration_time.not_nil! > Time.now).should eq true
end

it "not expired" do
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

It would be great to add more detail to the spec name, e.g. with expires

(parse_set_cookie("bla=1; expires=Thu, 01 Jan 2020 00:00:00 -0000").expiration_time.not_nil! >= Time.new(2020, 1, 1, 0, 0, 0)).should eq true
end

it "not expired" do
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

It would be great to add more detail to the spec name, e.g. without expiration

end

it "not expired" do
parse_set_cookie("bla=1").expiration_time.should eq nil
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

eq nil => be_nil

property domain : String?
property secure : Bool
property http_only : Bool
property extension : String?
property creation_time : Time
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

I don't think it makes sense to have creation_time writable, and I'm not sure if it should even be readable externally.

To have a customizeable time reference, I would rather add an optional argument to expiration_time and expired?. It could be called current_time or time_reference. That would be a better API, because you don't need to change the Cookie's internal data to check its expiration at a certain point of time.

Copy link
Contributor Author

@bmmcginty bmmcginty Oct 9, 2017

Choose a reason for hiding this comment

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

I think I set this up as you described, though I'm happy to rewrite. I added tests for using reference_time in specs as well, though the spec naming may be too verbose.

Copy link
Member

@straight-shoota straight-shoota Oct 9, 2017

Choose a reason for hiding this comment

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

Looks good!

However on a second thought, it would probably make sense to have creation_time readable just to be transparent about it. And I'd also suggest to add creation_time as an optional named parameter to initialize. This allows for time-independent testing as well as (re-)creating a Cookie instance which was retrieved at a different point in time, maybe for serialization.
I think it makes generally sense to allow all references to Time.new to be replaced with an optional parameter. So maybe even expired? should get a second, optional named parameter...

# Returns the `Time` at which this cookie will expire, or `nil` if it will not expire.
# Uses *max-age* and *expires* values to calculate the time.
def expiration_time
max_age = @max_age
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

I'd suggest to move the variable assignment into the condition.
The elsif branch is unnecessary because @expires is either nil or Time and this is what this method returns.

if max_age = @max_age
  creation_time + max_age
else
  @expires
end

def expired?
if e = expires
e < Time.now
max_age = @max_age
Copy link
Member

@straight-shoota straight-shoota Oct 6, 2017

Choose a reason for hiding this comment

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

This method has duplicated code from expiration_time and should use that method instead. This way it becomes very simple:

@max_age == 0.seconds || expiration_time.try &.<(Time.now) || false

elsif expires
expires
# By default, this function uses the creation time of this cookie as the offset for max-age, if max-age is set.
# To use a different offset, provide a `Time` object to *time_Reference*.
Copy link
Member

@straight-shoota straight-shoota Oct 9, 2017

Choose a reason for hiding this comment

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

Just a small typo: time_reference

describe "expiration_time" do
it "sets expiration_time to be current when max-age=0" do
cookie = parse_set_cookie("bla=1; max-age=0")
(Time.now - cookie.expiration_time.not_nil! <= 1.seconds).should be_true
Copy link
Member

@straight-shoota straight-shoota Oct 9, 2017

Choose a reason for hiding this comment

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

This could even be written (Time.now - cookie.expiration_time.not_nil!).should be.<= 1.seconds

I just learned about this ;)

describe "expiration_time" do
it "sets expiration_time to be current when max-age=0" do
cookie = parse_set_cookie("bla=1; max-age=0")
(Time.now - cookie.expiration_time.not_nil!).should be.<= 1.seconds
Copy link
Member

@straight-shoota straight-shoota Oct 10, 2017

Choose a reason for hiding this comment

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

I'm really sorry that I my suggestion was a bit inprecise. 😢 I don't know why I assumed the period to be necessary...

be <= 1.seconds works as well and reads much better.

Copy link
Contributor Author

@bmmcginty bmmcginty Oct 16, 2017

Choose a reason for hiding this comment

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

Will fix. No problem; I could have looked through more docs myself.

@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Oct 10, 2017

Great work! Unfortunately, I was misleading you about the proper syntax for comparison expectations. Sorry about that :/

domain = @domain
String.build do |header|
header << "#{URI.escape @name}=#{URI.escape value}"
header << "; domain=#{domain}" if domain
header << "; path=#{path}" if path
header << "; expires=#{HTTP.rfc1123_date(expires)}" if expires
header << "; max-age=#{max_age.total_seconds}" if max_age
Copy link
Contributor

@wontruefree wontruefree Jun 12, 2018

Choose a reason for hiding this comment

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

I think + is preferred in string concatenation.
before:

header << "; max-age=#{max_age.total_seconds}" if max_age

after:

header << "; max-age=" + max_age.total_seconds if max_age

Copy link
Contributor

@wontruefree wontruefree Jun 12, 2018

Choose a reason for hiding this comment

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

Seems like the rest of the headers are in the other style so this is not a change is not necessary with this PR.

Copy link
Member

@straight-shoota straight-shoota Jun 13, 2018

Choose a reason for hiding this comment

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

No, + is discouraged. Maybe << would be slightly faster, but interpolation is totally fine.

Copy link
Contributor

@wontruefree wontruefree Jun 13, 2018

Choose a reason for hiding this comment

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

In that case this is good.

@wontruefree
Copy link
Contributor

@wontruefree wontruefree commented Jun 12, 2018

@bmmcginty what is the status on this PR? I am just curious it looks pretty good to me.

@bmmcginty bmmcginty force-pushed the cookie_expiration branch 2 times, most recently from 74f9181 to e2e9b28 Compare Jun 15, 2018
end
end

def expired?(time_reference = @creation_time)
@max_age == 0.seconds || expiration_time(time_reference).try &.<(Time.now) || false
Copy link
Contributor

@RX14 RX14 Jun 15, 2018

Choose a reason for hiding this comment

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

Why || false?

I'd prefer

time = expiration_time(time_reference)
@max_age == 0.seconds || (time && time < Time.now)

Copy link
Member

@straight-shoota straight-shoota Jun 15, 2018

Choose a reason for hiding this comment

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

@max_age == 0.seconds || (time = expiration_time(time_reference) && time < Time.now)

Avoids calculation of expiration_time if the first condition is already true.

header << "; expires=#{HTTP.rfc1123_date(expires)}" if expires
header << "; max-age=#{max_age.total_seconds}" if max_age
header << "; expires=#{HTTP.format_time(expires)}" if expires
xheader << "; max-age=#{max_age.total_seconds}" if max_age
Copy link
Contributor

@Sija Sija Jun 15, 2018

Choose a reason for hiding this comment

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

xheader?

@bmmcginty
Copy link
Contributor Author

@bmmcginty bmmcginty commented Jun 15, 2018

Copy link
Contributor

@RX14 RX14 left a comment

And spec failed

end
end

# returns the expiration status of this cookie as a `Bool` given a creation time
Copy link
Contributor

@RX14 RX14 Jun 19, 2018

Choose a reason for hiding this comment

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

Returns true if this cookie is expired.
Uses max-age and expires values to calculate the time.
By default, this function uses the creation time of this cookie as the offset for max-age, if max-age is set.
To use a different offset, provide a Time object to time_reference.

@bmmcginty
Copy link
Contributor Author

@bmmcginty bmmcginty commented Jun 19, 2018

I'd like to say this has been rebased to match the master branch. At this point, though, I haven't a clue.

@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jun 19, 2018

It seems like you lost the specs and some code in cookie.cr at some point during rebasing.

@Sija
Copy link
Contributor

@Sija Sija commented Jun 19, 2018

@bmmcginty ac6a53e could be a recovery starting point.

@straight-shoota straight-shoota added the pr:needs-work label Feb 15, 2019
@RX14 RX14 closed this Jul 16, 2019
@straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jul 24, 2019

Closed in favour of #7925

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change kind:feature pr:needs-work topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants