Page MenuHomePhabricator
Differential D11395

[lib][native] Only reduce CSAT on setAccessTokenActionType
ClosedPublic
Actions

Authored by inka on Mar 27 2024, 4:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 16, 3:21 AM
Unknown Object (File)
Fri, Nov 1, 7:36 PM
Unknown Object (File)
Thu, Oct 31, 4:46 PM
Unknown Object (File)
Thu, Oct 31, 4:46 PM
Unknown Object (File)
Thu, Oct 31, 4:46 PM
Unknown Object (File)
Thu, Oct 31, 4:46 PM
Unknown Object (File)
Thu, Oct 31, 4:45 PM
Unknown Object (File)
Oct 16 2024, 9:49 AM
View All 70 Files
Subscribers
ashoat
varun

Details

Reviewers
michal
marcin
tomek
ashoat
Commits
rCOMM3a63c9460ff0: [lib][native] Only reduce CSAT on setAccessTokenActionType
Summary

issue: ENG-7428

during both identity registration and log in for both password and wallet users new comm services auth metadata are persisted in CommSecureStore and emitted to JS: https://github.com/CommE2E/comm/blob/master/native/identity-service/identity-service-context-provider.react.js#L329
and https://github.com/CommE2E/comm/blob/master/native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp#L1532-L1537

So we just need to stop removing it with resetUserSpecificState, and stop setting it with all actions but setAccessTokenActionType

We want to keep doing this on web, where we don't have an emitter

Test Plan
  1. Tested that on web CSAT is cleared on logout, and is present when user is logged in
  2. Tested that on logout CSAT is set to an empty string on native
  3. Tested that on login/register CSAT is set to a non empty string on native
  4. Tested that if setNewSessionActionType with cookieInvalidated is dispatched, then the user is logged out and the CSAT is cleared

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inka created this revision.Mar 27 2024, 4:58 AM
Herald added a subscriber: ashoat. · View Herald TranscriptMar 27 2024, 4:58 AM
inka added a parent revision: D11394: [lib][web][native] Handle idnentity login in reducers.Mar 27 2024, 4:58 AM
Harbormaster completed remote builds in B27700: Diff 38357.Mar 27 2024, 5:13 AM
inka requested review of this revision.Mar 27 2024, 5:13 AM
ashoat requested changes to this revision.Mar 27 2024, 5:25 AM
ashoat added inline comments.
lib/reducers/services-access-token-reducer.js
18–26 ↗(On Diff #38357)

How about these lines? Shouldn't they be removed as well for native? It seems like they have a similar risk of causing the data in Redux to become inconsistent with the data on the native side

29–33 ↗(On Diff #38357)

Is removing this the right decision for web? The CSAT is only stored in Redux there, right? Should we migrate this to some web-only reducer?

This revision now requires changes to proceed.Mar 27 2024, 5:25 AM
inka added inline comments.Mar 29 2024, 4:29 AM
lib/reducers/services-access-token-reducer.js
18–26 ↗(On Diff #38357)

Hm that's true. But this makes me wonder what about the situation when the auth keyservers session is invalidated. We were removing the CSAT to log the user out of identity as well. We probably want to keep this behaviour.

Maybe we could set a connectionIssue when cookie is invalidated, and let the keyserver connection handler dispatch a logout for the auth keyserver...

18–26 ↗(On Diff #38357)

After some more thinking I think we should do something else.

First of all I think that removing the CSAT doesn't log the user out currently. This is why I am facing this issue.

Second of all, since cookieInvalidated from auth keyserver causes the user to see the login screen, I think we should be dispatching a logout, to inform the identity as well.

Third of all, whenever cookieInvalidated is sent, cookieInvalidationRecovery is called. It dispatches setActiveSessionRecoveryActionType which causes a recovery login to occur. If it fails, and the keyserver is the auth keyserver, logout is dispatched. This doesn't happen only if !canResolveInvalidation, but it looks like it is always true when usingCommServicesAccessToken is true.

So I think we just need to remove those lines, because the logout action will be dispatched, and the token will be removed anyway

18–26 ↗(On Diff #38357)

As for logOutActionTypes.started, the diff that added reduceServicesAccessToken D7269 states that it was modeled on reduceDataLoaded. But I don't think it matters to set the CSAT to null immediately. So I think we can just remove this line too, for both web and native.

29–33 ↗(On Diff #38357)

As I mentioned in D11374 - On web we handle register and login in a special reducer D11238

ashoat added inline comments.Mar 29 2024, 11:53 AM
lib/reducers/services-access-token-reducer.js
18–26 ↗(On Diff #38357)

I like this proposal!

29–33 ↗(On Diff #38357)

Oh, interesting. Should this reducer be moved to native then, to make it more clear that it's only used there? Or am I missing something?

inka added a subscriber: varun.Apr 3 2024, 5:49 AM
inka added inline comments.
lib/reducers/services-access-token-reducer.js
18–26 ↗(On Diff #38357)

Here @varun mentioned that we don't remove the CSAT on logout yet. So I set this issue as blocked by ENG-6730, and will continue working on this diff when it is done

29–33 ↗(On Diff #38357)

If we remove all actions but setAccessTokenActionType from this reducer, than it looks like it can be a native specific reducer. Unless we plan to use this action on web in the future, but I don't think so

ashoat added inline comments.Apr 4 2024, 2:35 PM
lib/reducers/services-access-token-reducer.js
29–33 ↗(On Diff #38357)

What do you think of that idea? To me, it makes sense to remove all actions but setAccessTokenActionType from this reducer... it should make the coupling between native and JS more reliable here. (We would still want to look into actions that might modify the CSAT field that aren't the reducer, eg. resetUserSpecificState.)

ashoat added a comment.Apr 15 2024, 8:27 AM

Discussed this in a meeting with @inka and @tomek. We identified the following next steps, now that @tomek's work has been landed:

  1. We'd like to move reduceServicesAccessToken, setAccessTokenActionType, and related things to native, since on web we have a separate reducer that doesn't use setAccessTokenActionType
  2. We'd like to update this native-specific CSAT reducer to only look at setAccessTokenActionType
  3. We'd like to make sure that resetUserSpecificState on native does not affect the CSAT field in Redux, since that could cause the CSAT field in Redux to become inconsistent with the one stored on the native side
  4. We should add commServicesAccessToken to persistBlacklist on native
  5. We'll confirm that the CommServicesAuthMetadataEmitter clears the CSAT when a user logs out or deletes their account
inka updated this revision to Diff 39395.Apr 23 2024, 4:33 AM

Remove all actions but setAccessTokenActionType.
In the next diff I will move this reducer to native

inka retitled this revision from [lib][native] Stop deleting CSAT on login actions to [lib][native] Only reduce CSAT on setAccessTokenActionType.Apr 23 2024, 4:34 AM
inka edited the test plan for this revision. (Show Details)
inka edited the summary of this revision. (Show Details)
inka added a child revision: D11738: [lib][native] Move reduceServicesAccessToken and setAccessTokenActionType to native.Apr 23 2024, 4:39 AM
Harbormaster completed remote builds in B28400: Diff 39395.Apr 23 2024, 4:49 AM
ashoat accepted this revision.Apr 23 2024, 9:32 AM

Looks like steps 2 and 3 are completed here

This revision is now accepted and ready to land.Apr 23 2024, 9:32 AM
Closed by commit rCOMM3a63c9460ff0: [lib][native] Only reduce CSAT on setAccessTokenActionType (authored by inka). · Explain WhyApr 25 2024, 8:42 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMM3a63c9460ff0: [lib][native] Only reduce CSAT on setAccessTokenActionType.

Revision Contents
Changeset List

PathSize
lib/
reducers/
24 lines
native/
redux/
1 line

Diff 39492

View Options

lib/reducers/services-access-token-reducer.js

Loading...
View Options

native/redux/state-types.js

Loading...