From 072e63e98fb574d2dec3aecbbd18e01b0bc2f1da Mon Sep 17 00:00:00 2001 From: Leendert de Borst Date: Fri, 30 Aug 2024 21:36:16 +0200 Subject: [PATCH] Refactor and cleanup (#80) --- .../Controllers/AuthController.cs | 2 +- .../Security/TwoFactorAuthController.cs | 8 +-- .../Controllers/Tests/TestController.cs | 2 +- .../Components/RecentAuthLogsSection.razor | 26 --------- .../Configuration/DatabaseConfiguration.cs | 2 +- ...21164021_AddDataProtectionDatabaseTable.cs | 3 +- .../20240821204113_UpdateLogTable.cs | 3 +- .../20240830190840_AddAuthLogTable.cs | 3 +- .../Tests/Client/AuthTests.cs | 2 + .../AuthLoggingService.cs | 55 ++++++++----------- 10 files changed, 39 insertions(+), 67 deletions(-) diff --git a/src/AliasVault.Api/Controllers/AuthController.cs b/src/AliasVault.Api/Controllers/AuthController.cs index bacca317a..9fe97dba0 100644 --- a/src/AliasVault.Api/Controllers/AuthController.cs +++ b/src/AliasVault.Api/Controllers/AuthController.cs @@ -303,7 +303,7 @@ public class AuthController(IDbContextFactory dbContextFac public async Task Register([FromBody] SrpSignup model) { // Validate username, disallow "admin" as a username. - if (model.Username.ToLower() == "admin") + if (string.Equals(model.Username, "admin", StringComparison.OrdinalIgnoreCase)) { return BadRequest(ServerValidationErrorResponse.Create(["Username 'admin' is not allowed."], 400)); } diff --git a/src/AliasVault.Api/Controllers/Security/TwoFactorAuthController.cs b/src/AliasVault.Api/Controllers/Security/TwoFactorAuthController.cs index 41bb3574b..fd2b9fda8 100644 --- a/src/AliasVault.Api/Controllers/Security/TwoFactorAuthController.cs +++ b/src/AliasVault.Api/Controllers/Security/TwoFactorAuthController.cs @@ -39,7 +39,7 @@ public class TwoFactorAuthController(IDbContextFactory dbC var user = await GetCurrentUserAsync(); if (user is null) { - return Unauthorized("Not authenticated."); + return Unauthorized(); } var twoFactorEnabled = await GetUserManager().GetTwoFactorEnabledAsync(user); @@ -56,7 +56,7 @@ public class TwoFactorAuthController(IDbContextFactory dbC var user = await GetCurrentUserAsync(); if (user is null) { - return Unauthorized("Not authenticated."); + return Unauthorized(); } var authenticatorKey = await GetUserManager().GetAuthenticatorKeyAsync(user); @@ -83,7 +83,7 @@ public class TwoFactorAuthController(IDbContextFactory dbC var user = await GetCurrentUserAsync(); if (user is null) { - return Unauthorized("Not authenticated."); + return Unauthorized(); } var isValid = await GetUserManager().VerifyTwoFactorTokenAsync(user, GetUserManager().Options.Tokens.AuthenticatorTokenProvider, code); @@ -113,7 +113,7 @@ public class TwoFactorAuthController(IDbContextFactory dbC var user = await GetCurrentUserAsync(); if (user is null) { - return Unauthorized("Not authenticated."); + return Unauthorized(); } await using var context = await dbContextFactory.CreateDbContextAsync(); diff --git a/src/AliasVault.Api/Controllers/Tests/TestController.cs b/src/AliasVault.Api/Controllers/Tests/TestController.cs index cc3683c86..8f396bc6c 100644 --- a/src/AliasVault.Api/Controllers/Tests/TestController.cs +++ b/src/AliasVault.Api/Controllers/Tests/TestController.cs @@ -46,6 +46,6 @@ public class TestController(UserManager userManager) : Authentic public IActionResult TestCallError() { // Throw an exception here to test error handling. - throw new ApplicationException("Test error"); + throw new ArgumentException("Test error"); } } diff --git a/src/AliasVault.Client/Main/Pages/Settings/Security/Components/RecentAuthLogsSection.razor b/src/AliasVault.Client/Main/Pages/Settings/Security/Components/RecentAuthLogsSection.razor index 9f3eb96a1..d09887c04 100644 --- a/src/AliasVault.Client/Main/Pages/Settings/Security/Components/RecentAuthLogsSection.razor +++ b/src/AliasVault.Client/Main/Pages/Settings/Security/Components/RecentAuthLogsSection.razor @@ -83,30 +83,4 @@ IsLoading = false; StateHasChanged(); } - - /// - /// Revokes a specific session (refresh token) for the current user. - /// - /// The unique identifier of the session to revoke. - /// A task representing the asynchronous operation. - private async Task RevokeSession(Guid id) - { - try - { - var response = await Http.DeleteAsync($"api/v1/Security/sessions/{id}"); - if (response.IsSuccessStatusCode) - { - GlobalNotificationService.AddSuccessMessage("Session revoked successfully.", true); - await OnSessionsChanged.InvokeAsync(); - } - else - { - GlobalNotificationService.AddErrorMessage("Failed to revoke session.", true); - } - } - catch (Exception ex) - { - GlobalNotificationService.AddErrorMessage($"Failed to revoke session: {ex.Message}.", true); - } - } } diff --git a/src/Databases/AliasServerDb/Configuration/DatabaseConfiguration.cs b/src/Databases/AliasServerDb/Configuration/DatabaseConfiguration.cs index 66bfd2def..448a91e68 100644 --- a/src/Databases/AliasServerDb/Configuration/DatabaseConfiguration.cs +++ b/src/Databases/AliasServerDb/Configuration/DatabaseConfiguration.cs @@ -51,7 +51,7 @@ public static class DatabaseConfiguration return services; } - private static DbConnection CreateAndConfigureSqliteConnection(string connectionString) + private static SqliteConnection CreateAndConfigureSqliteConnection(string connectionString) { var connection = new SqliteConnection(connectionString); connection.Open(); diff --git a/src/Databases/AliasServerDb/Migrations/20240821164021_AddDataProtectionDatabaseTable.cs b/src/Databases/AliasServerDb/Migrations/20240821164021_AddDataProtectionDatabaseTable.cs index e180259b8..d19207b22 100644 --- a/src/Databases/AliasServerDb/Migrations/20240821164021_AddDataProtectionDatabaseTable.cs +++ b/src/Databases/AliasServerDb/Migrations/20240821164021_AddDataProtectionDatabaseTable.cs @@ -1,4 +1,5 @@ -using Microsoft.EntityFrameworkCore.Migrations; +// +using Microsoft.EntityFrameworkCore.Migrations; #nullable disable diff --git a/src/Databases/AliasServerDb/Migrations/20240821204113_UpdateLogTable.cs b/src/Databases/AliasServerDb/Migrations/20240821204113_UpdateLogTable.cs index 5d4212e54..ebe329109 100644 --- a/src/Databases/AliasServerDb/Migrations/20240821204113_UpdateLogTable.cs +++ b/src/Databases/AliasServerDb/Migrations/20240821204113_UpdateLogTable.cs @@ -1,4 +1,5 @@ -using Microsoft.EntityFrameworkCore.Migrations; +// +using Microsoft.EntityFrameworkCore.Migrations; #nullable disable diff --git a/src/Databases/AliasServerDb/Migrations/20240830190840_AddAuthLogTable.cs b/src/Databases/AliasServerDb/Migrations/20240830190840_AddAuthLogTable.cs index 9a5357b61..fe400de44 100644 --- a/src/Databases/AliasServerDb/Migrations/20240830190840_AddAuthLogTable.cs +++ b/src/Databases/AliasServerDb/Migrations/20240830190840_AddAuthLogTable.cs @@ -1,4 +1,5 @@ -using System; +// +using System; using Microsoft.EntityFrameworkCore.Migrations; #nullable disable diff --git a/src/Tests/AliasVault.E2ETests/Tests/Client/AuthTests.cs b/src/Tests/AliasVault.E2ETests/Tests/Client/AuthTests.cs index baf1720a1..841bc6ffc 100644 --- a/src/Tests/AliasVault.E2ETests/Tests/Client/AuthTests.cs +++ b/src/Tests/AliasVault.E2ETests/Tests/Client/AuthTests.cs @@ -108,6 +108,8 @@ public class AuthTests : ClientPlaywrightTest // Wait for account lockout message. await WaitForUrlAsync("user/login**", "locked out"); + var pageContent = await Page.TextContentAsync("body"); + Assert.That(pageContent, Does.Contain("locked out"), "No account lockout message."); } /// diff --git a/src/Utilities/AliasVault.AuthLogging/AuthLoggingService.cs b/src/Utilities/AliasVault.AuthLogging/AuthLoggingService.cs index b113fe04a..c1fe881a1 100644 --- a/src/Utilities/AliasVault.AuthLogging/AuthLoggingService.cs +++ b/src/Utilities/AliasVault.AuthLogging/AuthLoggingService.cs @@ -44,11 +44,11 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc DeviceType = DetermineDeviceType(httpContext), OperatingSystem = DetermineOperatingSystem(httpContext), Browser = DetermineBrowser(httpContext), - Country = DetermineCountry(httpContext), - IsSuspiciousActivity = DetermineSuspiciousActivity(), + Country = DetermineCountry(), + IsSuspiciousActivity = false, }; - dbContext.AuthLogs.Add(authAttempt); + await dbContext.AuthLogs.AddAsync(authAttempt); await dbContext.SaveChangesAsync(); } @@ -78,11 +78,11 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc DeviceType = DetermineDeviceType(httpContext), OperatingSystem = DetermineOperatingSystem(httpContext), Browser = DetermineBrowser(httpContext), - Country = DetermineCountry(httpContext), - IsSuspiciousActivity = DetermineSuspiciousActivity(), + Country = DetermineCountry(), + IsSuspiciousActivity = false, }; - dbContext.AuthLogs.Add(authAttempt); + await dbContext.AuthLogs.AddAsync(authAttempt); await dbContext.SaveChangesAsync(); } @@ -91,11 +91,14 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc /// /// The HttpContext containing the request information. /// A string representing the device type: "Mobile", "Tablet", "Smart TV", "Desktop", or "Unknown". - private string DetermineDeviceType(HttpContext? context) + private static string? DetermineDeviceType(HttpContext? context) { - if (context == null) return "Unknown"; + if (context is null) + { + return null; + } - return context.Request.Headers["User-Agent"].ToString().ToLower() switch + return context.Request.Headers.UserAgent.ToString().ToLower() switch { var ua when ua.Contains("mobile") || ua.Contains("android") || ua.Contains("iphone") => "Mobile", var ua when ua.Contains("tablet") || ua.Contains("ipad") => "Tablet", @@ -109,11 +112,11 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc /// /// The HttpContext containing the request information. /// A string representing the operating system: "Windows", "MacOS", "Linux", "Android", "iOS", or "Unknown". - private string DetermineOperatingSystem(HttpContext? context) + private static string? DetermineOperatingSystem(HttpContext? context) { if (context is null) { - return "Unknown"; + return null; } return context.Request.Headers.UserAgent.ToString().ToLower() switch @@ -123,7 +126,7 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc var ua when ua.Contains("linux") => "Linux", var ua when ua.Contains("android") => "Android", var ua when ua.Contains("iphone") || ua.Contains("ipad") => "iOS", - _ => "Unknown", + _ => null, }; } @@ -132,11 +135,11 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc /// /// The HttpContext containing the request information. /// A string representing the browser: "Firefox", "Chrome", "Safari", "Edge", "Opera", or "Unknown". - private string DetermineBrowser(HttpContext? context) + private static string? DetermineBrowser(HttpContext? context) { if (context is null) { - return "Unknown"; + return null; } return context.Request.Headers.UserAgent.ToString().ToLower() switch @@ -146,38 +149,29 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc var ua when ua.Contains("safari") && !ua.Contains("chrome") => "Safari", var ua when ua.Contains("edg") => "Edge", var ua when ua.Contains("opr") || ua.Contains("opera") => "Opera", - _ => "Unknown" + _ => null }; } /// /// Determines the country based on the IP address of the request. /// - /// The HttpContext containing the request information. /// A string representing the country or "Unknown" if the country cannot be determined. /// - /// This method currently returns a placeholder value. In a production environment, - /// it should be implemented using a Geo-IP database or service for accurate results. + /// This method currently returns null as the implementation is not yet complete. /// - private string DetermineCountry(HttpContext? context) + private static string? DetermineCountry() { // Implement later by using a Geo-IP database or service. - return "Unknown"; + return null; } - /// - /// Logic to determine if the activity is suspicious. For the moment it always returns false. - /// Needs to be implemented later. - /// - /// - private bool DetermineSuspiciousActivity() => false; - /// /// Extract IP address from HttpContext. /// /// HttpContext to extract the IP address from. /// - private string GetIpFromContext(HttpContext? httpContext) + private static string GetIpFromContext(HttpContext? httpContext) { string ipAddress = ""; @@ -189,13 +183,12 @@ public class AuthLoggingService(IServiceProvider serviceProvider, IHttpContextAc if (string.IsNullOrEmpty(ipAddress)) { // Check if X-Forwarded-For header exists, if so, extract first IP address from comma separated list. - if (httpContext.Request.Headers.ContainsKey("X-Forwarded-For")) + if (httpContext.Request.Headers.TryGetValue("X-Forwarded-For", out var xForwardedFor)) { - ipAddress = httpContext.Request.Headers["X-Forwarded-For"].ToString().Split(',')[0]; + ipAddress = xForwardedFor.ToString().Split(',')[0]; } else { - // Otherwise use RemoteIpAddress. ipAddress = httpContext.Connection.RemoteIpAddress?.ToString() ?? "0.0.0.0"; } }