Hi Jerome,
Emotionally, I usually prefer setters too, for their more literate
configuration. I think that's how the new TicketGrantingTicketExpirationPolicy
got setters rather than constructor injection of its parameters, because this:
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
p:maxTimeToLiveInMilliSeconds="28800000"
p:timeToKillInMilliSeconds="7200000" />
is more readable than:
<bean id="serviceTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.MultiTimeUseOrTimeoutExpirationPolicy">
<constructor-arg
index="0"
value="1" />
<constructor-arg
index="1"
value="10000" />
</bean>
However, the Spring 3.1 c: namespace solves the literate XML configuration
problem, such that injecting constructor parameters can be every bit as
literate as invoking JavaBean property mutators, vis:
<bean id="serviceTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.MultiTimeUseOrTimeoutExpirationPolicy"
c:numberOfUses="1" … />
Such that my greatest source of heartburn with constructor parameters has been
resolved by Spring 3.1. Great.
You raise an interesting point about setters being easier to cope with in
inheriting classes, rather than dealing with superclass constructors.
I don't look at the expiration policies and think what we need is more
composition by inheritance, however. Now that there's a suitable expiration
policy for TGTs (implementing sliding window and hard timeout) and a suitable
expiration policy for STs (implementing one-time-use and hard timeout), we're
done, right? Expected things should be easy. Unexpected things should be
possible -- and they are, one can always write a new implementation of
ExpirationPolicy. And, ideally, holler on the list about what additional
wrinkle about ticket expiration hasn't yet been fully explored in the shipping
code.
I guess I'm saying I'm not seeing a lot of value in designing for further
inheritance here, and I'd trade off designing for further inheritance for
instead designing for ease of configuration of the two expiration policies that
are going to be most commonly adopted, for good configuration checking on these.
I agree that seconds are an improvement over milliseconds. I don't agree that
adding the concept of TimeUnit is too heavy.
This:
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveUnit-ref="HOURS"
c:maxTimeToLive="8"
…. />
feels easier to understand than this:
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveInSeconds="28800"
… />
Especially since the first thing I'd do in configuring the latter is
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveInSeconds="28800" <!-- 8 hours -->
… />
Constructor injection allows the ticket expiration policy to do some argument
checking on construction and to be immutable after construction. I don't think
it matters much, but getting a helpful error message when I fat finger
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveUnit-ref="HOURS"
c:maxTimeToLive="-8"
… />
or indeed when I get confused about all this living and killing and try to
configure
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveUnit-ref="HOURS"
c:maxTimeToLive="2"
c:timeToKillUnit-ref="HOURS"
c:timeToKill="8"/>
I admit this configuration checking can be accomplished in an
afterPropertiesSet() such that it is available for setter injection as well,
and I admit that in the real world this is spring wired and it's not like
anything is going to call those setters after initialization to goof up the
configuration state that was verified by afterPropertiesSet, so I admit not
much at all rides on whether constructor or setter is used here and whether
TicketGrantingTicketExpirationPolicy is immutable or not. It doesn't matter
much. I'm saying I value making this clean and immutable more than I value
designing for inheritance of the implementations.
I don't value backwards compatibility here. I don't think it's necessary to
retain exactly the CAS 3.4 class names and configuration syntax. Adopters
either haven't touched ticketExpirationPolicies.xml, in which case they'll get
the new configuration by default via their Maven overlays, or they have touched
it, in which case they will want to re-do their configurations to make use of
the new TicketGrantingTicketExpirationPolicy and will, I expect, welcome a
configuration syntax with smaller, more literate quantities of time units
conceptually closer to what they're trying to accomplish. There will be a few
adopters who already updated configuration to implement the new
TicketGrantingTicketExpirationPolicy in 3.4.11; I really think they can cope
with touching this file again in reviewing their local changes versus the 3.5
base when they update their local overlays to adopt 3.5.
If I were a CAS adopter articulating the single sign on session policy to my
CIO in an elevator, I'd say
Single sign on sessions should last for up to 12 hours, timing out after 30
minutes of inactivity.
And then I'd go configure:
<bean id="grantingTicketExpirationPolicy"
class="org.jasig.cas.ticket.support.TicketGrantingTicketExpirationPolicy"
c:maxTimeToLiveUnit-ref="HOURS"
c:maxTimeToLive="12"
c:timeToKillUnit-ref="MINUTES"
c:timeToKill="30"/>
Or, even better, in cas.properties I'd put
## What is the longest time an end-user CAS single sign on session should last?
## After the session times out, users will need to log in to CAS again
## the next time they wish to use CAS to log in to an application.
## Units must be exactly HOURS , MINUTES , or SECONDS .
tgt.maxSingleSignOnDurationUnits=HOURS
tgt.maxSingleSignOnDuration=12
## Within that maximum duration of a single sign-on session, when should idle
sessions be expired?
## (Single sign-on sessions are 'used' and this idle timer reset when they're
exercised to obtain an ST
## to log in to an application)
## Units must be exactly HOURS, MINUTES, or SECONDS .
tgt.expireInactiveSingleSignOnSessionsAfterUnits=MINUTES
tgt.expireInactiveSingleSignOnSessionsAfter=30
and ticketExpirationPolicies.xml would include those placeholders, so I could
blissfully not fork that XML file in my local overlay.
That seems to me to be the very best way to serve existing CAS adopters who
have configured these timeouts in ticketExpirationPolicies.xml -- make touching
that file unnecessary for common configuration, supporting accomplishing that
in cas.properties.
Kind regards,
Andrew
On Mar 14, 2012, at 5:41 AM, jleleu wrote:
> Hi,
>
> I'm also for improving configuration experience as soon as possible.
>
> We need consistency and simplicity. As I look at the
> ticketExpirationPolicies.xml file, I see that the
> serviceTicketExpirationPolicy is configured by constructor and
> granticketTicketExpirationPolicy by setters.
>
> I don't like the current constructor of MultiTimeUseOrTimeoutExpirationPolicy
> as you don't know if it's a timeout in millisecondes or in seconds.
> I don't really like the idea of time unit as I find it as an « heavy » syntax.
> I always prefer setters to constructors because I find setters easier when
> inheriting classes.
>
> I like the syntax with getters : I would use it for all expiration policies.
> I would remove the multi-parameters constructor of
> MultiTimeUseOrTimeoutExpirationPolicy and use proper setters with explicit
> names refering to time units : setMaxTimeToLiveInMilliSeconds and
> setMaxTimeToLiveInSeconds (I think seconds and milliseconds are the only
> options we need to propose). Off course, setters don't have custom behaviour,
> they just set the value whatever the previous value.
>
> I would also consider creating a proper hierarchy of expiration policies with
> explicit names :
> - TimeToKillExpirationPolicy implements ExpirationPolicy (with timeToKill
> property and setters)
> - NumberOfUseTimeToKillExpirationPolicy inherits from
> TimeToKillExpirationPolicy (and add numberOfUses property with setter)
> - TimeToLiveTimeToKillExpirationPolicy inherits from
> TimeToKillExpirationPolicy (and add timeToLiveProperty with setters)
> To keep backward compatibility, I would create
> MultiTimeUseOrTimeoutExpirationPolicy class (which just inherits from
> NumberOfUseTimeToKillExpirationPolicy) and
> TicketGrantingTicketExpirationPolicy class (which just inherits from
> TimeToLiveTimeToKillExpirationPolicy).
>
> Best regards,
> Jérôme
>
> --
> You are currently subscribed to [email protected] as: [email protected]
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
--
You are currently subscribed to [email protected] as:
[email protected]
To unsubscribe, change settings or access archives, see
http://www.ja-sig.org/wiki/display/JSG/cas-dev
