apsislabs / phi_attrs Goto Github PK

Calling extend_phi_access does not adequately work for relationships that were defined with has_many instead of belongs to.

Example:

class MyAwesomeRecord < ApplicationRecord
  phi_model

  belongs_to :awesome_record_meta
  has_many :user_fields

  extend_phi_access :awesome_record_meta, :user_fields
end

...later

awesome = MyAwesomeRecord.first
awesome.allow_phi!("user", "context")

awesome.awesome_record_meta.meta_field # OK, works as expected
awesome.user_fields.first.user_field # Explodes with PHIAccessException

Fix will likely need to be in the phi_extend_access method in https://github.com/apsislabs/phi_attrs/blob/master/lib/phi_attrs/phi_record.rb#L286

As part of the test cleanup we added rubocop and created exceptions in https://github.com/apsislabs/phi_attrs/blob/master/.rubocop.yml to not impact existing behavior.

In the PR for this each of the # TODO: RUBOCOP should be an individual commit that removes the exception and fixes the offending file so that we can do a step by step comparison.

Add a rubocop cop for enforcing ! And block semantics.

Instead of using a raw stack with simple push and pops we should assign each entry a GUID so that we can have more controlled revoke behavior for easier mixing of block and ! syntax's.

• allow_phi! should return a GUID
• disallow_phi! should accept an optional GUID to remove
• allow_phi and disallow_phi blocks should track the GUID they create and then revoke that particular access.

This will better support weird mixes like the following, with at least consistent behavior (even if we still don't recommend it):

patient_john = PatientInfo.new

guid = patient_john.allow_phi!('allow1', 'reason)      # Stack: 'allow1'

patient_john.disallow_phi do     # Stack: 'allow1', 'disallow1'
   patient_john.disallow_phi(guid)    # Stack: 'disallow1'
   guid = patient_john.allow_phi('allow2')    # Stack: 'disallow1',  'allow2'
end

patient_john.name # Stack: 'allow2'

We should enable impersonation. "A viewed this while impersonating B"

With the addition of the access stack, more than one "user" may be responsible for a given PHI access request. We should update the automated access logging to surface all users on the current stack, rather than just the one on top.

# FooController
Foo.allow_phi('[email protected]', 'Displaying Foo') do
  foo = Foo.find(params[:id])
  MyService.show_foo(foo)
end

# MyService

def show_foo(foo)
  foo.allow_phi!('MyService', 'Show Foo')
  return foo.as_json # Should log something like "Foo access 0x00fd8a14 by [[email protected], MyService]"
end

# model with associations
class Foo < ActiveRecord::Base
  phi_model
  belongs_to :bar
  has_many :baz

  extend_phi_access :bar, :baz
end

# setup associations
foo = Foo.new
bar = Bar.new
baz = Baz.new
foo.bar = bar
foo.baz << baz

# PHI access is not extended until we call the wrapped method
foo.allow_phi!('me', 'reason')
foo.association(:bar).reader.phi_allowed? # => false
foo.bar.phi_allowed? # => true
foo.association(:bar).reader.phi_allowed? # => true

# desired outcome
foo.allow_phi!('me', 'reason')
foo.association(:bar).reader.phi_allowed? # => true
foo.bar.phi_allowed? # => true
foo.association(:bar).reader.phi_allowed? # => true

We should update allow_phi! to proactively iterate over PHI extensions and call allow PHI on them.

I don't think there's any reason to name the block-syntax method (allow_phi) differently from the standard use (allow_phi!). We can simply check for the existence of a block at the top. This would reduce the likelihood of doing the wrong thing by accidentally calling the wrong method.

For example, the following is currently perfectly good-looking code:

Foo.allow_phi!('henry', 'making a mistake') do
  puts Foo.first.phi_field
end

The issue is...

The block here is never run, because I accidentally used the bang method, which doesn't support blocks. 🤦‍♂️

Allow logger configuration and setup for shift_age and shift_size to support log rotation in Rails 5+

We should include a simple flag when calling phi_model that will exclude id, created_at, and updated_at from PHI wrapping. Instead of hard-coding ID, we should grab class.primary_key to know which one is the primary key.

I could see it being really useful to allow customizing how logging happens, beyond just the file, it'd be nice to able to log it by HTTP POST or starting an ActiveJob or persisting the data to the database.

missing corresponding method to see if there is class level access.

When dealing with a phi_model it's not that infrequent that we'll have joined phi_models. For convenience, we should have a shorthand method of extending allow_phi! to include those joins.

There should be two methods of doing this. One at the class level, which essentially acts as a permanent extension: allowing PHI access on this model will grant PHI access on these models. I'm thinking a syntax like:

class PatientInfo < ActiveRecord::Base
  phi_model
end

class Patient < ActiveRecord::Base
  has_one :patient_info

  phi_model
  extend_phi_access :patient_info
end

patient = Patient.new
patient.allow_phi!('[email protected]', 'reason')
patient.patient_info.first_name

Additionally, there should be a per-usage shorthand. Something that is passed as an argument to allow_phi!. I'm thinking something like:

class PatientInfo < ActiveRecord::Base
  phi_model
end

class Patient < ActiveRecord::Base
  has_one :patient_info

  phi_model
end

patient = Patient.new
patient.allow_phi!('[email protected]', 'reason', include: [:patient_info])
patient.patient_info.first_name

I have my User model as a phi_model and I have the following in the model.

phi_model
include_in_phi(*%i[
    uid
    name
    slug
    phone
    office_phone
    last_sign_in_ip
    unconfirmed_email
    current_sign_in_ip
  ])

In my ApplicationController I have a before_action that allows PHI info:

User.allow_phi!(current_user&.email, "Details: #{params[:controller]}, #{params[:action]}")

I am getting a PhiAttrs::Exceptions::PhiAccessException because I am trying to use the current_user&.email in the log but that is a field I need to include. Am I doing something wrong?

There is a bug/unlisted deprecation in Rails 7 regarding .select and how it creates attribute reflections for fields. To fix Add a has_attribute? check to anything acting on after_initialize will need to have these for us to be able to use .select.

Example:
if has_attribute?(:foo) && foo.blank?

We frequently find ourselves grabbing translations based on our current location:

class FoobarController < ApplicationController
  # ...
  def bazify
    # ...
    reason = t('foobar.bazify.phi_reason')
    # ...
  end
end

Or when we use slayer:

class FrooFrooCommand < Slayer::Command
  def call
    reason = I18n.t('commands.froofroo.phi_reason')
    # ...
  end
end

For at least controllers, commands, and services this should be formalized so that a null reason gets automatically filled out with a translation if present.

This is something I want:

Sometimes I want to capture PHI at a certain point in time, but not use it until the future (capture before/after data, send it).

Being able to grab phi into a Promise like object and then record the access when (and if) it's actually accessed would be really useful.

# Data accessed, but Access not recorded:
before_promise = phi_object.capture_phi { |po| { secret: po.phi_protected_field } }

if update_protected_fields(phi_object)
  # Access Recorded (data *not* freshly recorded):
  before = before_promise.result
  # Data accesed *and* access recorded:
  after = phi_object.allow_phi { |po| { secret: po.phi_protected_field } }
else
  # Since before_promise.result was *not* called, the access was not logged
end

In the context of a rails controller, we can provide a shorthand, so that we don't have to pass in an identifier or a reason. We can assume a current_user and an identification method (probably default to :email) and allow configuration of these.

For the reason, we can do something like:

def current_request
    "#{request.request_method} #{controller_name}::#{action_name}"
  end

This will give the reason as something like GET application_controller::index.

There are cases where it'd be nice to validate that an argument already permits phi access, for instance when a current_user is not available.

For those cases a simple require_phi! would be more ergonomic than raise ArgumentError, "phi_access required, please allow_phi first" unless x.allow_phi?

Implementation is super easy, obviously.

Add a static message that allows developers to log to the phi_access_log file manually.

The generator should create a simple spec file, and should not produce outdated method calls in the commands (fail! and pass!)

Tests are currently scattered in basically the order in which their functionality was written. They need reorganization and possibly also FactoryBot or another data helper.

This may be unnecessary depending on the implementation of #2, but right now, if you do something like:

class PatientInfo < ActiveRecord::Base
  phi_model
end

class Patient < ActiveRecord::Base
  has_one :patient_info
  phi_model
end

patient = Patient.find(1)
patient.allow_phi!('test', 'test')
patient.patient_info.allow_phi!('test', 'test')

patient.patient_info.first_name #=> 'John'

patient.reload!

patient.patient_info.first_name #=> raise PHI Access Exception

We might be able to hook into reload to re-call allow_phi! for all the models that are currently allowed to access PHI.

Implement a disallow_phi block in a way that is fully functional, or remove the method. Personally I don't think a block for this is necessary; it seems to encourage scary behavior.

Sometimes you just want a transitory MyModel.new(params).method_call

It's super annoying to have to assign a variable just to allow phi on the params you're already touching and are unprotected anyway.

We shouldn't 'engage' PHI_Attrs until it's been saved to the database.

Currently, the public disallow_phi! method just pops the top entry. This is pretty unintuitive IMO; I'd expect this call to absolutely, unequivocally disallow PHI access for whatever I've called it on. If we stick with a stack approach, I think this method should be made to clear the entire stack (both stacks in the event of a class-level call or maybe always), and the current behavior should be moved to an internal method.

If we move away from the access stack, this stops being a problem (though it should still probably be updated to remove access granted at both a class- and instance-level).

To extract PHI in a single location, once blocks are available we should capture the result of the block and return it, to allow constructs like this:

field_val = obj.allow_phi { obj.phi_field }

If we don't want to allow this sort of behavior (which I could see, since it explicitly "leaks" PHI to an uncontrolled location), we probably don't want blocks at all. Note that such "leaking" is already very easy to do, so this shouldn't be our only reason for not wanting this construct, e.g.

unprotected_data = []
obj.allow_phi!
unprotected_data << obj.phi_field
obj.disallow_phi!

The index method is easy to override:

def [](key)
  super(key)
end

Should we consider overriding this as well?

Translations are currently hard coded into the translation namespace phi:

en:
  phi:
    controller:
      action:
        model: "Whatever"

We could easily allow that to be customized like the log location.

def allow_phi should take a block that allows and then revokes phi access before and after the block.

Either document how to add a trace id (like request id) or generate one so that we can track allow through revoke.

The unit tests are wrong in spec/phi_attrs/logger_spec.rb lines 172, 179, 208, 215. as they are using allow! instead of block.

Need to fix the underlying logging and the tests,

To stay compliant we should make sure blank values are rejected.

We need the method, or documentation for it.