Fixes #27275 - Fix permission bypasses in controllers

app/controllers/foreman_tasks/api/tasks_controller.rb

@@ -20,7 +20,7 @@ class BadRequest < Apipie::ParamError
 
       api :GET, '/tasks/summary', 'Show task summary'
       def summary
-        render :json => ForemanTasks::Task::Summarizer.new.summarize_by_status
+        render :json => ForemanTasks::Task::Summarizer.new(resource_scope).summarize_by_status
       end
 
       api :GET, '/tasks/:id', 'Show task details'
@@ -267,7 +267,7 @@ def task_hash(task)
       end
 
       def find_task
-        @task = Task.find(params[:id])
+        @task = resource_scope.find(params[:id])
       end
 
       def resource_scope(_options = {})
@@ -276,7 +276,7 @@ def resource_scope(_options = {})
 
       def action_permission
         case params[:action]
-        when 'bulk_search'
+        when 'bulk_search', 'summary'
           :view
         when 'bulk_resume'
           :edit

app/controllers/foreman_tasks/tasks_controller.rb

@@ -6,7 +6,7 @@ class TasksController < ::ApplicationController
     before_action :restrict_dangerous_actions, :only => [:unlock, :force_unlock]
 
     def show
-      @task = Task.find(params[:id])
+      @task = resource_base.find(params[:id])
       render :layout => !request.xhr?
     end
 
@@ -25,11 +25,11 @@ def index
     end
 
     def summary
-      render json: Task::Summarizer.new(params[:recent_timeframe].to_i).summary
+      render json: Task::Summarizer.new(resource_base, params[:recent_timeframe].to_i).summary
     end
 
     def sub_tasks
-      task   = Task.find(params[:id])
+      task   = resource_base.find(params[:id])
       @tasks = filter(task.sub_tasks)
       render :index
     end

app/models/foreman_tasks/task/summarizer.rb

@@ -47,16 +47,17 @@ def update(field, counts)
       end
     end
 
-    def initialize(recent_timeframe = 24)
+    def initialize(scope = Task.authorized, recent_timeframe = 24)
       @recent_timeframe = recent_timeframe.hours
+      @scope = scope
     end
 
     def summarize_by_status
       aggregated_scope.where("result <> 'success'")
     end
 
     def latest_tasks_in_errors_warning(limit = 5)
-      ::ForemanTasks::Task.where('result in (?)', %w[error warning]).order('started_at DESC').limit(limit)
+      @scope.where('result in (?)', %w[error warning]).order('started_at DESC').limit(limit)
     end
 
     # Returns summary of tasks count, grouped by `state` and `result`, if form of:
@@ -94,8 +95,8 @@ def add_to_summary(aggregated_scope, field)
     end
 
     def aggregated_scope
-      ::ForemanTasks::Task.select('count(state) AS count, state, result, max(started_at) AS started_at')
-                          .group(:state, :result).order(:state)
+      @scope.select('count(state) AS count, state, result, max(started_at) AS started_at')
+            .group(:state, :result).order(:state)
     end
 
     def aggregated_recent_scope

test/controllers/api/tasks_controller_test.rb

@@ -25,6 +25,13 @@ class Api::TasksControllerTest < ActionController::TestCase
           assert_response :missing
           assert_includes @response.body, 'Resource task not found by id'
         end
+
+        it 'does not show task the user is not allowed to see' do
+          setup_user('view', 'foreman_tasks', 'owner.id = current_user')
+          get :show, params: { id: FactoryBot.create(:some_task).id },
+                     session: set_session_user(User.current)
+          assert_response :not_found
+        end
       end
 
       describe 'GET /api/tasks/summary' do
@@ -41,24 +48,41 @@ def run
               suspend
             end
           end
+
+          def self.while_suspended
+            triggered = ForemanTasks.trigger(DummyTestSummaryAction, true)
+            wait_for { ForemanTasks::Task.find_by(external_id: triggered.id).state == 'running' }
+            wait_for do
+              w = ForemanTasks.dynflow.world
+              w.persistence.load_step(triggered.id, 2, w).state == :suspended
+            end
+            yield
+            ForemanTasks.dynflow.world.event(triggered.id, 2, nil)
+            triggered.finished.wait
+          end
         end
 
         test_attributes :pid => 'bdcab413-a25d-4fe1-9db4-b50b5c31ebce'
         it 'get tasks summary' do
-          triggered = ForemanTasks.trigger(DummyTestSummaryAction, true)
-          wait_for { ForemanTasks::Task.find_by(external_id: triggered.id).state == 'running' }
-          wait_for do
-            w = ForemanTasks.dynflow.world
-            w.persistence.load_step(triggered.id, 2, w).state == :suspended
+          DummyTestSummaryAction.while_suspended do
+            get :summary
+            assert_response :success
+            response = JSON.parse(@response.body)
+            assert_kind_of Array, response
+            assert_not response.empty?
+            assert_kind_of Hash, response[0]
+          end
+        end
+
+        it 'gets tasks summary only for tasks the user is allowed to see' do
+          DummyTestSummaryAction.while_suspended do
+            setup_user('view', 'foreman_tasks', 'owner.id = current_user')
+            get :summary
+            assert_response :success
+            response = JSON.parse(@response.body)
+            assert_kind_of Array, response
+            assert response.empty?
           end
-          get :summary
-          assert_response :success
-          response = JSON.parse(@response.body)
-          assert_kind_of Array, response
-          assert_not response.empty?
-          assert_kind_of Hash, response[0]
-          ForemanTasks.dynflow.world.event(triggered.id, 2, nil)
-          triggered.finished.wait
         end
       end
 

test/controllers/tasks_controller_test.rb

@@ -32,6 +32,15 @@ def in_taxonomy_scope(organization, location = nil)
           response = JSON.parse(@response.body)
           assert response['running']
         end
+
+        it 'shows summary only for the tasks the user is allowed to see' do
+          setup_user('view', 'foreman_tasks', 'owner.id = current_user')
+          FactoryBot.create(:some_task)
+          get(:summary, params: { recent_timeframe: 24 }, session: set_session_user(User.current))
+          assert_response :success
+          response = JSON.parse(@response.body)
+          assert_equal 0, response['stopped']['total']
+        end
       end
 
       it 'supports csv export' do
@@ -42,6 +51,24 @@ def in_taxonomy_scope(organization, location = nil)
         assert_include response.body.lines[1], 'Some action'
       end
 
+      describe 'show' do
+        it 'does not allow user without permissions to see task details' do
+          setup_user('view', 'foreman_tasks', 'owner.id = current_user')
+          get :show, params: { id: FactoryBot.create(:some_task).id },
+                     session: set_session_user(User.current)
+          assert_response :not_found
+        end
+      end
+
+      describe 'sub_tasks' do
+        it 'does not allow user without permissions to see task details' do
+          setup_user('view', 'foreman_tasks', 'owner.id = current_user')
+          get :sub_tasks, params: { id: FactoryBot.create(:some_task).id },
+                          session: set_session_user(User.current)
+          assert_response :not_found
+        end
+      end
+
       describe 'taxonomy scoping' do
         let(:organizations) { (0..1).map { FactoryBot.create(:organization) } }
         let(:tasks) { organizations.map { |o| linked_task(o) } + [FactoryBot.create(:some_task)] }

test/unit/summarizer_test.rb

@@ -12,7 +12,7 @@ class SummarizerTest < ActiveSupport::TestCase
     end
 
     let :subject do
-      ForemanTasks::Task::Summarizer.new
+      ForemanTasks::Task::Summarizer.new(ForemanTasks::Task)
     end
 
     let :expected do