sql: support the GRANT ON SCHEMA command
#53344
Conversation
|
This change is |
There was a problem hiding this comment.
Generally LGTM. It would be nice if @ajwerner or @lucy-zhang could have a look at the resolver.go changes.
Random thought: we have that wildcard GRANT syntax for tables (GRANT SELECT ON TABLE db.* TO user`). Do we want to support that for schemas too? It's kind of weird and non-standard so I think it's ok if the answer is no.
Reviewed 7 of 16 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @rohany, and @solongordon)
pkg/sql/resolver.go, line 308 at r1 (raw file):
} if len(descs) == 0 {
How can descs be empty here? We just iterated over a non-empty Databases slice and appended a descriptor for each. Doesn't hurt anything I guess but I was confused by it. I think this applies below as well.
pkg/sql/logictest/testdata/logic_test/schema, line 246 at r1 (raw file):
# Test privilege interactions with schemas. subtest privileges
Could you test REVOKE FROM SCHEMA as well?
No, I don't think so. There are some postgres extensions for this that we should adopt instead (like grant.. on all tables in schema`) |
rohany
force-pushed
the
grant-schema
branch
2 times, most recently
from
9bcd4f3
to
f94de78
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/resolver.go, line 308 at r1 (raw file):
Previously, solongordon (Solon) wrote…
How can
descsbe empty here? We just iterated over a non-emptyDatabasesslice and appended a descriptor for each. Doesn't hurt anything I guess but I was confused by it. I think this applies below as well.
Not sure, I was just cargo culting from the other cases. Removed
pkg/sql/logictest/testdata/logic_test/schema, line 246 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Could you test REVOKE FROM SCHEMA as well?
Done
There was a problem hiding this comment.
That sounds right to me, thanks.
Reviewed 1 of 8 files at r2.
Reviewable status:
There was a problem hiding this comment.
We should add docs to clarify what GRANT ... ON <db>.* means now.
@ericharmeling is this your domain?
Reviewed 1 of 16 files at r1, 4 of 8 files at r2, 1 of 6 files at r3.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/grant_database, line 104 at r3 (raw file):
SHOW GRANTS ON DATABASE a ---- a crdb_internal admin ALL
I'm not clear on what part of this PR changed this set of privileges
There was a problem hiding this comment.
err didn't mean to block, just want to see how this plays into the information schema better
Reviewable status:
|
Yeah, grant * really means grant on all tables. Those tests changed because I adjusted what privileges schemas are allowed to have — the entries removed correspond to privileges no longer allowed on schemas |
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Oops, I started with #53358 and looked at it all at once without noticing that this was the parent.
Reviewed 9 of 16 files at r1, 8 of 8 files at r2, 6 of 6 files at r3.
Reviewable status:
|
TFTRs! bors r+ |
|
Merge conflict. |
Fixes cockroachdb#50879. This commit adds support for the `GRANT ... ON SCHEMA ... TO ...` command and adds the necessary permissions checks that were missing. Note that the `USAGE` permission is not implemented and will be done as a follow up (cockroachdb#53342). Release note (sql change): Add support for the `GRANT ... ON SCHEMA command`.
|
bors r+ |
|
Build succeeded: |
Fixes #50879.
This commit adds support for the
GRANT ... ON SCHEMA ... TO ...command and adds the necessary permissions checks that were missing.
Note that the
USAGEpermission is not implemented and will be done asa follow up (#53342).
Release note (sql change): Add support for the
GRANT ... ON SCHEMA command.