From 4381aa0454d10574ca4a9e7a2bb7d01367aa7b23 Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 16:45:12 -0700 Subject: [PATCH 1/6] Fixing issue where user login doesn't check current/future flags --- web/Classes/UserHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/Classes/UserHelper.cs b/web/Classes/UserHelper.cs index de762aa8f..4ec4b9b29 100644 --- a/web/Classes/UserHelper.cs +++ b/web/Classes/UserHelper.cs @@ -279,7 +279,7 @@ public bool HasPermission(RAPSContext? rapsContext, AaudUser? user, string permi if (HttpHelper.Cache != null && aaudContext != null) { user = HttpHelper.Cache.GetOrCreate("AaudUser-" + userLoginId, - _ => aaudContext.AaudUsers.FirstOrDefault(m => m.LoginId == loginId)); + _ => aaudContext.AaudUsers.FirstOrDefault(m => m.LoginId == loginId || (m.Current == 1 || m.Future == 1))); return user; } From 3bda2d19ef77b41bc9fddac83d912ef0f23b5699 Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 16:47:05 -0700 Subject: [PATCH 2/6] Fixing condition --- web/Classes/UserHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/Classes/UserHelper.cs b/web/Classes/UserHelper.cs index 4ec4b9b29..8b1df37eb 100644 --- a/web/Classes/UserHelper.cs +++ b/web/Classes/UserHelper.cs @@ -279,7 +279,7 @@ public bool HasPermission(RAPSContext? rapsContext, AaudUser? user, string permi if (HttpHelper.Cache != null && aaudContext != null) { user = HttpHelper.Cache.GetOrCreate("AaudUser-" + userLoginId, - _ => aaudContext.AaudUsers.FirstOrDefault(m => m.LoginId == loginId || (m.Current == 1 || m.Future == 1))); + _ => aaudContext.AaudUsers.FirstOrDefault(m => m.LoginId == loginId && (m.Current == 1 || m.Future == 1))); return user; } From f5bbd5f8be761847d40ebdbec4fc7389a535af03 Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 17:12:05 -0700 Subject: [PATCH 3/6] Use AsNoTracking for user retrieval from cache Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- web/Classes/UserHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/Classes/UserHelper.cs b/web/Classes/UserHelper.cs index 8b1df37eb..b9e9c3fc4 100644 --- a/web/Classes/UserHelper.cs +++ b/web/Classes/UserHelper.cs @@ -279,7 +279,7 @@ public bool HasPermission(RAPSContext? rapsContext, AaudUser? user, string permi if (HttpHelper.Cache != null && aaudContext != null) { user = HttpHelper.Cache.GetOrCreate("AaudUser-" + userLoginId, - _ => aaudContext.AaudUsers.FirstOrDefault(m => m.LoginId == loginId && (m.Current == 1 || m.Future == 1))); + _ => aaudContext.AaudUsers.AsNoTracking().FirstOrDefault(m => m.LoginId == loginId && (m.Current == 1 || m.Future == 1))) return user; } From ef6163041956e6d81ef0ada2888d78f30e36b118 Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 17:27:43 -0700 Subject: [PATCH 4/6] Code review comments - handling cache misses, users in cache too long, and tests --- test/Classes/UserHelperGetByLoginIdTests.cs | 134 ++++++++++++++++++++ web/Classes/UserHelper.cs | 22 ++-- 2 files changed, 145 insertions(+), 11 deletions(-) create mode 100644 test/Classes/UserHelperGetByLoginIdTests.cs diff --git a/test/Classes/UserHelperGetByLoginIdTests.cs b/test/Classes/UserHelperGetByLoginIdTests.cs new file mode 100644 index 000000000..d9973df75 --- /dev/null +++ b/test/Classes/UserHelperGetByLoginIdTests.cs @@ -0,0 +1,134 @@ +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Internal; +using Viper.Classes.SQLContext; +using Viper.Models.AAUD; +using Viper.test.Students; + +namespace Viper.test.Classes +{ + [Collection(HttpHelperCacheCollection.Name)] + public sealed class UserHelperGetByLoginIdTests : IDisposable + { + private const string LoginId = "jdoe"; + + private readonly FakeClock _clock = new(); + private readonly MemoryCache _memoryCache; + private readonly TestableAAUDContext _context; + + public UserHelperGetByLoginIdTests() + { + _memoryCache = new MemoryCache(new MemoryCacheOptions { Clock = _clock }); + HttpHelper.Configure(_memoryCache, null!, null!, null!, null!, null!, null!); + + var options = new DbContextOptionsBuilder() + .UseInMemoryDatabase(databaseName: Guid.NewGuid().ToString()) + .Options; + _context = new TestableAAUDContext(options); + } + + // Clear, not dispose: HttpHelper.Cache is a process-wide static, so a disposed instance would + // make every later test throw. + public void Dispose() + { + _memoryCache.Clear(); + _context.Dispose(); + } + + private void AddUser(int current = 1, int future = 0) + { + _context.AaudUsers.Add(new AaudUser + { + AaudUserId = 1, + ClientId = "C1", + MothraId = "00012345", + LoginId = LoginId, + FirstName = "Jane", + LastName = "Doe", + DisplayFirstName = "Jane", + DisplayLastName = "Doe", + DisplayFullName = "Jane Doe", + Current = current, + Future = future, + }); + _context.SaveChanges(); + _context.ChangeTracker.Clear(); + } + + private void RemoveUsers() + { + _context.AaudUsers.RemoveRange(_context.AaudUsers); + _context.SaveChanges(); + } + + [Theory] + [InlineData(1, 0)] + [InlineData(0, 1)] + public void GetByLoginId_ReturnsCurrentOrFutureUser(int current, int future) + { + AddUser(current, future); + + var result = new UserHelper().GetByLoginId(_context, LoginId); + + Assert.NotNull(result); + Assert.Equal(LoginId, result.LoginId); + } + + [Fact] + public void GetByLoginId_ReturnsNullForInactiveUser() + { + AddUser(current: 0, future: 0); + + Assert.Null(new UserHelper().GetByLoginId(_context, LoginId)); + } + + [Fact] + public void GetByLoginId_ServesFoundUserFromCache() + { + AddUser(); + var helper = new UserHelper(); + helper.GetByLoginId(_context, LoginId); + + RemoveUsers(); + + Assert.NotNull(helper.GetByLoginId(_context, LoginId)); + } + + [Fact] + public void GetByLoginId_DoesNotCacheMiss() + { + var helper = new UserHelper(); + Assert.Null(helper.GetByLoginId(_context, LoginId)); + + // A user who becomes active after a failed lookup must not be locked out by a cached null. + AddUser(); + + Assert.NotNull(helper.GetByLoginId(_context, LoginId)); + } + + [Fact] + public void GetByLoginId_CachedUserExpires() + { + AddUser(); + var helper = new UserHelper(); + helper.GetByLoginId(_context, LoginId); + + // A deactivated user must drop out once the cache entry expires. + RemoveUsers(); + AddUser(current: 0, future: 0); + _clock.UtcNow = _clock.UtcNow.AddMinutes(11); + + Assert.Null(helper.GetByLoginId(_context, LoginId)); + } + + [Fact] + public void GetByLoginId_ReturnsNullForNullLoginId() + { + Assert.Null(new UserHelper().GetByLoginId(_context, null)); + } + + private sealed class FakeClock : ISystemClock + { + public DateTimeOffset UtcNow { get; set; } = DateTimeOffset.UtcNow; + } + } +} diff --git a/web/Classes/UserHelper.cs b/web/Classes/UserHelper.cs index b9e9c3fc4..634d982b6 100644 --- a/web/Classes/UserHelper.cs +++ b/web/Classes/UserHelper.cs @@ -262,24 +262,24 @@ public bool HasPermission(RAPSContext? rapsContext, AaudUser? user, string permi { try { - string userLoginId = loginId.ToLower(); - AaudUser? user = null; + string cacheKey = "AaudUser-" + loginId.ToLower(); // if we have already cached this user as an UCD user then create a Viper user from that object - if (HttpHelper.Cache != null) + if (HttpHelper.Cache?.TryGetValue(cacheKey, out AaudUser? cachedUser) == true && cachedUser != null) { - user = HttpHelper.Cache.Get("AaudUser-" + userLoginId); - } - - if (user != null) - { - return user; + return cachedUser; } if (HttpHelper.Cache != null && aaudContext != null) { - user = HttpHelper.Cache.GetOrCreate("AaudUser-" + userLoginId, - _ => aaudContext.AaudUsers.AsNoTracking().FirstOrDefault(m => m.LoginId == loginId && (m.Current == 1 || m.Future == 1))) + AaudUser? user = aaudContext.AaudUsers.AsNoTracking() + .FirstOrDefault(m => m.LoginId == loginId && (m.Current == 1 || m.Future == 1)); + + // Only cache found users, and expire them so changes to active flags are picked up + if (user != null) + { + HttpHelper.Cache.Set(cacheKey, user, TimeSpan.FromMinutes(10)); + } return user; } From 5e358a3b746509d5d0f1870b93cba19c9fe66dff Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 18:30:49 -0700 Subject: [PATCH 5/6] Fixing missing using statement --- test/Classes/UserHelperGetByLoginIdTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Classes/UserHelperGetByLoginIdTests.cs b/test/Classes/UserHelperGetByLoginIdTests.cs index d9973df75..e4384621a 100644 --- a/test/Classes/UserHelperGetByLoginIdTests.cs +++ b/test/Classes/UserHelperGetByLoginIdTests.cs @@ -1,3 +1,4 @@ +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Internal; using Viper.Classes.SQLContext; From 68287bf64f8227030ae9fa3b5003e5e6cf0ca71b Mon Sep 17 00:00:00 2001 From: Brandon Edwards Date: Mon, 12 Oct 2026 19:31:19 -0700 Subject: [PATCH 6/6] Adding test that an inactive user with the same loginid as an active user is not returned --- test/Classes/UserHelperGetByLoginIdTests.cs | 41 +++++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/test/Classes/UserHelperGetByLoginIdTests.cs b/test/Classes/UserHelperGetByLoginIdTests.cs index e4384621a..a4d4535fa 100644 --- a/test/Classes/UserHelperGetByLoginIdTests.cs +++ b/test/Classes/UserHelperGetByLoginIdTests.cs @@ -35,13 +35,16 @@ public void Dispose() _context.Dispose(); } - private void AddUser(int current = 1, int future = 0) + private const int ActiveUserId = 1; + private const string ActiveMothraId = "00012345"; + + private void AddUser(int current = 1, int future = 0, int aaudUserId = ActiveUserId, string mothraId = ActiveMothraId) { _context.AaudUsers.Add(new AaudUser { - AaudUserId = 1, - ClientId = "C1", - MothraId = "00012345", + AaudUserId = aaudUserId, + ClientId = "C" + aaudUserId, + MothraId = mothraId, LoginId = LoginId, FirstName = "Jane", LastName = "Doe", @@ -71,7 +74,7 @@ public void GetByLoginId_ReturnsCurrentOrFutureUser(int current, int future) var result = new UserHelper().GetByLoginId(_context, LoginId); Assert.NotNull(result); - Assert.Equal(LoginId, result.LoginId); + Assert.Equal(ActiveUserId, result.AaudUserId); } [Fact] @@ -82,6 +85,30 @@ public void GetByLoginId_ReturnsNullForInactiveUser() Assert.Null(new UserHelper().GetByLoginId(_context, LoginId)); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void GetByLoginId_PrefersActiveUserOverFormerUserWithSameLoginId(bool formerInsertedFirst) + { + // Both orders, so the active-flag filter is proven rather than the row order. + if (formerInsertedFirst) + { + AddUser(current: 0, future: 0, aaudUserId: 2, mothraId: "00099999"); + AddUser(); + } + else + { + AddUser(); + AddUser(current: 0, future: 0, aaudUserId: 2, mothraId: "00099999"); + } + + var result = new UserHelper().GetByLoginId(_context, LoginId); + + Assert.NotNull(result); + Assert.Equal(ActiveUserId, result.AaudUserId); + Assert.Equal(ActiveMothraId, result.MothraId); + } + [Fact] public void GetByLoginId_ServesFoundUserFromCache() { @@ -91,7 +118,7 @@ public void GetByLoginId_ServesFoundUserFromCache() RemoveUsers(); - Assert.NotNull(helper.GetByLoginId(_context, LoginId)); + Assert.Equal(ActiveUserId, helper.GetByLoginId(_context, LoginId)?.AaudUserId); } [Fact] @@ -103,7 +130,7 @@ public void GetByLoginId_DoesNotCacheMiss() // A user who becomes active after a failed lookup must not be locked out by a cached null. AddUser(); - Assert.NotNull(helper.GetByLoginId(_context, LoginId)); + Assert.Equal(ActiveUserId, helper.GetByLoginId(_context, LoginId)?.AaudUserId); } [Fact]