[native] Store access token in Redux
ClosedPublic
Actions

Authored by tomek on Jan 29 2024, 5:43 AM.

Details

Reviewers

varun
inka
kamil
bartek

Commits

rCOMM145a81b1d298: [native] Store access token in Redux

Summary

Introduce a handler that:

Reads the token from core module
Listens for its changes
Updates the store with the most recent value

https://linear.app/comm/issue/ENG-6597/store-csat-in-redux

Depends on D10866

Test Plan

Call commCoreModule.setCommServicesAuthMetadata and check if the token in Redux is updated. Add a timeout with a call to commCoreModule.setCommServicesAuthMetadata and check if it results in the token being updated.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Jan 29 2024, 5:43 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJan 29 2024, 5:43 AM

tomek added a child revision: D10831: [lib] Run the keyserver auth only when it isn't running and a user isn't already authenticated.Jan 29 2024, 5:46 AM

Harbormaster completed remote builds in B26252: Diff 36263.Jan 29 2024, 6:02 AM

tomek requested review of this revision.Jan 29 2024, 6:02 AM

inka accepted this revision.Jan 29 2024, 7:35 AM

This revision is now accepted and ready to land.Jan 29 2024, 7:35 AM

bartek accepted this revision.Jan 29 2024, 7:47 AM

Closed by commit rCOMM145a81b1d298: [native] Store access token in Redux (authored by tomek). · Explain WhyJan 29 2024, 9:00 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMM145a81b1d298: [native] Store access token in Redux.

ashoat added inline comments.Jan 29 2024, 12:02 PM

native/identity-service/identity-service-context-provider.react.js
18 ↗	(On Diff #36288)	Nit: not sure why this was separated into its own `import`. Probably not worth fixing at this point, but might be worth checking if your IDE is doing this accidentally
41 ↗	(On Diff #36288)	I think it's a little weird that we're still using the auth metadata emitter for the `userID`, but we're accessing the CSAT from Redux. Is there a reason for this inconsistency?

tomek added inline comments.Jan 30 2024, 1:57 AM

native/identity-service/identity-service-context-provider.react.js
18 ↗	(On Diff #36288)	I think we should fix this in whole codebase - created a task to configure ESLint rule https://linear.app/comm/issue/ENG-6607/add-no-duplicate-imports-rule-to-eslint-config
41 ↗	(On Diff #36288)	At this point, we have two different `userID`s on a client: From a keyserver DB From identity service - UUID or a reserved ID Ultimately, these should match, but currently, they're different. In the future, the emitter should update Redux and we could just select the ID here. Now we don't have a place in Redux to store this ID and I don't think it is worth it to introduce it.

ashoat added inline comments.Jan 30 2024, 9:36 AM

native/identity-service/identity-service-context-provider.react.js
41 ↗	(On Diff #36288)	I would've assumed that it would be safe to use the `userID` from `currentUserInfo`, but I'm guessing that doesn't work for you because some reducer hasn't been updated yet, or maybe because your test plan isn't testing that part. Can you create a follow-up task to update this logic to use the `userID` from `currentUserInfo`? It could potentially be a child of ENG-4567

inka added inline comments.Feb 1 2024, 3:36 AM

native/identity-service/identity-service-context-provider.react.js
41 ↗	(On Diff #36288)	This will be partially addressed by ENG-6416 which will make `currentUserInfo` be set on identity login and register. Then we will have to update this code here to use it.