From 88ee36d1e56cf2bc9df738355ab884a6fb9ad56c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 12:48:57 -0400 Subject: [PATCH 1/8] Add ComposerFileStorage::getModifiedTime() --- src/ComposerFileStorage.php | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/ComposerFileStorage.php b/src/ComposerFileStorage.php index 41602c8..5be2e04 100644 --- a/src/ComposerFileStorage.php +++ b/src/ComposerFileStorage.php @@ -57,4 +57,29 @@ public static function create(string $url, Config $config): self ]); return new static($basePath); } + + /** + * Returns the time a stored file was last modified. + * + * @param string $name + * The name of the file to check, without its `.json` extension. + * + * @return \DateTimeImmutable|null + * The time the file was last modified, or null if the file doesn't exist. + * + * @throws \RuntimeException + * If the file exists but its modification time could not be determined. + */ + public function getModifiedTime(string $name): ?\DateTimeImmutable + { + $path = $this->toPath($name); + if (file_exists($path)) { + $modifiedTime = filemtime($path); + if (is_int($modifiedTime)) { + return (new \DateTimeImmutable)->setTimestamp($modifiedTime); + } + throw new \RuntimeException("Could not get the modification time for '$path'."); + } + return null; + } } From e8d2cb213b412e1837ba7847a6d16808971d1d0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 12:59:17 -0400 Subject: [PATCH 2/8] Unit test coverage --- tests/ComposerFileStorageTest.php | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/ComposerFileStorageTest.php b/tests/ComposerFileStorageTest.php index b1208d1..43a99ca 100644 --- a/tests/ComposerFileStorageTest.php +++ b/tests/ComposerFileStorageTest.php @@ -69,4 +69,35 @@ public function testCreate(): void ComposerFileStorage::create('https://example.net/packages', $config); $this->assertDirectoryExists($basePath); } + + /** + * @covers ::getModifiedTime + */ + public function testModifiedTime(): void + { + $config = new Config(); + + $basePath = implode(DIRECTORY_SEPARATOR, [ + ComposerFileStorage::basePath($config), + 'https---example.net-packages', + ]); + $storage = ComposerFileStorage::create('https://example.net/packages', $config); + + // A non-existent file should produce a null modification time. + $this->assertNull($storage->getModifiedTime('test')); + + // Once the file exists, we should get a modification time. + $path = $basePath . DIRECTORY_SEPARATOR . 'test.json'; + touch($path); + $modifiedTime = $storage->getModifiedTime('test'); + $this->assertInstanceOf(\DateTimeImmutable::class, $modifiedTime); + + // Change the modification time, and clear the file stat cache so we can + // be sure the new modification time is picked up; it seems that touch() + // doesn't do that automatically. + $newModifiedTime = $modifiedTime->getTimestamp() + 10; + touch($path, $newModifiedTime); + clearstatcache(filename: $path); + $this->assertSame($storage->getModifiedTime('test')->getTimestamp(), $newModifiedTime); + } } From c629c30ec49b97ea27a5b1ce4946ef360ba35afe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 13:20:25 -0400 Subject: [PATCH 3/8] Change Loader so that it will send If-Modified-Since header --- src/ComposerFileStorage.php | 13 +++++++++++- src/Loader.php | 28 ++++++++++++++++++++++---- src/TufValidatedComposerRepository.php | 15 +++++++------- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/ComposerFileStorage.php b/src/ComposerFileStorage.php index 5be2e04..44c2bb8 100644 --- a/src/ComposerFileStorage.php +++ b/src/ComposerFileStorage.php @@ -76,10 +76,21 @@ public function getModifiedTime(string $name): ?\DateTimeImmutable if (file_exists($path)) { $modifiedTime = filemtime($path); if (is_int($modifiedTime)) { - return (new \DateTimeImmutable)->setTimestamp($modifiedTime); + // The @ prefix tells \DateTimeImmutable that $modifiedTime is + // a UNIX timestamp. + return new \DateTimeImmutable("@$modifiedTime"); } throw new \RuntimeException("Could not get the modification time for '$path'."); } return null; } + + /** + * {@inheritDoc} + */ + public function read(string $name): ?string + { + return parent::read($name); + } + } diff --git a/src/Loader.php b/src/Loader.php index ca20e98..6364ab1 100644 --- a/src/Loader.php +++ b/src/Loader.php @@ -16,7 +16,7 @@ */ class Loader implements LoaderInterface { - public function __construct(private HttpDownloader $downloader, private string $baseUrl = '') + public function __construct(private HttpDownloader $downloader, private ComposerFileStorage $storage, private string $baseUrl = '') { } @@ -27,11 +27,31 @@ public function load(string $locator, int $maxBytes): StreamInterface { $url = $this->baseUrl . $locator; - try { + $options = [ // Add 1 to $maxBytes to work around a bug in Composer. // @see \Tuf\ComposerIntegration\ComposerCompatibleUpdater::getLength() - $content = $this->downloader->get($url, ['max_file_size' => $maxBytes + 1]) - ->getBody(); + 'max_file_size' => $maxBytes + 1, + ]; + + // The name of the file in persistent storage will differ from $locator. + $name = basename($locator, '.json'); + $name = ltrim($name, '.0123456789'); + + $modifiedTime = $this->storage->getModifiedTime($name); + if ($modifiedTime) { + // @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/If-Modified-Since. + $options['http']['header'][] = 'If-Modified-Since: ' . $modifiedTime->format('D, d M Y H:i:s') . ' GMT'; + } + + try { + $response = $this->downloader->get($url, $options); + // If we sent an If-Modified-Since header and received a 304 (Not Modified) + // response, we can just load the file from cache. + if ($response->getStatusCode() === 304) { + $content = $this->storage->read($name); + } else { + $content = $response->getBody(); + } return Utils::streamFor($content); } catch (TransportException $e) { if ($e->getStatusCode() === 404) { diff --git a/src/TufValidatedComposerRepository.php b/src/TufValidatedComposerRepository.php index ad29c67..ac33965 100644 --- a/src/TufValidatedComposerRepository.php +++ b/src/TufValidatedComposerRepository.php @@ -15,7 +15,6 @@ use Tuf\Exception\NotFoundException; use Tuf\Loader\SizeCheckingLoader; use Tuf\Metadata\RootMetadata; -use Tuf\Metadata\StorageInterface; /** * Defines a Composer repository that is protected by TUF. @@ -65,11 +64,11 @@ public function __construct(array $repoConfig, IOInterface $io, Config $config, Repository::$maxBytes = $maxBytes; } - $this->updater = new ComposerCompatibleUpdater( - new SizeCheckingLoader(new Loader($httpDownloader, $metadataUrl)), - // @todo: Write a custom implementation of FileStorage that stores repo keys to user's global composer cache? - $this->initializeStorage($url, $config) - ); + // @todo: Write a custom implementation of FileStorage that stores repo keys to user's global composer cache? + $storage = $this->initializeStorage($url, $config); + $loader = new Loader($httpDownloader, $storage, $metadataUrl); + $loader = new SizeCheckingLoader($loader); + $this->updater = new ComposerCompatibleUpdater($loader, $storage); $io->debug("[TUF] Packages from $url are verified by TUF."); $io->debug("[TUF] Metadata source: $metadataUrl"); @@ -88,13 +87,13 @@ public function __construct(array $repoConfig, IOInterface $io, Config $config, * @param Config $config * The Composer configuration. * - * @return \Tuf\Metadata\StorageInterface + * @return \Tuf\ComposerIntegration\ComposerFileStorage * A durable storage object for this repository's TUF data. * * @throws \RuntimeException * If no root metadata could be found for this repository. */ - private function initializeStorage(string $url, Config $config): StorageInterface + private function initializeStorage(string $url, Config $config): ComposerFileStorage { $storage = ComposerFileStorage::create($url, $config); From 006499bd2e49693195c56da39c92ef7e89b0ae88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 13:23:38 -0400 Subject: [PATCH 4/8] Fix LoaderTEst --- tests/LoaderTest.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/LoaderTest.php b/tests/LoaderTest.php index 406485e..d97bbd7 100644 --- a/tests/LoaderTest.php +++ b/tests/LoaderTest.php @@ -9,6 +9,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; use Psr\Http\Message\StreamInterface; +use Tuf\ComposerIntegration\ComposerFileStorage; use Tuf\ComposerIntegration\Loader; use Tuf\Exception\DownloadSizeException; use Tuf\Exception\RepoFileNotFound; @@ -23,7 +24,8 @@ class LoaderTest extends TestCase public function testLoader(): void { $downloader = $this->prophesize(HttpDownloader::class); - $loader = new Loader($downloader->reveal(), '/metadata/'); + $storage = $this->prophesize(ComposerFileStorage::class); + $loader = new Loader($downloader->reveal(), $storage->reveal(), '/metadata/'); $downloader->get('/metadata/root.json', ['max_file_size' => 129]) ->willReturn(new Response()) From aaf18400d81a316f2be4e3058cfb3f2a19a70d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 13:52:03 -0400 Subject: [PATCH 5/8] Add unit test coverage of new loader functionality --- tests/LoaderTest.php | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/tests/LoaderTest.php b/tests/LoaderTest.php index d97bbd7..ed2af95 100644 --- a/tests/LoaderTest.php +++ b/tests/LoaderTest.php @@ -2,10 +2,11 @@ namespace Tuf\ComposerIntegration\Tests; +use Composer\Config; use Composer\Downloader\MaxFileSizeExceededException; use Composer\Downloader\TransportException; +use Composer\Util\Http\Response; use Composer\Util\HttpDownloader; -use GuzzleHttp\Psr7\Response; use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; use Psr\Http\Message\StreamInterface; @@ -27,8 +28,9 @@ public function testLoader(): void $storage = $this->prophesize(ComposerFileStorage::class); $loader = new Loader($downloader->reveal(), $storage->reveal(), '/metadata/'); - $downloader->get('/metadata/root.json', ['max_file_size' => 129]) - ->willReturn(new Response()) + $url = '/metadata/root.json'; + $downloader->get($url, ['max_file_size' => 129]) + ->willReturn(new Response(['url' => $url], 200, [], null)) ->shouldBeCalled(); $this->assertInstanceOf(StreamInterface::class, $loader->load('root.json', 128)); @@ -73,4 +75,33 @@ public function testLoader(): void $this->assertSame($originalException, $e->getPrevious()); } } + + public function testNotModifiedResponse(): void + { + $config = new Config(); + $storage = ComposerFileStorage::create('https://example.net/packages', $config); + + $method = new \ReflectionMethod($storage, 'write'); + $method->invoke($storage, 'test', 'Some test data.'); + $modifiedTime = $storage->getModifiedTime('test')->format('D, d M Y H:i:s'); + + $downloader = $this->prophesize(HttpDownloader::class); + $options = [ + 'max_file_size' => 1025, + 'http' => [ + 'header' => [ + "If-Modified-Since: $modifiedTime GMT", + ], + ], + ]; + $url = '2.test.json'; + $response = new Response(['url' => $url], 304, [], null); + $downloader->get($url, $options)->willReturn($response)->shouldBeCalled(); + + $loader = new Loader($downloader->reveal(), $storage); + // Since the response has no actual body data, the fact that we get the contents + // of the file we wrote here is proof that it was ultimately read from persistent + // storage by the loader. + $this->assertSame('Some test data.', $loader->load('2.test.json', 1024)->getContents()); + } } From 55ec117e5f24165c4592f8da9298d4a0429c06ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 14:04:34 -0400 Subject: [PATCH 6/8] Use setAccessible --- tests/LoaderTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/LoaderTest.php b/tests/LoaderTest.php index ed2af95..0d76bef 100644 --- a/tests/LoaderTest.php +++ b/tests/LoaderTest.php @@ -83,6 +83,7 @@ public function testNotModifiedResponse(): void $method = new \ReflectionMethod($storage, 'write'); $method->invoke($storage, 'test', 'Some test data.'); + $method->setAccessible(true); $modifiedTime = $storage->getModifiedTime('test')->format('D, d M Y H:i:s'); $downloader = $this->prophesize(HttpDownloader::class); From ebcf3f7e18b5c8f366ab5c7b9461115430505935 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Tue, 24 Oct 2023 14:09:50 -0400 Subject: [PATCH 7/8] do that in the right place, though --- tests/LoaderTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/LoaderTest.php b/tests/LoaderTest.php index 0d76bef..9dc22a4 100644 --- a/tests/LoaderTest.php +++ b/tests/LoaderTest.php @@ -82,8 +82,8 @@ public function testNotModifiedResponse(): void $storage = ComposerFileStorage::create('https://example.net/packages', $config); $method = new \ReflectionMethod($storage, 'write'); - $method->invoke($storage, 'test', 'Some test data.'); $method->setAccessible(true); + $method->invoke($storage, 'test', 'Some test data.'); $modifiedTime = $storage->getModifiedTime('test')->format('D, d M Y H:i:s'); $downloader = $this->prophesize(HttpDownloader::class); From cd919ee3863acffe7ce211fcbf00982291ab8a20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ph=C3=A9na=20Proxima?= Date: Wed, 25 Oct 2023 16:49:03 -0400 Subject: [PATCH 8/8] Improve assertion --- tests/LoaderTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/LoaderTest.php b/tests/LoaderTest.php index 9dc22a4..b0c2836 100644 --- a/tests/LoaderTest.php +++ b/tests/LoaderTest.php @@ -96,8 +96,12 @@ public function testNotModifiedResponse(): void ], ]; $url = '2.test.json'; - $response = new Response(['url' => $url], 304, [], null); - $downloader->get($url, $options)->willReturn($response)->shouldBeCalled(); + $response = $this->prophesize(Response::class); + $response->getStatusCode()->willReturn(304)->shouldBeCalled(); + $response->getBody()->shouldNotBeCalled(); + $downloader->get($url, $options) + ->willReturn($response->reveal()) + ->shouldBeCalled(); $loader = new Loader($downloader->reveal(), $storage); // Since the response has no actual body data, the fact that we get the contents