Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow time to be fetched from a universal clock ⏱ #866

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/Provider/AbstractProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ abstract class AbstractProvider
*/
protected $optionProvider;

/**
* @var Clock
*/
protected $clock;

/**
* Constructs an OAuth 2.0 service provider.
*
Expand Down Expand Up @@ -138,6 +143,11 @@ public function __construct(array $options = [], array $collaborators = [])
$collaborators['optionProvider'] = new PostAuthOptionProvider();
}
$this->setOptionProvider($collaborators['optionProvider']);

if (empty($collaborators['clock'])) {
$collaborators['clock'] = new Clock();
}
$this->setClock($collaborators['clock']);
}

/**
Expand Down Expand Up @@ -252,6 +262,31 @@ public function getOptionProvider()
return $this->optionProvider;
}

/**
* Sets the clock.
*
* @param Clock $clock
*
* @return self
*/
public function setClock(Clock $clock)
{
$this->clock = $clock;

return $this;
}


/**
* Returns the clock.
*
* @return Clock
*/
public function getClock()
{
return $this->clock;
}

/**
* Returns the current value of the state parameter.
*
Expand Down Expand Up @@ -541,6 +576,7 @@ public function getAccessToken($grant, array $options = [])
);
}
$prepared = $this->prepareAccessTokenResponse($response);
$prepared['clock'] = $this->clock;
$token = $this->createAccessToken($prepared, $grant);

return $token;
Expand Down
20 changes: 20 additions & 0 deletions src/Provider/Clock.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace League\OAuth2\Client\Provider;

/**
* Represents an implementation of a Clock.
*/
class Clock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to have something like ClockInterface here to require the now() method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im all for it but interfaces seem to be used sparingly on this project... I wish providers had an interface.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an interface is appropriate here, because the implementation of ClockInterface should be final class ....

{

/**
* Get the current time.
*
* @return \DateTimeImmutable
*/
public function now()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this method should be getTime() instead of now(), so that Clock could return any DateTime object that an implementer wants to set (fixed, now, etc.)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used now as thats the same method implemented by the reference clock projects linked in summary. Also a getTime is strictly speaking not appropriate as you're getting more than just time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now() is my preference.

{
return new \DateTimeImmutable();
}
}
40 changes: 34 additions & 6 deletions src/Token/AccessToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace League\OAuth2\Client\Token;

use InvalidArgumentException;
use League\OAuth2\Client\Provider\Clock;
use RuntimeException;

/**
Expand Down Expand Up @@ -50,12 +51,21 @@ class AccessToken implements AccessTokenInterface, ResourceOwnerAccessTokenInter
protected $values = [];

/**
* @var int
* The current time, or NULL to get the true current time via PHP.
*
* @var int|null
*/
private static $timeNow;

/**
* Set the time now. This should only be used for testing purposes.
* The clock.
*
* @var Clock
*/
protected $clock;

/**
* Sets the current time.
*
* @param int $timeNow the time in seconds since epoch
* @return void
Expand All @@ -66,7 +76,7 @@ public static function setTimeNow($timeNow)
}

/**
* Reset the time now if it was set for test purposes.
* Reset the current time so the true current time via PHP is used.
*
* @return void
*/
Expand All @@ -76,11 +86,25 @@ public static function resetTimeNow()
}

/**
* @return int
* @inheritdoc
*/
public function setClock(Clock $clock)
{
$this->clock = $clock;
}

/**
* @inheritdoc
*/
public function getTimeNow()
{
return self::$timeNow ? self::$timeNow : time();
if (self::$timeNow) {
return self::$timeNow;
} elseif (isset($this->clock)) {
return $this->clock->now()->getTimestamp();
} else {
return time();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you implement a getClock(): Clock method that must always return a Clock, then there's no need to check for isset($this->clock). Instead, you can do:

if (self::$timeNow) {
    return self::$timeNow;
}

return $this->getClock()->now()->getTimestamp();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better for self::$timeNow to be a DateTimeImmutable, rather than an integer.

}
}

/**
Expand All @@ -106,6 +130,10 @@ public function __construct(array $options = [])
$this->refreshToken = $options['refresh_token'];
}

if (!empty($options['clock'])) {
$this->clock = $options['clock'];
}

// We need to know when the token expires. Show preference to
// 'expires_in' since it is defined in RFC6749 Section 5.1.
// Defer to 'expires' if it is provided instead.
Expand Down Expand Up @@ -196,7 +224,7 @@ public function hasExpired()
throw new RuntimeException('"expires" is not set on the token');
}

return $expires < time();
return $expires < $this->getTimeNow();
}

/**
Expand Down
17 changes: 17 additions & 0 deletions src/Token/AccessTokenInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace League\OAuth2\Client\Token;

use JsonSerializable;
use League\OAuth2\Client\Provider\Clock;
use RuntimeException;

interface AccessTokenInterface extends JsonSerializable
Expand Down Expand Up @@ -69,4 +70,20 @@ public function __toString();
* @return array
*/
public function jsonSerialize();

/**
* Sets the clock.
*
* @param Clock $clock a clock.
*
* @return void
*/
public function setClock(Clock $clock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically, we want interfaces to define things we can fetch, not necessarily how they are set. The way something is set could be left open to each implementation, but we'd want to ensure that all implementations have a way to get the clock (i.e., getClock(): Clock).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I dont understand what you're saying here. If this is about clock interfaces, im all for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying I would prefer to define a getClock() method on the AccessTokenInterface, rather than a setClock() method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to this interface don't make sense. An access token does not have a "clock", it has an expiry date/time. I think the only reasonable modification here would be AccessTokenInterface::getExpiresDate() that returns DateTimeImmutable. Given that getExpires returns a UNIX timestamp already, it makes no sense for the access token to have a separate clock.


/**
* Get the current time, whether real or simulated.
*
* @return int
*/
public function getTimeNow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these methods added to the interface represent a BC break.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, of course

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you choose to add getClock(): Clock, and the Clock has a now(): DateTimeInterface method, then is there any reason to have getTimeNow()?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont see how both getTimeNow and a theoretical public getClock() method would be useful for access tokens. See also my note about whether you'd prefer for the previously implemented time functionality (getTimeNow/setTimeNow/resetTimeNow/\League\OAuth2\Client\Token\AccessToken::$timeNow) to be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add a getClock() method, then I agree that getTimeNow() is no longer necessary.

}
9 changes: 8 additions & 1 deletion src/Tool/MacAuthorizationTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

namespace League\OAuth2\Client\Tool;

use League\OAuth2\Client\Provider\Clock;
use League\OAuth2\Client\Token\AccessToken;
use League\OAuth2\Client\Token\AccessTokenInterface;

Expand All @@ -24,6 +25,12 @@
*/
trait MacAuthorizationTrait
{

/**
* @var Clock
*/
protected $clock;

/**
* Returns the id of this token for MAC generation.
*
Expand Down Expand Up @@ -68,7 +75,7 @@ protected function getAuthorizationHeaders($token = null)
return [];
}

$ts = time();
$ts = $this->clock->now()->getTimestamp();
$id = $this->getTokenId($token);
$nonce = $this->getRandomState(16);
$mac = $this->getMacSignature($id, $ts, $nonce);
Expand Down
31 changes: 29 additions & 2 deletions test/src/Provider/AbstractProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace League\OAuth2\Client\Test\Provider;

use League\OAuth2\Client\OptionProvider\PostAuthOptionProvider;
use League\OAuth2\Client\Provider\Clock;
use Mockery;
use ReflectionClass;
use UnexpectedValueException;
Expand All @@ -24,6 +25,14 @@

class AbstractProviderTest extends TestCase
{

/**
* The current simulated time.
*
* @var int
*/
const NOW = 1359504000;

protected function getMockProvider()
{
return new MockProvider([
Expand All @@ -41,6 +50,14 @@ public function testGetOptionProvider()
);
}

public function testGetClock()
{
$this->assertInstanceOf(
Clock::class,
$this->getMockProvider()->getClock()
);
}

public function testInvalidGrantString()
{
$this->expectException(InvalidGrantException::class);
Expand Down Expand Up @@ -504,7 +521,7 @@ public function testGetAccessToken($method)

$provider->setAccessTokenMethod($method);

$raw_response = ['access_token' => 'okay', 'expires' => time() + 3600, 'resource_owner_id' => 3];
$raw_response = ['access_token' => 'okay', 'expires' => static::NOW + 3600, 'resource_owner_id' => 3];

$grant = Mockery::mock(AbstractGrant::class);
$grant
Expand Down Expand Up @@ -538,6 +555,9 @@ public function testGetAccessToken($method)
]);

$provider->setHttpClient($client);
$clock = (new ProgrammableClock())
->setTime(new \DateTimeImmutable('1st February 2013 1pm'));
$provider->setClock($clock);
$token = $provider->getAccessToken($grant, ['code' => 'mock_authorization_code']);

$this->assertInstanceOf(AccessTokenInterface::class, $token);
Expand All @@ -546,6 +566,13 @@ public function testGetAccessToken($method)
$this->assertSame($raw_response['access_token'], $token->getToken());
$this->assertSame($raw_response['expires'], $token->getExpires());

// Set the time to a different value so we know references to the
// original clock object in provider was not lost.
$newTime = new \DateTimeImmutable('2nd February 2013 1pm');
$clock->setTime($newTime);

$this->assertEquals($newTime->getTimestamp(), $token->getTimeNow());

$client
->shouldHaveReceived('send')
->once()
Expand Down Expand Up @@ -786,7 +813,7 @@ public function testExtendedProviderDoesNotErrorWhenUsingAccessTokenAsTheTypeHin
$token = new AccessToken([
'access_token' => 'mock_access_token',
'refresh_token' => 'mock_refresh_token',
'expires' => time(),
'expires' => 123,
'resource_owner_id' => 'mock_resource_owner_id',
]);

Expand Down
29 changes: 29 additions & 0 deletions test/src/Provider/FrozenClock.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php

namespace League\OAuth2\Client\Test\Provider;

use League\OAuth2\Client\Provider\Clock;

/**
* A clock with a frozen time for testing.
*/
class FrozenClock extends Clock
{

/**
* The simulated time.
*
* Evaluates to 1st January 2015 @ 12pm.
*
* @var int
*/
const NOW = 1420113600;

/**
* @inheritdoc
*/
public function now()
{
return (new \DateTimeImmutable('@' . static::NOW));
}
}
41 changes: 41 additions & 0 deletions test/src/Provider/ProgrammableClock.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

namespace League\OAuth2\Client\Test\Provider;

use League\OAuth2\Client\Provider\Clock;

/**
* A clock which must be initialised, and may be changed at any time.
*/
class ProgrammableClock extends Clock
{

/**
* @var \DateTimeImmutable|null
*/
protected $time = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why wouldn't this be initialized via __construct ?


/**
* @inheritdoc
*/
public function now()
{
if (!isset($this->time)) {
throw new \LogicException('Time must be set explicitly');
}
return $this->time;
}

/**
* Sets the current time.
*
* @param \DateTimeImmutable|null the current time.
* @return self
*/
public function setTime($time)
{
$this->time = $time;

return $this;
}
}
Loading