Cache avatars properly

* Set proper caching headers for avatars (15 minutes)
* For our own avatar use some extra logic to invalidate when we update

10 files changed, 98 insertions, 22 deletions

diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php
index 5b64320948a..484b6f524bf 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 e88d9c741a1..8e85dc91cb1 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 6ae9cf78a13..754400acd7b 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 17f895bc17d..b5466b6fc05 100644
--- a/core/templates/layout.user.php
+++ b/core/templates/layout.user.php
@@ -66,8 +66,8 @@
 					<div class="avatardiv<?php if ($_['userAvatarSet']) { print_unescaped(' avatardiv-shown'); } else { print_unescaped('" style="display: none'); } ?>">
 						<?php if ($_['userAvatarSet']): ?>
 							<img alt="" width="32" height="32"
-							src="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 32]));?>"
-							srcset="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 64]));?> 2x, <?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 128]));?> 4x"
+							src="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 32, 'v' => $_['userAvatarVersion']]));?>"
+							srcset="<?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 64, 'v' => $_['userAvatarVersion']]));?> 2x, <?php p(\OC::$server->getURLGenerator()->linkToRoute('core.avatar.getAvatar', ['userId' => $_['user_uid'], 'size' => 128, 'v' => $_['userAvatarVersion']]));?> 4x"
 							>
 						<?php endif; ?>
 					</div>
diff --git a/lib/private/Avatar.php b/lib/private/Avatar.php
index 80691774b64..9e8bd0136c2 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 e461a70608b..0eabc3a1754 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 6f6d403210d..494387ab6ca 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 7e5c120717f..da845d80d04 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 eed9aa7c1a2..0b162b453a0 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 16a8d184da6..e0c99ae774d 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