Page MenuHomePhabricator

[lib][native][web] Expose identity API by using a context
ClosedPublic

Authored by tomek on Jan 9 2024, 7:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 1:00 AM
Unknown Object (File)
Thu, Oct 24, 9:32 AM
Unknown Object (File)
Wed, Oct 23, 8:46 PM
Unknown Object (File)
Fri, Oct 18, 6:56 AM
Unknown Object (File)
Fri, Oct 18, 6:56 AM
Unknown Object (File)
Fri, Oct 18, 6:56 AM
Unknown Object (File)
Fri, Oct 18, 6:56 AM
Unknown Object (File)
Fri, Oct 18, 6:56 AM
Subscribers

Details

Summary

Create a context that exposes the API client and recreates it on every user change or token change. Simplify the client by avoiding a need to provide credentials on every call to the API.

Depends on D10449

https://linear.app/comm/issue/ENG-6404/move-identity-client-to-a-context

Test Plan

Checked if credentials are correctly provided to a client and if the proper context value is provided (on native and web).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 9 2024, 7:43 AM
Harbormaster failed remote builds in B25635: Diff 35437!
tomek requested review of this revision.Jan 9 2024, 8:20 AM
tomek edited the test plan for this revision. (Show Details)
native/identity-service/identity-service-context-provider.react.js
25–30 ↗(On Diff #35438)

Is this the best way to handle getting the deviceID? One risk is that it's possible for it to not be set by the time commRustModule.getKeyserverKeys is called.

I wonder if we should instead do something like:

const deviceIDPromiseRef = React.useRef<?Promise<string>>();
if (!deviceIDPromiseRef.current) {
  deviceIDPromiseRef.current = getContentSigningKey();
}

And then wherever deviceID is needed, we just call await deviceIDPromiseRef.current instead.

varun requested changes to this revision.Jan 9 2024, 9:54 PM
varun added inline comments.
native/identity-service/identity-service-context-provider.react.js
22 ↗(On Diff #35438)

should we use getCommServicesAuthMetadataEmitter to get the access token here?

This revision now requires changes to proceed.Jan 9 2024, 9:54 PM
native/identity-service/identity-service-context-provider.react.js
22 ↗(On Diff #35438)
native/identity-service/identity-service-context-provider.react.js
22 ↗(On Diff #35438)

If we go with the approach I proposed there, we can probably remove it from Redux, in favor of exposing it via this context

We'll also need an initial call to commCoreModule to get the current values of the auth metadata (the emitter just provides updates, I think)

It's unfortunate how different native and web are, but I think it makes sense given the technical decisions we've made

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

How do we handle it if the content signing key changes?

I think that right now, this can happen when the user logs out (or is logged out) and then logs in.

In the future, this can happen due to a primary device key rotation (which should be invisible to the user), or a restore (which happens while logged out).

28 ↗(On Diff #35489)

Nit: read-only

60–61 ↗(On Diff #35489)

Why do we need the ? here?

62–64 ↗(On Diff #35489)

Why do we need these runtime checks?

91 ↗(On Diff #35489)

Is this an unexpected scenario? Should we do something more than just returning null here? Eg. a console.log, throwing an exception, etc.

web/grpc/identity-service-client-wrapper.js
59 ↗(On Diff #35489)

Feels a little inconsistent that we use "Auth" vs "Unauthorized". There was a discussion in D10430 and we settled on "Unauth" for this... do you think you can rename?

web/grpc/identity-service-context-provider.react.js
26 ↗(On Diff #35489)

Similar question about error handling here

Regarding this comment I previously made:

If we go with the approach I proposed there, we can probably remove it from Redux, in favor of exposing it via this context

I think we'll want to keep commServicesAccessToken in Redux on web, but probably remove it on native. What do you think? If you agree, can you create a follow-up task to track this before landing? Could probably go under ENG-4567

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

It seems like the easiest solution might be to remove a performance optimization and always call getContentSigningKey when calling identity. We can also consider e.g. having the emitter provide updates of it, but not sure if it is worth it.

60–61 ↗(On Diff #35489)

authMetadataPromiseRef is typed as ?Promise which means that awaiting it could give us null. One way of avoiding it might be to implement https://linear.app/comm/issue/ENG-6401/introduce-a-hook-that-works-like-useref-and-allows-being-initialized which should hide this detail

62–64 ↗(On Diff #35489)
type CommServicesAuthMetadata = {
  +userID?: ?string,
  +deviceID?: ?string,
  +accessToken?: ?string,
}

CommServicesAuthMetadata is typed in a way that allows these to be null, but we need these to call authenticated endpoints. A different check would be required when calling an unauthenticated client.

91 ↗(On Diff #35489)

Yeah, it is. I think that throwing might be better here, as it means that a service broke a contract and returned an invalid response.

web/grpc/identity-service-client-wrapper.js
59 ↗(On Diff #35489)

Sure, I'll rename it

web/grpc/identity-service-context-provider.react.js
26 ↗(On Diff #35489)

Having authLayer = null will mean that only an unauthenticated client will be initialized. This makes sense in some scenarios. An error should be thrown only when we need to call a method that requires an authenticated client, but we don't have one.

It's unfortunate how different native and web are, but I think it makes sense given the technical decisions we've made

They can be made more similar by introducing a wrapper over commRustModule on native and by moving some state to redux. But I don't think it is worth it - the context is still able to hide these differences from the caller.

I think we'll want to keep commServicesAccessToken in Redux on web, but probably remove it on native. What do you think? If you agree, can you create a follow-up task to track this before landing? Could probably go under ENG-4567

Yeah, it makes sense. I've created https://linear.app/comm/issue/ENG-6423/remove-commservicesaccesstoken-from-redux-on-native to track it.

tomek marked 10 inline comments as done.

Address review

Rever import ordering change

ashoat added inline comments.
native/identity-service/identity-service-context-provider.react.js
53–54 ↗(On Diff #35515)

These two can be awaited together

Also accepting on behalf of @varun, who seems to have stayed up late last night and might not be available to accept this until later. His comment was addressed so I think this is good to land

This revision is now accepted and ready to land.Jan 11 2024, 5:11 AM
native/identity-service/identity-service-context-provider.react.js
53–54 ↗(On Diff #35515)

Initially, I tried to do this, but it doesn't work

Cannot call await with Promise.all(...) bound to p because null or undefined [1] is incompatible with null or
undefined [1] in array element of type argument R [2]. [incompatible-call]

     identity-service/identity-service-context-provider.react.js
 [1]   23│     React.useRef<?Promise<{ +userID: ?string, +accessToken: ?string }>>();
         :
       50│       +accessToken: string,
       51│     }>,
       52│   >(async () => {
       53│     const [deviceID, authMetadata] = await Promise.all([
       54│       getContentSigningKey(),
       55│       authMetadataPromiseRef.current,
       56│     ]);
       57│     const userID = authMetadata?.userID;
       58│     const accessToken = authMetadata?.accessToken;
       59│     if (!deviceID || !userID || !accessToken) {

     /private/tmp/flow/flowlib_ea0c1d4f3fb3e484_501/core.js
 [2] 1841│ declare class Promise<+R = mixed> {

This is caused by the fact that authMetadataPromiseRef contains an optional promise, which is required because we want to avoid unnecessary calls to getCommServicesAuthMetadata.

I've tried a couple of solutions, e.g. filtering the authMetadataPromiseRef.current out if it is null, creating promisses array and then passing it to Promise.all, storing an optional value in a promise, but all of the options I've tried caused more problems.

Overall, awaiting them in a sequence shouldn't affect the performance significantly because authMetadataPromiseRef.current, for most of the times, will immediately resolve (it happens some time after it is initially set, or when an emitter broadcasted a value). So awaiting getContentSigningKey would take almost the same time as awaiting all promises. Also, this code starts only getContentSigningKey, because authMetadataPromiseRef.current is already started.

native/identity-service/identity-service-context-provider.react.js
53–54 ↗(On Diff #35515)

Another solution to this might be to implement https://linear.app/comm/issue/ENG-6401/introduce-a-hook-that-works-like-useref-and-allows-being-initialized. I started writing the solution, but it also appeared to be a lot harder than I expected.

I think it's fine to land this as-is

varun added inline comments.
native/identity-service/identity-service-context-provider.react.js
67

we're missing an await here i think

native/identity-service/identity-service-context-provider.react.js
67

Good catch, yeah this should be fixed

native/identity-service/identity-service-context-provider.react.js
67

No, this code is correct. We're returning the promise which can be awaited by the caller.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

Async functions always return a promise. If the return value of an async function is not explicitly a promise, it will be implicitly wrapped in a promise.

So if we return a value from an async function it will be wrapped. But when we return a promise, nothing happens.