You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Rainer Jung <ra...@kippdata.de> on 2014/03/10 16:00:29 UTC
Re: Special requirements on session id generator
Here's a patch:
http://people.apache.org/~rjung/patches/TC8-SessionIdGenerator-extensible.patch
Any concerns about applying to TC 8? Please note "Known problems".
Some notes:
About the patch:
1) New interface org.apache.catalina.SessionIdGenerator
- setter and getter for jvmRoute
- setter and getter for session id length
- generateSessionId() (use jvmRoute if set) and
generateSessionId(String route).
2) Renamed org.apache.catalina.util.SessionIdGenerator to
org.apache.catalina.util.SessionIdGeneratorBase.
Not strictly needed but for consistency with other similar cases.
3) Changed message keys in
org.apache.catalina.util.LocalStrings.properties
etc. from sessionIdGenerator.* to sessionIdGeneratorBase.*.
4) Let org.apache.catalina.util.SessionIdGeneratorBase
implement org.apache.catalina.SessionIdGenerator.
5) Adjust visibility of getRandomBytes(byte bytes[]) in base
class from private to protected to allow extension.
6) Add getter and setter for SessionIdGenerator to Manager interface
7) Adjust SessionIdGenerator initialization in ManagerBase:
uses generator set in context config if present, otherwise
uses SessionIdGeneratorBase as default.
8) Implement SessionIdGenerator getter and setter in ManagerBase.
9) Add Context/Manager/SessionIdGenerator to digester ContextRuleSet.
Known problems:
- Docs missing (will add to configuration reference)
- Unrelated to the patch: Semantics of session id length is unclear.
Implemented as number of random bytes that get encoded into the id,
but documented as actual session id length (which is not true
currently and would after the patch also depend on the actual
encoding used by the id generator, e.g. hex or other,
added routing tags etc.).
I'd opt for keeping current semantics and only cleaning up docs.
- Session id length can not be set on the SessionIdGenerator level:
For compatibility the Manager has a default of 16 and forwards that
or whatever was explicitly configured for it to the
SessionIdGenerator during its own ManagerBase.startInternal().
That overwrites what the digester might have already set on the
SessionIdGenerator directly.
Possible workaround 1: only call setSessionIdLength() from Manager to
SessionIdGenerator if setSessionIdLength() was explicitely called
on the Manager itself and getSessionIdLength() does return <=0
(no sensible default or explicit setting on SessionIdGenerator)
Possible workaround 2: remove setSessionIdLength() and
getSessionIdLength() completely from Manager.
I'd opt for this one for TC 8. Obviously not for a possible backport.
I'd suggest workaround 1 for a possible backport.
Possible Backport (TC 7, can be decided later):
- new interface SessionIdGenerator allowed? Should be OK. Nobody
needs to implement it unless she wants to replace the generator.
- skip renaming of SessionIdGenerator to SessionIdGeneratorBase.
Fewer changes, lower chances of incompatibility.
- addition of SessionIdGenerator setter and getter to Manager
interface probably not allowed? Backported code should still
work except digester should throw an exception if the Manager impl
doesn't have a setSessionIdGenerator() method. Add that method
to ManagerBase so most Manager impls will automatically support
replacing the SessionIdGenerator.
- different workaround 1 for the setSessionIdLength() problem.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Special requirements on session id generator
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2014 15:47, Rainer Jung wrote:
> On 10.03.2014 16:08, Mark Thomas wrote:
>> On 10/03/2014 15:00, Rainer Jung wrote:
>>> Here's a patch:
>>>
>>> http://people.apache.org/~rjung/patches/TC8-SessionIdGenerator-extensible.patch
>>>
>>> Any concerns about applying to TC 8? Please note "Known problems".
>>>
>>> Some notes:
>>>
>>> About the patch:
>>>
>>> 1) New interface org.apache.catalina.SessionIdGenerator
>>> - setter and getter for jvmRoute
>>> - setter and getter for session id length
>>> - generateSessionId() (use jvmRoute if set) and
>>> generateSessionId(String route).
>>> 2) Renamed org.apache.catalina.util.SessionIdGenerator to
>>> org.apache.catalina.util.SessionIdGeneratorBase.
>>> Not strictly needed but for consistency with other similar cases.
>>
>> Is it consistent? I'd expect a class with a name that ends in Base to be
>> abstract rather than concrete. I'd expect the default implementation to
>> be named something like DefaultSessionIdGenerator.
>>
>> Other than that +1
>
> As always you are completely right. Sorry.
No need to apologise. As the archives will show, I make (probably more
than) my fair share of mistakes around here.
> So either the "Default" or the "Standard" bike shed.
Pick your favourite bike shed. I have no strong preference.
> We could also add a abstract base with
> everything except the two generateSessionId() methods, but that would
> seem a bit artificial to me.
I guess it depends how pure "object orientated" you want to be. I think
a base class is unnecessary but I don't feel strongly enough about it to
object to a base class if you want to go that way.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Special requirements on session id generator
Posted by Rainer Jung <ra...@kippdata.de>.
On 10.03.2014 16:08, Mark Thomas wrote:
> On 10/03/2014 15:00, Rainer Jung wrote:
>> Here's a patch:
>>
>> http://people.apache.org/~rjung/patches/TC8-SessionIdGenerator-extensible.patch
>>
>> Any concerns about applying to TC 8? Please note "Known problems".
>>
>> Some notes:
>>
>> About the patch:
>>
>> 1) New interface org.apache.catalina.SessionIdGenerator
>> - setter and getter for jvmRoute
>> - setter and getter for session id length
>> - generateSessionId() (use jvmRoute if set) and
>> generateSessionId(String route).
>> 2) Renamed org.apache.catalina.util.SessionIdGenerator to
>> org.apache.catalina.util.SessionIdGeneratorBase.
>> Not strictly needed but for consistency with other similar cases.
>
> Is it consistent? I'd expect a class with a name that ends in Base to be
> abstract rather than concrete. I'd expect the default implementation to
> be named something like DefaultSessionIdGenerator.
>
> Other than that +1
As always you are completely right. Sorry. So either the "Default" or
the "Standard" bike shed. We could also add a abstract base with
everything except the two generateSessionId() methods, but that would
seem a bit artificial to me.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Special requirements on session id generator
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2014 15:00, Rainer Jung wrote:
> Here's a patch:
>
> http://people.apache.org/~rjung/patches/TC8-SessionIdGenerator-extensible.patch
>
> Any concerns about applying to TC 8? Please note "Known problems".
>
> Some notes:
>
> About the patch:
>
> 1) New interface org.apache.catalina.SessionIdGenerator
> - setter and getter for jvmRoute
> - setter and getter for session id length
> - generateSessionId() (use jvmRoute if set) and
> generateSessionId(String route).
> 2) Renamed org.apache.catalina.util.SessionIdGenerator to
> org.apache.catalina.util.SessionIdGeneratorBase.
> Not strictly needed but for consistency with other similar cases.
Is it consistent? I'd expect a class with a name that ends in Base to be
abstract rather than concrete. I'd expect the default implementation to
be named something like DefaultSessionIdGenerator.
Other than that +1
Mark
> 3) Changed message keys in
> org.apache.catalina.util.LocalStrings.properties
> etc. from sessionIdGenerator.* to sessionIdGeneratorBase.*.
> 4) Let org.apache.catalina.util.SessionIdGeneratorBase
> implement org.apache.catalina.SessionIdGenerator.
> 5) Adjust visibility of getRandomBytes(byte bytes[]) in base
> class from private to protected to allow extension.
> 6) Add getter and setter for SessionIdGenerator to Manager interface
> 7) Adjust SessionIdGenerator initialization in ManagerBase:
> uses generator set in context config if present, otherwise
> uses SessionIdGeneratorBase as default.
> 8) Implement SessionIdGenerator getter and setter in ManagerBase.
> 9) Add Context/Manager/SessionIdGenerator to digester ContextRuleSet.
>
> Known problems:
>
> - Docs missing (will add to configuration reference)
>
> - Unrelated to the patch: Semantics of session id length is unclear.
> Implemented as number of random bytes that get encoded into the id,
> but documented as actual session id length (which is not true
> currently and would after the patch also depend on the actual
> encoding used by the id generator, e.g. hex or other,
> added routing tags etc.).
> I'd opt for keeping current semantics and only cleaning up docs.
>
> - Session id length can not be set on the SessionIdGenerator level:
> For compatibility the Manager has a default of 16 and forwards that
> or whatever was explicitly configured for it to the
> SessionIdGenerator during its own ManagerBase.startInternal().
> That overwrites what the digester might have already set on the
> SessionIdGenerator directly.
>
> Possible workaround 1: only call setSessionIdLength() from Manager to
> SessionIdGenerator if setSessionIdLength() was explicitely called
> on the Manager itself and getSessionIdLength() does return <=0
> (no sensible default or explicit setting on SessionIdGenerator)
>
> Possible workaround 2: remove setSessionIdLength() and
> getSessionIdLength() completely from Manager.
> I'd opt for this one for TC 8. Obviously not for a possible backport.
> I'd suggest workaround 1 for a possible backport.
>
> Possible Backport (TC 7, can be decided later):
>
> - new interface SessionIdGenerator allowed? Should be OK. Nobody
> needs to implement it unless she wants to replace the generator.
>
> - skip renaming of SessionIdGenerator to SessionIdGeneratorBase.
> Fewer changes, lower chances of incompatibility.
>
> - addition of SessionIdGenerator setter and getter to Manager
> interface probably not allowed? Backported code should still
> work except digester should throw an exception if the Manager impl
> doesn't have a setSessionIdGenerator() method. Add that method
> to ManagerBase so most Manager impls will automatically support
> replacing the SessionIdGenerator.
>
> - different workaround 1 for the setSessionIdLength() problem.
>
> Regards,
>
> Rainer
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org