apsislabs / phi_attrs

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

Return stuff from blocks. Deliberately leaks PHI (as per #17) #49

Closed Crisfole closed 3 years ago

Crisfole commented 3 years ago

Fixes #17. Makes some constructs, especially in unit tests, much easiser.

Crisfole commented 3 years ago

This needs debate and discussion. It's not necessarily a good idea.

wkirby commented 3 years ago

@Crisfole I don't like this, and the reason is ruby default returns. You can imagine this scenario:

def foo
  bar.allow_phi(user, "phi access") do
      do_thing_with_phi(bar.phi_field)
  end
end

If do_thing_with_phi returns the passed in value, now foo also returns PHI, which seems leaky.

I still think we're much better served by providing an explicit get_phi method if we want a way to escape PHI outside a block.

Crisfole commented 3 years ago

Sure. Good reason.

Crisfole commented 3 years ago

@wkirby I just moved the method into a get_phi I still like the syntax of using a block for extracting a single value. It also discourages using it heavily since it's a massively painful way to get more than one thing.