From fc04779a26ac4119cc5c63dd89bb6039b461d1d9 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 22 Dec 2016 09:46:10 +0100 Subject: [PATCH] Add ETag validation to appstore requests * If the ETag if present store it * If a stored ETag is present then pass it along (with the original response) to get * Add tests * Added files to classmap Signed-off-by: Roeland Jago Douma --- .../App/AppStore/Fetcher/AppFetcher.php | 7 +- lib/private/App/AppStore/Fetcher/Fetcher.php | 39 ++++- .../App/AppStore/Fetcher/AppFetcherTest.php | 4 + .../lib/App/AppStore/Fetcher/FetcherBase.php | 165 +++++++++++++++++- 4 files changed, 204 insertions(+), 11 deletions(-) diff --git a/lib/private/App/AppStore/Fetcher/AppFetcher.php b/lib/private/App/AppStore/Fetcher/AppFetcher.php index 9ebc12dbc2..7c5efafc92 100644 --- a/lib/private/App/AppStore/Fetcher/AppFetcher.php +++ b/lib/private/App/AppStore/Fetcher/AppFetcher.php @@ -59,11 +59,14 @@ class AppFetcher extends Fetcher { /** * Only returns the latest compatible app release in the releases array * + * @param string $ETag + * @param string $content + * * @return array */ - protected function fetch() { + protected function fetch($ETag, $content) { /** @var mixed[] $response */ - $response = parent::fetch(); + $response = parent::fetch($ETag, $content); $ncVersion = $this->config->getSystemValue('version'); $ncMajorVersion = explode('.', $ncVersion)[0]; diff --git a/lib/private/App/AppStore/Fetcher/Fetcher.php b/lib/private/App/AppStore/Fetcher/Fetcher.php index 2067242e81..dab79e1182 100644 --- a/lib/private/App/AppStore/Fetcher/Fetcher.php +++ b/lib/private/App/AppStore/Fetcher/Fetcher.php @@ -21,6 +21,7 @@ namespace OC\App\AppStore\Fetcher; +use OCP\AppFramework\Http; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IAppData; use OCP\Files\NotFoundException; @@ -62,15 +63,37 @@ abstract class Fetcher { /** * Fetches the response from the server * + * @param string $ETag + * @param string $content + * * @return array */ - protected function fetch() { + protected function fetch($ETag, $content) { + $options = []; + + if ($ETag !== '') { + $options['headers'] = [ + 'If-None-Match' => $ETag, + ]; + } + $client = $this->clientService->newClient(); - $response = $client->get($this->endpointUrl); + $response = $client->get($this->endpointUrl, $options); + $responseJson = []; - $responseJson['data'] = json_decode($response->getBody(), true); + if ($response->getStatusCode() === Http::STATUS_NOT_MODIFIED) { + $responseJson['data'] = json_decode($content, true); + } else { + $responseJson['data'] = json_decode($response->getBody(), true); + $ETag = $response->getHeader('ETag'); + } + $responseJson['timestamp'] = $this->timeFactory->getTime(); $responseJson['ncversion'] = $this->config->getSystemValue('version'); + if ($ETag !== '') { + $responseJson['ETag'] = $ETag; + } + return $responseJson; } @@ -82,6 +105,9 @@ abstract class Fetcher { public function get() { $rootFolder = $this->appData->getFolder('/'); + $ETag = ''; + $content = ''; + try { // File does already exists $file = $rootFolder->getFile($this->fileName); @@ -95,6 +121,11 @@ abstract class Fetcher { isset($jsonBlob['ncversion']) && $jsonBlob['ncversion'] === $this->config->getSystemValue('version', '0.0.0')) { return $jsonBlob['data']; } + + if (isset($jsonBlob['ETag'])) { + $ETag = $jsonBlob['ETag']; + $content = json_encode($jsonBlob['data']); + } } } catch (NotFoundException $e) { // File does not already exists @@ -103,7 +134,7 @@ abstract class Fetcher { // Refresh the file content try { - $responseJson = $this->fetch(); + $responseJson = $this->fetch($ETag, $content); $file->putContent(json_encode($responseJson)); return json_decode($file->getContent(), true)['data']; } catch (\Exception $e) { diff --git a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php index 3affab2dba..9d09898bb9 100644 --- a/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php +++ b/tests/lib/App/AppStore/Fetcher/AppFetcherTest.php @@ -105,6 +105,9 @@ EOD; ->expects($this->once()) ->method('getBody') ->willReturn(self::$responseJson); + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); $this->timeFactory ->expects($this->once()) ->method('getTime') @@ -1884,6 +1887,7 @@ EJL3BaQAQaASSsvFrcozYxrQG4VzEg== ), 'timestamp' => 1234, 'ncversion' => '11.0.0.2', + 'ETag' => '"myETag"', ); $dataToPut = $expected; diff --git a/tests/lib/App/AppStore/Fetcher/FetcherBase.php b/tests/lib/App/AppStore/Fetcher/FetcherBase.php index cb47d0e08a..73fcbbaab6 100644 --- a/tests/lib/App/AppStore/Fetcher/FetcherBase.php +++ b/tests/lib/App/AppStore/Fetcher/FetcherBase.php @@ -127,7 +127,10 @@ abstract class FetcherBase extends TestCase { ->expects($this->once()) ->method('getBody') ->willReturn('[{"id":"MyNewApp", "foo": "foo"}, {"id":"bar"}]'); - $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2"}'; + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; $file ->expects($this->at(0)) ->method('putContent') @@ -189,7 +192,10 @@ abstract class FetcherBase extends TestCase { ->expects($this->once()) ->method('getBody') ->willReturn('[{"id":"MyNewApp", "foo": "foo"}, {"id":"bar"}]'); - $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2"}'; + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; $file ->expects($this->at(1)) ->method('putContent') @@ -251,7 +257,10 @@ abstract class FetcherBase extends TestCase { ->expects($this->once()) ->method('getBody') ->willReturn('[{"id":"MyNewApp", "foo": "foo"}, {"id":"bar"}]'); - $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1201,"ncversion":"11.0.0.2"}'; + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1201,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; $file ->expects($this->at(1)) ->method('putContent') @@ -289,7 +298,7 @@ abstract class FetcherBase extends TestCase { $file ->expects($this->at(0)) ->method('getContent') - ->willReturn('{"timestamp":1200,"data":{"MyApp":{"id":"MyApp"}},"ncversion":"11.0.0.1"}'); + ->willReturn('{"timestamp":1200,"data":{"MyApp":{"id":"MyApp"}},"ncversion":"11.0.0.1"'); $this->timeFactory ->method('getTime') ->willReturn(1201); @@ -308,7 +317,10 @@ abstract class FetcherBase extends TestCase { ->expects($this->once()) ->method('getBody') ->willReturn('[{"id":"MyNewApp", "foo": "foo"}, {"id":"bar"}]'); - $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1201,"ncversion":"11.0.0.2"}'; + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"myETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1201,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; $file ->expects($this->at(1)) ->method('putContent') @@ -364,4 +376,147 @@ abstract class FetcherBase extends TestCase { $this->assertSame([], $this->fetcher->get()); } + + public function testGetMatchingETag() { + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + $folder + ->expects($this->once()) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + $origData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1200,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; + $file + ->expects($this->at(0)) + ->method('getContent') + ->willReturn($origData); + $this->timeFactory + ->expects($this->at(0)) + ->method('getTime') + ->willReturn(1501); + $this->timeFactory + ->expects($this->at(1)) + ->method('getTime') + ->willReturn(1502); + $client = $this->createMock(IClient::class); + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->willReturn($client); + $response = $this->createMock(IResponse::class); + $client + ->expects($this->once()) + ->method('get') + ->with( + $this->equalTo($this->endpoint), + $this->equalTo([ + 'headers' => [ + 'If-None-Match' => '"myETag"' + ] + ]) + )->willReturn($response); + $response->method('getStatusCode') + ->willReturn(304); + + $newData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'; + $file + ->expects($this->at(1)) + ->method('putContent') + ->with($newData); + $file + ->expects($this->at(2)) + ->method('getContent') + ->willReturn($newData); + + $expected = [ + [ + 'id' => 'MyNewApp', + 'foo' => 'foo', + ], + [ + 'id' => 'bar', + ], + ]; + + $this->assertSame($expected, $this->fetcher->get()); + } + + public function testGetNoMatchingETag() { + $folder = $this->createMock(ISimpleFolder::class); + $file = $this->createMock(ISimpleFile::class); + $this->appData + ->expects($this->once()) + ->method('getFolder') + ->with('/') + ->willReturn($folder); + $folder + ->expects($this->at(0)) + ->method('getFile') + ->with($this->fileName) + ->willReturn($file); + $file + ->expects($this->at(0)) + ->method('getContent') + ->willReturn('{"data":[{"id":"MyOldApp","abc":"def"}],"timestamp":1200,"ncversion":"11.0.0.2","ETag":"\"myETag\""}'); + $client = $this->createMock(IClient::class); + $this->clientService + ->expects($this->once()) + ->method('newClient') + ->willReturn($client); + $response = $this->createMock(IResponse::class); + $client + ->expects($this->once()) + ->method('get') + ->with( + $this->equalTo($this->endpoint), + $this->equalTo([ + 'headers' => [ + 'If-None-Match' => '"myETag"', + ] + ]) + ) + ->willReturn($response); + $response->method('getStatusCode') + ->willReturn(200); + $response + ->expects($this->once()) + ->method('getBody') + ->willReturn('[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}]'); + $response->method('getHeader') + ->with($this->equalTo('ETag')) + ->willReturn('"newETag"'); + $fileData = '{"data":[{"id":"MyNewApp","foo":"foo"},{"id":"bar"}],"timestamp":1502,"ncversion":"11.0.0.2","ETag":"\"newETag\""}'; + $file + ->expects($this->at(1)) + ->method('putContent') + ->with($fileData); + $file + ->expects($this->at(2)) + ->method('getContent') + ->willReturn($fileData); + $this->timeFactory + ->expects($this->at(0)) + ->method('getTime') + ->willReturn(1501); + $this->timeFactory + ->expects($this->at(1)) + ->method('getTime') + ->willReturn(1502); + + $expected = [ + [ + 'id' => 'MyNewApp', + 'foo' => 'foo', + ], + [ + 'id' => 'bar', + ], + ]; + $this->assertSame($expected, $this->fetcher->get()); + } }