Allow websocket connections with external auth #6912
Conversation
| @@ -2,6 +2,9 @@ module ApplicationCable | |||
| class Connection < ActionCable::Connection::Base | |||
| identified_by :current_user | |||
| def connect | |||
| if request.session[:auth_type] == 'remote' | |||
There was a problem hiding this comment.
Very nice. In comparing this implementation with MainController#login, I note that the latter uses a remote_auth? helper, which checks another session key. I suspect it would be prudent to do the same here?
There was a problem hiding this comment.
Hi @david-yz-liu - thanks for the review! Sessions are not available in cable (hence the use of request.session), so we can't directly re-use remote_auth?. We do check the same session key here as in that method. Would you be alright with keeping it this way for simplicity, or is there another approach we should be thinking of?
There was a problem hiding this comment.
@pretendWhale oh sorry you're totally right, I missed that even though I was looking at exactly this line. All good 👍
| @@ -2,6 +2,9 @@ module ApplicationCable | |||
| class Connection < ActionCable::Connection::Base | |||
| identified_by :current_user | |||
| def connect | |||
| if request.session[:auth_type] == 'remote' | |||
There was a problem hiding this comment.
@pretendWhale oh sorry you're totally right, I missed that even though I was looking at exactly this line. All good 👍
Motivation and Context
Currently, websocket connections fail when users are logged in via external auth (i.e. UTORAuth). This is because
current_useris not added to user sessions when logged in via external auth.Your Changes
Description:
Added a check for remote authorization when attempting to create a websocket connection.
Type of change (select all that apply):
Testing
Tested that when logged in via external auth websocket connections work in the web interface. Added a test for the case that a user is logged in via external auth.
Questions and Comments (if applicable)
I think the order of the checks is right - as it stands, we check for the remote auth first, then override with current user if it exists (which should catch role-switched users), then falling back to
real_user_nameifcurrent_useris not set by either previous check. But I'd appreciate a second opinion.Checklist
Pull request to make documentation changes (if applicable)