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

Allow session serializer key in config.session_store #13692

Merged

Conversation

lukesarnacki
Copy link
Contributor

MessageEncryptor has :serializer option, where any serializer object can be passed.
This commit make it possible to set serializer from configuration level.

There are predefined serializers and custom serializer can be passed
by adding custom class to ActionDispatch::Session.

This was suggested in #12881

module Session
class JsonSerializer
def self.load(value)
JSON.load(value)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't had a chance to help review this in full, but these should be JSON.parse/generate (with quirks_mode) as pointed out in the original discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chancancode good call, thanks ;)

@lukaszx0
Copy link
Member

Wouldn't it be worth to put those serializers under session/serializers directory?

/cc @mattetti @NZKoz @rafaelfranca

@NZKoz
Copy link
Member

NZKoz commented Jan 21, 2014

No strong opinions on the layout, might be nice to simply allow:

  serializer: MyCrazySerializer.new

rather than requiring symbols corresponding to class names, it's more extensible that way.

@mattetti
Copy link
Contributor

I like @koz' suggestion.

On Tue, Jan 21, 2014 at 2:04 PM, Michael Koziarski <notifications@github.com

wrote:

No strong opinions on the layout, might be nice to simply allow:

serializer: MyCrazySerializer.new

rather than requiring symbols corresponding to class names, it's more
extensible that way.


Reply to this email directly or view it on GitHubhttps://github.com//pull/13692#issuecomment-32968788
.

@lukesarnacki
Copy link
Contributor Author

@NZKoz , @mattetti, there are 2 reasons for using symbols:

  1. Some of the classes might not be loaded in config yet.
  2. I just thought that we could set :json_serializer as secure default in generators.

So maybe it would be better to enable both options. If symbol is passed, it will try to find it in ActionDispatch::Session namespace, if something else is passed (so i.e. new serializer class instance) it will just use it. And I can set :json_serializer in generator.

@NZKoz
Copy link
Member

NZKoz commented Jan 22, 2014

@lukesarnacki sounds good to me, it's definitely nice to be able to say :json for built in serializers, but it quickly gets annoying when you have a custom serializer and need to write your own code inside an ActiveSupport::SomeThing name space.

@lukesarnacki
Copy link
Contributor Author

@NZKoz @mattetti I made it possible to pass serializer object and added serializer: :json_serializer in generators. Please let me know if there is anything else that should be changed in order to merge :).

YourApp::Application.config.session_store :cookie_store, key: '_your_app_session', serializer: :json_serializer
```

Default serializer is `:marshal_serializer`. When Symbol or String is passed it will look for appropriate class in `ActionDispatch::Session` namespace, so passing `:my_custom serializer` would load `ActionDispatch::Session::MyCustomSerializer`.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please wrap this around 80 chars please ? We are slowly wrapping new additions even if this is not consistent with the current content.

Also, I think that you forgot an underscore between custom and serializer.

Thanks for your patch so far! :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, thanks for comment, should code also be wrapped or only text?

Copy link
Member

Choose a reason for hiding this comment

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

If you speak about the current examples, even if they are a bit a long, I think that they fit. In a general case, I would say that wrapping would be required but here, config options can be long so this should be ok. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I think it is ok now, thanks!

@lukesarnacki
Copy link
Contributor Author

@mattetti, @robin850, do you have any other suggestions? :)

@@ -41,6 +41,18 @@

*Alessandro Diaferia*

* Add :serializer option for config.session_store :cookie_store. This
Copy link
Member

Choose a reason for hiding this comment

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

This is really a nitpick, but I would add backticks and blank lines to make it a bit more readable, like this:

Add a `:serializer` option for `config.session_store :cookie_store`. This
changes default serializer when using `:cookie_store` to
`ActionDispatch::Session::MarshalSerializer` which is a wrapper around Marshal.

It is also possible to pass:

* `:json_serializer` which is secure wrapper on JSON using parse / generate API.
* any other Symbol or String like `:my_custom_serializer` which will be
camelized and constantized in `ActionDispatch::Session namespace`.
* serializer object with load / dump methods defined.

*Łukasz Sarnacki*

What do you think ? Apart from that, it looks great! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks better, thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robin850 I've just pushed corrected formatting, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

What about backticks ? I really think that this improve the reading but let me know if you don't think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, missed that :( I don't know why, but i had some strange belief that changelog is text file... I've just pushed code with backticks, thanks for your patience! ;)

Copy link
Member

Choose a reason for hiding this comment

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

@mattetti : By credit, do you mean your name in the changelog ? If it's the case, we can merge it as is and I can credit you in another commit to avoid Travis from running another build. What do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattetti Oh, sure!! So rude of me I haven't mentioned you, sorry. @robin850 sounds good to me (travis will hate me for my pushes, I think I made like millions of them). As this was partially code from your gist, maybe it would be worth to map this commit to us both (I belive there is such option but I am newbie to rails contributions, so not sure if this is a way to do it).

Copy link
Member

Choose a reason for hiding this comment

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

@mattetti of course you deserves credit for this. I'm adding now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattetti again, sorry about forgetting about you, I feel bad about this, but at least it is fixed now ;).

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries at all, no offense taken, I actually hesitated to say anything :)

On Wed, Jan 29, 2014 at 11:12 AM, Łukasz Sarnacki
notifications@github.comwrote:

In actionpack/CHANGELOG.md:

@@ -41,6 +41,18 @@

 *Alessandro Diaferia*

+* Add :serializer option for config.session_store :cookie_store. This

@mattetti https://github.com/mattetti again, sorry about forgetting
about you, I feel bad about this, but at least it is fixed now ;).


Reply to this email directly or view it on GitHubhttps://github.com//pull/13692/files#r9279557
.

MessageEncryptor has :serializer option, where any serializer object can
be passed. This commit make it possible to set this serializer from configuration
level.

There are predefined serializers (:marshal_serializer, :json_serialzier)
and custom serializer can be passed as String, Symbol (camelized and
constantized in ActionDispatch::Session namepspace) or serializer object.

Default :json_serializer was also added to generators to provide secure
defalt.
@guilleiguaran
Copy link
Member

Looks great 👍

guilleiguaran added a commit that referenced this pull request Jan 29, 2014
…rializer

Allow session serializer key in config.session_store
@guilleiguaran guilleiguaran merged commit b242552 into rails:master Jan 29, 2014
@lukaszx0
Copy link
Member

Great work @lukesarnacki ! Thanks @mattetti and @NZKoz for helping out. 👍

@mattetti
Copy link
Contributor

Łukasz sounds good to me.

On Wed, Jan 29, 2014 at 10:49 AM, Guillermo Iguaran <
notifications@github.com> wrote:

Merged #13692 #13692.


Reply to this email directly or view it on GitHubhttps://github.com//pull/13692
.

@lukesarnacki
Copy link
Contributor Author

@robin850 please add credits for @mattetti as you said, thanks! @mattetti thanks for helping with this, it wouldn't be done without you ❤️

@robin850 Also as I wrote in outdated diff, could it be mapped to both me and @mattetti ?

@rafaelfranca
Copy link
Member

Very good. Thank you @lukesarnacki

@lukesarnacki lukesarnacki deleted the change-default-session-serializer branch Jan 29, 2014
@robin850
Copy link
Member

Thank you guys! ❤️

@lukesarnacki : Guillermo added credit for @mattetti in 0f15610. However, since the commit is now merged, we can't amend the commit to map both your names, I'm sorry. 😢

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

Successfully merging this pull request may close these issues.

None yet

8 participants