fix AbstractToken endOfLife initialization

[Darunada]

The timestamp loaded from token storage is an absolute time, not a number of seconds from now.

I checked a hand full of services and they all set the lifetime as a relative number of seconds correctly using setLifetime(). However, when loading from token storage, it uses the constructor and passes in the absolute time the token expires, not a relative time, so it should be set with setEndOfLife().

[Darunada]

Oh, I didn't update the tests, I'll get it tomorrow. My bad.

[elliotchance]

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/OAuth/Common/Token/AbstractToken.php, line 116 [r2] (raw file): The implementation of this method is basically the same as setEndOfLife() except for this line. Wouldn't it make sense to reduce the duplicate logic into a single method and decide based on the size of the EOL what they actually mean. For example, if the expiry time is 3600 surely they mean "one hour from now" and not Thu, 01 Jan 1970 01:00:00 GMT.

public function setEndOfLife($endOfLife)
{
    $this->setLifetime($endOfLife);
}

public function setLifetime($lifetime)
{
    // take an EOL constant or an expiration timestamp
    if (0 === $endOfLife || static::EOL_NEVER_EXPIRES === $endOfLife) {
        $this->endOfLife = static::EOL_NEVER_EXPIRES;
    } elseif (null !== $endOfLife) {
        $eol = intval($endOfLife);

        // If the EOL is less than the number of seconds in a decade then surely
        // they mean a TTL. Otherwise they must be talking about an absolute
        // time.
        // 
        // We do not need to handle the zero condition here because it's handled
        // above.
        $tenYears = 86400 * 365 * 10;
        if ($eol < $tenYears) {
            $eol += time();
        }

        $this->endOfLife = $endOfLife;
    } else {
        $this->endOfLife = static::EOL_UNKNOWN;
    }
}

Then we could deprecate one of the methods.

---

Comments from the review on Reviewable.io

[Darunada]

I generally prefer to avoid those kinds of assumptions. To me, having multiple methods, one for an absolute time, and one for a relative time, is very explicit and behaves as expected.

Allowing setEndOfLife to accept the EOL constants was just a convenience that prevented a few of the service integrations from breaking. Bitly, Github, and Instagram among them. It would be just as easy to convert those services to use setLifetime for the constants, but the EOL prefix on the constants suggests to me they should in fact be used with setEndOfLife.

Also, you set the deadline at 10 years, which does seem reasonable. After all who would have a code that expired in 1979? But, my opinion on that, is who are we to set those kinds of arbitrary limits? (or denying that capability, if someone wanted to for some reason)

[elliotchance]

I agree with everything you say, explicitness is always better and it's seldom used in PHP. However, that's assuming the caller understands the explicitness (since the compiler won't enforce it*) and that it doesn't change in the future. It's possible that the caller would use the wrong method and for example would cause a token to live a lot longer than it should - this would be a hard bug to catch until tokens started expiring and you must debug it. Even more painful to fix and wait for a patch.

On top of human error, there are a lot of changes that just occur because they've changed their APIs (working in such a potentially brittle environment) and once again this changes are almost never caught on the way to production, especially since theres no integration tests. This makes me want to lean more towards a more full proof but also generic solution.

* In a strongly typed language I would use the types of enforce this like setEndOfLife(DateTime) and setLifetime(int). This should prevent almost all general mistakes, but PHP has to remain more flexible to handle all the conditions where we cannot rely on the type.

Perhaps theres another approach I'm not considering that gives the best of both... Thoughts?

[Darunada]

I initially found the problem because I was trying to see if my tokens were expired and getting an incorrect value. The AbstractToken constructor was taking the absolute expiration time from the constructor, (from a token storage), passing it through setLifeTime and it was overflowing the 32 bit integer value because it was doing time() + the absolute time, giving an obviously wrong negative timestamp. With a 64 bit integer it was going to the far future, 2062 if I recall correctly (or approximately time()+time()), for a token with a short lifetime.

So, aside from a handful of exceptions which are easily fixed, the current way it's being used is that the token storage interfaces use setEndOfLife (through the constructor, now) to initialize the token, and the services are using setLifeTime when they create new tokens.

What if we deprecate setEndOfLife, let the AbstractToken constructor take the end of life value and put it into the property directly instead of through the setter, and encourage service implementations to use setLifeTime for all cases, leaving the implementation of setLifeTime as it is?

[elliotchance]

How you discovered the bug; where it was doing a calculation when it wasn't needed is exactly the kind of error it needs to be resilient against. If we remove the human element (to a reasonable degree) ultimately it should work better than trying to educate, or protect against someone misunderstanding the service API.

I agree with you that deprecating one of them is ideal. However, I think the constructor should pass the life time through to the setter so there's one less moving part (if that's what you meant). However, I still think it's necessary to be reasonable in trying to understand what they mean by the "life time" to prevent obvious errors.

If it is the case they are doing something whack like "this token expires in 20 years" that's such an exceptional case that it can be handled specially where 20 years can be converted into an absolute time and fall through the same handler with safety.

Does that make sense or did I miss something?