Conflicting download plan rules lead to non workable downloads

[nicolasfranck]

Please complete the following fields as applicable:

What version of the DMPRoadmap code are you running? (e.g. v2.2.0)

4.1.0

Expected behaviour:

When a plan download is listed somewhere as downloadable, it should be downloadable

Actual behaviour:

Some plans, on your "dashboard", that are only "organizational or publicly" visible, have a download link. But some of those links lead to this error:

Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: I, [2023-09-26T13:01:52.471769 #16]  INFO -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca] Started GET "/plans/64841/export.pdf?export%5Bquestion_headings%5D=true" for 193.190.130.1 at 2023-09-26 13:01:52 +0000
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: I, [2023-09-26T13:01:52.474673 #16]  INFO -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca] Processing by PlanExportsController#show as PDF
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: I, [2023-09-26T13:01:52.474766 #16]  INFO -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca]   Parameters: {"export"=>{"question_headings"=>"true"}, "plan_id"=>"64841"}
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: E, [2023-09-26T13:01:52.511655 #16] ERROR -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca] You are not authorized to perform this action.
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: I, [2023-09-26T13:01:52.512251 #16]  INFO -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca] Completed 406 Not Acceptable in 37ms (ActiveRecord: 14.5ms | Allocations: 5859)
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: F, [2023-09-26T13:01:52.513138 #16] FATAL -- : [5dd0d329-62d8-4e70-a66d-4fb778576dca]   
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [5dd0d329-62d8-4e70-a66d-4fb778576dca] ActionController::UnknownFormat (ActionController::UnknownFormat):
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [5dd0d329-62d8-4e70-a66d-4fb778576dca]   
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [5dd0d329-62d8-4e70-a66d-4fb778576dca] app/controllers/application_controller.rb:189:in `render_respond_to_format_with_error_message'
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [5dd0d329-62d8-4e70-a66d-4fb778576dca] app/controllers/application_controller.rb:37:in `user_not_authorized'
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: I, [2023-09-26T13:01:52.572407 #16]  INFO -- : [f73e7576-4513-424b-9c2b-f3b5efecd6f9] Started GET "/favicon.ico" for 193.190.130.1 at 2023-09-26 13:01:52 +0000
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: F, [2023-09-26T13:01:52.574447 #16] FATAL -- : [f73e7576-4513-424b-9c2b-f3b5efecd6f9]   
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [f73e7576-4513-424b-9c2b-f3b5efecd6f9] ActionController::RoutingError (No route matches [GET] "/favicon.ico"):
Sep 26 15:01:52 dmponline 9bc6d88a2440[1233]: [f73e7576-4513-424b-9c2b-f3b5efecd6f9]   

This happened to a plan with the following characteristics (example):

• plan has as org_id value equal to current_user.org_id
• none of the plan.roles have users equal to the logged in user's organization. So that plan is only included in organizationally list because of that org_id in the plan record, not because it is affiliated with that organization.

Steps to reproduce:

• Make sure you are an administrator
• Make sure that you create a plan with org_id equal to X, and attach users that have as organization Y
• Make that plan organizationally visible
• Login with user of organization X
• You'll see the plan in "organizational" visible plans
• Click on the download next to it.

Notes and thoughts

• Normally the creator of the plan has the same organization as the organization of the plan. But according to this line, the organization of the plan itself can also be set when the primary research organization is set. Why should one do this? The primary research organization is only important for template selection, right? That is the only reason why this may happen, not?
• the plan export controller returns true for privately_visible? but not for export_params[:form].present? (See here). So that line is skipped, and then it checks if it publicly visible, which fails of course. Without that export_params[:form].present? it "works". As I have seen, that export[form]=true is there to differentiate between requests coming from the plan download page (where you can provide settings), and those coming from outside (publicly) where you cannot/should not provide settings. May the logic for allowance and formatting should not be put on line?

# authorization
if privately_authorized? || publicly_authorized?
  skip_authorization
else
  raise Pundit::NotAuthorizedError
end

# format settings
if privately_authorized? && export_params[:form].present?
   # provide settings from form
else
  # set default settings
end  

• When the error is thrown, it is handled by the main controller ("render_respond_to_format_with_error_message"), but that main controller clearly cannot handle the provided format (PDF). Why should that handle even try to respect the format?

[aaronskiba]

    # app/controllers/plan_exports_controller.rb
    if privately_authorized? && export_params[:form].present?
      skip_authorization
      @show_coversheet         = export_params[:project_details].present?
      @show_sections_questions = export_params[:question_headings].present?
      @show_unanswered         = export_params[:unanswered_questions].present?
      @show_custom_sections    = export_params[:custom_sections].present?
      @show_research_outputs   = export_params[:research_outputs].present?
      @public_plan             = false

    elsif publicly_authorized?
      skip_authorization
      @show_coversheet         = true
      @show_sections_questions = true
      @show_unanswered         = true
      @show_custom_sections    = true
      @show_research_outputs   = @plan.research_outputs&.any? || false
      @public_plan             = true

##################################################################

  def publicly_authorized?
    PublicPagePolicy.new(current_user, @plan).plan_organisationally_exportable? ||
      PublicPagePolicy.new(current_user, @plan).plan_export?
  end

  # app/policies/public_page_policy.rb
  def plan_export?
    @record.publicly_visible?
  end

  def plan_organisationally_exportable?
    if @record.is_a?(Plan) && @user.is_a?(User)
      return @record.publicly_visible? ||
             (@record.organisationally_visible? && @record.owner.present? &&
              @record.owner.org_id == @user.org_id)
    end

As @nicolasfranck pointed out, in the "Organization Plans section" of the /plans page, plans where plan.owner.org_id != current_user.org_id are listed. But for the elsif publicly_authorized? check to be true, @record.owner.org_id == @user.org_id must also be true. So we have a mismatch here. Either certain plans should not be listed in the first place, or else def plan_organisationally_exportable? must be updated to enable download of these organizational plans.

I'm also wondering about the following question posed by @nicolasfranck: "Normally the creator of the plan has the same organization as the organization of the plan. But according to this line, the organization of the plan itself can also be set when the primary research organization is set. Why should one do this? The primary research organization is only important for template selection, right? That is the only reason why this may happen, not?"