Update to use ensure block for closing PHI access (#73)

* Update to use ensure block for closing PHI access

* Update build to encompass newer versions of ruby and rails

* Restore version check for legacy_connection_handling

* Drop deprecated ruby version support

* Add spec for bug

* Fix method call in new spec

* Update both versions of get_phi, add specs for both

* Remove useless changelog

* Bump version

Author: wkirby

.github/workflows/build.yml

@@ -8,8 +8,7 @@ jobs:
     name: Ruby ${{ matrix.ruby }}
     strategy:
       matrix:
-        # Due to https://github.com/actions/runner/issues/849, we have to use quotes for '3.0'
-        ruby: ['2.7.2', '3.0', 3.1]
+        ruby: [3.1, 3.2, 3.3]
     env:
       RUBY_VERSION: ${{ matrix.ruby }}
     steps:

.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

CHANGELOG.md

@@ -14,7 +14,6 @@ WORKDIR $APP_HOME
 COPY . $APP_HOME/
 
 RUN gem update --system
-RUN bundle config set force_ruby_platform true
 
 EXPOSE 3000
 

Dockerfile

@@ -463,7 +463,7 @@ To release a new version, update the version number in `version.rb`, and then ru
 
 Bug reports and pull requests are welcome on GitHub at https://github.com/apsislabs/phi_attrs.
 
-Any PRs should be accompanied with documentation in `README.md`, and changes documented in [`CHANGELOG.md`](https://keepachangelog.com/).
+Any PRs should be accompanied with documentation in `README.md`.
 
 ## License
 

README.md

@@ -142,32 +142,32 @@ def get_phi(user_id = nil, reason = nil, allow_only: nil)
         # Save this so we don't revoke access previously extended outside the block
         frozen_instances = __instances_with_extended_phi.index_with { |obj| obj.instance_variable_get(:@__phi_relations_extended).clone }
 
-        if allow_only.nil?
-          allow_phi!(user_id, reason)
-        else
-          allow_only.each { |t| t.allow_phi!(user_id, reason) }
-        end
+        begin
+          if allow_only.nil?
+            allow_phi!(user_id, reason)
+          else
+            allow_only.each { |t| t.allow_phi!(user_id, reason) }
+          end
 
-        result = yield if block_given?
+          return yield
+        ensure
+          __instances_with_extended_phi.each do |obj|
+            if frozen_instances.include?(obj)
+              old_extensions = frozen_instances[obj]
+              new_extensions = obj.instance_variable_get(:@__phi_relations_extended) - old_extensions
+              obj.send(:revoke_extended_phi!, new_extensions) if new_extensions.any?
+            else
+              obj.send(:revoke_extended_phi!) # Instance is new to the set, so revoke everything
+            end
+          end
 
-        __instances_with_extended_phi.each do |obj|
-          if frozen_instances.include?(obj)
-            old_extensions = frozen_instances[obj]
-            new_extensions = obj.instance_variable_get(:@__phi_relations_extended) - old_extensions
-            obj.send(:revoke_extended_phi!, new_extensions) if new_extensions.any?
+          if allow_only.nil?
+            disallow_last_phi!
           else
-            obj.send(:revoke_extended_phi!) # Instance is new to the set, so revoke everything
+            allow_only.each { |t| t.disallow_last_phi!(preserve_extensions: true) }
+            # We've handled any newly extended allowances ourselves above
           end
         end
-
-        if allow_only.nil?
-          disallow_last_phi!
-        else
-          allow_only.each { |t| t.disallow_last_phi!(preserve_extensions: true) }
-          # We've handled any newly extended allowances ourselves above
-        end
-
-        result
       end
 
       # Explicitly disallow phi access in a specific area of code. This does not
@@ -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.

lib/phi_attrs/phi_record.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module PhiAttrs
-  VERSION = '0.3.1'
+  VERSION = '0.3.2'
 end

lib/phi_attrs/version.rb

@@ -25,8 +25,6 @@ class Application < Rails::Application
 
     if Rails.version.match?(/^6.0/)
       config.active_record.sqlite3.represent_boolean_as_integer = true
-    else
-      config.active_record.legacy_connection_handling = false
     end
 
     def require_environment!

spec/dummy/application.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

spec/dummy/db/schema.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

spec/phi_attrs/phi_record/class__allow_phi_spec.rb

@@ -124,6 +124,17 @@
       it 'get_phi with block returns value' do |t|
         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', :aggregate_failures do |t|
+        def name_getter(reason, description)
+          patient_jane.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 'collection' do