Claude: changes for issue #116 #121
No reviewers
Labels
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Grandiras/Ponente!121
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "claude/issue-116"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #116.
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>🤖 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.
💬 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:
signedInDid) is taken only fromctx.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 hiddenhandleform field is likewise overridden server-side by the cookie'shandleclaim whenever authenticated.KanbanDbContext.csthatAccessRequest.Didis left unconverted (plaintext,HasMaxLength(256)), so the newGetPendingForDidAsyncquery (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.request.Did ?? profile?.Didandprofile?.Handle ?? request.Handlecorrectly 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").IsDevelopment(), requires an explicit?handle=so a mistyped DID still 404s instead of silently authenticating a stranger — matches the documented rationale..access-identity code,#request-name,.request-verified, etc.) all match the actual markup added toAccessDenied.razorandRequestsPanel.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
@ -713,1 +758,4 @@!string.IsNullOrEmpty(signedInDid) ? $"/access-denied?error={error}": isInvite ? $"/request?kind=invite&error={error}": $"/request?error={error}";The "one open request per signed-in visitor" guarantee is implemented as a read-then-write (
GetPendingForDidAsynccheck, thenCreateAsync) 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'sSaveChangesAsynccommits) 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 onDidwhereStatus = Pending(or a serializable transaction around the check+insert) would close the gap that this check alone doesn't.🤖 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.
The finding was right, and it's fixed on
claude/issue-116(commitc4229e1).The finding
The reviewer's read of
Program.cswas accurate:GetPendingForDidAsyncran on one connection andCreateAsyncinserted 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
IX_AccessRequests_PendingDidonAccessRequests(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 becauseDidis one of the plaintext columns; the encrypted ones can never be constrained this way. Migration20260819163948_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 offPending.AccessRequestService.CreateAsyncnow holds both halves — the cheap pre-check that answers the ordinary reload, and acatch (DbUpdateException)matching the index by name that answers the race. Both return the newAccessRequestOutcome.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-deniedshowing the request they have.CreateAsyncreturns anAccessRequestCreation(following theBoardResolutionidiom) instead of a nullable that meant "backlog full".AccessRequestIndexTestspins the index's name and filter. The service recognises the violation byConstraintName, so that coupling is a string, and a rename touching only the model would turn the backstop into an unhandled 500.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.slnclean;dotnet test Ponente.sln— 297 passed, 0 failed (only the pre-existingCalendar.razorCS8604 warning).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.CreateAsynccalls for one DID → exactly 1Created, 7AlreadyPending, 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 provesIsPendingDidConflictreally 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
🤖 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.
✅ 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:
IX_AccessRequests_PendingDid) onAccessRequest.Did, filtered toStatus = 0 AND Did IS NOT NULL(Pending = 0confirmed in the enum), so a resolved request never blocks a fresh ask and anonymous rows (nullDid) never collide.AccessRequestService.CreateAsyncnow returns a properAccessRequestCreation/AccessRequestOutcomeresult instead of a bare nullable, withBacklogFulland the newAlreadyPendingdistinguished. The pre-check (AnyAsync) still handles the ordinary reload cheaply, and aDbUpdateExceptioncatch keyed onPostgresException.ConstraintName == KanbanDbContext.PendingRequestDidIndex(Postgres does report a unique-index violation's constraint name as the index name, even for a plainCREATE UNIQUE INDEX, so this matches correctly) catches the genuine race and returns the sameAlreadyPendingoutcome — so a losing double-click and a reload land the requester in the same place, as the docs describe.Program.cs's/requesthandler switches onresult.Outcome/result.Createdcorrectly:BacklogFullstill redirects to thefullerror, and bothCreated(for a signed-in poster) andAlreadyPendingredirect to/access-deniedrather than the anonymous status-token page — right, sinceAlreadyPendingnever produces a token to redirect to.AccessRequestIndexTestsbuilds 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.Up/Downare 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