From 702ae5a4b760cc2b73dd4b485d9c591bb7436491 Mon Sep 17 00:00:00 2001 From: Ray Carrick Date: Tue, 25 Jan 2022 12:07:46 +0000 Subject: [PATCH 1/2] fixes for authorize after security breach Dec 2022 --- app/controllers/contributors_controller.rb | 3 ++- app/controllers/notes_controller.rb | 4 ---- .../org_admin/phase_versions_controller.rb | 2 +- app/controllers/paginable/plans_controller.rb | 8 ++----- app/policies/department_policy.rb | 5 +++++ app/policies/paginable/plan_policy.rb | 21 ------------------- app/policies/phase_policy.rb | 4 ++++ app/policies/plan_policy.rb | 12 +++++++++++ app/policies/section_policy.rb | 4 ++-- 9 files changed, 28 insertions(+), 35 deletions(-) delete mode 100644 app/policies/paginable/plan_policy.rb diff --git a/app/controllers/contributors_controller.rb b/app/controllers/contributors_controller.rb index d5246f9a5b..0482b26980 100644 --- a/app/controllers/contributors_controller.rb +++ b/app/controllers/contributors_controller.rb @@ -30,7 +30,8 @@ def edit # rubocop:disable Metrics/AbcSize # POST /plans/:plan_id/contributors def create - authorize @plan + # to create a contributor you need to be able to edit the plan + authorize @plan, :edit? args = translate_roles(hash: contributor_params) args = process_org(hash: args) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index cfb49d8f7a..38e54c4647 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -13,10 +13,6 @@ def create @note = Note.new # take user id from current user rather than form as form can be spoofed @note.user_id = current_user.id - # ensure user has access to plan BEFORE creating/finding answer - unless Plan.find_by(id: note_params[:plan_id]).readable_by?(@note.user_id) - raise Pundit::NotAuthorizedError - end Answer.transaction do @answer = Answer.find_by( diff --git a/app/controllers/org_admin/phase_versions_controller.rb b/app/controllers/org_admin/phase_versions_controller.rb index c132c6c294..b043313f94 100644 --- a/app/controllers/org_admin/phase_versions_controller.rb +++ b/app/controllers/org_admin/phase_versions_controller.rb @@ -7,7 +7,7 @@ class OrgAdmin::PhaseVersionsController < ApplicationController # POST /org_admin/templates/:template_id/phases/:phase_id/versions def create @phase = Phase.find(params[:phase_id]) - authorize @phase, :create? + authorize @phase @new_phase = get_modifiable(@phase) flash[:notice] = if @new_phase == @phase "This template is already a draft" diff --git a/app/controllers/paginable/plans_controller.rb b/app/controllers/paginable/plans_controller.rb index e29d26bdf9..391e8497d1 100644 --- a/app/controllers/paginable/plans_controller.rb +++ b/app/controllers/paginable/plans_controller.rb @@ -6,9 +6,7 @@ class Paginable::PlansController < ApplicationController # /paginable/plans/privately_visible/:page def privately_visible - unless Paginable::PlanPolicy.new(current_user).privately_visible? - raise Pundit::NotAuthorizedError - end + authorize Plan paginable_renderise( partial: "privately_visible", @@ -20,9 +18,7 @@ def privately_visible # GET /paginable/plans/organisationally_or_publicly_visible/:page def organisationally_or_publicly_visible - unless Paginable::PlanPolicy.new(current_user).organisationally_or_publicly_visible? - raise Pundit::NotAuthorizedError - end + authorize Plan paginable_renderise( partial: "organisationally_or_publicly_visible", diff --git a/app/policies/department_policy.rb b/app/policies/department_policy.rb index 3e865a9f95..2fa6d4304e 100644 --- a/app/policies/department_policy.rb +++ b/app/policies/department_policy.rb @@ -12,6 +12,11 @@ def initialize(user, department) @department = department end + def index? + (@user.can_org_admin? && @user.org.id == @department.org_id) || + @user.can_super_admin? + end + def new? @user.can_org_admin? || @user.can_super_admin? end diff --git a/app/policies/paginable/plan_policy.rb b/app/policies/paginable/plan_policy.rb deleted file mode 100644 index 0008443321..0000000000 --- a/app/policies/paginable/plan_policy.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -module Paginable - - class PlanPolicy < ApplicationPolicy - - def initialize(user) - @user = user - end - - def privately_visible? - @user.is_a?(User) - end - - def organisationally_or_publicly_visible? - @user.is_a?(User) - end - - end - -end diff --git a/app/policies/phase_policy.rb b/app/policies/phase_policy.rb index 5ccb6bf384..fb611c7b10 100644 --- a/app/policies/phase_policy.rb +++ b/app/policies/phase_policy.rb @@ -25,6 +25,10 @@ def preview? user.can_modify_templates? && (phase.template.org_id == user.org_id) end + def edit? + user.can_modify_templates? && (phase.template.org_id == user.org_id) + end + def update? user.can_modify_templates? && (phase.template.org_id == user.org_id) end diff --git a/app/policies/plan_policy.rb b/app/policies/plan_policy.rb index 3ea2637022..c63d0a741c 100644 --- a/app/policies/plan_policy.rb +++ b/app/policies/plan_policy.rb @@ -16,6 +16,10 @@ def initialize(user, plan) @plan = plan end + def index? + @plan.readable_by?(@user.id) + end + def show? @plan.readable_by?(@user.id) end @@ -82,4 +86,12 @@ def update_guidances_list? @plan.editable_by?(@user.id) end + def privately_visible? + @user.is_a?(User) + end + + def organisationally_or_publicly_visible? + @user.is_a?(User) + end + end diff --git a/app/policies/section_policy.rb b/app/policies/section_policy.rb index 58c14a62ac..fde52f60e5 100644 --- a/app/policies/section_policy.rb +++ b/app/policies/section_policy.rb @@ -18,11 +18,11 @@ def initialize(user, section) ## def index? - user.present? + @user.present? end def show? - user.present? + @user.present? end def edit? From d2c46a9e90ed30b5655e8b3182e1fcbc57ee1805 Mon Sep 17 00:00:00 2001 From: Ray Carrick Date: Wed, 26 Jan 2022 10:29:37 +0000 Subject: [PATCH 2/2] Change to testing user signed in Helper user_signed_in? from devise isn't available in policy files. So using @user.present? instead which should be equivalent. --- app/policies/plan_policy.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/policies/plan_policy.rb b/app/policies/plan_policy.rb index c63d0a741c..17bec849b3 100644 --- a/app/policies/plan_policy.rb +++ b/app/policies/plan_policy.rb @@ -17,7 +17,7 @@ def initialize(user, plan) end def index? - @plan.readable_by?(@user.id) + @user.present? end def show? @@ -87,11 +87,11 @@ def update_guidances_list? end def privately_visible? - @user.is_a?(User) + @user.present? end def organisationally_or_publicly_visible? - @user.is_a?(User) + @user.present? end end