feat(group): add app.certified.group.import to promote existing accounts#30
Draft
aspiers wants to merge 3 commits into
Draft
Conversation
Design for app.certified.group.import (HYPER-469), the sibling to group.register that promotes an existing PDS account into a group rather than creating a new account. The #certified_group DID-document entry is addressed two ways, and import gates on neither: - Not currently relied upon: service proxying is not in use; clients call CGS directly (the shipped HYPER-453 workaround), which has no dependence on the group's #certified_group entry resolving. A group is fully functional without it. - Not possible via app password anyway: DID-doc updates require the ACCESS_FULL scope; app-password logins only ever receive AppPass/AppPassPrivileged. register sidesteps this by holding a genesis rotation key, which import has no equivalent for. So import neither adds nor requires the entry (no MissingServiceEntry error). If proxying ever becomes a requirement, the owner adds the entry out-of-band with a full-access credential. Other open questions resolved: - Credible exit: encrypted_recovery_key is NULL; the owner's own pre-existing credentials are the exit (CGS is not a key custodian). - App password: stored as supplied, owner-managed/revocable, not re-minted. No schema change needed (encrypted_recovery_key already nullable, pds_url already per-group). Notes that imported accounts may live on a different PDS, so pds_url is resolved from the DID doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add app.certified.group.import (HYPER-469), the sibling of group.register that imports an existing PDS account as a group instead of creating a new one. The caller supplies an app password so the service can act on the account's behalf; the account's PDS and handle are resolved from its DID document, so an imported account may live on a PDS other than GROUP_PDS_URL. Key behaviours (per docs/design/group-import.md): - Auth via a new AuthVerifier.verifyImport (aud = service DID, lxm = app.certified.group.import). Refactored the shared body out of verifyRegistration rather than duplicating it; register hardcodes its own NSID, so import could not reuse verifyRegistration directly. - No DID-document mutation. Service proxying is not currently relied upon (clients call CGS directly), and an app password cannot perform PLC operations anyway (needs ACCESS_FULL; app passwords only ever get AppPass/AppPassPrivileged). So import neither adds nor requires the #certified_group entry. - No recovery key: encrypted_recovery_key is left NULL (column already nullable). The owner's own pre-existing credentials are their credible exit; CGS is not a key custodian for imported groups. - App password stored as supplied, owner-managed and revocable. Extracted the shared register/import tail (encrypt + store credentials, migrate per-group DB, seed owner, audit-log) into finalizeGroup() so the two handlers share it instead of copy-pasting. Exposed IdResolver on AppContext for DID resolution. Mounted as a raw route with JSON parsing, alongside register. Tests: tests/import.test.ts (success, cross-PDS pds_url, InvalidAppPassword on login failure, GroupAlreadyRegistered, owner seeding, validation, auth mismatch, unauthenticated). Full suite green (226 tests). Docs and a changeset added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…outes
register and import were plain Express routes mounted outside the XRPC
server ("raw routes"), so they bypassed lexicon validation and did their
own JWT verification. They only existed that way because the XRPC
server's default auth (xrpcAuth -> verify) treats the JWT `aud` as a
group DID and looks it up in the `groups` table — which fails for a
group that doesn't exist yet. That is the group-bootstrapping case.
Both endpoints already use `aud` correctly (= the service DID), so the
fix is simply to register them as XRPC methods bound to the existing
service-auth verifier rather than the group-scoped one:
- Add registerServiceAuthMethod(server, nsid, ctx, { handler }) — the
service-auth analogue of registerAuthedMethod, binding
xrpcServiceAuth() (aud = service DID, lxm derived from the request
path) instead of xrpcAuth().
- Convert register and import handlers to (server, ctx) XRPC methods.
Their request bodies are now validated against register.json /
import.json by the XRPC server; the handlers read input.body and
auth.credentials.callerDid, and keep the callerDid === ownerDid check.
- Delete registerRawRoutes, the two express.json() mounts, and the now
dead verifyRegistration / verifyImport / verifyFixedNsidServiceAuth
(xrpcServiceAuth / verifyServiceAuth supersede them).
No client-facing change: same NSIDs, same URLs, same service-auth JWT
contract (aud = service DID, lxm = the method). Purely internal — net
-82 lines. Tests updated to drive both endpoints through a real XRPC
server (createTestApp); two verifier tests repointed from
verifyRegistration to verifyServiceAuth. Full suite green (226 tests),
typecheck and eslint clean.
This removes the last raw routes; every /xrpc endpoint now goes through
the XRPC server. Relates to the `aud` cleanup tracked in
#27.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughThis PR adds a new ChangesGroup Import Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
🚅 Deployed to the certified-group-service-pr-30 environment in CGS
|
There was a problem hiding this comment.
Pull request overview
Adds a new service-auth XRPC procedure app.certified.group.import to promote an existing atproto/PDS account into a managed “group”, and completes the refactor of the last remaining raw Express routes into lexicon-validated XRPC methods.
Changes:
- Introduces
app.certified.group.import(lexicon + handler) that resolves#atproto_pds+ handle from the DID doc, validates ownership via service-auth, verifies the app password via PDS login, then persists/seeds/audits. - Refactors
group.registerto be an XRPC method (service-auth) and extracts the shared “persist + migrate + seed + audit” tail intofinalizeGroup(). - Updates docs, changeset, and test infrastructure to support service-auth methods and DID resolution in context.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/verifier.test.ts | Renames/updates test cases to cover verifyServiceAuth behavior. |
| tests/register.test.ts | Updates register tests to use XRPC test app + new mockAuth helper shape. |
| tests/import.test.ts | Adds coverage for import success, cross-PDS behavior, auth failures, and invalid app password. |
| tests/helpers/mock-server.ts | Adds idResolver to test context; refactors test app creation; updates mockAuth for service-auth. |
| src/index.ts | Removes raw route mounting; routes all /xrpc/* via XRPC server; wires idResolver into context. |
| src/context.ts | Exposes IdResolver on AppContext for handlers to resolve atproto data. |
| src/auth/verifier.ts | Removes verifyRegistration; relies on generalized verifyServiceAuth for service-auth methods. |
| src/api/util.ts | Adds registerServiceAuthMethod() helper to register service-auth XRPC procedures. |
| src/api/index.ts | Registers group.register + new group.import as XRPC methods; removes raw route registrar. |
| src/api/group/register.ts | Converts register to XRPC handler; reuses finalizeGroup() for shared persistence/seeding/auditing. |
| src/api/group/import.ts | New import XRPC handler: resolves PDS/handle, app-password login verification, then finalizeGroup(). |
| src/api/group/finalize.ts | New shared helper for encrypt/store + DB init + owner seeding + audit logging. |
| lexicons/app/certified/group/import.json | New lexicon definition for app.certified.group.import. |
| docs/integration-guide.md | Documents import flow and adds method to endpoint table. |
| docs/design/group-import.md | Design doc describing import semantics, security rationale, and non-goals. |
| docs/api-reference.md | Adds group.import to audit action strings. |
| .changeset/group-import-existing-account.md | Release note describing the new procedure and operational notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+63
to
+69
| const pdsUrl = atprotoData.pds | ||
| const handle = atprotoData.handle | ||
|
|
||
| // Authenticate to the account's PDS with the supplied app password. This | ||
| // is a PDS-local createSession against the host PDS itself (no entryway), | ||
| // and both proves the credential works and confirms the account is there. | ||
| const agent = new AtpAgent({ service: pdsUrl }) |
|
|
||
| **Affects:** Client app developers, Operators | ||
|
|
||
| **Client app developers:** a new procedure `app.certified.group.import` is the sibling of `app.certified.group.register`. Where `register` creates a new account on the group PDS, `import` reuses an account that already exists. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/integration-guide.md (1)
337-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
putRecordrole table for non-authored updates.Line 341 incorrectly states
membercan update another member’s record; this should beadminto match runtime authorization.Proposed doc fix
| Scenario | Required role | | ---------------------------------------------------------------- | ------------- | | Creating new record (no existing author) | member | | Updating a record you authored | member | -| Updating another member's record | member | +| Updating another member's record | admin | | Editing the group profile (`app.bsky.actor.profile` rkey `self`) | admin |As per coding guidelines: "Only admins can
putAnyRecordordeleteAnyRecord; members can only edit/delete records they authored."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/integration-guide.md` around lines 337 - 343, The docs table for putRecord is wrong: the row "Updating another member's record" should list role admin, not member; update the integration-guide.md table entry so that the Scenario "Updating another member's record" has Required role set to admin, and optionally add a short note referencing the runtime auth rules (putRecord vs putAnyRecord/deleteAnyRecord) to make it clear that only admins can perform non-authored putAnyRecord/deleteAnyRecord operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/integration-guide.md`:
- Around line 337-343: The docs table for putRecord is wrong: the row "Updating
another member's record" should list role admin, not member; update the
integration-guide.md table entry so that the Scenario "Updating another member's
record" has Required role set to admin, and optionally add a short note
referencing the runtime auth rules (putRecord vs putAnyRecord/deleteAnyRecord)
to make it clear that only admins can perform non-authored
putAnyRecord/deleteAnyRecord operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c5a833b-9b9a-4141-966d-0e2d7c1a745f
📒 Files selected for processing (17)
.changeset/group-import-existing-account.mddocs/api-reference.mddocs/design/group-import.mddocs/integration-guide.mdlexicons/app/certified/group/import.jsonsrc/api/group/finalize.tssrc/api/group/import.tssrc/api/group/register.tssrc/api/index.tssrc/api/util.tssrc/auth/verifier.tssrc/context.tssrc/index.tstests/helpers/mock-server.tstests/import.test.tstests/register.test.tstests/verifier.test.ts
💤 Files with no reviewable changes (1)
- src/auth/verifier.ts
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
app.certified.group.import(HYPER-469) — the sibling ofapp.certified.group.registerthat promotes an existing PDS account into a group instead of creating a new one. The caller supplies an app password so the service can act on the account's behalf; the account's PDS and handle are resolved from its DID document.Also refactors
registerandimportfrom raw Express routes into proper XRPC methods, removing the last raw routes from the service.Design doc:
docs/design/group-import.md.What's in here
Feature —
group.import(5e5dc1b's parent):lexicons/app/certified/group/import.json—{ groupDid, appPassword, ownerDid }→{ groupDid, handle }.src/api/group/import.ts: verify owner → resolve#atproto_pds+ handle from the DID doc → log in with the app password → persist + seed owner + audit.src/api/group/finalize.ts(encrypt-store-migrate-seed-audit) — no copy-paste.IdResolverexposed onAppContextfor DID resolution.Refactor — raw routes → XRPC methods (
5e5dc1b):registerServiceAuthMethodhelper (service-auth analogue ofregisterAuthedMethod), binding the existingxrpcServiceAuth()verifier.registerandimportare now XRPC methods (same NSIDs), so their request bodies are validated against the lexicons.registerRawRoutes, the manualexpress.json()mounts, and the now-deadverifyRegistration/verifyImport/verifyFixedNsidServiceAuth. Net simplification.aud= service DID,lxm= the method).Key design decisions (see
docs/design/group-import.md)importneither adds nor requires the#certified_groupservice entry. Service proxying is not currently relied upon (clients call CGS directly), and an app password cannot perform PLC operations anyway (that needs theACCESS_FULLscope; app-password logins only ever getAppPass/AppPassPrivileged).registercan add the entry only because it holds a genesis rotation key, whichimporthas no equivalent for.encrypted_recovery_keyis leftNULL(column already nullable). The owner's own pre-existing credentials are their credible exit; the CGS is not a key custodian for imported groups.GROUP_PDS_URL; its#atproto_pdsis resolved from the DID doc and stored as the per-grouppds_url.Security: why both an ownership check and the app-password test
importperforms two independent checks, in this order:verifyServiceAuthproves the caller controlsownerDid(a service-auth JWT signed by that DID's key), and the handler requirescallerDid === ownerDid.InvalidAppPassword.Both are necessary; neither subsumes the other. Holding a working app password is evidence of involvement, but app passwords are deliberately lower-trust, more widely distributed, and revocable. Crucially, import creates a durable new control path (the CGS stores the app password and acts as the account indefinitely, and seeds an owner with RBAC powers) — so without the ownership check, a leaked app password would be a privilege-escalation / account-enrollment primitive: an attacker could bind someone else's account to the CGS and install themselves as owner. The ownership test requires the account's signing key (which an app password does not grant), closing that gap. Owner-auth runs first so we never attempt a PDS login on behalf of an unauthenticated/unauthorized caller.
Testing
tests/import.test.ts: success, cross-PDSpds_url,InvalidAppPassword,GroupAlreadyRegistered, owner seeding, input validation, auth-issuer mismatch, unauthenticated.Open questions (not blocking, tracked for follow-up)
import— end user via a client app (current service-auth model fits, same asregister) vs. a platform backend (which may not hold the owner's PDS session → the API-key use case, Generalised API-key framework for all CGS XRPCs #26). Undecided; the owner-auth requirement is correct under either.importmay want to accept an API key in addition to a service-auth JWT.audin CGS (deprecate group-DID-in-aud overload, backwards-compatible migration) #27 (audoverload), CGS service DID (did:web) does not resolve — /.well-known/did.json returns 404 #29 (CGSdid:webnot resolvable). Neither blocks this PR.Closes HYPER-469 once the live test passes.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation