[keyserver] Introduce `isCookieMissingSignedIdentityKeysBlob`
ClosedPublic
Actions

Authored by atul on Mar 12 2023, 4:15 PM.

Details

Reviewers

ashoat

Commits

rCOMM34785f4641f4: [keyserver] Introduce `isCookieMissingSignedIdentityKeysBlob`

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 created this revision.Mar 12 2023, 4:15 PM

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

fix..

Harbormaster completed remote builds in B17266: Diff 23654.Mar 12 2023, 4:30 PM

Harbormaster completed remote builds in B17267: Diff 23655.Mar 12 2023, 4:34 PM

atul added a child revision: D7051: [keyserver] Send `SIGNED_IDENTITY_KEYS_BLOB` request to client if data missing in DB.Mar 12 2023, 5:59 PM

atul removed a reviewer: tomek.Mar 13 2023, 7:35 AM

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

atul added inline comments.Mar 14 2023, 11:40 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.

atul added inline comments.Mar 14 2023, 12:20 PM

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 );