privileges: fix create temporary tables privilege #29279
Conversation
ti-chi-bot
added
release-note-none
|
I encountered an unstable test #29283 and try to run it again. |
|
/run-check_dev_2 |
planner/core/optimizer.go
Outdated
| newVisitInfo.table = "" | ||
| privVisitInfo = append(privVisitInfo, newVisitInfo) | ||
| } else { | ||
| privVisitInfo = append(privVisitInfo, v) |
There was a problem hiding this comment.
Do we need needCheckTmpTablePriv here?
There was a problem hiding this comment.
I don't think so. Any examples that there might be a bug here?
There was a problem hiding this comment.
For example: create table t1 from select * from tmp . Tough we do not support this syntax currently, adding some checks here can avoid hidden problems in the future.
| @@ -120,6 +121,68 @@ func CheckPrivilege(activeRoles []*auth.RoleIdentity, pm privilege.Manager, vs [ | |||
| return nil | |||
| } | |||
|
|
|||
| // VisitInfo4PrivCheck generates privilege check infos because privilege check of local temporary tables is different | |||
| // with normal tables. `CREATE` statement needs `CREATE TEMPORARY TABLE` privilege from the database, and subsequent | |||
| // statements do not need any privileges. | |||
There was a problem hiding this comment.
CREATEstatement needsCREATE TEMPORARY TABLEprivilege from the database, and subsequent
// statements do not need any privileges.
Why not check it in the DDL operation?
If you add it as an additional check, it slow down all operations a bit which is unnecessary.
There was a problem hiding this comment.
I know, but there are too many operations to check, not only DDL but also DML and others.
If I add all these checks in the planbuilder, it will be too dirty.
There was a problem hiding this comment.
Besides, in planbuilder, I cannot add visitInfo but do not require privilege checks in a graceful way. visitInfo and privilege checks are bound together.
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: c251b6b
|
ti-chi-bot
added
the
status/can-merge
|
/run-check_dev_2 |
ti-chi-bot
removed
the
status/can-merge
|
Another bug #29302 is just introduced after I submitted the PR, so I removed that test case until the bug is fixed. |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 96a818a
|
ti-chi-bot
added
the
status/can-merge
|
@djshow832: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/run-check_dev_2 |
What problem does this PR solve?
Issue Number: close #29271
Problem Summary:
In MySQL, creating a temporary table needs
create temporary tablesprivilege on the database, and subsequent statements on the table require no privileges.Not fixed issues:
#29280
#29281
#29282
What is changed and how it works?
After collecting the
visitInfo, filter the temporary tables.This doesn't need a release note because it's not documented yet.
Check List
Tests
Side effects
Documentation
Release note