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

fix(mysql): improve password compatibility mysql user #534

Closed
wants to merge 1 commit into from

Conversation

gompa
Copy link
Contributor

@gompa gompa commented Nov 14, 2017

improve password compatibility with mysql password requirements
by adding the generate-password node module

improve password compatibility
 with mysql password requirements
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.685% when pulling 7754314 on gompa:master into 8ab4892 on TryGhost:master.

@acburdine
Copy link
Member

hmm...now that I'm looking at this regex it might actually have issues 😕 I'll test it out though and see if it works/doesn't.

@vikaspotluri123
Copy link
Member

@acburdine imho the regex can be simplified to something like ^SET PASSWORD FOR 'ghost-[0-9]{1,4}'@'localhost' = PASSWORD\('.{10,}'\);$ (or can be condensed even more) - the tests are verifying queries and retries rather than a strong password is generated.

@vikaspotluri123
Copy link
Member

Also sidenote: I think this is similar to (if not the same idea as) #517

@acburdine
Copy link
Member

yeah it's a similar idea - we had a convo on Slack about this approach as well.

However, I figured out that the underlying issue wasn't actually the password itself (although that is a factor). The first issue we have to solve is that the CREATE USER 'ghost-xx'@'localhost' IDENTIFIED WITH mysql_native_password also throws an error, before we even get to the secure password part.

That mysql_native_password thing was added to fix #396 - we need to ensure that whatever solution doesn't make that issue happen again.

@acburdine
Copy link
Member

@gompa thanks for the fix! I went ahead and cherry-picked this commit and applied it to #546 -> I had to make some other changes alongside this one.

@acburdine acburdine closed this Nov 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants