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 <roeland@famdouma.nl>

diff --git a/lib/private/App/AppStore/Fetcher/AppFetcher.php b/lib/private/App/AppStore/Fetcher/AppFetcher.php
index 9ebc12dbc27c778166988716a46fd7b4b52cabb3..7c5efafc92facef3a67c98c85988fb92b04a96b9 100644 (file)
--- 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 2067242e81725a585865fe103b62b5a9be8a5b79..dab79e11821dd5bb43cd7118f67d0bf20142c1d0 100644 (file)
--- 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 3affab2dbaa4ba147fe1725a12b6e26d6c386549..9d09898bb95c9b15c039153f28531ad2eb5766bf 100644 (file)
--- 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 cb47d0e08ac6cccac547c78ec91d3059b1944c58..73fcbbaab6f55220c1736f9bb0d3bafbfa42ab65 100644 (file)
--- 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());
+       }
 }