if a native password user is missing a CSAT (and usingCommServicesAccessToken is true), we should try to log in the identity and get a new CSAT
Details
Details
using this patch, i cleared the access token on native and confirmed that i was logged in to identity again and received a new access token. i confirmed that if identity is unavailable, i get logged out and the alert displays explaining why i was logged out.
also confirmed that my avatar doesn't get removed, since we perform keyserver auth as part of the usePasswordLogIn hook
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- delete-user-client (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Not sure about not dispatching a Redux action here. See inline comment
| native/components/background-identity-login-handler.react.js | ||
|---|---|---|
| 38–53 ↗ | (On Diff #40814) | Same comment as in D11912: you should exit early and reduce indentation By keeping multiple branches open when you could quickly close one instead, you're forcing the reader to have to spend more "mental RAM" trying to understand your code |
| 41 ↗ | (On Diff #40814) | This is an unexpected case, right? Can we add a console.log here? |
| 44–47 ↗ | (On Diff #40814) | You don't seem to be dispatching the action, and my first thought was that you should be doing this here. But looking at the reducers, there are only three places where this action is reduced:
I think it's kind of weird not to dispatch an action here. It makes eg. our error reports worse, and a bit hard for developers to track what's going on while testing. But if we were to dispatch the action here linked in the code I linked at the start of this comment, then we would also need to fix up reduceCurrentUserInfo to do a merge, similar to how it currently handles setNewSessionActionType here. What do you think? |
| native/components/background-identity-login-handler.react.js | ||
|---|---|---|
| 44–47 ↗ | (On Diff #40814) | By the way, the reason it works without a dispatch here is that the SET_ACCESS_TOKEN action is dispatched separately via the CommServicesAuthMetadataEmitter |
| native/components/background-identity-login-handler.react.js | ||
|---|---|---|
| 44–47 ↗ | (On Diff #40814) | I'm actually using the function you linked in the first line of your comment. It handles dispatching the action |
| native/components/background-identity-login-handler.react.js | ||
|---|---|---|
| 48 | realized we should probably log out and alert here, too | |
Comment Actions
Talked to @varun offline about the Redux dispatch. I definitely misread this on first glance, and thought it was using useIdentityPasswordLogIn instead of usePasswordLogIn.
I asked him why we were doing keyserver auth as well, instead of just identity auth. He explained that we need the keyserver auth to make sure that the content Olm session is set up.
Note that my concerns about reduceCurrentUserInfo don't apply when using useIdentityPasswordLogIn. While it's true that the identity auth will clear the avatar, the subsequent authoritative keyserver auth should repopulate it. If the authoritative keyserver auth fails for some reason, the user will be logged out.
| native/components/background-identity-login-handler.react.js | ||
|---|---|---|
| 48 | Good call | |