fix(deletion): require sole owner at admission - #7966
Conversation
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
161e985 to
c593003
Compare
Signed-off-by: Codex <noreply@openai.com> Co-authored-by: Codex <noreply@openai.com>
Give create and transfer-in limit_reached rejections a stable code, keeping the message prefix for older clients. Document that an unsupported acknowledgement version returns 400 before the tuple comparison, that active-cap overrides above the lifetime cap are unreachable, and the sole-current-owner guard on the delete handler. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
c593003 to
ea6fba8
Compare
…rd in test Address Legolas P2s on #7966: the UUID conflict docs name every non-converging case, the runbook explains that legacy co-owned communities are rejected as community_not_found, and the preparation drift test matches the sole-owner authority guard instead of any owner deletion error. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
…rd in test Address Legolas P2s on #7966: the UUID conflict docs name every non-converging case, the runbook explains that legacy co-owned communities are rejected as community_not_found, and the preparation drift test matches the sole-owner authority guard instead of any owner deletion error. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
579e2dd to
a0fc97d
Compare
…nities Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz> Co-authored-by: Codex <noreply@openai.com>
…e host A known request UUID resent for a different host is a request conflict, not a community_id mismatch, even when community_id names that other host. The replay mismatch check now applies only to a stored request for the same host, and the fresh-path check runs after sole-owner authority so non-owners see the same 404 as an unknown host. Fold admit_owner_request_with_community_id into admit_owner_request with an expected_community_id parameter, and pin the UUID-collision, matching replay and non-owner cases in operator tests. Update the runbook. Signed-off-by: Elrond <28d6302a099e5225b02c4155ac4236e4912603df2ab08dbfc2f4fef08ce598c8@buzz.block.builderlab.xyz>
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 Automated multi-lane review at head 766eb07f, against base d7a35afa. Two independent code reviews plus an end-to-end verification run found no blocking issues. Thanks for closing the admission/completion gap from #7830.
What was checked
- Lock order. Admission locks the community row, then every owner membership in pubkey order, and checks both the owner count and the asserted pubkey against those locked rows, not an earlier read (
crates/buzz-db/src/store/deletion.rs:920-950). Preparation and ownership transfer take the community lock before owner rows in the same order. Admission never waits on the deletion advisory lock and reads existing requests without row locks, so it does not form a new wait cycle with the abort path. - Error precedence. Unsupported acknowledgement version →
400before the UUID lookup; a known UUID on a different host →409 deletion_request_conflictwithout resolving that host; a fresh UUID proves sole-owner authority before thecommunity_idcheck, so a non-owner still gets404. Replay reads only the stored request, so it does not reveal whether some other community currently exists. - Rename. No
deletion_api_errorreferences remain at this head; every former caller now usescoded_api_error. - Guards are load-bearing. Removing each of these one at a time turned exactly one PostgreSQL test red (70/71), and restoring it returned 71/71: the sole-owner count check, the
community_idmismatch check on a fresh submission, the same check on replay, and the same-host scoping of the replay check. The 48 deletion and 23 operator PostgreSQL tests pass unmodified. - Live HTTP run. Against a local relay with real PostgreSQL and Redis and signed NIP-98 requests, the runbook recovery worked end to end: a legacy co-owned deletion is rejected with
404and no intent row, then after unarchive, self-transfer, and re-archive, the same request UUID is accepted with202 submitted. Wrongcommunity_idvalues returned409on both fresh submission and replay.
MINOR — the combined replay error isn't documented
On a same-host replay, the community_id check runs before the owner, operator-origin, and stored-acknowledgement checks. So a replay that has a wrong community_id and a changed owner gets 409 community_id_mismatch. docs/operator-community-deletion.md:159-164 and the delete_community doc (crates/buzz-relay/src/api/operator.rs:471-476) say a changed owner under a known UUID is 409 deletion_request_conflict, with no exception noted. Both are 409s and neither mutates anything, so this is not an authorization issue. Either state that community_id_mismatch wins for a same-host replay, or move the ID check after the rest of the replay tuple matches, and add a regression test for the combined case.
MINOR — PR description is missing the community_id binding
ab0cd7130 and 766eb07fd add the optional community_id field and its same-host replay scoping, but the description doesn't mention either. Worth adding, since it's a new request field in the operator API.
Main added sole-owner admission for owner-requested community deletion (#7966). It changes deletion.rs, which this branch also changes, but in a different part of the file: main edits owner admission and its tests, this branch adds the two personal read tables to the purge lists. The merge is textually clean and needs no follow-on edit. This branch's diff against main is unchanged by the merge: the same 31 files, each with the same added and removed lines. Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Follow-up to #7830: reject owner-origin deletion admission for a legacy co-owned community before creating a request. Under the existing community-row lock, admission locks all current owner memberships in pubkey order and requires exactly one matching asserted owner. Existing-request idempotent replay remains unchanged; completion retains its independent post-lock revalidation.
New optional operator field:
community_id(ab0cd713,766eb07f). Owner-originPOST /operator/communities/deletenow accepts an optionalcommunity_idthat binds the request to the host's community:404 community_not_found. A mismatch returns409 community_id_mismatchand writes no request row.409 community_id_mismatch, and the stored request is unchanged. A known UUID sent with a different host goes through the existing convergence check and returns409 deletion_request_conflict.admit_owner_requesttakesexpected_community_id: Option<Uuid>rather than a second public entry point, and existing callers passNone.docs/operator-community-deletion.mddocuments the ordering.Also folds in the non-blocking review nits from #7969 (wpfleger96, at
e21151f4):limit_reachedhas a stablecode. Create (provision_community) and transfer-in now return409withcode: "limit_reached", matching the other coded lifecycle errors. Theerrormessage keeps itslimit_reached:prefix, so clients that match on the message still work. The provision limit test now asserts the code, and a new PostgreSQL test covers a transfer to an owner at the limit (coded 409, no membership change).docs/operator-community-deletion.mdand thedelete_communityhandler doc now say a changed host or owner under a known UUID is409 deletion_request_conflict, and an unsupported acknowledgement version is rejected first with400 unsupported_acknowledgement_version.max_communities_per_ownerdoc and the quota section now sayMAX_LIFETIME_COMMUNITIES_PER_OWNER(20) counts live ownership, soBUZZ_MAX_COMMUNITIES_PER_OWNERabove 20 can't be reached.404 community_not_foundand should be converged with a transfer first.DeletionSafetyplus "sole-owner authority drifted".delete_communityhandler doc now says "sole current owner", matching the admission check.What got simpler: admission and automatic preparation now enforce the same sole-owner authority, so no accepted-but-unexecutable co-owned request needs a new recovery path.
deletion_api_erroris renamed tocoded_api_error, since it now carries non-deletion codes too. There's one helper for coded operator errors instead of a second one.Rebased onto main after #7969 merged (
d7a35afa). The sole-owner commit applied cleanly.Earlier focused Blox verification (at the pre-rebase head
161e9857): the new admission regression was red before the change and green after it. Five temporary, restored mutants each failed its matching production-seam regression. At the current head766eb07f, the relayapi::operator::suite passes 23/0 on Blox. Each newcommunity_idguard, removed one at a time, fails its own regression test. CI owns the full suites.Remaining debt: the separate P2-A purge/completion membership/community lock-order cycle is unchanged.
Generated with Codex