this diff introduces a new olmAuth action which is responsible for calling the olm auth responder (next diff).
Details
Details
- Reviewers
ashoat inka tomek - Commits
- rCOMM46995ffde81d: [lib] add olm auth action
tested later in stack by dispatching the action and successfully calling the olm auth responder
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/types/account-types.js | ||
|---|---|---|
| 175–196 ↗ | (On Diff #35099) | i think it's better to introduce brand new types since we can enforce stricter checks for some of the fields here |
| lib/actions/user-actions.js | ||
|---|---|---|
| 301 ↗ | (On Diff #35099) | i took a lot of the multikeyserver logic here from the logIn higher order function. we'll eventually remove that function completely so not deduping |
| lib/actions/user-actions.js | ||
|---|---|---|
| 301 ↗ | (On Diff #35099) | |
| 309 ↗ | (On Diff #35099) | For login, we have source: logInActionSource here. The purpose of that is to let us distinguish the callsites triggering the login. I think we'll need the same for olm_auth, since an Olm auth can be triggered from a session invalidation, a connectionIssue, etc. Curious for @inka's perspective on whether we should reuse LogInActionSource, or introduce a new one. |
| lib/types/account-types.js | ||
| 166 ↗ | (On Diff #35099) | |
| 167 ↗ | (On Diff #35099) | Why do we need walletAddress? In the existing code, userID is set to the walletAddress for ETH users. Is this an attempt to future-proof, for a future use case where users can have both a username and a walletAddress? |
| 171–172 ↗ | (On Diff #35099) | Why are the properties here not read-only? |
| 175–196 ↗ | (On Diff #35099) | While reviewing this, I thought about trying to factor out the shared parts of some of the existing login types, but it's probably more complicated than it's worth |
| 185–195 ↗ | (On Diff #35099) | Rather that specify the encrypted messages individually, I think you should do this The extra $ReadOnly is necessary because "spreading" types drops read-only in Flow for some reason |
| lib/actions/user-actions.js | ||
|---|---|---|
| 279 ↗ | (On Diff #35099) | I am already creating this type in D10424. I also wrote a comment there about the name, can you answer either here or there? |
| 301 ↗ | (On Diff #35099) | I don't think we should be deduping this code. The code in old login action shouldn't be changing anymore, whereas this code will be changed according to needs. |
| 309 ↗ | (On Diff #35099) | I think it's fine to use LogInActionSource, if it still makes sense to distinguish between From and SIWE logins |
| 376 ↗ | (On Diff #35099) | In this action we shouldn't be returning currentUserInfo. It should come from the identity service. |
| lib/actions/user-actions.js | ||
|---|---|---|
| 376 ↗ | (On Diff #35099) | |
| lib/actions/user-actions.js | ||
|---|---|---|
| 376 ↗ | (On Diff #35099) | But we should make all fields that use responses[ashoatKeyserverID] as optional, since the action may not call Ashoat's keyserver at all |
| lib/actions/user-actions.js | ||
|---|---|---|
| 301 ↗ | (On Diff #35099) | not going to dedup for now |
| 309 ↗ | (On Diff #35099) | i could be wrong, but it doesn't appear that we actually use source anywhere on the keyserver... i think @atul omitted source from SIWEAuthRequest for this reason. |
| 376 ↗ | (On Diff #35099) | introducing KeyserverAuthResult type where currentUserInfo is optional. will use this type instead of LogInResult. |
| lib/types/account-types.js | ||
| 167 ↗ | (On Diff #35099) | yeah that's right, i was trying to future-proof this type. we can't trust the user to provide their walletAddress for the same reason that we can't trust them to provide their username, though, so we'll have to get this data directly from the identity service. |
| 171–172 ↗ | (On Diff #35099) | they should be, thanks |
Comment Actions
Please address inline comments before landing
| lib/actions/user-actions.js | ||
|---|---|---|
| 294 | I actually would like us to pass the logInActionSource up to the keyserver. I know it's not used, but when I initially introduced it, my thinking was it could potentially aid in live debugging, or allow us to treat certain kinds of logins differently in the future | |
| 297 | I think it would be good to pull out deviceTokenUpdateRequest here too, so that it's not part of restLogInInfo restLogInInfo is only used in one place below, where deviceTokenUpdateRequest ends up getting overriden... so technically, there's no difference. But I think it improves readability to remove it from restLogInInfo... that way, the reader doesn't have to consider what happens to it later when restLogInInfo is used | |
| lib/types/account-types.js | ||
|---|---|---|
| 166 | Wouldn't it be better if we used ?: so we could stop sending it completely in the future (and not the null dummy value)? | |
Comment Actions
Can you explain why we don't need signedIdentityKeysBlob anymore?
| lib/types/account-types.js | ||
|---|---|---|
| 99–100 | You left the olm names here, we probably want to update them too? | |
| lib/types/account-types.js | ||
|---|---|---|
| 166 | Yes, I guess we could have both +currentUserInfo?: ?LoggedInUserInfo, | |
Comment Actions
the keyserver will pull the keys from the identity service now
| lib/types/account-types.js | ||
|---|---|---|
| 99–100 | good point. will change these to keyserverAuthFromNative: KEYSERVER_AUTH_FROM_NATIVE and keyserverAuthFromWeb: KEYSERVER_AUTH_FROM_WEB | |