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)

Created a separate task to track, since this isn't directly related to the issue I am solving, which is It's possible to be logged in and not have a CSAT (ENG-7247): ENG-8046
Also this can be easily fixed in a separate diff, so this will help not make this one any larger

Created ENG-8047 to track, since it can be done in a separate diff

I think the logic would be correct, in the sense the user is only asked to accept the TERMS_OF_USE_AND_PRIVACY_POLICY by the current UI, so this is the only policy which we should mark as accepted.
So I cannot mark any other policies as accepted. On the other hand, we seem to require that all baseLegalPolicies are accepted to proceed. This is probably an inconsistency that should be addressed, but I think this is outside of the scope of this task.
Created ENG-8049 to track. Please let me know if you think I should take care of this ASAP, but I don't think this is in scope of this issue

My comment is addressing 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 used to be called at the beginning of processOLMAccountCreation, so we just move it here

This can be handled in a separate diff, since this is not an issue introduced by this diff. Created ENG-8052 to track

Actually, wouldn't it be way simpler to just call verifyCalendarQueryThreadIDs earlier, such as on line 756? (step 1)

My intention with this was imitate how the code worked before my changes. As I wrote in the inline below, this used to be called at the beginning of processOLMAccountCreation, which was called inside olmAccountCreationPromise.
I wanted to avoid introducing any changes to when verifyCalendarQueryThreadIDs is called, to reduce the complexity of this diff, and because that is not related to the issue I'm solving
Created ENG-8058 to track. Please let me know if you think this should be prioritised, but I don't think this is in scope of this issue