Add a Pii::ProfileCacher class by jmhooper · Pull Request #9509 · 18F/identity-idp

jmhooper · 2023-11-01T17:37:42Z

The Pii::Cacher is used for decrypting PII and moving it to the session when the user enters their password. It is also used to read the PII from the session and do other ad-hoc writes.

We are planning to change the way that PII is handled in the session. The goal is to be able to decrypt multiple profiles on sign in and get the correct profile when operating on PII for a given profile. To do this we will change the session to have PII stored in the session by profile ID.

Pii::ProfileCacher

While we are transitioning between the old way to store PII in the session and the new way we will need to write PII using both approaches. To facilitate this change this commit adds the Pii::ProfileCacher class.

The Pii::ProfileCacher class implements the same API as Pii::Cacher and is intended to supersede Pii::Cacher when this work is done.

Pii::Cacher methods were modified to nvoke the Pii::ProfileCacher methods. These are done under 2 circumstances:

A feature flag is enabled for reading or writing depending on the operation
A profile ID is passed in on invocations to #fetch and #save_decrypted_pii

New feature flags

The Pii::ProfileCacher is only invoked by Pii::Cacher when feature flags are in place telling it to do so:

session_encrypted_profiles_read_enabled: When this flag is enabled the #fetch method will read PII from the new place in the session
session_encrypted_profiles_write_enabled: When this flag is enabled the #save method and the save_decrypted_pii method will write to the new place in the session in addition to the place where PII is currently written

Session encryptor approach changes

Previously the SessionEncryptor transparently encrypted decrypted PII with KMS as it was written to and from the session store. This commit opts to change that behavior for Pii::ProfileCacher. Instead of letting the SessionEncryptor encrypt PII the Pii::ProfileCacher explicitly encrypts the PII.

This was done to make it more clear to readers of the code how the PII is encrypted when being written and read from the session.

As a note, when we replace the current Pii::Cacher implementation with the new Pii::ProfileCacher we should be able to remove the logic that transparently encrypted the PII as it was written to and from the session store.

The Pii::Cacher is used for decrypting PII and moving it to the session when the user enters their password. It is also used to read the PII from the session and do other ad-hoc writes. We are planning to change the way that PII is handled in the session. The goal is to be able to decrypt multiple profiles on sign in and get the correct profile when operating on PII for a given profile. To do this we will change the session to have PII stored in the session by profile ID. While we are transitioning between the old way to store PII in the session and the new way we will need to write PII using both approaches. To facilitate this change this commit adds the `Pii::ProfileCacher` class. The `Pii::ProfileCacher` class implements the same API as `Pii::Cacher` and is intended to supersede `Pii::Cacher` when this work is done. `Pii::Cacher` methods were modified to nvoke the `Pii::ProfileCacher` methods. These are done under 2 circumstances: 1. A feature flag is enabled for reading or writing depending on the operation 2. A profile ID is passed in on invocations to `#fetch` and `#save_decrypted_pii` The `Pii::ProfileCacher` is only invoked by `Pii::Cacher` when feature flags are in place telling it to do so: - `session_encrypted_profiles_read_enabled`: When this flag is enabled the `#fetch` method will read PII from the new place in the session - `session_encrypted_profiles_write_enabled`: When this flag is enabled the `#save` method and the `save_decrypted_pii` method will write to the new place in the session in addition to the place where PII is currently written Previously the `SessionEncryptor` transparently encrypted decrypted PII with KMS as it was written to and from the session store. This commit opts to change that behavior for `Pii::ProfileCacher`. Instead of letting the `SessionEncryptor` encrypt PII the `Pii::ProfileCacher` explicitly encrypts the PII. This was done to make it more clear to readers of the code how the PII is encrypted when being written and read from the session. As a note, when we replace the current `Pii::Cacher` implementation with the new `Pii::ProfileCacher` we should be able to remove the logic that transparently encrypted the PII as it was written to and from the session store. changelog: Internal, Pii management, The ProfileCacher was added.

mitchellhenke

One minor question, otherwise makes sense to me!

app/services/pii/profile_cacher.rb

This commit builds on #9509. That commit added the ability to write PII to the session under the profile ID associated with that PII. This commit starts using the functionality added in that PR to start writing both the pending and active profile to the session. Additionally in places where PII is written to the session outside the context of password authentication the corresponding profile ID is passed to the PII cacher. [skip changelog]

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit applies that capability to the OpenID Connect UserInfo endpoint to ensure that only the active profile's PII is used there. The user info endpoint is used by service providers to access PII after auth. As a result it is not a request made by users and does not have a user session. To read PII we use the out-of-band session accessor's `#load_pii` method. This commit adds a `profile_id` arg to that method and passes in the active profile's ID when reading PII. changelog: Internal, Active-pending profile, The active profile is now read from the session on the userinfo endpoint.

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profile's ID to fetch PII on the completions screen. The completions screen should always be showing PII of the active profile. changelog: Internal, Active-Pending profile, The active profile's PII is displayed on the completions screen.

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit applies that capability to the OpenID Connect UserInfo endpoint to ensure that only the active profile's PII is used there. The user info endpoint is used by service providers to access PII after auth. As a result it is not a request made by users and does not have a user session. To read PII we use the out-of-band session accessor's `#load_pii` method. This commit adds a `profile_id` arg to that method and passes in the active profile's ID when reading PII. changelog: Internal, Active-pending profile, The active profile is now read from the session on the userinfo endpoint.

…en (#9599) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profile's ID to fetch PII on the completions screen. The completions screen should always be showing PII of the active profile. changelog: Internal, Active-Pending profile, The active profile's PII is displayed on the completions screen.

In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profiles PII when encrypting recovery PII for the active profile when the active profile has a broken personal key. changelog: Internal, Pending and active profile, The active profile PII is fetched with the PII cacher when a user with a broken personal key on their active profile signs in.

…heir password In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. As a result of this change we can encrypt both the pending and active profile with user's password. This means both profiles can be decrypted on sign-in. There are consequences for recovery with personal key. When the user changes their password and their data is encrypted their recovery PII is also encrypted as a consequence. The pending and active profile are both encrypted with a different personal key. In this commit I elected to display the pending profile personal key to the user so that profile is recoverable when it becomes active. [skip changelog]

…key (#9601) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profiles PII when encrypting recovery PII for the active profile when the active profile has a broken personal key. changelog: Internal, Pending and active profile, The active profile PII is fetched with the PII cacher when a user with a broken personal key on their active profile signs in.

…heir password (#9607) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. As a result of this change we can encrypt both the pending and active profile with user's password. This means both profiles can be decrypted on sign-in. There are consequences for recovery with personal key. When the user changes their password and their data is encrypted their recovery PII is also encrypted as a consequence. The pending and active profile are both encrypted with a different personal key. In this commit I elected to display the pending profile personal key to the user so that profile is recoverable when it becomes active. [skip changelog]

* LG-11553 Remove recovery PII re-encryption from `PersonalKeyVerificationController` (#9602) The `PersonalKeyVerificationController` is used to verify a personal key as an MFA method and allow a user to sign in. When this is done a new personal key is issued. This controller had code for re-encrypting the users profile with the newly issued personal key. However, a user with an active profile was never able to reach this path. The `check_personal_key_enabled` calls `TwoFactorAuthentication::PersonalKeyPolicy#enabled?`. This method returns false if the user has any profiles. Since this code path is unreachable this commit removes it. I was not able to find any tests covering this re-encryption behavior. [skip changelog] * LG-11573: Add RISC events for account suspension, account reinstatement (#9594) * LG-11573: Add RISC events for account suspension, account reinstatement changelog: Internal, User suspension, Add RISC events for user suspension * feedback * feedback * feedback name changed * LG-11534 Load the active profile from the session on broken personal key (#9601) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. This commit uses the active profiles PII when encrypting recovery PII for the active profile when the active profile has a broken personal key. changelog: Internal, Pending and active profile, The active profile PII is fetched with the PII cacher when a user with a broken personal key on their active profile signs in. * Restructure analytics error_details as hash (#9572) changelog: Internal, Analytics, Adjust format of analytics logging to improve querying support * Remove failure_reason from Attempts API stub (#9576) * Remove Tracker#parse_failure_reason **Why**: Simplifies codebase * Remove failure_reason entirely **Why**: Inconsitent structure, and completely unused * Remove some more unused fake tracker code changelog: Internal, Source code, Clean up unused error tracking code * Use Icon List component for requested attributes consent (#9555) * Support tag options for IconListItemComponent * Use IconListComponent for requested attributes changelog: Internal, Components, Use consistent design system component for icon list * Use ButtonComponent for auth confirmation * Remove seemingly-unnecessary assertion content scoping * Update AAMVA test scripts (#9608) - Require optparse, test it - Use pretty-printing changelog: Internal, Testing, Add AAMVA test script covered by specs * LG 11432 Prevent duplicate F/T setup if user hits back button on second MFA prompt (#9587) * changelog: User-Facing Improvements, Webauthn, Prevent duplicate F/T setup on second MFA prompt * add spec coverage for platform auth redirect * move test to a function * move conditional to before_action * clean up validate platform authenticator method * LG 11145 Break up MFA selection presenter classes for Phone Presenters (#9560) * changelog: Internal, tech debt, Break up MFA presenter class for phone * split phone, voice, and sms presenter classes up * add tests cases for sign_in and set_up phone presenter class * split setup signin presenter spec for voice and sms * lint fix * remove old phone selection presenter spec * rename phone presenter in spec * remove unneeded configuration variable, leverage user for type method * remove info method from phone sub classes * update options presenter spec with newly split classes * merge sms and voice presenters * revise specs according to merged classes * remove deprecated spec and lint fix * change info to switch and fix regression with disabled? method * lint fix * fix spec * remove deprecated translations from setup presenter * move reader :method to phone sign in presenter * fix lint * fix lint * clean up selection presenter class * remove unneeded configuration setting * remove configuration from set up presenter spec * add sms and voice outage spec, standardize spec syntax * clarify some syntax * Refactor WebauthnVerificationForm to handle error messages (#9613) changelog: Internal, Code Quality, Move error messages for WebAuthn verification to form class * Add analytics property for WebAuthn sign-in frontend error (#9611) * Add analytics property for WebAuthn sign-in frontend error changelog: Internal, Analytics, Add analytics property for WebAuthn sign-in frontend error * Document frontend_error * Finalize cleanup for MFA selection presenters (#9612) * Update missed references to SignIn base selection presenter * Remove unused SelectionPresenter * Swap base presenters to raising NotImplementedError * Swap phone method comparison to use symbol Consistency with logic elsewhere in class * Raise on missing type method in base presenter classes * Consolidate presenter classes to define only type method * Rename phone selection presenter method to delivery_method * Add changelog changelog: Internal, Code Quality, Remove unused code related to MFA selection presenters * Update call sites to use new delivery_method constructor argument * Use setup-specific string for WebAuthn setup presenter * Use ActiveRecord built-in validator for WebAuthn error validation (#9614) * Use ActiveRecord built-in validator for WebAuthn error validation changelog: Internal, Code Quality, Move error messages for WebAuthn verification to form class * Update webauthn_verification_controller_spec.rb * LG-11535 Encrypt the pending and active profile when a user updates their password (#9607) In #9509 we added the ability to specify which profile to fetch PII from when reading PII from the session. As a result of this change we can encrypt both the pending and active profile with user's password. This means both profiles can be decrypted on sign-in. There are consequences for recovery with personal key. When the user changes their password and their data is encrypted their recovery PII is also encrypted as a consequence. The pending and active profile are both encrypted with a different personal key. In this commit I elected to display the pending profile personal key to the user so that profile is recoverable when it becomes active. [skip changelog] * LG-11542 Texas conditional hint text for id number (#9600) * add hint for tx * change name of file * change pack name * refactor show or hide hint function * update state guidance spec * changelog: User-Facing Improvements, State id page, tx specific id numner hint text * use text content instead of inner html * add translations for fr and es * lint fix for translations * update string * normalize yaml * add punctuation to en translation * update es translation * lint fix * Update Prettier to v3.1 (#9618) changelog: Internal, Code Formatting, Update Prettier code formatter to latest version * Log service_provider for RackAttack events (#9620) * Log service_provider for RackAttack events changelog: Internal, Logging, Log service_provider for RackAttack events * Update spec/requests/rack_attack_spec.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Update spec/requests/rack_attack_spec.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Remove GPO verification rake task (#9621) This was just glue to support running the job, and is no longer needed. [skip changelog] * LG-11520: Enable daily GPO expiration job (#9622) changelog: Internal, Identity verification, Automatically expire inactive GPO profiles. * Update State ID hint, move HTML out of translation strings into template (#9617) **Why**: Keeping complex HTML inside translation strings is error-prone and can be easy to accidentally mis-align across languages * Refactor to support toggling for state-specific hints - Add HTML view specs too changelog: Accessibility, Screen reader support, Bring back screenreader support for State ID number hint * LG-11477: doc class error for some identification card (#9597) * LG-11477: Document issuer type check. When document type is classified as 'Identification Card', it may mean a State issued identification card. It can also include US Passport Card, VHIC, US Social Security Card, TWIC etc, all of them are not issued by a State. So we also need to check whether the issuer type is StateProvince, not by a Country. Driver's License/Identification Card issued by PR, VI, MP etc, do have issuer type of StateProvince. So by checking IssuerType of the document, we effectively excludes IDs issued at Federal level which are not supported document. * LG-11477: update test. * LG-11477: generate error for doc type before metrics. * LG-11477: use constant for string. * changelog: User-Facing Improvements, Doc Auth, Error for unsupported documents. * LG-11477: id type support logic update for mock client response. * LG-11477: update fixtures with doc issuer type field. * LG-11477: short circuit logic, no need to continue to check. * LG-11477: null safe operation. * LG-11477: minor code format change. * LG-11477: no need in rails. * LG-11477: test clean up. * Update Stylelint config dependencies in preparation for release publish (#9619) * Update stylelint-config-recommended-scss changelog: Internal, Dependencies, Update dependencies to latest versions * Prepare 3.0.0 release * LG-11435: Track frontend analytics for changed country in phone input (#9616) * Update JavaScript analytics examples to use new naming convention * Add new event naming convention support to FrontendLogController * LG-11435: Track frontend analytics for changed country in phone input changelog: Internal, Analytics, Track event when country changed in phone input * Use symbol list syntax Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Improve handling of undefined country code --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * LG-11025: Only log acuantCaptureMode when using acuant SDK (#9610) * log acuantCaptureMode as null when an image is uploaded Co-authored-by: Dawei Wang<dawei.wang@gsa.gov> * update spec to expect acuantCaptureMode is null for images uploaded during doc auth * changelog: Internal, Analytics, Image added logs define acuantCaptureMode only if acuant is used to capture the image * update js test upload test * happy linting * nullify acuantCaptureMode in getAddAttemptAnalyticsPayload when image source is upload * Prepare publish normalize-yaml to NPM (#9627) * Bump YAML dependency to latest * Bump normalize-yaml to 2.0.0 changelog: Internal, Packages, Publish `@18f/identity-normalize-yaml@2.0.0` * Restore width collapse for unstyled buttons (#9632) changelog: Bug Fixes, Buttons, Fix appearance of inline buttons --------- Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Osman Latif <109746710+olatifflexion@users.noreply.github.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: dawei-nava <130466753+dawei-nava@users.noreply.github.com> Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov>

…` implementation The `Pii::ProfileCacher` was implemented in #9509. As described in that pull request: > The Pii::ProfileCacher class implements the same API as Pii::Cacher and is intended to supersede Pii::Cacher when this work is done. All of the invocations to `Pii::Cacher` that require the deprecated profile-unaware functionality have been removed and the new profile-aware functionality is fully implemented and in-user. This commit does the work of moving the `Pii::ProfileCacher` functionality into `Pii::Cacher` and removing `Pii::ProfileCacher` This should not be merged until both `session_encrypted_profiles_read_enabled` and `session_encrypted_profiles_write_enabled` have been set to true. This commit removes those feature flags and assumes they are set to true. changelog: Internal, Profile and session management, The pii cacher implementation was replaced with the profile cacher implementation

…` implementation (#9754) The `Pii::ProfileCacher` was implemented in #9509. As described in that pull request: > The Pii::ProfileCacher class implements the same API as Pii::Cacher and is intended to supersede Pii::Cacher when this work is done. All of the invocations to `Pii::Cacher` that require the deprecated profile-unaware functionality have been removed and the new profile-aware functionality is fully implemented and in-user. This commit does the work of moving the `Pii::ProfileCacher` functionality into `Pii::Cacher` and removing `Pii::ProfileCacher` This should not be merged until both `session_encrypted_profiles_read_enabled` and `session_encrypted_profiles_write_enabled` have been set to true. This commit removes those feature flags and assumes they are set to true. changelog: Internal, Profile and session management, The pii cacher implementation was replaced with the profile cacher implementation

jmhooper force-pushed the jmhooper-profile-cacher branch from e0efd90 to 3d3fad6 Compare November 1, 2023 17:38

jmhooper requested a review from a team November 1, 2023 17:42

zachmargolis mentioned this pull request Nov 1, 2023

Update documentation for action-account GSA-TTS/identity-handbook#479

Merged

delint

ae4670c

jmhooper mentioned this pull request Nov 1, 2023

Start writing decrypted PII to the session based on profile ID #9515

Merged

mitchellhenke approved these changes Nov 2, 2023

View reviewed changes

app/services/pii/profile_cacher.rb Show resolved Hide resolved

jmhooper merged commit 128d3e2 into main Nov 2, 2023

jmhooper deleted the jmhooper-profile-cacher branch November 2, 2023 14:42

amirbey mentioned this pull request Nov 2, 2023

Deploy RC 329 to Prod #9523

Merged

jmhooper mentioned this pull request Nov 15, 2023

LG-11537 Access the active profile in the userinfo endpoint #9596

Merged

jmhooper mentioned this pull request Nov 15, 2023

LG-11538 Show the PII from the active profile on the completions screen #9599

Merged

jmhooper mentioned this pull request Nov 15, 2023

LG-11534 Load the active profile from the session on broken personal key #9601

Merged

jmhooper mentioned this pull request Nov 16, 2023

LG-11535 Encrypt the pending and active profile when a user updates their password #9607

Merged

jmhooper mentioned this pull request Dec 13, 2023

Replace the Pii::Cacher implementation with the Pii::ProfileCacher implementation #9754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Pii::ProfileCacher class#9509

Add a Pii::ProfileCacher class#9509
jmhooper merged 2 commits intomainfrom
jmhooper-profile-cacher

jmhooper commented Nov 1, 2023

Uh oh!

mitchellhenke left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jmhooper commented Nov 1, 2023

Pii::ProfileCacher

New feature flags

Session encryptor approach changes

Uh oh!

mitchellhenke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants