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.
Details
Built app.
Tested all scenarios and verified that correct thing is displayed.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- fix/login-erros-msg
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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.
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
keyserver/src/responders/user-responders.js | ||
---|---|---|
232 ↗ | (On Diff #17574) | 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
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 ↗ | (On Diff #17574) | Not sure, but maybe we should keep showing the description e.g. The credentials you entered are incorrect? |
290–301 ↗ | (On Diff #17574) | 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 ↗ | (On Diff #17574) | Most of the code is the same for both branches. Maybe we can clear inputs before if and focus after it? |
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.
native/account/log-in-panel.react.js | ||
---|---|---|
258 ↗ | (On Diff #17574) | 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 ↗ | (On Diff #17574) | You're right, fixed |
web/account/log-in-form.react.js | ||
70–80 ↗ | (On Diff #17574) | Fixed |
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
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" |
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/
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. |
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 |
keyserver/src/responders/user-responders.js | ||
---|---|---|
231 ↗ | (On Diff #17634) | We forgot to update this after we released a new version |