fix(oauth): Move keyFetchToken and unwrapBKey to sensitiveDataClient #17994
Conversation
| (sensitiveData as unknown as { emailForAuth: string; authPW: string }) || | ||
| {}; | ||
| const { authPW, emailForAuth, unwrapBKey } = | ||
| (sensitiveData as { |
There was a problem hiding this comment.
Can we reuse the sensitive data client type?
There was a problem hiding this comment.
I think I'm just going to punt this to that same follow up mentioned. There are files I didn't touch in this PR that are doing something similar to this as well and I think it makes sense to look at this in a non-dot release scenario.
packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx
Outdated
Show resolved
Hide resolved
| @@ -112,7 +117,8 @@ export const InlineRecoveryKeySetupContainer = (_: RouteComponentProps) => { | |||
| !unwrapBKey || | |||
| !email || | |||
| !emailForAuth || | |||
There was a problem hiding this comment.
Is !emailForAuth duplicated here?
There was a problem hiding this comment.
Thanks for the catch. Weird that our linter doesn't complain about this, lol.
| @@ -125,6 +149,11 @@ export const InlineRecoverySetupContainer = ({ | |||
| if (oAuthDataError) { | |||
| return <OAuthDataError error={oAuthDataError} />; | |||
| } | |||
| // Note that we don't currently need this check on this page right now since AMO is the only | |||
| // RP requiring 2FA and it doesn't require keys. However it's here for consistency. | |||
There was a problem hiding this comment.
Very much appreciate the comments you've sprinkled throughout 🫶
| * password in a browser session restart/crash because the browser stores the flow state in | ||
| * memory and the query params won't match after a browser session is restored. | ||
| * | ||
| * TODO: Can we check session storage for if the user refreshed so we can redirect them |
There was a problem hiding this comment.
Have we had input from UX on how to display this error? Or have a ticket filed to review UX? I know it's a edge case, but I don't love that we display a screen with nothing but an error message and no path forward.
There was a problem hiding this comment.
Mark and I agreed on this for now, and also agreed it wasn't the best. I did note that reset PW refresh ticket in a comment here because I'm thinking we can bump up the points to a 3 and use that ticket to handle refreshes better here as well (if there's a refresh and not a browser crash we can redirect to /signin), but if a browser crash happens, the user really needs to just restart the flow from the profile menu.
LZoog
force-pushed
the
FXA-10709
branch
4 times, most recently
from
ff1ab74
to
1df5bb2
Compare
packages/fxa-settings/src/pages/Signup/ConfirmSignupCode/container.test.tsx
Outdated
Show resolved
Hide resolved
Because:
This commit:
closes FXA-10709, closes FXA-10529