feature: onfeedauthenticate to authenticate single feeds per stream

[martinheidegger]

Having a stream authentication is super-useful but it may be warranted to authenticate particular feeds to particular peers while others are supposed to work. The proposed API in this PR should work without much overhead, allowing to disable the replication for particular feeds.

[RangerMauve]

This would be amazing to have since we could have really flexible access control restrictions.

[mafintosh]

it's not safe to call onready again as it that will mess with the ifAvailable counter

[mafintosh]

If you call ifAvailable .wait() it might be fine I think

[martinheidegger]

Thank you for the feedback. I will try to address it, also 😨 I fear that won't be enough as the 'handshake' event may never occur (in case of error, or close) as such i probably need to listen/unlisten to other events as well?

[mafintosh]

Yea it's tricky. You need to hook up on(close) as well and call the ifavailable hook but ONLY if the happy path hasn't succeded.

[mafintosh]

prob have to setMaxListeners(0) here to avoid leak warnings if we use the event for flow control.

[martinheidegger]

I was wondering about this: .setMaxListeners on stream is weird in this location, shouldn't it be in the Protocol constructor of hypercore-protocol as the stream may be passed-in from the outside. (here maybe?) Anyways I will add it.

[mafintosh]

I normally avoid events for flow control in general because of this, but unsure if you can do this without it. Since this is the one taking on the responsibility of adding that listener, I'd put it here, so it's clear that we aren't just suppressing a leak from the outside.

[martinheidegger]

@mafintosh in the just pushed 9c93de3 I think I managed to take care of closing eventually opened event handlers using ifavailable before/after all possible delays; guarding against closing of stream/parallel replication at any point. Does that work for you?

[mafintosh]

why is the feed passed actually? is that because this is a corestore feature in practice?

[mafintosh]

If we just pass the discoveryKey it's a lot easier to port to the stream layer later on.

[martinheidegger]

I am passing the feed for an authentication method to have a bit more flexibility in choosing what to authenticate, such as: discoveryKey, publicKey, or size limits etc.

[mafintosh]

How is that different than doing:

feed.replicate({
  onfeedauthenticate (remotePublicKey, done) {
    if (feed.....) 
  }
})

[martinheidegger]

is that because this is a corestore feature in practice?

Yes, in a sense. This hook is for allowing selected feeds over a single stream. If the API doesn't provide information on which stream is replicated, we could bind the functions:

const feedA, feedB

function commonAuthLogic (feed, remotePublicKey, done) {}

feedA.replicate({ onfeedauthenticate: commonAuthLogic.bind(null, feedA) })
feedB.replicate({ onfeedauthenticate: commonAuthLogic.bind(null, feedB) })

but that is problematic if the overarching system (such as corestore) simply passes-on the replication options and doesn't manage the onfeedauthenticate logic.

new Corestore({
  onfeedauthenticate () {
    /* which feed? */
  }
})

Possibly requiring some packages to be aware of that logic.

A different question is what kind of identification to use. Initially I thought of using the hypercore.key as identifier. It is used in corestore.get(). But because of hyperswarm using discoveryKey I thought maybe discoveryKey as an identifier, as its the key for hyperswarm. Both keys in the API seem like overkill, which is why I went with simply the feed. The instance was there and it didn't seem harmful.

[mafintosh]

Ok 👍 It seems a bit weird that we add a "corestore/other-many-core" feature here then, as I think that should be implemented there but we can try it out - ie it might be removed in a future major.

[martinheidegger]

I hope it won't cause trouble down the road.

[mafintosh]

Yep I think it looks good.

There is an edgecase where the hook runs more than once for the same feed, but I don't think that can be avoided when implemented here. In general it's a more complicated than I prefer, but that's due to the actual stream hook not being exposed. In retrospect it would probably be a lot easier to implement on the replication stream itself as that can have a simple hook here https://github.com/mafintosh/simple-hypercore-protocol/blob/master/index.js#L185 i think, but we can start with this.

Asked a usage question above, cause was a bit confused after rereading it, but other than that it should be fine. Thanks for adding tests.

[martinheidegger]

In retrospect it would probably be a lot easier to implement on the replication stream itself as that can have a simple hook here https://github.com/mafintosh/simple-hypercore-protocol/blob/master/index.js#L185 i think, but we can start with this.

I was aware of this possibility, but I don't see the problem in practice: Before the ready hook, random-access-storage operations and other delayed checks may also happen more than once, resulting in unintuitive behavior.

Finding the right layer to put things in has been stressful 😅. The send method seems an interesting choice for it (I wonder what your thought process is). I would may have gone with the onopen method of hypercore-protocol but I am feeling less secure to touch even lower levels of the API 😅

[mafintosh]

@martinheidegger can you remove the commit from the other PR you made, that I just merged? then this should be good to go.

[martinheidegger]

rebased and squashed in b6de219

[mafintosh]

Thanks, 9.10.0

[mafintosh]

(contains the other pr also)

[RangerMauve]

This is super exciting. Thank you both for getting this functionality in. 💜