Claude: changes for issue #116 #121

Merged
Grandiras merged 2 commits from claude/issue-116 into main 2026-08-19 16:47:00 +00:00
Collaborator

Closes #116.

Closes #116.
Put the access request form on the access-denied page
All checks were successful
CI / build (pull_request) Successful in 29s
2036684a13
People sign in before they ask for a key: the common way to meet this
deployment is to try the front door, fail the allowlist, and land on
/access-denied. That page pointed at /request, where the first thing asked
for was the handle OAuth had just proven.

It now carries the form itself. No handle field — the identity came out of
the sign-in — and the name is prefilled from the Bluesky profile, so asking
is one button. Three things follow from the requester being identified:

- The request is stored with a verified DID. On a pending request that can
  only have come from OAuth, which the admin card now says ("Signed in"),
  and approval looks the profile up by DID rather than by handle — so a
  handle that has moved, or whose domain is down, no longer blocks it.
- No status link to keep: signing in again is the status check, so a second
  visit shows the pending state and the POST returns to /access-denied.
- One open request per person, since a duplicate is now recognisable.

Same POST /request endpoint, so the honeypot, rate limit and both field
gates stay in one place; it branches only on where errors render and where
the handle comes from. The anonymous /request page is unchanged.

The e2e fixture needed a visitor who is signed in and not a member, which
the Members table cannot produce because it *is* the allowlist — hence
/dev/login/{did}?handle=, still Development-only and still the one backdoor.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/292); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-121/run-292.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

💬 Verdict: comment — notes only, nothing blocking.

This is a large, well-scoped change implementing the /access-denied embedded request form, and it's unusually well cross-checked: the docs updates (access-requests.md, auth.md, testing.md), the entity/service changes, the shared POST /request handler, the dev-login ?handle= test hook, locale strings (both en/de), CSS, and the e2e spec all line up consistently with each other and with the extensive in-code rationale comments.

Walked the main risk areas:

  • Security: the DID stamped on a request (signedInDid) is taken only from ctx.User.FindFirst("did"), never from the posted form, so a signed-in visitor can't forge someone else's DID onto a request — good. The hidden handle form field is likewise overridden server-side by the cookie's handle claim whenever authenticated.
  • Encryption invariant: confirmed in KanbanDbContext.cs that AccessRequest.Did is left unconverted (plaintext, HasMaxLength(256)), so the new GetPendingForDidAsync query (WHERE Did == did) doesn't violate the "never filter on encrypted columns" rule from CLAUDE.md — the docs' claim that the column "stays plaintext" checks out against the actual model.
  • RequestsPanel approval logic: falling back to request.Did ?? profile?.Did and profile?.Handle ?? request.Handle correctly lets approval succeed even when the appview lookup fails for a verified request, matching the stated intent ("losing the appview costs the avatar and the display name, not the decision").
  • Dev-login test hook: gated behind IsDevelopment(), requires an explicit ?handle= so a mistyped DID still 404s instead of silently authenticating a stranger — matches the documented rationale.
  • Locale keys, CSS selectors, and the new e2e test's selectors (.access-identity code, #request-name, .request-verified, etc.) all match the actual markup added to AccessDenied.razor and RequestsPanel.razor.

One non-blocking note below on a duplicate-submission race; nothing here looks like it should block merging.


⏱ 1m 28s · 9 turns · tokens: 44.3k in / 7.1k out (+475.0k cache-read)
📜 Full session transcript

💬 **Verdict: comment** — notes only, nothing blocking. This is a large, well-scoped change implementing the /access-denied embedded request form, and it's unusually well cross-checked: the docs updates (access-requests.md, auth.md, testing.md), the entity/service changes, the shared POST /request handler, the dev-login `?handle=` test hook, locale strings (both en/de), CSS, and the e2e spec all line up consistently with each other and with the extensive in-code rationale comments. Walked the main risk areas: - **Security**: the DID stamped on a request (`signedInDid`) is taken only from `ctx.User.FindFirst("did")`, never from the posted form, so a signed-in visitor can't forge someone else's DID onto a request — good. The hidden `handle` form field is likewise overridden server-side by the cookie's `handle` claim whenever authenticated. - **Encryption invariant**: confirmed in `KanbanDbContext.cs` that `AccessRequest.Did` is left unconverted (plaintext, `HasMaxLength(256)`), so the new `GetPendingForDidAsync` query (`WHERE Did == did`) doesn't violate the "never filter on encrypted columns" rule from CLAUDE.md — the docs' claim that the column "stays plaintext" checks out against the actual model. - **RequestsPanel approval logic**: falling back to `request.Did ?? profile?.Did` and `profile?.Handle ?? request.Handle` correctly lets approval succeed even when the appview lookup fails for a verified request, matching the stated intent ("losing the appview costs the avatar and the display name, not the decision"). - **Dev-login test hook**: gated behind `IsDevelopment()`, requires an explicit `?handle=` so a mistyped DID still 404s instead of silently authenticating a stranger — matches the documented rationale. - Locale keys, CSS selectors, and the new e2e test's selectors (`.access-identity code`, `#request-name`, `.request-verified`, etc.) all match the actual markup added to `AccessDenied.razor` and `RequestsPanel.razor`. One non-blocking note below on a duplicate-submission race; nothing here looks like it should block merging. --- ⏱ 1m 28s · 9 turns · tokens: 44.3k in / 7.1k out (+475.0k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-121/run-292.md) <!-- claude-verdict:comment --> <!-- claude-reviewed-head:2036684a13b549a7e6011dd64d604d00dc7758ac -->
@ -713,1 +758,4 @@
!string.IsNullOrEmpty(signedInDid) ? $"/access-denied?error={error}"
: isInvite ? $"/request?kind=invite&error={error}"
: $"/request?error={error}";
Collaborator

The "one open request per signed-in visitor" guarantee is implemented as a read-then-write (GetPendingForDidAsync check, then CreateAsync) with no unique constraint or transaction tying the two together. Two near-simultaneous POSTs for the same DID (e.g. a genuine double-click before the first request's SaveChangesAsync commits) can both pass the pending check and both insert, producing two pending rows for the same person — the exact "impatient second click" scenario the surrounding comment says this prevents. Low severity (an admin just sees/denies a duplicate), but if the guarantee is meant to hold, a unique partial index on Did where Status = Pending (or a serializable transaction around the check+insert) would close the gap that this check alone doesn't.

The "one open request per signed-in visitor" guarantee is implemented as a read-then-write (`GetPendingForDidAsync` check, then `CreateAsync`) with no unique constraint or transaction tying the two together. Two near-simultaneous POSTs for the same DID (e.g. a genuine double-click before the first request's `SaveChangesAsync` commits) can both pass the pending check and both insert, producing two pending rows for the same person — the exact "impatient second click" scenario the surrounding comment says this prevents. Low severity (an admin just sees/denies a duplicate), but if the guarantee is meant to hold, a unique partial index on `Did` where `Status = Pending` (or a serializable transaction around the check+insert) would close the gap that this check alone doesn't.
Author
Collaborator

🤖 On it — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 On it — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/293); the reply will link the full session transcript. <!-- claude-run:comment --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-121/run-293.md) — updating every ~5 minutes while the task runs.
Make "one open request per person" a database rule
All checks were successful
CI / build (pull_request) Successful in 47s
c4229e1d61
The check in the /request handler read on one connection and inserted on
another, so two posts for the same DID racing each other could both pass
it and both insert — the double click the check exists to absorb.

Add a partial unique index over pending rows that carry a DID, move the
check into AccessRequestService.CreateAsync so both halves of the rule
live together, and answer the violation with the same AlreadyPending the
check returns: losing the race and reloading the page are the same event
from the requester's side. AccessRequestIndexTests pins the index name
and filter, since the service recognises the violation by name.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
Collaborator

The finding was right, and it's fixed on claude/issue-116 (commit c4229e1).

The finding

The reviewer's read of Program.cs was accurate: GetPendingForDidAsync ran on one connection and CreateAsync inserted on another, with nothing tying them together. Two posts for the same DID that overlap — exactly the impatient second click the check exists to absorb — both saw no pending request and both inserted. The check was the only thing standing behind the "one open request per person" guarantee the docs state, and it couldn't hold it alone.

What changed

  • A partial unique index is now the guarantee. IX_AccessRequests_PendingDid on AccessRequests(Did) WHERE "Status" = 0 AND "Did" IS NOT NULL — partial because a resolved request must not block someone asking again, and because every anonymous request has a null DID with nothing to collide on. Possible at all only because Did is one of the plaintext columns; the encrypted ones can never be constrained this way. Migration 20260819163948_AddPendingAccessRequestDidIndex, which cannot fail on existing data: before this PR a DID was written only at approval, in the same save that moves the row off Pending.
  • The rule moved into the service. The endpoint's read-then-write pair is gone; AccessRequestService.CreateAsync now holds both halves — the cheap pre-check that answers the ordinary reload, and a catch (DbUpdateException) matching the index by name that answers the race. Both return the new AccessRequestOutcome.AlreadyPending, which the endpoint treats as success, not error: from the requester's side losing the race and reloading are the same event, and both end at /access-denied showing the request they have. CreateAsync returns an AccessRequestCreation (following the BoardResolution idiom) instead of a nullable that meant "backlog full".
  • AccessRequestIndexTests pins the index's name and filter. The service recognises the violation by ConstraintName, so that coupling is a string, and a rename touching only the model would turn the backstop into an unhandled 500.
  • Docs updated in the same commit: access-requests.md (the "one open request per person" bullet now says why one check isn't enough), data.md (a bullet on uniqueness that a race can defeat in a service but not in the database), testing.md.

What I verified

  • dotnet build Ponente.sln clean; dotnet test Ponente.sln — 297 passed, 0 failed (only the pre-existing Calendar.razor CS8604 warning).
  • Installed PostgreSQL 15 in the container, applied the full migration chain with dotnet ef database update, and drove the index by hand: two anonymous pending rows (null DID) both insert, a first pending row for a DID inserts, a second pending row for the same DID is refused (duplicate key value violates unique constraint "IX_AccessRequests_PendingDid"), and approved/denied rows for that same DID still insert.
  • Drove the actual race against that database: eight concurrent CreateAsync calls for one DID → exactly 1 Created, 7 AlreadyPending, 1 row in the table, nothing thrown. Then re-ran it with the in-service pre-check deleted, so only the index and the catch were left — same result, which is what proves IsPendingDidConflict really matches the exception Npgsql raises rather than the pre-check quietly winning every time. That scratch test was removed before committing; it needs a live Postgres, which CI's unit suite doesn't have.

No image: the change is invisible from the UI — the same double-click reaches the same page, there is just no longer a second row behind it.

No new issues to file.


⏱ 8m 25s · 62 turns · tokens: 73.1k in / 29.5k out (+3982.3k cache-read)
📜 Full session transcript

The finding was right, and it's fixed on `claude/issue-116` (commit `c4229e1`). ## The finding The reviewer's read of `Program.cs` was accurate: `GetPendingForDidAsync` ran on one connection and `CreateAsync` inserted on another, with nothing tying them together. Two posts for the same DID that overlap — exactly the impatient second click the check exists to absorb — both saw no pending request and both inserted. The check was the only thing standing behind the "one open request per person" guarantee the docs state, and it couldn't hold it alone. ## What changed - **A partial unique index is now the guarantee.** `IX_AccessRequests_PendingDid` on `AccessRequests(Did)` `WHERE "Status" = 0 AND "Did" IS NOT NULL` — partial because a resolved request must not block someone asking again, and because every anonymous request has a null DID with nothing to collide on. Possible at all only because `Did` is one of the plaintext columns; the encrypted ones can never be constrained this way. Migration `20260819163948_AddPendingAccessRequestDidIndex`, which cannot fail on existing data: before this PR a DID was written only at approval, in the same save that moves the row off `Pending`. - **The rule moved into the service.** The endpoint's read-then-write pair is gone; `AccessRequestService.CreateAsync` now holds both halves — the cheap pre-check that answers the ordinary reload, and a `catch (DbUpdateException)` matching the index by name that answers the race. Both return the new `AccessRequestOutcome.AlreadyPending`, which the endpoint treats as success, not error: from the requester's side losing the race and reloading are the same event, and both end at `/access-denied` showing the request they have. `CreateAsync` returns an `AccessRequestCreation` (following the `BoardResolution` idiom) instead of a nullable that meant "backlog full". - **`AccessRequestIndexTests`** pins the index's name and filter. The service recognises the violation by `ConstraintName`, so that coupling is a string, and a rename touching only the model would turn the backstop into an unhandled 500. - Docs updated in the same commit: `access-requests.md` (the "one open request per person" bullet now says why one check isn't enough), `data.md` (a bullet on uniqueness that a race can defeat in a service but not in the database), `testing.md`. ## What I verified - `dotnet build Ponente.sln` clean; `dotnet test Ponente.sln` — 297 passed, 0 failed (only the pre-existing `Calendar.razor` CS8604 warning). - Installed PostgreSQL 15 in the container, applied the full migration chain with `dotnet ef database update`, and drove the index by hand: two anonymous pending rows (null DID) both insert, a first pending row for a DID inserts, a **second pending row for the same DID is refused** (`duplicate key value violates unique constraint "IX_AccessRequests_PendingDid"`), and approved/denied rows for that same DID still insert. - Drove the actual race against that database: eight concurrent `CreateAsync` calls for one DID → exactly **1 `Created`, 7 `AlreadyPending`, 1 row in the table**, nothing thrown. Then re-ran it with the in-service pre-check deleted, so only the index and the catch were left — same result, which is what proves `IsPendingDidConflict` really matches the exception Npgsql raises rather than the pre-check quietly winning every time. That scratch test was removed before committing; it needs a live Postgres, which CI's unit suite doesn't have. No image: the change is invisible from the UI — the same double-click reaches the same page, there is just no longer a second row behind it. No new issues to file. --- ⏱ 8m 25s · 62 turns · tokens: 73.1k in / 29.5k out (+3982.3k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/issue-121/run-293.md)
Author
Collaborator

🤖 Reviewing this PR — follow along in the action run; the reply will link the full session transcript.

📜 Live transcript — updating every ~5 minutes while the task runs.

🤖 Reviewing this PR — follow along in [the action run](https://git.grandiras.net/Grandiras/claude-bot/actions/runs/294); the reply will link the full session transcript. <!-- claude-run:pr_review --> 📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-121/run-294.md) — updating every ~5 minutes while the task runs.
claude-reviewer left a comment

Verdict: approve — no blocking issues found.

This is a follow-up commit on top of what I already reviewed positively (2026-08-19T16:34:59Z). It resolves the one non-blocking note from that pass: the "one open request per signed-in visitor" rule was previously a read-then-write with no atomicity guarantee, so two racing POSTs for the same DID could both slip past the check and both insert.

The fix is exactly what I suggested and it's implemented correctly:

  • A partial unique index (IX_AccessRequests_PendingDid) on AccessRequest.Did, filtered to Status = 0 AND Did IS NOT NULL (Pending = 0 confirmed in the enum), so a resolved request never blocks a fresh ask and anonymous rows (null Did) never collide.
  • AccessRequestService.CreateAsync now returns a proper AccessRequestCreation/AccessRequestOutcome result instead of a bare nullable, with BacklogFull and the new AlreadyPending distinguished. The pre-check (AnyAsync) still handles the ordinary reload cheaply, and a DbUpdateException catch keyed on PostgresException.ConstraintName == KanbanDbContext.PendingRequestDidIndex (Postgres does report a unique-index violation's constraint name as the index name, even for a plain CREATE UNIQUE INDEX, so this matches correctly) catches the genuine race and returns the same AlreadyPending outcome — so a losing double-click and a reload land the requester in the same place, as the docs describe.
  • Program.cs's /request handler switches on result.Outcome/result.Created correctly: BacklogFull still redirects to the full error, and both Created (for a signed-in poster) and AlreadyPending redirect to /access-denied rather than the anonymous status-token page — right, since AlreadyPending never produces a token to redirect to.
  • AccessRequestIndexTests builds the model against the Npgsql provider (no live DB needed) and pins both the index's uniqueness/column and its filter string, which is exactly the brittle string-matching contract (ConstraintName) that would otherwise silently degrade into an unhandled 500 on a rename — good coverage for a coupling that a compiler can't catch.
  • The migration's Up/Down are the plain create/drop-index pair you'd expect, and the Designer/snapshot files reflect the new index consistently.

I didn't find any new correctness or security issues in this delta. The prior pass's other checks (DID never taken from the form, encrypted-column invariants, dev-login gating, locale/CSS/test consistency) aren't touched by this commit and still hold. Good to merge.


⏱ 1m 28s · 15 turns · tokens: 56.4k in / 6.1k out (+1122.5k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a follow-up commit on top of what I already reviewed positively (2026-08-19T16:34:59Z). It resolves the one non-blocking note from that pass: the "one open request per signed-in visitor" rule was previously a read-then-write with no atomicity guarantee, so two racing POSTs for the same DID could both slip past the check and both insert. The fix is exactly what I suggested and it's implemented correctly: - A partial unique index (`IX_AccessRequests_PendingDid`) on `AccessRequest.Did`, filtered to `Status = 0 AND Did IS NOT NULL` (`Pending = 0` confirmed in the enum), so a resolved request never blocks a fresh ask and anonymous rows (null `Did`) never collide. - `AccessRequestService.CreateAsync` now returns a proper `AccessRequestCreation`/`AccessRequestOutcome` result instead of a bare nullable, with `BacklogFull` and the new `AlreadyPending` distinguished. The pre-check (`AnyAsync`) still handles the ordinary reload cheaply, and a `DbUpdateException` catch keyed on `PostgresException.ConstraintName == KanbanDbContext.PendingRequestDidIndex` (Postgres does report a unique-index violation's constraint name as the index name, even for a plain `CREATE UNIQUE INDEX`, so this matches correctly) catches the genuine race and returns the same `AlreadyPending` outcome — so a losing double-click and a reload land the requester in the same place, as the docs describe. - `Program.cs`'s `/request` handler switches on `result.Outcome`/`result.Created` correctly: `BacklogFull` still redirects to the `full` error, and both `Created` (for a signed-in poster) and `AlreadyPending` redirect to `/access-denied` rather than the anonymous status-token page — right, since `AlreadyPending` never produces a token to redirect to. - `AccessRequestIndexTests` builds the model against the Npgsql provider (no live DB needed) and pins both the index's uniqueness/column and its filter string, which is exactly the brittle string-matching contract (`ConstraintName`) that would otherwise silently degrade into an unhandled 500 on a rename — good coverage for a coupling that a compiler can't catch. - The migration's `Up`/`Down` are the plain create/drop-index pair you'd expect, and the Designer/snapshot files reflect the new index consistently. I didn't find any new correctness or security issues in this delta. The prior pass's other checks (DID never taken from the form, encrypted-column invariants, dev-login gating, locale/CSS/test consistency) aren't touched by this commit and still hold. Good to merge. --- ⏱ 1m 28s · 15 turns · tokens: 56.4k in / 6.1k out (+1122.5k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/Ponente/pr-121/run-294.md) <!-- claude-verdict:approve --> <!-- claude-reviewed-head:c4229e1d619cb3c44621cb4719a665b5ec46c1ad -->
Grandiras deleted branch claude/issue-116 2026-08-19 16:47:00 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Grandiras/Ponente!121
No description provided.