Added Signed Cookie and optionally used it in Session - Other Speedups #148
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added signed cookie made session take advantage of signed or encrypted cookies. only save code on changed? conditional added comment with store options
elorest
changed the title
drujensen
approved these changes
Jul 26, 2017
src/amber/controller/base.cr
Outdated
|
|
||
| # protected getter flash : Amber::Router::Flash::FlashStore? | ||
| # protected getter cookies : Amber::Router::Cookies::Store? | ||
| # protected getter session : Amber::Router::Session::AbstractStore? |
There was a problem hiding this comment.
Yeah I should definitely cut that.
src/amber/router/cookies.cr
Outdated
| raise Exceptions::CookieOverflow.new if cookie.value.bytesize > MAX_COOKIE_SIZE | ||
| @store[name] = cookie | ||
| end | ||
|
|
||
| private def decrypt_and_verify(encrypted_message) | ||
| private def verify_and_decrypt(encrypted_message) | ||
| String.new(@encryptor.decrypt_and_verify(encrypted_message)) |
There was a problem hiding this comment.
kinda weird that the method is called verify_and_decrypt and then it calls @encryptor.decrypt_and_verify
src/amber/router/cookies.cr
Outdated
| String.new(@encryptor.decrypt_and_verify(encrypted_message)) | ||
| rescue e | ||
| "{\"value\":\"\"}" | ||
| rescue e # TODO: This should probably actually raise the exception instead of rescueing from it. |
There was a problem hiding this comment.
You think I should remove the TODO?
src/amber/router/session.cr
Outdated
| @@ -24,7 +24,7 @@ module Amber::Router | |||
| end | |||
|
|
|||
| def cookie | |||
| Session::CookieStore.new(cookies, session[:key].to_s, session[:expires].to_i, session[:secret].to_s) | |||
| Session::CookieStore.new((session[:store] == :encrypted_cookie) ? cookies.encrypted : cookies.signed, session[:key].to_s, session[:expires].to_i, session[:secret].to_s, ) | |||
| @@ -25,15 +26,15 @@ module Amber::Controller | |||
| end | |||
|
|
|||
| def cookies | |||
| @cookies ||= context.cookies | |||
| context.cookies | |||
fridgerator
approved these changes
Jul 27, 2017
|
issue #141 |
marksiemers
pushed a commit
to marksiemers/amber
that referenced
this pull request
Oct 28, 2017
amberframework#148) * ran a bunch of tests, fixed some issues and cleaned up the code. added signed cookie made session take advantage of signed or encrypted cookies. only save code on changed? conditional added comment with store options * added test to prove that cookies with no signature return empty values * the deep magic before the dawn of time. * cleaned up store initialization at @drujensen's request * added delete and mark as changed when it delete
elorest
added a commit
that referenced
this pull request
Nov 17, 2017
#148) * ran a bunch of tests, fixed some issues and cleaned up the code. added signed cookie made session take advantage of signed or encrypted cookies. only save code on changed? conditional added comment with store options * added test to prove that cookies with no signature return empty values * the deep magic before the dawn of time. * cleaned up store initialization at @drujensen's request * added delete and mark as changed when it delete
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
Added Signed Cookies. Small cleanups and speed optimizations while staying true to @eliasjpr vision for cookies and session stores. 😓
:store => [:signed_cookie, :encrypted_cookie, :redis]Alternate Designs
Currently different types of cookie store are passed into session cookie store which then handles everything exactly the same. This could be broken out into a completely new session cookie store but literally every line would be duplicate (NOT DRY). I think this way is a better approach. In the future it would probably be better to configure the Session Pipe with session settings instead of the Server it's self.
Why Should This Be In Core?
Sessions are important. This keeps all of the improvements from @eliasjpr session while bringing the speed really close to what it was with the older session.
Benefits
Here are benchmarks. Index sets cookie and show page reads it. Text rendered is
index => "session set"andshow => "session works and is good!"Benchmarks are shown in order of fastest to slowest.
No pipes enabled
Old session without flash
Modified New Signed Session without Flash
Modified New Encrypted Session without Flash
Modified New Signed Session with Flash
Old Session and Flash
New Encryped Session without Flash
Modified New Encryption Session with Flash
New Encrypted Session With Flash