Page MenuHomePhabricator

Read desktop code version during socket connection creation
ClosedPublic

Authored by marcin on Dec 4 2023, 5:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 2:44 AM
Unknown Object (File)
Fri, Nov 22, 1:57 AM
Unknown Object (File)
Thu, Nov 21, 8:51 PM
Unknown Object (File)
Thu, Nov 21, 5:02 AM
Unknown Object (File)
Thu, Nov 21, 5:01 AM
Unknown Object (File)
Thu, Nov 21, 5:01 AM
Unknown Object (File)
Thu, Nov 21, 5:01 AM
Unknown Object (File)
Thu, Nov 21, 5:01 AM
Subscribers

Details

Summary

This differential resolves urgent issue available under: https://linear.app/comm/issue/ENG-6007/macos-desktop-version-is-not-actually-exported-to-the-keyserver.
The issue consisted of 3 parts:

  1. During socket connection creation the keyserver simply ignored potential majorDesktopVersion field.
  2. The code in message-creator.js tried to read desktop code version from the database by looking for desktopCodeVersion field in fetched rows while it was actually persisted as majorDesktopVersion.
  3. Finally versionKeyRegex allowed exclusively strings in <number>|<number> format but we need it to allow <number>|<number>|<number> as well.

This differential addresses each of the issues listed above.

Test Plan
  1. Build desktop app connected to your local keyserver.
  2. Ensure that olm sessions are present both in MariaDB and IndexedDB.
  3. Send some notifications and ensure that version field is incremented with each notification.

Repeat for both dev and prod (testing for prod will require to remove isStaffOrDev condition).
Execute similar test for web and native (for native ignore IndexedDB check) in order to test that functionality for this platforms is not broken.

It is essential to note that my initial testing plans for the entire e2e MacOS notifications assumed that NEXT_CODE_VERSION was temporarily updated to 0 which was the root of the issue this differential resolves. This testing plan assumes that no changes to NEXT_CODE_VERSION are done and that it is executed on the master code as it is.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin edited the test plan for this revision. (Show Details)
marcin edited the test plan for this revision. (Show Details)
marcin requested review of this revision.Dec 4 2023, 5:38 AM

Could you also quickly test if everything works if someone is using an older version of the desktop app, one that doesn't advertise its version (you can just comment out the desktop version line in registerConfig)?

keyserver/src/session/cookies.js
112–130 ↗(On Diff #34179)

Nit: can this be simplfied like this?

This revision is now accepted and ready to land.Dec 4 2023, 6:20 AM
  1. Simplify changes made to fetchUserViewer.
  2. Rebase before landing