Page MenuHomePhabricator

[keyserver] Introduce `isCookieMissingSignedIdentityKeysBlob`
ClosedPublic

Authored by atul on Mar 12 2023, 4:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 10:16 PM
Unknown Object (File)
Thu, Mar 21, 1:01 AM
Unknown Object (File)
Fri, Mar 8, 6:49 PM
Unknown Object (File)
Fri, Mar 8, 6:49 PM
Unknown Object (File)
Fri, Mar 8, 6:49 PM
Unknown Object (File)
Fri, Mar 8, 6:49 PM
Unknown Object (File)
Tue, Mar 5, 11:09 PM
Unknown Object (File)
Tue, Mar 5, 11:09 PM
Subscribers
None

Details

Summary

Checks if there's an existing row in cookies table with id=cookieID with a NULL signed_identity_keys column.

We're going to use this check in handleInitialClientSocketMessage to determine whether or not to send SIGNED_IDENTITY_KEYS_BLOB request to client from keyserver.


Depends on D7049

Test Plan

Called function from handleInitialClientSocketMessage:

if (viewer.cookieID) {
  isCookieMissingSignedIdentityKeysBlob(viewer.cookieID);
}

and set breakpoint within isCookieMissingSignedIdentityKeysBlob to check whether queryResult was as expected when signed_identity_keys was NULL and a valid stringified SignedIdentityKeysBlob:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Mar 12 2023, 4:16 PM

I think this diff would be easier to review if you included this new function's usage in the same diff

keyserver/src/session/cookies.js
820–826 ↗(On Diff #23655)

I'm not a fan of if () return true else return false constructions. This sort of construction can always be rewritten:

return !!(
  queryResult.length === 1 &&
  queryResult[0].signed_identity_keys === null
);

Separately, I might consider either making the comparison > 0 or factoring out the check that there aren't 2 or more results into an invariant

This revision is now accepted and ready to land.Mar 13 2023, 11:58 AM
keyserver/src/session/cookies.js
820–826 ↗(On Diff #23655)

Personally prefer being explicit instead of the casting trick... but I'll update this diff.

keyserver/src/session/cookies.js
820–826 ↗(On Diff #23655)

Separately, I might consider either making the comparison > 0 or factoring out the check that there aren't 2 or more results into an invariant

It's totally impossible for more than one row to have the same id so I think it's fine to check === 1

return !!(
  queryResult.length === 1 &&
  queryResult[0].signed_identity_keys === null
);

Shouldn't this just be

return (
  queryResult.length === 1 &&
  queryResult[0].signed_identity_keys === null
);
keyserver/src/session/cookies.js
820–826 ↗(On Diff #23655)

Yeah

remove if then and just return

This revision was landed with ongoing or failed builds.Mar 14 2023, 12:24 PM
This revision was automatically updated to reflect the committed changes.