apsislabs / phi_attrs

HIPAA compliant PHI access logging for Ruby on Rails.
MIT License
27 stars 3 forks source link

Feature/allow phi with block #8

Closed Crisfole closed 5 years ago

Crisfole commented 5 years ago

This adds allow_phi methods to both the classes and instances of PhiRecord classes. It implements it with a stack so that the previous context can be restored regardless of whether or not allow_phi! has been called before.

Crisfole commented 5 years ago

Some thought needs to be put into what happens if you call the mutating phi access methods (allow_phi!) or (disallow_phi!) inside a block...I'm inclined to simply disallow it.

wkirby commented 5 years ago

This looks interesting. I want to do a real code walk through with you though.

Crisfole commented 5 years ago

Yeah, for sure. I'm not 100% convinced that maintaining a stack of contexts is necessary, but it was planning ahead for another idea I had and solved a problem I had here (albeit more complexly than perhaps necessary).

The other feature was a PhiPromise: basically allow creating an object with a value method that only logged access when value was called the first time. Important in code where you want to gather data to consume later, but other code might mean that the phi never gets used.

HenryKeiter commented 5 years ago

This is now fully implemented, along with all the edge cases I could think of that weren't too hideous to fix. There are a lot. As a result, adding the access stack has become a somewhat heavier proposition than it was previously. @Crisfole You should take a look; a lot has changed.

I think this isn't a bad starting point—it should be reasonably stable. However, the amount of footwork needed to support both blocks and unlimited declarative allowances leads me to think that we should choose and target one of two architectures for 0.2.0:

Regardless, there's more discussion we need to have around the sorts of use cases we want to support/encourage/discourage. Implementing this has brought up some really good questions about where we want the public API to go. Whatever we end up with for 0.2.0, we want something that will alleviate (or ideally disallow) the extra headache associated with scenarios like this toy one:

class Foo
  has_one :bar
  extend_phi_access :bar
end

foo = Foo.first
bar = foo.bar
bar.allow_phi!('eric')
foo.allow_phi('henry') do
  puts foo.quux # Log: [foo][Henry]
  puts foo.bar.baz # Calls foo.bar.allow_phi!('henry')
                   # Log: [Bar][Henry:reason]
  bar.allow_phi!('chris') # (called in a service three levels down)
end # foo.disallow_phi!; foo.revoke_extended_phi! -> Calls -> foo.bar.disallow_phi! (pop 'chris')
puts bar.baz # Log [Bar][Henry:reason]
bar.disallow_phi! # pop 'henry'
puts bar.baz # Log [Bar][Eric:reason]