[AvatarController] Restrict UploadAvatar to current user (#3839)

Author: imnasnainaec

Backend.Tests/Controllers/AvatarControllerTests.cs

@@ -32,6 +32,8 @@ protected virtual void Dispose(bool disposing)
         }
 
         private User _jwtAuthenticatedUser = null!;
+        private const string FileName = "combine.png";  // File in Backend.Tests/Assets/
+        private readonly string _filePath = Path.Combine(Util.AssetsDir, FileName);
 
         [SetUp]
         public void Setup()
@@ -48,6 +50,7 @@ public void Setup()
             _userRepo.Create(_jwtAuthenticatedUser);
             _jwtAuthenticatedUser = _permissionService.Authenticate(_jwtAuthenticatedUser.Username,
                 _jwtAuthenticatedUser.Password).Result ?? throw new UserAuthenticationException();
+            _avatarController.ControllerContext.HttpContext.Request.Headers["UserId"] = _jwtAuthenticatedUser.Id;
         }
 
         /// <summary> Delete the image file stored on disk for a particular user. </summary>
@@ -62,19 +65,67 @@ private static void DeleteAvatarFile(string userId)
         }
 
         [Test]
-        public void TestAvatarImport()
+        public void TestDownloadAvatarNoUser()
         {
-            const string fileName = "combine.png";  // file in Backend.Tests/Assets/
-            var filePath = Path.Combine(Util.AssetsDir, fileName);
-            using var stream = File.OpenRead(filePath);
-            var file = new FormFile(stream, 0, stream.Length, "dave", fileName);
+            var result = _avatarController.DownloadAvatar("false-user-id").Result;
+            Assert.That(result, Is.InstanceOf<NotFoundResult>());
+        }
+
+        [Test]
+        public void TestDownloadAvatarNoAvatar()
+        {
+            var result = _avatarController.DownloadAvatar(_jwtAuthenticatedUser.Id).Result;
+            Assert.That(result, Is.InstanceOf<NotFoundResult>());
+        }
+
+        [Test]
+        public void TestUploadAvatarUnauthorizedUser()
+        {
+            using var stream = File.OpenRead(_filePath);
+            var file = new FormFile(stream, 0, stream.Length, "formFileName", FileName);
+            _avatarController.ControllerContext.HttpContext = PermissionServiceMock.UnauthorizedHttpContext();
 
-            _ = _avatarController.UploadAvatar(_jwtAuthenticatedUser.Id, file).Result;
+            var result = _avatarController.UploadAvatar(file).Result;
+            Assert.That(result, Is.InstanceOf<ForbidResult>());
+        }
+
+        [Test]
+        public void TestUploadAudioFileNullFile()
+        {
+            var result = _avatarController.UploadAvatar(null).Result;
+            Assert.That(result, Is.InstanceOf<BadRequestObjectResult>());
+        }
+
+        [Test]
+        public void TestUploadAudioFileEmptyFile()
+        {
+            using var stream = File.OpenRead(_filePath);
+            // Use 0 for the third argument to simulate an empty file.
+            var file = new FormFile(stream, 0, 0, "formFileName", FileName);
+
+            var result = _avatarController.UploadAvatar(file).Result;
+            Assert.That(result, Is.InstanceOf<BadRequestObjectResult>());
+        }
+
+        [Test]
+        public void TestUploadAvatarAndDownloadAvatar()
+        {
+            using var stream = File.OpenRead(_filePath);
+            var file = new FormFile(stream, 0, stream.Length, "formFileName", FileName);
+            var uploadResult = _avatarController.UploadAvatar(file).Result;
+            Assert.That(uploadResult, Is.TypeOf<OkResult>());
 
             var foundUser = _userRepo.GetUser(_jwtAuthenticatedUser.Id).Result;
             Assert.That(foundUser?.Avatar, Is.Not.Null);
 
+            // No permissions should be required to download an avatar.
+            _avatarController.ControllerContext.HttpContext = PermissionServiceMock.UnauthorizedHttpContext();
+
+            var fileResult = _avatarController.DownloadAvatar(_jwtAuthenticatedUser.Id).Result as FileStreamResult;
+            Assert.That(fileResult, Is.TypeOf<FileStreamResult>());
+
             // Clean up.
+            fileResult!.FileStream.Dispose();
             DeleteAvatarFile(_jwtAuthenticatedUser.Id);
         }
     }

Backend/Controllers/AvatarController.cs

@@ -25,37 +25,34 @@ public AvatarController(IUserRepository userRepo, IPermissionService permissionS
 
         /// <summary> Get user's avatar on disk </summary>
         /// <returns> Stream of local avatar file </returns>
+        [AllowAnonymous]
         [HttpGet("download", Name = "DownloadAvatar")]
         [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(FileContentResult))]
+        [ProducesResponseType(StatusCodes.Status404NotFound)]
         public async Task<IActionResult> DownloadAvatar(string userId)
         {
-            // SECURITY: Omitting authentication so the frontend can use the API endpoint directly as a URL.
-            // if (!await _permissionService.HasProjectPermission(HttpContext, Permission.WordEntry))
-            // {
-            //     return Forbid();
-            // }
-
-            var user = await _userRepo.GetUser(userId, false);
-            var avatar = string.IsNullOrEmpty(user?.Avatar) ? null : user.Avatar;
-
-            if (avatar is null)
+            var avatar = (await _userRepo.GetUser(userId, false))?.Avatar;
+            if (string.IsNullOrEmpty(avatar))
             {
-                return NotFound(userId);
+                return NotFound();
             }
 
             var imageFile = System.IO.File.OpenRead(avatar);
             return File(imageFile, "application/octet-stream");
         }
 
         /// <summary>
-        /// Adds an avatar image to a <see cref="User"/> and saves locally to ~/.CombineFiles/{ProjectId}/Avatars
+        /// Adds an avatar image to current <see cref="User"/> and saves locally to ~/.CombineFiles/{ProjectId}/Avatars
         /// </summary>
         /// <returns> Path to local avatar file </returns>
         [HttpPost("upload", Name = "UploadAvatar")]
         [ProducesResponseType(StatusCodes.Status200OK)]
-        public async Task<IActionResult> UploadAvatar(string userId, IFormFile? file)
+        [ProducesResponseType(StatusCodes.Status400BadRequest, Type = typeof(string))]
+        [ProducesResponseType(StatusCodes.Status403Forbidden)]
+        [ProducesResponseType(StatusCodes.Status404NotFound)]
+        public async Task<IActionResult> UploadAvatar(IFormFile? file)
         {
-            if (!_permissionService.IsUserIdAuthorized(HttpContext, userId))
+            if (!_permissionService.IsCurrentUserAuthorized(HttpContext))
             {
                 return Forbid();
             }
@@ -72,10 +69,11 @@ public async Task<IActionResult> UploadAvatar(string userId, IFormFile? file)
             }
 
             // Get user to apply avatar to.
+            var userId = _permissionService.GetUserId(HttpContext);
             var user = await _userRepo.GetUser(userId, false);
             if (user is null)
             {
-                return NotFound(userId);
+                return NotFound();
             }
 
             // Generate path to store avatar file.

src/backend/index.ts

@@ -169,11 +169,13 @@ export function getAudioUrl(wordId: string, fileName: string): string {
 
 /* AvatarController.cs */
 
-export async function uploadAvatar(userId: string, file: File): Promise<void> {
+/** Uploads avatar for current user. */
+export async function uploadAvatar(file: File): Promise<void> {
+  // Backend ignores userId and gets current user from HttpContext,
+  // but userId is still required in the url and helpful for analytics.
+  const userId = LocalStorage.getUserId();
   await avatarApi.uploadAvatar({ userId, file }, fileUploadOptions());
-  if (userId === LocalStorage.getUserId()) {
-    LocalStorage.setAvatar(await avatarSrc(userId));
-  }
+  LocalStorage.setAvatar(await avatarSrc(userId));
 }
 
 /** Returns the string to display the image inline in Base64 <img src= */

src/components/UserSettings/ClickableAvatar.tsx

@@ -3,7 +3,7 @@ import { Avatar } from "@mui/material";
 import { ReactElement, useState } from "react";
 
 import { uploadAvatar } from "backend";
-import { getAvatar, getUserId } from "backend/localStorage";
+import { getAvatar } from "backend/localStorage";
 import { UploadImageDialog } from "components/Dialogs";
 
 const avatarStyle = { height: 60, width: 60 };
@@ -53,7 +53,7 @@ export default function ClickableAvatar(
         close={closeDialog}
         open={avatarDialogOpen}
         titleId="userSettings.uploadAvatarTitle"
-        uploadImage={(imgFile: File) => uploadAvatar(getUserId(), imgFile)}
+        uploadImage={(imgFile: File) => uploadAvatar(imgFile)}
       />
     </>
   );