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

Stop trying to reconnect on unauthorized cable connections #34194

Merged

Conversation

@staugaard
Copy link
Contributor

@staugaard staugaard commented Oct 11, 2018

Summary

When calling reject_unauthorized_connection from an ActionCable::Connection, the client would keep retrying to establish a connection. These would keep failing as the client is still unauthorized.

This stops that. If you call reject_unauthorized_connection, the connection is now closed and is not retried.

@rails-bot
Copy link

@rails-bot rails-bot commented Oct 11, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@staugaard staugaard force-pushed the staugaard/actioncable_unauthorized branch from 869150e to 5b919f8 Oct 11, 2018
@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Oct 17, 2018

Not sure what to do to get some eyes on this? @schneems?

@taleh007
Copy link

@taleh007 taleh007 commented Oct 18, 2018

Not sure what to do to get some eyes on this? @schneems?

You can do nothing. I think he is very busy man

@jeremy
Copy link
Member

@jeremy jeremy commented Oct 18, 2018

/cc @rmacklin @palkan 😅

@palkan
Copy link
Contributor

@palkan palkan commented Oct 18, 2018

💯 for this feature! @staugaard thanks!

Few other thoughts:

  • What about making a message type more generic name? I would suggest smth more low-level with additional customizable fields, like { "type": "disconnect_notice", "reconnect": false, "reason": "Unauthorized" }
    • or we can allow to customize the message through reject_unauthorized_connection("You are not authorized")
  • It would be great to make it possible to notify client about not-reconnecting when using ActionCable.server.remote_connections.where(current_user: User.find(1)).disconnect
    • e.g. disconnect(reconnect: false, reason: "human-friendly error to show to the user")
  • I think, we should provide an API to catch this disconnect event on JS consumer, e.g. ActionCable.onDisconnect(msg)

@@ -89,6 +89,9 @@ class ActionCable.Connection
when message_types.welcome
@monitor.recordConnect()
@subscriptions.reload()
when message_types.unauthorized
ActionCable.log("Unauthorized. Stopping monitor and disconnecting.")
@close(allowReconnect: false)
Copy link
Contributor

@rmacklin rmacklin Oct 19, 2018

I think the reason I was mentioned may be because this would cause a significant conflict with my open pull request: #34177

I just addressed the last comment on the PR so I'm hoping that we'll be able to merge it soon. So, do we think it's reasonable to wait to merge this PR until that ES2015 conversion is merged?

Copy link
Contributor Author

@staugaard staugaard Oct 19, 2018

I'll happily rebase on master once #34177 is merged.

Copy link
Contributor

@rmacklin rmacklin Oct 19, 2018

I'm happy to help if you'd like

@staugaard staugaard force-pushed the staugaard/actioncable_unauthorized branch 2 times, most recently from fcc416f to c8bc6ff Nov 6, 2018
@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Nov 6, 2018

OK @palkan, @rmacklin, and @schneems. I've rebased on master since #34177 was merged. Good to go?

@jeremy
Copy link
Member

@jeremy jeremy commented Nov 7, 2018

@palkan Regarding your three thoughts: which should be addressed in this PR, and which are topics for further PRs?

@palkan
Copy link
Contributor

@palkan palkan commented Nov 7, 2018

@jeremy

I think, it makes sense to define on the message type (and meta information along with this message). I mean:

What about making a message type more generic name? I would suggest smth more low-level with additional customizable fields, like { "type": "disconnect_notice", "reconnect": false, "reason": "Unauthorized" }

WDYT?

Others two could be added later separately and shouldn't block this PR, IMO.

@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Nov 7, 2018

Something like this @palkan ?

@palkan
Copy link
Contributor

@palkan palkan commented Nov 7, 2018

Something like this @palkan ?

Yep. Looks good 👍

@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Nov 8, 2018

OK, I think we are good to go on this one then. Should I squash?

palkan added a commit to anycable/anycable-go that referenced this issue Nov 8, 2018
Action Cable may send a message before disconnecting an unauthorized client.
See rails/rails#34194
@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Nov 16, 2018

@taleh007
Copy link

@taleh007 taleh007 commented Nov 21, 2018

@schneems ?

no way to catch him online

@mlwilkerson
Copy link

@mlwilkerson mlwilkerson commented Nov 25, 2018

@staugaard How might the JavaScript client be able to respond to this event?

I've been trying to figure out an elegant-ish way to add a short-lived access token to the cable URL as a GET param, updating the cable url when a refreshed access token comes back in a custom header. (I'd much prefer to add a custom HTTP header, as with a normal JWT auth token, but that doesn't seem to be an option here in the WebSocket world). Most of the time this works, except apparently in some cases where the ConnectionMonitor/reopen logic happens out of band with the access token refresh logic. In that case, it just tries forever to reconnect with an old access token. So this feature where the server sends back a distinct message about the unauthorized connection to stop reconnect attempts is helpful. But then I'd need the client to be able to respond to that event by getting a refreshed access token, rebuilding the cable url with it, and then trying again to connect.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Could you squash and add a CHANGELOG entry?

@staugaard staugaard force-pushed the staugaard/actioncable_unauthorized branch from 1cd178b to a7040f7 Nov 27, 2018
@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Nov 27, 2018

@rafaelfranca @schneems, I squashed and added CHANGELOG entry.

@staugaard
Copy link
Contributor Author

@staugaard staugaard commented Dec 5, 2018

This is taking you guys a while. Now there is a conflict. I'll rebase.

@staugaard staugaard force-pushed the staugaard/actioncable_unauthorized branch from a7040f7 to 58dbc1c Dec 5, 2018
@rafaelfranca rafaelfranca merged commit d981654 into rails:master Dec 5, 2018
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants