The context is here. Previously desktop devices would present themselves as web during device key upload. Platform details from device key upload always win against platform details from platform details synchronization.
Details
- Build desktop app before applying this commit.
- Check that platform details for its device id in dynamodb suggest that it is a web platform.
- Apply this diff and reload desktop app.
- Repeat point 2. Ensure that platform details for its device id in dynamodb suggest that it is a desktop device
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-8900
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
web/grpc/interceptor.js | ||
---|---|---|
37–43 ↗ | (On Diff #42845) | I am of opinion that identity protos should accommodate to types that we have been using across the codebase. Therefore they should be updated to use MACOS instead of MAC_OS`. Once it is done this temporary change will be deleted. |
now that we supply platform details as metadata, should we be sending the device type in the key upload at all? seems like we should just remove it? cc @bartek
Not entirely sure, but probably yes. Would be good to create a follow-up task to discuss this more.
lib/types/identity-service-types.js | ||
---|---|---|
338–346 ↗ | (On Diff #42845) | Should this be an Object.freeze? |
web/grpc/interceptor.js | ||
37–43 ↗ | (On Diff #42845) | Should we update this proto enum? We'd have to rename MAC_OS -> MACOS to get rid of this workaround. Might not be worth though, if we're going to deprecate device type in DeviceKeysUpload |
Use Object.freeze for constants mapping from platform types to identity platform types. Rebase before landing
web/grpc/interceptor.js | ||
---|---|---|
37–43 ↗ | (On Diff #42896) | Why didn't we use the number format for the deviceType here instead of introducing a new string one that was inconsistent with the existing string on device-types.js? Any background on this decision? |
web/grpc/interceptor.js | ||
---|---|---|
37–43 ↗ | (On Diff #42896) | The gRPC metadata are string values, and device_type metadata was always a string, since it was introduced. One of the reasons is that it can also have non-enum values, like service in service-to-service communication. Using this metadata also for updating device types in DDB (SyncPlatformDetails RPC) required converting this string metadata to the proto-generated Rust code, which was generated like this: // *** Proto-generated code *** impl DeviceType { /// String value of the enum field names used in the ProtoBuf definition. /// /// The values are not transformed in any way and thus are considered stable /// (if the ProtoBuf definition does not change) and safe for programmatic use. pub fn from_str_name(value: &str) -> ::core::option::Option<Self> { match value { "KEYSERVER" => Some(Self::Keyserver), "WEB" => Some(Self::Web), "IOS" => Some(Self::Ios), "ANDROID" => Some(Self::Android), "WINDOWS" => Some(Self::Windows), "MAC_OS" => Some(Self::MacOs), _ => None, } } } We're using this function directly to convert metadata values. So the problem is an inconsistency between strings in device-types.js vs strings generated by proto enum. We could fix this server-side as well by writing a custom conversion. But my opinion is that we should go one step further and deprecate the proto-generated enum (as suggested by @varun: https://phab.comm.dev/D12900#365033) and replace it with compatible enum introduced by @kamil in D12937. This is tracked by ENG-8904. |