From 14136295b7568c6e34504c101eba0ee10f5c74fd Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Aug 2016 14:55:23 +0200 Subject: [PATCH 1/2] Cache avatars properly * Set proper caching headers for avatars (15 minutes) * For our own avatar use some extra logic to invalidate when we update --- core/Controller/AvatarController.php | 34 +++++++++++++++++++---- core/js/config.php | 14 ++++++++-- core/js/jquery.avatar.js | 23 ++++++++++++--- core/templates/layout.user.php | 4 +-- lib/private/Avatar.php | 14 +++++++++- lib/private/AvatarManager.php | 11 ++++++-- lib/private/Server.php | 3 +- lib/private/TemplateLayout.php | 1 + lib/public/AppFramework/Http/Response.php | 3 +- settings/js/personal.js | 13 +++++++-- 10 files changed, 98 insertions(+), 22 deletions(-) diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php index 5b64320948..484b6f524b 100644 --- a/core/Controller/AvatarController.php +++ b/core/Controller/AvatarController.php @@ -27,6 +27,7 @@ */ namespace OC\Core\Controller; +use OC\AppFramework\Utility\TimeFactory; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataDisplayResponse; @@ -73,6 +74,9 @@ class AvatarController extends Controller { /** @var string */ protected $userId; + /** @var TimeFactory */ + protected $timeFactory; + /** * @param string $appName * @param IRequest $request @@ -83,6 +87,7 @@ class AvatarController extends Controller { * @param IRootFolder $rootFolder * @param ILogger $logger * @param string $userId + * @param TimeFactory $timeFactory */ public function __construct($appName, IRequest $request, @@ -92,7 +97,8 @@ class AvatarController extends Controller { IUserManager $userManager, IRootFolder $rootFolder, ILogger $logger, - $userId) { + $userId, + TimeFactory $timeFactory) { parent::__construct($appName, $request); $this->avatarManager = $avatarManager; @@ -102,6 +108,7 @@ class AvatarController extends Controller { $this->rootFolder = $rootFolder; $this->logger = $logger; $this->userId = $userId; + $this->timeFactory = $timeFactory; } /** @@ -126,6 +133,21 @@ class AvatarController extends Controller { Http::STATUS_OK, ['Content-Type' => $avatar->getMimeType()]); $resp->setETag($avatar->getEtag()); + + // Let cache this! + $resp->addHeader('Pragma', 'public'); + // Cache for 15 minutes + $resp->cacheFor(900); + // Set last modified + $lastModified = new \DateTime(); + $lastModified->setTimestamp($avatar->getMTime()); + $resp->setLastModified($lastModified); + + $expires = new \DateTime(); + $expires->setTimestamp($this->timeFactory->getTime()); + $expires->add(new \DateInterval('PT15M')); + $resp->addHeader('Expires', $expires->format(\DateTime::RFC2822)); + } catch (NotFoundException $e) { $user = $this->userManager->get($userId); $resp = new JSONResponse([ @@ -133,18 +155,20 @@ class AvatarController extends Controller { 'displayname' => $user->getDisplayName(), ], ]); + // Don't cache this + $resp->cacheFor(0); + $resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT'))); } catch (\Exception $e) { $resp = new JSONResponse([ 'data' => [ 'displayname' => '', ], ]); + // Don't cache this + $resp->cacheFor(0); + $resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT'))); } - $resp->addHeader('Pragma', 'public'); - $resp->cacheFor(0); - $resp->setLastModified(new \DateTime('now', new \DateTimeZone('GMT'))); - return $resp; } diff --git a/core/js/config.php b/core/js/config.php index e88d9c741a..8e85dc91cb 100644 --- a/core/js/config.php +++ b/core/js/config.php @@ -149,8 +149,8 @@ $array = array( "firstDay" => json_encode($l->l('firstday', null)) , "oc_config" => json_encode( array( - 'session_lifetime' => min(\OCP\Config::getSystemValue('session_lifetime', OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), - 'session_keepalive' => \OCP\Config::getSystemValue('session_keepalive', true), + 'session_lifetime' => min($config->getSystemValue('session_lifetime', OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), OC::$server->getIniWrapper()->getNumeric('session.gc_maxlifetime')), + 'session_keepalive' => $config->getSystemValue('session_keepalive', true), 'version' => implode('.', \OCP\Util::getVersion()), 'versionstring' => OC_Util::getVersionString(), 'enable_avatars' => \OC::$server->getConfig()->getSystemValue('enable_avatars', true) === true, @@ -187,9 +187,17 @@ $array = array( 'longFooter' => $defaults->getLongFooter(), 'folder' => OC_Util::getTheme(), ) - ) + ), ); +if (OC_User::getUser() !== null && OC_User::getUser() !== false) { + $array['oc_userconfig'] = json_encode([ + 'avatar' => [ + 'version' => (int)$config->getUserValue(OC_User::getUser(), 'avatar', 'version', 0), + ] + ]); +} + // Allow hooks to modify the output values OC_Hook::emit('\OCP\Config', 'js', array('array' => &$array)); diff --git a/core/js/jquery.avatar.js b/core/js/jquery.avatar.js index 6ae9cf78a1..754400acd7 100644 --- a/core/js/jquery.avatar.js +++ b/core/js/jquery.avatar.js @@ -74,10 +74,25 @@ user = String(user).replace(/\//g,''); var $div = this; + var url; - var url = OC.generateUrl( - '/avatar/{user}/{size}', - {user: user, size: Math.ceil(size * window.devicePixelRatio)}); + // If this is our own avatar we have to use the version attribute + if (user === OC.getCurrentUser().uid) { + url = OC.generateUrl( + '/avatar/{user}/{size}?v={version}', + { + user: user, + size: Math.ceil(size * window.devicePixelRatio), + version: oc_userconfig.avatar.version + }); + } else { + url = OC.generateUrl( + '/avatar/{user}/{size}', + { + user: user, + size: Math.ceil(size * window.devicePixelRatio) + }); + } // If the displayname is not defined we use the old code path if (typeof(displayname) === 'undefined') { @@ -122,7 +137,7 @@ $div.show(); $div.text(''); $div.append(img); - } + }; img.width = size; img.height = size; diff --git a/core/templates/layout.user.php b/core/templates/layout.user.php index 17f895bc17..b5466b6fc0 100644 --- a/core/templates/layout.user.php +++ b/core/templates/layout.user.php @@ -66,8 +66,8 @@
diff --git a/lib/private/Avatar.php b/lib/private/Avatar.php index 80691774b6..9e8bd0136c 100644 --- a/lib/private/Avatar.php +++ b/lib/private/Avatar.php @@ -34,6 +34,7 @@ use OCP\Files\File; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\IAvatar; +use OCP\IConfig; use OCP\IImage; use OCP\IL10N; use OC_Image; @@ -52,6 +53,8 @@ class Avatar implements IAvatar { private $user; /** @var ILogger */ private $logger; + /** @var IConfig */ + private $config; /** * constructor @@ -60,12 +63,18 @@ class Avatar implements IAvatar { * @param IL10N $l * @param User $user * @param ILogger $logger + * @param IConfig $config */ - public function __construct (Folder $folder, IL10N $l, $user, ILogger $logger) { + public function __construct(Folder $folder, + IL10N $l, + $user, + ILogger $logger, + IConfig $config) { $this->folder = $folder; $this->l = $l; $this->user = $user; $this->logger = $logger; + $this->config = $config; } /** @@ -137,6 +146,9 @@ class Avatar implements IAvatar { $regex = '/^avatar\.([0-9]+\.)?(jpg|png)$/'; $avatars = $this->folder->getDirectoryListing(); + $this->config->setUserValue($this->user->getUID(), 'avatar', 'version', + (int)$this->config->getUserValue($this->user->getUID(), 'avatar', 'version', 0) + 1); + foreach ($avatars as $avatar) { if (preg_match($regex, $avatar->getName())) { $avatar->delete(); diff --git a/lib/private/AvatarManager.php b/lib/private/AvatarManager.php index e461a70608..0eabc3a175 100644 --- a/lib/private/AvatarManager.php +++ b/lib/private/AvatarManager.php @@ -30,6 +30,7 @@ namespace OC; use OCP\Files\Folder; use OCP\Files\NotFoundException; use OCP\IAvatarManager; +use OCP\IConfig; use OCP\ILogger; use OCP\IUserManager; use OCP\Files\IRootFolder; @@ -52,6 +53,9 @@ class AvatarManager implements IAvatarManager { /** @var ILogger */ private $logger; + /** @var IConfig */ + private $config; + /** * AvatarManager constructor. * @@ -59,16 +63,19 @@ class AvatarManager implements IAvatarManager { * @param IRootFolder $rootFolder * @param IL10N $l * @param ILogger $logger + * @param IConfig $config */ public function __construct( IUserManager $userManager, IRootFolder $rootFolder, IL10N $l, - ILogger $logger) { + ILogger $logger, + IConfig $config) { $this->userManager = $userManager; $this->rootFolder = $rootFolder; $this->l = $l; $this->logger = $logger; + $this->config = $config; } /** @@ -94,6 +101,6 @@ class AvatarManager implements IAvatarManager { /** @var Folder $folder */ $folder = $this->rootFolder->get($dir); - return new Avatar($folder, $this->l, $user, $this->logger); + return new Avatar($folder, $this->l, $user, $this->logger, $this->config); } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 6f6d403210..494387ab6c 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -361,7 +361,8 @@ class Server extends ServerContainer implements IServerContainer { $c->getUserManager(), $c->getRootFolder(), $c->getL10N('lib'), - $c->getLogger() + $c->getLogger(), + $c->getConfig() ); }); $this->registerService('Logger', function (Server $c) { diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 7e5c120717..da845d80d0 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -112,6 +112,7 @@ class TemplateLayout extends \OC_Template { $this->assign('userAvatarSet', false); } else { $this->assign('userAvatarSet', \OC::$server->getAvatarManager()->getAvatar(\OC_User::getUser())->exists()); + $this->assign('userAvatarVersion', \OC::$server->getConfig()->getUserValue(\OC_User::getUser(), 'avatar', 'version', 0)); } } else if ($renderAs == 'error') { diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index eed9aa7c1a..0b162b453a 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -92,8 +92,7 @@ class Response { public function cacheFor($cacheSeconds) { if($cacheSeconds > 0) { - $this->addHeader('Cache-Control', 'max-age=' . $cacheSeconds . - ', must-revalidate'); + $this->addHeader('Cache-Control', 'max-age=' . $cacheSeconds . ', must-revalidate'); } else { $this->addHeader('Cache-Control', 'no-cache, no-store, must-revalidate'); } diff --git a/settings/js/personal.js b/settings/js/personal.js index 16a8d184da..e0c99ae774 100644 --- a/settings/js/personal.js +++ b/settings/js/personal.js @@ -100,6 +100,9 @@ function updateAvatar (hidedefault) { var $headerdiv = $('#header .avatardiv'); var $displaydiv = $('#displayavatar .avatardiv'); + //Bump avatar avatarversion + oc_userconfig.avatar.version = -(Math.floor(Math.random() * 1000)); + if (hidedefault) { $headerdiv.hide(); $('#header .avatardiv').removeClass('avatardiv-shown'); @@ -112,7 +115,10 @@ function updateAvatar (hidedefault) { $displaydiv.avatar(OC.currentUser, 145, true); $.get(OC.generateUrl( '/avatar/{user}/{size}', - {user: OC.currentUser, size: 1} + { + user: OC.currentUser, + size: 1 + } ), function (result) { if (typeof(result) === 'string') { // Show the delete button when the avatar is custom @@ -358,7 +364,10 @@ $(document).ready(function () { // does the user have a custom avatar? if he does show #removeavatar $.get(OC.generateUrl( '/avatar/{user}/{size}', - {user: OC.currentUser, size: 1} + { + user: OC.currentUser, + size: 1 + } ), function (result) { if (typeof(result) === 'string') { // Show the delete button when the avatar is custom From 6a85882f610ed34c971a1510f5876f0564fd7612 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 29 Aug 2016 15:17:59 +0200 Subject: [PATCH 2/2] Fix tests --- .../Core/Controller/AvatarControllerTest.php | 7 +++- tests/lib/AvatarTest.php | 42 +++++++++++++------ 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/tests/Core/Controller/AvatarControllerTest.php b/tests/Core/Controller/AvatarControllerTest.php index fe1a44b28a..af75c4bb75 100644 --- a/tests/Core/Controller/AvatarControllerTest.php +++ b/tests/Core/Controller/AvatarControllerTest.php @@ -31,6 +31,7 @@ function is_uploaded_file($filename) { namespace Tests\Core\Controller; +use OC\AppFramework\Utility\TimeFactory; use OC\Core\Controller\AvatarController; use OCP\AppFramework\Http; use OCP\Files\Cache\ICache; @@ -74,6 +75,8 @@ class AvatarControllerTest extends \Test\TestCase { private $logger; /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ private $request; + /** @var TimeFactory|\PHPUnit_Framework_MockObject_MockObject */ + private $timeFactory; protected function setUp() { parent::setUp(); @@ -87,6 +90,7 @@ class AvatarControllerTest extends \Test\TestCase { $this->request = $this->getMockBuilder('OCP\IRequest')->getMock(); $this->rootFolder = $this->getMockBuilder('OCP\Files\IRootFolder')->getMock(); $this->logger = $this->getMockBuilder('OCP\ILogger')->getMock(); + $this->timeFactory = $this->getMockBuilder('OC\AppFramework\Utility\TimeFactory')->getMock(); $this->avatarMock = $this->getMockBuilder('OCP\IAvatar')->getMock(); $this->userMock = $this->getMockBuilder('OCP\IUser')->getMock(); @@ -100,7 +104,8 @@ class AvatarControllerTest extends \Test\TestCase { $this->userManager, $this->rootFolder, $this->logger, - 'userid' + 'userid', + $this->timeFactory ); // Configure userMock diff --git a/tests/lib/AvatarTest.php b/tests/lib/AvatarTest.php index 0a00f5d561..f515f0d013 100644 --- a/tests/lib/AvatarTest.php +++ b/tests/lib/AvatarTest.php @@ -20,15 +20,26 @@ class AvatarTest extends \Test\TestCase { /** @var \OC\User\User | \PHPUnit_Framework_MockObject_MockObject $user */ private $user; + /** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */ + private $config; + public function setUp() { parent::setUp(); - $this->folder = $this->getMock('\OCP\Files\Folder'); + $this->folder = $this->getMockBuilder('OCP\Files\Folder')->getMock(); /** @var \OCP\IL10N | \PHPUnit_Framework_MockObject_MockObject $l */ - $l = $this->getMock('\OCP\IL10N'); + $l = $this->getMockBuilder('OCP\IL10N')->getMock(); $l->method('t')->will($this->returnArgument(0)); - $this->user = $this->getMockBuilder('\OC\User\User')->disableOriginalConstructor()->getMock(); - $this->avatar = new \OC\Avatar($this->folder, $l, $this->user, $this->getMock('\OCP\ILogger')); + $this->user = $this->getMockBuilder('OC\User\User')->disableOriginalConstructor()->getMock(); + $this->config = $this->getMockBuilder('OCP\IConfig')->getMock(); + + $this->avatar = new \OC\Avatar( + $this->folder, + $l, + $this->user, + $this->getMockBuilder('\OCP\ILogger')->getMock(), + $this->config + ); } public function testGetNoAvatar() { @@ -44,7 +55,7 @@ class AvatarTest extends \Test\TestCase { $expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png'); - $file = $this->getMock('\OCP\Files\File'); + $file = $this->getMockBuilder('OCP\Files\File')->getMock(); $file->method('getContent')->willReturn($expected->data()); $this->folder->method('get')->with('avatar.128.jpg')->willReturn($file); @@ -59,7 +70,7 @@ class AvatarTest extends \Test\TestCase { $expected = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png'); - $file = $this->getMock('\OCP\Files\File'); + $file = $this->getMockBuilder('OCP\Files\File')->getMock(); $file->method('getContent')->willReturn($expected->data()); $this->folder->method('get')->with('avatar.jpg')->willReturn($file); @@ -77,7 +88,7 @@ class AvatarTest extends \Test\TestCase { $expected2 = new \OC_Image(\OC::$SERVERROOT . '/tests/data/testavatar.png'); $expected2->resize(32); - $file = $this->getMock('\OCP\Files\File'); + $file = $this->getMockBuilder('OCP\Files\File')->getMock(); $file->method('getContent')->willReturn($expected->data()); $this->folder->method('get') @@ -91,7 +102,7 @@ class AvatarTest extends \Test\TestCase { } )); - $newFile = $this->getMock('\OCP\Files\File'); + $newFile = $this->getMockBuilder('OCP\Files\File')->getMock(); $newFile->expects($this->once()) ->method('putContent') ->with($expected2->data()); @@ -129,22 +140,22 @@ class AvatarTest extends \Test\TestCase { } public function testSetAvatar() { - $avatarFileJPG = $this->getMock('\OCP\Files\File'); + $avatarFileJPG = $this->getMockBuilder('OCP\Files\File')->getMock(); $avatarFileJPG->method('getName') ->willReturn('avatar.jpg'); $avatarFileJPG->expects($this->once())->method('delete'); - $avatarFilePNG = $this->getMock('\OCP\Files\File'); + $avatarFilePNG = $this->getMockBuilder('OCP\Files\File')->getMock(); $avatarFilePNG->method('getName') ->willReturn('avatar.png'); $avatarFilePNG->expects($this->once())->method('delete'); - $resizedAvatarFile = $this->getMock('\OCP\Files\File'); + $resizedAvatarFile = $this->getMockBuilder('OCP\Files\File')->getMock(); $resizedAvatarFile->method('getName') ->willReturn('avatar.32.jpg'); $resizedAvatarFile->expects($this->once())->method('delete'); - $nonAvatarFile = $this->getMock('\OCP\Files\File'); + $nonAvatarFile = $this->getMockBuilder('OCP\Files\File')->getMock(); $nonAvatarFile->method('getName') ->willReturn('avatarX'); $nonAvatarFile->expects($this->never())->method('delete'); @@ -152,7 +163,7 @@ class AvatarTest extends \Test\TestCase { $this->folder->method('getDirectoryListing') ->willReturn([$avatarFileJPG, $avatarFilePNG, $resizedAvatarFile, $nonAvatarFile]); - $newFile = $this->getMock('\OCP\Files\File'); + $newFile = $this->getMockBuilder('OCP\Files\File')->getMock(); $this->folder->expects($this->once()) ->method('newFile') ->with('avatar.png') @@ -163,6 +174,11 @@ class AvatarTest extends \Test\TestCase { ->method('putContent') ->with($image->data()); + $this->config->expects($this->once()) + ->method('setUserValue'); + $this->config->expects($this->once()) + ->method('getUserValue'); + // One on remove and once on setting the new avatar $this->user->expects($this->exactly(2))->method('triggerChange');