Skip to content

Modify Pii::Cacher#save_decrypted_pii_json to be Pii:Cacher#save_decrypted_pii#9498

Merged
jmhooper merged 7 commits intomainfrom
jmhooper-remove-save-decrypted-pii-json
Nov 1, 2023
Merged

Modify Pii::Cacher#save_decrypted_pii_json to be Pii:Cacher#save_decrypted_pii#9498
jmhooper merged 7 commits intomainfrom
jmhooper-remove-save-decrypted-pii-json

Conversation

@jmhooper
Copy link
Contributor

The #save_decrypted_pii_json method consumed a JSON PII object and wrote it into the session under the decrypted_pii key.

This method was called in 2 places:

  • The ReactivateAccountController
  • The Idv::Session

In ReactivateAccountController the PII is available as Pii::Attributes and is converted to JSON to enable a call to #save_decrypted_pii_json.

In the Idv::Session the PII is available as Pii::Attributes, but prior to this commit underwent quite the Rube Goldberg process to find it's way into a JSON string that was then written to the session by save_decrypted_pii_json.

This commit changes save_decrypted_pii_json to be save_decrypted_pii and take a Pii::Attributes instead of a JSON string argument. This will make it easier to implement a version of this method that handles multiple profiles when we start encrypting both the active and pending profile in the session.

This commit also short circuits the Rube Goldberg machine in Idv::Session and simply writes the attributes to the user session when the profile is created.

@jmhooper jmhooper requested review from a team, mitchellhenke and zachmargolis October 31, 2023 20:08
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell this acts like an ivar, but since this object represents a session it resulted in the PII actually being written to the session. Thankfully the entirety of the idv session is encrypted so this PII is encrypted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wrote the PII to the Idv session as decrypted_pii. At this point the PII was available as decrypted_pii and pii in the IdV session. Later in the old implementation of move_pii_to_user_session the decrypted_pii value was deleted from the IdV session and copied over into the user session.

I cannot explain any of this but I'm reasonably confident none of it is necessary.

…ecrypted_pii`

This `#save_decrypted_pii_json` method consumed a JSON PII object and wrote it into the session under the `decrypted_pii` key.

This method was called in 2 places:

- The `ReactivateAccountController`
- The `Idv::Session`

In `ReactivateAccountController` the PII is available as `Pii::Attributes` and is converted to JSON to enable a call to `#save_decrypted_pii_json`.

In the `Idv::Session` the PII is available as `Pii::Attributes`, but prior to this commit underwent quite the Rube Goldberg process to find it's way into a JSON string that was then written to the session.

This commit changes `save_decrypted_pii_json` to be `save_decrypted_pii` and take a `Pii::Attributes` instead of a JSON string argument. This will make it easier to implement a version of this method that handles multiple profiles when we start encrypting both the active and pending profile in the session.

changelog: Internal, PII session encryption, The Pii::Cacher#save_decrypted_pii_json was modified to be the Pii:Cacher#save_decrypted_pii.
@jmhooper jmhooper force-pushed the jmhooper-remove-save-decrypted-pii-json branch from 60aecf9 to 484c7b3 Compare October 31, 2023 21:02
Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for sorting this out. I've been puzzled by it but haven't dug in.

@jmhooper jmhooper merged commit fa208c0 into main Nov 1, 2023
@jmhooper jmhooper deleted the jmhooper-remove-save-decrypted-pii-json branch November 1, 2023 12:41
@amirbey amirbey mentioned this pull request Nov 2, 2023
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.

2 participants