From ea5909abe04e90ff995d0235bccbd9d8c8fa1d80 Mon Sep 17 00:00:00 2001
From: Wyatt Kirby <wyatt@apsis.io>
Date: Fri, 11 Oct 2024 16:42:49 -0400
Subject: [PATCH] Update both versions of get_phi, add specs for both

---
 .gitignore                                    |  5 +++--
 Dockerfile                                    |  1 -
 lib/phi_attrs/phi_record.rb                   | 20 +++++++++----------
 spec/dummy/db/schema.rb                       | 13 +-----------
 .../phi_record/class__allow_phi_spec.rb       | 11 ++++++++++
 .../phi_record/instance__allow_phi_spec.rb    |  2 +-
 6 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/.gitignore b/.gitignore
index 8d444e6..6d1f9dd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,11 +13,12 @@
 
 /spec/dummy/tmp/
 /spec/dummy/db/schema.rb
-*.sqlite
-*.sqlite3
+*.sqlite*
+*.sqlite3*
 *.log
 .rspec_status
 gemfiles/*.gemfile.lock
+gemfiles/.bundle
 
 # Macs. Ugh.
 .DS_Store
diff --git a/Dockerfile b/Dockerfile
index a4efe1d..55376af 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -14,7 +14,6 @@ WORKDIR $APP_HOME
 COPY . $APP_HOME/
 
 RUN gem update --system
-RUN bundle config set force_ruby_platform true
 
 EXPOSE 3000
 
diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb
index 8bcddbb..c73cd53 100644
--- a/lib/phi_attrs/phi_record.rb
+++ b/lib/phi_attrs/phi_record.rb
@@ -149,7 +149,7 @@ def get_phi(user_id = nil, reason = nil, allow_only: nil)
             allow_only.each { |t| t.allow_phi!(user_id, reason) }
           end
 
-          return yield if block_given?
+          return yield
         ensure
           __instances_with_extended_phi.each do |obj|
             if frozen_instances.include?(obj)
@@ -385,15 +385,15 @@ def get_phi(user_id = nil, reason = nil)
       raise ArgumentError, 'block required' unless block_given?
 
       extended_instances = @__phi_relations_extended.clone
-      allow_phi!(user_id, reason)
-
-      result = yield if block_given?
-
-      new_extensions = @__phi_relations_extended - extended_instances
-      disallow_last_phi!(preserve_extensions: true)
-      revoke_extended_phi!(new_extensions) if new_extensions.any?
-
-      result
+      begin
+        allow_phi!(user_id, reason)
+
+        return yield
+      ensure
+        new_extensions = @__phi_relations_extended - extended_instances
+        disallow_last_phi!(preserve_extensions: true)
+        revoke_extended_phi!(new_extensions) if new_extensions.any?
+      end
     end
 
     # Revoke all PHI access for a single instance of this class.
diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb
index 563d7b3..32bfe61 100755
--- a/spec/dummy/db/schema.rb
+++ b/spec/dummy/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.0].define(version: 2017_02_14_100255) do
+ActiveRecord::Schema[7.2].define(version: 2017_02_14_100255) do
   create_table "addresses", force: :cascade do |t|
     t.integer "patient_info_id"
     t.string "address"
@@ -23,16 +23,6 @@
     t.index ["patient_info_id"], name: "index_health_records_on_patient_info_id"
   end
 
-  create_table "missing_attribute_model", force: :cascade  do |t|
-    t.datetime "created_at", precision: nil, null: false
-    t.datetime "updated_at", precision: nil, null: false
-  end
-
-  create_table "missing_extend_model", force: :cascade  do |t|
-    t.datetime "created_at", precision: nil, null: false
-    t.datetime "updated_at", precision: nil, null: false
-  end
-
   create_table "patient_details", force: :cascade do |t|
     t.integer "patient_info_id"
     t.string "detail"
@@ -48,5 +38,4 @@
     t.datetime "created_at", precision: nil, null: false
     t.datetime "updated_at", precision: nil, null: false
   end
-
 end
diff --git a/spec/phi_attrs/phi_record/class__allow_phi_spec.rb b/spec/phi_attrs/phi_record/class__allow_phi_spec.rb
index 255ad13..68cde40 100644
--- a/spec/phi_attrs/phi_record/class__allow_phi_spec.rb
+++ b/spec/phi_attrs/phi_record/class__allow_phi_spec.rb
@@ -111,6 +111,17 @@
     it 'get_phi with block returns value' do |t|
       expect(PatientInfo.get_phi(file_name, t.full_description) { patient_jane.first_name }).to eq('Jane')
     end
+
+    it 'does not leak phi allowance if get_phi returns', :aggregate_failures do |t|
+      def name_getter(reason, description)
+        PatientInfo.get_phi(reason, description) { return patient_jane.first_name }
+      end
+
+      expect(patient_jane.phi_allowed?).to be false
+      first_name = name_getter(file_name, t.full_description)
+      expect(first_name).to eq('Jane')
+      expect(patient_jane.phi_allowed?).to be false
+    end
   end
 
   context 'extended authorization' do
diff --git a/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb b/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb
index 07c0ab0..47cba25 100644
--- a/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb
+++ b/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb
@@ -125,7 +125,7 @@
         expect(patient_jane.get_phi(file_name, t.full_description) { patient_jane.first_name }).to eq('Jane')
       end
 
-      it 'does not leak phi allowance if get_phi returns' do |t|
+      it 'does not leak phi allowance if get_phi returns', :aggregate_failures do |t|
         def name_getter(reason, description)
           patient_jane.get_phi(reason, description) { return patient_jane.first_name }
         end