-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: add privilege check for show bindings #14443
Conversation
executor/show.go
Outdated
schema = v.defaultDB | ||
} | ||
if !v.is.TableExists(model.NewCIStr(schema), x.Name) { | ||
v.ok = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about setting v.ok
to true
if table does not exist in the information schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v.ok=v.is.TableExists(model.NewCIStr(schema), x.Name)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eurekaka Why? If the table does not exist anymore, why should we show it?
@crazycs520 No, ok
may change from fasle to true then, which should not happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the table is re-created, then the binding would be shown then? would it confuse users since they don't touch bindings actually but the results change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REST LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
cherry pick to release-3.0 failed |
What problem does this PR solve?
Add privilege check for show bindings.
What is changed and how it works?
A binding will only be appended to result if the user has all tables' select priviledge in the sql.
Check List
Tests
Code changes
Side effects
Related changes
Release note