+beaton (for domain member question) I'll add some docs to the committed code. expiresAt is informational, isExpired() is canonical and convenient enough that I'm betting that most implementers will use it, and it can encompass things like a CRL, blacklist, etc. depending on the implementation.
I don't think that ContainerConfig calls can be spoofed when you're using BlobCrypter which guarantees against tampering. I am concerned that there are a number of pieces of code that iterate through all containers, this works for small numbers of containers, but not large populations (where a container == a third party site). On Sat, May 8, 2010 at 7:59 PM, John Hjelmstad <[email protected]> wrote: > On Sat, May 8, 2010 at 7:33 AM, Paul Lindner <[email protected]> wrote: > > > I'd like to add isExpired() to the SecurityToken API to support use-cases > > where the expiration time is not known but the token is known to be > > expired. > > To avoid boilerplate I've created an AbstractSecurityToken class that > > implementations can extend. It has this method: > > > > boolean isExpired() { > > Long expiresAt = getExpiresAt(); > > return (expiresAt == null) ? false : System.currentTimeMillis() > > > expiresAt; > > } > > > > An abstract base class helps avoid boilerplate, but I'm curious what use > cases you have in mind for tokens that are > expired-but-not-because-expiry-time-has-passed. > > The one that comes to mind for me are single-use tokens, which might have a > hybrid scheme (for example): expires by time X *OR* if ever used at all, > whichever comes first. > > In such case, an isExpired method definitely makes good sense, though I'd > remain very mildly concerned about code paths that confuse the interface's > API use ie. checking getExpiredAt() when isExpired() says "false" anyway. I > guess the best we can do in this situation is just document that > isExpired() > takes primacy over getExpiredAt()? Or do you have any other thoughts on > specific use cases? > > > > > > Also, more questions > > > > * getDomain() is only called once, it's used to add a default consumer > key > > for oauth requests. Since this is set by the ContainerConfig might it > make > > sense to not pass around this info in the token and instead grab it from > > the > > config as needed? We already store the container name in the token. > > > > To be concrete, the idea is to drop getDomain() from the API and replace it > w/ something ContainerConfig driven? > > Seems sensible, though I'd defer to Brian Eaton and others for input. The > only vague concern is that ContainerConfig values can generally be spoofed, > since container values can trivially be switched out. I'm not sure if > there's any issue there. > > > > > > * BlobCrypter classes use a fixed expiration time and stores the > generated > > time instead. Refactoring will be needed here... > > > > Agreed. > > Cheers, > John > > > > > > On Fri, May 7, 2010 at 7:55 PM, John Hjelmstad <[email protected]> wrote: > > > > > On Fri, May 7, 2010 at 5:59 PM, Paul Lindner <[email protected]> > wrote: > > > > > > > I propose that we add expiration methods to the SecurityToken > > interface. > > > > This is necessary for future OAuth2 compatibility. Here's the > > proposed > > > > interface: > > > > > > > > /** > > > > * @return the UTC timestamp (in milliseconds) that this token > expires > > > or > > > > null if unknown or indeterminate > > > > */ > > > > Long getExpiresAt(); > > > > > > > > > > +1 > > > > > > > > > > > > > > /** > > > > * @return true if the token is no longer valid > > > > */ > > > > boolean isExpired(); > > > > > > > > > > +0 or maybe -0.1 -- is this really necessary? Seems like boilerplate. > > > > > > --j > > > > > >
