From 2f617b538f4a8e106d00654fdb405b33009aa3fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20G=C3=B6pel?= Date: Sun, 26 Jul 2026 01:32:34 +0800 Subject: [PATCH 1/3] =?UTF-8?q?refactor:=20convention=20cleanup=20?= =?UTF-8?q?=E2=80=94=20comments,=20sealed=20visibility,=20records,=20Blazo?= =?UTF-8?q?r=20UI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 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. --- ...tyComponentsEndpointRouteBuilderExtensions.cs | 1 - ...ityRevalidatingAuthenticationStateProvider.cs | 2 -- .../Components/Account/Pages/AccessDenied.razor | 3 +++ .../Account/Pages/Manage/ChangePassword.razor | 10 ++-------- .../Pages/Manage/DeletePersonalData.razor | 9 +-------- .../Account/Pages/Manage/Disable2fa.razor | 16 ++++++++++++---- .../Components/Account/Pages/Manage/Email.razor | 12 +----------- .../Pages/Manage/EnableAuthenticator.razor | 2 -- .../Account/Pages/Manage/Passkeys.razor | 2 -- .../Account/Pages/Manage/PersonalData.razor | 3 --- .../Account/Pages/Manage/Profile.razor | 7 +------ .../Account/Pages/Manage/RenamePasskey.razor | 7 +------ .../Pages/Manage/ResetAuthenticator.razor | 2 -- .../Components/Account/Pages/Register.razor | 4 ---- .../Components/Account/PasskeyInputModel.cs | 2 +- .../Account/Shared/PasskeySubmit.razor | 2 -- .../Http/CookieLoginMiddleware.cs | 2 +- .../Http/SetupRedirectMiddleware.cs | 2 +- .../Roles/RoleProjection.cs | 2 +- .../Services/CurrentUserService.cs | 3 ++- .../UserRoles/UserRoleAssignment.cs | 6 +++--- .../UserRoles/UserRoleAssignmentProjection.cs | 2 +- src/AndreGoepel.Marten.Identity/Users/User.cs | 1 - .../Users/UserProjection.cs | 8 +++----- 24 files changed, 34 insertions(+), 76 deletions(-) diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs index 1ca7f85..f345679 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityComponentsEndpointRouteBuilderExtensions.cs @@ -15,7 +15,6 @@ namespace AndreGoepel.Marten.Identity.Blazor.Components.Account; public static class IdentityComponentsEndpointRouteBuilderExtensions { - // These endpoints are required by the Identity Razor components defined in the /Components/Account/Pages directory of this project. public static IEndpointConventionBuilder MapAdditionalIdentityEndpoints( this IEndpointRouteBuilder endpoints ) diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityRevalidatingAuthenticationStateProvider.cs b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityRevalidatingAuthenticationStateProvider.cs index 7afc5dc..d690945 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityRevalidatingAuthenticationStateProvider.cs +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/IdentityRevalidatingAuthenticationStateProvider.cs @@ -9,8 +9,6 @@ namespace AndreGoepel.Marten.Identity.Blazor.Components.Account; -// This is a server-side AuthenticationStateProvider that revalidates the security stamp for the connected user -// every 30 minutes an interactive circuit is connected. internal sealed class IdentityRevalidatingAuthenticationStateProvider( ILoggerFactory loggerFactory, IServiceScopeFactory scopeFactory, diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/AccessDenied.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/AccessDenied.razor index b8f2fbd..0a388d8 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/AccessDenied.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/AccessDenied.razor @@ -1,8 +1,11 @@ @page "/Account/AccessDenied" @attribute [AllowAnonymous] +@inherits IdentityLocalizedComponentBase @inject NavigationManager NavigationManager + + @code { protected override void OnInitialized() { diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/ChangePassword.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/ChangePassword.razor index 3a8b30f..b7bb2df 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/ChangePassword.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/ChangePassword.razor @@ -85,7 +85,7 @@ string.IsNullOrWhiteSpace(model.NewPassword) || string.IsNullOrWhiteSpace(model.ConfirmPassword)) { - return; // Validatoren sollten das bereits abfangen + return; // Validators should already catch this } if (model.NewPassword != model.ConfirmPassword) @@ -95,7 +95,6 @@ } _isProcessing = true; - StateHasChanged(); try { @@ -115,7 +114,6 @@ ); Model = new(); - StateHasChanged(); } else { @@ -139,14 +137,10 @@ finally { _isProcessing = false; - StateHasChanged(); } } - // Validation lives in the markup as Radzen validator components. RadzenTemplateForm - // (wrapped by CardForm) does not evaluate DataAnnotations and no DataAnnotationsValidator - // is present, so attributes here would never run — they were removed rather than - // translated into a second, silently-dead set of messages (#114). + // Validation lives in the markup as Radzen validators — RadzenTemplateForm/CardForm don't run DataAnnotations (#114). private sealed class ChangePasswordModel { [DataType(DataType.Password)] diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/DeletePersonalData.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/DeletePersonalData.razor index baace69..6d1c5d2 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/DeletePersonalData.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/DeletePersonalData.razor @@ -20,7 +20,6 @@ - e.Description))); + Logger.LogError( + "Failed to disable 2FA for user {UserId}: {Errors}", + _user.Id, + string.Join(", ", result.Errors.Select(e => e.Description)) + ); + NotificationService.Notify( + NotificationSeverity.Error, + summary: T("Disable2fa.ErrorTitle"), + detail: T("Disable2fa.DisableFailedDetail") + ); + return; } var userId = await UserManager.GetUserIdAsync(_user); @@ -91,7 +100,7 @@ } catch (Exception ex) { - Logger.LogError(ex, "Failed to disable 2FA for user {UserId}", _user?.Id); + Logger.LogError(ex, "Unexpected error while disabling 2FA for user {UserId}", _user?.Id); NotificationService.Notify( NotificationSeverity.Error, summary: T("Disable2fa.ErrorTitle"), @@ -101,7 +110,6 @@ finally { _isProcessing = false; - StateHasChanged(); } } diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Email.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Email.razor index 3edf30d..ee7d57e 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Email.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Email.razor @@ -105,7 +105,6 @@ } _isChangingEmail = true; - StateHasChanged(); try { @@ -132,9 +131,6 @@ T("Email.SuccessTitle"), T("Email.ConfirmationSentDetail") ); - - // Optional: zurück zur Profile-Seite - // NavigationManager.NavigateTo("/Account/Manage/Profile"); } catch (Exception) { @@ -147,7 +143,6 @@ finally { _isChangingEmail = false; - StateHasChanged(); } } @@ -157,7 +152,6 @@ return; _isSendingVerification = true; - StateHasChanged(); try { @@ -195,14 +189,10 @@ finally { _isSendingVerification = false; - StateHasChanged(); } } - // Validation lives in the markup as Radzen validator components. RadzenTemplateForm - // (wrapped by CardForm) does not evaluate DataAnnotations and no DataAnnotationsValidator - // is present, so attributes here would never run — they were removed rather than - // translated into a second, silently-dead set of messages (#114). + // Validation lives in the markup as Radzen validators — RadzenTemplateForm/CardForm don't run DataAnnotations (#114). private sealed class ChangeEmailModel { public string? NewEmail { get; set; } diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/EnableAuthenticator.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/EnableAuthenticator.razor index c83c2bb..f350bfb 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/EnableAuthenticator.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/EnableAuthenticator.razor @@ -121,7 +121,6 @@ else return; } - // Strip spaces and hyphens var verificationCode = model.Code?.Replace(" ", string.Empty).Replace("-", string.Empty) ?? ""; var is2FaTokenValid = await UserManager.VerifyTwoFactorTokenAsync( @@ -151,7 +150,6 @@ else private async ValueTask LoadSharedKeyAndQrCodeUriAsync(User user) { - // Load the authenticator key & QR code URI to display on the form var unformattedKey = await UserManager.GetAuthenticatorKeyAsync(user); if (string.IsNullOrEmpty(unformattedKey)) { diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Passkeys.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Passkeys.razor index ff29d60..96201ac 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Passkeys.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/Passkeys.razor @@ -105,7 +105,6 @@ protected override async Task OnInitializedAsync() { _isLoading = true; - StateHasChanged(); try { @@ -128,7 +127,6 @@ finally { _isLoading = false; - StateHasChanged(); } } diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/PersonalData.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/PersonalData.razor index e92775e..8c6f78a 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/PersonalData.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Manage/PersonalData.razor @@ -19,12 +19,10 @@ - @T("PersonalData.IntroText") - @@ -35,7 +33,6 @@
-
-
@T("ResetAuthenticator.AlertLine1") @@ -28,7 +27,6 @@
- diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Register.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Register.razor index 15dd391..8ca105b 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Register.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Pages/Register.razor @@ -87,10 +87,6 @@ if (!result.Succeeded) { - // Pre-existing bug fixed in passing: this joined the IdentityError objects - // themselves, not their Description — IdentityError doesn't override ToString(), - // so the notification showed the bare type name instead of the actual reason. - // No test covered the text, only that a notification fired, so it went unnoticed. var reasons = string.Join(' ', result.Errors.Select(e => e.Description)); NotificationService.Notify( NotificationSeverity.Error, diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/PasskeyInputModel.cs b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/PasskeyInputModel.cs index 179c88d..ad38231 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/PasskeyInputModel.cs +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/PasskeyInputModel.cs @@ -1,6 +1,6 @@ namespace AndreGoepel.Marten.Identity.Blazor.Components.Account; -public class PasskeyInputModel +public sealed class PasskeyInputModel { public string? CredentialJson { get; set; } public string? Error { get; set; } diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Shared/PasskeySubmit.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Shared/PasskeySubmit.razor index f92bdb1..9a82658 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Shared/PasskeySubmit.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Account/Shared/PasskeySubmit.razor @@ -34,7 +34,6 @@ if (_module is null) return; _isBusy = true; - StateHasChanged(); try { @@ -56,7 +55,6 @@ finally { _isBusy = false; - StateHasChanged(); } } diff --git a/src/AndreGoepel.Marten.Identity/Http/CookieLoginMiddleware.cs b/src/AndreGoepel.Marten.Identity/Http/CookieLoginMiddleware.cs index e6d1092..2851476 100644 --- a/src/AndreGoepel.Marten.Identity/Http/CookieLoginMiddleware.cs +++ b/src/AndreGoepel.Marten.Identity/Http/CookieLoginMiddleware.cs @@ -34,7 +34,7 @@ public sealed record RecoveryCodeLoginInfo(string Code, string? ReturnUrl); /// session fixation). The handle itself is opaque and strictly single-use. /// /// -public class CookieLoginMiddleware(RequestDelegate next) +public sealed class CookieLoginMiddleware(RequestDelegate next) { private const string DefaultRedirect = "/dashboard"; diff --git a/src/AndreGoepel.Marten.Identity/Http/SetupRedirectMiddleware.cs b/src/AndreGoepel.Marten.Identity/Http/SetupRedirectMiddleware.cs index 28a803a..d8115d1 100644 --- a/src/AndreGoepel.Marten.Identity/Http/SetupRedirectMiddleware.cs +++ b/src/AndreGoepel.Marten.Identity/Http/SetupRedirectMiddleware.cs @@ -3,7 +3,7 @@ namespace AndreGoepel.Marten.Identity.Http; -public class SetupRedirectMiddleware(RequestDelegate next) +public sealed class SetupRedirectMiddleware(RequestDelegate next) { private static volatile bool _isConfigured; diff --git a/src/AndreGoepel.Marten.Identity/Roles/RoleProjection.cs b/src/AndreGoepel.Marten.Identity/Roles/RoleProjection.cs index d772a97..de1e077 100644 --- a/src/AndreGoepel.Marten.Identity/Roles/RoleProjection.cs +++ b/src/AndreGoepel.Marten.Identity/Roles/RoleProjection.cs @@ -4,7 +4,7 @@ namespace AndreGoepel.Marten.Identity.Roles; -internal partial class RoleProjection : SingleStreamProjection +internal sealed partial class RoleProjection : SingleStreamProjection { [SuppressMessage( "Performance", diff --git a/src/AndreGoepel.Marten.Identity/Services/CurrentUserService.cs b/src/AndreGoepel.Marten.Identity/Services/CurrentUserService.cs index 8192245..6c41eca 100644 --- a/src/AndreGoepel.Marten.Identity/Services/CurrentUserService.cs +++ b/src/AndreGoepel.Marten.Identity/Services/CurrentUserService.cs @@ -4,7 +4,8 @@ namespace AndreGoepel.Marten.Identity.Services; -public class CurrentUserService(AuthenticationStateProvider authStateProvider) : ICurrentUserService +internal sealed class CurrentUserService(AuthenticationStateProvider authStateProvider) + : ICurrentUserService { public async Task GetCurrentUserIdAsync(CancellationToken cancellationToken = default) { diff --git a/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignment.cs b/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignment.cs index cc89e92..80e6cd0 100644 --- a/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignment.cs +++ b/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignment.cs @@ -3,11 +3,11 @@ namespace AndreGoepel.Marten.Identity.UserRoles; -public class UserRoleAssignment +public sealed record UserRoleAssignment { public string Id => $"{UserId}:{RoleId}"; public Guid UserGuid => UserId; - public UserId UserId { get; set; } + public UserId UserId { get; init; } public Guid RoleGuid => RoleId; - public RoleId RoleId { get; set; } + public RoleId RoleId { get; init; } } diff --git a/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignmentProjection.cs b/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignmentProjection.cs index 5af6655..919f351 100644 --- a/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignmentProjection.cs +++ b/src/AndreGoepel.Marten.Identity/UserRoles/UserRoleAssignmentProjection.cs @@ -5,7 +5,7 @@ namespace AndreGoepel.Marten.Identity.UserRoles; -internal class UserRoleAssignmentProjection : IProjection +internal sealed class UserRoleAssignmentProjection : IProjection { public static void Apply(IDocumentOperations operations, RoleAssigned @event) { diff --git a/src/AndreGoepel.Marten.Identity/Users/User.cs b/src/AndreGoepel.Marten.Identity/Users/User.cs index 895130d..65c38fe 100644 --- a/src/AndreGoepel.Marten.Identity/Users/User.cs +++ b/src/AndreGoepel.Marten.Identity/Users/User.cs @@ -31,7 +31,6 @@ public Guid StreamId public string? AuthenticatorKey { get; set; } public string? RecoveryCodes { get; set; } - // Todo: Use Hashset public Dictionary Passkeys { get; set; } = []; public HashSet Roles { get; set; } = []; diff --git a/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs b/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs index 7c50646..7f84491 100644 --- a/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs +++ b/src/AndreGoepel.Marten.Identity/Users/UserProjection.cs @@ -4,7 +4,7 @@ namespace AndreGoepel.Marten.Identity.Users; -internal partial class UserProjection : SingleStreamProjection +internal sealed partial class UserProjection : SingleStreamProjection { [SuppressMessage( "Performance", @@ -145,8 +145,7 @@ public void Apply(UserUpdated @event, User user) )] public void Apply(PasskeyCreated @event, User user) { - // Masked (erased) events carry a null payload; skip them so a projection - // rebuild over a GDPR-erased stream stays safe (#67). + // Masked events carry a null payload; skip so a rebuild over an erased stream stays safe (#67). if (@event.Passkey is null) return; @@ -161,8 +160,7 @@ public void Apply(PasskeyCreated @event, User user) )] public void Apply(PasskeyUpdated @event, User user) { - // Masked (erased) events carry a null payload; skip them so a projection - // rebuild over a GDPR-erased stream stays safe (#67). + // Masked events carry a null payload; skip so a rebuild over an erased stream stays safe (#67). if (@event.Passkey is null) return; From d2d2eee3cd5bbe9bbdc1aa8ed9dbe337581479b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20G=C3=B6pel?= Date: Sun, 26 Jul 2026 01:32:55 +0800 Subject: [PATCH 2/3] refactor: unify AddToRole/RemoveFromRole error contract, group UserStore members with #region MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 gains public Task-returning AddToRoleAsync/RemoveFromRoleAsync overloads (matching the Create/Delete/Restore pattern) that our own call sites use directly. - IUserRoleStore'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 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. --- .../Dialogs/RoleUserDialog.razor | 3 +- .../Dialogs/UserRoleDialog.razor | 21 ++++- .../Resources/IdentityStrings.de.resx | 5 ++ .../Resources/IdentityStrings.resx | 5 ++ .../IdentityAuthorizationException.cs | 12 +-- .../Users/UserInvitationService.cs | 22 ++--- .../Users/UserStore.cs | 89 +++++++++++++++---- .../Users/UserStoreAuthorizationTests.cs | 29 +++--- 8 files changed, 132 insertions(+), 54 deletions(-) diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/RoleUserDialog.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/RoleUserDialog.razor index 1a01654..7a0a448 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/RoleUserDialog.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/RoleUserDialog.razor @@ -2,6 +2,7 @@ @inject DialogService DialogService @inject ConfirmService Confirm @inject UserManager UserManager +@inject UserStore UserStore @inject RoleManager RoleManager @@ -56,7 +57,7 @@ return; } - var deleteResult = await UserManager.RemoveFromRoleAsync(user, _role.Name); + var deleteResult = await UserStore.RemoveFromRoleAsync(user, _role.Name, CancellationToken.None); if (deleteResult.Succeeded) { _users = _users.Where(u => u.Id != user.Id).ToList(); diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/UserRoleDialog.razor b/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/UserRoleDialog.razor index 246314b..cb20583 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/UserRoleDialog.razor +++ b/src/AndreGoepel.Marten.Identity.Blazor/Components/Administration/Dialogs/UserRoleDialog.razor @@ -1,6 +1,7 @@ @inherits IdentityLocalizedComponentBase @inject DialogService DialogService @inject UserManager UserManager +@inject UserStore UserStore @inject RoleManager RoleManager @@ -47,14 +48,30 @@ join role in _roles on value equals role.RoleId select role.Name; - await UserManager.AddToRolesAsync(_user, newRoles); + foreach (var role in newRoles) + { + var result = await UserStore.AddToRoleAsync(_user, role!, CancellationToken.None); + if (!result.Succeeded) + { + await DialogService.Alert(T("UserRoleDialog.RolesCouldNotBeSaved"), T("UserRoleDialog.ErrorTitle")); + return; + } + } var removedRoles = from value in _user.Roles.Except(_values) join role in _roles on value equals role.RoleId select role.Name; - await UserManager.RemoveFromRolesAsync(_user, removedRoles); + foreach (var role in removedRoles) + { + var result = await UserStore.RemoveFromRoleAsync(_user, role!, CancellationToken.None); + if (!result.Succeeded) + { + await DialogService.Alert(T("UserRoleDialog.RolesCouldNotBeSaved"), T("UserRoleDialog.ErrorTitle")); + return; + } + } DialogService.Close(true); } diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.de.resx b/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.de.resx index 11287c7..9a9d30e 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.de.resx +++ b/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.de.resx @@ -162,6 +162,9 @@ + + + @@ -684,4 +687,6 @@ {0} is the Administrator role name (data, not translated). + + diff --git a/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.resx b/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.resx index 928481f..affa26c 100644 --- a/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.resx +++ b/src/AndreGoepel.Marten.Identity.Blazor/Resources/IdentityStrings.resx @@ -195,6 +195,9 @@ + + + @@ -792,4 +795,6 @@ {0} is the Administrator role name (data, not translated). + + diff --git a/src/AndreGoepel.Marten.Identity/Services/IdentityAuthorizationException.cs b/src/AndreGoepel.Marten.Identity/Services/IdentityAuthorizationException.cs index ee87d4a..fe50ae6 100644 --- a/src/AndreGoepel.Marten.Identity/Services/IdentityAuthorizationException.cs +++ b/src/AndreGoepel.Marten.Identity/Services/IdentityAuthorizationException.cs @@ -1,10 +1,12 @@ namespace AndreGoepel.Marten.Identity.Services; /// -/// Thrown when a privileged identity store operation that returns no result — the -/// role-assignment methods (IUserRoleStore) — is invoked without the required -/// authorization (#69/#41). Operations that return -/// surface the same condition -/// as a failed result instead. +/// Thrown only by UserStore's explicit IUserRoleStore implementation, which +/// adapts a failed role-assignment +/// back into a throw for hosts calling through UserManager — that interface returns a +/// plain Task and has no channel to carry a result (#69/#41). Callers that use +/// UserStore's own AddToRoleAsync/RemoveFromRoleAsync overloads directly +/// get the failed instead and +/// never see this exception. /// public sealed class IdentityAuthorizationException(string message) : Exception(message); diff --git a/src/AndreGoepel.Marten.Identity/Users/UserInvitationService.cs b/src/AndreGoepel.Marten.Identity/Users/UserInvitationService.cs index 1d5be12..e2aa8bc 100644 --- a/src/AndreGoepel.Marten.Identity/Users/UserInvitationService.cs +++ b/src/AndreGoepel.Marten.Identity/Users/UserInvitationService.cs @@ -23,6 +23,7 @@ namespace AndreGoepel.Marten.Identity.Users; /// public sealed class UserInvitationService( UserManager userManager, + UserStore userStore, IIdentityAuthorizer authorizer ) { @@ -71,24 +72,11 @@ public async Task InviteAsync( return UserInvitationResult.Failed(createResult); var roleList = roles?.ToArray() ?? []; - if (roleList.Length > 0) + foreach (var role in roleList) { - try - { - var roleResult = await userManager.AddToRolesAsync(user, roleList); - if (!roleResult.Succeeded) - return UserInvitationResult.Failed(roleResult); - } - catch (IdentityAuthorizationException ex) - { - // The role-assignment store methods signal an authorization failure by - // throwing rather than returning a result (#69/#41). Reaching this after - // the check above would mean authority was lost mid-operation; surface it - // as a normal failure rather than letting it escape as an exception. - return UserInvitationResult.Failed( - Failure(IdentityErrorCodes.NotAuthorized, ex.Message) - ); - } + var roleResult = await userStore.AddToRoleAsync(user, role, cancellationToken); + if (!roleResult.Succeeded) + return UserInvitationResult.Failed(roleResult); } return UserInvitationResult.Success(user, await GenerateTokenAsync(user)); diff --git a/src/AndreGoepel.Marten.Identity/Users/UserStore.cs b/src/AndreGoepel.Marten.Identity/Users/UserStore.cs index 8787761..f997aae 100644 --- a/src/AndreGoepel.Marten.Identity/Users/UserStore.cs +++ b/src/AndreGoepel.Marten.Identity/Users/UserStore.cs @@ -492,7 +492,7 @@ public void Dispose() GC.SuppressFinalize(this); } - // IUserPasswordStore + #region IUserPasswordStore public Task SetPasswordHashAsync( TUser user, @@ -513,7 +513,9 @@ public Task HasPasswordAsync(TUser user, CancellationToken cancellationTok return Task.FromResult(hasPassword); } - // IUserSecurityStampStore + #endregion IUserPasswordStore + + #region IUserSecurityStampStore public Task SetSecurityStampAsync(TUser user, string stamp, CancellationToken cancellationToken) { @@ -528,7 +530,9 @@ public Task SetSecurityStampAsync(TUser user, string stamp, CancellationToken ca return Task.FromResult(user.SecurityStamp); } - // IUserEmailStore + #endregion IUserSecurityStampStore + + #region IUserEmailStore public Task SetEmailAsync(TUser user, string? email, CancellationToken cancellationToken) { @@ -573,7 +577,9 @@ CancellationToken cancellationToken return Task.CompletedTask; } - // IUserPhoneNumberStore + #endregion IUserEmailStore + + #region IUserPhoneNumberStore public Task SetPhoneNumberAsync( TUser user, @@ -603,7 +609,9 @@ CancellationToken cancellationToken return Task.CompletedTask; } - // IUserTwoFactorStore + #endregion IUserPhoneNumberStore + + #region IUserTwoFactorStore public Task SetTwoFactorEnabledAsync( TUser user, @@ -620,7 +628,9 @@ public Task GetTwoFactorEnabledAsync(TUser user, CancellationToken cancell return Task.FromResult(user.TwoFactorEnabled); } - // IUserAuthenticatorKeyStore + #endregion IUserTwoFactorStore + + #region IUserAuthenticatorKeyStore public Task SetAuthenticatorKeyAsync( TUser user, @@ -642,7 +652,9 @@ CancellationToken cancellationToken ); } - // IUserTwoFactorRecoveryCodeStore + #endregion IUserAuthenticatorKeyStore + + #region IUserTwoFactorRecoveryCodeStore public Task ReplaceCodesAsync( TUser user, @@ -698,6 +710,8 @@ public Task CountCodesAsync(TUser user, CancellationToken cancellationToken return Task.FromResult(count); } + #endregion IUserTwoFactorRecoveryCodeStore + public async Task AddOrUpdatePasskeyAsync( TUser user, UserPasskeyInfo passkey, @@ -711,7 +725,7 @@ CancellationToken cancellationToken await querySession .Query() .FirstOrDefaultAsync(x => x.Id == user.Id, cancellationToken) - ?? throw new Exception("User not found"); + ?? throw new InvalidOperationException("User not found"); var isUpdate = userEntity.Passkeys.ContainsKey(credentialId); @@ -747,7 +761,7 @@ CancellationToken cancellationToken await querySession .Query() .FirstOrDefaultAsync(x => x.Id == user.Id, cancellationToken) - ?? throw new Exception("User not found"); + ?? throw new InvalidOperationException("User not found"); return [.. userEntity.Passkeys.Select(kvp => kvp.Value.PasskeyInfo)]; } @@ -773,7 +787,7 @@ CancellationToken cancellationToken await querySession .Query() .FirstOrDefaultAsync(x => x.Id == user.Id, cancellationToken) - ?? throw new Exception("User not found"); + ?? throw new InvalidOperationException("User not found"); return userEntity.Passkeys.TryGetValue( Convert.ToBase64String(credentialId), @@ -794,7 +808,16 @@ CancellationToken cancellationToken await session.SaveChangesAsync(cancellationToken); } - public async Task AddToRoleAsync( + /// + /// Assigns a role, returning a failed (matching the + /// CreateAsync/DeleteAsync/RestoreAsync contract) rather than throwing when the caller + /// lacks authority. This is the overload our own call sites use; the + /// explicit implementation below adapts a failure + /// back into a throw only for hosts driving role assignment through + /// UserManager, which has no channel to surface an + /// from this operation. + /// + public async Task AddToRoleAsync( TUser user, string roleName, CancellationToken cancellationToken @@ -810,7 +833,7 @@ CancellationToken cancellationToken // (self-assigning Administrator), so it is refused for any non-admin caller — // including one reaching the store directly. if (!await authorizer.IsCurrentUserAdministratorAsync(cancellationToken)) - throw new IdentityAuthorizationException( + return NotAuthorized( await AdminRequiredMessageAsync( "Assigning a role requires administrator authority.", cancellationToken @@ -836,9 +859,28 @@ await currentUserService.GetCurrentUserIdAsync(cancellationToken) ) ); await session.SaveChangesAsync(cancellationToken); + return IdentityResult.Success; + } + + // IUserRoleStore can only return Task, so a Failed result from the overload + // above is adapted back into a throw here — the only channel UserManager understands. + async Task IUserRoleStore.AddToRoleAsync( + TUser user, + string roleName, + CancellationToken cancellationToken + ) + { + var result = await AddToRoleAsync(user, roleName, cancellationToken); + if (!result.Succeeded) + throw new IdentityAuthorizationException(result.Errors.First().Description); } - public async Task RemoveFromRoleAsync( + /// + /// Removes a role assignment; see for why this overload + /// returns while the + /// explicit implementation below still throws. + /// + public async Task RemoveFromRoleAsync( TUser user, string roleName, CancellationToken cancellationToken @@ -852,7 +894,7 @@ CancellationToken cancellationToken // Defence in depth (#69/#41): removing a role is an administrator-only operation, // independent of any UI [Authorize] guard. if (!await authorizer.IsCurrentUserAdministratorAsync(cancellationToken)) - throw new IdentityAuthorizationException( + return NotAuthorized( await AdminRequiredMessageAsync( "Removing a role requires administrator authority.", cancellationToken @@ -892,6 +934,20 @@ await currentUserService.GetCurrentUserIdAsync(cancellationToken) ) ); await session.SaveChangesAsync(cancellationToken); + return IdentityResult.Success; + } + + // IUserRoleStore can only return Task, so a Failed result from the overload + // above is adapted back into a throw here — the only channel UserManager understands. + async Task IUserRoleStore.RemoveFromRoleAsync( + TUser user, + string roleName, + CancellationToken cancellationToken + ) + { + var result = await RemoveFromRoleAsync(user, roleName, cancellationToken); + if (!result.Succeeded) + throw new IdentityAuthorizationException(result.Errors.First().Description); } public async Task> GetRolesAsync(TUser user, CancellationToken cancellationToken) @@ -926,7 +982,7 @@ await querySession return user.Roles.Any(r => r == role.RoleId); } - // IUserLockoutStore + #region IUserLockoutStore public Task GetLockoutEndDateAsync( TUser user, @@ -983,7 +1039,6 @@ CancellationToken cancellationToken using var session = documentStore.LightweightSession(); - // Acquire the exclusive stream lock before reading current state. await session.Events.AppendExclusive(userId.Value); var current = await session.LoadAsync(userId.Value, cancellationToken) ?? user; @@ -1032,6 +1087,8 @@ CancellationToken cancellationToken return Task.CompletedTask; } + #endregion IUserLockoutStore + public async Task> GetUsersInRoleAsync( string roleName, CancellationToken cancellationToken diff --git a/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreAuthorizationTests.cs b/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreAuthorizationTests.cs index 072033c..80512f3 100644 --- a/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreAuthorizationTests.cs +++ b/tests/AndreGoepel.Marten.Identity.IntegrationTests/Users/UserStoreAuthorizationTests.cs @@ -22,32 +22,34 @@ public class UserStoreAuthorizationTests(MartenFixture fixture) : IAsyncLifetime public ValueTask DisposeAsync() => ValueTask.CompletedTask; - // --- Role assignment (throws IdentityAuthorizationException) --- + // --- Role assignment (returns IdentityResult) --- [Fact] - public async Task AddToRoleAsync_NonAdminActor_Throws() + public async Task AddToRoleAsync_NonAdminActor_ReturnsNotAuthorized() { var target = await SeedUserAsync("target@example.com"); await SeedRoleAsync("Member"); var (store, _) = BuildEnforcing(UserId.New()); var user = await store.FindByIdAsync(target.ToString(), Ct); - await Assert.ThrowsAsync(() => - store.AddToRoleAsync(user!, "MEMBER", Ct) - ); + var result = await store.AddToRoleAsync(user!, "MEMBER", Ct); + + Assert.False(result.Succeeded); + Assert.Contains(result.Errors, e => e.Code == "NotAuthorized"); } [Fact] - public async Task AddToRoleAsync_AnonymousActor_Throws() + public async Task AddToRoleAsync_AnonymousActor_ReturnsNotAuthorized() { var target = await SeedUserAsync("target@example.com"); await SeedRoleAsync("Member"); var (store, _) = BuildEnforcing(default); // Guid.Empty — fails closed var user = await store.FindByIdAsync(target.ToString(), Ct); - await Assert.ThrowsAsync(() => - store.AddToRoleAsync(user!, "MEMBER", Ct) - ); + var result = await store.AddToRoleAsync(user!, "MEMBER", Ct); + + Assert.False(result.Succeeded); + Assert.Contains(result.Errors, e => e.Code == "NotAuthorized"); } [Fact] @@ -66,7 +68,7 @@ public async Task AddToRoleAsync_AdminActor_Succeeds() } [Fact] - public async Task RemoveFromRoleAsync_NonAdminActor_Throws() + public async Task RemoveFromRoleAsync_NonAdminActor_ReturnsNotAuthorized() { var target = await SeedUserAsync("target@example.com"); await SeedRoleAsync("Member"); @@ -74,9 +76,10 @@ public async Task RemoveFromRoleAsync_NonAdminActor_Throws() var (store, _) = BuildEnforcing(UserId.New()); var user = await store.FindByIdAsync(target.ToString(), Ct); - await Assert.ThrowsAsync(() => - store.RemoveFromRoleAsync(user!, "MEMBER", Ct) - ); + var result = await store.RemoveFromRoleAsync(user!, "MEMBER", Ct); + + Assert.False(result.Succeeded); + Assert.Contains(result.Errors, e => e.Code == "NotAuthorized"); } // --- Delete (self-or-admin; returns IdentityResult) --- From a88d464d8d932ab33b1f9a5790dd3eec8a155f6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20G=C3=B6pel?= Date: Sun, 26 Jul 2026 01:33:10 +0800 Subject: [PATCH 3/3] fix: thread CancellationToken through cleanup schedule startup fire-and-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. --- .../CleanupScheduleStartupService.cs | 58 ++++++++++++------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/src/AndreGoepel.Marten.Identity/CleanupScheduleStartupService.cs b/src/AndreGoepel.Marten.Identity/CleanupScheduleStartupService.cs index 483b813..c11c650 100644 --- a/src/AndreGoepel.Marten.Identity/CleanupScheduleStartupService.cs +++ b/src/AndreGoepel.Marten.Identity/CleanupScheduleStartupService.cs @@ -20,40 +20,54 @@ ILogger logger public Task StartAsync(CancellationToken cancellationToken) { - lifetime.ApplicationStarted.Register(() => _ = ApplyStoredScheduleAsync()); + var stoppingToken = lifetime.ApplicationStopping; + lifetime.ApplicationStarted.Register(() => + { + _ = RunApplyStoredScheduleAsync(stoppingToken); + }); return Task.CompletedTask; } public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; - private async Task ApplyStoredScheduleAsync() + // Guards the fire-and-forget invocation from StartAsync: an exception surfacing from an + // unawaited Task here would be unobserved and could crash the process. + private async Task RunApplyStoredScheduleAsync(CancellationToken cancellationToken) { try { - await using var session = documentStore.QuerySession(); - var settings = await session.LoadAsync(CleanupSettings.DocumentId); - if (settings is null) - return; - - var scheduler = await schedulerFactory.GetScheduler(); - var newTrigger = TriggerBuilder - .Create() - .WithIdentity(TriggerKey) - .ForJob(JobKey) - .WithCronSchedule(settings.CronSchedule) - .Build(); - - var nextFire = await scheduler.RescheduleJob(TriggerKey, newTrigger); - if (nextFire.HasValue) - logger.LogInformation( - "Applied stored cleanup schedule '{CronSchedule}'. Next run: {NextFire:u}.", - settings.CronSchedule, - nextFire.Value - ); + await ApplyStoredScheduleAsync(cancellationToken); } catch (Exception ex) { logger.LogError(ex, "Failed to apply stored cleanup schedule on startup."); } } + + private async Task ApplyStoredScheduleAsync(CancellationToken cancellationToken) + { + await using var session = documentStore.QuerySession(); + var settings = await session.LoadAsync( + CleanupSettings.DocumentId, + cancellationToken + ); + if (settings is null) + return; + + var scheduler = await schedulerFactory.GetScheduler(cancellationToken); + var newTrigger = TriggerBuilder + .Create() + .WithIdentity(TriggerKey) + .ForJob(JobKey) + .WithCronSchedule(settings.CronSchedule) + .Build(); + + var nextFire = await scheduler.RescheduleJob(TriggerKey, newTrigger, cancellationToken); + if (nextFire.HasValue) + logger.LogInformation( + "Applied stored cleanup schedule '{CronSchedule}'. Next run: {NextFire:u}.", + settings.CronSchedule, + nextFire.Value + ); + } }