From 182b08ce57ddc49fce78cfd0a171cd61805716c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Thu, 17 Sep 2026 16:34:58 +0200 Subject: [PATCH 1/2] feat(auth): disable the login when a worker is resigned here, and refuse disabled accounts Two gaps in the same chain, both in this plugin. Resigning from the time-planning settings screen wrote AssignedSite.Resigned and touched no login at all, so the person kept an account that still signs in -- Resigned is a visibility flag no authentication code reads. UpdateAssignedSite now syncs EformUser.IsActive after the settings row is committed, resolving the worker through Sites -> SiteWorkers -> Workers and matching the address the same way the avatar lookup in this file already does. ExecuteUpdate writes the one column: UserManager would run Identity's validators against addresses part of this population cannot satisfy. A sync failure is logged and captured rather than failing the save, and the case that matters -- no login row matched -- is a warning rather than a zero buried in a success line. TimePlanningAuthGrpcService is a second, parallel login that mints the same JWT as core's REST path, and it had no IsActive check: without this a resigned employee's flutter-time app keeps working regardless of everything else. It now refuses disabled accounts on login and on refresh, checked after the password comparison so a disabled account does not answer faster than a wrong one. The three messages that distinguished unknown account from wrong password are collapsed into one, matching what core now returns; the class remarks that advertised the old strings are corrected. Core must be deployed before this plugin: EformUser resolves from the host's assemblies, so IsActive is a missing member on an older host. Part of microting/eform-angular-frontend#8072. Lockout parity and delegating this login to core's IAuthService stay open in microting/eform-angular-frontend#8077. Co-Authored-By: Claude Opus 5 --- .../TimePlanningAuthGrpcServiceTests.cs | 88 +++++++++++++++++-- .../TimePlanningAuthGrpcService.cs | 33 +++++-- .../TimeSettingService.cs | 70 +++++++++++++++ 3 files changed, 179 insertions(+), 12 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs index dfd02900..5927cd54 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs @@ -30,18 +30,22 @@ public class TimePlanningAuthGrpcServiceTests public void SetUp() { _deviceService = Substitute.For(); - // RefreshToken-related deps are not exercised by ActivateDevice tests; - // null-pass these. UserManager/RoleManager have no usable interface to - // substitute against, so we pass null — any test that exercises - // RefreshToken would need to construct real instances. + // RoleManager is only reached once a login succeeds, which no test here does, so + // it stays null. UserManager substitutes fine through its virtual members. _userService = Substitute.For(); - _userManager = null; + _userManager = SubstituteUserManager(); _roleManager = null; _tokenOptions = Substitute.For>(); _grpcService = new TimePlanningAuthGrpcService( _deviceService, _userService, _userManager, _roleManager, _tokenOptions); } + [TearDown] + public void TearDown() + { + _userManager?.Dispose(); + } + [Test] public async Task ActivateDevice_Success_ReturnsToken() { @@ -124,4 +128,78 @@ public async Task ActivateDevice_InvalidCustomerNo_DefaultsToZero() await _deviceService.Received(1).Activate( Arg.Is(m => m.CustomerNo == 0)); } + + // This service is a second, parallel login implementation that mints the same JWT as + // the JSON path, so a disabled account has to be refused here too - otherwise a + // resigned employee's flutter-time app keeps working. Every credential failure answers + // with one message, for the same reason the JSON path does. + // Spelled out rather than referenced from the production constant: asserting against + // the constant would pass however the message changed. + private const string ExpectedMessage = "You have entered an invalid username or password"; + + private static UserManager SubstituteUserManager() => + Substitute.For>( + Substitute.For>(), null, null, null, null, null, null, null, null); + + private TimePlanningAuthGrpcService ServiceWith(UserManager userManager) => + new(_deviceService, _userService, userManager, _roleManager, _tokenOptions); + + private static EformUser DisabledUser() => new() + { + Id = 42, + UserName = "someone@example.com", + Email = "someone@example.com", + EmailConfirmed = true, + IsActive = false + }; + + [Test] + public async Task AuthenticateUser_DisabledAccount_IsRefused() + { + var userManager = SubstituteUserManager(); + userManager.FindByNameAsync(Arg.Any()).Returns(DisabledUser()); + userManager.CheckPasswordAsync(Arg.Any(), Arg.Any()).Returns(true); + + var response = await ServiceWith(userManager).AuthenticateUser( + new AuthenticateUserRequest { Username = "someone@example.com", Password = "right" }, TestServerCallContextFactory.Create()); + + Assert.That(response.Success, Is.False, "a disabled account must not be able to log in"); + Assert.That(response.Message, Is.EqualTo(ExpectedMessage)); + } + + [Test] + public async Task AuthenticateUser_DisabledAccount_ReturnsSameMessageAsUnknownAccount() + { + var disabledManager = SubstituteUserManager(); + disabledManager.FindByNameAsync(Arg.Any()).Returns(DisabledUser()); + disabledManager.CheckPasswordAsync(Arg.Any(), Arg.Any()).Returns(true); + var disabled = await ServiceWith(disabledManager).AuthenticateUser( + new AuthenticateUserRequest { Username = "someone@example.com", Password = "right" }, TestServerCallContextFactory.Create()); + + var unknownManager = SubstituteUserManager(); + unknownManager.FindByNameAsync(Arg.Any()).Returns((EformUser)null!); + unknownManager.FindByEmailAsync(Arg.Any()).Returns((EformUser)null!); + var unknown = await ServiceWith(unknownManager).AuthenticateUser( + new AuthenticateUserRequest { Username = "someone@example.com", Password = "right" }, TestServerCallContextFactory.Create()); + + Assert.That(disabled.Message, Is.EqualTo(unknown.Message), + "a disabled account must not be distinguishable from one that does not exist"); + Assert.That(unknown.Message, Does.Not.Contain("someone@example.com"), + "the response must not repeat what was typed"); + } + + [Test] + public async Task RefreshToken_DisabledAccount_IsRefused() + { + // The refusal returns before the token is minted, so the null UserManager this + // fixture passes is never reached. + _userService.UserId.Returns(42); + _userService.GetByIdAsync(Arg.Any()).Returns(DisabledUser()); + + var response = await _grpcService.RefreshToken(new RefreshTokenRequest(), TestServerCallContextFactory.Create()); + + Assert.That(response.Success, Is.False, + "a disabled account must not be able to roll its session forward"); + Assert.That(response.Message, Is.EqualTo(ExpectedMessage)); + } } diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningAuthGrpcService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningAuthGrpcService.cs index 3f102e21..e5fb25da 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningAuthGrpcService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/GrpcServices/TimePlanningAuthGrpcService.cs @@ -29,6 +29,16 @@ public class TimePlanningAuthGrpcService : TimePlanningAuthService.TimePlanningA private readonly RoleManager _roleManager; private readonly IOptions _tokenOptions; + /// + /// The one answer every credential failure gives: unknown account, wrong password or + /// disabled account. Telling them apart turns a login into a list of which addresses + /// have accounts. The text matches core's UserNameOrPasswordIncorrect resource, which + /// a plugin cannot reach - microting/eform-angular-frontend#8077 tracks closing that + /// gap properly. + /// + private const string InvalidCredentialsMessage = + "You have entered an invalid username or password"; + public TimePlanningAuthGrpcService( ITimePlanningRegistrationDeviceService registrationDeviceService, IUserService userService, @@ -91,8 +101,10 @@ public override async Task ActivateDevice( /// plugin-accessible. The next REST call rebuilds the cache lazily. /// /// Failure messages mirror the JSON oracle so the contract diff stays - /// shape-clean: "Empty username or password", "User with username X not - /// found", "Incorrect password.", "Email X not confirmed". + /// shape-clean - including its refusal to say WHICH credential was wrong: + /// unknown account, wrong password and disabled account all answer with + /// InvalidCredentialsMessage. "Empty username or password" and + /// "Email X not confirmed" stay distinct, as they do there. /// public override async Task AuthenticateUser( AuthenticateUserRequest request, ServerCallContext context) @@ -118,17 +130,21 @@ public override async Task AuthenticateUser( return new AuthenticateUserResponse { Success = false, - Message = $"User with username {request.Username} not found" + Message = InvalidCredentialsMessage }; } var passwordOk = await _userManager.CheckPasswordAsync(user, request.Password); - if (!passwordOk) + + // Checked after the password, not before: answering earlier for a disabled + // account would answer faster than a wrong password does, which is a timing + // oracle. This mirrors the JSON path in core's AuthService. + if (!passwordOk || !user.IsActive) { return new AuthenticateUserResponse { Success = false, - Message = "Incorrect password." + Message = InvalidCredentialsMessage }; } @@ -205,12 +221,15 @@ public override async Task RefreshToken( try { var user = await _userService.GetByIdAsync(_userService.UserId); - if (user == null) + + // Without this a disabled account rolls its session forward indefinitely: + // this endpoint mints a fresh token from any still-valid one. + if (user == null || !user.IsActive) { return new RefreshTokenResponse { Success = false, - Message = $"User with id {_userService.UserId} not found" + Message = InvalidCredentialsMessage }; } diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs index c5d78afb..2986e038 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs @@ -966,6 +966,74 @@ planRegistrationForToday is } } + /// + /// Resigning here must disable the person's login, exactly as resigning through the + /// device-users screen does. Resigned is only a visibility flag - no authentication + /// code reads it - so without this a worker resigned from time-planning keeps an + /// account that still signs in, including the flutter apps. Written with ExecuteUpdate + /// rather than UserManager, which would run Identity's validators against addresses + /// part of this population cannot satisfy (non-ASCII local parts). + /// + private async Task SyncLoginStateAsync(int sdkSiteMicrotingUid, bool resigned) + { + try + { + var sdkCore = await core.GetCore().ConfigureAwait(false); + var sdkDbContext = sdkCore.DbContextHelper.GetDbContext(); + + var email = await ( + from s in sdkDbContext.Sites + join sw in sdkDbContext.SiteWorkers on s.Id equals sw.SiteId + join w in sdkDbContext.Workers on sw.WorkerId equals w.Id + where s.MicrotingUid == sdkSiteMicrotingUid + && s.WorkflowState != Constants.WorkflowStates.Removed + && sw.WorkflowState != Constants.WorkflowStates.Removed + && w.WorkflowState != Constants.WorkflowStates.Removed + select w.Email).FirstOrDefaultAsync().ConfigureAwait(false); + + // Matched the same way the avatar lookup above does it - the two must agree, + // or that lookup finds a login this one misses. + var workerEmail = (email ?? "").Trim().ToLower(); + if (string.IsNullOrEmpty(workerEmail)) + { + logger.LogWarning( + "No worker email for site {SdkSiteId}; Resigned={Resigned} saved, no login state written", + sdkSiteMicrotingUid, resigned); + return; + } + + var isActive = !resigned; + var affected = await baseDbContext.Users + .Where(x => x.Email.ToLower() == workerEmail) + .ExecuteUpdateAsync(x => x.SetProperty(u => u.IsActive, isActive)) + .ConfigureAwait(false); + + if (affected == 0) + { + // The resignation did not reach a login: the worker's address matches no + // account. That is the failure this method exists to report. + logger.LogWarning( + "No login row matched the worker of site {SdkSiteId}; IsActive={IsActive} not written", + sdkSiteMicrotingUid, isActive); + return; + } + + logger.LogInformation( + "Set IsActive={IsActive} on {Count} login(s) for site {SdkSiteId}", + isActive, affected, sdkSiteMicrotingUid); + } + catch (Exception ex) + { + // The settings row is already committed, and a resignation that does not reach + // the login is a security gap rather than a broken save - so report it and let + // the caller succeed. + SentrySdk.CaptureException(ex); + logger.LogError(ex, + "Could not sync login state for site {SdkSiteId}", + sdkSiteMicrotingUid); + } + } + public async Task UpdateAssignedSite(Infrastructure.Models.Settings.AssignedSite site) { var siteId = site.SiteId; @@ -1092,6 +1160,8 @@ public async Task UpdateAssignedSite(Infrastructure.Models.Sett await dbAssignedSite.Update(dbContext); + await SyncLoginStateAsync(dbAssignedSite.SiteId, site.Resigned).ConfigureAwait(false); + // Fire-and-forget: tell the worker's device(s) that their assigned-site // settings changed so personal mode can auto-refresh. Sent AFTER the row // is committed; a push failure must NEVER fail the settings update. From dfa6d1ca750e69162471d9e63481ddb26b3b1967 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Thu, 17 Sep 2026 16:38:47 +0200 Subject: [PATCH 2/2] fix(auth): only touch the login when the resignation changes, and resolve the worker deterministically Review findings on the previous commit. The IsActive write fired on every settings save, so an unrelated change -- a break divider, a GPS flag -- re-asserted account state. Worse, the request model binds from the body, so a payload that omits "resigned" deserializes to false: saving anything for a resigned worker would silently re-enable their login, and nothing else could ever disable an account without a settings save undoing it. The old value is now captured before the assignment and the login is only touched when the resignation actually changes. The SDK lookup used FirstOrDefault with no ordering, so a site carrying more than one live SiteWorker row resolved to whichever row the database happened to return -- and disabled the wrong person's login, permanently, with no UI to undo it. This repo already has SiteWorkerResolver for exactly that failure; the query now orders by SiteWorker id the same way. Both gRPC tests asserted Success is false, which they did anyway: without a role stubbed the method returns "Role not found" regardless, and RefreshToken hit an NRE on the null UserManager. They now stub a role and assert no token is issued, so they fail if the refusal is removed. Co-Authored-By: Claude Opus 5 --- .../TimePlanningAuthGrpcServiceTests.cs | 8 +++++-- .../TimeSettingService.cs | 21 ++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs index 5927cd54..4a8564f8 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/GrpcServices/TimePlanningAuthGrpcServiceTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Generic; using System.Threading.Tasks; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Options; @@ -159,12 +160,16 @@ public async Task AuthenticateUser_DisabledAccount_IsRefused() var userManager = SubstituteUserManager(); userManager.FindByNameAsync(Arg.Any()).Returns(DisabledUser()); userManager.CheckPasswordAsync(Arg.Any(), Arg.Any()).Returns(true); + // A role, so the call would otherwise get past every other check and mint a token - + // without this the method returns "Role ... not found" and the refusal proves nothing. + userManager.GetRolesAsync(Arg.Any()).Returns(new List { "admin" }); var response = await ServiceWith(userManager).AuthenticateUser( new AuthenticateUserRequest { Username = "someone@example.com", Password = "right" }, TestServerCallContextFactory.Create()); Assert.That(response.Success, Is.False, "a disabled account must not be able to log in"); Assert.That(response.Message, Is.EqualTo(ExpectedMessage)); + Assert.That(response.Model, Is.Null, "no token may be issued for a disabled account"); } [Test] @@ -191,8 +196,6 @@ public async Task AuthenticateUser_DisabledAccount_ReturnsSameMessageAsUnknownAc [Test] public async Task RefreshToken_DisabledAccount_IsRefused() { - // The refusal returns before the token is minted, so the null UserManager this - // fixture passes is never reached. _userService.UserId.Returns(42); _userService.GetByIdAsync(Arg.Any()).Returns(DisabledUser()); @@ -201,5 +204,6 @@ public async Task RefreshToken_DisabledAccount_IsRefused() Assert.That(response.Success, Is.False, "a disabled account must not be able to roll its session forward"); Assert.That(response.Message, Is.EqualTo(ExpectedMessage)); + Assert.That(response.Model, Is.Null, "no fresh token may be minted for a disabled account"); } } diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs index 2986e038..7b49e731 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs @@ -967,8 +967,9 @@ planRegistrationForToday is } /// - /// Resigning here must disable the person's login, exactly as resigning through the - /// device-users screen does. Resigned is only a visibility flag - no authentication + /// Resigning here must disable the person's login, as resigning through the + /// device-users screen will once its half lands + /// (microting/eform-backendconfiguration-plugin#1283). Resigned is only a visibility flag - no authentication /// code reads it - so without this a worker resigned from time-planning keeps an /// account that still signs in, including the flutter apps. Written with ExecuteUpdate /// rather than UserManager, which would run Identity's validators against addresses @@ -989,6 +990,11 @@ join w in sdkDbContext.Workers on sw.WorkerId equals w.Id && s.WorkflowState != Constants.WorkflowStates.Removed && sw.WorkflowState != Constants.WorkflowStates.Removed && w.WorkflowState != Constants.WorkflowStates.Removed + // Deterministic by the lowest SiteWorker id, for the reason + // SiteWorkerResolver documents: a site carrying more than one live + // SiteWorker row would otherwise resolve to whichever the database + // happened to return, and disable the wrong person's login. + orderby sw.Id select w.Email).FirstOrDefaultAsync().ConfigureAwait(false); // Matched the same way the avatar lookup above does it - the two must agree, @@ -1050,6 +1056,12 @@ public async Task UpdateAssignedSite(Infrastructure.Models.Sett dbAssignedSite.AllowEditOfRegistrations = site.AllowEditOfRegistrations; dbAssignedSite.AllowPersonalTimeRegistration = site.AllowPersonalTimeRegistration; dbAssignedSite.AllowAcceptOfPlannedHours = site.AllowAcceptOfPlannedHours; + // Captured before the assignment below: the login is only touched when the + // resignation actually changes. An unrelated settings save must not re-assert + // account state - not least because a body that omits "resigned" deserializes + // to false, and that would silently re-enable a disabled account. + var wasResigned = dbAssignedSite.Resigned; + dbAssignedSite.Resigned = site.Resigned; // Record WHEN one-minute intervals took effect, so every later flex // recomputation keeps pre-switch days on 5-minute rules instead of @@ -1160,7 +1172,10 @@ public async Task UpdateAssignedSite(Infrastructure.Models.Sett await dbAssignedSite.Update(dbContext); - await SyncLoginStateAsync(dbAssignedSite.SiteId, site.Resigned).ConfigureAwait(false); + if (site.Resigned != wasResigned) + { + await SyncLoginStateAsync(dbAssignedSite.SiteId, site.Resigned).ConfigureAwait(false); + } // Fire-and-forget: tell the worker's device(s) that their assigned-site // settings changed so personal mode can auto-refresh. Sent AFTER the row