diff --git a/eFormAPI/eFormAPI.Web.Integration.Tests/Services/AuthServiceDisabledAccountTests.cs b/eFormAPI/eFormAPI.Web.Integration.Tests/Services/AuthServiceDisabledAccountTests.cs new file mode 100644 index 0000000000..b9609e3881 --- /dev/null +++ b/eFormAPI/eFormAPI.Web.Integration.Tests/Services/AuthServiceDisabledAccountTests.cs @@ -0,0 +1,257 @@ +/* +The MIT License (MIT) + +Copyright (c) 2007 - 2026 Microting A/S + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. +*/ + +using System.Collections.Generic; +using System.Threading.Tasks; +using eFormAPI.Web.Abstractions; +using eFormAPI.Web.Abstractions.Security; +using eFormAPI.Web.Hosting.Helpers.DbOptions; +using eFormAPI.Web.Services; +using eFormAPI.Web.Services.Cache.AuthCache; +using Microsoft.AspNetCore.Identity; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Microting.eFormApi.BasePn.Abstractions; +using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; +using Microting.eFormApi.BasePn.Infrastructure.Models.Application; +using Microting.eFormApi.BasePn.Infrastructure.Models.Auth; +using NSubstitute; +using NUnit.Framework; + +namespace eFormAPI.Web.Integration.Tests.Services +{ + /// + /// A disabled account (EformUser.IsActive == false) must be refused, and must answer + /// with the same message as an account that does not exist — otherwise the login box + /// tells any visitor which emails have accounts. That sameness is the requirement, so + /// it is asserted by comparing the responses to each other rather than by checking each + /// message in isolation. + /// + /// Pure unit tests: every collaborator is a substitute and nothing touches the + /// database, so this fixture deliberately does not inherit DbTestFixture. + /// + [TestFixture] + public class AuthServiceDisabledAccountTests + { + private const string Password = "correct-horse-battery-staple"; + private const string GenericMessageKey = "UserNameOrPasswordIncorrect"; + + private IUserService _userService; + private SignInManager _signInManager; +#pragma warning disable NUnit1032 + private UserManager _userManager; +#pragma warning restore NUnit1032 + private AuthService _authService; + + [SetUp] + public void Setup() + { + var tokenOptions = Substitute.For>(); + var appSettings = Substitute.For>(); + var localizationService = Substitute.For(); + + _userService = Substitute.For(); + _signInManager = Substitute.For>( + Substitute.For>( + Substitute.For>(), null, null, null, null, null, null, null, null), + Substitute.For(), + Substitute.For>(), + null, null, null, null); + _userManager = Substitute.For>( + Substitute.For>(), null, null, null, null, null, null, null, null); + + // The substitute echoes the key back, so an assertion naming the key proves the + // localised resource is used rather than a hard-coded literal. + localizationService.GetString(Arg.Any()).Returns(args => args.Arg()); + // A signing key, so the successful-login test can get as far as minting a + // token. The value is irrelevant, only that token generation does not throw. + tokenOptions.Value.Returns(new EformTokenOptions + { + SigningKey = "test-signing-key-that-is-long-enough-for-hmac-sha256", + Issuer = "tests" + }); + appSettings.Value.Returns(new ApplicationSettings()); + + _authService = new AuthService( + tokenOptions, + Substitute.For>(), + appSettings, + Substitute.For>( + Substitute.For>(), null, null, null, null), + _signInManager, + _userManager, + _userService, + localizationService, + Substitute.For(), + Substitute.For()); + } + + private static EformUser User(bool isActive) => new() + { + Id = 42, + UserName = "someone@example.com", + Email = "someone@example.com", + EmailConfirmed = true, + IsActive = isActive + }; + + private static LoginModel Login() => new() + { + Username = "someone@example.com", + Password = Password + }; + + private void GivenUser(EformUser user) => + _userService.GetByUsernameAsync(Arg.Any()).Returns(user); + + private void GivenSignInResult(SignInResult result) => + _signInManager.CheckPasswordSignInAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(result); + + [Test] + public async Task AuthenticateUser_DisabledAccount_IsRefused() + { + GivenUser(User(isActive: false)); + GivenSignInResult(SignInResult.Success); + + var result = await _authService.AuthenticateUser(Login()); + + Assert.That(result.Success, Is.False, "a disabled account must not be able to log in"); + Assert.That(result.Message, Is.EqualTo(GenericMessageKey)); + } + + [Test] + public async Task AuthenticateUser_DisabledAccount_IsIndistinguishableFromAnUnknownOne() + { + GivenUser(User(isActive: false)); + GivenSignInResult(SignInResult.Success); + var disabled = await _authService.AuthenticateUser(Login()); + + GivenUser(null); + var unknown = await _authService.AuthenticateUser(Login()); + + Assert.That(disabled.Success, Is.EqualTo(unknown.Success)); + Assert.That(disabled.Message, Is.EqualTo(unknown.Message), + "a disabled account must not be distinguishable from one that does not exist"); + } + + [Test] + public async Task AuthenticateUser_WrongPassword_GivesTheSameAnswerAsAnUnknownAccount() + { + GivenUser(User(isActive: true)); + GivenSignInResult(SignInResult.Failed); + var wrongPassword = await _authService.AuthenticateUser(Login()); + + GivenUser(null); + var unknown = await _authService.AuthenticateUser(Login()); + + Assert.That(wrongPassword.Message, Is.EqualTo(unknown.Message), + "the login box must not reveal which usernames exist"); + Assert.That(wrongPassword.Message, Is.EqualTo(GenericMessageKey)); + } + + [Test] + public async Task AuthenticateUser_UnknownAccount_DoesNotEchoTheSubmittedUsername() + { + GivenUser(null); + + var result = await _authService.AuthenticateUser(Login()); + + Assert.That(result.Message, Does.Not.Contain("someone@example.com"), + "the response must not repeat what was typed into the login box"); + } + + [Test] + public async Task AuthenticateUser_LockedOut_GivesTheSameAnswerAsAnUnknownAccount() + { + GivenUser(User(isActive: true)); + GivenSignInResult(SignInResult.LockedOut); + var lockedOut = await _authService.AuthenticateUser(Login()); + + GivenUser(null); + var unknown = await _authService.AuthenticateUser(Login()); + + // Only an existing, active account can reach lockout, so a distinct message + // here would tell an anonymous caller that an address has a live account. + Assert.That(lockedOut.Message, Is.EqualTo(unknown.Message), + "a locked-out account must not be distinguishable from one that does not exist"); + Assert.That(lockedOut.Message, Is.EqualTo(GenericMessageKey)); + } + + [Test] + public async Task AuthenticateUser_ActiveAccountWithTheRightPassword_Succeeds() + { + var user = User(isActive: true); + GivenUser(user); + GivenSignInResult(SignInResult.Success); + _userManager.GetRolesAsync(user).Returns(new List { "admin" }); + _userService.GetFirstUserIdInDb().Returns(user.Id); + + var result = await _authService.AuthenticateUser(Login()); + + Assert.That(result.Success, Is.True, + "an active account with the right password must still be able to log in"); + } + + [Test] + public async Task AuthenticateUser_DisabledAccount_StillVerifiesThePasswordFirst() + { + GivenUser(User(isActive: false)); + GivenSignInResult(SignInResult.Success); + + await _authService.AuthenticateUser(Login()); + + // Refusing before the hash would answer faster than a wrong password does, + // which is a timing oracle, and would also exempt disabled accounts from + // lockout counting. + await _signInManager.Received().CheckPasswordSignInAsync( + Arg.Any(), Arg.Any(), Arg.Any()); + } + + [Test] + public async Task GetGoogleAuthenticator_DisabledAccount_IsRefused() + { + _userManager.FindByNameAsync(Arg.Any()).Returns(User(isActive: false)); + + var result = await _authService.GetGoogleAuthenticator(Login()); + + Assert.That(result.Success, Is.False, + "the anonymous 2FA-key endpoint confirms a credential, so it must refuse too"); + Assert.That(result.Message, Is.EqualTo(GenericMessageKey)); + } + + [Test] + public async Task RefreshToken_DisabledAccount_IsRefused() + { + _userService.UserId.Returns(42); + _userService.GetByIdAsync(Arg.Any()).Returns(User(isActive: false)); + + var result = await _authService.RefreshToken(); + + Assert.That(result.Success, Is.False, + "a disabled account must not be able to roll its session forward"); + Assert.That(result.Message, Is.EqualTo(GenericMessageKey)); + } + } +} diff --git a/eFormAPI/eFormAPI.Web/Resources/SharedResource.da.resx b/eFormAPI/eFormAPI.Web/Resources/SharedResource.da.resx index 0dcafcb112..e87c34b487 100644 --- a/eFormAPI/eFormAPI.Web/Resources/SharedResource.da.resx +++ b/eFormAPI/eFormAPI.Web/Resources/SharedResource.da.resx @@ -324,7 +324,7 @@ Fejl under oprettelse af enhed - Brugernavn eller adgangskode er forkert + Du har indtastet et ugyldigt brugernavn eller adgangskode Bruger ikke fundet diff --git a/eFormAPI/eFormAPI.Web/Resources/SharedResource.resx b/eFormAPI/eFormAPI.Web/Resources/SharedResource.resx index 04121a9274..5c35442e43 100644 --- a/eFormAPI/eFormAPI.Web/Resources/SharedResource.resx +++ b/eFormAPI/eFormAPI.Web/Resources/SharedResource.resx @@ -382,7 +382,7 @@ Error while creating unit - Username or password is incorrect + You have entered an invalid username or password User not found diff --git a/eFormAPI/eFormAPI.Web/Services/AuthService.cs b/eFormAPI/eFormAPI.Web/Services/AuthService.cs index 254e080443..343c08cf79 100644 --- a/eFormAPI/eFormAPI.Web/Services/AuthService.cs +++ b/eFormAPI/eFormAPI.Web/Services/AuthService.cs @@ -72,23 +72,28 @@ public async Task> AuthenticateUser(Lo return new OperationDataResult(false, "Empty username or password"); var user = await userService.GetByUsernameAsync(model.Username); + if (user == null) - return new OperationDataResult(false, - $"User with username {model.Username} not found"); + { + return InvalidCredentialsResult(); + } var signInResult = await signInManager.CheckPasswordSignInAsync(user, model.Password, true); - if (!signInResult.Succeeded && !signInResult.RequiresTwoFactor) + // Deliberately after the password check, not before. Checking IsActive first would + // be cheaper, but it would answer without computing the password hash, putting + // disabled accounts in the same fast bucket as accounts that do not exist while a + // wrong password takes tens of milliseconds - a timing oracle. It also keeps + // lockout counting identical for disabled accounts; see InvalidCredentialsResult. + if (!user.IsActive) { - if (signInResult.IsLockedOut) - { - return new OperationDataResult(false, - "Locked Out. Please, try again after 10 min"); - } + return InvalidCredentialsResult(); + } - // Credentials are invalid, or account doesn't exist - return new OperationDataResult(false, "Incorrect password."); + if (!signInResult.Succeeded && !signInResult.RequiresTwoFactor) + { + return InvalidCredentialsResult(); } // Confirmed email check @@ -173,9 +178,14 @@ public async Task> AuthenticateUser(Lo public async Task> RefreshToken() { var user = await userService.GetByIdAsync(userService.UserId); - if (user == null) - return new OperationDataResult(false, - $"User with id {userService.UserId} not found"); + + // Refusing here matters as much as refusing at login: this endpoint mints a fresh + // 24h token from any still-valid one, so without the IsActive check a disabled + // account could roll its session forward indefinitely. + if (user == null || !user.IsActive) + { + return InvalidCredentialsResult(); + } var token = await GenerateToken(user); var roleList = await userManager.GetRolesAsync(user); @@ -409,26 +419,21 @@ public async Task> GetGoogleAuthen { // try to sign in with user credentials var user = await userManager.FindByNameAsync(loginModel.Username); + + // This endpoint is anonymous and confirms a username/password pair, so a disabled + // account has to be refused here too, or it stays a working credential oracle. if (user == null) { - return new OperationDataResult(false, - localizationService.GetString("UserNameOrPasswordIncorrect")); + return InvalidCredentialsResult(); } var signInResult = await signInManager.CheckPasswordSignInAsync(user, loginModel.Password, true); - if (!signInResult.Succeeded) + // After the password check, for the timing reason given in AuthenticateUser. + if (!user.IsActive || !signInResult.Succeeded) { - if (signInResult.IsLockedOut) - { - return new OperationDataResult(false, - "Locked Out. Please, try again after 10 min"); - } - - // Credentials are invalid, or account doesn't exist - return new OperationDataResult(false, - localizationService.GetString("UserNameOrPasswordIncorrect")); + return InvalidCredentialsResult(); } // check if two factor is enabled @@ -463,4 +468,18 @@ public async Task> GetGoogleAuthen // return return new OperationDataResult(true, model); } + + /// + /// The single answer every credential failure gives: account unknown, password wrong, + /// account disabled, or locked out. Telling them apart is what turns a login box into a + /// list of which emails have accounts. + /// + /// Lockout is included deliberately, even though its own message would be friendlier. + /// Only an existing, active account can ever reach the lockout state, so a distinct + /// lockout message is an enumeration primitive: eleven anonymous requests with a wrong + /// password tell you whether an address has a live account. + /// + private OperationDataResult InvalidCredentialsResult() => + new(false, localizationService.GetString("UserNameOrPasswordIncorrect")); + } \ No newline at end of file diff --git a/eFormAPI/eFormAPI.Web/eFormAPI.Web.csproj b/eFormAPI/eFormAPI.Web/eFormAPI.Web.csproj index 1b39d11300..205b3505c7 100644 --- a/eFormAPI/eFormAPI.Web/eFormAPI.Web.csproj +++ b/eFormAPI/eFormAPI.Web/eFormAPI.Web.csproj @@ -56,8 +56,8 @@ - - + +