Skip to content

Create common interface for accessing PII bundle in session#6054

Merged
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/standardize-pii-cache-fetch
Mar 14, 2022
Merged

Create common interface for accessing PII bundle in session#6054
mitchellhenke merged 3 commits intomainfrom
mitchellhenke/standardize-pii-cache-fetch

Conversation

@mitchellhenke
Copy link
Contributor

On the road to different methods of encrypting/decrypting user data and being more precise in our usage of KMS, changing how we interface with accessing PII bundles opens up some more opportunities with how we can handle it.

With a common interface, it becomes possible for us to do things like lazily decrypt sensitive data, which will reduce the amount of time and number of times where a request is holding sensitive decrypted data.

These changes remove all of the instances where decrypted_pii is checked in place of using the Pii::Cacher#fetch. Some areas still need to use the decrypted string rather than Pii::Attributes, so a fetch_string method was added to support those cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for these .fetch.blank? and .fetch.present?, what if we added a method for that?

Suggested change
Pii::Cacher.new(current_user, user_session).fetch.blank?
Pii::Cacher.new(current_user, user_session).exists_in_session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea, it would allow us to defer decryption a bit further even since it could check for whether the value exists in either decrypted/encrypted form

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the future it would let us defer decryption, but I think by the time we're here, we've already decrypted the whole session payload?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, getting ahead of myself, but yeah exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in ba75087

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking of using this same tooling to handle the PII that is moving through the proofing flow? I'm curious what that looks like if so.

May be an irrelevant question in an FSMv2 world where we store the unverified PII on the client. But I suspect we'll want the changes to reduce KMS calls in place before we get there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, yeah. It kind of requires a more central way to manage which FSMv2 can hopefully bring.

We do know the top-level keys that are used (idv/doc_auth and idv), but one of those is probably accessed during each proofing step anyway? It may be easier to encrypt/decrypt those in their entirety during the proofing process.

@mitchellhenke mitchellhenke marked this pull request as ready for review March 14, 2022 13:53
Mitchell Henke and others added 3 commits March 14, 2022 08:59
changelog: Internal, Data, Create common interface for accessing PII bundle in session
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/standardize-pii-cache-fetch branch from c1f6a1f to ba75087 Compare March 14, 2022 14:08
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants