diff --git a/app/controllers/api/v1/pipeline_items/products_controller.rb b/app/controllers/api/v1/pipeline_items/products_controller.rb index ee27ea133..13bd8451b 100644 --- a/app/controllers/api/v1/pipeline_items/products_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/pipeline_items_controller.rb b/app/controllers/api/v1/pipeline_items_controller.rb index 9e531ff1b..ccbec8327 100644 --- a/app/controllers/api/v1/pipeline_items_controller.rb +++ b/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__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 diff --git a/app/policies/pipeline_policy.rb b/app/policies/pipeline_policy.rb index 4c0cd0710..8986a76a2 100644 --- a/app/policies/pipeline_policy.rb +++ b/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 diff --git a/spec/fixtures/rbac/permission_catalog.yml b/spec/fixtures/rbac/permission_catalog.yml index b5e8815ae..9117c1c52 100644 --- a/spec/fixtures/rbac/permission_catalog.yml +++ b/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 diff --git a/spec/rbac/mutating_actions_gate_guard_spec.rb b/spec/rbac/mutating_actions_gate_guard_spec.rb index 60849dbca..6cba58bef 100644 --- a/spec/rbac/mutating_actions_gate_guard_spec.rb +++ b/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. diff --git a/spec/requests/api/v1/pipeline_items_archived_spec.rb b/spec/requests/api/v1/pipeline_items_archived_spec.rb index 7c6f01070..14a2dd1a5 100644 --- a/spec/requests/api/v1/pipeline_items_archived_spec.rb +++ b/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 } diff --git a/spec/requests/api/v1/pipeline_items_permission_rbac_spec.rb b/spec/requests/api/v1/pipeline_items_permission_rbac_spec.rb new file mode 100644 index 000000000..a6a749ed7 --- /dev/null +++ b/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 diff --git a/spec/requests/api/v1/pipeline_items_write_authz_spec.rb b/spec/requests/api/v1/pipeline_items_write_authz_rbac_spec.rb similarity index 63% rename from spec/requests/api/v1/pipeline_items_write_authz_spec.rb rename to spec/requests/api/v1/pipeline_items_write_authz_rbac_spec.rb index 6244b551b..0a5d5cbdf 100644 --- a/spec/requests/api/v1/pipeline_items_write_authz_spec.rb +++ b/spec/requests/api/v1/pipeline_items_write_authz_rbac_spec.rb @@ -2,11 +2,12 @@ require 'rails_helper' -# Pipeline item mutating actions authorize against the pipeline WRITE policy -# (PipelinePolicy#update?), while reads stay at #view?. Previously every action -# — including create/update/destroy — authorized only #view? (a read-level -# check) on the parent pipeline. These specs prove the split: a caller allowed -# to view but not update the pipeline can list items but cannot create one. +# Pipeline item mutating actions authorize against the dedicated CARD-write policy +# (PipelinePolicy#update_items? -> pipeline_items.update), while reads stay at +# #view?. These specs prove the split: a caller allowed to VIEW the pipeline but +# lacking card-write can list items but cannot create one. (The permission-level +# proof — that pipeline_items.update, not pipelines.update, is what gates card +# writes — lives in pipeline_items_permission_rbac_spec.rb.) RSpec.describe 'Pipeline item write-level authorization', type: :request do let(:user) { User.create!(name: 'Perm Probe', email: "probe-#{SecureRandom.hex(4)}@example.com") } let(:pipeline) { Pipeline.create!(name: 'Sales', pipeline_type: 'sales', created_by: user) } @@ -17,9 +18,9 @@ Current.user = probe Current.evo_permission_cache ||= {} end - # View is permitted, write is not: isolates the read-vs-write policy level. + # View is permitted, card-write is not: isolates the read-vs-card-write level. allow_any_instance_of(PipelinePolicy).to receive(:view?).and_return(true) - allow_any_instance_of(PipelinePolicy).to receive(:update?).and_return(false) + allow_any_instance_of(PipelinePolicy).to receive(:update_items?).and_return(false) end after { Current.reset }