user/edit/password: check haveibeenpwned.com - #1744
Conversation
|
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
It's looking nice! I've added several comments, but a lot of them are just hints for working with the Frontend codebase.
| (async () => { | ||
| const isPwned = await checkPasswordPwnage(this.request, this.newPassword); |
There was a problem hiding this comment.
I'm noticing the use of this.newPassword here and below, but I'm realizing that the <form-group> isn't disabled during the request. So users could theoretically change this.newPassword between the pwned check and the request to Backend. We don't usually disable form fields during requests, but I think that'd be a reasonable choice here in order to prevent this unlikely but awkward possibility. Alternatively, you could save this.newPassword to a local variable at the start of validate(), then use that local variable instead of the potentially changing this.newPassword.
There was a problem hiding this comment.
We don't usually disable form fields during requests
Why not?
There was a problem hiding this comment.
IIRC issa had a preference along those lines. The idea was to not lock down parts of the UI if we don't need to. If the user wants to edit those fields during the request for some reason (perhaps anticipating an error response), we don't restrict them from doing so.
In terms of code complexity, it's probably a wash. It's a little extra to disable fields during requests, but usually it's just one or two attributes or props. But this is the downside of not disabling them, that we have to consider the possibility of the data property changing during the request.
I don't have a strong preference either way. Maybe it's something we could ask Nicole or the larger team on Slack.
2 participants
Closes getodk/central#2150
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
fetch()call?How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
I don't think so.