Page MenuHomePhabricator
Differential D5368

[web][native][keyserver] Changing login screen so it displays identical error on wrong username and/or password.
ClosedPublic
Actions

Authored by • przemek on Oct 14 2022, 5:53 AM.
Tags
None
Referenced Files
F3522025: D5368.diff
Mon, Dec 23, 5:37 AM
Unknown Object (File)
Sat, Nov 30, 5:51 AM
Unknown Object (File)
Mon, Nov 25, 10:37 PM
Unknown Object (File)
Sun, Nov 24, 12:41 PM
Unknown Object (File)
Sun, Nov 24, 12:41 PM
Unknown Object (File)
Sun, Nov 24, 12:41 PM
Unknown Object (File)
Sun, Nov 24, 12:41 PM
Unknown Object (File)
Sun, Nov 24, 12:41 PM
View All Files
Subscribers
• abosh
ashoat
atul
kamil
tomek

Details

Reviewers
tomek
ashoat
kamil
atul
Commits
rCOMMce7bc1377e20: [web][native][keyserver] Changing login screen so it displays identical error…
Summary

Solving issue: https://linear.app/comm/issue/ENG-2047/[web]-[native][keyserver][security]-on-login-wrong-user-and-wrong
Keyserver returns only "invalid_parameters" instead of either "invalid_parameters" or "invalid_credentials". So it is impossible to say which of the fields was wrong based on the error.
As Tomek mentioned it is still possible to perform timing attack but it will be taken care of in another issue on Linear. On web and native we are reacting to "invalid_parameters" displaying message about incorrect credentials, not specifying which one was incorrect.

Screenshot 2022-10-14 at 14.55.35.png (340×606 px, 116 KB)

Screenshot 2022-10-14 at 14.55.12.png (538×492 px, 33 KB)

Test Plan

Built app.
Tested all scenarios and verified that correct thing is displayed.

Diff Detail

Repository
rCOMM Comm
Branch
fix/login-errors
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• przemek created this revision.Oct 14 2022, 5:53 AM
Herald added a subscriber: • abosh. · View Herald TranscriptOct 14 2022, 5:53 AM
• przemek edited the summary of this revision. (Show Details)Oct 14 2022, 5:57 AM
• przemek added 1 blocking reviewer(s): tomek.
• przemek attached a referenced file: F198994: Screenshot 2022-10-14 at 14.55.12.png. (Show Details)
• przemek attached a referenced file: F198995: Screenshot 2022-10-14 at 14.55.35.png. (Show Details)
Harbormaster completed remote builds in B12794: Diff 17570.Oct 14 2022, 6:04 AM
• przemek requested review of this revision.Oct 14 2022, 6:04 AM
ashoat requested changes to this revision.Oct 14 2022, 6:30 AM
ashoat added inline comments.
keyserver/src/responders/user-responders.js
230 ↗(On Diff #17570)

See https://linear.app/comm/issue/ENG-2047#comment-160f2fcf

web/account/log-in-form.react.js
73 ↗(On Diff #17570)

Don't need a template literal here, not sure why it was a template literal before

This revision now requires changes to proceed.Oct 14 2022, 6:30 AM
• przemek updated this revision to Diff 17573.Oct 14 2022, 6:38 AM

Updating D5368: [web][native][keyserver] Changing login screen so it displays identical error on wrong username and/or password.

Used hasMinCodeVersion as Ashoat recommended. Kamil at SWM told me to use 99999 as version number for now, so person creating new release can swap it for its number.

• przemek updated this revision to Diff 17574.Oct 14 2022, 6:44 AM

Update ticks and uppercases.

ashoat requested changes to this revision.Oct 14 2022, 6:46 AM
ashoat added inline comments.
native/account/log-in-panel.react.js
286–305 ↗(On Diff #17573)

Shouldn't these be deleted? Or is something still using them?

web/account/log-in-form.react.js
73 ↗(On Diff #17573)

See comment in previous review: template literal not needed

This revision now requires changes to proceed.Oct 14 2022, 6:46 AM
Harbormaster completed remote builds in B12797: Diff 17573.Oct 14 2022, 6:49 AM
Harbormaster completed remote builds in B12798: Diff 17574.Oct 14 2022, 6:54 AM
• przemek added a comment.Oct 14 2022, 7:00 AM

They are still used. onUnsuccessfulLoginAlertAckowledged is new function used when either username or password. onUsernameAlertAcknowledged is still used when verifying correctness of username. I'm planning on changing them as we don't want to clear form fields as mentioned here: https://linear.app/comm/issue/ENG-2050/[web][native]-add-see-password-button-on-the-login-page-and-dont-clear

ashoat accepted this revision.Oct 14 2022, 7:01 AM

Thanks!

This revision now requires review to proceed.Oct 14 2022, 7:01 AM
tomek requested changes to this revision.Oct 14 2022, 8:49 AM
tomek added inline comments.
keyserver/src/responders/user-responders.js
232

In https://linear.app/comm/issue/ENG-2047/on-login-wrong-user-and-wrong-password-should-be-one-error#comment-160f2fcf @ashoat suggested to use

a new string that new clients can handle to display an "wrong username or password" error

But I think it is fine to return the same string and handle both of them with the same alert. Previously, when invalid_parameters was received, we were specific - now we're less specific, so it is still correct to show the message in the old context. @ashoat what do you think?

native/account/log-in-panel.react.js
258

Not sure, but maybe we should keep showing the description e.g. The credentials you entered are incorrect?

290–301

Aren't these swapped? I guess that if username is incorrect, we should clear only the username, but I might be wrong

web/account/log-in-form.react.js
70–80

Most of the code is the same for both branches. Maybe we can clear inputs before if and focus after it?

This revision now requires changes to proceed.Oct 14 2022, 8:49 AM
• przemek updated this revision to Diff 17586.Oct 14 2022, 9:24 AM
• przemek marked an inline comment as done.

Fixed smaller issues Tomek mentioned.
The one about Ashoat's suggestion is lef for discussion.
Now I realized it would be better to refactor it, so we use invalid_credentials now, as
it would make sense it context of whole application and how invalid_credentials and invalid_parameters are used.
Waiting for your suggestions.

• przemek added inline comments.Oct 14 2022, 9:25 AM
native/account/log-in-panel.react.js
258

Not sure, really. Check out a screenshot in summary. I think there is enough of text there and adding description would make it more clumsy.

290–301

You're right, fixed

web/account/log-in-form.react.js
70–80

Fixed

Harbormaster completed remote builds in B12808: Diff 17586.Oct 14 2022, 9:35 AM
ashoat added a comment.Oct 14 2022, 10:29 AM

I had the same thought when I accepted – using the same string is different than what I initially suggested, but I think it actually makes sense here

ashoat added inline comments.Oct 14 2022, 10:30 AM
native/account/log-in-panel.react.js
258 ↗(On Diff #17586)

Let's say "Either that user doesn't exist, or the password is incorrect"

tomek accepted this revision.Oct 17 2022, 2:58 AM

Let's keep the server error string but please include the informative text to the alert.

native/account/log-in-panel.react.js
258 ↗(On Diff #17586)

According to https://developer.apple.com/design/human-interface-guidelines/components/presentation/alerts/

Include informative text only if it adds value. If you need to add an informative message, keep it as short as possible, using complete sentences, sentence-style capitalization, and appropriate punctuation.

In this case I think that adding info that a user could not exist is an additional value, so either we should include it in the description or update the title so that the description isn't needed. For now we should use the text @ashoat suggested and maybe revisit later (but I don't think we should spend more time on it now).

Also, we probably should end this message with a period, just like the guidelines suggest.

This revision is now accepted and ready to land.Oct 17 2022, 2:58 AM
ashoat added inline comments.Oct 17 2022, 5:34 AM
native/account/log-in-panel.react.js
258 ↗(On Diff #17586)

I wouldn't worry too much about what Apple thinks here, personally. Regarding the period, let's prioritize consistency across the app over Apple's opinions. I think we generally don't include a period at the end, but I might be wrong

• przemek updated this revision to Diff 17632.EditedOct 18 2022, 12:07 AM

Added description text and rebased.

Harbormaster completed remote builds in B12843: Diff 17632.Oct 18 2022, 12:18 AM
Closed by commit rCOMMce7bc1377e20: [web][native][keyserver] Changing login screen so it displays identical error… (authored by • przemek). · Explain WhyOct 18 2022, 12:24 AM
This revision was automatically updated to reflect the committed changes.
• przemek added a commit: rCOMMce7bc1377e20: [web][native][keyserver] Changing login screen so it displays identical error….
ashoat added inline comments.Dec 16 2022, 4:42 PM
keyserver/src/responders/user-responders.js
231 ↗(On Diff #17634)

We forgot to update this after we released a new version

Revision Contents
Changeset List

PathSize
keyserver/
src/
responders/
7 lines
native/
account/
44 lines
web/
account/
9 lines

Diff 17574

View Options

keyserver/src/responders/user-responders.js

Loading...
View Options

native/account/log-in-panel.react.js

Loading...
View Options

web/account/log-in-form.react.js

Loading...