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

sql: support the USAGE privilege on schemas #53358

Merged
merged 2 commits into from Aug 25, 2020
Merged

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Aug 24, 2020

Fixes #53342.

This commit adds the USAGE privilege to schemas. The USAGE privilege
allows a user to resolve objects under a schema.

Release note (sql change): Support the USAGE privelege on schemas.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany
Copy link
Contributor Author

rohany commented Aug 24, 2020

First commit is #53344

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, @rohany, and @solongordon)


pkg/sql/logictest/testdata/logic_test/schema, line 293 at r2 (raw file):

statement ok
CREATE TABLE privs.usage_tbl (x INT);
CREATE TYPE privs.usage_typ AS ENUM ('usage');

Would it make this test stronger if you also granted SELECT on the table and USAGE on the type?

@rohany
Copy link
Contributor Author

rohany commented Aug 24, 2020

Would it make this test stronger if you also granted SELECT on the table and USAGE on the type?

I don't think so, as Postgres(and we now) check the schema permissions first before the object permissions. I'm asserting that I get a schema permission error as well, so we know that we're getting the right error

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang, @rohany, and @solongordon)

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

speaking of @solongordon's question, do we properly deal with the USAGE privilege on enums?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang, @rohany, and @solongordon)

Copy link
Contributor

@thoszhang thoszhang left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 16 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany)


pkg/sql/grant_revoke.go, line 231 at r2 (raw file):

			}
		case *schemadesc.Mutable:
			if err := p.writeSchemaDescChange(

Don't have to change anything right now, but I think eventually we should be doing all these writes in the same batch.

@rohany
Copy link
Contributor Author

rohany commented Aug 25, 2020

speaking of @solongordon's question, do we properly deal with the USAGE privilege on enums?

No, the usage privilege on enums is a bit different. USAGE on enums doesn't disallow just using the type in a SELECT 'x'::mytype. It only disallows you from using the type in a table or something that would cause a back reference.

@ajwerner
Copy link
Contributor

speaking of @solongordon's question, do we properly deal with the USAGE privilege on enums?

No, the usage privilege on enums is a bit different. USAGE on enums doesn't disallow just using the type in a SELECT 'x'::mytype. It only disallows you from using the type in a table or something that would cause a back reference.

Just so we're clear, what you mean is if you have USAGE then you can create tables, right? Not having it disallows using it in tables. Nothing prevents selecting it IIUC so long as you can resolve it. That was my reading of

For types and domains, allows use of the type or domain in the creation of tables, functions, and other schema objects. (Note that this privilege does not control all “usage” of the type, such as values of the type appearing in queries. It only prevents objects from being created that depend on the type. The main purpose of this privilege is controlling which users can create dependencies on a type, which could prevent the owner from changing the type later.)

https://www.postgresql.org/docs/12/ddl-priv.html

@rohany
Copy link
Contributor Author

rohany commented Aug 25, 2020

Yes, thats what I meant. I was trying to say that having usage or not doesn't affect this particular test case.

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`.
@rohany
Copy link
Contributor Author

rohany commented Aug 25, 2020

bors r+

@otan
Copy link
Contributor

otan commented Aug 25, 2020

bors r-

looks like this is broken on CI

@craig
Copy link
Contributor

craig bot commented Aug 25, 2020

Canceled.

@rohany
Copy link
Contributor Author

rohany commented Aug 25, 2020

hmmmm

@otan
Copy link
Contributor

otan commented Aug 25, 2020

hmm

:D

Fixes cockroachdb#53342.

This commit adds the `USAGE` privilege to schemas. The `USAGE` privilege
allows a user to resolve objects under a schema.

Release note (sql change): Support the `USAGE` privelege on schemas.
@rohany
Copy link
Contributor Author

rohany commented Aug 25, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 25, 2020

Build succeeded:

@craig craig bot merged commit 62214d5 into cockroachdb:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: support the USAGE permission for schemas
6 participants