From 1da912d5e95f89fd3b4e2ca1fdaf9aeb7eb44366 Mon Sep 17 00:00:00 2001 From: Heimdall Date: Wed, 11 Nov 2026 16:39:48 +0000 Subject: [PATCH] Members: approve refuses no-membership/unknown accounts; neutral text for legacy suspensions (#65) --- docs/registration-gates.md | 2 +- .../Pages/App/Members/Index.cshtml | 4 +- .../Pages/App/Members/Index.cshtml.cs | 29 +++- .../MembersPageNoticeTests.cs | 131 ++++++++++++++++++ .../SuspensionReasonExposureTests.cs | 2 +- 5 files changed, 160 insertions(+), 8 deletions(-) diff --git a/docs/registration-gates.md b/docs/registration-gates.md index e9896f7..8bfb15e 100644 --- a/docs/registration-gates.md +++ b/docs/registration-gates.md @@ -11,7 +11,7 @@ This page scopes the three that did not exist, says where each lives, and record | Rate limits and brake | `OidcGuardMiddleware` (`docs/oidc-issuer.md`) | Merged. Covers `/connect/*` only, not the register endpoint. | | Reserved names | `BifrostIdentityService.IsReserved`, 28 names | Merged, at creation and at authorize. | | Chat tier, no vote | `Authenticated` / `PendingApproval` accounts chat and open no member or governance surface | Merged. | -| Suspension | `MembershipSuspensionService` | Merged. The reason is `Membership.SuspensionReason`, written with the status change and its receipt, cleared on unsuspend, projected only to admins on the members page; the receipt (`AuditEvent.Detail`) holds no reason text. Receipts written before the column existed still hold their reasons as history; the member dashboard keeps filtering suspension receipts out. An unsuspend still requires a reason, but nothing stores it. | +| Suspension | `MembershipSuspensionService` | Merged. The reason is `Membership.SuspensionReason`, written with the status change and its receipt, cleared on unsuspend, projected only to admins on the members page; the receipt (`AuditEvent.Detail`) holds no reason text. Receipts written before the column existed still hold their reasons as history; the member dashboard keeps filtering suspension receipts out. A Suspended row with no stored reason (suspended before the column existed) shows admins the fixed text "Suspended before reasons were recorded"; the old text is never read out of the receipt. An unsuspend still requires a reason, but nothing stores it. | | CAPTCHA | `Features/Registration/RegistrationChallengeService` | Built here. | | Consent receipt | `ConsentReceipt`, `TrainingNotice` | Built here. | | Email verification | nowhere | Scoped below, not built. | diff --git a/src/Bifrost.Web/Pages/App/Members/Index.cshtml b/src/Bifrost.Web/Pages/App/Members/Index.cshtml index 94e791d..6521c2b 100644 --- a/src/Bifrost.Web/Pages/App/Members/Index.cshtml +++ b/src/Bifrost.Web/Pages/App/Members/Index.cshtml @@ -191,9 +191,9 @@ @if (Model.Actor.CanManageMembership) { - @if (user.Status == MembershipStatus.Suspended && !string.IsNullOrEmpty(user.SuspensionReason)) + @if (user.Status == MembershipStatus.Suspended) { -
Suspended: @user.SuspensionReason
+
@(string.IsNullOrWhiteSpace(user.SuspensionReason) ? Bifrost.Web.Pages.App.Members.IndexModel.LegacySuspensionText : "Suspended: " + user.SuspensionReason)
}
diff --git a/src/Bifrost.Web/Pages/App/Members/Index.cshtml.cs b/src/Bifrost.Web/Pages/App/Members/Index.cshtml.cs index 55fa91f..a4bb901 100644 --- a/src/Bifrost.Web/Pages/App/Members/Index.cshtml.cs +++ b/src/Bifrost.Web/Pages/App/Members/Index.cshtml.cs @@ -35,6 +35,8 @@ public sealed class IndexModel( }; internal const string ApproveSuspendedNotice = "Nothing changed: a suspended account can only be lifted with Unsuspend."; + /// Admin-only text for a Suspended row whose reason predates the SuspensionReason column; the old text is never read back from the audit trail. + internal const string LegacySuspensionText = "Suspended before reasons were recorded"; internal const string NoMembershipNotice = "Nothing changed: that account has no membership yet."; internal const string OwnAdminRoleNotice = "Nothing changed: you cannot remove or change your own platform admin role."; internal const string RoleOnSuspendedNotice = "Nothing changed: a suspended account's roles cannot be changed until it is unsuspended."; @@ -119,9 +121,16 @@ public sealed class IndexModel( var user = await dbContext.UserAccounts .Include(x => x.Membership!) .ThenInclude(x => x.RoleAssignments) - .SingleAsync(x => x.Id == userAccountId, cancellationToken); + .SingleOrDefaultAsync(x => x.Id == userAccountId, cancellationToken); - if (user.Membership!.Status == MembershipStatus.Suspended) + // The chat tier holds no membership by design (docs/oidc-issuer.md); approval refuses rather than inventing one. + if (user is null || user.Membership is null) + { + Notice = user is null ? RefusalNotices[SuspensionOutcome.NoSuchAccount] : NoMembershipNotice; + return RedirectToPage(); + } + + if (user.Membership.Status == MembershipStatus.Suspended) { Notice = ApproveSuspendedNotice; // only Unsuspend lifts a suspension return RedirectToPage(); @@ -181,7 +190,13 @@ public sealed class IndexModel( var user = await dbContext.UserAccounts .Include(x => x.Membership!) .ThenInclude(x => x.RoleAssignments) - .SingleAsync(x => x.Id == userAccountId, cancellationToken); + .SingleOrDefaultAsync(x => x.Id == userAccountId, cancellationToken); + + if (user is null) + { + Notice = RefusalNotices[SuspensionOutcome.NoSuchAccount]; + return RedirectToPage(); + } if (user.Membership is null) { @@ -259,7 +274,13 @@ public sealed class IndexModel( var user = await dbContext.UserAccounts .Include(x => x.Membership!) .ThenInclude(x => x.TierSnapshots) - .SingleAsync(x => x.Id == userAccountId, cancellationToken); + .SingleOrDefaultAsync(x => x.Id == userAccountId, cancellationToken); + + if (user is null) + { + Notice = RefusalNotices[SuspensionOutcome.NoSuchAccount]; + return RedirectToPage(); + } if (user.Membership is null) { diff --git a/tests/Bifrost.Web.Tests/MembersPageNoticeTests.cs b/tests/Bifrost.Web.Tests/MembersPageNoticeTests.cs index b4261d8..ec87c6d 100644 --- a/tests/Bifrost.Web.Tests/MembersPageNoticeTests.cs +++ b/tests/Bifrost.Web.Tests/MembersPageNoticeTests.cs @@ -174,6 +174,137 @@ public sealed class MembersPageNoticeTests(TestWebApplicationFactory factory) : Assert.Contains("has no membership yet", Alert.Match(html).Groups[1].Value); } + private async Task<(int Memberships, int Roles, int ApprovalReceipts)> RowCountsAsync() + { + using var scope = factory.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + return (await db.Memberships.CountAsync(), await db.RoleAssignments.CountAsync(), await db.AuditEvents.CountAsync(x => x.Action == "membership.approved")); + } + + [Fact] + public async Task Approving_an_account_without_a_membership_is_refused_with_a_notice_and_writes_nothing() + { + using var client = Client(); + _ = await SignedInAdminIdAsync(client); + var account = await SeedMemberAsync(MembershipStatus.Active, withMembership: false); + var before = await RowCountsAsync(); + + var html = await PostThenFollowAsync(client, "Approve", account, ""); + + Assert.Contains("has no membership yet", Alert.Match(html).Groups[1].Value); + Assert.Equal(before, await RowCountsAsync()); + using var scope = factory.Services.CreateScope(); + Assert.False(await scope.ServiceProvider.GetRequiredService().Memberships.AnyAsync(x => x.UserAccountId == account)); + } + + [Fact] + public async Task Approving_an_unknown_account_is_refused_with_a_notice_and_writes_nothing() + { + using var client = Client(); + _ = await SignedInAdminIdAsync(client); + var before = await RowCountsAsync(); + + var html = await PostThenFollowAsync(client, "Approve", Guid.NewGuid(), ""); + + Assert.Contains("does not exist", Alert.Match(html).Groups[1].Value); + Assert.Equal(before, await RowCountsAsync()); + } + + [Fact] + public async Task An_admin_sees_the_neutral_text_for_a_legacy_suspended_row_and_the_stored_reason_otherwise() + { + using var client = Client(); + _ = await SignedInAdminIdAsync(client); + var legacy = await SeedMemberAsync(MembershipStatus.Suspended); + var withReason = await SeedMemberAsync(MembershipStatus.Suspended); + using (var scope = factory.Services.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + (await db.Memberships.SingleAsync(x => x.UserAccountId == withReason)).SuspensionReason = "spam-ring-5521"; + await db.SaveChangesAsync(); + } + + var html = await client.GetStringAsync("/App/Members"); + int legacyRows; + using (var scope = factory.Services.CreateScope()) + { + legacyRows = await scope.ServiceProvider.GetRequiredService().Memberships + .CountAsync(x => x.Status == MembershipStatus.Suspended && (x.SuspensionReason == null || x.SuspensionReason.Trim() == "")); + } + + Assert.Contains("Suspended before reasons were recorded", html); + Assert.Contains("Suspended: spam-ring-5521", html); + Assert.Equal(legacyRows, Regex.Matches(html, "Suspended before reasons were recorded").Count); // the reasoned row shows its reason, not the neutral text + } + + [Fact] + public async Task A_non_admin_sees_neither_the_neutral_text_nor_a_reason() + { + using var client = Client(); + var legacy = await SeedMemberAsync(MembershipStatus.Suspended); + var viewer = $"plain-{Guid.NewGuid():N}"[..20]; + using (var scope = factory.Services.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + db.UserAccounts.Add(new UserAccount { DisplayName = viewer, BifrostIdentity = viewer, NormalizedBifrostIdentity = viewer, Membership = new Bifrost.Web.Domain.Membership { Status = MembershipStatus.Active, ApprovedAtUtc = DateTimeOffset.UtcNow } }); + await db.SaveChangesAsync(); + } + + using var request = new HttpRequestMessage(HttpMethod.Get, "/App/Members"); + request.Headers.Add("X-Test-Bifrost-Identity", viewer); + using var response = await client.SendAsync(request); + var html = await response.Content.ReadAsStringAsync(); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.DoesNotContain("before reasons were recorded", html); + } + + [Theory] + [InlineData("ToggleRole")] + [InlineData("SetTier")] + public async Task ToggleRole_and_SetTier_on_an_unknown_account_are_refused_with_a_notice_and_write_nothing(string handler) + { + using var client = Client(); + _ = await SignedInAdminIdAsync(client); + var before = await RowCountsAsync(); + int receipts; + using (var scope = factory.Services.CreateScope()) + { + receipts = await scope.ServiceProvider.GetRequiredService().AuditEvents.CountAsync(); + } + + var html = await PostThenFollowAsync(client, handler, Guid.NewGuid(), "", role: "Producer"); + + Assert.Contains("does not exist", Alert.Match(html).Groups[1].Value); + Assert.Equal(before, await RowCountsAsync()); + using var after = factory.Services.CreateScope(); + Assert.Equal(receipts, await after.ServiceProvider.GetRequiredService().AuditEvents.CountAsync()); + } + + [Fact] + public async Task A_whitespace_only_stored_reason_shows_the_neutral_text_to_an_admin() + { + using var client = Client(); + _ = await SignedInAdminIdAsync(client); + var member = await SeedMemberAsync(MembershipStatus.Suspended); + using (var scope = factory.Services.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + (await db.Memberships.SingleAsync(x => x.UserAccountId == member)).SuspensionReason = " "; + await db.SaveChangesAsync(); + } + + var html = await client.GetStringAsync("/App/Members"); + + Assert.DoesNotContain("Suspended: <", html); + using var check = factory.Services.CreateScope(); + var db2 = check.ServiceProvider.GetRequiredService(); + var whitespaceRows = await db2.Memberships.CountAsync(x => x.Status == MembershipStatus.Suspended && x.SuspensionReason != null && x.SuspensionReason.Trim() == ""); + Assert.True(whitespaceRows >= 1); + Assert.Equal(await db2.Memberships.CountAsync(x => x.Status == MembershipStatus.Suspended && (x.SuspensionReason == null || x.SuspensionReason.Trim() == "")), + Regex.Matches(html, "Suspended before reasons were recorded").Count); + } + [Fact] public async Task An_admin_toggling_their_own_platform_admin_role_is_refused_and_keeps_the_role() { diff --git a/tests/Bifrost.Web.Tests/SuspensionReasonExposureTests.cs b/tests/Bifrost.Web.Tests/SuspensionReasonExposureTests.cs index 0921036..0a02cb0 100644 --- a/tests/Bifrost.Web.Tests/SuspensionReasonExposureTests.cs +++ b/tests/Bifrost.Web.Tests/SuspensionReasonExposureTests.cs @@ -65,7 +65,7 @@ public sealed class SuspensionReasonExposureTests return model.Users; } - Assert.All(await UsersSeenBy(admin: false), u => Assert.DoesNotContain(Canary, u.SuspensionReason)); + Assert.All(await UsersSeenBy(admin: false), u => Assert.Equal(string.Empty, u.SuspensionReason)); // not even the legacy-row marker leaves the projection for a non-admin Assert.Contains(await UsersSeenBy(admin: true), u => u.SuspensionReason == Canary); }