From ca9a17ca23c4fbc34845fb5bcd84c6cfe90a9054 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:16:33 -0400 Subject: [PATCH 01/11] update gem tooling and support matrix --- .github/workflows/build.yml | 28 ++-- .github/workflows/publish.yml | 76 ++++++--- .rubocop.yml | 97 ------------ .standard.yml | 14 ++ Appraisals | 31 +++- Gemfile | 2 +- README.md | 49 ++++-- Rakefile | 23 ++- bin/release | 84 ++++++++++ bin/rubo_fix | 4 +- bin/run_tests | 16 +- config.ru | 4 +- gemfiles/rails_7.0.gemfile | 6 +- gemfiles/rails_7.1.gemfile | 10 ++ gemfiles/rails_7.2.gemfile | 10 ++ gemfiles/rails_8.0.gemfile | 10 ++ lib/phi_attrs.rb | 22 +-- lib/phi_attrs/configure.rb | 2 +- lib/phi_attrs/exceptions.rb | 2 +- lib/phi_attrs/logger.rb | 2 +- lib/phi_attrs/phi_record.rb | 92 +++++------ lib/phi_attrs/railtie.rb | 6 +- lib/phi_attrs/rspec.rb | 6 +- lib/phi_attrs/version.rb | 2 +- phi_attrs.gemspec | 68 ++++---- spec/dummy/app/models/patient_info.rb | 6 +- spec/dummy/application.rb | 18 +-- spec/dummy/config/initializers/phi_attrs.rb | 2 +- spec/dummy/factories/addresses.rb | 2 +- spec/dummy/factories/patient_details.rb | 2 +- spec/dummy/factories/patient_infos.rb | 4 +- spec/phi_attrs/configure_spec.rb | 32 ++-- .../controllers/current_user_spec.rb | 14 +- .../i18n_nested_controller_spec.rb | 22 +-- .../i18n_sample_controller_spec.rb | 22 +-- spec/phi_attrs/delegations_spec.rb | 22 +-- spec/phi_attrs/exceptions_spec.rb | 14 +- spec/phi_attrs/logger_spec.rb | 96 ++++++------ .../phi_record/class__allow_phi_spec.rb | 88 +++++------ .../phi_record/class__disallow_phi_spec.rb | 28 ++-- .../phi_record/class__phi_allowed_spec.rb | 30 ++-- .../phi_record/instance__allow_phi_spec.rb | 148 +++++++++--------- .../phi_record/instance__disallow_phi_spec.rb | 28 ++-- .../phi_record/instance__phi_allowed_spec.rb | 102 ++++++------ .../phi_attrs/phi_record/phi_wrapping_spec.rb | 10 +- spec/phi_attrs/version_spec.rb | 6 +- spec/phi_attrs_spec.rb | 2 +- spec/spec_helper.rb | 20 +-- 48 files changed, 757 insertions(+), 627 deletions(-) delete mode 100644 .rubocop.yml create mode 100644 .standard.yml create mode 100755 bin/release create mode 100644 gemfiles/rails_7.1.gemfile create mode 100644 gemfiles/rails_7.2.gemfile create mode 100644 gemfiles/rails_8.0.gemfile diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b0816ca..620da5d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,26 +1,30 @@ -name: Spec CI +name: Test & Lint -on: [push, pull_request] +on: + push: + branches: [main] + pull_request: jobs: build: runs-on: ubuntu-latest - name: Ruby ${{ matrix.ruby }} + name: Ruby ${{ matrix.ruby }} / ${{ matrix.appraisal }} strategy: + fail-fast: false matrix: - ruby: [3.2, 3.3, 3.4] + ruby: ["3.2", "3.3", "3.4"] + appraisal: [rails_7.0, rails_7.1, rails_7.2, rails_8.0] env: - RUBY_VERSION: ${{ matrix.ruby }} + BUNDLE_GEMFILE: gemfiles/${{ matrix.appraisal }}.gemfile steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v4 - name: Set up Ruby ${{ matrix.ruby }} uses: ruby/setup-ruby@v1 with: ruby-version: ${{ matrix.ruby }} bundler-cache: true - - name: Install dependencies - run: | - bundle exec appraisal install - - name: Run rspec - run: | - bundle exec appraisal "rake dummy:db:create dummy:db:migrate && rspec" + - name: Lint + if: matrix.ruby == '3.4' && matrix.appraisal == 'rails_8.0' + run: bundle exec standardrb + - name: Run tests + run: bundle exec rake spec diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 7cc2b21..56ae745 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -1,36 +1,74 @@ name: Publish Gem on: - release: - types: [published] + push: + tags: + - "v*" + +permissions: + contents: write jobs: - build: + publish: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v6 - - uses: ruby/setup-ruby@v1 + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 with: - ruby-version: '3.2' + ruby-version: "3.4" bundler-cache: true - - name: Release Gem - if: contains(github.ref, 'refs/tags/v') + + - name: Build gem + run: | + set -euo pipefail + gemspec="$(ls *.gemspec | head -n 1)" + gem_name="${gemspec%.gemspec}" + version="${GITHUB_REF_NAME#v}" + gem_file="pkg/${gem_name}-${version}.gem" + + echo "Building ${gem_name} ${version}..." + mkdir -p pkg + bundle exec gem build "$gemspec" --output "$gem_file" + test -f "$gem_file" + + - name: Publish to RubyGems env: - RUBYGEMS_API_KEY: ${{secrets.RUBYGEMS_API_KEY}} - TAG: ${{ github.event.release.tag_name }} + RUBYGEMS_API_KEY: ${{ secrets.RUBYGEMS_API_KEY }} run: | - echo "Setting up gem credentials..." - mkdir -p ~/.gem + set -euo pipefail - cat << EOF > ~/.gem/credentials - --- - :rubygems_api_key: ${RUBYGEMS_API_KEY} - EOF + if [[ -z "${RUBYGEMS_API_KEY:-}" ]]; then + echo "::error::Missing RUBYGEMS_API_KEY secret." + exit 1 + fi + gem_file="$(ls pkg/*.gem | head -n 1)" + echo "Publishing ${gem_file}..." + + mkdir -p ~/.gem + printf -- "---\n:rubygems_api_key: %s\n" "$RUBYGEMS_API_KEY" > ~/.gem/credentials chmod 0600 ~/.gem/credentials - bundle exec rake build + gem push "$gem_file" + + - name: Create GitHub release + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + + prerelease_flag=() + if [[ "$GITHUB_REF_NAME" == *-* ]]; then + prerelease_flag=(--prerelease) + fi - echo "Running gem release task..." - gem push pkg/phi_attrs-${TAG#v}.gem + echo "Creating GitHub release ${GITHUB_REF_NAME}..." + gh release create "$GITHUB_REF_NAME" pkg/*.gem \ + --verify-tag \ + --title "$GITHUB_REF_NAME" \ + --notes "Published to RubyGems." \ + "${prerelease_flag[@]}" diff --git a/.rubocop.yml b/.rubocop.yml deleted file mode 100644 index 19fd7ee..0000000 --- a/.rubocop.yml +++ /dev/null @@ -1,97 +0,0 @@ -require: rubocop-rails - -Rails: - Enabled: true - -AllCops: - Exclude: - - 'bin/**/*' - - 'gemfiles/**/*' - - 'spec/dummy/db/schema.rb' - NewCops: enable - TargetRubyVersion: 2.7 - -Gemspec/RequireMFA: - Enabled: false - -Layout/IndentationWidth: - Enabled: true - -Layout/LineLength: - Exclude: - - 'spec/**/*' - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - Max: 140 - -Lint/UnreachableCode: - Exclude: - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - -Lint/UnusedMethodArgument: - Exclude: - - 'lib/phi_attrs.rb' # TODO: RUBOCOP Cleanup exclusion - -Metrics/AbcSize: - Max: 30 - Exclude: - - 'spec/internal/db/**/*' - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - -Metrics/BlockLength: - Enabled: false - -Metrics/ClassLength: - Max: 1500 - -Metrics/CyclomaticComplexity: - Exclude: - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - -Metrics/MethodLength: - Exclude: - - 'spec/**/*' - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - Max: 20 - -Metrics/ModuleLength: - Exclude: - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - -Metrics/PerceivedComplexity: - Exclude: - - 'lib/phi_attrs/phi_record.rb' # TODO: RUBOCOP Cleanup exclusion - -Naming/PredicateName: - Enabled: false - -Rails/DynamicFindBy: - Exclude: - - 'spec/spec_helper.rb' # TODO: RUBOCOP Cleanup exclusion - -# Style/BracesAroundHashParameters: -# Enabled: false - -Style/ClassVars: - Enabled: false - -Style/CommentedKeyword: - Exclude: - - 'spec/**/*' - -Style/ConditionalAssignment: - Enabled: false - -Style/Documentation: - Enabled: false - -Style/EmptyMethod: - EnforcedStyle: expanded - -Style/SymbolArray: - EnforcedStyle: brackets - -Style/RedundantReturn: - Enabled: false - -Style/WordArray: - MinSize: 4 diff --git a/.standard.yml b/.standard.yml new file mode 100644 index 0000000..8dd42e0 --- /dev/null +++ b/.standard.yml @@ -0,0 +1,14 @@ +# Standard Ruby configuration +# https://github.com/testdouble/standard + +ruby_version: 3.2 + +ignore: + - "bin/**/*" + - "gemfiles/**/*" + - "spec/dummy/db/schema.rb" + - "vendor/**/*" + - "tmp/**/*" + - "coverage/**/*" + +fix: true diff --git a/Appraisals b/Appraisals index 000b6b7..a80f727 100644 --- a/Appraisals +++ b/Appraisals @@ -1,8 +1,29 @@ # frozen_string_literal: true -appraise 'rails_7.0' do - gem 'rails', '~> 7.0' - gem 'rspec', '~> 3.12' - gem 'rspec-rails', '~> 7.0' - gem 'sqlite3', '~> 1.5' +appraise "rails_7.0" do + gem "rails", "~> 7.0.0" + gem "rspec", "~> 3.13" + gem "rspec-rails", "~> 7.0" + gem "sqlite3", "~> 1.7" +end + +appraise "rails_7.1" do + gem "rails", "~> 7.1.0" + gem "rspec", "~> 3.13" + gem "rspec-rails", "~> 7.0" + gem "sqlite3", "~> 1.7" +end + +appraise "rails_7.2" do + gem "rails", "~> 7.2.0" + gem "rspec", "~> 3.13" + gem "rspec-rails", "~> 7.0" + gem "sqlite3", "~> 2.0" +end + +appraise "rails_8.0" do + gem "rails", "~> 8.0.0" + gem "rspec", "~> 3.13" + gem "rspec-rails", "~> 8.0" + gem "sqlite3", "~> 2.0" end diff --git a/Gemfile b/Gemfile index 8af9bd8..4b4e526 100644 --- a/Gemfile +++ b/Gemfile @@ -1,6 +1,6 @@ # frozen_string_literal: true -source 'https://rubygems.org' +source "https://rubygems.org" git_source(:github) { |repo_name| "https://github.com/#{repo_name}" } diff --git a/README.md b/README.md index b0d7b60..47ad70f 100644 --- a/README.md +++ b/README.md @@ -430,15 +430,20 @@ It is recommended to use the provided `docker-compose` environment for developme ### Tests -Tests are written using [RSpec](https://rspec.info/) and are setup to use [Appraisal](https://github.com/thoughtbot/appraisal) to run tests over multiple rails versions. +Tests are written using [RSpec](https://rspec.info/) and are setup to use [Appraisal](https://github.com/thoughtbot/appraisal) to run tests over multiple Rails versions. Supported runtimes are Ruby 3.2+ and Rails 7.0 through 8.0. $ bin/run_tests or for individual tests: $ bin/ssh_to_container $ bundle exec appraisal rspec spec/path/to/spec.rb -To run just a particular rails version: - $ bundle exec appraisal rails_7.0 rspec +To run just a particular Rails version: + + $ bundle exec appraisal rails_7.0 rspec + +Linting uses [standardrb](https://github.com/standardrb/standard): + + $ bundle exec standardrb ### Console @@ -449,13 +454,38 @@ An interactive prompt that will allow you to experiment with the gem. ### Local Install -Run `bin/setup` to install dependencies. Then, run `bundle exec appraisal rspec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. +Run `bin/setup` to install dependencies. Then, run `bundle exec rake` to run linting and tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. To install this gem onto your local machine, run `bundle exec rake install`. -### Versioning +### Releases + +Releases are driven by git tags. The version lives in `lib/phi_attrs/version.rb`, and the gemspec reads `PhiAttrs::VERSION`. + +```bash +bundle install +bin/release patch # or: minor, major +``` + +To publish an RC for a specific version: + +```bash +bin/release rc 1.2.0 +``` + +This sets the version to `1.2.0-rc` and tags it as `v1.2.0-rc`. + +Prereleases can also use `bump`'s native `pre` increment: + +```bash +bin/release pre +``` + +`pre` cycles prerelease labels in order: `alpha`, `beta`, `rc`, then the final version. For example, `1.2.0-rc` becomes `1.2.0`, tagged as `v1.2.0`. + +`bin/release` uses `bump`, commits the version file, creates a `v` tag, pushes the branch, and pushes the tag. -To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org). +GitHub Actions publishes only when a `v*` tag is pushed. The publish workflow builds the gem, pushes it to RubyGems with `RUBYGEMS_API_KEY`, and creates a GitHub release with the built gem attached. Tags containing a prerelease suffix, such as `v1.2.0-rc`, are marked as prereleases on GitHub. ## Contributing @@ -466,10 +496,9 @@ Any PRs should be accompanied with documentation in `README.md`. ### Releasing -* Squash and merge your PR, including a bump to `lib/phi_attrs/version.rb` -* Draft a new release, creating a new tag with the new version number from `version.rb`, i.e. `v0.3.2` -* Auto-generate release notes, add any context if necessary -* Publish release; release will be automatically built and published to rubygems +* Squash and merge your PR. +* Run `bin/release patch`, `bin/release minor`, `bin/release major`, or `bin/release rc X.Y.Z`. +* The pushed `v*` tag will be automatically built and published to RubyGems. ## License diff --git a/Rakefile b/Rakefile index 61b71a6..9d48bf4 100644 --- a/Rakefile +++ b/Rakefile @@ -1,12 +1,23 @@ # frozen_string_literal: true -require 'rubygems' -require 'bundler/setup' -require 'bundler/gem_tasks' - -require 'rake' +require "rubygems" +require "bundler/setup" +require "bundler/gem_tasks" +require "rspec/core/rake_task" +require "rake" namespace :dummy do - require_relative 'spec/dummy/application' + require_relative "spec/dummy/application" Dummy::Application.load_tasks end + +RSpec::Core::RakeTask.new(:spec) +Rake::Task[:spec].enhance(["dummy:db:create", "dummy:db:migrate"]) + +begin + require "standard/rake" +rescue LoadError + # Standard not available +end + +task default: [:standard, :spec] diff --git a/bin/release b/bin/release new file mode 100755 index 0000000..75c3a51 --- /dev/null +++ b/bin/release @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +set -euo pipefail + +usage() { + echo "Usage: bin/release patch|minor|major|pre" + echo " bin/release rc X.Y.Z" +} + +level="${1:-}" +target_version="${2:-}" +case "$level" in + patch|minor|major|pre) + if [[ $# -ne 1 ]]; then + usage + exit 1 + fi + ;; + rc) + if [[ $# -ne 2 || ! "$target_version" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + usage + exit 1 + fi + ;; + *) + usage + exit 1 + ;; +esac + +if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then + echo "Error: bin/release must be run inside a git repository." + exit 1 +fi + +branch="$(git symbolic-ref --quiet --short HEAD)" || { + echo "Error: releases must be run from a branch, not a detached HEAD." + exit 1 +} + +version_file="$(bundle exec bump file | awk '{print $NF}')" +current_version="$(bundle exec bump current --value-only)" +if [[ "$level" == "rc" ]]; then + next_version="${target_version}-rc" +else + next_version="$(bundle exec bump show-next "$level" --value-only)" +fi +tag="v${next_version}" + +if [[ -z "$version_file" || -z "$current_version" || -z "$next_version" ]]; then + echo "Error: could not determine gem version information." + exit 1 +fi + +if ! git diff --quiet -- "$version_file" || ! git diff --cached --quiet -- "$version_file"; then + echo "Error: $version_file has uncommitted changes." + exit 1 +fi + +if git rev-parse "$tag" >/dev/null 2>&1; then + echo "Error: tag $tag already exists." + exit 1 +fi + +echo "Releasing ${tag} (${level})" +echo "Version: ${current_version} -> ${next_version}" +echo "Branch: ${branch}" + +if [[ "$level" == "rc" ]]; then + bundle exec bump set "$next_version" --no-commit --no-bundle +else + bundle exec bump "$level" --no-commit --no-bundle +fi + +git add "$version_file" +git commit -m "$tag" +git tag "$tag" + +echo "Pushing ${branch}..." +git push origin "$branch" + +echo "Pushing ${tag}..." +git push origin "$tag" + +echo "Released ${tag}." diff --git a/bin/rubo_fix b/bin/rubo_fix index 5483a24..3684dfd 100755 --- a/bin/rubo_fix +++ b/bin/rubo_fix @@ -2,5 +2,5 @@ source bin/helpers/docker runOnDocker ruby3 -echo "== Starting rubocop ==" -bundle exec rubocop --format worst --format simple --format offenses --autocorrect +echo "== Starting standardrb auto-fix ==" +bundle exec standardrb --fix diff --git a/bin/run_tests b/bin/run_tests index ebbbaa7..fdee4bf 100755 --- a/bin/run_tests +++ b/bin/run_tests @@ -2,18 +2,10 @@ source bin/helpers/docker runOnDocker ruby3 +set -e + echo "== Starting unit tests ==" bundle exec appraisal rspec -if [ $? -ne 0 ]; then - echo -e "\n== RSpec failed; push aborted! ==\n" - exit 1 -fi -echo "== Starting rubocop ==" -bundle exec rubocop --format worst --format simple --format offenses -if [ $? -ne 0 ]; then - echo -e "\n== Rubocop failed; push aborted! ==\n" - echo -e "To auto-correct errors run:" - echo -e "\tbin/rubo_fix" - exit 1 -fi +echo "== Starting standardrb ==" +bundle exec standardrb diff --git a/config.ru b/config.ru index af62932..776beb0 100644 --- a/config.ru +++ b/config.ru @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'rubygems' -require 'bundler' +require "rubygems" +require "bundler" Bundler.require :default, :development diff --git a/gemfiles/rails_7.0.gemfile b/gemfiles/rails_7.0.gemfile index b139dcb..1b13e35 100755 --- a/gemfiles/rails_7.0.gemfile +++ b/gemfiles/rails_7.0.gemfile @@ -2,9 +2,9 @@ source "https://rubygems.org" -gem "rails", "~> 7.0" -gem "rspec", "~> 3.12" +gem "rails", "~> 7.0.0" +gem "rspec", "~> 3.13" gem "rspec-rails", "~> 7.0" -gem "sqlite3", "~> 1.5" +gem "sqlite3", "~> 1.7" gemspec path: "../" diff --git a/gemfiles/rails_7.1.gemfile b/gemfiles/rails_7.1.gemfile new file mode 100644 index 0000000..dba031d --- /dev/null +++ b/gemfiles/rails_7.1.gemfile @@ -0,0 +1,10 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "rails", "~> 7.1.0" +gem "rspec", "~> 3.13" +gem "rspec-rails", "~> 7.0" +gem "sqlite3", "~> 1.7" + +gemspec path: "../" diff --git a/gemfiles/rails_7.2.gemfile b/gemfiles/rails_7.2.gemfile new file mode 100644 index 0000000..50d0bd5 --- /dev/null +++ b/gemfiles/rails_7.2.gemfile @@ -0,0 +1,10 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "rails", "~> 7.2.0" +gem "rspec", "~> 3.13" +gem "rspec-rails", "~> 7.0" +gem "sqlite3", "~> 2.0" + +gemspec path: "../" diff --git a/gemfiles/rails_8.0.gemfile b/gemfiles/rails_8.0.gemfile new file mode 100644 index 0000000..dfc89a9 --- /dev/null +++ b/gemfiles/rails_8.0.gemfile @@ -0,0 +1,10 @@ +# This file was generated by Appraisal + +source "https://rubygems.org" + +gem "rails", "~> 8.0.0" +gem "rspec", "~> 3.13" +gem "rspec-rails", "~> 8.0" +gem "sqlite3", "~> 2.0" + +gemspec path: "../" diff --git a/lib/phi_attrs.rb b/lib/phi_attrs.rb index 4bfa2f4..c4d3e44 100644 --- a/lib/phi_attrs.rb +++ b/lib/phi_attrs.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true -require 'rails' -require 'active_support' -require 'request_store' - -require 'phi_attrs/version' -require 'phi_attrs/configure' -require 'phi_attrs/railtie' if defined?(Rails) -require 'phi_attrs/formatter' -require 'phi_attrs/logger' -require 'phi_attrs/exceptions' -require 'phi_attrs/phi_record' +require "rails" +require "active_support" +require "request_store" + +require "phi_attrs/version" +require "phi_attrs/configure" +require "phi_attrs/railtie" if defined?(Rails) +require "phi_attrs/formatter" +require "phi_attrs/logger" +require "phi_attrs/exceptions" +require "phi_attrs/phi_record" module PhiAttrs def self.log_phi_access(user, message) diff --git a/lib/phi_attrs/configure.rb b/lib/phi_attrs/configure.rb index c669a12..ad1f172 100644 --- a/lib/phi_attrs/configure.rb +++ b/lib/phi_attrs/configure.rb @@ -5,7 +5,7 @@ module PhiAttrs @@log_shift_age = 0 # Default to disabled @@log_shift_size = 1_048_576 # 1MB - Default from logger class @@current_user_method = nil - @@translation_prefix = 'phi' + @@translation_prefix = "phi" def self.configure yield self if block_given? diff --git a/lib/phi_attrs/exceptions.rb b/lib/phi_attrs/exceptions.rb index aa087d9..a760617 100644 --- a/lib/phi_attrs/exceptions.rb +++ b/lib/phi_attrs/exceptions.rb @@ -3,7 +3,7 @@ module PhiAttrs module Exceptions class PhiAccessException < StandardError - TAG = 'UNAUTHORIZED ACCESS' + TAG = "UNAUTHORIZED ACCESS" def initialize(msg) PhiAttrs::Logger.tagged(TAG) { PhiAttrs::Logger.error(msg) } diff --git a/lib/phi_attrs/logger.rb b/lib/phi_attrs/logger.rb index bade0b1..34f8fea 100644 --- a/lib/phi_attrs/logger.rb +++ b/lib/phi_attrs/logger.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module PhiAttrs - PHI_ACCESS_LOG_TAG = 'PHI Access Log' + PHI_ACCESS_LOG_TAG = "PHI Access Log" class Logger class << self diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 46d6e1a..584bde5 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -71,17 +71,17 @@ def extend_phi_access(*methods) # Foo.allow_phi!('user@example.com', 'viewing patient record') # def allow_phi!(user_id = nil, reason = nil) - raise ArgumentError, 'block not allowed. use allow_phi with block' if block_given? + raise ArgumentError, "block not allowed. use allow_phi with block" if block_given? user_id ||= current_user reason ||= i18n_reason - raise ArgumentError, 'user_id and reason cannot be blank' if user_id.blank? || reason.blank? + raise ArgumentError, "user_id and reason cannot be blank" if user_id.blank? || reason.blank? __phi_stack.push({ - phi_access_allowed: true, - user_id: user_id, - reason: reason - }) + phi_access_allowed: true, + user_id: user_id, + reason: reason + }) PhiAttrs::Logger.tagged(PHI_ACCESS_LOG_TAG, name) do PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") @@ -109,7 +109,7 @@ def allow_phi!(user_id = nil, reason = nil) # def allow_phi(user_id = nil, reason = nil, allow_only: nil, &block) get_phi(user_id, reason, allow_only: allow_only, &block) - return + nil end # Enable PHI access for any instance of this class in the block given only @@ -131,12 +131,12 @@ def allow_phi(user_id = nil, reason = nil, allow_only: nil, &block) # end # def get_phi(user_id = nil, reason = nil, allow_only: nil) - raise ArgumentError, 'block required' unless block_given? + raise ArgumentError, "block required" unless block_given? if allow_only.present? - raise ArgumentError, 'allow_only must be iterable with each' unless allow_only.respond_to?(:each) + raise ArgumentError, "allow_only must be iterable with each" unless allow_only.respond_to?(:each) raise ArgumentError, "allow_only must all be `#{name}` objects" unless allow_only.all? { |t| t.is_a?(self) } - raise ArgumentError, 'allow_only must all have `allow_phi!` methods' unless allow_only.all? { |t| t.respond_to?(:allow_phi!) } + raise ArgumentError, "allow_only must all have `allow_phi!` methods" unless allow_only.all? { |t| t.respond_to?(:allow_phi!) } end # Save this so we don't revoke access previously extended outside the block @@ -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 + yield ensure __instances_with_extended_phi.each do |obj| if frozen_instances.include?(obj) @@ -191,11 +191,11 @@ def get_phi(user_id = nil, reason = nil, allow_only: nil) # end # # PHI Access Allowed Again def disallow_phi - raise ArgumentError, 'block required. use disallow_phi! without block' unless block_given? + raise ArgumentError, "block required. use disallow_phi! without block" unless block_given? __phi_stack.push({ - phi_access_allowed: false - }) + phi_access_allowed: false + }) yield if block_given? @@ -208,9 +208,9 @@ def disallow_phi # Foo.disallow_phi! # def disallow_phi! - raise ArgumentError, 'block not allowed. use disallow_phi with block' if block_given? + raise ArgumentError, "block not allowed. use disallow_phi with block" if block_given? - message = __phi_stack.present? ? "PHI access disabled for #{__user_id_string(__phi_stack)}" : 'PHI access disabled. No class level access was granted.' + message = __phi_stack.present? ? "PHI access disabled for #{__user_id_string(__phi_stack)}" : "PHI access disabled. No class level access was granted." __reset_phi_stack @@ -225,10 +225,10 @@ def disallow_phi! # Foo.disallow_last_phi! # def disallow_last_phi! - raise ArgumentError, 'block not allowed' if block_given? + raise ArgumentError, "block not allowed" if block_given? removed_access = __phi_stack.pop - message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : 'PHI access disabled. No class level access was granted.' + message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : "PHI access disabled. No class level access was granted." PhiAttrs::Logger.tagged(PHI_ACCESS_LOG_TAG, name) do PhiAttrs::Logger.info(message) @@ -262,7 +262,7 @@ def __reset_phi_stack def __user_id_string(access_list) access_list ||= [] - access_list.map { |c| "'#{c[:user_id]}'" }.join(',') + access_list.map { |c| "'#{c[:user_id]}'" }.join(",") end def current_user @@ -277,7 +277,7 @@ def i18n_reason i18n_path = [PhiAttrs.translation_prefix] + __path_to_controller_and_action(controller, action) i18n_path.push(*__path_to_class) - i18n_key = i18n_path.join('.') + i18n_key = i18n_path.join(".") return I18n.t(i18n_key) if I18n.exists?(i18n_key) @@ -287,16 +287,16 @@ def i18n_reason end def __path_to_controller_and_action(controller, action) - module_paths = controller.underscore.split('/') - class_name_parts = module_paths.pop.split('_') - class_name_parts.pop if class_name_parts[-1] == 'controller' - module_paths.push(class_name_parts.join('_'), action) + module_paths = controller.underscore.split("/") + class_name_parts = module_paths.pop.split("_") + class_name_parts.pop if class_name_parts[-1] == "controller" + module_paths.push(class_name_parts.join("_"), action) end def __path_to_class - module_paths = name.underscore.split('/') - class_name_parts = module_paths.pop.split('_') - module_paths.push(class_name_parts.join('_')) + module_paths = name.underscore.split("/") + class_name_parts = module_paths.pop.split("_") + module_paths.push(class_name_parts.join("_")) end end @@ -329,18 +329,18 @@ def __phi_extended_methods # foo.allow_phi!('user@example.com', 'viewing patient record') # def allow_phi!(user_id = nil, reason = nil) - raise ArgumentError, 'block not allowed. use allow_phi with block' if block_given? + raise ArgumentError, "block not allowed. use allow_phi with block" if block_given? user_id ||= self.class.current_user reason ||= self.class.i18n_reason - raise ArgumentError, 'user_id and reason cannot be blank' if user_id.blank? || reason.blank? + raise ArgumentError, "user_id and reason cannot be blank" if user_id.blank? || reason.blank? PhiAttrs::Logger.tagged(*phi_log_keys) do @__phi_access_stack.push({ - phi_access_allowed: true, - user_id: user_id, - reason: reason - }) + phi_access_allowed: true, + user_id: user_id, + reason: reason + }) PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") end @@ -362,7 +362,7 @@ def allow_phi!(user_id = nil, reason = nil) # def allow_phi(user_id = nil, reason = nil, &block) get_phi(user_id, reason, &block) - return + nil end # Enable PHI access for a single instance of this class inside the block. @@ -382,13 +382,13 @@ def allow_phi(user_id = nil, reason = nil, &block) # end # def get_phi(user_id = nil, reason = nil) - raise ArgumentError, 'block required' unless block_given? + raise ArgumentError, "block required" unless block_given? extended_instances = @__phi_relations_extended.clone begin allow_phi!(user_id, reason) - return yield + yield ensure new_extensions = @__phi_relations_extended - extended_instances disallow_last_phi!(preserve_extensions: true) @@ -403,7 +403,7 @@ def get_phi(user_id = nil, reason = nil) # foo.disallow_phi! # def disallow_phi! - raise ArgumentError, 'block not allowed. use disallow_phi with block' if block_given? + raise ArgumentError, "block not allowed. use disallow_phi with block" if block_given? PhiAttrs::Logger.tagged(*phi_log_keys) do removed_access_for = self.class.__user_id_string(@__phi_access_stack) @@ -411,7 +411,7 @@ def disallow_phi! revoke_extended_phi! @__phi_access_stack = [] - message = removed_access_for.present? ? "PHI access disabled for #{removed_access_for}" : 'PHI access disabled. No instance level access was granted.' + message = removed_access_for.present? ? "PHI access disabled for #{removed_access_for}" : "PHI access disabled. No instance level access was granted." PhiAttrs::Logger.info(message) end end @@ -431,7 +431,7 @@ def disallow_phi! # # PHI Access Disallowed Here # def disallow_phi - raise ArgumentError, 'block required. use disallow_phi! without block' unless block_given? + raise ArgumentError, "block required. use disallow_phi! without block" unless block_given? add_disallow_flag! add_disallow_flag_to_extended_phi! @@ -449,13 +449,13 @@ def disallow_phi # foo.disallow_last_phi! # def disallow_last_phi!(preserve_extensions: false) - raise ArgumentError, 'block not allowed' if block_given? + raise ArgumentError, "block not allowed" if block_given? PhiAttrs::Logger.tagged(*phi_log_keys) do removed_access = @__phi_access_stack.pop revoke_extended_phi! unless preserve_extensions - message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : 'PHI access disabled. No instance level access was granted.' + message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : "PHI access disabled. No instance level access was granted." PhiAttrs::Logger.info(message) end end @@ -499,7 +499,7 @@ def phi_allowed? # end # def require_phi! - raise PhiAccessException, 'PHI Access required, please call allow_phi or allow_phi! first' unless phi_allowed? + raise PhiAccessException, "PHI Access required, please call allow_phi or allow_phi! first" unless phi_allowed? end def reload @@ -513,8 +513,8 @@ def reload # @private since subject to change def add_disallow_flag! @__phi_access_stack.push({ - phi_access_allowed: false - }) + phi_access_allowed: false + }) end # removes the last item in instance internal stack. @@ -672,7 +672,7 @@ def phi_wrap_extension(method_name) self.class.send(:define_method, wrapped_method) do |*args, **kwargs, &block| relation = send(unwrapped_method, *args, **kwargs, &block) - if phi_allowed? && (relation.present? && relation_klass(relation).included_modules.include?(PhiRecord)) + if phi_allowed? && relation.present? && relation_klass(relation).included_modules.include?(PhiRecord) relations = relation.is_a?(Enumerable) ? relation : [relation] relations.each do |r| r.allow_phi!(phi_allowed_by, phi_access_reason) unless @__phi_relations_extended.include?(r) @@ -720,7 +720,7 @@ def relation_klass(rel) return rel.klass if rel.is_a?(ActiveRecord::Relation) return rel.first.class if rel.is_a?(Enumerable) - return rel.class + rel.class end def wrapped_extended_name(method_name) diff --git a/lib/phi_attrs/railtie.rb b/lib/phi_attrs/railtie.rb index ce9d6f5..dc9aaec 100644 --- a/lib/phi_attrs/railtie.rb +++ b/lib/phi_attrs/railtie.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true -require 'phi_attrs' -require 'rails' +require "phi_attrs" +require "rails" module PhiAttrs class Railtie < Rails::Railtie - initializer 'phi_attrs.initialize' do |_app| + initializer "phi_attrs.initialize" do |_app| ActiveSupport.on_load(:active_record) do ActiveSupport.on_load(:active_record) { extend PhiAttrs::Model } ActiveSupport.on_load(:action_controller) { include PhiAttrs::Controller } diff --git a/lib/phi_attrs/rspec.rb b/lib/phi_attrs/rspec.rb index b0dfe6f..4987491 100644 --- a/lib/phi_attrs/rspec.rb +++ b/lib/phi_attrs/rspec.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -require 'rspec/expectations' +require "rspec/expectations" -DO_NOT_SPECIFY = 'do not specify `allowed_by` or `with_access_reason` for negated `allow_phi_access`' +DO_NOT_SPECIFY = "do not specify `allowed_by` or `with_access_reason` for negated `allow_phi_access`" RSpec::Matchers.define :allow_phi_access do match do |result| @@ -31,7 +31,7 @@ failure_message do |result| msgs = [] - msgs = ['PHI Access was not allowed.'] unless @allowed + msgs = ["PHI Access was not allowed."] unless @allowed msgs << "PHI Access was allowed by '#{result.phi_allowed_by}' (not '#{@user_id}')." unless @user_id_matches msgs << "PHI Access was allowed because '#{result.phi_access_reason}' (not because '#{@reason}')." unless @reason_matches diff --git a/lib/phi_attrs/version.rb b/lib/phi_attrs/version.rb index 9e5b731..17a006b 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.2' + VERSION = "0.3.2" end diff --git a/phi_attrs.gemspec b/phi_attrs.gemspec index 226e03c..fc95f35 100644 --- a/phi_attrs.gemspec +++ b/phi_attrs.gemspec @@ -1,18 +1,18 @@ # frozen_string_literal: true -lib = File.expand_path('lib', __dir__) +lib = File.expand_path("lib", __dir__) $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) -require 'phi_attrs/version' +require "phi_attrs/version" Gem::Specification.new do |spec| - spec.name = 'phi_attrs' - spec.version = PhiAttrs::VERSION - spec.authors = ['Wyatt Kirby'] - spec.email = ['wyatt@apsis.io'] - - spec.summary = 'PHI Access Restriction & Logging for Rails ActiveRecord' - spec.homepage = 'https://www.apsis.io' - spec.license = 'MIT' + spec.name = "phi_attrs" + spec.version = PhiAttrs::VERSION + spec.authors = ["Wyatt Kirby"] + spec.email = ["wyatt@apsis.io"] + + spec.summary = "PHI Access Restriction & Logging for Rails ActiveRecord" + spec.homepage = "https://www.apsis.io" + spec.license = "MIT" spec.post_install_message = ' Thank you for installing phi_attrs! By installing this gem, you acknowledge and agree to the disclaimer as provided in the @@ -21,26 +21,30 @@ Gem::Specification.new do |spec| For full details, see: https://github.com/apsislabs/phi_attrs/blob/main/DISCLAIMER.txt ' - spec.required_ruby_version = '>= 2.7.0' - - spec.files = Dir['{app,config,lib}/**/*', 'CHANGELOG.md', 'DISCLAIMER.txt', 'LICENSE.txt', 'README.md'] - - spec.bindir = 'exe' - spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } - spec.require_paths = ['lib'] - - spec.add_dependency 'rails', '>= 7.0.0' - spec.add_dependency 'request_store', '~> 1.4' - - spec.add_development_dependency 'appraisal' - spec.add_development_dependency 'bundler', '>= 2.2.33' - spec.add_development_dependency 'byebug' - spec.add_development_dependency 'factory_bot_rails' - spec.add_development_dependency 'faker' - spec.add_development_dependency 'rake' - spec.add_development_dependency 'rubocop' - spec.add_development_dependency 'rubocop-rails' - spec.add_development_dependency 'simplecov', '~> 0.16' - spec.add_development_dependency 'tzinfo-data' - spec.metadata['rubygems_mfa_required'] = 'false' + spec.required_ruby_version = ">= 3.2.0" + + spec.files = Dir["{app,config,lib}/**/*", "CHANGELOG.md", "DISCLAIMER.txt", "LICENSE.txt", "README.md"] + + spec.bindir = "exe" + spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) } + spec.require_paths = ["lib"] + + spec.add_dependency "benchmark" + spec.add_dependency "rails", ">= 7.0.0", "< 9.0" + spec.add_dependency "request_store", "~> 1.4" + + spec.add_development_dependency "appraisal", "~> 2.5" + spec.add_development_dependency "bump" + spec.add_development_dependency "bundler", ">= 2.4" + spec.add_development_dependency "debug" + spec.add_development_dependency "factory_bot_rails" + spec.add_development_dependency "faker" + spec.add_development_dependency "rake", "~> 13.3" + spec.add_development_dependency "rspec", "~> 3.13" + spec.add_development_dependency "rspec-rails" + spec.add_development_dependency "simplecov" + spec.add_development_dependency "sqlite3" + spec.add_development_dependency "standardrb" + spec.add_development_dependency "tzinfo-data" + spec.metadata["rubygems_mfa_required"] = "false" end diff --git a/spec/dummy/app/models/patient_info.rb b/spec/dummy/app/models/patient_info.rb index dfef066..bf65178 100644 --- a/spec/dummy/app/models/patient_info.rb +++ b/spec/dummy/app/models/patient_info.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true class PatientInfo < ApplicationRecord - has_one :patient_detail, inverse_of: 'patient_info' - has_one :address, inverse_of: 'patient_info' - has_many :health_records, inverse_of: 'patient_info' + has_one :patient_detail, inverse_of: "patient_info" + has_one :address, inverse_of: "patient_info" + has_many :health_records, inverse_of: "patient_info" phi_model diff --git a/spec/dummy/application.rb b/spec/dummy/application.rb index 92ae09e..739098f 100644 --- a/spec/dummy/application.rb +++ b/spec/dummy/application.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -require 'rails/all' +require "rails/all" -require 'phi_attrs' +require "phi_attrs" module Dummy APP_ROOT = File.expand_path(__dir__).freeze @@ -11,17 +11,17 @@ class Application < Rails::Application config.root = APP_ROOT config.action_controller.perform_caching = false - config.action_mailer.default_url_options = { host: 'dummy.example.com' } + config.action_mailer.default_url_options = {host: "dummy.example.com"} config.action_mailer.delivery_method = :test config.active_support.deprecation = :stderr config.eager_load = false - config.paths['app/controllers'] << "#{APP_ROOT}/app/controllers" - config.paths['app/models'] << "#{APP_ROOT}/app/models" - config.paths['app/views'] << "#{APP_ROOT}/app/views" - config.paths['config/database'] = "#{APP_ROOT}/config/database.yml" - config.paths['log'] = 'tmp/log/development.log' - config.paths.add 'config/routes.rb', with: "#{APP_ROOT}/config/routes.rb" + config.paths["app/controllers"] << "#{APP_ROOT}/app/controllers" + config.paths["app/models"] << "#{APP_ROOT}/app/models" + config.paths["app/views"] << "#{APP_ROOT}/app/views" + config.paths["config/database"] = "#{APP_ROOT}/config/database.yml" + config.paths["log"] = "tmp/log/development.log" + config.paths.add "config/routes.rb", with: "#{APP_ROOT}/config/routes.rb" def require_environment! initialize! diff --git a/spec/dummy/config/initializers/phi_attrs.rb b/spec/dummy/config/initializers/phi_attrs.rb index ecd3e58..0d56f10 100644 --- a/spec/dummy/config/initializers/phi_attrs.rb +++ b/spec/dummy/config/initializers/phi_attrs.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true PhiAttrs.configure do |conf| - conf.log_path = Rails.root.join('log/phi_access.log') + conf.log_path = Rails.root.join("log/phi_access.log") # Log Rotation - disabled by default # See: https://apidock.com/ruby/Logger/new/class diff --git a/spec/dummy/factories/addresses.rb b/spec/dummy/factories/addresses.rb index f3fa218..785cfba 100644 --- a/spec/dummy/factories/addresses.rb +++ b/spec/dummy/factories/addresses.rb @@ -2,7 +2,7 @@ FactoryBot.define do factory :address do - address { '123 Little Whinging' } + address { "123 Little Whinging" } trait :all_random do address { Faker::Movies::HarryPotter.location } diff --git a/spec/dummy/factories/patient_details.rb b/spec/dummy/factories/patient_details.rb index 7274d11..9b4c3a8 100644 --- a/spec/dummy/factories/patient_details.rb +++ b/spec/dummy/factories/patient_details.rb @@ -2,7 +2,7 @@ FactoryBot.define do factory :patient_detail do - detail { 'Generic Spell' } + detail { "Generic Spell" } trait :all_random do detail { Faker::Movies::HarryPotter.spell } diff --git a/spec/dummy/factories/patient_infos.rb b/spec/dummy/factories/patient_infos.rb index d16f5c0..cff5830 100644 --- a/spec/dummy/factories/patient_infos.rb +++ b/spec/dummy/factories/patient_infos.rb @@ -2,8 +2,8 @@ FactoryBot.define do factory :patient_info do - first_name { 'Joe Johnson' } - last_name { 'All Houses' } + first_name { "Joe Johnson" } + last_name { "All Houses" } association :address, factory: :address, strategy: :build association :patient_detail, factory: :patient_detail, strategy: :build diff --git a/spec/phi_attrs/configure_spec.rb b/spec/phi_attrs/configure_spec.rb index f4f5a38..5eab169 100644 --- a/spec/phi_attrs/configure_spec.rb +++ b/spec/phi_attrs/configure_spec.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'configure' do +RSpec.describe "configure" do orig_method = nil orig_path = nil orig_age = nil @@ -35,58 +35,58 @@ end end - context 'current user' do + context "current user" do subject(:current_user_method) { PhiAttrs.current_user_method } it { is_expected.to be_nil } - it 'can be set' do + it "can be set" do PhiAttrs.configure { |c| c.current_user_method = :phi_user } expect(current_user_method).to be(:phi_user) end end - context 'log_path' do + context "log_path" do subject(:log_path) { PhiAttrs.log_path } it { is_expected.to be_nil } - it 'can be set' do - PhiAttrs.configure { |c| c.log_path = 'deep_path' } - expect(log_path).to be('deep_path') + it "can be set" do + PhiAttrs.configure { |c| c.log_path = "deep_path" } + expect(log_path).to be("deep_path") end end - context 'log_age' do + context "log_age" do subject(:log_shift_age) { PhiAttrs.log_shift_age } it { is_expected.to be_nil } - it 'can be set' do + it "can be set" do PhiAttrs.configure { |c| c.log_shift_age = 7 } expect(log_shift_age).to be(7) end end - context 'log_size' do + context "log_size" do subject(:log_shift_size) { PhiAttrs.log_shift_size } it { is_expected.to be_nil } - it 'can be set' do + it "can be set" do PhiAttrs.configure { |c| c.log_shift_size = 100.megabytes } expect(log_shift_size).to be(100.megabytes) end end - context 'translation prefix' do + context "translation prefix" do subject(:translation_prefix) { PhiAttrs.translation_prefix } it { is_expected.to eq(nil) } - it 'can be set' do - PhiAttrs.configure { |c| c.translation_prefix = 'phi_gem' } - expect(translation_prefix).to be('phi_gem') + it "can be set" do + PhiAttrs.configure { |c| c.translation_prefix = "phi_gem" } + expect(translation_prefix).to be("phi_gem") end end end diff --git a/spec/phi_attrs/controllers/current_user_spec.rb b/spec/phi_attrs/controllers/current_user_spec.rb index c24c5cf..1580cda 100644 --- a/spec/phi_attrs/controllers/current_user_spec.rb +++ b/spec/phi_attrs/controllers/current_user_spec.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" class SampleController < ApplicationController; end -RSpec.describe 'default user', type: :controller do +RSpec.describe "default user", type: :controller do controller SampleController do def index PatientInfo.allow_phi do @@ -31,14 +31,14 @@ def phi_user create(:patient_info, :all_random, :with_multiple_health_records) end - context 'with translation' do - it 'uses the translation file for a null reason' do - message = I18n.t('phi.sample.index.patient_info') + context "with translation" do + it "uses the translation file for a null reason" do + message = I18n.t("phi.sample.index.patient_info") allow(PhiAttrs::Logger.logger).to receive(:info) - get :index, params: { phi_user: 'Madame Pomfrey' } + get :index, params: {phi_user: "Madame Pomfrey"} - expect(PhiAttrs::Logger.logger).to have_received(:info).with(include('Madame Pomfrey')).at_least(:once) + expect(PhiAttrs::Logger.logger).to have_received(:info).with(include("Madame Pomfrey")).at_least(:once) expect(PhiAttrs::Logger.logger).to have_received(:info).with(end_with message).at_least(:once) end end diff --git a/spec/phi_attrs/controllers/i18n_nested_controller_spec.rb b/spec/phi_attrs/controllers/i18n_nested_controller_spec.rb index abe33ee..f7689f3 100644 --- a/spec/phi_attrs/controllers/i18n_nested_controller_spec.rb +++ b/spec/phi_attrs/controllers/i18n_nested_controller_spec.rb @@ -1,22 +1,22 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" module Namespace class NestedController < ApplicationController; end end -RSpec.describe 'i18n in controller', type: :controller do +RSpec.describe "i18n in controller", type: :controller do controller Namespace::NestedController do def index - PatientInfo.allow_phi('public_user') do + PatientInfo.allow_phi("public_user") do render json: PatientInfo.all.map(&:summary_json) end end def show pi = PatientInfo.find(params[:id]) - pi.allow_phi('public_user') do + pi.allow_phi("public_user") do render json: pi.detail_json end end @@ -26,20 +26,20 @@ def show create(:patient_info, :all_random, :with_multiple_health_records) end - context 'with translation' do - it 'uses the translation file for a null reason' do + context "with translation" do + it "uses the translation file for a null reason" do allow(PhiAttrs::Logger.logger).to receive(:info) - get :show, params: { id: PatientInfo.first.id } + get :show, params: {id: PatientInfo.first.id} - message = I18n.t('phi.namespace.nested.show.patient_info') + message = I18n.t("phi.namespace.nested.show.patient_info") expect(PhiAttrs::Logger.logger).to have_received(:info).with(end_with message).at_least(:once) end end - context 'without translation' do - it 'warns the user when a translation file was not found' do - message = 'No en PHI Reason found for phi.namespace.nested.index.patient_info' + context "without translation" do + it "warns the user when a translation file was not found" do + message = "No en PHI Reason found for phi.namespace.nested.index.patient_info" allow(PhiAttrs::Logger.logger).to receive(:warn) expect do diff --git a/spec/phi_attrs/controllers/i18n_sample_controller_spec.rb b/spec/phi_attrs/controllers/i18n_sample_controller_spec.rb index ceb67ea..72a8167 100644 --- a/spec/phi_attrs/controllers/i18n_sample_controller_spec.rb +++ b/spec/phi_attrs/controllers/i18n_sample_controller_spec.rb @@ -1,20 +1,20 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" class SampleController < ApplicationController; end -RSpec.describe 'i18n in controller', type: :controller do +RSpec.describe "i18n in controller", type: :controller do controller SampleController do def index - PatientInfo.allow_phi('public_user') do + PatientInfo.allow_phi("public_user") do render json: PatientInfo.all.map(&:summary_json) end end def show pi = PatientInfo.find(params[:id]) - pi.allow_phi('public_user') do + pi.allow_phi("public_user") do render json: pi.detail_json end end @@ -24,9 +24,9 @@ def show create(:patient_info, :all_random, :with_multiple_health_records) end - context 'with translation' do - it 'uses the translation file for a null reason' do - message = I18n.t('phi.sample.index.patient_info') + context "with translation" do + it "uses the translation file for a null reason" do + message = I18n.t("phi.sample.index.patient_info") allow(PhiAttrs::Logger.logger).to receive(:info) get :index @@ -34,13 +34,13 @@ def show end end - context 'without translation' do - it 'warns the user when a translation file was not found' do - message = 'No en PHI Reason found for phi.sample.show.patient_info' + context "without translation" do + it "warns the user when a translation file was not found" do + message = "No en PHI Reason found for phi.sample.show.patient_info" allow(PhiAttrs::Logger.logger).to receive(:warn) expect do - get :show, params: { id: PatientInfo.first.id } + get :show, params: {id: PatientInfo.first.id} end.to raise_error(ArgumentError) expect(PhiAttrs::Logger.logger).to have_received(:warn).with(message) end diff --git a/spec/phi_attrs/delegations_spec.rb b/spec/phi_attrs/delegations_spec.rb index f4941e0..1143a33 100644 --- a/spec/phi_attrs/delegations_spec.rb +++ b/spec/phi_attrs/delegations_spec.rb @@ -1,37 +1,37 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'delegations' do +RSpec.describe "delegations" do file_name = __FILE__ let(:address) { build(:address) } - let(:kwargs) { { avoid_phi: 'avoid' } } + let(:kwargs) { {avoid_phi: "avoid"} } - context 'authorized' do - it 'delegates to default attribute' do |t| + context "authorized" do + it "delegates to default attribute" do |t| address.allow_phi(file_name, t.full_description) do expect { address.address }.not_to raise_error end end - it 'delegates arguments correctly' do |t| + it "delegates arguments correctly" do |t| address.allow_phi(file_name, t.full_description) do expect(address.inlined).to eq(address.address) expect(address.inlined(avoid_phi: nil)).to eq(address.address) end # These calls should never touch the PHI field if delegated correctly - expect { address.inlined(avoid_phi: 'avoid') }.not_to raise_error + expect { address.inlined(avoid_phi: "avoid") }.not_to raise_error expect { address.inlined(**kwargs) }.not_to raise_error - expect(address.inlined(avoid_phi: 'avoid')).to eq('avoid') - expect(address.inlined(**kwargs)).to eq('avoid') + expect(address.inlined(avoid_phi: "avoid")).to eq("avoid") + expect(address.inlined(**kwargs)).to eq("avoid") end end - context 'unauthorized' do - it 'raises errors with delegated arguments' do + context "unauthorized" do + it "raises errors with delegated arguments" do # These calls should try to try to access phi, and fail expect { address.inlined }.to raise_error(access_error) expect { address.inlined(avoid_phi: false) }.to raise_error(access_error) diff --git a/spec/phi_attrs/exceptions_spec.rb b/spec/phi_attrs/exceptions_spec.rb index 0ad32eb..096d2e2 100644 --- a/spec/phi_attrs/exceptions_spec.rb +++ b/spec/phi_attrs/exceptions_spec.rb @@ -1,20 +1,20 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'exceptions' do - let(:patient_john) { build(:patient_info, first_name: 'John') } +RSpec.describe "exceptions" do + let(:patient_john) { build(:patient_info, first_name: "John") } - context 'unauthorized' do - it 'raises an error on default attribute' do + context "unauthorized" do + it "raises an error on default attribute" do expect { patient_john.first_name }.to raise_error(access_error) end - it 'raises an error on included method' do + it "raises an error on included method" do expect { patient_john.birthday }.to raise_error(access_error) end - it 'does not raise an error on excluded attribute' do + it "does not raise an error on excluded attribute" do expect { patient_john.last_name }.not_to raise_error end end diff --git a/spec/phi_attrs/logger_spec.rb b/spec/phi_attrs/logger_spec.rb index 92fcc51..df3994f 100644 --- a/spec/phi_attrs/logger_spec.rb +++ b/spec/phi_attrs/logger_spec.rb @@ -1,16 +1,16 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" RSpec.describe Logger do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } - context 'log' do - context 'error' do - it 'when raising an exception' do - message = 'my error message' + context "log" do + context "error" do + it "when raising an exception" do + message = "my error message" expect(PhiAttrs::Logger.logger).to receive(:error).with(message) expect do @@ -18,62 +18,62 @@ end.to raise_error(access_error) end - it 'for unauthorized access' do + it "for unauthorized access" do expect(PhiAttrs::Logger.logger).to receive(:error) expect { patient_jane.birthday }.to raise_error(access_error) end end - context 'info' do - it 'when granting phi to instance' do |t| + context "info" do + it "when granting phi to instance" do |t| expect(PhiAttrs::Logger.logger).to receive(:info) patient_jane.allow_phi!(file_name, t.full_description) end - it 'when granting phi to class' do |t| + it "when granting phi to class" do |t| expect(PhiAttrs::Logger.logger).to receive(:info) PatientInfo.allow_phi!(file_name, t.full_description) end - it 'when revokes phi to class, no current access' do + it "when revokes phi to class, no current access" do expect(PhiAttrs::Logger.logger).to receive(:info).with(/No class level/) PatientInfo.disallow_phi! end - it 'when revokes phi to instance, no current access' do + it "when revokes phi to instance, no current access" do expect(PhiAttrs::Logger.logger).to receive(:info).with(/No instance level/) patient_jane.disallow_phi! end - it 'when revokes phi to class, with current access' do |t| + it "when revokes phi to class, with current access" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info).with(Regexp.new(file_name)) PatientInfo.disallow_phi! end - it 'when revokes phi to instance, with current access' do |t| + it "when revokes phi to instance, with current access" do |t| patient_jane.allow_phi!(file_name, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info).with(Regexp.new(file_name)) patient_jane.disallow_phi! end - it 'when accessing method' do |t| + it "when accessing method" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info) patient_jane.first_name end end - context 'identifier' do - context 'allowed' do - it 'object_id for unpersisted' do |t| + context "identifier" do + context "allowed" do + it "object_id for unpersisted" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:tagged).with(PhiAttrs::PHI_ACCESS_LOG_TAG, PatientInfo.name, "Object: #{patient_jane.object_id}").and_call_original expect(PhiAttrs::Logger.logger).to receive(:info) patient_jane.first_name end - it 'id for persisted' do |t| + it "id for persisted" do |t| PatientInfo.allow_phi!(file_name, t.full_description) patient_jane.save expect(patient_jane.persisted?).to be true @@ -83,15 +83,15 @@ end end - context 'unauthorized' do - it 'object_id for unpersisted' do + context "unauthorized" do + it "object_id for unpersisted" do expect(PhiAttrs::Logger.logger).to receive(:tagged).with(PhiAttrs::PHI_ACCESS_LOG_TAG, PatientInfo.name, "Object: #{patient_jane.object_id}").and_call_original expect(PhiAttrs::Logger.logger).to receive(:tagged).with(PhiAttrs::Exceptions::PhiAccessException::TAG).and_call_original expect(PhiAttrs::Logger.logger).to receive(:error) expect { patient_jane.first_name }.to raise_error(access_error) end - it 'id for persisted' do + it "id for persisted" do patient_jane.save # expect(patient_jane.persisted?).to be true expect(PhiAttrs::Logger.logger).to receive(:tagged).with(PhiAttrs::PHI_ACCESS_LOG_TAG, PatientInfo.name, "Key: #{patient_jane.id}").and_call_original @@ -101,24 +101,24 @@ end end - it 'user for manual' do - user = 'Test User' - message = 'Access Granted Message' + it "user for manual" do + user = "Test User" + message = "Access Granted Message" expect(PhiAttrs::Logger.logger).to receive(:tagged).with(PhiAttrs::PHI_ACCESS_LOG_TAG, user).and_call_original expect(PhiAttrs::Logger.logger).to receive(:info).with(message) PhiAttrs.log_phi_access(user, message) end end - context 'frequency' do - it 'once when accessing multiple methods' do |t| + context "frequency" do + it "once when accessing multiple methods" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info) patient_jane.first_name patient_jane.birthday end - it 'multiple times for nested allow_phi calls' do |t| + it "multiple times for nested allow_phi calls" do |t| expect(PhiAttrs::Logger.logger).to receive(:info).exactly(6) PatientInfo.allow_phi(file_name, t.full_description) do # Logged allowed @@ -129,7 +129,7 @@ end # Logged Disallowed end - it 'multiple times for nested allows and disallows' do |t| + it "multiple times for nested allows and disallows" do |t| PatientInfo.allow_phi!("#{file_name}1", t.full_description) PatientInfo.allow_phi!("#{file_name}2", t.full_description) PatientInfo.allow_phi!("#{file_name}3", t.full_description) @@ -145,12 +145,12 @@ end end - context 'full stack' do - let(:first_allow) { 'first@allow.com' } - let(:second_allow) { 'second@allow.com' } + context "full stack" do + let(:first_allow) { "first@allow.com" } + let(:second_allow) { "second@allow.com" } let(:regexp) { Regexp.new("#{first_allow}.+#{second_allow}|#{second_allow}.+#{first_allow}") } - context 'for multiple allows' do + context "for multiple allows" do def test_logger expect(PhiAttrs::Logger.logger).to receive(:info).with(regexp) patient_jane.first_name @@ -160,20 +160,20 @@ def expect_disallow_message(allowed) expect(PhiAttrs::Logger.logger).to receive(:info).with("PHI access disabled for #{allowed}") end - context 'first class' do - it 'then class' do |t| + context "first class" do + it "then class" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) PatientInfo.allow_phi!(second_allow, t.full_description) test_logger end - it 'then instance' do |t| + it "then instance" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) patient_jane.allow_phi!(second_allow, t.full_description) test_logger end - it 'then class block' do |t| + it "then class block" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) PatientInfo.allow_phi(second_allow, t.full_description) do test_logger @@ -181,7 +181,7 @@ def expect_disallow_message(allowed) end end - it 'then instance block' do |t| + it "then instance block" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) patient_jane.allow_phi(second_allow, t.full_description) do test_logger @@ -189,7 +189,7 @@ def expect_disallow_message(allowed) end end - it 'only one when previously revoked' do |t| + it "only one when previously revoked" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) PatientInfo.disallow_phi! patient_jane.allow_phi!(second_allow, t.full_description) @@ -198,20 +198,20 @@ def expect_disallow_message(allowed) end end - context 'first instance' do - it 'then class' do |t| + context "first instance" do + it "then class" do |t| patient_jane.allow_phi!(first_allow, t.full_description) PatientInfo.allow_phi!(second_allow, t.full_description) test_logger end - it 'then instance' do |t| + it "then instance" do |t| patient_jane.allow_phi!(first_allow, t.full_description) patient_jane.allow_phi!(second_allow, t.full_description) test_logger end - it 'then class block' do |t| + it "then class block" do |t| patient_jane.allow_phi!(first_allow, t.full_description) PatientInfo.allow_phi(second_allow, t.full_description) do test_logger @@ -219,7 +219,7 @@ def expect_disallow_message(allowed) end end - it 'then instance block' do |t| + it "then instance block" do |t| patient_jane.allow_phi!(first_allow, t.full_description) patient_jane.allow_phi(second_allow, t.full_description) do test_logger @@ -227,7 +227,7 @@ def expect_disallow_message(allowed) end end - it 'only one when previously revoked' do |t| + it "only one when previously revoked" do |t| patient_jane.allow_phi!(first_allow, t.full_description) patient_jane.disallow_phi! PatientInfo.allow_phi!(second_allow, t.full_description) @@ -237,15 +237,15 @@ def expect_disallow_message(allowed) end end - context 'for disallow_phi!' do - it 'class' do |t| + context "for disallow_phi!" do + it "class" do |t| PatientInfo.allow_phi!(first_allow, t.full_description) PatientInfo.allow_phi!(second_allow, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info).with(regexp) PatientInfo.disallow_phi! end - it 'instance' do |t| + it "instance" do |t| patient_jane.allow_phi!(first_allow, t.full_description) patient_jane.allow_phi!(second_allow, t.full_description) expect(PhiAttrs::Logger.logger).to receive(:info).with(regexp) 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 68cde40..5c796d7 100644 --- a/spec/phi_attrs/phi_record/class__allow_phi_spec.rb +++ b/spec/phi_attrs/phi_record/class__allow_phi_spec.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'class allow_phi' do +RSpec.describe "class allow_phi" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } let(:patient_detail) { build(:patient_detail) } - let(:patient_with_detail) { build(:patient_info, first_name: 'Jack', patient_detail: patient_detail) } + let(:patient_with_detail) { build(:patient_info, first_name: "Jack", patient_detail: patient_detail) } - context 'authorized' do - it 'allows access to any instance' do |t| + context "authorized" do + it "allows access to any instance" do |t| expect { patient_jane.first_name }.to raise_error(access_error) PatientInfo.allow_phi(file_name, t.full_description) do expect { patient_jane.first_name }.not_to raise_error @@ -19,7 +19,7 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'only allows access to the authorized class' do |t| + it "only allows access to the authorized class" do |t| expect { patient_detail.detail }.to raise_error(access_error) expect { patient_jane.first_name }.to raise_error(access_error) @@ -37,7 +37,7 @@ expect { patient_detail.detail }.to raise_error(access_error) end - it 'revokes access after calling disallow_phi!' do |t| + it "revokes access after calling disallow_phi!" do |t| expect { patient_jane.first_name }.to raise_error(access_error) PatientInfo.allow_phi!(file_name, t.full_description) @@ -49,15 +49,15 @@ expect { patient_jane.first_name }.to raise_error(access_error) end - it 'raises ArgumentError for allow_phi! with blank values' do - expect { PatientInfo.allow_phi! '', '' }.to raise_error(ArgumentError) - expect { PatientInfo.allow_phi! 'ok', '' }.to raise_error(ArgumentError) - expect { PatientInfo.allow_phi! '', 'ok' }.to raise_error(ArgumentError) - expect { PatientInfo.allow_phi! 'ok', 'ok' }.not_to raise_error + it "raises ArgumentError for allow_phi! with blank values" do + expect { PatientInfo.allow_phi! "", "" }.to raise_error(ArgumentError) + expect { PatientInfo.allow_phi! "ok", "" }.to raise_error(ArgumentError) + expect { PatientInfo.allow_phi! "", "ok" }.to raise_error(ArgumentError) + expect { PatientInfo.allow_phi! "ok", "ok" }.not_to raise_error end - it 'allow_phi persists after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail)) + it "allow_phi persists after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail)) PatientInfo.allow_phi(file_name, t.full_description) do expect { dumbledore.first_name }.not_to raise_error dumbledore.reload @@ -65,8 +65,8 @@ end end - it 'allow_phi persists extended phi after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi persists extended phi after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) expect { dumbledore.patient_detail.detail }.to raise_error(access_error) PatientInfo.allow_phi(file_name, t.full_description) do @@ -78,8 +78,8 @@ expect { dumbledore.patient_detail.detail }.to raise_error(access_error) end - it 'allow_phi persists extended phi after a reload _and_ respects previous data' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi persists extended phi after a reload _and_ respects previous data" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) PatientInfo.allow_phi!(file_name, t.full_description) expect { dumbledore.patient_detail.detail }.not_to raise_error @@ -92,49 +92,49 @@ expect { dumbledore.patient_detail.detail }.not_to raise_error end - it 'allow_phi! persists after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail)) + it "allow_phi! persists after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail)) PatientInfo.allow_phi!(file_name, t.full_description) expect { dumbledore.first_name }.not_to raise_error dumbledore.reload expect { dumbledore.first_name }.not_to raise_error end - it 'allow_phi! persists extended phi after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi! persists extended phi after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) PatientInfo.allow_phi!(file_name, t.full_description) expect { dumbledore.patient_detail.detail }.not_to raise_error dumbledore.reload expect { dumbledore.patient_detail.detail }.not_to raise_error end - 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') + 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| + 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(first_name).to eq("Jane") expect(patient_jane.phi_allowed?).to be false end end - context 'extended authorization' do + context "extended authorization" do let(:patient_mary) { create(:patient_info, :with_multiple_health_records) } - it 'extends access to associations' do |t| + it "extends access to associations" do |t| expect { patient_mary.patient_detail.detail }.to raise_error(access_error) PatientInfo.allow_phi!(file_name, t.full_description) expect { patient_mary.patient_detail.detail }.not_to raise_error end - it 'extends access with a block' do |t| + it "extends access with a block" do |t| expect { patient_mary.patient_detail.detail }.to raise_error(access_error) PatientInfo.allow_phi(file_name, t.full_description) do @@ -144,7 +144,7 @@ def name_getter(reason, description) expect { patient_mary.patient_detail.detail }.to raise_error(access_error) end - it 'does not revoke access for untouched associations' do |t| + it "does not revoke access for untouched associations" do |t| # Here we extend access to two different associations. # When the block terminates, it should revoke (the one frame of) the `health_records` access, # but it should NOT revoke (the only frame of) the `patient_detail` access. @@ -169,8 +169,8 @@ def name_getter(reason, description) end end - context 'nested allowances' do - it 'retains outer access when disallowed at inner level' do |t| + context "nested allowances" do + it "retains outer access when disallowed at inner level" do |t| PatientInfo.allow_phi(file_name, t.full_description) do expect { patient_with_detail.first_name }.not_to raise_error @@ -187,22 +187,22 @@ def name_getter(reason, description) end end - context 'block checks' do - context 'allow_phi' do - it 'succeeds' do - expect { PatientInfo.allow_phi!('ok', 'ok') }.not_to raise_error + context "block checks" do + context "allow_phi" do + it "succeeds" do + expect { PatientInfo.allow_phi!("ok", "ok") }.not_to raise_error end - it 'raises ArgumentError with block' do - expect { PatientInfo.allow_phi!('ok', 'ok') { do_nothing } }.to raise_error(ArgumentError) + it "raises ArgumentError with block" do + expect { PatientInfo.allow_phi!("ok", "ok") { do_nothing } }.to raise_error(ArgumentError) end end - context 'allow_phi!' do - it 'succeeds' do - expect { PatientInfo.allow_phi('ok', 'ok') { do_nothing } }.not_to raise_error + context "allow_phi!" do + it "succeeds" do + expect { PatientInfo.allow_phi("ok", "ok") { do_nothing } }.not_to raise_error end - it 'raises ArgumentError for allow_phi! without block' do - expect { PatientInfo.allow_phi('ok', 'ok') }.to raise_error(ArgumentError) + it "raises ArgumentError for allow_phi! without block" do + expect { PatientInfo.allow_phi("ok", "ok") }.to raise_error(ArgumentError) end end end diff --git a/spec/phi_attrs/phi_record/class__disallow_phi_spec.rb b/spec/phi_attrs/phi_record/class__disallow_phi_spec.rb index c16cd6a..fb065b0 100644 --- a/spec/phi_attrs/phi_record/class__disallow_phi_spec.rb +++ b/spec/phi_attrs/phi_record/class__disallow_phi_spec.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'class disallow_phi' do +RSpec.describe "class disallow_phi" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } - let(:patient_john) { build(:patient_info, first_name: 'John') } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_john) { build(:patient_info, first_name: "John") } - context 'block' do - it 'disables all allowances within the block' do |t| + context "block" do + it "disables all allowances within the block" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect { patient_jane.first_name }.not_to raise_error @@ -18,7 +18,7 @@ end end - it 'returns permission after the block' do |t| + it "returns permission after the block" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect { patient_jane.first_name }.not_to raise_error @@ -29,7 +29,7 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'does not affect explicit instance allow' do |t| + it "does not affect explicit instance allow" do |t| PatientInfo.allow_phi!(file_name, t.full_description) patient_john.allow_phi!(file_name, t.full_description) @@ -45,13 +45,13 @@ expect { patient_john.first_name }.not_to raise_error end - it 'raises ArgumentError without block' do + it "raises ArgumentError without block" do expect { PatientInfo.disallow_phi }.to raise_error(ArgumentError) end end - context 'disallow_phi!' do - it 'disallows whole stack' do |t| + context "disallow_phi!" do + it "disallows whole stack" do |t| PatientInfo.allow_phi!("#{file_name}1", t.full_description) expect { patient_jane.first_name }.not_to raise_error PatientInfo.allow_phi!("#{file_name}2", t.full_description) @@ -60,7 +60,7 @@ expect { patient_jane.first_name }.to raise_error(access_error) end - it 'disallows does not affect instance allows' do |t| + it "disallows does not affect instance allows" do |t| PatientInfo.allow_phi!("#{file_name}1", t.full_description) expect { patient_jane.first_name }.not_to raise_error patient_jane.allow_phi!("#{file_name}2", t.full_description) @@ -69,7 +69,7 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'allows access after disallow' do |t| + it "allows access after disallow" do |t| PatientInfo.allow_phi!("#{file_name}1", t.full_description) expect { patient_jane.first_name }.not_to raise_error PatientInfo.disallow_phi! @@ -78,7 +78,7 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'raises ArgumentError with block' do + it "raises ArgumentError with block" do expect { PatientInfo.disallow_phi! { do_nothing } }.to raise_error(ArgumentError) end end diff --git a/spec/phi_attrs/phi_record/class__phi_allowed_spec.rb b/spec/phi_attrs/phi_record/class__phi_allowed_spec.rb index 49f777e..094e282 100644 --- a/spec/phi_attrs/phi_record/class__phi_allowed_spec.rb +++ b/spec/phi_attrs/phi_record/class__phi_allowed_spec.rb @@ -1,13 +1,13 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'class phi_allowed?' do +RSpec.describe "class phi_allowed?" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } - context 'authorized' do - it 'works' do |t| + context "authorized" do + it "works" do |t| expect(PatientInfo.phi_allowed?).to be false PatientInfo.allow_phi(file_name, t.full_description) do expect(PatientInfo.phi_allowed?).to be true @@ -17,7 +17,7 @@ expect(PatientInfo.phi_allowed?).to be true end - it 'only allows access to the authorized class' do |t| + it "only allows access to the authorized class" do |t| expect(PatientDetail.phi_allowed?).to be false PatientInfo.allow_phi(file_name, t.full_description) do @@ -34,7 +34,7 @@ expect(PatientDetail.phi_allowed?).to be false end - it 'revokes access after calling disallow_phi!' do |t| + it "revokes access after calling disallow_phi!" do |t| expect(PatientInfo.phi_allowed?).to be false PatientInfo.allow_phi!(file_name, t.full_description) @@ -46,7 +46,7 @@ expect(PatientInfo.phi_allowed?).to be false end - it 'revokes access for disallow_phi block' do |t| + it "revokes access for disallow_phi block" do |t| expect(PatientInfo.phi_allowed?).to be false PatientInfo.allow_phi!(file_name, t.full_description) @@ -61,8 +61,8 @@ end end - context 'nested allowances' do - it 'retains outer access when disallowed at inner level' do |t| + context "nested allowances" do + it "retains outer access when disallowed at inner level" do |t| PatientInfo.allow_phi(file_name, t.full_description) do expect(PatientInfo.phi_allowed?).to be true @@ -76,7 +76,7 @@ expect(PatientInfo.phi_allowed?).to be false end - it 'retains outer access when disallow block at inner level' do |t| + it "retains outer access when disallow block at inner level" do |t| PatientInfo.allow_phi(file_name, t.full_description) do expect(PatientInfo.phi_allowed?).to be true @@ -90,7 +90,7 @@ expect(PatientInfo.phi_allowed?).to be false end - it 'retains outer access with nested disallow blocks' do |t| + it "retains outer access with nested disallow blocks" do |t| PatientInfo.allow_phi!(file_name, t.full_description) PatientInfo.disallow_phi do @@ -107,15 +107,15 @@ end end - context 'with instance' do - it 'allow_phi does not change status' do |t| + context "with instance" do + it "allow_phi does not change status" do |t| expect(PatientInfo.phi_allowed?).to be false patient_jane.allow_phi(file_name, t.full_description) do expect(PatientInfo.phi_allowed?).to be false end end - it 'disallow_phi does not change status' do |t| + it "disallow_phi does not change status" do |t| expect(PatientInfo.phi_allowed?).to be false PatientInfo.allow_phi!(file_name, t.full_description) expect(PatientInfo.phi_allowed?).to be true 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 47cba25..a4fef4c 100644 --- a/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb +++ b/spec/phi_attrs/phi_record/instance__allow_phi_spec.rb @@ -1,18 +1,18 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'instance allow_phi' do +RSpec.describe "instance allow_phi" do file_name = __FILE__ - let(:patient_john) { build(:patient_info, first_name: 'John') } - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } + let(:patient_john) { build(:patient_info, first_name: "John") } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } let(:patient_detail) { build(:patient_detail) } - let(:patient_with_detail) { build(:patient_info, first_name: 'Jack', patient_detail: patient_detail) } + let(:patient_with_detail) { build(:patient_info, first_name: "Jack", patient_detail: patient_detail) } - context 'authorized' do - context 'single record' do - it 'allows access to an authorized instance' do |t| + context "authorized" do + context "single record" do + it "allows access to an authorized instance" do |t| expect { patient_jane.first_name }.to raise_error(access_error) patient_jane.allow_phi(file_name, t.full_description) do @@ -26,7 +26,7 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'only allows access to the authorized instance' do |t| + it "only allows access to the authorized instance" do |t| patient_jane.allow_phi(file_name, t.full_description) do expect { patient_jane.first_name }.not_to raise_error expect { patient_john.first_name }.to raise_error(access_error) @@ -38,7 +38,7 @@ expect { patient_john.first_name }.to raise_error(access_error) end - it 'revokes access after calling disallow_phi!' do |t| + it "revokes access after calling disallow_phi!" do |t| expect { patient_jane.first_name }.to raise_error(access_error) patient_jane.allow_phi!(file_name, t.full_description) @@ -50,8 +50,8 @@ expect { patient_jane.first_name }.to raise_error(access_error) end - it 'allows access on an instance that already exists' do |t| - john = create(:patient_info, first_name: 'John') + it "allows access on an instance that already exists" do |t| + john = create(:patient_info, first_name: "John") expect { john.first_name }.to raise_error(access_error) john_id = john.id @@ -62,18 +62,18 @@ john.allow_phi!(file_name, t.full_description) expect { john.first_name }.not_to raise_error - expect(john.first_name).to eq 'John' + expect(john.first_name).to eq "John" end - it 'rejects calls to allow_phi! with blank values' do - expect { patient_jane.allow_phi! '', '' }.to raise_error(ArgumentError) - expect { patient_jane.allow_phi! 'ok', '' }.to raise_error(ArgumentError) - expect { patient_jane.allow_phi! '', 'ok' }.to raise_error(ArgumentError) - expect { patient_jane.allow_phi! 'ok', 'ok' }.not_to raise_error + it "rejects calls to allow_phi! with blank values" do + expect { patient_jane.allow_phi! "", "" }.to raise_error(ArgumentError) + expect { patient_jane.allow_phi! "ok", "" }.to raise_error(ArgumentError) + expect { patient_jane.allow_phi! "", "ok" }.to raise_error(ArgumentError) + expect { patient_jane.allow_phi! "ok", "ok" }.not_to raise_error end - it 'allow_phi persists after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail)) + it "allow_phi persists after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail)) dumbledore.allow_phi(file_name, t.full_description) do expect { dumbledore.first_name }.not_to raise_error dumbledore.reload @@ -81,8 +81,8 @@ end end - it 'allow_phi persists extended phi after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi persists extended phi after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) dumbledore.allow_phi(file_name, t.full_description) do expect { dumbledore.patient_detail.detail }.not_to raise_error dumbledore.reload @@ -91,8 +91,8 @@ expect { dumbledore.patient_detail.detail }.to raise_error(access_error) end - it 'allow_phi persists extended phi after a reload _and_ respects previous data' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi persists extended phi after a reload _and_ respects previous data" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) dumbledore.allow_phi!(file_name, t.full_description) expect { dumbledore.patient_detail.detail }.not_to raise_error @@ -105,45 +105,45 @@ expect { dumbledore.patient_detail.detail }.not_to raise_error end - it 'allow_phi! persists after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail)) + it "allow_phi! persists after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail)) dumbledore.allow_phi!(file_name, t.full_description) expect { dumbledore.first_name }.not_to raise_error dumbledore.reload expect { dumbledore.first_name }.not_to raise_error end - it 'allow_phi! persists extended phi after a reload' do |t| - dumbledore = create(:patient_info, first_name: 'Albus', patient_detail: build(:patient_detail, :all_random)) + it "allow_phi! persists extended phi after a reload" do |t| + dumbledore = create(:patient_info, first_name: "Albus", patient_detail: build(:patient_detail, :all_random)) dumbledore.allow_phi!(file_name, t.full_description) expect { dumbledore.patient_detail.detail }.not_to raise_error dumbledore.reload expect { dumbledore.patient_detail.detail }.not_to raise_error end - 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') + 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| + 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(first_name).to eq("Jane") expect(patient_jane.phi_allowed?).to be false end end - context 'collection' do - let(:jay) { create(:patient_info, first_name: 'Jay') } - let(:bob) { create(:patient_info, first_name: 'Bob') } - let(:moe) { create(:patient_info, first_name: 'Moe') } + context "collection" do + let(:jay) { create(:patient_info, first_name: "Jay") } + let(:bob) { create(:patient_info, first_name: "Bob") } + let(:moe) { create(:patient_info, first_name: "Moe") } let(:patients) { [jay, bob, moe] } - it 'allows access when fetched as a collection' do |t| + it "allows access when fetched as a collection" do |t| expect(patients).to contain_exactly(jay, bob, moe) expect { patients.map(&:first_name) }.to raise_error(access_error) @@ -151,22 +151,22 @@ def name_getter(reason, description) expect { patients.map(&:first_name) }.not_to raise_error end - context 'with targets' do - let(:non_target) { create(:patient_info, first_name: 'Private') } + context "with targets" do + let(:non_target) { create(:patient_info, first_name: "Private") } - it 'allow_phi allows access to all members of a collection' do |t| + it "allow_phi allows access to all members of a collection" do |t| patients.each do |patient| expect { patient.first_name }.to raise_error(access_error) end expect do PatientInfo.allow_phi(file_name, t.full_description, allow_only: patients) do - expect(patients.map(&:first_name)).to contain_exactly('Jay', 'Bob', 'Moe') + expect(patients.map(&:first_name)).to contain_exactly("Jay", "Bob", "Moe") end end.not_to raise_error end - it 'allow_phi does not allow access to non-targets' do |t| + it "allow_phi does not allow access to non-targets" do |t| expect { non_target.first_name }.to raise_error(access_error) expect do @@ -176,8 +176,8 @@ def name_getter(reason, description) end.not_to raise_error end - context 'invalid targets' do - it 'raises exception when targeting an unexpected class' do |t| + context "invalid targets" do + it "raises exception when targeting an unexpected class" do |t| address = create(:address) expect do @@ -187,7 +187,7 @@ def name_getter(reason, description) end.to raise_error(ArgumentError) end - it 'raises exception when given a non-iterable' do |t| + it "raises exception when given a non-iterable" do |t| expect do PatientInfo.allow_phi(file_name, t.full_description, allow_only: jay) do jay.first_name @@ -199,11 +199,11 @@ def name_getter(reason, description) end end - context 'extended authorization' do + context "extended authorization" do let(:patient_mary) { create(:patient_info, :with_multiple_health_records) } - context 'plain access' do - it 'extends access to extended association' do |t| + context "plain access" do + it "extends access to extended association" do |t| expect { patient_mary.first_name }.to raise_error(access_error) expect { patient_mary.patient_detail.detail }.to raise_error(access_error) @@ -211,10 +211,10 @@ def name_getter(reason, description) expect { patient_mary.first_name }.not_to raise_error expect { patient_mary.patient_detail.detail }.not_to raise_error - expect(patient_mary.patient_detail.detail).to eq 'Generic Spell' + expect(patient_mary.patient_detail.detail).to eq "Generic Spell" end - it 'does not extend to unextended association' do |t| + it "does not extend to unextended association" do |t| expect { patient_mary.first_name }.to raise_error(access_error) expect { patient_mary.address.address }.to raise_error(access_error) @@ -224,10 +224,10 @@ def name_getter(reason, description) patient_mary.address.allow_phi!(file_name, t.full_description) expect { patient_mary.address.address }.not_to raise_error - expect(patient_mary.address.address).to eq '123 Little Whinging' + expect(patient_mary.address.address).to eq "123 Little Whinging" end - it 'extends access to :has_many associations' do |t| + it "extends access to :has_many associations" do |t| expect { patient_mary.health_records.first.data }.to raise_error(access_error) patient_mary.allow_phi!(file_name, t.full_description) @@ -235,19 +235,19 @@ def name_getter(reason, description) end end - context 'block access' do - it 'extends access to extended association' do |t| + context "block access" do + it "extends access to extended association" do |t| expect { patient_mary.first_name }.to raise_error(access_error) expect { patient_mary.patient_detail.detail }.to raise_error(access_error) patient_mary.allow_phi(file_name, t.full_description) do expect { patient_mary.first_name }.not_to raise_error expect { patient_mary.patient_detail.detail }.not_to raise_error - expect(patient_mary.patient_detail.detail).to eq('Generic Spell') + expect(patient_mary.patient_detail.detail).to eq("Generic Spell") end end - it 'does not extend to unextended association' do |t| + it "does not extend to unextended association" do |t| expect { patient_mary.first_name }.to raise_error(access_error) expect { patient_mary.address.address }.to raise_error(access_error) @@ -258,10 +258,10 @@ def name_getter(reason, description) patient_mary.address.allow_phi!(file_name, t.full_description) expect { patient_mary.address.address }.not_to raise_error - expect(patient_mary.address.address).to eq('123 Little Whinging') + expect(patient_mary.address.address).to eq("123 Little Whinging") end - it 'extends access to :has_many associations' do |t| + it "extends access to :has_many associations" do |t| expect { patient_mary.health_records.first.data }.to raise_error(access_error) patient_mary.allow_phi(file_name, t.full_description) do @@ -269,10 +269,10 @@ def name_getter(reason, description) end end - it 'revokes access after block' do |t| + it "revokes access after block" do |t| patient_mary.allow_phi(file_name, t.full_description) do expect { patient_mary.patient_detail.detail }.not_to raise_error - expect(patient_mary.patient_detail.detail).to eq('Generic Spell') + expect(patient_mary.patient_detail.detail).to eq("Generic Spell") end expect { patient_mary.first_name }.to raise_error(access_error) @@ -280,7 +280,7 @@ def name_getter(reason, description) expect { patient_mary.health_records.first.data }.to raise_error(access_error) end - it 'does not revoke access for untouched associations' do |t| + it "does not revoke access for untouched associations" do |t| # Here we extend access to two different associations. # When the block terminates, it should revoke (the one frame of) the `health_records` access, # but it should NOT revoke (the only frame of) the `patient_detail` access. @@ -306,8 +306,8 @@ def name_getter(reason, description) end end - context 'nested allowances' do - it 'retains outer access when disallowed at inner level' do |t| + context "nested allowances" do + it "retains outer access when disallowed at inner level" do |t| patient_with_detail.allow_phi(file_name, t.full_description) do expect { patient_with_detail.first_name }.not_to raise_error @@ -324,23 +324,23 @@ def name_getter(reason, description) end end - context 'block checks' do - context 'allow_phi' do - it 'succeeds' do - expect { patient_jane.allow_phi!('ok', 'ok') }.not_to raise_error + context "block checks" do + context "allow_phi" do + it "succeeds" do + expect { patient_jane.allow_phi!("ok", "ok") }.not_to raise_error end - it 'raises ArgumentError with block' do - expect { patient_jane.allow_phi!('ok', 'ok') { do_nothing } }.to raise_error(ArgumentError) + it "raises ArgumentError with block" do + expect { patient_jane.allow_phi!("ok", "ok") { do_nothing } }.to raise_error(ArgumentError) end end - context 'allow_phi!' do - it 'succeeds' do - expect { patient_jane.allow_phi('ok', 'ok') { do_nothing } }.not_to raise_error + context "allow_phi!" do + it "succeeds" do + expect { patient_jane.allow_phi("ok", "ok") { do_nothing } }.not_to raise_error end - it 'raises ArgumentError for allow_phi! without block' do - expect { patient_jane.allow_phi('ok', 'ok') }.to raise_error(ArgumentError) + it "raises ArgumentError for allow_phi! without block" do + expect { patient_jane.allow_phi("ok", "ok") }.to raise_error(ArgumentError) end end end diff --git a/spec/phi_attrs/phi_record/instance__disallow_phi_spec.rb b/spec/phi_attrs/phi_record/instance__disallow_phi_spec.rb index 966fa7e..9c15b71 100644 --- a/spec/phi_attrs/phi_record/instance__disallow_phi_spec.rb +++ b/spec/phi_attrs/phi_record/instance__disallow_phi_spec.rb @@ -1,15 +1,15 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'instance disallow_phi' do +RSpec.describe "instance disallow_phi" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } - let(:patient_john) { build(:patient_info, first_name: 'John') } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_john) { build(:patient_info, first_name: "John") } - context 'block' do - it 'disables all allowances within the block' do |t| + context "block" do + it "disables all allowances within the block" do |t| patient_john.allow_phi!(file_name, t.full_description) expect { patient_john.first_name }.not_to raise_error @@ -18,7 +18,7 @@ end end - it 'returns permission after the block' do |t| + it "returns permission after the block" do |t| patient_john.allow_phi!(file_name, t.full_description) expect { patient_john.first_name }.not_to raise_error @@ -29,7 +29,7 @@ expect { patient_john.first_name }.not_to raise_error end - it 'allows other patient access' do |t| + it "allows other patient access" do |t| patient_john.allow_phi!(file_name, t.full_description) patient_jane.allow_phi!(file_name, t.full_description) expect { patient_john.first_name }.not_to raise_error @@ -44,13 +44,13 @@ expect { patient_jane.first_name }.not_to raise_error end - it 'raises ArgumentError without block' do + it "raises ArgumentError without block" do expect { patient_john.disallow_phi }.to raise_error(ArgumentError) end end - context 'disallow_phi!' do - it 'disallows whole stack' do |t| + context "disallow_phi!" do + it "disallows whole stack" do |t| patient_john.allow_phi!("#{file_name}1", t.full_description) expect { patient_john.first_name }.not_to raise_error patient_john.allow_phi!("#{file_name}2", t.full_description) @@ -59,7 +59,7 @@ expect { patient_john.first_name }.to raise_error(access_error) end - it 'disallows does not affect Class allows' do |t| + it "disallows does not affect Class allows" do |t| PatientInfo.allow_phi!(file_name, t.full_description) expect { patient_john.first_name }.not_to raise_error patient_john.allow_phi!("#{file_name}2", t.full_description) @@ -68,7 +68,7 @@ expect { patient_john.first_name }.not_to raise_error end - it 'allows access after disallow' do |t| + it "allows access after disallow" do |t| patient_john.allow_phi!("#{file_name}1", t.full_description) expect { patient_john.first_name }.not_to raise_error patient_john.disallow_phi! @@ -77,7 +77,7 @@ expect { patient_john.first_name }.not_to raise_error end - it 'raises ArgumentError with block' do + it "raises ArgumentError with block" do expect { patient_john.disallow_phi! { do_nothing } }.to raise_error(ArgumentError) end end diff --git a/spec/phi_attrs/phi_record/instance__phi_allowed_spec.rb b/spec/phi_attrs/phi_record/instance__phi_allowed_spec.rb index 2b03f87..0d9068f 100644 --- a/spec/phi_attrs/phi_record/instance__phi_allowed_spec.rb +++ b/spec/phi_attrs/phi_record/instance__phi_allowed_spec.rb @@ -1,19 +1,19 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'instance phi_allowed?' do +RSpec.describe "instance phi_allowed?" do file_name = __FILE__ - let(:patient_john) { build(:patient_info, first_name: 'John') } - let(:patient_jane) { build(:patient_info, first_name: 'Jane') } + let(:patient_john) { build(:patient_info, first_name: "John") } + let(:patient_jane) { build(:patient_info, first_name: "Jane") } let(:patient_detail) { build(:patient_detail) } - let(:patient_with_detail) { build(:patient_info, first_name: 'Jack', patient_detail: patient_detail) } + let(:patient_with_detail) { build(:patient_info, first_name: "Jack", patient_detail: patient_detail) } - context 'with instance allow_phi' do - context 'authorized' do - context 'single record' do - it 'allows access to an authorized instance' do |t| + context "with instance allow_phi" do + context "authorized" do + context "single record" do + it "allows access to an authorized instance" do |t| expect(patient_jane.phi_allowed?).to be false patient_jane.allow_phi(file_name, t.full_description) do @@ -27,7 +27,7 @@ expect(patient_jane.phi_allowed?).to be true end - it 'only allows access to the authorized instance' do |t| + it "only allows access to the authorized instance" do |t| patient_jane.allow_phi(file_name, t.full_description) do expect(patient_jane.phi_allowed?).to be true expect(patient_john.phi_allowed?).to be false @@ -39,7 +39,7 @@ expect(patient_john.phi_allowed?).to be false end - it 'revokes access after calling disallow_phi!' do |t| + it "revokes access after calling disallow_phi!" do |t| expect(patient_jane.phi_allowed?).to be false patient_jane.allow_phi!(file_name, t.full_description) @@ -51,8 +51,8 @@ expect(patient_jane.phi_allowed?).to be false end - it 'allows access on an instance that already exists' do |t| - john = create(:patient_info, first_name: 'John') + it "allows access on an instance that already exists" do |t| + john = create(:patient_info, first_name: "John") expect(john.phi_allowed?).to be false john_id = john.id @@ -64,7 +64,7 @@ expect(john.phi_allowed?).to be true end - it 'revokes access for disallow_phi block' do |t| + it "revokes access for disallow_phi block" do |t| expect(patient_jane.phi_allowed?).to be false patient_jane.allow_phi!(file_name, t.full_description) @@ -79,13 +79,13 @@ end end - context 'collection' do - let(:jay) { create(:patient_info, first_name: 'Jay') } - let(:bob) { create(:patient_info, first_name: 'Bob') } - let(:moe) { create(:patient_info, first_name: 'Moe') } + context "collection" do + let(:jay) { create(:patient_info, first_name: "Jay") } + let(:bob) { create(:patient_info, first_name: "Bob") } + let(:moe) { create(:patient_info, first_name: "Moe") } let(:patients) { [jay, bob, moe] } - it 'allows access when fetched as a collection' do |t| + it "allows access when fetched as a collection" do |t| expect(patients).to contain_exactly(jay, bob, moe) expect(patients.map(&:phi_allowed?)).to contain_exactly(false, false, false) @@ -93,10 +93,10 @@ expect(patients.map(&:phi_allowed?)).to contain_exactly(true, true, true) end - context 'with targets' do - let(:non_target) { create(:patient_info, first_name: 'Private') } + context "with targets" do + let(:non_target) { create(:patient_info, first_name: "Private") } - it 'allow_phi allows access to all members of a collection' do |t| + it "allow_phi allows access to all members of a collection" do |t| patients.each do |patient| expect(patient.phi_allowed?).to be false end @@ -108,7 +108,7 @@ end.not_to raise_error end - it 'allow_phi does not allow access to non-targets' do |t| + it "allow_phi does not allow access to non-targets" do |t| expect(non_target.phi_allowed?).to be false expect do @@ -121,11 +121,11 @@ end end - context 'extended authorization' do + context "extended authorization" do let(:patient_mary) { create(:patient_info, :with_multiple_health_records) } - context 'plain access' do - it 'extends access to extended association' do |t| + context "plain access" do + it "extends access to extended association" do |t| expect(patient_mary.phi_allowed?).to be false expect(patient_mary.patient_detail.phi_allowed?).to be false @@ -135,7 +135,7 @@ expect(patient_mary.patient_detail.phi_allowed?).to be true end - it 'does not extend to unextended association' do |t| + it "does not extend to unextended association" do |t| expect(patient_mary.phi_allowed?).to be false expect(patient_mary.address.phi_allowed?).to be false @@ -147,7 +147,7 @@ expect(patient_mary.address.phi_allowed?).to be true end - it 'extends access to :has_many associations' do |t| + it "extends access to :has_many associations" do |t| expect(patient_mary.health_records.first.phi_allowed?).to be false patient_mary.allow_phi!(file_name, t.full_description) @@ -155,8 +155,8 @@ end end - context 'block access' do - it 'extends access to extended association' do |t| + context "block access" do + it "extends access to extended association" do |t| expect(patient_mary.phi_allowed?).to be false expect(patient_mary.patient_detail.phi_allowed?).to be false @@ -166,7 +166,7 @@ end end - it 'does not extend to unextended association' do |t| + it "does not extend to unextended association" do |t| expect(patient_mary.phi_allowed?).to be false expect(patient_mary.address.phi_allowed?).to be false @@ -179,7 +179,7 @@ expect(patient_mary.address.phi_allowed?).to be true end - it 'extends access to :has_many associations' do |t| + it "extends access to :has_many associations" do |t| expect(patient_mary.health_records.first.phi_allowed?).to be false patient_mary.allow_phi(file_name, t.full_description) do @@ -187,7 +187,7 @@ end end - it 'revokes access after block' do |t| + it "revokes access after block" do |t| patient_mary.allow_phi(file_name, t.full_description) do expect(patient_mary.patient_detail.phi_allowed?).to be true end @@ -197,7 +197,7 @@ expect(patient_mary.health_records.first.phi_allowed?).to be false end - it 'does not revoke access for untouched associations' do |t| + it "does not revoke access for untouched associations" do |t| # Here we extend access to two different associations. # When the block terminates, it should revoke (the one frame of) the `health_records` access, # but it should NOT revoke (the only frame of) the `patient_detail` access. @@ -222,8 +222,8 @@ end end - context 'block disallow' do - it 'disallows access to extended association' do |t| + context "block disallow" do + it "disallows access to extended association" do |t| expect(patient_mary.phi_allowed?).to be false expect(patient_mary.patient_detail.phi_allowed?).to be false @@ -238,7 +238,7 @@ end end - it 'disallows access to :has_many associations' do |t| + it "disallows access to :has_many associations" do |t| expect(patient_mary.health_records.first.phi_allowed?).to be false patient_mary.allow_phi!(file_name, t.full_description) @@ -252,8 +252,8 @@ end end - context 'nested allowances' do - it 'retains outer access when disallowed at inner level' do |t| + context "nested allowances" do + it "retains outer access when disallowed at inner level" do |t| patient_with_detail.allow_phi(file_name, t.full_description) do expect(patient_with_detail.phi_allowed?).to be true @@ -270,7 +270,7 @@ end end - it 'retains outer access when disallow block at inner level' do |t| + it "retains outer access when disallow block at inner level" do |t| patient_jane.allow_phi(file_name, t.full_description) do expect(patient_jane.phi_allowed?).to be true @@ -284,7 +284,7 @@ expect(patient_jane.phi_allowed?).to be false end - it 'retains outer access with nested disallow blocks' do |t| + it "retains outer access with nested disallow blocks" do |t| patient_jane.allow_phi!(file_name, t.full_description) patient_jane.disallow_phi do @@ -301,9 +301,9 @@ end end - context 'with class allow_phi' do - context 'authorized' do - it 'allows access to any instance' do |t| + context "with class allow_phi" do + context "authorized" do + it "allows access to any instance" do |t| expect(patient_jane.phi_allowed?).to be false PatientInfo.allow_phi(file_name, t.full_description) do expect(patient_jane.phi_allowed?).to be true @@ -313,7 +313,7 @@ expect(patient_jane.phi_allowed?).to be true end - it 'only allows for the authorized class' do |t| + it "only allows for the authorized class" do |t| expect(patient_detail.phi_allowed?).to be false expect(patient_jane.phi_allowed?).to be false @@ -331,7 +331,7 @@ expect(patient_jane.phi_allowed?).to be true end - it 'revokes access after calling disallow_phi!' do |t| + it "revokes access after calling disallow_phi!" do |t| expect(patient_jane.phi_allowed?).to be false PatientInfo.allow_phi!(file_name, t.full_description) @@ -343,7 +343,7 @@ expect(patient_jane.phi_allowed?).to be false end - it 'disallow_phi does not change status' do |t| + it "disallow_phi does not change status" do |t| expect(patient_jane.phi_allowed?).to be false patient_jane.allow_phi!(file_name, t.full_description) expect(patient_jane.phi_allowed?).to be true @@ -354,10 +354,10 @@ end end - context 'extended authorization' do + context "extended authorization" do let(:patient_mary) { create(:patient_info, :with_multiple_health_records) } - it 'does not revoke access for untouched associations' do |t| + it "does not revoke access for untouched associations" do |t| # Here we extend access to two different associations. # When the block terminates, it should revoke (the one frame of) the `health_records` access, # but it should NOT revoke (the only frame of) the `patient_detail` access. @@ -383,8 +383,8 @@ end end - context 'nested allowances' do - it 'retains outer access when disallowed at inner level' do |t| + context "nested allowances" do + it "retains outer access when disallowed at inner level" do |t| PatientInfo.allow_phi(file_name, t.full_description) do expect(patient_with_detail.phi_allowed?).to be true diff --git a/spec/phi_attrs/phi_record/phi_wrapping_spec.rb b/spec/phi_attrs/phi_record/phi_wrapping_spec.rb index f2a6dae..de16c30 100644 --- a/spec/phi_attrs/phi_record/phi_wrapping_spec.rb +++ b/spec/phi_attrs/phi_record/phi_wrapping_spec.rb @@ -1,17 +1,17 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" -RSpec.describe 'phi_wrapping' do +RSpec.describe "phi_wrapping" do let(:missing_attribute_model) { build(:missing_attribute_model) } let(:missing_extend_model) { build(:missing_extend_model) } - context 'non existent attributes' do - it 'wrapping a method' do + context "non existent attributes" do + it "wrapping a method" do expect { missing_attribute_model }.not_to raise_error end - it 'extending a model' do + it "extending a model" do expect { missing_extend_model }.to raise_error(NameError) end end diff --git a/spec/phi_attrs/version_spec.rb b/spec/phi_attrs/version_spec.rb index 83d0fbc..08b25d4 100644 --- a/spec/phi_attrs/version_spec.rb +++ b/spec/phi_attrs/version_spec.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true -require 'spec_helper' +require "spec_helper" RSpec.describe PhiAttrs do - context 'gem' do - it 'has a version number' do + context "gem" do + it "has a version number" do expect(PhiAttrs::VERSION).not_to be nil end end diff --git a/spec/phi_attrs_spec.rb b/spec/phi_attrs_spec.rb index 2777a80..33390d9 100644 --- a/spec/phi_attrs_spec.rb +++ b/spec/phi_attrs_spec.rb @@ -3,7 +3,7 @@ # Try to spread out test from all running in one large file, # but if there isn't a good spot put it here for now. RSpec.describe PhiAttrs do - context 'global' do + context "global" do # Nothing here for now. end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 73b2acb..034c4c1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,27 +1,27 @@ # frozen_string_literal: true -require 'simplecov' +require "simplecov" SimpleCov.start -require 'rails/all' -require 'dummy/application' +require "rails/all" +require "dummy/application" -require 'byebug' -require 'rspec/rails' -require 'factory_bot_rails' -require 'faker' -require 'phi_attrs' +require "debug" +require "rspec/rails" +require "factory_bot_rails" +require "faker" +require "phi_attrs" Dummy::Application.initialize! # Adds all support files -Dir[File.join(Gem::Specification.find_by_name('phi_attrs').gem_dir.to_s, 'spec', 'support', '**', '*.rb')].sort.each { |f| require f } +Dir[File.join(Gem::Specification.find_by_name("phi_attrs").gem_dir.to_s, "spec", "support", "**", "*.rb")].sort.each { |f| require f } RSpec.configure do |config| config.use_transactional_fixtures = true # Enable flags like --only-failures and --next-failure - config.example_status_persistence_file_path = '.rspec_status' + config.example_status_persistence_file_path = ".rspec_status" # Disable RSpec exposing methods globally on `Module` and `main` config.disable_monkey_patching! From 42ae156d3d335f7ba3ea6e35352acfbfbd16a17a Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:27:28 -0400 Subject: [PATCH 02/11] security: sanitize log inputs and document send usage --- lib/phi_attrs.rb | 2 ++ lib/phi_attrs/phi_record.rb | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/lib/phi_attrs.rb b/lib/phi_attrs.rb index c4d3e44..db8b72b 100644 --- a/lib/phi_attrs.rb +++ b/lib/phi_attrs.rb @@ -41,6 +41,8 @@ def record_i18n_data return if PhiAttrs.current_user_method.nil? return unless respond_to?(PhiAttrs.current_user_method, true) + # send intentional: current_user_method is developer-configured, not user input, + # and Rails controller helpers are conventionally private RequestStore.store[:phi_attrs_current_user] = send(PhiAttrs.current_user_method) end end diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 584bde5..789ba5c 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -77,6 +77,9 @@ def allow_phi!(user_id = nil, reason = nil) reason ||= i18n_reason raise ArgumentError, "user_id and reason cannot be blank" if user_id.blank? || reason.blank? + user_id = user_id.to_s.gsub(/[\r\n]/, " ") + reason = reason.to_s.gsub(/[\r\n]/, " ") + __phi_stack.push({ phi_access_allowed: true, user_id: user_id, @@ -335,6 +338,9 @@ def allow_phi!(user_id = nil, reason = nil) reason ||= self.class.i18n_reason raise ArgumentError, "user_id and reason cannot be blank" if user_id.blank? || reason.blank? + user_id = user_id.to_s.gsub(/[\r\n]/, " ") + reason = reason.to_s.gsub(/[\r\n]/, " ") + PhiAttrs::Logger.tagged(*phi_log_keys) do @__phi_access_stack.push({ phi_access_allowed: true, From 9b3a02fec95447e16235e2e8e1fc835e89bde7cf Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:28:43 -0400 Subject: [PATCH 03/11] fix: exception safety, railtie hooks, class_attribute mutation, dead references --- lib/phi_attrs.rb | 2 +- lib/phi_attrs/phi_record.rb | 30 +++++++++---------- lib/phi_attrs/railtie.rb | 6 ++-- .../20170214100255_create_patient_infos.rb | 2 +- spec/dummy/db/schema.rb | 16 +++++----- 5 files changed, 27 insertions(+), 29 deletions(-) diff --git a/lib/phi_attrs.rb b/lib/phi_attrs.rb index db8b72b..db79176 100644 --- a/lib/phi_attrs.rb +++ b/lib/phi_attrs.rb @@ -20,7 +20,7 @@ def self.log_phi_access(user, message) end module Model - def phi_model(with: nil, except: nil) + def phi_model include PhiRecord end end diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 789ba5c..ff8c550 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -196,13 +196,12 @@ def get_phi(user_id = nil, reason = nil, allow_only: nil) def disallow_phi raise ArgumentError, "block required. use disallow_phi! without block" unless block_given? - __phi_stack.push({ - phi_access_allowed: false - }) - - yield if block_given? - - __phi_stack.pop + __phi_stack.push({phi_access_allowed: false}) + begin + yield + ensure + __phi_stack.pop + end end # Revoke all PHI access for this class, if enabled by PhiRecord#allow_phi! @@ -441,11 +440,12 @@ def disallow_phi add_disallow_flag! add_disallow_flag_to_extended_phi! - - yield if block_given? - - remove_disallow_flag_from_extended_phi! - remove_disallow_flag! + begin + yield + ensure + remove_disallow_flag_from_extended_phi! + remove_disallow_flag! + end end # Revoke last PHI access for a single instance of this class. @@ -642,7 +642,7 @@ def phi_wrap_method(method_name) self.class.send(:define_method, wrapped_method) do |*args, **kwargs, &block| PhiAttrs::Logger.tagged(*phi_log_keys) do unless phi_allowed? - raise PhiAttrs::Exceptions::PhiAccessException, "Attempted PHI access for #{self.class.name} #{@__phi_user_id}" + raise PhiAttrs::Exceptions::PhiAccessException, "Attempted PHI access for #{self.class.name} (#{phi_log_keys.last})" end unless all_phi_context_logged? @@ -658,7 +658,7 @@ def phi_wrap_method(method_name) self.class.send(:alias_method, unwrapped_method, method_name) self.class.send(:alias_method, method_name, wrapped_method) - self.class.__phi_methods_wrapped << method_name + self.class.__phi_methods_wrapped = self.class.__phi_methods_wrapped + [method_name] end # Core logic for wrapping methods in PHI access extensions. Almost @@ -694,7 +694,7 @@ def phi_wrap_extension(method_name) self.class.send(:alias_method, unwrapped_method, method_name) self.class.send(:alias_method, method_name, wrapped_method) - self.class.__phi_methods_to_extend << method_name + self.class.__phi_methods_to_extend = self.class.__phi_methods_to_extend + [method_name] end # Revoke PHI access for all `extend`ed relations (or only those given) diff --git a/lib/phi_attrs/railtie.rb b/lib/phi_attrs/railtie.rb index dc9aaec..2cdd661 100644 --- a/lib/phi_attrs/railtie.rb +++ b/lib/phi_attrs/railtie.rb @@ -6,10 +6,8 @@ module PhiAttrs class Railtie < Rails::Railtie initializer "phi_attrs.initialize" do |_app| - ActiveSupport.on_load(:active_record) do - ActiveSupport.on_load(:active_record) { extend PhiAttrs::Model } - ActiveSupport.on_load(:action_controller) { include PhiAttrs::Controller } - end + ActiveSupport.on_load(:active_record) { extend PhiAttrs::Model } + ActiveSupport.on_load(:action_controller) { include PhiAttrs::Controller } end end end diff --git a/spec/dummy/db/migrate/20170214100255_create_patient_infos.rb b/spec/dummy/db/migrate/20170214100255_create_patient_infos.rb index d9abdd3..690c4a6 100644 --- a/spec/dummy/db/migrate/20170214100255_create_patient_infos.rb +++ b/spec/dummy/db/migrate/20170214100255_create_patient_infos.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class CreatePatientInfos < ActiveRecord::Migration[5.0] +class CreatePatientInfos < ActiveRecord::Migration[7.0] def change create_table :patient_infos do |t| t.string :first_name diff --git a/spec/dummy/db/schema.rb b/spec/dummy/db/schema.rb index 4f49904..24eb1bd 100755 --- a/spec/dummy/db/schema.rb +++ b/spec/dummy/db/schema.rb @@ -24,20 +24,20 @@ end create_table "missing_attribute_models", force: :cascade do |t| - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end create_table "missing_extend_models", force: :cascade do |t| - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end create_table "patient_details", force: :cascade do |t| t.integer "patient_info_id" t.string "detail" - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false t.index ["patient_info_id"], name: "index_patient_details_on_patient_info_id" end @@ -45,7 +45,7 @@ t.string "first_name" t.string "last_name" t.string "public_id" - t.datetime "created_at", precision: nil, null: false - t.datetime "updated_at", precision: nil, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end end From 98a3669c1fdce6865a83b610c4cec11bfed88834 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:29:09 -0400 Subject: [PATCH 04/11] chore: replace class variables with mattr_accessor, scope FORMAT constant --- lib/phi_attrs/configure.rb | 50 ++++---------------------------------- lib/phi_attrs/formatter.rb | 4 +-- 2 files changed, 7 insertions(+), 47 deletions(-) diff --git a/lib/phi_attrs/configure.rb b/lib/phi_attrs/configure.rb index ad1f172..ee10aa0 100644 --- a/lib/phi_attrs/configure.rb +++ b/lib/phi_attrs/configure.rb @@ -1,53 +1,13 @@ # frozen_string_literal: true module PhiAttrs - @@log_path = nil - @@log_shift_age = 0 # Default to disabled - @@log_shift_size = 1_048_576 # 1MB - Default from logger class - @@current_user_method = nil - @@translation_prefix = "phi" + mattr_accessor :log_path, default: nil + mattr_accessor :log_shift_age, default: 0 + mattr_accessor :log_shift_size, default: 1_048_576 + mattr_accessor :current_user_method, default: nil + mattr_accessor :translation_prefix, default: "phi" def self.configure yield self if block_given? end - - def self.log_path - @@log_path - end - - def self.log_path=(value) - @@log_path = value - end - - def self.log_shift_age - @@log_shift_age - end - - def self.log_shift_age=(value) - @@log_shift_age = value - end - - def self.log_shift_size - @@log_shift_size - end - - def self.log_shift_size=(value) - @@log_shift_size = value - end - - def self.translation_prefix - @@translation_prefix - end - - def self.translation_prefix=(value) - @@translation_prefix = value - end - - def self.current_user_method - @@current_user_method - end - - def self.current_user_method=(value) - @@current_user_method = value - end end diff --git a/lib/phi_attrs/formatter.rb b/lib/phi_attrs/formatter.rb index 6658792..cf0ae14 100644 --- a/lib/phi_attrs/formatter.rb +++ b/lib/phi_attrs/formatter.rb @@ -1,10 +1,10 @@ # frozen_string_literal: true module PhiAttrs - FORMAT = "%s %5s: %s\n" - # https://github.com/ruby/ruby/blob/trunk/lib/logger.rb#L587 class Formatter < ::Logger::Formatter + FORMAT = "%s %5s: %s\n" + def call(severity, timestamp, _progname, msg) format(FORMAT, format_datetime(timestamp), severity, msg2str(msg)) end From 599b9110e73e750f4b708950b7226d8bc7d3165e Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:31:22 -0400 Subject: [PATCH 05/11] perf: move method wrapping to class level, remove dead instance variable --- lib/phi_attrs/phi_record.rb | 232 +++++++++++++----------------------- 1 file changed, 84 insertions(+), 148 deletions(-) diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index ff8c550..80d7392 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -14,14 +14,11 @@ module PhiRecord class_attribute :__phi_exclude_methods class_attribute :__phi_include_methods class_attribute :__phi_extend_methods - class_attribute :__phi_methods_wrapped - class_attribute :__phi_methods_to_extend + class_attribute :__phi_methods_wrapped, default: [] + class_attribute :__phi_methods_to_extend, default: [] + class_attribute :__phi_class_wrapped, default: false - after_initialize :wrap_phi - - # These have to default to an empty array - self.__phi_methods_wrapped = [] - self.__phi_methods_to_extend = [] + after_initialize :__phi_init end class_methods do @@ -300,25 +297,85 @@ def __path_to_class class_name_parts = module_paths.pop.split("_") module_paths.push(class_name_parts.join("_")) end - end - # Get all method names to be wrapped with PHI access logging - # - # @return [Array] the method names to be wrapped with PHI access logging - # - def __phi_wrapped_methods - excluded_methods = self.class.__phi_exclude_methods.to_a - included_methods = self.class.__phi_include_methods.to_a + def __computed_phi_wrapped_methods + @__computed_phi_wrapped_methods ||= begin + excluded = __phi_exclude_methods.to_a + included = __phi_include_methods.to_a + attribute_names - excluded + included - [primary_key] + end + end - attribute_names - excluded_methods + included_methods - [self.class.primary_key] - end + def __computed_phi_extended_methods + @__computed_phi_extended_methods ||= __phi_extend_methods.to_a + end - # Get all method names to be wrapped with PHI access extension - # - # @return [Array] the method names to be wrapped with PHI access extension - # - def __phi_extended_methods - self.class.__phi_extend_methods.to_a + def __phi_wrap_class_methods!(instance) + return if __phi_class_wrapped + + __computed_phi_wrapped_methods.each { |m| __phi_wrap_method!(m, instance) } + __computed_phi_extended_methods.each { |m| __phi_wrap_extension!(m, instance) } + self.__phi_class_wrapped = true + end + + def __phi_wrap_method!(method_name, instance) + unless instance.respond_to?(method_name) + PhiAttrs::Logger.warn("#{name} tried to wrap non-existent method (#{method_name})") + return + end + return if __phi_methods_wrapped.include?(method_name) + + wrapped_method = :"__#{method_name}_phi_wrapped" + unwrapped_method = :"__#{method_name}_phi_unwrapped" + + define_method(wrapped_method) do |*args, **kwargs, &block| + PhiAttrs::Logger.tagged(*phi_log_keys) do + unless phi_allowed? + raise PhiAttrs::Exceptions::PhiAccessException, "Attempted PHI access for #{self.class.name} (#{phi_log_keys.last})" + end + + unless all_phi_context_logged? + PhiAttrs::Logger.info("#{self.class.name} access by [#{all_phi_allowed_by}]. Triggered by method: #{method_name}") + set_all_phi_context_logged + end + + send(unwrapped_method, *args, **kwargs, &block) + end + end + + alias_method unwrapped_method, method_name + alias_method method_name, wrapped_method + + self.__phi_methods_wrapped = __phi_methods_wrapped + [method_name] + end + + def __phi_wrap_extension!(method_name, instance) + raise NameError, "Undefined relationship in `extend_phi_access`: #{method_name}" unless instance.respond_to?(method_name) + return if __phi_methods_to_extend.include?(method_name) + + wrapped_method = :"__#{method_name}_phi_access_extended" + unwrapped_method = :"__#{method_name}_phi_access_original" + + define_method(wrapped_method) do |*args, **kwargs, &block| + relation = send(unwrapped_method, *args, **kwargs, &block) + + if phi_allowed? && relation.present? && relation_klass(relation).included_modules.include?(PhiRecord) + relations = relation.is_a?(Enumerable) ? relation : [relation] + relations.each do |r| + r.allow_phi!(phi_allowed_by, phi_access_reason) unless @__phi_relations_extended.include?(r) + end + @__phi_relations_extended.merge(relations) + self.class.__instances_with_extended_phi.add(self) + end + + relation + end + + alias_method unwrapped_method, method_name + alias_method method_name, wrapped_method + + self.__phi_methods_to_extend = __phi_methods_to_extend + [method_name] + end end # Enable PHI access for a single instance of this class. @@ -531,22 +588,6 @@ def remove_disallow_flag! private - # Entry point for wrapping methods with PHI access logging. This is called - # by an `after_initialize` hook from ActiveRecord. - # - # @private - # - def wrap_phi - # Disable PHI access by default - @__phi_access_stack = [] - @__phi_methods_extended = Set.new - @__phi_relations_extended = Set.new - - # Wrap attributes with PHI Logger and Access Control - __phi_wrapped_methods.each { |m| phi_wrap_method(m) } - __phi_extended_methods.each { |m| phi_wrap_extension(m) } - end - # Log Key for an instance of this class. If the instance is persisted in the # database, then it is the primary key; otherwise it is the Ruby object_id # in memory. @@ -597,107 +638,12 @@ def set_all_phi_context_logged all_phi_context.each { |c| c[:logged] = true } end - # Core logic for wrapping methods in PHI access logging and access restriction. - # - # This method takes a single method name, and creates a new method using - # define_method; once this method is defined, the original method name - # is aliased to the new method, and the original method is renamed to a - # known key. - # - # @private - # - # @example - # Foo::phi_wrap_method(:bar) - # - # foo = Foo.find(1) - # foo.bar # => raises PHI Access Exception - # - # foo.allow_phi!('user@example.com', 'testing') - # - # foo.bar # => returns original value of Foo#bar - # - # # defines two new methods: - # # __bar_phi_wrapped - # # __bar_phi_unwrapped - # # - # # After these methods are defined - # # an alias chain is created that - # # roughly maps: - # # - # # bar => __bar_phi_wrapped => __bar_phi_unwrapped - # # - # # This ensures that all calls to Foo#bar pass - # # through access logging. - # - def phi_wrap_method(method_name) - unless respond_to?(method_name) - PhiAttrs::Logger.warn("#{self.class.name} tried to wrap non-existent method (#{method_name})") - return - end - return if self.class.__phi_methods_wrapped.include? method_name - - wrapped_method = :"__#{method_name}_phi_wrapped" - unwrapped_method = :"__#{method_name}_phi_unwrapped" - - self.class.send(:define_method, wrapped_method) do |*args, **kwargs, &block| - PhiAttrs::Logger.tagged(*phi_log_keys) do - unless phi_allowed? - raise PhiAttrs::Exceptions::PhiAccessException, "Attempted PHI access for #{self.class.name} (#{phi_log_keys.last})" - end - - unless all_phi_context_logged? - PhiAttrs::Logger.info("#{self.class.name} access by [#{all_phi_allowed_by}]. Triggered by method: #{method_name}") - set_all_phi_context_logged - end - - send(unwrapped_method, *args, **kwargs, &block) - end - end - - # method_name => wrapped_method => unwrapped_method - self.class.send(:alias_method, unwrapped_method, method_name) - self.class.send(:alias_method, method_name, wrapped_method) - - self.class.__phi_methods_wrapped = self.class.__phi_methods_wrapped + [method_name] - end - - # Core logic for wrapping methods in PHI access extensions. Almost - # functionally equivalent to the phi_wrap_method call above, - # this method doesn't add any logging or access restriction, but - # simply proxies the PhiRecord#allow_phi! call. - # - # @private - # - def phi_wrap_extension(method_name) - raise NameError, "Undefined relationship in `extend_phi_access`: #{method_name}" unless respond_to?(method_name) - return if self.class.__phi_methods_to_extend.include? method_name - - wrapped_method = wrapped_extended_name(method_name) - unwrapped_method = unwrapped_extended_name(method_name) - - self.class.send(:define_method, wrapped_method) do |*args, **kwargs, &block| - relation = send(unwrapped_method, *args, **kwargs, &block) - - if phi_allowed? && relation.present? && relation_klass(relation).included_modules.include?(PhiRecord) - relations = relation.is_a?(Enumerable) ? relation : [relation] - relations.each do |r| - r.allow_phi!(phi_allowed_by, phi_access_reason) unless @__phi_relations_extended.include?(r) - end - @__phi_relations_extended.merge(relations) - self.class.__instances_with_extended_phi.add(self) - end - - relation - end - - # method_name => wrapped_method => unwrapped_method - self.class.send(:alias_method, unwrapped_method, method_name) - self.class.send(:alias_method, method_name, wrapped_method) - - self.class.__phi_methods_to_extend = self.class.__phi_methods_to_extend + [method_name] + def __phi_init + @__phi_access_stack = [] + @__phi_relations_extended = Set.new + self.class.__phi_wrap_class_methods!(self) end - # Revoke PHI access for all `extend`ed relations (or only those given) def revoke_extended_phi!(relations = nil) relations ||= @__phi_relations_extended relations.each do |relation| @@ -706,7 +652,6 @@ def revoke_extended_phi!(relations = nil) @__phi_relations_extended.subtract(relations) end - # Adds a disallow PHI access to the stack for block syntax for all `extend`ed relations (or only those given) def add_disallow_flag_to_extended_phi!(relations = nil) relations ||= @__phi_relations_extended relations.each do |relation| @@ -714,7 +659,6 @@ def add_disallow_flag_to_extended_phi!(relations = nil) end end - # Adds a disallow PHI access to the stack for all for all `extend`ed relations (or only those given) def remove_disallow_flag_from_extended_phi!(relations = nil) relations ||= @__phi_relations_extended relations.each do |relation| @@ -728,13 +672,5 @@ def relation_klass(rel) rel.class end - - def wrapped_extended_name(method_name) - :"__#{method_name}_phi_access_extended" - end - - def unwrapped_extended_name(method_name) - :"__#{method_name}_phi_access_original" - end end end From cb30efbbbce56478b62348979a22b05163c50651 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:31:50 -0400 Subject: [PATCH 06/11] perf: cache phi_log_keys, eliminate per-access array allocations --- lib/phi_attrs/phi_record.rb | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 80d7392..a6b1fee 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -600,8 +600,13 @@ def remove_disallow_flag! # @return [Array] log key for an instance of this class # def phi_log_keys - @__phi_log_id = persisted? ? "Key: #{attributes[self.class.primary_key]}" : "Object: #{object_id}" - @__phi_log_keys = [PHI_ACCESS_LOG_TAG, self.class.name, @__phi_log_id] + current_id = persisted? ? id : object_id + if @__phi_log_cached_id != current_id + @__phi_log_cached_id = current_id + prefix = persisted? ? "Key: #{current_id}" : "Object: #{current_id}" + @__phi_log_keys = [PHI_ACCESS_LOG_TAG, self.class.name, prefix].freeze + end + @__phi_log_keys end def phi_context @@ -623,19 +628,19 @@ def class_phi_context # @return String of all the user_id's passed in to allow_phi! # def all_phi_allowed_by - self.class.__user_id_string(all_phi_context) - end - - def all_phi_context - (@__phi_access_stack || []) + (self.class.__phi_stack || []) + ids = (@__phi_access_stack || []).filter_map { |c| "'#{c[:user_id]}'" } + ids.concat(self.class.__phi_stack.filter_map { |c| "'#{c[:user_id]}'" }) + ids.join(",") end def all_phi_context_logged? - all_phi_context.all? { |v| v[:logged] } + (@__phi_access_stack.nil? || @__phi_access_stack.all? { |v| v[:logged] }) && + self.class.__phi_stack.all? { |v| v[:logged] } end def set_all_phi_context_logged - all_phi_context.each { |c| c[:logged] = true } + @__phi_access_stack&.each { |c| c[:logged] = true } + self.class.__phi_stack.each { |c| c[:logged] = true } end def __phi_init From 2eb8f5720545d418d7a6f5638800e826ef32cb45 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:44:28 -0400 Subject: [PATCH 07/11] test: add missing specs for security fixes, bug fixes, and edge cases - exception_safety_spec: disallow_phi/allow_phi/get_phi block exception safety (class + instance) - require_phi_spec: PhiAccessException raised/not-raised under class and instance access - disallow_last_phi_spec: pops most recent allow, no-op on empty stack - phi_allowed_by_spec: nil when no access, correct user_id/reason, stacked allows, class fallback - log_sanitization_spec: newlines stripped from user_id and reason at allow_phi! entry - request_isolation_spec: class allow cleared by RequestStore.clear!, instance stack survives, extended instances cleared - rspec_matcher_spec: allow_phi_access positive/negative, allowed_by/with_access_reason chains, negated raises ArgumentError Also fix PhiAccessException unqualified constant in require_phi! (was PhiAccessException, now PhiAttrs::Exceptions::PhiAccessException) --- lib/phi_attrs/phi_record.rb | 6 +- spec/phi_attrs/disallow_last_phi_spec.rb | 23 ++++++ spec/phi_attrs/exception_safety_spec.rb | 100 +++++++++++++++++++++++ spec/phi_attrs/log_sanitization_spec.rb | 23 ++++++ spec/phi_attrs/phi_allowed_by_spec.rb | 47 +++++++++++ spec/phi_attrs/request_isolation_spec.rb | 43 ++++++++++ spec/phi_attrs/require_phi_spec.rb | 22 +++++ spec/phi_attrs/rspec_matcher_spec.rb | 60 ++++++++++++++ 8 files changed, 321 insertions(+), 3 deletions(-) create mode 100644 spec/phi_attrs/disallow_last_phi_spec.rb create mode 100644 spec/phi_attrs/exception_safety_spec.rb create mode 100644 spec/phi_attrs/log_sanitization_spec.rb create mode 100644 spec/phi_attrs/phi_allowed_by_spec.rb create mode 100644 spec/phi_attrs/request_isolation_spec.rb create mode 100644 spec/phi_attrs/require_phi_spec.rb create mode 100644 spec/phi_attrs/rspec_matcher_spec.rb diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index a6b1fee..4b3ab45 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -529,7 +529,7 @@ def disallow_last_phi!(preserve_extensions: false) # @return [String] the user_id passed in to allow_phi! # def phi_allowed_by - phi_context[:user_id] + phi_context&.dig(:user_id) end # The access reason for allowing access to this instance. @@ -538,7 +538,7 @@ def phi_allowed_by # @return [String] the reason passed in to allow_phi! # def phi_access_reason - phi_context[:reason] + phi_context&.dig(:reason) end # Whether PHI access is allowed for a single instance of this class @@ -562,7 +562,7 @@ def phi_allowed? # end # def require_phi! - raise PhiAccessException, "PHI Access required, please call allow_phi or allow_phi! first" unless phi_allowed? + raise PhiAttrs::Exceptions::PhiAccessException, "PHI Access required, please call allow_phi or allow_phi! first" unless phi_allowed? end def reload diff --git a/spec/phi_attrs/disallow_last_phi_spec.rb b/spec/phi_attrs/disallow_last_phi_spec.rb new file mode 100644 index 0000000..7132b42 --- /dev/null +++ b/spec/phi_attrs/disallow_last_phi_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "class disallow_last_phi!" do + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + it "pops only the most recent allow" do |t| + PatientInfo.allow_phi!("first_user", t.full_description) + PatientInfo.allow_phi!("second_user", t.full_description) + expect(PatientInfo.phi_allowed?).to be true + + PatientInfo.disallow_last_phi! + expect(PatientInfo.phi_allowed?).to be true + + PatientInfo.disallow_last_phi! + expect(PatientInfo.phi_allowed?).to be false + end + + it "logs gracefully on empty stack" do + expect { PatientInfo.disallow_last_phi! }.not_to raise_error + end +end diff --git a/spec/phi_attrs/exception_safety_spec.rb b/spec/phi_attrs/exception_safety_spec.rb new file mode 100644 index 0000000..a2087e1 --- /dev/null +++ b/spec/phi_attrs/exception_safety_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "exception safety" do + file_name = __FILE__ + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + context "class-level disallow_phi block" do + it "restores permission when block raises" do |t| + PatientInfo.allow_phi!(file_name, t.full_description) + expect(PatientInfo.phi_allowed?).to be true + + expect do + PatientInfo.disallow_phi do + expect(PatientInfo.phi_allowed?).to be false + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(PatientInfo.phi_allowed?).to be true + end + end + + context "instance-level disallow_phi block" do + it "restores permission when block raises" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + expect(patient_jane.phi_allowed?).to be true + + expect do + patient_jane.disallow_phi do + expect(patient_jane.phi_allowed?).to be false + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(patient_jane.phi_allowed?).to be true + end + end + + context "class-level allow_phi block" do + it "revokes access when block raises" do |t| + expect(PatientInfo.phi_allowed?).to be false + + expect do + PatientInfo.allow_phi(file_name, t.full_description) do + expect(PatientInfo.phi_allowed?).to be true + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(PatientInfo.phi_allowed?).to be false + end + end + + context "instance-level allow_phi block" do + it "revokes access when block raises" do |t| + expect(patient_jane.phi_allowed?).to be false + + expect do + patient_jane.allow_phi(file_name, t.full_description) do + expect(patient_jane.phi_allowed?).to be true + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(patient_jane.phi_allowed?).to be false + end + end + + context "class-level get_phi block" do + it "revokes access when block raises" do |t| + expect(PatientInfo.phi_allowed?).to be false + + expect do + PatientInfo.get_phi(file_name, t.full_description) do + expect(PatientInfo.phi_allowed?).to be true + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(PatientInfo.phi_allowed?).to be false + end + end + + context "instance-level get_phi block" do + it "revokes access when block raises" do |t| + expect(patient_jane.phi_allowed?).to be false + + expect do + patient_jane.get_phi(file_name, t.full_description) do + expect(patient_jane.phi_allowed?).to be true + raise "boom" + end + end.to raise_error(RuntimeError, "boom") + + expect(patient_jane.phi_allowed?).to be false + end + end +end diff --git a/spec/phi_attrs/log_sanitization_spec.rb b/spec/phi_attrs/log_sanitization_spec.rb new file mode 100644 index 0000000..a025ba9 --- /dev/null +++ b/spec/phi_attrs/log_sanitization_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "log sanitization" do + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + it "strips newlines from user_id" do + patient_jane.allow_phi!("user\nINJECTED", "reason") + expect(patient_jane.phi_allowed_by).to eq("user INJECTED") + end + + it "strips newlines from reason" do + patient_jane.allow_phi!("user", "reason\r\nINJECTED") + expect(patient_jane.phi_access_reason).to eq("reason INJECTED") + end + + it "strips newlines from class-level allow" do + PatientInfo.allow_phi!("user\nINJECTED", "reason\nINJECTED") + expect(PatientInfo.__phi_stack.last[:user_id]).to eq("user INJECTED") + expect(PatientInfo.__phi_stack.last[:reason]).to eq("reason INJECTED") + end +end diff --git a/spec/phi_attrs/phi_allowed_by_spec.rb b/spec/phi_attrs/phi_allowed_by_spec.rb new file mode 100644 index 0000000..d2da4f3 --- /dev/null +++ b/spec/phi_attrs/phi_allowed_by_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "phi_allowed_by and phi_access_reason" do + file_name = __FILE__ + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + context "phi_allowed_by" do + it "returns nil when no access granted" do + expect(patient_jane.phi_allowed_by).to be_nil + end + + it "returns user_id after allow_phi!" do + patient_jane.allow_phi!("test_user", "test_reason") + expect(patient_jane.phi_allowed_by).to eq("test_user") + end + + it "returns most recent user_id with stacked allows" do + patient_jane.allow_phi!("first_user", "first_reason") + patient_jane.allow_phi!("second_user", "second_reason") + expect(patient_jane.phi_allowed_by).to eq("second_user") + end + + it "falls back to class-level user_id" do |t| + PatientInfo.allow_phi!(file_name, t.full_description) + expect(patient_jane.phi_allowed_by).to eq(file_name) + end + end + + context "phi_access_reason" do + it "returns nil when no access granted" do + expect(patient_jane.phi_access_reason).to be_nil + end + + it "returns reason after allow_phi!" do + patient_jane.allow_phi!("test_user", "test_reason") + expect(patient_jane.phi_access_reason).to eq("test_reason") + end + + it "returns most recent reason with stacked allows" do + patient_jane.allow_phi!("user", "first_reason") + patient_jane.allow_phi!("user", "second_reason") + expect(patient_jane.phi_access_reason).to eq("second_reason") + end + end +end diff --git a/spec/phi_attrs/request_isolation_spec.rb b/spec/phi_attrs/request_isolation_spec.rb new file mode 100644 index 0000000..67e0689 --- /dev/null +++ b/spec/phi_attrs/request_isolation_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "request isolation" do + file_name = __FILE__ + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + it "class-level allow is not visible after RequestStore.clear!" do |t| + PatientInfo.allow_phi!(file_name, t.full_description) + expect(PatientInfo.phi_allowed?).to be true + + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + + expect(PatientInfo.phi_allowed?).to be false + end + + it "instance still sees its own stack after RequestStore.clear!" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + expect(patient_jane.phi_allowed?).to be true + + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + + expect(patient_jane.phi_allowed?).to be true + expect(PatientInfo.phi_allowed?).to be false + end + + it "class-level extended phi instances are cleared across requests" do |t| + patient = create(:patient_info, :with_multiple_health_records) + PatientInfo.allow_phi!(file_name, t.full_description) + patient.patient_detail.detail + + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + + expect(PatientInfo.__instances_with_extended_phi).to be_empty + end +end diff --git a/spec/phi_attrs/require_phi_spec.rb b/spec/phi_attrs/require_phi_spec.rb new file mode 100644 index 0000000..ab96521 --- /dev/null +++ b/spec/phi_attrs/require_phi_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "require_phi!" do + file_name = __FILE__ + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + it "raises PhiAccessException when no access granted" do + expect { patient_jane.require_phi! }.to raise_error(access_error) + end + + it "does not raise when instance access granted" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + expect { patient_jane.require_phi! }.not_to raise_error + end + + it "does not raise when class access granted" do |t| + PatientInfo.allow_phi!(file_name, t.full_description) + expect { patient_jane.require_phi! }.not_to raise_error + end +end diff --git a/spec/phi_attrs/rspec_matcher_spec.rb b/spec/phi_attrs/rspec_matcher_spec.rb new file mode 100644 index 0000000..a2f909b --- /dev/null +++ b/spec/phi_attrs/rspec_matcher_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require "spec_helper" +require "phi_attrs/rspec" + +RSpec.describe "allow_phi_access matcher" do + file_name = __FILE__ + let(:patient_jane) { build(:patient_info, first_name: "Jane") } + + context "positive match" do + it "matches when phi access is allowed" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + expect(patient_jane).to allow_phi_access + end + + it "does not match when phi access is not allowed" do + expect(patient_jane).not_to allow_phi_access + end + end + + context "allowed_by chain" do + it "matches when user_id matches" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + expect(patient_jane).to allow_phi_access.allowed_by(file_name) + end + + it "fails when user_id differs" do |t| + patient_jane.allow_phi!(file_name, t.full_description) + matcher = allow_phi_access.allowed_by("wrong_user") + expect(matcher.matches?(patient_jane)).to be false + end + end + + context "with_access_reason chain" do + it "matches when reason matches" do + patient_jane.allow_phi!("user", "the_reason") + expect(patient_jane).to allow_phi_access.with_access_reason("the_reason") + end + + it "fails when reason differs" do + patient_jane.allow_phi!("user", "the_reason") + matcher = allow_phi_access.with_access_reason("wrong_reason") + expect(matcher.matches?(patient_jane)).to be false + end + end + + context "negated matcher" do + it "raises ArgumentError when allowed_by specified" do + expect do + expect(patient_jane).not_to allow_phi_access.allowed_by("user") + end.to raise_error(ArgumentError) + end + + it "raises ArgumentError when with_access_reason specified" do + expect do + expect(patient_jane).not_to allow_phi_access.with_access_reason("reason") + end.to raise_error(ArgumentError) + end + end +end From b8ee0692c861837ce347966002fd2679112fcfc3 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 14:48:12 -0400 Subject: [PATCH 08/11] perf: use Struct for phi stack entries instead of Hash PhiStackEntry = Struct.new(:phi_access_allowed, :user_id, :reason, :logged, keyword_init: true) Structs are cheaper to allocate than Hashes and expose named accessors. Replaces every push({...}) and [:key] access site in both class-level and instance-level stacks. --- lib/phi_attrs/phi_record.rb | 46 ++++++++++--------------- spec/phi_attrs/log_sanitization_spec.rb | 4 +-- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/lib/phi_attrs/phi_record.rb b/lib/phi_attrs/phi_record.rb index 4b3ab45..d123e0a 100644 --- a/lib/phi_attrs/phi_record.rb +++ b/lib/phi_attrs/phi_record.rb @@ -2,6 +2,8 @@ # Namespace for classes and modules that handle PHI Attribute Access Logging module PhiAttrs + PhiStackEntry = Struct.new(:phi_access_allowed, :user_id, :reason, :logged, keyword_init: true) + # Module for extending ActiveRecord models to handle PHI access logging # and restrict access to attributes. # @@ -77,11 +79,7 @@ def allow_phi!(user_id = nil, reason = nil) user_id = user_id.to_s.gsub(/[\r\n]/, " ") reason = reason.to_s.gsub(/[\r\n]/, " ") - __phi_stack.push({ - phi_access_allowed: true, - user_id: user_id, - reason: reason - }) + __phi_stack.push(PhiStackEntry.new(phi_access_allowed: true, user_id: user_id, reason: reason)) PhiAttrs::Logger.tagged(PHI_ACCESS_LOG_TAG, name) do PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") @@ -193,7 +191,7 @@ def get_phi(user_id = nil, reason = nil, allow_only: nil) def disallow_phi raise ArgumentError, "block required. use disallow_phi! without block" unless block_given? - __phi_stack.push({phi_access_allowed: false}) + __phi_stack.push(PhiStackEntry.new(phi_access_allowed: false)) begin yield ensure @@ -227,7 +225,7 @@ def disallow_last_phi! raise ArgumentError, "block not allowed" if block_given? removed_access = __phi_stack.pop - message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : "PHI access disabled. No class level access was granted." + message = removed_access.present? ? "PHI access disabled for #{removed_access.user_id}" : "PHI access disabled. No class level access was granted." PhiAttrs::Logger.tagged(PHI_ACCESS_LOG_TAG, name) do PhiAttrs::Logger.info(message) @@ -242,7 +240,7 @@ def disallow_last_phi! # @return [Boolean] whether PHI access is allowed for this instance # def phi_allowed? - __phi_stack.present? && __phi_stack[-1][:phi_access_allowed] + __phi_stack.present? && __phi_stack[-1].phi_access_allowed end def __instances_with_extended_phi @@ -261,7 +259,7 @@ def __reset_phi_stack def __user_id_string(access_list) access_list ||= [] - access_list.map { |c| "'#{c[:user_id]}'" }.join(",") + access_list.map { |c| "'#{c.user_id}'" }.join(",") end def current_user @@ -398,11 +396,7 @@ def allow_phi!(user_id = nil, reason = nil) reason = reason.to_s.gsub(/[\r\n]/, " ") PhiAttrs::Logger.tagged(*phi_log_keys) do - @__phi_access_stack.push({ - phi_access_allowed: true, - user_id: user_id, - reason: reason - }) + @__phi_access_stack.push(PhiStackEntry.new(phi_access_allowed: true, user_id: user_id, reason: reason)) PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") end @@ -518,7 +512,7 @@ def disallow_last_phi!(preserve_extensions: false) removed_access = @__phi_access_stack.pop revoke_extended_phi! unless preserve_extensions - message = removed_access.present? ? "PHI access disabled for #{removed_access[:user_id]}" : "PHI access disabled. No instance level access was granted." + message = removed_access.present? ? "PHI access disabled for #{removed_access.user_id}" : "PHI access disabled. No instance level access was granted." PhiAttrs::Logger.info(message) end end @@ -529,7 +523,7 @@ def disallow_last_phi!(preserve_extensions: false) # @return [String] the user_id passed in to allow_phi! # def phi_allowed_by - phi_context&.dig(:user_id) + phi_context&.user_id end # The access reason for allowing access to this instance. @@ -538,7 +532,7 @@ def phi_allowed_by # @return [String] the reason passed in to allow_phi! # def phi_access_reason - phi_context&.dig(:reason) + phi_context&.reason end # Whether PHI access is allowed for a single instance of this class @@ -550,7 +544,7 @@ def phi_access_reason # @return [Boolean] whether PHI access is allowed for this instance # def phi_allowed? - !phi_context.nil? && phi_context[:phi_access_allowed] + !phi_context.nil? && phi_context.phi_access_allowed end # Require phi access. Raises an error pre-emptively if it has not been granted. @@ -575,9 +569,7 @@ def reload # Adds a disallow phi flag to instance internal stack. # @private since subject to change def add_disallow_flag! - @__phi_access_stack.push({ - phi_access_allowed: false - }) + @__phi_access_stack.push(PhiStackEntry.new(phi_access_allowed: false)) end # removes the last item in instance internal stack. @@ -628,19 +620,19 @@ def class_phi_context # @return String of all the user_id's passed in to allow_phi! # def all_phi_allowed_by - ids = (@__phi_access_stack || []).filter_map { |c| "'#{c[:user_id]}'" } - ids.concat(self.class.__phi_stack.filter_map { |c| "'#{c[:user_id]}'" }) + ids = (@__phi_access_stack || []).filter_map { |c| "'#{c.user_id}'" } + ids.concat(self.class.__phi_stack.filter_map { |c| "'#{c.user_id}'" }) ids.join(",") end def all_phi_context_logged? - (@__phi_access_stack.nil? || @__phi_access_stack.all? { |v| v[:logged] }) && - self.class.__phi_stack.all? { |v| v[:logged] } + (@__phi_access_stack.nil? || @__phi_access_stack.all? { |v| v.logged }) && + self.class.__phi_stack.all? { |v| v.logged } end def set_all_phi_context_logged - @__phi_access_stack&.each { |c| c[:logged] = true } - self.class.__phi_stack.each { |c| c[:logged] = true } + @__phi_access_stack&.each { |c| c.logged = true } + self.class.__phi_stack.each { |c| c.logged = true } end def __phi_init diff --git a/spec/phi_attrs/log_sanitization_spec.rb b/spec/phi_attrs/log_sanitization_spec.rb index a025ba9..2b97ea9 100644 --- a/spec/phi_attrs/log_sanitization_spec.rb +++ b/spec/phi_attrs/log_sanitization_spec.rb @@ -17,7 +17,7 @@ it "strips newlines from class-level allow" do PatientInfo.allow_phi!("user\nINJECTED", "reason\nINJECTED") - expect(PatientInfo.__phi_stack.last[:user_id]).to eq("user INJECTED") - expect(PatientInfo.__phi_stack.last[:reason]).to eq("reason INJECTED") + expect(PatientInfo.__phi_stack.last.user_id).to eq("user INJECTED") + expect(PatientInfo.__phi_stack.last.reason).to eq("reason INJECTED") end end From 8295efe8db3e624ca28f423f063adae868e1332a Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 15:58:55 -0400 Subject: [PATCH 09/11] chore: update to use latest checkouts version --- .github/workflows/build.yml | 2 +- .github/workflows/publish.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 620da5d..a2facbe 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -17,7 +17,7 @@ jobs: env: BUNDLE_GEMFILE: gemfiles/${{ matrix.appraisal }}.gemfile steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Ruby ${{ matrix.ruby }} uses: ruby/setup-ruby@v1 with: diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 56ae745..607e09f 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -14,7 +14,7 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@v6 - name: Set up Ruby uses: ruby/setup-ruby@v1 From dad0ceea8d744e4d5a17d6974896b0afc9d99ccb Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 16:23:27 -0400 Subject: [PATCH 10/11] chore: remove unnecessary rescue --- Rakefile | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Rakefile b/Rakefile index 9d48bf4..098dd96 100644 --- a/Rakefile +++ b/Rakefile @@ -14,10 +14,6 @@ end RSpec::Core::RakeTask.new(:spec) Rake::Task[:spec].enhance(["dummy:db:create", "dummy:db:migrate"]) -begin - require "standard/rake" -rescue LoadError - # Standard not available -end +require "standard/rake" task default: [:standard, :spec] From 74537047a6f2c6143fcf3d4baf60135ef334db39 Mon Sep 17 00:00:00 2001 From: Wyatt Kirby Date: Thu, 28 May 2026 19:57:20 -0400 Subject: [PATCH 11/11] fix: update tests to use create instead of build to account for upstream changes --- spec/phi_attrs/exception_safety_spec.rb | 2 +- spec/phi_attrs/log_sanitization_spec.rb | 2 +- spec/phi_attrs/phi_allowed_by_spec.rb | 2 +- spec/phi_attrs/require_phi_spec.rb | 2 +- spec/phi_attrs/rspec_matcher_spec.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/phi_attrs/exception_safety_spec.rb b/spec/phi_attrs/exception_safety_spec.rb index a2087e1..a5e181b 100644 --- a/spec/phi_attrs/exception_safety_spec.rb +++ b/spec/phi_attrs/exception_safety_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "exception safety" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_jane) { create(:patient_info, first_name: "Jane") } context "class-level disallow_phi block" do it "restores permission when block raises" do |t| diff --git a/spec/phi_attrs/log_sanitization_spec.rb b/spec/phi_attrs/log_sanitization_spec.rb index 2b97ea9..225cfd3 100644 --- a/spec/phi_attrs/log_sanitization_spec.rb +++ b/spec/phi_attrs/log_sanitization_spec.rb @@ -3,7 +3,7 @@ require "spec_helper" RSpec.describe "log sanitization" do - let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_jane) { create(:patient_info, first_name: "Jane") } it "strips newlines from user_id" do patient_jane.allow_phi!("user\nINJECTED", "reason") diff --git a/spec/phi_attrs/phi_allowed_by_spec.rb b/spec/phi_attrs/phi_allowed_by_spec.rb index d2da4f3..201d00b 100644 --- a/spec/phi_attrs/phi_allowed_by_spec.rb +++ b/spec/phi_attrs/phi_allowed_by_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "phi_allowed_by and phi_access_reason" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_jane) { create(:patient_info, first_name: "Jane") } context "phi_allowed_by" do it "returns nil when no access granted" do diff --git a/spec/phi_attrs/require_phi_spec.rb b/spec/phi_attrs/require_phi_spec.rb index ab96521..fdd6025 100644 --- a/spec/phi_attrs/require_phi_spec.rb +++ b/spec/phi_attrs/require_phi_spec.rb @@ -4,7 +4,7 @@ RSpec.describe "require_phi!" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_jane) { create(:patient_info, first_name: "Jane") } it "raises PhiAccessException when no access granted" do expect { patient_jane.require_phi! }.to raise_error(access_error) diff --git a/spec/phi_attrs/rspec_matcher_spec.rb b/spec/phi_attrs/rspec_matcher_spec.rb index a2f909b..3d8864b 100644 --- a/spec/phi_attrs/rspec_matcher_spec.rb +++ b/spec/phi_attrs/rspec_matcher_spec.rb @@ -5,7 +5,7 @@ RSpec.describe "allow_phi_access matcher" do file_name = __FILE__ - let(:patient_jane) { build(:patient_info, first_name: "Jane") } + let(:patient_jane) { create(:patient_info, first_name: "Jane") } context "positive match" do it "matches when phi access is allowed" do |t|