Page MenuHomePhabricator

[lib][native][web] Parse keyserver keys payload and validate the result
ClosedPublic

Authored by tomek on Jan 17 2024, 8:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 11:38 PM
Unknown Object (File)
Mon, Oct 21, 1:36 AM
Unknown Object (File)
Fri, Oct 18, 12:53 AM
Unknown Object (File)
Fri, Oct 18, 12:53 AM
Unknown Object (File)
Fri, Oct 18, 12:53 AM
Unknown Object (File)
Fri, Oct 18, 12:53 AM
Unknown Object (File)
Fri, Oct 18, 12:53 AM
Unknown Object (File)
Sep 21 2024, 6:17 PM
Subscribers

Details

Summary

A signature of the returned keys changes - the new one is more convenient to use with Olm. Additionally, this diff introduces more efficient validation.

Depends on D10665

Test Plan

Check if fetching keys returns correct values on native and web.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek held this revision as a draft.
tomek published this revision for review.Jan 17 2024, 8:53 AM
native/identity-service/identity-service-context-provider.react.js
97 ↗(On Diff #35728)

Should we always validate responses from the identity service? Wondering if we've discussed a "best practice" for this, and whether it's being done in all cases

100 ↗(On Diff #35728)

Can you explain why this is necessary?

Is there a better alternative, such as adding a typehint to the resultObject definition on line 78?

Some notes about convention so please address comments but overall LGTM

lib/types/identity-service-types.js
5–6 ↗(On Diff #35728)

can be merged

43 ↗(On Diff #35728)

I have mixed feelings about this.

It assumes that oneTimeKey is mandatory - while in proto file it is optional.

We should I think check it before and throw an error before that oneTimeKey is missing and you can not create a session rather than a validation error - because that is not true in some sense.

native/identity-service/identity-service-context-provider.react.js
97 ↗(On Diff #35728)

I think it makes a lot of sense on the web because we get responses directly to the JS side, but on the native, it first goes through statically typed Rust and any issues with type should probably cause panic or err on Rust side, like here, but we then return this value as string so maybe it make some sense? And we also already have validators for the web we can make it more "symmetric".

If we agree to add tcomb validation also here I will add this to the methods I implemented while adding those to this interface.

100 ↗(On Diff #35728)

Is there a better alternative, such as adding a typehint to the resultObject definition on line 78?

Personally, I don't like it, because in line 78 we don't know it yet, we can truly say which type it is after validation, this is "lying for a moment" - but not feel strongly about this and this is only my personal opinion

I think we have convention like here (I've seen it multiple times in the codebase) which at a logical point makes the most sense and avoids explicit any-casting

This revision is now accepted and ready to land.Jan 18 2024, 10:25 AM
lib/types/identity-service-types.js
43 ↗(On Diff #35728)

Makes sense - I'll update the validation

native/identity-service/identity-service-context-provider.react.js
97 ↗(On Diff #35728)

Adding validation on native makes the code easier to reason about - we don't need to check the Rust implementation to make sure that the shape is ok (we're sending a plain string, so it's like casting through any). Overall, the more validation we have, the better - it's a lot easier to deal with inconsistencies when they are caught on validation than when they are caught later in a random place in the logic.

100 ↗(On Diff #35728)

It looks like we can use assertWithValidator function that will either throw or return the correct type.

tomek marked 6 inline comments as done.

Improve validation