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

*: kill one's own connection doesn't require SUPER privilege #6954

Merged
merged 4 commits into from Jul 5, 2018

Conversation

@tiancaiamao
Copy link
Contributor

commented Jul 2, 2018

What have you changed? (mandatory)

kill tidb connID, if the user is the owner of that connection, there
is no need to check the SUPER privilege.
SessionManager interface is slightly modified.

What are the type of the changes (mandatory)?

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested (mandatory)?

It's somehow difficult to test this change. I check it manually.

mysql> select current_user();
+----------------+
| current_user() |
+----------------+
| kill@127.0.0.1 |
+----------------+
1 row in set (0.00 sec)

mysql> show processlist;
+------+------+-----------+------+---------+------+-------+------------------+------+
| Id   | User | Host      | db   | Command | Time | State | Info             | Mem  |
+------+------+-----------+------+---------+------+-------+------------------+------+
|    3 | kill | 127.0.0.1 |      | Query   |    2 | 2     | select sleep(40) |    0 |
|    4 | kill | 127.0.0.1 |      | Query   |    0 | 2     | show processlist |    0 |
|    2 | root | 127.0.0.1 |      | Query   |    0 | 2     |                  |    0 |
+------+------+-----------+------+---------+------+-------+------------------+------+
3 rows in set (0.00 sec)

mysql> kill tidb 2;                 // Kill other's connection without SUPER privilege would fail.
ERROR 1105 (HY000): privilege check fail
mysql> kill tidb 3;                 // Kill the user's own connection.
Query OK, 0 rows affected (0.00 sec)

PTAL @jackysp

`kill tidb connID`, if the user is the owner of that connection, there
is no need to check the SUPER privilege.
SessionManager interface is slightly modified.
// If you have the SUPER privilege, you can kill all threads and statements.
// Otherwise, you can kill only your own threads and statements.
sm := b.ctx.GetSessionManager()
if sm != nil {

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

Could the sm be nil?

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jul 4, 2018

Author Contributor

In the test code

@@ -33,6 +33,6 @@ type ProcessInfo struct {
// SessionManager is an interface for session manage. Show processlist and
// kill statement rely on this interface.
type SessionManager interface {
ShowProcessList() []ProcessInfo
ShowProcessList() map[uint64]ProcessInfo

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

Add comment for the return value.

// Otherwise, you can kill only your own threads and statements.
sm := b.ctx.GetSessionManager()
if sm != nil {
processList := sm.ShowProcessList()

This comment has been minimized.

Copy link
@shenli

shenli Jul 2, 2018

Member

Could you add an interface for SessionManger like GetProcessByConnID(connID) ?

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jul 4, 2018

Author Contributor

GetProcessByConnID make SessionManager more complex.
GetProcessByConnID can be implemented using ShowProcessList which is already in SessionManager
I assume Kill will not be too frequency so this choice would not hurt the performance.
@shenli

This comment has been minimized.

Copy link
@shenli

shenli Jul 4, 2018

Member

So the interface ShowProcessList returns a map instead of a list.

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jul 4, 2018

Author Contributor

Yes.

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

@tiancaiamao Please fix the CI.

tiancaiamao added 2 commits Jul 4, 2018
@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

@jackysp

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

/run-all-tests

Copy link
Member

left a comment

LGTM

Copy link
Member

left a comment

LGTM

@zimulala zimulala added the status/LGT2 label Jul 4, 2018
@tiancaiamao

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

/run-all-tests

@coocood

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

/run-all-tests

@coocood coocood merged commit 7682a74 into pingcap:master Jul 5, 2018
11 checks passed
11 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
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Jul 6, 2018
…#6954)

`kill tidb connID`, if the user is the owner of that connection, there
is no need to check the SUPER privilege.
SessionManager interface is slightly modified.
@tiancaiamao tiancaiamao deleted the tiancaiamao:kill-self branch Jul 6, 2018
zhexuany added a commit to zhexuany/tidb that referenced this pull request Jul 13, 2018
…#6954)

`kill tidb connID`, if the user is the owner of that connection, there
is no need to check the SUPER privilege.
SessionManager interface is slightly modified.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.