Grandiras/ATProto.NET

Fork 0

fix(spaces)!: simplespace independent read/write access and authority interop #148

Merged

Grandiras merged 1 commit from issue-113-simplespace-access into main

2026-09-24 18:09:09 +00:00

Grandiras commented

2026-09-24 18:06:30 +00:00

Owner

Closes #113.

Brings com.atproto.simplespace in line with upstream's independent read/write access (bluesky-social/atproto#5496, change 1ce408be). The spaces-alpha PDS and bulletin already run it. Against them, v0.6 got a 400 from createSpace, threw on getSpace, had updateSpace(policy:) silently ignored, and called an addMember that no longer exists. This PR also fixes the authority-side interop bugs that surfaced while checking the SDK against the current reference. Wire shapes were checked against the lexicons, simplespace/manager.ts, space/util.ts, and the 003-space-access migration at permissioned-data@787a730f, and against bulletin 0acf237b.

Client (`ATProtoNet`)

createSpace / updateSpace / getSpace carry readPolicy + writePolicy in place of policy. CreateSpaceAsync defaults both to MemberListPolicy and always sends both, since both are required on the wire. On update each one is optional and replaces the old value wholesale.
addMember is removed. PutMemberAsync(space, did, read, write) is an upsert that replaces both flags, with a SpaceUri overload. PutSimpleSpaceMemberRequest replaces AddSimpleSpaceMemberRequest.
listMembers entries (SimpleSpaceMember) gain required Read / Write.
CheckUserAccessAsync(space, user, access, clientId?) takes the new required access parameter. SimpleSpaceAccess.Read / .Write are the known values.

Server (`ATProtoNet.Server`)

SimpleSpaceRecord, ISimpleSpaceStore, the in-memory and EF stores, and the EF entities carry ReadPolicy / WritePolicy. Members carry Read / Write. AddMemberAsync / IsMemberAsync become PutMemberAsync / GetMemberAsync.
PutSimpleSpaceMemberEndpoint (owner only) replaces addMember. Create, update, and get carry both policies. createSpace refuses a null policy instead of storing it.
ISpaceAccessPolicy also decides writes. SpaceAccessRequest.Access is SpaceAccessKind.Read (the default) or Write. SimpleSpaceAccessPolicy evaluates readPolicy + appAccess for credentials and writePolicy alone for writes. The owner is always admitted.
notifyWrite gates on write access and answers 403 to a refused writer, which is neither recorded nor forwarded. Pre-existing: there was no user perimeter at all.
Accepted notifyWrites are forwarded to registerNotify subscribers in the background (SpaceWriteNotifier.ForwardWriteAsync, never awaited on the request path, never faults), skipping the authority's own registration. Pre-existing: NotifyWriteAsync had no caller on the authority side. NotifySpaceDeletedAsync also skips the authority's own registration now.
Managing app: sends user= (was did=, pre-existing) and access=read|write, and omits clientId on write checks. aud is the full service identifier (did:web:…#forum), which is what bulletin verifies.
Delivery to an authority: {authority}#atproto_space_host resolves with the #atproto_pds fallback (SpaceAuthority.GetServiceEndpoint) and is addressed to the bare authority DID. Pre-existing: these notifications were dropped.
Outbound aud for every other subscriber is the service identifier it registered, fragment and all. Pre-existing: it was the bare DID.
Inbound notifyWrite aud accepts {authority, authority#atproto_space_host, ServiceDid}. Pre-existing: a host configured with its own ServiceDid refused every reference PDS.
notifyWrite.rev must be a TID, and hash must be present. Both are checked as InvalidRequest before auth.

Docs, sample, tests

docs/testing-spaces.md documents the alpha artifacts: the GHCR image, pinned by digest sha256:4b19b376… (built from 158c439b, which includes #5496), @atproto/pds@alpha / @atproto/dev-env@alpha, and the hosted PDS. The source recipe is pinned to 787a730f, and there is a new npm recipe that I ran. It needs pnpm overrides because several @atproto/lex* packages have broken plain 0.0.0 releases.
docs/spaces.md covers read/write policies, putMember, and writer-set semantics: writers admitted by writePolicy, an authority may exclude writers, and public write policy tracks non-members. It also covers the authority-side notification flow, a bespoke ISpaceAccessPolicy handling Write, the EF upgrade migration, and a link to samples/SpacesSample. docs/api-reference.md is updated to match.
The SpacesSample, SpaceNetworkFixture, and SimpleSpacePolicyTests are updated. Five new integration tests are added, including putMember(read: true, write: false): the member gets a credential but stays out of the writer set.

Breaking changes (migration)

CreateSpaceAsync / UpdateSpaceAsync: policy: becomes readPolicy: + writePolicy:. Pass the same value to both to keep the old behaviour. GetSimpleSpaceResponse.Policy becomes ReadPolicy / WritePolicy.
AddMemberAsync(space, did) becomes PutMemberAsync(space, did, read: true, write: true).
CheckUserAccessAsync: access is inserted before clientId.
SimpleSpaceRecord takes both policies. ISimpleSpaceStore has PutMemberAsync / GetMemberAsync in place of AddMemberAsync / IsMemberAsync. SpaceNsids.AddSimpleSpaceMember and AddSimpleSpaceMemberEndpoint become PutSimpleSpaceMember and PutSimpleSpaceMemberEndpoint.
EF schema: AtProtoSimpleSpaces.Policy becomes ReadPolicy + WritePolicy, and AtProtoSimpleSpaceMembers gains Read + Write. Consumers own their migrations. Copy Policy into both new columns before dropping it, and default the member flags to true for existing rows. A snippet is in docs/spaces.md. The model deliberately has no DB default on the flags, because EF would then skip sending false; a regression test covers this.
ISpaceAccessPolicy implementations must handle SpaceAccessKind.Write without an app perimeter, or they refuse every writer. ISimpleSpaceManagingAppClient.CheckUserAccessAsync takes a SpaceAccessKind.
ISpaceServiceAuthVerifier.VerifyAsync takes IReadOnlyCollection<string> accepted audiences. SpaceServiceAuthVerifier keeps a single-audience overload.
NotifyWriteEndpoint's constructor takes ISpaceAccessPolicy, plus an optional notifier and logger. DI users are unaffected.

Verification

Full solution build: 0 warnings, 0 errors.
Unit tests: 1751 passed, 0 failed, 5 skipped (pre-existing Aspire skips). The Spaces subset is 414 tests; the new and changed ones cover every item above. That includes managing-app wire params and aud, delivery to an ordinary-PDS authority, fragment aud, forwarding over a TestServer host, write-policy refusal (403), the three accepted audiences, TID rev rejected before auth, member flag round trips in both stores, and the EF false-flag regression.
Integration tests against the live alpha: I ran a TestNetworkNoAppView from @atproto/dev-env@0.0.0-spaces-alpha-20260915165437 (the same build as the GHCR image). All 31 Space* integration tests pass (26 existing, 5 new) with ATPROTO_REQUIRE_INTEGRATION=1, so none were skipped.
Upstream issues re-checked on 2026-09-24: #5487 (type → spaceType), #5491 (a repo param on registerNotify), and #5462 (an unauthenticated policy variant) are all still open. permissioned-data is still at 787a730f, and nothing has landed in its lexicons, so nothing was adopted.

Deferred / not in scope

G14–G18, G22, G23 (DID-doc strictness, kid allow-list, DPoP strictness, CAR hardening, space-type name/collections, open unions, sync.getBlob note) belong to the hardening PR.
The policy unions keep the closed [JsonPolymorphic] style, as agreed. An unknown $type from a future server (e.g. #5462) still throws.
Not fixed, flagged: an SDK repo host signs notifyWrite as its own ServiceDid, but the reference authority requires iss == repo. So notifications from SDK repo hosts to reference authorities are still refused. Likewise, bulletin requires iss = the space authority on checkUserAccess, which an SDK authority only satisfies when ServiceDid is the authority's DID. Fixing either needs per-account signing, which is outside this issue.

🤖 Generated with Claude Code

Closes #113. Brings `com.atproto.simplespace` in line with upstream's independent read/write access ([bluesky-social/atproto#5496](https://github.com/bluesky-social/atproto/pull/5496), change `1ce408be`). The spaces-alpha PDS and bulletin already run it. Against them, v0.6 got a 400 from `createSpace`, threw on `getSpace`, had `updateSpace(policy:)` silently ignored, and called an `addMember` that no longer exists. This PR also fixes the authority-side interop bugs that surfaced while checking the SDK against the current reference. Wire shapes were checked against the lexicons, `simplespace/manager.ts`, `space/util.ts`, and the `003-space-access` migration at `permissioned-data@787a730f`, and against bulletin `0acf237b`. ## Client (`ATProtoNet`) - `createSpace` / `updateSpace` / `getSpace` carry `readPolicy` + `writePolicy` in place of `policy`. `CreateSpaceAsync` defaults both to `MemberListPolicy` and always sends both, since both are required on the wire. On update each one is optional and replaces the old value wholesale. - `addMember` is removed. `PutMemberAsync(space, did, read, write)` is an upsert that replaces both flags, with a `SpaceUri` overload. `PutSimpleSpaceMemberRequest` replaces `AddSimpleSpaceMemberRequest`. - `listMembers` entries (`SimpleSpaceMember`) gain required `Read` / `Write`. - `CheckUserAccessAsync(space, user, access, clientId?)` takes the new required `access` parameter. `SimpleSpaceAccess.Read` / `.Write` are the known values. ## Server (`ATProtoNet.Server`) - `SimpleSpaceRecord`, `ISimpleSpaceStore`, the in-memory and EF stores, and the EF entities carry `ReadPolicy` / `WritePolicy`. Members carry `Read` / `Write`. `AddMemberAsync` / `IsMemberAsync` become `PutMemberAsync` / `GetMemberAsync`. - `PutSimpleSpaceMemberEndpoint` (owner only) replaces `addMember`. Create, update, and get carry both policies. `createSpace` refuses a `null` policy instead of storing it. - `ISpaceAccessPolicy` also decides writes. `SpaceAccessRequest.Access` is `SpaceAccessKind.Read` (the default) or `Write`. `SimpleSpaceAccessPolicy` evaluates `readPolicy` + `appAccess` for credentials and `writePolicy` alone for writes. The owner is always admitted. - **`notifyWrite` gates on write access** and answers 403 to a refused writer, which is neither recorded nor forwarded. *Pre-existing*: there was no user perimeter at all. - **Accepted `notifyWrite`s are forwarded** to `registerNotify` subscribers in the background (`SpaceWriteNotifier.ForwardWriteAsync`, never awaited on the request path, never faults), skipping the authority's own registration. *Pre-existing*: `NotifyWriteAsync` had no caller on the authority side. `NotifySpaceDeletedAsync` also skips the authority's own registration now. - **Managing app**: sends `user=` (was `did=`, *pre-existing*) and `access=read|write`, and omits `clientId` on write checks. `aud` is the full service identifier (`did:web:…#forum`), which is what bulletin verifies. - **Delivery to an authority**: `{authority}#atproto_space_host` resolves with the `#atproto_pds` fallback (`SpaceAuthority.GetServiceEndpoint`) and is addressed to the bare authority DID. *Pre-existing*: these notifications were dropped. - **Outbound `aud`** for every other subscriber is the service identifier it registered, fragment and all. *Pre-existing*: it was the bare DID. - **Inbound `notifyWrite` `aud`** accepts `{authority, authority#atproto_space_host, ServiceDid}`. *Pre-existing*: a host configured with its own `ServiceDid` refused every reference PDS. - `notifyWrite.rev` must be a TID, and `hash` must be present. Both are checked as `InvalidRequest` before auth. ## Docs, sample, tests - `docs/testing-spaces.md` documents the alpha artifacts: the GHCR image, pinned by digest `sha256:4b19b376…` (built from `158c439b`, which includes #5496), `@atproto/pds@alpha` / `@atproto/dev-env@alpha`, and the hosted PDS. The source recipe is pinned to `787a730f`, and there is a new npm recipe that I ran. It needs pnpm overrides because several `@atproto/lex*` packages have broken plain `0.0.0` releases. - `docs/spaces.md` covers read/write policies, `putMember`, and writer-set semantics: writers admitted by `writePolicy`, an authority may exclude writers, and `public` write policy tracks non-members. It also covers the authority-side notification flow, a bespoke `ISpaceAccessPolicy` handling `Write`, the EF upgrade migration, and a link to `samples/SpacesSample`. `docs/api-reference.md` is updated to match. - The `SpacesSample`, `SpaceNetworkFixture`, and `SimpleSpacePolicyTests` are updated. Five new integration tests are added, including `putMember(read: true, write: false)`: the member gets a credential but stays out of the writer set. ## Breaking changes (migration) - `CreateSpaceAsync` / `UpdateSpaceAsync`: `policy:` becomes `readPolicy:` + `writePolicy:`. Pass the same value to both to keep the old behaviour. `GetSimpleSpaceResponse.Policy` becomes `ReadPolicy` / `WritePolicy`. - `AddMemberAsync(space, did)` becomes `PutMemberAsync(space, did, read: true, write: true)`. - `CheckUserAccessAsync`: `access` is inserted before `clientId`. - `SimpleSpaceRecord` takes both policies. `ISimpleSpaceStore` has `PutMemberAsync` / `GetMemberAsync` in place of `AddMemberAsync` / `IsMemberAsync`. `SpaceNsids.AddSimpleSpaceMember` and `AddSimpleSpaceMemberEndpoint` become `PutSimpleSpaceMember` and `PutSimpleSpaceMemberEndpoint`. - EF schema: `AtProtoSimpleSpaces.Policy` becomes `ReadPolicy` + `WritePolicy`, and `AtProtoSimpleSpaceMembers` gains `Read` + `Write`. Consumers own their migrations. Copy `Policy` into both new columns before dropping it, and default the member flags to `true` for existing rows. A snippet is in docs/spaces.md. The model deliberately has no DB default on the flags, because EF would then skip sending `false`; a regression test covers this. - `ISpaceAccessPolicy` implementations must handle `SpaceAccessKind.Write` without an app perimeter, or they refuse every writer. `ISimpleSpaceManagingAppClient.CheckUserAccessAsync` takes a `SpaceAccessKind`. - `ISpaceServiceAuthVerifier.VerifyAsync` takes `IReadOnlyCollection<string>` accepted audiences. `SpaceServiceAuthVerifier` keeps a single-audience overload. - `NotifyWriteEndpoint`'s constructor takes `ISpaceAccessPolicy`, plus an optional notifier and logger. DI users are unaffected. ## Verification - Full solution build: 0 warnings, 0 errors. - Unit tests: 1751 passed, 0 failed, 5 skipped (pre-existing Aspire skips). The Spaces subset is 414 tests; the new and changed ones cover every item above. That includes managing-app wire params and `aud`, delivery to an ordinary-PDS authority, fragment `aud`, forwarding over a `TestServer` host, write-policy refusal (403), the three accepted audiences, TID `rev` rejected before auth, member flag round trips in both stores, and the EF `false`-flag regression. - **Integration tests against the live alpha**: I ran a `TestNetworkNoAppView` from `@atproto/dev-env@0.0.0-spaces-alpha-20260915165437` (the same build as the GHCR image). All 31 `Space*` integration tests pass (26 existing, 5 new) with `ATPROTO_REQUIRE_INTEGRATION=1`, so none were skipped. - Upstream issues re-checked on 2026-09-24: [#5487](https://github.com/bluesky-social/atproto/issues/5487) (`type` → `spaceType`), [#5491](https://github.com/bluesky-social/atproto/issues/5491) (a `repo` param on `registerNotify`), and [#5462](https://github.com/bluesky-social/atproto/issues/5462) (an unauthenticated policy variant) are all **still open**. `permissioned-data` is still at `787a730f`, and nothing has landed in its lexicons, so nothing was adopted. ## Deferred / not in scope - G14–G18, G22, G23 (DID-doc strictness, `kid` allow-list, DPoP strictness, CAR hardening, space-type name/collections, open unions, `sync.getBlob` note) belong to the hardening PR. - The policy unions keep the closed `[JsonPolymorphic]` style, as agreed. An unknown `$type` from a future server (e.g. #5462) still throws. - **Not fixed, flagged**: an SDK repo host signs `notifyWrite` as its own `ServiceDid`, but the reference authority requires `iss == repo`. So notifications from SDK repo hosts to *reference* authorities are still refused. Likewise, bulletin requires `iss` = the space authority on `checkUserAccess`, which an SDK authority only satisfies when `ServiceDid` is the authority's DID. Fixing either needs per-account signing, which is outside this issue. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Grandiras added 1 commit

2026-09-24 18:06:30 +00:00

fix(spaces)!: simplespace independent read/write access and authority interop (closes #113 )

CI / pds-integration (pull_request) Successful in 26s

Details

CI / build-and-test (pull_request) Successful in 59s

Details

Sync Closures to GitHub / sync-closure (pull_request) Successful in 7s

Details

4fce799b5e

Upstream split com.atproto.simplespace into independent read and write
access (bluesky-social/atproto#5496). The spaces-alpha PDS and bulletin
already run it, so every simplespace call except deleteSpace/removeMember
was wire-incompatible: createSpace 400, getSpace threw, updateSpace's
policy was silently ignored, and addMember no longer exists.

Client: readPolicy/writePolicy replace policy, putMember(read, write)
replaces addMember, listMembers carries the flags, and checkUserAccess
takes the required access parameter.

Server: the store, record, EF model and endpoints follow. ISpaceAccessPolicy
now also decides writes (SpaceAccessKind.Write, no app perimeter), and
notifyWrite answers 403 to a writer the write policy refuses. That fixes
several interop bugs that were already in v0.6.0:
- the authority applied no user perimeter to notifyWrite
- it never forwarded accepted writes to registerNotify subscribers
- the managing-app call sent did= instead of user=
- notifications to an authority on an ordinary PDS were dropped for
  want of the #atproto_pds fallback
- outbound aud dropped the subscriber's service fragment
- a host with its own ServiceDid refused reference PDSes' notifyWrite
- rev is now validated as a TID before auth

Docs: document the published alpha (GHCR image pinned by digest, npm
alpha, hosted PDS) and the EF data migration. The integration suite
passes 31/31 against @atproto/dev-env@0.0.0-spaces-alpha-20260915165437.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Grandiras commented

2026-09-24 18:06:39 +00:00

Author

Owner

@claude /review

claude-bot commented

2026-09-24 18:06:44 +00:00

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/583); the reply will link the full session transcript.  📜 [Live transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-148/run-583.md) — updating every ~5 minutes while the task runs.

claude-reviewer approved these changes

2026-09-24 18:07:51 +00:00

claude-reviewer left a comment

✅ Verdict: approve — no blocking issues found.

This is a large, carefully executed breaking-change PR bringing simplespace in line with upstream's read/write policy split. I traced the trickiest parts of the diff rather than skimming:

EF Core PutMemberAsync race handling (EfCoreSimpleSpaceStore.cs): the retry-on-insert-conflict logic correctly scopes the catch (DbUpdateException) when (entity is null) to only the insert path, and a losing insert falls through to a second attempt that finds the winner's row and overwrites both flags. Sound.
Fire-and-forget write forwarding (SpaceWriteNotifier.ForwardWriteAsync, called via Task.Run off the request path): I checked whether this risks using ASP.NET-scoped, request-disposed dependencies after the response completes. SpaceWriteNotifier, ISpaceAuthorityStore, and ISimpleSpaceStore are all registered TryAddSingleton in SpaceServerExtensions.cs, and the EF store uses IDbContextFactory per-call, so this is safe.
Audience/endpoint resolution for the authority's own #atproto_space_host registration vs. other subscribers: SpaceAuthority.GetServiceEndpoint's new HostServiceId branch correctly reuses GetHostEndpoint (with its #atproto_pds fallback), and SpaceWriteNotifier.Audience/IsAuthority correctly special-case only the authority's own subscription to address by bare DID while every other subscriber gets its full registered service identifier — consistent with the PR's stated fix for previously-dropped/misaddressed notifications.
notifyWrite validation ordering and accepted audiences: malformed rev/hash are rejected before the service-auth check, and AcceptedAudiences correctly includes the bare authority DID, {authority}#atproto_space_host, and (when distinct) the configured ServiceDid.
Wire models, endpoint routing (putMember replacing addMember), CHANGELOG entries, and the extensive doc/sample/test updates are all consistent with the described breaking changes.

I did not find any correctness issues worth blocking on.

⏱ 0m 58s · 9 turns · tokens: 85.0k in / 5.9k out (+754.2k cache-read)
📜 Full session transcript

✅ **Verdict: approve** — no blocking issues found. This is a large, carefully executed breaking-change PR bringing `simplespace` in line with upstream's read/write policy split. I traced the trickiest parts of the diff rather than skimming: - **EF Core `PutMemberAsync` race handling** (`EfCoreSimpleSpaceStore.cs`): the retry-on-insert-conflict logic correctly scopes the `catch (DbUpdateException) when (entity is null)` to only the insert path, and a losing insert falls through to a second attempt that finds the winner's row and overwrites both flags. Sound. - **Fire-and-forget write forwarding** (`SpaceWriteNotifier.ForwardWriteAsync`, called via `Task.Run` off the request path): I checked whether this risks using ASP.NET-scoped, request-disposed dependencies after the response completes. `SpaceWriteNotifier`, `ISpaceAuthorityStore`, and `ISimpleSpaceStore` are all registered `TryAddSingleton` in `SpaceServerExtensions.cs`, and the EF store uses `IDbContextFactory` per-call, so this is safe. - **Audience/endpoint resolution** for the authority's own `#atproto_space_host` registration vs. other subscribers: `SpaceAuthority.GetServiceEndpoint`'s new `HostServiceId` branch correctly reuses `GetHostEndpoint` (with its `#atproto_pds` fallback), and `SpaceWriteNotifier.Audience`/`IsAuthority` correctly special-case only the authority's own subscription to address by bare DID while every other subscriber gets its full registered service identifier — consistent with the PR's stated fix for previously-dropped/misaddressed notifications. - **`notifyWrite` validation ordering and accepted audiences**: malformed `rev`/`hash` are rejected before the service-auth check, and `AcceptedAudiences` correctly includes the bare authority DID, `{authority}#atproto_space_host`, and (when distinct) the configured `ServiceDid`. - Wire models, endpoint routing (`putMember` replacing `addMember`), CHANGELOG entries, and the extensive doc/sample/test updates are all consistent with the described breaking changes. I did not find any correctness issues worth blocking on. --- ⏱ 0m 58s · 9 turns · tokens: 85.0k in / 5.9k out (+754.2k cache-read) 📜 [Full session transcript](https://git.grandiras.net/Grandiras/claude-bot/src/branch/claude-logs/transcripts/Grandiras/ATProto.NET/pr-148/run-583.md)

Grandiras merged commit 3083494757 into main

2026-09-24 18:09:09 +00:00

Grandiras referenced this pull request from a commit

2026-09-24 18:09:09 +00:00

fix(spaces)!: simplespace independent read/write access and authority interop (#148)

Grandiras deleted branch issue-113-simplespace-access

2026-09-24 18:09:09 +00:00

Grandiras referenced this pull request

2026-09-24 18:11:16 +00:00

fix(repo): spec-correct MST and hardened DAG-CBOR / CAR parsing #147

Grandiras referenced this pull request

2026-09-24 18:14:11 +00:00