Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

SignedCookieJar and PermanentCookieJar properly delegate everything to parent_jar #6745

Closed
wants to merge 3 commits into from

5 participants

@fxposter

Earlier it was not possible to do cookies.signed.delete or cookies.permanent.clear, cause this and other methods were not properly delegated to parent jar and were called on SignedCookieJar (or PermanentCookieJar) itself, which raised errors. Now SignedCookieJar and PermanentCookieJar can be used anywhere, where regular CookieJar is acceptable.

@steveklabnik
Collaborator

Won't making them not CookieJars anymore cause things to break? Or were these the only methods that were being used by the parent class? Basically, I'm not sure why you removed the inheritance.

@fxposter

@steveklabnik Try to call PermanentCookieJar#delete - if there is inheritance - it will not work at all. if we remove inheritance - this call will be properly dispatched to parent cookie jar.

@rafaelfranca

This needs a rebase. @spastorino @NZKoz what do you think?

@spastorino
Owner

:+1:, but @fxposter can you rebase?.
We now also added encrypted cookie store so the same thing needs to be done for that

@fxposter

I'm sorry, I could not easily rebase these changes against current master, but I'll do it today or tomorrow.
And yes, I saw encrypted cookies jar class, so I'd like to know, what is the real public interface of CookieJar classes.

Cause my changes only properly propagate everything to parent cookie jar, which is okay for #delete or #clear methods, but actually fail for #update (so cookies.signed.update will not get the signed part really working).

Also, #signed_using_old_secret should only be available on cookies, or cookies.permanent should also get it?

@fxposter

I had time to walk through current CookieJar code. As I understand, the real public interface of CookieJar is [], []=, delete, clear, key?, has_key?, deleted? and subclass creation methods: permanent, signed, encrypted and signed_using_old_secret (is this meant to be chained?).

@fxposter

So, as long as I understand - we can run all calls except [] and []= through method_missing chain.

fxposter added some commits
@fxposter fxposter SignedCookieJar and PermanentCookieJar properly delegate everything t…
…o parent_jar

Earlier it was not possible to do cookies.signed.delete or
cookies.permanent.clear, cause this and other methods were not properly
deletegated to parent jar and were called on SignedCookieJar (or
PermanentCookieJar) itself, which raised errors. Now SignedCookieJar and
PermanentCookieJar can be used anywhere, where regular CookieJar is
acceptable.
fa46aa1
@fxposter fxposter Add more cookie tests 52a4346
@fxposter fxposter Add even more cookie tests, fixed InvalidMessage exception scope ba7d38a
@fxposter

Rebased, and added more tests. /cc @spastorino

@spastorino
Owner

@fxposter what we would need to just allow [], []=, permanent, signed and encrypted. And deprecate method_missing without doing anything. Doesn't make sense to do cookies.signed.delete, just do cookies.delete

@fxposter

But in that case we can allow users to pass any cookie object (signed or encrypted) to any part of the system without having to know if it is a signed cookie or the default one..

@rafaelfranca

@spastorino are there blockers for accept this pull request?

@spastorino spastorino was assigned
@spastorino
Owner

@fxposter not sure I follow what you mean. My english is terrible. /cc @NZKoz
I don't see any problem of doing what I've said. Try to explain again please :)

@fxposter

The main reason I want this to be submitted - is that I want methods like #clear and #delete and others to be accessible through other cookie jars (encrypted, signed, etc) and not only through default one.
Use case is simple - If I make a library, which needs to do something with cookies - I can pass cookie jar object to it and it can set or delete values to it. I want the user to have ability to pass any cookie jar to it:

class MyLibrary
  def initialize(cookies)
    @cookies = cookies
  end

  def do_something_useful
    @cookies.delete(:wrong_key)
  end
end

# in controller
MyLibrary.new(cookies).do_something_useful
MyLibrary.new(cookies.signed).do_something_useful
@NZKoz
Owner

The problem I have with that use case, is that passing the signed cookies jar is incredibly misleading because:

cookies[:not_signed] = "hello"
cookies.signed.delete(:not_signed)

I'd personally say that exposing delete and clear to the other cookie jars is likely to lead to people thinking "I can call clear because it'll only ever clear the signed cookies" and being horribly surprised. The other cookie jars should probably just expose [] and []= and leave the rest to the top-level cookie jar.

@spastorino
Owner

agree with @NZKoz that's what I wanted to do. Sorry for my broken english :P

@fxposter

@NZKoz what do you mean by "leave the rest to the top-level cookie jar"? Now if I try to call cookies.signed.clear it will raise NoMethodError: undefined method 'each_key' for nil:NilClass, which is really confusing.

@NZKoz
Owner

I think that the signed jar simply shouldn't respond to clear or delete at all, so the NoMethodError is fine, but the message is clearly confusing

@spastorino
Owner

I would do something like this https://gist.github.com/a8a1f6f58a040951cf7b
But better programmed ;). Could be nice to make it more DRY.

@spastorino
Owner

I've committed the code for now 2b773e1 we can start refactoring. I have plans to make all the cookies code better because the current status it's not good and my commit is not making it any better ;)

@spastorino spastorino closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 22, 2012
  1. @fxposter

    SignedCookieJar and PermanentCookieJar properly delegate everything t…

    fxposter authored
    …o parent_jar
    
    Earlier it was not possible to do cookies.signed.delete or
    cookies.permanent.clear, cause this and other methods were not properly
    deletegated to parent jar and were called on SignedCookieJar (or
    PermanentCookieJar) itself, which raised errors. Now SignedCookieJar and
    PermanentCookieJar can be used anywhere, where regular CookieJar is
    acceptable.
  2. @fxposter

    Add more cookie tests

    fxposter authored
  3. @fxposter
This page is out of date. Refresh to see the latest.
View
57 actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -293,13 +293,47 @@ def write_cookie?(cookie)
end
end
- class PermanentCookieJar < CookieJar #:nodoc:
+ class ProxyCookieJar #:nodoc:
def initialize(parent_jar, key_generator, options = {})
@parent_jar = parent_jar
@key_generator = key_generator
@options = options
end
+ def is_a?(klass)
+ super || @parent_jar.is_a?(klass)
+ end
+
+ def permanent
+ if is_a? PermanentCookieJar
+ self
+ else
+ @permanent ||= PermanentCookieJar.new(self, @key_generator, @options)
+ end
+ end
+
+ def signed
+ if is_a? SignedCookieJar
+ self
+ else
+ @signed ||= SignedCookieJar.new(self, @key_generator, @options)
+ end
+ end
+
+ def encrypted
+ if is_a? EncryptedCookieJar
+ self
+ else
+ @encrypted ||= EncryptedCookieJar.new(self, @key_generator, @options)
+ end
+ end
+
+ def method_missing(method, *arguments, &block)
+ @parent_jar.send(method, *arguments, &block)
+ end
+ end
+
+ class PermanentCookieJar < ProxyCookieJar #:nodoc:
def []=(key, options)
if options.is_a?(Hash)
options.symbolize_keys!
@@ -310,20 +344,15 @@ def []=(key, options)
options[:expires] = 20.years.from_now
@parent_jar[key] = options
end
-
- def method_missing(method, *arguments, &block)
- @parent_jar.send(method, *arguments, &block)
- end
end
- class SignedCookieJar < CookieJar #:nodoc:
+ class SignedCookieJar < ProxyCookieJar #:nodoc:
MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
def initialize(parent_jar, key_generator, options = {})
- @parent_jar = parent_jar
- @options = options
+ super
secret = key_generator.generate_key(@options[:signed_cookie_salt])
- @verifier = ActiveSupport::MessageVerifier.new(secret)
+ @verifier = ActiveSupport::MessageVerifier.new(secret)
end
def [](name)
@@ -345,10 +374,6 @@ def []=(key, options)
raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@parent_jar[key] = options
end
-
- def method_missing(method, *arguments, &block)
- @parent_jar.send(method, *arguments, &block)
- end
end
class EncryptedCookieJar < SignedCookieJar #:nodoc:
@@ -358,8 +383,8 @@ def initialize(parent_jar, key_generator, options = {})
"Set config.secret_key_base in config/initializers/secret_token.rb"
end
- @parent_jar = parent_jar
- @options = options
+ super
+
secret = key_generator.generate_key(@options[:encrypted_cookie_salt])
sign_secret = key_generator.generate_key(@options[:encrypted_signed_cookie_salt])
@encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret)
@@ -370,7 +395,7 @@ def [](name)
@encryptor.decrypt_and_verify(encrypted_message)
end
rescue ActiveSupport::MessageVerifier::InvalidSignature,
- ActiveSupport::MessageVerifier::InvalidMessage
+ ActiveSupport::MessageEncryptor::InvalidMessage
nil
end
View
59 actionpack/test/dispatch/cookies_test.rb
@@ -94,6 +94,27 @@ def delete_and_set_cookie
head :ok
end
+ def delete_signed_cookie
+ cookies.signed.delete :user_name
+ head :ok
+ end
+
+ def delete_permanent_cookie
+ cookies.permanent.delete :user_name
+ head :ok
+ end
+
+ def clear_encrypted_cookie
+ cookies.encrypted.clear
+ head :ok
+ end
+
+ def delete_and_set_signed_permanent_encrypted_cookie
+ cookies.signed.permanent.encrypted.delete :user_name
+ cookies.signed.permanent.encrypted[:user_surname] = 'Smith'
+ head :ok
+ end
+
def set_cookie_with_domain
cookies[:user_name] = {:value => "rizwanreza", :domain => :all}
head :ok
@@ -327,6 +348,44 @@ def test_permanent_signed_cookie
assert_equal 100, @controller.send(:cookies).signed[:remember_me]
end
+ def test_delete_signed_cookie
+ @controller.send(:cookies).signed[:user_name] = 'Joe'
+ get :delete_signed_cookie
+ assert_nil @controller.send(:cookies).signed[:user_name]
+ assert_nil @controller.send(:cookies)[:user_name]
+ assert @controller.send(:cookies).deleted?(:user_name)
+ assert @controller.send(:cookies).signed.deleted?(:user_name)
+ end
+
+ def test_delete_permanent_cookie
+ @controller.send(:cookies).permanent[:user_name] = 'Joe'
+ get :delete_permanent_cookie
+ assert_nil @controller.send(:cookies).permanent[:user_name]
+ assert_nil @controller.send(:cookies)[:user_name]
+ assert @controller.send(:cookies).deleted?(:user_name)
+ assert @controller.send(:cookies).permanent.deleted?(:user_name)
+ end
+
+ def test_clear_encrypted_cookie
+ @controller.send(:cookies).permanent[:user_name] = 'Joe'
+ @controller.send(:cookies).permanent[:user_surname] = 'Smith'
+ get :clear_encrypted_cookie
+ assert_nil @controller.send(:cookies)[:user_name]
+ assert_nil @controller.send(:cookies)[:user_surname]
+ assert @controller.send(:cookies).deleted?(:user_name)
+ assert @controller.send(:cookies).encrypted.deleted?(:user_name)
+ end
+
+ def test_delete_and_set_signed_permanent_encrypted_cookie
+ @controller.send(:cookies).signed.permanent.encrypted[:user_name] = 'Joe'
+ get :delete_and_set_signed_permanent_encrypted_cookie
+ assert_nil @controller.send(:cookies)[:user_name]
+ assert_nil @controller.send(:cookies).signed.permanent.encrypted[:user_name]
+ assert_equal @controller.send(:cookies).signed.permanent.encrypted[:user_surname], 'Smith'
+ assert_not_equal @controller.send(:cookies)[:user_surname], 'Smith'
+ assert_not_equal @controller.send(:cookies).encrypted[:user_surname], 'Smith'
+ end
+
def test_delete_and_set_cookie
request.cookies[:user_name] = 'Joe'
get :delete_and_set_cookie
Something went wrong with that request. Please try again.