Custom serializers and deserializers in MessageVerifier and MessageEncryptor. #3031

Merged
merged 2 commits into from Sep 15, 2011

Conversation

Projects
None yet
3 participants
Contributor

wvanbergen commented Sep 15, 2011

By default, these ActiveSupport classes use Marshal for serializing and deserializing messages. Unfortunately, the Marshal format is closely associated with Ruby internals. This makes the resulting message very hard to unserialize messages generated by these classes in other environments like node.js.

This patch solves this by allowing you to set your own custom serializer and deserializer lambda functions. By default, it still uses Marshal to be backwards compatible. Simple example:

verifier = ActiveSupport::MessageVerifier.new("Hey, I'm a secret!")
verifier.serializer = lambda { |value| ActiveSupport::JSON.encode(value) }
verifier.deserializer = lambda { |value| ActiveSupport::JSON.decode(value) }

message = verifier.generate({ :foo => 123, 'bar' => Time.local(2010) })
p verifier.verify(message) # => { "foo" => 123, "bar" => "2010-01-01T00:00:00-05:00" }

Note that based on the serializer and deserializer, it may not be possible to do a lossless roundtrip as seen in the example. That might not be a problem depending on your needs, but using Marshal as the default makes sense so users don't have to worry about that.

The way I implemented it may not be optimal, so I welcome any feedback!

wvanbergen added some commits Sep 15, 2011

@wvanbergen wvanbergen Custom serializers and deserializers in MessageVerifier and MessageEn…
…cryptor.

By default, these classes use Marshal for serializing and deserializing messages. Unfortunately, the Marshal format is closely associated with Ruby internals and even changes between different interpreters. This makes the resulting message very hard to impossible to unserialize messages generated by these classes in other environments like node.js.

This patch solves this by allowing you to set your own custom serializer and deserializer lambda functions. By default, it still uses Marshal to be backwards compatible.
bffaa88
@wvanbergen wvanbergen Fixed tests so that they will also run properly in other timezones. a8aaef6

josevalim merged commit 06c3ca8 into rails:master Sep 15, 2011

Member

jonleighton commented Sep 15, 2011

Hey, I know this has already been merged now, but I think a better API would be:

ActiveSupport::MessageVerifier.new("Hey, I'm a secret!", serializer)

Where serializer is an object that responds to load and dump. So the default would be:

ActiveSupport::MessageVerifier.new("Hey, I'm a secret!", Marshal)

A JSON serializer would be:

class JSONSerializer
  def load(value)
    ActiveSupport::JSON.decode(value)
  end

  def dump(value)
    ActiveSupport::JSON.encode(value)
  end
end

This is better because:

  • It relies on only one parameter, rather than two
  • The parameter is set in the initializer. I don't think the serialization strategy should change once the object has been created.
  • The load/dump requirement is the same as the new serialize API in Active Record, so the same serialization objects could be used.
Member

josevalim commented Sep 15, 2011

I agree. @wvanbergen could you send another pull request? pretty please with sugar on top? :D

Contributor

wvanbergen commented Sep 15, 2011

The problem with this is that the second parameter is already in use for the hash/encryption algorithm to use. I don't expect many people to ever use this parameter because the default value is usually what you want. It would break backwards compatibility to change this though.

So I can add it as a third parameter which requires people to explicitly provide any time they want to change the serializer, or do some magic parameter type detection to make this work. I like neither of these solutions, but I will implement whatever you think is the best of those two evils :).

Member

josevalim commented Sep 15, 2011

It is fine to be a third argument but for convenience we should also have attr_accessor as well. So you could do:

message_verifier = ActiveSupport::MessageVerifier.new("Hey, I'm a secret!")
message_verifier.serializer = JSONSerializer

I guess this is the best of both worlds, no?

Contributor

wvanbergen commented Sep 15, 2011

Works for me. Give me one sec.

Contributor

wvanbergen commented Sep 15, 2011

Voilà. See #3035.

Member

jonleighton commented Sep 15, 2011

I am still not a massive fan of specifying it via attr_accessor. I think in an ideal world we'd use an options hash, e.g.

ActiveSupport::MessageVerifier.new(
  "Hey, I'm a secret!",
  :serializer => JSONSerializer,
  :cipher     => 'bcrypt'
)

It would be possible to implement this with a deprecation for if the second argument is not a hash. What do you guys think?

Member

jonleighton commented Sep 15, 2011

Also, can the docs/changelog be updated please? (Once we've decided out what to do.)

Contributor

wvanbergen commented Sep 15, 2011

I like that too, but it still breaks backwards compatibility, unless we do some magic type checking on the second parameter.

Member

jonleighton commented Sep 15, 2011

Yeah, that's what I am suggesting:

def initialize(secret, options = {})
  unless options.respond_to?(:keys)
    ActiveSupport::Deprecation.warn "bla bla"
    options = { :cipher => options }
  end

  options = { cipher => '...', :serializer => Marshal) }.merge(options)
end
Contributor

wvanbergen commented Sep 15, 2011

I have to code written for this patch. @josevalim: do you want me to create another pull request with these changes or do you want to keep it like it is right now?

Member

josevalim commented Sep 15, 2011

Using a hash on initialize is better. Pull request pls!

Contributor

wvanbergen commented Sep 15, 2011

See #3037.

Contributor

wvanbergen commented Sep 15, 2011

All merged.

For those interested, a compatible MessageVerifier implementation for node.js: https://gist.github.com/1220401

@paukul paukul pushed a commit to paukul/rails that referenced this pull request Feb 20, 2012

@joshk joshk + Santiago Pastorino and Emilio Tagua Updated the json date regex to recognize xmlschema formatted date tim…
…es during json decoding. [#3031 state:resolved]

Signed-off-by: Santiago Pastorino and Emilio Tagua <santiago+emilioe@wyeworks.com>
73b9e43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment