From 12fa748c49773d57f03052ef3cee4c72a2d3cdcc Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 11 May 2020 16:18:01 +0200 Subject: [PATCH] Move the notmodified check to middleware where it belongs Signed-off-by: Roeland Jago Douma --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../DependencyInjection/DIContainer.php | 2 + lib/private/AppFramework/Http.php | 19 +--- lib/private/AppFramework/Http/Dispatcher.php | 3 +- .../Middleware/NotModifiedMiddleware.php | 56 +++++++++ .../lib/AppFramework/Http/DispatcherTest.php | 10 +- tests/lib/AppFramework/Http/HttpTest.php | 32 ------ .../Middleware/NotModifiedMiddlewareTest.php | 106 ++++++++++++++++++ 9 files changed, 169 insertions(+), 61 deletions(-) create mode 100644 lib/private/AppFramework/Middleware/NotModifiedMiddleware.php create mode 100644 tests/lib/AppFramework/Middleware/NotModifiedMiddlewareTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 718e27c78f..c7b966fd59 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -532,6 +532,7 @@ return array( 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => $baseDir . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', + 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', 'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index aa0528b176..8212d3dafc 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -561,6 +561,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\AppFramework\\Middleware\\AdditionalScriptsMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/AdditionalScriptsMiddleware.php', 'OC\\AppFramework\\Middleware\\CompressionMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/CompressionMiddleware.php', 'OC\\AppFramework\\Middleware\\MiddlewareDispatcher' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/MiddlewareDispatcher.php', + 'OC\\AppFramework\\Middleware\\NotModifiedMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php', 'OC\\AppFramework\\Middleware\\OCSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/OCSMiddleware.php', 'OC\\AppFramework\\Middleware\\PublicShare\\Exceptions\\NeedAuthenticationException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/Exceptions/NeedAuthenticationException.php', 'OC\\AppFramework\\Middleware\\PublicShare\\PublicShareMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 15ae8ab6b0..82a2780eb2 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -188,6 +188,8 @@ class DIContainer extends SimpleContainer implements IAppContainer { $c->query(OC\AppFramework\Middleware\CompressionMiddleware::class) ); + $dispatcher->registerMiddleware($c->query(OC\AppFramework\Middleware\NotModifiedMiddleware::class)); + $dispatcher->registerMiddleware( $c->query(OC\AppFramework\Middleware\Security\ReloadExecutionMiddleware::class) ); diff --git a/lib/private/AppFramework/Http.php b/lib/private/AppFramework/Http.php index 3c4a52fe37..88e49816eb 100644 --- a/lib/private/AppFramework/Http.php +++ b/lib/private/AppFramework/Http.php @@ -116,24 +116,7 @@ class Http extends BaseHttp { * @param string $ETag the etag * @return string */ - public function getStatusHeader($status, \DateTime $lastModified=null, - $ETag=null) { - if (!is_null($lastModified)) { - $lastModified = $lastModified->format(\DateTime::RFC2822); - } - - // if etag or lastmodified have not changed, return a not modified - if ((isset($this->server['HTTP_IF_NONE_MATCH']) - && trim(trim($this->server['HTTP_IF_NONE_MATCH']), '"') === (string)$ETag) - - || - - (isset($this->server['HTTP_IF_MODIFIED_SINCE']) - && trim($this->server['HTTP_IF_MODIFIED_SINCE']) === - $lastModified)) { - $status = self::STATUS_NOT_MODIFIED; - } - + public function getStatusHeader($status) { // we have one change currently for the http 1.0 header that differs // from 1.1: STATUS_TEMPORARY_REDIRECT should be STATUS_FOUND // if this differs any more, we want to create childclasses for this diff --git a/lib/private/AppFramework/Http/Dispatcher.php b/lib/private/AppFramework/Http/Dispatcher.php index 629905e5da..3892bb667f 100644 --- a/lib/private/AppFramework/Http/Dispatcher.php +++ b/lib/private/AppFramework/Http/Dispatcher.php @@ -116,8 +116,7 @@ class Dispatcher { $controller, $methodName, $response); // depending on the cache object the headers need to be changed - $out[0] = $this->protocol->getStatusHeader($response->getStatus(), - $response->getLastModified(), $response->getETag()); + $out[0] = $this->protocol->getStatusHeader($response->getStatus()); $out[1] = array_merge($response->getHeaders()); $out[2] = $response->getCookies(); $out[3] = $this->middlewareDispatcher->beforeOutput( diff --git a/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php b/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php new file mode 100644 index 0000000000..e34310b411 --- /dev/null +++ b/lib/private/AppFramework/Middleware/NotModifiedMiddleware.php @@ -0,0 +1,56 @@ + + * + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\AppFramework\Middleware; + +use OCP\AppFramework\Http; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; +use OCP\IRequest; + +class NotModifiedMiddleware extends Middleware { + /** @var IRequest */ + private $request; + + public function __construct(IRequest $request) { + $this->request = $request; + } + + public function afterController($controller, $methodName, Response $response) { + $etagHeader = $this->request->getHeader('IF_NONE_MATCH'); + if ($etagHeader !== '' && $response->getETag() !== null && trim($etagHeader) === '"' . $response->getETag() . '"') { + $response->setStatus(Http::STATUS_NOT_MODIFIED); + return $response; + } + + $modifiedSinceHeader = $this->request->getHeader('IF_MODIFIED_SINCE'); + if ($modifiedSinceHeader !== '' && $response->getLastModified() !== null && trim($modifiedSinceHeader) === $response->getLastModified()->format(\DateTime::RFC2822)) { + $response->setStatus(Http::STATUS_NOT_MODIFIED); + return $response; + } + + return $response; + } +} diff --git a/tests/lib/AppFramework/Http/DispatcherTest.php b/tests/lib/AppFramework/Http/DispatcherTest.php index 74dbf68c28..c4c973aec9 100644 --- a/tests/lib/AppFramework/Http/DispatcherTest.php +++ b/tests/lib/AppFramework/Http/DispatcherTest.php @@ -180,20 +180,12 @@ class DispatcherTest extends \Test\TestCase { $this->response->expects($this->once()) ->method('getStatus') ->willReturn(Http::STATUS_OK); - $this->response->expects($this->once()) - ->method('getLastModified') - ->willReturn($this->lastModified); - $this->response->expects($this->once()) - ->method('getETag') - ->willReturn($this->etag); $this->response->expects($this->once()) ->method('getHeaders') ->willReturn($responseHeaders); $this->http->expects($this->once()) ->method('getStatusHeader') - ->with($this->equalTo(Http::STATUS_OK), - $this->equalTo($this->lastModified), - $this->equalTo($this->etag)) + ->with($this->equalTo(Http::STATUS_OK)) ->willReturn($httpHeaders); $this->middlewareDispatcher->expects($this->once()) diff --git a/tests/lib/AppFramework/Http/HttpTest.php b/tests/lib/AppFramework/Http/HttpTest.php index 009bb39260..d3d23425f7 100644 --- a/tests/lib/AppFramework/Http/HttpTest.php +++ b/tests/lib/AppFramework/Http/HttpTest.php @@ -53,38 +53,6 @@ class HttpTest extends \Test\TestCase { $this->assertEquals('HTTP/1.0 200 OK', $header); } - - public function testEtagMatchReturnsNotModified() { - $http = new Http(['HTTP_IF_NONE_MATCH' => 'hi']); - - $header = $http->getStatusHeader(Http::STATUS_OK, null, 'hi'); - $this->assertEquals('HTTP/1.1 304 Not Modified', $header); - } - - - public function testQuotedEtagMatchReturnsNotModified() { - $http = new Http(['HTTP_IF_NONE_MATCH' => '"hi"']); - - $header = $http->getStatusHeader(Http::STATUS_OK, null, 'hi'); - $this->assertEquals('HTTP/1.1 304 Not Modified', $header); - } - - - public function testLastModifiedMatchReturnsNotModified() { - $dateTime = new \DateTime(null, new \DateTimeZone('GMT')); - $dateTime->setTimestamp('12'); - - $http = new Http( - [ - 'HTTP_IF_MODIFIED_SINCE' => 'Thu, 01 Jan 1970 00:00:12 +0000'] - ); - - $header = $http->getStatusHeader(Http::STATUS_OK, $dateTime); - $this->assertEquals('HTTP/1.1 304 Not Modified', $header); - } - - - public function testTempRedirectBecomesFoundInHttp10() { $http = new Http([], 'HTTP/1.0'); diff --git a/tests/lib/AppFramework/Middleware/NotModifiedMiddlewareTest.php b/tests/lib/AppFramework/Middleware/NotModifiedMiddlewareTest.php new file mode 100644 index 0000000000..89ae75bcac --- /dev/null +++ b/tests/lib/AppFramework/Middleware/NotModifiedMiddlewareTest.php @@ -0,0 +1,106 @@ + + * + * @author Roeland Jago Douma + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace Test\AppFramework\Middleware; + +use OC\AppFramework\Middleware\NotModifiedMiddleware; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http; +use OCP\IRequest; + +class NotModifiedMiddlewareTest extends \Test\TestCase { + + /** @var IRequest */ + private $request; + /** @var Controller */ + private $controller; + /** @var NotModifiedMiddleware */ + private $middleWare; + + protected function setUp(): void { + parent::setUp(); + + $this->request = $this->createMock(IRequest::class); + $this->middleWare = new NotModifiedMiddleware( + $this->request + ); + + $this->controller = $this->createMock(Controller::class); + } + + public function dataModified(): array { + $now = new \DateTime(); + + return [ + [null, '', null, '', false], + ['etag', 'etag', null, '', false], + ['etag', '"wrongetag"', null, '', false], + ['etag', '', null, '', false], + [null, '"etag"', null, '', false], + ['etag', '"etag"', null, '', true], + + [null, '', $now, $now->format(\DateTime::RFC2822), true], + [null, '', $now, $now->format(\DateTime::ATOM), false], + [null, '', null, $now->format(\DateTime::RFC2822), false], + [null, '', $now, '', false], + + ['etag', '"etag"', $now, $now->format(\DateTime::ATOM), true], + ['etag', '"etag"', $now, $now->format(\DateTime::RFC2822), true], + ]; + } + + /** + * @dataProvider dataModified + */ + public function testMiddleware(?string $etag, string $etagHeader, ?\DateTime $lastModified, string $lastModifiedHeader, bool $notModifiedSet) { + $this->request->method('getHeader') + ->willReturnCallback(function (string $name) use ($etagHeader, $lastModifiedHeader) { + if ($name === 'IF_NONE_MATCH') { + return $etagHeader; + } + if ($name === 'IF_MODIFIED_SINCE') { + return $lastModifiedHeader; + } + return ''; + }); + + $response = new Http\Response(); + if ($etag !== null) { + $response->setETag($etag); + } + if ($lastModified !== null) { + $response->setLastModified($lastModified); + } + $response->setStatus(Http::STATUS_OK); + + $result = $this->middleWare->afterController($this->controller, 'myfunction', $response); + + if ($notModifiedSet) { + $this->assertSame(Http::STATUS_NOT_MODIFIED, $result->getStatus()); + } else { + $this->assertSame(Http::STATUS_OK, $result->getStatus()); + } + } +}