Disallow ops users from viewing PII for restricted patients

Author: samcoy3

app/components/app_timeline_component.rb

@@ -63,6 +63,8 @@ def format_description(item)
       "#{id_info}#{formatted_details}"
     elsif item[:description].present?
       item[:description]
+    else
+      ""
     end
   end
 

app/controllers/inspect/graphs_controller.rb

@@ -14,104 +14,137 @@ class GraphsController < ApplicationController
 
     def show
       authorize :inspect, :graph?
-      @primary_type = safe_get_primary_type
-      if @primary_type.nil?
+      if primary_type.nil?
         render plain:
                  "You don't have permission to view object type: #{params[:object_type].to_s.downcase.singularize}",
                status: :bad_request and return
       end
-      @primary_id = params[:object_id]
 
       # Set default relationships when loading a page for the first time
       if params[:relationships].blank? &&
-           GraphRecords::DEFAULT_TRAVERSALS.key?(@primary_type)
-        default_rels = GraphRecords::DEFAULT_TRAVERSALS[@primary_type] || {}
+           GraphRecords::DEFAULT_TRAVERSALS.key?(primary_type)
+        default_rels = GraphRecords::DEFAULT_TRAVERSALS[primary_type] || {}
 
         new_params = params.to_unsafe_h.merge("relationships" => default_rels)
         redirect_to inspect_path(new_params) and return
       end
 
-      @object = @primary_type.to_s.classify.constantize.find(@primary_id)
-
-      @traversals_config = build_traversals_config
-      @graph_params = build_graph_params
-      set_pii_settings
-
       @graph_record =
         GraphRecords.new(
-          traversals_config: @traversals_config,
-          primary_type: @primary_type,
+          traversals_config:,
+          primary_type:,
           clickable: true,
-          show_pii: @show_pii
+          show_pii:
         )
-      @mermaid = @graph_record.graph(**@graph_params).join("\n")
+      @mermaid = @graph_record.graph(**graph_params).join("\n")
     end
 
     private
 
-    def set_pii_settings
-      @user_is_allowed_to_access_pii = policy(:inspect).show_pii?
-      @show_pii =
-        @user_is_allowed_to_access_pii &&
-          (params[:show_pii] || SHOW_PII_BY_DEFAULT)
+    def show_pii
+      @show_pii ||=
+        user_is_allowed_to_access_pii && !sensitive_patient_in_graph &&
+          pii_access_requested_by_user
     end
 
-    def build_traversals_config
-      traversals_config = {}
-      to_process = Set.new([@primary_type])
-      processed = Set.new
+    def user_is_allowed_to_access_pii
+      @user_is_allowed_to_access_pii ||= policy(:inspect).show_pii?
+    end
 
-      # Process types until we've explored all connected relationships
-      while (type = to_process.first)
-        to_process.delete(type)
-        processed.add(type)
+    def sensitive_patient_in_graph
+      @sensitive_patient_in_graph ||=
+        begin
+          graph_with_pii =
+            GraphRecords.new(
+              traversals_config:,
+              primary_type:,
+              clickable: true,
+              show_pii: true
+            )
+          graph_with_pii.graph(**graph_params)
+          graph_with_pii.patients_with_pii_in_graph.any?(&:restricted?)
+        end
+    end
 
-        selected_rels =
-          Array(params.dig(:relationships, type)).reject(&:blank?).map(&:to_sym)
+    def pii_access_requested_by_user
+      @pii_access_requested_by_user ||= params[:show_pii] || SHOW_PII_BY_DEFAULT
+    end
 
-        traversals_config[type] = selected_rels
+    def traversals_config
+      @traversals_config ||=
+        begin
+          traversals = {}
+          to_process = Set.new([primary_type])
+          processed = Set.new
+
+          # Process types until we've explored all connected relationships
+          while (type = to_process.first)
+            to_process.delete(type)
+            processed.add(type)
+
+            selected_rels =
+              Array(params.dig(:relationships, type)).reject(&:blank?).map(
+                &:to_sym
+              )
 
-        # Add target types to process queue
-        klass = type.to_s.classify.constantize
-        selected_rels.each do |rel|
-          association = klass.reflect_on_association(rel)
-          next unless association
+            traversals[type] = selected_rels
 
-          target_type = association.klass.name.underscore.to_sym
-          to_process.add(target_type) unless processed.include?(target_type)
-        end
-      end
+            # Add target types to process queue
+            klass = type.to_s.classify.constantize
+            selected_rels.each do |rel|
+              association = klass.reflect_on_association(rel)
+              next unless association
 
-      traversals_config
+              target_type = association.klass.name.underscore.to_sym
+              to_process.add(target_type) unless processed.include?(target_type)
+            end
+          end
+
+          traversals
+        end
     end
 
-    def build_graph_params
-      graph_params = { @primary_type => [@object.id] }
-
-      if params[:additional_ids].present?
-        params[:additional_ids].each do |type, ids_string|
-          next if ids_string.blank?
-          additional_ids = ids_string.split(",").map { |s| s.strip.to_i }
-          next unless additional_ids.any?
-          type_sym = type.to_sym
-          graph_params[type_sym] ||= []
-          graph_params[type_sym].concat(additional_ids)
+    def graph_params
+      @graph_params ||=
+        begin
+          graph_params = { primary_type => [primary_object.id] }
+
+          if params[:additional_ids].present?
+            params[:additional_ids].each do |type, ids_string|
+              next if ids_string.blank?
+              additional_ids = ids_string.split(",").map { |s| s.strip.to_i }
+              next unless additional_ids.any?
+              type_sym = type.to_sym
+              graph_params[type_sym] ||= []
+              graph_params[type_sym].concat(additional_ids)
+            end
+          end
+
+          graph_params
         end
-      end
+    end
 
-      graph_params
+    def primary_type
+      @primary_type ||=
+        begin
+          singular_type = params[:object_type].downcase.singularize
+          return nil unless GraphRecords::ALLOWED_TYPES.include?(singular_type)
+          singular_type.to_sym
+        end
     end
 
-    def safe_get_primary_type
-      singular_type = params[:object_type].downcase.singularize
-      return nil unless GraphRecords::ALLOWED_TYPES.include?(singular_type)
-      singular_type.to_sym
+    def primary_object
+      @primary_object ||=
+        begin
+          @primary_id = params[:object_id]
+          primary_type.to_s.classify.constantize.find(@primary_id)
+        end
     end
 
     def pii_accessed?
-      return false unless @show_pii
+      return false unless show_pii
 
-      build_traversals_config.any? do |from_type, rels|
+      traversals_config.any? do |from_type, rels|
         GraphRecords::EXTRA_DETAIL_WHITELIST_WITH_PII.key?(
           from_type.name.underscore.to_sym
         ) ||
@@ -142,7 +175,7 @@ def record_access_log_entry
             controller: "graph",
             action: "show_pii",
             request_details: {
-              primary_type: @primary_type,
+              primary_type:,
               primary_id: @primary_id,
               additional_ids: additional_ids.presence,
               visible_fields: request_details

app/controllers/inspect/timeline/patients_controller.rb

@@ -13,12 +13,9 @@ class Inspect::Timeline::PatientsController < ApplicationController
 
   def show
     authorize :inspect, :timeline?
-    set_pii_settings
 
     params[:audit_config] ||= {}
 
-    compare_option = params[:compare_option] || nil
-
     # Set default values if none present
     if params[:detail_config].nil? && params[:event_names].nil? &&
          params[:show_pii].nil?
@@ -40,17 +37,16 @@ def show
         params[:audit_config][:include_filtered_audit_changes]
     }
 
-    @compare_patient = sample_patient(params[:compare_option]) if compare_option
     @event_names = params[:event_names] || []
 
     record_access_log_entry
 
     @patient_timeline =
       TimelineRecords.new(
         @patient,
-        detail_config: build_details_config,
-        audit_config: audit_config,
-        show_pii: @show_pii
+        details_config:,
+        audit_config:,
+        show_pii:
       ).load_timeline_events(@event_names)
 
     @no_events_message = true if @patient_timeline.empty?
@@ -61,9 +57,9 @@ def show
       @compare_patient_timeline =
         TimelineRecords.new(
           @compare_patient,
-          detail_config: build_details_config,
-          audit_config: audit_config,
-          show_pii: @show_pii
+          details_config:,
+          audit_config:,
+          show_pii:
         ).load_timeline_events(@event_names)
 
       @no_events_compare_message = true if @compare_patient_timeline.empty?
@@ -72,18 +68,33 @@ def show
 
   private
 
-  def set_pii_settings
-    @user_is_allowed_to_access_pii = policy(:inspect).show_pii?
-    @show_pii =
-      @user_is_allowed_to_access_pii &&
-        (params[:show_pii] || SHOW_PII_BY_DEFAULT)
+  def show_pii
+    @show_pii ||=
+      user_is_allowed_to_access_pii && !patient_accessed_is_sensitive &&
+        pii_access_requested_by_user
+  end
+
+  def user_is_allowed_to_access_pii
+    @user_is_allowed_to_access_pii ||= policy(:inspect).show_pii?
+  end
+
+  def patient_accessed_is_sensitive
+    @patient_accessed_is_sensitive ||=
+      @patient.restricted? || @compare_patient&.restricted?
+  end
+
+  def pii_access_requested_by_user
+    @pii_access_requested_by_user ||= params[:show_pii] || SHOW_PII_BY_DEFAULT
   end
 
   def set_patient
     @patient = Patient.find(params[:id])
     timeline = TimelineRecords.new(@patient)
     @patient_events = timeline.patient_events(@patient)
     @additional_events = timeline.additional_events(@patient)
+
+    compare_option = params[:compare_option] || nil
+    @compare_patient = sample_patient(params[:compare_option]) if compare_option
   end
 
   # TODO: Fix so that a new comparison patient isn't sampled every time
@@ -118,54 +129,57 @@ def sample_patient(compare_option)
     end
   end
 
-  def build_details_config
-    details_params = params[:detail_config] || {}
-    details_params = details_params.to_unsafe_h unless details_params.is_a?(
-      Hash
-    )
-
-    event_list_details =
-      (@event_names - ["audits"]).map { [it.to_sym, []] }.to_h
-    user_submitted_details =
-      details_params.each_with_object(
-        event_list_details
-      ) do |(event_type, fields), hash|
-        selected_fields = Array(fields).reject(&:blank?).map(&:to_sym)
-        hash[event_type.to_sym] = selected_fields
-      end
+  def details_config
+    @details_config ||=
+      begin
+        details_params = params[:detail_config] || {}
+        details_params = details_params.to_unsafe_h unless details_params.is_a?(
+          Hash
+        )
 
-    details_mask =
-      (
-        if @show_pii
-          TimelineRecords::AVAILABLE_DETAILS_CONFIG_WITH_PII
-        else
-          TimelineRecords::AVAILABLE_DETAILS_CONFIG
+        event_list_details =
+          (@event_names - ["audits"]).map { [it.to_sym, []] }.to_h
+        user_submitted_details =
+          details_params.each_with_object(
+            event_list_details
+          ) do |(event_type, fields), hash|
+            selected_fields = Array(fields).reject(&:blank?).map(&:to_sym)
+            hash[event_type.to_sym] = selected_fields
+          end
+
+        # Removes details from the config which the user does not have permission to view
+        details_mask =
+          (
+            if show_pii
+              TimelineRecords::AVAILABLE_DETAILS_CONFIG_WITH_PII
+            else
+              TimelineRecords::AVAILABLE_DETAILS_CONFIG
+            end
+          )
+        (details_mask.keys & user_submitted_details.keys).index_with do |key|
+          details_mask[key] & user_submitted_details[key]
         end
-      )
-    (details_mask.keys & user_submitted_details.keys).index_with do |key|
-      details_mask[key] & user_submitted_details[key]
-    end
+      end
   end
 
   def pii_accessed?
-    return false unless @show_pii
-    detail_config = build_details_config
-    detail_config.any? do |event_type, selected_fields|
+    return false unless show_pii
+    details_config.any? do |event_type, selected_fields|
       pii_fields =
         TimelineRecords::AVAILABLE_DETAILS_CONFIG_PII[event_type] || []
       (selected_fields & pii_fields).any?
     end
   end
 
   def audit_pii_accessed?
-    true if @show_pii && @event_names.include?("audits")
+    true if show_pii && @event_names.include?("audits")
   end
 
   def record_access_log_entry
     return unless pii_accessed? || audit_pii_accessed?
 
     details_accessed =
-      build_details_config.reverse_merge(
+      details_config.reverse_merge(
         @event_names.map { |key| [key.to_sym, []] }.to_h
       )
     details_accessed[:audits] = :accessed if details_accessed.key?(:audits)