this diff introduces the olm auth responder
Details
- Reviewers
ashoat atul inka tomek - Commits
- rCOMM5274f74c1c2a: [keyserver] olm auth responder
successfully called the olm auth responder by calling the olm auth action from native.
- confirmed that we don't create a new account on keyserver if doNotRegister is true
- if olm session creation fails, the responder returns an error and the OLM_AUTH_FAILED action type is dispatched
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
keyserver/src/responders/user-responders.js | ||
---|---|---|
334–349 | i think i can just do this actually |
keyserver/src/responders/user-responders.js | ||
---|---|---|
719 | We need doNotRegister in siweAuthResponder because we have two places where we allow users to log in with an existing SIWE account but NOT register a new one:
We don't have that problem in other responders, where we separate login and registration. Regarding the new olm_auth responder, I'm not sure we need doNotRegister. It's no longer the responsibility of the keyserver to handle registration. | |
740 | I think we can drop this since we don't need doNotRegister | |
752 | We don't need to wait for the earlier await before this one. This can be moved up to that Promise.all | |
756–759 | What's the difference between identityInfo.userId and identifier.userID? |
keyserver/src/responders/user-responders.js | ||
---|---|---|
740 | Actually, we DO need this so we know to trigger processOLMAccountCreation. But we should be able to fetch the same data based on userID rather than username |
keyserver/src/responders/user-responders.js | ||
---|---|---|
330–333 | Why do we block on this now? Previously it looks like we were able to generate the cookie at the same time | |
334–349 | That makes sense. I would additionally wrap the whole thing in an async IIFE so you can run it in parallel with the policies check (assuming we want to keep that in parallel) | |
388 | Can we add a comment explaining why we do content vs notifications differently here? | |
772 | Can we add a comment explaining why we do content vs notifications differently here? |
keyserver/src/responders/user-responders.js | ||
---|---|---|
29 ↗ | (On Diff #35331) | not sure what happened here but i'll make sure to add the .js suffix back |
PS – created ENG-6369 for the iOS CI issue
keyserver/src/responders/user-responders.js | ||
---|---|---|
329 ↗ | (On Diff #35331) | Nit: I'd reverse condition and reduce indentation here... it's more lines than the other ones |
772–803 ↗ | (On Diff #35331) | Can we do steps 3 and 4 at the same time? |
788 ↗ | (On Diff #35331) | Do we really need this variable? Looks like it's always equal to !existingUsername |