refactor: convention cleanup, unify AddToRole/RemoveFromRole error contract - #144
Merged
Conversation
…Blazor UI - Delete narrative/decorative/dead/non-English comments across src/ and compress genuine one-line "why" rationale (Register.razor changelog comment, dead commented-out navigation in Email.razor, German comments in DeletePersonalData.razor/ChangePassword.razor, decorative HTML-comment section labels, duplicated multi-line blocks in UserProjection.cs). - Add `sealed` to RoleProjection, UserRoleAssignmentProjection, SetupRedirectMiddleware, CookieLoginMiddleware; narrow CurrentUserService to `internal sealed` (no external consumers of the concrete type, confirmed via a solution-wide search). - Convert UserRoleAssignment to a `sealed record` (plain, never-mutated data holder); add `sealed` to PasskeyInputModel (kept as a class per the form-model convention — evaluated for record conversion but its shape matches a mutable bind-target, not an immutable DTO). - Delete the stale "Todo: Use Hashset" comment on User.Passkeys — the dictionary is keyed by credential id and queried via LINQ-to-Marten, so switching collection types is not a safe, trivial change; flagged as a possible future follow-up instead. - Blazor: add <AppPageTitle> to AccessDenied.razor; remove StateHasChanged() calls that are redundant given Blazor's automatic post-handler render (kept the one call preceded by an earlier await, e.g. after a confirm dialog, since the framework's single pre-first-await auto-render was already spent there); replace a throw-then-immediately-catch in Disable2fa.razor with direct branching.
…ore members with #region
UserStore.AddToRoleAsync/RemoveFromRoleAsync used to throw
IdentityAuthorizationException on an authorization-denied condition while
every sibling store method (CreateAsync, DeleteAsync, RestoreAsync) returns
IdentityResult.Failed for the equivalent condition. This is a real, public
API behavior change:
- UserStore<TUser> gains public Task<IdentityResult>-returning
AddToRoleAsync/RemoveFromRoleAsync overloads (matching the
Create/Delete/Restore pattern) that our own call sites use directly.
- IUserRoleStore<TUser>'s AddToRoleAsync/RemoveFromRoleAsync only permit a
plain Task return per the framework interface (verified empirically —
there is no IdentityResult channel through UserManager for this
operation), so the explicit interface implementation adapts a Failed
result back into a throw only for hosts driving role assignment through
UserManager. IdentityAuthorizationException's doc comment is updated to
describe this narrower, adapter-only scope.
- UserInvitationService.InviteAsync now loops over UserStore.AddToRoleAsync
and checks .Succeeded instead of catching IdentityAuthorizationException
around UserManager.AddToRolesAsync.
- RoleUserDialog.razor and UserRoleDialog.razor (Administration role
assignment UI) now call UserStore.AddToRoleAsync/RemoveFromRoleAsync
directly instead of the UserManager equivalents, so an authorization
failure surfaces as a DialogService.Alert instead of an unhandled
exception; UserRoleDialog gained the error-surfacing it was missing
entirely for AddToRolesAsync/RemoveFromRolesAsync (new
UserRoleDialog.ErrorTitle/RolesCouldNotBeSaved resource strings, en+de).
- UserStoreAuthorizationTests: the three tests asserting
Assert.ThrowsAsync<IdentityAuthorizationException> on
AddToRoleAsync/RemoveFromRoleAsync now assert on the returned
IdentityResult instead, per the new contract.
Also replaces the bare "// IUserXxxStore" section-header comments in
UserStore.cs with #region/#endregion, matching the pattern already used in
UserProjection.cs, and fixes the untyped `throw new Exception("User not
found")` in the passkey read path to `InvalidOperationException`, matching
the "role not found" pattern already used in the same file.
…nd-forget CleanupScheduleStartupService.StartAsync fired ApplyStoredScheduleAsync without awaiting it and without threading any CancellationToken, even though the method does DB and scheduler I/O and the host already exposes IHostApplicationLifetime.ApplicationStopping. Capture ApplicationStopping and pass it through to the DB load and scheduler calls, and wrap the fire-and-forget invocation in a new RunApplyStoredScheduleAsync that keeps the existing try/catch + log-and-swallow behavior, so an unobserved exception from the discarded Task can no longer crash the process.
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.
Summary
Quick-fix cleanup pass from the convention audit (PR A of two — PR B, a
separate later effort, will normalize the test suite). Three commits:
Convention cleanup — deleted narrative/decorative/dead/non-English
comments across
src/and compressed genuine one-line "why" rationale(a changelog-style comment in Register.razor, dead commented-out
navigation in Email.razor, German comments/HTML-comment labels in
several Manage pages, a duplicated 4-line comment repeated verbatim
across 5 Blazor pages, two duplicated blocks in UserProjection.cs).
Added
sealedto RoleProjection, UserRoleAssignmentProjection,SetupRedirectMiddleware, CookieLoginMiddleware; narrowed
CurrentUserServicetointernal sealedafter confirming no consumeroutside the assembly references the concrete type. Converted
UserRoleAssignmentto asealed record. Blazor: added<AppPageTitle>to AccessDenied.razor, removedStateHasChanged()calls that are redundant given Blazor's automatic post-handler render,
and replaced a throw-then-immediately-catch in Disable2fa.razor with
direct branching.
Unify the
AddToRole/RemoveFromRoleerror contract (the onebehavior-affecting change to the public API) —
UserStore.AddToRoleAsyncand
RemoveFromRoleAsyncused tothrow IdentityAuthorizationExceptionon an authorization-denied condition, while every sibling store method
(
CreateAsync,DeleteAsync,RestoreAsync) returnsIdentityResult.Failedfor the equivalent condition.UserStore<TUser>now exposes publicTask<IdentityResult>-returningAddToRoleAsync/RemoveFromRoleAsyncoverloads matching that pattern.This could not be done on the
IUserRoleStore<TUser>interface membersthemselves — verified empirically that the framework interface only
permits a plain
Taskreturn with noIdentityResultchannel backthrough
UserManager— so the explicitIUserRoleStore<TUser>implementation adapts a Failed result back into a throw, but only for
hosts that drive role assignment through
UserManagerdirectly.IdentityAuthorizationException's doc comment now describes thisnarrower, adapter-only scope.
Call sites updated:
UserInvitationService.InviteAsync— now loops overUserStore.AddToRoleAsyncand checks.Succeededinstead ofcatching
IdentityAuthorizationExceptionaroundUserManager.AddToRolesAsync.RoleUserDialog.razor(admin role-removal UI) — now callsUserStore.RemoveFromRoleAsyncdirectly instead ofUserManager.RemoveFromRoleAsync, so an authorization failuresurfaces as the existing
DialogService.Alertinstead of anunhandled exception.
UserRoleDialog.razor(admin role-assignment UI) — now callsUserStore.AddToRoleAsync/RemoveFromRoleAsyncdirectly instead ofUserManager.AddToRolesAsync/RemoveFromRolesAsync, and nowsurfaces a failure via
DialogService.Alert(newUserRoleDialog.ErrorTitle/RolesCouldNotBeSavedresource strings,en+de) — this dialog previously had no error handling at all for
this operation.
UserStoreAuthorizationTests— the three tests that assertedAssert.ThrowsAsync<IdentityAuthorizationException>onAddToRoleAsync/RemoveFromRoleAsyncnow assert on the returnedIdentityResultinstead (AddToRoleAsync_NonAdminActor_Throws→..._ReturnsNotAuthorized, etc.) — required by the contract change,scoped to only what item 6 needed.
Also replaced the bare
// IUserXxxStoresection-header comments inUserStore.cswith#region/#endregion, matching the patternalready used in
UserProjection.cs, and fixed the untypedthrow new Exception("User not found")in the passkey read path toInvalidOperationException, matching the existing "role not found"pattern in the same file.
Cancellation fix —
CleanupScheduleStartupService.StartAsyncfiredApplyStoredScheduleAsyncwithout awaiting it and without threading aCancellationToken, despite doing DB/scheduler I/O and the hostalready exposing
IHostApplicationLifetime.ApplicationStopping.Threaded
ApplicationStoppingthrough, and wrapped the fire-and-forgetinvocation so an unobserved exception can no longer crash the process
(existing log-and-swallow behavior preserved).
Notable judgment calls
User.PasskeysHashSet TODO: left asDictionary<string, UserPasskey>and just deleted the stale comment. The dictionary is keyed by
credential id and queried via LINQ-to-Marten
(
x.Passkeys.Keys.Contains(...)); switching collection types wouldchange serialization shape and query semantics against existing
production data — not a safe, trivial change. Flagging as a possible
future follow-up.
PasskeyInputModel: evaluated for conversion to a record but keptas
sealed class. No live usage was found anywhere in the codebase(looks like leftover/unused code), but its shape (two mutable
string?properties, one of them an
Errorfield meant to be set after thefact) matches this repo's documented "form models are mutable classes,
not records" convention rather than an immutable DTO, so it was judged
safer to keep as a class.
Test plan
dotnet csharpier format .— clean, no outstanding diffsdotnet build— 0 errors, 91 pre-existing warnings (all in testfiles untouched by this PR — CS8625/xUnit1051), no new warnings
dotnet test --filter "FullyQualifiedName!~E2ETests"— 413/413passed (191 unit + 130 Blazor + 92 integration; Docker was
available so the Postgres-backed integration tests ran too)
dotnet list package --vulnerable --include-transitive— novulnerable packages
UserStoreAuthorizationTests+UserInvitationServiceTests(16 tests) — all passedCloses #143
🤖 Generated with Claude Code