[lib] Don't clear authoritative keyserver cookie during identity auth
ClosedPublic
Actions

Authored by ashoat on Fri, Apr 12, 2:13 PM.

Details

Reviewers

inka

Commits

rCOMM7fe96ea72b76: [lib] Don't clear authoritative keyserver cookie during identity auth

Summary

IDENTITY_REGISTER_SUCCESS and IDENTITY_LOG_IN_SUCCESS currently result in the authoritative keyserver's cookie being cleared (anonymous cookie replaced with null), which results in a bunch of rerenders. It's one of the factors contributing to ENG-7385, since it results in the very first keyserverAuth attempt (triggered by SET_ACCESS_TOKEN) being cancelled almost immediately (by IDENTITY_REGISTER_SUCCESS).

Test Plan

I added some utils that track why a memo is being re-run, and was able to confirm that the cookie was no longer causing memos to re-run

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Fri, Apr 12, 2:13 PM

Herald added a subscriber: tomek. · View Herald TranscriptFri, Apr 12, 2:13 PM

ashoat added a child revision: D11655: [lib] Introduce usePreRequestUserState.Fri, Apr 12, 2:14 PM

Harbormaster completed remote builds in B28202: Diff 39092.Fri, Apr 12, 2:30 PM

ashoat requested review of this revision.Fri, Apr 12, 2:30 PM

I worry that this may cause problems. Identity login and register clear data in case they are dispatched before logout success, so before user's data is cleared. This change makes it technically possible that the following will happen:

User A logs in with identity and the keyserver - gets a user cookie
User A logs out
User B logs in, and a login success is dispatched before logout success

Now user B has user A's keyserver cookie

It is also weird to me that we try to auth with the keyserver before identity login success. Maybe we should change that?

It is also weird to me that we try to auth with the keyserver before identity login success.

What do you mean by "identity login success"? keyserverAuth is not initiated until a CSAT is in the store, and the CSAT is populated only after identity login success.

Are you perhaps referring specifically to the IDENTITY_LOG_IN_SUCCESS action? If so: yes, this is the case. It's happening because SET_ACCESS_TOKEN is getting dispatched before IDENTITY_LOG_IN_SUCCESS.

Maybe we should change that?

What would be achieved? I agree it might be better, but to prioritize this right now I think we need a strong reason.

Note that our legacy keyserver auth logic works somewhat similarly (cookie is set by SET_NEW_SESSION before LOG_IN_SUCCESS is dispatched).

I worry that this may cause problems. Identity login and register clear data in case they are dispatched before logout success, so before user's data is cleared. This change makes it technically possible that the following will happen:

User A logs in with identity and the keyserver - gets a user cookie

User A logs out

User B logs in, and a login success is dispatched before logout success

Now user B has user A's keyserver cookie

That's a fair concern. I can think of two potential solutions:

We make sure that we only keep the cookie if it's an anonymous cookie.
We make sure that we only keep the cookie if the user was previously logged-out.

I think option 1 is more simple. Option 2 could be achieved by looking at either preRequestUserState or perhaps by looking at the currentUserInfo in Redux prior to the action, but both options are potentially more complicated than option 1.