vmware-archive / postfacto

Self-hosted retro tool aimed at helping remote teams
GNU Affero General Public License v3.0
8 stars 22 forks source link

Investigate "forced logout" feature #130

Open seadowg opened 5 years ago

seadowg commented 5 years ago

Postfacto attempts to disconnect any existing Action Cable connections when the password for a Retro is changed (https://github.com/pivotal/postfacto/blob/master/api/app/services/retro_session_service.rb).

The implementation of the "session" storage it uses to do this (https://github.com/pivotal/postfacto/blob/master/api/app/services/retro_session_service.rb) doesn't seem to make sense however (at least not to me) as it is just storing sessions in memory. This means that when a "force logout" occurs, as far as I can tell, it will only disconnect the Action Cable connections for the Retro that happen to be connected to the same Rails instance as the one that received the change password request. The implementation is potentially not taking into account the app running as more than one instance.

seadowg commented 5 years ago

Bumping this. I think this could be fairly a fairly important issue to look into.

textbook commented 5 years ago

I've done some investigation, but am not sure I've got to anywhere useful. Results so far:

If I'm reading all of this correctly, RetroSessionService is just for keeping a hash matching retro IDs to an array of the UUIDs of the connections. That mapping will indeed only be for connections to that specific server instance, relying on the clients connected to other instances to act on a 'force_relogin' request in a trustworthy manner. This seems like A Bad Idea.

Instead, it looks from e.g. https://stackoverflow.com/a/56369357/3001761 like we should be doing this via the ActionCable.server.remote_connections, accessing RemoteConnections. Per the docs this works "across all servers running on all machines, because it uses the internal channel that all of these servers are subscribed to", which is what we're looking for.

However, all the examples of that assume you want to kick a single user, i.e. can uniquely identify a single RemoteConnection to disconnect. There's a pending PR (rails/rails#35319, also via that SO post) to make this more accessible, but still looks like just a single connection. I don't know enough about the internal channel stuff to know how to grab all of the connections that relate to a single channel, but I guess that's the thing to do.

ollieh-m commented 4 years ago

Hi. I noticed this via rails/rails#35319 and thought I might chip in:

grab all of the connections that relate to a single channel

My understanding is that we can only find connections using identifiers declared in ActionCable::Connection. We declare identifiers like this:

identified_by :current_user, :uuid

Then when a consumer connects we set the values of the declared identifiers like this:

def connect
  self.current_user = find_verified_user
  self.uuid = find_uuid
end

That then creates a subscription to the connection's internal channel which is defined by the list of connection identifiers. The internal channel is what the server.remote_connections broadcasts to...

So ultimately as I understand it remote_connections allows us to interact with connections on any server by using the connection identifiers. I don't think there's a way to interact with connections based on what channels the connection has subscribed to; only what identifiers have been set upfront.

When the consumer connects, you need to identify it by some property that will let you know it's a consumer you want to unsubscribe or completely disconnect down the line...

But if you want to kill a channel at some point, and stop all consumers subscribing to it, you could stop broadcasting to that channel, and create a new channel (with a new ID) that consumers need to start subscribing to...