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

executor,planner: Improve SET PASSWORD #8426

Merged
merged 5 commits into from Nov 26, 2018

Conversation

Projects
None yet
5 participants
@morgo
Member

morgo commented Nov 23, 2018

What problem does this PR solve?

Fixes #7522

What is changed and how it works?

  • Set password was too strict in privilege checking, and required SUPER for all use cases. Now it only requires SUPER for changing other user's passwords.
  • Set password did not use the AuthUsername + AuthPassword session properties. So if a user account was a wildcard, it was not possible to change the password.
  • Error messages/codes are now mysql compatible.

This PR will conflict with #8417 as they both add the same error to the executor. I will see which one is merged first and then fix the code :-)

There remains an issue common to all GRANT/SET password/CREATE user commands in that they do not invalidate the privilege cache (as MySQL versions do). See #8427

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Code changes

  • minimal

Side effects

  • Breaking backward compatibility (broken apps)

Related changes

  • Need to be included in the release note

This change is Reviewable

executor,planner: Improve SET PASSWORD
MySQL compatible behavior
@morgo

This comment has been minimized.

Member

morgo commented Nov 23, 2018

/run-all-tests

@morgo

This comment has been minimized.

Member

morgo commented Nov 23, 2018

/run-unit-test

@morgo

This comment has been minimized.

Member

morgo commented Nov 26, 2018

@tiancaiamao

This comment has been minimized.

Contributor

tiancaiamao commented Nov 26, 2018

LGTM
Good job! @morgo

vars := e.ctx.GetSessionVars()
s.User = vars.User
if s.User == nil {
if e.ctx.GetSessionVars().User == nil {
return errors.New("Session error is empty")

This comment has been minimized.

@jackysp

jackysp Nov 26, 2018

Member

Why the error is "Session error is empty"? Although it is not related to this PR.

This comment has been minimized.

@morgo

morgo Nov 26, 2018

Member

When the test-suite runs, it doesn't always have a session. This error is pre-existing, I just moved it around.

morgo added some commits Nov 26, 2018

@zz-jason

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Nov 26, 2018

@zz-jason zz-jason merged commit 7a88c33 into pingcap:master Nov 26, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@morgo morgo deleted the morgo:set-password branch Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment