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

CLOSED messages for relays that want to reject REQs and NIP-42 AUTH integration #902

Merged
merged 9 commits into from
Dec 6, 2023

Conversation

fiatjaf
Copy link
Member

@fiatjaf fiatjaf commented Nov 27, 2023

I will quote my own message from #841 (comment) to explain this:

Basically we have two distinct problems:

  1. knowing why a relay is asking for AUTH
  2. knowing why a subscription has failed

adding a third element to AUTH only solves them in the occasion that the two happen together.

creating a new message solves 2 and making AUTH be subservient to that (and also to OK) solves 1.

More relevant messages were posted to that other thread but should probably have been posted to this.

@fiatjaf fiatjaf mentioned this pull request Nov 27, 2023
01.md Outdated Show resolved Hide resolved
42.md Outdated Show resolved Hide resolved
42.md Outdated Show resolved Hide resolved
@monlovesmango
Copy link
Member

monlovesmango commented Nov 27, 2023

some minor changes, but I approve.

personally think we should just use "CLOSE" rather than "CLOSED" to mirror the client side "CLOSE" (in the same way that "EVENT" is the same on client and relay sides and mean the same thing). seems way more consistent to me.

ps. really like the "CLOSE" verb over the ones that I suggested

fiatjaf and others added 3 commits November 27, 2023 16:16
Co-authored-by: monlovesmango <96307647+monlovesmango@users.noreply.github.com>
Co-authored-by: monlovesmango <96307647+monlovesmango@users.noreply.github.com>
Co-authored-by: monlovesmango <96307647+monlovesmango@users.noreply.github.com>
@fiatjaf
Copy link
Member Author

fiatjaf commented Nov 27, 2023

My rationale for CLOSED was that when a client is sending a CLOSE it is requesting the relay to close the subscription, but when a relay sends it it is just notifying the client that it has already been done.

Also I prefer to have different words, makes the parsing code simpler.

01.md Outdated Show resolved Hide resolved
@arthurfranca
Copy link
Contributor

ACK!! The flow is very clear now with examples and all.

Personally I would just tell relays to always send AUTH right after connection is established. It leaves less room for errors (like sending CLOSED/OK before AUTH).

42.md Outdated Show resolved Hide resolved
This NIP defines two new prefixes that can be used in `OK` (in response to event writes by clients) and `CLOSED` (in response to rejected
subscriptions by clients):

- `"auth-required: "` - for when a client has not performed `AUTH` and the relay requires that to fulfill the query or write the event.
Copy link

@proudmuslim-dev proudmuslim-dev Nov 27, 2023

Choose a reason for hiding this comment

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

Suggested change
- `"auth-required: "` - for when a client has not performed `AUTH` and the relay requires that to fulfill the query or write the event.
- `"auth-required: "` - for when a client has not performed `AUTH` and the relay requires it to fulfill the query or write the event.

@staab
Copy link
Member

staab commented Nov 27, 2023

From #841 (comment)

I think it won't hurt at all to have a message akin to OK but specifically for REQs.

This makes sense. We could probably use the same verb for COUNT and other future read-verbs like HASH too? Not necessary to specify, just a thought.

@jb55 jb55 self-requested a review November 27, 2023 20:10
Copy link
Contributor

@jb55 jb55 left a comment

Choose a reason for hiding this comment

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

LGTM

@monlovesmango
Copy link
Member

should this also apply to nip 45?

@mikedilger
Copy link
Contributor

+1

At any moment the relay may send an `AUTH` message to the client containing a challenge. After receiving that the client may decide to
authenticate itself or not. The challenge is expected to be valid for the duration of the connection or until a next challenge is sent by
the relay.
At any moment the relay may send an `AUTH` message to the client containing a challenge. The challenge is valid for the duration of the connection or until another challenge is sent by the relay. The client MAY decide to send its `AUTH` event at any point and the authenticated session is valid afterwards for the duration of the connection.

Choose a reason for hiding this comment

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

Suggested change
At any moment the relay may send an `AUTH` message to the client containing a challenge. The challenge is valid for the duration of the connection or until another challenge is sent by the relay. The client MAY decide to send its `AUTH` event at any point and the authenticated session is valid afterwards for the duration of the connection.
At any moment the relay may send an `AUTH` message to the client containing a challenge. The challenge is valid for the duration of the connection or until another challenge is sent by the relay. The client MAY decide to send its `AUTH` event at any point and the authenticated session is then valid for the remainder of the connection.

@fiatjaf
Copy link
Member Author

fiatjaf commented Nov 29, 2023

@fiatjaf
Copy link
Member Author

fiatjaf commented Nov 29, 2023

Implemented in https://nostr.wine

@nostr-wine
Copy link
Contributor

Ha - you beat me to it. We implemented CLOSED with the new flow for DM REQs on all nostr.wine regional relays and will work on the rest of our relays today.

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.

8 participants