[identity] Add GetDeviceListForUser RPC
ClosedPublic
Actions

Authored by bartek on Dec 12 2023, 12:55 AM.

Details

Reviewers

ashoat
varun
michal

Commits

rCOMM7892c40d9141: [identity] Add GetDeviceListForUser RPC

Summary

Resolves ENG-5049. According to the whitepaper and the Linear task, client needs all device list updates since the last time it asked for them. This RPC is used to get device list history for a user.

The response format complies to the whitepaper (Fig. 4) although it doesn't include signing information yet (this can be easily added in the future).

Depends on D10247

Test Plan

Similiar to D10247 test plan:
Added three device list updates to DDB. Then called the RPC

Without timestamp, all three are returned. They're JSON-encoded and JSON.parse() returns a valid JS object.
Given timestamp T, where U1 < T < U2 < U3, the function returned U2 and U3.
Empty list is returned if timestamp is after all updates.
Empty list is returned if the user has no device lists yet (of the new format, i.e. not yet migrated).

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Dec 12 2023, 12:55 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptDec 12 2023, 12:55 AM

bartek added a child revision: D10302: [identity] Make set_prekey() update devices table too.Dec 12 2023, 12:58 AM

Harbormaster completed remote builds in B24972: Diff 34510.Dec 12 2023, 1:33 AM

Added @ashoat for proto changes, @varun and @michal for rust

bartek mentioned this in D10257: [services] Fix warnings.Dec 12 2023, 4:21 AM

Some questions

shared/protos/identity_authenticated.proto
167	Ultimately this is supposed to match the following format: SignedDeviceList = { "rawDeviceList": "{\"devices\":[\"6iVZ6YnAZXt8ZmR9IsCHPV1pKss8lu5 yUgF6oNxVoHw\",\"4ManHI400UxeQqoTb700qd3se2fg6RAC8CTvrejW4Rs\", \"JpCLbtkVSKGpiMqdlmSD37X2EUYOlfyeT1P5ciL_Ph8\"],\"timestamp\": 1699077686}", "curPrimarySignature": "UgZP0jLUo5RsOvnfGzcyqgHo_esmNNvnjdL b1DOrz8Qbcw5SXL-A_NmOak1jmkfUP2q4aiSOYdYirUKVU-sNDg", "lastPrimarySignature": "3T6zpXnuImKaS1lA7TAFGdenR_ApeptQLL- e5KanBMWexqtaHJV_TFb3mR6hpN6cqwjsg7b3NWzO5Pi0VeBoAw" } Do we really want each field to be represented individually in the gRPC message? I think it may be better to represent the whole thing as eg. a stringified JSON blob, in case (for instance) the client needs to persist the `SignedDeviceList` locally. By using a more "cross-platform" format (such as a stringified JSON blob) we allow for more consistency in how this blob is stored.
169–172	This generally seems to match the whitepaper: RawDeviceList = { "devices": [ "6iVZ6YnAZXt8ZmR9IsCHPV1pKss8lu5yUgF6oNxVoHw", "4ManHI400UxeQqoTb700qd3se2fg6RAC8CTvrejW4Rs", "JpCLbtkVSKGpiMqdlmSD37X2EUYOlfyeT1P5ciL_Ph8" ], "timestamp": 1699077686000 } However, the order of keys appears to be different. This is important because the signature will be different. Can we enforce an ordering that matches the whitepaper?

This revision now requires changes to proceed.Dec 12 2023, 7:21 AM

bartek planned changes to this revision.Dec 13 2023, 1:06 AM

bartek added inline comments.

shared/protos/identity_authenticated.proto
167	That's a really good point! I totally agree with this one, I'll do this.
169–172	Yes, serialization order is deterministic for user-defined `struct`s in Rust. The struct defined above (L422 in `authenticated.rs`) has correct order, this doc comment is just wrong. I'll fix it

Made SignedDeviceList a JSON-stringified object
Added unit tests to make sure that serialized JSON has correct format
Fixed field order in doc comment

bartek added inline comments.Dec 14 2023, 7:08 AM

services/identity/src/grpc_services/authenticated.rs
502–503 ↗	(On Diff #34641)	These are expected items of the `repeated string device_list_updates` proto response. These lines are >80 chars long, I don't know why `cargo fmt` forces this to be single-line.

Harbormaster completed remote builds in B25076: Diff 34641.Dec 14 2023, 7:46 AM

ashoat accepted this revision.Dec 14 2023, 9:48 AM

just one q inline

services/identity/src/grpc_services/authenticated.rs
406 ↗	(On Diff #34641)	if they're not sorted do we need to fix the state somehow?
429 ↗	(On Diff #34641)	device
453 ↗	(On Diff #34641)	love how easy you've made it to add signing in the future

This revision is now accepted and ready to land.Dec 14 2023, 10:15 PM

Fix typo

bartek added a child revision: D10221: [identity] Add devices to the new list after login and register.Jan 5 2024, 12:54 AM

bartek removed a child revision: D10302: [identity] Make set_prekey() update devices table too.

bartek added inline comments.

services/identity/src/grpc_services/authenticated.rs
406 ↗	(On Diff #34641)	Not sure if this is possible and if ever really happens. If they're not sorted, this means that DDB engine failed to sort items by sort key internally.