sql: implement pg_has_role #69939

nvanbenschoten · 2021-09-08T20:50:23Z

Needed for #69010.
Related to #22734.

This commit implements the pg_has_role builtin function. pg_has_role
returns whether the user has privileges for a specified role or not.
Allowable privilege types are MEMBER and USAGE. MEMBER denotes direct or
indirect membership in the role (that is, the right to do SET ROLE),
while USAGE denotes whether the privileges of the role are immediately
available without doing SET ROLE.

pg_has_role was the last remaining unimplemented "access privilege
inquiry functions", and was omitted from 94c25be because our role-based
access control system was not mature enough to support it at the time.

The commit also makes a small modification to pg_catalog.pg_roles and
pg_catalog.pg_authid to reflect that fact that all users and roles
inherit the privileges of roles they are members of.

Release note (sql change): The pg_has_role builtin function is now
supported, which returns whether a given user has privileges for a
specified role or not.

Release justification: None, waiting for v22.1.

cockroach-teamcity · 2021-09-08T20:50:33Z

This change is

nvanbenschoten · 2021-09-13T02:25:07Z

I've added one more commit here resolving an incompatibility in CockroachDB's handling of the privilege string accepted by each of these Access Privilege Inquiry Functions. The intention was for the functions to return whether any of the comma-separated privileges were held. However, since their introduction, they were returning whether all of the privileges were held.

The intended behavior is mentioned in https://www.postgresql.org/docs/current/functions-info.html.

Also, multiple privilege types can be listed separated by commas, in which case the result will be true if any of the listed privileges is held.

I'm also happy to pull this into a follow-on PR if that's easier for reviewers.

rafiss

thanks for these improvements!

could you also add the null-handling change in the first commit to a release note

overall this looks good, but i think this would benefit from @RichardJCai reviewing the last 2 or 3 commits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @nvanbenschoten, and @RichardJCai)

pkg/sql/sem/builtins/pg_builtins.go, line 1812 at r4 (raw file):

						"role %s does not exist", roleArg)
				case *tree.DOid:
					// Postgres returns NULL if no matching tablespace is found when given

nit: should be "no matching role"

pkg/sql/sem/builtins/pg_builtins.go, line 2199 at r4 (raw file):

// isSuperuser returns whether the specified user has superuser privileges.
// This is defined to match the pg_roles.rolsuper field.

heads up: we are fixing the rolsuper field so that it determines superuser status based on admin membership: #69943

pkg/sql/sem/builtins/pg_builtins.go, line 503 at r5 (raw file):

// on an object to others if it is the owner of the object or if it itself
// holds both (1) that privilege on the object and (2) the GRANT privilege
// on the object.

nit: might want to link the tracking issue #67410

RichardJCai

Mostly looks good to me as well modulo the part on with grant opt.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @nvanbenschoten)

pkg/sql/resolver.go, line 327 at r5 (raw file):

		return true, nil
	}
	// For WITH GRANT OPTION, check the roles also has the GRANT privilege.

I'm not sure if we should remove this or not. On one hand we technically don't support with grant opt, but having GRANT privilege is needed to actually grant a privilege. For correctness it actually does make sense to check GRANT privilege to substitute for WITH GRANT OPTION right now.

nvanbenschoten

could you also add the null-handling change in the first commit to a release note

Done.

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @rafiss, and @RichardJCai)

pkg/sql/resolver.go, line 327 at r5 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I'm not sure if we should remove this or not. On one hand we technically don't support with grant opt, but having GRANT privilege is needed to actually grant a privilege. For correctness it actually does make sense to check GRANT privilege to substitute for WITH GRANT OPTION right now.

This isn't quite being removed. Rather, the change is removing the withGrantOption from this interface and calling HasPrivilege twice, one as before and once with privilege.GRANT when WITH GRANT OPTION is provided. An analogous change was made to evalPrivilegeCheck. This is explained in some new commentary in pg_builtin.go.

I thought this was a nice simplification to these interfaces at first because it allowed us to consolidate the WITH GRANT OPTION handling to a single place (runSinglePrivilegeCheck). But with the addition of the last commit here, I'm less sure now. I think it depends on how we intend to address #67410 and how we intend to represent a "privilege with grant option" in the code.

What do you think?

pkg/sql/sem/builtins/pg_builtins.go, line 1812 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: should be "no matching role"

Done.

pkg/sql/sem/builtins/pg_builtins.go, line 2199 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

heads up: we are fixing the rolsuper field so that it determines superuser status based on admin membership: #69943

Thanks for the heads up! Fixed.

pkg/sql/sem/builtins/pg_builtins.go, line 503 at r5 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: might want to link the tracking issue #67410

Done.

RichardJCai

LGTM modulo nits

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @nvanbenschoten, and @rafiss)

pkg/sql/resolver.go, line 327 at r5 (raw file):
Ah oops, I missed it.

how we intend to represent a "privilege with grant option" in the code.
Fair, I don't think we have a concrete idea of how this will look right now.

This sounds good to me.

pkg/sql/sem/builtins/pg_builtins.go, line 1962 at r10 (raw file):

				return tree.DNull, nil
			}
			// All users have USAGE privileges to all types.

I think we can remove this hardcoded case and rely on the privilege check (the Public role is explicitly granted USAGE on new types).

pkg/sql/sem/builtins/pg_builtins.go, line 2462 at r10 (raw file):

	}

	// Superusers have every privilege, so are part of every role.

nit: so are part of every role

I think this shit be and are part of every role.

…}_privilege Postgres used to return true in these cases instead of NULL, which was strange. We copied the behavior out of an abundance of caution. PG fixed the bug at some point before version 13, so there's no reason to keep it in CRDB. Release note (sql change): The has_tablespace_privilege, has_server_privilege, and has_foreign_data_wrapper_privilege builtin functions now return NULL instead of true when provided with a non-existed OID reference, which matches the behavior of newer versions of PostgreSQL.

…lege This is functionally equivalent, but a little cleaner and more explicit.

… checking This commit refactors the implementation of the has_foo_privilege builtins. It splits the parsing of the privilege string from the privilege checking step, and provides a bit more flexibility in the privilege strings accepted, which will be helpful for the implementation of `pg_has_role`.

Needed for cockroachdb#69010. This commit implements the `pg_has_role` builtin function. `pg_has_role` returns whether the user has privileges for a specified role or not. Allowable privilege types are MEMBER and USAGE. MEMBER denotes direct or indirect membership in the role (that is, the right to do SET ROLE), while USAGE denotes whether the privileges of the role are immediately available without doing SET ROLE. `pg_has_role` was the last remaining unimplemented "access privilege inquiry functions", and was omitted from 94c25be because our role-based access control system was not mature enough to support it at the time. The commit also makes a small modification to `pg_catalog.pg_roles` and `pg_catalog.pg_authid` to reflect that fact that all users and roles inherit the privileges of roles they are members of. Release note (sql change): The pg_has_role builtin function is now supported, which returns whether a given user has privileges for a specified role or not. Release justification: None, waiting for v22.1.

…Functions This commit resolves an incompatibility in CockroachDB's handling of the privilege string accepted by each Access Privilege Inquiry Functions. The intention was for the functions to return whether _any_ of the comma-separated privileges were held. However, since their introduction, they were returning whether _all_ of the privileges were held. The intended behavior is mentioned in https://www.postgresql.org/docs/current/functions-info.html. > Also, multiple privilege types can be listed separated by commas, in which > case the result will be true if any of the listed privileges is held. We can also see this in the Postgres source. Note the use of `ACLMASK_ANY` in https://github.com/postgres/postgres/blob/master/src/backend/catalog/aclchk.c. Release note (bug fix): The Postgres-compatible "Access Privilege Inquiry Functions" (e.g. has_foo_privilege) were incorrectly returning whether all comma-separated privileges were held, instead of whether any of the provided privileges were held. This incompatibility has been resolved. Release justification: None

nvanbenschoten

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @rafiss, and @RichardJCai)

pkg/sql/sem/builtins/pg_builtins.go, line 1962 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I think we can remove this hardcoded case and rely on the privilege check (the Public role is explicitly granted USAGE on new types).

Good point. I'll do this in a separate PR.

pkg/sql/sem/builtins/pg_builtins.go, line 2462 at r10 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: so are part of every role

I think this shit be and are part of every role.

Done.

nvanbenschoten · 2021-09-16T14:05:29Z

bors r+

craig · 2021-09-16T15:14:18Z

Build succeeded:

GitHub CI (Cockroach)

nvanbenschoten requested review from RichardJCai, a team and shermanCRL and removed request for a team September 8, 2021 20:50

nvanbenschoten mentioned this pull request Sep 8, 2021

sql: Initial support for PostgREST #69010

Closed

22 tasks

rafiss self-requested a review September 9, 2021 02:55

shermanCRL requested review from adityamaru and removed request for shermanCRL September 9, 2021 15:51

nvanbenschoten force-pushed the nvanbenschoten/pg_has_role_real branch from a0f2bff to e13f038 Compare September 10, 2021 04:30

nvanbenschoten force-pushed the nvanbenschoten/pg_has_role_real branch 2 times, most recently from 524849a to d419c8a Compare September 13, 2021 14:06

rafiss requested changes Sep 13, 2021

View reviewed changes

RichardJCai suggested changes Sep 13, 2021

View reviewed changes

nvanbenschoten force-pushed the nvanbenschoten/pg_has_role_real branch from d419c8a to e579c5e Compare September 15, 2021 04:39

nvanbenschoten commented Sep 15, 2021

View reviewed changes

RichardJCai approved these changes Sep 15, 2021

View reviewed changes

nvanbenschoten added 5 commits September 15, 2021 15:01

sql: call ParseDOid directly in has_function_privilege/has_type_privi…

915b64b

…lege This is functionally equivalent, but a little cleaner and more explicit.

nvanbenschoten force-pushed the nvanbenschoten/pg_has_role_real branch from e579c5e to 50b326e Compare September 15, 2021 19:29

nvanbenschoten commented Sep 15, 2021

View reviewed changes

craig bot merged commit 407f504 into cockroachdb:master Sep 16, 2021

cockroach-teamcity mentioned this pull request Sep 16, 2021

sql: implement pg_has_role cockroachdb/docs#11697

Open

cockroach-teamcity mentioned this pull request Sep 16, 2021

sql: implement pg_has_role cockroachdb/docs#11698

Open

nvanbenschoten deleted the nvanbenschoten/pg_has_role_real branch October 4, 2021 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: implement pg_has_role #69939

sql: implement pg_has_role #69939

nvanbenschoten commented Sep 8, 2021

cockroach-teamcity commented Sep 8, 2021

nvanbenschoten commented Sep 13, 2021

rafiss left a comment

RichardJCai left a comment

nvanbenschoten left a comment

RichardJCai left a comment

nvanbenschoten left a comment

nvanbenschoten commented Sep 16, 2021

craig bot commented Sep 16, 2021

sql: implement pg_has_role #69939

sql: implement pg_has_role #69939

Conversation

nvanbenschoten commented Sep 8, 2021

cockroach-teamcity commented Sep 8, 2021

nvanbenschoten commented Sep 13, 2021

rafiss left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

nvanbenschoten commented Sep 16, 2021

craig bot commented Sep 16, 2021