chore(settings): Move remaining SensitveDataClient calls to new function #18136
Conversation
jonalmeida
force-pushed
the
FXA-10849
branch
from
5513415 to
9a45c0f
Compare
| @@ -67,7 +67,10 @@ export async function tryFinalizeUpgrade( | |||
| } finally { | |||
| // Clear out the state. No reason to keep trying this... | |||
| // Note that the upgradeClient will report issues to Sentry. | |||
| sensitiveDataClient.KeyStretchUpgradeData = undefined; | |||
| sensitiveDataClient.setDataType( | |||
There was a problem hiding this comment.
Eek, missed this one in the previous large patch that landed yesterday after fixing the conflicts during the rebase.
jonalmeida
force-pushed
the
FXA-10849
branch
from
9a45c0f to
9d20398
Compare
| const newRecoveryKey = formatRecoveryKey(newRecoveryKeyData.buffer); | ||
| // We keep the previous non-null assertion on 'newRecoveryKey' here because the | ||
| // flow dictates we definitely have it. This is not good practice. | ||
| const { recoveryKey: newRecoveryKeyData } = sensitiveDataClient.getDataType( |
There was a problem hiding this comment.
That's a good call out. I'm wondering what the alternate would be - if recoveryKey is undefined, navigate to ResetPasswordConfirmed?
There was a problem hiding this comment.
Looks like this is causing a type error that we do want to handle: https://mozilla-hub.atlassian.net/browse/FXA-10869
There was a problem hiding this comment.
Yeah, something like that would work. Basically the same flow/style how we use CompleteResetPasswordContainer.hasConfirmedRecoveryKey.
Would you prefer I should fix that in this patch, or in an (immediate) follow-up?
There was a problem hiding this comment.
Looks like this is causing a type error that we do want to handle: https://mozilla-hub.atlassian.net/browse/FXA-10869
Oh cool. To double-check for my own sanity, I verified that my previous patch hasn't made it into the release where this exception occurs so this patch won't fix the problem, but it at least won't make it worse!
I think a separate patch for FXA-10869 will work with your solution.
There was a problem hiding this comment.
I think a separate patch for FXA-10869 will work with your solution.
I've updated this patch to point to the bug for now.
| @@ -64,7 +63,6 @@ const AccountRecoveryConfirmKeyContainer = (_: RouteComponentProps) => { | |||
| uid | |||
| ); | |||
|
|
|||
| sensitiveDataClient.setData('reset', { kB }); | |||
There was a problem hiding this comment.
Looking at this - I think the intent was to remove kB from location state and pass with sensitiveDataClient but we may have missed retrieving it from sensitiveDataClient in the CompleteResetPassword container. I don't think the duplication with location state was intentional, and we should treat kB as sensitive.
There was a problem hiding this comment.
Thanks for the catch, fixed!
jonalmeida
force-pushed
the
FXA-10849
branch
from
9d20398 to
9d30323
Compare
jonalmeida
force-pushed
the
FXA-10849
branch
from
9d30323 to
772499d
Compare
| private sensitiveData: { [key: string]: object } = {}; | ||
|
|
||
| // TODO: Fast follow, use this pattern instead for simpler and better type safety. | ||
| public KeyStretchUpgradeData: |
There was a problem hiding this comment.
Why store things in a map when we could just store on an object?
There was a problem hiding this comment.
The map already existed before. I had moved the KeyStretchUpgradeData in a previous patch - this was the remaining bit.
If you're asking for the general case - my goal was to create a pattern that allowed others to copy-paste without thinking about it too much. I don't have strong opinions besides consistency, so if we get everything using the same pattern we can move everything over to holding it in either one.
There was a problem hiding this comment.
I was one of the original reviewers on the sensitive data client. I wish I would have said something/caught this when it was created. When I added the KeyStrechUpgrade data field, it was my intent to go back and clean this up by switching over to using object fields.
As far as I can see, there really isn't a reason to have the map. We can use fields on the object directly. I'd recommend fixing this up. IMO, using a generic here is a misuse of generics.
Because
This pull request
setData/getDatawithsetDataType/getDataType.Issue that this pull request solves
Closes: FXA-10849
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
n/a
Other information (Optional)
n/a