Enhanced cookie settings

[roadrunner2]

We needed to control additional cookie attributes such as the domain and httpOnly flag. Hence the first few commits. Furthermore, our code was already using SessionCookieConfig from Servlet 3.0 to set these values, so I figured in the interest of making this plugin a "drop-in replacement" to add support for that too in the last commit. Note that because there's no clear time at which the values in SessionCookieConfig will have been set (we are doing it in Bootstrap.init), and in fact the values could change at any time, the lookup has to be done each time a cookie is created, rather than once at initialization time.

[benlucchesi]

roadrunner2,

thanks for the updates. I'll run these through the test project to check for version compatibility issues in grails and compatibility with the various libraries that this plugin needs to support (webflow, spring security, etc). I'm very interested to see what happens with the kryo update. That library has been a pain in my a\ for some time, between incompatibilities between the kryo and the kryo-serializers and their dependencies with grails dependencies.

I'll keep you posted.

thanks!

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Tuesday, November 12, 2013 8:09 PM To: benlucchesi/grails-cookie-session-v2 Subject: [grails-cookie-session-v2] Enhanced cookie settings (#23)

We needed to control additional cookie attributes such as the domain and httpOnly flag. Hence the first few commits. Furthermore, our code was already using SessionCookieConfig from Servlet 3.0 to set these values, so I figured in the interest of making this plugin a "drop-in replacement" to add support for that too in the last commit. Note that because there's no clear time at which the values in SessionCookieConfig will have been set (we are doing it in Bootstrap.init), and in fact the values could change at any time, the lookup has to be done each time a cookie is created, rather than once at initialization time.

---

You can merge this Pull Request by running

git pull https://github.com/roadrunner2/grails-cookie-session-v2 enhanced-cookie-settings

Or view, comment on, or merge it at:

https://github.com/benlucchesi/grails-cookie-session-v2/pull/23

Commit Summary

• Factored out cookie creation into common function.
• Minor logging fix.
• Added support for setting path, domain, httpOnly, and comment attributes too.
• Add support for using cookie settings from SessionCookieConfig.

File Changes

• M README.mdhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23/files#diff-0 (27)
• M src/groovy/com/granicus/grails/plugins/cookiesession/CookieSessionRepository.groovyhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23/files#diff-1 (127)

Patch Links:

• https://github.com/benlucchesi/grails-cookie-session-v2/pull/23.patch
• https://github.com/benlucchesi/grails-cookie-session-v2/pull/23.diff

[roadrunner2]

Thanks for considering the patches. Our app uses spring security, but not webflow, so that's one datapoint in the test matrix (though we certainly don't use all aspects of spring security).

[benlucchesi]

Thanks for this contribution! I believe I preserved the intent of your code, but I modified how the configuration variables were handled. My preference was not to introduce new types for handling configuration and to avoid method calls and parameter passing, not necessarily for performance, but for overall understandability of the code. Unfortunately I'm not the bright, so I need to keep things as simple as possible.

To deal with the issue of modifying SessionCookieConfig in a running application, I attached a couple of listeners to the class's setters so that changes could be detected and update the CookieSessionRepository setting. The scheme I employed should work fine when accessing and updating the SessionCookieConfig from within the grails application, as you are in the BootStrap init method. However, if you were updating SessionCookieConfig from pure java code or from outside of the grails application, then the changes won't be detected. To handle that scenario, I consolidated the configuration code to a single method that can be called to reconfigure the CookieSessionRepository on the fly so that you could orchestrate updates.

Please let me know if you run into problems with this and thanks again for the great contributions!

[roadrunner2]

Hi Ben,

Thanks for this contribution! I believe I preserved the intent of your code, but I modified how the configuration variables were handled. My preference was not to introduce new types for handling configuration and to avoid method calls and parameter passing, not necessarily for performance, but for overall understandability of the code. Unfortunately I'm not the bright, so I need to keep things as simple as possible.

Sounds good.

To deal with the issue of modifying SessionCookieConfig in a running application, I attached a couple of listeners to the class's setters so that changes could be detected and update the CookieSessionRepository setting. The scheme I employed should work fine when accessing and updating the SessionCookieConfig from within the grails application, as you are in the BootStrap init method. However, if you were updating SessionCookieConfig from pure java code or from outside of the grails application, then the changes won't be detected. To handle that scenario, I consolidated the configuration code to a single method that can be called to reconfigure the CookieSessionRepository on the fly so that you could orchestrate updates.

Please let me know if you run into problems with this and thanks again for the great contributions!

Ok, so this should address the concern I raised in my previous email. However, I just tried it (in BootStrap.groovy in the init closure I'm calling 'servletContext.getSessionCookieConfig().setDomain(domain)'), and I don't see it working - where are the listeners attached? I pulled the latest code and I can't see where this is done.

Cheers,

Ronald

[benlucchesi]

So I added another configuration option to explicitly enable SessionCookieConfig use...

add this to your config:

grails.plugin.cookiesession.usesessioncookieconfig = true

I added this config option to ensure backwards compatibility and to avoid "mystery" behavior when upgrading from servlet 2.5 to 3.0.

Also, is this set this in your build config?

grails.servlet.version = "3.0" // Change depending on target container compliance (2.5 or 3.0)

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Wednesday, November 20, 2013 9:05 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

Hi Ben,

Thanks for this contribution! I believe I preserved the intent of your code, but I modified how the configuration variables were handled. My preference was not to introduce new types for handling configuration and to avoid method calls and parameter passing, not necessarily for performance, but for overall understandability of the code. Unfortunately I'm not the bright, so I need to keep things as simple as possible.

Sounds good.

To deal with the issue of modifying SessionCookieConfig in a running application, I attached a couple of listeners to the class's setters so that changes could be detected and update the CookieSessionRepository setting. The scheme I employed should work fine when accessing and updating the SessionCookieConfig from within the grails application, as you are in the BootStrap init method. However, if you were updating SessionCookieConfig from pure java code or from outside of the grails application, then the changes won't be detected. To handle that scenario, I consolidated the configuration code to a single method that can be called to reconfigure the CookieSessionRepository on the fly so that you could orchestrate updates.

Please let me know if you run into problems with this and thanks again for the great contributions!

Ok, so this should address the concern I raised in my previous email. However, I just tried it (in BootStrap.groovy in the init closure I'm calling 'servletContext.getSessionCookieConfig().setDomain(domain)'), and I don't see it working - where are the listeners attached? I pulled the latest code and I can't see where this is done.

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-28958461.

[benlucchesi]

Also... the listeners are set in the CookieSessionRepository in the assignSettingFromSessionCookieConfig method. The strategy is to grab the getter and setter methods from the SessionCookieConfig object, save them and replace them with setters and getters that can update the CookieSessionRepository before calling original getters and setters on the SessionCookieConfig.

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Wednesday, November 20, 2013 9:05 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

Hi Ben,

Thanks for this contribution! I believe I preserved the intent of your code, but I modified how the configuration variables were handled. My preference was not to introduce new types for handling configuration and to avoid method calls and parameter passing, not necessarily for performance, but for overall understandability of the code. Unfortunately I'm not the bright, so I need to keep things as simple as possible.

Sounds good.

To deal with the issue of modifying SessionCookieConfig in a running application, I attached a couple of listeners to the class's setters so that changes could be detected and update the CookieSessionRepository setting. The scheme I employed should work fine when accessing and updating the SessionCookieConfig from within the grails application, as you are in the BootStrap init method. However, if you were updating SessionCookieConfig from pure java code or from outside of the grails application, then the changes won't be detected. To handle that scenario, I consolidated the configuration code to a single method that can be called to reconfigure the CookieSessionRepository on the fly so that you could orchestrate updates.

Please let me know if you run into problems with this and thanks again for the great contributions!

Ok, so this should address the concern I raised in my previous email. However, I just tried it (in BootStrap.groovy in the init closure I'm calling 'servletContext.getSessionCookieConfig().setDomain(domain)'), and I don't see it working - where are the listeners attached? I pulled the latest code and I can't see where this is done.

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-28958461.

[benlucchesi]

so did you try setting this in your config? grails.plugin.cookiesession.usesessioncookieconfig = true

[roadrunner2]

So I added another configuration option to explicitly enable SessionCookieConfig use...

add this to your config:

grails.plugin.cookiesession.usesessioncookieconfig = true

I added this config option to ensure backwards compatibility and to avoid "mystery" behavior when upgrading from servlet 2.5 to 3.0.

Ok.

Also, is this set this in your build config?

Yes.

So where is the hook that detects changes to SessionCookieConfig?

Cheers,

Ronald

[benlucchesi]

Its in the CookieSessionRepository.assignSettingFromSessionCookieConfig

The strategy is to grab the getter method off the SessionCookieConfig object and replace the setter with one that updates the CookieSessionRepository before calling the original getter.

The following is the code that installs the hook.

def servletContext = applicationContext.getBean('servletContext')
def setSetting = servletContext.sessionCookieConfig.metaClass.methods.find{ it.name.equalsIgnoreCase("set${settingName}") }
def getSetting = servletContext.sessionCookieConfig.metaClass.methods.find{ it.name.equalsIgnoreCase("get${settingName}") || it.name.equalsIgnoreCase("is${settingName}") }

if( setSetting && getSetting ){
  servletContext.sessionCookieConfig.class.metaClass."${setSetting.name}" = { newSettingValue ->
  try{
    if( newSettingValue != null ){
      this.log.trace "detected sessionCookieConfig setting changed: ${newSettingValue}"
      this.(targetPropertyName.toString()) = newSettingValue
      setSetting.invoke(servletContext.sessionCookieConfig,newSettingValue)
    }
   }
   catch( excp ){
    this.log.error "error updating setting from sessionCookieConfig ", excp
   }
  }

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Wednesday, November 20, 2013 10:10 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

So I added another configuration option to explicitly enable SessionCookieConfig use...

add this to your config:

grails.plugin.cookiesession.usesessioncookieconfig = true

I added this config option to ensure backwards compatibility and to avoid "mystery" behavior when upgrading from servlet 2.5 to 3.0.

Ok.

Also, is this set this in your build config?

Yes.

So where is the hook that detects changes to SessionCookieConfig?

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-28960394.

[benlucchesi]

is it working now that usesessioncookieconfig is enabled?

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Wednesday, November 20, 2013 10:10 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

So I added another configuration option to explicitly enable SessionCookieConfig use...

add this to your config:

grails.plugin.cookiesession.usesessioncookieconfig = true

I added this config option to ensure backwards compatibility and to avoid "mystery" behavior when upgrading from servlet 2.5 to 3.0.

Ok.

Also, is this set this in your build config?

Yes.

So where is the hook that detects changes to SessionCookieConfig?

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-28960394.

[roadrunner2]

So I added another configuration option to explicitly enable SessionCookieConfig use... add this to your config: grails.plugin.cookiesession.usesessioncookieconfig = true

Found one problem: it's actually

grails.plugin.cookiesession.useSessionCookieConfig = true

(note the mixed case!)

[snip]

Also, is this set this in your build config? grails.servlet.version = "3.0" // Change depending on target container compliance (2.5 or 3.0)

Ah, that has to set explicitly. Ok. For some reason I was thinking that was set automatically, but I see I was mistaken.

Also... the listeners are set in the CookieSessionRepository in the assignSettingFromSessionCookieConfig method. The strategy is to grab [snip]

Got it. Thanks. Something's still not kosher, but I'm digging into it and will let you know when I've found it.

Now, one more issue: 'secret' is now expected to be String - before it supported both String and byte[]. String is problematic, because you do a getBytes() on it, which uses the platform default encoding. But that means that on platforms with, say, a default encoding of UTF-8 (a majority these days, I think) you now can't set a full key: assuming a 128 bit key, the maximum entropy a String.getBytes() will give you under UTF-8 is 7 bits per char, i.e. 112 bits (any char > 127 is encoded as 2+ bytes under UTF-8, 5+ bits of which are fixed, so the best you can do is use chars in the range [0,127]).

Cheers,

Ronald

[roadrunner2]

Also... the listeners are set in the CookieSessionRepository in the assignSettingFromSessionCookieConfig method. The strategy is to grab [snip]

Got it. Thanks. Something's still not kosher, but I'm digging into it and will let you know when I've found it.

Ok, I found the issue. First of all, I was calling 'sessionCookieConfig.setDomain(domain)' rather than doing 'sessionCookieConfig.domain = domain' - the latter works, the former doesn't. And the reason is because the current code is not actually overriding the setters, but is instead adding new overloaded ones with slightly different signatures, namely ones that take Object as the parameter rather than the specific type (String, boolean, or int). The following little groovy script demonstrates this:

Date d = new Date()

d.metaClass.'setYear' = { y -> println("setting year '$y'") }
d.metaClass.'setMonth' = { int m -> println("setting month '$m'") }

d.year = 1
d.setYear(2)
d.month = 3
d.setMonth(4)

If you run this you'll notice that the setYear() call is not being intercepted - if you change the closure to take 'int y' like for month, it works.

I tried to apply this to CookieSessionRepository, but for some (other?) reason it's still not working. This is what I tried:

--- a/src/groovy/com/granicus/grails/plugins/cookiesession/CookieSessionRepository.groovy
+++ b/src/groovy/com/granicus/grails/plugins/cookiesession/CookieSessionRepository.groovy
@@ -234,7 +234,7 @@ class CookieSessionRepository implements SessionRepository, InitializingBean, Ap
     def getSetting = servletContext.sessionCookieConfig.metaClass.methods.find{ it.name.equalsIgnoreCase("get${settingName}") || it.name.equalsIgnoreCase("is${settingName}") }

     if( setSetting && getSetting ){
-      servletContext.sessionCookieConfig.class.metaClass."${setSetting.name}" = { newSettingValue ->
+      Closure newSetter = { newSettingValue ->
       try{
         if( newSettingValue != null ){
           this.log.trace "detected sessionCookieConfig setting changed: ${newSettingValue}"
@@ -246,6 +246,20 @@ class CookieSessionRepository implements SessionRepository, InitializingBean, Ap
         this.log.error "error updating setting from sessionCookieConfig ", excp
        }
       }
+      def metaClass = servletContext.sessionCookieConfig.class.metaClass
+      switch( setSetting.nativeParameterTypes[0] ){
+        case String:
+          metaClass."${setSetting.name}" = { String newSettingValue -> newSetter(newSettingValue) }
+          break
+        case boolean:
+          metaClass."${setSetting.name}" = { boolean newSettingValue -> newSetter(newSettingValue) }
+          break
+        case int:
+          metaClass."${setSetting.name}" = { int newSettingValue -> newSetter(newSettingValue) }
+          break
+        default:
+          throw new Error("Internal error: unexpected argument type ${setSetting.nativeParameterTypes[0]} in $setSetting")
+      }

       def settingValue = getSetting.invoke(servletContext.sessionCookieConfig)
       if( settingValue != null ){

I.e. I create closures with proper signatures depending on the expected type, and call through to the common closure. But like I said, it still doesn't work for some reason.

Anyway, I can certainly (and have) change my code to use the assignment rather than calling the setter, but it would potentially cause less confusion if the setter worked too.

Cheers,

Ronald

[benlucchesi]

Hi Ronald,

grails.plugin.cookiesession.useSessionCookieConfig = true case shouldn't matter. The code that reads the setting does a case-insensitive comparison. I think your subsequent email that where you found the issue with closure parameter types was the root cause.

Now, one more issue: 'secret' is now expected to be String - before it supported both String and byte[] For the 'secret' expecting a string, I'll update that to accept both and do the right thing with whatever is assigned.

From your other email...

Ok, I found the issue. First of all, I was calling 'sessionCookieConfig.setDomain(domain)' rather than doing 'sessionCookieConfig.domain = domain'

I'm going to update the code so that it works for both scenarios. Great catch!

Thank for spending the time to dig into these issues. I really appreciate it!

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Wednesday, November 20, 2013 10:35 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

So I added another configuration option to explicitly enable SessionCookieConfig use... add this to your config: grails.plugin.cookiesession.usesessioncookieconfig = true

Found one problem: it's actually

grails.plugin.cookiesession.useSessionCookieConfig = true

(note the mixed case!)

[snip]

Also, is this set this in your build config? grails.servlet.version = "3.0" // Change depending on target container compliance (2.5 or 3.0)

Ah, that has to set explicitly. Ok. For some reason I was thinking that was set automatically, but I see I was mistaken.

Also... the listeners are set in the CookieSessionRepository in the assignSettingFromSessionCookieConfig method. The strategy is to grab [snip]

Got it. Thanks. Something's still not kosher, but I'm digging into it and will let you know when I've found it.

Now, one more issue: 'secret' is now expected to be String - before it supported both String and byte[]. String is problematic, because you do a getBytes() on it, which uses the platform default encoding. But that means that on platforms with, say, a default encoding of UTF-8 (a majority these days, I think) you now can't set a full key: assuming a 128 bit key, the maximum entropy a String.getBytes() will give you under UTF-8 is 7 bits per char, i.e. 112 bits (any char > 127 is encoded as 2+ bytes under UTF-8, 5+ bits of which are fixed, so the best you can do is use chars in the range [0,127]).

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-28961214.

[benlucchesi]

ok, I reworked the configuration code so that the interceptors work for both property sets and method calls. I also updated the secret handling code so that it detects string, byte[] and ArrayList and tries to do the "right" thing with them. Its all in version 2.0.13 in both develop and master. Can you look at the code. if it looks ok to you, I'll push to the plugin repo.

thanks!

[roadrunner2]

ok, I reworked the configuration code so that the interceptors work for both property sets and method calls. I also updated the secret handling code so that it detects string, byte[] and ArrayList and tries to do the "right" thing with them. Its all in version 2.0.13 in both develop and master. Can you look at the code. if it looks ok to you, I'll push to the plugin repo.

Thanks! Just pulled master and tested, and it works perfectly now.

Great work.

Cheers,

Ronald

[benlucchesi]

fantastic! I'll push 2.0.13 to the central plugin repo. I really appreciate the work you put into this!

Ben Lucchesi | Chief Software Architect | Granicus Inc. 600 Harrison Street, Suite 120 San Francisco, CA 94107 work: 415.357.3618 x1300 | fax: 415.618.0102 | cell: 775.250.3396

---

From: roadrunner2 [notifications@github.com] Sent: Thursday, November 21, 2013 7:17 PM To: benlucchesi/grails-cookie-session-v2 Cc: Benjamin Lucchesi Subject: Re: [grails-cookie-session-v2] Enhanced cookie settings (#23)

ok, I reworked the configuration code so that the interceptors work for both property sets and method calls. I also updated the secret handling code so that it detects string, byte[] and ArrayList and tries to do the "right" thing with them. Its all in version 2.0.13 in both develop and master. Can you look at the code. if it looks ok to you, I'll push to the plugin repo.

Thanks! Just pulled master and tested, and it works perfectly now.

Great work.

Cheers,

Ronald

— Reply to this email directly or view it on GitHubhttps://github.com/benlucchesi/grails-cookie-session-v2/pull/23#issuecomment-29046119.