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

Fix bug where session cookie was not always written at the right time. #1160

Merged
merged 5 commits into from
May 31, 2020
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
44 changes: 0 additions & 44 deletions spec/lucky/flash_handler_spec.cr

This file was deleted.

90 changes: 0 additions & 90 deletions spec/lucky/session_handler_spec.cr

This file was deleted.

223 changes: 181 additions & 42 deletions spec/lucky/text_response_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -4,60 +4,194 @@ include ContextHelper

describe Lucky::TextResponse do
describe "#print" do
it "uses the default status if none is set" do
context = build_context
print_response(context, status: nil)
context.response.status_code.should eq Lucky::TextResponse::DEFAULT_STATUS
end
context "flash" do
it "writes the flash to the session" do
context = build_context
context.flash.success = "Yay!"
flash_json = {success: "Yay!"}.to_json

it "uses the passed in status" do
context = build_context
print_response(context, status: 300)
context.response.status_code.should eq 300
end
print_response_with_body(context)

context.session.get(Lucky::FlashStore::SESSION_KEY).should eq(flash_json)
end

it "only keeps the flash for one request" do
context1 = build_context
now_json = {success: "Yay!"}.to_json
context1.session.set(Lucky::FlashStore::SESSION_KEY, now_json)
next_json = context1.flash.to_json

context1.flash.success.should eq("Yay!")

print_response_with_body(context1)

context2 = build_context
context2.session.set(Lucky::FlashStore::SESSION_KEY, next_json)

context2.flash.success.should eq("")
end

it "keeps the flash for the next request" do
context1 = build_context
context1.flash.success = "Yay!"
next_json = context1.flash.to_json

print_response_with_body(context1)

it "uses the response status if it's set, and Lucky::TextResponse status is nil" do
context = build_context
context.response.status_code = 300
print_response(context, status: nil)
context.response.status_code.should eq 300
context2 = build_context
context2.session.set(Lucky::FlashStore::SESSION_KEY, next_json)

context2.flash.success.should eq("Yay!")
end
end

it "prints no body with a head call" do
context = build_context("HEAD")
print_response_with_body(context, "Body", status: nil)
context.request.method.should eq "HEAD"
context.request.body.to_s.should eq ""
context.response.status_code.should eq 200
context "cookies" do
it "sets a cookie" do
context = build_context
context.cookies.set(:email, "test@example.com")

print_response_with_body(context)

context.response.headers.has_key?("Set-Cookie").should be_true
context.response.headers["Set-Cookie"].should contain("email=")
end

it "persist cookies across multiple requests using response headers from Lucky and request headers from the browser" do
context_1 = build_context
context_1.cookies.set(:email, "test@example.com")

print_response_with_body(context_1)

browser_request = build_request
cookie_header = context_1.response.cookies.map do |cookie|
cookie.to_cookie_header
end.join(", ")
browser_request.headers.add("Cookie", cookie_header)
context_2 = build_context("/", request: browser_request)

context_2.cookies.get(:email).should eq "test@example.com"
end

it "only writes updated cookies to the response" do
request = build_request
# set initial cookies via header
request.headers.add("Cookie", "cookie1=value1; cookie2=value2")
context = build_context("/", request: request)
context.cookies.set_raw(:cookie2, "updated2")

print_response_with_body(context)

context.response.headers["Set-Cookie"].should contain("cookie2=updated2")
context.response.headers["Set-Cookie"].should_not contain("cookie1")
end

it "sets a session" do
context = build_context
context.session.set(:email, "test@example.com")

print_response_with_body(context)

context.response.headers.has_key?("Set-Cookie").should be_true
context.response.headers["Set-Cookie"].should contain("_app_session")
end

it "persists the session across multiple requests" do
context_1 = build_context
context_1.session.set(:email, "test@example.com")

print_response_with_body(context_1)

request = build_request
cookie_header = context_1.response.cookies.map do |cookie|
cookie.to_cookie_header
end.join("; ")
request.headers.add("Cookie", cookie_header)
context_2 = build_context("/", request: request)
print_response_with_body(context_2)

context_2.session.get(:email).should eq("test@example.com")
end

it "writes all the proper headers when a cookie is set" do
context = build_context
context
.cookies
.set(:yo, "lo")
.path("/awesome")
.expires(Time.utc(2000, 1, 1))
.domain("luckyframework.org")
.secure(true)
.http_only(true)

print_response_with_body(context)

header = context.response.headers["Set-Cookie"]
header.should contain("path=/awesome")
header.should contain("expires=Sat, 01 Jan 2000")
header.should contain("domain=luckyframework.org")
header.should contain("Secure")
header.should contain("HttpOnly")
end
end

it "gzips if enabled" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"
context "status" do
it "uses the default status if none is set" do
context = build_context
print_response(context, status: nil)
context.response.status_code.should eq Lucky::TextResponse::DEFAULT_STATUS
end

print_response_with_body(context, status: 200, body: "some body")
context.response.close
it "uses the passed in status" do
context = build_context
print_response(context, status: 300)
context.response.status_code.should eq 300
end

context.response.headers["Content-Encoding"].should eq "gzip"
expected_io = IO::Memory.new
Gzip::Writer.open(expected_io) { |gzw| gzw.print "some body" }
output.to_s.ends_with?(expected_io.to_s).should be_true
it "uses the response status if it's set, and Lucky::TextResponse status is nil" do
context = build_context
context.response.status_code = 300
print_response(context, status: nil)
context.response.status_code.should eq 300
end

it "prints no body with a head call" do
context = build_context("HEAD")
print_response_with_body(context, "Body", status: nil)
context.request.method.should eq "HEAD"
context.request.body.to_s.should eq ""
context.response.status_code.should eq 200
end
end

it "doesn't gzip when content type isn't in Lucky::Server.gzip_content_types" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"
context "compression" do
it "gzips if enabled" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"

print_response_with_body(context, status: 200, body: "some body")
context.response.close

context.response.headers["Content-Encoding"].should eq "gzip"
expected_io = IO::Memory.new
Gzip::Writer.open(expected_io) { |gzw| gzw.print "some body" }
output.to_s.ends_with?(expected_io.to_s).should be_true
end
end

it "doesn't gzip when content type isn't in Lucky::Server.gzip_content_types" do
Lucky::Server.temp_config(gzip_enabled: true) do
output = IO::Memory.new
context = build_context_with_io(output)
context.request.headers["Accept-Encoding"] = "gzip"

print_response_with_body(context, status: 200, body: "some body", content_type: "foo/bar")
context.response.close
print_response_with_body(context, status: 200, body: "some body", content_type: "foo/bar")
context.response.close

context.response.headers["Content-Encoding"]?.should_not eq "gzip"
output.to_s.ends_with?("some body").should be_true
context.response.headers["Content-Encoding"]?.should_not eq "gzip"
output.to_s.ends_with?("some body").should be_true
end
end
end
end
Expand All @@ -67,6 +201,11 @@ private def print_response(context : HTTP::Server::Context, status : Int32?)
print_response_with_body(context, "", status)
end

private def print_response_with_body(context : HTTP::Server::Context, body = "", status = 200, content_type = "text/html")
private def print_response_with_body(
context : HTTP::Server::Context,
Copy link
Member

Choose a reason for hiding this comment

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

Did the formatter do this? I ask because the one below has the first arg start on the same line as the signature. I'm thinking maybe we make them the same (one way or the other). I was curious if the formatter allowed both, or if it prefers one vs the other way. Does that make sense?

Copy link
Contributor Author

@wout wout May 29, 2020

Choose a reason for hiding this comment

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

I tend to put the first argument on a new line if everything does not fit on the same line. But the formatter does respect that and will put all following arguments on the same indentation level.

I like it because that will consistently put all multi-line arguments on the same indentation throughout the file. Otherwise, they are hanging at different places, sometimes with odd-numbered indentation levels, depending on the length of the name of the method.

Here again, if you prefer the first argument at the same line as the method, I'll change that. ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, I have been moving a lot of code around there, because that's where I initially added the changes. But then I realised it should be somewhere else. That's why it has been changed.

Copy link
Member

Choose a reason for hiding this comment

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

I think this look good 👍

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be cleaner indeed, so I added those changes as well.

body = "",
status = 200,
content_type = "text/html"
)
Lucky::TextResponse.new(context, content_type, body, status: status).print
end
2 changes: 1 addition & 1 deletion src/lucky/cookies/session.cr
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class Lucky::Session
end

def destroy
{% raise "CookieJar#destroy has been renamed to CookieJar#clear to match Hash#clear" %}
{% raise "Session#destroy has been renamed to Session#clear to match Hash#clear" %}
end

def reset
Expand Down
Loading