Merge pull request #232 from basecamp/security/room-type-scoping

Scope room administration, authorize message streams, and make unread rooms per-user

Dockerfile

@@ -66,9 +66,6 @@ ENV HTTP_WRITE_TIMEOUT=300
 COPY --from=build --chown=rails:rails /usr/local/bundle /usr/local/bundle
 COPY --from=build --chown=rails:rails /rails /rails
 
-# Install ONCE backup/restore hooks
-COPY --chmod=755 hooks /hooks
-
 # Set version and revision
 ARG APP_VERSION
 ENV APP_VERSION=$APP_VERSION

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

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

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

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

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

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

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

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

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

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 %>
 

config/initializers/turbo_streams_authorization.rb

@@ -0,0 +1,3 @@
+Rails.application.config.to_prepare do
+  Turbo::StreamsChannel.prepend RoomStreamsAreAuthorized
+end

hooks/post-restore

@@ -1,12 +0,0 @@
-#!/bin/bash
-set -euo pipefail
-
-db_name="${RAILS_ENV:-production}.sqlite3"
-snapshot="/rails/storage/backups/$db_name"
-db="/rails/storage/db/$db_name"
-
-if [ -f "$snapshot" ]; then
-  mkdir -p "$(dirname "$db")"
-  cp "$snapshot" "$db"
-  rm -f "$db-wal" "$db-shm"
-fi

hooks/pre-backup

@@ -1,2 +0,0 @@
-#!/bin/bash
-exec /rails/script/admin/prepare-backup

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

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

test/controllers/messages_controller_test.rb

@@ -57,9 +57,22 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "creating a message broadcasts unread room" do
+  test "creating a message broadcasts unread room to each member" do
-    assert_broadcasts "unread_rooms", 1 do
+    @room.users.each do |member|
-      post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } }
+      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
 

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 ] })

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

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