From a512004402103009d468a8a130ab526abede15db Mon Sep 17 00:00:00 2001 From: jarvis Date: Mon, 26 Jan 2026 01:47:10 +0000 Subject: [PATCH] refactor: reduce cyclomatic complexity in Identity module - Extract helper methods in UserService.Lifecycle.cs - Simplify SearchUsersQueryHandler with extracted methods - Fixes CA1827 warnings (Count vs Any) --- .../SearchUsers/SearchUsersQueryHandler.cs | 60 +++++--- .../Services/UserService.Lifecycle.cs | 145 ++++++++++++------ 2 files changed, 139 insertions(+), 66 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/SearchUsers/SearchUsersQueryHandler.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/SearchUsers/SearchUsersQueryHandler.cs index 7c820e31b5..70c79f5e50 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/SearchUsers/SearchUsersQueryHandler.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/SearchUsers/SearchUsersQueryHandler.cs @@ -1,3 +1,4 @@ +using System.Linq.Expressions; using FSH.Framework.Core.Context; using FSH.Framework.Persistence; using FSH.Framework.Shared.Persistence; @@ -107,6 +108,15 @@ public async ValueTask> Handle(SearchUsersQuery query, Ca }; } + private static readonly Dictionary>> SortableFields = new(StringComparer.OrdinalIgnoreCase) + { + ["firstname"] = u => u.FirstName, + ["lastname"] = u => u.LastName, + ["email"] = u => u.Email, + ["username"] = u => u.UserName, + ["isactive"] = u => u.IsActive + }; + private static IQueryable ApplySorting(IQueryable query, string? sort) { if (string.IsNullOrWhiteSpace(sort)) @@ -119,30 +129,44 @@ private static IQueryable ApplySorting(IQueryable query, strin foreach (var part in sortParts) { - var descending = part.StartsWith('-'); - var field = descending ? part[1..] : part; - - orderedQuery = (orderedQuery, field.ToLowerInvariant()) switch + var (field, descending) = ParseSortField(part); + + if (!SortableFields.TryGetValue(field, out var selector)) { - (null, "firstname") => descending ? query.OrderByDescending(u => u.FirstName) : query.OrderBy(u => u.FirstName), - (null, "lastname") => descending ? query.OrderByDescending(u => u.LastName) : query.OrderBy(u => u.LastName), - (null, "email") => descending ? query.OrderByDescending(u => u.Email) : query.OrderBy(u => u.Email), - (null, "username") => descending ? query.OrderByDescending(u => u.UserName) : query.OrderBy(u => u.UserName), - (null, "isactive") => descending ? query.OrderByDescending(u => u.IsActive) : query.OrderBy(u => u.IsActive), - (null, _) => query.OrderBy(u => u.FirstName), - - (not null, "firstname") => descending ? orderedQuery.ThenByDescending(u => u.FirstName) : orderedQuery.ThenBy(u => u.FirstName), - (not null, "lastname") => descending ? orderedQuery.ThenByDescending(u => u.LastName) : orderedQuery.ThenBy(u => u.LastName), - (not null, "email") => descending ? orderedQuery.ThenByDescending(u => u.Email) : orderedQuery.ThenBy(u => u.Email), - (not null, "username") => descending ? orderedQuery.ThenByDescending(u => u.UserName) : orderedQuery.ThenBy(u => u.UserName), - (not null, "isactive") => descending ? orderedQuery.ThenByDescending(u => u.IsActive) : orderedQuery.ThenBy(u => u.IsActive), - (not null, _) => orderedQuery.ThenBy(u => u.FirstName) - }; + selector = u => u.FirstName; // Default fallback + } + + orderedQuery = ApplySortExpression(query, orderedQuery, selector, descending); } return orderedQuery ?? query.OrderBy(u => u.FirstName); } + private static (string field, bool descending) ParseSortField(string part) + { + var descending = part.StartsWith('-'); + var field = descending ? part[1..] : part; + return (field, descending); + } + + private static IOrderedQueryable ApplySortExpression( + IQueryable query, + IOrderedQueryable? orderedQuery, + Expression> selector, + bool descending) + { + if (orderedQuery is null) + { + return descending + ? query.OrderByDescending(selector) + : query.OrderBy(selector); + } + + return descending + ? orderedQuery.ThenByDescending(selector) + : orderedQuery.ThenBy(selector); + } + private string? ResolveImageUrl(string? imageUrl) { if (string.IsNullOrWhiteSpace(imageUrl)) diff --git a/src/Modules/Identity/Modules.Identity/Services/UserService.Lifecycle.cs b/src/Modules/Identity/Modules.Identity/Services/UserService.Lifecycle.cs index 883902736b..5cf648492f 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserService.Lifecycle.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserService.Lifecycle.cs @@ -1,6 +1,7 @@ using FSH.Framework.Core.Exceptions; using FSH.Framework.Shared.Constants; using FSH.Modules.Auditing.Contracts; +using FSH.Modules.Identity.Domain; using Microsoft.EntityFrameworkCore; namespace FSH.Modules.Identity.Services; @@ -27,84 +28,102 @@ public async Task ToggleStatusAsync(bool activateUser, string userId, Cancellati { EnsureValidTenant(); + var context = await BuildToggleContextAsync(userId, activateUser, cancellationToken); + + await ValidateTogglePermissionsAsync(context, cancellationToken); + + ApplyStatusChange(context); + + await SaveAndAuditAsync(context, cancellationToken); + } + + private async Task BuildToggleContextAsync( + string userId, + bool activateUser, + CancellationToken cancellationToken) + { var actorId = _currentUser.GetUserId(); if (actorId == Guid.Empty) { throw new UnauthorizedException("authenticated user required to toggle status"); } - var actor = await userManager.FindByIdAsync(actorId.ToString()); - _ = actor ?? throw new UnauthorizedException("current user not found"); + var actor = await userManager.FindByIdAsync(actorId.ToString()) + ?? throw new UnauthorizedException("current user not found"); - async ValueTask AuditPolicyFailureAsync(string reason, CancellationToken ct) - { - var tenant = multiTenantContextAccessor?.MultiTenantContext?.TenantInfo?.Id ?? "unknown"; - var claims = new Dictionary - { - ["actorId"] = actorId.ToString(), - ["targetUserId"] = userId, - ["tenant"] = tenant, - ["action"] = activateUser ? "activate" : "deactivate" - }; - - await _auditClient.WriteSecurityAsync( - SecurityAction.PolicyFailed, - subjectId: actorId.ToString(), - reasonCode: reason, - claims: claims, - severity: AuditSeverity.Warning, - source: "Identity", - ct: ct).ConfigureAwait(false); - } + var targetUser = await userManager.Users + .Where(u => u.Id == userId) + .FirstOrDefaultAsync(cancellationToken) + ?? throw new NotFoundException("User Not Found."); + + return new ToggleStatusContext( + ActorId: actorId, + Actor: actor, + TargetUser: targetUser, + ActivateUser: activateUser, + TenantId: multiTenantContextAccessor?.MultiTenantContext?.TenantInfo?.Id); + } - if (!await userManager.IsInRoleAsync(actor, RoleConstants.Admin)) + private async Task ValidateTogglePermissionsAsync( + ToggleStatusContext context, + CancellationToken cancellationToken) + { + if (!await userManager.IsInRoleAsync(context.Actor, RoleConstants.Admin)) { - await AuditPolicyFailureAsync("ActorNotAdmin", cancellationToken); + await AuditPolicyFailureAsync(context, "ActorNotAdmin", cancellationToken); throw new CustomException("Only administrators can toggle user status."); } - if (!activateUser && string.Equals(actor.Id, userId, StringComparison.Ordinal)) + if (!context.ActivateUser && context.ActorId.ToString() == context.TargetUser.Id) { - await AuditPolicyFailureAsync("SelfDeactivationBlocked", cancellationToken); + await AuditPolicyFailureAsync(context, "SelfDeactivationBlocked", cancellationToken); throw new CustomException("Users cannot deactivate themselves."); } - var user = await userManager.Users.Where(u => u.Id == userId).FirstOrDefaultAsync(cancellationToken); - _ = user ?? throw new NotFoundException("User Not Found."); - - bool targetIsAdmin = await userManager.IsInRoleAsync(user, RoleConstants.Admin); - if (targetIsAdmin) + if (await userManager.IsInRoleAsync(context.TargetUser, RoleConstants.Admin)) { - await AuditPolicyFailureAsync("AdminDeactivationBlocked", cancellationToken); + await AuditPolicyFailureAsync(context, "AdminDeactivationBlocked", cancellationToken); throw new CustomException("Administrators cannot be deactivated."); } - if (!activateUser) + if (!context.ActivateUser) + { + await EnsureMinimumActiveAdminsAsync(context, cancellationToken); + } + } + + private async Task EnsureMinimumActiveAdminsAsync( + ToggleStatusContext context, + CancellationToken cancellationToken) + { + var activeAdmins = await userManager.GetUsersInRoleAsync(RoleConstants.Admin); + if (!activeAdmins.Any(u => u.IsActive)) { - var activeAdmins = await userManager.GetUsersInRoleAsync(RoleConstants.Admin); - int activeAdminCount = activeAdmins.Count(u => u.IsActive); - if (activeAdminCount == 0) - { - await AuditPolicyFailureAsync("NoActiveAdmins", cancellationToken); - throw new CustomException("Tenant must have at least one active administrator."); - } + await AuditPolicyFailureAsync(context, "NoActiveAdmins", cancellationToken); + throw new CustomException("Tenant must have at least one active administrator."); } + } - var tenantId = multiTenantContextAccessor?.MultiTenantContext?.TenantInfo?.Id; - if (activateUser) + private static void ApplyStatusChange(ToggleStatusContext context) + { + if (context.ActivateUser) { - user.Activate(actorId.ToString(), tenantId); + context.TargetUser.Activate(context.ActorId.ToString(), context.TenantId); } else { - user.Deactivate(actorId.ToString(), "Status toggled by administrator", tenantId); + context.TargetUser.Deactivate(context.ActorId.ToString(), "Status toggled by administrator", context.TenantId); } + } - var result = await userManager.UpdateAsync(user); + private async Task SaveAndAuditAsync( + ToggleStatusContext context, + CancellationToken cancellationToken) + { + var result = await userManager.UpdateAsync(context.TargetUser); if (!result.Succeeded) { - var errors = result.Errors.Select(error => error.Description).ToList(); - throw new CustomException("Toggle status failed", errors); + throw new CustomException("Toggle status failed", result.Errors.Select(e => e.Description).ToList()); } await _auditClient.WriteActivityAsync( @@ -115,10 +134,40 @@ await _auditClient.WriteActivityAsync( captured: BodyCapture.None, requestSize: 0, responseSize: 0, - requestPreview: new { actorId = actorId.ToString(), targetUserId = userId, action = activateUser ? "activate" : "deactivate", tenant = tenantId ?? "unknown" }, + requestPreview: new { actorId = context.ActorId.ToString(), targetUserId = context.TargetUser.Id, action = context.ActivateUser ? "activate" : "deactivate", tenant = context.TenantId ?? "unknown" }, responsePreview: new { outcome = "success" }, severity: AuditSeverity.Information, source: "Identity", ct: cancellationToken).ConfigureAwait(false); } + + private async Task AuditPolicyFailureAsync( + ToggleStatusContext context, + string reason, + CancellationToken cancellationToken) + { + var claims = new Dictionary + { + ["actorId"] = context.ActorId.ToString(), + ["targetUserId"] = context.TargetUser.Id, + ["tenant"] = context.TenantId ?? "unknown", + ["action"] = context.ActivateUser ? "activate" : "deactivate" + }; + + await _auditClient.WriteSecurityAsync( + SecurityAction.PolicyFailed, + subjectId: context.ActorId.ToString(), + reasonCode: reason, + claims: claims, + severity: AuditSeverity.Warning, + source: "Identity", + ct: cancellationToken).ConfigureAwait(false); + } + + private sealed record ToggleStatusContext( + Guid ActorId, + FshUser Actor, + FshUser TargetUser, + bool ActivateUser, + string? TenantId); }