Skip to content

executor,planner: Improve SET PASSWORD#8426

Merged
zz-jason merged 5 commits intopingcap:masterfrom
morgo:set-password
Nov 26, 2018
Merged

executor,planner: Improve SET PASSWORD#8426
zz-jason merged 5 commits intopingcap:masterfrom
morgo:set-password

Conversation

@morgo
Copy link
Contributor

@morgo 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

MySQL compatible behavior
@morgo
Copy link
Contributor Author

morgo commented Nov 23, 2018

/run-all-tests

@morgo
Copy link
Contributor Author

morgo commented Nov 23, 2018

/run-unit-test

@morgo
Copy link
Contributor Author

morgo commented Nov 26, 2018

PTAL @tiancaiamao

@tiancaiamao
Copy link
Contributor

LGTM
Good job! @morgo

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 26, 2018
s.User = vars.User
if s.User == nil {
if e.ctx.GetSessionVars().User == nil {
return errors.New("Session error is empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 26, 2018
@zz-jason zz-jason merged commit 7a88c33 into pingcap:master Nov 26, 2018
@morgo morgo deleted the set-password branch November 26, 2018 14:38
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Nov 28, 2019
@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2. type/compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set password='xx' fails privilege check

6 participants