onflow / flow-go

A fast, secure, and developer-friendly blockchain built to support the next generation of games, apps, and the digital assets that power them.
GNU Affero General Public License v3.0
534 stars 179 forks source link

[Access] Add client id for subscriptions #6637

Open Guitarheroua opened 1 week ago

Guitarheroua commented 1 week ago

Should be discussed with FCL team due to comment: https://github.com/onflow/flow-go/pull/6630#discussion_r1835495225

jribbink commented 1 week ago

Thanks for making this issue @Guitarheroua.

@illia-malachyn 's comment from the other thread:

The ID in SubscribeMessageResponse is generated on the server side to ensure that each subscription has a unique identifier. This approach prevents clients from spoofing or forging IDs, enhancing security by reducing the potential for unauthorized access or manipulation. By associating the Topic with this unique ID in the response, clients can reliably match a SubscribeMessageRequest to its corresponding SubscribeMessageResponse.

Topic is indeed redundant and might not be included in a response. We're still discussing it

My concern is that associating subscribe requests to responses on the basis of Topic alone can be ambiguous if a client sends multiple consequetive SubscribeMessageRequest messages to the server with the same Topic.

Presumably this is a potential scenario when a user wants to subscribe to the same topic multiple times, but with different parameters.

It seems to me that either:

  1. Some sort of identifier must be sent from the client to associate SubscribeMessageRequest and SubscribeMessageResponse messages.

    Sorry for initially stating that this should correspond to the same ID as SubscribeMessageResponse was maybe a mistake on my part - this can be a totally different identifier.

  2. OR The server must guarantee in-order delivery of SubscribeMessageResponse messages. In this case, I can just add pending subscriptions to a queue on the client-side & associate them with incoming responses in-order. Maybe #2 is already a guarantee, and in this case this would not be an issue to worry about. But I just wanted to bring it up in the case that it isn't.