Skip to content

feat: nostr auth endpoint#89

Open
escapedcat wants to merge 8 commits into
masterfrom
feature/nostr-auth-endpoint
Open

feat: nostr auth endpoint#89
escapedcat wants to merge 8 commits into
masterfrom
feature/nostr-auth-endpoint

Conversation

@escapedcat
Copy link
Copy Markdown
Collaborator

@escapedcat escapedcat commented May 10, 2026

Related FE PR: teambtcmap/btcmap.org#990

FE looks like this

image

Using private key directly (no extension)

image

Summary by CodeRabbit

  • New Features

    • Added Nostr authentication support for user sign-in
    • Automatic user account creation upon first Nostr login
    • Access tokens generated after successful Nostr authentication
  • Bug Fixes

    • Improved authentication flow to properly handle users provisioned without passwords

Review Change Stack

Migration 101 + matching schema.sql update.

NIP-98 sign-in (`POST /v4/auth/nostr`, landing in a follow-up commit)
auto-creates a user when an unknown npub authenticates. Two concurrent
first-time logins for the same pubkey could otherwise produce two user
rows; a unique index makes that race impossible at the DB layer — the
second insert errors and the handler retries select_by_npub.

Partial index (WHERE npub IS NOT NULL) so existing accounts with NULL
npub are unaffected and multiple anonymous users may continue to
coexist. Constraint applies only to non-null values: at most one row
per npub.
Source: BTCMAP_API_BASE_URL env var, default http://127.0.0.1:8000.

The NostrAuth extractor (landed in #84) needs a trusted base URL to
reconstruct the URL the signed event must bind to. We deliberately do
not derive this from the request's Host or X-Forwarded-* headers — an
attacker who tricked a user into signing a bogus-host URL could
otherwise replay the event against the real server by spoofing those
headers. The extractor's spoofed_host_header_is_ignored test pins
this defense.

ApiBaseUrl is per-deployment infrastructure (dev/staging/prod each
have their own URL), so it lives in env, not in Conf — Conf is
DB-backed and reserved for runtime-tunable values shared across
deployments. Production deployments must set BTCMAP_API_BASE_URL to
the public URL (e.g. https://api.btcmap.org). The localhost default
is dev-friendly.

The extractor's no_base_url_configured_yields_none test continues to
pin the failure mode if anyone forgets this wiring.
Adds a single-statement INSERT that sets name + password + npub.
The NIP-98 sign-in endpoint (landing in a follow-up commit) auto-
creates a user when an unknown npub authenticates; doing the insert
in one shot lets migration 101's unique partial index fire on
concurrent first-time logins for the same pubkey, so exactly one
row is created. The handler can then recover by select_by_npub.

Marked #[allow(dead_code)] until the consumer lands on the next
commit.
Exchanges a NIP-98 signed event for a BTC Map Bearer token.

Auth via canonical Authorization: Nostr <base64(event)> (no body).
The NostrAuth extractor (#84) verifies kind 27235, ±60s window,
unique u/method tags, exact URL/method match against the URL pinned
by ApiBaseUrl, and Schnorr signature.

On verified-Some(npub):
- select_by_npub finds existing user → mint Bearer token, return
  {token, username, npub}.
- not found → auto-create: generate name (same Generator as
  POST /v4/users), empty password, role User, npub set in one
  insert via insert_with_npub. Mint token, return as above.

Concurrency: simultaneous first-time logins for the same pubkey
race on the unique partial index from migration 101. The loser's
insert errors; create_or_recover then re-selects by npub and uses
the winning row, so exactly one user is created.

Response shape {token, username, npub} is a strict superset of
what the existing FE proxy expects ({token, username}), so it
won't break btcmap.org PR #911.

Tests cover: missing header, malformed Authorization, known npub,
unknown npub auto-create, and the concurrent-insert race.
PR #84 landed the NIP-98 plumbing in isolation and used dead_code
suppression on the items that had no consumer yet. Now that the
POST /v4/auth/nostr endpoint wires them through, the markers are
no longer needed:

- src/service/nip98.rs: drop #![allow(dead_code)] (verify,
  extract_nostr_auth, VerifiedNip98Event are reached via the
  extractor → endpoint chain).
- src/rest/nostr_auth.rs: drop #![allow(dead_code)] (NostrAuth and
  ApiBaseUrl are consumed by the endpoint and main.rs respectively).
- src/db/main/user/queries.rs: drop #[allow(dead_code)] on
  insert_with_npub (just added) and select_by_npub (existed since
  the npub column landed; consumed by the endpoint now).

No behavior change.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 10, 2026

📝 Walkthrough

Walkthrough

This PR implements NIP-98 Nostr authentication for the API. It adds a database index to enforce unique npub values, query helpers for user insertion, a POST /v4/auth/nostr endpoint that exchanges verified NIP-98 events for bearer tokens with automatic user provisioning, and comprehensive tests covering validation, existing users, auto-creation, and concurrent sign-in recovery.

Changes

Nostr Authentication via NIP-98

Layer / File(s) Summary
Database Schema & Constraints
src/db/main/migrations/101.sql, src/db/main/schema.sql
Adds unique partial index user_npub_unique on user(npub) to enforce single npub per user while allowing NULL values.
Database Query Helpers
src/db/main/user/blocking_queries.rs, src/db/main/user/queries.rs
New async and blocking functions insert_with_npub to insert users with npub values; blocking version uses projection/mapper and serializes roles; removes #[allow(dead_code)] from select_by_npub.
Response Type Definition
src/rest/v4/nostr.rs
Introduces AuthNostrResponse struct with token, username, and npub JSON fields.
Endpoint Handler & Recovery
src/rest/v4/nostr.rs
POST /v4/auth/nostr endpoint validates NIP-98 event via NostrAuth extractor, looks up user by npub, auto-creates user if unknown (with generated numbered username and Role::User), inserts access token, and returns auth response. Includes create_or_recover helper and is_unique_violation_on.
Password Handling Adjustment
src/rest/v4/users.rs
create_token now returns unauthorized early when user.password is empty to avoid password verification for auto-created users.
Dead Code Cleanup
src/rest/nostr_auth.rs, src/service/nip98.rs
Removes module-level #![allow(dead_code)] attributes.
Application Integration
src/main.rs, src/rest/v4/mod.rs
Loads BTCMAP_API_BASE_URL (default http://127.0.0.1:8000), injects ApiBaseUrl into Actix app state, registers /v4/auth route scope with auth_nostr handler, and exports rest::v4::nostr module.
Tests
src/rest/v4/nostr.rs
Adds tests for missing/invalid Authorization (401), existing-user token issuance and persistence, unknown-user auto-creation with Role::User and token binding, and a concurrency test ensuring exactly one user row per npub across parallel sign-ins.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Handler as auth_nostr
  participant Verifier as NostrAuth
  participant DB as UserQueries
  participant TokenStore as AccessTokenStorage
  Client->>Handler: POST /v4/auth/nostr (Authorization: NIP-98 event)
  Handler->>Verifier: validate & extract npub
  Handler->>DB: select_by_npub(npub)
  alt user exists
    Handler->>TokenStore: insert access_token(user_id)
  else user missing
    Handler->>DB: insert_with_npub (create_or_recover)
    alt insert races
      Handler->>DB: select_by_npub(npub) (recover winner)
    end
    Handler->>TokenStore: insert access_token(user_id)
  end
  TokenStore-->>Handler: token
  Handler-->>Client: AuthNostrResponse { token, username, npub }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • teambtcmap/btcmap-api#84: Main PR builds on and wires up the NIP‑98 extractor/service introduced in PR #84 (adds endpoint, DB unique index, and user npub insertion using the previously added nostr auth and nip98 modules).

Suggested reviewers

  • bubelov

Poem

🐰 A rabbit hops through crypto doors,
I wink and sign with nimble paws,
New npubs find a home,
Tokens hum and roam,
Hooray — one user, no more wars!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'feat: nostr auth endpoint' directly and concisely describes the main change: adding a new Nostr authentication endpoint for user login.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/nostr-auth-endpoint

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as resolved.

…users

Address CodeRabbit feedback on PR #89:

- insert_with_npub now writes name/password/npub/roles in a single
  INSERT, closing the window where a row could be observed with empty
  roles by a concurrent first-time login.
- create_or_recover only treats SQLite ConstraintViolation as a lost
  race; pool errors, panics, and other SQLite failures propagate.
- auth_nostr handler returns RestResult<T> to match v4 conventions.
- create_token returns 401 when stored password is empty, so
  Nostr-provisioned users cannot be password-authenticated.

This comment was marked as resolved.

Address Copilot review on PR #89:

- auth_nostr now mints the access token with empty roles, matching the
  password-based create_token. rpc::handler reads token roles only when
  non-empty, otherwise falls back to user.roles. Without this change, an
  admin's role revocation would not propagate to existing Nostr-minted
  tokens.
- create_or_recover splits UNIQUE-violation handling by column. A
  user.npub collision is the lost-race recovery path; a user.name
  collision (the Name::Numbered generator can clash) retries with a fresh
  name up to NAME_RETRIES times; any other constraint failure propagates.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/db/main/user/blocking_queries.rs (1)

24-45: ⚡ Quick win

Consider adding a unit test for insert_with_npub.

The function implementation is correct and follows the established pattern (consistent with insert and set_roles). However, the test module below lacks coverage for this new function. Adding a unit test would verify the atomic insertion of npub and roles and improve maintainability.

📋 Suggested test case
#[test]
fn insert_with_npub() -> Result<()> {
    let conn = conn();
    let name = "test_user";
    let password = "test_pwd";
    let npub = "npub1test123";
    let roles = vec![Role::User, Role::Admin];
    
    let user = super::insert_with_npub(name, password, npub, &roles, &conn)?;
    
    assert_eq!(name, user.name);
    assert_eq!(password, user.password);
    assert_eq!(Some(npub.to_string()), user.npub);
    assert_eq!(roles, user.roles);
    
    // Verify it was actually stored
    let retrieved = super::select_by_id(user.id, &conn)?;
    assert_eq!(user.npub, retrieved.npub);
    assert_eq!(user.roles, retrieved.roles);
    
    Ok(())
}

As per coding guidelines, tests should be inline using #[cfg(test)] modules and use db::test::pool() for in-memory SQLite test databases.

🤖 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 `@src/db/main/user/blocking_queries.rs` around lines 24 - 45, Add a unit test
for insert_with_npub in the module's #[cfg(test)] tests: create an in-memory
connection via db::test::pool() (or conn()), call insert_with_npub(name,
password, npub, &roles, &conn) with roles like vec![Role::User, Role::Admin],
assert returned User has expected name, password, Some(npub.to_string()) for
npub and matching roles, then call select_by_id(user.id, &conn) to verify the
record was persisted with the same npub and roles; place the test in the
existing test module and return Result<()> so it follows existing test patterns.
🤖 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.

Nitpick comments:
In `@src/db/main/user/blocking_queries.rs`:
- Around line 24-45: Add a unit test for insert_with_npub in the module's
#[cfg(test)] tests: create an in-memory connection via db::test::pool() (or
conn()), call insert_with_npub(name, password, npub, &roles, &conn) with roles
like vec![Role::User, Role::Admin], assert returned User has expected name,
password, Some(npub.to_string()) for npub and matching roles, then call
select_by_id(user.id, &conn) to verify the record was persisted with the same
npub and roles; place the test in the existing test module and return Result<()>
so it follows existing test patterns.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f971fb2a-17c6-4278-a0bd-ee75330bc1ed

📥 Commits

Reviewing files that changed from the base of the PR and between afa4cb4 and e7046bd.

📒 Files selected for processing (4)
  • src/db/main/user/blocking_queries.rs
  • src/db/main/user/queries.rs
  • src/rest/v4/nostr.rs
  • src/rest/v4/users.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/rest/v4/nostr.rs

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.

2 participants