Skip to content

feat: add approver-side Pubky Auth FFI#83

Merged
ben-kaufman merged 3 commits intomasterfrom
feat/pubky-approve-auth
Mar 31, 2026
Merged

feat: add approver-side Pubky Auth FFI#83
ben-kaufman merged 3 commits intomasterfrom
feat/pubky-approve-auth

Conversation

@ben-kaufman
Copy link
Copy Markdown
Collaborator

@ben-kaufman ben-kaufman commented Mar 30, 2026

Summary

  • Add approve_pubky_auth(auth_url, secret_key_hex) — signs an AuthToken and POSTs it to the relay, enabling approval of auth requests from other apps
  • Add parse_pubky_auth_url(auth_url) — parses a pubkyauth:// URL into a PubkyAuthDetails record for UI display (kind, capabilities, relay, homeserver, signup_token)
  • Add 9 tests covering signin/signup/old-format URL parsing, missing params, seed_export rejection, and invalid key/URL error handling
  • Bump version and regenerate Swift/Kotlin/Python bindings

Test plan

  • cargo test -- pubky — all 42 tests pass
  • cargo build — compiles cleanly
  • Verify approvePubkyAuth and parsePubkyAuthUrl appear in generated Swift/Kotlin bindings
  • Integration test with a live relay (requires running homeserver)

🤖 Generated with Claude Code

@ovitrif
Copy link
Copy Markdown
Collaborator

ovitrif commented Mar 30, 2026

@ben-kaufman pls "fix" merge conflicts, can just ask claude that PR #81 got merged meanwhile, and it should figure out what's causing the merge conflicts (hint: the cargo fmt applied to all rust files), can't be that hard to reformat the code before or after these changes).

I usually ask it to make a backup branch (with "-backup" suffix), then reapply our PR's changes on top of default branch, then double-check nothing got lost in the process, commit, bumpm version, regenerate bindngs, check if we should update CHANGELOG.md, push, delete and recreate release on latest commit (if created), and update PR description as needed if applicable (for example, when I have release linked or something).

so far I keep repeating all this every time xD, should make an automation one day though.

Copy link
Copy Markdown
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just needs new bindings after conflicts are resolved

@ovitrif
Copy link
Copy Markdown
Collaborator

ovitrif commented Mar 30, 2026

Approved — code is clean and consistent with existing patterns. A few nits/observations for consideration (none blocking):

Nits

  1. Error flattening in approve_pubky_auth (auth.rs:123) — Five distinct upstream error types (network timeout, relay HTTP error, URL parse, validation, expired request) all map to PubkyError::AuthFailed. The mobile app can't programmatically distinguish "check your internet" from "scan the QR again, it expired." Consider at least prefixing the reason string with the failure category in a follow-up.

  2. PubkyAuthDetails implicit invariants (auth.rs:71-83) — The relationship between kind and optional fields (homeserver, signup_token) is documented in comments but not enforced by the type. A sum-type enum with associated data would make illegal states unrepresentable, but that's constrained by FFI ergonomics. Fine as-is since construction only happens in one place.

  3. Add PartialEq to PubkyAuthDetails — Zero-cost quick win, simplifies test assertions:

    #[derive(uniffi::Record, Debug, Clone, PartialEq)]
  4. SeedExport error message (auth.rs:106) — Could be more user-actionable, e.g., "This is a seed export link, not an auth URL."

  5. Missing test for unreachable relay — The most common production failure (valid key + valid URL + relay down) isn't covered. A test with relay=https://127.0.0.1:1/link would exercise that path.

  6. capabilities as raw String — Also flagged by @coreyphillips in this review comment: the upstream Capabilities type has structure (path + permission pairs) that gets discarded when stringified. Consider exposing it as Vec<Capability { path, permission }> for downstream apps.

What's good

  • Both FFI wrappers follow the exact ensure_runtime() / rt.spawn() / unwrap_or_else pattern
  • Exhaustive DeepLink match — no wildcards
  • 9 tests covering all match arms, optional field presence/absence, legacy format, and error variant discrimination
  • Secret keys not logged or included in error messages
  • PubkyAuthKind enum design is solid

ben-kaufman and others added 3 commits March 31, 2026 09:39
Add approve_pubky_auth and parse_pubky_auth_url to enable approving
auth requests from other apps. Bump version and regenerate bindings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the raw string kind field with a proper PubkyAuthKind enum
(Signin/Signup) for type safety in generated bindings. Bump version
and regenerate bindings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ben-kaufman ben-kaufman force-pushed the feat/pubky-approve-auth branch from b723c97 to 47cac66 Compare March 31, 2026 08:24
@ben-kaufman ben-kaufman requested a review from ovitrif March 31, 2026 08:26
@claude
Copy link
Copy Markdown

claude bot commented Mar 31, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@ovitrif
Copy link
Copy Markdown
Collaborator

ovitrif commented Mar 31, 2026

Looks like the update is just a rebase + rustfmt — the nits from our earlier comments are still open. They're all non-blocking, but the PartialEq derive (#3) and the structured capabilities (#6, per @coreyphillips's comment) would be quick wins worth addressing before merge. The rest can be tracked as follow-up.

Copy link
Copy Markdown
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK — code is clean and follows existing conventions. Nits noted in comments.

@ben-kaufman ben-kaufman merged commit 6dc2e68 into master Mar 31, 2026
3 checks passed
@ovitrif ovitrif deleted the feat/pubky-approve-auth branch March 31, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants