From f8f20d9eb01f5e7d101944d560c58e7b20759982 Mon Sep 17 00:00:00 2001 From: Bernhard Windisch Date: Tue, 2 Jun 2026 07:38:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(identity):=20AppBase=20v3/v4=20hardening=20?= =?UTF-8?q?=E2=80=94=20passkey=20lifecycle,=20email=20normalization,=20GDP?= =?UTF-8?q?R=20erase=20PII=20sweep,=20recovery=20CLI?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the transferable hardening from AppBase v3/v4 that applies to Modgud's shared auth slice, and completes the gaps an adversarial self-review of the first pass surfaced. (The structural Control-Plane / system-DB separation is deferred to its own change; the external-login IsActive bypass is a separate security PR; the WritableStore runtime-config is a deliberate divergence — RealmSettings already delivers it.) Passkey lifecycle: - Register StoredPasskeyCredential in Marten (Identity + UserId index). - Cascade-delete passkeys on permanent erase (the reversible recycle-bin deliberately does NOT). - Passkey login now rejects IsDeleted users -- the direct LoadAsync bypasses the Identity store filter, so this is defense-in-depth for the soft-delete bypass. Unlink last-auth-method guard now counts passkeys: a passkey-only user (no password) was wrongly blocked from unlinking their last external identity. Email uniqueness -- normalize the app-level write-path checks to a case-insensitive NormalizedEmail/upper compare so case-variants are rejected with a clean error instead of a raw partial-unique-index conflict (the DB index stays the backstop): CreateUser, UpdateUser, recover set-email, AND self-registration (the fourth path the first pass missed). The compare uses the un-trimmed value to match how NormalizedEmail + the index are actually computed. GDPR permanent-erase now drops every user-keyed PII document, not just sessions / security-data / external-claims: also StoredPasskeyCredential, UserChangeRequest (payload holds name/email -- the section comment already claimed this), and EmailOtpChallenge (plaintext email). recover rebuild-projections: build the projection daemon for the resolved realm (honors --realm; defaults to system). MasterTableTenancy disables Marten's default tenant, so the previous no-arg call threw on this break-glass path. AuthLog: drop the gratuitous solo "marten" Marten schema -- public schema now. Full backend suite green: 1039 unit + 216 integration. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Features/Users/Commands/CreateUserCommand.cs | 5 +++-- .../Features/Users/Commands/UpdateUserCommand.cs | 5 +++-- .../Api/Account/PasskeyEndpoints.cs | 5 ++++- .../Modgud.Authentication/Api/Admin/RecoveryCli.cs | 14 +++++++++----- .../Api/ExternalAuth/ProfileLinkEndpoints.cs | 6 +++--- .../Modgud.Authentication/Gdpr/GdprService.cs | 14 ++++++++++++++ .../SelfRegistration/SelfRegistrationService.cs | 2 +- .../Setup/MartenStoreOptionsExtensions.cs | 11 ++++++++++- 8 files changed, 47 insertions(+), 15 deletions(-) diff --git a/src/dotnet/Modgud.Api/Features/Users/Commands/CreateUserCommand.cs b/src/dotnet/Modgud.Api/Features/Users/Commands/CreateUserCommand.cs index 606deed2..b81b038e 100644 --- a/src/dotnet/Modgud.Api/Features/Users/Commands/CreateUserCommand.cs +++ b/src/dotnet/Modgud.Api/Features/Users/Commands/CreateUserCommand.cs @@ -41,14 +41,15 @@ public async Task> Handle( // their own Email field but we care about collisions across both. if (!string.IsNullOrWhiteSpace(command.Email)) { + var normalizedEmail = command.Email.ToUpperInvariant(); var personEmailTaken = await session.Query() - .Where(p => p.Email == command.Email && !p.IsDeleted) + .Where(p => p.NormalizedEmail == normalizedEmail && !p.IsDeleted) .AnyAsync(ct); if (personEmailTaken) return DomainErrors.User.EmailTaken(command.Email); var groupEmailTaken = await session.Query() - .Where(g => g.Email == command.Email && !g.IsDeleted) + .Where(g => g.Email != null && g.Email.ToUpper() == normalizedEmail && !g.IsDeleted) .AnyAsync(ct); if (groupEmailTaken) return DomainErrors.User.EmailTaken(command.Email); diff --git a/src/dotnet/Modgud.Api/Features/Users/Commands/UpdateUserCommand.cs b/src/dotnet/Modgud.Api/Features/Users/Commands/UpdateUserCommand.cs index 7ddb4268..74e22d1b 100644 --- a/src/dotnet/Modgud.Api/Features/Users/Commands/UpdateUserCommand.cs +++ b/src/dotnet/Modgud.Api/Features/Users/Commands/UpdateUserCommand.cs @@ -57,14 +57,15 @@ public async Task> Handle( if (command.Email.HasValue && !string.IsNullOrWhiteSpace(command.Email.Value)) { var email = command.Email.Value; + var normalizedEmail = email.ToUpperInvariant(); var personEmailTaken = await session.Query() - .Where(p => p.Email == email && p.Id != command.UserId && !p.IsDeleted) + .Where(p => p.NormalizedEmail == normalizedEmail && p.Id != command.UserId && !p.IsDeleted) .AnyAsync(ct); if (personEmailTaken) return DomainErrors.User.EmailTaken(email); var groupEmailTaken = await session.Query() - .Where(g => g.Email == email && !g.IsDeleted) + .Where(g => g.Email != null && g.Email.ToUpper() == normalizedEmail && !g.IsDeleted) .AnyAsync(ct); if (groupEmailTaken) return DomainErrors.User.EmailTaken(email); diff --git a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs index 4ffeb8f7..1637484c 100644 --- a/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/Account/PasskeyEndpoints.cs @@ -306,7 +306,10 @@ public static WebApplication MapPasskeyEndpoints(this WebApplication application // Sign in var user = await session.LoadAsync(storedCredential.UserId); - if (user is null || !user.IsActive) + // Defense-in-depth: passkey login loads the user directly (bypassing + // the Identity store's filters), so reject deleted users explicitly — + // not just inactive ones — closing the soft-delete auth-bypass. + if (user is null || !user.IsActive || user.IsDeleted) { ModgudMeters.RecordLogin(ModgudMeters.LoginMethod.Passkey, ModgudMeters.LoginOutcome.Failure); return Results.Json(new { Message = "Invalid credentials" }, statusCode: 401); diff --git a/src/dotnet/Modgud.Authentication/Api/Admin/RecoveryCli.cs b/src/dotnet/Modgud.Authentication/Api/Admin/RecoveryCli.cs index 60b268df..17b564e4 100644 --- a/src/dotnet/Modgud.Authentication/Api/Admin/RecoveryCli.cs +++ b/src/dotnet/Modgud.Authentication/Api/Admin/RecoveryCli.cs @@ -64,7 +64,7 @@ public static async Task RunAsync(IServiceProvider services, string[] args, "reset-2fa" => await Reset2FaAsync(session, userManager, args), "set-email" => await SetEmailAsync(session, userManager, args), "magic-link" => await MagicLinkAsync(session, scope.ServiceProvider, args, conf, env), - "rebuild-projections" => await RebuildProjectionsAsync(scope.ServiceProvider), + "rebuild-projections" => await RebuildProjectionsAsync(scope.ServiceProvider, realmSlug), "bootstrap-admin" => await BootstrapAdminAsync(scope.ServiceProvider, args, realmSlug), "migrate-cc-credentials" => await MigrateClientCredentialsAsync(scope.ServiceProvider, args, realmSlug), "realm-add-domain" => await RealmAddDomainAsync(scope.ServiceProvider, args), @@ -248,11 +248,12 @@ private static async Task SetEmailAsync( // Uniqueness guard — same check UpdateUserCommand runs. The polymorphic // Principal projection is inline, so this is strongly consistent. + var normalizedEmail = newEmail.ToUpperInvariant(); var personConflict = await session.Query() - .Where(p => p.Email == newEmail && p.Id != user.Id && !p.IsDeleted) + .Where(p => p.NormalizedEmail == normalizedEmail && p.Id != user.Id && !p.IsDeleted) .AnyAsync(); var groupConflict = await session.Query() - .Where(g => g.Email == newEmail && !g.IsDeleted) + .Where(g => g.Email != null && g.Email.ToUpper() == normalizedEmail && !g.IsDeleted) .AnyAsync(); if (personConflict || groupConflict) return Error($"Email already in use by another principal: {newEmail}"); @@ -346,7 +347,7 @@ private static async Task MagicLinkAsync( /// change leaves mt_doc_principal empty so no user can claim /// app:admin until the principal projection is replayed. /// - private static async Task RebuildProjectionsAsync(IServiceProvider services) + private static async Task RebuildProjectionsAsync(IServiceProvider services, string tenantId) { var store = services.GetRequiredService(); var timeout = TimeSpan.FromMinutes(10); @@ -354,7 +355,10 @@ private static async Task RebuildProjectionsAsync(IServiceProvider services Console.WriteLine("Rebuilding Marten projections..."); Serilog.Log.Warning("Auth: Recovery rebuild-projections initiated"); - using var daemon = await store.BuildProjectionDaemonAsync(); + // MasterTableTenancy disables Marten's default tenant, so the no-arg + // overload throws DefaultTenantUsageDisabledException — build the daemon + // for the resolved realm's DB explicitly (honors --realm; default system). + using var daemon = await store.BuildProjectionDaemonAsync(tenantId); await daemon.RebuildProjectionAsync("ViewProjections", timeout, CancellationToken.None); Console.WriteLine(" OK ViewProjections"); diff --git a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs index 7075c9a7..34ce0ef9 100644 --- a/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs +++ b/src/dotnet/Modgud.Authentication/Api/ExternalAuth/ProfileLinkEndpoints.cs @@ -110,9 +110,9 @@ public static void MapProfileLinkEndpoints(this IEndpointRouteBuilder endpoints, .Where(l => l.UserId == userId.Value && !l.IsUnlinked && l.Id != link.Id) .AnyAsync(ct); var hasPassword = await userManager.HasPasswordAsync(user); - // Passkey count check would require a dedicated store query — approximate - // by trusting local auth fallback when a password is set. - if (!otherLinks && !hasPassword) + var hasPasskey = await writeSession.Query() + .AnyAsync(c => c.UserId == userId.Value, ct); + if (!otherLinks && !hasPassword && !hasPasskey) { return Results.BadRequest(new { diff --git a/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs b/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs index 7e23912b..cbd5599e 100644 --- a/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs +++ b/src/dotnet/Modgud.Authentication/Gdpr/GdprService.cs @@ -251,6 +251,20 @@ private async Task> PerformPermanentEraseAsync(Guid userId, Guid? // fully erases its PII (no stream to mask). Rides this same batch. session.Delete(userId); + // WebAuthn passkeys are raw-crypto docs keyed on the user id — drop + // them so a permanently-erased user leaves no orphaned credentials. + // (Recycle-bin / deactivate must NOT do this — that path is reversible.) + session.DeleteWhere(c => c.UserId == userId); + + // Terminal profile change-requests are retained for audit, but their + // payload carries the user's name/email — drop them so no plaintext + // PII survives the erase (the section comment above promised this). + session.DeleteWhere(r => r.UserId == userId); + + // Email-OTP challenge is a 1:1 doc (Id = userId) holding a plaintext + // email — drop it too. + session.Delete(userId); + // External identity links carry Email, DisplayName, and the raw IdP // claim payload on their OWN streams (keyed by link id). Drop the // projection doc here; the PII-bearing events are masked + archived diff --git a/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs b/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs index 802680ec..1e7ab4f1 100644 --- a/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs +++ b/src/dotnet/Modgud.Authentication/SelfRegistration/SelfRegistrationService.cs @@ -145,7 +145,7 @@ public async Task RegisterAsync( // STILL return the same success message — no email is sent so // the original account holder isn't bothered. var emailTaken = await session.Query() - .AnyAsync(p => p.Email == normalizedEmail && !p.IsDeleted, ct); + .AnyAsync(p => p.NormalizedEmail == normalizedEmail.ToUpperInvariant() && !p.IsDeleted, ct); if (emailTaken) return GenericSuccess; // Username collision IS surfaced — usernames are public-shape diff --git a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs index 0ec41b0a..870615eb 100644 --- a/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs +++ b/src/dotnet/Modgud.Authentication/Setup/MartenStoreOptionsExtensions.cs @@ -81,6 +81,13 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) .Index(x => x.UserId) .Index(x => x.ExpiresAt); + // WebAuthn/passkey credentials (raw crypto, not event-sourced). One per + // enrolled authenticator; indexed by UserId for the per-user list/login + // lookup and the cascade-delete on permanent erase. + options.Schema.For() + .Identity(x => x.Id) + .Index(x => x.UserId); + // GDPR: per-user deletion bookkeeping (pending request + masked flag). // Keyed on the user id so we can simply Load it. options.Schema.For() @@ -107,8 +114,10 @@ public static StoreOptions UseModgudAuthentication(this StoreOptions options) .Index(x => x.LoginProviderId) .Index(x => x.IsUnlinked); + // AuthLogDocument lives in the default (public) schema like every other + // auth doc — dropped the gratuitous solo "marten" schema (one schema + // fewer per tenant DB; aligns with AppBase v4). options.Schema.For() - .DatabaseSchemaName("marten") .Identity(x => x.Id) .Index(x => x.Timestamp);