feat(auth): disable the login when a worker is resigned, and refuse disabled accounts - #1717
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new login-state write lacks sufficient authorization and can miss accounts whose stored email contains whitespace.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Disables logins when workers resign and blocks disabled accounts in the time-planning gRPC authentication flow.
Changes:
- Synchronizes
EformUser.IsActiveafter assigned-site updates. - Rejects inactive users during authentication and token refresh.
- Adds tests for disabled-account behavior.
File summaries
| File | Description |
|---|---|
TimeSettingService.cs |
Syncs worker resignation status to login state. |
TimePlanningAuthGrpcService.cs |
Refuses inactive accounts and normalizes credential errors. |
TimePlanningAuthGrpcServiceTests.cs |
Tests disabled login and refresh behavior. |
Review details
Suppressed comments (1)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs:1163
- This new call makes the existing
PUT /assigned-siteendpoint an account-disable/re-enable endpoint, but the controller only requiresGetWorkingHoursand this service does not verify that the caller may change the target worker's employment status. A caller with that claim can submit anotherSiteIdwithResigned=true(or false) and change that login'sIsActive. Require the same authorization as the device-user resignation path, or validate target-site management permission before syncing the login.
var globalSettings = await dbContext.PluginConfigurationValues.AsNoTracking().FirstOrDefaultAsync(x => x.Name == "TimePlanningBaseSettings:GpsEnabled");
- Files reviewed: 3/3 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.
|
|
||
| var isActive = !resigned; | ||
| var affected = await baseDbContext.Users | ||
| .Where(x => x.Email.ToLower() == workerEmail) |
There was a problem hiding this comment.
🟡 Changes recommended
Partial settings payloads can re-enable resigned accounts, and critical synchronization and password-ordering paths lack coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs:1177
wasResignedcannot distinguish an omittedResignedJSON property from an explicitfalse, because the request model uses a non-nullablebool. For an already-resigned site, a partial settings update therefore deserializes tofalse, enters this branch, and callsSyncLoginStateAsync(..., false), re-enabling the disabled login while also clearing the resignation. Use a nullable/PATCH field or preserve the stored value whenResignedwas omitted.
if (site.Resigned != wasResigned)
{
await SyncLoginStateAsync(dbAssignedSite.SiteId, site.Resigned).ConfigureAwait(false);
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+161
to
+162
| userManager.FindByNameAsync(Arg.Any<string>()).Returns(DisabledUser()); | ||
| userManager.CheckPasswordAsync(Arg.Any<EformUser>(), Arg.Any<string>()).Returns(true); |
Comment on lines
+1012
to
+1014
| var affected = await baseDbContext.Users | ||
| .Where(x => x.Email.ToLower() == workerEmail) | ||
| .ExecuteUpdateAsync(x => x.SetProperty(u => u.IsActive, isActive)) |
…use 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 <noreply@anthropic.com>
…olve 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 <noreply@anthropic.com>
renemadsen
force-pushed
the
feat/disable-login-on-resign-tp
branch
from
September 17, 2026 16:10
70e7695 to
dfa6d1c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new SDK join has incompatible nullable key types and the cross-database login-state synchronization lacks effective integration coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs:1177
- The existing
UpdateAssignedSitetests construct this service with a nullBaseDbContextand do not seed the SDKSite/SiteWorker/Workeror an Identity user, so a resignation test would only exercise the catch-and-log path. Add an integration test that verifies a real resignation setsUsers.IsActive = falseand reinstatement restores it; otherwise the security-critical cross-database write can regress without a failing test.
if (site.Resigned != wasResigned)
{
await SyncLoginStateAsync(dbAssignedSite.SiteId, site.Resigned).ConfigureAwait(false);
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
|
|
||
| var email = await ( | ||
| from s in sdkDbContext.Sites | ||
| join sw in sdkDbContext.SiteWorkers on s.Id equals sw.SiteId |
Comment on lines
137
to
+142
| 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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes step 4 of microting/eform-angular-frontend#8072 on this side. Core refuses
EformUser.IsActive = falseat every credential surface (microting/eform-angular-frontend#8075, merged); the backendconfiguration half is microting/eform-backendconfiguration-plugin#1283. This closes the two gaps that live here.1. Resigning from time-planning did not touch the login
TimeSettingService.UpdateAssignedSitewroteAssignedSite.Resignedand nothing else.Resignedis a visibility flag that no authentication code reads, so the person kept a working account — including the flutter apps, which authenticate with anEformUserJWT.It now syncs
IsActiveafter the settings row is committed: resolve the worker throughSites → SiteWorkers → Workers, match the address the way the avatar lookup in this same file already does (Trim().ToLower()on both sides — the two must agree, or that lookup finds a login this one misses), and write the single column withExecuteUpdate.UserManager.UpdateAsyncis not usable here: it runs Identity's validators, which part of this population cannot satisfy (non-ASCII local parts).A sync failure is captured and logged rather than failing a save that has already committed. The case that matters — no login row matched, i.e. the resignation reached no account — is a warning, not a zero buried in a success-shaped line.
2. This plugin's gRPC login had no
IsActivecheckTimePlanningAuthGrpcServiceis a second, parallel login implementation that mints the same JWT as core's REST path. Without a check here, a resigned employee's flutter-time app keeps working no matter what else shipped.It now refuses disabled accounts on both
AuthenticateUserandRefreshToken. The check sits after the password comparison: answering earlier would let a disabled account answer faster than a wrong password does, which is a timing oracle — the same ordering core settled on.The three distinguishable messages (
User with username X not found,Incorrect password., and the refresh equivalent) are collapsed into one, matching what core now returns, so this path stops being an account-enumeration oracle. The class remarks that advertised the old strings are corrected — they listed two messages this change deletes.Tests
Three in
TimePlanningAuthGrpcServiceTests: a disabled account is refused; its answer is byte-identical to an unknown account's and does not echo the submitted username; andRefreshTokenrefuses it too.Verified by removing the check and confirming exactly those three go red, then restoring it — they are not vacuous. Per this project's rule the suite itself runs in CI, not locally.
SyncLoginStateAsyncis private and spans two DbContexts, so it has no direct test; that is a known gap worth closing if the email→IsActivewrite is ever extracted.Deploy ordering — not optional
Core must be deployed before this plugin.
EformUserresolves from the host's assemblies via the plugin loader's shared-types list, so on an older hostIsActiveis a missing member at runtime.Still open
CheckPasswordAsync, notSignInManager), and the proper fix is to delegate to core'sIAuthServicerather than re-implement.🤖 Generated with Claude Code