Added privilege checks for every currently supported statement #797
Conversation
|
No explicit tests because I didn't have the time (the existing tests do some testing but it's not exhaustive at all), and also I can't verify that every statement checks for the correct privileges as I didn't have time. Most statements should have the correct privileges though. |
There was a problem hiding this comment.
At first I was a bit turned off by the addition of a new interface method to Node, but I have to admit it makes a lot of sense. Nice work.
Only comment here is that some of these implementations seem like they should really be checking permissions on child nodes, but aren't because the analyzer does for them. 1) Make sure there are permissions tests of the correct behavior, 2) probably do the check anyway.
| // WithChildren implements the Node interface. | ||
| func (p *CreateForeignKey) WithChildren(children ...sql.Node) (sql.Node, error) { | ||
| return NillaryWithChildren(p, children...) | ||
| } | ||
|
|
||
| // CheckPrivileges implements the interface sql.Node. | ||
| func (p *CreateForeignKey) CheckPrivileges(ctx *sql.Context, opChecker sql.PrivilegedOperationChecker) bool { | ||
| return opChecker.UserHasPrivileges(ctx, |
There was a problem hiding this comment.
MySQL is not consistent on what privileges are needed at times. Take CREATE USER, which can handle creating a user, renaming them, and dropping them. And, for some reason, specifically the REVOKE ALL PRIVILEGES statement, but no other REVOKE statement.
| @@ -74,6 +74,12 @@ func (sc ShowCreateTable) WithChildren(children ...sql.Node) (sql.Node, error) { | |||
| return &sc, nil | |||
| } | |||
|
|
|||
| // CheckPrivileges implements the interface sql.Node. | |||
| func (sc *ShowCreateTable) CheckPrivileges(ctx *sql.Context, opChecker sql.PrivilegedOperationChecker) bool { | |||
| // The table won't be visible during the resolution step if the user doesn't have the correct privileges | |||
There was a problem hiding this comment.
Make sure there are tests of this behavior
| @@ -187,6 +187,12 @@ func (s ShowColumns) WithChildren(children ...sql.Node) (sql.Node, error) { | |||
| return &s, nil | |||
| } | |||
|
|
|||
| // CheckPrivileges implements the interface sql.Node. | |||
| func (s *ShowColumns) CheckPrivileges(ctx *sql.Context, opChecker sql.PrivilegedOperationChecker) bool { | |||
| // The table won't be visible during the resolution step if the user doesn't have the correct privileges | |||
There was a problem hiding this comment.
Again, tests
It also seems strange that we wouldn't just call Child.CheckPrivileges, we have the table in the node
There was a problem hiding this comment.
So failing CheckPrivileges results in an error being thrown. In some of these statements, we don't want to error, we'll just show nothing, so we let those other checks handle those parts. I can't quite verify that for this particular statement (most of these SHOW statements were done last), but that's the general idea for most of these SHOW statements that just return true.
| // CheckPrivileges implements the interface sql.Node. | ||
| func (p *Truncate) CheckPrivileges(ctx *sql.Context, opChecker sql.PrivilegedOperationChecker) bool { | ||
| return opChecker.UserHasPrivileges(ctx, | ||
| sql.NewPrivilegedOperation(p.db, getTableName(p.Child), "", sql.PrivilegeType_Drop)) |
There was a problem hiding this comment.
Seems like it should be, but perhaps it's closer to a DROP TABLE internally so they rely on DROP instead of DELETE.
Hydrocharged
force-pushed
the
daylon/users-10
branch
from
b71477f
to
7913f3e
Compare
This adds privileges for every supported statement.