Improve security - #8
Conversation
wozza35
left a comment
There was a problem hiding this comment.
Looks good!
Minor comments
| return root unless magic_token.present? | ||
|
|
||
| cookies.signed[magic_token_key] = magic_token.token if scope | ||
| expiry = MagicLinks.magic_token_cookie_expiry&.from_now |
There was a problem hiding this comment.
Could extract this line into a cookie_expires_at method for clarity maybe?
There was a problem hiding this comment.
Also, I could be wrong, but according to the docs expires: can take a duration. So maybe the from_now call isn't even needed
There was a problem hiding this comment.
Yes looks like it accepts a duration!
| environments. | ||
|
|
||
| ### Magic token cookie expiry | ||
| By default, the magic token cookie expiry (not to be confused with the expiry of the magic link) is set to 1 hour. You |
There was a problem hiding this comment.
Might be worth explaining what the cookie does for people who aren't familiar with the gem?
'A magic token cookie is dropped on the client when a magic link is used. This cookie is used to authenticate subsequent requests. By default...'
| expect(request.cookie_jar.encrypted[:user_magic_token]).to eq magic_token.token | ||
| end | ||
|
|
||
| context 'when cookie expiry is set to nil' do |
There was a problem hiding this comment.
Is the to nil bit meant to be there?
| # Therefore we need only test that our middleware sets the cookie via ActionDispatch | ||
| Rack::MockResponse.new(*subject.call(request.env)) | ||
| expect(request.cookie_jar.signed[:user_magic_token]).to eq magic_token.token | ||
| expect(request.cookie_jar.encrypted[:user_magic_token]).to eq magic_token.token |
There was a problem hiding this comment.
There are some tests that still include the word 'signed' cookie. Change to 'encrypted'?
cookies.encryptedinstead ofcookies.signedso cookie cannot be read