fix(sessions): Add env var for unverified session state + bandaid FE fix #19501
Conversation
| const skipTokenVerification = (request: AuthRequest, account: any) => { | ||
| // Skip all checks to simulate an unverified session token state | ||
| if (this.config.signinConfirmation.skipTokenVerification === true) { | ||
| return false; |
There was a problem hiding this comment.
The way this reads is confusing to me. The function is called skipTokenVerification and the config setting is called skipTokenVerification. When the config setting is true, the function returns false? Is that a mistake?
There was a problem hiding this comment.
Yeah it should, but I can update the name of this too since looking at it again it is a bit confusing. Basically this.config.signinConfirmation.skipTokenVerification is saying, "skip the checks done in this function and return the default" - and the default at the end of this function returns false.
Calling it skipSkipTokenVerification I guess is more accurate but is confusing too, so maybe this.config.signinConfirmation.tokenVerification with a default env var value of true, that when set to false it skips this and returns false, is more clear?
LZoog
force-pushed
the
FXA-12406-v2
branch
2 times, most recently
from
a65d3e8 to
cee11da
Compare
| storeAccountData(accountData); | ||
| } | ||
|
|
||
| isVerified = await session.isSessionVerified(); |
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
We paired on this, and I think once the tests are passing, should be good to go. 
LZoog
force-pushed
the
FXA-12406-v2
branch
7 times, most recently
from
3841d13 to
db4677a
Compare
Because: * Users in a non-Sync, non-2FA unverified session state are taken directly to Settings This commit: * Adds a new env var for testing to simulate an account/session in this state * Adds a bandaid fix for what auth-server is returning in this state around 'verified', which directly references the session verification state, until a better solution is implemented in a follow up * Adds our new auth strategy to the 'account/attached_client/destroy' endpoint closes FXA-12406
| testAccountTracker, | ||
| }) => { | ||
| const { email } = await signInSyncAccount( | ||
| const credentials = await testAccountTracker.signUp(); |
There was a problem hiding this comment.
I stole these functional test changes from this PR we had to revert.
We have this exact test except non-Sync in our react conversion directory, which is passing... this was going through Sync. I tested it manually in Sync and without Sync and the cached signin works as expected.
|
|
||
| @Query((returns) => SessionType) | ||
| @UseGuards(GqlAuthGuard) | ||
| @UseGuards(UnverifiedSessionGuard) |
There was a problem hiding this comment.
This was changed because we could not use session.isSessionVerified in some unverified session cases where it would be false, because it would fail with a "Must Verify" error. The whole point was to check if it was verified or not.
Because:
This commit:
closes FXA-12406
closes FXA-12402
closes partially? FXA-12361
I've sent DM's on Slack to Valerie/Dan about this PR. There is going to be a follow up issue I will file that explains some of the changes here and will correct them so we don't have to "bandaid" override what the server returns to us about the session state. I'll leave a note on the ticket as well for QA.
To test this, turn on the env var and sign up/verify an account normally. Sign out. Now when you sign in next with password, you'll get back an "unverified session" and you'll be taken to
signin_token_code. Similarly you can test this on the cached sign in for that token.I see locally there are a few settings tests failing around the key stretching upgrade - I think those will be quick to look at in the morning.