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.

Sign up for GitHub

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

Jump to bottom

Fix ActionController::TestSession#id to return Rack::Session::SessionId instance #38063

Merged
merged 3 commits into from Jan 10, 2020
+8 −2
Merged

Fix ActionController::TestSession#id to return Rack::Session::SessionId instance #38063

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1bff494
Fix ActionController::TestSession#id to return Rack::Session::Session…
abcang Dec 21, 2019
ace8e1a
test SessionId#public_id
abcang Jan 6, 2020
9d075d6
test session["session_id"]
abcang Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions actionpack/lib/action_controller/test_case.rb
View file
Open in desktop
Expand Up @@ -176,12 +176,12 @@ class LiveTestResponse < Live::Response

# Methods #destroy and #load! are overridden to avoid calling methods on the
# @store object, which does not exist for the TestSession class.
class TestSession < Rack::Session::Abstract::SessionHash #:nodoc:
class TestSession < Rack::Session::Abstract::PersistedSecure::SecureSessionHash #:nodoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why the superclass has to change. Can you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rack::Session::Abstract::PersistedSecure uses Rack::Session::Abstract::PersistedSecure::SecureSessionHash instead of Rack::Session::Abstract::SessionHash. Therefore, I changed the super class of TestSession to use SecureSessionHash as well.
The difference between SessionHash and SecureSessionHash is the behavior when accessing ["session_id"]. In the case of SecureSessionHash, the public_id of @id is returned.

https://github.com/rack/rack/blob/7fecaee81f59926b6e1913511c90650e76673b38/lib/rack/session/abstract/id.rb#L461-L463

DEFAULT_OPTIONS = Rack::Session::Abstract::Persisted::DEFAULT_OPTIONS

def initialize(session = {})
super(nil, nil)
@id = SecureRandom.hex(16)
@id = Rack::Session::SessionId.new(SecureRandom.hex(16))
@data = stringify_keys(session)
@loaded = true
end
Expand Down
5 changes: 5 additions & 0 deletions actionpack/test/dispatch/session/test_session_test.rb
View file
Open in desktop
Expand Up @@ -62,4 +62,9 @@ def test_fetch_returns_block_value
session = ActionController::TestSession.new(one: "1")
assert_equal(2, session.fetch("2") { |key| key.to_i })
end

def test_session_id
session = ActionController::TestSession.new
assert_instance_of String, session.id.public_id
Copy link
Member

Choose a reason for hiding this comment

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

Should we test session["session_id"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added test for session["session_id"]

end
end