Details

Reviewers

michal
kamil

Commits

rCOMM382fda8c9fb9: Expose major version of desktop code to the keyserver

Summary

This differential exposes major version of desktop code to the keyserver. It is necessary since ability to decrypt desktop notifications deoends on both web
and desktop code. Those two can be out of sync if user doesn't accept desktop app update.

Test Plan

Apply this patch to the keyserver code: https://gist.github.com/marcinwasowicz/33a1d560f092a38d12ad9fa1e88c9eb1
Log-in from dev build of desktop app.
Use select * from olm_sessions; in MariaDB console.
Ensure that no session was created.
Update NEXT_CODE_VERSION to 0.
Use select * from olm_sessions; in MariaDB console.
Ensure that new session was created for cookie id associated with currently logged in user in desktop app.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Nov 24 2023, 9:02 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptNov 24 2023, 9:02 AM

marcin added a parent revision: D9964: Implement main <-> renderer IPC and use it to decrypt notifications.Nov 24 2023, 9:03 AM

Fix NEXT_CODE_VERSION accidentally set to 0

marcin edited the test plan for this revision. (Show Details)Nov 24 2023, 9:10 AM

Harbormaster completed remote builds in B24399: Diff 33598.Nov 24 2023, 9:38 AM

Harbormaster completed remote builds in B24400: Diff 33599.Nov 24 2023, 9:55 AM

marcin requested review of this revision.Nov 24 2023, 9:55 AM

marcin added a child revision: D10056: Rename IOS to APNs in function responsible for notifications encryption.Nov 27 2023, 6:45 AM

michal requested changes to this revision.Nov 28 2023, 2:19 AM

michal added inline comments.

lib/shared/version-utils.js
42–44 ↗	(On Diff #33599)	We probably don't want to return here if there is `minMajorDesktopVersion` specified. Additionally we should check that both `minCodeVersion` and `minMajorDesktopVersion` pass if they are both included. Otherwise a client could have a new web version with an older desktop version and still pass this check.
87 ↗	(On Diff #33599)	Nit: if we aren't depending on mutating the array, I find `[0]` much more straightforward than `shift`
lib/types/device-types.js
29 ↗	(On Diff #33599)	Should we use `isDesktopPlatform` in this?
46 ↗	(On Diff #33599)	Is there a reason we don't just type this as `+majorDesktopVersion?: number`. Currently it would be undefined on native, null on web, and a number on desktop. I don't think we need this distinction between web and native, we already have the platform field.
web/app.react.js
97–99 ↗	(On Diff #33599)	Related to my other comment, but I think it would be better to just not pass anything on web instead of null. `majorDesktopVersion` doesn't apply to the web in the same way as it doesn't apply to native.

This revision now requires changes to proceed.Nov 28 2023, 2:19 AM

marcin added inline comments.Nov 28 2023, 2:40 AM

lib/shared/version-utils.js
42–44 ↗	(On Diff #33599)	We probably don't want to return here if there is minMajorDesktopVersion specified. I will introduce that. Additionally we should check that both minCodeVersion and minMajorDesktopVersion pass if they are both included. Otherwise a client could have a new web version with an older desktop version and still pass this check. This is indeed checked against. This code doesn't allow new web version to pass if desktop version is too old.

michal added inline comments.Nov 28 2023, 3:25 AM

lib/shared/version-utils.js
42–44 ↗	(On Diff #33599)	This is indeed checked against. This code doesn't allow new web version to pass if desktop version is too old. Oh right, sorry I got confused!