Session interface modification?

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Session interface modification?

Les Hazlewood
Administrator
So in my review of the code in prep for RC1 last night, I found something
I'd never really thought about before:

Should the Session *Interface* have the two methods getStopTimestamp() and
isExpired() at all?

I remember those existing in the implementation specifically to handle the
case where Sessions were persistent beyond their user lifetime - e.g. in
environments that do user activity reporting, they could organize user
events based on sessions, etc.  But the framework couldn't use stopped or
expired sessions anymore.

It seems as if implementation methods bubbled their way up into the
interface.

Would a user ever need to call getStopTimestamp() or isExpired()?

In our web support for example, these methods would never return anything
useful:  getStopTimestamp() would _always_ return null and isExpired() would
_always_ return false, because of our WebSessionManager logic:

When a Subject is being constructed for the incoming Request, the
WebSessionManager it tries to acquire the user's corresponding session (if
it exists).  If it does exist, but that session is invalid (that is, stopped
or expired), it suppresses resulting InvalidSessionException (prints out a
trace message really) and returns null to the caller.  The caller views this
as an indication that the session doesn't exist, and it must create a brand
new one if it needs to use a Session.

Because of this logic, those two methods are probably meaningless.  I also
think that our logic to ignore invalid sessions and create a brand new one
when necessary is the right way to go - it is how web containers work, and
everyone is pretty much happy with that behavior.

Do you guys agree?  Do you see a need to retain them in the interface? (They
would stay in the implementation because the implementation needs to use
them to determine if in fact it is invalid or not).

I'm just looking for your thoughts if you have any...

Les
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Jeremy Haile
I've often wished when developing web apps that I could have more  
insight into user's sessions (i.e. what sessions are active, expired,  
when was the session created, etc.)   This could be really useful for  
an administrative interface, for example.

These types of methods could be useful for that, but don't seem needed  
for normal session usage - the user shouldn't even see expired  
sessions! (so I agree these shouldn't be in the Session interface)  If  
anything, I'd move these to a sub-interface called ManagedSession or  
something like that.  I'm not against removing them completely, but  
think it'd be worth talking through the above mentioned usage first.

The SessionEventListener could help solve that usage, although it'd  
sure be nice to be able to call SessionManager.getActiveSessions(),  
etc.  (we'd want to make sure the returned info is immutable for the  
most part  I think)

I'm not saying we have to fully solve this usage now, just wanted to  
consider it before making the decision to remove metadata methods from  
the interface.

Jeremy


On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:

> So in my review of the code in prep for RC1 last night, I found  
> something
> I'd never really thought about before:
>
> Should the Session *Interface* have the two methods  
> getStopTimestamp() and
> isExpired() at all?
>
> I remember those existing in the implementation specifically to  
> handle the
> case where Sessions were persistent beyond their user lifetime -  
> e.g. in
> environments that do user activity reporting, they could organize user
> events based on sessions, etc.  But the framework couldn't use  
> stopped or
> expired sessions anymore.
>
> It seems as if implementation methods bubbled their way up into the
> interface.
>
> Would a user ever need to call getStopTimestamp() or isExpired()?
>
> In our web support for example, these methods would never return  
> anything
> useful:  getStopTimestamp() would _always_ return null and  
> isExpired() would
> _always_ return false, because of our WebSessionManager logic:
>
> When a Subject is being constructed for the incoming Request, the
> WebSessionManager it tries to acquire the user's corresponding  
> session (if
> it exists).  If it does exist, but that session is invalid (that is,  
> stopped
> or expired), it suppresses resulting InvalidSessionException (prints  
> out a
> trace message really) and returns null to the caller.  The caller  
> views this
> as an indication that the session doesn't exist, and it must create  
> a brand
> new one if it needs to use a Session.
>
> Because of this logic, those two methods are probably meaningless.  
> I also
> think that our logic to ignore invalid sessions and create a brand  
> new one
> when necessary is the right way to go - it is how web containers  
> work, and
> everyone is pretty much happy with that behavior.
>
> Do you guys agree?  Do you see a need to retain them in the  
> interface? (They
> would stay in the implementation because the implementation needs to  
> use
> them to determine if in fact it is invalid or not).
>
> I'm just looking for your thoughts if you have any...
>
> Les

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Les Hazlewood
Administrator
On Wed, Jul 30, 2008 at 11:40 AM, Jeremy Haile <[hidden email]> wrote:

> I've often wished when developing web apps that I could have more insight
> into user's sessions (i.e. what sessions are active, expired, when was the
> session created, etc.)   This could be really useful for an administrative
> interface, for example.


I agree, but wouldn't the interface want to interact with the SessionDAO or
SessionManager directly?

And this would of course only be useful if the application wishes to retain
stopped/expired sessions, for, say reporting purposes that you describe.
That means they need to ensure additional cleanup logic clears out the
retained sessions based on some criteria to ensure their session data store
(RDBMS, file-system, cache, etc) doesn't become huge.

We could provide this behavior of course, but we don't have anything like
that in place today.

These types of methods could be useful for that, but don't seem needed for
> normal session usage - the user shouldn't even see expired sessions! (so I
> agree these shouldn't be in the Session interface)  If anything, I'd move
> these to a sub-interface called ManagedSession or something like that.  I'm
> not against removing them completely, but think it'd be worth talking
> through the above mentioned usage first.


I think it'd probably be better to remove those two from the current
end-user facing API and only create the ManagedSession interface if we
decided to go down the road of providing the above support.  In other words,
I don't know if it is something we need to tackle for 0.9 final.  If you'd
like for us to provide this kind of support, would you mind opening a Jira
issue so it is captured?

Or maybe such an issue is better suited for a UI implementation task?
That'd be sweet, but it is not trivial, for sure.  To support this, realms
would also need to be mutable to receive end-user configuration changes
during runtime...


> The SessionEventListener could help solve that usage, although it'd sure be
> nice to be able to call SessionManager.getActiveSessions(), etc.  (we'd want
> to make sure the returned info is immutable for the most part  I think)


Yes, that would work.  Currently the onstop/onexpired notifications do occur
*before* the SessionManager changes its state to stopped/expired, in case
the listener wanted to use the session one last time - for example, maybe to
retrieve an attribute from the session to do further processing.

Should it be immutable?  If so, we could provide a wrapper
'ImmutableSession' implementation that throws exceptions on any method call
that tries to alter state and delegates to the wrapped Session when just
reading state, like getAttribute().  The wrapper would only be used during
listener notification, and the session would be 'unwrapped' for any
remaining processing after listener notification....

Is this something you think we should do now?  Is it something that we
should even enforce?  Maybe its as simple as making a boolean attribute in
the SessionManager implementation that turns on or off this wrapping
behavior and the default value does the wrapping?  Then if they really feel
they needed to modify the session on these listener methods, they could
override the boolean attribute?

I have no idea what the best practice would be, but for some reason I really
like the sound of the default not allowing modification.  It seems more
logical to me, because those notifications are supposed to indicate that the
session is in an invalid state and probably shouldn't be messed with
anymore.


On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:

>
>  So in my review of the code in prep for RC1 last night, I found something
>> I'd never really thought about before:
>>
>> Should the Session *Interface* have the two methods getStopTimestamp() and
>> isExpired() at all?
>>
>> I remember those existing in the implementation specifically to handle the
>> case where Sessions were persistent beyond their user lifetime - e.g. in
>> environments that do user activity reporting, they could organize user
>> events based on sessions, etc.  But the framework couldn't use stopped or
>> expired sessions anymore.
>>
>> It seems as if implementation methods bubbled their way up into the
>> interface.
>>
>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>
>> In our web support for example, these methods would never return anything
>> useful:  getStopTimestamp() would _always_ return null and isExpired()
>> would
>> _always_ return false, because of our WebSessionManager logic:
>>
>> When a Subject is being constructed for the incoming Request, the
>> WebSessionManager it tries to acquire the user's corresponding session (if
>> it exists).  If it does exist, but that session is invalid (that is,
>> stopped
>> or expired), it suppresses resulting InvalidSessionException (prints out a
>> trace message really) and returns null to the caller.  The caller views
>> this
>> as an indication that the session doesn't exist, and it must create a
>> brand
>> new one if it needs to use a Session.
>>
>> Because of this logic, those two methods are probably meaningless.  I also
>> think that our logic to ignore invalid sessions and create a brand new one
>> when necessary is the right way to go - it is how web containers work, and
>> everyone is pretty much happy with that behavior.
>>
>> Do you guys agree?  Do you see a need to retain them in the interface?
>> (They
>> would stay in the implementation because the implementation needs to use
>> them to determine if in fact it is invalid or not).
>>
>> I'm just looking for your thoughts if you have any...
>>
>> Les
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Jeremy Haile
> I agree, but wouldn't the interface want to interact with the  
> SessionDAO or
> SessionManager directly?

Yes - definitely.   We're on the same page there.

> And this would of course only be useful if the application wishes to  
> retain
> stopped/expired sessions, for, say reporting purposes that you  
> describe.
> That means they need to ensure additional cleanup logic clears out the
> retained sessions based on some criteria to ensure their session  
> data store
> (RDBMS, file-system, cache, etc) doesn't become huge.

Yeah - the support and performance of various method calls may vary by  
implementation.  We'd probably need to take this into account in  
designing the API and methods on SessionManager.


> We could provide this behavior of course, but we don't have anything  
> like
> that in place today.

Definitely.  As I mentioned in my original email, this are forward  
thinking thoughts.  I was only thinking that at some point in the  
future, we may want to have additional meta-data available about a  
session for this purpose.  Perhaps that additional data is made  
available via a sub-interface (i.e. ManagedSession)  What that meta-
data is and when we want to tackle this problem (if at all), is up in  
the air.

> I think it'd probably be better to remove those two from the current
> end-user facing API and only create the ManagedSession interface if we
> decided to go down the road of providing the above support.  In  
> other words,
> I don't know if it is something we need to tackle for 0.9 final.  If  
> you'd
> like for us to provide this kind of support, would you mind opening  
> a Jira
> issue so it is captured?

I don't mind opening one, although at this point I'd like to just  
gauge community interest in this feature.  I don't have a critical  
need for it - it's just something I've wished for in the past.  And I  
could see it being a beneficial feature for security-conscious  
applications.  For example, it could allow an administrator to  
invalidate an abusive user's session through an admin GUI.

> Or maybe such an issue is better suited for a UI implementation task?
> That'd be sweet, but it is not trivial, for sure.  To support this,  
> realms
> would also need to be mutable to receive end-user configuration  
> changes
> during runtime...

Yeah - I could see several UI sub-projects for JSecurity.  I think the  
core JSecurity library would be enhanced to provide the hooks needed  
by the GUI.  Then we could have optional wars that could be deployed  
such as jsecurity-session-management.war  or jsecurity-user-
management.war, etc.

> Should it be immutable?  If so, we could provide a wrapper
> 'ImmutableSession' implementation that throws exceptions on any  
> method call
> that tries to alter state and delegates to the wrapped Session when  
> just
> reading state, like getAttribute().  The wrapper would only be used  
> during
> listener notification, and the session would be 'unwrapped' for any
> remaining processing after listener notification....

My thoughts exactly.  We'd need to look closely at the session to see  
what should be mutable vs immutable.  But it's something that  
security-conscious framework should think about.  Immutability is good  
in many cases.

> Is this something you think we should do now?  Is it something that we
> should even enforce?

Maybe for 1.0, not for 0.9.0.

> I have no idea what the best practice would be, but for some reason  
> I really
> like the sound of the default not allowing modification.  It seems  
> more
> logical to me, because those notifications are supposed to indicate  
> that the
> session is in an invalid state and probably shouldn't be messed with
> anymore.

Yeah - I like the idea of certain parts of the session being immutable  
always - and other parts of the session being wrapped in an immutable  
implementation for listener events, etc.

We do need to think about things like when someone wants to use a  
session listener to set an attribute when a session is created.  That  
may be valid - so this does require some more thought as to what  
should be immutable and when it should be immutable.

>
>
>
> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>
>> So in my review of the code in prep for RC1 last night, I found  
>> something
>>> I'd never really thought about before:
>>>
>>> Should the Session *Interface* have the two methods  
>>> getStopTimestamp() and
>>> isExpired() at all?
>>>
>>> I remember those existing in the implementation specifically to  
>>> handle the
>>> case where Sessions were persistent beyond their user lifetime -  
>>> e.g. in
>>> environments that do user activity reporting, they could organize  
>>> user
>>> events based on sessions, etc.  But the framework couldn't use  
>>> stopped or
>>> expired sessions anymore.
>>>
>>> It seems as if implementation methods bubbled their way up into the
>>> interface.
>>>
>>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>>
>>> In our web support for example, these methods would never return  
>>> anything
>>> useful:  getStopTimestamp() would _always_ return null and  
>>> isExpired()
>>> would
>>> _always_ return false, because of our WebSessionManager logic:
>>>
>>> When a Subject is being constructed for the incoming Request, the
>>> WebSessionManager it tries to acquire the user's corresponding  
>>> session (if
>>> it exists).  If it does exist, but that session is invalid (that is,
>>> stopped
>>> or expired), it suppresses resulting InvalidSessionException  
>>> (prints out a
>>> trace message really) and returns null to the caller.  The caller  
>>> views
>>> this
>>> as an indication that the session doesn't exist, and it must  
>>> create a
>>> brand
>>> new one if it needs to use a Session.
>>>
>>> Because of this logic, those two methods are probably  
>>> meaningless.  I also
>>> think that our logic to ignore invalid sessions and create a brand  
>>> new one
>>> when necessary is the right way to go - it is how web containers  
>>> work, and
>>> everyone is pretty much happy with that behavior.
>>>
>>> Do you guys agree?  Do you see a need to retain them in the  
>>> interface?
>>> (They
>>> would stay in the implementation because the implementation needs  
>>> to use
>>> them to determine if in fact it is invalid or not).
>>>
>>> I'm just looking for your thoughts if you have any...
>>>
>>> Les
>>>
>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Les Hazlewood
Administrator
> Should it be immutable?  If so, we could provide a wrapper
>> 'ImmutableSession' implementation that throws exceptions on any method
>> call
>> that tries to alter state and delegates to the wrapped Session when just
>> reading state, like getAttribute().  The wrapper would only be used during
>> listener notification, and the session would be 'unwrapped' for any
>> remaining processing after listener notification....
>>
>
> My thoughts exactly.  We'd need to look closely at the session to see what
> should be mutable vs immutable.  But it's something that  security-conscious
> framework should think about.  Immutability is good in many cases.
>
>  Is this something you think we should do now?  Is it something that we
>> should even enforce?
>>
>
> Maybe for 1.0, not for 0.9.0.


Actually, I don't see any problems with adding this even today unless you
have objections - it is very unintrusive and wouldn't take any time at all.
The only methods that I can see that would throw an exception are:

setTimeout(long);
touch();
stop();
setAttribute(key,value);
removeAttribute(key);

Everything else is read-only already.

In fact, I would prefer that it is in now, to minimize any surprise later.
I'm not sure anyone is even using the SessionListener construct yet, so it'd
be nice to get this in before that might happen....



> I have no idea what the best practice would be, but for some reason I
>> really
>> like the sound of the default not allowing modification.  It seems more
>> logical to me, because those notifications are supposed to indicate that
>> the
>> session is in an invalid state and probably shouldn't be messed with
>> anymore.
>>
>
> Yeah - I like the idea of certain parts of the session being immutable
> always - and other parts of the session being wrapped in an immutable
> implementation for listener events, etc.
>
> We do need to think about things like when someone wants to use a session
> listener to set an attribute when a session is created.
>
 That may be valid - so this does require some more thought as to what
> should be immutable and when it should be immutable.


I definitely think its ok for a listener to do whatever they want to the
session upon creation.  But I do think the wrapper concept should be in
place today, and we can tweak exactly what the behavior may do in the future
- just starting off by throwing exceptions for the above methods.  That ok?


> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>
>>>
>>> So in my review of the code in prep for RC1 last night, I found something
>>>
>>>> I'd never really thought about before:
>>>>
>>>> Should the Session *Interface* have the two methods getStopTimestamp()
>>>> and
>>>> isExpired() at all?
>>>>
>>>> I remember those existing in the implementation specifically to handle
>>>> the
>>>> case where Sessions were persistent beyond their user lifetime - e.g. in
>>>> environments that do user activity reporting, they could organize user
>>>> events based on sessions, etc.  But the framework couldn't use stopped
>>>> or
>>>> expired sessions anymore.
>>>>
>>>> It seems as if implementation methods bubbled their way up into the
>>>> interface.
>>>>
>>>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>>>
>>>> In our web support for example, these methods would never return
>>>> anything
>>>> useful:  getStopTimestamp() would _always_ return null and isExpired()
>>>> would
>>>> _always_ return false, because of our WebSessionManager logic:
>>>>
>>>> When a Subject is being constructed for the incoming Request, the
>>>> WebSessionManager it tries to acquire the user's corresponding session
>>>> (if
>>>> it exists).  If it does exist, but that session is invalid (that is,
>>>> stopped
>>>> or expired), it suppresses resulting InvalidSessionException (prints out
>>>> a
>>>> trace message really) and returns null to the caller.  The caller views
>>>> this
>>>> as an indication that the session doesn't exist, and it must create a
>>>> brand
>>>> new one if it needs to use a Session.
>>>>
>>>> Because of this logic, those two methods are probably meaningless.  I
>>>> also
>>>> think that our logic to ignore invalid sessions and create a brand new
>>>> one
>>>> when necessary is the right way to go - it is how web containers work,
>>>> and
>>>> everyone is pretty much happy with that behavior.
>>>>
>>>> Do you guys agree?  Do you see a need to retain them in the interface?
>>>> (They
>>>> would stay in the implementation because the implementation needs to use
>>>> them to determine if in fact it is invalid or not).
>>>>
>>>> I'm just looking for your thoughts if you have any...
>>>>
>>>> Les
>>>>
>>>>
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Jeremy Haile
I don't have a problem adding it in now.  So would onStart() receive a  
mutable copy, but onStop/onExpiration receives an immutable copy?



On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote:

>> Should it be immutable?  If so, we could provide a wrapper
>>> 'ImmutableSession' implementation that throws exceptions on any  
>>> method
>>> call
>>> that tries to alter state and delegates to the wrapped Session  
>>> when just
>>> reading state, like getAttribute().  The wrapper would only be  
>>> used during
>>> listener notification, and the session would be 'unwrapped' for any
>>> remaining processing after listener notification....
>>>
>>
>> My thoughts exactly.  We'd need to look closely at the session to  
>> see what
>> should be mutable vs immutable.  But it's something that  security-
>> conscious
>> framework should think about.  Immutability is good in many cases.
>>
>> Is this something you think we should do now?  Is it something that  
>> we
>>> should even enforce?
>>>
>>
>> Maybe for 1.0, not for 0.9.0.
>
>
> Actually, I don't see any problems with adding this even today  
> unless you
> have objections - it is very unintrusive and wouldn't take any time  
> at all.
> The only methods that I can see that would throw an exception are:
>
> setTimeout(long);
> touch();
> stop();
> setAttribute(key,value);
> removeAttribute(key);
>
> Everything else is read-only already.
>
> In fact, I would prefer that it is in now, to minimize any surprise  
> later.
> I'm not sure anyone is even using the SessionListener construct yet,  
> so it'd
> be nice to get this in before that might happen....
>
>
>
>> I have no idea what the best practice would be, but for some reason I
>>> really
>>> like the sound of the default not allowing modification.  It seems  
>>> more
>>> logical to me, because those notifications are supposed to  
>>> indicate that
>>> the
>>> session is in an invalid state and probably shouldn't be messed with
>>> anymore.
>>>
>>
>> Yeah - I like the idea of certain parts of the session being  
>> immutable
>> always - and other parts of the session being wrapped in an immutable
>> implementation for listener events, etc.
>>
>> We do need to think about things like when someone wants to use a  
>> session
>> listener to set an attribute when a session is created.
>>
> That may be valid - so this does require some more thought as to what
>> should be immutable and when it should be immutable.
>
>
> I definitely think its ok for a listener to do whatever they want to  
> the
> session upon creation.  But I do think the wrapper concept should be  
> in
> place today, and we can tweak exactly what the behavior may do in  
> the future
> - just starting off by throwing exceptions for the above methods.  
> That ok?
>
>
>> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>>
>>>>
>>>> So in my review of the code in prep for RC1 last night, I found  
>>>> something
>>>>
>>>>> I'd never really thought about before:
>>>>>
>>>>> Should the Session *Interface* have the two methods  
>>>>> getStopTimestamp()
>>>>> and
>>>>> isExpired() at all?
>>>>>
>>>>> I remember those existing in the implementation specifically to  
>>>>> handle
>>>>> the
>>>>> case where Sessions were persistent beyond their user lifetime -  
>>>>> e.g. in
>>>>> environments that do user activity reporting, they could  
>>>>> organize user
>>>>> events based on sessions, etc.  But the framework couldn't use  
>>>>> stopped
>>>>> or
>>>>> expired sessions anymore.
>>>>>
>>>>> It seems as if implementation methods bubbled their way up into  
>>>>> the
>>>>> interface.
>>>>>
>>>>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>>>>
>>>>> In our web support for example, these methods would never return
>>>>> anything
>>>>> useful:  getStopTimestamp() would _always_ return null and  
>>>>> isExpired()
>>>>> would
>>>>> _always_ return false, because of our WebSessionManager logic:
>>>>>
>>>>> When a Subject is being constructed for the incoming Request, the
>>>>> WebSessionManager it tries to acquire the user's corresponding  
>>>>> session
>>>>> (if
>>>>> it exists).  If it does exist, but that session is invalid (that  
>>>>> is,
>>>>> stopped
>>>>> or expired), it suppresses resulting InvalidSessionException  
>>>>> (prints out
>>>>> a
>>>>> trace message really) and returns null to the caller.  The  
>>>>> caller views
>>>>> this
>>>>> as an indication that the session doesn't exist, and it must  
>>>>> create a
>>>>> brand
>>>>> new one if it needs to use a Session.
>>>>>
>>>>> Because of this logic, those two methods are probably  
>>>>> meaningless.  I
>>>>> also
>>>>> think that our logic to ignore invalid sessions and create a  
>>>>> brand new
>>>>> one
>>>>> when necessary is the right way to go - it is how web containers  
>>>>> work,
>>>>> and
>>>>> everyone is pretty much happy with that behavior.
>>>>>
>>>>> Do you guys agree?  Do you see a need to retain them in the  
>>>>> interface?
>>>>> (They
>>>>> would stay in the implementation because the implementation  
>>>>> needs to use
>>>>> them to determine if in fact it is invalid or not).
>>>>>
>>>>> I'm just looking for your thoughts if you have any...
>>>>>
>>>>> Les
>>>>>
>>>>>
>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Les Hazlewood
Administrator
Yep, that's what I'm thinking.

I'll commit the adjustments in just a few minutes so you can see if it works
for you or not...

On Thu, Jul 31, 2008 at 10:46 AM, Jeremy Haile <[hidden email]> wrote:

> I don't have a problem adding it in now.  So would onStart() receive a
> mutable copy, but onStop/onExpiration receives an immutable copy?
>
>
>
>
> On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote:
>
>  Should it be immutable?  If so, we could provide a wrapper
>>>
>>>> 'ImmutableSession' implementation that throws exceptions on any method
>>>> call
>>>> that tries to alter state and delegates to the wrapped Session when just
>>>> reading state, like getAttribute().  The wrapper would only be used
>>>> during
>>>> listener notification, and the session would be 'unwrapped' for any
>>>> remaining processing after listener notification....
>>>>
>>>>
>>> My thoughts exactly.  We'd need to look closely at the session to see
>>> what
>>> should be mutable vs immutable.  But it's something that
>>>  security-conscious
>>> framework should think about.  Immutability is good in many cases.
>>>
>>> Is this something you think we should do now?  Is it something that we
>>>
>>>> should even enforce?
>>>>
>>>>
>>> Maybe for 1.0, not for 0.9.0.
>>>
>>
>>
>> Actually, I don't see any problems with adding this even today unless you
>> have objections - it is very unintrusive and wouldn't take any time at
>> all.
>> The only methods that I can see that would throw an exception are:
>>
>> setTimeout(long);
>> touch();
>> stop();
>> setAttribute(key,value);
>> removeAttribute(key);
>>
>> Everything else is read-only already.
>>
>> In fact, I would prefer that it is in now, to minimize any surprise later.
>> I'm not sure anyone is even using the SessionListener construct yet, so
>> it'd
>> be nice to get this in before that might happen....
>>
>>
>>
>>  I have no idea what the best practice would be, but for some reason I
>>>
>>>> really
>>>> like the sound of the default not allowing modification.  It seems more
>>>> logical to me, because those notifications are supposed to indicate that
>>>> the
>>>> session is in an invalid state and probably shouldn't be messed with
>>>> anymore.
>>>>
>>>>
>>> Yeah - I like the idea of certain parts of the session being immutable
>>> always - and other parts of the session being wrapped in an immutable
>>> implementation for listener events, etc.
>>>
>>> We do need to think about things like when someone wants to use a session
>>> listener to set an attribute when a session is created.
>>>
>>>  That may be valid - so this does require some more thought as to what
>>
>>> should be immutable and when it should be immutable.
>>>
>>
>>
>> I definitely think its ok for a listener to do whatever they want to the
>> session upon creation.  But I do think the wrapper concept should be in
>> place today, and we can tweak exactly what the behavior may do in the
>> future
>> - just starting off by throwing exceptions for the above methods.  That
>> ok?
>>
>>
>>  On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>>
>>>>
>>>>
>>>>> So in my review of the code in prep for RC1 last night, I found
>>>>> something
>>>>>
>>>>>  I'd never really thought about before:
>>>>>>
>>>>>> Should the Session *Interface* have the two methods getStopTimestamp()
>>>>>> and
>>>>>> isExpired() at all?
>>>>>>
>>>>>> I remember those existing in the implementation specifically to handle
>>>>>> the
>>>>>> case where Sessions were persistent beyond their user lifetime - e.g.
>>>>>> in
>>>>>> environments that do user activity reporting, they could organize user
>>>>>> events based on sessions, etc.  But the framework couldn't use stopped
>>>>>> or
>>>>>> expired sessions anymore.
>>>>>>
>>>>>> It seems as if implementation methods bubbled their way up into the
>>>>>> interface.
>>>>>>
>>>>>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>>>>>
>>>>>> In our web support for example, these methods would never return
>>>>>> anything
>>>>>> useful:  getStopTimestamp() would _always_ return null and isExpired()
>>>>>> would
>>>>>> _always_ return false, because of our WebSessionManager logic:
>>>>>>
>>>>>> When a Subject is being constructed for the incoming Request, the
>>>>>> WebSessionManager it tries to acquire the user's corresponding session
>>>>>> (if
>>>>>> it exists).  If it does exist, but that session is invalid (that is,
>>>>>> stopped
>>>>>> or expired), it suppresses resulting InvalidSessionException (prints
>>>>>> out
>>>>>> a
>>>>>> trace message really) and returns null to the caller.  The caller
>>>>>> views
>>>>>> this
>>>>>> as an indication that the session doesn't exist, and it must create a
>>>>>> brand
>>>>>> new one if it needs to use a Session.
>>>>>>
>>>>>> Because of this logic, those two methods are probably meaningless.  I
>>>>>> also
>>>>>> think that our logic to ignore invalid sessions and create a brand new
>>>>>> one
>>>>>> when necessary is the right way to go - it is how web containers work,
>>>>>> and
>>>>>> everyone is pretty much happy with that behavior.
>>>>>>
>>>>>> Do you guys agree?  Do you see a need to retain them in the interface?
>>>>>> (They
>>>>>> would stay in the implementation because the implementation needs to
>>>>>> use
>>>>>> them to determine if in fact it is invalid or not).
>>>>>>
>>>>>> I'm just looking for your thoughts if you have any...
>>>>>>
>>>>>> Les
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Jeremy Haile
Sounds good.


On Jul 31, 2008, at 10:57 AM, Les Hazlewood wrote:

> Yep, that's what I'm thinking.
>
> I'll commit the adjustments in just a few minutes so you can see if  
> it works
> for you or not...
>
> On Thu, Jul 31, 2008 at 10:46 AM, Jeremy Haile <[hidden email]>  
> wrote:
>
>> I don't have a problem adding it in now.  So would onStart()  
>> receive a
>> mutable copy, but onStop/onExpiration receives an immutable copy?
>>
>>
>>
>>
>> On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote:
>>
>> Should it be immutable?  If so, we could provide a wrapper
>>>>
>>>>> 'ImmutableSession' implementation that throws exceptions on any  
>>>>> method
>>>>> call
>>>>> that tries to alter state and delegates to the wrapped Session  
>>>>> when just
>>>>> reading state, like getAttribute().  The wrapper would only be  
>>>>> used
>>>>> during
>>>>> listener notification, and the session would be 'unwrapped' for  
>>>>> any
>>>>> remaining processing after listener notification....
>>>>>
>>>>>
>>>> My thoughts exactly.  We'd need to look closely at the session to  
>>>> see
>>>> what
>>>> should be mutable vs immutable.  But it's something that
>>>> security-conscious
>>>> framework should think about.  Immutability is good in many cases.
>>>>
>>>> Is this something you think we should do now?  Is it something  
>>>> that we
>>>>
>>>>> should even enforce?
>>>>>
>>>>>
>>>> Maybe for 1.0, not for 0.9.0.
>>>>
>>>
>>>
>>> Actually, I don't see any problems with adding this even today  
>>> unless you
>>> have objections - it is very unintrusive and wouldn't take any  
>>> time at
>>> all.
>>> The only methods that I can see that would throw an exception are:
>>>
>>> setTimeout(long);
>>> touch();
>>> stop();
>>> setAttribute(key,value);
>>> removeAttribute(key);
>>>
>>> Everything else is read-only already.
>>>
>>> In fact, I would prefer that it is in now, to minimize any  
>>> surprise later.
>>> I'm not sure anyone is even using the SessionListener construct  
>>> yet, so
>>> it'd
>>> be nice to get this in before that might happen....
>>>
>>>
>>>
>>> I have no idea what the best practice would be, but for some  
>>> reason I
>>>>
>>>>> really
>>>>> like the sound of the default not allowing modification.  It  
>>>>> seems more
>>>>> logical to me, because those notifications are supposed to  
>>>>> indicate that
>>>>> the
>>>>> session is in an invalid state and probably shouldn't be messed  
>>>>> with
>>>>> anymore.
>>>>>
>>>>>
>>>> Yeah - I like the idea of certain parts of the session being  
>>>> immutable
>>>> always - and other parts of the session being wrapped in an  
>>>> immutable
>>>> implementation for listener events, etc.
>>>>
>>>> We do need to think about things like when someone wants to use a  
>>>> session
>>>> listener to set an attribute when a session is created.
>>>>
>>>> That may be valid - so this does require some more thought as to  
>>>> what
>>>
>>>> should be immutable and when it should be immutable.
>>>>
>>>
>>>
>>> I definitely think its ok for a listener to do whatever they want  
>>> to the
>>> session upon creation.  But I do think the wrapper concept should  
>>> be in
>>> place today, and we can tweak exactly what the behavior may do in  
>>> the
>>> future
>>> - just starting off by throwing exceptions for the above methods.  
>>> That
>>> ok?
>>>
>>>
>>> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>>>
>>>>>
>>>>>
>>>>>> So in my review of the code in prep for RC1 last night, I found
>>>>>> something
>>>>>>
>>>>>> I'd never really thought about before:
>>>>>>>
>>>>>>> Should the Session *Interface* have the two methods  
>>>>>>> getStopTimestamp()
>>>>>>> and
>>>>>>> isExpired() at all?
>>>>>>>
>>>>>>> I remember those existing in the implementation specifically  
>>>>>>> to handle
>>>>>>> the
>>>>>>> case where Sessions were persistent beyond their user lifetime  
>>>>>>> - e.g.
>>>>>>> in
>>>>>>> environments that do user activity reporting, they could  
>>>>>>> organize user
>>>>>>> events based on sessions, etc.  But the framework couldn't use  
>>>>>>> stopped
>>>>>>> or
>>>>>>> expired sessions anymore.
>>>>>>>
>>>>>>> It seems as if implementation methods bubbled their way up  
>>>>>>> into the
>>>>>>> interface.
>>>>>>>
>>>>>>> Would a user ever need to call getStopTimestamp() or  
>>>>>>> isExpired()?
>>>>>>>
>>>>>>> In our web support for example, these methods would never return
>>>>>>> anything
>>>>>>> useful:  getStopTimestamp() would _always_ return null and  
>>>>>>> isExpired()
>>>>>>> would
>>>>>>> _always_ return false, because of our WebSessionManager logic:
>>>>>>>
>>>>>>> When a Subject is being constructed for the incoming Request,  
>>>>>>> the
>>>>>>> WebSessionManager it tries to acquire the user's corresponding  
>>>>>>> session
>>>>>>> (if
>>>>>>> it exists).  If it does exist, but that session is invalid  
>>>>>>> (that is,
>>>>>>> stopped
>>>>>>> or expired), it suppresses resulting InvalidSessionException  
>>>>>>> (prints
>>>>>>> out
>>>>>>> a
>>>>>>> trace message really) and returns null to the caller.  The  
>>>>>>> caller
>>>>>>> views
>>>>>>> this
>>>>>>> as an indication that the session doesn't exist, and it must  
>>>>>>> create a
>>>>>>> brand
>>>>>>> new one if it needs to use a Session.
>>>>>>>
>>>>>>> Because of this logic, those two methods are probably  
>>>>>>> meaningless.  I
>>>>>>> also
>>>>>>> think that our logic to ignore invalid sessions and create a  
>>>>>>> brand new
>>>>>>> one
>>>>>>> when necessary is the right way to go - it is how web  
>>>>>>> containers work,
>>>>>>> and
>>>>>>> everyone is pretty much happy with that behavior.
>>>>>>>
>>>>>>> Do you guys agree?  Do you see a need to retain them in the  
>>>>>>> interface?
>>>>>>> (They
>>>>>>> would stay in the implementation because the implementation  
>>>>>>> needs to
>>>>>>> use
>>>>>>> them to determine if in fact it is invalid or not).
>>>>>>>
>>>>>>> I'm just looking for your thoughts if you have any...
>>>>>>>
>>>>>>> Les
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Session interface modification?

Les Hazlewood
Administrator
Done and committed.

Cheers,

Les

On Thu, Jul 31, 2008 at 11:01 AM, Jeremy Haile <[hidden email]> wrote:

> Sounds good.
>
>
>
> On Jul 31, 2008, at 10:57 AM, Les Hazlewood wrote:
>
>  Yep, that's what I'm thinking.
>>
>> I'll commit the adjustments in just a few minutes so you can see if it
>> works
>> for you or not...
>>
>> On Thu, Jul 31, 2008 at 10:46 AM, Jeremy Haile <[hidden email]>
>> wrote:
>>
>>  I don't have a problem adding it in now.  So would onStart() receive a
>>> mutable copy, but onStop/onExpiration receives an immutable copy?
>>>
>>>
>>>
>>>
>>> On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote:
>>>
>>> Should it be immutable?  If so, we could provide a wrapper
>>>
>>>>
>>>>>  'ImmutableSession' implementation that throws exceptions on any method
>>>>>> call
>>>>>> that tries to alter state and delegates to the wrapped Session when
>>>>>> just
>>>>>> reading state, like getAttribute().  The wrapper would only be used
>>>>>> during
>>>>>> listener notification, and the session would be 'unwrapped' for any
>>>>>> remaining processing after listener notification....
>>>>>>
>>>>>>
>>>>>>  My thoughts exactly.  We'd need to look closely at the session to see
>>>>> what
>>>>> should be mutable vs immutable.  But it's something that
>>>>> security-conscious
>>>>> framework should think about.  Immutability is good in many cases.
>>>>>
>>>>> Is this something you think we should do now?  Is it something that we
>>>>>
>>>>>  should even enforce?
>>>>>>
>>>>>>
>>>>>>  Maybe for 1.0, not for 0.9.0.
>>>>>
>>>>>
>>>>
>>>> Actually, I don't see any problems with adding this even today unless
>>>> you
>>>> have objections - it is very unintrusive and wouldn't take any time at
>>>> all.
>>>> The only methods that I can see that would throw an exception are:
>>>>
>>>> setTimeout(long);
>>>> touch();
>>>> stop();
>>>> setAttribute(key,value);
>>>> removeAttribute(key);
>>>>
>>>> Everything else is read-only already.
>>>>
>>>> In fact, I would prefer that it is in now, to minimize any surprise
>>>> later.
>>>> I'm not sure anyone is even using the SessionListener construct yet, so
>>>> it'd
>>>> be nice to get this in before that might happen....
>>>>
>>>>
>>>>
>>>> I have no idea what the best practice would be, but for some reason I
>>>>
>>>>>
>>>>>  really
>>>>>> like the sound of the default not allowing modification.  It seems
>>>>>> more
>>>>>> logical to me, because those notifications are supposed to indicate
>>>>>> that
>>>>>> the
>>>>>> session is in an invalid state and probably shouldn't be messed with
>>>>>> anymore.
>>>>>>
>>>>>>
>>>>>>  Yeah - I like the idea of certain parts of the session being
>>>>> immutable
>>>>> always - and other parts of the session being wrapped in an immutable
>>>>> implementation for listener events, etc.
>>>>>
>>>>> We do need to think about things like when someone wants to use a
>>>>> session
>>>>> listener to set an attribute when a session is created.
>>>>>
>>>>> That may be valid - so this does require some more thought as to what
>>>>>
>>>>
>>>>  should be immutable and when it should be immutable.
>>>>>
>>>>>
>>>>
>>>> I definitely think its ok for a listener to do whatever they want to the
>>>> session upon creation.  But I do think the wrapper concept should be in
>>>> place today, and we can tweak exactly what the behavior may do in the
>>>> future
>>>> - just starting off by throwing exceptions for the above methods.  That
>>>> ok?
>>>>
>>>>
>>>> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote:
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>  So in my review of the code in prep for RC1 last night, I found
>>>>>>> something
>>>>>>>
>>>>>>> I'd never really thought about before:
>>>>>>>
>>>>>>>>
>>>>>>>> Should the Session *Interface* have the two methods
>>>>>>>> getStopTimestamp()
>>>>>>>> and
>>>>>>>> isExpired() at all?
>>>>>>>>
>>>>>>>> I remember those existing in the implementation specifically to
>>>>>>>> handle
>>>>>>>> the
>>>>>>>> case where Sessions were persistent beyond their user lifetime -
>>>>>>>> e.g.
>>>>>>>> in
>>>>>>>> environments that do user activity reporting, they could organize
>>>>>>>> user
>>>>>>>> events based on sessions, etc.  But the framework couldn't use
>>>>>>>> stopped
>>>>>>>> or
>>>>>>>> expired sessions anymore.
>>>>>>>>
>>>>>>>> It seems as if implementation methods bubbled their way up into the
>>>>>>>> interface.
>>>>>>>>
>>>>>>>> Would a user ever need to call getStopTimestamp() or isExpired()?
>>>>>>>>
>>>>>>>> In our web support for example, these methods would never return
>>>>>>>> anything
>>>>>>>> useful:  getStopTimestamp() would _always_ return null and
>>>>>>>> isExpired()
>>>>>>>> would
>>>>>>>> _always_ return false, because of our WebSessionManager logic:
>>>>>>>>
>>>>>>>> When a Subject is being constructed for the incoming Request, the
>>>>>>>> WebSessionManager it tries to acquire the user's corresponding
>>>>>>>> session
>>>>>>>> (if
>>>>>>>> it exists).  If it does exist, but that session is invalid (that is,
>>>>>>>> stopped
>>>>>>>> or expired), it suppresses resulting InvalidSessionException (prints
>>>>>>>> out
>>>>>>>> a
>>>>>>>> trace message really) and returns null to the caller.  The caller
>>>>>>>> views
>>>>>>>> this
>>>>>>>> as an indication that the session doesn't exist, and it must create
>>>>>>>> a
>>>>>>>> brand
>>>>>>>> new one if it needs to use a Session.
>>>>>>>>
>>>>>>>> Because of this logic, those two methods are probably meaningless.
>>>>>>>>  I
>>>>>>>> also
>>>>>>>> think that our logic to ignore invalid sessions and create a brand
>>>>>>>> new
>>>>>>>> one
>>>>>>>> when necessary is the right way to go - it is how web containers
>>>>>>>> work,
>>>>>>>> and
>>>>>>>> everyone is pretty much happy with that behavior.
>>>>>>>>
>>>>>>>> Do you guys agree?  Do you see a need to retain them in the
>>>>>>>> interface?
>>>>>>>> (They
>>>>>>>> would stay in the implementation because the implementation needs to
>>>>>>>> use
>>>>>>>> them to determine if in fact it is invalid or not).
>>>>>>>>
>>>>>>>> I'm just looking for your thoughts if you have any...
>>>>>>>>
>>>>>>>> Les
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>
Loading...