This is only called in keyserverAuthResponder

The cookie is created in processSuccessfulLogin called by keyserverAuthResponder

This is handled later in keyserverAuthResponder by processSuccessfulLogin

This needs the cookie. I moved this out to keyserverAuthResponder after processSuccessfulLogin, which creates the cookie.

We create the cookie here

processAccountCreationCommon was previously called inside processOLMAccountCreation (line 806 above). processOLMAccountCreation was only called if !existingUsername!

We didn't used to check this here, but we used to check that in all places where processSuccessfulLogin is used:

• in logInResponder always
• in keyserverAuth in case of registration
• in siweAuthResponder in case of registration

But I think since we were checking it always for login, we don't want to allow the query to ever contain non-existant threads. So we can just check it here

Why do we have an any type here? This should be typed better. Can you try this?

{
  +deviceTokenUpdateRequest?: ?DeviceTokenUpdateRequest,
  +platformDetails: PlatformDetails,
  +watchedIDs: $ReadOnlyArray<string>,
  ...
}

It would probably be better to actually just insert these three params directly into the ProcessSuccessfulLoginParams list

We should be marking policies as accepted on registration, but not on login. I think you've made it too easy for somebody to make a mistake here. This parameter isn't named in a way where it's clear that it should be specified only for registration. See my shouldMarkPoliciesAsAcceptedAfterCookieCreation suggestion below

Indentation is broken here

1. Why are we passing this in as a function? I think it would be more clear what's going on if we pass in shouldMarkPoliciesAsAcceptedAfterCookieCreation: boolean, and it also avoids some level of misdirection (it's confusing to see viewerAcknowledgmentUpdater called here when it's really being called in processSuccessfulLogin)
2. It appears that you're always passing in markPoliciesAccepted here, but it should only be called on registration, not on login

Oops, I was wrong about point 2 (got confused by the broken indentation)

Don't we have the same problem here that you solved for processSuccessfulLogin? It seems like the notif session creation needs to be attempted before we do any writes to the database, so that if it fails we don't end up in an inconsistent state.

I think it's confusing that siweAuthResponder calls processSuccessfulLogin, which has been modified to support handling this viewerAcknowledgmentUpdater call... but instead we handle it in a custom way here.

I think if we go with my shouldMarkPoliciesAsAcceptedAfterCookieCreation suggestion below, it should be clear that this corresponds to registration. So it makes sense for siweAuthResponder to set that in the registration case.

In general, I think it's better to have a consistent mechanisms for these things. Unifying codepaths makes it easier for the reader to understand how things work, and that makes it easier to update the code without making mistakes.

This async IIFE isn't doing anything anymore, and should have been removed. But this comment can be ignored because my next comment below suggests assigning the promise

Discussed this with @inka in our 1:1 and I suggested to replace these lines with the following for two reasons:

1. Slightly improves performance in the case where markPoliciesAccepted does not need to be called, by avoiding blocking the fetchNotAcknowledgedPolicies call on the cookie creation
2. Makes more explicit the requirement that the cookie is created before markPoliciesAccepted is called, and that fetchNotAcknowledgedPolicies should be called after markPoliciesAccepted
3. Lets us skip fetchNotAcknowledgedPolicies if we just marked the policies as acknowledged

const setNewCookiePromise = (async () => {
  const [userViewerData] = await Promise.all([
    createNewUserCookie(userID, {
      platformDetails: request.platformDetails,
      deviceToken,
      socialProof,
      signedIdentityKeysBlob,
    }),
    deleteCookie(viewer.cookieID),
  ]);
  viewer.setNewCookie(userViewerData);
})();
const policiesCheckAndUpdate = (async () => {
  if (markPoliciesAccepted) {
    await setNewCookiePromise;
    await markPoliciesAccepted(viewer);
    return [];
  }
  return await fetchNotAcknowledgedPolicies(
    userID,
    baseLegalPolicies,
  );
})();
const [notAcknowledgedPolicies] = await Promise.all([
  policiesCheckAndUpdate,
  setNewCookiePromise,
]);

Two quick notes on this logic:

1. Note that this logic will have to be updated to reflect my shouldMarkPoliciesAsAcceptedAfterCookieCreation suggestion below.
2. It would be good to make the relationship between baseLegalPolicies (passed to fetchNotAcknowledgedPolicies) and policyTypes.tosAndPrivacyPolicy (passed to viewerAcknowledgmentUpdater) more explicit. If somebody were to add another policy to baseLegalPolicies, it seems like this logic would break: we would be assuming it's okay to proceed because tosAndPrivacyPolicy has been acknowledged, but we're not checking about other policies

This check used to happen earlier in some cases, such as logInResponder.

1. Now the check doesn't even happen if the policies haven't been acknowledged
2. If the check happens and fails, it can leave us in a weird state where a user has a cookie but no session

I initially thought we could just move this call to the start of processSuccessfulLogin, but then it presents a different risk: if called during registration, we might see a situation where a user is added to the users table, but then a crash occurs when verifyCalendarQueryThreadIDs is called. On subsequent calls we would think that the user is already created, and as a result the policies would never be set as acknowledged.

Instead, it seems like we will need to move this check to occur earlier, and probably it will need to be called separately for each responder before any writes occur.

processSIWEAccountCreation handles cookie creation when siweAuthResponder is called for registration. But in that case, we don't seem to be doing anything to prevent processSuccessfulLogin from creating the cookie as well.

As a result, it looks like registration via siweAuthResponder results in two cookies being created. This issue was originally tracked in ENG-2767, but it looks like it was never fully fixed up. (cc @varun)

Prior to your changes, we could have passed cookieHasBeenSet to processSuccessfulLogin here to disable cookie creation. But now it seems like the best approach would be to do the same thing we did for processOLMAccountCreation:

1. Get rid of processSIWEAccountCreation
2. Inline the parts of processSIWEAccountCreation that handle creating the row in the users table
3. Rely on processSuccessfulLogin to handle the rest (cookie creation and policy stuff)
4. Have siweAuthResponder call createAndSendReservedUsernameMessage directly (only for registration)