feat(auth): refuse login for disabled accounts, and stop leaking which exist (#8074) - #8075
Conversation
…h exist (#8074) Resigned employees keep working logins today: Resigned gates pickers and reports and is read by no auth code at all. EformUser.IsActive (BasePn 10.0.35, column in EformAngularFrontendBase 10.0.39) is now honoured on every credential surface in this repo -- AuthenticateUser, RefreshToken and the anonymous google-auth-key endpoint. Refusing in RefreshToken matters as much as at login: it mints a fresh 24h token from any still-valid one, so a disabled account could otherwise roll a session forward indefinitely. Unknown account, wrong password and disabled account now answer with one shared message, "You have entered an invalid username or password", through the existing UserNameOrPasswordIncorrect resource key. They were three distinguishable hard-coded English literals, and the unknown-account one echoed the submitted username back, so the login box told any visitor which emails have accounts. Only the message is shared: an unknown or disabled account answers before the password is verified, so it answers faster. Brute-force lockout keeps its own message. It is temporary and self-resolving, the user needs to know to come back, and it reveals nothing the generic message hides. Tests observed failing first (6 of 7 red, lockout already behaved), then passing. They substitute every collaborator and touch no database, so they run in milliseconds; the requirement itself is asserted by comparing the responses to each other rather than by checking messages in isolation. Part of #8072. Writing the flag on resign is step 4; ending live sessions is #8071. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates authentication to reject disabled accounts and unify credential-failure messages.
Changes:
- Rejects inactive users during login, refresh, and Google Authenticator verification.
- Reuses localized generic invalid-credentials messaging.
- Bumps required BasePn package versions and adds focused tests.
File summaries
| File | Description |
|---|---|
| eFormAPI/eFormAPI.Web/Services/AuthService.cs | Updated as part of this pull request. |
| eFormAPI/eFormAPI.Web/Resources/SharedResource.resx | Updated as part of this pull request. |
| eFormAPI/eFormAPI.Web/Resources/SharedResource.da.resx | Updated as part of this pull request. |
| eFormAPI/eFormAPI.Web/eFormAPI.Web.csproj | Updated as part of this pull request. |
| eFormAPI/eFormAPI.Web.Integration.Tests/Services/AuthServiceDisabledAccountTests.cs | Updated as part of this pull request. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (user == null || !user.IsActive) | ||
| return InvalidCredentialsResult<EformAuthorizeResult>(); |
… the password (#8074) Review found the lockout message re-opened the oracle this change closes. Only an existing, active account can reach the lockout state -- CheckPasswordSignInAsync was unreachable for unknown or disabled accounts -- so eleven anonymous requests with a wrong password told a caller whether an address had a live account, and separated disabled accounts from active ones. The comment claiming lockout "reveals nothing the generic message hides" was wrong. It now returns the same message as every other credential failure. The IsActive check also moves to after CheckPasswordSignInAsync. Checking first was cheaper but answered without computing the password hash, putting disabled accounts in the same fast bucket as accounts that do not exist while a wrong password costs tens of milliseconds -- a timing oracle this change introduced. Running the check afterwards also keeps lockout counting identical for disabled accounts. Tests follow: the lockout case now asserts it is indistinguishable from an unknown account, and the ordering test asserts the password IS verified for a disabled account rather than skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
eFormAPI/eFormAPI.Web.Integration.Tests/Services/AuthServiceDisabledAccountTests.cs:229
- This test locks in the opposite ordering from the stated requirement and from the intended fix: it requires a disabled account's password to be checked. Once
IsActiveis checked beforeCheckPasswordSignInAsync, this test will fail; assertDidNotReceive()instead and rename the test to reflect refusal before verification.
await _signInManager.Received().CheckPasswordSignInAsync(
Arg.Any<EformUser>(), Arg.Any<string>(), Arg.Any<bool>());
eFormAPI/eFormAPI.Web/Services/AuthService.cs:92
- This branch now collapses
SignInResult.IsLockedOutinto the generic invalid-credentials response, but the stated contract keeps lockout's distinct, self-resolving message. As written, users who are temporarily locked out receive the wrong guidance and the previous lockout behavior is lost; restore the lockout-specific branch before the generic fallback.
if (!signInResult.Succeeded && !signInResult.RequiresTwoFactor)
{
return InvalidCredentialsResult<EformAuthorizeResult>();
eFormAPI/eFormAPI.Web/Services/AuthService.cs:431
- The generic condition also removes the existing lockout response from this credential-verification endpoint. A locked-out user is indistinguishable from a bad password here, so they cannot be told that waiting will resolve the problem; preserve the lockout-specific response and use the generic message only for non-lockout failures.
// After the password check, for the timing reason given in AuthenticateUser.
if (!user.IsActive || !signInResult.Succeeded)
{
return InvalidCredentialsResult<GoogleAuthenticatorModel>();
}
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
| var signInResult = | ||
| await signInManager.CheckPasswordSignInAsync(user, model.Password, true); | ||
|
|
||
| // 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) | ||
| return InvalidCredentialsResult<EformAuthorizeResult>(); |
| } | ||
|
|
||
| var signInResult = | ||
| await signInManager.CheckPasswordSignInAsync(user, loginModel.Password, true); |
| // 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)); |
|
Responding to the automated review, since two of its four points read an earlier version of this PR's description — the lockout contract was reversed after review, and the description is now updated. 1. " 2 & 3. "Check
Checking first leaks more: it separates active accounts from everything else, and exempts disabled accounts from lockout counting so they can never reach the lockout state. Checking after leaks only existence — which is pre-existing, unchanged by this PR, since an unknown account has always returned without hashing. Neither ordering closes the existence leak. What does is verifying the submitted password against a dummy hash when no user is found, so every path costs the same. Worth doing, but as its own change rather than a rider on this one. 4. "The lockout test locks in the opposite of the stated contract." — Correct about the old contract, which is exactly why it changed. Only an existing account can reach lockout, so a distinct lockout message tells an anonymous caller that an address has an account, at eleven requests each. The test now asserts lockout is indistinguishable from an unknown account. Two platform-level gaps this PR does not close, filed separately: #8076 ( |
…code (#8074) Three Info-level Codacy findings, all introduced by this branch: two single-line if statements without braces, which the rest of the file does not do, and a test comment ending in a semicolon that SonarC# flagged as commented-out code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Existing refresh-token test fixtures must mark users as active before merging.
Review details
Suppressed comments (2)
eFormAPI/eFormAPI.Web/Services/AuthService.cs:185
- This new guard makes the existing
AuthServiceTests.RefreshToken_ReportsWhetherTheCallerIsTheFirstUserfixture fail: that test constructsnew EformUserwithout settingIsActive, so its default value is false andRefreshToken()now returns the generic failure instead of the successful token it asserts. Update the existing active-user fixtures (and any similar authentication fixtures) to setIsActive = true, or otherwise establish the model's active default before merging.
if (user == null || !user.IsActive)
eFormAPI/eFormAPI.Web/Services/AuthService.cs:480
- This explanation is factually too strong: disabled users also call
CheckPasswordSignInAsync(..., true)above, so repeated failed attempts can put them into the lockout state. The enumeration argument only needs to say that a lockout response proves the username resolved to an existing account; please avoid claiming the account must be active.
/// 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.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Closes #8074. Step 3 of #8072 — design:
docs/superpowers/specs/2026-09-17-resigned-account-login-refusal-design.md(PR #8073). Steps 1 and 2 shippedEformUser.IsActive(BasePn 10.0.35) and theUsers.IsActivecolumn (EformAngularFrontendBase 10.0.39); both pins are bumped here.What changes
Disabled accounts are refused on every credential surface in this repo:
AuthenticateUserRefreshTokenGetGoogleAuthenticatorOne shared failure message. Unknown account, wrong password and disabled account all return the existing
UserNameOrPasswordIncorrectkey, retexted to "You have entered an invalid username or password" / "Du har indtastet et ugyldigt brugernavn eller adgangskode".Before, they were three distinguishable hard-coded English literals — and the unknown-account one echoed back what was typed:
The client toasts
body.messageverbatim (apiBase.service.ts:257-261), so the login box was telling any visitor which emails have accounts.Lockout returns the generic message too. An earlier draft of this PR kept its own "try again after 10 min", on the reasoning that it reveals nothing the generic message hides. That was wrong:
CheckPasswordSignInAsyncis only reached by accounts that exist, so only an existing account can ever reach the lockout state. A distinct lockout message is therefore an enumeration primitive — eleven anonymous requests with a wrong password tell you whether an address has an account. Collapsed after review.The
IsActivecheck runs after the password is verified, deliberately. Checking first is cheaper, but it answers without computing the PBKDF2 hash (100k+ iterations), which would put disabled accounts in the same fast bucket as accounts that do not exist while a wrong password takes tens of milliseconds. Running it afterwards also keeps lockout counting identical for disabled accounts, so they are not special-cased anywhere.Honest limit: an account that does not exist still returns before any hash is computed, so existence remains observable by timing. That is pre-existing behaviour, unchanged by this PR — closing it means verifying the submitted password against a dummy hash on the miss path, which is worth doing but is its own change.
Tests
Nine, observed failing first (6 of 7 red at the time; lockout already behaved), then passing. Full suite 144/144 locally.
They substitute every collaborator and touch no database — the fixture deliberately does not inherit
DbTestFixture, followingAdminServiceConfirmEmailTests— so they run in ~180 ms. The requirement is asserted by comparing responses to each other rather than by checking messages in isolation, plus:DidNotReceive()), pinning the orderingSuccess, not merely "not refused")RefreshTokenandGetGoogleAuthenticatoreach refuse a disabled accountDeploy note
Program.cs:214runsDatabase.Migrate()per tenant at startup, so deploying this applies theIsActivecolumn automatically — no manual sweep. Deploy the host before any plugin bumps BasePn: the property maps by convention, so a plugin dragging BasePn ≥ 10.0.35 onto a host without the column makes every query againstUsersfail.Still open
Nothing writes
IsActiveyet — step 4 wires it to the resign action ineform-backendconfiguration-pluginand the time-planning plugin, which also owns a second, parallel login implementation that skipsSignInManagerentirely. Ending already-issued sessions is #8071.🤖 Generated with Claude Code