this diff introduces the olm auth responder
Details
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 334–349 ↗ | (On Diff #35102) | i think i can just do this actually |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 719 ↗ | (On Diff #35102) | 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 ↗ | (On Diff #35102) | I think we can drop this since we don't need doNotRegister |
| 752 ↗ | (On Diff #35102) | We don't need to wait for the earlier await before this one. This can be moved up to that Promise.all |
| 756–759 ↗ | (On Diff #35102) | What's the difference between identityInfo.userId and identifier.userID? |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 740 ↗ | (On Diff #35102) | 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 ↗ | (On Diff #35102) | Why do we block on this now? Previously it looks like we were able to generate the cookie at the same time |
| 334–349 ↗ | (On Diff #35102) | 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 ↗ | (On Diff #35102) | Can we add a comment explaining why we do content vs notifications differently here? |
| 772 ↗ | (On Diff #35102) | Can we add a comment explaining why we do content vs notifications differently here? |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 330–333 ↗ | (On Diff #35102) | we shouldn't. wrapped the set cookie logic in an async iife so that we don't have to block |
| 756–759 ↗ | (On Diff #35102) | identityInfo.userId is the keyserver's user ID (needed to validate the CSAT). identifier.userID is the userID of the auth responder caller. |
| 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 |
Comment Actions
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 |