[Backend] Conform string handling & user getting (#2194)

Cover CA1305, CA1309, CA1310 of #2111; Fix GetUser default argument mismatch between UserRepository and IUserRepository; Add sanitize-user option to GetUserBy* functions; Remove usage of (string)someString.Clone()
sillsdev · Jun 5, 2023 · 9ca6066 · 9ca6066
1 parent 3092c92
commit 9ca6066
Show file tree

Hide file tree

Showing 32 changed files with 431 additions and 379 deletions.
diff --git a/.editorconfig b/.editorconfig
@@ -7,6 +7,8 @@ indent_size = 2
 [*.cs]
 max_line_length = 120
 indent_size = 4
+# CA1305 requires using a FormatProvider with int.Parse and string.Format.
+dotnet_diagnostic.CA1305.severity = none
 dotnet_diagnostic.CA1710.severity = warning
 # CA1816 is our only exception to <AnalysisMode>Minimum</AnalysisMode>.
 dotnet_diagnostic.CA1816.severity = none

diff --git a/Backend.Tests/Backend.Tests.csproj b/Backend.Tests/Backend.Tests.csproj
@@ -9,7 +9,7 @@
     <AnalysisMode>Minimum</AnalysisMode>
     <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
     <GenerateDocumentationFile>true</GenerateDocumentationFile>
-    <NoWarn>$(NoWarn);CA1816;CS1591</NoWarn>
+    <NoWarn>$(NoWarn);CA1305;CA1816;CS1591</NoWarn>
   </PropertyGroup>
   <ItemGroup>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.6.0" />

diff --git a/Backend.Tests/Controllers/LiftControllerTests.cs b/Backend.Tests/Controllers/LiftControllerTests.cs
@@ -270,7 +270,8 @@ public async Task TestDeletedWordsExportToLift()
             // Make sure we exported 2 live and one dead entry
             Assert.That(Regex.Matches(text, "<entry").Count, Is.EqualTo(3));
             // There is only one deleted word
-            Assert.That(text.IndexOf("dateDeleted"), Is.EqualTo(text.LastIndexOf("dateDeleted")));
+            Assert.That(text.IndexOf("dateDeleted", StringComparison.Ordinal),
+                Is.EqualTo(text.LastIndexOf("dateDeleted", StringComparison.Ordinal)));
 
             // Delete the export
             await _liftController.DeleteLiftFile(UserId);

diff --git a/Backend.Tests/Controllers/UserControllerTests.cs b/Backend.Tests/Controllers/UserControllerTests.cs
@@ -26,7 +26,12 @@ public void Setup()
 
         private static User RandomUser()
         {
-            var user = new User { Username = Util.RandString(10), Password = Util.RandString(10) };
+            var user = new User
+            {
+                Username = Util.RandString(10),
+                Password = Util.RandString(10),
+                Email = $"{Util.RandString(5)}@{Util.RandString(5)}.com",
+            };
             return user;
         }
 
@@ -147,7 +152,7 @@ public void TestDeleteUser()
         }
 
         [Test]
-        public void TestCheckUsername()
+        public void TestIsUsernameUnavailable()
         {
             var user1 = RandomUser();
             var user2 = RandomUser();
@@ -156,24 +161,24 @@ public void TestCheckUsername()
             _userRepo.Create(user1);
             _userRepo.Create(user2);
 
-            var result1 = (ObjectResult)_userController.CheckUsername(username1.ToLowerInvariant()).Result;
+            var result1 = (ObjectResult)_userController.IsUsernameUnavailable(username1.ToLowerInvariant()).Result;
             Assert.IsTrue((bool)result1.Value!);
 
-            var result2 = (ObjectResult)_userController.CheckUsername(username2.ToUpperInvariant()).Result;
+            var result2 = (ObjectResult)_userController.IsUsernameUnavailable(username2.ToUpperInvariant()).Result;
             Assert.IsTrue((bool)result2.Value!);
 
-            var result3 = (ObjectResult)_userController.CheckUsername(username1).Result;
+            var result3 = (ObjectResult)_userController.IsUsernameUnavailable(username1).Result;
             Assert.IsTrue((bool)result3.Value!);
 
-            var result4 = (ObjectResult)_userController.CheckUsername("NewUsername").Result;
+            var result4 = (ObjectResult)_userController.IsUsernameUnavailable("NewUsername").Result;
             Assert.IsFalse((bool)result4.Value!);
 
-            var result5 = (ObjectResult)_userController.CheckUsername("").Result;
+            var result5 = (ObjectResult)_userController.IsUsernameUnavailable("").Result;
             Assert.IsTrue((bool)result5.Value!);
         }
 
         [Test]
-        public void TestCheckEmail()
+        public void TestIsEmailUnavailable()
         {
             var user1 = RandomUser();
             var user2 = RandomUser();
@@ -182,19 +187,19 @@ public void TestCheckEmail()
             _userRepo.Create(user1);
             _userRepo.Create(user2);
 
-            var result1 = (ObjectResult)_userController.CheckEmail(email1.ToLowerInvariant()).Result;
+            var result1 = (ObjectResult)_userController.IsEmailUnavailable(email1.ToLowerInvariant()).Result;
             Assert.IsTrue((bool)result1.Value!);
 
-            var result2 = (ObjectResult)_userController.CheckEmail(email2.ToUpperInvariant()).Result;
+            var result2 = (ObjectResult)_userController.IsEmailUnavailable(email2.ToUpperInvariant()).Result;
             Assert.IsTrue((bool)result2.Value!);
 
-            var result3 = (ObjectResult)_userController.CheckEmail(email1).Result;
+            var result3 = (ObjectResult)_userController.IsEmailUnavailable(email1).Result;
             Assert.IsTrue((bool)result3.Value!);
 
-            var result4 = (ObjectResult)_userController.CheckEmail("NewEmail").Result;
+            var result4 = (ObjectResult)_userController.IsEmailUnavailable("new@e.mail").Result;
             Assert.IsFalse((bool)result4.Value!);
 
-            var result5 = (ObjectResult)_userController.CheckEmail("").Result;
+            var result5 = (ObjectResult)_userController.IsEmailUnavailable("").Result;
             Assert.IsTrue((bool)result5.Value!);
         }
     }

diff --git a/Backend.Tests/Helper/FileStorageTests.cs b/Backend.Tests/Helper/FileStorageTests.cs
@@ -1,5 +1,5 @@
 using System;
-using BackendFramework.Helper;
+using static BackendFramework.Helper.FileStorage;
 using NUnit.Framework;
 
 namespace Backend.Tests.Helper
@@ -9,32 +9,32 @@ public class FileStorageTests
         [Test]
         public void TestFileTypeExtension()
         {
-            Assert.AreEqual(FileStorage.FileTypeExtension(FileStorage.FileType.Audio), ".webm");
-            Assert.AreEqual(FileStorage.FileTypeExtension(FileStorage.FileType.Avatar), ".jpg");
-            Assert.Throws<NotImplementedException>(() => { FileStorage.FileTypeExtension((FileStorage.FileType)99); });
+            Assert.That(FileTypeExtension(FileType.Audio), Is.EqualTo(".webm"));
+            Assert.That(FileTypeExtension(FileType.Avatar), Is.EqualTo(".jpg"));
+            Assert.Throws<NotImplementedException>(() => { FileTypeExtension((FileType)99); });
         }
 
         [Test]
         public void TestFilePathIdSanitization()
         {
             const string invalidId = "@";
             const string validId = "a";
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateAudioFilePathForWord(invalidId, validId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateAudioFilePathForWord(validId, invalidId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateAudioFilePath(invalidId, "file.mp3"));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateAudioFileDirPath(invalidId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateImportExtractedLocationDirPath(invalidId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateLiftImportDirPath(invalidId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GenerateAvatarFilePath(invalidId));
-            Assert.Throws<FileStorage.InvalidIdException>(
-                () => FileStorage.GetProjectDir(invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateAudioFilePathForWord(invalidId, validId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateAudioFilePathForWord(validId, invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateAudioFilePath(invalidId, "file.mp3"));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateAudioFileDirPath(invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateImportExtractedLocationDirPath(invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateLiftImportDirPath(invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GenerateAvatarFilePath(invalidId));
+            Assert.Throws<InvalidIdException>(
+                () => GetProjectDir(invalidId));
         }
     }
 }
diff --git a/Backend.Tests/Helper/PasswordHashTests.cs b/Backend.Tests/Helper/PasswordHashTests.cs
@@ -1,4 +1,4 @@
-using BackendFramework.Helper;
+using static BackendFramework.Helper.PasswordHash;
 using NUnit.Framework;
 
 namespace Backend.Tests.Helper
@@ -10,26 +10,26 @@ public class PasswordHashTests
         [Test]
         public void HashPasswordValidRoundtrip()
         {
-            var hash = PasswordHash.HashPassword(Password);
+            var hash = HashPassword(Password);
             Assert.AreNotEqual(Password, hash);
-            Assert.That(PasswordHash.ValidatePassword(hash, Password));
+            Assert.That(ValidatePassword(hash, Password));
         }
 
         [Test]
         public void HashPasswordInvalidHashByteRoundtrip()
         {
-            var hash = PasswordHash.HashPassword(Password);
+            var hash = HashPassword(Password);
             // Change a single byte of the hash and validate that the hash fails.
             hash[0] ^= 0xff;
-            Assert.IsFalse(PasswordHash.ValidatePassword(hash, Password));
+            Assert.IsFalse(ValidatePassword(hash, Password));
         }
 
         [Test]
         public void HashPasswordInvalidPasswordCharacterRoundtrip()
         {
-            var hash = PasswordHash.HashPassword(Password);
+            var hash = HashPassword(Password);
             var mutatedPassword = $"Z{Password}";
-            Assert.IsFalse(PasswordHash.ValidatePassword(hash, mutatedPassword));
+            Assert.IsFalse(ValidatePassword(hash, mutatedPassword));
         }
     }
 }
diff --git a/Backend.Tests/Helper/SanitizationTests.cs b/Backend.Tests/Helper/SanitizationTests.cs
@@ -1,8 +1,7 @@
 using System.Collections.Generic;
+using static BackendFramework.Helper.Sanitization;
 using NUnit.Framework;
 
-using BackendFramework.Helper;
-
 namespace Backend.Tests.Helper
 {
     public class SanitizationTests
@@ -16,7 +15,7 @@ public class SanitizationTests
         [TestCaseSource(nameof(_validIds))]
         public void TestValidIds(string id)
         {
-            Assert.That(Sanitization.SanitizeId(id));
+            Assert.That(SanitizeId(id));
         }
 
         private static List<string> _invalidIds = new()
@@ -49,7 +48,7 @@ public void TestValidIds(string id)
         [TestCaseSource(nameof(_invalidIds))]
         public void TestInvalidIds(string id)
         {
-            Assert.False(Sanitization.SanitizeId(id));
+            Assert.False(SanitizeId(id));
         }
 
         private static List<string> _validFileNames = new()
@@ -69,7 +68,7 @@ public void TestInvalidIds(string id)
         [TestCaseSource(nameof(_validFileNames))]
         public void TestValidFileNames(string fileName)
         {
-            Assert.That(Sanitization.SanitizeFileName(fileName));
+            Assert.That(SanitizeFileName(fileName));
         }
 
         private static List<string> _invalidFileNames = new()
@@ -98,7 +97,7 @@ public void TestValidFileNames(string fileName)
         [TestCaseSource(nameof(_invalidFileNames))]
         public void TestInvalidFileNames(string fileName)
         {
-            Assert.False(Sanitization.SanitizeFileName(fileName));
+            Assert.False(SanitizeFileName(fileName));
         }
 
         private static List<List<string>> _namesUnfriendlyFriendly = new()
@@ -114,17 +113,17 @@ public void TestInvalidFileNames(string fileName)
         [TestCaseSource(nameof(_namesUnfriendlyFriendly))]
         public void TestMakeFriendlyForPath(List<string> nameName)
         {
-            Assert.That(Sanitization.MakeFriendlyForPath(nameName[0]), Is.EqualTo(nameName[1]));
+            Assert.That(MakeFriendlyForPath(nameName[0]), Is.EqualTo(nameName[1]));
         }
 
         [Test]
         public void TestMakeFriendlyForPathFallback()
         {
             const string fallback = "Lift";
             const string nonEmpty = "qwerty";
-            Assert.That(Sanitization.MakeFriendlyForPath(""), Is.EqualTo(""));
-            Assert.That(Sanitization.MakeFriendlyForPath("", fallback), Is.EqualTo(fallback));
-            Assert.That(Sanitization.MakeFriendlyForPath(nonEmpty, fallback), Is.EqualTo(nonEmpty));
+            Assert.That(MakeFriendlyForPath(""), Is.EqualTo(""));
+            Assert.That(MakeFriendlyForPath("", fallback), Is.EqualTo(fallback));
+            Assert.That(MakeFriendlyForPath(nonEmpty, fallback), Is.EqualTo(nonEmpty));
         }
     }
 }
diff --git a/Backend.Tests/Helper/TimeTests.cs b/Backend.Tests/Helper/TimeTests.cs
@@ -1,6 +1,6 @@
 using System;
+using static BackendFramework.Helper.Time;
 using NUnit.Framework;
-using BackendFramework.Helper;
 
 namespace Backend.Tests.Helper
 {
@@ -9,16 +9,13 @@ public class TimeTests
         [Test]
         public void TestToUtcIso8601()
         {
-            Assert.AreEqual(
-                Time.ToUtcIso8601(new DateTime(2020, 1, 1)),
-                "2020-01-01T00:00:00.0000000"
-            );
+            Assert.That(ToUtcIso8601(new DateTime(2020, 1, 1)), Is.EqualTo("2020-01-01T00:00:00.0000000"));
         }
 
         [Test]
         public void TestUtcNowIso8601()
         {
-            var time = Time.UtcNowIso8601();
+            var time = UtcNowIso8601();
             Assert.That(time.Contains('T'));
             Assert.That(time.EndsWith('Z'));
             Assert.That(DateTime.Now, Is.EqualTo(DateTime.Parse(time)).Within(TimeSpan.FromSeconds(10)));

diff --git a/Backend.Tests/Mocks/UserRepositoryMock.cs b/Backend.Tests/Mocks/UserRepositoryMock.cs
@@ -55,13 +55,21 @@ public Task<bool> Delete(string id)
             return Task.FromResult(success);
         }
 
-        public Task<User?> GetUserByEmail(string email)
+        public Task<User?> GetUserByEmail(string email, bool sanitize = true)
         {
             var user = _users.Find(u => u.Email.ToLowerInvariant() == email.ToLowerInvariant());
             return Task.FromResult(user);
         }
 
-        public Task<User?> GetUserByUsername(string username)
+        public Task<User?> GetUserByEmailOrUsername(string emailOrUsername, bool sanitize = true)
+        {
+            var user = _users.Find(u =>
+                u.Email.ToLowerInvariant() == emailOrUsername.ToLowerInvariant() ||
+                u.Username.ToLowerInvariant() == emailOrUsername.ToLowerInvariant());
+            return Task.FromResult(user);
+        }
+
+        public Task<User?> GetUserByUsername(string username, bool sanitize = true)
         {
             var user = _users.Find(u => u.Username.ToLowerInvariant() == username.ToLowerInvariant());
             return Task.FromResult(user);

diff --git a/Backend/BackendFramework.csproj b/Backend/BackendFramework.csproj
@@ -7,7 +7,7 @@
     <AnalysisMode>Minimum</AnalysisMode>
     <EnforceCodeStyleInBuild>true</EnforceCodeStyleInBuild>
     <GenerateDocumentationFile>true</GenerateDocumentationFile>
-    <NoWarn>$(NoWarn);CA1816;CS1591</NoWarn>
+    <NoWarn>$(NoWarn);CA1305;CA1816;CS1591</NoWarn>
   </PropertyGroup>
   <ItemGroup>
     <None Remove="Data\sdList.txt" />

diff --git a/Backend/Controllers/AvatarController.cs b/Backend/Controllers/AvatarController.cs
@@ -35,7 +35,7 @@ public async Task<IActionResult> DownloadAvatar(string userId)
             //     return Forbid();
             // }
 
-            var user = await _userRepo.GetUser(userId);
+            var user = await _userRepo.GetUser(userId, false);
             var avatar = string.IsNullOrEmpty(user?.Avatar) ? null : user.Avatar;
 
             if (avatar is null)
@@ -73,7 +73,7 @@ public async Task<IActionResult> UploadAvatar(string userId, [FromForm] FileUplo
             }
 
             // Get user to apply avatar to.
-            var user = await _userRepo.GetUser(userId);
+            var user = await _userRepo.GetUser(userId, false);
             if (user is null)
             {
                 return NotFound(userId);

diff --git a/Backend/Controllers/ProjectController.cs b/Backend/Controllers/ProjectController.cs
@@ -103,7 +103,7 @@ public async Task<IActionResult> CreateProject([FromBody, BindRequired] Project
 
             // Get user.
             var currentUserId = _permissionService.GetUserId(HttpContext);
-            var currentUser = await _userRepo.GetUser(currentUserId);
+            var currentUser = await _userRepo.GetUser(currentUserId, false);
             if (currentUser is null)
             {
                 return NotFound(currentUserId);