fix(pipeline): gate card writes on pipeline_items.update, not pipelines.update

app/controllers/api/v1/pipeline_items/products_controller.rb

@@ -6,7 +6,9 @@ class Api::V1::PipelineItems::ProductsController < Api::V1::BaseController
     destroy: 'pipelines.update'
   })
 
-  # Mirrors PipelineItemsController: reads gate on :view?, mutations on :update?.
+  # Product-catalog writes are MANAGER-level (pipelines.update via require_permissions
+  # above) — unlike pipeline CARD writes, which CRM-178 moved to the agent's
+  # pipeline_items.update. Editing a pipeline's product catalog is not attendance.
   WRITE_ACTIONS = %w[create update destroy].freeze
 
   before_action :fetch_pipeline_item

app/controllers/api/v1/pipeline_items_controller.rb

@@ -6,11 +6,22 @@ class Api::V1::PipelineItemsController < Api::V1::BaseController
 
   # Mutating actions authorize against the pipeline write policy; reads stay at
   # view level.
+  # Card writes an AGENT may run — gated on the dedicated pipeline_items.update key
+  # (PipelinePolicy#update_items?). `destroy` is deliberately NOT here: deleting a
+  # card cascades to its stage_movements/tasks/products (a destructive
+  # restructuring), so it stays manager-level (pipelines.update). See
+  # ensure_authorized_user.
   WRITE_ACTIONS = %w[
-    create update destroy bulk_move move_conversation
+    create update bulk_move move_conversation
     move_to_stage update_conversation update_custom_fields
   ].freeze
 
+  # Card writes authorize via Pundit (PipelinePolicy#update_items?), not the
+  # require_permissions/check_<action>_permission! named gate — the scope check
+  # (accessible_record?) has to run on the resolved pipeline. Register the key so
+  # the auth catalog-conformance guard still sees it (CRM-178 review LOW 9).
+  EvoPermissionConcern.register_permission_key('pipeline_items.update')
+
   before_action :set_pipeline
   before_action :set_pipeline_item, only: [:update, :destroy, :move_to_stage, :update_conversation, :update_custom_fields]
   before_action :ensure_authorized_user
@@ -236,7 +247,11 @@ def update
   # rubocop:enable Metrics/MethodLength, Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
 
   # rubocop:disable Metrics/MethodLength, Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
-  def update_notesconversation
+  # Routed as PATCH .../pipeline_items/:id/update_conversation (was defined as the
+  # typo `update_notesconversation`, which no route reached — CRM-178 review LOW 10).
+  # It IS a card write, so it must keep the name the route/WRITE_ACTIONS/
+  # set_pipeline_item all reference, or it silently falls to :view? read-level.
+  def update_conversation
     # Handle stage change
     if params[:stage_id].present?
       new_stage = @pipeline.pipeline_stages.find(params[:stage_id])
@@ -549,10 +564,23 @@ def relocate_conversation(conversation, stage)
     elsif current_item.pipeline_id == @pipeline.id
       ['same_pipeline', service.move_to_stage(current_item, stage)]
     else
+      authorize_source_pipeline!(current_item.pipeline)
       ['cross_pipeline', service.move_to_pipeline_stage(current_item, stage)]
     end
   end
 
+  # A cross-pipeline relocate REMOVES the card from its previous pipeline, and
+  # ensure_authorized_user only checked the TARGET (the one in the URL). Card writes
+  # are agent-level since CRM-178, so without this the agent could pull a card out of
+  # a funnel it cannot even see by naming a funnel it can. Same predicate as the
+  # target, so accessible_record? runs on the source too. Service tokens (evo-flow
+  # journeys) are exempt, like ensure_authorized_user.
+  def authorize_source_pipeline!(source_pipeline)
+    return if service_authenticated?
+
+    authorize source_pipeline, :update_items?
+  end
+
   # Attaches notes to the most recent stage_movement so a journey/manual note
   # persists even when the stage didn't change. If the item somehow has no
   # movement yet, create a manual one carrying the note (mirrors the
@@ -785,7 +813,24 @@ def apply_sorting
   def ensure_authorized_user
     return if service_authenticated?
 
-    authorize @pipeline, WRITE_ACTIONS.include?(action_name) ? :update? : :view?
+    # Three levels, all preserving accessible_record? inside the predicate:
+    #   - destroy  -> :update? (pipelines.update, MANAGER): deleting a card cascades
+    #     to its stage_movements/tasks/products — a destructive restructuring, not
+    #     attendance (mirrors CRM-182 keeping deletes off the agent).
+    #   - other card writes (create/move/edit) -> :update_items? (pipeline_items.update,
+    #     AGENT): the salesperson moves/creates cards without the manager's power to
+    #     reshape/archive the funnel.
+    #   - reads -> :view? (pipelines.read).
+    predicate =
+      if action_name == 'destroy'
+        :update?
+      elsif WRITE_ACTIONS.include?(action_name)
+        :update_items?
+      else
+        :view?
+      end
+
+    authorize @pipeline, predicate
   end
 end
 # rubocop:enable Metrics/ClassLength

app/policies/pipeline_policy.rb

@@ -42,6 +42,16 @@ def update?
     permitted_write? && accessible_record?
   end
 
+  # Card (pipeline_items) writes: create a card, pull a conversation in, move a card
+  # between stages, edit card fields. Gated by the dedicated `pipeline_items.update`
+  # permission — the salesperson's routine — NOT the manager-level `pipelines.update`
+  # that reshapes/archives the funnel. The pipeline must still be accessible (same
+  # Scope as #update?), so an agent cannot touch cards in a private funnel it cannot
+  # see. PipelineItemsController authorizes its WRITE_ACTIONS against this predicate.
+  def update_items?
+    permitted_item_write? && accessible_record?
+  end
+
   def destroy?
     permitted_delete? && accessible_record?
   end
@@ -75,13 +85,19 @@ def permitted_write?
     @user&.administrator? || @user&.has_permission?('pipelines.update')
   end
 
+  # Card-level write gate. Distinct from permitted_write? (pipelines.update) so the
+  # agent can move/create cards without the manager's power to edit/archive the funnel.
+  def permitted_item_write?
+    @user&.administrator? || @user&.has_permission?('pipeline_items.update')
+  end
+
   def permitted_delete?
     @user&.administrator? || @user&.has_permission?('pipelines.delete')
   end
 
   # EVO-2204: routes through the SAME Scope#resolve that filters #index, so detail and
   # list can never disagree. It is a READ predicate reused as the write gate — tightening
-  # that is not local, pipeline_items authorizes card writes against the same `update?`.
+  # that is not local: every write predicate here composes it, `update_items?` included.
   def accessible_record?
     scope.exists?(id: @record.id)
   end

spec/fixtures/rbac/permission_catalog.yml

@@ -222,6 +222,7 @@
 - oauth_applications.read
 - oauth_applications.regenerate_secret
 - oauth_applications.update
+- pipeline_items.update
 - pipeline_stages.create
 - pipeline_stages.delete
 - pipeline_stages.read

spec/rbac/mutating_actions_gate_guard_spec.rb

@@ -46,8 +46,9 @@
     # matching how sibling update/destroy are authorized in these controllers.
     'api/v1/scheduled_actions' => %w[create],
     'api/v1/pipeline_tasks' => %w[create],
-    # Pundit-gated on the parent pipeline (authorize @pipeline): mutations at
-    # write-level (:update?), reads at :view?; skipped for service tokens.
+    # Pundit-gated on the parent pipeline (authorize @pipeline), three levels since
+    # CRM-178: destroy at :update? (manager), other card writes at :update_items?
+    # (agent), reads at :view?; skipped for service tokens.
     'api/v1/pipeline_items' =>
       %w[create update destroy bulk_move move_conversation move_to_stage update_conversation update_custom_fields],
     # POST-shaped reads inside the new-conversation flow.

spec/requests/api/v1/pipeline_items_archived_spec.rb

@@ -21,8 +21,12 @@
       Current.user = probe
       Current.evo_permission_cache ||= {}
     end
+    # Card writes now authorize against #update_items? (CRM-178); keep #update?
+    # and #view? stubbed too so this spec stays focused on the archived-guard, not
+    # on the permission split.
     allow_any_instance_of(PipelinePolicy).to receive(:view?).and_return(true)
     allow_any_instance_of(PipelinePolicy).to receive(:update?).and_return(true)
+    allow_any_instance_of(PipelinePolicy).to receive(:update_items?).and_return(true)
   end
 
   after { Current.reset }

spec/requests/api/v1/pipeline_items_permission_rbac_spec.rb

@@ -0,0 +1,212 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+require 'yaml'
+
+# Permission-level proof for the pipeline card-write gate (card #178).
+# PipelineItemsController authorizes its WRITE_ACTIONS via PipelinePolicy#update_items?,
+# which requires the dedicated `pipeline_items.update` permission — NOT the
+# manager-level `pipelines.update`. This guards against a future edit silently
+# re-coupling card writes to pipelines.update (the over-grant that let an agent
+# archive the funnel) or dropping the key from the catalog.
+RSpec.describe 'Pipeline card-write permission (pipeline_items.update)', type: :request do
+  let(:user) { User.create!(name: 'Perm Probe', email: "probe-#{SecureRandom.hex(4)}@example.com") }
+  # Public pipeline so accessible_record? passes for any user — isolates the
+  # permission check from the visibility check.
+  let(:pipeline) do
+    Pipeline.create!(name: 'Sales', pipeline_type: 'sales', visibility: :public, created_by: user)
+  end
+
+  before do
+    probe = user
+    allow_any_instance_of(Api::BaseController).to receive(:authenticate_request!) do
+      Current.user = probe
+      Current.evo_permission_cache ||= {}
+    end
+  end
+
+  after { Current.reset }
+
+  # Stubs the permission seam (User#has_permission? -> PermissionResolver ->
+  # EvoAuthService#check_user_permission) to a literal allow-list.
+  def grant_permissions(*granted)
+    allow_any_instance_of(EvoAuthService).to receive(:check_user_permission) do |_svc, _uid, permission|
+      granted.include?(permission)
+    end
+  end
+
+  let(:stage) { PipelineStage.create!(pipeline: pipeline, name: 'New', position: 1) }
+  # PipelineItem requires a conversation or a contact — a contact is the cheapest.
+  let(:card_contact) { Contact.create!(name: "Card #{SecureRandom.hex(3)}") }
+  let(:card) { PipelineItem.create!(pipeline: pipeline, pipeline_stage: stage, contact: card_contact) }
+
+  # A real card create: a contact placed on the pipeline's first stage. Needs a
+  # stage to exist, so touch `stage`.
+  def create_card
+    stage
+    contact = Contact.create!(name: "Lead #{SecureRandom.hex(3)}")
+    post "/api/v1/pipelines/#{pipeline.id}/pipeline_items",
+         params: { type: 'contact', item_id: contact.id }, as: :json
+  end
+
+  it 'DENIES a card write to a user without pipeline_items.update' do
+    grant_permissions('pipelines.read')
+
+    expect { create_card }.not_to change(PipelineItem, :count)
+    expect(response).to have_http_status(:unauthorized)
+  end
+
+  it 'DENIES a card write to a holder of pipelines.update but NOT pipeline_items.update (the split is real)' do
+    # Before card #178, granting pipelines.update was the only way to unblock the
+    # card — but it also unlocked archive/set_as_default. It must NOT imply card writes.
+    grant_permissions('pipelines.read', 'pipelines.update')
+
+    expect { create_card }.not_to change(PipelineItem, :count)
+    expect(response).to have_http_status(:unauthorized)
+  end
+
+  it 'AUTHORIZES the create for a holder of pipeline_items.update — the card is created (2xx)' do
+    grant_permissions('pipelines.read', 'pipeline_items.update')
+
+    expect { create_card }.to change(PipelineItem, :count).by(1)
+    expect(response).to have_http_status(:success)
+  end
+
+  describe 'move_to_stage is a card write (pipeline_items.update), not manager-level' do
+    it 'AUTHORIZES move_to_stage for a holder of pipeline_items.update (gate opens)' do
+      target = PipelineStage.create!(pipeline: pipeline, name: 'Won', position: 2)
+      grant_permissions('pipelines.read', 'pipeline_items.update')
+
+      patch "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}/move_to_stage",
+            params: { pipeline_stage_id: target.id }, as: :json
+
+      # The authorization gate opened (Pundit would 401 without the key); the
+      # move itself resolves the card via the conversation-first lookup, out of
+      # scope for this authz spec.
+      expect(response).not_to have_http_status(:unauthorized)
+    end
+
+    it 'DENIES move_to_stage without pipeline_items.update' do
+      grant_permissions('pipelines.read')
+
+      patch "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}/move_to_stage",
+            params: { pipeline_stage_id: stage.id }, as: :json
+
+      expect(response).to have_http_status(:unauthorized)
+    end
+  end
+
+  # CRM-178 review (achado 2): deleting a card cascades to its
+  # stage_movements/tasks/products, so `destroy` must stay MANAGER-level
+  # (pipelines.update) — the agent's pipeline_items.update must NOT unlock it, the
+  # same way CRM-182 kept deletes off the agent.
+  describe 'DELETE (destroy) stays manager-level — the agent key does NOT unlock it' do
+    it 'DENIES destroy to a holder of pipeline_items.update (agent) and keeps the card' do
+      card
+      grant_permissions('pipelines.read', 'pipeline_items.update')
+
+      delete "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}", as: :json
+
+      expect(response).to have_http_status(:unauthorized)
+      expect(PipelineItem.exists?(card.id)).to be(true)
+    end
+
+    it 'ALLOWS destroy for a holder of pipelines.update (manager)' do
+      card
+      grant_permissions('pipelines.read', 'pipelines.update')
+
+      delete "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}", as: :json
+
+      expect(response).not_to have_http_status(:unauthorized)
+    end
+  end
+
+  # CRM-178 review (achado 5): `update_conversation` was defined as the typo
+  # `update_notesconversation`, so PATCH .../update_conversation reached no action and
+  # the body never ran. Fixing the name makes a never-executed endpoint live, so it
+  # gets covered here — gate AND effect — instead of shipping on inspection alone.
+  describe 'update_conversation (route was dead until CRM-178) is a card write' do
+    let(:target_stage) { PipelineStage.create!(pipeline: pipeline, name: 'Won', position: 2) }
+
+    it 'DENIES update_conversation without pipeline_items.update' do
+      card
+      grant_permissions('pipelines.read')
+
+      patch "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}/update_conversation",
+            params: { stage_id: target_stage.id }, as: :json
+
+      expect(response).to have_http_status(:unauthorized)
+      expect(card.reload.pipeline_stage_id).to eq(stage.id)
+    end
+
+    it 'MOVES the card and persists the note for a holder of pipeline_items.update' do
+      card
+      grant_permissions('pipelines.read', 'pipeline_items.update')
+
+      patch "/api/v1/pipelines/#{pipeline.id}/pipeline_items/#{card.id}/update_conversation",
+            params: { stage_id: target_stage.id, notes: 'moved by the agent' }, as: :json
+
+      expect(response).to have_http_status(:success)
+      expect(card.reload.pipeline_stage_id).to eq(target_stage.id)
+      # Not `.last`: stage_movements has a uuid PK, so an unordered #last is not
+      # chronological. The note landing on any movement is the claim under test.
+      expect(card.stage_movements.pluck(:notes)).to include('moved by the agent')
+    end
+  end
+
+  # CRM-178 review (achado 3): a cross-pipeline move REMOVES the card from its previous
+  # pipeline, and the action only authorized the pipeline named in the URL. With card
+  # writes now agent-level, that let an agent pull a card out of a funnel it cannot see
+  # by naming one it can.
+  describe 'move_conversation authorizes the SOURCE pipeline too' do
+    let(:other_user) { User.create!(name: 'Owner', email: "owner-#{SecureRandom.hex(4)}@example.com") }
+    let(:channel) { Channel::WebWidget.create!(website_url: 'https://crm178.example.com') }
+    let(:inbox) { Inbox.create!(name: "Inbox #{SecureRandom.hex(3)}", channel: channel) }
+    let(:conversation_contact) { Contact.create!(name: "Contact #{SecureRandom.hex(3)}") }
+    let(:contact_inbox) do
+      ContactInbox.create!(inbox: inbox, contact: conversation_contact, source_id: SecureRandom.hex(8))
+    end
+    let(:conversation) do
+      Conversation.create!(inbox: inbox, contact: conversation_contact, contact_inbox: contact_inbox)
+    end
+
+    # Private and owned by someone else: outside PipelinePolicy::Scope for our probe.
+    let(:private_pipeline) do
+      Pipeline.create!(name: 'Diretoria', pipeline_type: 'sales', visibility: :private, created_by: other_user)
+    end
+    let(:private_stage) { PipelineStage.create!(pipeline: private_pipeline, name: 'Held', position: 1) }
+    let!(:private_card) do
+      PipelineItem.create!(pipeline: private_pipeline, pipeline_stage: private_stage, conversation: conversation)
+    end
+
+    def move_into_target
+      stage
+      patch "/api/v1/pipelines/#{pipeline.id}/pipeline_items/move_conversation",
+            params: { conversation_id: conversation.id, pipeline_stage_id: stage.id }, as: :json
+    end
+
+    it 'DENIES pulling a card out of a pipeline the caller cannot see, and leaves it there' do
+      grant_permissions('pipelines.read', 'pipeline_items.update')
+
+      move_into_target
+
+      expect(response).to have_http_status(:unauthorized)
+      expect(private_card.reload.pipeline_id).to eq(private_pipeline.id)
+    end
+
+    it 'ALLOWS the relocate when the source pipeline IS accessible' do
+      private_pipeline.update!(visibility: :public)
+      grant_permissions('pipelines.read', 'pipeline_items.update')
+
+      move_into_target
+
+      expect(response).not_to have_http_status(:unauthorized)
+      expect(private_card.reload.pipeline_id).to eq(pipeline.id)
+    end
+  end
+
+  it 'gates on a permission key that exists in the auth catalog mirror' do
+    catalog = YAML.safe_load_file(Rails.root.join('spec/fixtures/rbac/permission_catalog.yml')).to_set
+    expect(catalog).to include('pipeline_items.update')
+  end
+end