Ensure OTKs are uploaded when the session is created

[bmarty]

The sync response can omit the field device_one_time_keys_count.signed_curve25519 and the SDK was waiting to know this value to eventually upload some OTKs, even after a session creation.

Now the SDK uploads the OTK when it uploads the device keys.

This is due to recent change on Synapse sync response: https://github.com/matrix-org/synapse/pull/10214/files/7978990f5139067e1868d100d223e28e4447d2fb#diff-70f29654c1c37fb886340dc6f22a99571503f4a54dc46942ca6028b8896b00b7L254

[BillCarsonFr]

LGTM.
Only issue is that existing devices impacted won't recover the missing sessionIds (even key request should fail with withheld I think).
Only option could be to do a megolm key import on background (if there is a backup and the keys were uploaded by another session)

[BillCarsonFr]

I think we might need to force to upload new OTK with this patch

[bmarty]

I think we might need to force to upload new OTK with this patch

Done, see commit 0598810

[BillCarsonFr]

just a bit of logging for errors?

[deepbluev7]

You are already uploading one time keys here if deviceOneTimeKeysCount != null but signedCurve25519 is null: https://github.com/vector-im/element-android/blob/2864101e2aa2368ceac89e7532b6944530a8bdb4/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt#L417

Considering that you already upload keys when the subfield is null, why not do so, when deviceOneTimeKeysCount is null? Then you don't need to force an OTK upload after the upgrade and the logic would work fine with the synapse changes. Yes, the spec isn't clear here, but you are already depending on the subfield to not be null. I think you can extend the requirement to the whole parent object.

[bmarty]

You are already uploading one time keys here if deviceOneTimeKeysCount != null but signedCurve25519 is null:

https://github.com/vector-im/element-android/blob/2864101e2aa2368ceac89e7532b6944530a8bdb4/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt#L417

Considering that you already upload keys when the subfield is null, why not do so, when deviceOneTimeKeysCount is null? Then you don't need to force an OTK upload after the upgrade and the logic would work fine with the synapse changes. Yes, the spec isn't clear here, but you are already depending on the subfield to not be null. I think you can extend the requirement to the whole parent object.

I'm not sure to follow you. In our case syncResponse.deviceOneTimeKeysCount is always null so we are stucked.

[deepbluev7]

Instead of

                if (syncResponse.deviceOneTimeKeysCount != null) {
                    val currentCount = syncResponse.deviceOneTimeKeysCount.signedCurve25519 ?: 0
                    oneTimeKeysUploader.updateOneTimeKeyCount(currentCount)
                }

do:

                val currentCount = syncResponse.deviceOneTimeKeysCount?.signedCurve25519 ?: 0
                oneTimeKeysUploader.updateOneTimeKeyCount(currentCount)

The difference is minimal. Now you also update OTKs, if the field is ompletely missing instead of just the signedCurve25519 field being missing. Since you already interpret missing fields as being 0, this should not really be a change from the spec side, even if that spec is unclear if the field can be missing, when nothing changed. And it fixes it even on synapses that leave out the field completely without any need for migration, I think. (I am not familiar with kotlin or the codebase though)

Or in other words, if (syncResponse.deviceOneTimeKeysCount != null) doesn't make much sense. Either remove it or do if (syncResponse.deviceOneTimeKeysCount != null && syncResponse.deviceOneTimeKeysCount.signedCurve25519 != null). Removing it fixes the OTK issue reliably.

[bmarty]

@deepbluev7 if syncResponse.deviceOneTimeKeysCount is null it does not mean that the server has 0 OTK, it mean that the server does not tell us how many OTK he has.

[deepbluev7]

@bmarty Why does signedCurve25519 being null tell us then, that the HS has 0 OTKs? Imo that is weird and doesn't match the logic I have seen in other clients. But I guess that needs some spec clarification.

[bmarty]

But I guess that needs some spec clarification

definitly

Empirically, when Synapse sends syncResponse.deviceOneTimeKeysCount, signedCurve25519 is not null. But I agree with you that if it was not the case, we should not fallback to 0 as well...

We keep the nullability everywhere because of optionality, and multiple server implementation / changes, so it always a bit painful to deal with such fallback.