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

Wait for auth multiplex #31

Closed

Conversation

mattkrick
Copy link
Contributor

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.

Whenever a client calls the authenticate method, or receives a
successful handshake, or gets a #setAuthToken, the auth state is set to
true.
When a client is disconnected or an auth token is removed, or an auth
token expires, the isAuthorized state turns false.

Also, performs check for malformed tokens before sending them to the
server.
Use case: create a connection, dirty it up with callbacks, extra props,
etc, disconnect. Connect again, you still have dirty callbacks. Better
to destroy.
@mattkrick
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

@jondubois
Copy link
Member

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.

@jondubois jondubois closed this Nov 29, 2015
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.

Add waitForAuth to subscribe method Reuse connection instance
2 participants