Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added new PasswordStrengthScore method #2599

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Conversation

noeliaSD
Copy link
Contributor

@noeliaSD noeliaSD commented Mar 22, 2022

What does the PR do

By integrating zxcvbn module, it has been added a new method to get the specific password strength score information.

Closes #5096 together with related nim-go and desktop PRs.

NOTE: Added specific get score method althought there is already PasswordStrength one, that provides the same but extended information, due to an issue parsing JSON response in the NIM site since the zxcvbnreturns big int crack time numbers when the password lenght is big and nim json cannot parse the response correctly. See the following post.

By integrating `zxcvbn` module, it has been added a new method to get the specific password strength score information.
@status-github-bot
Copy link

status-github-bot bot commented Mar 22, 2022

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
  • Have you tested changes with mobile?
  • Have you tested changes with desktop?

@status-im-auto
Copy link
Member

status-im-auto commented Mar 22, 2022

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ a12521e #1 2022-03-22 08:38:15 ~2 min linux 📦zip
✔️ a12521e #1 2022-03-22 08:39:50 ~3 min android 📦aar
✔️ a12521e #1 2022-03-22 08:41:13 ~5 min ios 📦zip

@noeliaSD noeliaSD self-assigned this Mar 23, 2022
if err != nil {
return makeJSONResponse(fmt.Errorf("Error marshalling to json: %v", err))
}
return string(data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it not be makeJSONResponse?

same question for function GetPasswordStrengthScore

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm

Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

due to an issue parsing JSON response in the NIM site since the zxcvbnreturns big int crack time numbers when the password lenght is big and nim json cannot parse the response correctly

I'm not sure if I fully understand this. You mean that in Nim we have issues parsing the response because the Int returned is too big?
If that's the case, we can use the stint library, which is used to parse big numbers.

If we can, having only one function for Strength and Score would be great

@noeliaSD
Copy link
Contributor Author

Looks good.

due to an issue parsing JSON response in the NIM site since the zxcvbnreturns big int crack time numbers when the password lenght is big and nim json cannot parse the response correctly

I'm not sure if I fully understand this. You mean that in Nim we have issues parsing the response because the Int returned is too big? If that's the case, we can use the stint library, which is used to parse big numbers.

If we can, having only one function for Strength and Score would be great

Looks good.

due to an issue parsing JSON response in the NIM site since the zxcvbnreturns big int crack time numbers when the password lenght is big and nim json cannot parse the response correctly

I'm not sure if I fully understand this. You mean that in Nim we have issues parsing the response because the Int returned is too big? If that's the case, we can use the stint library, which is used to parse big numbers.

If we can, having only one function for Strength and Score would be great

Let me try to explain it better.. And I've realised the post link was wrong so I can understand why you cannot understand my comment!!

I firstly created the generic GetPasswordStrength method from status-go that incorporates the complete information the library we are using provides. It provides entropy, crack time, score, match sequence... Then I created all the chain in nim-status-go and in status-desktop passing a json with all that information and in some point in status-desktop it was got the score that now it is just the only info that is of our interest.
Then, testing some passwords with a considerable length, where for example, its crack time starts to be huge, the json parser in the nim site raised exceptions regarding the integer values received so looking at the internet I saw the above post and seems the nim json library has some limitations so I decided to create that new method that just only retreives the score and then we can manage it well in nim but keeping both methods still we have the door open in status-go to get a better and more complete information.

I hope it is more clear now and it has sense :)

@jrainville
Copy link
Member

@noeliaSD Aaaah yes it's much clearer now.

I already approved, so here is a thumbs up instead 👍

@noeliaSD noeliaSD merged commit becfa09 into develop Mar 23, 2022
@noeliaSD noeliaSD deleted the feat/issue-5096 branch March 23, 2022 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Integrate password validation (zxcvbn lib) in new password screens
4 participants