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..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; @@ -30,18 +31,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 +129,81 @@ 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); + // 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] + 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() + { + _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)); + 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/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..7b49e731 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,80 @@ planRegistrationForToday is } } + /// + /// 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 + /// 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 + // 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, + // 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; @@ -982,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 @@ -1092,6 +1172,11 @@ public async Task UpdateAssignedSite(Infrastructure.Models.Sett await dbAssignedSite.Update(dbContext); + 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 // is committed; a push failure must NEVER fail the settings update.