diff --git a/app/channels/concerns/room_streams_are_authorized.rb b/app/channels/concerns/room_streams_are_authorized.rb new file mode 100644 index 0000000..ad45270 --- /dev/null +++ b/app/channels/concerns/room_streams_are_authorized.rb @@ -0,0 +1,13 @@ +# Prepended onto Turbo::StreamsChannel. The subscriber names the channel it wants, so +# authorizing room messages only in RoomMessagesChannel would leave the stock channel as +# a way around it: same signed stream name, no membership check. Turn those names away +# here and RoomMessagesChannel becomes the only door. +module RoomStreamsAreAuthorized + def subscribed + if RoomMessagesChannel.guarded_stream?(verified_stream_name_from_params) + reject + else + super + end + end +end diff --git a/app/channels/room_messages_channel.rb b/app/channels/room_messages_channel.rb new file mode 100644 index 0000000..1575a4f --- /dev/null +++ b/app/channels/room_messages_channel.rb @@ -0,0 +1,55 @@ +# Authorizes the room message stream when the subscription is made, so that revoking a +# membership actually stops delivery. +# +# Turbo's stock channel verifies only the signature on the stream name. That name carries +# no expiry and no binding to a user, so one harvested while a member keeps working after +# the membership is gone. Reconnecting makes it worse rather than better: revoking a +# membership disconnects the user with reconnect: true, and the client then replays its +# subscriptions on the fresh socket. +# +# The room is derived from the verified stream name rather than taken as a parameter, so +# there's nothing for a subscriber to point somewhere else. The subscriber also doesn't +# get to choose the channel: Turbo::StreamsChannel turns these stream names away, so this +# is the only way onto them. See config/initializers/turbo_streams_authorization.rb. +class RoomMessagesChannel < ApplicationCable::Channel + extend Turbo::Streams::StreamName + include Turbo::Streams::StreamName::ClassMethods + + STREAM_SUFFIX = "messages" + + class << self + # True for the stream names this channel exists to guard, whoever is asking. + def guarded_stream?(stream_name) + stream_name.to_s.split(":", 2).second == STREAM_SUFFIX + end + + def subscribable_room(user, stream_name) + gid_param, suffix = stream_name.to_s.split(":", 2) + + if suffix == STREAM_SUFFIX && room = room_from(gid_param) + user.rooms.find_by(id: room.id) + end + end + + private + def room_from(gid_param) + GlobalID::Locator.locate gid_param, only: Room + rescue ActiveRecord::RecordNotFound + nil + end + end + + def subscribed + if stream_name = authorized_stream_name + stream_from stream_name + else + reject + end + end + + private + def authorized_stream_name + stream_name = verified_stream_name_from_params + stream_name if stream_name.present? && self.class.subscribable_room(current_user, stream_name) + end +end diff --git a/app/channels/unread_rooms_channel.rb b/app/channels/unread_rooms_channel.rb index f7fcc08..a3f6cc0 100644 --- a/app/channels/unread_rooms_channel.rb +++ b/app/channels/unread_rooms_channel.rb @@ -1,5 +1,12 @@ class UnreadRoomsChannel < ApplicationCable::Channel + # Scoped per user, like ReadRoomsChannel. A single global stream would tell every + # authenticated connection when any room on the account is active, including closed + # rooms and direct conversations they're not part of. + def self.stream_name_for(user_id) + "user_#{user_id}_unreads" + end + def subscribed - stream_from "unread_rooms" + stream_from self.class.stream_name_for(current_user.id) end end diff --git a/app/controllers/rooms/closeds_controller.rb b/app/controllers/rooms/closeds_controller.rb index 0e8bdf7..c0d01d4 100644 --- a/app/controllers/rooms/closeds_controller.rb +++ b/app/controllers/rooms/closeds_controller.rb @@ -42,6 +42,12 @@ class Rooms::ClosedsController < RoomsController @room = @room.becomes!(Rooms::Closed) end + # Open and closed rooms convert into each other, so both are in reach here. Direct + # rooms never are: converting one would let its creator revise who's in it. + def room_scope + Current.user.rooms.without_directs + end + def grantees User.where(id: grantee_ids) end diff --git a/app/controllers/rooms/directs_controller.rb b/app/controllers/rooms/directs_controller.rb index dc31090..d9b1ec4 100644 --- a/app/controllers/rooms/directs_controller.rb +++ b/app/controllers/rooms/directs_controller.rb @@ -29,8 +29,13 @@ class Rooms::DirectsController < RoomsController end end - # All users in a direct room can administer it + # All users in a direct room can administer it. Only direct rooms, though: this + # relaxation is why room_scope below has to keep every other type out of reach. def ensure_can_administer true end + + def room_scope + Current.user.rooms.directs + end end diff --git a/app/controllers/rooms/opens_controller.rb b/app/controllers/rooms/opens_controller.rb index c2bda9e..2605d22 100644 --- a/app/controllers/rooms/opens_controller.rb +++ b/app/controllers/rooms/opens_controller.rb @@ -40,6 +40,12 @@ class Rooms::OpensController < RoomsController @room = @room.becomes!(Rooms::Open) end + # Open and closed rooms convert into each other, so both are in reach here. Direct + # rooms never are: promoting one would republish its history to the whole account. + def room_scope + Current.user.rooms.without_directs + end + def broadcast_create_room(room) broadcast_prepend_to :rooms, target: :shared_rooms, partial: "users/sidebars/rooms/shared", locals: { room: room } end diff --git a/app/controllers/rooms_controller.rb b/app/controllers/rooms_controller.rb index 2c308c8..1f1d28e 100644 --- a/app/controllers/rooms_controller.rb +++ b/app/controllers/rooms_controller.rb @@ -20,13 +20,19 @@ class RoomsController < ApplicationController private def set_room - if room = Current.user.rooms.find_by(id: params[:room_id] || params[:id]) + if room = room_scope.find_by(id: params[:room_id] || params[:id]) @room = room else redirect_to root_url, alert: "Room not found or inaccessible" end end + # Subclasses narrow this to the room types they're allowed to act on, so that one + # room namespace can't be used to reach another's rooms. + def room_scope + Current.user.rooms + end + def ensure_can_administer head :forbidden unless Current.user.can_administer?(@room) end diff --git a/app/models/message/broadcasts.rb b/app/models/message/broadcasts.rb index 1f909dc..4623f2a 100644 --- a/app/models/message/broadcasts.rb +++ b/app/models/message/broadcasts.rb @@ -1,10 +1,19 @@ module Message::Broadcasts def broadcast_create broadcast_append_to room, :messages, target: [ room, :messages ] - ActionCable.server.broadcast("unread_rooms", { roomId: room.id }) + broadcast_unread_room end def broadcast_remove broadcast_remove_to room, :messages end + + private + # Fanned out to the room's members rather than published on one global stream, so + # that the timing of activity in a room only reaches people who are in it. + def broadcast_unread_room + room.memberships.pluck(:user_id).each do |user_id| + ActionCable.server.broadcast UnreadRoomsChannel.stream_name_for(user_id), { roomId: room.id } + end + end end diff --git a/app/models/room.rb b/app/models/room.rb index a9d97ac..20865fc 100644 --- a/app/models/room.rb +++ b/app/models/room.rb @@ -22,6 +22,8 @@ class Room < ApplicationRecord belongs_to :creator, class_name: "User", default: -> { Current.user } + validate :direct_rooms_keep_their_type, on: :update + scope :opens, -> { where(type: "Rooms::Open") } scope :closeds, -> { where(type: "Rooms::Closed") } scope :directs, -> { where(type: "Rooms::Direct") } @@ -65,6 +67,15 @@ class Room < ApplicationRecord end private + # Open and closed rooms convert into each other freely. A direct room can't become + # either: its participants agreed to a private conversation, not to one whose + # audience someone else gets to widen afterwards. + def direct_rooms_keep_their_type + if type_changed? && type_was == "Rooms::Direct" + errors.add :type, "can't be changed for a direct room" + end + end + def unread_memberships(message) memberships.visible.disconnected.where.not(user: message.creator).update_all(unread_at: message.created_at, updated_at: Time.current) end diff --git a/app/views/rooms/show.html.erb b/app/views/rooms/show.html.erb index 8f6bd07..ef76ecd 100644 --- a/app/views/rooms/show.html.erb +++ b/app/views/rooms/show.html.erb @@ -17,7 +17,7 @@ <%= render partial: "messages/message", collection: @messages, cached: true %> <% end %> - <%= turbo_stream_from @room, :messages %> + <%= turbo_stream_from @room, :messages, channel: "RoomMessagesChannel" %> <%= button_to_jump_to_newest_message %> <% end %> diff --git a/config/initializers/turbo_streams_authorization.rb b/config/initializers/turbo_streams_authorization.rb new file mode 100644 index 0000000..9837ea6 --- /dev/null +++ b/config/initializers/turbo_streams_authorization.rb @@ -0,0 +1,3 @@ +Rails.application.config.to_prepare do + Turbo::StreamsChannel.prepend RoomStreamsAreAuthorized +end diff --git a/test/channels/room_messages_channel_test.rb b/test/channels/room_messages_channel_test.rb new file mode 100644 index 0000000..9c2d112 --- /dev/null +++ b/test/channels/room_messages_channel_test.rb @@ -0,0 +1,86 @@ +require "test_helper" + +class RoomMessagesChannelTest < ActionCable::Channel::TestCase + tests RoomMessagesChannel + + setup do + @room = rooms(:designers) + @signed_stream_name = Turbo::StreamsChannel.signed_stream_name [ @room, :messages ] + end + + test "a member may subscribe to a room's message stream" do + stub_connection(current_user: users(:kevin)) + + subscribe signed_stream_name: @signed_stream_name + + assert subscription.confirmed? + assert_has_stream Turbo.signed_stream_verifier.verified(@signed_stream_name) + end + + test "a user who was never a member may not subscribe" do + stub_connection(current_user: users(:bender)) + + subscribe signed_stream_name: @signed_stream_name + + assert subscription.rejected? + end + + test "a revoked member may not re-subscribe with a stream name harvested while a member" do + stub_connection(current_user: users(:kevin)) + + subscribe signed_stream_name: @signed_stream_name + assert subscription.confirmed?, "kevin must start out able to subscribe" + + @room.memberships.revoke_from users(:kevin) + + subscribe signed_stream_name: @signed_stream_name + + assert subscription.rejected? + end + + test "an unsigned stream name is rejected" do + stub_connection(current_user: users(:kevin)) + + subscribe signed_stream_name: Turbo.signed_stream_verifier.verified(@signed_stream_name) + + assert subscription.rejected? + end + + test "a missing stream name is rejected" do + stub_connection(current_user: users(:kevin)) + + subscribe + + assert subscription.rejected? + end + + test "a validly signed stream name for another room the user isn't in is rejected" do + stub_connection(current_user: users(:bender)) + + subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ rooms(:hq), :messages ]) + + assert subscription.rejected? + end +end + +class RoomMessagesViaStockTurboChannelTest < ActionCable::Channel::TestCase + tests Turbo::StreamsChannel + + # The subscriber picks the channel, so the stock channel has to turn these names away + # too. Otherwise a revoked member just names Turbo::StreamsChannel instead. + test "the stock turbo channel refuses to serve a room message stream" do + stub_connection(current_user: users(:kevin)) + + subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ rooms(:designers), :messages ]) + + assert subscription.rejected? + end + + test "the stock turbo channel still serves the room list stream" do + stub_connection(current_user: users(:kevin)) + + subscribe signed_stream_name: Turbo::StreamsChannel.signed_stream_name([ :rooms ]) + + assert subscription.confirmed? + end +end diff --git a/test/channels/unread_rooms_channel_test.rb b/test/channels/unread_rooms_channel_test.rb new file mode 100644 index 0000000..4444eb1 --- /dev/null +++ b/test/channels/unread_rooms_channel_test.rb @@ -0,0 +1,47 @@ +require "test_helper" + +class UnreadRoomsChannelTest < ActionCable::Channel::TestCase + test "streams only the subscriber's own unread stream" do + stub_connection(current_user: users(:jz)) + + subscribe + + assert subscription.confirmed? + assert_has_stream "user_#{users(:jz).id}_unreads" + assert_not_includes subscription.streams, "unread_rooms" + end + + test "an outsider is not told about activity in a room they can't see" do + direct = rooms(:bender_and_kevin) + assert_not direct.users.include?(users(:jz)), "jz must be an outsider for this test to mean anything" + + broadcasts = capture_unread_broadcasts_for(users(:jz)) do + direct.messages.create!(body: "Private", creator: users(:kevin), client_message_id: "outsider").broadcast_create + end + + assert_empty broadcasts + end + + test "a member is told about activity in their own room" do + direct = rooms(:bender_and_kevin) + + broadcasts = capture_unread_broadcasts_for(users(:kevin)) do + direct.messages.create!(body: "Private", creator: users(:bender), client_message_id: "member").broadcast_create + end + + assert_equal [ direct.id ], broadcasts.collect { |broadcast| broadcast["roomId"] } + end + + private + def capture_unread_broadcasts_for(user) + stub_connection(current_user: user) + subscribe + + stream = subscription.streams.sole + before = ActionCable.server.pubsub.broadcasts(stream).size + + yield + + ActionCable.server.pubsub.broadcasts(stream).drop(before).collect { |broadcast| JSON.parse(broadcast) } + end +end diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb index 1f556bf..8088694 100644 --- a/test/controllers/messages_controller_test.rb +++ b/test/controllers/messages_controller_test.rb @@ -57,9 +57,22 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest end end - test "creating a message broadcasts unread room" do - assert_broadcasts "unread_rooms", 1 do - post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } } + test "creating a message broadcasts unread room to each member" do + @room.users.each do |member| + assert_broadcasts UnreadRoomsChannel.stream_name_for(member.id), 1 do + post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one #{member.id}", client_message_id: member.id } } + end + end + end + + test "creating a message doesn't broadcast unread room to non-members" do + outsiders = User.where.not(id: @room.users.map(&:id)) + assert outsiders.any?, "need someone outside the room for this test to mean anything" + + outsiders.each do |outsider| + assert_no_broadcasts UnreadRoomsChannel.stream_name_for(outsider.id) do + post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } } + end end end diff --git a/test/controllers/rooms/closeds_controller_test.rb b/test/controllers/rooms/closeds_controller_test.rb index d8976ac..504745d 100644 --- a/test/controllers/rooms/closeds_controller_test.rb +++ b/test/controllers/rooms/closeds_controller_test.rb @@ -66,6 +66,18 @@ class Rooms::ClosedsControllerTest < ActionDispatch::IntegrationTest assert rooms(:designers).reload.name, "Designers" end + test "a direct room can't be converted to closed and have its participants revised" do + sign_in :kevin + direct = rooms(:bender_and_kevin) + + put rooms_closed_url(direct), params: { + room: { name: "Watercooler" }, user_ids: [ users(:kevin).id, users(:jz).id ] + } + + assert_equal "Rooms::Direct", Room.find(direct.id).type + assert_equal [ users(:bender).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort + end + test "remove yourself" do assert_difference -> { users(:david).rooms.count }, -1 do put rooms_closed_url(rooms(:designers), params: { room: { name: "Designers" }, user_ids: [ users(:jason).id, users(:jz).id ] }) diff --git a/test/controllers/rooms/directs_controller_test.rb b/test/controllers/rooms/directs_controller_test.rb index 18aeb7d..a1d7b06 100644 --- a/test/controllers/rooms/directs_controller_test.rb +++ b/test/controllers/rooms/directs_controller_test.rb @@ -29,4 +29,34 @@ class Rooms::DirectsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to root_url end end + + test "destroy can't reach a closed room the member didn't create" do + sign_in :kevin + + assert_no_difference -> { Room.count } do + delete rooms_direct_url(rooms(:designers)) + end + + assert rooms(:designers).reload.persisted? + end + + test "destroy can't reach an open room the member didn't create" do + sign_in :kevin + + assert_no_difference -> { Room.count } do + delete rooms_direct_url(rooms(:hq)) + end + + assert rooms(:hq).reload.persisted? + end + + test "destroy can't reach a room the member isn't in at all" do + sign_in :jz + + assert_no_difference -> { Room.count } do + delete rooms_direct_url(rooms(:david_and_kevin)) + end + + assert rooms(:david_and_kevin).reload.persisted? + end end diff --git a/test/controllers/rooms/opens_controller_test.rb b/test/controllers/rooms/opens_controller_test.rb index 0f91294..680c2e1 100644 --- a/test/controllers/rooms/opens_controller_test.rb +++ b/test/controllers/rooms/opens_controller_test.rb @@ -57,4 +57,23 @@ class Rooms::OpensControllerTest < ActionDispatch::IntegrationTest put rooms_open_url(rooms(:designers)), params: { room: { name: "Doesn't matter" } } assert_equal rooms(:designers).memberships.count, User.count end + + test "a direct room can't be promoted to open by its creator" do + sign_in :kevin + direct = rooms(:bender_and_kevin) + + put rooms_open_url(direct), params: { room: { name: "Watercooler" } } + + assert_equal "Rooms::Direct", Room.find(direct.id).type + assert_equal [ users(:bender).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort + end + + test "a direct room can't be promoted to open by an administrator either" do + direct = rooms(:david_and_kevin) + + put rooms_open_url(direct), params: { room: { name: "Watercooler" } } + + assert_equal "Rooms::Direct", Room.find(direct.id).type + assert_equal [ users(:david).id, users(:kevin).id ].sort, Room.find(direct.id).user_ids.sort + end end