Skip to content

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

Merged
jmhooper merged 1 commit into
mainfrom
jmhooper-profile-cacher-save
Nov 6, 2023
Merged

Start writing decrypted PII to the session based on profile ID#9515
jmhooper merged 1 commit into
mainfrom
jmhooper-profile-cacher-save

Conversation

@jmhooper

@jmhooper jmhooper commented Nov 1, 2023

Copy link
Copy Markdown
Contributor

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.

@jmhooper jmhooper requested a review from a team November 1, 2023 19:23

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am considering wrapping this in a block to only run it if the write flag is enabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually that does not work because then the pending profile will never get read 🤔

Base automatically changed from jmhooper-profile-cacher to main November 2, 2023 14:42
@jmhooper jmhooper force-pushed the jmhooper-profile-cacher-save branch from 0fbee76 to 173e687 Compare November 2, 2023 14:43

@jmhooper jmhooper Nov 2, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had a little trouble chasing down where this one was called. It is the method that is invoked after a user enters their personal key and their recovery PII is decrypted. The recovery PII is written to the session. The user is sent to the #new action of VerifyPasswordController and then submits to #update which invokes the #decrypted_pii method a few lines down to read that PII from the session and re-encrypt it.

The controllers involved here all have a before action that checks to make sure the user does in fact have a password reset profile and that is the profile that is being operated on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit - Is there a way to make sure that pattern is carried through to future controllers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this in regards to the before action? The before action to ensure there is a password reset profile is pretty essential to the recovery of a password reset profile. I would expect anybody operating on this code to ensure some sort of control like that is in place. I am not sure of a good way to enforce that that fits within the scope of this change request. There are lots of other bits of code dependent on the password reset profile spread out across these controllers.

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]
@jmhooper jmhooper force-pushed the jmhooper-profile-cacher-save branch from 173e687 to fc67398 Compare November 6, 2023 14:31

@jmax-gsa jmax-gsa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with one nit; ignore or take up as you see fit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit - Is there a way to make sure that pattern is carried through to future controllers?

@soniaconnolly soniaconnolly left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I'm assuming there are specs that test cache_profiles with 0, 1, and 2 active/pending profiles. I looked around a little bit and didn't immediately find them.

@jmhooper

jmhooper commented Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

I'm assuming there are specs that test cache_profiles with 0, 1, and 2 active/pending profiles

I believe this may be what you are looking for:

it 'writes decrypted PII to user_session for multiple profiles' do

@jmhooper jmhooper merged commit a95d7a5 into main Nov 6, 2023
@jmhooper jmhooper deleted the jmhooper-profile-cacher-save branch November 6, 2023 17:25
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