diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b0816ca..a2facbe 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,16 +1,21 @@ -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 - name: Set up Ruby ${{ matrix.ruby }} @@ -18,9 +23,8 @@ jobs: 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..607e09f 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@v6 + + - 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 c678835..1f24722 100644 --- a/README.md +++ b/README.md @@ -443,15 +443,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 @@ -462,13 +467,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 @@ -479,10 +509,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..098dd96 100644 --- a/Rakefile +++ b/Rakefile @@ -1,12 +1,19 @@ # 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"]) + +require "standard/rake" + +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/configure.rb b/lib/phi_attrs/configure.rb index c669a12..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/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/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 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 2abfdf6..ab32109 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, :uuid, keyword_init: true) + # Module for extending ActiveRecord models to handle PHI access logging # and restrict access to attributes. # @@ -77,13 +79,15 @@ 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]/, " ") uuid = SecureRandom.uuid - __phi_stack.push({ + __phi_stack.push(PhiStackEntry.new( phi_access_allowed: true, user_id: user_id, reason: reason, uuid: uuid - }) + )) PhiAttrs::Logger.tagged(PHI_ACCESS_LOG_TAG, name, uuid) do PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") @@ -195,13 +199,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(PhiStackEntry.new(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! @@ -232,7 +235,7 @@ def disallow_last_phi! removed_access = __phi_stack.pop uuid = __uuid_string(removed_access) - 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, uuid) do PhiAttrs::Logger.info(message) @@ -247,7 +250,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 @@ -266,12 +269,12 @@ 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 __uuid_string(access_list) access_list ||= [] - Array.wrap(access_list).map { |c| c[:uuid] }.join(',').presence || 'none' + Array.wrap(access_list).map(&:uuid).join(',').presence || 'none' end def current_user @@ -344,13 +347,15 @@ 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]/, " ") uuid = SecureRandom.uuid - @__phi_access_stack.push({ + @__phi_access_stack.push(PhiStackEntry.new( phi_access_allowed: true, user_id: user_id, reason: reason, uuid: uuid, - }) + )) PhiAttrs::Logger.tagged(*phi_log_keys, uuid) do PhiAttrs::Logger.info("PHI Access Enabled for '#{user_id}': #{reason}") @@ -448,11 +453,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. @@ -465,12 +471,11 @@ def disallow_last_phi!(preserve_extensions: false) raise ArgumentError, 'block not allowed' if block_given? removed_access = @__phi_access_stack.pop - uuid = removed_access.present? ? removed_access[:uuid] : nil + uuid = removed_access&.uuid PhiAttrs::Logger.tagged(*phi_log_keys, uuid) do - 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 @@ -483,7 +488,7 @@ def disallow_last_phi!(preserve_extensions: false) def phi_allowed_by return '__new_record__' if new_record? - phi_context[:user_id] + phi_context&.user_id end # The access reason for allowing access to this instance. @@ -494,7 +499,7 @@ def phi_allowed_by def phi_access_reason return 'new record' if new_record? - phi_context[:reason] + phi_context&.reason end # Whether PHI access is allowed for a single instance of this class @@ -506,7 +511,7 @@ def phi_access_reason # @return [Boolean] whether PHI access is allowed for this instance # def phi_allowed? - new_record? || (!phi_context.nil? && phi_context[:phi_access_allowed]) + new_record? || (!phi_context.nil? && phi_context.phi_access_allowed) end # Require phi access. Raises an error pre-emptively if it has not been granted. @@ -518,7 +523,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 @@ -531,9 +536,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. @@ -572,8 +575,13 @@ def wrap_phi # @return [Array] log key for an instance of this class # def phi_log_keys - @__phi_log_id = persisted? ? "Key: #{public_send(self.class.primary_key)}" : "Object: #{object_id}" - @__phi_log_keys = [PHI_ACCESS_LOG_TAG, self.class.name, @__phi_log_id] + current_id = persisted? ? public_send(self.class.primary_key) : 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 @@ -595,19 +603,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 # Core logic for wrapping methods in PHI access logging and access restriction. diff --git a/lib/phi_attrs/railtie.rb b/lib/phi_attrs/railtie.rb index ce9d6f5..2cdd661 100644 --- a/lib/phi_attrs/railtie.rb +++ b/lib/phi_attrs/railtie.rb @@ -1,15 +1,13 @@ # 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| - ActiveSupport.on_load(:active_record) do - ActiveSupport.on_load(:active_record) { extend PhiAttrs::Model } - ActiveSupport.on_load(:action_controller) { include PhiAttrs::Controller } - end + initializer "phi_attrs.initialize" do |_app| + ActiveSupport.on_load(:active_record) { extend PhiAttrs::Model } + ActiveSupport.on_load(:action_controller) { include PhiAttrs::Controller } end end end 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 76f2c0a..edc2ed2 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/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 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/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..a5e181b --- /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) { create(: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..225cfd3 --- /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) { create(: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..201d00b --- /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) { create(: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/phi_record/phi_wrapping_spec.rb b/spec/phi_attrs/phi_record/phi_wrapping_spec.rb index 93fe212..f0cceb4 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/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..fdd6025 --- /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) { create(: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..3d8864b --- /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) { create(: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 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!