Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Session] Fix stale emailAuthFactor and emailConfirmed on the client #3835

Merged
merged 2 commits into from
May 3, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 3, 2024

Extracted from #3728. The fix is b0aa70d.

The intent of this code was to use the value from the session and fallback to the stored value if the session isn't present. However, if session.emailAuthFactor (newer) is false but account.emailAuthFactor (older) is true, the session?.emailAuthFactor || account.emailAuthFactor expression will keep giving us true even though false is the fresh value here. So we'll store and display the incorrect stored true value.

The fix is to use ?? instead. I've updated a few other places we use || for fallback values for consistency in the second commit, but that shouldn't affect the behavior.

Test Plan

To trigger this, you'd need to get a "session changed" event from the agent with the new session info. One way you can get there is by having your token expired. I don't know how to make that happen though.

A simpler way to trigger it is to comment out these lines so that resumeSessionWithFreshAccount() itself starts, but then we're rely on the logic in onAgentSessionChange to actually apply the update.

Then the test plan becomes:

  1. Switch to main and comment out the lines mentioned above
  2. In another browser window, open prod and turn on 2FA in Settings
  3. Refresh browser locally, observe that 2FA flag is on in Settings
  4. In another browser window, open prod and turn off 2FA in Settings
  5. Refresh browser locally, observe that 2FA flag is still on in Settings
  6. Refresh browser locally, observe that 2FA flag is still on in Settings
  7. Switch to this branch and comment out the lines mentioned above
  8. Refresh browser locally, observe that 2FA flag is off in Settings, as expected

From that point on, toggling it off and on in another browser window is correctly applied on session refresh.

Copy link

render bot commented May 3, 2024

Copy link

github-actions bot commented May 3, 2024

Old size New size Diff
6.86 MB 6.86 MB 64 B (0.00%)

@gaearon gaearon merged commit c13685a into main May 3, 2024
6 checks passed
@gaearon gaearon deleted the session-more branch May 3, 2024 01:52
@gaearon gaearon mentioned this pull request May 8, 2024
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.

2 participants