Merge pull request #1659 from qdraw/feature/202408-code-smells-20

fix code smells
qdraw · Aug 21, 2024 · ad206c4 · ad206c4
2 parents 65e6f14 + 4da55e3
commit ad206c4
Show file tree

Hide file tree

Showing 10 changed files with 125 additions and 29 deletions.
diff --git a/starsky/build/helpers/SonarQube.cs b/starsky/build/helpers/SonarQube.cs
@@ -28,7 +28,7 @@ public static class SonarQube
 		/// <summary>
 		/// @see: https://www.nuget.org/packages/dotnet-sonarscanner
 		/// </summary>
-		public const string SonarQubePackageVersion = "6.2.0";
+		public const string SonarQubePackageVersion = "8.0.0";
 
 		public const string SonarQubeDotnetSonarScannerApi =
 			"https://api.nuget.org/v3-flatcontainer/dotnet-sonarscanner/index.json";

diff --git a/starsky/starsky.feature.webhtmlpublish/starsky.feature.webhtmlpublish.csproj b/starsky/starsky.feature.webhtmlpublish/starsky.feature.webhtmlpublish.csproj
@@ -21,7 +21,7 @@
     <ItemGroup>
         <PackageReference Include="RazorLight" Version="2.3.1"/>
         <PackageReference Include="SixLabors.ImageSharp" Version="3.1.5" />
-        <PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.3" />
+        <PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.4" />
     </ItemGroup>
 
     <ItemGroup>

diff --git a/starsky/starsky.foundation.accountmanagement/Helpers/Pbkdf2Hasher.cs b/starsky/starsky.foundation.accountmanagement/Helpers/Pbkdf2Hasher.cs
@@ -22,7 +22,7 @@ public static string ComputeHash(string password, byte[] salt)
 					password: password,
 					salt: salt,
 					prf: KeyDerivationPrf.HMACSHA1,
-					iterationCount: 10000,
+					iterationCount: 100_000,
 					numBytesRequested: 256 / 8
 				)
 			);

diff --git a/starsky/starsky.foundation.platform/starsky.foundation.platform.csproj b/starsky/starsky.foundation.platform/starsky.foundation.platform.csproj
@@ -25,7 +25,7 @@
         <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="8.0.1" />
         <PackageReference Include="System.Buffers" Version="4.5.1"/>
         <PackageReference Include="System.ComponentModel.Annotations" Version="5.0.0"/>
-        <PackageReference Include="System.Threading.Tasks.Dataflow" Version="8.0.0"/>
+        <PackageReference Include="System.Threading.Tasks.Dataflow" Version="8.0.1" />
         <PackageReference Include="TimeZoneConverter" Version="6.1.0"/>
     </ItemGroup>
 

diff --git a/starsky/starsky.foundation.thumbnailgeneration/starsky.foundation.thumbnailgeneration.csproj b/starsky/starsky.foundation.thumbnailgeneration/starsky.foundation.thumbnailgeneration.csproj
@@ -19,7 +19,7 @@
 
     <ItemGroup>
         <PackageReference Include="SixLabors.ImageSharp" Version="3.1.5" />
-        <PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.3" />
+        <PackageReference Include="SixLabors.ImageSharp.Drawing" Version="2.1.4" />
     </ItemGroup>
 
     <PropertyGroup Condition=" '$(noSonar)' == 'true' ">

diff --git a/starsky/starsky.foundation.webtelemetry/starsky.foundation.webtelemetry.csproj b/starsky/starsky.foundation.webtelemetry/starsky.foundation.webtelemetry.csproj
@@ -25,13 +25,13 @@
         <PackageReference Include="Microsoft.Extensions.Logging" Version="8.0.0" />
         <!--      instead of : Microsoft.AspNetCore.Http.Abstractions" Version="2.2.0-->
         <PackageReference Include="Microsoft.Extensions.Logging.Console" Version="8.0.0" />
-        <PackageReference Include="OpenTelemetry" Version="1.8.0"/>
-        <PackageReference Include="OpenTelemetry.Api" Version="1.8.0"/>
-        <PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.8.0"/>
-        <PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.8.0"/>
-        <PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.8.1"/>
+        <PackageReference Include="OpenTelemetry" Version="1.9.0"/>
+        <PackageReference Include="OpenTelemetry.Api" Version="1.9.0"/>
+        <PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.9.0"/>
+        <PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.9.0"/>
+        <PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.9.0"/>
         <PackageReference Include="OpenTelemetry.Instrumentation.EntityFrameworkCore" Version="1.0.0-beta.11" />
-        <PackageReference Include="OpenTelemetry.Instrumentation.Runtime" Version="1.8.0" />
+        <PackageReference Include="OpenTelemetry.Instrumentation.Runtime" Version="1.9.0" />
         <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="8.0.1" />
         <PackageReference Include="System.Text.Json" Version="8.0.4" />
     </ItemGroup>

diff --git a/starsky/starsky/Controllers/AccountController.cs b/starsky/starsky/Controllers/AccountController.cs
@@ -7,7 +7,6 @@
 using Microsoft.AspNetCore.Authorization;
 using Microsoft.AspNetCore.Mvc;
 using starsky.foundation.accountmanagement.Interfaces;
-using starsky.foundation.accountmanagement.Models;
 using starsky.foundation.accountmanagement.Models.Account;
 using starsky.foundation.platform.Models;
 using starsky.foundation.storage.Interfaces;
@@ -23,6 +22,8 @@ public sealed class AccountController : Controller
 		private readonly IAntiforgery _antiForgery;
 		private readonly IStorage _storageHostFullPathFilesystem;
 
+		private const string ModelError = "Model is not correct";
+
 		public AccountController(IUserManager userManager, AppSettings appSettings,
 			IAntiforgery antiForgery, ISelectorStorage selectorStorage)
 		{
@@ -34,7 +35,7 @@ public AccountController(IUserManager userManager, AppSettings appSettings,
 		}
 
 		/// <summary>
-		/// Check the account status of the current logged in user
+		/// Check the account status of the current logged-in user
 		/// </summary>
 		/// <response code="200">Logged in</response>
 		/// <response code="401">When not logged in</response>
@@ -113,6 +114,8 @@ public async Task<IActionResult> Status()
 		[AllowAnonymous]
 		public IActionResult LoginGet(string? returnUrl = null, bool? fromLogout = null)
 		{
+			if ( !ModelState.IsValid ) return BadRequest(ModelError);
+
 			new AntiForgeryCookie(_antiForgery).SetAntiForgeryCookie(HttpContext);
 			var clientApp = Path.Combine(_appSettings.BaseDirectoryProject,
 				"clientapp", "build", "index.html");
@@ -143,7 +146,9 @@ public IActionResult LoginGet(string? returnUrl = null, bool? fromLogout = null)
 		[AllowAnonymous]
 		public async Task<IActionResult> LoginPost(LoginViewModel model)
 		{
-			ValidateResult validateResult =
+			if ( !ModelState.IsValid ) return BadRequest(ModelError);
+
+			var validateResult =
 				await _userManager.ValidateAsync("Email", model.Email, model.Password);
 
 			if ( !validateResult.Success )
@@ -192,6 +197,8 @@ public IActionResult LogoutJson()
 		[AllowAnonymous]
 		public IActionResult Logout(string? returnUrl = null)
 		{
+			if ( !ModelState.IsValid ) return BadRequest(ModelError);
+
 			_userManager.SignOut(HttpContext);
 			// fromLogout is used in middleware
 			return RedirectToAction(nameof(LoginGet),
@@ -205,7 +212,7 @@ public IActionResult Logout(string? returnUrl = null)
 		/// <returns>Change secret status</returns>
 		/// <response code="200">successful login</response>
 		/// <response code="400">Model is not correct</response>
-		/// <response code="401"> please login first or your current password is not correct</response>
+		/// <response code="401">please log in first or your current password is not correct</response>
 		[HttpPost("/api/account/change-secret")]
 		[ProducesResponseType(typeof(string), 200)]
 		[ProducesResponseType(typeof(string), 400)]
@@ -222,7 +229,7 @@ public async Task<IActionResult> ChangeSecret(ChangePasswordViewModel model)
 			if ( !ModelState.IsValid ||
 			     model.ChangedPassword != model.ChangedConfirmPassword )
 			{
-				return BadRequest("Model is not correct");
+				return BadRequest(ModelError);
 			}
 
 			var currentUserId = _userManager.GetCurrentUser(HttpContext)?.Id;
@@ -280,7 +287,7 @@ public async Task<IActionResult> Register(RegisterViewModel model)
 
 			// If we got this far, something failed, redisplay form
 			Response.StatusCode = 400;
-			return Json("Model is not correct");
+			return Json(ModelError);
 		}
 
 		/// <summary>

diff --git a/starsky/starsky/starsky.csproj b/starsky/starsky/starsky.csproj
@@ -61,7 +61,7 @@
     <ItemGroup>
         <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="8.0.8" />
         <PackageReference Include="Microsoft.Extensions.Hosting.WindowsServices" Version="8.0.0" />
-        <PackageReference Include="Swashbuckle.AspNetCore" Version="6.5.0"/>
+        <PackageReference Include="Swashbuckle.AspNetCore" Version="6.7.1" />
     </ItemGroup>
     <!-- generate xml file for swagger -->
     <PropertyGroup>

diff --git a/starsky/starskytest/Controllers/AccountControllerTest.cs b/starsky/starskytest/Controllers/AccountControllerTest.cs
@@ -116,7 +116,7 @@ private static ClaimsPrincipal SetTestClaimsSet(string name, string id)
 		public async Task AccountController_NoLogin_Login_And_newAccount_Test()
 		{
 			// Arrange
-			var userId = "TestUserA";
+			const string userId = "TestUserA";
 			var claims = new List<Claim> { new Claim(ClaimTypes.NameIdentifier, userId) };
 
 			var httpContext = _serviceProvider.GetRequiredService<IHttpContextAccessor>()
@@ -168,8 +168,8 @@ public async Task AccountController_NoLogin_Login_And_newAccount_Test()
 			// The logout is mocked so this will not actual log it out and controller.Logout() not crashing is good enough
 			controller.Logout();
 
-			// And clean afterwards
-			var itemWithId = _dbContext.Users.FirstOrDefault(p => p.Name == newAccount.Name);
+			// And clean afterward
+			var itemWithId = await _dbContext.Users.FirstOrDefaultAsync(p => p.Name == newAccount.Name);
 			Assert.IsNotNull(itemWithId);
 			_dbContext.Users.Remove(itemWithId);
 			await _dbContext.SaveChangesAsync();
@@ -467,7 +467,7 @@ public async Task AccountController_LoginContext_GetRegisterPage_AccountCreated(
 			Assert.IsNotNull(actionResult);
 			Assert.AreEqual("Account Created", actionResult.Value);
 
-			var getUser = _dbContext.Users.FirstOrDefault(p => p.Name == user.Name);
+			var getUser = await _dbContext.Users.FirstOrDefaultAsync(p => p.Name == user.Name);
 			Assert.IsNotNull(getUser);
 			_dbContext.Users.Remove(getUser);
 			await _dbContext.SaveChangesAsync();
@@ -515,8 +515,8 @@ public async Task AccountController_IndexGetLoginSuccessful()
 			await controller.Status();
 			Assert.AreEqual(200, controller.Response.StatusCode);
 
-			// And clean afterwards
-			var getUser = _dbContext.Users.FirstOrDefault(p => p.Name == user.Name);
+			// And clean afterward
+			var getUser = await _dbContext.Users.FirstOrDefaultAsync(p => p.Name == user.Name);
 			Assert.IsNotNull(getUser);
 			_dbContext.Users.Remove(getUser);
 			await _dbContext.SaveChangesAsync();
@@ -629,7 +629,6 @@ public async Task AccountController_newAccount_TryToOverwrite_ButItKeepsTheSameP
 			// controller.Logout() not crashing is good enough
 			controller.LogoutJson();
 
-
 			var newAccountDuplicate = new RegisterViewModel
 			{
 				Password = "test11234567890", // DIFFERENT
@@ -650,7 +649,7 @@ public async Task AccountController_newAccount_TryToOverwrite_ButItKeepsTheSameP
 			controller.LogoutJson();
 
 			// Clean afterwards            
-			var user = _dbContext.Users.FirstOrDefault(p => p.Name == userId);
+			var user = await _dbContext.Users.FirstOrDefaultAsync(p => p.Name == userId);
 			Assert.IsNotNull(user);
 			_dbContext.Users.Remove(user);
 			await _dbContext.SaveChangesAsync();
@@ -821,5 +820,95 @@ public void Permissions()
 			Assert.IsNotNull(expectedPermission);
 			Assert.AreEqual(expectedPermission, list.FirstOrDefault());
 		}
+
+		[TestMethod]
+		public void Logout_ModelStateIsInvalid_ReturnsBadRequest()
+		{
+			// Arrange
+			var controller =
+				new AccountController(_userManager, _appSettings, _antiForgery, _selectorStorage)
+				{
+					ControllerContext =
+					{
+						HttpContext = new DefaultHttpContext()
+					}
+				};
+			controller.ModelState.AddModelError("Key", "ErrorMessage");
+
+			// Act
+			var result = controller.Logout();
+
+			// Assert
+			Assert.IsInstanceOfType<BadRequestObjectResult>(result);
+		}
+
+		[TestMethod]
+		public void LoginGet_ModelStateIsInvalid_ReturnsBadRequest()
+		{
+			// Arrange
+			var controller =
+				new AccountController(_userManager, _appSettings, _antiForgery, _selectorStorage)
+				{
+					ControllerContext =
+					{
+						HttpContext = new DefaultHttpContext()
+					}
+				};
+			controller.ModelState.AddModelError("Key", "ErrorMessage");
+
+			// Act
+			var result = controller.LoginGet();
+
+			// Assert
+			Assert.IsInstanceOfType<BadRequestObjectResult>(result);
+		}
+
+		[TestMethod]
+		public async Task ChangeSecret_ModelStateIsInvalid_ReturnsBadRequest()
+		{
+			// Arrange
+			var controller =
+				new AccountController(_userManager, _appSettings, _antiForgery, _selectorStorage)
+				{
+					ControllerContext =
+					{
+						HttpContext = new DefaultHttpContext
+						{
+							User = SetTestClaimsSet("test", "1")
+						}
+					}
+				};
+			controller.ModelState.AddModelError("Key", "ErrorMessage");
+
+			// Act
+			var result = await controller.ChangeSecret(new ChangePasswordViewModel());
+
+			// Assert
+			Assert.IsInstanceOfType<BadRequestObjectResult>(result);
+		}
+
+		[TestMethod]
+		public async Task LoginPost_ModelStateIsInvalid_ReturnsBadRequest()
+		{
+			// Arrange
+			var controller =
+				new AccountController(_userManager, _appSettings, _antiForgery, _selectorStorage)
+				{
+					ControllerContext =
+					{
+						HttpContext = new DefaultHttpContext
+						{
+							User = SetTestClaimsSet("test", "1")
+						}
+					}
+				};
+			controller.ModelState.AddModelError("Key", "ErrorMessage");
+
+			// Act
+			var result = await controller.LoginPost(new LoginViewModel());
+
+			// Assert
+			Assert.IsInstanceOfType<BadRequestObjectResult>(result);
+		}
 	}
 }
diff --git a/starsky/starskytest/starskytest.csproj b/starsky/starskytest/starskytest.csproj
@@ -25,9 +25,9 @@
         <PackageReference Include="Microsoft.EntityFrameworkCore" Version="8.0.8" />
         <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
         <PackageReference Include="Microsoft.Extensions.Hosting.WindowsServices" Version="8.0.0" />
-        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.9.0"/>
-        <PackageReference Include="MSTest.TestAdapter" Version="3.3.1"/>
-        <PackageReference Include="MSTest.TestFramework" Version="3.3.1"/>
+        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.0" />
+        <PackageReference Include="MSTest.TestAdapter" Version="3.5.2" />
+        <PackageReference Include="MSTest.TestFramework" Version="3.5.2" />
         <PackageReference Include="coverlet.collector" Version="6.0.2">
             <PrivateAssets>all</PrivateAssets>
             <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>