Merge pull request #31010 from nextcloud/techdebt/noid/fixed-avatar-sizes

Only allow avatars in 64 and 512 pixel size

build/integration/features/avatar.feature

@@ -8,7 +8,7 @@ Feature: avatar
     Then The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 0 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
 
   Scenario: get default user avatar as an anonymous user
@@ -16,7 +16,7 @@ Feature: avatar
     Then The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 0 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
 
 
@@ -39,7 +39,7 @@ Feature: avatar
     Then The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 0 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
 
 
@@ -57,13 +57,13 @@ Feature: avatar
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
     And user "anonymous" gets avatar for user "user0"
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
 
   Scenario: set user avatar from internal path
@@ -112,26 +112,26 @@ Feature: avatar
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
     And user "anonymous" gets avatar for user "user0"
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
     When logged in user deletes the user avatar
     Then user "user0" gets avatar for user "user0"
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 0 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
     And user "anonymous" gets avatar for user "user0"
     And The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 0 |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
 
 
@@ -148,7 +148,7 @@ Feature: avatar
     Then The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 192
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
 
   Scenario: get user avatar with a smaller size than the original one
@@ -163,7 +163,7 @@ Feature: avatar
     Then The following headers should be set
       | Content-Type | image/png |
       | X-NC-IsCustomAvatar | 1 |
-    And last avatar is a square of size 96
+    And last avatar is a square of size 512
     And last avatar is a single "#FF0000" color
 
 
@@ -172,12 +172,12 @@ Feature: avatar
     When user "user0" gets avatar for guest "guest0"
     Then The following headers should be set
       | Content-Type | image/png |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color
 
   Scenario: get default guest avatar as an anonymous user
     When user "anonymous" gets avatar for guest "guest0"
     Then The following headers should be set
       | Content-Type | image/png |
-    And last avatar is a square of size 128
+    And last avatar is a square of size 512
     And last avatar is not a single color

core/Controller/AvatarController.php

@@ -114,11 +114,16 @@ class AvatarController extends Controller {
 	 * @return JSONResponse|FileDisplayResponse
 	 */
 	public function getAvatar($userId, $size) {
-		// min/max size
-		if ($size > 2048) {
-			$size = 2048;
-		} elseif ($size <= 0) {
+		if ($size <= 64) {
+			if ($size !== 64) {
+				$this->logger->debug('Avatar requested in deprecated size ' . $size);
+			}
 			$size = 64;
+		} else {
+			if ($size !== 512) {
+				$this->logger->debug('Avatar requested in deprecated size ' . $size);
+			}
+			$size = 512;
 		}
 
 		try {

core/Controller/GuestAvatarController.php

@@ -76,11 +76,16 @@ class GuestAvatarController extends Controller {
 	public function getAvatar($guestName, $size) {
 		$size = (int) $size;
 
-		// min/max size
-		if ($size > 2048) {
-			$size = 2048;
-		} elseif ($size <= 0) {
+		if ($size <= 64) {
+			if ($size !== 64) {
+				$this->logger->debug('Avatar requested in deprecated size ' . $size);
+			}
 			$size = 64;
+		} else {
+			if ($size !== 512) {
+				$this->logger->debug('Avatar requested in deprecated size ' . $size);
+			}
+			$size = 512;
 		}
 
 		try {

tests/Core/Controller/AvatarControllerTest.php

@@ -193,24 +193,7 @@ class AvatarControllerTest extends \Test\TestCase {
 		$this->assertEquals(Http::STATUS_NOT_FOUND, $response->getStatus());
 	}
 
-	/**
-	 * Make sure we get the correct size
-	 */
-	public function testGetAvatarSize() {
-		$this->avatarMock->expects($this->once())
-			->method('getFile')
-			->with($this->equalTo(32))
-			->willReturn($this->avatarFile);
-
-		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
-
-		$this->avatarController->getAvatar('userId', 32);
-	}
-
-	/**
-	 * We cannot get avatars that are 0 or negative
-	 */
-	public function testGetAvatarSizeMin() {
+	public function testGetAvatarSize64(): void {
 		$this->avatarMock->expects($this->once())
 			->method('getFile')
 			->with($this->equalTo(64))
@@ -218,21 +201,78 @@ class AvatarControllerTest extends \Test\TestCase {
 
 		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
 
-		$this->avatarController->getAvatar('userId', 0);
+		$this->logger->expects($this->never())
+			->method('debug');
+
+		$this->avatarController->getAvatar('userId', 64);
 	}
 
-	/**
-	 * We do not support avatars larger than 2048*2048
-	 */
-	public function testGetAvatarSizeMax() {
+	public function testGetAvatarSize512(): void {
 		$this->avatarMock->expects($this->once())
 			->method('getFile')
-			->with($this->equalTo(2048))
+			->with($this->equalTo(512))
 			->willReturn($this->avatarFile);
 
 		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
 
-		$this->avatarController->getAvatar('userId', 2049);
+		$this->logger->expects($this->never())
+			->method('debug');
+
+		$this->avatarController->getAvatar('userId', 512);
+	}
+
+	/**
+	 * Small sizes return 64 and generate a log
+	 */
+	public function testGetAvatarSizeTooSmall(): void {
+		$this->avatarMock->expects($this->once())
+			->method('getFile')
+			->with($this->equalTo(64))
+			->willReturn($this->avatarFile);
+
+		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
+
+		$this->logger->expects($this->once())
+			->method('debug')
+			->with('Avatar requested in deprecated size 32');
+
+		$this->avatarController->getAvatar('userId', 32);
+	}
+
+	/**
+	 * Avatars between 64 and 512 are upgraded to 512
+	 */
+	public function testGetAvatarSizeBetween(): void {
+		$this->avatarMock->expects($this->once())
+			->method('getFile')
+			->with($this->equalTo(512))
+			->willReturn($this->avatarFile);
+
+		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
+
+		$this->logger->expects($this->once())
+			->method('debug')
+			->with('Avatar requested in deprecated size 65');
+
+		$this->avatarController->getAvatar('userId', 65);
+	}
+
+	/**
+	 * We do not support avatars larger than 512
+	 */
+	public function testGetAvatarSizeTooBig(): void {
+		$this->avatarMock->expects($this->once())
+			->method('getFile')
+			->with($this->equalTo(512))
+			->willReturn($this->avatarFile);
+
+		$this->avatarManager->method('getAvatar')->willReturn($this->avatarMock);
+
+		$this->logger->expects($this->once())
+			->method('debug')
+			->with('Avatar requested in deprecated size 513');
+
+		$this->avatarController->getAvatar('userId', 513);
 	}
 
 	/**

tests/Core/Controller/GuestAvatarControllerTest.php

@@ -77,7 +77,7 @@ class GuestAvatarControllerTest extends \Test\TestCase {
 
 		$this->avatar->expects($this->once())
 			->method('getFile')
-			->with(128)
+			->with(512)
 			->willReturn($this->file);
 
 		$this->file->method('getMimeType')