From aa61ee4b928108d2f5d53ba7e4cf60915e3ff4b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20R=C5=AF=C5=BEi=C4=8Dka?= Date: Mon, 15 Jul 2019 13:33:59 +0200 Subject: [PATCH] Fixes #27275 - Fix permission bypasses in controllers --- .../foreman_tasks/api/tasks_controller.rb | 6 +-- .../foreman_tasks/tasks_controller.rb | 6 +-- app/models/foreman_tasks/task/summarizer.rb | 9 ++-- test/controllers/api/tasks_controller_test.rb | 50 ++++++++++++++----- test/controllers/tasks_controller_test.rb | 27 ++++++++++ test/unit/summarizer_test.rb | 2 +- 6 files changed, 76 insertions(+), 24 deletions(-) diff --git a/app/controllers/foreman_tasks/api/tasks_controller.rb b/app/controllers/foreman_tasks/api/tasks_controller.rb index 497a8c0f5..84d45c90a 100644 --- a/app/controllers/foreman_tasks/api/tasks_controller.rb +++ b/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 diff --git a/app/controllers/foreman_tasks/tasks_controller.rb b/app/controllers/foreman_tasks/tasks_controller.rb index 3b11da61f..59bd4b7e3 100644 --- a/app/controllers/foreman_tasks/tasks_controller.rb +++ b/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 diff --git a/app/models/foreman_tasks/task/summarizer.rb b/app/models/foreman_tasks/task/summarizer.rb index 34036e059..289647a17 100644 --- a/app/models/foreman_tasks/task/summarizer.rb +++ b/app/models/foreman_tasks/task/summarizer.rb @@ -47,8 +47,9 @@ 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 @@ -56,7 +57,7 @@ def summarize_by_status 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 diff --git a/test/controllers/api/tasks_controller_test.rb b/test/controllers/api/tasks_controller_test.rb index 56ea52ea6..360d482ac 100644 --- a/test/controllers/api/tasks_controller_test.rb +++ b/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 diff --git a/test/controllers/tasks_controller_test.rb b/test/controllers/tasks_controller_test.rb index d8f9f6e7e..717f173c5 100644 --- a/test/controllers/tasks_controller_test.rb +++ b/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)] } diff --git a/test/unit/summarizer_test.rb b/test/unit/summarizer_test.rb index bc4d6b138..b99162f66 100644 --- a/test/unit/summarizer_test.rb +++ b/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