Page MenuHomePhabricator
Differential D6960

[keyserver] try registerUser if loginUser fails in keyserver logInResponder
ClosedPublic
Actions

Authored by varun on Mar 6 2023, 10:38 PM.
Tags
None
Referenced Files
F3514495: D6960.id23473.diff
Sun, Dec 22, 5:16 AM
F3513398: D6960.diff
Sun, Dec 22, 12:19 AM
Unknown Object (File)
Sun, Dec 15, 2:05 PM
Unknown Object (File)
Wed, Dec 4, 7:46 AM
Unknown Object (File)
Wed, Dec 4, 7:46 AM
Unknown Object (File)
Wed, Dec 4, 7:46 AM
Unknown Object (File)
Wed, Dec 4, 7:27 AM
Unknown Object (File)
Oct 28 2024, 10:17 PM
View All Files
Subscribers
atul

Details

Reviewers
ashoat
• jon
bartek
tomek
Commits
rCOMM030104821306: [keyserver] try registerUser if loginUser fails in keyserver logInResponder
Summary

Changes are as follows:

  1. Modified Identity service to send back "not found" status to caller (it was previously eating this up and closing the connection)
  2. Modified loginUser function in rust-node-addon to propagate the not found error to js if encountered
  3. Wrapped the loginUser call in logInResponder in a try-catch so that if we get back the "not found" error, we attempt a registerUser call instead

Normally I'd make this 3 diffs, but this is urgent

Test Plan

tried logging in with a user in my local MariaDB instance but not in localstack DDB, saw that a new entry was made in DDB upon successful login

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun created this revision.Mar 6 2023, 10:38 PM
Herald added a subscriber: atul. · View Herald TranscriptMar 6 2023, 10:38 PM
Harbormaster completed remote builds in B17133: Diff 23473.Mar 6 2023, 10:54 PM
varun requested review of this revision.Mar 6 2023, 10:54 PM
bartek accepted this revision.Mar 6 2023, 11:24 PM

Rust looks OK.

keyserver/src/responders/user-responders.js
441

Generally, I'm not a fan of recognizing errors by comparing message strings, if anybody ever changes that string, this stops working. Ideally we'd want error messages to have unified format, e.g. starting with some error code, like ERR_USER_NOT_FOUND: user not found. Sometimes regex match is sufficient.

But as this diff seems urgent, I'm not blocking on this comment - this is just something worth considering in the future.

ashoat accepted this revision.Mar 7 2023, 5:52 AM
ashoat added inline comments.
keyserver/src/responders/user-responders.js
441

I agree with @bartek, but also don't want to block this diff. @varun, could you create a follow-up task and link it here before landing?

This revision is now accepted and ready to land.Mar 7 2023, 5:52 AM
varun added a comment.Mar 7 2023, 8:10 AM

https://linear.app/comm/issue/ENG-3253/improve-handling-of-rust-node-addon-errors

Closed by commit rCOMM030104821306: [keyserver] try registerUser if loginUser fails in keyserver logInResponder (authored by varun). · Explain WhyMar 7 2023, 8:10 AM
This revision was automatically updated to reflect the committed changes.
varun added a commit: rCOMM030104821306: [keyserver] try registerUser if loginUser fails in keyserver logInResponder.

Revision Contents
Changeset List

PathSize
keyserver/
addons/
rust-node-addon/
src/
identity_client/
7 lines
2 lines
src/
responders/
24 lines
services/
identity/
src/
15 lines
service/
11 lines
17 lines

Diff 23473

View Options

keyserver/addons/rust-node-addon/src/identity_client/login_user.rs

Loading...
View Options

keyserver/addons/rust-node-addon/src/identity_client/mod.rs

Loading...
View Options

keyserver/src/responders/user-responders.js

Loading...
View Options

services/identity/src/service.rs

Loading...
View Options

services/identity/src/service/login.rs

Loading...
View Options

services/identity/src/service/update.rs

Loading...