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

[WIP]expression: support ValidatePasswordStrength() #9812

Closed
wants to merge 12 commits into from

Conversation

wjhuang2016
Copy link
Member

What problem does this PR solve?

Support ValidatePasswordStrength() except checking by a dictionary file, See #9741 or #2632

What is changed and how it works?

Accroding to https://dev.mysql.com/doc/refman/8.0/en/validate-password-options-variables.html
and https://dev.mysql.com/doc/refman/8.0/en/encryption-functions.html#function_validate-password-strength, we need to check the password strength by userauth_userlengthmixed_case_countnumber_countspecial_char_count, even a dictionary file.
Presently I check the above conditions except dictionary file, and I'm not sure whether we can read a file directly in a distributed environment.

In MySQL, a user can decide whether to use validate_password component. If we apply the valide_password before setting a password, we may need a option for user to decide whether to check
password by valide_password.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

Side effects

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@shenli
Copy link
Member

shenli commented Mar 19, 2019

@wjhuang2016 Thanks for your PR!

@shenli shenli added the contribution This PR is from a community contributor. label Mar 19, 2019
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #9812 into master will decrease coverage by 0.1651%.
The diff coverage is 66.3551%.

@@               Coverage Diff                @@
##             master      #9812        +/-   ##
================================================
- Coverage   81.5133%   81.3482%   -0.1652%     
================================================
  Files           434        424        -10     
  Lines         93765      90919      -2846     
================================================
- Hits          76431      73961      -2470     
+ Misses        11881      11634       -247     
+ Partials       5453       5324       -129

@morgo
Copy link
Contributor

morgo commented Mar 19, 2019

Thank you for working on this! It is the last missing function on this list, so I am excited to see it get merged.

@morgo
Copy link
Contributor

morgo commented Mar 21, 2019

@wjhuang2016 please resolve conflicts. Thank you!

@qw4990
Copy link
Contributor

qw4990 commented Jul 26, 2019

Friendly ping @wjhuang2016 and please resolve conflicts 🤣.

@wjhuang2016
Copy link
Member Author

Friendly ping @wjhuang2016 and please resolve conflicts .

Already solved

expression/builtin_encryption.go Outdated Show resolved Hide resolved
expression/builtin_encryption.go Show resolved Hide resolved
sessionctx/variable/sysvar.go Outdated Show resolved Hide resolved
expression/builtin_encryption.go Show resolved Hide resolved
expression/builtin_encryption.go Show resolved Hide resolved
sessionctx/variable/sysvar.go Show resolved Hide resolved
@SunRunAway
Copy link
Contributor

Hi, @wjhuang2016
PTAL

@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@zz-jason zz-jason requested a review from SunRunAway August 1, 2019 04:33
@SunRunAway SunRunAway removed their request for review August 1, 2019 09:09
@SunRunAway
Copy link
Contributor

Hi, @wjhuang2016
Have one last comment to address.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 2, 2019
@qw4990 qw4990 removed their request for review August 5, 2019 07:52
expression/builtin_encryption.go Outdated Show resolved Hide resolved
expression/builtin_encryption.go Outdated Show resolved Hide resolved
expression/builtin_encryption.go Outdated Show resolved Hide resolved
expression/builtin_encryption.go Outdated Show resolved Hide resolved
@wjhuang2016 wjhuang2016 changed the title expression: support ValidatePasswordStrength() [WIP]expression: support ValidatePasswordStrength() Aug 14, 2019
@tiancaiamao
Copy link
Contributor

PTAL @zz-jason

@wjhuang2016
Copy link
Member Author

This PR is WIP, there are some corner cases to deal with.

@shonge
Copy link
Member

shonge commented Feb 23, 2022

Hi @wjhuang2016 , do you have any plan to continue developing this feature?

@wjhuang2016 wjhuang2016 deleted the validate_password branch November 17, 2022 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants