Details

Reviewers

inka
kamil
atul
ashoat

Commits

rCOMM24c786a1a2bf: [keyserver] Remove cookieSources

Summary

ENG-4747
Depends on D9291

After D9290 and D9291 all cookies are from the request body so we can stop tracking this information. This diff removes cookieSources and simplifies all conditionals where it mattered.

Note: Like D9291, landing this diff will need to be defered for a while to allow older clients to migrate

Test Plan

yarn flow-all, check if web app works. Set the current web version as unsupported and check that the user gets logged out.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-web-cookie

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Oct 16 2023, 5:31 AM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 16 2023, 5:31 AM

michal added inline comments.Oct 16 2023, 5:33 AM

keyserver/src/socket/socket.js
212–217 ↗	(On Diff #32034)	`fetchViewerForSocket` no longer can return null
279 ↗	(On Diff #32034)	After removing the code block above we never call this without a viewer, so we can add an invariant like in the other errors.

michal edited the summary of this revision. (Show Details)Oct 16 2023, 5:35 AM

michal added a parent revision: D9291: [keyserver] Stop accepting http cookies.

michal removed a parent revision: D9290: [keyserver] Stop sending http cookies.

Harbormaster completed remote builds in B23261: Diff 32034.Oct 16 2023, 5:49 AM

michal requested review of this revision.Oct 16 2023, 5:49 AM

Looks reasonable, thanks for updating comments.

Adding @ashoat as blocking since he has more context and anything having to do w/ auth/sessions is important to get right

I think one of the conditions in the existing code had a mistake and was accidentally reversed

keyserver/src/session/cookies.js
52–69 ↗	(On Diff #32034)	We should update types when we touch them. Can you make these types readonly? (In the future, please make sure to update the types before your reviewer notices... ideally I don't have to repeat myself)
keyserver/src/socket/socket.js
304 ↗	(On Diff #32034)	It seems like we should add a `sessionChange` here since the cookie is being deleted. The comment you removed from `AuthErrorServerSocketMessage` explained that `sessionChange` was only omitted for `HEADER` cookies... now that we don't use those, shouldn't we include the `sessionChange`? I suspect that the condition on the old line 313 was accidentally reversed from the start... based on the removed comment, it seems like `sessionChange` should be provided for `BODY` cookies but NOT for `HEADER` cookies.
lib/types/socket-types.js
335–343 ↗	(On Diff #32034)	Update types please The comment that you removed seems to imply that `sessionChange` should now always be specified. Is that the case?

This revision now requires changes to proceed.Oct 18 2023, 1:51 PM

Update types and client_version_unsupported logic

keyserver/src/socket/socket.js
304 ↗	(On Diff #32034)	Thank you for the comment, that makes a lot of sense. I haven't considered the the existing code was wrong... I have updated the code to always include the `sessionChange` and amended the test plan for web (checks that the user is logged out after `client_version_unsupported`). Additionally I think I have discovered more issues with handling `client_version_unsupported` on native and created a task here: ENG-5648.
lib/types/socket-types.js
335–343 ↗	(On Diff #32034)	Updated the `sessionChange` so that it's always specified