From 5c5c82b27a0cb44e1cb2a8037e620d648dc89af6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 14:55:04 -0700 Subject: [PATCH] Scope room lookup to the type each controller administers Rooms::DirectsController relaxes ensure_can_administer to true, because every participant in a direct room may administer it. set_room was inherited unscoped, though, so that relaxation applied to any room the caller was merely a member of: DELETE /rooms/directs/ destroyed open and closed rooms and all their messages. The same unscoped lookup let a direct room be loaded by the opens and closeds controllers, where force_room_type promoted it. Promoting a DM to open grants every user on the account membership and republishes the whole conversation, including the other participant's messages; converting it to closed lets the initiator revise who is in it and lock the other participant out. Each controller now narrows room_scope to the types it may act on. Opens and closeds keep reach into each other, since converting between them is a feature. Neither can reach a direct room, and directs can only reach directs. Room also refuses to change type away from Rooms::Direct, so the invariant holds for any future caller of becomes! rather than only these two controllers. --- app/controllers/rooms/closeds_controller.rb | 6 ++++ app/controllers/rooms/directs_controller.rb | 7 ++++- app/controllers/rooms/opens_controller.rb | 6 ++++ app/controllers/rooms_controller.rb | 8 ++++- app/models/room.rb | 11 +++++++ .../rooms/closeds_controller_test.rb | 12 ++++++++ .../rooms/directs_controller_test.rb | 30 +++++++++++++++++++ .../rooms/opens_controller_test.rb | 19 ++++++++++++ 8 files changed, 97 insertions(+), 2 deletions(-) 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/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/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