Skip to content

Conversation

@yumitsu
Copy link
Contributor

@yumitsu yumitsu commented Jan 24, 2018

Good day. I found that Authenticate doesn't support signed and encrypted cookies yet, so I decided to implement it.
I will be glad if you'll accept PR and maybe even bump gem version. Thanks.

@tomichj
Copy link
Owner

tomichj commented Feb 21, 2018

Hello @yumitsu, thanks for the contribution, this is a great start!

I'm still reading through the PR and considering the change. I do see one potential issue that probably needs some refinement. Authenticate Configuration options are provided for both encrypted and signed cookies, but Session#get_cookies_class defers to @cookies.signed_or_encrypted if either Configuration#signed_cookie or Configuration#encrypted_cookie is true. But signed_or_encrypted will always call encrypted if the secret key base is present.

I believe this means that both Authenticate configuration options, Authenticate.configuration.signed and Authenticate.configuration.encrypted, will result in an encrypted cookie being generated if the app's secret key base is preset, or a signed cookie being generated if the secret key base is not present.

I think the solution is for Session (or it's delegate) to avoid calling the cookie_jar.signed_or_encrypted, and instead have Session call either cookie_jar.encrypted or cookie_jar.signed depending on the configuration and presence of the secret key base in the request.

@yumitsu
Copy link
Contributor Author

yumitsu commented Feb 26, 2018

@tomichj Thanks for your review. Yes, I agree with you, this PR need some changes.

@yumitsu
Copy link
Contributor Author

yumitsu commented Feb 28, 2018

@tomichj Please look at new changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants