From 3085447395087b5c7d3834913ed216406070ce1c Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Fri, 11 Oct 2024 16:53:31 -0400 Subject: [PATCH] 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 --- .github/workflows/build.yml | 3 +- .gitignore | 5 +- CHANGELOG.md | 17 ------ Dockerfile | 1 - README.md | 2 +- lib/phi_attrs/phi_record.rb | 60 +++++++++---------- lib/phi_attrs/version.rb | 2 +- spec/dummy/application.rb | 2 - spec/dummy/db/schema.rb | 13 +--- .../phi_record/class__allow_phi_spec.rb | 11 ++++ .../phi_record/instance__allow_phi_spec.rb | 11 ++++ 11 files changed, 59 insertions(+), 68 deletions(-) delete mode 100644 CHANGELOG.md diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 3d0d9e5..cf3624e 100644 --- a/.github/workflows/build.yml +++ b/.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: 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/CHANGELOG.md b/CHANGELOG.md deleted file mode 100644 index 93e9199..0000000 --- a/CHANGELOG.md +++ /dev/null @@ -1,17 +0,0 @@ -# Changelog -All notable changes to this project will be documented in this file. - -The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - -## [Unreleased] -### Added -- Added documentation for CHANGELOG to README - -## [v0.1.4] - 2018-07-05 -## [v0.1.3] - 2018-07-05 -## [v0.1.2] - 2018-07-05 -## [v0.1.1] - 2018-07-05 - -## [v0.1.0] - 2018-07-04 -Initial release 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/README.md b/README.md index 742a615..f79e071 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 43a02c4..c73cd53 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -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. diff --git a/lib/phi_attrs/version.rb b/lib/phi_attrs/version.rb index 475ff63..9e5b731 100644 --- a/lib/phi_attrs/version.rb +++ b/lib/phi_attrs/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module PhiAttrs - VERSION = '0.3.1' + VERSION = '0.3.2' end diff --git a/spec/dummy/application.rb b/spec/dummy/application.rb index 491b815..9e971d6 100644 --- a/spec/dummy/application.rb +++ b/spec/dummy/application.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! 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 900c1d4..47cba25 100644 --- a/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb +++ b/spec/phi_attrs/phi_record/instance__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