From 8f91587119d2e9ae03cecc735201f32d122fb219 Mon Sep 17 00:00:00 2001 From: Kevin Quinn Date: Sun, 18 Jul 2021 14:14:15 -0400 Subject: [PATCH 1/3] Implement some changes based on review This adds two new interfaces: - `ClockInterface` This is basically the spec from https://github.com/php-fig/fig-standards/blob/master/proposed/clock.md except without a return typehint to support PHP 5.6 for now - `ClockAwareInterface` - this may not be necessary but it seemed reasonable to have a `getClock(): ClockInterface` contract, given that we expect to be able to get a clock in both `AccessToken` and `AbstractProvider`. This also removes some methods added in https://github.com/thephpleague/oauth2-client/pull/852, specifically: - `AccessToken::setTimeNow()` - `AccessToken::getTimeNow()` - `AccessToken::resetTimeNow()` - `AccessToken::$timeNow` That technically represents a BC break, but: 1. That code has not existed for very long and likely has very little usage. 2. This *should* only break test code unless someone was doing something very strange in production. For the existing `AccessTokenInterface`: - remove the `setClock()` and `getTimeNow()` - `getTimeNow()` is no longer needed anyway - `setClock()` better not to have a setter on an interface in this case - both of these changes were BC breaks so they were removed I believe the rest of the changes here are just cleaning up tests that used the older static method of setting the current time. The previous hack `tearDownForBackwardsCompatibility()` is no longer needed so it was removed. --- .gitignore | 1 + src/Provider/AbstractProvider.php | 9 +- src/Provider/Clock.php | 2 +- src/Provider/ClockAwareInterface.php | 13 ++ src/Provider/ClockInterface.php | 19 +++ src/Token/AccessToken.php | 64 +++------- src/Token/AccessTokenInterface.php | 18 +-- src/Tool/MacAuthorizationTrait.php | 2 +- test/src/Provider/AbstractProviderTest.php | 2 +- test/src/Provider/FrozenClock.php | 4 +- test/src/Provider/ProgrammableClock.php | 15 ++- test/src/Token/AccessTokenTest.php | 137 ++++----------------- 12 files changed, 98 insertions(+), 188 deletions(-) create mode 100644 src/Provider/ClockAwareInterface.php create mode 100644 src/Provider/ClockInterface.php diff --git a/.gitignore b/.gitignore index 8ef05678..f0d15e7c 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ testing/ nbproject/private/ test/log build +.phpunit.result.cache diff --git a/src/Provider/AbstractProvider.php b/src/Provider/AbstractProvider.php index a3fd9e94..8b374a37 100644 --- a/src/Provider/AbstractProvider.php +++ b/src/Provider/AbstractProvider.php @@ -37,7 +37,7 @@ * * @link http://tools.ietf.org/html/rfc6749#section-1.1 Roles (RFC 6749, §1.1) */ -abstract class AbstractProvider +abstract class AbstractProvider implements ClockAwareInterface { use ArrayAccessorTrait; use GuardedPropertyTrait; @@ -265,11 +265,10 @@ public function getOptionProvider() /** * Sets the clock. * - * @param Clock $clock - * + * @param ClockInterface $clock the clock * @return self */ - public function setClock(Clock $clock) + public function setClock(ClockInterface $clock) { $this->clock = $clock; @@ -284,7 +283,7 @@ public function setClock(Clock $clock) */ public function getClock() { - return $this->clock; + return $this->clock ? $this->clock : new Clock(); } /** diff --git a/src/Provider/Clock.php b/src/Provider/Clock.php index bb5a2d50..5e82ad4d 100644 --- a/src/Provider/Clock.php +++ b/src/Provider/Clock.php @@ -5,7 +5,7 @@ /** * Represents an implementation of a Clock. */ -class Clock +final class Clock implements ClockInterface { /** diff --git a/src/Provider/ClockAwareInterface.php b/src/Provider/ClockAwareInterface.php new file mode 100644 index 00000000..87a3c911 --- /dev/null +++ b/src/Provider/ClockAwareInterface.php @@ -0,0 +1,13 @@ +clock = $clock; - } - - /** - * @inheritdoc - */ - public function getTimeNow() - { - if (self::$timeNow) { - return self::$timeNow; - } elseif (isset($this->clock)) { - return $this->clock->now()->getTimestamp(); - } else { - return time(); - } + return $this->clock ? $this->clock : new Clock(); } /** @@ -134,6 +96,8 @@ public function __construct(array $options = []) $this->clock = $options['clock']; } + $ts = $this->getClock()->now()->getTimestamp(); + // 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. @@ -142,14 +106,14 @@ public function __construct(array $options = []) throw new \InvalidArgumentException('expires_in value must be an integer'); } - $this->expires = $options['expires_in'] != 0 ? $this->getTimeNow() + $options['expires_in'] : 0; + $this->expires = $options['expires_in'] != 0 ? $ts + $options['expires_in'] : 0; } elseif (!empty($options['expires'])) { // Some providers supply the seconds until expiration rather than // the exact timestamp. Take a best guess at which we received. $expires = $options['expires']; if (!$this->isExpirationTimestamp($expires)) { - $expires += $this->getTimeNow(); + $expires += $ts; } $this->expires = $expires; @@ -224,7 +188,7 @@ public function hasExpired() throw new RuntimeException('"expires" is not set on the token'); } - return $expires < $this->getTimeNow(); + return $expires < $this->getClock()->now()->getTimestamp(); } /** diff --git a/src/Token/AccessTokenInterface.php b/src/Token/AccessTokenInterface.php index 132397db..e87ba7b7 100644 --- a/src/Token/AccessTokenInterface.php +++ b/src/Token/AccessTokenInterface.php @@ -15,7 +15,7 @@ namespace League\OAuth2\Client\Token; use JsonSerializable; -use League\OAuth2\Client\Provider\Clock; +use League\OAuth2\Client\Provider\ClockInterface; use RuntimeException; interface AccessTokenInterface extends JsonSerializable @@ -70,20 +70,4 @@ public function __toString(); * @return array */ public function jsonSerialize(); - - /** - * Sets the clock. - * - * @param Clock $clock a clock. - * - * @return void - */ - public function setClock(Clock $clock); - - /** - * Get the current time, whether real or simulated. - * - * @return int - */ - public function getTimeNow(); } diff --git a/src/Tool/MacAuthorizationTrait.php b/src/Tool/MacAuthorizationTrait.php index 7de6f3d5..ca712581 100644 --- a/src/Tool/MacAuthorizationTrait.php +++ b/src/Tool/MacAuthorizationTrait.php @@ -14,7 +14,7 @@ namespace League\OAuth2\Client\Tool; -use League\OAuth2\Client\Provider\Clock; +use League\OAuth2\Client\Provider\ClockInterface; use League\OAuth2\Client\Token\AccessToken; use League\OAuth2\Client\Token\AccessTokenInterface; diff --git a/test/src/Provider/AbstractProviderTest.php b/test/src/Provider/AbstractProviderTest.php index 39b081c1..9b988a88 100644 --- a/test/src/Provider/AbstractProviderTest.php +++ b/test/src/Provider/AbstractProviderTest.php @@ -571,7 +571,7 @@ public function testGetAccessToken($method) $newTime = new \DateTimeImmutable('2nd February 2013 1pm'); $clock->setTime($newTime); - $this->assertEquals($newTime->getTimestamp(), $token->getTimeNow()); + $this->assertEquals($newTime->getTimestamp(), $token->getClock()->now()->getTimestamp()); $client ->shouldHaveReceived('send') diff --git a/test/src/Provider/FrozenClock.php b/test/src/Provider/FrozenClock.php index ff2b0983..8ec901ba 100644 --- a/test/src/Provider/FrozenClock.php +++ b/test/src/Provider/FrozenClock.php @@ -3,11 +3,13 @@ namespace League\OAuth2\Client\Test\Provider; use League\OAuth2\Client\Provider\Clock; +use League\OAuth2\Client\Provider\ClockInterface; + /** * A clock with a frozen time for testing. */ -class FrozenClock extends Clock +class FrozenClock implements ClockInterface { /** diff --git a/test/src/Provider/ProgrammableClock.php b/test/src/Provider/ProgrammableClock.php index 7231b8bd..f12e40dc 100644 --- a/test/src/Provider/ProgrammableClock.php +++ b/test/src/Provider/ProgrammableClock.php @@ -2,12 +2,13 @@ namespace League\OAuth2\Client\Test\Provider; -use League\OAuth2\Client\Provider\Clock; +use League\OAuth2\Client\Provider\ClockInterface; + /** * A clock which must be initialised, and may be changed at any time. */ -class ProgrammableClock extends Clock +class ProgrammableClock implements ClockInterface { /** @@ -38,4 +39,14 @@ public function setTime($time) return $this; } + + /** + * @param int $time seconds since epoch + * @return self + */ + public static function newFromTimestamp($time) + { + return (new self()) + ->setTime(new \DateTimeImmutable('@' . $time)); + } } diff --git a/test/src/Token/AccessTokenTest.php b/test/src/Token/AccessTokenTest.php index 989ad85d..c7a72425 100644 --- a/test/src/Token/AccessTokenTest.php +++ b/test/src/Token/AccessTokenTest.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use League\OAuth2\Client\Test\Provider\FrozenClock; +use League\OAuth2\Client\Test\Provider\ProgrammableClock; use League\OAuth2\Client\Token\AccessToken; use Mockery; use PHPUnit\Framework\TestCase; @@ -19,27 +20,11 @@ class AccessTokenTest extends TestCase */ const NOW = 1359504000; - /** - * BC teardown. - * - * This is for backwards compatibility of older PHP versions. Ideally we would just implement a tearDown() here but - * older PHP versions this library supports don't have return typehint support, so this is the workaround. - * - * @return void - */ - private static function tearDownForBackwardsCompatibility() - { - /* reset the test double time if it was set */ - AccessToken::resetTimeNow(); - } - public function testInvalidRefreshToken() { $this->expectException(InvalidArgumentException::class); $token = $this->getAccessToken(['invalid_access_token' => 'none']); - - self::tearDownForBackwardsCompatibility(); } protected function getAccessToken($options = []) @@ -51,87 +36,41 @@ public function testExpiresInCorrection() { // Correction happens in constructor so time needs to be set before // object is created. - AccessToken::setTimeNow(static::NOW); - $options = ['access_token' => 'access_token', 'expires_in' => 100]; + $options = [ + 'access_token' => 'access_token', + 'expires_in' => 100, + 'clock' => new FrozenClock(), + ]; $token = $this->getAccessToken($options); $expires = $token->getExpires(); - $this->assertEquals(static::NOW + 100, $expires); - - self::tearDownForBackwardsCompatibility(); + $this->assertEquals(FrozenClock::NOW + 100, $expires); } public function testExpiresInCorrectionUsingSetTimeNow() { - /* set fake time at 2020-01-01 00:00:00 */ - AccessToken::setTimeNow(1577836800); - $options = ['access_token' => 'access_token', 'expires_in' => 100]; + $options = [ + 'access_token' => 'access_token', + 'expires_in' => 100, + 'clock' => new FrozenClock(), + ]; $token = $this->getAccessToken($options); $expires = $token->getExpires(); $this->assertNotNull($expires); - $this->assertEquals(1577836900, $expires); - - self::tearDownForBackwardsCompatibility(); - } - - public function testSetClockConstructor() - { - $clock = new FrozenClock(); - $token = $this->getAccessToken(['access_token' => 'asdf', 'clock' => $clock]); - $this->assertEquals(FrozenClock::NOW, $token->getTimeNow()); - } - - public function testSetClockMethod() - { - $clock = new FrozenClock(); - $token = $this->getAccessToken(['access_token' => 'asdf']); - $token->setClock($clock); - $this->assertEquals(FrozenClock::NOW, $token->getTimeNow()); - } - - public function testSetTimeNow() - { - AccessToken::setTimeNow(1577836800); - $timeNow = $this->getAccessToken(['access_token' => 'asdf'])->getTimeNow(); - - $this->assertEquals(1577836800, $timeNow); - - self::tearDownForBackwardsCompatibility(); - } - - public function testSetClockAndSetTime() - { - // When both a clock and time set, time wins over clock. - $clock = new FrozenClock(); - AccessToken::setTimeNow(static::NOW); - $token = $this->getAccessToken(['access_token' => 'asdf', 'clock' => $clock]); - $this->assertEquals(static::NOW, $token->getTimeNow()); - } - - public function testResetTimeNow() - { - AccessToken::setTimeNow(static::NOW); - $token = $this->getAccessToken(['access_token' => 'asdf']); - - $this->assertEquals(static::NOW, $token->getTimeNow()); - AccessToken::resetTimeNow(); - - $this->assertNotEquals(static::NOW, $token->getTimeNow()); - - $timeBeforeAssertion = time(); - $this->assertGreaterThanOrEqual($timeBeforeAssertion, $token->getTimeNow()); - - self::tearDownForBackwardsCompatibility(); + $this->assertEquals(FrozenClock::NOW + 100, $expires); } public function testExpiresPastTimestamp() { - $options = ['access_token' => 'access_token', 'expires' => static::NOW - 1]; + $options = [ + 'access_token' => 'access_token', + 'expires' => static::NOW - 1, + 'clock' => ProgrammableClock::newFromTimestamp(static::NOW), + ]; $token = $this->getAccessToken($options); - AccessToken::setTimeNow(static::NOW); $this->assertTrue($token->hasExpired()); @@ -139,8 +78,6 @@ public function testExpiresPastTimestamp() $token = $this->getAccessToken($options); $this->assertFalse($token->hasExpired()); - - self::tearDownForBackwardsCompatibility(); } public function testGetRefreshToken() @@ -154,47 +91,35 @@ public function testGetRefreshToken() $refreshToken = $token->getRefreshToken(); $this->assertEquals($options['refresh_token'], $refreshToken); - - self::tearDownForBackwardsCompatibility(); } public function testHasNotExpiredWhenPropertySetInFuture() { - $options = [ - 'access_token' => 'access_token' - ]; + $clock = ProgrammableClock::newFromTimestamp(static::NOW); + $token = Mockery::mock(AccessToken::class)->makePartial(); - $expectedExpires = static::NOW + 1; + $token + ->shouldReceive('getClock') + ->once() + ->andReturn(ProgrammableClock::newFromTimestamp(static::NOW)); - AccessToken::setTimeNow(static::NOW); - $token = Mockery::mock(AccessToken::class, [$options])->makePartial(); $token ->shouldReceive('getExpires') ->once() - ->andReturn($expectedExpires); + ->andReturn(static::NOW + 1); $this->assertFalse($token->hasExpired()); - - self::tearDownForBackwardsCompatibility(); } public function testHasExpiredWhenPropertySetInPast() { - $options = [ - 'access_token' => 'access_token' - ]; - - $expectedExpires = static::NOW - 1; - - $token = Mockery::mock(AccessToken::class, [$options])->makePartial(); + $token = Mockery::mock(AccessToken::class)->makePartial(); $token ->shouldReceive('getExpires') ->once() - ->andReturn($expectedExpires); + ->andReturn(static::NOW - 1); $this->assertTrue($token->hasExpired()); - - self::tearDownForBackwardsCompatibility(); } public function testCannotReportExpiredWhenNoExpirationSet() @@ -207,8 +132,6 @@ public function testCannotReportExpiredWhenNoExpirationSet() $this->expectException(RuntimeException::class); $hasExpired = $token->hasExpired(); - - self::tearDownForBackwardsCompatibility(); } public function testInvalidExpiresIn() @@ -221,8 +144,6 @@ public function testInvalidExpiresIn() $this->expectException(InvalidArgumentException::class); $token = $this->getAccessToken($options); - - self::tearDownForBackwardsCompatibility(); } @@ -239,8 +160,6 @@ public function testJsonSerializable() $jsonToken = json_encode($token); $this->assertEquals($options, json_decode($jsonToken, true)); - - self::tearDownForBackwardsCompatibility(); } public function testValues() @@ -260,7 +179,5 @@ public function testValues() $this->assertTrue(is_array($values)); $this->assertArrayHasKey('custom_thing', $values); $this->assertSame($options['custom_thing'], $values['custom_thing']); - - self::tearDownForBackwardsCompatibility(); } } From 099377570b6a8e5a03fecaaaa7fcfbda4d433e09 Mon Sep 17 00:00:00 2001 From: Kevin Quinn Date: Sun, 18 Jul 2021 14:30:49 -0400 Subject: [PATCH 2/3] Remove return typehint for php compatibility --- src/Token/AccessToken.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Token/AccessToken.php b/src/Token/AccessToken.php index 7aca5ccb..925c2f30 100644 --- a/src/Token/AccessToken.php +++ b/src/Token/AccessToken.php @@ -64,7 +64,7 @@ class AccessToken implements AccessTokenInterface, ResourceOwnerAccessTokenInter /** * {@inheritDoc} */ - public function getClock(): ClockInterface + public function getClock() { return $this->clock ? $this->clock : new Clock(); } From a19b76d79dbe7ede9c9e37c7faba2356b580ee71 Mon Sep 17 00:00:00 2001 From: Kevin Quinn Date: Sun, 18 Jul 2021 16:38:53 -0400 Subject: [PATCH 3/3] Use self instead of static in test --- test/src/Token/AccessTokenTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/src/Token/AccessTokenTest.php b/test/src/Token/AccessTokenTest.php index c7a72425..1bb96f01 100644 --- a/test/src/Token/AccessTokenTest.php +++ b/test/src/Token/AccessTokenTest.php @@ -10,7 +10,7 @@ use PHPUnit\Framework\TestCase; use RuntimeException; -class AccessTokenTest extends TestCase +final class AccessTokenTest extends TestCase { /** @@ -67,8 +67,8 @@ public function testExpiresPastTimestamp() { $options = [ 'access_token' => 'access_token', - 'expires' => static::NOW - 1, - 'clock' => ProgrammableClock::newFromTimestamp(static::NOW), + 'expires' => self::NOW - 1, + 'clock' => ProgrammableClock::newFromTimestamp(self::NOW), ]; $token = $this->getAccessToken($options); @@ -95,18 +95,18 @@ public function testGetRefreshToken() public function testHasNotExpiredWhenPropertySetInFuture() { - $clock = ProgrammableClock::newFromTimestamp(static::NOW); + $clock = ProgrammableClock::newFromTimestamp(self::NOW); $token = Mockery::mock(AccessToken::class)->makePartial(); $token ->shouldReceive('getClock') ->once() - ->andReturn(ProgrammableClock::newFromTimestamp(static::NOW)); + ->andReturn(ProgrammableClock::newFromTimestamp(self::NOW)); $token ->shouldReceive('getExpires') ->once() - ->andReturn(static::NOW + 1); + ->andReturn(self::NOW + 1); $this->assertFalse($token->hasExpired()); } @@ -117,7 +117,7 @@ public function testHasExpiredWhenPropertySetInPast() $token ->shouldReceive('getExpires') ->once() - ->andReturn(static::NOW - 1); + ->andReturn(self::NOW - 1); $this->assertTrue($token->hasExpired()); } @@ -152,7 +152,7 @@ public function testJsonSerializable() $options = [ 'access_token' => 'mock_access_token', 'refresh_token' => 'mock_refresh_token', - 'expires' => static::NOW + 3600, + 'expires' => self::NOW + 3600, 'resource_owner_id' => 'mock_resource_owner_id', ]; @@ -167,7 +167,7 @@ public function testValues() $options = [ 'access_token' => 'mock_access_token', 'refresh_token' => 'mock_refresh_token', - 'expires' => static::NOW + 3600, + 'expires' => self::NOW + 3600, 'resource_owner_id' => 'mock_resource_owner_id', 'custom_thing' => 'i am a test!', ];