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

executor: grant table level privilege should check table exist #14540

Merged
merged 4 commits into from Jan 30, 2020

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Jan 19, 2020

What problem does this PR solve?

MySQL:

mysql> grant Select,Insert,Update on test.nonexist to genius;
ERROR 1146 (42S02): Table 'test.nonexist' doesn't exist

TiDB:

mysql> grant Select,Insert,Update on test.nonexist to genius;
Query OK, 0 rows affected (0.05 sec)

Allow grant on non-exist tables causes some more serious bugs than I first thought.

mysql> grant Select,Insert,Update on test.nonexist to genius;
Query OK, 0 rows affected (0.02 sec)

mysql> grant Create on test.Nonexist to genius;
Query OK, 0 rows affected (0.02 sec)

mysql> show grants for genius;
+---------------------------------------------------------------------+
| Grants for genius@%                                                 |
+---------------------------------------------------------------------+
| GRANT Select,Insert,Update ON test.nonexist TO 'genius'@'%'         |
| GRANT Create ON test.Nonexist TO 'genius'@'%'         |
+---------------------------------------------------------------------+
4 rows in set (0.00 sec)

mysql> select * from mysql.tables_priv;                                                                                                                                                               +------+------+--------+------------+----------------+---------------------+------------------------------------+----------------------+
| Host | DB   | User   | Table_name | Grantor        | Timestamp           | Table_priv                         | Column_priv          |
+------+------+--------+------------+----------------+---------------------+------------------------------------+----------------------+
| %    | test | genius | nonexist   | root@127.0.0.1 | 2020-01-19 21:28:35 | Select,Insert,Update
| %    | test | genius | Nonexist   | root@127.0.0.1 | 2020-01-19 21:30:59 | Create
+------+------+--------+------------+----------------+---------------------+------------------------------------+----------------------+
3 rows in set (0.00 sec)

There are two records in mysql.tables_priv for genius@% and those two records are loaded, they overwrite each other.

The user genius@% doesn't really have Select,Insert,Update,Create privileges.

What is changed and how it works?

Check the table exist before the grant operation.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.

Fix a bug that TiDB can grant privileges on non-exist table.
This check also fix the misspelled case sensitive table name case.
@tiancaiamao tiancaiamao requested a review from a team as a code owner January 19, 2020 14:33
@ghost ghost requested review from eurekaka and francis0407 and removed request for a team January 19, 2020 14:33
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/980

@tiancaiamao
Copy link
Contributor Author

PTAL @imtbkcat @jackysp

@imtbkcat imtbkcat self-requested a review January 20, 2020 08:03
@@ -192,6 +193,8 @@ func (s *testSuite3) TestColumnScope(c *C) {
func (s *testSuite3) TestIssue2456(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("CREATE USER 'dduser'@'%' IDENTIFIED by '123456';")
tk.MustExec("CREATE DATABASE `dddb_%`;")
tk.MustExec("CREATE table `dddb_%`.`te%` (id int);")

Choose a reason for hiding this comment

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

this mean we will never support wildcard in table name ?

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added this to the v4.0.0-beta.1 milestone Jan 20, 2020
@shenli
Copy link
Member

shenli commented Jan 29, 2020

LGTM

@shenli
Copy link
Member

shenli commented Jan 29, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 29, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 29, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jan 29, 2020

@tiancaiamao merge failed.

@tiancaiamao
Copy link
Contributor Author

This one should be merged together with https://github.com/pingcap/tidb-test/pull/980

@jackysp
Copy link
Member

jackysp commented Jan 30, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jan 30, 2020

/run-all-tests

@sre-bot sre-bot merged commit e4eb58c into pingcap:master Jan 30, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jan 30, 2020

cherry pick to release-3.0 failed

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/privilege status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants