Page MenuHomePhabricator

[bugfix] Fix "unkown error" on login.
ClosedPublic

Authored by przemek on Dec 22 2022, 2:02 AM.
Tags
None
Referenced Files
F3248040: D5987.id20515.diff
Fri, Nov 15, 8:18 AM
Unknown Object (File)
Thu, Nov 7, 5:54 AM
Unknown Object (File)
Thu, Nov 7, 2:26 AM
Unknown Object (File)
Wed, Nov 6, 6:30 AM
Unknown Object (File)
Mon, Nov 4, 11:10 PM
Unknown Object (File)
Sat, Nov 2, 10:44 PM
Unknown Object (File)
Sat, Nov 2, 10:44 PM
Unknown Object (File)
Sat, Nov 2, 10:44 PM

Details

Summary

Fixing this: https://linear.app/comm/issue/ENG-2479/unknown-error-when-password-is-wrong#comment-b1eb427d

We had that problem both on native and web. I introduced new error message in new version,
so it's not confused with anything old anymore. Also updated codeVersion od 167, as I figured we want it ASAP.
I have not set it to 166, as that version was already released.

Code it written in a such way, that code that would run when client has lower version than 167 is identical
to what we had before I introduced changes here: https://phab.comm.dev/rCOMMce7bc1377e206a8fbd8470dfef269556d243728f.

So it is:

if (hasMinCodeVersion(viewer.platformDetails, 167)) {
  // ... new responses
} else {
  // ... old responses
}
Test Plan

Tested with codeVersion set to 166 and worked on current native and web.
I had a lot of problems with testing on old clients and I'm not really sure what I've done wrong.
I followed instructions but couldn't make realese build to build and wasted a lot of time on it.
Code is simple enough that it should work on older versions, but it should still be tested. Maybe I'll get some help on that after Christmast in the office.

Diff Detail

Repository
rCOMM Comm
Branch
fix/code-version
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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

we doing the same in both in if and else. Why not just userRow = userResult[0]; ?

230

I am not sure if it is safe, there is no 100% confidence that this will be released in 167. I feel like a specific version should be set after this code will be released but deferring this to others' opinion.

238

I will also consider checking for userResult.length === 0 or !userRow.hash || !bcrypt.compareSync(request.password, userRow.hash and only if they're truthy throw an error depending on user version to reduce the number of conditions to check and make this code more readable

Why aren't we just returning invalid_credentials for the clients with the new code? I'm a bit confused why we need a new invalid_login_credentials

tomek requested changes to this revision.Dec 23 2022, 5:13 AM
tomek added inline comments.
keyserver/src/responders/user-responders.js
234

Why do we need to introduce a new error code? Could you explain this in detail? It seems like setting the code version should be enough to make it working and invalid_credentials would be handled by the newer clients just fine.

This revision now requires changes to proceed.Dec 23 2022, 5:13 AM

This diff needs to be prioritized

You're all right. I'm not sure what happened here.
It works now. Tested on web and native. I runned into huge problems with testing on older versions and wasted a lot of time on trying to do it. I'm not sure what approach should we take here?
I have analyzed the code and in really old versions it shoudl still work, but I feel like it's insufficient, but I don't want to waste more time figuring out how to run old version of client.

tomek requested changes to this revision.Jan 4 2023, 2:11 AM
tomek added inline comments.
keyserver/src/responders/user-responders.js
300 ↗(On Diff #20515)

Are we sure we want to target so high code version? Could you check in which code version https://phab.comm.dev/rCOMMce7bc1377e206a8fbd8470dfef269556d243728f was included and target that version?

This revision now requires changes to proceed.Jan 4 2023, 2:11 AM
This comment was removed by przemek.
keyserver/src/responders/user-responders.js
300 ↗(On Diff #20515)

codeVersion 150 was introduced 24th October, and linked changes come fro m18th October, so I'm setting codeVersion to 150

przemek marked an inline comment as done.

I also needed to add extra checks in 2 places, because on web when password is empty it was checked on a server side in one of validation-utils.js functions. This caused another unknown error, but is fixed now.

tomek requested changes to this revision.Jan 5 2023, 3:40 AM
tomek added inline comments.
keyserver/src/responders/user-responders.js
300 ↗(On Diff #20515)

A safer approach than relying on dates is to check out a tag. It seems like indeed in version 149 we were returning invalid_parameters when username was missing and when db result was empty, and invalid_credentials was returned when passwords weren't matching. Clients with version code 150 support invalid_credentials error by having incorrect username or password message. So the conditions seem correct.

keyserver/src/utils/validation-utils.js
17–33 ↗(On Diff #20595)

Have you checked where these functions were used? It seems like your change would break almost all the endpoints in the entire app. It is critical to verify that your changes doesn't break anything and to find a proper solution instead of the first one that seems to work.

We probably should keep this function as it was.

because on web when password is empty it was checked on a server side in one of validation-utils.js functions. This caused another unknown error, but is fixed now.

We shouldn't allow, on the client, to send a request with empty password. If we don't block that, we can handle it later as a separate Linear issue.

This revision now requires changes to proceed.Jan 5 2023, 3:40 AM

Fix - make viewer param optional.

tomek requested changes to this revision.Jan 5 2023, 4:58 AM
tomek added inline comments.
keyserver/src/utils/validation-utils.js
17–33 ↗(On Diff #20595)

This approach isn't good either. Providing viewer shouldn't affect the behavior so significantly - for most of the endpoints invalid_credentials error doesn't make any sense.

Also checkInputValidator still has viewer as the first parameter while the function declares it is the 3rd one.

Can't we just revert this change as I suggested in the previous comment?

This revision now requires changes to proceed.Jan 5 2023, 4:58 AM

Sorry, I missed your comments after build fail and tried to fix it myself. That's why I haven't addressed them.

przemek marked 2 inline comments as done.

Reverted changes in utility function. That was unnecessary.
We still get unkonw error when password is empty on web.

keyserver/src/utils/validation-utils.js
17–33 ↗(On Diff #20595)

You're absolutely right. Sorry that I missed those comments before submitting other changes. Handling empty password on frontend feels like a right thing to do, I'll create task for that in a moment.

tomek added a reviewer: ashoat.

Looks good to me! Added @ashoat as blocking to make sure that my idea about using this code version is ok.

keyserver/src/utils/validation-utils.js
17–33 ↗(On Diff #20595)

Thanks for the explanation!

This revision is now accepted and ready to land.Jan 5 2023, 4:10 PM