Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configurable nostr signer #1757

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

riccardobl
Copy link
Member

@riccardobl riccardobl commented Dec 22, 2024

Description

Users can choose their primary Nostr signer, either NIP-07 (browser extension) or an attached NIP-46 signer.
The NIP-46 bunker URL is stored locally or in the encrypted vault if device sync is enabled.
The selected signer is then used for crossposting.

Screenshots

image
image

Additional Context

The main issue I encountered was the reuse of NIP-46 bunker URLs. While this is discouraged for authentication, it is necessary for crossposting without requiring the user to input a new NIP-46 token every session, given the separation of the Nostr identity from the authentication method.

Fortunately, there is a spec-compliant approach to achieve this. However, I couldn’t find a way to implement it directly in NDK. To resolve this, I overrode the blockUntilReady() method.

Checklist

Are your changes backwards compatible? Please answer below:
yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
yes

Did you introduce any new environment variables? If so, call them out explicitly here:
no


const [suggestion, setSuggestion] = useState(null)
const suggestionTimeout = useRef(null)

Copy link
Member Author

Choose a reason for hiding this comment

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

In this file, I've separated the logic related to the NIP-46 authentication challenge into dedicated hooks and components, making it reusable for the crosspost's NIP-46 authentication.


const { challengeResolver: nostrAuthChallengeResolver, setStatus: nostrAuthSetStatus } = useNostrAuthStateModal({
challengeTitle: 'Crossposting to Nostr'
})
Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, the NIP-46 challenge should be resolved, and the connection should already be established, as we perform an initial test connection on the settings page. However, the specification does not clearly state whether the challenge can be requested only during the first connection or at any time afterward. To be extra cautious, I've added the challenge hook here as well.

const { settings: { privates: settings } } = useMemo(() => data ?? ssrData, [data, ssrData])
const [settings, setSettingsState] = useState(() => (data ?? ssrData)?.settings?.privates)

const { challengeResolver: nostrAuthChallengeResolver, setStatus: nostrAuthSetStatus } = useNostrAuthStateModal({
Copy link
Member Author

Choose a reason for hiding this comment

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

is it ok to use a modal here?

if (testSignerInstance instanceof NDKNip46Signer) {
testSignerInstance.once('authUrl', nostrAuthChallengeResolver)
}
await testSignerInstance.blockUntilReady()
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, we initiate and test the connection to the signer, providing two benefits:

  1. The user immediately knows if the NIP-46 signer is functioning without needing to attempt a crosspost.
  2. We complete the challenge and authentication at this stage, preventing interruptions during navigation later.

* @returns {NDKPrivateKeySigner | NDKNip46Signer | NDKNip07Signer | null} - a signer instance
*/
getSigner ({ privKey, nip46token, supportNip07 = true } = {}) {
getSigner ({ userPreferences: { signerType: userSignerTypePreference, signer: userSignerPreference, signerInstanceKey } = {}, privKey, nip46token, nip07, nip46LocalSigner } = {}) {
Copy link
Member Author

Choose a reason for hiding this comment

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

signerInstanceKey is the client key pair, which is one of the key parts needed to reestablish the connection for NIP-46 crossposts across user sessions.

confirm(resolve)
.then(() => clearTimeout(connector))
.catch((err) => console.error('Error restoring connection:', err))
})
Copy link
Member Author

Choose a reason for hiding this comment

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

The spec is not very clear about how to re-establish connections between user sessions. It only mentions that the client key pair is disposable and suggests storing it locally for the user session. It also states that new connections cannot be established with a reused secret.

The key point here is that nsec.app will respond to connections from an authorized client key pair but will reject new connect RPCs from the same secret and key pair. The issue is that the NDK auth flow doesn’t seem to account for this behavior.

To work around this, I implemented a solution where two requests,one for "connect" and one for "get_public_key", are raced against each other. The first request to complete without an error is used to proceed with authentication.

@riccardobl riccardobl marked this pull request as ready for review December 23, 2024 20:38
@riccardobl riccardobl requested a review from Copilot December 23, 2024 20:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 14 changed files in this pull request and generated 3 comments.

Files not reviewed (9)
  • components/nav/common.js: Evaluated as low risk
  • api/typeDefs/user.js: Evaluated as low risk
  • components/vault/use-vault-configurator.js: Evaluated as low risk
  • fragments/users.js: Evaluated as low risk
  • worker/nostr.js: Evaluated as low risk
  • pages/settings/passphrase/index.js: Evaluated as low risk
  • lib/validate.js: Evaluated as low risk
  • components/use-crossposter.js: Evaluated as low risk
  • api/resolvers/user.js: Evaluated as low risk

components/use-encrypted-privates.js Outdated Show resolved Hide resolved
components/use-encrypted-privates.js Outdated Show resolved Hide resolved
components/use-local-state.js Show resolved Hide resolved
@riccardobl riccardobl added nostr enhancement improvements to existing features labels Dec 24, 2024
@ekzyis ekzyis self-requested a review December 28, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features nostr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant