ClientManager for token-based clients?

[froodian]

ClientManager is a great class, it would be nice to have a version of this class (TokenClientManager?) which works for JWT/Auth token based clients. It looks like this could be done without too much effort (make the Get method take a *token.Token, change cacheKey to be based on *token.Token instead of tls.Certificate, make Factory take a *token.Token instead of a tls.Certificate).

[sideshow]

cc/- @dwieeb as he created this awesome class.
This would be great. I had overlooked this when merging the token stuff.
Will happily merge a PR if you have any thoughts @froodian / @dwieeb

[imhoffd]

The reason I added a PR for ClientManager is that, when given a certificate, Client sets up a TLS connection using that certificate. Each Client has an independent connection by necessity, and managing all those connections was difficult as a user of this library. My impression is that Apple allowed JWT authentication to make things much easier. No reason to manually specify certificates in Client--it will use system certificates to secure the TLS connection.

So it seems to me one could use a single Client, maintaining a single connection, and setting the token to use for each push notification.

I believe this would be possible today in user code with something like this:

mu.Lock()
client.Token = token
client.PushWithContext(...)
mu.Unlock()

[avinassh]

@dwieeb so, we don't need client manager at all for token clients?

[bdandy]

@dwieeb you're wrong, we've got an issue with JWT tokens - Apple do not allow to reuse same connection for different tokens. I've made PR to add token support for ClientManager

[lakim]

Apple documentation clearly states:

APNs does not support authentication tokens from multiple developer accounts over a single connection.

In our case, we are building a system that sends Push Notifications to different apps from different developer accounts, using JWT tokens.
We'll try this PR: #160
@bdandy have you been using your PR in production?

[bdandy]

@lakim we've used those changes in production since PR was created, but I don't have any reports about issues

[lakim]

@bdandy nice, thanks for the feedback. We'll test it out soon.
@sideshow we might eventually take over #160, add tests and submit it back. In a few weeks probably.

[lakim]

@bdandy @sideshow
FYI, I've created an up to date PR with tests:
#213