From 79f909765010eb02e8768e6c02e39001749ff992 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 4 May 2026 12:28:45 -0400 Subject: [PATCH 1/3] fix(actioncable): unify authentication via engine and add diff-lcs dependency to unblock deploy Use shared CoPlan::Authentication for both HTTP and ActionCable contexts, falling back when connection.current_user is absent. This fixes websocket auth failures in hosts without ApplicationCable::Connection. Also add missing diff-lcs dependency required by diff_to_operations to fix deploy error. --- Gemfile.lock | 1 + .../channels/coplan/plan_presence_channel.rb | 19 +++++++--- .../coplan/application_controller.rb | 29 +-------------- engine/app/services/coplan/authentication.rb | 37 +++++++++++++++++++ engine/coplan.gemspec | 3 +- .../coplan/plan_presence_channel_spec.rb | 14 +++++++ 6 files changed, 69 insertions(+), 34 deletions(-) create mode 100644 engine/app/services/coplan/authentication.rb diff --git a/Gemfile.lock b/Gemfile.lock index 59b6734..fcb1672 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -3,6 +3,7 @@ PATH specs: coplan-engine (0.4.0) commonmarker + diff-lcs diffy importmap-rails jbuilder diff --git a/engine/app/channels/coplan/plan_presence_channel.rb b/engine/app/channels/coplan/plan_presence_channel.rb index 5c140c2..f25270c 100644 --- a/engine/app/channels/coplan/plan_presence_channel.rb +++ b/engine/app/channels/coplan/plan_presence_channel.rb @@ -2,25 +2,26 @@ module CoPlan class PlanPresenceChannel < ActionCable::Channel::Base def subscribed @plan = Plan.find_by(id: params[:plan_id]) - policy = @plan && PlanPolicy.new(current_user, @plan) - unless @plan && policy.show? + @current_user = resolve_current_user + policy = @plan && @current_user && PlanPolicy.new(@current_user, @plan) + unless @plan && policy&.show? reject return end - PlanViewer.track(plan: @plan, user: current_user) + PlanViewer.track(plan: @plan, user: @current_user) broadcast_viewers end def unsubscribed - return unless @plan + return unless @plan && current_user PlanViewer.expire(plan: @plan, user: current_user) broadcast_viewers end def ping - return unless @plan + return unless @plan && current_user PlanViewer.track(plan: @plan, user: current_user) broadcast_viewers @@ -29,7 +30,13 @@ def ping private def current_user - connection.current_user + @current_user ||= resolve_current_user + end + + def resolve_current_user + return connection.current_user if connection.respond_to?(:current_user) && connection.current_user + + CoPlan::Authentication.user_from_request(connection.request) end def broadcast_viewers diff --git a/engine/app/controllers/coplan/application_controller.rb b/engine/app/controllers/coplan/application_controller.rb index f9abf2a..b2419f9 100644 --- a/engine/app/controllers/coplan/application_controller.rb +++ b/engine/app/controllers/coplan/application_controller.rb @@ -38,13 +38,8 @@ def signed_in? end def authenticate_coplan_user! - callback = CoPlan.configuration.authenticate - unless callback - raise "CoPlan.configure { |c| c.authenticate = ->(request) { ... } } is required" - end - - attrs = callback.call(request) - unless attrs && attrs[:external_id].present? + @current_coplan_user = CoPlan::Authentication.user_from_request(request) + unless @current_coplan_user if agent_request? render plain: agent_redirect_instructions, content_type: "text/markdown", status: :unauthorized elsif CoPlan.configuration.sign_in_path @@ -52,26 +47,6 @@ def authenticate_coplan_user! else head :unauthorized end - return - end - - external_id = attrs[:external_id].to_s - @current_coplan_user = CoPlan::User.find_or_initialize_by(external_id: external_id) - sync_user_attrs(@current_coplan_user, attrs) - if @current_coplan_user.new_record? || @current_coplan_user.changed? - @current_coplan_user.save! - end - rescue ActiveRecord::RecordNotUnique - @current_coplan_user = CoPlan::User.find_by!(external_id: external_id) - sync_user_attrs(@current_coplan_user, attrs) - @current_coplan_user.save! if @current_coplan_user.changed? - end - - def sync_user_attrs(user, attrs) - safe_attrs = attrs.slice(:name, :username, :admin, :avatar_url, :title, :team).compact - user.assign_attributes(safe_attrs) - if attrs.key?(:metadata) && attrs[:metadata].is_a?(Hash) - user.metadata = (user.metadata || {}).merge(attrs[:metadata]) end end diff --git a/engine/app/services/coplan/authentication.rb b/engine/app/services/coplan/authentication.rb new file mode 100644 index 0000000..f55bc1f --- /dev/null +++ b/engine/app/services/coplan/authentication.rb @@ -0,0 +1,37 @@ +module CoPlan + module Authentication + MISSING_AUTHENTICATE_MESSAGE = "CoPlan.configure { |c| c.authenticate = ->(request) { ... } } is required" + + module_function + + def user_from_request(request, callback: CoPlan.configuration.authenticate) + raise MISSING_AUTHENTICATE_MESSAGE unless callback + + attrs = callback.call(request) + return nil unless attrs && attrs[:external_id].present? + + provision_user!(attrs) + end + + def provision_user!(attrs) + external_id = attrs[:external_id].to_s + user = CoPlan::User.find_or_initialize_by(external_id: external_id) + sync_user_attrs(user, attrs) + user.save! if user.new_record? || user.changed? + user + rescue ActiveRecord::RecordNotUnique + user = CoPlan::User.find_by!(external_id: external_id) + sync_user_attrs(user, attrs) + user.save! if user.changed? + user + end + + def sync_user_attrs(user, attrs) + safe_attrs = attrs.slice(:name, :username, :admin, :avatar_url, :title, :team).compact + user.assign_attributes(safe_attrs) + if attrs.key?(:metadata) && attrs[:metadata].is_a?(Hash) + user.metadata = (user.metadata || {}).merge(attrs[:metadata]) + end + end + end +end diff --git a/engine/coplan.gemspec b/engine/coplan.gemspec index 49160ae..670bbb5 100644 --- a/engine/coplan.gemspec +++ b/engine/coplan.gemspec @@ -3,7 +3,7 @@ require_relative "lib/coplan/version" Gem::Specification.new do |spec| spec.name = "coplan-engine" spec.version = CoPlan::VERSION - spec.authors = ["Block"] + spec.authors = [ "Block" ] spec.summary = "CoPlan — AI-assisted engineering design doc review" spec.description = "A Rails Engine for collaborative plan review with AI-powered feedback." spec.license = "Apache-2.0" @@ -14,6 +14,7 @@ Gem::Specification.new do |spec| spec.add_dependency "rails", ">= 8.0" spec.add_dependency "commonmarker" + spec.add_dependency "diff-lcs" spec.add_dependency "diffy" spec.add_dependency "ruby-openai" spec.add_dependency "propshaft" diff --git a/spec/channels/coplan/plan_presence_channel_spec.rb b/spec/channels/coplan/plan_presence_channel_spec.rb index cf88988..9e7da87 100644 --- a/spec/channels/coplan/plan_presence_channel_spec.rb +++ b/spec/channels/coplan/plan_presence_channel_spec.rb @@ -16,6 +16,20 @@ expect(CoPlan::PlanViewer.where(plan: plan, user: user)).to exist end + it "authenticates through the engine callback when the connection has no current_user" do + stub_connection(request: ActionDispatch::Request.empty) + allow(CoPlan.configuration).to receive(:authenticate).and_return( + ->(_request) { { external_id: "websocket-user", name: "Websocket User" } } + ) + plan = create(:plan, created_by_user: user) + + subscribe(plan_id: plan.id) + + websocket_user = CoPlan::User.find_by!(external_id: "websocket-user") + expect(subscription).to be_confirmed + expect(CoPlan::PlanViewer.where(plan: plan, user: websocket_user)).to exist + end + it "rejects when plan does not exist" do subscribe(plan_id: "nonexistent") expect(subscription).to be_rejected From 73685d8e94f6587bc6502c168a81c184f52a70a7 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 4 May 2026 12:44:11 -0400 Subject: [PATCH 2/3] fix(actioncable): require authentication explicitly in channel subscription Reject unauthenticated websocket subscriptions early instead of relying on implicit policy checks. Simplifies lifecycle methods and aligns with reviewer feedback. --- engine/app/channels/coplan/plan_presence_channel.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/engine/app/channels/coplan/plan_presence_channel.rb b/engine/app/channels/coplan/plan_presence_channel.rb index f25270c..2b50aee 100644 --- a/engine/app/channels/coplan/plan_presence_channel.rb +++ b/engine/app/channels/coplan/plan_presence_channel.rb @@ -1,9 +1,14 @@ module CoPlan class PlanPresenceChannel < ActionCable::Channel::Base def subscribed - @plan = Plan.find_by(id: params[:plan_id]) @current_user = resolve_current_user - policy = @plan && @current_user && PlanPolicy.new(@current_user, @plan) + unless @current_user + reject + return + end + + @plan = Plan.find_by(id: params[:plan_id]) + policy = @plan && PlanPolicy.new(@current_user, @plan) unless @plan && policy&.show? reject return @@ -14,14 +19,14 @@ def subscribed end def unsubscribed - return unless @plan && current_user + return unless @plan PlanViewer.expire(plan: @plan, user: current_user) broadcast_viewers end def ping - return unless @plan && current_user + return unless @plan PlanViewer.track(plan: @plan, user: current_user) broadcast_viewers From 7617c71ad439cde57cc37b0443d9f6093ce958a2 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 4 May 2026 13:25:37 -0400 Subject: [PATCH 3/3] fix(actioncable): use current_user accessor in presence channel Route channel subscription auth through the memoized current_user method instead of reading @current_user directly. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- engine/app/channels/coplan/plan_presence_channel.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/engine/app/channels/coplan/plan_presence_channel.rb b/engine/app/channels/coplan/plan_presence_channel.rb index 2b50aee..e6ee4a8 100644 --- a/engine/app/channels/coplan/plan_presence_channel.rb +++ b/engine/app/channels/coplan/plan_presence_channel.rb @@ -1,20 +1,19 @@ module CoPlan class PlanPresenceChannel < ActionCable::Channel::Base def subscribed - @current_user = resolve_current_user - unless @current_user + unless current_user reject return end @plan = Plan.find_by(id: params[:plan_id]) - policy = @plan && PlanPolicy.new(@current_user, @plan) + policy = @plan && PlanPolicy.new(current_user, @plan) unless @plan && policy&.show? reject return end - PlanViewer.track(plan: @plan, user: @current_user) + PlanViewer.track(plan: @plan, user: current_user) broadcast_viewers end