SocketCluster / socketcluster-client

JavaScript client for SocketCluster
MIT License
292 stars 91 forks source link

Wait for auth multiplex #31

Closed mattkrick closed 8 years ago

mattkrick commented 8 years ago

Sorry for combining issues, but there was a bit of overlap.

First up fixes #27. The external API doesn't change, but behind the scenes it caches connections based on url. Also offers up a multiplexoption if you don't want to cache your connection. Finally, it offers a destroy method. you'd call it just like connect, eg socketCluster.destroy(options). It looks up the id based on your options & deletes it from the cache.

Second fixes #29 First, it puts 2 props on SCSocket: isAuthenticated (boolean) and _tokenExpiration (timeoutHandler). On every successful handshake, #setAuthToken, and authenticate method, it sets isAuthenticated to true & calculates the time to live for that to remain true. Once that time is up, it turns false. Also, on every disconnect or #removeAuthToken it turns false. Additionally, I added a safety check to make sure the token is legit before it's sent off to the server.

With that in place, it adds on options argument to the subscribe method. if waitForAuth is true, then it waits. before subscribing.

mattkrick commented 8 years ago

Continued from #28 Nice catch, yep, those error callbacks should both be HOFs. I agree they aren't ideal, but here's why I did it: The handshake from the server does not return a signedAuthToken. This is nice, it keeps traffic low. However, _setAuthStatus needs a token. We could either pass it by wrapping the callback in a HOF, or we could read the token inside _setAuthStatus. By using the HOF, we're reading from memory guaranteed, whereas if we re-read it, it may come from the disk, or maybe even another server, depending on the dev's AuthEngine. This is a pretty small price to pay for an extra closure (if I had written it right!). Additionally, the dev now has access to the token for his error callback, which could be useful (eg token: "myToken: asldkjfalkds" is invalid).

mattkrick commented 8 years ago

Regarding waitForAuth, dateTimes are always stored as milliseconds from unix epoc UTC, so no need to worry about timezones. What COULD happen is if the client adjusted his computer to be 5 minutes fast, then the token would expire 5 minutes early (converse also applies). Still no security issue since authorization occurs on the server, in server time. This can be fixed by #32

mattkrick commented 8 years ago

Additionally, we could store the token itself in the socket object. The AuthEngine would save the token, but when we need the token, we get it direct from the object. One thing to note is that since the handshake happens on the transport layer, we'd either have to save it there, or have the transport layer depend on the socket layer, so it doesn't really affect this issue.

jondubois commented 8 years ago

I merged most of the commits. I left out the bits related to the isAuthenticated state since this is likely to change in v4.

For the expiry case, I think the server should tell the client when the token has expired (E.g. when the token-expiry middleware will emit a deauthenticate event on the client).

This has been published in v3.1.0 on npm and bower.

jondubois commented 8 years ago

See commits: https://github.com/SocketCluster/socketcluster-client/commits/matt

jondubois commented 8 years ago

I'm closing this since this has been manually merged. Feel free to keep this branch in your fork if you feel that this other (unmerged) code might be useful as part of v4 release.

I prefer if the client is optimistic about its isAuthenticated state and let the server correct it (E.g. using a deauthenticate event) if it is not accurate. With this approach, we might get false positives (whereby an unauthenticated client wrongly believes that it is authenticated), but we would never get false negatives. False positives can be easily corrected (using the deauth event or a token expired error).

If we let the client do expiry on its own, we might end up with false positives OR false negatives depending on whether the client's clock is ahead or behind the server time and this complicates things.