From 4e65ef1ff7d7a5cba57349f25fd6738f822a97b4 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 27 Aug 2026 22:02:03 -0700 Subject: [PATCH 1/4] test(clinical-scheduler,effort): serialize tests sharing HttpHelper.Cache HttpHelper.Cache is a process-wide static, and xUnit runs each test class as its own collection in parallel. Classes that call HttpHelper.Configure could swap the cache out from under each other mid-test, so a test could seed one instance and assert against another. - IntegrationTestBase configures the cache for every subclass, so two classes were racing without referencing TestDataBuilder themselves --- test/ClinicalScheduler/CliniciansControllerTest.cs | 1 + test/ClinicalScheduler/EmailNotificationTest.cs | 1 + .../Integration/ControllerServiceIntegrationTest.cs | 1 + .../Integration/PermissionServiceIntegrationTest.cs | 1 + .../Integration/ServiceLayerIntegrationTest.cs | 1 + test/ClinicalScheduler/PermissionsControllerTest.cs | 1 + test/ClinicalScheduler/RotationsControllerTest.cs | 1 + .../ScheduleEditServiceRollbackTest.cs | 1 + test/ClinicalScheduler/ScheduleEditServiceTest.cs | 1 + .../SchedulePermissionServiceTest.cs | 1 + test/Effort/EffortTypesControllerIntegrationTests.cs | 1 + .../Integration/EffortPermissionIntegrationTests.cs | 1 + .../PercentAssignTypesControllerIntegrationTests.cs | 1 + test/HttpHelperCacheCollection.cs | 11 +++++++++++ 14 files changed, 24 insertions(+) create mode 100644 test/HttpHelperCacheCollection.cs diff --git a/test/ClinicalScheduler/CliniciansControllerTest.cs b/test/ClinicalScheduler/CliniciansControllerTest.cs index f4730e9ce..9ef8fc2e0 100644 --- a/test/ClinicalScheduler/CliniciansControllerTest.cs +++ b/test/ClinicalScheduler/CliniciansControllerTest.cs @@ -12,6 +12,7 @@ namespace Viper.test.ClinicalScheduler { + [Collection(HttpHelperCacheCollection.Name)] public class CliniciansControllerTest : ClinicalSchedulerTestBase { private readonly AAUDContext _aaudContext; diff --git a/test/ClinicalScheduler/EmailNotificationTest.cs b/test/ClinicalScheduler/EmailNotificationTest.cs index 7dd97c420..0df55e64c 100644 --- a/test/ClinicalScheduler/EmailNotificationTest.cs +++ b/test/ClinicalScheduler/EmailNotificationTest.cs @@ -17,6 +17,7 @@ namespace Viper.test.ClinicalScheduler /// Tests for email notification functionality when primary evaluators are changed. /// Covers email sending, content validation, and error handling scenarios. /// + [Collection(HttpHelperCacheCollection.Name)] public class EmailNotificationTest : IDisposable { private readonly IScheduleAuditService _mockAuditService; diff --git a/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs b/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs index 9bb83e89c..feb65371a 100644 --- a/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs +++ b/test/ClinicalScheduler/Integration/ControllerServiceIntegrationTest.cs @@ -24,6 +24,7 @@ namespace Viper.test.ClinicalScheduler.Integration /// because the PermissionAttribute creates its own UserHelper instance, preventing proper mocking. /// These tests are kept for documentation purposes and can be converted to unit tests. /// + [Collection(HttpHelperCacheCollection.Name)] public class ControllerServiceIntegrationTest : IntegrationTestBase { private readonly IPersonService _personService; diff --git a/test/ClinicalScheduler/Integration/PermissionServiceIntegrationTest.cs b/test/ClinicalScheduler/Integration/PermissionServiceIntegrationTest.cs index 8cd7365b0..bdf32143f 100644 --- a/test/ClinicalScheduler/Integration/PermissionServiceIntegrationTest.cs +++ b/test/ClinicalScheduler/Integration/PermissionServiceIntegrationTest.cs @@ -10,6 +10,7 @@ namespace Viper.test.ClinicalScheduler.Integration /// Tests the complete flow of permission checks after consolidating /// ClinicalScheduleSecurityService into SchedulePermissionService. /// + [Collection(HttpHelperCacheCollection.Name)] public class PermissionServiceIntegrationTest : IntegrationTestBase { private readonly ILogger _mockLogger; diff --git a/test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs b/test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs index 6c92581e7..f3bb6260e 100644 --- a/test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs +++ b/test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs @@ -16,6 +16,7 @@ namespace Viper.test.ClinicalScheduler.Integration /// Tests the new service architecture with StudentScheduleService, InstructorScheduleService, /// and how ClinicalScheduleService delegates to them. /// + [Collection(HttpHelperCacheCollection.Name)] public class ServiceLayerIntegrationTest : IntegrationTestBase { private static readonly DateTime ScheduleStart = new(2024, 1, 1, 0, 0, 0, DateTimeKind.Local); diff --git a/test/ClinicalScheduler/PermissionsControllerTest.cs b/test/ClinicalScheduler/PermissionsControllerTest.cs index 7280cbff3..34ae6f87b 100644 --- a/test/ClinicalScheduler/PermissionsControllerTest.cs +++ b/test/ClinicalScheduler/PermissionsControllerTest.cs @@ -8,6 +8,7 @@ namespace Viper.test.ClinicalScheduler { + [Collection(HttpHelperCacheCollection.Name)] public class PermissionsControllerTest : ClinicalSchedulerTestBase { private readonly ISchedulePermissionService _mockPermissionService; diff --git a/test/ClinicalScheduler/RotationsControllerTest.cs b/test/ClinicalScheduler/RotationsControllerTest.cs index d9a1b8090..91afaf46d 100644 --- a/test/ClinicalScheduler/RotationsControllerTest.cs +++ b/test/ClinicalScheduler/RotationsControllerTest.cs @@ -9,6 +9,7 @@ namespace Viper.test.ClinicalScheduler { + [Collection(HttpHelperCacheCollection.Name)] public class RotationsControllerTest : ClinicalSchedulerTestBase { private readonly ILogger _mockLogger; diff --git a/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs index 7d08884f6..94543d878 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceRollbackTest.cs @@ -18,6 +18,7 @@ namespace Viper.test.ClinicalScheduler /// prove a failed audit write rolls the schedule change back, not just that the /// exception surfaces. /// + [Collection(HttpHelperCacheCollection.Name)] public class ScheduleEditServiceRollbackTest : IDisposable { private readonly SqliteConnection _connection; diff --git a/test/ClinicalScheduler/ScheduleEditServiceTest.cs b/test/ClinicalScheduler/ScheduleEditServiceTest.cs index a05887886..4cf982080 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceTest.cs @@ -13,6 +13,7 @@ namespace Viper.test.ClinicalScheduler { + [Collection(HttpHelperCacheCollection.Name)] public class ScheduleEditServiceTest : IDisposable { private readonly ISchedulePermissionService _mockPermissionService; diff --git a/test/ClinicalScheduler/SchedulePermissionServiceTest.cs b/test/ClinicalScheduler/SchedulePermissionServiceTest.cs index 9f0cc4616..cc7672c65 100644 --- a/test/ClinicalScheduler/SchedulePermissionServiceTest.cs +++ b/test/ClinicalScheduler/SchedulePermissionServiceTest.cs @@ -4,6 +4,7 @@ namespace Viper.test.ClinicalScheduler { + [Collection(HttpHelperCacheCollection.Name)] public class SchedulePermissionServiceTest : ClinicalSchedulerTestBase { private readonly ILogger _mockLogger; diff --git a/test/Effort/EffortTypesControllerIntegrationTests.cs b/test/Effort/EffortTypesControllerIntegrationTests.cs index 7cd3d28bf..7d2866cf3 100644 --- a/test/Effort/EffortTypesControllerIntegrationTests.cs +++ b/test/Effort/EffortTypesControllerIntegrationTests.cs @@ -13,6 +13,7 @@ namespace Viper.test.Effort; /// Integration tests for EffortTypesController. /// Tests the full stack: Controller -> Service -> DbContext. /// +[Collection(HttpHelperCacheCollection.Name)] public class EffortTypesControllerIntegrationTests : EffortIntegrationTestBase { private readonly EffortTypesController _controller; diff --git a/test/Effort/Integration/EffortPermissionIntegrationTests.cs b/test/Effort/Integration/EffortPermissionIntegrationTests.cs index b1bde6d61..1bb00070e 100644 --- a/test/Effort/Integration/EffortPermissionIntegrationTests.cs +++ b/test/Effort/Integration/EffortPermissionIntegrationTests.cs @@ -8,6 +8,7 @@ namespace Viper.test.Effort.Integration; /// Tests the complete flow of permission checks for department-level, /// full access, and self-service permission models. /// +[Collection(HttpHelperCacheCollection.Name)] public class EffortPermissionIntegrationTests : EffortIntegrationTestBase { private readonly EffortPermissionService _permissionService; diff --git a/test/Effort/PercentAssignTypesControllerIntegrationTests.cs b/test/Effort/PercentAssignTypesControllerIntegrationTests.cs index 43f291048..07b95d868 100644 --- a/test/Effort/PercentAssignTypesControllerIntegrationTests.cs +++ b/test/Effort/PercentAssignTypesControllerIntegrationTests.cs @@ -12,6 +12,7 @@ namespace Viper.test.Effort; /// Integration tests for PercentAssignTypesController. /// Tests the full stack: Controller -> Service -> DbContext. /// +[Collection(HttpHelperCacheCollection.Name)] public class PercentAssignTypesControllerIntegrationTests : EffortIntegrationTestBase { private readonly PercentAssignTypesController _controller; diff --git a/test/HttpHelperCacheCollection.cs b/test/HttpHelperCacheCollection.cs new file mode 100644 index 000000000..909d5f305 --- /dev/null +++ b/test/HttpHelperCacheCollection.cs @@ -0,0 +1,11 @@ +namespace Viper.test; + +/// +/// HttpHelper.Configure swaps a process-wide static cache, and xUnit runs test classes in parallel by +/// default. Every class that configures the cache must join this collection. +/// +[CollectionDefinition(HttpHelperCacheCollection.Name)] +public static class HttpHelperCacheCollection +{ + public const string Name = "HttpHelperCache"; +} From cd9446182fa8845dcd9249345b7e864a4d212ae6 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 27 Aug 2026 22:03:34 -0700 Subject: [PATCH 2/4] fix(raps): require Duo 2FA on RAPS pages and guard unauthenticated endpoints RAPSController had the 2FA policy commented out while every RAPS API controller required it, so the pages and nav rendered for a user without a Duo credential and every call behind them returned 403. The frontend rewrites 403s to a generic message, so the real reason never surfaced. - MembersController Search and Get had no authorization at all: attribute routes are not covered by RequireAuthorization on the conventional routes. They now also need one of the permissions behind the pages that use them, which the RAPS role alone did not imply - Gate the Role List nav item on the rule its action enforces, scoped to the requested instance so a delegate is not sent to a list filtered to nothing - Show the Directory emulate button only with SVMSecure.SU, matching the table view, and keep the app base on its link - Memoize the delegate-role lookup, which Nav queried once per instance --- test/RAPS/RapsControllerAuthorizationTests.cs | 48 +++++++++ test/RAPS/RapsSecurityServiceTests.cs | 100 ++++++++++++++++++ web/Areas/Directory/Views/Card.cshtml | 11 +- web/Areas/Directory/Views/Table.cshtml | 2 +- web/Areas/RAPS/Controllers/AuditController.cs | 2 + .../RAPS/Controllers/MembersController.cs | 6 ++ web/Areas/RAPS/Controllers/RAPSController.cs | 11 +- .../RAPS/Services/RAPSSecurityService.cs | 58 ++++++++-- 8 files changed, 217 insertions(+), 21 deletions(-) create mode 100644 test/RAPS/RapsControllerAuthorizationTests.cs create mode 100644 test/RAPS/RapsSecurityServiceTests.cs diff --git a/test/RAPS/RapsControllerAuthorizationTests.cs b/test/RAPS/RapsControllerAuthorizationTests.cs new file mode 100644 index 000000000..5fc92de44 --- /dev/null +++ b/test/RAPS/RapsControllerAuthorizationTests.cs @@ -0,0 +1,48 @@ +using System.Reflection; +using Microsoft.AspNetCore.Authorization; +using Viper.Areas.RAPS.Controllers; + +namespace Viper.test.RAPS +{ + /// + /// Every controller in the area has to sit behind both the RAPS roles and Duo. A gap is invisible in + /// Development, where Duo auto-succeeds, and shows up on Test/Prod as a page whose API 403s. + /// + public class RapsControllerAuthorizationTests + { + public static TheoryData RapsControllers() + { + return new TheoryData(typeof(RAPSController).Assembly.GetTypes() + .Where(t => t.Namespace == typeof(RAPSController).Namespace && t.Name.EndsWith("Controller")) + .OrderBy(t => t.Name)); + } + + [Theory] + [MemberData(nameof(RapsControllers))] + public void EveryRapsController_RequiresDuoTwoFactor(Type controller) + { + var authorize = controller.GetCustomAttributes(inherit: true) + .FirstOrDefault(a => a.Policy == "2faAuthentication"); + + Assert.True(authorize is not null, $"{controller.Name} is missing [Authorize(Policy = \"2faAuthentication\")]"); + } + + [Theory] + [MemberData(nameof(RapsControllers))] + public void EveryRapsController_RequiresARapsRole(Type controller) + { + var roles = controller.GetCustomAttributes(inherit: true) + .Select(a => a.Roles) + .FirstOrDefault(r => !string.IsNullOrEmpty(r)); + + Assert.False(string.IsNullOrEmpty(roles), $"{controller.Name} is missing an [Authorize(Roles = ...)] restriction"); + } + + [Fact] + public void RapsControllersAreDiscovered() + { + // Guards the two theories above against silently passing on an empty set. + Assert.NotEmpty(RapsControllers()); + } + } +} diff --git a/test/RAPS/RapsSecurityServiceTests.cs b/test/RAPS/RapsSecurityServiceTests.cs new file mode 100644 index 000000000..0aec1b4a4 --- /dev/null +++ b/test/RAPS/RapsSecurityServiceTests.cs @@ -0,0 +1,100 @@ +using MockQueryable.NSubstitute; +using NSubstitute; +using Viper.Areas.RAPS.Services; +using Viper.Classes.SQLContext; +using Viper.Models.AAUD; +using Viper.Models.RAPS; + +namespace Viper.test.RAPS +{ + /// + /// Covers CanViewRoleList, which gates both the Role List nav item and the RoleList action. The nav + /// item used to be unconditional, so a user without access saw a link that landed on a 403. + /// + public class RapsSecurityServiceTests + { + private const string MemberId = "10000001"; + + private static RAPSContext ContextWithRoles(List roles) + { + // BuildMockDbSet() makes its own NSubstitute calls, so build it before opening the Returns() call + var mockSet = roles.BuildMockDbSet(); + var context = Substitute.For(); + context.TblRoles.Returns(mockSet); + return context; + } + + private static IUserHelper UserWith(params string[] permissions) + { + var userHelper = Substitute.For(); + userHelper.GetCurrentUser().Returns(new AaudUser { MothraId = MemberId }); + userHelper.HasPermission(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(call => permissions.Contains(call.ArgAt(2))); + return userHelper; + } + + /// + /// A delegate role (Application = 1) the user belongs to, which puts the named role under their + /// control. The controlled role's name is what decides the instance it counts for. + /// + private static List RolesWithDelegatedRole(string controlledRole) + { + var controlled = new TblRole { RoleId = 2, Role = controlledRole }; + var delegateRole = new TblRole + { + RoleId = 1, + Role = "VIPER.DelegateRole", + Application = 1, + TblRoleMembers = { new TblRoleMember { RoleId = 1, MemberId = MemberId } }, + ChildRoles = { new TblAppRole { AppRoleId = 1, RoleId = 2, Role = controlled } } + }; + return new List { delegateRole, controlled }; + } + + [Fact] + public void AdminCanViewRoleList() + { + var service = new RAPSSecurityService(ContextWithRoles(new List()), UserWith("RAPS.Admin")); + + Assert.True(service.CanViewRoleList("VIPER")); + } + + [Theory] + [InlineData("VMACS.VMTH", true)] + [InlineData("VIPER", false)] + public void HelpDeskCanViewRoleList_OnlyInVMACSInstances(string instance, bool expected) + { + var service = new RAPSSecurityService(ContextWithRoles(new List()), UserWith("RAPS.ViewRoles")); + + Assert.Equal(expected, service.CanViewRoleList(instance)); + } + + [Theory] + [InlineData("VMACS.VMTH", true)] + [InlineData("VIPER", false)] + public void DelegateCanViewRoleList_OnlyInTheInstanceHoldingTheControlledRole(string instance, bool expected) + { + var service = new RAPSSecurityService(ContextWithRoles(RolesWithDelegatedRole("VMACS.VMTH.Controlled")), UserWith()); + + // The role list filters to the instance, so a delegate whose controlled role is in + // VMACS must not be offered the VIPER list that would come back empty. + Assert.Equal(expected, service.CanViewRoleList(instance)); + } + + [Fact] + public void DelegateCanViewRoleList_WithoutAnyRapsPermission() + { + var service = new RAPSSecurityService(ContextWithRoles(RolesWithDelegatedRole("VIPER.Controlled")), UserWith()); + + Assert.True(service.CanViewRoleList("VIPER")); + } + + [Fact] + public void CannotViewRoleList_WithoutPermissionsOrDelegatedRoles() + { + var service = new RAPSSecurityService(ContextWithRoles(new List()), UserWith()); + + Assert.False(service.CanViewRoleList("VMACS.VMTH")); + } + } +} diff --git a/web/Areas/Directory/Views/Card.cshtml b/web/Areas/Directory/Views/Card.cshtml index 53158cec2..5ef4c0687 100644 --- a/web/Areas/Directory/Views/Card.cshtml +++ b/web/Areas/Directory/Views/Card.cshtml @@ -38,10 +38,13 @@ Email {{user.mailId}}@@ucdavis.edu - - - Emulate {{user.name}} - + @if (UserHelper.HasPermission(rapsContext, UserHelper.GetCurrentUser(), "SVMSecure.SU")) + { + + + Emulate {{user.name}} + + } AAUD Check diff --git a/web/Areas/Directory/Views/Table.cshtml b/web/Areas/Directory/Views/Table.cshtml index 6427c31a1..41fb5bcd8 100644 --- a/web/Areas/Directory/Views/Table.cshtml +++ b/web/Areas/Directory/Views/Table.cshtml @@ -114,7 +114,7 @@ } @if (UserHelper.HasPermission(rapsContext, UserHelper.GetCurrentUser(), "SVMSecure.SU")) { - @: + @: } diff --git a/web/Areas/RAPS/Controllers/AuditController.cs b/web/Areas/RAPS/Controllers/AuditController.cs index 4e6b4e9c9..5ca712687 100644 --- a/web/Areas/RAPS/Controllers/AuditController.cs +++ b/web/Areas/RAPS/Controllers/AuditController.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Viper.Areas.RAPS.Models; @@ -10,6 +11,7 @@ namespace Viper.Areas.RAPS.Controllers { [Route("raps/{instance}/[controller]")] + [Authorize(Roles = "VMDO SVM-IT,RAPS Users", Policy = "2faAuthentication")] [Permission(Allow = "RAPS.Admin,RAPS.ViewAuditTrail")] public class AuditController : ApiController { diff --git a/web/Areas/RAPS/Controllers/MembersController.cs b/web/Areas/RAPS/Controllers/MembersController.cs index 1825847a6..363e92bf4 100644 --- a/web/Areas/RAPS/Controllers/MembersController.cs +++ b/web/Areas/RAPS/Controllers/MembersController.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Viper.Areas.RAPS.Models; @@ -11,6 +12,7 @@ namespace Viper.Areas.RAPS.Controllers { [Route("raps/{Instance=VIPER}/[controller]")] [ApiController] + [Authorize(Roles = "VMDO SVM-IT,RAPS Users", Policy = "2faAuthentication")] public class MembersController : ControllerBase { private readonly RAPSContext _context; @@ -26,6 +28,9 @@ public MembersController(RAPSContext context, AAUDContext aaudContext) _rapsCacheService = new RAPSCacheService(_context, aaudContext); } // GET: + // The union of what the three pages using this typeahead require. Without it, any RAPS Users + // member could enumerate identities here. + [Permission(Allow = "RAPS.Admin,RAPS.UserLookup,RAPS.EditRoleMembership,RAPS.EditMemberPermissions")] [HttpGet] public async Task>> Search(string search, string active = "active") { @@ -68,6 +73,7 @@ public async Task>> Search(string s } // GET /12345678 + [Permission(Allow = "RAPS.Admin,RAPS.UserLookup,RAPS.EditRoleMembership,RAPS.EditMemberPermissions")] [HttpGet("{memberId}")] public async Task> Get(string memberId) { diff --git a/web/Areas/RAPS/Controllers/RAPSController.cs b/web/Areas/RAPS/Controllers/RAPSController.cs index aaaf3c877..4f960d03c 100644 --- a/web/Areas/RAPS/Controllers/RAPSController.cs +++ b/web/Areas/RAPS/Controllers/RAPSController.cs @@ -6,7 +6,6 @@ using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; -using Microsoft.IdentityModel.Tokens; using NLog; using Viper.Areas.RAPS.Services; using Viper.Classes; @@ -18,7 +17,7 @@ namespace Viper.Areas.RAPS.Controllers { [Area("RAPS")] [Route("[area]/[action]")] - [Authorize(Roles = "VMDO SVM-IT,RAPS Users")]//, Policy = "2faAuthentication" + [Authorize(Roles = "VMDO SVM-IT,RAPS Users", Policy = "2faAuthentication")] public class RAPSController : AreaController { private readonly RAPSContext _RAPSContext; @@ -121,7 +120,10 @@ public async Task Nav(int? roleId, int? permissionId, string? memberId, nav.Add(new NavMenuItem { MenuItemText = inst, MenuItemURL = "~/raps/" + inst + "/" + (usePage ? page : "RoleList") }); } nav.Add(new NavMenuItem { MenuItemText = "Roles", IsHeader = true }); - nav.Add(new NavMenuItem { MenuItemText = "Role List", MenuItemURL = "Rolelist" }); + if (_securityService.CanViewRoleList(instance)) + { + nav.Add(new NavMenuItem { MenuItemText = "Role List", MenuItemURL = "Rolelist" }); + } if (_securityService.IsAllowedTo("EditRoleMembership", instance)) { nav.Add(new NavMenuItem { MenuItemText = "Role Comparison", MenuItemURL = "RolePermissionsComparison" }); @@ -236,8 +238,7 @@ public IActionResult RoleList(string instance) return View("~/Areas/RAPS/Views/Roles/ListAdmin.cshtml"); } - if (_securityService.IsAllowedTo("ViewAllRoles", instance) || - !_securityService.GetControlledRoleIds(UserHelper.GetCurrentUser()?.MothraId).IsNullOrEmpty()) + if (_securityService.CanViewRoleList(instance)) { return View("~/Areas/RAPS/Views/Roles/List.cshtml"); } diff --git a/web/Areas/RAPS/Services/RAPSSecurityService.cs b/web/Areas/RAPS/Services/RAPSSecurityService.cs index 76cc25ea2..90d27d114 100644 --- a/web/Areas/RAPS/Services/RAPSSecurityService.cs +++ b/web/Areas/RAPS/Services/RAPSSecurityService.cs @@ -10,6 +10,7 @@ public class RAPSSecurityService { private readonly IUserHelper _userHelper; private readonly RAPSContext _context; + private readonly Dictionary> _appRolesForUser = new(); public RAPSSecurityService(RAPSContext context, IUserHelper? userHelper = null) { @@ -212,13 +213,23 @@ public bool IsAllowedTo(string action, string instance, TblRole Role) /// A list of delegate roles the user is assigned to public List GetAppRolesForUser(string? userId) { + // Nav() asks once per instance on every page load, so memoize for the life of the + // service, which is one request. + string key = userId ?? string.Empty; + if (_appRolesForUser.TryGetValue(key, out List? cached)) + { + return cached; + } + List roles = _context.TblRoles - .Include(r => r.TblRoleMembers) + .AsNoTracking() .Include(r => r.ChildRoles) .ThenInclude(cr => cr.Role) .Where(r => r.Application == 1) .Where(r => r.TblRoleMembers.Any(rm => rm.MemberId == userId)) .ToList(); + + _appRolesForUser[key] = roles; return roles; } @@ -229,16 +240,41 @@ public List GetAppRolesForUser(string? userId) /// List of roleIds the user controls public List GetControlledRoleIds(string? userId) { - List controlledRoles = GetAppRolesForUser(userId); - List controlledRoleIds = new(); - foreach (TblRole controlledRole in controlledRoles) - { - foreach (TblAppRole childRole in controlledRole.ChildRoles) - { - controlledRoleIds.Add(childRole.Role.RoleId); - } - } - return controlledRoleIds; + return ControlledRoles(userId).Select(r => r.RoleId).ToList(); + } + + /// + /// The role ids a user controls through delegate roles, limited to one instance. + /// + /// The instance + /// User mothra id + /// List of roleIds the user controls in that instance + public List GetControlledRoleIdsInInstance(string instance, string? userId) + { + return ControlledRoles(userId) + .Where(r => RoleBelongsToInstance(instance, r)) + .Select(r => r.RoleId) + .Distinct() + .ToList(); + } + + private IEnumerable ControlledRoles(string? userId) + { + return GetAppRolesForUser(userId).SelectMany(r => r.ChildRoles).Select(cr => cr.Role); + } + + /// + /// Check if the user can see the role list for an instance. Admins see every role, others see + /// all roles only in VMACS instances, or just the roles delegated to them. + /// + /// The instance + /// true if the user can view the role list, false otherwise + public bool CanViewRoleList(string instance) + { + // Instance-scoped because the role list is: an unscoped count would offer a delegate a + // nav link to a list filtered down to nothing. + return IsAllowedTo("ViewAllRoles", instance) + || GetControlledRoleIdsInInstance(instance, _userHelper.GetCurrentUser()?.MothraId).Count > 0; } /// From f4c06037087147562f37afd5e62d2e3f1cf52d59 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 27 Aug 2026 22:06:34 -0700 Subject: [PATCH 3/4] fix(raps): invalidate the permission cache on every RAPS write Invalidation lived at each call site and several write paths never called it: the nightly role refresh (RoleViews via RapsRoleRefreshScheduledJob), the OU group sync, and role CRUD all changed membership without evicting. Entries never expired either, so a revoked permission kept working until the app pool recycled. - Move invalidation into a SaveChanges interceptor on RAPSContext so any path reaching the database is covered, including ones added later - Expand a role-level permission change to that role's current members - Key the cache on MothraId, not the nullable LoginId, which collided across every user without one and is what the RAPS tables key on anyway --- test/Classes/UserHelperCacheTests.cs | 96 ++++++++++ .../RapsCacheInvalidationInterceptorTests.cs | 177 ++++++++++++++++++ test/RAPS/RoleMembersControllerTests.cs | 7 +- test/RAPS/RoleTemplateCrudTests.cs | 5 +- test/RAPS/RoleTemplatesControllerTests.cs | 7 +- .../MemberPermissionsController.cs | 10 +- .../RAPS/Controllers/MembersController.cs | 5 +- .../RAPS/Controllers/RoleMembersController.cs | 10 +- .../Controllers/RolePermissionsController.cs | 17 +- .../Controllers/RoleTemplatesController.cs | 6 +- web/Areas/RAPS/Services/RAPSCacheService.cs | 28 --- .../RapsCacheInvalidationInterceptor.cs | 140 ++++++++++++++ .../Services/EmergencyContactService.cs | 25 --- web/Classes/UserHelper.cs | 41 +++- web/Program.cs | 7 +- 15 files changed, 458 insertions(+), 123 deletions(-) create mode 100644 test/Classes/UserHelperCacheTests.cs create mode 100644 test/RAPS/RapsCacheInvalidationInterceptorTests.cs delete mode 100644 web/Areas/RAPS/Services/RAPSCacheService.cs create mode 100644 web/Areas/RAPS/Services/RapsCacheInvalidationInterceptor.cs diff --git a/test/Classes/UserHelperCacheTests.cs b/test/Classes/UserHelperCacheTests.cs new file mode 100644 index 000000000..94e034f2b --- /dev/null +++ b/test/Classes/UserHelperCacheTests.cs @@ -0,0 +1,96 @@ +using Microsoft.Extensions.Caching.Memory; +using Viper.Models.AAUD; + +namespace Viper.test.Classes +{ + [Collection(HttpHelperCacheCollection.Name)] + public sealed class UserHelperCacheTests : IDisposable + { + private MemoryCache? _installed; + + // Empty, not dispose: HttpHelper.Cache is a process-wide static, so a disposed instance would + // make every later test throw. + public void Dispose() + { + _installed?.Clear(); + } + + private const string MothraId = "00012345"; + + // Spelled out rather than shared with UserHelper so a rename there has to be deliberate. + private static readonly string[] CacheKeys = + { + "Roles-" + MothraId, + "PermissionsAssigned-" + MothraId + "-True", + "PermissionsAssigned-" + MothraId + "-False", + "PermissionsInherited-" + MothraId + "-True", + "PermissionsInherited-" + MothraId + "-False", + }; + + private IMemoryCache ConfigureCache() + { + var memoryCache = new MemoryCache(new MemoryCacheOptions()); + HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!); + _installed = memoryCache; + return memoryCache; + } + + [Fact] + public void ClearCachedRolesAndPermissions_RemovesEveryRoleAndPermissionKey() + { + var memoryCache = ConfigureCache(); + foreach (string key in CacheKeys) + { + memoryCache.Set(key, "cached"); + } + + new UserHelper().ClearCachedRolesAndPermissions(new AaudUser { MothraId = MothraId }); + + foreach (string key in CacheKeys) + { + Assert.False(memoryCache.TryGetValue(key, out _), $"{key} was left in the cache"); + } + } + + [Fact] + public void ClearCachedRolesAndPermissions_LeavesOtherUsersEntriesAlone() + { + var memoryCache = ConfigureCache(); + memoryCache.Set("Roles-99999999", "cached"); + + new UserHelper().ClearCachedRolesAndPermissions(new AaudUser { MothraId = MothraId }); + + Assert.True(memoryCache.TryGetValue("Roles-99999999", out _)); + } + + [Fact] + public void ClearCachedRolesAndPermissions_ByMothraId_NeedsNoAaudUser() + { + var memoryCache = ConfigureCache(); + foreach (string key in CacheKeys) + { + memoryCache.Set(key, "cached"); + } + + // The interceptor only has the RAPS-side id, so this overload is the one it calls. + UserHelper.ClearCachedRolesAndPermissions(MothraId); + + foreach (string key in CacheKeys) + { + Assert.False(memoryCache.TryGetValue(key, out _), $"{key} was left in the cache"); + } + } + + [Fact] + public void ClearCachedRolesAndPermissions_IgnoresAnEmptyMothraId() + { + var memoryCache = ConfigureCache(); + memoryCache.Set("Roles-", "cached"); + + UserHelper.ClearCachedRolesAndPermissions(string.Empty); + + // Guards the old LoginId behaviour, where users without one shared the "Roles-" key. + Assert.True(memoryCache.TryGetValue("Roles-", out _)); + } + } +} diff --git a/test/RAPS/RapsCacheInvalidationInterceptorTests.cs b/test/RAPS/RapsCacheInvalidationInterceptorTests.cs new file mode 100644 index 000000000..a440b5ddf --- /dev/null +++ b/test/RAPS/RapsCacheInvalidationInterceptorTests.cs @@ -0,0 +1,177 @@ +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Caching.Memory; +using Viper.Areas.RAPS.Services; +using Viper.Classes.SQLContext; +using Viper.Models.RAPS; + +namespace Viper.test.RAPS +{ + /// + /// Pins the interceptor that replaced the per-call-site invalidation, which several write paths + /// (the nightly role refresh, the OU group sync) never called. + /// + [Collection(HttpHelperCacheCollection.Name)] + public class RapsCacheInvalidationInterceptorTests : IAsyncLifetime + { + private const string MothraId = "00012345"; + + private static readonly string[] CacheKeys = + { + "Roles-" + MothraId, + "PermissionsAssigned-" + MothraId + "-True", + "PermissionsAssigned-" + MothraId + "-False", + "PermissionsInherited-" + MothraId + "-True", + "PermissionsInherited-" + MothraId + "-False", + }; + + private SqliteConnection _connection = null!; + private RAPSContext _context = null!; + + public async ValueTask InitializeAsync() + { + _connection = new SqliteConnection("Filename=:memory:"); + await _connection.OpenAsync(TestContext.Current.CancellationToken); + _context = await NewContextAsync(_connection); + } + + public async ValueTask DisposeAsync() + { + await _context.DisposeAsync(); + await _connection.DisposeAsync(); + } + + private static async Task NewContextAsync(SqliteConnection connection) + { + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .AddInterceptors(new RapsCacheInvalidationInterceptor()) + .Options; + var context = new RAPSContext(options); + await context.Database.EnsureCreatedAsync(TestContext.Current.CancellationToken); + + // Parent rows for the foreign keys the writes below depend on. + context.TblRoles.AddRange( + new TblRole { RoleId = 1, Role = "VIPER.Test", Application = 0, UpdateFreq = 0, AllowAllUsers = false }, + new TblRole { RoleId = 7, Role = "VIPER.Shared", Application = 0, UpdateFreq = 0, AllowAllUsers = false }); + context.TblPermissions.Add(new TblPermission { PermissionId = 1, Permission = "SVMSecure.Test" }); + // TblRoleMember.MemberId and TblMemberPermission.MemberId both FK to VwAaudUser. + context.VwAaudUser.AddRange( + new VwAaudUser { MothraId = MothraId, DisplayFirstName = "Test", DisplayLastName = "User", DisplayFullName = "User, Test" }, + new VwAaudUser { MothraId = "99999999", DisplayFirstName = "Other", DisplayLastName = "User", DisplayFullName = "User, Other" }); + await context.SaveChangesAsync(TestContext.Current.CancellationToken); + return context; + } + + private static IMemoryCache SeedCache() + { + var memoryCache = new MemoryCache(new MemoryCacheOptions()); + HttpHelper.Configure(memoryCache, null!, null!, null!, null!, null!); + foreach (string key in CacheKeys) + { + memoryCache.Set(key, "cached"); + } + return memoryCache; + } + + private static void AssertCleared(IMemoryCache cache) + { + foreach (string key in CacheKeys) + { + Assert.False(cache.TryGetValue(key, out _), $"{key} was left in the cache"); + } + } + + [Fact] + public async Task AddingARoleMember_EvictsThatMember() + { + var cache = SeedCache(); + + _context.TblRoleMembers.Add(new TblRoleMember { RoleId = 1, MemberId = MothraId }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + AssertCleared(cache); + } + + [Fact] + public async Task RemovingARoleMember_EvictsThatMember() + { + var member = new TblRoleMember { RoleId = 1, MemberId = MothraId }; + _context.TblRoleMembers.Add(member); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + // Revocation is the direction that matters: a stale allow keeps access alive. + var cache = SeedCache(); + _context.TblRoleMembers.Remove(member); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + AssertCleared(cache); + } + + [Fact] + public async Task ChangingAnIndividualPermission_EvictsThatMember() + { + var cache = SeedCache(); + + _context.TblMemberPermissions.Add(new TblMemberPermission { PermissionId = 1, MemberId = MothraId, Access = 1 }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + AssertCleared(cache); + } + + [Fact] + public async Task ChangingARolesPermissions_EvictsEveryMemberOfThatRole() + { + _context.TblRoleMembers.Add(new TblRoleMember { RoleId = 7, MemberId = MothraId }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + // The write names the role, not the people, so the interceptor has to expand it. + var cache = SeedCache(); + _context.TblRolePermissions.Add(new TblRolePermission { RoleId = 7, PermissionId = 1, Access = 1 }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + AssertCleared(cache); + } + + [Fact] + public async Task ChangingTheRoleItself_EvictsEveryMemberOfThatRole() + { + _context.TblRoleMembers.Add(new TblRoleMember { RoleId = 7, MemberId = MothraId }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + // Renaming a role repoints the permissions its members inherit, so it has to expand too. + var cache = SeedCache(); + TblRole role = await _context.TblRoles.SingleAsync(r => r.RoleId == 7, TestContext.Current.CancellationToken); + role.Description = "Renamed"; + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + AssertCleared(cache); + } + + [Fact] + public void SynchronousSave_EvictsToo() + { + var cache = SeedCache(); + + // Not every RAPS write path is async, and the sync overrides stash and evict separately. + _context.TblRoleMembers.Add(new TblRoleMember { RoleId = 1, MemberId = MothraId }); + _context.SaveChanges(); + + AssertCleared(cache); + } + + [Fact] + public async Task AnUnrelatedWrite_LeavesTheCacheAlone() + { + var cache = SeedCache(); + + _context.TblRoleMembers.Add(new TblRoleMember { RoleId = 1, MemberId = "99999999" }); + await _context.SaveChangesAsync(TestContext.Current.CancellationToken); + + foreach (string key in CacheKeys) + { + Assert.True(cache.TryGetValue(key, out _), $"{key} should not have been evicted"); + } + } + } +} diff --git a/test/RAPS/RoleMembersControllerTests.cs b/test/RAPS/RoleMembersControllerTests.cs index 5ee37333a..84d3050f4 100644 --- a/test/RAPS/RoleMembersControllerTests.cs +++ b/test/RAPS/RoleMembersControllerTests.cs @@ -14,7 +14,6 @@ public class RoleMembersControllerTests : IAsyncLifetime { private SqliteConnection _connection = null!; private RAPSContext _context = null!; - private AAUDContext _aaudContext = null!; public async ValueTask InitializeAsync() { @@ -24,19 +23,15 @@ public async ValueTask InitializeAsync() .UseSqlite(_connection) .Options); await _context.Database.EnsureCreatedAsync(TestContext.Current.CancellationToken); - _aaudContext = new AAUDContext(new DbContextOptionsBuilder() - .UseSqlite(_connection) - .Options); } public async ValueTask DisposeAsync() { await _context.DisposeAsync(); - await _aaudContext.DisposeAsync(); await _connection.DisposeAsync(); } - private RoleMembersController CreateController() => new(_context, _aaudContext); + private RoleMembersController CreateController() => new(_context); [Fact] public async Task PushRolesToVMACS_EmptyRoleIds_ReturnsBadRequest() diff --git a/test/RAPS/RoleTemplateCrudTests.cs b/test/RAPS/RoleTemplateCrudTests.cs index 31cc8760b..85421cc33 100644 --- a/test/RAPS/RoleTemplateCrudTests.cs +++ b/test/RAPS/RoleTemplateCrudTests.cs @@ -91,12 +91,9 @@ private static async Task CreateContextAsync(SqliteConnection conne return context; } - // The AAUD context only reaches the cache service, which neither create nor update touches. private static RoleTemplatesController CreateController(RAPSContext context) { - var aaudContext = new AAUDContext(new DbContextOptionsBuilder() - .UseInMemoryDatabase("AAUD_" + Guid.NewGuid()).Options); - return new RoleTemplatesController(context, aaudContext); + return new RoleTemplatesController(context); } private static async Task SeedTemplateAsync(RAPSContext context) diff --git a/test/RAPS/RoleTemplatesControllerTests.cs b/test/RAPS/RoleTemplatesControllerTests.cs index 66cbcf6d2..4c467a363 100644 --- a/test/RAPS/RoleTemplatesControllerTests.cs +++ b/test/RAPS/RoleTemplatesControllerTests.cs @@ -24,7 +24,6 @@ public class RoleTemplatesControllerTests : IAsyncLifetime private SqliteConnection _connection = null!; private RAPSContext _context = null!; - private AAUDContext _aaudContext = null!; public async ValueTask InitializeAsync() { @@ -34,9 +33,6 @@ public async ValueTask InitializeAsync() .UseSqlite(_connection) .Options); await _context.Database.EnsureCreatedAsync(TestContext.Current.CancellationToken); - _aaudContext = new AAUDContext(new DbContextOptionsBuilder() - .UseSqlite(_connection) - .Options); var alreadyHeld = new TblRole { RoleId = RoleAlreadyHeldId, Role = "VIPER.AlreadyHeld", Description = "Held" }; var toAdd = new TblRole { RoleId = RoleToAddId, Role = "VIPER.ToAdd", Description = "Not held" }; @@ -66,11 +62,10 @@ public async ValueTask InitializeAsync() public async ValueTask DisposeAsync() { await _context.DisposeAsync(); - await _aaudContext.DisposeAsync(); await _connection.DisposeAsync(); } - private RoleTemplatesController CreateController() => new(_context, _aaudContext); + private RoleTemplatesController CreateController() => new(_context); [Fact] public async Task PreviewRoleTemplateApply_UnknownMember_ReturnsOkWithNullResult() diff --git a/web/Areas/RAPS/Controllers/MemberPermissionsController.cs b/web/Areas/RAPS/Controllers/MemberPermissionsController.cs index ef3fb54ad..9f68dfa2f 100644 --- a/web/Areas/RAPS/Controllers/MemberPermissionsController.cs +++ b/web/Areas/RAPS/Controllers/MemberPermissionsController.cs @@ -18,16 +18,14 @@ public class MemberPermissionsController : ApiController private readonly RAPSContext _context; private readonly RAPSSecurityService _securityService; private readonly RAPSAuditService _auditService; - private readonly RAPSCacheService _rapsCacheService; public IUserHelper UserHelper { get; private set; } - public MemberPermissionsController(RAPSContext context, AAUDContext aaudContext) + public MemberPermissionsController(RAPSContext context) { _context = context; _securityService = new RAPSSecurityService(_context); _auditService = new RAPSAuditService(_context); UserHelper = new UserHelper(); - _rapsCacheService = new RAPSCacheService(context, aaudContext, UserHelper); } // GET: Members/12345678/Permissions @@ -204,8 +202,6 @@ public async Task PutTblMemberPermission(string instance, string throw; } - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return NoContent(); } @@ -262,8 +258,6 @@ public async Task> PostTblMemberPermission(str throw; } - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return CreatedAtAction("GetTblMemberPermission", new { memberId, permissionId }, tblMemberPermission); } @@ -292,8 +286,6 @@ public async Task DeleteTblMemberPermission(string instance, stri _auditService.AuditPermissionMemberChange(tblMemberPermission, RAPSAuditService.AuditActionType.Delete); await _context.SaveChangesAsync(); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return NoContent(); } diff --git a/web/Areas/RAPS/Controllers/MembersController.cs b/web/Areas/RAPS/Controllers/MembersController.cs index 363e92bf4..b97610e2d 100644 --- a/web/Areas/RAPS/Controllers/MembersController.cs +++ b/web/Areas/RAPS/Controllers/MembersController.cs @@ -18,14 +18,12 @@ public class MembersController : ControllerBase private readonly RAPSContext _context; private readonly RAPSSecurityService _securityService; private readonly RAPSAuditService _auditService; - private readonly RAPSCacheService _rapsCacheService; - public MembersController(RAPSContext context, AAUDContext aaudContext) + public MembersController(RAPSContext context) { _context = context; _securityService = new RAPSSecurityService(_context); _auditService = new RAPSAuditService(_context); - _rapsCacheService = new RAPSCacheService(_context, aaudContext); } // GET: // The union of what the three pages using this typeahead require. Without it, any RAPS Users @@ -223,7 +221,6 @@ public async Task Clone(string instance, string sourceMemberId, st } await new CloneService(_context).Clone(instance, sourceMemberId, targetMemberId, objectsToClone); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(targetMemberId); return NoContent(); } diff --git a/web/Areas/RAPS/Controllers/RoleMembersController.cs b/web/Areas/RAPS/Controllers/RoleMembersController.cs index 21d3bf3e3..e46f6f395 100644 --- a/web/Areas/RAPS/Controllers/RoleMembersController.cs +++ b/web/Areas/RAPS/Controllers/RoleMembersController.cs @@ -17,14 +17,12 @@ public class RoleMembersController : ApiController private readonly RAPSContext _context; private readonly RAPSSecurityService _securityService; private readonly RAPSAuditService _auditService; - private readonly RAPSCacheService _rapsCacheService; - public RoleMembersController(RAPSContext context, AAUDContext aaudContext) + public RoleMembersController(RAPSContext context) { _context = context; _securityService = new RAPSSecurityService(_context); _auditService = new RAPSAuditService(_context); - _rapsCacheService = new RAPSCacheService(_context, aaudContext); } //GET: Roles/5/Members @@ -102,8 +100,6 @@ public async Task>> PostTblRoleMembers(s return BadRequest(result); } - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - TblRoleMember? tblRoleMember = await _context.TblRoleMembers.FindAsync(roleId, memberId); return CreatedAtAction("GetTblRole", new { roleId, memberId }, tblRoleMember); } @@ -139,8 +135,6 @@ public async Task>> PutTblRoleMembers(st _auditService.AuditRoleMemberChange(tblRoleMember, RAPSAuditService.AuditActionType.Update, roleMemberCreateUpdate.Comment); await _context.SaveChangesAsync(); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return NoContent(); } @@ -169,8 +163,6 @@ public async Task DeleteTblRoleMembers(string instance, int roleI _auditService.AuditRoleMemberChange(tblRoleMember, RAPSAuditService.AuditActionType.Delete, comment); await _context.SaveChangesAsync(); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return NoContent(); } diff --git a/web/Areas/RAPS/Controllers/RolePermissionsController.cs b/web/Areas/RAPS/Controllers/RolePermissionsController.cs index 82f8b3b65..3c218e429 100644 --- a/web/Areas/RAPS/Controllers/RolePermissionsController.cs +++ b/web/Areas/RAPS/Controllers/RolePermissionsController.cs @@ -18,14 +18,12 @@ public class RolePermissionsController : ApiController private readonly RAPSContext _context; private readonly RAPSSecurityService _securityService; private readonly RAPSAuditService _auditService; - private readonly RAPSCacheService _rapsCacheService; - public RolePermissionsController(RAPSContext context, AAUDContext aaudContext) + public RolePermissionsController(RAPSContext context) { _context = context; _securityService = new RAPSSecurityService(_context); _auditService = new RAPSAuditService(_context); - _rapsCacheService = new RAPSCacheService(_context, aaudContext); } private ActionResult? CheckRoleAndPermissionParams(string instance, int? roleId, int? permissionId) @@ -174,8 +172,6 @@ public async Task> PostTblRolePermission(string await _context.SaveChangesAsync(); await transaction.CommitAsync(); - await ClearCacheForAllRoleMembers(rolePermission.RoleId); - return CreatedAtAction("GetTblRole", new { roleId, permissionId, tblRolePermission.Access }, tblRolePermission); } @@ -206,8 +202,6 @@ public async Task> DeleteTblRolePermission(string in _auditService.AuditRolePermissionChange(tblRolePermission, RAPSAuditService.AuditActionType.Delete); await _context.SaveChangesAsync(); - await ClearCacheForAllRoleMembers(roleId); - return NoContent(); } @@ -219,14 +213,5 @@ private static void UpdateTblRolePermissionsWithDto(TblRolePermission tblRolePer tblRolePermission.ModTime = DateTime.Now; tblRolePermission.ModBy = new UserHelper().GetCurrentUser()?.LoginId; } - - private async Task ClearCacheForAllRoleMembers(int roleId) - { - var roleMembers = await _context.TblRoleMembers.Where(rm => rm.RoleId == roleId).ToListAsync(); - foreach (var member in roleMembers) - { - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(member.MemberId); - } - } } } diff --git a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs index 2903fc97e..a65fbbc9d 100644 --- a/web/Areas/RAPS/Controllers/RoleTemplatesController.cs +++ b/web/Areas/RAPS/Controllers/RoleTemplatesController.cs @@ -16,14 +16,12 @@ namespace Viper.Areas.RAPS.Controllers public class RoleTemplatesController : ApiController { private readonly RAPSContext _context; - private readonly RAPSCacheService _rapsCacheService; public IUserHelper UserHelper { get; private set; } - public RoleTemplatesController(RAPSContext context, AAUDContext aaudContext) + public RoleTemplatesController(RAPSContext context) { _context = context; UserHelper = new UserHelper(); - _rapsCacheService = new RAPSCacheService(context, aaudContext, UserHelper); } // GET: RoleTemplates @@ -125,8 +123,6 @@ public async Task> RoleTemplateApply(stri await roleMemberService.AddMemberToRole(role.RoleId, memberId, null, null, string.Format("Added via role template {0}", roleTemplate.TemplateName)); } - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - return NoContent(); } diff --git a/web/Areas/RAPS/Services/RAPSCacheService.cs b/web/Areas/RAPS/Services/RAPSCacheService.cs deleted file mode 100644 index 1590c1fb6..000000000 --- a/web/Areas/RAPS/Services/RAPSCacheService.cs +++ /dev/null @@ -1,28 +0,0 @@ -using Microsoft.EntityFrameworkCore; -using Viper.Classes.SQLContext; -using Viper.Models.AAUD; - -namespace Viper.Areas.RAPS.Services -{ - public class RAPSCacheService - { - private readonly AAUDContext aaudContext; - private readonly IUserHelper userHelper; - public RAPSCacheService(RAPSContext rapsContext, AAUDContext aaudContext, IUserHelper? userHelper = null) - { - this.aaudContext = aaudContext; - this.userHelper = userHelper ?? new UserHelper(); - } - - - public void ClearCachedRolesAndPermissionsForUser(string mothraId) - { - AaudUser? user = aaudContext.AaudUsers.AsNoTracking().FirstOrDefault(u => u.MothraId == mothraId); - if (user != null) - { - userHelper.ClearCachedRolesAndPermissions(user); - } - } - } - -} diff --git a/web/Areas/RAPS/Services/RapsCacheInvalidationInterceptor.cs b/web/Areas/RAPS/Services/RapsCacheInvalidationInterceptor.cs new file mode 100644 index 000000000..d822be79e --- /dev/null +++ b/web/Areas/RAPS/Services/RapsCacheInvalidationInterceptor.cs @@ -0,0 +1,140 @@ +using System.Runtime.CompilerServices; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; +using Viper.Classes.SQLContext; +using Viper.Models.RAPS; + +namespace Viper.Areas.RAPS.Services +{ + /// + /// Evicts cached roles and permissions for whoever a RAPS write affected. Lives on the context + /// rather than at each call site so every path reaching the database is covered, including ones + /// added later. + /// + public class RapsCacheInvalidationInterceptor : SaveChangesInterceptor + { + private sealed record Affected(HashSet MemberIds, HashSet RoleIds); + + // Keyed on the context instance so a scoped context's pending set cannot leak into another + // request's. ConditionalWeakTable drops entries when the context is collected. + private static readonly ConditionalWeakTable Pending = new(); + + public override InterceptionResult SavingChanges(DbContextEventData eventData, InterceptionResult result) + { + StashAffected(eventData); + return base.SavingChanges(eventData, result); + } + + public override ValueTask> SavingChangesAsync(DbContextEventData eventData, + InterceptionResult result, CancellationToken cancellationToken = default) + { + StashAffected(eventData); + return base.SavingChangesAsync(eventData, result, cancellationToken); + } + + public override int SavedChanges(SaveChangesCompletedEventData eventData, int result) + { + Evict(eventData); + return base.SavedChanges(eventData, result); + } + + public override ValueTask SavedChangesAsync(SaveChangesCompletedEventData eventData, int result, + CancellationToken cancellationToken = default) + { + Evict(eventData); + return base.SavedChangesAsync(eventData, result, cancellationToken); + } + + // Read the ChangeTracker before SaveChanges: afterwards deleted entries are detached and their + // ids are gone. + private static void StashAffected(DbContextEventData eventData) + { + var context = eventData.Context; + if (context == null) + { + return; + } + + var affected = new Affected(new HashSet(StringComparer.OrdinalIgnoreCase), new HashSet()); + + foreach (var entry in context.ChangeTracker.Entries()) + { + if (entry.State is not (EntityState.Added or EntityState.Modified or EntityState.Deleted)) + { + continue; + } + + switch (entry.Entity) + { + // A membership or an individual grant changes exactly one person. + case TblRoleMember roleMember: + Add(affected.MemberIds, roleMember.MemberId); + break; + case TblMemberPermission memberPermission: + Add(affected.MemberIds, memberPermission.MemberId); + break; + // A role's permissions change everyone currently in that role. + case TblRolePermission rolePermission: + affected.RoleIds.Add(rolePermission.RoleId); + break; + case TblRole role: + affected.RoleIds.Add(role.RoleId); + break; + } + } + + Pending.Remove(context); + if (affected.MemberIds.Count > 0 || affected.RoleIds.Count > 0) + { + Pending.Add(context, affected); + } + } + + private static void Evict(DbContextEventData eventData) + { + var context = eventData.Context; + if (context == null) + { + return; + } + + if (!Pending.TryGetValue(context, out var affected)) + { + return; + } + // Dropped from the table, so the sets below are ours to mutate. + Pending.Remove(context); + + // Expand role-level changes to that role's current members. Runs after the save, so the + // membership read reflects what was just committed. + if (affected.RoleIds.Count > 0 && context is RAPSContext rapsContext) + { + // EF.Parameter so a bulk change (role template apply, OU sync) translates through + // OPENJSON rather than inlining every id. + List changedRoleIds = affected.RoleIds.ToList(); + foreach (string memberId in rapsContext.TblRoleMembers + .AsNoTracking() + .Where(rm => EF.Parameter(changedRoleIds).Contains(rm.RoleId)) + .Select(rm => rm.MemberId) + .Distinct() + .ToList()) + { + Add(affected.MemberIds, memberId); + } + } + + foreach (string mothraId in affected.MemberIds) + { + UserHelper.ClearCachedRolesAndPermissions(mothraId); + } + } + + private static void Add(HashSet set, string? memberId) + { + if (!string.IsNullOrEmpty(memberId)) + { + set.Add(memberId); + } + } + } +} diff --git a/web/Areas/Students/Services/EmergencyContactService.cs b/web/Areas/Students/Services/EmergencyContactService.cs index 38d81e1e1..cffbed1da 100644 --- a/web/Areas/Students/Services/EmergencyContactService.cs +++ b/web/Areas/Students/Services/EmergencyContactService.cs @@ -17,7 +17,6 @@ public class EmergencyContactService : IEmergencyContactService private readonly AAUDContext _aaudContext; private readonly IUserHelper _userHelper; private readonly ILogger _logger; - private readonly RAPSCacheService _rapsCacheService; private readonly RAPSAuditService _rapsAuditService; public EmergencyContactService( @@ -32,7 +31,6 @@ public EmergencyContactService( _aaudContext = aaudContext; _userHelper = userHelper; _logger = logger; - _rapsCacheService = new RAPSCacheService(rapsContext, aaudContext, userHelper); _rapsAuditService = new RAPSAuditService(rapsContext, userHelper); } @@ -311,7 +309,6 @@ public async Task ToggleAppAccessAsync() _rapsAuditService.AuditRolePermissionChange(rolePermission!, RAPSAuditService.AuditActionType.Delete); _rapsContext.TblRolePermissions.Remove(rolePermission!); await _rapsContext.SaveChangesAsync(); - ClearCacheForRoleMembers(roleId); return false; } @@ -337,7 +334,6 @@ public async Task ToggleAppAccessAsync() _rapsAuditService.AuditRolePermissionChange(rolePermission, RAPSAuditService.AuditActionType.Update); } await _rapsContext.SaveChangesAsync(); - ClearCacheForRoleMembers(roleId); return true; } @@ -374,7 +370,6 @@ public async Task ToggleIndividualAccessAsync(int personId) } _rapsContext.TblMemberPermissions.RemoveRange(existing); await _rapsContext.SaveChangesAsync(); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(user.MothraId); return false; } @@ -391,7 +386,6 @@ public async Task ToggleIndividualAccessAsync(int personId) _rapsContext.TblMemberPermissions.Add(memberPermission); _rapsAuditService.AuditPermissionMemberChange(memberPermission, RAPSAuditService.AuditActionType.Create); await _rapsContext.SaveChangesAsync(); - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(user.MothraId); return true; } @@ -598,25 +592,6 @@ private async Task IsCurrentDvmStudentAsync(int personId) return null; } - /// - /// Clears the cached roles and permissions for all members of a given role, - /// so that permission changes take effect immediately. - /// - private void ClearCacheForRoleMembers(int roleId) - { - var memberIds = _rapsContext.TblRoleMembers - .Where(rm => rm.RoleId == roleId - && (rm.StartDate == null || rm.StartDate <= DateTime.Now) - && (rm.EndDate == null || rm.EndDate >= DateTime.Now)) - .Select(rm => rm.MemberId) - .ToList(); - - foreach (var memberId in memberIds) - { - _rapsCacheService.ClearCachedRolesAndPermissionsForUser(memberId); - } - } - private static void ValidatePhone(string? value, string fieldName, List invalidFields) { if (!PhoneHelper.IsValidPhone(value)) diff --git a/web/Classes/UserHelper.cs b/web/Classes/UserHelper.cs index 849af0d1e..de762aa8f 100644 --- a/web/Classes/UserHelper.cs +++ b/web/Classes/UserHelper.cs @@ -17,6 +17,33 @@ public class UserHelper : IUserHelper { private readonly AAUDContext? _aaudContext; + // Keyed by MothraId, not the nullable LoginId, which collided across every user without one. + // Entries do not expire; RapsCacheInvalidationInterceptor evicts them. + private static string RolesCacheKey(string mothraId) => "Roles-" + mothraId; + + private static string AssignedPermissionsCacheKey(string mothraId, bool deny) => "PermissionsAssigned-" + mothraId + "-" + deny; + + private static string InheritedPermissionsCacheKey(string mothraId, bool deny) => "PermissionsInherited-" + mothraId + "-" + deny; + + /// + /// Evict one user's cached roles and permissions. Takes a MothraId so callers holding only the + /// RAPS-side identifier need no AaudUser lookup. + /// + public static void ClearCachedRolesAndPermissions(string mothraId) + { + if (HttpHelper.Cache == null || string.IsNullOrEmpty(mothraId)) + { + return; + } + + HttpHelper.Cache.Remove(RolesCacheKey(mothraId)); + foreach (bool deny in new[] { true, false }) + { + HttpHelper.Cache.Remove(AssignedPermissionsCacheKey(mothraId, deny)); + HttpHelper.Cache.Remove(InheritedPermissionsCacheKey(mothraId, deny)); + } + } + public UserHelper() { } public UserHelper(AAUDContext aaudContext) @@ -56,7 +83,7 @@ public IEnumerable GetRoles(RAPSContext rapsContext, AaudUser user) if (HttpHelper.Cache != null && rapsContext != null) { - result = HttpHelper.Cache.GetOrCreate("Roles-" + user.LoginId, entry => + result = HttpHelper.Cache.GetOrCreate(RolesCacheKey(user.MothraId), _ => { return (from role in rapsContext.TblRoles join memberRoles in rapsContext.TblRoleMembers @@ -117,7 +144,7 @@ public IEnumerable GetAssignedPermissions(RAPSContext rapsContext if (HttpHelper.Cache != null && rapsContext != null) { - result = HttpHelper.Cache.GetOrCreate("PermissionsAssigned-" + user.LoginId + "-" + deny, entry => + result = HttpHelper.Cache.GetOrCreate(AssignedPermissionsCacheKey(user.MothraId, deny), _ => { return (from permission in rapsContext.TblPermissions join memberPermissions in rapsContext.TblMemberPermissions @@ -157,7 +184,7 @@ public static IEnumerable GetInheritedPermissions(RAPSContext rap if (HttpHelper.Cache != null && rapsContext != null) { - result = HttpHelper.Cache.GetOrCreate("PermissionsInherited-" + user.LoginId + "-" + deny, entry => + result = HttpHelper.Cache.GetOrCreate(InheritedPermissionsCacheKey(user.MothraId, deny), _ => { return (from permission in rapsContext.TblPermissions join rolePermissions in rapsContext.TblRolePermissions @@ -346,13 +373,9 @@ public bool IsEmulating() public void ClearCachedRolesAndPermissions(AaudUser? user) { - if (user != null && HttpHelper.Cache != null) + if (user != null) { - HttpHelper.Cache.Remove("Roles-" + user.LoginId); - HttpHelper.Cache.Remove("PermissionsAssigned-" + user.LoginId + "-" + true); - HttpHelper.Cache.Remove("PermissionsAssigned-" + user.LoginId + "-" + false); - HttpHelper.Cache.Remove("PermissionsInherited-" + user.LoginId + "-" + false); - HttpHelper.Cache.Remove("PermissionsInherited-" + user.LoginId + "-" + false); + ClearCachedRolesAndPermissions(user.MothraId); } } diff --git a/web/Program.cs b/web/Program.cs index 126ab15c8..d51acaf3d 100644 --- a/web/Program.cs +++ b/web/Program.cs @@ -31,6 +31,7 @@ using Viper.Areas.Effort; using Viper.Areas.Effort.Data; using Viper.Areas.Effort.Services.Harvest; +using Viper.Areas.RAPS.Services; using Viper.Classes; using Viper.Classes.HealthChecks; using Viper.Classes.Scheduler; @@ -200,7 +201,7 @@ // Configure DbContext options with connection strings via DI var enableDetailedErrors = builder.Environment.EnvironmentName != "Production"; - void RegisterDbContext(string connectionStringKey) where TContext : DbContext + void RegisterDbContext(string connectionStringKey, Action? configure = null) where TContext : DbContext { var connStr = builder.Configuration.GetConnectionString(connectionStringKey) ?? throw new InvalidOperationException($"Connection string '{connectionStringKey}' not configured"); @@ -209,6 +210,7 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db // Match our SQL Server 2016 compat level (130) so EF Core 10 generates optimal SQL for our DB version options.UseSqlServer(connStr, o => o.UseCompatibilityLevel(130)); if (enableDetailedErrors) options.EnableDetailedErrors(); + configure?.Invoke(options); }); } @@ -216,7 +218,8 @@ void RegisterDbContext(string connectionStringKey) where TContext : Db RegisterDbContext("Courses"); RegisterDbContext("CREST"); RegisterDbContext("Dictionary"); - RegisterDbContext("RAPS"); + // The interceptor evicts affected users' cached permissions on any RAPS write, whatever made it. + RegisterDbContext("RAPS", o => o.AddInterceptors(new RapsCacheInvalidationInterceptor())); RegisterDbContext("VIPER"); RegisterDbContext("ClinicalScheduler"); RegisterDbContext("SIS"); From de8ef5a417db5681d4985167b181b7e727f8812e Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Thu, 27 Aug 2026 23:15:32 -0700 Subject: [PATCH 4/4] refactor(auth): only set the Duo failure message when Duo actually fails The Development bypass succeeded the requirement and then still stored the "two-factor authentication is required" message, which was never surfaced but is misleading to anyone inspecting HttpContext.Items locally. - Add coverage pinning the bypass, so it is not mistaken for an oversight and removed: no Duo credential can be issued for a localhost callback --- .../DuoAuthenticationRequirementTests.cs | 75 +++++++++++++++++++ web/Classes/DuoAuthenticationRequirement.cs | 7 +- 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 test/Classes/DuoAuthenticationRequirementTests.cs diff --git a/test/Classes/DuoAuthenticationRequirementTests.cs b/test/Classes/DuoAuthenticationRequirementTests.cs new file mode 100644 index 000000000..89caf518f --- /dev/null +++ b/test/Classes/DuoAuthenticationRequirementTests.cs @@ -0,0 +1,75 @@ +using System.Security.Claims; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; +using Web.Authorization; + +namespace Viper.test.Classes +{ + /// + /// Pins the deliberate Development bypass, since no Duo credential can be issued for a localhost + /// callback, and pins that the failure message is set only when the requirement fails. + /// + public class DuoAuthenticationRequirementTests + { + private const string ErrorKey = "ErrorMessage"; + + private static HttpContext ContextFor(string environmentName) + { + var env = Substitute.For(); + env.EnvironmentName = environmentName; + + var services = new ServiceCollection(); + services.AddSingleton(env); + + return new DefaultHttpContext { RequestServices = services.BuildServiceProvider() }; + } + + private static ClaimsPrincipal UserWithDuo() => + new(new ClaimsIdentity(new[] { new Claim("credentialType", "DuoCredential") }, "test")); + + private static async Task<(bool Succeeded, object? Error)> EvaluateAsync(ClaimsPrincipal user, string environmentName) + { + var httpContext = ContextFor(environmentName); + var requirement = new DuoAuthenticationRequirement(); + var context = new AuthorizationHandlerContext(new[] { requirement }, user, httpContext); + + await requirement.HandleAsync(context); + + httpContext.Items.TryGetValue(ErrorKey, out object? error); + return (context.HasSucceeded, error); + } + + [Fact] + public async Task DuoCredential_Succeeds() + { + var (succeeded, error) = await EvaluateAsync(UserWithDuo(), "Production"); + + Assert.True(succeeded); + Assert.Null(error); + } + + [Fact] + public async Task Development_SucceedsWithoutDuo() + { + var (succeeded, error) = await EvaluateAsync(new ClaimsPrincipal(new ClaimsIdentity()), "Development"); + + Assert.True(succeeded); + // The bypass is not a failure, so it must not leave a failure message behind. + Assert.Null(error); + } + + [Theory] + [InlineData("Production")] + [InlineData("Test")] + public async Task OutsideDevelopment_FailsWithoutDuoAndExplainsWhy(string environmentName) + { + var (succeeded, error) = await EvaluateAsync(new ClaimsPrincipal(new ClaimsIdentity()), environmentName); + + Assert.False(succeeded); + Assert.Equal("DUO two-factor authentication is required", error); + } + } +} diff --git a/web/Classes/DuoAuthenticationRequirement.cs b/web/Classes/DuoAuthenticationRequirement.cs index dc927775d..25f85c6d4 100644 --- a/web/Classes/DuoAuthenticationRequirement.cs +++ b/web/Classes/DuoAuthenticationRequirement.cs @@ -32,12 +32,17 @@ protected override Task HandleRequirementAsync(AuthorizationHandlerContext conte { if (httpContext is not null) { + // No Duo credential can be issued for a localhost callback, so Development bypasses + // the check. RapsControllerAuthorizationTests is what guards the policy itself. var env = httpContext.RequestServices.GetRequiredService(); if (env != null && env.EnvironmentName == "Development") { context.Succeed(requirement); } - httpContext.Items["ErrorMessage"] = "DUO two-factor authentication is required"; + else + { + httpContext.Items["ErrorMessage"] = "DUO two-factor authentication is required"; + } } else {